From 21c7af1c539cd52b66470a45bca1e609c8829571 Mon Sep 17 00:00:00 2001 From: Takashi Norimatsu Date: Thu, 12 Nov 2020 21:22:47 +0900 Subject: [PATCH] KEYCLOAK-14207 Client Policy - Executor : Enforce more secure client signature algorithm when client registration --- ...SecureSigningAlgorithmEnforceExecutor.java | 135 ++++++++++++++++++ ...igningAlgorithmEnforceExecutorFactory.java | 65 +++++++++ ...ecutor.ClientPolicyExecutorProviderFactory | 3 +- .../client/ClientPolicyBasicsTest.java | 118 +++++++++++++++ 4 files changed, 320 insertions(+), 1 deletion(-) create mode 100644 services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureSigningAlgorithmEnforceExecutor.java create mode 100644 services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureSigningAlgorithmEnforceExecutorFactory.java 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 new file mode 100644 index 0000000000..cc9701843e --- /dev/null +++ b/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureSigningAlgorithmEnforceExecutor.java @@ -0,0 +1,135 @@ +/* + * Copyright 2020 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.services.clientpolicy.executor; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Set; + +import org.apache.commons.compress.utils.Sets; +import org.jboss.logging.Logger; + +import org.keycloak.OAuthErrorException; +import org.keycloak.component.ComponentModel; +import org.keycloak.crypto.Algorithm; +import org.keycloak.models.KeycloakSession; +import org.keycloak.protocol.oidc.OIDCConfigAttributes; +import org.keycloak.representations.idm.ClientRepresentation; +import org.keycloak.services.clientpolicy.AdminClientRegisterContext; +import org.keycloak.services.clientpolicy.AdminClientUpdateContext; +import org.keycloak.services.clientpolicy.ClientPolicyContext; +import org.keycloak.services.clientpolicy.ClientPolicyException; +import org.keycloak.services.clientpolicy.ClientPolicyLogger; +import org.keycloak.services.clientpolicy.DynamicClientRegisterContext; +import org.keycloak.services.clientpolicy.DynamicClientUpdateContext; + +public class SecureSigningAlgorithmEnforceExecutor implements ClientPolicyExecutorProvider { + + private static final Logger logger = Logger.getLogger(SecureSigningAlgorithmEnforceExecutor.class); + + private final KeycloakSession session; + private final ComponentModel componentModel; + + private static final List sigTargets = Arrays.asList( + OIDCConfigAttributes.USER_INFO_RESPONSE_SIGNATURE_ALG, + OIDCConfigAttributes.REQUEST_OBJECT_SIGNATURE_ALG, + OIDCConfigAttributes.ID_TOKEN_SIGNED_RESPONSE_ALG, + OIDCConfigAttributes.TOKEN_ENDPOINT_AUTH_SIGNING_ALG); + + private static final List sigTargetsAdminRestApiOnly = Arrays.asList( + OIDCConfigAttributes.ACCESS_TOKEN_SIGNED_RESPONSE_ALG); + + public SecureSigningAlgorithmEnforceExecutor(KeycloakSession session, ComponentModel componentModel) { + this.session = session; + this.componentModel = componentModel; + } + + @Override + public String getName() { + return componentModel.getName(); + } + + @Override + public String getProviderId() { + return componentModel.getProviderId(); + } + + @Override + public void executeOnEvent(ClientPolicyContext context) throws ClientPolicyException { + switch (context.getEvent()) { + case REGISTER: + if (context instanceof AdminClientRegisterContext) { + verifySecureSigningAlgorithm(((AdminClientRegisterContext)context).getProposedClientRepresentation(), true, false); + } else if (context instanceof DynamicClientRegisterContext) { + verifySecureSigningAlgorithm(((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); + } else if (context instanceof DynamicClientUpdateContext) { + verifySecureSigningAlgorithm(((DynamicClientUpdateContext)context).getProposedClientRepresentation(), false, true); + } else { + throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "not allowed input format."); + } + break; + default: + return; + } + } + + 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."); + } + + for (String sigTarget : sigTargets) { + verifySecureSigningAlgorithm(sigTarget, clientRep.getAttributes().get(sigTarget)); + } + + // no client metadata found in RFC 7591 OAuth Dynamic Client Registration Metadata + if (byAdminRestApi) { + for (String sigTarget : sigTargetsAdminRestApiOnly) { + verifySecureSigningAlgorithm(sigTarget, clientRep.getAttributes().get(sigTarget)); + } + } + } + + private void verifySecureSigningAlgorithm(String sigTarget, String sigAlg) throws ClientPolicyException { + if (sigAlg == null) { + ClientPolicyLogger.logv(logger, "Signing algorithm not specified explicitly. signature target = {0}", sigTarget); + return; + } + switch (sigAlg) { + case Algorithm.PS256: + case Algorithm.PS384: + case Algorithm.PS512: + case Algorithm.ES256: + case Algorithm.ES384: + case Algorithm.ES512: + ClientPolicyLogger.logv(logger, "Passed. signature target = {0}, signature algorithm = {1}", sigTarget, sigAlg); + return; + } + ClientPolicyLogger.logv(logger, "NOT allowed signatureAlgorithm. signature target = {0}, signature algorithm = {1}", sigTarget, sigAlg); + throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST, "not allowed signature algorithm."); + } + +} 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 new file mode 100644 index 0000000000..3071e6837d --- /dev/null +++ b/services/src/main/java/org/keycloak/services/clientpolicy/executor/SecureSigningAlgorithmEnforceExecutorFactory.java @@ -0,0 +1,65 @@ +/* + * Copyright 2020 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.services.clientpolicy.executor; + +import java.util.Collections; +import java.util.List; + +import org.keycloak.Config.Scope; +import org.keycloak.component.ComponentModel; +import org.keycloak.models.KeycloakSession; +import org.keycloak.models.KeycloakSessionFactory; +import org.keycloak.provider.ProviderConfigProperty; + +public class SecureSigningAlgorithmEnforceExecutorFactory implements ClientPolicyExecutorProviderFactory { + + public static final String PROVIDER_ID = "securesignalg-enforce-executor"; + + @Override + public ClientPolicyExecutorProvider create(KeycloakSession session, ComponentModel model) { + return new SecureSigningAlgorithmEnforceExecutor(session, model); + } + + @Override + public void init(Scope config) { + } + + @Override + public void postInit(KeycloakSessionFactory factory) { + } + + @Override + public void close() { + } + + @Override + public String getId() { + return PROVIDER_ID; + } + + @Override + public String getHelpText() { + return "It refuses the client whose signature algorithms are considered not to be secure. It accepts ES256, ES384, ES512, PS256, PS384 and PS512."; + } + + @Override + public List getConfigProperties() { + return Collections.emptyList(); + } + +} diff --git a/services/src/main/resources/META-INF/services/org.keycloak.services.clientpolicy.executor.ClientPolicyExecutorProviderFactory b/services/src/main/resources/META-INF/services/org.keycloak.services.clientpolicy.executor.ClientPolicyExecutorProviderFactory index 4fb3071840..35c98f4b08 100644 --- a/services/src/main/resources/META-INF/services/org.keycloak.services.clientpolicy.executor.ClientPolicyExecutorProviderFactory +++ b/services/src/main/resources/META-INF/services/org.keycloak.services.clientpolicy.executor.ClientPolicyExecutorProviderFactory @@ -2,4 +2,5 @@ org.keycloak.services.clientpolicy.executor.SecureResponseTypeExecutorFactory org.keycloak.services.clientpolicy.executor.SecureRequestObjectExecutorFactory org.keycloak.services.clientpolicy.executor.SecureClientAuthEnforceExecutorFactory org.keycloak.services.clientpolicy.executor.PKCEEnforceExecutorFactory -org.keycloak.services.clientpolicy.executor.SecureSessionEnforceExecutorFactory \ No newline at end of file +org.keycloak.services.clientpolicy.executor.SecureSessionEnforceExecutorFactory +org.keycloak.services.clientpolicy.executor.SecureSigningAlgorithmEnforceExecutorFactory \ No newline at end of file diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPolicyBasicsTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPolicyBasicsTest.java index f4d54bbfde..8141890746 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPolicyBasicsTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPolicyBasicsTest.java @@ -29,6 +29,7 @@ import java.security.MessageDigest; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.function.Consumer; @@ -62,6 +63,7 @@ import org.keycloak.events.EventType; import org.keycloak.jose.jws.Algorithm; import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.protocol.oidc.OIDCAdvancedConfigWrapper; +import org.keycloak.protocol.oidc.OIDCConfigAttributes; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.protocol.oidc.utils.OIDCResponseType; import org.keycloak.representations.AccessToken; @@ -90,6 +92,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.SecureSigningAlgorithmEnforceExecutorFactory; import org.keycloak.testsuite.AbstractKeycloakTest; import org.keycloak.testsuite.AssertEvents; import org.keycloak.testsuite.admin.ApiUtil; @@ -860,6 +863,121 @@ public class ClientPolicyBasicsTest extends AbstractKeycloakTest { } } + @Test + public void testSecureSigningAlgorithmEnforceExecutor() throws ClientRegistrationException, ClientPolicyException { + String policyName = "MyPolicy"; + createPolicy(policyName, DefaultClientPolicyProviderFactory.PROVIDER_ID, null, null, null); + logger.info("... Created Policy : " + policyName); + + createCondition("ClientUpdateContextCondition", ClientUpdateContextConditionFactory.PROVIDER_ID, null, (ComponentRepresentation provider) -> { + setConditionRegistrationMethods(provider, new ArrayList<>(Arrays.asList( + ClientUpdateContextConditionFactory.BY_AUTHENTICATED_USER, + ClientUpdateContextConditionFactory.BY_INITIAL_ACCESS_TOKEN, + ClientUpdateContextConditionFactory.BY_REGISTRATION_ACCESS_TOKEN))); + }); + registerCondition("ClientUpdateContextCondition", policyName); + logger.info("... Registered Condition : ClientUpdateContextConditionFactory"); + + createExecutor("SecureSigningAlgorithmEnforceExecutor", SecureSigningAlgorithmEnforceExecutorFactory.PROVIDER_ID, null, (ComponentRepresentation provider) -> { + }); + registerExecutor("SecureSigningAlgorithmEnforceExecutor", policyName); + logger.info("... Registered Executor : SecureSigningAlgorithmEnforceExecutor"); + + String cAppAdminId = null; + String cAppDynamicId = null; + try { + + // create by Admin REST API - fail + try { + createClientByAdmin("App-by-Admin", (ClientRepresentation clientRep) -> { + clientRep.setDefaultRoles((String[]) Arrays.asList("sample-client-role-beta").toArray(new String[1])); + clientRep.setSecret("secretBeta"); + clientRep.setAttributes(new HashMap<>()); + clientRep.getAttributes().put(OIDCConfigAttributes.USER_INFO_RESPONSE_SIGNATURE_ALG, Algorithm.none.name()); + }); + fail(); + } catch (ClientPolicyException e) { + assertEquals(Errors.INVALID_REGISTRATION, e.getMessage()); + } + + // create by Admin REST API - success + cAppAdminId = createClientByAdmin("App-by-Admin", (ClientRepresentation clientRep) -> { + clientRep.setAttributes(new HashMap<>()); + clientRep.getAttributes().put(OIDCConfigAttributes.USER_INFO_RESPONSE_SIGNATURE_ALG, Algorithm.PS256.name()); + clientRep.getAttributes().put(OIDCConfigAttributes.REQUEST_OBJECT_SIGNATURE_ALG, Algorithm.ES256.name()); + clientRep.getAttributes().put(OIDCConfigAttributes.ID_TOKEN_SIGNED_RESPONSE_ALG, Algorithm.ES256.name()); + clientRep.getAttributes().put(OIDCConfigAttributes.TOKEN_ENDPOINT_AUTH_SIGNING_ALG, Algorithm.ES256.name()); + clientRep.getAttributes().put(OIDCConfigAttributes.ACCESS_TOKEN_SIGNED_RESPONSE_ALG, Algorithm.ES256.name()); + }); + + // update by Admin REST API - fail + try { + updateClientByAdmin(cAppAdminId, (ClientRepresentation clientRep) -> { + clientRep.setAttributes(new HashMap<>()); + clientRep.getAttributes().put(OIDCConfigAttributes.ACCESS_TOKEN_SIGNED_RESPONSE_ALG, Algorithm.RS512.name()); + }); + } catch (Exception e) { + assertEquals("HTTP 400 Bad Request", e.getMessage()); + } + ClientRepresentation cRep = getClientByAdmin(cAppAdminId); + assertEquals(Algorithm.ES256.name(), cRep.getAttributes().get(OIDCConfigAttributes.ACCESS_TOKEN_SIGNED_RESPONSE_ALG)); + + // update by Admin REST API - success + updateClientByAdmin(cAppAdminId, (ClientRepresentation clientRep) -> { + clientRep.setAttributes(new HashMap<>()); + clientRep.getAttributes().put(OIDCConfigAttributes.ACCESS_TOKEN_SIGNED_RESPONSE_ALG, Algorithm.PS384.name()); + }); + cRep = getClientByAdmin(cAppAdminId); + assertEquals(Algorithm.PS384.name(), cRep.getAttributes().get(OIDCConfigAttributes.ACCESS_TOKEN_SIGNED_RESPONSE_ALG)); + + // create dynamically - fail + try { + createClientByAdmin("App-in-Dynamic", (ClientRepresentation clientRep) -> { + clientRep.setDefaultRoles((String[]) Arrays.asList("sample-client-role-beta").toArray(new String[1])); + clientRep.setSecret("secretBeta"); + clientRep.setAttributes(new HashMap<>()); + clientRep.getAttributes().put(OIDCConfigAttributes.USER_INFO_RESPONSE_SIGNATURE_ALG, Algorithm.RS384.name()); + }); + fail(); + } catch (ClientPolicyException e) { + assertEquals(Errors.INVALID_REGISTRATION, e.getMessage()); + } + + // create dynamically - success + cAppDynamicId = createClientDynamically("App-in-Dynamic", (OIDCClientRepresentation clientRep) -> { + clientRep.setUserinfoSignedResponseAlg(Algorithm.ES256.name()); + clientRep.setRequestObjectSigningAlg(Algorithm.ES256.name()); + clientRep.setIdTokenSignedResponseAlg(Algorithm.PS256.name()); + clientRep.setTokenEndpointAuthSigningAlg(Algorithm.PS256.name()); + }); + events.expect(EventType.CLIENT_REGISTER).client(cAppDynamicId).user(Matchers.isEmptyOrNullString()).assertEvent(); + getClientDynamically(cAppDynamicId); + + // update dynamically - fail + try { + updateClientDynamically(cAppDynamicId, (OIDCClientRepresentation clientRep) -> { + clientRep.setIdTokenSignedResponseAlg(Algorithm.RS256.name()); + }); + fail(); + } catch (ClientRegistrationException e) { + assertEquals("Failed to send request", e.getMessage()); + } + OIDCClientRepresentation oidcCRep = getClientDynamically(cAppDynamicId); + assertEquals(Algorithm.PS256.name(), oidcCRep.getIdTokenSignedResponseAlg()); + + // update dynamically - success + updateClientDynamically(cAppDynamicId, (OIDCClientRepresentation clientRep) -> { + clientRep.setIdTokenSignedResponseAlg(Algorithm.ES384.name()); + }); + oidcCRep = getClientDynamically(cAppDynamicId); + assertEquals(Algorithm.ES384.name(), oidcCRep.getIdTokenSignedResponseAlg()); + + } finally { + deleteClientByAdmin(cAppAdminId); + deleteClientDynamically(cAppDynamicId); + } + } + private AuthorizationEndpointRequestObject createValidRequestObjectForSecureRequestObjectExecutor(String clientId) throws URISyntaxException { AuthorizationEndpointRequestObject requestObject = new AuthorizationEndpointRequestObject(); requestObject.id(KeycloakModelUtils.generateId());