diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureSigningAlgorithmEnforceExecutor.java b/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureSigningAlgorithmEnforceExecutor.java index 996a5d29f5..6b809461a3 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureSigningAlgorithmEnforceExecutor.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureSigningAlgorithmEnforceExecutor.java @@ -18,7 +18,10 @@ package org.keycloak.services.clientpolicy.executor; import java.util.Arrays; +import java.util.HashMap; import java.util.List; +import java.util.Map; +import java.util.Optional; import org.jboss.logging.Logger; @@ -34,14 +37,18 @@ import org.keycloak.services.clientpolicy.context.AdminClientUpdateContext; import org.keycloak.services.clientpolicy.context.DynamicClientRegisterContext; import org.keycloak.services.clientpolicy.context.DynamicClientUpdateContext; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; + /** * @author Takashi Norimatsu */ -public class SecureSigningAlgorithmEnforceExecutor implements ClientPolicyExecutorProvider { +public class SecureSigningAlgorithmEnforceExecutor implements ClientPolicyExecutorProvider { private static final Logger logger = Logger.getLogger(SecureSigningAlgorithmEnforceExecutor.class); private final KeycloakSession session; + private Configuration configuration; private static final List sigTargets = Arrays.asList( OIDCConfigAttributes.USER_INFO_RESPONSE_SIGNATURE_ALG, @@ -52,6 +59,8 @@ public class SecureSigningAlgorithmEnforceExecutor implements ClientPolicyExecut private static final List sigTargetsAdminRestApiOnly = Arrays.asList( OIDCConfigAttributes.ACCESS_TOKEN_SIGNED_RESPONSE_ALG); + private static final String DEFAULT_ALGORITHM_VALUE = Algorithm.PS256; + public SecureSigningAlgorithmEnforceExecutor(KeycloakSession session) { this.session = session; } @@ -61,23 +70,53 @@ public class SecureSigningAlgorithmEnforceExecutor implements ClientPolicyExecut return SecureSigningAlgorithmEnforceExecutorFactory.PROVIDER_ID; } + @Override + public void setupConfiguration(SecureSigningAlgorithmEnforceExecutor.Configuration config) { + this.configuration = Optional.ofNullable(config).orElse(createDefaultConfiguration()); + if (config.getDefaultAlgorithm() == null || !isSecureAlgorithm(config.getDefaultAlgorithm())) config.setDefaultAlgorithm(DEFAULT_ALGORITHM_VALUE); + } + + @Override + public Class getExecutorConfigurationClass() { + return Configuration.class; + } + + @JsonIgnoreProperties(ignoreUnknown = true) + public static class Configuration extends ClientPolicyExecutorConfiguration { + @JsonProperty("default-algorithm") + protected String defaultAlgorithm; + + public String getDefaultAlgorithm() { + return defaultAlgorithm; + } + + public void setDefaultAlgorithm(String defaultAlgorithm) { + if (isSecureAlgorithm(defaultAlgorithm)) { + this.defaultAlgorithm = defaultAlgorithm; + } else { + logger.tracev("defaultAlgorithm = {0}, fall back to {1}.", defaultAlgorithm, DEFAULT_ALGORITHM_VALUE); + this.defaultAlgorithm = DEFAULT_ALGORITHM_VALUE; + } + } + } + @Override public void executeOnEvent(ClientPolicyContext context) throws ClientPolicyException { switch (context.getEvent()) { case REGISTER: if (context instanceof AdminClientRegisterContext) { - verifySecureSigningAlgorithm(((AdminClientRegisterContext)context).getProposedClientRepresentation(), true, false); + verifyAndEnforceSecureSigningAlgorithm(((AdminClientRegisterContext)context).getProposedClientRepresentation(), true, false); } else if (context instanceof DynamicClientRegisterContext) { - verifySecureSigningAlgorithm(((DynamicClientRegisterContext)context).getProposedClientRepresentation(), false, false); + verifyAndEnforceSecureSigningAlgorithm(((DynamicClientRegisterContext)context).getProposedClientRepresentation(), false, false); } else { throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "not allowed input format."); } break; case UPDATE: if (context instanceof AdminClientUpdateContext) { - verifySecureSigningAlgorithm(((AdminClientUpdateContext)context).getProposedClientRepresentation(), true, true); + verifyAndEnforceSecureSigningAlgorithm(((AdminClientUpdateContext)context).getProposedClientRepresentation(), true, true); } else if (context instanceof DynamicClientUpdateContext) { - verifySecureSigningAlgorithm(((DynamicClientUpdateContext)context).getProposedClientRepresentation(), false, true); + verifyAndEnforceSecureSigningAlgorithm(((DynamicClientUpdateContext)context).getProposedClientRepresentation(), false, true); } else { throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "not allowed input format."); } @@ -87,28 +126,45 @@ public class SecureSigningAlgorithmEnforceExecutor implements ClientPolicyExecut } } - private void verifySecureSigningAlgorithm(ClientRepresentation clientRep, boolean byAdminRestApi, boolean isUpdate) throws ClientPolicyException { - if (clientRep.getAttributes() == null) { - throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "no signature algorithm was specified."); - } + private Configuration createDefaultConfiguration() { + Configuration conf = new Configuration(); + conf.setDefaultAlgorithm(DEFAULT_ALGORITHM_VALUE); + return conf; + } + private void verifyAndEnforceSecureSigningAlgorithm(ClientRepresentation clientRep, boolean byAdminRestApi, boolean isUpdate) throws ClientPolicyException { for (String sigTarget : sigTargets) { - verifySecureSigningAlgorithm(sigTarget, clientRep.getAttributes().get(sigTarget)); + verifyAndEnforceSecureSigningAlgorithm(sigTarget, clientRep); } // no client metadata found in RFC 7591 OAuth Dynamic Client Registration Metadata if (byAdminRestApi) { for (String sigTarget : sigTargetsAdminRestApiOnly) { - verifySecureSigningAlgorithm(sigTarget, clientRep.getAttributes().get(sigTarget)); + verifyAndEnforceSecureSigningAlgorithm(sigTarget, clientRep); } } } - private void verifySecureSigningAlgorithm(String sigTarget, String sigAlg) throws ClientPolicyException { + private void verifyAndEnforceSecureSigningAlgorithm(String sigTarget, ClientRepresentation clientRep) throws ClientPolicyException { + Map attributes = Optional.ofNullable(clientRep.getAttributes()).orElse(new HashMap<>()); + String sigAlg = attributes.get(sigTarget); if (sigAlg == null) { - logger.tracev("Signing algorithm not specified explicitly. signature target = {0}", sigTarget); + logger.tracev("Signing algorithm not specified explicitly, signature target = {0}. set default algorithm = {1}.", sigTarget, configuration.getDefaultAlgorithm()); + attributes.put(sigTarget, configuration.getDefaultAlgorithm()); + clientRep.setAttributes(attributes); return; } + + if (isSecureAlgorithm(sigAlg)) { + logger.tracev("Passed. signature target = {0}, signature algorithm = {1}", sigTarget, sigAlg); + return; + } + + logger.tracev("NOT allowed signatureAlgorithm. signature target = {0}, signature algorithm = {1}", sigTarget, sigAlg); + throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "not allowed signature algorithm."); + } + + private static boolean isSecureAlgorithm(String sigAlg) { switch (sigAlg) { case Algorithm.PS256: case Algorithm.PS384: @@ -116,11 +172,9 @@ public class SecureSigningAlgorithmEnforceExecutor implements ClientPolicyExecut case Algorithm.ES256: case Algorithm.ES384: case Algorithm.ES512: - logger.tracev("Passed. signature target = {0}, signature algorithm = {1}", sigTarget, sigAlg); - return; + return true; } - logger.tracev("NOT allowed signatureAlgorithm. signature target = {0}, signature algorithm = {1}", sigTarget, sigAlg); - throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "not allowed signature algorithm."); + return false; } } diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureSigningAlgorithmEnforceExecutorFactory.java b/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureSigningAlgorithmEnforceExecutorFactory.java index 0e1eb62d3d..f8b6e15a2a 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureSigningAlgorithmEnforceExecutorFactory.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureSigningAlgorithmEnforceExecutorFactory.java @@ -17,10 +17,12 @@ package org.keycloak.services.clientpolicy.executor; -import java.util.Collections; +import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import org.keycloak.Config.Scope; +import org.keycloak.crypto.Algorithm; import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSessionFactory; import org.keycloak.provider.ProviderConfigProperty; @@ -32,6 +34,11 @@ public class SecureSigningAlgorithmEnforceExecutorFactory implements ClientPolic public static final String PROVIDER_ID = "securesignalg-enforce-executor"; + public static final String DEFAULT_ALGORITHM = "default-algorithm"; + + private static final ProviderConfigProperty DEFAULT_ALGORITHM_PROPERTY = new ProviderConfigProperty( + DEFAULT_ALGORITHM, null, null, ProviderConfigProperty.STRING_TYPE, Algorithm.PS256); + @Override public ClientPolicyExecutorProvider create(KeycloakSession session) { return new SecureSigningAlgorithmEnforceExecutor(session); @@ -61,7 +68,7 @@ public class SecureSigningAlgorithmEnforceExecutorFactory implements ClientPolic @Override public List getConfigProperties() { - return Collections.emptyList(); + return new ArrayList<>(Arrays.asList(DEFAULT_ALGORITHM_PROPERTY)); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AbstractClientPoliciesTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AbstractClientPoliciesTest.java index 4e64c636f9..a7d9624e33 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AbstractClientPoliciesTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AbstractClientPoliciesTest.java @@ -127,6 +127,7 @@ import org.keycloak.services.clientpolicy.executor.SecureRequestObjectExecutor; import org.keycloak.services.clientpolicy.executor.SecureRequestObjectExecutorFactory; import org.keycloak.services.clientpolicy.executor.SecureResponseTypeExecutorFactory; import org.keycloak.services.clientpolicy.executor.SecureSessionEnforceExecutorFactory; +import org.keycloak.services.clientpolicy.executor.SecureSigningAlgorithmEnforceExecutor; import org.keycloak.services.clientpolicy.executor.SecureSigningAlgorithmEnforceExecutorFactory; import org.keycloak.services.clientpolicy.executor.SecureSigningAlgorithmForSignedJwtEnforceExecutor; import org.keycloak.services.clientpolicy.executor.SecureSigningAlgorithmForSignedJwtEnforceExecutorFactory; @@ -685,6 +686,13 @@ public abstract class AbstractClientPoliciesTest extends AbstractKeycloakTest { // Registration/Initial Access Token acquisition for Dynamic Client Registration + protected void restartAuthenticatedClientRegistrationSetting() throws ClientRegistrationException { + reg.close(); + reg = ClientRegistration.create().url(suiteContext.getAuthServerInfo().getContextRoot() + "/auth", REALM_NAME).build(); + ClientInitialAccessPresentation token = adminClient.realm(REALM_NAME).clientInitialAccess().create(new ClientInitialAccessCreatePresentation(0, 10)); + reg.auth(Auth.token(token)); + } + protected void authCreateClients() { reg.auth(Auth.token(getToken("create-clients", "password"))); } @@ -873,6 +881,12 @@ public abstract class AbstractClientPoliciesTest extends AbstractKeycloakTest { return config; } + protected Object createSecureSigningAlgorithmEnforceExecutorConfig(String defaultAlgorithm) { + SecureSigningAlgorithmEnforceExecutor.Configuration config = new SecureSigningAlgorithmEnforceExecutor.Configuration(); + config.setDefaultAlgorithm(defaultAlgorithm); + return config; + } + // Client Policies CRUD Operation protected static class ClientPoliciesBuilder { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java index 1153ed90fb..31e63c4a3d 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java @@ -50,6 +50,8 @@ import org.keycloak.authentication.authenticators.client.ClientIdAndSecretAuthen import org.keycloak.authentication.authenticators.client.JWTClientAuthenticator; import org.keycloak.authentication.authenticators.client.JWTClientSecretAuthenticator; import org.keycloak.authentication.authenticators.client.X509ClientAuthenticator; +import org.keycloak.client.registration.Auth; +import org.keycloak.client.registration.ClientRegistration; import org.keycloak.client.registration.ClientRegistrationException; import org.keycloak.common.Profile; import org.keycloak.events.Details; @@ -65,6 +67,8 @@ import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.protocol.oidc.utils.OIDCResponseType; import org.keycloak.representations.AccessToken; import org.keycloak.representations.RefreshToken; +import org.keycloak.representations.idm.ClientInitialAccessCreatePresentation; +import org.keycloak.representations.idm.ClientInitialAccessPresentation; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.CredentialRepresentation; import org.keycloak.representations.idm.EventRepresentation; @@ -1118,7 +1122,6 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { ).toString(); updatePolicies(json); - // create by Admin REST API - fail try { createClientByAdmin(generateSuffixedName("App-by-Admin"), (ClientRepresentation clientRep) -> { @@ -1141,6 +1144,16 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { clientRep.getAttributes().put(OIDCConfigAttributes.ACCESS_TOKEN_SIGNED_RESPONSE_ALG, org.keycloak.crypto.Algorithm.ES256); }); + // create by Admin REST API - success, PS256 enforced + String cAppAdmin2Id = createClientByAdmin(generateSuffixedName("App-by-Admin2"), (ClientRepresentation client2Rep) -> { + }); + ClientRepresentation cRep2 = getClientByAdmin(cAppAdmin2Id); + assertEquals(org.keycloak.crypto.Algorithm.PS256, cRep2.getAttributes().get(OIDCConfigAttributes.USER_INFO_RESPONSE_SIGNATURE_ALG)); + assertEquals(org.keycloak.crypto.Algorithm.PS256, cRep2.getAttributes().get(OIDCConfigAttributes.REQUEST_OBJECT_SIGNATURE_ALG)); + assertEquals(org.keycloak.crypto.Algorithm.PS256, cRep2.getAttributes().get(OIDCConfigAttributes.ID_TOKEN_SIGNED_RESPONSE_ALG)); + assertEquals(org.keycloak.crypto.Algorithm.PS256, cRep2.getAttributes().get(OIDCConfigAttributes.TOKEN_ENDPOINT_AUTH_SIGNING_ALG)); + assertEquals(org.keycloak.crypto.Algorithm.PS256, cRep2.getAttributes().get(OIDCConfigAttributes.ACCESS_TOKEN_SIGNED_RESPONSE_ALG)); + // update by Admin REST API - fail try { updateClientByAdmin(cAppAdminId, (ClientRepresentation clientRep) -> { @@ -1161,6 +1174,39 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { cRep = getClientByAdmin(cAppAdminId); assertEquals(org.keycloak.crypto.Algorithm.PS384, cRep.getAttributes().get(OIDCConfigAttributes.ACCESS_TOKEN_SIGNED_RESPONSE_ALG)); + // update profiles, ES256 enforced + json = (new ClientProfilesBuilder()).addProfile( + (new ClientProfileBuilder()).createProfile(PROFILE_NAME, "Den Forsta Profilen", Boolean.FALSE, null) + .addExecutor(SecureSigningAlgorithmEnforceExecutorFactory.PROVIDER_ID, + createSecureSigningAlgorithmEnforceExecutorConfig(org.keycloak.crypto.Algorithm.ES256)) + .toRepresentation() + ).toString(); + updateProfiles(json); + + // update by Admin REST API - success + updateClientByAdmin(cAppAdmin2Id, (ClientRepresentation client2Rep) -> { + client2Rep.getAttributes().remove(OIDCConfigAttributes.USER_INFO_RESPONSE_SIGNATURE_ALG); + client2Rep.getAttributes().remove(OIDCConfigAttributes.REQUEST_OBJECT_SIGNATURE_ALG); + client2Rep.getAttributes().remove(OIDCConfigAttributes.ID_TOKEN_SIGNED_RESPONSE_ALG); + client2Rep.getAttributes().remove(OIDCConfigAttributes.TOKEN_ENDPOINT_AUTH_SIGNING_ALG); + client2Rep.getAttributes().remove(OIDCConfigAttributes.ACCESS_TOKEN_SIGNED_RESPONSE_ALG); + }); + cRep2 = getClientByAdmin(cAppAdmin2Id); + assertEquals(org.keycloak.crypto.Algorithm.ES256, cRep2.getAttributes().get(OIDCConfigAttributes.USER_INFO_RESPONSE_SIGNATURE_ALG)); + assertEquals(org.keycloak.crypto.Algorithm.ES256, cRep2.getAttributes().get(OIDCConfigAttributes.REQUEST_OBJECT_SIGNATURE_ALG)); + assertEquals(org.keycloak.crypto.Algorithm.ES256, cRep2.getAttributes().get(OIDCConfigAttributes.ID_TOKEN_SIGNED_RESPONSE_ALG)); + assertEquals(org.keycloak.crypto.Algorithm.ES256, cRep2.getAttributes().get(OIDCConfigAttributes.TOKEN_ENDPOINT_AUTH_SIGNING_ALG)); + assertEquals(org.keycloak.crypto.Algorithm.ES256, cRep2.getAttributes().get(OIDCConfigAttributes.ACCESS_TOKEN_SIGNED_RESPONSE_ALG)); + + // update profiles, fall back to PS256 + json = (new ClientProfilesBuilder()).addProfile( + (new ClientProfileBuilder()).createProfile(PROFILE_NAME, "Den Forsta Profilen", Boolean.FALSE, null) + .addExecutor(SecureSigningAlgorithmEnforceExecutorFactory.PROVIDER_ID, + createSecureSigningAlgorithmEnforceExecutorConfig(org.keycloak.crypto.Algorithm.RS512)) + .toRepresentation() + ).toString(); + updateProfiles(json); + // create dynamically - fail try { createClientByAdmin(generateSuffixedName("App-in-Dynamic"), (ClientRepresentation clientRep) -> { @@ -1181,7 +1227,6 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { clientRep.setTokenEndpointAuthSigningAlg(org.keycloak.crypto.Algorithm.PS256); }); events.expect(EventType.CLIENT_REGISTER).client(cAppDynamicClientId).user(Matchers.isEmptyOrNullString()).assertEvent(); - getClientDynamically(cAppDynamicClientId); // update dynamically - fail try { @@ -1199,6 +1244,38 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { clientRep.setIdTokenSignedResponseAlg(org.keycloak.crypto.Algorithm.ES384); }); assertEquals(org.keycloak.crypto.Algorithm.ES384, getClientDynamically(cAppDynamicClientId).getIdTokenSignedResponseAlg()); + + // create dynamically - success, PS256 enforced + restartAuthenticatedClientRegistrationSetting(); + String cAppDynamicClient2Id = createClientDynamically(generateSuffixedName("App-in-Dynamic"), (OIDCClientRepresentation client2Rep) -> { + }); + OIDCClientRepresentation cAppDynamicClient2Rep = getClientDynamically(cAppDynamicClient2Id); + assertEquals(org.keycloak.crypto.Algorithm.PS256, cAppDynamicClient2Rep.getUserinfoSignedResponseAlg()); + assertEquals(org.keycloak.crypto.Algorithm.PS256, cAppDynamicClient2Rep.getRequestObjectSigningAlg()); + assertEquals(org.keycloak.crypto.Algorithm.PS256, cAppDynamicClient2Rep.getIdTokenSignedResponseAlg()); + assertEquals(org.keycloak.crypto.Algorithm.PS256, cAppDynamicClient2Rep.getTokenEndpointAuthSigningAlg()); + + // update profiles, enforce ES256 + json = (new ClientProfilesBuilder()).addProfile( + (new ClientProfileBuilder()).createProfile(PROFILE_NAME, "Den Forsta Profilen", Boolean.FALSE, null) + .addExecutor(SecureSigningAlgorithmEnforceExecutorFactory.PROVIDER_ID, + createSecureSigningAlgorithmEnforceExecutorConfig(org.keycloak.crypto.Algorithm.ES256)) + .toRepresentation() + ).toString(); + updateProfiles(json); + + // update dynamically - success, ES256 enforced + updateClientDynamically(cAppDynamicClient2Id, (OIDCClientRepresentation client2Rep) -> { + client2Rep.setUserinfoSignedResponseAlg(null); + client2Rep.setRequestObjectSigningAlg(null); + client2Rep.setIdTokenSignedResponseAlg(null); + client2Rep.setTokenEndpointAuthSigningAlg(null); + }); + cAppDynamicClient2Rep = getClientDynamically(cAppDynamicClient2Id); + assertEquals(org.keycloak.crypto.Algorithm.ES256, cAppDynamicClient2Rep.getUserinfoSignedResponseAlg()); + assertEquals(org.keycloak.crypto.Algorithm.ES256, cAppDynamicClient2Rep.getRequestObjectSigningAlg()); + assertEquals(org.keycloak.crypto.Algorithm.ES256, cAppDynamicClient2Rep.getIdTokenSignedResponseAlg()); + assertEquals(org.keycloak.crypto.Algorithm.ES256, cAppDynamicClient2Rep.getTokenEndpointAuthSigningAlg()); } @Test