Repository navigation
Enable configurable client_id parameter validation for federated client assertions - #48026
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-out for client_id form-parameter validation against the JWT sub when processing federated client assertions, and extends tests/utilities to send client_id alongside client_assertion when explicitly provided.
Changes:
- Add
isClientIdParamValidationEnabled()toggle (default enabled) to relaxclient_idvssubvalidation when needed. - Thread a new
clientIdParamValidationEnabledflag throughFederatedJWTClientValidatorinstantiation (disabled for Kubernetes). - Extend Kubernetes client-auth tests and the HTTP request helper to cover requests that include
client_idwith client assertions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/utils-shared/src/main/java/org/keycloak/testsuite/util/oauth/AbstractHttpPostRequest.java |
Sends client_id with client assertions when explicitly set on the request builder. |
tests/base/src/test/java/org/keycloak/tests/client/authentication/external/KubernetesClientAuthTest.java |
Adds coverage for Kubernetes federated assertions when client_id is present/mismatched. |
services/src/main/java/org/keycloak/broker/spiffe/SpiffeIdentityProvider.java |
Updates validator construction for new flag (currently set to validate client_id). |
services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java |
Updates validator construction for new flag (keeps validation enabled). |
services/src/main/java/org/keycloak/broker/kubernetes/KubernetesIdentityProvider.java |
Disables client_id-param validation for Kubernetes assertions. |
services/src/main/java/org/keycloak/authentication/authenticators/client/FederatedJWTClientValidator.java |
Adds constructor flag and overrides isClientIdParamValidationEnabled(). |
services/src/main/java/org/keycloak/authentication/authenticators/client/AbstractJWTClientValidator.java |
Gates client_id mismatch failure behind isClientIdParamValidationEnabled(). |
|
Failures don't seem to be related |
| @@ -103,8 +103,10 @@ private boolean validateClient() { | |||
|
|
|||
| String clientIdParam = context.getHttpRequest().getDecodedFormParameters().getFirst(OAuth2Constants.CLIENT_ID); | |||
| if (clientIdParam != null && !clientIdParam.equals(clientId)) { | |||
| logger.debug("client_id parameter does not match JWT subject"); | |||
| return failure("client_id parameter does not match sub claim"); | |||
| if (isClientIdParamValidationEnabled()) { | |||
There was a problem hiding this comment.
I suggest that instead of introducing isClientIdParamValidationEnabled() , we will check that:
-
if
client_idparameter is provided, we always check that it matches with the clientId of the clientModel obtained fromclientAssertionState(the client obtained on the line 115 byClientModel client = clientAssertionState.getClient();) -
So this means that this whole check can be moved below (after line 115).
-
I agree that it does not makes sense to always check that
client_idparameter matches with thesubclaim as this would be true just for JWT provided byprivate_key_jwtorclient_secret_jwtclient authentication, but it likely will not be true for the cases like spiffe (as per https://datatracker.ietf.org/doc/draft-ietf-oauth-spiffe-client-auth/ , thesubclaim does not equalclient_id, but it is something different and I suppose that for kubernetes it is the case as well).
However I think that in all cases, we can check if client_id (if sent) matches with the client identified by the authenticator. If it does not match, it is clear that something strange is going on as I don't see any case why client_id parameter would be different value than identified client?
If doing this, it will simplify this PR (as there will not be any changes needed to support isClientIdParamValidationEnabled() and send different values from different IDPs as well.)
WDYT?
There was a problem hiding this comment.
Thanks a million for taking the time to look into this!
I 100% agree with you! The simplified code indeed looks much better and cleaner.
There was a problem hiding this comment.
@mposolda The PR has been updated and should be ready for re-review. Thanks!
d0d8221 to
2ebc003
Compare
Signed-off-by: Sebastian Łaskawiec <sebastian.laskawiec@defenseunicorns.com>
2ebc003 to
48d28d3
Compare
|
Moving to draft to investigate failures. |
Signed-off-by: Sebastian Łaskawiec <sebastian.laskawiec@defenseunicorns.com>
3456755 to
d0e7834
Compare
…ient assertions (keycloak#48026) Closes keycloak#48024 Signed-off-by: Sebastian Łaskawiec <sebastian.laskawiec@defenseunicorns.com>
Summary
This Pull Request introduces a configurable
client_idvalidation for the Signed JWTs and disables it for Kubernetes Service Accounts.As per RFC 7523, the Client ID for Client Assertions is optional.
Related tickets
Closes #48024
Relates to keycloak/terraform-provider-keycloak#1542
Relates to keycloak/terraform-provider-keycloak#1552