Add legacy_hash_algo to support backward-compatible hash_algo changes
#351
Conversation
|
I think it is great to have the option for setting a legacy hash algo, so people can smoothly upgrade cookies, thanks! IMO having the algo set together with the cookie doesn't add much value, we could simply try to sign using algo, and if that fails try legacy algo if there is one, if it still fails then fail the check? Another point, it would be nice if when detecting a cookie signed with a legacy algo, a response listener would be registered to update the cookie with a new signature, that way we ensure that cookies get rotated ASAP, and it's easier to say "we set legacy algo for 30days, and after that remove it", making sure that any user hitting the website in those 30days will be fully migrated. Obviously that response listener should be careful to not overwrite cookie removals, or newly written cookies with older values. Finally just thinking of how we can upgrade the default algo, I guess we'd need to trigger a deprecation if algo is unset to make people set it to something else than sha256, and then in the next major we can change the default. |
martijnc
force-pushed
the
feature/cookie-algo-upgrade-path
branch
from
114f320
to
15ce1d6
Compare
|
Thanks for the feedback! Updated this PR and removed the changes that added the used algorithm to the cookie value. Also opened #355 to deprecated the default hash algorithm in the configuration for the next minor. The upgrade listener is a bit trickier because the bundle needs extra information (expiration, path, ...) than it gets from the browser (only cookie name and value). The application will need to provide the other options for this to work correctly and securely. I've explored this in #356. |
src/Signer.php
Outdated
| for ($i = 0, $j = \strlen($signature); $i < $j; ++$i) { | ||
| $result |= \ord($signature[$i]) ^ \ord($signature2[$i]); | ||
| $expectedLegacySignature = $this->generateSignature($value, $this->legacyAlgo); | ||
| if (hash_equals($expectedLegacySignature, $signature)) { |
There was a problem hiding this comment.
Ah nice, didn't realize we still were using some legacy version of hash_equals here :D
|
Thanks! |
The goal of this PR is to allow backward-compatible signed cookie
hash_algoupgrades. With the current code any existing signed cookies would be rejected when thehash_algois changed which makes upgrading to a more secure hashing algorithm tricky.This PR addresses this by;
legacy_hash_algoto support a previously configured algorithm during verification.This is a stepping stone to address #324 but does not yet fix it. Changing the
hash_algodefault value would still break any existing cookies that don't include the hash algorithm in their signed value yet; thelegacy_hash_algowould need to be configured manually for them to keep working. I don't currently see a backward-compatible way to change thehash_algodefault value without also (potentially) opening the door for downgrade attacks (e.g. by setting a default value forlegacy_hash_algoor hardcoding asha256fallback).This PR also adds a
SignerInterfaceto allow for custom signers.