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.
This commit is contained in:
Luis Moreira 2022-07-06 04:58:37 +01:00 committed by GitHub
parent f1bce69b5d
commit c6b611433b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 76 additions and 7 deletions

View File

@ -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>

View File

@ -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());
}

View File

@ -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";

View File

@ -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>