From c6b611433b98298fbdf3d6e37fa64051a13b4e2f Mon Sep 17 00:00:00 2001 From: Luis Moreira Date: Wed, 6 Jul 2022 04:58:37 +0100 Subject: [PATCH] saml: Fix SAML SSO plugin redirect URL (#6457) This PR fixes the issue #6427 -> SAML request must be appended to an IdP URL as a query param with an ampersand, if the URL already contains a question mark, as opposed to always assume that IdP URLs don't have any query params. Google's IdP URL for instance looks like this: https://accounts.google.com/o/saml2/idp?idpid=, therefore the expected redirect URL would be https://accounts.google.com/o/saml2/idp?idpid=&SAMLRequest= This code change is backwards compatible with the current behaviour. --- plugins/user-authenticators/saml2/pom.xml | 6 ++ .../org/apache/cloudstack/saml/SAMLUtils.java | 3 +- .../org/apache/cloudstack/SAMLUtilsTest.java | 73 +++++++++++++++++-- pom.xml | 1 + 4 files changed, 76 insertions(+), 7 deletions(-) diff --git a/plugins/user-authenticators/saml2/pom.xml b/plugins/user-authenticators/saml2/pom.xml index 9cfe5cb390c..11e34ee4a9e 100644 --- a/plugins/user-authenticators/saml2/pom.xml +++ b/plugins/user-authenticators/saml2/pom.xml @@ -53,5 +53,11 @@ ${cs.xercesImpl.version} test + + org.assertj + assertj-core + ${cs.assertj.version} + test + diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java index cbbdbd28bf8..c65e4c09be6 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java @@ -150,7 +150,8 @@ public class SAMLUtils { if (spMetadata.getKeyPair() != null) { privateKey = spMetadata.getKeyPair().getPrivate(); } - redirectUrl = idpMetadata.getSsoUrl() + "?" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(authnRequest), privateKey, signatureAlgorithm); + String appendOperator = idpMetadata.getSsoUrl().contains("?") ? "&" : "?"; + redirectUrl = idpMetadata.getSsoUrl() + appendOperator + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(authnRequest), privateKey, signatureAlgorithm); } catch (ConfigurationException | FactoryConfigurationError | MarshallingException | IOException | NoSuchAlgorithmException | InvalidKeyException | java.security.SignatureException e) { s_logger.error("SAML AuthnRequest message building error: " + e.getMessage()); } diff --git a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java index 433fdf3224a..cf265199b42 100644 --- a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java +++ b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java @@ -19,18 +19,22 @@ package org.apache.cloudstack; -import java.security.KeyPair; -import java.security.PrivateKey; -import java.security.PublicKey; -import java.util.regex.Pattern; - +import junit.framework.TestCase; +import org.apache.cloudstack.saml.SAML2AuthManager; +import org.apache.cloudstack.saml.SAMLProviderMetadata; import org.apache.cloudstack.saml.SAMLUtils; import org.apache.cloudstack.utils.security.CertUtils; import org.junit.Test; import org.opensaml.saml2.core.AuthnRequest; import org.opensaml.saml2.core.LogoutRequest; -import junit.framework.TestCase; +import java.net.URI; +import java.security.KeyPair; +import java.security.PrivateKey; +import java.security.PublicKey; +import java.util.regex.Pattern; + +import static org.assertj.core.api.Assertions.assertThat; public class SAMLUtilsTest extends TestCase { @@ -60,6 +64,63 @@ public class SAMLUtilsTest extends TestCase { assertEquals(req.getIssuer().getValue(), spId); } + @Test + public void testBuildAuthnRequestUrlWithoutQueryParam() throws Exception { + String urlScheme = "http"; + + String spDomain = "sp.domain.example"; + String spUrl = urlScheme + "://" + spDomain; + String spId = "serviceProviderId"; + + String idpDomain = "idp.domain.example"; + String idpUrl = urlScheme + "://" + idpDomain; + String idpId = "identityProviderId"; + + String authnId = SAMLUtils.generateSecureRandomId(); + + SAMLProviderMetadata spMetadata = new SAMLProviderMetadata(); + spMetadata.setEntityId(spId); + spMetadata.setSsoUrl(spUrl); + + SAMLProviderMetadata idpMetadata = new SAMLProviderMetadata(); + idpMetadata.setSsoUrl(idpUrl); + idpMetadata.setEntityId(idpId); + + URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value())); + assertThat(redirectUrl).hasScheme(urlScheme).hasHost(idpDomain).hasParameter("SAMLRequest"); + assertEquals(urlScheme, redirectUrl.getScheme()); + assertEquals(idpDomain, redirectUrl.getHost()); + } + + @Test + public void testBuildAuthnRequestUrlWithQueryParam() throws Exception { + String urlScheme = "http"; + + String spDomain = "sp.domain.example"; + String spUrl = urlScheme + "://" + spDomain; + String spId = "cloudstack"; + + String idpDomain = "idp.domain.example"; + String idpQueryParam = "idpid=CX1298373"; + String idpUrl = urlScheme + "://" + idpDomain + "?" + idpQueryParam; + String idpId = "identityProviderId"; + + String authnId = SAMLUtils.generateSecureRandomId(); + + SAMLProviderMetadata spMetadata = new SAMLProviderMetadata(); + spMetadata.setEntityId(spId); + spMetadata.setSsoUrl(spUrl); + + SAMLProviderMetadata idpMetadata = new SAMLProviderMetadata(); + idpMetadata.setSsoUrl(idpUrl); + idpMetadata.setEntityId(idpId); + + URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value())); + assertThat(redirectUrl).hasScheme(urlScheme).hasHost(idpDomain).hasParameter("idpid").hasParameter("SAMLRequest"); + assertEquals(urlScheme, redirectUrl.getScheme()); + assertEquals(idpDomain, redirectUrl.getHost()); + } + @Test public void testBuildLogoutRequest() throws Exception { String logoutUrl = "http://logoutUrl"; diff --git a/pom.xml b/pom.xml index 08d8d0ba704..eb67edc485c 100644 --- a/pom.xml +++ b/pom.xml @@ -118,6 +118,7 @@ 7.1.0 2.27.2 2.12.2 + 3.23.1 5.10.0