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