8000 Mask secrets in client installations when view-only permissions are used by vmuzikar · Pull Request #53758 · keycloak/keycloak · GitHub
[go: up one dir, main page]

Skip to content

Mask secrets in client installations when view-only permissions are used - #53758

Merged
mposolda merged 3 commits into
keycloak:mainfrom
vmuzikar:fix/53751-installation-secret-disclosure
Oct 19, 2026
Merged

mposolda merged 3 commits into
keycloak:mainfrom
vmuzikar:fix/53751-installation-secret-disclosure

Conversation

@vmuzikar
@vmuzikar vmuzikar commented Oct 8, 2026
Copy link
Copy Markdown
Contributor

This also extends the masking to SAML private keys. Those are not persisted any more (see #49239) but existing clients still could contain those.

Closes #53751

@vmuzikar
vmuzikar requested a balanced review from Copilot October 8, 2026 17:21
Copilot AI left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Nested client-secret-jwt credentials remain exposed to view-only callers.

1 open finding
What changed in this PR

Adds permission-aware secret masking to client installation configurations.

Changes:

  • Propagates includeSecrets through installation providers.
  • Masks OIDC secrets and stored SAML private keys for view-only callers.
  • Adds regression tests and upgrade documentation.
File Description
tests/​base/​src/​test/​java/​org/​keycloak/​tests/​client/​AdapterInstallationConfigTest.java Tests registration endpoint masking.
tests/​base/​src/​test/​java/​org/​keycloak/​tests/​admin/​ClientSecretVisibilityTest.java Tests OIDC and SAML visibility.
services/​src/​main/​java/​org/​keycloak/​services/​resources/​admin/​ClientResource.java Derives secret access from management permission.
services/​src/​main/​java/​org/​keycloak/​services/​managers/​ClientManager.java Applies permission-aware credential generation.
services/​src/​main/​java/​org/​keycloak/​services/​managers/​ClientInstallationUtils.java Centralizes OIDC secret masking.
services/​src/​main/​java/​org/​keycloak/​services/​clientregistration/​AdapterInstallationClientRegistrationProvider.java Masks view-only registration responses.
services/​src/​main/​java/​org/​keycloak/​protocol/​saml/​installation/​SamlSPDescriptorClientInstallation.java Adopts the provider signature.
services/​src/​main/​java/​org/​keycloak/​protocol/​saml/​installation/​ModAuthMellonClientInstallation.java Masks exported signing keys.
services/​src/​main/​java/​org/​keycloak/​protocol/​saml/​installation/​KeycloakSamlSubsystemInstallation.java Propagates secret visibility.
services/​src/​main/​java/​org/​keycloak/​protocol/​saml/​installation/​KeycloakSamlSubsystemCliInstallation.java Masks CLI private keys.
services/​src/​main/​java/​org/​keycloak/​protocol/​saml/​installation/​KeycloakSamlClientInstallation.java Masks XML private keys.
services/​src/​main/​java/​org/​keycloak/​protocol/​oidc/​installation/​KeycloakOIDCJbossSubsystemClientInstallation.java Applies OIDC masking to XML.
services/​src/​main/​java/​org/​keycloak/​protocol/​oidc/​installation/​KeycloakOIDCJbossSubsystemClientCliInstallation.java Applies OIDC masking to CLI output.
services/​src/​main/​java/​org/​keycloak/​protocol/​oidc/​installation/​KeycloakOIDCClientInstallation.java Applies masking to JSON output.
services/​src/​main/​java/​org/​keycloak/​protocol/​docker/​installation/​DockerVariableOverrideInstallationProvider.java Adopts the provider signature.
services/​src/​main/​java/​org/​keycloak/​protocol/​docker/​installation/​DockerRegistryConfigFileInstallationProvider.java Adopts the provider signature.
services/​src/​main/​java/​org/​keycloak/​protocol/​docker/​installation/​DockerComposeYamlInstallationProvider.java Adopts the provider signature.
server-spi-private/​src/​main/​java/​org/​keycloak/​protocol/​ClientInstallationProvider.java Adds the includeSecrets contract.
docs/​documentation/​upgrading/​topics/​changes/​changes-26_8_1.adoc Documents masking and API changes.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@vmuzikar
vmuzikar force-pushed the fix/53751-installation-secret-disclosure branch from 0945e43 to b9b47b6 Compare October 8, 2026 17:35
@vmuzikar
vmuzikar requested a balanced review from Copilot October 8, 2026 17:35
Copilot AI left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Permission-aware masking is consistently propagated across providers and covered by focused regression tests.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@vmuzikar
vmuzikar marked this pull request as ready for review October 8, 2026 18:57
@vmuzikar
vmuzikar requested a review from a team as a code owner October 8, 2026 18:57
Copilot AI balanced review requested due to automatic review settings October 8, 2026 19:07
@vmuzikar
vmuzikar force-pushed the fix/53751-installation-secret-disclosure branch from b9b47b6 to f9a3ca7 Compare October 8, 2026 19:07
Copilot AI left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this 10BC8 comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The security-sensitive authorization and credential-handling changes span a provider SPI and multiple installation formats, warranting final human review.

0 open findings

🧠 Review effort: Balanced

@vmuzikar
vmuzikar requested a review from mposolda October 12, 2026 12:42
@mposolda mposolda self-assigned this Oct 13, 2026
@mposolda mposolda left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vmuzikar Thanks! I've added 2 minor comments inline.

Moreover, can you please double-check if we don't have some other "alternative" way how only guys with permissions to "view clients" can obtain client secrets? As otherwise, we would have another CVE report for the same thing soon :-) Feel free to use some AI tool or whatever. Please confirm if you already did this and that this point is fine now.

@vmuzikar
Copy link
Copy Markdown
Contributor Author

@mposolda Thanks for the review!

I removed the Upgrading Guide note.

Moreover, can you please double-check if we don't have some other "alternative" way how only guys with permissions to "view clients" can obtain client secrets? As otherwise, we would have another CVE report for the same thing soon :-) Feel free to use some AI tool or whatever. Please confirm if you already did this and that this point is fine now.

Yes, I poked around also with Fable 5.1, and even that thinks the fix is complete. Only thing I missed was a dead code that could reveal the secrets but it was not used and I removed it in the last commit.

Copilot AI balanced review requested due to automatic review settings October 13, 2026 16:04
@vmuzikar
vmuzikar force-pushed the fix/53751-installation-secret-disclosure branch from f9a3ca7 to fbca6e7 Compare October 13, 2026 16:04
Copilot AI left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Custom client authenticators can still disclose credentials stored under keys other than secret.

1 open finding

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 13, 2026 18:17
Copilot AI left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Two existing testsuite authenticators do not implement the newly required SPI method and will fail compilation.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

@vmuzikar
vmuzikar force-pushed the fix/53751-installation-secret-disclosure branch from f4e304f to fd3623d Compare October 13, 2026 18:22
Copilot AI balanced review requested due to automatic review settings October 13, 2026 18:22
@vmuzikar
vmuzikar requested review from a team as code owners October 13, 2026 18:22
Copilot AI left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Permission-aware masking is consistently propagated and covered across the affected OIDC and SAML installation paths.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@lhanusov
Copy link
Copy Markdown
Contributor

this issue is partially related to -> #49240

@vmuzikar
Copy link
Copy Markdown
Contributor Author

this issue is partially related to -> #49240

Yes but that's not for Client Installations. I'd tackle that in a separate PR.

@mposolda mposolda left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vmuzikar Thanks!

I've tried to do manual rebase, hope that helps to fix the remaining test failure (as I was not able to re-run the tests)

Closes keycloak#53751

Signed-off-by: Václav Muzikář <vmuzikar@ibm.com>
Signed-off-by: Václav Muzikář <vmuzikar@ibm.com>
Signed-off-by: Václav Muzikář <vmuzikar@ibm.com>
Copilot AI balanced review requested due to automatic review settings October 19, 2026 13:25
@mposolda
mposolda force-pushed the fix/53751-installation-secret-disclosure branch from fd3623d to 71a1f77 Compare October 19, 2026 13:25
Copilot AI left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Credential-disclosure remediation and provider contract changes warrant human security and compatibility validation.

No open issues 🎉

Review effort: Balanced

@mposolda
mposolda merged commit 32d8958 into keycloak:main Oct 19, 2026
98 checks passed
@vmuzikar
vmuzikar deleted the fix/53751-installation-secret-disclosure branch October 19, 2026 15:22
vmuzikar added a commit to vmuzikar/keycloak that referenced this pull request Oct 20, 2026
…sed (keycloak#53758)

Closes keycloak#53751

Signed-off-by: Václav Muzikář <vmuzikar@ibm.com>
(cherry picked from commit 32d8958)
Signed-off-by: Václav Muzikář <vmuzikar@ibm.com>
PadzerButch pushed a commit to PadzerButch/-50764_test_migration_keycloak that referenced this pull request Oct 22, 2026
…sed (keycloak#53758)

Closes keycloak#53751


Signed-off-by: Václav Muzikář <vmuzikar@ibm.com>
PadzerButch pushed a commit to PadzerButch/-50764_test_migration_keycloak that referenced this pull request Oct 23, 2026
…sed (keycloak#53758)

Closes keycloak#53751


Signed-off-by: Václav Muzikář <vmuzikar@ibm.com>
vmuzikar added a commit that referenced this pull request Nov 6, 2026
…sed (#53758) (#54120)

Closes #53751


(cherry picked from commit 32d8958)

Signed-off-by: Václav Muzikář <vmuzikar@ibm.com>
mposolda pushed a commit that referenced this pull request Nov 12, 2026
…sed (#53758)

Closes #53751

Signed-off-by: Václav Muzikář <vmuzikar@ibm.com>
(cherry picked from commit 32d8958)
Signed-off-by: Václav Muzikář <vmuzikar@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CVE-2026-107604: Incomplete fix for CVE-2026-89298 - view-clients role allows client secret retrieval via installation providers

4 participants

0