From 3d611abc8f35aa31098e3d8994a55386ee3d61a2 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Thu, 13 Feb 2025 16:54:40 +0100 Subject: [PATCH] Signature tests for the SAML broker with artifact binding Signed-off-by: rmartinc --- .../keycloak/broker/saml/SAMLEndpoint.java | 25 +++ .../KcSamlBrokerArtifactBindingTest.java | 165 ++++++++++++++++-- 2 files changed, 172 insertions(+), 18 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 1f26507a2fa..10b7660f4fb 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java @@ -903,15 +903,40 @@ public class SAMLEndpoint { } protected class ArtifactBinding extends Binding { + + private boolean unencryptedSignaturesVerified = false; + @Override protected boolean containsUnencryptedSignature(SAMLDocumentHolder documentHolder) { + if (unencryptedSignaturesVerified) { + return true; + } NodeList nl = documentHolder.getSamlDocument().getElementsByTagNameNS(XMLSignature.XMLNS, "Signature"); return (nl != null && nl.getLength() > 0); } @Override protected void verifySignature(String key, SAMLDocumentHolder documentHolder) throws VerificationException { + if (unencryptedSignaturesVerified) { + // this is the second pass and signatures were already verified in the artifact response time + return; + } + if (!containsUnencryptedSignature(documentHolder)) { + List assertions = null; + if (documentHolder.getSamlObject() instanceof ResponseType responseType) { + assertions = responseType.getAssertions(); + } else if (documentHolder.getSamlObject() instanceof ArtifactResponseType artifactResponseType + && artifactResponseType.getAny() instanceof ResponseType responseType) { + assertions = responseType.getAssertions(); + } + if (assertions != null && !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()); + unencryptedSignaturesVerified = true; // mark signatures as verified } @Override diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerArtifactBindingTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerArtifactBindingTest.java index 6ea838fed8c..7d4cca95c14 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerArtifactBindingTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerArtifactBindingTest.java @@ -1,11 +1,17 @@ package org.keycloak.testsuite.broker; +import java.io.Closeable; +import java.io.IOException; +import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; import org.junit.Test; -import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.broker.saml.SAMLIdentityProviderConfig; +import org.keycloak.crypto.Algorithm; import org.keycloak.protocol.saml.SamlConfigAttributes; -import org.keycloak.representations.idm.ClientRepresentation; -import org.keycloak.representations.idm.IdentityProviderRepresentation; +import org.keycloak.testsuite.Assert; +import org.keycloak.testsuite.updaters.ClientAttributeUpdater; +import org.keycloak.testsuite.updaters.IdentityProviderAttributeUpdater; +import org.keycloak.testsuite.util.KeyUtils; public final class KcSamlBrokerArtifactBindingTest extends AbstractInitializedBaseBrokerTest { @@ -14,28 +20,151 @@ public final class KcSamlBrokerArtifactBindingTest extends AbstractInitializedBa return KcSamlBrokerConfiguration.INSTANCE; } + @Test + public void testLoginNoSignatures() throws Exception { + try (Closeable idpUpdater = new IdentityProviderAttributeUpdater(identityProviderResource) + .setAttribute(SAMLIdentityProviderConfig.ARTIFACT_RESOLUTION_SERVICE_URL, + BrokerTestTools.getProviderRoot() + "/auth/realms/" + bc.providerRealmName() + "/protocol/saml/resolve") + .setAttribute(SAMLIdentityProviderConfig.ARTIFACT_BINDING_RESPONSE, Boolean.TRUE.toString()) + .update(); + Closeable clientUpdater = ClientAttributeUpdater.forClient(adminClient, bc.providerRealmName(), bc.getIDPClientIdInProviderRealm()) + .setAttribute(SamlConfigAttributes.SAML_ARTIFACT_BINDING, Boolean.TRUE.toString()) + .update()) { + login(true); + } + } @Test - public void testLogin() { - // configure artifact binding to the broker - IdentityProviderRepresentation idpRep = identityProviderResource.toRepresentation(); - String baseSamlUrl = idpRep.getConfig().get(SAMLIdentityProviderConfig.ARTIFACT_RESOLUTION_SERVICE_URL); - idpRep.getConfig().put(SAMLIdentityProviderConfig.ARTIFACT_RESOLUTION_SERVICE_URL, baseSamlUrl + "/resolve"); - idpRep.getConfig().put(SAMLIdentityProviderConfig.ARTIFACT_BINDING_RESPONSE, Boolean.TRUE.toString()); - identityProviderResource.update(idpRep); + public void testLoginWithSignatureAtResponseLevelSuccess() throws Exception { + // success just with signature at response level + performTestSuccess(true, false, false, + false, false); + } - // configure artifact binding to the broker client - RealmResource providerRealm = realmsResouce().realm(bc.providerRealmName()); - ClientRepresentation brokerClient = providerRealm.clients().findByClientId(bc.getIDPClientIdInProviderRealm()).get(0); - brokerClient.getAttributes().put(SamlConfigAttributes.SAML_ARTIFACT_BINDING, Boolean.TRUE.toString()); - providerRealm.clients().get(brokerClient.getId()).update(brokerClient); + @Test + public void testLoginErrorNoSignature() throws Exception { + // failure with no signature at all + performTestFailure(false, false, false, + false, false); + } + @Test + public void testLoginWithSignatureAtResponseLevelErrorInvalidSignature() throws Exception { + // failure with invalid signature at response level + performTestFailureInvalidCert(true, false, false, + false, false); + } + + @Test + public void testLoginWithSignatureAtAssertionLevelSuccess() throws Exception { + // success with signature at assertion level + performTestSuccess(false, true, false, + false, false); + } + + @Test + public void testLoginWithSignatureAtAssertionLevelWantAssertionSignatureSuccess() throws Exception { + // success with signature at assertion level and wanting assertion signed + performTestSuccess(false, true, false, + true, false); + } + + @Test + public void testLoginWithSignatureAtAssertionLevelErrorOnlySignatureAtResponseLevel() throws Exception { + // failure when signature in response level only but wanting assertion signed + performTestFailure(true, false, false, + true, false); + } + + @Test + public void testLoginWithSignatureAtAssertionLevelErrorInvalidSignature() throws Exception { + // failure when assertion level signature with invalid cert + performTestFailureInvalidCert(false, true, false, + false, false); + } + + @Test + public void testLoginWithSignatureAndEncryptionAtAssertionLevelSuccess() throws Exception { + // success with assertion encrypted and signed + performTestSuccess(false, true, true, + true, true); + } + + @Test + public void testLoginWithOnlyEncryptionAtAssertionLevelErrorNoSignature() throws Exception { + // failure with assertion encrypted but not signed + performTestFailure(false, false, true, + false, false); + } + + private void performTestSuccess(boolean providerSignsResponse, boolean providerSignsAssertion, boolean providerEncryptsAssertion, + boolean consumerWantsAssertionSigned, boolean consumerWantsAssertionEncrypted) throws IOException { + performTest(providerSignsResponse, providerSignsAssertion, providerEncryptsAssertion, + consumerWantsAssertionSigned, consumerWantsAssertionEncrypted, + false, true); + } + + private void performTestFailure(boolean providerSignsResponse, boolean providerSignsAssertion, boolean providerEncryptsAssertion, + boolean consumerWantsAssertionSigned, boolean consumerWantsAssertionEncrypted) throws IOException { + performTest(providerSignsResponse, providerSignsAssertion, providerEncryptsAssertion, + consumerWantsAssertionSigned, consumerWantsAssertionEncrypted, + false, false); + } + + private void performTestFailureInvalidCert(boolean providerSignsResponse, boolean providerSignsAssertion, boolean providerEncryptsAssertion, + boolean consumerWantsAssertionSigned, boolean consumerWantsAssertionEncrypted) throws IOException { + performTest(providerSignsResponse, providerSignsAssertion, providerEncryptsAssertion, + consumerWantsAssertionSigned, consumerWantsAssertionEncrypted, + true, false); + } + + private void performTest(boolean providerSignsResponse, boolean providerSignsAssertion, boolean providerEncryptsAssertion, + boolean consumerWantsAssertionSigned, boolean consumerWantsAssertionEncrypted, + boolean incorrectSigningKey, boolean success) throws IOException { + String consumerSigCert = KeyUtils.findActiveSigningKey(adminClient.realm(bc.consumerRealmName()), Algorithm.RS256).getCertificate(); + MatcherAssert.assertThat(consumerSigCert, Matchers.notNullValue()); + String consumerEncCert = KeyUtils.findActiveEncryptingKey(adminClient.realm(bc.consumerRealmName()), Algorithm.RSA_OAEP).getCertificate(); + MatcherAssert.assertThat(consumerEncCert, Matchers.notNullValue()); + + try (Closeable idpUpdater = new IdentityProviderAttributeUpdater(identityProviderResource) + .setAttribute(SAMLIdentityProviderConfig.ARTIFACT_RESOLUTION_SERVICE_URL, + BrokerTestTools.getProviderRoot() + "/auth/realms/" + bc.providerRealmName() + "/protocol/saml/resolve") + .setAttribute(SAMLIdentityProviderConfig.ARTIFACT_BINDING_RESPONSE, Boolean.TRUE.toString()) + .setAttribute(SAMLIdentityProviderConfig.WANT_AUTHN_REQUESTS_SIGNED, Boolean.TRUE.toString()) // always sign requests + .setAttribute(SAMLIdentityProviderConfig.VALIDATE_SIGNATURE, Boolean.TRUE.toString()) // always validate signatures + .setAttribute(SAMLIdentityProviderConfig.WANT_ASSERTIONS_SIGNED, Boolean.toString(consumerWantsAssertionSigned)) + .setAttribute(SAMLIdentityProviderConfig.WANT_ASSERTIONS_ENCRYPTED, Boolean.toString(consumerWantsAssertionEncrypted)) + .setAttribute(SAMLIdentityProviderConfig.METADATA_DESCRIPTOR_URL, + BrokerTestTools.getProviderRoot() + "/auth/realms/" + bc.providerRealmName() + "/protocol/saml/descriptor") + .setAttribute(SAMLIdentityProviderConfig.USE_METADATA_DESCRIPTOR_URL, Boolean.toString(!incorrectSigningKey)) // use metadata only if correct cert + .setAttribute(SAMLIdentityProviderConfig.SIGNING_CERTIFICATE_KEY, consumerSigCert) + .update(); + Closeable clientUpdater = ClientAttributeUpdater.forClient(adminClient, bc.providerRealmName(), bc.getIDPClientIdInProviderRealm()) + .setAttribute(SamlConfigAttributes.SAML_ARTIFACT_BINDING, Boolean.TRUE.toString()) + .setAttribute(SamlConfigAttributes.SAML_SIGNING_CERTIFICATE_ATTRIBUTE, consumerSigCert) + .setAttribute(SamlConfigAttributes.SAML_CLIENT_SIGNATURE_ATTRIBUTE, Boolean.TRUE.toString()) + .setAttribute(SamlConfigAttributes.SAML_SERVER_SIGNATURE, Boolean.toString(providerSignsResponse)) + .setAttribute(SamlConfigAttributes.SAML_ASSERTION_SIGNATURE, Boolean.toString(providerSignsAssertion)) + .setAttribute(SamlConfigAttributes.SAML_ENCRYPT, Boolean.toString(providerEncryptsAssertion)) + .setAttribute(SamlConfigAttributes.SAML_ENCRYPTION_CERTIFICATE_ATTRIBUTE, consumerEncCert) + .update()) { + login(success); + } + } + + private void login(boolean success) { // login using artifact binding oauth.clientId("broker-app"); loginPage.open(bc.consumerRealmName()); logInWithBroker(bc); - updateAccountInformationPage.assertCurrent(); - updateAccountInformationPage.updateAccountInformation("f", "l"); - appPage.assertCurrent(); + + if (success) { + updateAccountInformationPage.assertCurrent(); + updateAccountInformationPage.updateAccountInformation("f", "l"); + appPage.assertCurrent(); + } else { + errorPage.assertCurrent(); + Assert.assertEquals("Invalid signature in response from identity provider.", errorPage.getError()); + } } }