Repository navigation
Fix of getting client keystore without private pem. - #54006
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The focused authorization change aligns with the sibling endpoint, with consistent test expectations and upgrade guidance.
0 open findings
What changed in this PR
Restricts client keystore downloads to callers with configure permission, preventing view-only administrators from accessing private keys.
Changes:
- Replaces the download endpoint’s view permission check with configure permission.
- Updates permission-test expectations.
- Documents the access change and required permissions.
| File | Description |
|---|---|
| tests/base/src/test/java/org/keycloak/tests/admin/PermissionsTest.java | Updates keystore download permission expectations. |
| services/src/main/java/org/keycloak/services/resources/admin/ClientAttributeCertificateResource.java | Requires configure permission for downloads. |
| docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc | Explains the restriction and upgrade impact. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
82c8177 to
c198b26
Compare
c198b26 to
5acf066
Compare
| @Operation( summary = "Get a keystore file for the client, containing the public certificate", deprecated = true) | ||
| public byte[] getKeystore(@Parameter(description = "Keystore configuration as JSON") final KeyStoreConfig config) { | ||
| auth.clients().requireView(client); | ||
| auth.clients().requireConfigure(client); |
There was a problem hiding this comment.
If we do this change, we are effectively requiring that seeing "publicKey" and "certificate" of client requires "manage client" permission. But in that case, we would need to change all other places where those are accessed . Otherwise, we will likely receive bunch of follow-up CVEs like You changed getKeystore to require "configure-client" permissions, however data can be accessed by endpoint XY just with "view-clients" permissions :-)
This might be rather complicated and error-prone... As AFAIK for example normal "client GET" endpoint will include those attributes by default...
TBH I am not seeing a problem to return just "publicKey" and "certificate" with only view-clients permissions. The GH report mentioned that the problem is rather the fact that getKeystore might return privateKey information. Could we rather keep requireView for this endpoint, but just make sure that privateKey information is never returned from the getKeystore?
So I wonder if the method getKeystore could be slightly simplified? It seems that CertificateInfoHelper.getCertificateFromClient seems to never return private-key inside the representation? If it is the case, maybe the method can omit privatePem variable and related conditions and call to getKeystore might be instead just something like getKeystore(config, null, certPem) ?
IMO in this case, it will not be needed to make any upgrading note at all and maybe not even changes to PermissionsTest ? Also I am not sure if we can test this as the behavior is probably unchanged (I recall there was a related fix by @jimmychakkalakal which may already fixed this and filtered privateKey).
In summary: I see the scope of the PR (and fix) is just a small refactoring of that method getKeystore to simplify it and harden it to make sure that it can never return privateKey . WDYT?
5acf066 to
6bc0c0a
Compare
Signed-off-by: Lukas Hanusovsky <lhanusov@redhat.com>
6bc0c0a to
3288c5d
Compare
Closes #49240
Co-authored: Claude noreply@anthropic.com