Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The rotated-secret attribute validation path lacks a RAT-authenticated update test verifying that Vault expressions are rejected with HTTP 400 and no persisted changes.
1 open issue
What changed in this PR
This PR addresses #54382 by blocking Vault-expression injection through non-admin Dynamic Client Registration (DCR) requests and preventing installation configurations from exposing resolved Vault secrets.
Changes:
- Reject Vault expressions in client secrets and attributes submitted by non-admin DCR callers.
- Return raw secrets in adapter configurations for both secret-based client authenticators.
- Add integration tests for DCR rejection and adapter serialization.
| File | Description |
|---|---|
| tests/base/src/test/java/org/keycloak/tests/client/DcrVaultSecretDisclosureTest.java | Adds Vault-backed DCR and adapter configuration tests. |
| tests/base/src/test/java/org/keycloak/tests/admin/ClientVaultTest.java | Makes the existing test class public. |
| services/src/main/java/org/keycloak/services/clientregistration/AbstractClientRegistrationProvider.java | Validates untrusted secrets and attributes during registration and updates. |
| services/src/main/java/org/keycloak/authentication/authenticators/client/JWTClientSecretAuthenticator.java | Stops resolving Vault secrets in JWT adapter configurations. |
| services/src/main/java/org/keycloak/authentication/authenticators/client/ClientIdAndSecretAuthenticator.java | Stops resolving Vault secrets in client-secret adapter configurations. |
Review effort: Balanced
3d59980 to
bdb5c3d
Compare
There was a problem hiding this comment.
@jimmychakkalakal
Allowing the same reference in the same field grants no new Vault access, provided adapter export never resolves it. It does not establish who originally set the reference, but rejecting an unchanged value would not remove previously injected references either.
Remaining gaps: non-admin creation rejection and moving a reference to a different attribute key.
9451214 to
0e15a65
Compare
There was a problem hiding this comment.
@jimmychakkalakal Thanks for the PR!
One general remark: Are they similar issues possible by other ways? If yes, we will likely receive follow-up CVE reports as usual...
I am concerned about those questions:
AFAIK Vault feature has support to store secrets of identity providers and user storage providers (like LDAP secrets). If user has permission to manage identity providers, does it allow him to set the vault secret of the IDP same as some client secret of specific client foo ? Does it then allow him to obtain the secret of the client foo? If yes, then it might be a problem as user with the role manage-identity-providers should not be allowed to obtain client secrets as he may have permissions to CRUD identity providers, but not to CRUD clients. I am not 100% sure if this is a real issue, but can you please check it?
One additional though: Maybe this should be also partially handled as the issue to improve the documentation? For example if client administrator of the client foo creates client secret with the key ${foo.secret} , then the administrator of the IDP (or administrator of the other client bar) may have easier task as he can guess the vault key ${foo.secret} , which may allow him to see the secret of the client foo (if my concern from the previous paragraph is correct). In that case, it might be good if the documentation of the vault is improved, so that it contains some hints that the key itself should not be easy to guess. But again, not 100% sure if this is real issue?
Closes keycloak#54382 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
Closes keycloak#54382 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
Closes keycloak#54382 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
Closes keycloak#54382 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
6ce7c0b to
17ecae6
Compare
@mposolda Thanks, the concern about IdP secrets is valid. I was able to reproduce it locally with an integration test. As this is a separate authorization gap and limited to Vault scope. I added a note to the Vault docs about guessable keys and sharing keys across components, as you suggested. |
|
Hi @jimmychakkalakal and @mposolda, I saw the follow-up discussion on the analogous Vault paths and @jimmychakkalakal’s plan to handle it separately, so I don’t want to duplicate anything. If regression tests or review would be useful, I’d be glad to help. Otherwise, no worries. |
There was a problem hiding this comment.
@jimmychakkalakal Sounds good, Thanks!
…eycloak#54463) Closes keycloak#54382 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com> (cherry picked from commit df2914e) Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
…eycloak#54463) Closes keycloak#54382 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com> (cherry picked from commit df2914e) Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
Prevent Vault secret resolution and injection in DCR client secrets
Closes #54382