Option to try all OIDC JWK keys as fallback

[c15yi]

Instead of changing more logic to check multiple keys as fallback, which would mean bigger changes and also negatively impact performance, I added the option to specify a specific key as a fallback key in case no key could be determined.

• Fixes: OIDC: List of JWKS is not checked without specified kid #41931

[sberyozkin]

Thanks for the effort, @c15yi, PR looks neat.
But does it really work beyond the moment OIDC server recycles JWKs keys ?
I feel we have to give it a bit more thinking. If we have 2/3 keys only, the performance impact may or may not be noticeable, you may as well get the first key matching the signing key... In fact I think I recall now that this situation is optionally tested during the OIDC RP conformance testing, it is OK not to support it but they do offer it as an optional test...

[c15yi]

In my case, the kid would stay the same after recycling the keys, but you are correct, that might not always be the case.

Do I understand your suggestion correctly, to try out the keys with JsonWebSignature:verifySignature until a matching one was found in OidcProvider.JsonWebKeyResolver:resolveKey and then return that key, if one could be found?

I would create an option to check all keys, but only if there is no kid or x5t set in the JOSE header. If a key is specified in the JOSE header, then it doesn't make sense to check all keys, if that key isn't present in the jwks, does it?

[c15yi]

@sberyozkin updated the PR 🙂

[sberyozkin]

@c15yi, I guess something like this:

public TokenVerificationResult verifyJwtToken(String token, boolean enforceAudienceVerification, boolean subjectRequired,  String nonce)  throws InvalidJwtException {

        if (oidcConfig.jwks.tryAll) {
               TryAllJsonWebKeyResolver resolver = new TryAllJsonWebKeyResolver(jwks.getAllKeys(), asymmetricKeyResolver);
               for (int i = 0; i < resolver.allKeysSize(); i++) {
                   try {
                      return verifyJwtTokenInternal(customizeJwtToken(token), enforceAudienceVerification, subjectRequired, nonce,  (requiredAlgorithmConstraints != null ? requiredAlgorithmConstraints : ASYMMETRIC_ALGORITHM_CONSTRAINTS), resolver, true, oidcConfig.token.isIssuedAtRequired());
                   } catch (InvalidJsonJsonWebKeyException ex) {
                        if (signatureFailure(ex) && resolver.keyIndex() != -1) {
                             continue;
                        }
                        throw ex;
                   }
              }
       } else {
            return verifyJwtTokenInternal(customizeJwtToken(token), enforceAudienceVerification, subjectRequired, nonce,
                    (requiredAlgorithmConstraints != null ? requiredAlgorithmConstraints : ASYMMETRIC_ALGORITHM_CONSTRAINTS), asymmetricKeyResolver, true, oidcConfig.token.isIssuedAtRequired());
       }
    }

where TryAllJsonWebKeyResolver provides an index based access to the algorithm specific list of keys but only if the JsonWebKeyResolver which it delegates to produced null key...JsonWebKeyResolver itself remains unchanged

Please think about it

[c15yi]

@sberyozkin, checking the signature twice is not optimal. What is the expected runtime duration for such check? I am just trying to understand the trade-off here.

I have a few questions regarding your solution proposal.

• Is it worth (in terms of runtime) to cycle through verifyJwtTokenInternal multiple times until a valid key was found?
• The TryAllJsonWebKeyResolver would need to hold a state.
  • Every time resolveKey is called on it would return the next key in its list. Feels somehow not so clean.

Edit: When getting the keys from the jwks we would get all keys (for all algorithms) right? Or did you have some implementation in mind to select the correct algorithm based on the string jwt?

[sberyozkin]

@c15yi This is only a very rough prototype

Is it worth (in terms of runtime) to cycle through verifyJwtTokenInternal multiple times until a valid key was found?

As far as I understand, it could be the most effective, albeit the messy one. In this case, given 3 keys, you'll get at most 3 verifications in the worst case but only 1 in the best case. While with your last update, with 3 keys, in the worst case, you'll get 4 and in the best case 2 verifications. I've typed it and I think it is indeed not a big deal, so let's scrap my proposal, thanks for raising concerns about it.

But it really should be a Map of algorithms to the list of keys. Each JWK must have an alg property so you can use it to prepare this map, and then you can get an algorithm from a token header to acquire a matching List of keys to check with the brute force technique... JWKs without the alg property can not be really accepted for this case

Thanks

[sberyozkin]

Thanks for your work on this PR, @c15yi, it really looks good already, but I've asked for a few minor updates, thanks

[sberyozkin]

@c15yi FYI, I'll be going on PTO later today so we'll continue in mid of August, talk to you then

[c15yi]

@sberyozkin, in case you are still available, I just implemented your suggestions.

Otherwise, have a great PTO, thanks for the reviews and talk to you then.

[c15yi]

Because our application is depending on this, would it be possible for someone else to take over the PR review, @sberyozkin? In the issue I created @pedroigor was mentioned for oidc, too. What do you think?

[sberyozkin]

Can you clarify this comment before uncommenting ?

[c15yi]

In the code of the DynamicVerificationKeyResolver there is no JsonWebSignature, but it need such object to perform the brute force algorithm on it with the different keys.
Therefore, I commented out the code, because it's not functional and I didn't see an easy solution to getting this to work there.

[sberyozkin]

@c15yi ok, a jose4j instance for verifying a signature can be built manually for this case with some custom basic resolver delegating to your new method, but I can take care of it later

[sberyozkin]

@c15yi Sure, @pedroigor can definitely review.

Your PR is nearly ready to go, couple of questions only there, and please squash it, keeping the main commit message only.

Note though, I think it will only make it into 3.14, I'll be back by then:
https://github.com/quarkusio/quarkus/wiki/Release-Planning#314-proposal

But I may be able to merge approve later today as well, once the last minor details are sorted out

[sberyozkin]

I believe it's ready to go, dynamic resolver update can wait, no need to rush

[c15yi]

@sberyozkin squashed the commits and added an explanation to your question on the DynamicVerificationKeyResolver. :)

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 41de0f5.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[geoand]

@sberyozkin should this be merged?

[c15yi]

@geoand, @sberyozkin is currently on vacation and will be back mid of August. He approved it, but the pipelines failed, because I missed an unused import. Now everything should be ready I guess.

[geoand]

Thanks.

Let's wait for @sberyozkin to re-check

[sberyozkin]

Thanks @c15yi, @geoand, sorry but did not really have good options for merging it since heading to the airport on 27th July, but we still have a few days to have it merged and backported :-)
I'll keep the commented out code in place for now, it is on a non-critical path as far as this issue is concerned and I'll clean it up a bit later

[sberyozkin]

No need to backport, it is just in time for 3.14 CR1

[michalvavrik]

this variable is never used

[michalvavrik]

I think this is not useful to have this commented out code.