chore: upgrade to jose v4 library

[mitar]

Related Issue or Design Document

Fixes https://github.com/ory/fosite/issues/797.

Checklist

• [x] I have read the contributing guidelines and signed the CLA.
• [x] I have referenced an issue containing the design document if my change introduces a new feature.
• [x] I have read the security policy.
• [x] 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 https://github.com/ory/fosite/pull/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]

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)

[aeneasr]

There's a couple of failing things and merge conflicts. Would you mind taking a look? It would be good to get this merged

[mitar]

Yes, I can check.

[sword-jin]

@mitar thanks for your great work. I'd like to have a friendly ping, I am also very looking forward this PR.

[mitar]

I still have this on my radar, but I am currently swamped with some other work. I can give anyone access to my branch, or feel free to fork my branch and just do another PR with updated PR? I do not mind. Otherwise, I hope I will be able to get to this at the end of this month, but we will see.

[mitar]

I refreshed the PR with the latest master branch. Fosite tests are passing. Only Hydra tests are not because Hydra itself will have to be upgraded to v4 version of the jose library. Not sure what is the process of getting them to pass here? It is a chicken and an egg problem?

[james-d-elliott]

You can make a similar PR in hydra that updates to this commit sha, and then reference that commit sha here for tests.

[mitar]

But then that will have to be removed afterwards after both PRs are merged?

[mitar]

@aeneasr How do I go about tests here?

[mitar]

@aeneasr Please advise here how do I go about tests here.

[aeneasr]

Sorry, I missed that. You will need to create a PR on hydra, and use a go mod rewrite to use your fork and git commit as a dependency for fosite.

Then here in fosite, you need to change https://github.com/ory/fosite/actions/runs/15810823776/workflow?pr=824#L16-L17 (which I just saw is still pointing to canonical ;) ) to your fork of hydra and git commit / branch

[mitar]

@aeneasr Tests now pass. Please merge.

Also see corresponding MR in Hydra: https://github.com/ory/hydra/pull/4032

[clifg]

Thanks for your work here, @mitar ! @aeneasr , is this change good to merge and if not do you have any other changes/fixes you'd need in order to merge? I'd like to resolve our code's govulncheck errors due to the indirect usage of go-jose/v3. Thanks!