-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
OIDC JWK refresh support #9891
OIDC JWK refresh support #9891
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for some minor changes.
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java
Outdated
Show resolved
Hide resolved
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcUtils.java
Outdated
Show resolved
Hide resolved
@@ -193,7 +193,8 @@ public SecurityIdentity apply(Throwable throwable) { | |||
LOG.debug("State parameter can not be empty or multi-valued"); | |||
return Uni.createFrom().failure(new AuthenticationCompletionException()); | |||
} else if (!stateCookie.getValue().startsWith(values.get(0))) { | |||
LOG.debug("State cookie does not match the state parameter"); | |||
LOG.debugf("State cookie value '%s' does not match the state query parameter value '%s'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it is a good idea to log the state value considering it is used in OAuth2 to prevent CSRF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedroigor I just added it because the user was getting this error, but I suppose seeing 2 different uuids in the logs won't help the users to figure out why it happened :-). OK, let me revert it
@@ -32,6 +35,31 @@ private OidcUtils() { | |||
|
|||
} | |||
|
|||
public static boolean isOpaqueToken(String token) { | |||
return new StringTokenizer(token, ".").countTokens() != 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor comment. Right now we only support JWS, but JWE uses 5 tokens.
But I guess we would handle JWEs as opaque tokens considering that you would need an introspection call to check claims.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedroigor Right, at the moment no local JWE decryption is possible, so they are technically opaque for Quarkus
Fixes #5003
Fixes #7286
This PR:
missingKeyHandler
Vertx Auth 3.9.1 feature with a controlled refresh interval period (10 mins by default). Right now, if the rotation happens and the current token has no local JWK with a matchingkid
to verify it then 2 requests will go to OIDC server, 1 JWK refresh from our handler and one fallback introspection request - this is not really a problem for now as it will happen only once in some long enough irregular rotation interval in a long running applicationOidcResource
server endpoint to emulate KeycloakCC @pmlopes