From b029fdc8ff20ef20e64870bd2c37a9fe0e8f77a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Barto=C5=A1?= Date: Wed, 15 Jul 2026 11:40:21 +0200 Subject: [PATCH 1/2] Rotated client secret remains valid when the feature is disabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #50855 Signed-off-by: Martin Bartoš --- .../oidc/OIDCClientSecretConfigWrapper.java | 17 +++- .../resources/admin/ClientResource.java | 2 + .../ClientSecretRotationDisabledTest.java | 82 +++++++++++++++++++ .../client/ClientSecretRotationTest.java | 44 +++++----- 4 files changed, 121 insertions(+), 24 deletions(-) create mode 100644 tests/base/src/test/java/org/keycloak/tests/client/ClientSecretRotationDisabledTest.java diff --git a/services/src/main/java/org/keycloak/protocol/oidc/OIDCClientSecretConfigWrapper.java b/services/src/main/java/org/keycloak/protocol/oidc/OIDCClientSecretConfigWrapper.java index 54f71dc2bf97..468d57d0f5ea 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/OIDCClientSecretConfigWrapper.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/OIDCClientSecretConfigWrapper.java @@ -9,6 +9,8 @@ import java.util.Objects; import java.util.concurrent.TimeUnit; +import org.keycloak.common.Profile; +import org.keycloak.common.Profile.Feature; import org.keycloak.common.util.Time; import org.keycloak.models.ClientModel; import org.keycloak.models.ClientSecretConstants; @@ -32,9 +34,11 @@ * @author Marcelo Sales */ public class OIDCClientSecretConfigWrapper extends AbstractClientConfigWrapper { + private final boolean isRotationFeatureEnabled; private OIDCClientSecretConfigWrapper(ClientModel client, ClientRepresentation clientRep) { super(client, clientRep); + this.isRotationFeatureEnabled = Profile.isFeatureEnabled(Feature.CLIENT_SECRET_ROTATION); } public static OIDCClientSecretConfigWrapper fromClientModel(ClientModel client) { @@ -84,7 +88,7 @@ public void removeClientSecretRotationInfo() { } public void removeClientSecretRotated() { - if (hasRotatedSecret()) { + if (hasRotatedSecretAttributes()) { setAttribute(CLIENT_ROTATED_SECRET, null); setAttribute(CLIENT_ROTATED_SECRET_CREATION_TIME, null); setAttribute(CLIENT_ROTATED_SECRET_EXPIRATION_TIME, null); @@ -101,9 +105,18 @@ public void setClientSecretCreationTime(long creationTime) { } public boolean hasRotatedSecret() { - return StringUtil.isNotBlank(getAttribute(CLIENT_ROTATED_SECRET)) && StringUtil.isNotBlank(getAttribute(CLIENT_ROTATED_SECRET_CREATION_TIME)); + return isRotationFeatureEnabled && hasRotatedSecretAttributes(); + } + + private boolean hasRotatedSecretAttributes() { + return StringUtil.isNotBlank(getAttribute(CLIENT_ROTATED_SECRET)) + && StringUtil.isNotBlank(getAttribute(CLIENT_ROTATED_SECRET_CREATION_TIME)); } + /** + * Returns the rotated client secret value resolved through the vault. + * Use {@link #hasRotatedSecret()} to check whether a rotated secret is effectively present before calling this method. + */ public String getClientRotatedSecret(KeycloakSession session) { String secret = getAttribute(CLIENT_ROTATED_SECRET); return session == null ? getAttribute(CLIENT_ROTATED_SECRET) : session.vault().getStringSecret(secret).get().orElse(secret); diff --git a/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java b/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java index 2ef31464a83e..ddf945c2c10d 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java @@ -800,6 +800,8 @@ public Response invalidateRotatedSecret() { logger.debug("delete rotated secret"); + // Always remove rotated secret attributes even when the feature is disabled, + // so stale secrets cannot become valid again if the feature is re-enabled. OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientModel(client); CredentialRepresentation rep = new CredentialRepresentation(); diff --git a/tests/base/src/test/java/org/keycloak/tests/client/ClientSecretRotationDisabledTest.java b/tests/base/src/test/java/org/keycloak/tests/client/ClientSecretRotationDisabledTest.java new file mode 100644 index 000000000000..4e3dfb1c3e78 --- /dev/null +++ b/tests/base/src/test/java/org/keycloak/tests/client/ClientSecretRotationDisabledTest.java @@ -0,0 +1,82 @@ +package org.keycloak.tests.client; + +import org.keycloak.common.util.Time; +import org.keycloak.models.ClientSecretConstants; +import org.keycloak.testframework.annotations.InjectUser; +import org.keycloak.testframework.annotations.KeycloakIntegrationTest; +import org.keycloak.testframework.oauth.OAuthClient; +import org.keycloak.testframework.oauth.annotations.InjectOAuthClient; +import org.keycloak.testframework.realm.ManagedUser; +import org.keycloak.testframework.remote.runonserver.InjectRunOnServer; +import org.keycloak.testframework.remote.runonserver.RunOnServerClient; +import org.keycloak.tests.common.TestRealmUserConfig; +import org.keycloak.testsuite.util.oauth.AccessTokenResponse; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +@KeycloakIntegrationTest +public class ClientSecretRotationDisabledTest { + + private static final String CLIENT_ID = "test-app"; + + @InjectOAuthClient + OAuthClient oauth; + + @InjectUser(config = TestRealmUserConfig.class) + ManagedUser user; + + @InjectRunOnServer + RunOnServerClient runOnServer; + + /** + * Verifies that rotated client secrets are not accepted when the CLIENT_SECRET_ROTATION + * feature is disabled, even if the rotated secret attributes remain in the database. + * + * @see Issue #50855 + */ + @Test + public void rotatedSecretNotAcceptedWhenFeatureDisabled() { + String originalSecret = oauth.clientResource().getSecret().getValue(); + + // Verify the original secret works before rotating + oauth.client(CLIENT_ID, originalSecret); + oauth.doLogin(user.getUsername(), "password"); + String code = oauth.parseLoginResponse().getCode(); + AccessTokenResponse response = oauth.doAccessTokenRequest(code); + assertEquals(200, response.getStatusCode()); + assertNotNull(response.getAccessToken()); + + String newSecret = oauth.clientResource().generateNewSecret().getValue(); + + // Set rotated secret attributes directly on the server-side model to bypass the admin + // API cleanup in ClientResource.update() which always removes rotation info when no + // rotation executor is active. + String creationTime = String.valueOf(Time.currentTimeSeconds()); + String expirationTime = String.valueOf(Time.currentTimeSeconds() + 3600); + runOnServer.run(session -> { + var realmModel = session.getContext().getRealm(); + var client = realmModel.getClientByClientId(CLIENT_ID); + client.setAttribute(ClientSecretConstants.CLIENT_ROTATED_SECRET, originalSecret); + client.setAttribute(ClientSecretConstants.CLIENT_ROTATED_SECRET_CREATION_TIME, creationTime); + client.setAttribute(ClientSecretConstants.CLIENT_ROTATED_SECRET_EXPIRATION_TIME, expirationTime); + }); + + // Authentication with the rotated (original) secret must fail when feature is disabled + oauth.client(CLIENT_ID, originalSecret); + oauth.openLoginForm(); + code = oauth.parseLoginResponse().getCode(); + response = oauth.doAccessTokenRequest(code); + assertEquals(401, response.getStatusCode()); + + // Authentication with the current (new) secret must always work + oauth.client(CLIENT_ID, newSecret); + oauth.openLoginForm(); + code = oauth.parseLoginResponse().getCode(); + response = oauth.doAccessTokenRequest(code); + assertEquals(200, response.getStatusCode()); + assertNotNull(response.getAccessToken()); + } +} diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientSecretRotationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientSecretRotationTest.java index 15cb6d7f4c35..0d55422b3dc9 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientSecretRotationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientSecretRotationTest.java @@ -54,6 +54,7 @@ import org.keycloak.testsuite.util.oauth.AuthorizationEndpointResponse; import org.keycloak.util.JsonSerialization; import org.keycloak.util.TokenUtil; +import org.keycloak.utils.StringUtil; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; @@ -231,11 +232,11 @@ public void regenerateSecretOnCurrentSecretNotExpired() throws Exception { .get(cidConfidential); String firstSecret = clientResource.getSecret().getValue(); String secondSecret = clientResource.generateNewSecret().getValue(); - OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation( - clientResource.toRepresentation()); + ClientRepresentation rep = clientResource.toRepresentation(); + OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation(rep); assertThat(secondSecret, not(equalTo(firstSecret))); - assertThat(wrapper.hasRotatedSecret(), is(Boolean.TRUE)); + assertThat(hasRotatedSecretAttributes(rep), is(Boolean.TRUE)); assertThat(wrapper.getClientRotatedSecret(null), equalTo(firstSecret)); } @@ -254,9 +255,7 @@ public void regenerateSecretAfterCurrentSecretExpires() throws Exception { String firstSecret = clientResource.getSecret().getValue(); String secondSecret = clientResource.generateNewSecret().getValue(); assertThat(secondSecret, not(equalTo(firstSecret))); - OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation( - clientResource.toRepresentation()); - assertThat(wrapper.hasRotatedSecret(), is(Boolean.FALSE)); + assertThat(hasRotatedSecretAttributes(clientResource.toRepresentation()), is(Boolean.FALSE)); //apply policy configureDefaultProfileAndPolicy(); @@ -266,9 +265,9 @@ public void regenerateSecretAfterCurrentSecretExpires() throws Exception { String newSecret = clientResource.generateNewSecret().getValue(); assertThat(newSecret, not(equalTo(secondSecret))); - wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation( - clientResource.toRepresentation()); - assertThat(wrapper.hasRotatedSecret(), is(Boolean.TRUE)); + ClientRepresentation rep = clientResource.toRepresentation(); + OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation(rep); + assertThat(hasRotatedSecretAttributes(rep), is(Boolean.TRUE)); assertThat(wrapper.getClientRotatedSecret(null), equalTo(secondSecret)); long rotatedCreationTime = wrapper.getClientSecretCreationTime(); assertThat(rotatedCreationTime, is(notNullValue())); @@ -308,15 +307,12 @@ public void updateClientPolicyEnabledSecretExpired() throws Exception { clientRepresentation.setDescription("Force second Updated"); clientResource.update(clientRepresentation); - wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation( - clientResource.toRepresentation()); - assertThat(clientResource.getSecret().getValue(), not(equalTo(firstSecret))); - wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation( - clientResource.toRepresentation()); + ClientRepresentation updatedRep = clientResource.toRepresentation(); + wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation(updatedRep); assertThat(wrapper.getClientSecretCreationTime(), not(equalTo(secretCreationTime))); - assertThat(wrapper.hasRotatedSecret(), is(Boolean.TRUE)); + assertThat(hasRotatedSecretAttributes(updatedRep), is(Boolean.TRUE)); assertThat(wrapper.getClientRotatedSecret(null), equalTo(firstSecret)); } @@ -526,9 +522,7 @@ public void authenticateWithRotatedSecretWithZeroExpirationTime() throws Excepti //confirms rotation assertThat(updatedSecret, not(equalTo(firstSecret))); - OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation( - clientResource.toRepresentation()); - assertThat(wrapper.hasRotatedSecret(), is(Boolean.FALSE)); + assertThat(hasRotatedSecretAttributes(clientResource.toRepresentation()), is(Boolean.FALSE)); oauth.client(clientId, firstSecret); @@ -703,7 +697,7 @@ public void removingPolicyMustClearRotationInformationFromClientOnUpdate() throw clientRepresentation = clientResource.toRepresentation(); OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation(clientRepresentation); - assertThat(wrapper.hasRotatedSecret(), is(false)); + assertThat(hasRotatedSecretAttributes(clientRepresentation), is(false)); assertThat(wrapper.getClientSecretExpirationTime(),is(0L)); } @@ -733,16 +727,22 @@ public void removingPolicyMustClearRotationInformationFromClientOnRequestNewSecr //client must not have any information about rotation in it ClientRepresentation clientRepresentation = clientResource.toRepresentation(); - OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation(clientRepresentation); assertThat(clientResource.getSecret().getValue(),equalTo(newSecret)); - assertThat(wrapper.hasRotatedSecret(), is(false)); - assertThat(wrapper.getClientSecretExpirationTime(),is(0L)); + assertThat(hasRotatedSecretAttributes(clientRepresentation), is(false)); + assertThat(OIDCClientSecretConfigWrapper.fromClientRepresentation(clientRepresentation).getClientSecretExpirationTime(),is(0L)); } /** * -------------------- support methods -------------------- **/ + private static boolean hasRotatedSecretAttributes(ClientRepresentation rep) { + Map attrs = rep.getAttributes(); + return attrs != null + && StringUtil.isNotBlank(attrs.get(ClientSecretConstants.CLIENT_ROTATED_SECRET)) + && StringUtil.isNotBlank(attrs.get(ClientSecretConstants.CLIENT_ROTATED_SECRET_CREATION_TIME)); + } + private void configureCustomProfileAndPolicy(int secretExpiration, int rotatedExpiration, int remainingExpiration) throws Exception { ClientProfileBuilder profileBuilder = new ClientProfileBuilder(); From 770b8ace59db3d7fb70d8bfe4eba8ca6e121ff18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Barto=C5=A1?= Date: Mon, 20 Jul 2026 11:08:34 +0200 Subject: [PATCH 2/2] Simplify tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ricardo Martin Signed-off-by: Martin Bartoš --- .../client/ClientSecretRotationTest.java | 52 +++++++++++-------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientSecretRotationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientSecretRotationTest.java index 0d55422b3dc9..d4e47eed9a07 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientSecretRotationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientSecretRotationTest.java @@ -19,7 +19,9 @@ import org.keycloak.admin.client.resource.ClientPoliciesPoliciesResource; import org.keycloak.admin.client.resource.ClientResource; import org.keycloak.authentication.authenticators.client.ClientIdAndSecretAuthenticator; +import org.keycloak.common.Profile; import org.keycloak.common.Profile.Feature; +import org.keycloak.common.profile.CommaSeparatedListProfileConfigResolver; import org.keycloak.common.util.Time; import org.keycloak.events.Details; import org.keycloak.events.EventType; @@ -54,13 +56,13 @@ import org.keycloak.testsuite.util.oauth.AuthorizationEndpointResponse; import org.keycloak.util.JsonSerialization; import org.keycloak.util.TokenUtil; -import org.keycloak.utils.StringUtil; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import org.jboss.logging.Logger; import org.jetbrains.annotations.NotNull; import org.junit.After; +import org.junit.BeforeClass; import org.junit.Rule; import org.junit.Test; @@ -109,6 +111,11 @@ public class ClientSecretRotationTest extends AbstractRestServiceTest { private static final int DEFAULT_REMAIN_EXPIRATION_PERIOD = Long.valueOf( TimeUnit.MINUTES.toSeconds(30)).intValue(); + @BeforeClass + public static void beforeAll() { + Profile.configure(new CommaSeparatedListProfileConfigResolver(Feature.CLIENT_SECRET_ROTATION.getVersionedKey(), "")); + } + @Rule public AssertEvents events = new AssertEvents(this); @@ -232,11 +239,11 @@ public void regenerateSecretOnCurrentSecretNotExpired() throws Exception { .get(cidConfidential); String firstSecret = clientResource.getSecret().getValue(); String secondSecret = clientResource.generateNewSecret().getValue(); - ClientRepresentation rep = clientResource.toRepresentation(); - OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation(rep); + OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation( + clientResource.toRepresentation()); assertThat(secondSecret, not(equalTo(firstSecret))); - assertThat(hasRotatedSecretAttributes(rep), is(Boolean.TRUE)); + assertThat(wrapper.hasRotatedSecret(), is(Boolean.TRUE)); assertThat(wrapper.getClientRotatedSecret(null), equalTo(firstSecret)); } @@ -255,7 +262,9 @@ public void regenerateSecretAfterCurrentSecretExpires() throws Exception { String firstSecret = clientResource.getSecret().getValue(); String secondSecret = clientResource.generateNewSecret().getValue(); assertThat(secondSecret, not(equalTo(firstSecret))); - assertThat(hasRotatedSecretAttributes(clientResource.toRepresentation()), is(Boolean.FALSE)); + OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation( + clientResource.toRepresentation()); + assertThat(wrapper.hasRotatedSecret(), is(Boolean.FALSE)); //apply policy configureDefaultProfileAndPolicy(); @@ -265,9 +274,9 @@ public void regenerateSecretAfterCurrentSecretExpires() throws Exception { String newSecret = clientResource.generateNewSecret().getValue(); assertThat(newSecret, not(equalTo(secondSecret))); - ClientRepresentation rep = clientResource.toRepresentation(); - OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation(rep); - assertThat(hasRotatedSecretAttributes(rep), is(Boolean.TRUE)); + wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation( + clientResource.toRepresentation()); + assertThat(wrapper.hasRotatedSecret(), is(Boolean.TRUE)); assertThat(wrapper.getClientRotatedSecret(null), equalTo(secondSecret)); long rotatedCreationTime = wrapper.getClientSecretCreationTime(); assertThat(rotatedCreationTime, is(notNullValue())); @@ -307,12 +316,15 @@ public void updateClientPolicyEnabledSecretExpired() throws Exception { clientRepresentation.setDescription("Force second Updated"); clientResource.update(clientRepresentation); + wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation( + clientResource.toRepresentation()); + assertThat(clientResource.getSecret().getValue(), not(equalTo(firstSecret))); - ClientRepresentation updatedRep = clientResource.toRepresentation(); - wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation(updatedRep); + wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation( + clientResource.toRepresentation()); assertThat(wrapper.getClientSecretCreationTime(), not(equalTo(secretCreationTime))); - assertThat(hasRotatedSecretAttributes(updatedRep), is(Boolean.TRUE)); + assertThat(wrapper.hasRotatedSecret(), is(Boolean.TRUE)); assertThat(wrapper.getClientRotatedSecret(null), equalTo(firstSecret)); } @@ -522,7 +534,9 @@ public void authenticateWithRotatedSecretWithZeroExpirationTime() throws Excepti //confirms rotation assertThat(updatedSecret, not(equalTo(firstSecret))); - assertThat(hasRotatedSecretAttributes(clientResource.toRepresentation()), is(Boolean.FALSE)); + OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation( + clientResource.toRepresentation()); + assertThat(wrapper.hasRotatedSecret(), is(Boolean.FALSE)); oauth.client(clientId, firstSecret); @@ -697,7 +711,7 @@ public void removingPolicyMustClearRotationInformationFromClientOnUpdate() throw clientRepresentation = clientResource.toRepresentation(); OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation(clientRepresentation); - assertThat(hasRotatedSecretAttributes(clientRepresentation), is(false)); + assertThat(wrapper.hasRotatedSecret(), is(false)); assertThat(wrapper.getClientSecretExpirationTime(),is(0L)); } @@ -727,22 +741,16 @@ public void removingPolicyMustClearRotationInformationFromClientOnRequestNewSecr //client must not have any information about rotation in it ClientRepresentation clientRepresentation = clientResource.toRepresentation(); + OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation(clientRepresentation); assertThat(clientResource.getSecret().getValue(),equalTo(newSecret)); - assertThat(hasRotatedSecretAttributes(clientRepresentation), is(false)); - assertThat(OIDCClientSecretConfigWrapper.fromClientRepresentation(clientRepresentation).getClientSecretExpirationTime(),is(0L)); + assertThat(wrapper.hasRotatedSecret(), is(false)); + assertThat(wrapper.getClientSecretExpirationTime(),is(0L)); } /** * -------------------- support methods -------------------- **/ - private static boolean hasRotatedSecretAttributes(ClientRepresentation rep) { - Map attrs = rep.getAttributes(); - return attrs != null - && StringUtil.isNotBlank(attrs.get(ClientSecretConstants.CLIENT_ROTATED_SECRET)) - && StringUtil.isNotBlank(attrs.get(ClientSecretConstants.CLIENT_ROTATED_SECRET_CREATION_TIME)); - } - private void configureCustomProfileAndPolicy(int secretExpiration, int rotatedExpiration, int remainingExpiration) throws Exception { ClientProfileBuilder profileBuilder = new ClientProfileBuilder();