You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by ro...@apache.org on 2022/07/06 03:58:43 UTC
[cloudstack] branch 4.17 updated: saml: Fix SAML SSO plugin redirect URL (#6457)
This is an automated email from the ASF dual-hosted git repository.
rohit pushed a commit to branch 4.17
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.17 by this push:
new c6b611433b saml: Fix SAML SSO plugin redirect URL (#6457)
c6b611433b is described below
commit c6b611433b98298fbdf3d6e37fa64051a13b4e2f
Author: Luis Moreira <Lu...@users.noreply.github.com>
AuthorDate: Wed Jul 6 04:58:37 2022 +0100
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=<ID>, therefore the expected redirect URL would be https://accounts.google.com/o/saml2/idp?idpid=<ID>&SAMLRequest=<SAMLRequest>
This code change is backwards compatible with the current behaviour.
---
plugins/user-authenticators/saml2/pom.xml | 6 ++
.../java/org/apache/cloudstack/saml/SAMLUtils.java | 3 +-
.../java/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 9cfe5cb390..11e34ee4a9 100644
--- a/plugins/user-authenticators/saml2/pom.xml
+++ b/plugins/user-authenticators/saml2/pom.xml
@@ -53,5 +53,11 @@
<version>${cs.xercesImpl.version}</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.assertj</groupId>
+ <artifactId>assertj-core</artifactId>
+ <version>${cs.assertj.version}</version>
+ <scope>test</scope>
+ </dependency>
</dependencies>
</project>
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 cbbdbd28bf..c65e4c09be 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 433fdf3224..cf265199b4 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 08d8d0ba70..eb67edc485 100644
--- a/pom.xml
+++ b/pom.xml
@@ -118,6 +118,7 @@
<cs.testng.version>7.1.0</cs.testng.version>
<cs.wiremock.version>2.27.2</cs.wiremock.version>
<cs.xercesImpl.version>2.12.2</cs.xercesImpl.version>
+ <cs.assertj.version>3.23.1</cs.assertj.version>
<!-- Dependencies versions -->
<cs.amqp-client.version>5.10.0</cs.amqp-client.version>