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

Support for dynamic OIDC JWK set resolution #36935

Merged

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Nov 7, 2023

Fixes #36563

PR to support resolving keys at the moment when the token is available, with token claims or headers containing the information required to fetch correct verification keys as well as additionally affect an optional JWKS endpoint specific authentication.

Typically, JSON web keys are resolved early from the public endpoint, when the OIDC connection is established, when the JWKS endpoint address is discovered or manually configured. In the use case which this PR targeting, it is impossible because the JWKS endpoint authentication is required and the authentication credentials for the key retrieval is derived from the token - which is not available.

So this PR does the following:

  • Adds quarkus.oidc.jwks.resolve-early configuration property, true by default, setting it to falseenables a dynamic, just in time JWK resolution.
  • Since the JWK resolution is driven by the current token, as opposed to the read-once approach at the initialization time, caching resolved keys is necessary, so the defautt in-memory cache support is provided
  • The actual OIDC logic changes are barely affected, quarkus.oidc.jwks.resolve-early=false then the keys are simply resolved at the token verification time
  • Test confirming the use case is now supported is provided, I've just updated the existing test but more tests will follow incrementally, it is hard to cover all the variations from the start, the important point, it is not enabled by default as well.
  • As it happens TokenIntrospection/UserInfo cache but also BackChannel logout cache had the same code I added to thus dynamic resolver, so it was a good opportunity to refactor it all into a single MemoryCache class and remove a ton of duplicated code - this is also an early step toward supporting customizing cache support only for all of these features

CC @calvernaz

@quarkus-bot quarkus-bot bot added the area/oidc label Nov 7, 2023
@quarkus-bot quarkus-bot bot added the area/oidc label Nov 7, 2023
@sberyozkin sberyozkin force-pushed the oidc_verification_key_resolver_provider branch 4 times, most recently from d161990 to c2483bb Compare November 10, 2023 18:15
@sberyozkin sberyozkin changed the title Support for OIDC verification time JWK set resolution Support for dynamic OIDC JWK set resolution Nov 10, 2023
@sberyozkin sberyozkin force-pushed the oidc_verification_key_resolver_provider branch from c2483bb to 0e35374 Compare November 10, 2023 18:34
@sberyozkin sberyozkin marked this pull request as ready for review November 10, 2023 18:36
@sberyozkin
Copy link
Member Author

Dear reviewers, please wait till Monday or so before starting reviewing, in case I may change something, will ping you, thanks

@gastaldi gastaldi removed their request for review November 10, 2023 18:45
@gastaldi
Copy link
Contributor

I'll be on vacation until Nov 29th, so I'll leave it to the experts 😉

This comment has been minimized.

@sberyozkin
Copy link
Member Author

Super, have a great time off, @gastaldi 👍

@sberyozkin sberyozkin force-pushed the oidc_verification_key_resolver_provider branch from 0e35374 to 6befdce Compare November 13, 2023 15:09
@sberyozkin
Copy link
Member Author

@pedroigor @michalvavrik I've added a MemoryCacheTest unit test, simply because of the refactoring the existing code duplicated across several places into its own class, the memory cache is not a key feature of this PR, it will probably be replaced over time, so for now I'd like to propose not to pay the main attention to how it is done, as far as the dynamic jwk resolver is concerned it only reuses the memory cache code which has been there for a while - and it is also integration tested for the introspection/userinfo cache case.

Also made it easier to extract token in the OIDC request context properties.

So as described in the issue description, the use case is as follows: OIDC connection is initialized but JSON web keys must not be fetched just yet, but only when a token (ID token or access token) is already available, so the only main thing this PR does is just optionally delaying fetching the keys, nearly the same case as refreshing them.

Then once the token is available, it must be available to the OIDC request filter which will use this token to create an authentication token to the JWKS endpoint to make sure the right keys are returned.

PR is ready for review

@sberyozkin sberyozkin marked this pull request as draft November 13, 2023 16:45
@sberyozkin
Copy link
Member Author

Sorry, I've rerun the affected test but obviously something else got broken

@sberyozkin
Copy link
Member Author

Hmm it is the memory cache test which was passing for me

@sberyozkin sberyozkin force-pushed the oidc_verification_key_resolver_provider branch from 6befdce to 4a6db62 Compare November 13, 2023 16:58
@sberyozkin sberyozkin marked this pull request as ready for review November 13, 2023 16:58
@sberyozkin
Copy link
Member Author

Giving the await condition a couple of extra seconds seems to stabilize it

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

I tried to follow code the best I could, but I'm not able to foresee all consequences of your changes, for I don't know OIDC well enough. I'll let @pedroigor to review alone or please find additional reviewer. Thanks

Copy link
Contributor

@pedroigor pedroigor left a comment

Choose a reason for hiding this comment

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

@sberyozkin LGTM.

This comment has been minimized.

@sberyozkin
Copy link
Member Author

Thanks @pedroigor , let me do a few minor updates as proposed by @michalvavrik and I'll merge, cheers

@sberyozkin
Copy link
Member Author

@michalvavrik Thanks for the review, I'll apply minor changes tomorrow as I have no laptop right now.

@sberyozkin
Copy link
Member Author

@michalvavrik FYI, as far as OIDC is concerned, there are no any fundamental changes, tokens have to be verified by public keys fetched from OIDC keys endpoint which is done at the OIDC connection and discovery time, but the use case is to delay it until the token is available. So if this requirement is enabled then we simply retrieve keys at the token verification time

@sberyozkin sberyozkin force-pushed the oidc_verification_key_resolver_provider branch from 4a6db62 to af65f8a Compare November 14, 2023 10:58
Copy link

quarkus-bot bot commented Nov 14, 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 5066de2 into quarkusio:main Nov 14, 2023
21 checks passed
@sberyozkin sberyozkin deleted the oidc_verification_key_resolver_provider branch November 14, 2023 12:10
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Nov 14, 2023
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.

OIDC ID token introspection
4 participants