From 825192ab8033f2d40a2036feedfcbd8ae54a2f16 Mon Sep 17 00:00:00 2001 From: Filip Hanik Date: Mon, 30 Sep 2019 09:42:52 -0700 Subject: [PATCH 1/2] Correctly set "Destination" in AuthNRequest message Fixes gh-7494 https://github.com/spring-projects/spring-security/issues/7494 --- ...aml2WebSsoAuthenticationRequestFilter.java | 7 +- .../samples/Saml2LoginIntegrationTests.java | 72 +++++++++++++++---- 2 files changed, 58 insertions(+), 21 deletions(-) diff --git a/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationRequestFilter.java b/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationRequestFilter.java index f8357cd9a3d..b68ec7b3fd5 100644 --- a/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationRequestFilter.java +++ b/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationRequestFilter.java @@ -107,12 +107,7 @@ private Saml2AuthenticationRequest createAuthenticationRequest(RelyingPartyRegis String localSpEntityId = Saml2Utils.getServiceProviderEntityId(relyingParty, request); return new Saml2AuthenticationRequest( localSpEntityId, - Saml2Utils.resolveUrlTemplate( - relyingParty.getAssertionConsumerServiceUrlTemplate(), - Saml2Utils.getApplicationUri(request), - relyingParty.getRemoteIdpEntityId(), - relyingParty.getRegistrationId() - ), + relyingParty.getIdpWebSsoUrl(), relyingParty.getSigningCredentials() ); } diff --git a/samples/boot/saml2login/src/integration-test/java/org/springframework/security/samples/Saml2LoginIntegrationTests.java b/samples/boot/saml2login/src/integration-test/java/org/springframework/security/samples/Saml2LoginIntegrationTests.java index 091f4bb197f..ce6b0a3d6c7 100644 --- a/samples/boot/saml2login/src/integration-test/java/org/springframework/security/samples/Saml2LoginIntegrationTests.java +++ b/samples/boot/saml2login/src/integration-test/java/org/springframework/security/samples/Saml2LoginIntegrationTests.java @@ -15,21 +15,10 @@ */ package org.springframework.security.samples; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.SpringBootConfiguration; -import org.springframework.boot.autoconfigure.EnableAutoConfiguration; -import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; -import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.context.annotation.ComponentScan; -import org.springframework.http.MediaType; -import org.springframework.security.saml2.provider.service.authentication.Saml2AuthenticationException; -import org.springframework.test.context.junit4.SpringRunner; -import org.springframework.test.util.AssertionErrors; -import org.springframework.test.web.servlet.MockMvc; -import org.springframework.test.web.servlet.ResultActions; -import org.springframework.test.web.servlet.ResultMatcher; - +import net.shibboleth.utilities.java.support.component.ComponentInitializationException; +import net.shibboleth.utilities.java.support.xml.BasicParserPool; import net.shibboleth.utilities.java.support.xml.SerializeSupport; +import net.shibboleth.utilities.java.support.xml.XMLParserException; import org.hamcrest.Matcher; import org.joda.time.DateTime; import org.junit.Test; @@ -38,8 +27,10 @@ import org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport; import org.opensaml.core.xml.io.MarshallerFactory; import org.opensaml.core.xml.io.MarshallingException; +import org.opensaml.core.xml.io.UnmarshallingException; import org.opensaml.saml.common.SignableSAMLObject; import org.opensaml.saml.saml2.core.Assertion; +import org.opensaml.saml.saml2.core.AuthnRequest; import org.opensaml.saml.saml2.core.EncryptedAssertion; import org.opensaml.saml.saml2.core.EncryptedID; import org.opensaml.saml.saml2.core.Response; @@ -55,9 +46,26 @@ import org.opensaml.xmlsec.signature.support.SignatureConstants; import org.opensaml.xmlsec.signature.support.SignatureException; import org.opensaml.xmlsec.signature.support.SignatureSupport; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.SpringBootConfiguration; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.context.annotation.ComponentScan; +import org.springframework.http.MediaType; +import org.springframework.security.saml2.provider.service.authentication.Saml2AuthenticationException; +import org.springframework.test.context.junit4.SpringRunner; +import org.springframework.test.util.AssertionErrors; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.ResultActions; +import org.springframework.test.web.servlet.ResultMatcher; +import org.springframework.util.MultiValueMap; +import org.springframework.web.util.UriComponentsBuilder; +import org.w3c.dom.Document; import org.w3c.dom.Element; import java.io.ByteArrayInputStream; +import java.net.URLDecoder; import java.nio.charset.StandardCharsets; import java.security.KeyException; import java.security.PrivateKey; @@ -78,6 +86,7 @@ import static org.springframework.security.samples.OpenSamlActionTestingSupport.buildSubjectConfirmation; import static org.springframework.security.samples.OpenSamlActionTestingSupport.buildSubjectConfirmationData; import static org.springframework.security.samples.OpenSamlActionTestingSupport.encryptNameId; +import static org.springframework.security.samples.OpenSamlActionTestingSupport.inflate; import static org.springframework.security.test.web.servlet.response.SecurityMockMvcResultMatchers.authenticated; import static org.springframework.security.test.web.servlet.response.SecurityMockMvcResultMatchers.unauthenticated; import static org.springframework.security.web.WebAttributes.AUTHENTICATION_EXCEPTION; @@ -131,6 +140,29 @@ public void authenticateRequestWhenRelayStateThenRespondsWithRedirectAndEncodedR .andExpect(header().string("Location", containsString("RelayState=relay%20state%20value%20with%20spaces"))); } + @Test + public void authenticateRequestWhenWorkingThenDestinationAttributeIsSet() throws Exception { + final String redirectedUrl = mockMvc.perform(get("http://localhost:8080/saml2/authenticate/simplesamlphp")) + .andExpect(status().is3xxRedirection()) + .andReturn() + .getResponse() + .getRedirectedUrl(); + MultiValueMap parameters = + UriComponentsBuilder.fromUriString(redirectedUrl).build(true).getQueryParams(); + String request = parameters.getFirst("SAMLRequest"); + AssertionErrors.assertNotNull("SAMLRequest parameter is missing", request); + request = URLDecoder.decode(request); + request = inflate(OpenSamlActionTestingSupport.decode(request)); + AuthnRequest authnRequest = (AuthnRequest) fromXml(request); + String destination = authnRequest.getDestination(); + assertEquals( + "Destination must match", + "https://simplesaml-for-spring-saml.cfapps.io/saml2/idp/SSOService.php", + destination + ); + } + + @Test public void authenticateWhenResponseIsSignedThenItSucceeds() throws Exception { Assertion assertion = buildAssertion(USERNAME); @@ -248,7 +280,7 @@ public void authenticateWhenIssuerIsInvalidThenItFails() throws Exception { "invalid_issuer", containsString( "Response issuer 'invalid issuer' doesn't match "+ - "'https://simplesaml-for-spring-saml.cfapps.io/saml2/idp/metadata.php'" + "'https://simplesaml-for-spring-saml.cfapps.io/saml2/idp/metadata.php'" ) ) ); @@ -330,6 +362,16 @@ private String toXml(XMLObject object) throws MarshallingException { return SerializeSupport.nodeToString(element); } + private XMLObject fromXml(String xml) + throws XMLParserException, UnmarshallingException, ComponentInitializationException { + BasicParserPool parserPool = new BasicParserPool(); + parserPool.initialize(); + Document document = parserPool.parse(new ByteArrayInputStream(xml.getBytes(UTF_8))); + Element element = document.getDocumentElement(); + return XMLObjectProviderRegistrySupport.getUnmarshallerFactory().getUnmarshaller(element).unmarshall(element); + + } + private X509Certificate decodeCertificate(String source) { try { final CertificateFactory factory = CertificateFactory.getInstance("X.509"); From 140fd1cde393992d908a78aa7f995e0e8446b063 Mon Sep 17 00:00:00 2001 From: Filip Hanik Date: Mon, 30 Sep 2019 10:01:22 -0700 Subject: [PATCH 2/2] Improve the Saml2AuthenticationRequest object - introduce the AssertionConsumerServiceURL attribute - add javadoc - align property name with SAML XML for AuthNRequest --- .../OpenSamlAuthenticationRequestFactory.java | 7 +- .../Saml2AuthenticationRequest.java | 146 ++++++++++++++++-- ...aml2WebSsoAuthenticationRequestFilter.java | 19 ++- .../samples/Saml2LoginIntegrationTests.java | 7 + 4 files changed, 158 insertions(+), 21 deletions(-) diff --git a/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/OpenSamlAuthenticationRequestFactory.java b/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/OpenSamlAuthenticationRequestFactory.java index 1c494ef0a2a..2755a288ddb 100644 --- a/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/OpenSamlAuthenticationRequestFactory.java +++ b/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/OpenSamlAuthenticationRequestFactory.java @@ -45,13 +45,14 @@ public String createAuthenticationRequest(Saml2AuthenticationRequest request) { auth.setIsPassive(Boolean.FALSE); auth.setProtocolBinding("urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect"); Issuer issuer = this.saml.buildSAMLObject(Issuer.class); - issuer.setValue(request.getLocalSpEntityId()); + issuer.setValue(request.getIssuer()); auth.setIssuer(issuer); - auth.setDestination(request.getWebSsoUri()); + auth.setDestination(request.getDestination()); + auth.setAssertionConsumerServiceURL(request.getAssertionConsumerServiceUrl()); return this.saml.toXml( auth, request.getCredentials(), - request.getLocalSpEntityId() + request.getIssuer() ); } diff --git a/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/Saml2AuthenticationRequest.java b/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/Saml2AuthenticationRequest.java index 4bf6c6bea76..89adbedac72 100644 --- a/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/Saml2AuthenticationRequest.java +++ b/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/Saml2AuthenticationRequest.java @@ -19,27 +19,36 @@ import org.springframework.security.saml2.credentials.Saml2X509Credential; import org.springframework.util.Assert; +import java.util.Collection; import java.util.LinkedList; import java.util.List; +import java.util.function.Consumer; /** * Data holder for information required to send an {@code AuthNRequest} * from the service provider to the identity provider + * https://www.oasis-open.org/committees/download.php/35711/sstc-saml-core-errata-2.0-wd-06-diff.pdf (line 2031) * * @see {@link Saml2AuthenticationRequestFactory} - * @see https://www.oasis-open.org/committees/download.php/35711/sstc-saml-core-errata-2.0-wd-06-diff.pdf (line 2031) * @since 5.2 */ -public class Saml2AuthenticationRequest { - private final String localSpEntityId; +public final class Saml2AuthenticationRequest { + private final String issuer; private final List credentials; - private String webSsoUri; + private final String destination; + private final String assertionConsumerServiceUrl; - public Saml2AuthenticationRequest(String localSpEntityId, String webSsoUri, List credentials) { - Assert.hasText(localSpEntityId, "localSpEntityId cannot be null"); - Assert.hasText(localSpEntityId, "webSsoUri cannot be null"); - this.localSpEntityId = localSpEntityId; - this.webSsoUri = webSsoUri; + private Saml2AuthenticationRequest( + String issuer, + String destination, + String assertionConsumerServiceUrl, + List credentials) { + Assert.hasText(issuer, "issuer cannot be null"); + Assert.hasText(destination, "destination cannot be null"); + Assert.hasText(assertionConsumerServiceUrl, "spAssertionConsumerServiceUrl cannot be null"); + this.issuer = issuer; + this.destination = destination; + this.assertionConsumerServiceUrl = assertionConsumerServiceUrl; this.credentials = new LinkedList<>(); for (Saml2X509Credential c : credentials) { if (c.isSigningCredential()) { @@ -50,15 +59,126 @@ public Saml2AuthenticationRequest(String localSpEntityId, String webSsoUri, List } - public String getLocalSpEntityId() { - return this.localSpEntityId; + /** + * returns the issuer, the local SP entity ID, for this authentication request. + * This property should be used to populate the {@code AuthNRequest.Issuer} XML element. + * This value typically is a URI, but can be an arbitrary string. + * @return issuer + */ + public String getIssuer() { + return this.issuer; } - public String getWebSsoUri() { - return this.webSsoUri; + /** + * returns the destination, the WEB Single Sign On URI, for this authentication request. + * This property populates the {@code AuthNRequest#Destination} XML attribute. + * @return destination + */ + public String getDestination() { + return this.destination; } + /** + * Returns the desired {@code AssertionConsumerServiceUrl} that this SP wishes to receive the + * assertion on. The IDP may or may not honor this request. + * This property populates the {@code AuthNRequest#AssertionConsumerServiceURL} XML attribute. + * @return the AssertionConsumerServiceURL value + */ + public String getAssertionConsumerServiceUrl() { + return assertionConsumerServiceUrl; + } + + /** + * Returns a list of credentials that can be used to sign the {@code AuthNRequest} object + * @return signing credentials + */ public List getCredentials() { return this.credentials; } + + /** + * A builder for {@link Saml2AuthenticationRequest}. + * returns a builder object + */ + public static Builder builder() { + return new Builder(); + } + + /** + * A builder for {@link Saml2AuthenticationRequest}. + */ + public static class Builder { + private String issuer; + private List credentials = new LinkedList<>(); + private String destination; + private String assertionConsumerServiceUrl; + + private Builder() { + } + + /** + * Sets the issuer for the authentication request. + * @param issuer - a required value + * @return this {@code Builder} + */ + public Builder issuer(String issuer) { + this.issuer = issuer; + return this; + } + + /** + * Modifies the collection of {@link Saml2X509Credential} credentials + * used in communication between IDP and SP, specifically signing the + * authentication request. + * For example: + * + * Saml2X509Credential credential = ...; + * return Saml2AuthenticationRequest.withLocalSpEntityId("id") + * .credentials(c -> c.add(credential)) + * ... + * .build(); + * + * @param credentials - a consumer that can modify the collection of credentials + * @return this object + */ + public Builder credentials(Consumer> credentials) { + credentials.accept(this.credentials); + return this; + } + + /** + * Sets the Destination for the authentication request. Typically the {@code Service Provider EntityID} + * @param destination - a required value + * @return this {@code Builder} + */ + public Builder destination(String destination) { + this.destination = destination; + return this; + } + + /** + * Sets the {@code assertionConsumerServiceURL} for the authentication request. + * Typically the {@code Service Provider EntityID} + * @param assertionConsumerServiceUrl - a required value + * @return this {@code Builder} + */ + public Builder assertionConsumerServiceUrl(String assertionConsumerServiceUrl) { + this.assertionConsumerServiceUrl = assertionConsumerServiceUrl; + return this; + } + + /** + * Creates a {@link Saml2AuthenticationRequest} object. + * @return the Saml2AuthenticationRequest object + * @throws {@link IllegalArgumentException} if a required property is not set + */ + public Saml2AuthenticationRequest build() { + return new Saml2AuthenticationRequest( + this.issuer, + this.destination, + this.assertionConsumerServiceUrl, + this.credentials + ); + } + } } diff --git a/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationRequestFilter.java b/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationRequestFilter.java index b68ec7b3fd5..7bc25a0994a 100644 --- a/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationRequestFilter.java +++ b/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/servlet/filter/Saml2WebSsoAuthenticationRequestFilter.java @@ -105,11 +105,20 @@ private String createSamlRequestRedirectUrl(HttpServletRequest request, RelyingP private Saml2AuthenticationRequest createAuthenticationRequest(RelyingPartyRegistration relyingParty, HttpServletRequest request) { String localSpEntityId = Saml2Utils.getServiceProviderEntityId(relyingParty, request); - return new Saml2AuthenticationRequest( - localSpEntityId, - relyingParty.getIdpWebSsoUrl(), - relyingParty.getSigningCredentials() - ); + return Saml2AuthenticationRequest + .builder() + .issuer(localSpEntityId) + .destination(relyingParty.getIdpWebSsoUrl()) + .credentials(c -> c.addAll(relyingParty.getCredentials())) + .assertionConsumerServiceUrl( + Saml2Utils.resolveUrlTemplate( + relyingParty.getAssertionConsumerServiceUrlTemplate(), + Saml2Utils.getApplicationUri(request), + relyingParty.getRemoteIdpEntityId(), + relyingParty.getRegistrationId() + ) + ) + .build(); } } diff --git a/samples/boot/saml2login/src/integration-test/java/org/springframework/security/samples/Saml2LoginIntegrationTests.java b/samples/boot/saml2login/src/integration-test/java/org/springframework/security/samples/Saml2LoginIntegrationTests.java index ce6b0a3d6c7..661b57c46f0 100644 --- a/samples/boot/saml2login/src/integration-test/java/org/springframework/security/samples/Saml2LoginIntegrationTests.java +++ b/samples/boot/saml2login/src/integration-test/java/org/springframework/security/samples/Saml2LoginIntegrationTests.java @@ -160,6 +160,13 @@ public void authenticateRequestWhenWorkingThenDestinationAttributeIsSet() throws "https://simplesaml-for-spring-saml.cfapps.io/saml2/idp/SSOService.php", destination ); + String acsURL = authnRequest.getAssertionConsumerServiceURL(); + assertEquals( + "AssertionConsumerServiceURL must match", + "http://localhost:8080/login/saml2/sso/simplesamlphp", + acsURL + ); + }