Improve VerificationJwkSelector

[Halyna Kropelnytska]

Per OIDC protocol:

If there are multiple keys in the referenced JWK Set document, a kid value MUST be provided in the JOSE Header. 

but we always select the 1st key despite whether kid is set or not.

This is included in OpenID Connect Conformance Profiles v3.0 test: Reject ID Token without kid claim if multiple JWKs supplied in jwks_uri

[Brian Campbell]

It’s not clear to me what exactly you are asking for with this.

That text from OIDC is intended to be something for the signer to do to help disambiguate when there are multiple keys present. It's not supposed to be something the verifier enforces. That test even acknowledges that saying that it’s “optional” or that “rejection allowed”.

The first key is not always selected. Rather elegible keys are selected based on factors like key type, algorithm, intended use, key id, etc.. The first of that list of elegible keys is then selected. Which may or may not be the right key. But either should be okay for that conformance test. Assuming you’re using HttpsJwksVerificationKeyResolver, you can also use setDisambiguateWithVerifySignature(…) and actual signature checks will be used by calling verificationJwkSelector.selectWithVerifySignatureDisambiguate` to help chose the right key in situations where the eligible list has more than one. I’d think that would make it more likely to get a successful signature verify in that test. But, again, it doesn’t look like any particular behavior is needed from the test.

Best I can tell, the library is behaving appropriately and this conformance test doesn’t care anyway (appropriately). So I don’t understand what you were hoping to accomplish with this issue.

[Halyna Kropelnytska]

Many sorries for not responding for such a long time, Brian.
And, of course, sorry for such a short description of this enhancement ticket.

Yes, the setDisambiguateWithVerifySignature will get the right key based on key type, algorithm, intended use, key id, etc.. 

But that will not solve the case when we have multiple keys in the referenced JWK Set document, and at the same time the kid value will not be provided. We will have selected the key based on key type or algorithm or other filters.

So, what I meant there was to have this if statement inVerificationJwkSelector on 4-5 lines:

public List<JsonWebKey> selectList(JsonWebSignature jws, Collection<JsonWebKey> keys) throws JoseException {
    SimpleJwkFilter filter = SelectorSupport.filterForInboundSigned(jws);

    if (filter.getKeyId() == null && keys.size()){
      throw new UnresolvableKeyException("There are multiple keys in the referenced JWK Set document, but the kid value was not provided in the JOSE Header.");
    }

    List<JsonWebKey> filtered = filter.filter(keys);
    if (this.hasMoreThanOne(filtered)) {
      filter.setAlg(jws.getAlgorithmHeaderValue(), SimpleJwkFilter.OMITTED_OKAY);
      filtered = filter.filter(filtered);
    }
    ....

that will catch the case mentioned in the ticket description.

Please let me know if anything (my vacation has ended, so I will respond to you right after).

Many thanks for looking into this!

[Brian Campbell]

I can’t make a change like that to VerificationJwkSelector because it would break lots of existing legitimate scenarios that users of this library have.

That particular conformance is arguably a bit misguided but you could use a simple extension of HttpsJwksVerificationKeyResolver as shown below and in 7b25548 to meet the test requirement.

public static class CustomHttpsJwksVerificationKeyResolver extends HttpsJwksVerificationKeyResolver
    {
        CustomHttpsJwksVerificationKeyResolver(HttpsJwks httpsJkws)
        {
            super(httpsJkws);
        }

        @Override
        protected JsonWebKey select(JsonWebSignature jws, List<JsonWebKey> jsonWebKeys) throws JoseException
        {
            if (jws.getKeyIdHeaderValue() == null && jsonWebKeys.size() > 1)
            {
                throw new UnresolvableKeyException("There are multiple keys in the referenced JWK Set document, but the kid value was not provided in the JOSE Header.");
            }
            return super.select(jws, jsonWebKeys);
        }
    }

‌

‌

‌

‌

[Halyna Kropelnytska]

Yeah, that change should go into a separate method (i.e selectListConsideringKeyId or smth like that) to not break the legacy code. The custom keys resolver was already in my code, just wanted to give a note here, that having this additional method for such keys selection would be nice to have.

Agree that this case of keys selection is optional.
Please let me know if you want to leave this lib as it is, so I will close this ticket.

And again, thanks a bunch for this library - it’s really cool and simple to use!

[Brian Campbell]

Yeah, please go ahead and close out this ticket. While I understand that it’d be nice to have, I don’t want to add to the codebase in support of a conformance test of questionable value.

[Halyna Kropelnytska]

This request was an optional to have in this library. Until more use cases appear, this is a redundant request.