Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better support for OIDC JWK keys without kid #35529

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

sberyozkin
Copy link
Member

This PR is the last one to get the OIDC Basic RP test plan passing:
https://www.certification.openid.net/plan-detail.html?plan=RPUGzpD20SQYS&public=true

This PR improves how verification keys without kid property are handled.
By default, the token includes a key identifier, kid header and it is used to find a matching JWK, example, if the token kid is 1, then, given [{"kid":"1", "kty":"RSA", "alg":"RS256"}, {{"kid":"2", "kty":"RSA", "alg":"RS256"}], the key with the "kid":"1" will be selected.

There is a case (this was driven by a concrete user requirement), when the token does not include a key identifier (kid) property. Quarkus already supports verifying such tokens, but only (and this restriction was created without any specific justification) if the verification key set has a single JWK key only without kid, for example: [{"kty":"RSA", "alg":"RS256"}] - i.e, single key set only with this single key having no kid.

As it happens, the the OIDC Basic RP test plan tests this case too, but the verification key set contains more than one key without kid, with each key having its own key type, for example: [{"kty":"RSA", "alg":"RS256"}, {{"kty":"EC", "alg":"ES256"}] - i.e here 2 keys, one RSA, another EC, both without kid are included, and the test is expected to get the token without kid, signed with an RSA key, verified by the key with the matching key type, in this example, the first one, from the set.

So this PR just makes sure that all the keys without kid (or X509 thumbprint) are keyed by their type, so when the token without kid arrives, a single key with the matching key type is chosen.

The test plan has another test, where more than one key without kid but with the same key type is available, for example:

[{"kty":"RSA", "alg":"RS256"}, {{""kty":"RSA", "alg":"RS256"}] - with 2 results supported - the first one, if the token has no kid then Quarkus fails to verify it because more than one key without kid with the matching RSA type is available; and the 2nd result is that Quarkus iterates over all such matching keys until the verification succeeds. I don't see the 2nd result being an option for us at the moment - it can take a lot of time to iterate over all such keys, the whole such scenario seems very much test-specific (not practical) so I chose the 1st option - fail to verify in this case - which matches the current main expectations too.

So to summarize, the only real improvement which this PR brings is it allows correctly identify a matching JWK without kid in a set containing more than one JWK, with each JWK having its own key type, with only JWK per specific key type possible.

Tests added to OidcProviderTest to show it, integration-tests/oidc-wiremock already has tests for the current case where a JWK set has a single key only

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments. Looks good except for the LinkedList usage :)

@sberyozkin sberyozkin added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 24, 2023
@sberyozkin
Copy link
Member Author

@geoand @gastaldi Proposed changes are done, cheers

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 24, 2023

✔️ 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.

@sberyozkin sberyozkin merged commit 4abf865 into quarkusio:main Aug 24, 2023
@quarkus-bot quarkus-bot bot added this to the 3.4 - main milestone Aug 24, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 24, 2023
@sberyozkin sberyozkin deleted the oidc_jwk_no_kid branch August 24, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants