KEYCLOAK-5644 Skip Advice tag in SAML messages

2017-12-11 13:45:00 +01:00 · 2017-12-11 13:45:00 +01:00 · e6a64e234b
commit e6a64e234b
parent b793e42c53
9 changed files with 383 additions and 13 deletions
--- a/saml-core-api/src/main/java/org/keycloak/saml/common/constants/JBossSAMLConstants.java
+++ b/saml-core-api/src/main/java/org/keycloak/saml/common/constants/JBossSAMLConstants.java
@ -23,7 +23,7 @@ package org.keycloak.saml.common.constants;
 * @since Dec 10, 2008
 */
 public enum JBossSAMLConstants {
-    ADDRESS("Address"), ADDITIONAL_METADATA_LOCATION("AdditionalMetadataLocation"), AFFILIATION_DESCRIPTOR(
+    ADDRESS("Address"), ADVICE("Advice"), ADDITIONAL_METADATA_LOCATION("AdditionalMetadataLocation"), AFFILIATION_DESCRIPTOR(
            "AffiliationDescriptor"), ALLOW_CREATE("AllowCreate"), ARTIFACT("Artifact"), ARTIFACT_RESOLVE("ArtifactResolve"), ARTIFACT_RESPONSE(
            "ArtifactResponse"), ARTIFACT_RESOLUTION_SERVICE("ArtifactResolutionService"), ASSERTION("Assertion"), ASSERTION_CONSUMER_SERVICE(
            "AssertionConsumerService"), ASSERTION_CONSUMER_SERVICE_URL("AssertionConsumerServiceURL"), ASSERTION_CONSUMER_SERVICE_INDEX(
--- a/saml-core/pom.xml
+++ b/saml-core/pom.xml
@ -65,6 +65,12 @@
            <version>1.3</version>
            <scope>test</scope>
        </dependency>
+        <dependency>
+            <groupId>commons-io</groupId>
+            <artifactId>commons-io</artifactId>
+            <version>2.6</version>
+            <scope>test</scope>
+        </dependency>
    </dependencies>
    <build>
        <resources>
--- a/saml-core/src/main/java/org/keycloak/saml/common/DefaultPicketLinkLogger.java
+++ b/saml-core/src/main/java/org/keycloak/saml/common/DefaultPicketLinkLogger.java
@ -407,7 +407,7 @@ public class DefaultPicketLinkLogger implements PicketLinkLogger {
     */
    @Override
    public ParsingException parserExpectedEndTag(String tagName) {
-        return new ParsingException(ErrorCodes.EXPECTED_END_TAG + "RequestAbstract or XACMLAuthzDecisionQuery");
+        return new ParsingException(ErrorCodes.EXPECTED_END_TAG + tagName);
    }

    /*
--- a/saml-core/src/main/java/org/keycloak/saml/common/util/StaxParserUtil.java
+++ b/saml-core/src/main/java/org/keycloak/saml/common/util/StaxParserUtil.java
@ -49,6 +49,7 @@ import javax.xml.validation.Schema;
 import javax.xml.validation.SchemaFactory;
 import javax.xml.validation.Validator;
 import java.io.InputStream;
+import java.util.Objects;
 import java.util.concurrent.atomic.AtomicBoolean;

 /**
@ -75,7 +76,9 @@ public class StaxParserUtil {
    }

    /**
-     * Bypass an entire XML element block from startElement to endElement
+     * Bypass an entire XML element block from startElement to endElement.
+     * It is expected that the {@code xmlEventReader} is positioned at (has not yet read)
+     * the start element of the block it should bypass.
     *
     * @param xmlEventReader
     * @param tag Tag of the XML element that we need to bypass
@ -83,16 +86,48 @@ public class StaxParserUtil {
     * @throws org.keycloak.saml.common.exceptions.ParsingException
     */
    public static void bypassElementBlock(XMLEventReader xmlEventReader, String tag) throws ParsingException {
-        while (xmlEventReader.hasNext()) {
-            EndElement endElement = getNextEndElement(xmlEventReader);
-            if (endElement == null)
-                return;
+        XMLEvent xmlEvent = bypassElementBlock(xmlEventReader);

-            if (StaxParserUtil.matches(endElement, tag))
-                return;
+        if (! (xmlEvent instanceof EndElement) || ! Objects.equals(((EndElement) xmlEvent).getName().getLocalPart(), tag)) {
+            throw logger.parserExpectedEndTag(tag);
        }
    }

+    /**
+     * Bypass an entire XML element block.
+     * It is expected that the {@code xmlEventReader} is positioned at (has not yet read)
+     * the start element of the block it should bypass.
+     *
+     * @param xmlEventReader
+     * @returns Last XML event which is {@link EndElement} corresponding to the first startElement when no error occurs ({@code null} if not available)
+     *
+     * @throws org.keycloak.saml.common.exceptions.ParsingException
+     */
+    public static XMLEvent bypassElementBlock(XMLEventReader xmlEventReader) throws ParsingException {
+        XMLEvent xmlEvent;
+        int levelOfNesting = 0;
+        if (! xmlEventReader.hasNext()) {
+            return null;
+        }
+
+        try {
+            do {
+                xmlEvent = xmlEventReader.nextEvent();
+                if (xmlEvent instanceof StartElement) {
+                    levelOfNesting++;
+                } else if (xmlEvent instanceof EndElement) {
+                    levelOfNesting--;
+                }
+            } while (levelOfNesting > 0 && xmlEventReader.hasNext());
+        } catch (XMLStreamException e) {
+            throw logger.parserException(e);
+        }
+
+        return xmlEvent;
+    }
+
+
+
    /**
     * Advances reader if character whitespace encountered
     *
--- a/saml-core/src/main/java/org/keycloak/saml/processing/core/parsers/saml/SAMLAssertionParser.java
+++ b/saml-core/src/main/java/org/keycloak/saml/processing/core/parsers/saml/SAMLAssertionParser.java
@ -34,7 +34,6 @@ import org.keycloak.saml.common.exceptions.ProcessingException;
 import org.keycloak.saml.common.parsers.ParserNamespaceSupport;
 import org.keycloak.saml.common.util.DocumentUtil;
 import org.keycloak.saml.common.util.StaxParserUtil;
-import org.keycloak.saml.common.util.StringUtil;
 import org.keycloak.saml.processing.core.parsers.util.SAMLParserUtil;
 import org.keycloak.saml.processing.core.saml.v2.util.XMLTimeUtil;
 import org.w3c.dom.Element;
@ -42,7 +41,6 @@ import org.w3c.dom.Element;
 import javax.xml.datatype.XMLGregorianCalendar;
 import javax.xml.namespace.QName;
 import javax.xml.stream.XMLEventReader;
-import javax.xml.stream.events.Attribute;
 import javax.xml.stream.events.EndElement;
 import javax.xml.stream.events.StartElement;
 import javax.xml.stream.events.XMLEvent;
@ -131,6 +129,9 @@ public class SAMLAssertionParser implements ParserNamespaceSupport {
                ConditionsType conditions = (ConditionsType) conditionsParser.parse(xmlEventReader);

                assertion.setConditions(conditions);
+            } else if (JBossSAMLConstants.ADVICE.get().equalsIgnoreCase(tag)) {
+                StaxParserUtil.bypassElementBlock(xmlEventReader);
+                logger.debug("SAML Advice tag is ignored");
            } else if (JBossSAMLConstants.AUTHN_STATEMENT.get().equalsIgnoreCase(tag)) {
                AuthnStatementType authnStatementType = SAMLParserUtil.parseAuthnStatement(xmlEventReader);
                assertion.addStatement(authnStatementType);
--- a/saml-core/src/main/java/org/keycloak/saml/processing/core/parsers/saml/SAMLSloResponseParser.java
+++ b/saml-core/src/main/java/org/keycloak/saml/processing/core/parsers/saml/SAMLSloResponseParser.java
@ -58,7 +58,6 @@ public class SAMLSloResponseParser extends SAMLStatusResponseTypeParser implemen
                issuer.setValue(StaxParserUtil.getElementText(xmlEventReader));
                response.setIssuer(issuer);
            } else if (JBossSAMLConstants.SIGNATURE.get().equals(elementName)) {
-                startElement = StaxParserUtil.getNextStartElement(xmlEventReader);
                StaxParserUtil.bypassElementBlock(xmlEventReader, JBossSAMLConstants.SIGNATURE.get());
            } else if (JBossSAMLConstants.EXTENSIONS.get().equals(elementName)) {
                SAMLExtensionsParser extensionsParser = new SAMLExtensionsParser();
--- a/saml-core/src/test/java/org/keycloak/saml/common/util/StaxParserUtilTest.java
+++ b/saml-core/src/test/java/org/keycloak/saml/common/util/StaxParserUtilTest.java
@ -0,0 +1,178 @@
+/*
+ * Copyright 2017 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.keycloak.saml.common.util;
+
+import org.keycloak.saml.common.exceptions.ParsingException;
+import java.nio.charset.Charset;
+import javax.xml.stream.XMLEventReader;
+import javax.xml.stream.XMLStreamException;
+import javax.xml.stream.events.Characters;
+import javax.xml.stream.events.EndElement;
+import javax.xml.stream.events.StartDocument;
+import javax.xml.stream.events.StartElement;
+import javax.xml.stream.events.XMLEvent;
+import org.apache.commons.io.IOUtils;
+import org.hamcrest.Matcher;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import static org.junit.Assert.assertThat;
+import static org.hamcrest.CoreMatchers.*;
+
+/**
+ *
+ * @author hmlnarik
+ */
+public class StaxParserUtilTest {
+
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
+
+    private void assertStartTag(XMLEvent event, String tagName) {
+        assertThat(event, instanceOf(StartElement.class));
+        assertThat(((StartElement) event).getName().getLocalPart(), is(tagName));
+    }
+
+    private void assertEndTag(XMLEvent event, String tagName) {
+        assertThat(event, instanceOf(EndElement.class));
+        assertThat(((EndElement) event).getName().getLocalPart(), is(tagName));
+    }
+
+    private void assertCharacters(XMLEvent event, Matcher<String> matcher) {
+        assertThat(event, instanceOf(Characters.class));
+        assertThat(((Characters) event).getData(), matcher);
+    }
+
+    @Test
+    public void testBypassElementBlock() throws XMLStreamException, ParsingException {
+        String xml = "<a><b><c>test</c>"
+          + "<d>aa</d></b></a>";
+        XMLEventReader reader = StaxParserUtil.getXMLEventReader(IOUtils.toInputStream(xml, Charset.defaultCharset()));
+
+        assertThat(reader.nextEvent(), instanceOf(StartDocument.class));
+
+        assertStartTag(reader.nextEvent(), "a");
+        assertStartTag(reader.nextEvent(), "b");
+        assertStartTag(reader.nextEvent(), "c");
+        assertCharacters(reader.nextEvent(), is("test"));
+        assertEndTag(reader.nextEvent(), "c");
+
+        StaxParserUtil.bypassElementBlock(reader, "d");
+
+        assertEndTag(reader.nextEvent(), "b");
+        assertEndTag(reader.nextEvent(), "a");
+    }
+
+    @Test
+    public void testBypassElementBlockAnon() throws XMLStreamException, ParsingException {
+        String xml = "<a><b><c>test</c>"
+          + "<d>aa</d></b></a>";
+        XMLEventReader reader = StaxParserUtil.getXMLEventReader(IOUtils.toInputStream(xml, Charset.defaultCharset()));
+
+        assertThat(reader.nextEvent(), instanceOf(StartDocument.class));
+
+        assertStartTag(reader.nextEvent(), "a");
+        assertStartTag(reader.nextEvent(), "b");
+        assertStartTag(reader.nextEvent(), "c");
+        assertCharacters(reader.nextEvent(), is("test"));
+        assertEndTag(reader.nextEvent(), "c");
+
+        StaxParserUtil.bypassElementBlock(reader);
+
+        assertEndTag(reader.nextEvent(), "b");
+        assertEndTag(reader.nextEvent(), "a");
+    }
+
+    @Test
+    public void testBypassElementBlockNested() throws XMLStreamException, ParsingException {
+        String xml = "<a><b><c>test</c>"
+          + "<d>aa<d>nestedD</d></d></b></a>";
+        XMLEventReader reader = StaxParserUtil.getXMLEventReader(IOUtils.toInputStream(xml, Charset.defaultCharset()));
+
+        assertThat(reader.nextEvent(), instanceOf(StartDocument.class));
+
+        assertStartTag(reader.nextEvent(), "a");
+        assertStartTag(reader.nextEvent(), "b");
+        assertStartTag(reader.nextEvent(), "c");
+        assertCharacters(reader.nextEvent(), is("test"));
+        assertEndTag(reader.nextEvent(), "c");
+
+        StaxParserUtil.bypassElementBlock(reader, "d");
+
+        assertEndTag(reader.nextEvent(), "b");
+        assertEndTag(reader.nextEvent(), "a");
+    }
+
+    @Test
+    public void testBypassElementBlockNestedAnon() throws XMLStreamException, ParsingException {
+        String xml = "<a><b><c>test</c>"
+          + "<d>aa<d>nestedD</d></d></b></a>";
+        XMLEventReader reader = StaxParserUtil.getXMLEventReader(IOUtils.toInputStream(xml, Charset.defaultCharset()));
+
+        assertThat(reader.nextEvent(), instanceOf(StartDocument.class));
+
+        assertStartTag(reader.nextEvent(), "a");
+        assertStartTag(reader.nextEvent(), "b");
+        assertStartTag(reader.nextEvent(), "c");
+        assertCharacters(reader.nextEvent(), is("test"));
+        assertEndTag(reader.nextEvent(), "c");
+
+        StaxParserUtil.bypassElementBlock(reader);
+
+        assertEndTag(reader.nextEvent(), "b");
+        assertEndTag(reader.nextEvent(), "a");
+    }
+
+    @Test
+    public void testBypassElementBlockWrongPairing() throws XMLStreamException, ParsingException {
+        String xml = "<a><b><c>test</c>"
+          + "<d><b>aa</d><d>nestedD</d></d></b></a>";
+        XMLEventReader reader = StaxParserUtil.getXMLEventReader(IOUtils.toInputStream(xml, Charset.defaultCharset()));
+
+        assertThat(reader.nextEvent(), instanceOf(StartDocument.class));
+
+        assertStartTag(reader.nextEvent(), "a");
+        assertStartTag(reader.nextEvent(), "b");
+        assertStartTag(reader.nextEvent(), "c");
+        assertCharacters(reader.nextEvent(), is("test"));
+        assertEndTag(reader.nextEvent(), "c");
+
+        expectedException.expect(ParsingException.class);
+        StaxParserUtil.bypassElementBlock(reader, "d");
+    }
+
+    @Test
+    public void testBypassElementBlockNestedPrematureEnd() throws XMLStreamException, ParsingException {
+        String xml = "<a><b><c>test</c>"
+          + "<d>aa<d>nestedD</d></d>";
+        XMLEventReader reader = StaxParserUtil.getXMLEventReader(IOUtils.toInputStream(xml, Charset.defaultCharset()));
+
+        assertThat(reader.nextEvent(), instanceOf(StartDocument.class));
+
+        assertStartTag(reader.nextEvent(), "a");
+        assertStartTag(reader.nextEvent(), "b");
+        assertStartTag(reader.nextEvent(), "c");
+        assertCharacters(reader.nextEvent(), is("test"));
+        assertEndTag(reader.nextEvent(), "c");
+
+        StaxParserUtil.bypassElementBlock(reader, "d");
+
+        expectedException.expect(XMLStreamException.class);
+        reader.nextEvent();
+    }
+
+}
--- 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
@ -34,7 +34,6 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;

-import org.hamcrest.CustomMatcher;
 import org.keycloak.common.util.Base64;
 import org.keycloak.common.util.DerUtils;
 import org.keycloak.common.util.StreamUtil;
@ -359,6 +358,13 @@ public class SAMLParserTest {
        }
    }

+    @Test
+    public void testSaml20AssertionsAdviceTag() throws IOException, ParsingException {
+        try (InputStream st = SAMLParserTest.class.getResourceAsStream("saml20-assertion-advice.xml")) {
+            parser.parse(st);
+        }
+    }
+
    private InputStream removeAttribute(String resourceName, String attribute) throws IOException {
        try (InputStream st = SAMLParserTest.class.getResourceAsStream(resourceName)) {
            String str = StreamUtil.readString(st, StandardCharsets.UTF_8);
--- a/saml-core/src/test/resources/org/keycloak/saml/processing/core/parsers/saml/saml20-assertion-advice.xml
+++ b/saml-core/src/test/resources/org/keycloak/saml/processing/core/parsers/saml/saml20-assertion-advice.xml
@ -0,0 +1,145 @@
+<saml:Assertion xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xs="http://www.w3.org/2001/XMLSchema"
+                ID="_3c39bc0fe7b13769cab2f6f45eba801b1245264310738"
+                IssueInstant="2009-06-17T18:45:10.738Z" Version="2.0">
+    <saml:Issuer Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity">
+        https://www.salesforce.com
+    </saml:Issuer>
+
+    <Signature xmlns="http://www.w3.org/2000/09/xmldsig#">
+        <SignedInfo>
+            <CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
+            <SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
+            <Reference URI="#_3c39bc0fe7b13769cab2f6f45eba801b1245264310738">
+                <Transforms>
+                    <Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
+                    <Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#">
+                        <ec:InclusiveNamespaces PrefixList="ds saml xs" xmlns:ec="http://www.w3.org/2001/10/xml-exc-c14n#"/>
+                    </Transform>
+                </Transforms>
+                <DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
+                <DigestValue>vzR9Hfp8d16576tEDeq/zhpmLoo=
+                </DigestValue>
+            </Reference>
+        </SignedInfo>
+        <SignatureValue>
+            AzID5hhJeJlG2llUDvZswNUrlrPtR7S37QYH2W+Un1n8c6kTC
+            Xr/lihEKPcA2PZt86eBntFBVDWTRlh/W3yUgGOqQBJMFOVbhK
+            M/CbLHbBUVT5TcxIqvsNvIFdjIGNkf1W0SBqRKZOJ6tzxCcLo
+            9dXqAyAUkqDpX5+AyltwrdCPNmncUM4dtRPjI05CL1rRaGeyX
+            3kkqOL8p0vjm0fazU5tCAJLbYuYgU1LivPSahWNcpvRSlCI4e
+            Pn2oiVDyrcc4et12inPMTc2lGIWWWWJyHOPSiXRSkEAIwQVjf
+            Qm5cpli44Pv8FCrdGWpEE0yXsPBvDkM9jIzwCYGG2fKaLBag==
+        </SignatureValue>
+        <KeyInfo>
+            <X509Data>
+                <X509Certificate>
+                    MIIEATCCAumgAwIBAgIBBTANBgkqhkiG9w0BAQ0FADCBgzELM
+                    [Certificate truncated for readability...]
+                </X509Certificate>
+            </X509Data>
+        </KeyInfo>
+    </Signature>
+
+    <saml:Subject>
+        <saml:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified">
+            saml01@salesforce.com
+        </saml:NameID>
+
+        <saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer">
+            <saml:SubjectConfirmationData NotOnOrAfter="2009-06-17T18:50:10.738Z"
+                                          Recipient="https://login.salesforce.com"/>
+        </saml:SubjectConfirmation>
+    </saml:Subject>
+
+    <saml:Conditions NotBefore="2009-06-17T18:45:10.738Z"
+                     NotOnOrAfter="2009-06-17T18:50:10.738Z">
+
+        <saml:AudienceRestriction>
+            <saml:Audience>https://saml.salesforce.com</saml:Audience>
+        </saml:AudienceRestriction>
+    </saml:Conditions>
+
+    <saml:Advice>
+        <saml:AssertionIDRef>NCName</saml:AssertionIDRef>
+    </saml:Advice>
+
+    <saml:Advice>
+        <saml:AssertionURIRef>NCName</saml:AssertionURIRef>
+        <a:a xmlns:a="urn:a">
+            <a:b>nested
+                <a:c>element text</a:c>
+            </a:b>
+        </a:a>
+    </saml:Advice>
+
+    <saml:AuthnStatement AuthnInstant="2009-06-17T18:45:10.738Z">
+        <saml:AuthnContext>
+            <saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:unspecified
+            </saml:AuthnContextClassRef>
+        </saml:AuthnContext>
+    </saml:AuthnStatement>
+
+    <saml:AttributeStatement>
+
+        <saml:Attribute Name="portal_id">
+            <saml:AttributeValue xsi:type="xs:anyType">060D00000000SHZ
+            </saml:AttributeValue>
+        </saml:Attribute>
+
+        <saml:Attribute Name="organization_id">
+            <saml:AttributeValue xsi:type="xs:anyType">
+                <n1:elem2 xmlns:n1="http://example.net" xml:lang="en">
+                    <n3:stuff xmlns:n3="ftp://example.org">00DD0000000F7L5</n3:stuff>
+                </n1:elem2>
+            </saml:AttributeValue>
+        </saml:Attribute>
+
+        <saml:Attribute Name="status">
+            <saml:AttributeValue xsi:type="xs:anyType">
+                <status>
+                    <code>
+                        <status>XYZ</status>
+                    </code>
+                </status>
+            </saml:AttributeValue>
+        </saml:Attribute>
+
+        <saml:Attribute Name="has_sub_organization">
+            <saml:AttributeValue xsi:type="xs:boolean">true</saml:AttributeValue>
+        </saml:Attribute>
+
+        <saml:Attribute Name="anytype_test">
+            <saml:AttributeValue>
+                <elem1 atttr1="en">
+                    <elem2>val2</elem2>
+                </elem1>
+            </saml:AttributeValue>
+        </saml:Attribute>
+
+        <saml:Attribute Name="anytype_no_xml_test">
+            <saml:AttributeValue>value_no_xml</saml:AttributeValue>
+        </saml:Attribute>
+
+        <saml:Attribute Name="ssostartpage"
+                        NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified">
+
+            <saml:AttributeValue xsi:type="xs:anyType">
+                http://www.salesforce.com/security/saml/saml20-gen.jsp
+            </saml:AttributeValue>
+        </saml:Attribute>
+
+        <saml:Attribute Name="logouturl"
+                        NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
+
+            <saml:AttributeValue xsi:type="xs:string">
+                http://www.salesforce.com/security/del_auth/SsoLogoutPage.html
+            </saml:AttributeValue>
+        </saml:Attribute>
+
+        <saml:Attribute Name="nil_value_attribute">
+            <saml:AttributeValue xsi:nil="true" xsi:type="xs:anyType"/>
+        </saml:Attribute>
+
+
+    </saml:AttributeStatement>
+</saml:Assertion>