-
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
Support OIDC authorization code flow nonce #35039
Conversation
095f4fe
to
87c84f3
Compare
🙈 The PR is closed and the preview is expired. |
87c84f3
to
1e4a22d
Compare
One test resource was not added, sorry |
docs/src/main/asciidoc/security-oidc-code-flow-authentication.adoc
Outdated
Show resolved
Hide resolved
extensions/oidc/runtime/src/main/java/io/quarkus/oidc/OidcTenantConfig.java
Outdated
Show resolved
Hide resolved
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 once the changes I suggested are added
This comment has been minimized.
This comment has been minimized.
Thanks @gastaldi I'll deal with your comments, Sorry , I've been focusing on the integration tests... should be an easy fix though to the test failure, should check a new property name now |
0ed992b
to
9ea734d
Compare
✖ This workflow run has failed but no jobs reported an error. Something weird happened, please check the workflow run page carefully: it might be an issue with the workflow configuration itself. |
Maven and Gradle tests were skipped |
@gastaldi @michalvavrik Just run the nonce test https://www.certification.openid.net/plan-detail.html?plan=RPUGzpD20SQYS&public=true, (3rd from the bottom), with the changes on |
Fixes #23580.
I've finally have looked at fixing #23580 because there is an OIDC conformance test that checks that a non-matching nonce causes a failure, but it can be a generally useful OIDC code flow hardening enhancement in any case.
From https://openid.net/specs/openid-connect-core-1_0.html#IDToken:
String value used to associate a Client session with an ID Token, and to mitigate replay attacks.
.Quarkus will include a
nonce
query parameter in the redirect to the OIDC provider, will have a matching value securely stored in the state cookie and then when the code flow is completed - it verifies that ID tokennonce
claim matches the one in the cookie - so if the current redirect back to Quarkus has been comprimized - ID token nonce returned from the secure Keycloak/etc connection will help detecting it, and vice versa, if the connection to Keycloak has been affected.So this PR does introduce a new OIDC feature - but it is a very simple PR - we simply store the new nonce securely in the state cookie and then check ID token claim and both values must match.
As far as storing the nonce securely in the state cookie is concerned - it depends on what has already been done for the PKCE code_verifier property - which is encrypted in the state cookie as it must not be visible in the cookie - so this PR adds a nonce property to the
CodeFlowAuthenticationBean
which captures the data which must be encrypred and then retrieves it from the state cookie, after the secured state has been decrypted.With
nonce
we now can have either PKCEcode_verifier
ornonce
or both of these properties having to be encrypted. More such properties may have to be secured in the state cookie. Therefore I deprecatedquarkus.oidc.authentication.pkce-secret
as this secret is not only about encrypting the (pkce)code_verifier
but at least 2 or possibly more properties, and the new property is now calledquarkus.oidc.authentication.state-secret
.quarkus.oidc.authentication.pkce-secret
will be supported for a ling while anyway I guess.Updated tests, one of the existing test tenants,
tenant-https
has been updated to requirenonce
- this tenant also requires pkce, so 4 related tests now check that both thecode_verifier
andnonce
are available in the decrypted state cookie.Also added 2 tests where only
nonce
is expected,tenant-nonce
. Positive test confirms the decrypted state cookie containsnonce
only, and negative test - that401
is returned if the state cookie is modified to remove the nonce from it.So overall, the changes are not really some significant OIDC changes, but @gastaldi, @michalvavrik, we can wait for @pedroigor to review when he is back, if you'd like, thanks