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

Request re-authentication if the OIDC session key is unresolved #45659

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Jan 16, 2025

This PR is about improving the OIDC user experience in a multi-tab application.

First, a minor clarification how OIDC session cookie signatures are verified:

When an already authenticated user returns with the session cookie, the ID token's key identifier is used to find a matching key in a key set fetched from the OIDC provider. If no matching key can be found, the key set is refreshed and a new key set is fetched. If no matching key is found at this stage, 401 is returned to the user, an empty screen in prod.

As explained in #45582, what might happen in a multi-tab application, is that one of the open tabs which an already authenticated user has opened may get stale, where the session cookie's ID token key id has no matching verification key due to the key set refreshed after the same user accessed Quarkus from another tab.
The end result of it is that one tab works, another tab returns 401 (which is returned after the process described above).

This PR, instead of terminating the flow with 401 for the session cookie with non-matching key, instead redirects the user to re-authenticate which improves the experience.

The only thing that I'm not quite happy about is that I'm allowing to do it by default, it makes sense to return 302 in the case described in #45582, but Quarkus OIDC, when it sees a token with a non-matched key, can not be sure that the token has no matching key due to the case in #45582, in general a non-matching key can be a pretty serious error deserving the flow termination.

So what I'm saying, that as much as I'm concerned about growing an already large set of OIDC properties, I'll have to add one more property to allow soft redirects in case of unmatched keys during the session re-authentication... And then the PR will be ready for review without any concerns on my part.

@geoand @pedroigor, FYI

@sberyozkin sberyozkin force-pushed the unmatched_key_for_oidc_session_reauthentication branch from 674888b to 28af00b Compare January 22, 2025 19:22
@sberyozkin
Copy link
Member Author

Sorry Georgios, @geoand, for pinging you :-), sometimes I confuse your alias with George's @gastaldi :-), but in any case, you are very welcome to comment on the OIDC/security PRs too, we are getting it integrated quite closely with Quarkus LangChain4j now, so it can be of interest

@geoand
Copy link
Contributor

geoand commented Jan 23, 2025

👌

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.

2 participants