From 0c25da542c16e85fb0ca6950131571dbd3c027c3 Mon Sep 17 00:00:00 2001 From: Marcelo Daniel Silva Sales Date: Thu, 10 Mar 2022 13:03:09 +0100 Subject: [PATCH] Update secret rotation when the policy is disabled (#10674) Closes #10667 --- .../oidc/OIDCClientSecretConfigWrapper.java | 7 ++ .../ClientSecretRotationExecutor.java | 3 +- .../resources/admin/ClientResource.java | 17 +++- .../client/ClientSecretRotationTest.java | 81 ++++++++++++++++++- 4 files changed, 103 insertions(+), 5 deletions(-) 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 03b7fc253e..b24f413200 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/OIDCClientSecretConfigWrapper.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/OIDCClientSecretConfigWrapper.java @@ -18,6 +18,7 @@ import static org.keycloak.models.ClientSecretConstants.CLIENT_ROTATED_SECRET_CR import static org.keycloak.models.ClientSecretConstants.CLIENT_ROTATED_SECRET_EXPIRATION_TIME; import static org.keycloak.models.ClientSecretConstants.CLIENT_SECRET_CREATION_TIME; import static org.keycloak.models.ClientSecretConstants.CLIENT_SECRET_EXPIRATION; +import static org.keycloak.models.ClientSecretConstants.CLIENT_SECRET_REMAINING_EXPIRATION_TIME; /** * @author Marcelo Sales @@ -61,6 +62,12 @@ public class OIDCClientSecretConfigWrapper extends AbstractClientConfigWrapper { } } + public void removeClientSecretRotationInfo() { + setAttribute(CLIENT_SECRET_EXPIRATION, null); + setAttribute(CLIENT_SECRET_REMAINING_EXPIRATION_TIME, null); + removeClientSecretRotated(); + } + public void removeClientSecretRotated() { if (hasRotatedSecret()) { setAttribute(CLIENT_ROTATED_SECRET, null); diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/executor/ClientSecretRotationExecutor.java b/services/src/main/java/org/keycloak/services/clientpolicy/executor/ClientSecretRotationExecutor.java index c83c0e5416..e675843669 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/executor/ClientSecretRotationExecutor.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/executor/ClientSecretRotationExecutor.java @@ -6,6 +6,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import org.jboss.logging.Logger; import org.keycloak.common.util.Time; import org.keycloak.models.ClientModel; +import org.keycloak.models.ClientSecretConstants; import org.keycloak.models.KeycloakSession; import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.protocol.oidc.OIDCClientSecretConfigWrapper; @@ -48,7 +49,7 @@ public class ClientSecretRotationExecutor implements public void executeOnEvent(ClientPolicyContext context) throws ClientPolicyException { if (!session.getContext().getClient().isPublicClient() && !session.getContext().getClient() .isBearerOnly()) { - + session.setAttribute(ClientSecretConstants.CLIENT_SECRET_ROTATION_ENABLED,Boolean.TRUE); switch (context.getEvent()) { case REGISTERED: case UPDATED: 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 326be4f8f3..5047ebd4bc 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 @@ -32,6 +32,7 @@ import org.keycloak.events.admin.ResourceType; import org.keycloak.models.AuthenticatedClientSessionModel; import org.keycloak.models.ClientModel; import org.keycloak.models.ClientScopeModel; +import org.keycloak.models.ClientSecretConstants; import org.keycloak.models.Constants; import org.keycloak.models.KeycloakSession; import org.keycloak.models.ModelDuplicateException; @@ -141,6 +142,7 @@ public class ClientResource { auth.clients().requireConfigure(client); try { + session.setAttribute(ClientSecretConstants.CLIENT_SECRET_ROTATION_ENABLED,Boolean.FALSE); session.clientPolicy().triggerOnEvent(new AdminClientUpdateContext(rep, client, auth.adminAuth())); updateClientFromRep(rep, client, session); @@ -155,6 +157,12 @@ public class ClientResource { session.clientPolicy().triggerOnEvent(new AdminClientUpdatedContext(rep, client, auth.adminAuth())); + if (!(boolean) session.getAttribute(ClientSecretConstants.CLIENT_SECRET_ROTATION_ENABLED)){ + logger.debugv("Removing the previous rotation info for client {0}{1}, if there is",client.getClientId(),client.getName()); + OIDCClientSecretConfigWrapper.fromClientModel(client).removeClientSecretRotationInfo(); + } + session.removeAttribute(ClientSecretConstants.CLIENT_SECRET_ROTATION_ENABLED); + adminEvent.operation(OperationType.UPDATE).resourcePath(session.getContext().getUri()).representation(rep).success(); return Response.noContent().build(); } catch (ModelDuplicateException e) { @@ -253,6 +261,7 @@ public class ClientResource { auth.clients().requireConfigure(client); logger.debug("regenerateSecret"); + session.setAttribute(ClientSecretConstants.CLIENT_SECRET_ROTATION_ENABLED,Boolean.FALSE); ClientRepresentation representation = ModelToRepresentation.toRepresentation(client, session); ClientSecretRotationContext secretRotationContext = new ClientSecretRotationContext( @@ -265,10 +274,14 @@ public class ClientResource { CredentialRepresentation rep = new CredentialRepresentation(); rep.setType(CredentialRepresentation.SECRET); rep.setValue(secret); - rep.setCreatedDate( - (long) OIDCClientSecretConfigWrapper.fromClientModel(client).getClientSecretCreationTime()); + + if (!(boolean) session.getAttribute(ClientSecretConstants.CLIENT_SECRET_ROTATION_ENABLED)){ + logger.debugv("Removing the previous rotation info for client {0}{1}, if there is",client.getClientId(),client.getName()); + OIDCClientSecretConfigWrapper.fromClientModel(client).removeClientSecretRotationInfo(); + } adminEvent.operation(OperationType.ACTION).resourcePath(session.getContext().getUri()).representation(rep).success(); + session.removeAttribute(ClientSecretConstants.CLIENT_SECRET_ROTATION_ENABLED); return rep; } catch (ClientPolicyException cpe) { 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 6ae0fc031c..7de81ca4de 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 @@ -615,12 +615,78 @@ public class ClientSecretRotationTest extends AbstractRestServiceTest { try { configureCustomProfileAndPolicy(60, 61, 30); } catch (Exception e) { - assertThat(e,instanceOf(ClientPolicyException.class)); + assertThat(e, instanceOf(ClientPolicyException.class)); } // no police must have been created due to the above error ClientPoliciesPoliciesResource policiesResource = adminClient.realm(REALM_NAME).clientPoliciesPoliciesResource(); ClientPoliciesRepresentation policies = policiesResource.getPolicies(); - assertThat(policies.getPolicies(),is(empty())); + assertThat(policies.getPolicies(), is(empty())); + } + + /** + * When there is a client that has a secret rotated and the policy is disabled, Rotation information must be removed after updating a client + * + * @throws Exception + */ + @Test + public void removingPolicyMustClearRotationInformationFromClientOnUpdate() throws Exception { + //create and enable the profile + configureDefaultProfileAndPolicy(); + //create client + String cidConfidential = createClientByAdmin(DEFAULT_CLIENT_ID); + ClientResource clientResource = adminClient.realm(REALM_NAME).clients().get(cidConfidential); + String firstSecret = clientResource.getSecret().getValue(); + String newSecret = clientResource.generateNewSecret().getValue(); + String rotatedSecret = clientResource.getClientRotatedSecret().getValue(); + assertThat(firstSecret, not(equalTo(newSecret))); + assertThat(firstSecret, equalTo(rotatedSecret)); + + //disable the profile + disableProfile(); + + //force a update + ClientRepresentation clientRepresentation = clientResource.toRepresentation(); + clientRepresentation.setDescription("New Description Updated"); + clientResource.update(clientRepresentation); + + //client must not have any information about rotation in it + clientRepresentation = clientResource.toRepresentation(); + OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientRepresentation(clientRepresentation); + + assertThat(wrapper.hasRotatedSecret(), is(false)); + assertThat(wrapper.getClientSecretExpirationTime(),is(0)); + } + + /** + * When there is a client that has a secret rotated and the policy is disabled, Rotation information must be removed after request a new secret + * + * @throws Exception + */ + @Test + public void removingPolicyMustClearRotationInformationFromClientOnRequestNewSecret() throws Exception { + //create and enable the profile + configureDefaultProfileAndPolicy(); + //create client + String cidConfidential = createClientByAdmin(DEFAULT_CLIENT_ID); + ClientResource clientResource = adminClient.realm(REALM_NAME).clients().get(cidConfidential); + String firstSecret = clientResource.getSecret().getValue(); + String newSecret = clientResource.generateNewSecret().getValue(); + String rotatedSecret = clientResource.getClientRotatedSecret().getValue(); + assertThat(firstSecret, not(equalTo(newSecret))); + assertThat(firstSecret, equalTo(rotatedSecret)); + + //disable the profile + disableProfile(); + + //Request a new secret + newSecret = clientResource.generateNewSecret().getValue(); + + //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(0)); } /** @@ -646,6 +712,17 @@ public class ClientSecretRotationTest extends AbstractRestServiceTest { doConfigProfileAndPolicy(profileBuilder, profileConfig); } + private void disableProfile() throws Exception { + Configuration config = new Configuration(); + config.setType(Arrays.asList(ClientAccessTypeConditionFactory.TYPE_CONFIDENTIAL)); + String json = (new ClientPoliciesBuilder()).addPolicy( + (new ClientPolicyBuilder()).createPolicy(POLICY_NAME, + "Policy for Client Secret Rotation", + Boolean.FALSE).addCondition(ClientAccessTypeConditionFactory.PROVIDER_ID, config) + .addProfile(PROFILE_NAME).toRepresentation()).toString(); + updatePolicies(json); + } + private void doConfigProfileAndPolicy(ClientProfileBuilder profileBuilder, ClientSecretRotationExecutor.Configuration profileConfig) throws Exception { String json = (new ClientProfilesBuilder()).addProfile(