From f289b281a0b2149fdb2b3f7e223f56cc534393fe Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Thu, 19 Jan 2017 15:58:18 +0100 Subject: [PATCH 1/4] KEYCLOAK-4262 --- .../keycloak/protocol/saml/SamlProtocol.java | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) 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 89d6bf3cf7..69025c23d8 100755 --- a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java +++ b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java @@ -161,13 +161,15 @@ public class SamlProtocol implements LoginProtocol { @Override public Response sendError(ClientSessionModel clientSession, Error error) { try { - if ("true".equals(clientSession.getClient().getAttribute(SAML_IDP_INITIATED_LOGIN))) { + ClientModel client = clientSession.getClient(); + + if ("true".equals(client.getAttribute(SAML_IDP_INITIATED_LOGIN))) { if (error == Error.CANCELLED_BY_USER) { UriBuilder builder = RealmsResource.protocolUrl(uriInfo).path(SamlService.class, "idpInitiatedSSO"); Map params = new HashMap<>(); params.put("realm", realm.getName()); params.put("protocol", LOGIN_PROTOCOL); - params.put("client", clientSession.getClient().getAttribute(SAML_IDP_INITIATED_SSO_URL_NAME)); + params.put("client", client.getAttribute(SAML_IDP_INITIATED_SSO_URL_NAME)); URI redirect = builder.buildFromMap(params); return Response.status(302).location(redirect).build(); } else { @@ -177,6 +179,27 @@ public class SamlProtocol implements LoginProtocol { SAML2ErrorResponseBuilder builder = new SAML2ErrorResponseBuilder().destination(clientSession.getRedirectUri()).issuer(getResponseIssuer(realm)).status(translateErrorToSAMLStatus(error).get()); try { JaxrsSAML2BindingBuilder binding = new JaxrsSAML2BindingBuilder().relayState(clientSession.getNote(GeneralConstants.RELAY_STATE)); + SamlClient samlClient = new SamlClient(client); + KeyManager keyManager = session.keys(); + if (samlClient.requiresRealmSignature()) { + KeyManager.ActiveRsaKey keys = keyManager.getActiveRsaKey(realm); + String keyName = samlClient.getXmlSigKeyInfoKeyNameTransformer().getKeyName(keys.getKid(), keys.getCertificate()); + String canonicalization = samlClient.getCanonicalizationMethod(); + if (canonicalization != null) { + binding.canonicalizationMethod(canonicalization); + } + binding.signatureAlgorithm(samlClient.getSignatureAlgorithm()).signWith(keyName, keys.getPrivateKey(), keys.getPublicKey(), keys.getCertificate()).signDocument(); + } + if (samlClient.requiresEncryption()) { + PublicKey publicKey; + try { + publicKey = SamlProtocolUtils.getEncryptionValidationKey(client); + } catch (Exception e) { + logger.error("failed", e); + return ErrorPage.error(session, Messages.FAILED_TO_PROCESS_RESPONSE); + } + binding.encrypt(publicKey); + } Document document = builder.buildDocument(); return buildErrorResponse(clientSession, binding, document); } catch (Exception e) { From 350b9550c3dca1664c41802c585530e025a2213d Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Thu, 19 Jan 2017 15:59:28 +0100 Subject: [PATCH 2/4] KEYCLOAK-4264 --- .../AbstractSamlAuthenticationHandler.java | 38 +++++++++-- .../keycloaksaml/SamlAdapterTest.java | 8 ++- .../keycloaksaml/SamlAdapterTestStrategy.java | 67 ++++++++++++++++--- .../testsuite/samlfilter/SamlAdapterTest.java | 2 +- .../org/keycloak/testsuite/JettySamlTest.java | 8 ++- .../org/keycloak/testsuite/JettySamlTest.java | 8 ++- .../org/keycloak/testsuite/JettySamlTest.java | 8 ++- .../org/keycloak/testsuite/JettySamlTest.java | 8 ++- .../keycloak/testsuite/TomcatSamlTest.java | 8 ++- .../keycloak/testsuite/TomcatSamlTest.java | 8 ++- .../keycloak/testsuite/TomcatSamlTest.java | 8 ++- 11 files changed, 139 insertions(+), 32 deletions(-) diff --git a/adapters/saml/core/src/main/java/org/keycloak/adapters/saml/profile/AbstractSamlAuthenticationHandler.java b/adapters/saml/core/src/main/java/org/keycloak/adapters/saml/profile/AbstractSamlAuthenticationHandler.java index 1bbdf6dae5..cb9b4d9fd7 100644 --- a/adapters/saml/core/src/main/java/org/keycloak/adapters/saml/profile/AbstractSamlAuthenticationHandler.java +++ b/adapters/saml/core/src/main/java/org/keycloak/adapters/saml/profile/AbstractSamlAuthenticationHandler.java @@ -60,6 +60,7 @@ import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder; import org.keycloak.saml.processing.core.saml.v2.util.AssertionUtil; import org.keycloak.saml.processing.web.util.PostBindingUtil; import org.w3c.dom.Document; +import org.w3c.dom.Element; import org.w3c.dom.Node; import java.io.IOException; @@ -70,14 +71,11 @@ import java.security.KeyManagementException; import java.security.PublicKey; import java.security.Signature; import java.security.SignatureException; -import java.util.HashSet; -import java.util.List; -import java.util.Set; +import java.util.*; import org.keycloak.dom.saml.v2.SAML2Object; import org.keycloak.dom.saml.v2.protocol.ExtensionsType; import org.keycloak.rotation.KeyLocator; import org.keycloak.saml.processing.core.util.KeycloakKeySamlExtensionGenerator; -import org.w3c.dom.Element; /** * @@ -196,7 +194,7 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic challenge = new AuthChallenge() { @Override public boolean challenge(HttpFacade exchange) { - SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.INVALID_SIGNATURE); + SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.INVALID_SIGNATURE, statusResponse); exchange.getRequest().setError(error); exchange.getResponse().sendError(403); return true; @@ -312,8 +310,25 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic return false; } - protected AuthOutcome handleLoginResponse(ResponseType responseType, boolean postBinding, OnSessionCreated onCreateSession) { + protected AuthOutcome handleLoginResponse(final ResponseType responseType, boolean postBinding, OnSessionCreated onCreateSession) { AssertionType assertion = null; + if (! isSuccessfulSamlResponse(responseType) || responseType.getAssertions() == null || responseType.getAssertions().isEmpty()) { + challenge = new AuthChallenge() { + @Override + public boolean challenge(HttpFacade exchange) { + SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.ERROR_STATUS, responseType); + exchange.getRequest().setError(error); + exchange.getResponse().sendError(403); + return true; + } + + @Override + public int getResponseCode() { + return 403; + } + }; + return AuthOutcome.FAILED; + } try { assertion = AssertionUtil.getAssertion(responseType, deployment.getDecryptionKey()); if (AssertionUtil.hasExpired(assertion)) { @@ -335,6 +350,7 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic return 403; } }; + return AuthOutcome.FAILED; } if (deployment.getIDP().getSingleSignOnService().validateAssertionSignature()) { @@ -346,7 +362,7 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic challenge = new AuthChallenge() { @Override public boolean challenge(HttpFacade exchange) { - SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.INVALID_SIGNATURE); + SamlAuthenticationError error = new SamlAuthenticationError(SamlAuthenticationError.Reason.INVALID_SIGNATURE, responseType); exchange.getRequest().setError(error); exchange.getResponse().sendError(403); return true; @@ -449,6 +465,14 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic return AuthOutcome.AUTHENTICATED; } + private boolean isSuccessfulSamlResponse(ResponseType responseType) { + return responseType != null + && responseType.getStatus() != null + && responseType.getStatus().getStatusCode() != null + && responseType.getStatus().getStatusCode().getValue() != null + && Objects.equals(responseType.getStatus().getStatusCode().getValue().toString(), JBossSAMLURIConstants.STATUS_SUCCESS.get()); + } + private String getAttributeValue(Object attrValue) { String value = null; if (attrValue instanceof String) { diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/keycloaksaml/SamlAdapterTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/keycloaksaml/SamlAdapterTest.java index c4e2a365bb..d0c770e78b 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/keycloaksaml/SamlAdapterTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/keycloaksaml/SamlAdapterTest.java @@ -117,8 +117,12 @@ public class SamlAdapterTest { testStrategy.testSavedPostRequest(); } @Test - public void testErrorHandling() throws Exception { - testStrategy.testErrorHandling(); + public void testErrorHandlingSigned() throws Exception { + testStrategy.testErrorHandlingSigned(); + } + @Test + public void testErrorHandlingUnsigned() throws Exception { + testStrategy.testErrorHandlingUnsigned(); } @Test public void testMetadataPostSignedLoginLogout() throws Exception { diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/keycloaksaml/SamlAdapterTestStrategy.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/keycloaksaml/SamlAdapterTestStrategy.java index 9589fe95d0..91074e3936 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/keycloaksaml/SamlAdapterTestStrategy.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/keycloaksaml/SamlAdapterTestStrategy.java @@ -20,10 +20,12 @@ package org.keycloak.testsuite.keycloaksaml; import org.apache.commons.io.IOUtils; import org.junit.Assert; import org.junit.rules.ExternalResource; + import org.keycloak.adapters.saml.SamlAuthenticationError; import org.keycloak.adapters.saml.SamlPrincipal; import org.keycloak.admin.client.Keycloak; import org.keycloak.admin.client.resource.RealmResource; +import org.keycloak.common.util.*; import org.keycloak.models.ClientModel; import org.keycloak.models.Constants; import org.keycloak.models.KeycloakSession; @@ -38,9 +40,8 @@ import org.keycloak.protocol.saml.mappers.RoleNameMapper; import org.keycloak.protocol.saml.mappers.UserAttributeStatementMapper; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.RealmRepresentation; -import org.keycloak.saml.BaseSAML2BindingBuilder; -import org.keycloak.saml.SAML2ErrorResponseBuilder; -import org.keycloak.saml.common.constants.JBossSAMLURIConstants; +import org.keycloak.saml.*; +import org.keycloak.saml.common.constants.*; import org.keycloak.saml.processing.core.saml.v2.constants.X500SAMLProfileConstants; import org.keycloak.services.managers.RealmManager; import org.keycloak.testsuite.KeycloakServer; @@ -51,6 +52,7 @@ import org.keycloak.testsuite.rule.ErrorServlet; import org.keycloak.testsuite.rule.KeycloakRule; import org.keycloak.testsuite.rule.WebResource; import org.keycloak.testsuite.rule.WebRule; + import org.openqa.selenium.WebDriver; import org.w3c.dom.Document; @@ -61,10 +63,11 @@ import javax.ws.rs.core.Form; import javax.ws.rs.core.Response; import java.io.IOException; import java.net.URI; -import java.util.HashSet; -import java.util.LinkedList; -import java.util.List; -import java.util.Set; +import java.security.*; +import java.security.spec.*; +import java.util.*; +import java.util.Base64; +import java.util.logging.*; import static org.junit.Assert.assertEquals; @@ -77,6 +80,24 @@ public class SamlAdapterTestStrategy extends ExternalResource { protected String APP_SERVER_BASE_URL = "http://localhost:8081"; protected AbstractKeycloakRule keycloakRule; + private static final String REALM_PRIVATE_KEY_STR = "MIICXAIBAAKBgQCrVrCuTtArbgaZzL1hvh0xtL5mc7o0NqPVnYXkLvgcwiC3BjLGw1tGEGoJaXDuSaRllobm53JBhjx33UNv+5z/UMG4kytBWxheNVKnL6GgqlNabMaFfPLPCF8kAgKnsi79NMo+n6KnSY8YeUmec/p2vjO2NjsSAVcWEQMVhJ31LwIDAQABAoGAfmO8gVhyBxdqlxmIuglbz8bcjQbhXJLR2EoS8ngTXmN1bo2L90M0mUKSdc7qF10LgETBzqL8jYlQIbt+e6TH8fcEpKCjUlyq0Mf/vVbfZSNaVycY13nTzo27iPyWQHK5NLuJzn1xvxxrUeXI6A2WFpGEBLbHjwpx5WQG9A+2scECQQDvdn9NE75HPTVPxBqsEd2z10TKkl9CZxu10Qby3iQQmWLEJ9LNmy3acvKrE3gMiYNWb6xHPKiIqOR1as7L24aTAkEAtyvQOlCvr5kAjVqrEKXalj0Tzewjweuxc0pskvArTI2Oo070h65GpoIKLc9jf+UA69cRtquwP93aZKtW06U8dQJAF2Y44ks/mK5+eyDqik3koCI08qaC8HYq2wVl7G2QkJ6sbAaILtcvD92ToOvyGyeE0flvmDZxMYlvaZnaQ0lcSQJBAKZU6umJi3/xeEbkJqMfeLclD27XGEFoPeNrmdx0q10Azp4NfJAY+Z8KRyQCR2BEG+oNitBOZ+YXF9KCpH3cdmECQHEigJhYg+ykOvr1aiZUMFT72HU0jnmQe2FVekuG+LJUt2Tm7GtMjTFoGpf0JwrVuZN39fOYAlo+nTixgeW7X8Y="; + private static PrivateKey REALM_PRIVATE_KEY; + private static final String REALM_PUBLIC_KEY_STR = "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQCrVrCuTtArbgaZzL1hvh0xtL5mc7o0NqPVnYXkLvgcwiC3BjLGw1tGEGoJaXDuSaRllobm53JBhjx33UNv+5z/UMG4kytBWxheNVKnL6GgqlNabMaFfPLPCF8kAgKnsi79NMo+n6KnSY8YeUmec/p2vjO2NjsSAVcWEQMVhJ31LwIDAQAB"; + private static PublicKey REALM_PUBLIC_KEY; + + static { + try { + KeyFactory kf = KeyFactory.getInstance("RSA"); + byte[] encoded = Base64.getDecoder().decode(REALM_PUBLIC_KEY_STR); + REALM_PUBLIC_KEY = (PublicKey) kf.generatePublic(new X509EncodedKeySpec(encoded)); + + encoded = Base64.getDecoder().decode(REALM_PRIVATE_KEY_STR); + REALM_PRIVATE_KEY = (PrivateKey) kf.generatePrivate(new PKCS8EncodedKeySpec(encoded)); + } catch (NoSuchAlgorithmException | InvalidKeySpecException ex) { + Logger.getLogger(SamlAdapterTestStrategy.class.getName()).log(Level.SEVERE, null, ex); + } + } + public SamlAdapterTestStrategy(String AUTH_SERVER_URL, String APP_SERVER_BASE_URL, AbstractKeycloakRule keycloakRule) { this.AUTH_SERVER_URL = AUTH_SERVER_URL; this.APP_SERVER_BASE_URL = APP_SERVER_BASE_URL; @@ -173,7 +194,7 @@ public class SamlAdapterTestStrategy extends ExternalResource { - public void testErrorHandling() throws Exception { + public void testErrorHandlingUnsigned() throws Exception { ErrorServlet.authError = null; Client client = ClientBuilder.newClient(); // make sure @@ -194,6 +215,36 @@ public class SamlAdapterTestStrategy extends ExternalResource { client.close(); Assert.assertNotNull(ErrorServlet.authError); SamlAuthenticationError error = (SamlAuthenticationError)ErrorServlet.authError; + Assert.assertEquals(SamlAuthenticationError.Reason.INVALID_SIGNATURE, error.getReason()); + Assert.assertNotNull(error.getStatus()); + ErrorServlet.authError = null; + + } + + public void testErrorHandlingSigned() throws Exception { + ErrorServlet.authError = null; + Client client = ClientBuilder.newClient(); + // make sure + Response response = client.target(APP_SERVER_BASE_URL + "/employee-sig/").request().get(); + response.close(); + SAML2ErrorResponseBuilder builder = new SAML2ErrorResponseBuilder() + .destination(APP_SERVER_BASE_URL + "/employee-sig/saml") + .issuer(AUTH_SERVER_URL + "/realms/demo") + .status(JBossSAMLURIConstants.STATUS_REQUEST_DENIED.get()); + BaseSAML2BindingBuilder binding = new BaseSAML2BindingBuilder() + .relayState(null) + .signatureAlgorithm(SignatureAlgorithm.RSA_SHA256) + .signWith(KeyUtils.createKeyId(REALM_PRIVATE_KEY), REALM_PRIVATE_KEY, REALM_PUBLIC_KEY) + .signDocument(); + Document document = builder.buildDocument(); + URI uri = binding.generateRedirectUri(GeneralConstants.SAML_RESPONSE_KEY, APP_SERVER_BASE_URL + "/employee-sig/saml", document); + response = client.target(uri).request().get(); + String errorPage = response.readEntity(String.class); + response.close(); + Assert.assertTrue(errorPage.contains("Error Page")); + client.close(); + Assert.assertNotNull(ErrorServlet.authError); + SamlAuthenticationError error = (SamlAuthenticationError)ErrorServlet.authError; Assert.assertEquals(SamlAuthenticationError.Reason.ERROR_STATUS, error.getReason()); Assert.assertNotNull(error.getStatus()); ErrorServlet.authError = null; diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/samlfilter/SamlAdapterTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/samlfilter/SamlAdapterTest.java index acc27a6388..b3cdec4c21 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/samlfilter/SamlAdapterTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/samlfilter/SamlAdapterTest.java @@ -128,7 +128,7 @@ public class SamlAdapterTest { @Test public void testPostPassiveLoginLogout() { - testStrategy.testPostPassiveLoginLogout(false); + testStrategy.testPostPassiveLoginLogout(true); } @Test diff --git a/testsuite/jetty/jetty81/src/test/java/org/keycloak/testsuite/JettySamlTest.java b/testsuite/jetty/jetty81/src/test/java/org/keycloak/testsuite/JettySamlTest.java index 58d4f718ae..064d46e72c 100755 --- a/testsuite/jetty/jetty81/src/test/java/org/keycloak/testsuite/JettySamlTest.java +++ b/testsuite/jetty/jetty81/src/test/java/org/keycloak/testsuite/JettySamlTest.java @@ -111,8 +111,12 @@ public class JettySamlTest { @Test - public void testErrorHandling() throws Exception { - testStrategy.testErrorHandling(); + public void testErrorHandlingSigned() throws Exception { + testStrategy.testErrorHandlingSigned(); + } + @Test + public void testErrorHandlingUnsigned() throws Exception { + testStrategy.testErrorHandlingUnsigned(); } @Test diff --git a/testsuite/jetty/jetty91/src/test/java/org/keycloak/testsuite/JettySamlTest.java b/testsuite/jetty/jetty91/src/test/java/org/keycloak/testsuite/JettySamlTest.java index 58d4f718ae..064d46e72c 100755 --- a/testsuite/jetty/jetty91/src/test/java/org/keycloak/testsuite/JettySamlTest.java +++ b/testsuite/jetty/jetty91/src/test/java/org/keycloak/testsuite/JettySamlTest.java @@ -111,8 +111,12 @@ public class JettySamlTest { @Test - public void testErrorHandling() throws Exception { - testStrategy.testErrorHandling(); + public void testErrorHandlingSigned() throws Exception { + testStrategy.testErrorHandlingSigned(); + } + @Test + public void testErrorHandlingUnsigned() throws Exception { + testStrategy.testErrorHandlingUnsigned(); } @Test diff --git a/testsuite/jetty/jetty92/src/test/java/org/keycloak/testsuite/JettySamlTest.java b/testsuite/jetty/jetty92/src/test/java/org/keycloak/testsuite/JettySamlTest.java index 7f1ec9865f..f1e72a4d30 100755 --- a/testsuite/jetty/jetty92/src/test/java/org/keycloak/testsuite/JettySamlTest.java +++ b/testsuite/jetty/jetty92/src/test/java/org/keycloak/testsuite/JettySamlTest.java @@ -111,8 +111,12 @@ public class JettySamlTest { @Test - public void testErrorHandling() throws Exception { - testStrategy.testErrorHandling(); + public void testErrorHandlingSigned() throws Exception { + testStrategy.testErrorHandlingSigned(); + } + @Test + public void testErrorHandlingUnsigned() throws Exception { + testStrategy.testErrorHandlingUnsigned(); } @Test diff --git a/testsuite/jetty/jetty93/src/test/java/org/keycloak/testsuite/JettySamlTest.java b/testsuite/jetty/jetty93/src/test/java/org/keycloak/testsuite/JettySamlTest.java index 7f1ec9865f..f1e72a4d30 100644 --- a/testsuite/jetty/jetty93/src/test/java/org/keycloak/testsuite/JettySamlTest.java +++ b/testsuite/jetty/jetty93/src/test/java/org/keycloak/testsuite/JettySamlTest.java @@ -111,8 +111,12 @@ public class JettySamlTest { @Test - public void testErrorHandling() throws Exception { - testStrategy.testErrorHandling(); + public void testErrorHandlingSigned() throws Exception { + testStrategy.testErrorHandlingSigned(); + } + @Test + public void testErrorHandlingUnsigned() throws Exception { + testStrategy.testErrorHandlingUnsigned(); } @Test diff --git a/testsuite/tomcat6/src/test/java/org/keycloak/testsuite/TomcatSamlTest.java b/testsuite/tomcat6/src/test/java/org/keycloak/testsuite/TomcatSamlTest.java index d306ede222..dc3bd094af 100755 --- a/testsuite/tomcat6/src/test/java/org/keycloak/testsuite/TomcatSamlTest.java +++ b/testsuite/tomcat6/src/test/java/org/keycloak/testsuite/TomcatSamlTest.java @@ -120,8 +120,12 @@ public class TomcatSamlTest { } @Test - public void testErrorHandling() throws Exception { - testStrategy.testErrorHandling(); + public void testErrorHandlingSigned() throws Exception { + testStrategy.testErrorHandlingSigned(); + } + @Test + public void testErrorHandlingUnsigned() throws Exception { + testStrategy.testErrorHandlingUnsigned(); } @Test diff --git a/testsuite/tomcat7/src/test/java/org/keycloak/testsuite/TomcatSamlTest.java b/testsuite/tomcat7/src/test/java/org/keycloak/testsuite/TomcatSamlTest.java index 0eb7b94889..b4a6d1c5b2 100755 --- a/testsuite/tomcat7/src/test/java/org/keycloak/testsuite/TomcatSamlTest.java +++ b/testsuite/tomcat7/src/test/java/org/keycloak/testsuite/TomcatSamlTest.java @@ -98,8 +98,12 @@ public class TomcatSamlTest { @Test - public void testErrorHandling() throws Exception { - testStrategy.testErrorHandling(); + public void testErrorHandlingSigned() throws Exception { + testStrategy.testErrorHandlingSigned(); + } + @Test + public void testErrorHandlingUnsigned() throws Exception { + testStrategy.testErrorHandlingUnsigned(); } @Test diff --git a/testsuite/tomcat8/src/test/java/org/keycloak/testsuite/TomcatSamlTest.java b/testsuite/tomcat8/src/test/java/org/keycloak/testsuite/TomcatSamlTest.java index b347f48b63..14564962ec 100755 --- a/testsuite/tomcat8/src/test/java/org/keycloak/testsuite/TomcatSamlTest.java +++ b/testsuite/tomcat8/src/test/java/org/keycloak/testsuite/TomcatSamlTest.java @@ -99,8 +99,12 @@ public class TomcatSamlTest { @Test - public void testErrorHandling() throws Exception { - testStrategy.testErrorHandling(); + public void testErrorHandlingSigned() throws Exception { + testStrategy.testErrorHandlingSigned(); + } + @Test + public void testErrorHandlingUnsigned() throws Exception { + testStrategy.testErrorHandlingUnsigned(); } @Test From 99fcc510193fe6ee7a67491a8dc5ecfac3637fb7 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Thu, 19 Jan 2017 16:02:53 +0100 Subject: [PATCH 3/4] KEYCLOAK-4261 Fix response type to SAML AuthnRequest messages --- .../main/java/org/keycloak/saml/SAML2ErrorResponseBuilder.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/saml-core/src/main/java/org/keycloak/saml/SAML2ErrorResponseBuilder.java b/saml-core/src/main/java/org/keycloak/saml/SAML2ErrorResponseBuilder.java index 6da6799b42..5873a7fd75 100755 --- a/saml-core/src/main/java/org/keycloak/saml/SAML2ErrorResponseBuilder.java +++ b/saml-core/src/main/java/org/keycloak/saml/SAML2ErrorResponseBuilder.java @@ -22,6 +22,7 @@ import java.util.List; import org.keycloak.dom.saml.v2.assertion.NameIDType; import org.keycloak.dom.saml.v2.protocol.ExtensionsType; import org.keycloak.dom.saml.v2.protocol.StatusResponseType; +import org.keycloak.dom.saml.v2.protocol.ResponseType; import org.keycloak.saml.common.exceptions.ConfigurationException; import org.keycloak.saml.common.exceptions.ParsingException; import org.keycloak.saml.common.exceptions.ProcessingException; @@ -66,7 +67,7 @@ public class SAML2ErrorResponseBuilder implements SamlProtocolExtensionsAwareBui public Document buildDocument() throws ProcessingException { try { - StatusResponseType statusResponse = new StatusResponseType(IDGenerator.create("ID_"), XMLTimeUtil.getIssueInstant()); + StatusResponseType statusResponse = new ResponseType(IDGenerator.create("ID_"), XMLTimeUtil.getIssueInstant()); statusResponse.setStatus(JBossSAMLAuthnResponseFactory.createStatusTypeForResponder(status)); NameIDType issuer = new NameIDType(); From 5da491c2706c5863f02aeac2b530643f98f7f5ee Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Thu, 19 Jan 2017 16:04:04 +0100 Subject: [PATCH 4/4] KEYCLOAK-4181 Fix handling of SAML error code in broker --- .../keycloak/broker/saml/SAMLEndpoint.java | 18 +++++++ .../keycloak/testsuite/pages/ConsentPage.java | 7 +++ .../broker/AbstractBaseBrokerTest.java | 5 -- .../testsuite/broker/AbstractBrokerTest.java | 51 +++++++++++++++++-- .../testsuite/broker/BrokerConfiguration.java | 5 ++ .../broker/KcOidcBrokerConfiguration.java | 6 +++ .../broker/KcSamlBrokerConfiguration.java | 7 ++- .../broker/KcSamlSignedBrokerTest.java | 2 - .../testsuite/util/ClientBuilder.java | 5 ++ 9 files changed, 93 insertions(+), 13 deletions(-) diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java index 39a9d2efba..38e57cb256 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java @@ -19,6 +19,7 @@ package org.keycloak.broker.saml; import org.jboss.logging.Logger; import org.jboss.resteasy.annotations.cache.NoCache; + import org.keycloak.broker.provider.BrokeredIdentityContext; import org.keycloak.broker.provider.IdentityBrokerException; import org.keycloak.broker.provider.IdentityProvider; @@ -78,10 +79,13 @@ import java.security.Key; import java.security.cert.X509Certificate; import java.util.LinkedList; import java.util.List; + import org.keycloak.rotation.HardcodedKeyLocator; import org.keycloak.rotation.KeyLocator; import org.keycloak.saml.processing.core.util.KeycloakKeySamlExtensionGenerator; +import java.util.*; + /** * @author Bill Burke * @version $Revision: 1 $ @@ -333,6 +337,13 @@ public class SAMLEndpoint { try { KeyManager.ActiveRsaKey keys = session.keys().getActiveRsaKey(realm); + if (! isSuccessfulSamlResponse(responseType)) { + String statusMessage = responseType.getStatus() == null ? Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR : responseType.getStatus().getStatusMessage(); + return callback.error(relayState, statusMessage); + } + if (responseType.getAssertions() == null || responseType.getAssertions().isEmpty()) { + return callback.error(relayState, Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR); + } AssertionType assertion = AssertionUtil.getAssertion(responseType, keys.getPrivateKey()); SubjectType subject = assertion.getSubject(); SubjectType.STSubType subType = subject.getSubType(); @@ -395,6 +406,13 @@ public class SAMLEndpoint { } + private boolean isSuccessfulSamlResponse(ResponseType responseType) { + return responseType != null + && responseType.getStatus() != null + && responseType.getStatus().getStatusCode() != null + && responseType.getStatus().getStatusCode().getValue() != null + && Objects.equals(responseType.getStatus().getStatusCode().getValue().toString(), JBossSAMLURIConstants.STATUS_SUCCESS.get()); + } public Response handleSamlResponse(String samlResponse, String relayState, String clientId) { diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/ConsentPage.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/ConsentPage.java index b8b244d545..5110b328cc 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/ConsentPage.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/ConsentPage.java @@ -28,10 +28,17 @@ public class ConsentPage extends AbstractPage { @FindBy(id = "kc-login") private WebElement submitButton; + @FindBy(id = "kc-cancel") + private WebElement cancelButton; + public void confirm() { submitButton.click(); } + public void cancel() { + cancelButton.click(); + } + @Override public boolean isCurrent() { return driver.getTitle().equalsIgnoreCase("grant access"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBaseBrokerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBaseBrokerTest.java index 217a4e7550..c2c628d942 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBaseBrokerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBaseBrokerTest.java @@ -20,10 +20,7 @@ package org.keycloak.testsuite.broker; import java.util.List; import org.jboss.arquillian.graphene.page.Page; -import org.junit.Before; -import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.representations.idm.RealmRepresentation; -import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.AbstractKeycloakTest; import org.keycloak.testsuite.Assert; import org.keycloak.testsuite.Retry; @@ -35,8 +32,6 @@ import org.keycloak.testsuite.pages.LoginPage; import org.keycloak.testsuite.pages.UpdateAccountInformationPage; import org.openqa.selenium.TimeoutException; -import static org.keycloak.testsuite.admin.ApiUtil.createUserWithAdminClient; -import static org.keycloak.testsuite.admin.ApiUtil.resetUserPassword; import static org.keycloak.testsuite.broker.BrokerTestTools.encodeUrl; import static org.keycloak.testsuite.broker.BrokerTestTools.waitForPage; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBrokerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBrokerTest.java index 8950d1b087..6f3314fb29 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBrokerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/AbstractBrokerTest.java @@ -2,28 +2,31 @@ package org.keycloak.testsuite.broker; import org.junit.Before; import org.junit.Test; + import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.admin.client.resource.UsersResource; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.Assert; -import org.keycloak.testsuite.util.RealmBuilder; +import org.keycloak.testsuite.pages.ConsentPage; +import org.keycloak.testsuite.util.*; import org.openqa.selenium.TimeoutException; import java.util.List; +import java.util.concurrent.TimeUnit; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.keycloak.testsuite.admin.ApiUtil.createUserWithAdminClient; import static org.keycloak.testsuite.admin.ApiUtil.resetUserPassword; import static org.keycloak.testsuite.broker.BrokerTestConstants.USER_EMAIL; -import static org.keycloak.testsuite.broker.BrokerTestTools.*; import static org.keycloak.testsuite.util.MailAssert.assertEmailAndGetUrl; -import org.keycloak.testsuite.util.MailServer; -import org.keycloak.testsuite.util.MailServerConfiguration; -import org.keycloak.testsuite.util.UserBuilder; + +import org.jboss.arquillian.graphene.page.Page; + +import static org.keycloak.testsuite.broker.BrokerTestTools.*; public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { @@ -256,6 +259,44 @@ public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { assertEquals("Account is disabled, contact admin.", errorPage.getError()); } + @Page + ConsentPage consentPage; + + // KEYCLOAK-4181 + @Test + public void loginWithExistingUserWithErrorFromProviderIdP() { + ClientRepresentation client = adminClient.realm(bc.providerRealmName()) + .clients() + .findByClientId(bc.getIDPClientIdInProviderRealm(suiteContext)) + .get(0); + + adminClient.realm(bc.providerRealmName()) + .clients() + .get(client.getId()) + .update(ClientBuilder.edit(client).consentRequired(true).build()); + + driver.navigate().to(getAccountUrl(bc.consumerRealmName())); + + log.debug("Clicking social " + bc.getIDPAlias()); + accountLoginPage.clickSocial(bc.getIDPAlias()); + + waitForPage(driver, "log in to"); + + Assert.assertTrue("Driver should be on the provider realm page right now", + driver.getCurrentUrl().contains("/auth/realms/" + bc.providerRealmName() + "/")); + + log.debug("Logging in"); + accountLoginPage.login(bc.getUserLogin(), bc.getUserPassword()); + + driver.manage().timeouts().pageLoadTimeout(30, TimeUnit.MINUTES); + + waitForPage(driver, "grant access"); + consentPage.cancel(); + + waitForPage(driver, "log in to"); + } + + protected void testSingleLogout() { log.debug("Testing single log out"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/BrokerConfiguration.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/BrokerConfiguration.java index 0e00a7001e..5605cf6238 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/BrokerConfiguration.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/BrokerConfiguration.java @@ -42,6 +42,11 @@ public interface BrokerConfiguration { */ String consumerRealmName(); + /** + * @return Client ID of the identity provider as set in provider realm. + */ + String getIDPClientIdInProviderRealm(SuiteContext suiteContext); + /** * @return User login name of the brokered user */ diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerConfiguration.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerConfiguration.java index aa3e56efe3..61664a9fc4 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerConfiguration.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerConfiguration.java @@ -50,6 +50,7 @@ public class KcOidcBrokerConfiguration implements BrokerConfiguration { public List createProviderClients(SuiteContext suiteContext) { ClientRepresentation client = new ClientRepresentation(); client.setId(CLIENT_ID); + client.setClientId(getIDPClientIdInProviderRealm(suiteContext)); client.setName(CLIENT_ID); client.setSecret(CLIENT_SECRET); client.setEnabled(true); @@ -123,6 +124,11 @@ public class KcOidcBrokerConfiguration implements BrokerConfiguration { return USER_LOGIN; } + @Override + public String getIDPClientIdInProviderRealm(SuiteContext suiteContext) { + return CLIENT_ID; + } + @Override public String getUserPassword() { return USER_PASSWORD; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerConfiguration.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerConfiguration.java index 7da71ad233..e5f5b8df81 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerConfiguration.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerConfiguration.java @@ -54,7 +54,7 @@ public class KcSamlBrokerConfiguration implements BrokerConfiguration { public List createProviderClients(SuiteContext suiteContext) { ClientRepresentation client = new ClientRepresentation(); - client.setClientId(getAuthRoot(suiteContext) + "/auth/realms/" + REALM_CONS_NAME); + client.setClientId(getIDPClientIdInProviderRealm(suiteContext)); client.setEnabled(true); client.setProtocol(IDP_SAML_PROVIDER_ID); client.setRedirectUris(Collections.singletonList( @@ -156,6 +156,11 @@ public class KcSamlBrokerConfiguration implements BrokerConfiguration { return REALM_CONS_NAME; } + @Override + public String getIDPClientIdInProviderRealm(SuiteContext suiteContext) { + return getAuthRoot(suiteContext) + "/auth/realms/" + consumerRealmName(); + } + @Override public String getUserLogin() { return USER_LOGIN; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java index cd315f3e09..b4825cdb38 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedBrokerTest.java @@ -15,8 +15,6 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { public static class KcSamlSignedBrokerConfiguration extends KcSamlBrokerConfiguration { - public static final KcSamlSignedBrokerConfiguration INSTANCE = new KcSamlSignedBrokerConfiguration(); - @Override public RealmRepresentation createProviderRealm() { RealmRepresentation realm = super.createProviderRealm(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientBuilder.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientBuilder.java index 9de5ae2bda..12fe4a1189 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientBuilder.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientBuilder.java @@ -61,6 +61,11 @@ public class ClientBuilder { return this; } + public ClientBuilder consentRequired(boolean consentRequired) { + rep.setConsentRequired(consentRequired); + return this; + } + public ClientBuilder publicClient() { rep.setPublicClient(true); return this;