4.2.0 key size validation breaking change

[mfn]

I was just affected by #864 after a composer upgrade, in my case due to it being a dependency on https://github.com/PHP-Open-Source-Saver/jwt-auth which has this constraint "lcobucci/jwt": "^4.0"

I was lucky I caught this in a test suite which uses a (now) invalid key, my production is affected as well though. It might catch ppl by surprise who might have a valid key in their test/dev setup but a different one in prod and as we know. Further, changing the key might not be trivial in some cases.

I guess revert this change and saving it for a major release is not an option?

Thanks!

[mfn]

I guess revert this change and saving it for a major release is not an option?

Was implicitly answered via #877 (comment)

I disagree: it was a security bug, hence it has been considered a security bug fix.
BC breaks need to be addressed within the same MAJOR, if they are security bug fixes

and #877 (comment)

An insecure system is worse than a broken system. Better for it to be broken than compromised, and security takes priority over BC.

[lcobucci]

@mfn thanks for opening this discussion.

Increasing visibility of this too:

Introducing behavioural BC-breaks is never fun and it was not an arbitrary decision.
If you look at the statement we had in the documentation, we've tried to point users to the correct path but overlooked the critical details on the RFC, where they clearly request implementors to conform with minimum security guidelines to prevent people from having insecure systems.

I completely understand the sentiment and feel bad about forcing breakages to downstream packages. At the same, I feel responsible for providing abstractions that don't compromise security of end-users, which is why I decided to release the breakage as a minor version and not a patch (giving end-users a migration path) - even though it's a security issue.

When analysing all the trade-offs, I still feel like we've made the correct decision with these behavioural BC-breaks as they will lead to safer solutions across all channels. I really don't like the feeling of overlooking these details and not enforcing the constraints from day 0 but I'm also human and entitled to mistakes.

I look forward to helping the affected users of this package to find solutions and get their systems into the desired state.

[burgoyn1]

@lcobucci While I do get where you are coming from, and going forward forcing it/throwing the error is the best way for overall security, I am in the situation where I actually can't change hundreds of incorrect length keys, or at least doing so will take a phenomenal amount of work either issuing all new keys and asking people to update them, by using a different JWT package altogether, or forking your package and making my own changes.

Ideally, could you just keep the "UnsafeSha(256/384/512)" classes in the code instead of removing them in v5.0.0? This would very much help me (and probably others) in the long run, otherwise I will have to go through the process of using a different package in my code for some users.

[lcobucci]

@burgoyn1 I understand your situation and my recommendation would be to analyse the possibility of pulling the signer implementation to your system (or even a package you maintain to help others).

I say this because the algorithm implementation (signer) is an extension point of the library, anyone can provide their own implementation if they want. So, if you are using only the unsafe version of HS256, this means that you'd only have one class to pull into your responsibility.

That would allow you to handle the transition in your own pace and be able to use v5.0 even with the removals.

[burgoyn1]

There is no intention to change the Signer interface/functions in v5? If so, then that might actually work for my case.

[lcobucci]

That's correct, @burgoyn1

v5.0 will be v4.2 without deprecated components and adopting PHP 8.1 features.
The only interface BC-break we have is on the Builder#getToken() so that we can point to abstraction instead of the concrete implementation.

99.999% of the changes in src are already done, btw: 4.2.1...5.0.x
I'm working on making tests more useful to reduce maintenance pain and ship it.