From 3c537f5f2875d7097eda94fb3c6c5957482698e2 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Tue, 25 Jul 2017 23:00:07 +0200 Subject: [PATCH] KEYCLOAK-4446 Do not encrypt SAML status messages SAML status messages are not encryptable per Chapter 6 of saml-core-2.0-os.pdf. Only assertions, attributes, base ID and name ID can be encrypted. --- .../saml/SamlAuthenticationError.java | 17 ++++++- .../keycloak/protocol/saml/SamlProtocol.java | 13 ++---- .../adapter/page/SalesPostServlet.java | 1 + .../updaters/ClientAttributeUpdater.java | 5 +++ .../AbstractSAMLServletsAdapterTest.java | 44 +++++++++++++++++++ 5 files changed, 69 insertions(+), 11 deletions(-) diff --git a/adapters/saml/core-public/src/main/java/org/keycloak/adapters/saml/SamlAuthenticationError.java b/adapters/saml/core-public/src/main/java/org/keycloak/adapters/saml/SamlAuthenticationError.java index 29bbbfa566..f44534d542 100755 --- a/adapters/saml/core-public/src/main/java/org/keycloak/adapters/saml/SamlAuthenticationError.java +++ b/adapters/saml/core-public/src/main/java/org/keycloak/adapters/saml/SamlAuthenticationError.java @@ -18,7 +18,10 @@ package org.keycloak.adapters.saml; import org.keycloak.adapters.spi.AuthenticationError; +import org.keycloak.dom.saml.v2.protocol.StatusCodeType; import org.keycloak.dom.saml.v2.protocol.StatusResponseType; +import org.keycloak.saml.common.constants.JBossSAMLURIConstants; +import java.util.Objects; /** * Object that describes the SAML error that happened. @@ -27,6 +30,7 @@ import org.keycloak.dom.saml.v2.protocol.StatusResponseType; * @version $Revision: 1 $ */ public class SamlAuthenticationError implements AuthenticationError { + public static enum Reason { EXTRACTION_FAILURE, INVALID_SIGNATURE, @@ -59,7 +63,18 @@ public class SamlAuthenticationError implements AuthenticationError { @Override public String toString() { - return "SamlAuthenticationError [reason=" + reason + ", status=" + status + "]"; + return "SamlAuthenticationError [reason=" + reason + ", status=" + + ((status == null || status.getStatus() == null) ? "UNKNOWN" : extractStatusCode(status.getStatus().getStatusCode())) + + "]"; } + private String extractStatusCode(StatusCodeType statusCode) { + if (statusCode == null || statusCode.getValue() == null) { + return "UNKNOWN"; + } + if (Objects.equals(JBossSAMLURIConstants.STATUS_RESPONDER.get(), statusCode.getValue().toString())) { + return extractStatusCode(statusCode.getStatusCode()); + } + return statusCode.getValue().toString(); + } } diff --git a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java index caa97095ef..66a7609568 100755 --- a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java +++ b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java @@ -190,16 +190,9 @@ public class SamlProtocol implements LoginProtocol { } binding.signatureAlgorithm(samlClient.getSignatureAlgorithm()).signWith(keyName, keys.getPrivateKey(), keys.getPublicKey(), keys.getCertificate()).signDocument(); } - if (samlClient.requiresEncryption()) { - PublicKey publicKey; - try { - publicKey = SamlProtocolUtils.getEncryptionKey(client); - } catch (Exception e) { - logger.error("failed", e); - return ErrorPage.error(session, Messages.FAILED_TO_PROCESS_RESPONSE); - } - binding.encrypt(publicKey); - } + // There is no support for encrypting status messages in SAML. + // Only assertions, attributes, base ID and name ID can be encrypted + // See Chapter 6 of saml-core-2.0-os.pdf Document document = builder.buildDocument(); return buildErrorResponse(authSession, binding, document); } catch (Exception e) { diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/adapter/page/SalesPostServlet.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/adapter/page/SalesPostServlet.java index cd9ea11854..01b8b089f9 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/adapter/page/SalesPostServlet.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/adapter/page/SalesPostServlet.java @@ -27,6 +27,7 @@ import java.net.URL; */ public class SalesPostServlet extends SAMLServlet { public static final String DEPLOYMENT_NAME = "sales-post"; + public static final String CLIENT_NAME = "http://localhost:8081/sales-post/"; @ArquillianResource @OperateOnDeployment(DEPLOYMENT_NAME) diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java index e967a33042..7b82f9b9a7 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java @@ -35,6 +35,11 @@ public class ClientAttributeUpdater { return this; } + public ClientAttributeUpdater setConsentRequired(Boolean consentRequired) { + rep.setConsentRequired(consentRequired); + return this; + } + public ClientAttributeUpdater setFrontchannelLogout(Boolean frontchannelLogout) { rep.setFrontchannelLogout(frontchannelLogout); return this; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/AbstractSAMLServletsAdapterTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/AbstractSAMLServletsAdapterTest.java index 77f6281056..649fa7a56a 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/AbstractSAMLServletsAdapterTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/AbstractSAMLServletsAdapterTest.java @@ -655,6 +655,50 @@ public abstract class AbstractSAMLServletsAdapterTest extends AbstractServletsAd } } + @Test + public void salesPostEncRejectConsent() throws Exception { + ClientRepresentation salesPostEncClient = testRealmResource().clients().findByClientId(SalesPostEncServlet.CLIENT_NAME).get(0); + try (Closeable client = new ClientAttributeUpdater(testRealmResource().clients().get(salesPostEncClient.getId())) + .setConsentRequired(true) + .update()) { + new SamlClientBuilder() + .navigateTo(salesPostEncServletPage.toString()) + .processSamlResponse(Binding.POST).build() + .login().user(bburkeUser).build() + .consentRequired().approveConsent(false).build() + .processSamlResponse(Binding.POST).build() + + .execute(r -> { + assertThat(r, statusCodeIsHC(Response.Status.OK)); + assertThat(r, bodyHC(containsString("urn:oasis:names:tc:SAML:2.0:status:RequestDenied"))); // TODO: revisit - should the HTTP status be 403 too? + }); + } finally { + salesPostEncServletPage.logout(); + } + } + + @Test + public void salesPostRejectConsent() throws Exception { + ClientRepresentation salesPostClient = testRealmResource().clients().findByClientId(SalesPostServlet.CLIENT_NAME).get(0); + try (Closeable client = new ClientAttributeUpdater(testRealmResource().clients().get(salesPostClient.getId())) + .setConsentRequired(true) + .update()) { + new SamlClientBuilder() + .navigateTo(salesPostServletPage.toString()) + .processSamlResponse(Binding.POST).build() + .login().user(bburkeUser).build() + .consentRequired().approveConsent(false).build() + .processSamlResponse(Binding.POST).build() + + .execute(r -> { + assertThat(r, statusCodeIsHC(Response.Status.OK)); + assertThat(r, bodyHC(containsString("urn:oasis:names:tc:SAML:2.0:status:RequestDenied"))); // TODO: revisit - should the HTTP status be 403 too? + }); + } finally { + salesPostServletPage.logout(); + } + } + @Test public void salesPostPassiveTest() { salesPostPassiveServletPage.navigateTo();