From d52d685161336d68413bc633a81b223c66886c7a Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Wed, 19 Jul 2017 13:47:03 +0200 Subject: [PATCH] KEYCLOAK-4818 Fix undeclared namespace error in context serialization --- pom.xml | 1 + .../saml/v2/writers/SAMLAssertionWriter.java | 11 ++- .../saml/v2/writers/SAMLResponseWriter.java | 11 ++- services/pom.xml | 6 ++ .../broker/saml/SAMLDataMarshaller.java | 4 +- .../broker/saml/SAMLDataMarshallerTest.java | 47 +++++++++- .../saml-response-ds-ns-above-signature.xml | 89 +++++++++++++++++++ .../saml/saml-response-ds-ns-in-signature.xml | 88 ++++++++++++++++++ 8 files changed, 250 insertions(+), 7 deletions(-) create mode 100644 services/src/test/resources/org/keycloak/test/broker/saml/saml-response-ds-ns-above-signature.xml create mode 100644 services/src/test/resources/org/keycloak/test/broker/saml/saml-response-ds-ns-in-signature.xml diff --git a/pom.xml b/pom.xml index 503b1014fb..68efc36f76 100755 --- a/pom.xml +++ b/pom.xml @@ -80,6 +80,7 @@ 2.2.11 20140925 1.4.11.Final + 5.0.3 2.0.5 diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/writers/SAMLAssertionWriter.java b/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/writers/SAMLAssertionWriter.java index 867aceb2fd..41461bf889 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/writers/SAMLAssertionWriter.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/writers/SAMLAssertionWriter.java @@ -47,6 +47,7 @@ import java.net.URI; import java.util.List; import java.util.Set; +import javax.xml.crypto.dsig.XMLSignature; import static org.keycloak.saml.common.constants.JBossSAMLURIConstants.ASSERTION_NSURI; /** @@ -69,8 +70,17 @@ public class SAMLAssertionWriter extends BaseWriter { * @throws org.keycloak.saml.common.exceptions.ProcessingException */ public void write(AssertionType assertion) throws ProcessingException { + write(assertion, false); + } + + public void write(AssertionType assertion, boolean forceWriteDsigNamespace) throws ProcessingException { + Element sig = assertion.getSignature(); + StaxUtil.writeStartElement(writer, ASSERTION_PREFIX, JBossSAMLConstants.ASSERTION.get(), ASSERTION_NSURI.get()); StaxUtil.writeNameSpace(writer, ASSERTION_PREFIX, ASSERTION_NSURI.get()); + if (forceWriteDsigNamespace && sig != null && sig.getPrefix() != null && ! sig.hasAttribute("xmlns:" + sig.getPrefix())) { + StaxUtil.writeNameSpace(writer, sig.getPrefix(), XMLSignature.XMLNS); + } StaxUtil.writeDefaultNameSpace(writer, ASSERTION_NSURI.get()); // Attributes @@ -82,7 +92,6 @@ public class SAMLAssertionWriter extends BaseWriter { if (issuer != null) write(issuer, new QName(ASSERTION_NSURI.get(), JBossSAMLConstants.ISSUER.get(), ASSERTION_PREFIX)); - Element sig = assertion.getSignature(); if (sig != null) StaxUtil.writeDOMElement(writer, sig); diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/writers/SAMLResponseWriter.java b/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/writers/SAMLResponseWriter.java index 9327a73651..d2a59b94f7 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/writers/SAMLResponseWriter.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/core/saml/v2/writers/SAMLResponseWriter.java @@ -38,6 +38,7 @@ import javax.xml.stream.XMLStreamWriter; import java.net.URI; import java.util.List; import org.keycloak.dom.saml.v2.protocol.ExtensionsType; +import javax.xml.crypto.dsig.XMLSignature; /** * Write a SAML Response to stream @@ -63,8 +64,17 @@ public class SAMLResponseWriter extends BaseWriter { * @throws org.keycloak.saml.common.exceptions.ProcessingException */ public void write(ResponseType response) throws ProcessingException { + write(response, false); + } + + public void write(ResponseType response, boolean forceWriteDsigNamespace) throws ProcessingException { + Element sig = response.getSignature(); + StaxUtil.writeStartElement(writer, PROTOCOL_PREFIX, JBossSAMLConstants.RESPONSE.get(), JBossSAMLURIConstants.PROTOCOL_NSURI.get()); + if (forceWriteDsigNamespace && sig != null && sig.getPrefix() != null && ! sig.hasAttribute("xmlns:" + sig.getPrefix())) { + StaxUtil.writeNameSpace(writer, sig.getPrefix(), XMLSignature.XMLNS); + } StaxUtil.writeNameSpace(writer, PROTOCOL_PREFIX, JBossSAMLURIConstants.PROTOCOL_NSURI.get()); StaxUtil.writeNameSpace(writer, ASSERTION_PREFIX, JBossSAMLURIConstants.ASSERTION_NSURI.get()); @@ -75,7 +85,6 @@ public class SAMLResponseWriter extends BaseWriter { write(issuer, new QName(JBossSAMLURIConstants.ASSERTION_NSURI.get(), JBossSAMLConstants.ISSUER.get(), ASSERTION_PREFIX)); } - Element sig = response.getSignature(); if (sig != null) { StaxUtil.writeDOMElement(writer, sig); } diff --git a/services/pom.xml b/services/pom.xml index b5741f9a85..733b81277e 100755 --- a/services/pom.xml +++ b/services/pom.xml @@ -144,6 +144,12 @@ com.fasterxml.jackson.core jackson-annotations + + com.fasterxml.woodstox + woodstox-core + ${woodstox.version} + test + com.google.zxing javase diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLDataMarshaller.java b/services/src/main/java/org/keycloak/broker/saml/SAMLDataMarshaller.java index 508fcbd2f5..dc324633cf 100644 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLDataMarshaller.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLDataMarshaller.java @@ -51,11 +51,11 @@ public class SAMLDataMarshaller extends DefaultDataMarshaller { if (obj instanceof ResponseType) { ResponseType responseType = (ResponseType) obj; SAMLResponseWriter samlWriter = new SAMLResponseWriter(StaxUtil.getXMLStreamWriter(bos)); - samlWriter.write(responseType); + samlWriter.write(responseType, true); } else if (obj instanceof AssertionType) { AssertionType assertion = (AssertionType) obj; SAMLAssertionWriter samlWriter = new SAMLAssertionWriter(StaxUtil.getXMLStreamWriter(bos)); - samlWriter.write(assertion); + samlWriter.write(assertion, true); } else if (obj instanceof AuthnStatementType) { AuthnStatementType authnStatement = (AuthnStatementType) obj; SAMLAssertionWriter samlWriter = new SAMLAssertionWriter(StaxUtil.getXMLStreamWriter(bos)); diff --git a/services/src/test/java/org/keycloak/test/broker/saml/SAMLDataMarshallerTest.java b/services/src/test/java/org/keycloak/test/broker/saml/SAMLDataMarshallerTest.java index 9a686217c8..c8647f347a 100755 --- a/services/src/test/java/org/keycloak/test/broker/saml/SAMLDataMarshallerTest.java +++ b/services/src/test/java/org/keycloak/test/broker/saml/SAMLDataMarshallerTest.java @@ -25,17 +25,22 @@ import org.keycloak.dom.saml.v2.assertion.AssertionType; import org.keycloak.dom.saml.v2.assertion.AuthnStatementType; import org.keycloak.dom.saml.v2.assertion.NameIDType; import org.keycloak.dom.saml.v2.protocol.ResponseType; +import org.keycloak.saml.processing.core.parsers.saml.SAMLParser; +import java.io.InputStream; +import org.hamcrest.CoreMatchers; +import static org.hamcrest.CoreMatchers.instanceOf; +import static org.junit.Assert.assertThat; /** * @author Marek Posolda */ public class SAMLDataMarshallerTest { - private static final String TEST_RESPONSE = "http://localhost:8082/auth/realms/realm-with-saml-idp-basichttp://localhost:8082/auth/realms/realm-with-saml-idp-basictest-userhttp://localhost:8081/auth/realms/realm-with-brokerurn:oasis:names:tc:SAML:2.0:ac:classes:unspecified617-666-7777test-user@localhostmanager"; + private static final String TEST_RESPONSE = "http://localhost:8082/auth/realms/realm-with-saml-idp-basichttp://localhost:8082/auth/realms/realm-with-saml-idp-basictest-userhttp://localhost:8081/auth/realms/realm-with-brokerurn:oasis:names:tc:SAML:2.0:ac:classes:unspecified617-666-7777test-user@localhostmanager"; - private static final String TEST_ASSERTION = "http://localhost:8082/auth/realms/realm-with-saml-idp-basictest-userhttp://localhost:8081/auth/realms/realm-with-brokerurn:oasis:names:tc:SAML:2.0:ac:classes:unspecified617-666-7777test-user@localhostmanager"; + private static final String TEST_ASSERTION = "http://localhost:8082/auth/realms/realm-with-saml-idp-basictest-userhttp://localhost:8081/auth/realms/realm-with-brokerurn:oasis:names:tc:SAML:2.0:ac:classes:unspecified617-666-7777test-user@localhostmanager"; - private static final String TEST_ASSERTION_WITH_NAME_ID = "http://localhost:8082/auth/realms/realm-with-saml-idp-basictest-userhttp://localhost:8081/auth/realms/realm-with-brokerurn:oasis:names:tc:SAML:2.0:ac:classes:unspecified617-666-7777test-user@localhostb2c6275838784dba219c92f53ea5493c8ef4da09"; + private static final String TEST_ASSERTION_WITH_NAME_ID = "http://localhost:8082/auth/realms/realm-with-saml-idp-basictest-userhttp://localhost:8081/auth/realms/realm-with-brokerurn:oasis:names:tc:SAML:2.0:ac:classes:unspecified617-666-7777test-user@localhostb2c6275838784dba219c92f53ea5493c8ef4da09"; private static final String TEST_AUTHN_TYPE = "urn:oasis:names:tc:SAML:2.0:ac:classes:unspecified"; @@ -95,4 +100,40 @@ public class SAMLDataMarshallerTest { String serialized = serializer.serialize(authnStatement); Assert.assertEquals(TEST_AUTHN_TYPE, serialized); } + + @Test + public void testSerializeWithNamespaceInSignatureElement() throws Exception { + SAMLParser parser = new SAMLParser(); + try (InputStream st = SAMLDataMarshallerTest.class.getResourceAsStream("saml-response-ds-ns-in-signature.xml")) { + Object parsedObject = parser.parse(st); + assertThat(parsedObject, instanceOf(ResponseType.class)); + + ResponseType response = (ResponseType) parsedObject; + + SAMLDataMarshaller serializer = new SAMLDataMarshaller(); + String serialized = serializer.serialize(response.getAssertions().get(0).getAssertion()); + + AssertionType deserialized = serializer.deserialize(serialized, AssertionType.class); + assertThat(deserialized, CoreMatchers.notNullValue()); + assertThat(deserialized.getID(), CoreMatchers.is("id-4r-Xj702KQsM0gJyu3Fqpuwfe-LvDrEcQZpxKrhC")); + } + } + + @Test + public void testSerializeWithNamespaceNotInSignatureElement() throws Exception { + SAMLParser parser = new SAMLParser(); + try (InputStream st = SAMLDataMarshallerTest.class.getResourceAsStream("saml-response-ds-ns-above-signature.xml")) { + Object parsedObject = parser.parse(st); + assertThat(parsedObject, instanceOf(ResponseType.class)); + + ResponseType response = (ResponseType) parsedObject; + + SAMLDataMarshaller serializer = new SAMLDataMarshaller(); + String serialized = serializer.serialize(response.getAssertions().get(0).getAssertion()); + + AssertionType deserialized = serializer.deserialize(serialized, AssertionType.class); + assertThat(deserialized, CoreMatchers.notNullValue()); + assertThat(deserialized.getID(), CoreMatchers.is("id-4r-Xj702KQsM0gJyu3Fqpuwfe-LvDrEcQZpxKrhC")); + } + } } diff --git a/services/src/test/resources/org/keycloak/test/broker/saml/saml-response-ds-ns-above-signature.xml b/services/src/test/resources/org/keycloak/test/broker/saml/saml-response-ds-ns-above-signature.xml new file mode 100644 index 0000000000..dfa74aa12a --- /dev/null +++ b/services/src/test/resources/org/keycloak/test/broker/saml/saml-response-ds-ns-above-signature.xml @@ -0,0 +1,89 @@ + + SSO + + + + + SSO + + + + + + + + + + + DIGEST + + + SIG_VAL + + + my_email@my_provider.com + + + + + + + http://SERVER/auth/realms/MY_REALM + + + + + urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport + + + + + Yadav + + + H183561 + + + my_email@my_provider.com + + + MY_NAME + + + + diff --git a/services/src/test/resources/org/keycloak/test/broker/saml/saml-response-ds-ns-in-signature.xml b/services/src/test/resources/org/keycloak/test/broker/saml/saml-response-ds-ns-in-signature.xml new file mode 100644 index 0000000000..8460b8ec5b --- /dev/null +++ b/services/src/test/resources/org/keycloak/test/broker/saml/saml-response-ds-ns-in-signature.xml @@ -0,0 +1,88 @@ + + SSO + + + + + SSO + + + + + + + + + + + DIGEST + + + SIG_VAL + + + my_email@my_provider.com + + + + + + + http://SERVER/auth/realms/MY_REALM + + + + + urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport + + + + + Yadav + + + H183561 + + + my_email@my_provider.com + + + MY_NAME + + + +