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

Spomky · 2023-08-20T12:10:16Z

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	yes
Deprecations?	no
Tickets	Fix #46362
License	MIT
Doc PR	symfony/symfony-docs#... TODO

This PR is a proposal for fixing #46362. It appears the username and IP address may be both available in the log or the caching system.
The proposed feature uses the already existing kernel secret to hash the data.

src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php

mtarld · 2023-08-22T08:08:19Z

src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php

    {
+        if (!$secret) {
+            trigger_deprecation('symfony/security-http', '6.4', 'Calling "%s()" with an empty secret is deprecated. A non-empty secret will be mandatory in version 7.0.', __METHOD__);
+            // throw new \InvalidArgumentException('A non-empty secret is required.');


This will be for 7.0, but maybe it'll be better to use Symfony\Component\Security\Core\Exception\InvalidArgumentException instead?

Correct. I changed the line

src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php

nicolas-grekas · 2023-08-23T07:05:53Z

Thank you @Spomky.

Seldaek · 2023-11-21T14:18:34Z

src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php

+
+    private function hash(string $data): string
+    {
+        return strtr(substr(base64_encode(hash_hmac('sha256', $data, $this->secret, true)), 0, 8), '/+', '._');


I came here to check as I was worried I'd have to complain about a massive cache size increase, but I'm happy to see that it was taken into account and this will actually probably even reduce the cache storage requirements. Thanks!

Honestly, I fell into the trap 😅🤦‍♂️. But there are people here who are very quick-witted and who paid attention to that 😊. #bestcommunityever 👌

The `secret` parameter has been added in #51434 with a default value of `''` and a deprecation message that it is required / may not be empty. Which is fine and doesn't hurt backwards compatiblity. The later ticket #52469 changes the deprecation into an exception, as it is undesirable that no secret is used (in any scenario). This leads to the unintended side effect that there is a BC breakage when a developer manually creates a `DefaultLoginRateLimiter` as it is now actually required to provide a (non empty) value due to the check and exception. Allowing the service / class to be used without providing the secret parameter, in a backwards compatible manner, but then still breaking the backwards compatibility by throwing due to the default value is confusing. So making the `secret` required makes more sense from a developer perspective as it is clear in that the parameter must be provided.

…r (RobertMe) This PR was merged into the 6.4 branch. Discussion ---------- [Security] make secret required for DefaultLoginRateLimiter | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes/no | New feature? | no | Deprecations? | yes/no | Issues | | License | MIT This tickets results from the discussion here: #52469 (review) and `@nicolas`-grekas requested a PR for it. The `secret` parameter has been added in #51434 with a default value of `''` and a deprecation message that it is required / may not be empty. Which is fine and doesn't hurt backwards compatibility. The later ticket #52469 changes the deprecation into an exception, as it is undesirable that no secret is used (in any scenario). This leads to the unintended side effect that there is a BC breakage when a developer manually creates a `DefaultLoginRateLimiter` as it is now actually required to provide a (non empty) value due to the check and exception. Allowing the service / class to be used without providing the secret parameter, in a backwards compatible manner, but then still breaking the backwards compatibility by throwing due to the default value is confusing. So making the `secret` required makes more sense from a developer perspective as it is clear in that the parameter must be provided. Commits ------- ecbf0e9 [Security] make secret required for DefaultLoginRateLimiter

…kernel.secret% with the rate-limiter (nicolas-grekas) This PR was merged into the 7.1 branch. Discussion ---------- [SecurityBundle] Use %container.build_hash% instead of %kernel.secret% with the rate-limiter | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT In #51434, we decided to hash the username and the client IP in order to anonymize logs. I propose to use %container.build_hash% instead of %kernel.secret% in order to use %kernel.secret% one less time. %container.build_hash% looks good enough to me, and it doesn't need any external configuration. Related to the discussion happening in symfony/recipes#1314 Commits ------- 574f573 [SecurityBundle] Use %container.build_hash% instead of %kernel.secret% with the rate-limiter

Spomky requested a review from chalasr as a code owner August 20, 2023 12:10

carsonbot added Status: Needs Review Bug Feature Security labels Aug 20, 2023

carsonbot added this to the 6.4 milestone Aug 20, 2023

carsonbot changed the title ~~[Security][Throttling] Hide username and client ip in logs~~ [Security] [Throttling] Hide username and client ip in logs Aug 20, 2023

Spomky mentioned this pull request Aug 20, 2023

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

Closed

OskarStark approved these changes Aug 20, 2023

View reviewed changes

src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 20, 2023

Spomky force-pushed the noip-in-cache branch from dbf4759 to da1e536 Compare August 20, 2023 13:19