Check whether secrets are empty and mark them all as sensitive #52469
Conversation
5011059 to
b7464ed
Compare
There was a problem hiding this comment.
thanks! was not aware the secret is used in so many places.
imho it would make sense to deprecate the empty secret in all currently supported symfony versions, not only 6.4.
about the throwing exceptions immediately i am less sure. this problem has been around for quite a long time, and is only a problem when mistakenly not setting the secret (likely some deployment / environment configuration mistake). i would not throw the exception in 6.4 so people can notice the issue in their deprecation logs if they care. and when upgrading to 7.0 we start throwing.
|
As having an empty secret can only be a mistake, I would throw an exception in 6.4. |
|
Would it make sense (in addition) to throw an exception at build time when the secret is empty (fail early)? |
at build time we reference an env var so we cannot check this reliably I think. |
b7464ed to
49f5822
Compare
49f5822 to
52ca699
Compare
Updated! |
| * @param string $parameter Query string parameter to use | ||
| */ | ||
| public function __construct(#[\SensitiveParameter] string $secret, string $parameter = '_hash') | ||
| { | ||
| if (!$secret) { |
There was a problem hiding this comment.
Do we consider '0' as an empty secret?
There was a problem hiding this comment.
yes, it's equally weak
|
I like this, but I wonder if the error message is enough to fully understand the problem and how to solve it. Imagine reading this with not much additional context --> Would something like the following be acceptable? |
I'm not following this proposal because the exceptions should be agnostic of FWB. |
| 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 \Symfony\Component\Security\Core\Exception\InvalidArgumentException('A non-empty secret is required.'); | ||
| if (!$secret) { |
There was a problem hiding this comment.
This is a breaking change. Both the argument, as the deprecation, have been added in #51434 (@Spomky), which is also 6.4+. Converting the deprecation to an exception means that on upgrade from 6.3 and lower users of this class will immediately run into this exception. (Because it is a new argument it obviously isn't provided yet).
It isn't a big deal to fix, but just wanted to give a heads up about this. Wheteher you decide to revert to a deprecation on 6.4 and keep throwing on 7.0 is up to you. As said, it's not a big issue to fix as an end user, but it's a BC break without previous warning nevertheless.
There was a problem hiding this comment.
We decided it's OK to add this exception when security is at risk.
That being said, I'm not sure why we use the secret here to hash the logged info.
There was a problem hiding this comment.
'm not sure why we use the secret here to hash the logged info.
This is the pepper for preventing rainbow tables to be used on IP addresses or username/IPs combinations
Edit: article (in French) on bad design where simple hashes are used issued few days after the PR => https://theconversation.com/les-dangers-de-la-cryptographie-non-maitrisee-212881
There was a problem hiding this comment.
As noted in the MR, if you notice this breaking change happening, you had your app severely misconfigured and should send thanks to the core team for pointing it out. 😃
There was a problem hiding this comment.
We decided it's OK to add this exception when security is at risk.
I do get that argument for the existing $secret arguments, this as it indeed is a security risk to not provide it (/use an empty string). But in this case it isn't an existing argument for which the user (knowingly) provided an unsecure value. It's a new parameter, so to not break BC it does require a default value. But then this default value is considered invalid and still throws an exception.
So if it is decided that the exception must stay, I now think it makes sense to just drop the default value. Then users are at least made aware they're not providing a (now required, for security) parameter. Instead of the currently "vague" exception that they're providing an invalid value (to then discover they don't provide any value at all, the default is "broken", and they must provide a value themselfs).
There was a problem hiding this comment.
Can you please send a PR on branch 6.4?
There was a problem hiding this comment.
So in short / to put it differently:
On version 6.3 one must provide 2 parameters, $globalFactory and $localFactory. But the exact same code on 6.4 leads to an exception, that a non-empty secret must be provided. But I'm not providing a secret at all. So in an ordinary case this would be an "illegal" change as it breaks BC. For the sake of security I do get the "we want to require a secret". But IMO then the error should be about the missing parameter (so just the PHP error), and not a custom exception. I.e.: dropping the default value so PHP gives an error for the invalid method call / missing parameter, and keeping the check it's not empty as well. As IMO the standard PHP error is a better DX because it's a common error, the custom error requires more time to figure out what exactly is going wrong and why (to still come to the conclusion the argument is now required).
And in all(/most?) other cases changed by this PR the $secret parameter already existed. So IMO indeed can be considered a bug / security fix where values which have always been invalid now actually are detected and result in an error. But that's not the case for this specific scenario (as it's a new parameter which could not have been provided beforehand).
There was a problem hiding this comment.
(Sorry, I apparently missed some comments in between)
Can you please send a PR on branch 6.4?
I'll try to do so, but it will most likely be after the weekend. (I read the blogpost about #51434 on the go, and it reminded me about the fix I had to make yesterday).
And just to be sure: I presume a PR to drop the default value?
As noted in the MR, if you notice this breaking change happening, you had your app severely misconfigured and should send thanks to the core team for pointing it out. 😃
That's not the case here. Because it is a new parameter. And I can't severly misconfigure anything when there was nothing to configure it in the first place. The "configuration" is added in 6.4 and the default is "severely misconfigured".
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
…oduction (wouterj) This PR was merged into the 6.4 branch. Discussion ---------- [Security] Document BC break with $secret parameter introduction | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT Deprecation was converted to an exception #52469 Commits ------- 0182b9f Document BC break with $secret parameter introduction
Some security-hardening.
I made this throw a deprecation (as was already the case in some places) but I'd be fine making them throw an exception right away since an empty secret is a bold mistake. WDYT? (Webhook was experimental in 6.3 so it's fine throwing there I think.)