Authenticator config surfaces expose raw reCAPTCHA secrets#51033
Open
vramik wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Masks sensitive authenticator configuration values to address CVE-2026-16104.
Changes:
- Applies secret stripping to authenticator configuration endpoints and realm exports.
- Discovers secret properties from authenticator factories.
- Adds masking, vault-reference, and null-config tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
AuthenticationManagementResource.java |
Masks authenticator configuration responses. |
StripSecretsUtils.java |
Adds authenticator secret detection and masking. |
StripSecretsUtilsTest.java |
Tests authenticator configuration masking. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
server-spi-private/src/main/java/org/keycloak/models/utils/StripSecretsUtils.java:155
- The new unknown-key masking changes the existing integration contract, but
AuthenticatorConfigTest.testUpdateConfigstill expects its undeclaredconfigKey2value to round-trip asconfigValue2(lines 153–161), so that test will now fail. Update that integration test for the masked response and add a secret GET→PUT round-trip assertion for the preservation branch.
config.replaceAll((key, value) ->
nonSecretPropertyNames.contains(key) ? value : maskNonVaultValue(value));
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.BrowserFlowTest#testUserWithOneAdditionalFactorOtpSuccessorg.keycloak.testsuite.oauth.ClientAuthPostMethodTest#testPostAuthenticationNotAllowedWhenBasicRequestedorg.keycloak.testsuite.oauth.ClientAuthPostMethodTest#testBasicAuthenticationNotAllowedWhenPostRequestedorg.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClickKeycloak CI - Forms IT (firefox) |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.BrowserFlowTest#testUserWithOneAdditionalFactorOtpSuccessorg.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRefreshInTab1Keycloak CI - Forms IT (chrome) org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRegisterClickKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.oauth.ClientAuthPostMethodTest#testPostAuthenticationNotAllowedWhenBasicRequestedorg.keycloak.testsuite.oauth.ClientAuthPostMethodTest#testBasicAuthenticationNotAllowedWhenPostRequested |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
server-spi-private/src/main/java/org/keycloak/models/utils/StripSecretsUtils.java:175
- This allowlist is global rather than scoped to the factory that owns this configuration. An unknown or legacy secret is therefore returned raw whenever its key happens to be declared non-secret by any other authenticator (the integration test itself attaches
site.keyto an IdP authenticator), defeating the fail-closed handling of unknown keys; resolve properties from the owning execution/factory and mask all values for orphaned or ambiguous configurations.
nonSecretNames.add(p.getName());
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.BrowserFlowTest#testUserWithOneAdditionalFactorOtpSuccessorg.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRequiredActionKeycloak CI - Forms IT (chrome) org.keycloak.testsuite.oauth.ClientAuthPostMethodTest#testPostAuthenticationNotAllowedWhenBasicRequestedorg.keycloak.testsuite.oauth.ClientAuthPostMethodTest#testBasicAuthenticationNotAllowedWhenPostRequested |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
services/src/main/java/org/keycloak/services/resources/admin/AuthenticationManagementResource.java:1627
- Masking this response breaks the Admin UI's execution-move path:
FlowDetails.tsx:109-150reads this representation, deletes the original execution/config, then passes the masked map toPOST /executions/{id}/config, which stores"**********"as the new secret because that endpoint has no original value left to restore. Moving a configured reCAPTCHA execution will therefore invalidate its credential; the move/copy operation needs a server-side secret-preserving path (or must require a replacement secret) before masked GET responses can be reused there.
return StripSecretsUtils.stripSecrets(session, ModelToRepresentation.toRepresentation(config));
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.BrowserFlowTest#testUserWithOneAdditionalFactorOtpSuccess |
Comment on lines
+1686
to
+1695
| Map<String, String> newConfig = RepresentationToModel.removeEmptyString(rep.getConfig()); | ||
| if (newConfig != null && exists.getConfig() != null) { | ||
| newConfig.entrySet().removeIf(e -> | ||
| ComponentRepresentation.SECRET_VALUE.equals(e.getValue()) && !exists.getConfig().containsKey(e.getKey())); | ||
| newConfig.replaceAll((key, value) -> | ||
| ComponentRepresentation.SECRET_VALUE.equals(value) | ||
| ? exists.getConfig().get(key) | ||
| : value); | ||
| } | ||
| exists.setConfig(newConfig); |
|
|
||
| } | ||
| return ModelToRepresentation.toRepresentation(config); | ||
| return StripSecretsUtils.stripSecrets(session, ModelToRepresentation.toRepresentation(config)); |
Comment on lines
+1688
to
+1693
| newConfig.entrySet().removeIf(e -> | ||
| ComponentRepresentation.SECRET_VALUE.equals(e.getValue()) && !exists.getConfig().containsKey(e.getKey())); | ||
| newConfig.replaceAll((key, value) -> | ||
| ComponentRepresentation.SECRET_VALUE.equals(value) | ||
| ? exists.getConfig().get(key) | ||
| : value); |
This was referenced Jul 21, 2026
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.MultipleTabsLoginTest#multipleTabsParallelLoginTestWithAuthSessionExpiredAndRefreshInTab1Keycloak CI - Forms IT (chrome) |
Closes keycloak#51005 Signed-off-by: vramik <vramik@redhat.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/base/src/test/java/org/keycloak/tests/admin/authentication/AuthenticatorConfigTest.java:333
- This drops representation validation for CREATE events, leaving the issue's administrative-log exposure path untested even though
AdminEventBuildernow relies on the new formatter to sanitize this payload. Keep an assertion in the secret test that the CREATE event containsSECRET_VALUErather thanmyRealSecret.
AdminEventAssertion.assertEvent(adminEvents.poll(), OperationType.CREATE, AdminEventPaths.authAddExecutionConfigPath(executionId), ResourceType.AUTHENTICATOR_CONFIG);
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.BrowserFlowTest#testUserWithOneAdditionalFactorOtpSuccess |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #51005