From 6b968796cea9b5ef57061869887fb337e75b8642 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Wed, 20 Jun 2018 13:55:51 +0200 Subject: [PATCH] KEYCLOAK-7667 Fix namespace handling when decrypting assertion --- .../AbstractSamlAuthenticationHandler.java | 2 +- .../core/saml/v2/util/AssertionUtil.java | 11 ++- .../core/parsers/saml/SAMLParserTest.java | 10 ++- .../keycloak/broker/saml/SAMLEndpoint.java | 2 +- .../testsuite/broker/AbstractBrokerTest.java | 4 ++ .../broker/KcSamlSignedBrokerTest.java | 70 +++++++++++++------ 6 files changed, 69 insertions(+), 30 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 12cb924868..be7de79443 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 @@ -339,7 +339,7 @@ public abstract class AbstractSamlAuthenticationHandler implements SamlAuthentic return AuthOutcome.FAILED; } try { - assertion = AssertionUtil.getAssertion(responseType, deployment.getDecryptionKey()); + assertion = AssertionUtil.getAssertion(responseHolder, responseType, deployment.getDecryptionKey()); if (AssertionUtil.hasExpired(assertion)) { return initiateLogin(); } diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/util/AssertionUtil.java b/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/util/AssertionUtil.java index 0de9c86edd..887df88aa5 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/util/AssertionUtil.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/util/AssertionUtil.java @@ -67,6 +67,7 @@ import java.util.Set; import org.keycloak.rotation.HardcodedKeyLocator; import org.keycloak.saml.common.constants.GeneralConstants; +import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder; /** * Utility to deal with assertions @@ -552,7 +553,7 @@ public class AssertionUtil { return roles; } - public static AssertionType getAssertion(ResponseType responseType, PrivateKey privateKey) throws ParsingException, ProcessingException, ConfigurationException { + public static AssertionType getAssertion(SAMLDocumentHolder holder, ResponseType responseType, PrivateKey privateKey) throws ParsingException, ProcessingException, ConfigurationException { List assertions = responseType.getAssertions(); if (assertions.isEmpty()) { @@ -566,7 +567,7 @@ public class AssertionUtil { if (privateKey == null) { throw new ProcessingException("Encryptd assertion and decrypt private key is null"); } - decryptAssertion(responseType, privateKey); + decryptAssertion(holder, responseType, privateKey); } return responseType.getAssertions().get(0).getAssertion(); @@ -588,10 +589,8 @@ public class AssertionUtil { * @param responseType a response containg an encrypted assertion * @return the assertion element as it was decrypted. This can be used in signature verification. */ - public static Element decryptAssertion(ResponseType responseType, PrivateKey privateKey) throws ParsingException, ProcessingException, ConfigurationException { - SAML2Response saml2Response = new SAML2Response(); - - Document doc = saml2Response.convert(responseType); + public static Element decryptAssertion(SAMLDocumentHolder holder, ResponseType responseType, PrivateKey privateKey) throws ParsingException, ProcessingException, ConfigurationException { + Document doc = holder.getSamlDocument(); Element enc = DocumentUtil.getElement(doc, new QName(JBossSAMLConstants.ENCRYPTED_ASSERTION.get())); if (enc == null) { diff --git a/saml-core/src/test/java/org/keycloak/saml/processing/core/parsers/saml/SAMLParserTest.java b/saml-core/src/test/java/org/keycloak/saml/processing/core/parsers/saml/SAMLParserTest.java index c54bbeaa2d..870ec5139e 100644 --- a/saml-core/src/test/java/org/keycloak/saml/processing/core/parsers/saml/SAMLParserTest.java +++ b/saml-core/src/test/java/org/keycloak/saml/processing/core/parsers/saml/SAMLParserTest.java @@ -67,7 +67,9 @@ import org.keycloak.saml.common.constants.JBossSAMLURIConstants; import org.keycloak.saml.common.exceptions.ConfigurationException; import org.keycloak.saml.common.exceptions.ParsingException; import org.keycloak.saml.common.exceptions.ProcessingException; +import org.keycloak.saml.processing.api.saml.v2.request.SAML2Request; import org.keycloak.saml.processing.api.saml.v2.response.SAML2Response; +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.core.saml.v2.util.XMLTimeUtil; import org.w3c.dom.Element; @@ -137,6 +139,8 @@ public class SAMLParserTest { Object parsedObject; if (SAML2Object.class.isAssignableFrom(expectedType)) { parsedObject = new SAML2Response().getSAML2ObjectFromStream(st); + } else if (SAMLDocumentHolder.class.isAssignableFrom(expectedType)) { + parsedObject = SAML2Request.getSAML2ObjectFromStream(st); } else { parsedObject = parser.parse(st); } @@ -185,7 +189,9 @@ public class SAMLParserTest { @Test public void testSaml20EncryptedAssertionWithNewlines() throws Exception { - ResponseType resp = assertParsed("KEYCLOAK-4489-encrypted-assertion-with-newlines.xml", ResponseType.class); + SAMLDocumentHolder holder = assertParsed("KEYCLOAK-4489-encrypted-assertion-with-newlines.xml", SAMLDocumentHolder.class); + assertThat(holder.getSamlObject(), instanceOf(ResponseType.class)); + ResponseType resp = (ResponseType) holder.getSamlObject(); assertThat(resp.getAssertions().size(), is(1)); ResponseType.RTChoiceType rtChoiceType = resp.getAssertions().get(0); @@ -193,7 +199,7 @@ public class SAMLParserTest { assertNotNull(rtChoiceType.getEncryptedAssertion()); PrivateKey privateKey = DerUtils.decodePrivateKey(Base64.decode(PRIVATE_KEY)); - AssertionUtil.decryptAssertion(resp, privateKey); + AssertionUtil.decryptAssertion(holder, resp, privateKey); rtChoiceType = resp.getAssertions().get(0); assertNotNull(rtChoiceType.getAssertion()); 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 e25d8ca57a..c8b8cd4542 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java @@ -366,7 +366,7 @@ public class SAMLEndpoint { if (assertionIsEncrypted) { // This methods writes the parsed and decrypted assertion back on the responseType parameter: - assertionElement = AssertionUtil.decryptAssertion(responseType, keys.getPrivateKey()); + assertionElement = AssertionUtil.decryptAssertion(holder, responseType, keys.getPrivateKey()); } else { /* We verify the assertion using original document to handle cases where the IdP includes whitespace and/or newlines inside tags. */ 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 ab5ff26015..f9f141d265 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 @@ -65,6 +65,9 @@ public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { resetUserPassword(realmResource.users().get(userId), bc.getUserPassword(), false); if (testContext.isInitialized()) { + if (identityProviderResource == null) { + identityProviderResource = (IdentityProviderResource) testContext.getCustomValue("identityProviderResource"); + } return; } @@ -72,6 +75,7 @@ public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { RealmResource realm = adminClient.realm(bc.consumerRealmName()); realm.identityProviders().create(bc.setUpIdentityProvider(suiteContext)).close(); identityProviderResource = realm.identityProviders().get(bc.getIDPAlias()); + testContext.setCustomValue("identityProviderResource", identityProviderResource); // addClients List clients = bc.createProviderClients(suiteContext); 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 f53e90fa2e..e447998b86 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 @@ -36,7 +36,6 @@ import org.w3c.dom.NamedNodeMap; import org.w3c.dom.Node; import org.w3c.dom.NodeList; import static org.keycloak.testsuite.broker.BrokerTestConstants.*; -import static org.keycloak.testsuite.broker.BrokerTestTools.encodeUrl; import static org.keycloak.testsuite.util.Matchers.isSamlResponse; public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { @@ -113,8 +112,7 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { return new KcSamlSignedBrokerConfiguration(); } - @Test - public void testSignedEncryptedAssertions() throws Exception { + public void withSignedEncryptedAssertions(Runnable testBody, boolean signedAssertion, boolean encryptedAssertion) throws Exception { ClientRepresentation client = adminClient.realm(bc.providerRealmName()) .clients() .findByClientId(bc.getIDPClientIdInProviderRealm(suiteContext)) @@ -130,33 +128,46 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { Assert.assertThat(consumerCert, Matchers.notNullValue()); try (Closeable idpUpdater = new IdentityProviderAttributeUpdater(identityProviderResource) - .setAttribute(SAMLIdentityProviderConfig.VALIDATE_SIGNATURE, "true") - .setAttribute(SAMLIdentityProviderConfig.WANT_ASSERTIONS_SIGNED, "true") - .setAttribute(SAMLIdentityProviderConfig.WANT_ASSERTIONS_ENCRYPTED, "true") + .setAttribute(SAMLIdentityProviderConfig.VALIDATE_SIGNATURE, Boolean.toString(signedAssertion)) + .setAttribute(SAMLIdentityProviderConfig.WANT_ASSERTIONS_SIGNED, Boolean.toString(signedAssertion)) + .setAttribute(SAMLIdentityProviderConfig.WANT_ASSERTIONS_ENCRYPTED, Boolean.toString(encryptedAssertion)) .setAttribute(SAMLIdentityProviderConfig.WANT_AUTHN_REQUESTS_SIGNED, "false") .setAttribute(SAMLIdentityProviderConfig.SIGNING_CERTIFICATE_KEY, providerCert) .update(); Closeable clientUpdater = new ClientAttributeUpdater(clientResource) - .setAttribute(SamlConfigAttributes.SAML_ENCRYPT, "true") + .setAttribute(SamlConfigAttributes.SAML_ENCRYPT, Boolean.toString(encryptedAssertion)) .setAttribute(SamlConfigAttributes.SAML_ENCRYPTION_CERTIFICATE_ATTRIBUTE, consumerCert) .setAttribute(SamlConfigAttributes.SAML_SERVER_SIGNATURE, "false") // only sign assertions - .setAttribute(SamlConfigAttributes.SAML_ASSERTION_SIGNATURE, "true") + .setAttribute(SamlConfigAttributes.SAML_ASSERTION_SIGNATURE, Boolean.toString(signedAssertion)) .setAttribute(SamlConfigAttributes.SAML_CLIENT_SIGNATURE_ATTRIBUTE, "false") .update()) { - // Login should pass because assertion is signed. - loginUser(); - - // Logout should fail because logout response is not signed. - driver.navigate().to(BrokerTestTools.getAuthRoot(suiteContext) - + "/auth/realms/" + bc.providerRealmName() - + "/protocol/" + "openid-connect" - + "/logout?redirect_uri=" + encodeUrl(getAccountUrl(bc.providerRealmName()))); - - errorPage.assertCurrent(); + testBody.run(); } } + @Test + public void testSignedEncryptedAssertions() throws Exception { + withSignedEncryptedAssertions(this::testAssertionSignatureRespected, true, true); + } + + @Test + public void testSignedAssertion() throws Exception { + withSignedEncryptedAssertions(this::testAssertionSignatureRespected, true, false); + } + + private void testAssertionSignatureRespected() { + // Login should pass because assertion is signed. + loginUser(); + + // Logout should fail because logout response is not signed. + final String redirectUri = getAccountUrl(bc.providerRealmName()); + final String logoutUri = oauth.realm(bc.providerRealmName()).getLogoutUrl().redirectUri(redirectUri).build(); + driver.navigate().to(logoutUri); + + errorPage.assertCurrent(); + } + private Document extractNamespacesToTopLevelElement(Document original) { HashMap namespaces = new HashMap<>(); enumerateAndRemoveNamespaces(original.getDocumentElement(), namespaces); @@ -202,10 +213,15 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { // KEYCLOAK-5581 @Test - public void loginUserAllNamespacesInTopElement() throws Exception { + public void loginUserAllNamespacesInTopElement() { AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(AbstractSamlTest.SAML_CLIENT_ID_SALES_POST, AbstractSamlTest.SAML_ASSERTION_CONSUMER_URL_SALES_POST, null); - Document doc = extractNamespacesToTopLevelElement(SAML2Request.convert(loginRep)); + Document doc; + try { + doc = extractNamespacesToTopLevelElement(SAML2Request.convert(loginRep)); + } catch (Exception ex) { + throw new RuntimeException(ex); + } SAMLDocumentHolder samlResponse = new SamlClientBuilder() .authnRequest(getAuthServerSamlEndpoint(bc.consumerRealmName()), doc, Binding.POST).build() // Request to consumer IdP @@ -232,4 +248,18 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { Assert.assertThat(samlResponse.getSamlObject(), isSamlResponse(JBossSAMLURIConstants.STATUS_SUCCESS)); } + @Test + public void loginUserAllNamespacesInTopElementSignedEncryptedAssertion() throws Exception { + withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, true, true); + } + + @Test + public void loginUserAllNamespacesInTopElementSignedAssertion() throws Exception { + withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, true, false); + } + + @Test + public void loginUserAllNamespacesInTopElementEncryptedAssertion() throws Exception { + withSignedEncryptedAssertions(this::loginUserAllNamespacesInTopElement, false, true); + } }