Change of default algorithm may cause problems

[desmoteo]

I just wanted to share with you my experience that the change in the default signing algorithm from HS256 to HS512 can break things in case of JSONWebSignatureSerializer that need to be persistent (e.g. stored in a db).

On our server, previously generated JWTs started causing BadSignature exceptions, resulting in authentication failure.

[mitsuhiko]

Ugh sorry. I did not see that this was changed. @davidism can we roll this back?

[mitsuhiko]

I guess this made it into a 1.0 release which sucks. Not sure what a good path forward is here now. This should not have been done :(

Rolling back is not really an option but I think we should potentially just accept both SHA1 and SHA512 in the default implementation now. I will investigate if this can work.

[mitsuhiko]

This makes even less sense because it was supposed to stay compatible to Django. If we want to change that we should also get away from the crappy django compat key derivation. I really think this should be rolled back because it is going to cause a lot of issues for users.

[mitsuhiko]

So my proposal is to add a fallback_digest_methods to HMACAlgorithm and Signer. Then set that to [sha512] and change the default back to sha1 then make a 1.0.1 release.

[mitsuhiko]

Damn. This does not work because our key derivation also uses the hash method. :-/

[mitsuhiko]

The only fix would involve moving derive_key into the hash algorithm as well and derive different keys for all fallbacks.

[mitsuhiko]

This is impossible to fix now without making a massive mess of the API. I'm considering wiping the 1.0 release right now and hoping not enough people upgraded yet and then try to figure out how to deal with this. Thoughts? I definitely do not want to go forward with 512 hashes by default.

[ThiefMaster]

https://github.com/search?q=%22itsdangerous%3E%3D1.0%22&type=Commits

when nuking the release we should should probably ask those projects to revert to an older version as well

[mitsuhiko]

@ThiefMaster yeah. I'm not sure but this is not a great situation right now :( The way I designed (or misdesigned) the key derivation system leaves no good way now to support both hashes that the default was changed without redesigning the entire API.

That said, it does show that generally fallbacks need to be supported anyways so the positive aspect of this is that it forces that conversation now.

[mitsuhiko]

I removed the 1.0.0 release. Will figure out the plan tonight.

[desmoteo]

Maybe it's bad practice, but wouldn't it be possible to just select the decoding algorithm according to the header?

[RR2DO2]

The header isn't trusted data (you're using it before signature verification) and thus it'd allow malicious third parties to downgrade the algorithm to a potentially exploitable one. This has been abused in the past in plenty of JWT libraries with the 'none' algorithm.

[desmoteo]

@RR2DO2 so even something like this, with a protection against 'none' algorithm, is not advisable i guess desmoteo@0167057

[arianmaykon]

Had some problems with the upgrade too, Flask requires >=0.24 so we got some breaking changes, we did adjusted/updated to a non breaking change with the latest versions 1.0.0 but just found out this #112.

:/ Fixing again...

[kthy]

I realize your team is probably not having the best of weekends, but could you perhaps find two minutes to put up a brief notice on the Pallets blog that the package was pulled?

[davidism]

1.1.0 has been released. It reverts to SHA-1, and adds a fallback mechanism to safely upgrade signing parameters in the future. It also reverts the package name to all lowercase "itsdangerous".

You can read a longer explanation here: https://palletsprojects.com/blog/itsdangerous-1-1-0-released/