-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 OIDC MTLS binding #45121
Conversation
646fa73
to
2b7225c
Compare
🙈 The PR is closed and the preview is expired. |
2eceece
to
06399b9
Compare
612d582
to
d26833d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@cescoffier @michalvavrik, would you like to review, comment ? This is an important OIDC/OAuth2 token security hardening feature, but the implementation is really straightforward, with no OIDC logic impact of any kind. If Quarkus expects bearer access tokens be certificate bound, then the token must have a certificate tumbprint claim which must match the current MTLS client certificate (chain leaf) thumbprint - this is all to it. |
docs/src/main/asciidoc/security-oidc-bearer-token-authentication.adoc
Outdated
Show resolved
Hide resolved
d26833d
to
365dc84
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
365dc84
to
00e05f5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
@sberyozkin Nice to see yet another key capability supported.
Are you also updating client registrations as per https://datatracker.ietf.org/doc/html/rfc8705#name-client-registration-metadata-2?
[source,properties] | ||
---- | ||
quarkus.oidc.auth-server-url=${your_oidc_provider_url} | ||
quarkus.oidc.token.certificate-bound=true <1> |
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.
Would it make sense to have a specific namespace for sender-constrained tokens such as quarkus.oidc.token.binding
and have certificate-bound tokens configured as quarkus.oidc.token.binding.certificate=true
?
Thinking about DPoP, for instance, we could use the same namespace to configure it. Something like quarkus.oidc.token.binding.dpop.<properties>
.
It could also help to add more capabilities, when needed, for each individual token-binding method.
Not sure, only a suggestion.
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.
Hi @pedroigor I think you are right, thanks for thinking ahead, we'd want to have dpop configured at the same level. I'll work on encapsulating this configuration as you suggested
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.
Hey @pedroigor, I've encapsulated it as token.binding.certificate
. So far it is the only properties, but one case I've heard about is about TLS termination so we may want to support a header passing the cert thumbprint internally for those cases, and like you said, other binding customizations will be easier to handle.
I'm not sure yet if we will put dpop
inside or next to binding
as it is a bit different, and there is also dpop related to the authorization code flow, but we can discuss it in a month or two
thanks
Hi @pedroigor, thanks for the review, approval, and useful feedback - I'll definitely deal with your configuration related suggestion before merging.
I've totally missed it, so right now the client is setup in the test realm, it has the So another option would be to register a client dynamically using our new I'll keep this option in mind and open an issue to investigate - may be we can work with the Keycloak team to use |
@michalvavrik Thanks for your comments as well |
1f75202
to
f7a8946
Compare
Status for workflow
|
This comment has been minimized.
This comment has been minimized.
Some strange native test build failure, fine on my machine, rerunning the job |
Status for workflow
|
Fixes #4482.
This PR adds support for OIDC MTLS Binding, to support cases where it is absolutely necessary to prove the access token was issued to the client who is presenting it. The client must authenticate over MTLS and the client certificate's thumbprint must match the JWT token or token introspection confirmation thumbprint.
It took a while to deal with this issue, setting up the tests was tricky, but finally, with the help from Keycloak devservice, inclusive authentication, certificate generation, it all got in place.
The actual source update is quite simple - if the access token must be certificate bound then the MTLS certificate thumbprint is stored in the routing context when the bearer access token is about to be verified, and then it is compared to the JWT token or token introspection confirmation
cnf
x5t#S256
thumbprint.Docs have been updated and tests added to check: