Merge pull request #4118 from skjolber/feature/KEYCLOAK-3056-verify-signature-2

Some adjustments for KEYCLOAK-3056 / PR #3893

saml-core/src/main/java/org/keycloak/saml/common/util/DocumentUtil.java

@@ -52,6 +52,7 @@ import java.io.InputStream;
 import java.io.Reader;
 import java.io.StringReader;
 import java.io.StringWriter;
+import java.util.Objects;
 
 /**
  * Utility dealing with DOM
@@ -554,4 +555,33 @@ public class DocumentUtil {
 
         return documentBuilderFactory;
     }
+
+    /**
+     * Get a (direct) child {@linkplain Element} from the parent {@linkplain Element}. 
+     *
+     * @param parent parent element
+     * @param targetNamespace namespace URI
+     * @param targetLocalName local name
+     * @return a child element matching the target namespace and localname, where {@linkplain Element#getParentNode()} is the parent input parameter
+     * @return
+     */
+    
+    public static Element getDirectChildElement(Element parent, String targetNamespace, String targetLocalName) {
+        Node child = parent.getFirstChild();
+        
+        while(child != null) {
+            if(child instanceof Element) {
+                Element childElement = (Element)child;
+                
+                String ns = childElement.getNamespaceURI();
+                String localName = childElement.getLocalName();
+                
+                if(Objects.equals(targetNamespace, ns) && Objects.equals(targetLocalName, localName)) {
+                    return childElement;
+                }
+            }
+            child = child.getNextSibling();
+        }
+        return null;
+    }
 }

saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/sig/SAML2Signature.java

@@ -49,8 +49,6 @@ public class SAML2Signature {
 
     private static final PicketLinkLogger logger = PicketLinkLoggerFactory.getLogger();
 
-    private static final String ID_ATTRIBUTE_NAME = "ID";
-
     private String signatureMethod = SignatureMethod.RSA_SHA1;
 
     private String digestMethod = DigestMethod.SHA1;
@@ -156,7 +154,7 @@ public class SAML2Signature {
      */
     public void signSAMLDocument(Document samlDocument, String keyName, KeyPair keypair, String canonicalizationMethodType) throws ProcessingException {
         // Get the ID from the root
-        String id = samlDocument.getDocumentElement().getAttribute(ID_ATTRIBUTE_NAME);
+        String id = samlDocument.getDocumentElement().getAttribute(JBossSAMLConstants.ID.get());
         try {
             sign(samlDocument, id, keyName, keypair, canonicalizationMethodType);
         } catch (ParserConfigurationException | GeneralSecurityException | MarshalException | XMLSignatureException e) {
@@ -210,18 +208,20 @@ public class SAML2Signature {
      *
      * @param document SAML document to have its ID attribute configured.
      */
-    private void configureIdAttribute(Document document) {
+    public static void configureIdAttribute(Document document) {
         // Estabilish the IDness of the ID attribute.
-        document.getDocumentElement().setIdAttribute(ID_ATTRIBUTE_NAME, true);
+        configureIdAttribute(document.getDocumentElement());
 
         NodeList nodes = document.getElementsByTagNameNS(JBossSAMLURIConstants.ASSERTION_NSURI.get(),
                 JBossSAMLConstants.ASSERTION.get());
 
         for (int i = 0; i < nodes.getLength(); i++) {
-            Node n = nodes.item(i);
+            configureIdAttribute((Element) nodes.item(i));
-            if (n instanceof Element) {
-                ((Element) n).setIdAttribute(ID_ATTRIBUTE_NAME, true);
-            }
         }
     }
+    
+    public static void configureIdAttribute(Element element) {
+        element.setIdAttribute(JBossSAMLConstants.ID.get(), true);
+    }
+
 }

saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/util/AssertionUtil.java

@@ -49,11 +49,12 @@ import org.keycloak.saml.processing.core.parsers.saml.SAMLParser;
 import org.keycloak.saml.processing.core.saml.v2.writers.SAMLAssertionWriter;
 import org.keycloak.saml.processing.core.util.JAXPValidationUtil;
 import org.keycloak.saml.processing.core.util.XMLEncryptionUtil;
-
+import org.keycloak.saml.processing.core.util.XMLSignatureUtil;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
 import org.w3c.dom.Node;
 
+import javax.xml.crypto.dsig.XMLSignature;
 import javax.xml.datatype.XMLGregorianCalendar;
 import javax.xml.namespace.QName;
 import java.io.ByteArrayInputStream;
@@ -267,42 +268,56 @@ public class AssertionUtil {
     }
 
     /**
-     * Given an assertion element, validate the signature
+     * Given an {@linkplain Element}, validate the Signature direct child element
      *
-     * @param assertionElement
+     * @param element parent {@linkplain Element}
      * @param publicKey the {@link PublicKey}
      *
-     * @return
+     * @return true if signature is present and valid
      */
-    public static boolean isSignatureValid(Element assertionElement, PublicKey publicKey) {
+    public static boolean isSignatureValid(Element element, PublicKey publicKey) {
-        try {
+        return isSignatureValid(element, new HardcodedKeyLocator(publicKey));
-            Document doc = DocumentUtil.createDocument();
-            Node n = doc.importNode(assertionElement, true);
-            doc.appendChild(n);
-
-            return new SAML2Signature().validate(doc, new HardcodedKeyLocator(publicKey));
-        } catch (Exception e) {
-            logger.signatureAssertionValidationError(e);
-        }
-        return false;
     }
 
     /**
-     * Given an assertion element, validate the signature.
+     * Given an {@linkplain Element}, validate the Signature direct child element
+     *
+     * @param element parent {@linkplain Element}
+     * @param keyLocator the {@link KeyLocator}
+     *
+     * @return true if signature is present and valid
      */
-    public static boolean isSignatureValid(Element assertionElement, KeyLocator keyLocator) {
+    
+    public static boolean isSignatureValid(Element element, KeyLocator keyLocator) {
         try {
-            Document doc = DocumentUtil.createDocument();
+            SAML2Signature.configureIdAttribute(element);
-            Node n = doc.importNode(assertionElement, true);
+            
-            doc.appendChild(n);
+            Element signature = getSignature(element);
-
+            if(signature != null) {
-            return new SAML2Signature().validate(doc, keyLocator);
+                return XMLSignatureUtil.validateSingleNode(signature, keyLocator);
+            }
         } catch (Exception e) {
             logger.signatureAssertionValidationError(e);
         }
         return false;
     }
+    
+    /**
+     * 
+     * Given an {@linkplain Element}, check if there is a Signature direct child element
+     * 
+     * @param element parent {@linkplain Element}
+     * @return true if signature is present
+     */
 
+    public static boolean isSignedElement(Element element) {
+        return getSignature(element) != null;
+    }
+    
+    protected static Element getSignature(Element element) {
+        return DocumentUtil.getDirectChildElement(element, XMLSignature.XMLNS, "Signature");
+    }
+    
     /**
      * Check whether the assertion has expired
      *
@@ -570,8 +585,8 @@ public class AssertionUtil {
 
     /**
      * This method modifies the given responseType, and replaces the encrypted assertion with a decrypted version.
-     *
+     * @param responseType a response containg an encrypted assertion
-     * It returns the assertion element as it was decrypted. This can be used in sginature verification.
+     * @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();

saml-core/src/main/java/org/keycloak/saml/processing/core/util/XMLSignatureUtil.java

@@ -468,7 +468,7 @@ public class XMLSignatureUtil {
         return true;
     }
 
-    private static boolean validateSingleNode(Node signatureNode, final KeyLocator locator) throws MarshalException, XMLSignatureException {
+    public static boolean validateSingleNode(Node signatureNode, final KeyLocator locator) throws MarshalException, XMLSignatureException {
         KeySelectorUtilizingKeyNameHint sel = new KeySelectorUtilizingKeyNameHint(locator);
         try {
             if (validateUsingKeySelector(signatureNode, sel)) {

saml-core/src/test/java/org/keycloak/saml/processing/core/saml/v2/util/AssertionUtilTest.java

@@ -0,0 +1,58 @@
+package org.keycloak.saml.processing.core.saml.v2.util;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.security.cert.X509Certificate;
+
+import org.bouncycastle.util.Arrays;
+import org.junit.Test;
+import org.keycloak.common.util.Base64;
+import org.keycloak.common.util.DerUtils;
+import org.w3c.dom.Document;
+import org.w3c.dom.Element;
+
+public class AssertionUtilTest {
+
+    private static final String PRIVATE_KEY = "MIICWwIBAAKBgQDVG8a7xGN6ZIkDbeecySygcDfsypjUMNPE4QJjis8B316CvsZQ0hcTTLUyiRpHlHZys2k3xEhHBHymFC1AONcvzZzpb40tAhLHO1qtAnut00khjAdjR3muLVdGkM/zMC7G5s9iIwBVhwOQhy+VsGnCH91EzkjZ4SVEr55KJoyQJQIDAQABAoGADaTtoG/+foOZUiLjRWKL/OmyavK9vjgyFtThNkZY4qHOh0h3og0RdSbgIxAsIpEa1FUwU2W5yvI6mNeJ3ibFgCgcxqPk6GkAC7DWfQfdQ8cS+dCuaFTs8ObIQEvU50YzeNPiiFxRA+MnauCUXaKm/PnDfjd4tPgru7XZvlGh0wECQQDsBbN2cKkBKpr/b5oJiBcBaSZtWiMNuYBDn9x8uORj+Gy/49BUIMHF2EWyxOWz6ocP5YiynNRkPe21Zus7PEr1AkEA5yWQOkxUTIg43s4pxNSeHtL+Ebqcg54lY2xOQK0yufxUVZI8ODctAKmVBMiCKpU3mZQquOaQicuGtocpgxlScQI/YM31zZ5nsxLGf/5GL6KhzPJT0IYn2nk7IoFu7bjn9BjwgcPurpLA52TNMYWQsTqAKwT6DEhG1NaRqNWNpb4VAkBehObAYBwMm5udyHIeEc+CzUalm0iLLa0eRdiN7AUVNpCJ2V2Uo0NcxPux1AgeP5xXydXafDXYkwhINWcNO9qRAkEA58ckAC5loUGwU5dLaugsGH/a2Q8Ac8bmPglwfCstYDpl8Gp/eimb1eKyvDEELOhyImAv4/uZV9wN85V0xZXWsw==";
+
+    /**
+     * The public certificate that corresponds to {@link #PRIVATE_KEY}.
+     */
+    private static final String PUBLIC_CERT = "MIIDdzCCAl+gAwIBAgIEbySuqTANBgkqhkiG9w0BAQsFADBsMRAwDgYDVQQGEwdVbmtub3duMRAwDgYDVQQIEwdVbmtub3duMRAwDgYDVQQHEwdVbmtub3duMRAwDgYDVQQKEwdVbmtub3duMRAwDgYDVQQLEwdVbmtub3duMRAwDgYDVQQDEwdVbmtub3duMB4XDTE1MDEyODIyMTYyMFoXDTE3MTAyNDIyMTYyMFowbDEQMA4GA1UEBhMHVW5rbm93bjEQMA4GA1UECBMHVW5rbm93bjEQMA4GA1UEBxMHVW5rbm93bjEQMA4GA1UEChMHVW5rbm93bjEQMA4GA1UECxMHVW5rbm93bjEQMA4GA1UEAxMHVW5rbm93bjCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAII/K9NNvXi9IySl7+l2zY/kKrGTtuR4WdCI0xLW/Jn4dLY7v1/HOnV4CC4ecFOzhdNFPtJkmEhP/q62CpmOYOKApXk3tfmm2rwEz9bWprVxgFGKnbrWlz61Z/cjLAlhD3IUj2ZRBquYgSXQPsYfXo1JmSWF5pZ9uh1FVqu9f4wvRqY20ZhUN+39F+1iaBsoqsrbXypCn1HgZkW1/9D9GZug1c3vB4wg1TwZZWRNGtxwoEhdK6dPrNcZ+6PdanVilWrbQFbBjY4wz8/7IMBzssoQ7Usmo8F1Piv0FGfaVeJqBrcAvbiBMpk8pT+27u6p8VyIX6LhGvnxIwM07NByeSUCAwEAAaMhMB8wHQYDVR0OBBYEFFlcNuTYwI9W0tQ224K1gFJlMam0MA0GCSqGSIb3DQEBCwUAA4IBAQB5snl1KWOJALtAjLqD0mLPg1iElmZP82Lq1htLBt3XagwzU9CaeVeCQ7lTp+DXWzPa9nCLhsC3QyrV3/+oqNli8C6NpeqI8FqN2yQW/QMWN1m5jWDbmrWwtQzRUn/rh5KEb5m3zPB+tOC6e/2bV3QeQebxeW7lVMD0tSCviUg1MQf1l2gzuXQo60411YwqrXwk6GMkDOhFDQKDlMchO3oRbQkGbcP8UeiKAXjMeHfzbiBr+cWz8NYZEtxUEDYDjTpKrYCSMJBXpmgVJCZ00BswbksxJwaGqGMPpUKmCV671pf3m8nq3xyiHMDGuGwtbU+GE8kVx85menmp8+964nin";
+
+    @Test
+    public void testSaml20Signed() throws Exception {
+        
+        X509Certificate decodeCertificate = DerUtils.decodeCertificate(new ByteArrayInputStream(Base64.decode(PUBLIC_CERT)));
+        
+        try (InputStream st = AssertionUtilTest.class.getResourceAsStream("saml20-signed-response.xml")) {
+            Document document = DocumentUtil.getDocument(st);
+            
+            Element assertion = DocumentUtil.getDirectChildElement(document.getDocumentElement(), "urn:oasis:names:tc:SAML:2.0:assertion", "Assertion");
+            
+            assertTrue(AssertionUtil.isSignatureValid(assertion, decodeCertificate.getPublicKey()));
+            
+            // test manipulation of signature
+            Element signatureElement = AssertionUtil.getSignature(assertion);
+            byte[] validSignature = Base64.decode(signatureElement.getTextContent());
+            
+            // change the signature value slightly
+            byte[] invalidSignature = Arrays.clone(validSignature);
+            invalidSignature[0] ^= invalidSignature[0];
+            signatureElement.setTextContent(Base64.encodeBytes(invalidSignature));
+            
+            // check that signature now is invalid
+            assertFalse(AssertionUtil.isSignatureValid(document.getDocumentElement(), decodeCertificate.getPublicKey()));
+            
+            // restore valid signature, but remove Signature element, check that still invalid
+            signatureElement.setTextContent(Base64.encodeBytes(validSignature));
+
+            assertion.removeChild(signatureElement);
+            assertFalse(AssertionUtil.isSignatureValid(document.getDocumentElement(), decodeCertificate.getPublicKey()));
+        }
+    }
+
+}

services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java

@@ -372,13 +372,13 @@ public class SAMLEndpoint {
                     assertionElement = DocumentUtil.getElement(holder.getSamlDocument(), new QName(JBossSAMLConstants.ASSERTION.get()));
                 }
 
-                if (config.isWantAssertionsSigned() && config.isValidateSignature()) {
+                boolean signed = AssertionUtil.isSignedElement(assertionElement);
-                    if (!AssertionUtil.isSignatureValid(assertionElement, getIDPKeyLocator())) {
+                if ((config.isWantAssertionsSigned() && !signed)
-                        logger.error("validation failed");
+                        || (signed && config.isValidateSignature() && !AssertionUtil.isSignatureValid(assertionElement, getIDPKeyLocator()))) {
-                        event.event(EventType.IDENTITY_PROVIDER_RESPONSE);
+                    logger.error("validation failed");
-                        event.error(Errors.INVALID_SIGNATURE);
+                    event.event(EventType.IDENTITY_PROVIDER_RESPONSE);
-                        return ErrorPage.error(session, Messages.INVALID_REQUESTER);
+                    event.error(Errors.INVALID_SIGNATURE);
-                    }
+                    return ErrorPage.error(session, Messages.INVALID_REQUESTER);
                 }
 
                 AssertionType assertion = responseType.getAssertions().get(0).getAssertion();

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlSignedDocumentOnlyBrokerTest.java

@@ -0,0 +1,83 @@
+package org.keycloak.testsuite.broker;
+
+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 java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.keycloak.testsuite.broker.BrokerTestConstants.*;
+
+public class KcSamlSignedDocumentOnlyBrokerTest extends KcSamlBrokerTest {
+
+    public static class KcSamlSignedBrokerConfiguration extends KcSamlBrokerConfiguration {
+
+        @Override
+        public RealmRepresentation createProviderRealm() {
+            RealmRepresentation realm = super.createProviderRealm();
+
+            realm.setPublicKey(REALM_PUBLIC_KEY);
+            realm.setPrivateKey(REALM_PRIVATE_KEY);
+
+            return realm;
+        }
+
+        @Override
+        public RealmRepresentation createConsumerRealm() {
+            RealmRepresentation realm = super.createConsumerRealm();
+
+            realm.setPublicKey(REALM_PUBLIC_KEY);
+            realm.setPrivateKey(REALM_PRIVATE_KEY);
+
+            return realm;
+        }
+
+        @Override
+        public List<ClientRepresentation> createProviderClients(SuiteContext suiteContext) {
+            List<ClientRepresentation> clientRepresentationList = super.createProviderClients(suiteContext);
+
+            for (ClientRepresentation client : clientRepresentationList) {
+                client.setClientAuthenticatorType("client-secret");
+                client.setSurrogateAuthRequired(false);
+
+                Map<String, String> attributes = client.getAttributes();
+                if (attributes == null) {
+                    attributes = new HashMap<>();
+                    client.setAttributes(attributes);
+                }
+
+                attributes.put("saml.assertion.signature", "false");
+                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);
+            }
+
+            return clientRepresentationList;
+        }
+
+        @Override
+        public IdentityProviderRepresentation setUpIdentityProvider(SuiteContext suiteContext) {
+            IdentityProviderRepresentation result = super.setUpIdentityProvider(suiteContext);
+
+            Map<String, String> config = result.getConfig();
+
+            config.put("validateSignature", "true");
+            config.put("wantAssertionsSigned", "false");
+            config.put("wantAuthnRequestsSigned", "true");
+            config.put("signingCertificate", IDP_SAML_SIGN_CERT);
+
+            return result;
+        }
+    }
+
+    @Override
+    protected BrokerConfiguration getBrokerConfiguration() {
+        return KcSamlSignedBrokerConfiguration.INSTANCE;
+    }
+
+}