chore: upgrade to jose v4 library

[mitar]

Related Issue or Design Document

Fixes #797.

Checklist

• I have read the contributing guidelines and signed the CLA.
• I have referenced an issue containing the design document if my change introduces a new feature.
• I have read the security policy.
• I confirm that this pull request does not address a security vulnerability.
  If this pull request addresses a security vulnerability,
  I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
• I have added tests that prove my fix is effective or that my feature works.
• I have added the necessary documentation within the code base (if appropriate).

Further comments

This is just a formality upgrade to v4 of the library, but it does not really take advantage of the new features v4 support. For that I think we should have a more thorough rewrite of current codebase related to JWTs. I was looking into that but it is really hard to do it unless we have clear understanding that this should be done and what can we break in terms of backwards compatibility. Because there are some simple structures we have in fosite/token/jwt package which could probably just be removed and delegated to go-jose directly. I think current fosite/token/jwt was written to be backwards compatible with prior version of fosite/token/jwt which used a much smaller jwt library so a lot of logic had to be done in fosite/token/jwt. But now fosite/token/jwt is doing very strange loops where it duplicates a lot of structs and work of go-jose. For example, fosite/token/jwt validates the token using go-jose (using all reasonable algorithms) and then checks if it matches the expected signing algorithm. It would be much better if we would just give to go-jose the signing algorithm which we expect in the first place and then we do not have to do no checks anymore in fosite/token/jwt (which was the reason for v4 in the first place). Similarly with private/public keys. We do a lot of work where we take private/public keys and inspect them and reconstruct them and then pass them to go-jose. But go-jose already supports all of that and you can already pass keys directly (which is what #799 was about), even more, you can pass whole key sets directly and it picks the correct key (which fosite also tries to do). I also think fosite is probably susceptible to this attack with current approach of just blindly validating tokens and only then (sometimes) checking if signing algorithm is what was expected (I see GetRequestObjectSigningAlgorithm and GetTokenEndpointAuthSigningAlgorithm, but nothing to check when validating tokens themselves, like in introspect endpoint). Maybe I am missing something.

TLDR; I think this upgrade is the first step, but somebody with executive permissions to break backwards compatibility should give some love and cleanup current jwt codebase in fosite.

[mitar]

(CI is failing because Hydra has not been upgraded to jose v4.)

[aeneasr]

One question, otherwise looks good to me :)

[aeneasr]

Could we have missed some here that would cause some breaking changes in existing systems because the validation has changed?

[mitar]

I listed everything standardized and available and secure. In the worst case though, this is a publicly accessible variable in Go, so people can extend it if we did miss something. Not nice API though.

But as I mentioned in my PR description, I really think we should do a backwards incompatible change after this, to address current code rot around JWTs in fosite and address some vulnerabilities available exactly because we allow for example to try all algorithms and not just the one expected. But this is probably something only you can decide on how much breaking change would you approve (not in terms of external users changes, but in terms of API changes for fosite integrators - so some fosite structs we could just get rid of and delegate to go-jose).

[aeneasr]

I'm pretty sure we validate the signing method somewhere, do we not?

[aeneasr]

I also think fosite is probably susceptible to this attack with current approach of just blindly validating tokens and only then (sometimes) checking if signing algorithm is what was expected (I see GetRequestObjectSigningAlgorithm and GetTokenEndpointAuthSigningAlgorithm, but nothing to check when validating tokens themselves, like in introspect endpoint). Maybe I am missing something.

I did take a look into this. The biggest issue of the attack is using alg:"none" which we support. However, to pass, the key func must return a constant type UnsafeAllowNoneSignatureType which we use only in authorizeRequestParametersFromOpenIDConnectRequest.

So at least from that perspective we're safe. However, I totally agree with you that the JWT code is horrific and needs some good refactoring and simplification. I agree that backwards compatibility will be quite challenging but we can probably test for it?

[mitar]

However, I totally agree with you that the JWT code is horrific and needs some good refactoring and simplification. I agree that backwards compatibility will be quite challenging but we can probably test for it?

What I meant is that it might beneficial to drop backwards compatibility here and just get rid of the whole jwt fosite package and document how any users of the code should migrate to directly use go-jose v4. Probably it is possible to keep backwards compatibility, but it would require us to a) maintain multiple structs and copy data between them b) probably still change some of the methods, like to pass now an expected sign algorithm.

[aeneasr]

What I meant is that it might beneficial to drop backwards compatibility here and just get rid of the whole jwt fosite package and document how any users of the code should migrate to directly use go-jose v4.

API BC breaks are totally fine with me, breaking existing implementations / expectations on the API behavior is not (because we'll be busy with customer support for a week if that happens :D)