From 43477e03c6fcd5870139ff7153255bb9ebf34da2 Mon Sep 17 00:00:00 2001 From: Luis Moreira Date: Tue, 14 Jun 2022 14:07:31 +0100 Subject: [PATCH 1/5] Fix SAML SSO plugin redirect URL --- .../org/apache/cloudstack/saml/SAMLUtils.java | 2 +- .../org/apache/cloudstack/SAMLUtilsTest.java | 26 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) 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 cbbdbd28bf8a..9ed47b62bf48 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,7 @@ public static String buildAuthnRequestUrl(final String authnId, final SAMLProvid if (spMetadata.getKeyPair() != null) { privateKey = spMetadata.getKeyPair().getPrivate(); } - redirectUrl = idpMetadata.getSsoUrl() + "?" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(authnRequest), privateKey, signatureAlgorithm); + redirectUrl = idpMetadata.getSsoUrl().contains("?") ? idpMetadata.getSsoUrl() + "&" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(authnRequest), privateKey, signatureAlgorithm) : idpMetadata.getSsoUrl() + "?" + 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 433fdf3224ad..014cd1c4bf09 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 @@ -24,9 +24,11 @@ import java.security.PublicKey; import java.util.regex.Pattern; +import org.apache.cloudstack.saml.SAML2AuthManager; import org.apache.cloudstack.saml.SAMLUtils; import org.apache.cloudstack.utils.security.CertUtils; import org.junit.Test; +import org.opensaml.DefaultBootstrap; import org.opensaml.saml2.core.AuthnRequest; import org.opensaml.saml2.core.LogoutRequest; @@ -60,6 +62,30 @@ public void testBuildAuthnRequestObject() throws Exception { assertEquals(req.getIssuer().getValue(), spId); } + @Test + public void testbuildAuthnRequestUrlWithoutQueryParam() throws Exception { + String consumerUrl = "http://someurl.com"; + String idpUrl = "http://idp.domain.example"; + String spId = "cloudstack"; + String authnId = SAMLUtils.generateSecureRandomId(); + DefaultBootstrap.bootstrap(); + AuthnRequest req = SAMLUtils.buildAuthnRequestObject(authnId, spId, idpUrl, consumerUrl); + String redirectUrl = idpUrl.contains("?") ? idpUrl + "&" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value()) : idpUrl + "?" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value()); + assertEquals(redirectUrl, idpUrl + "?" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value())); + } + + @Test + public void testbuildAuthnRequestUrlWithQueryParam() throws Exception { + String consumerUrl = "http://someurl.com"; + String idpUrl = "http://idp.domain.example?idpid=CX1298373"; + String spId = "cloudstack"; + String authnId = SAMLUtils.generateSecureRandomId(); + DefaultBootstrap.bootstrap(); + AuthnRequest req = SAMLUtils.buildAuthnRequestObject(authnId, spId, idpUrl, consumerUrl); + String redirectUrl = idpUrl.contains("?") ? idpUrl + "&" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value()) : idpUrl + "?" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value()); + assertEquals(redirectUrl, idpUrl + "&" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value())); + } + @Test public void testBuildLogoutRequest() throws Exception { String logoutUrl = "http://logoutUrl"; From bf61ad8d44c996c46bef23ad9e9d23a860f67705 Mon Sep 17 00:00:00 2001 From: Luis Moreira Date: Tue, 21 Jun 2022 17:09:49 +0100 Subject: [PATCH 2/5] Fix redirect url test methods naming convention --- .../src/test/java/org/apache/cloudstack/SAMLUtilsTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 014cd1c4bf09..81199ad0bd0b 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 @@ -63,7 +63,7 @@ public void testBuildAuthnRequestObject() throws Exception { } @Test - public void testbuildAuthnRequestUrlWithoutQueryParam() throws Exception { + public void testBuildAuthnRequestUrlWithoutQueryParam() throws Exception { String consumerUrl = "http://someurl.com"; String idpUrl = "http://idp.domain.example"; String spId = "cloudstack"; @@ -75,7 +75,7 @@ public void testbuildAuthnRequestUrlWithoutQueryParam() throws Exception { } @Test - public void testbuildAuthnRequestUrlWithQueryParam() throws Exception { + public void testBuildAuthnRequestUrlWithQueryParam() throws Exception { String consumerUrl = "http://someurl.com"; String idpUrl = "http://idp.domain.example?idpid=CX1298373"; String spId = "cloudstack"; From 749dca23b7fe86c4dbf811c1afe14ef5f444f413 Mon Sep 17 00:00:00 2001 From: Luis Moreira Date: Thu, 23 Jun 2022 12:29:35 +0100 Subject: [PATCH 3/5] Address PR comments --- .../src/main/java/org/apache/cloudstack/saml/SAMLUtils.java | 3 ++- .../src/test/java/org/apache/cloudstack/SAMLUtilsTest.java | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) 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 9ed47b62bf48..c65e4c09be6e 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 static String buildAuthnRequestUrl(final String authnId, final SAMLProvid if (spMetadata.getKeyPair() != null) { privateKey = spMetadata.getKeyPair().getPrivate(); } - redirectUrl = idpMetadata.getSsoUrl().contains("?") ? idpMetadata.getSsoUrl() + "&" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(authnRequest), privateKey, signatureAlgorithm) : 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 81199ad0bd0b..ece9bdc64453 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 @@ -70,7 +70,8 @@ public void testBuildAuthnRequestUrlWithoutQueryParam() throws Exception { String authnId = SAMLUtils.generateSecureRandomId(); DefaultBootstrap.bootstrap(); AuthnRequest req = SAMLUtils.buildAuthnRequestObject(authnId, spId, idpUrl, consumerUrl); - String redirectUrl = idpUrl.contains("?") ? idpUrl + "&" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value()) : idpUrl + "?" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value()); + String appendOperator = idpUrl.contains("?") ? "&" : "?"; + String redirectUrl = idpUrl + appendOperator + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value()); assertEquals(redirectUrl, idpUrl + "?" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value())); } @@ -82,7 +83,8 @@ public void testBuildAuthnRequestUrlWithQueryParam() throws Exception { String authnId = SAMLUtils.generateSecureRandomId(); DefaultBootstrap.bootstrap(); AuthnRequest req = SAMLUtils.buildAuthnRequestObject(authnId, spId, idpUrl, consumerUrl); - String redirectUrl = idpUrl.contains("?") ? idpUrl + "&" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value()) : idpUrl + "?" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value()); + String appendOperator = idpUrl.contains("?") ? "&" : "?"; + String redirectUrl = idpUrl + appendOperator + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value()); assertEquals(redirectUrl, idpUrl + "&" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value())); } From c21bed8328c6da917cf984bc3e9ac09f29fb748f Mon Sep 17 00:00:00 2001 From: Luis Moreira Date: Wed, 29 Jun 2022 00:08:02 +0100 Subject: [PATCH 4/5] Validate the final redirect URL as a result of the method buildAuthnRequestUrl --- plugins/user-authenticators/saml2/pom.xml | 6 ++ .../org/apache/cloudstack/SAMLUtilsTest.java | 82 ++++++++++++++----- pom.xml | 1 + 3 files changed, 67 insertions(+), 22 deletions(-) diff --git a/plugins/user-authenticators/saml2/pom.xml b/plugins/user-authenticators/saml2/pom.xml index 9730a9b5817c..ade87aca704b 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/test/java/org/apache/cloudstack/SAMLUtilsTest.java b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAMLUtilsTest.java index ece9bdc64453..60dc1bf767be 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,20 +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.DefaultBootstrap; 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 { @@ -64,28 +66,64 @@ public void testBuildAuthnRequestObject() throws Exception { @Test public void testBuildAuthnRequestUrlWithoutQueryParam() throws Exception { - String consumerUrl = "http://someurl.com"; - String idpUrl = "http://idp.domain.example"; - String spId = "cloudstack"; + 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(); - DefaultBootstrap.bootstrap(); - AuthnRequest req = SAMLUtils.buildAuthnRequestObject(authnId, spId, idpUrl, consumerUrl); - String appendOperator = idpUrl.contains("?") ? "&" : "?"; - String redirectUrl = idpUrl + appendOperator + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value()); - assertEquals(redirectUrl, idpUrl + "?" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value())); + + 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); + assertEquals(urlScheme, redirectUrl.getScheme()); + assertThat(redirectUrl).hasHost(idpDomain); + assertEquals(idpDomain, redirectUrl.getHost()); + assertThat(redirectUrl).hasParameter("SAMLRequest"); } @Test public void testBuildAuthnRequestUrlWithQueryParam() throws Exception { - String consumerUrl = "http://someurl.com"; - String idpUrl = "http://idp.domain.example?idpid=CX1298373"; + 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(); - DefaultBootstrap.bootstrap(); - AuthnRequest req = SAMLUtils.buildAuthnRequestObject(authnId, spId, idpUrl, consumerUrl); - String appendOperator = idpUrl.contains("?") ? "&" : "?"; - String redirectUrl = idpUrl + appendOperator + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value()); - assertEquals(redirectUrl, idpUrl + "&" + SAMLUtils.generateSAMLRequestSignature("SAMLRequest=" + SAMLUtils.encodeSAMLRequest(req), null, SAML2AuthManager.SAMLSignatureAlgorithm.value())); + + 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); + assertEquals(urlScheme, redirectUrl.getScheme()); + assertThat(redirectUrl).hasHost(idpDomain); + assertEquals(idpDomain, redirectUrl.getHost()); + assertThat(redirectUrl).hasParameter("idpid"); + assertThat(redirectUrl).hasParameter("SAMLRequest"); } @Test diff --git a/pom.xml b/pom.xml index e309e1e02a5e..131c9886d96a 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 From 2deaefb8d7120b9ee48adebdd18b6fd28c7969b3 Mon Sep 17 00:00:00 2001 From: Luis Moreira Date: Tue, 5 Jul 2022 09:43:34 +0100 Subject: [PATCH 5/5] PR comments --- .../test/java/org/apache/cloudstack/SAMLUtilsTest.java | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) 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 60dc1bf767be..cf265199b426 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 @@ -87,11 +87,9 @@ public void testBuildAuthnRequestUrlWithoutQueryParam() throws Exception { idpMetadata.setEntityId(idpId); URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value())); - assertThat(redirectUrl).hasScheme(urlScheme); + assertThat(redirectUrl).hasScheme(urlScheme).hasHost(idpDomain).hasParameter("SAMLRequest"); assertEquals(urlScheme, redirectUrl.getScheme()); - assertThat(redirectUrl).hasHost(idpDomain); assertEquals(idpDomain, redirectUrl.getHost()); - assertThat(redirectUrl).hasParameter("SAMLRequest"); } @Test @@ -118,12 +116,9 @@ public void testBuildAuthnRequestUrlWithQueryParam() throws Exception { idpMetadata.setEntityId(idpId); URI redirectUrl = new URI(SAMLUtils.buildAuthnRequestUrl(authnId, spMetadata, idpMetadata, SAML2AuthManager.SAMLSignatureAlgorithm.value())); - assertThat(redirectUrl).hasScheme(urlScheme); + assertThat(redirectUrl).hasScheme(urlScheme).hasHost(idpDomain).hasParameter("idpid").hasParameter("SAMLRequest"); assertEquals(urlScheme, redirectUrl.getScheme()); - assertThat(redirectUrl).hasHost(idpDomain); assertEquals(idpDomain, redirectUrl.getHost()); - assertThat(redirectUrl).hasParameter("idpid"); - assertThat(redirectUrl).hasParameter("SAMLRequest"); } @Test