Skip to content

[Security][Throttling] Hide username and client ip in logs #46362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
xelaris opened this issue May 16, 2022 · 8 comments · Fixed by #51434
Closed

[Security][Throttling] Hide username and client ip in logs #46362

xelaris opened this issue May 16, 2022 · 8 comments · Fixed by #51434
Labels

Comments

@xelaris
Copy link
Contributor

xelaris commented May 16, 2022

Description

When using the login throttling feature, username and IP may appear in the debug logs (e.g. when an error occurs in prod with fingers_crossed logger):

Successfully acquired the "username_ip_login-random.username-1.2.3.4" lock.
Expiration defined for "username_ip_login-random.username-1.2.3.4" lock for "300" seconds.

What do you think about (optionally) masking these information? For example by adding a MaskingLoginRateLimiter in addition to the DefaultLoginRateLimiter. A straight forward approach would be to use a hash of username and client ip as the key for the limiter(s).

This would not only mask the log messages but would impact all appearances of the keys, e.g. in storage. This might be an advantage or a disadvantage. Although, at least the PdoStore hashes the key anyway. The Lock class, where the logs originate, seems to be the wrong place for tweaking, as it's (and should not be) aware of the content of the key.

Another approach, probably on application level, would be a monolog processor, which replaces the username and client ip, but this seems less efficient and not that robust.

Example

protected function getLimiters(Request $request): array
{
    $username = $request->attributes->get(Security::LAST_USERNAME, '');

    $globalKey = hash('sha256', $request->getClientIp());
    $localKey = hash('sha256', $username.'-'.$request->getClientIp());

    return [
        $this->globalFactory->create($globalKey),
        $this->localFactory->create($localKey),
    ];
}
@alamirault
Copy link
Contributor

Maybe we can add hashKeys option in construct of DefaultLoginRateLimiter default false and allow to define it in configuration

@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@carsonbot
Copy link

Friendly ping? Should this still be open? I will close if I don't hear anything.

@94noni
Copy link
Contributor

94noni commented Jan 3, 2023

what about using #[\SensitiveParameter] ?
for ex like #48274

@carsonbot carsonbot removed the Stalled label Jan 3, 2023
@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@carsonbot
Copy link

Could I get a reply or should I close this?

@xelaris
Copy link
Contributor Author

xelaris commented Jul 18, 2023

@carsonbot I think this issue is still relevant

@Spomky
Copy link
Contributor

Spomky commented Aug 20, 2023

Hi,

This seems legit to me, especially when the GDPR requires personal data to be protected.
See #51434.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants