From a3ccac2012feaf75d65ed4c8f02e2590f62e7f05 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Tue, 27 Jun 2017 09:27:36 +0200 Subject: [PATCH] KEYCLOAK-4377 --- .../keycloak/broker/saml/SAMLEndpoint.java | 17 +++- .../saml/SAMLIdentityProviderConfig.java | 84 +++++++++++-------- .../keycloak/protocol/saml/SamlProtocol.java | 4 +- .../protocol/saml/SamlProtocolUtils.java | 2 +- .../ClientAttributeUpdater.java | 2 +- .../IdentityProviderAttributeUpdater.java | 55 ++++++++++++ .../RealmAttributeUpdater.java | 2 +- .../{util => updaters}/SetSystemProperty.java | 2 +- .../AbstractSAMLServletsAdapterTest.java | 1 + .../testsuite/broker/AbstractBrokerTest.java | 27 +++--- .../broker/KcSamlBrokerConfiguration.java | 38 +++++---- .../broker/KcSamlSignedBrokerTest.java | 81 +++++++++++++++--- 12 files changed, 233 insertions(+), 82 deletions(-) rename testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/{util => updaters}/ClientAttributeUpdater.java (97%) create mode 100644 testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/IdentityProviderAttributeUpdater.java rename testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/{util => updaters}/RealmAttributeUpdater.java (97%) rename testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/{util => updaters}/SetSystemProperty.java (97%) 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 4451b8c6d8..ce5b77ffcd 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java @@ -54,7 +54,6 @@ import org.keycloak.saml.common.constants.JBossSAMLURIConstants; import org.keycloak.saml.common.exceptions.ConfigurationException; import org.keycloak.saml.common.exceptions.ProcessingException; import org.keycloak.saml.common.util.DocumentUtil; -import org.keycloak.saml.processing.api.saml.v2.sig.SAML2Signature; import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder; import org.keycloak.saml.processing.core.saml.v2.constants.X500SAMLProfileConstants; import org.keycloak.saml.processing.core.saml.v2.util.AssertionUtil; @@ -88,10 +87,13 @@ import java.util.List; import org.keycloak.rotation.HardcodedKeyLocator; import org.keycloak.rotation.KeyLocator; import org.keycloak.saml.processing.core.util.KeycloakKeySamlExtensionGenerator; -import org.w3c.dom.Document; +import org.keycloak.saml.processing.core.util.XMLEncryptionUtil; import org.w3c.dom.Element; import java.util.*; +import javax.xml.crypto.dsig.XMLSignature; +import org.w3c.dom.Document; +import org.w3c.dom.NodeList; /** * @author Bill Burke @@ -517,6 +519,17 @@ public class SAMLEndpoint { protected class PostBinding extends Binding { @Override protected void verifySignature(String key, SAMLDocumentHolder documentHolder) throws VerificationException { + NodeList nl = documentHolder.getSamlDocument().getElementsByTagNameNS(XMLSignature.XMLNS, "Signature"); + boolean anyElementSigned = (nl != null && nl.getLength() > 0); + if ((! anyElementSigned) && (documentHolder.getSamlObject() instanceof ResponseType)) { + ResponseType responseType = (ResponseType) documentHolder.getSamlObject(); + List assertions = responseType.getAssertions(); + if (! assertions.isEmpty() ) { + // Only relax verification if the response is an authnresponse and contains (encrypted/plaintext) assertion. + // In that case, signature is validated on assertion element + return; + } + } SamlProtocolUtils.verifyDocumentSignature(documentHolder.getSamlDocument(), getIDPKeyLocator()); } diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java index 4d200a00a5..c6d999c3e2 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java @@ -27,6 +27,24 @@ public class SAMLIdentityProviderConfig extends IdentityProviderModel { public static final XmlKeyInfoKeyNameTransformer DEFAULT_XML_KEY_INFO_KEY_NAME_TRANSFORMER = XmlKeyInfoKeyNameTransformer.NONE; + public static final String ADD_EXTENSIONS_ELEMENT_WITH_KEY_INFO = "addExtensionsElementWithKeyInfo"; + public static final String BACKCHANNEL_SUPPORTED = "backchannelSupported"; + public static final String ENCRYPTION_PUBLIC_KEY = "encryptionPublicKey"; + public static final String FORCE_AUTHN = "forceAuthn"; + public static final String NAME_ID_POLICY_FORMAT = "nameIDPolicyFormat"; + public static final String POST_BINDING_AUTHN_REQUEST = "postBindingAuthnRequest"; + public static final String POST_BINDING_LOGOUT = "postBindingLogout"; + public static final String POST_BINDING_RESPONSE = "postBindingResponse"; + public static final String SIGNATURE_ALGORITHM = "signatureAlgorithm"; + public static final String SIGNING_CERTIFICATE_KEY = "signingCertificate"; + public static final String SINGLE_LOGOUT_SERVICE_URL = "singleLogoutServiceUrl"; + public static final String SINGLE_SIGN_ON_SERVICE_URL = "singleSignOnServiceUrl"; + public static final String VALIDATE_SIGNATURE = "validateSignature"; + public static final String WANT_ASSERTIONS_ENCRYPTED = "wantAssertionsEncrypted"; + public static final String WANT_ASSERTIONS_SIGNED = "wantAssertionsSigned"; + public static final String WANT_AUTHN_REQUESTS_SIGNED = "wantAuthnRequestsSigned"; + public static final String XML_SIG_KEY_INFO_KEY_NAME_TRANSFORMER = "xmlSigKeyInfoKeyNameTransformer"; + public SAMLIdentityProviderConfig() { } @@ -35,35 +53,35 @@ public class SAMLIdentityProviderConfig extends IdentityProviderModel { } public String getSingleSignOnServiceUrl() { - return getConfig().get("singleSignOnServiceUrl"); + return getConfig().get(SINGLE_SIGN_ON_SERVICE_URL); } public void setSingleSignOnServiceUrl(String singleSignOnServiceUrl) { - getConfig().put("singleSignOnServiceUrl", singleSignOnServiceUrl); + getConfig().put(SINGLE_SIGN_ON_SERVICE_URL, singleSignOnServiceUrl); } public String getSingleLogoutServiceUrl() { - return getConfig().get("singleLogoutServiceUrl"); + return getConfig().get(SINGLE_LOGOUT_SERVICE_URL); } public void setSingleLogoutServiceUrl(String singleLogoutServiceUrl) { - getConfig().put("singleLogoutServiceUrl", singleLogoutServiceUrl); + getConfig().put(SINGLE_LOGOUT_SERVICE_URL, singleLogoutServiceUrl); } public boolean isValidateSignature() { - return Boolean.valueOf(getConfig().get("validateSignature")); + return Boolean.valueOf(getConfig().get(VALIDATE_SIGNATURE)); } public void setValidateSignature(boolean validateSignature) { - getConfig().put("validateSignature", String.valueOf(validateSignature)); + getConfig().put(VALIDATE_SIGNATURE, String.valueOf(validateSignature)); } public boolean isForceAuthn() { - return Boolean.valueOf(getConfig().get("forceAuthn")); + return Boolean.valueOf(getConfig().get(FORCE_AUTHN)); } public void setForceAuthn(boolean forceAuthn) { - getConfig().put("forceAuthn", String.valueOf(forceAuthn)); + getConfig().put(FORCE_AUTHN, String.valueOf(forceAuthn)); } /** @@ -103,82 +121,80 @@ public class SAMLIdentityProviderConfig extends IdentityProviderModel { return crt.split(","); } - public static final String SIGNING_CERTIFICATE_KEY = "signingCertificate"; - public String getNameIDPolicyFormat() { - return getConfig().get("nameIDPolicyFormat"); + return getConfig().get(NAME_ID_POLICY_FORMAT); } public void setNameIDPolicyFormat(String nameIDPolicyFormat) { - getConfig().put("nameIDPolicyFormat", nameIDPolicyFormat); + getConfig().put(NAME_ID_POLICY_FORMAT, nameIDPolicyFormat); } public boolean isWantAuthnRequestsSigned() { - return Boolean.valueOf(getConfig().get("wantAuthnRequestsSigned")); + return Boolean.valueOf(getConfig().get(WANT_AUTHN_REQUESTS_SIGNED)); } public void setWantAuthnRequestsSigned(boolean wantAuthnRequestsSigned) { - getConfig().put("wantAuthnRequestsSigned", String.valueOf(wantAuthnRequestsSigned)); + getConfig().put(WANT_AUTHN_REQUESTS_SIGNED, String.valueOf(wantAuthnRequestsSigned)); } public boolean isWantAssertionsSigned() { - return Boolean.valueOf(getConfig().get("wantAssertionsSigned")); + return Boolean.valueOf(getConfig().get(WANT_ASSERTIONS_SIGNED)); } public void setWantAssertionsSigned(boolean wantAssertionsSigned) { - getConfig().put("wantAssertionsSigned", String.valueOf(wantAssertionsSigned)); + getConfig().put(WANT_ASSERTIONS_SIGNED, String.valueOf(wantAssertionsSigned)); } public boolean isWantAssertionsEncrypted() { - return Boolean.valueOf(getConfig().get("wantAssertionsEncrypted")); + return Boolean.valueOf(getConfig().get(WANT_ASSERTIONS_ENCRYPTED)); } public void setWantAssertionsEncrypted(boolean wantAssertionsEncrypted) { - getConfig().put("wantAssertionsEncrypted", String.valueOf(wantAssertionsEncrypted)); + getConfig().put(WANT_ASSERTIONS_ENCRYPTED, String.valueOf(wantAssertionsEncrypted)); } public boolean isAddExtensionsElementWithKeyInfo() { - return Boolean.valueOf(getConfig().get("addExtensionsElementWithKeyInfo")); + return Boolean.valueOf(getConfig().get(ADD_EXTENSIONS_ELEMENT_WITH_KEY_INFO)); } public void setAddExtensionsElementWithKeyInfo(boolean addExtensionsElementWithKeyInfo) { - getConfig().put("addExtensionsElementWithKeyInfo", String.valueOf(addExtensionsElementWithKeyInfo)); + getConfig().put(ADD_EXTENSIONS_ELEMENT_WITH_KEY_INFO, String.valueOf(addExtensionsElementWithKeyInfo)); } public String getSignatureAlgorithm() { - return getConfig().get("signatureAlgorithm"); + return getConfig().get(SIGNATURE_ALGORITHM); } public void setSignatureAlgorithm(String signatureAlgorithm) { - getConfig().put("signatureAlgorithm", signatureAlgorithm); + getConfig().put(SIGNATURE_ALGORITHM, signatureAlgorithm); } public String getEncryptionPublicKey() { - return getConfig().get("encryptionPublicKey"); + return getConfig().get(ENCRYPTION_PUBLIC_KEY); } public void setEncryptionPublicKey(String encryptionPublicKey) { - getConfig().put("encryptionPublicKey", encryptionPublicKey); + getConfig().put(ENCRYPTION_PUBLIC_KEY, encryptionPublicKey); } public boolean isPostBindingAuthnRequest() { - return Boolean.valueOf(getConfig().get("postBindingAuthnRequest")); + return Boolean.valueOf(getConfig().get(POST_BINDING_AUTHN_REQUEST)); } public void setPostBindingAuthnRequest(boolean postBindingAuthnRequest) { - getConfig().put("postBindingAuthnRequest", String.valueOf(postBindingAuthnRequest)); + getConfig().put(POST_BINDING_AUTHN_REQUEST, String.valueOf(postBindingAuthnRequest)); } public boolean isPostBindingResponse() { - return Boolean.valueOf(getConfig().get("postBindingResponse")); + return Boolean.valueOf(getConfig().get(POST_BINDING_RESPONSE)); } public void setPostBindingResponse(boolean postBindingResponse) { - getConfig().put("postBindingResponse", String.valueOf(postBindingResponse)); + getConfig().put(POST_BINDING_RESPONSE, String.valueOf(postBindingResponse)); } public boolean isPostBindingLogout() { - String postBindingLogout = getConfig().get("postBindingLogout"); + String postBindingLogout = getConfig().get(POST_BINDING_LOGOUT); if (postBindingLogout == null) { // To maintain unchanged behavior when adding this field, we set the inital value to equal that // of the binding for the response: @@ -188,15 +204,15 @@ public class SAMLIdentityProviderConfig extends IdentityProviderModel { } public void setPostBindingLogout(boolean postBindingLogout) { - getConfig().put("postBindingLogout", String.valueOf(postBindingLogout)); + getConfig().put(POST_BINDING_LOGOUT, String.valueOf(postBindingLogout)); } public boolean isBackchannelSupported() { - return Boolean.valueOf(getConfig().get("backchannelSupported")); + return Boolean.valueOf(getConfig().get(BACKCHANNEL_SUPPORTED)); } public void setBackchannelSupported(boolean backchannel) { - getConfig().put("backchannelSupported", String.valueOf(backchannel)); + getConfig().put(BACKCHANNEL_SUPPORTED, String.valueOf(backchannel)); } /** @@ -204,11 +220,11 @@ public class SAMLIdentityProviderConfig extends IdentityProviderModel { * @return Configured ransformer of {@link #DEFAULT_XML_KEY_INFO_KEY_NAME_TRANSFORMER} if not set. */ public XmlKeyInfoKeyNameTransformer getXmlSigKeyInfoKeyNameTransformer() { - return XmlKeyInfoKeyNameTransformer.from(getConfig().get("xmlSigKeyInfoKeyNameTransformer"), DEFAULT_XML_KEY_INFO_KEY_NAME_TRANSFORMER); + return XmlKeyInfoKeyNameTransformer.from(getConfig().get(XML_SIG_KEY_INFO_KEY_NAME_TRANSFORMER), DEFAULT_XML_KEY_INFO_KEY_NAME_TRANSFORMER); } public void setXmlSigKeyInfoKeyNameTransformer(XmlKeyInfoKeyNameTransformer xmlSigKeyInfoKeyNameTransformer) { - getConfig().put("xmlSigKeyInfoKeyNameTransformer", + getConfig().put(XML_SIG_KEY_INFO_KEY_NAME_TRANSFORMER, xmlSigKeyInfoKeyNameTransformer == null ? null : xmlSigKeyInfoKeyNameTransformer.name()); 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 a8218c17ac..caa97095ef 100755 --- a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java +++ b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java @@ -193,7 +193,7 @@ public class SamlProtocol implements LoginProtocol { if (samlClient.requiresEncryption()) { PublicKey publicKey; try { - publicKey = SamlProtocolUtils.getEncryptionValidationKey(client); + publicKey = SamlProtocolUtils.getEncryptionKey(client); } catch (Exception e) { logger.error("failed", e); return ErrorPage.error(session, Messages.FAILED_TO_PROCESS_RESPONSE); @@ -457,7 +457,7 @@ public class SamlProtocol implements LoginProtocol { if (samlClient.requiresEncryption()) { PublicKey publicKey = null; try { - publicKey = SamlProtocolUtils.getEncryptionValidationKey(client); + publicKey = SamlProtocolUtils.getEncryptionKey(client); } catch (Exception e) { logger.error("failed", e); return ErrorPage.error(session, Messages.FAILED_TO_PROCESS_RESPONSE); diff --git a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java index 026a54a599..7ab97a4a99 100755 --- a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java +++ b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java @@ -103,7 +103,7 @@ public class SamlProtocolUtils { * @return Public key for encryption. * @throws VerificationException */ - public static PublicKey getEncryptionValidationKey(ClientModel client) throws VerificationException { + public static PublicKey getEncryptionKey(ClientModel client) throws VerificationException { return getPublicKey(client, SamlConfigAttributes.SAML_ENCRYPTION_CERTIFICATE_ATTRIBUTE); } diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/ClientAttributeUpdater.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java similarity index 97% rename from testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/ClientAttributeUpdater.java rename to testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java index d3effb9d7e..0272a1bd1a 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/ClientAttributeUpdater.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java @@ -1,4 +1,4 @@ -package org.keycloak.testsuite.util; +package org.keycloak.testsuite.updaters; import org.keycloak.admin.client.resource.ClientResource; import org.keycloak.representations.idm.ClientRepresentation; diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/IdentityProviderAttributeUpdater.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/IdentityProviderAttributeUpdater.java new file mode 100644 index 0000000000..b8eb487b1a --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/IdentityProviderAttributeUpdater.java @@ -0,0 +1,55 @@ +package org.keycloak.testsuite.updaters; + +import org.keycloak.admin.client.resource.IdentityProviderResource; +import org.keycloak.representations.idm.IdentityProviderRepresentation; +import java.io.Closeable; +import java.util.HashMap; +import java.util.Map; + +/** + * + * @author hmlnarik + */ +public class IdentityProviderAttributeUpdater { + + private final Map originalAttributes = new HashMap<>(); + + private final IdentityProviderResource identityProviderResource; + + private final IdentityProviderRepresentation rep; + + public IdentityProviderAttributeUpdater(IdentityProviderResource identityProviderResource) { + this.identityProviderResource = identityProviderResource; + this.rep = identityProviderResource.toRepresentation(); + if (this.rep.getConfig() == null) { + this.rep.setConfig(new HashMap<>()); + } + } + + public IdentityProviderAttributeUpdater setAttribute(String name, String value) { + if (! originalAttributes.containsKey(name)) { + this.originalAttributes.put(name, this.rep.getConfig().put(name, value)); + } else { + this.rep.getConfig().put(name, value); + } + return this; + } + + public IdentityProviderAttributeUpdater removeAttribute(String name) { + if (! originalAttributes.containsKey(name)) { + this.originalAttributes.put(name, this.rep.getConfig().put(name, null)); + } else { + this.rep.getConfig().put(name, null); + } + return this; + } + + public Closeable update() { + identityProviderResource.update(rep); + + return () -> { + rep.getConfig().putAll(originalAttributes); + identityProviderResource.update(rep); + }; + } +} diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/RealmAttributeUpdater.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/RealmAttributeUpdater.java similarity index 97% rename from testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/RealmAttributeUpdater.java rename to testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/RealmAttributeUpdater.java index 909bfcab18..72c2c4b184 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/RealmAttributeUpdater.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/RealmAttributeUpdater.java @@ -1,4 +1,4 @@ -package org.keycloak.testsuite.util; +package org.keycloak.testsuite.updaters; import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.representations.idm.RealmRepresentation; diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SetSystemProperty.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/SetSystemProperty.java similarity index 97% rename from testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SetSystemProperty.java rename to testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/SetSystemProperty.java index 757a385103..9a798b24b5 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SetSystemProperty.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/SetSystemProperty.java @@ -15,7 +15,7 @@ * limitations under the License. */ -package org.keycloak.testsuite.util; +package org.keycloak.testsuite.updaters; import java.io.Closeable; 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 420fad8c48..26c398ca7e 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 @@ -17,6 +17,7 @@ package org.keycloak.testsuite.adapter.servlet; +import org.keycloak.testsuite.updaters.ClientAttributeUpdater; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpUriRequest; 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 6500ad0ce1..6332dd0c78 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 @@ -1,6 +1,5 @@ package org.keycloak.testsuite.broker; -import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -14,7 +13,6 @@ import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.RoleRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.Assert; -import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.pages.ConsentPage; import org.keycloak.testsuite.util.*; @@ -40,6 +38,8 @@ import static org.keycloak.testsuite.broker.BrokerTestTools.*; public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { + protected IdentityProviderResource identityProviderResource; + @Before public void beforeBrokerTest() { log.debug("creating user for realm " + bc.providerRealmName()); @@ -61,7 +61,8 @@ public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { log.debug("adding identity provider to realm " + bc.consumerRealmName()); RealmResource realm = adminClient.realm(bc.consumerRealmName()); - realm.identityProviders().create(bc.setUpIdentityProvider(suiteContext)); + realm.identityProviders().create(bc.setUpIdentityProvider(suiteContext)).close(); + identityProviderResource = realm.identityProviders().get(bc.getIDPAlias()); // addClients List clients = bc.createProviderClients(suiteContext); @@ -70,7 +71,7 @@ public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { for (ClientRepresentation client : clients) { log.debug("adding client " + client.getName() + " to realm " + bc.providerRealmName()); - providerRealm.clients().create(client); + providerRealm.clients().create(client).close(); } } @@ -80,7 +81,7 @@ public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { for (ClientRepresentation client : clients) { log.debug("adding client " + client.getName() + " to realm " + bc.consumerRealmName()); - consumerRealm.clients().create(client); + consumerRealm.clients().create(client).close(); } } @@ -90,6 +91,12 @@ public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { @Test public void testLogInAsUserInIDP() { + loginUser(); + + testSingleLogout(); + } + + protected void loginUser() { driver.navigate().to(getAccountUrl(bc.consumerRealmName())); log.debug("Clicking social " + bc.getIDPAlias()); @@ -98,16 +105,16 @@ public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { waitForPage(driver, "log in to"); Assert.assertTrue("Driver should be on the provider realm page right now", - driver.getCurrentUrl().contains("/auth/realms/" + bc.providerRealmName() + "/")); + driver.getCurrentUrl().contains("/auth/realms/" + bc.providerRealmName() + "/")); log.debug("Logging in"); accountLoginPage.login(bc.getUserLogin(), bc.getUserPassword()); waitForPage(driver, "update account information"); - Assert.assertTrue(updateAccountInformationPage.isCurrent()); + updateAccountInformationPage.assertCurrent(); Assert.assertTrue("We must be on correct realm right now", - driver.getCurrentUrl().contains("/auth/realms/" + bc.consumerRealmName() + "/")); + driver.getCurrentUrl().contains("/auth/realms/" + bc.consumerRealmName() + "/")); log.debug("Updating info on updateAccount page"); updateAccountInformationPage.updateAccountInformation(bc.getUserLogin(), bc.getUserEmail(), "Firstname", "Lastname"); @@ -128,9 +135,7 @@ public abstract class AbstractBrokerTest extends AbstractBaseBrokerTest { } Assert.assertTrue("There must be user " + bc.getUserLogin() + " in realm " + bc.consumerRealmName(), - isUserFound); - - testSingleLogout(); + isUserFound); } @Test 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 e5f5b8df81..da0bc2bbcc 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 @@ -6,6 +6,7 @@ package org.keycloak.testsuite.broker; import org.keycloak.protocol.ProtocolMapperUtils; +import org.keycloak.protocol.saml.SamlConfigAttributes; import org.keycloak.protocol.saml.SamlProtocol; import org.keycloak.protocol.saml.mappers.AttributeStatementHelper; import org.keycloak.protocol.saml.mappers.UserAttributeStatementMapper; @@ -22,6 +23,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import static org.keycloak.broker.saml.SAMLIdentityProviderConfig.*; import static org.keycloak.testsuite.broker.BrokerTestConstants.*; import static org.keycloak.testsuite.broker.BrokerTestTools.*; @@ -63,17 +65,17 @@ public class KcSamlBrokerConfiguration implements BrokerConfiguration { Map attributes = new HashMap<>(); - attributes.put("saml.authnstatement", "true"); - attributes.put("saml_single_logout_service_url_post", + attributes.put(SamlConfigAttributes.SAML_AUTHNSTATEMENT, "true"); + attributes.put(SamlProtocol.SAML_SINGLE_LOGOUT_SERVICE_URL_POST_ATTRIBUTE, getAuthRoot(suiteContext) + "/auth/realms/" + REALM_CONS_NAME + "/broker/" + IDP_SAML_ALIAS + "/endpoint"); - attributes.put("saml_assertion_consumer_url_post", + attributes.put(SamlProtocol.SAML_ASSERTION_CONSUMER_URL_POST_ATTRIBUTE, getAuthRoot(suiteContext) + "/auth/realms/" + REALM_CONS_NAME + "/broker/" + IDP_SAML_ALIAS + "/endpoint"); - attributes.put("saml_force_name_id_format", "true"); - attributes.put("saml_name_id_format", "username"); - attributes.put("saml.assertion.signature", "false"); - attributes.put("saml.server.signature", "false"); - attributes.put("saml.client.signature", "false"); - attributes.put("saml.encrypt", "false"); + attributes.put(SamlConfigAttributes.SAML_FORCE_NAME_ID_FORMAT_ATTRIBUTE, "true"); + attributes.put(SamlConfigAttributes.SAML_NAME_ID_FORMAT_ATTRIBUTE, "username"); + attributes.put(SamlConfigAttributes.SAML_ASSERTION_SIGNATURE, "false"); + attributes.put(SamlConfigAttributes.SAML_SERVER_SIGNATURE, "false"); + attributes.put(SamlConfigAttributes.SAML_CLIENT_SIGNATURE_ATTRIBUTE, "false"); + attributes.put(SamlConfigAttributes.SAML_ENCRYPT, "false"); client.setAttributes(attributes); @@ -133,15 +135,15 @@ public class KcSamlBrokerConfiguration implements BrokerConfiguration { Map config = idp.getConfig(); - config.put("singleSignOnServiceUrl", getAuthRoot(suiteContext) + "/auth/realms/" + REALM_PROV_NAME + "/protocol/saml"); - config.put("singleLogoutServiceUrl", getAuthRoot(suiteContext) + "/auth/realms/" + REALM_PROV_NAME + "/protocol/saml"); - config.put("nameIDPolicyFormat", "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress"); - config.put("forceAuthn", "true"); - config.put("postBindingResponse", "true"); - config.put("postBindingAuthnRequest", "true"); - config.put("validateSignature", "false"); - config.put("wantAuthnRequestsSigned", "false"); - config.put("backchannelSupported", "true"); + config.put(SINGLE_SIGN_ON_SERVICE_URL, getAuthRoot(suiteContext) + "/auth/realms/" + REALM_PROV_NAME + "/protocol/saml"); + config.put(SINGLE_LOGOUT_SERVICE_URL, getAuthRoot(suiteContext) + "/auth/realms/" + REALM_PROV_NAME + "/protocol/saml"); + config.put(NAME_ID_POLICY_FORMAT, "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress"); + config.put(FORCE_AUTHN, "true"); + config.put(POST_BINDING_RESPONSE, "true"); + config.put(POST_BINDING_AUTHN_REQUEST, "true"); + config.put(VALIDATE_SIGNATURE, "false"); + config.put(WANT_AUTHN_REQUESTS_SIGNED, "false"); + config.put(BACKCHANNEL_SUPPORTED, "true"); return idp; } 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 b4825cdb38..ef23a9a8e9 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 @@ -1,19 +1,29 @@ package org.keycloak.testsuite.broker; +import org.keycloak.admin.client.resource.ClientResource; +import org.keycloak.broker.saml.SAMLIdentityProviderConfig; +import org.keycloak.protocol.saml.SamlConfigAttributes; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.IdentityProviderRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.testsuite.arquillian.SuiteContext; +import org.keycloak.testsuite.updaters.ClientAttributeUpdater; +import org.keycloak.testsuite.updaters.IdentityProviderAttributeUpdater; +import java.io.Closeable; import java.util.HashMap; import java.util.List; import java.util.Map; +import org.hamcrest.Matchers; +import org.junit.Assert; +import org.junit.Test; import static org.keycloak.testsuite.broker.BrokerTestConstants.*; +import static org.keycloak.testsuite.broker.BrokerTestTools.encodeUrl; public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { - public static class KcSamlSignedBrokerConfiguration extends KcSamlBrokerConfiguration { + public class KcSamlSignedBrokerConfiguration extends KcSamlBrokerConfiguration { @Override public RealmRepresentation createProviderRealm() { @@ -39,6 +49,9 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { public List createProviderClients(SuiteContext suiteContext) { List clientRepresentationList = super.createProviderClients(suiteContext); + String consumerCert = adminClient.realm(consumerRealmName()).keys().getKeyMetadata().getKeys().get(0).getCertificate(); + Assert.assertThat(consumerCert, Matchers.notNullValue()); + for (ClientRepresentation client : clientRepresentationList) { client.setClientAuthenticatorType("client-secret"); client.setSurrogateAuthRequired(false); @@ -49,12 +62,11 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { client.setAttributes(attributes); } - attributes.put("saml.assertion.signature", "true"); - attributes.put("saml.server.signature", "true"); - attributes.put("saml.client.signature", "true"); - attributes.put("saml.signature.algorithm", "RSA_SHA256"); - attributes.put("saml.signing.private.key", IDP_SAML_SIGN_KEY); - attributes.put("saml.signing.certificate", IDP_SAML_SIGN_CERT); + attributes.put(SamlConfigAttributes.SAML_ASSERTION_SIGNATURE, "true"); + attributes.put(SamlConfigAttributes.SAML_SERVER_SIGNATURE, "true"); + attributes.put(SamlConfigAttributes.SAML_CLIENT_SIGNATURE_ATTRIBUTE, "true"); + attributes.put(SamlConfigAttributes.SAML_SIGNATURE_ALGORITHM, "RSA_SHA256"); + attributes.put(SamlConfigAttributes.SAML_SIGNING_CERTIFICATE_ATTRIBUTE, consumerCert); } return clientRepresentationList; @@ -64,11 +76,15 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { public IdentityProviderRepresentation setUpIdentityProvider(SuiteContext suiteContext) { IdentityProviderRepresentation result = super.setUpIdentityProvider(suiteContext); + String providerCert = adminClient.realm(providerRealmName()).keys().getKeyMetadata().getKeys().get(0).getCertificate(); + Assert.assertThat(providerCert, Matchers.notNullValue()); + Map config = result.getConfig(); - config.put("validateSignature", "true"); - config.put("wantAuthnRequestsSigned", "true"); - config.put("signingCertificate", IDP_SAML_SIGN_CERT); + config.put(SAMLIdentityProviderConfig.VALIDATE_SIGNATURE, "true"); + config.put(SAMLIdentityProviderConfig.WANT_ASSERTIONS_SIGNED, "true"); + config.put(SAMLIdentityProviderConfig.WANT_AUTHN_REQUESTS_SIGNED, "true"); + config.put(SAMLIdentityProviderConfig.SIGNING_CERTIFICATE_KEY, providerCert); return result; } @@ -76,7 +92,50 @@ public class KcSamlSignedBrokerTest extends KcSamlBrokerTest { @Override protected BrokerConfiguration getBrokerConfiguration() { - return KcSamlSignedBrokerConfiguration.INSTANCE; + return new KcSamlSignedBrokerConfiguration(); } + @Test + public void testSignedEncryptedAssertions() throws Exception { + ClientRepresentation client = adminClient.realm(bc.providerRealmName()) + .clients() + .findByClientId(bc.getIDPClientIdInProviderRealm(suiteContext)) + .get(0); + + final ClientResource clientResource = realmsResouce().realm(bc.providerRealmName()).clients().get(client.getId()); + Assert.assertThat(clientResource, Matchers.notNullValue()); + + String providerCert = adminClient.realm(bc.providerRealmName()).keys().getKeyMetadata().getKeys().get(0).getCertificate(); + Assert.assertThat(providerCert, Matchers.notNullValue()); + + String consumerCert = adminClient.realm(bc.consumerRealmName()).keys().getKeyMetadata().getKeys().get(0).getCertificate(); + 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.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_ENCRYPTION_CERTIFICATE_ATTRIBUTE, consumerCert) + .setAttribute(SamlConfigAttributes.SAML_SERVER_SIGNATURE, "false") // only sign assertions + .setAttribute(SamlConfigAttributes.SAML_ASSERTION_SIGNATURE, "true") + .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(); + } + } }