Apple base416 saml fixes (#236)

* Add EncryptedElementType key resolver to SAML plugin * 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. * Apply backport for SAML session cookie path https://github.com/apache/cloudstack/pull/6149 * ui: Logout before login (#6193) This PR calls the logout API before login, to cleanup any duplicate sessionkey, as it was done on the legacy UI: #4326 Fixes: #6127 --------- Co-authored-by: Marcus Sorensen <mls@apple.com> Co-authored-by: Luis Moreira <Luis-3M@users.noreply.github.com> Co-authored-by: Nicolas Vazquez <nicovazquez90@gmail.com>
2026-08-02 05:26:35 +00:00 · 2023-02-23 09:40:09 -07:00 · 2023-02-23 09:40:09 -07:00 · 6df183f407
commit 6df183f407
parent 0b7619b36d
6 changed files with 81 additions and 5 deletions
--- a/plugins/user-authenticators/saml2/pom.xml
+++ b/plugins/user-authenticators/saml2/pom.xml
@ -54,5 +54,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>
--- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java
+++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java
@ -55,9 +55,10 @@ import org.opensaml.saml2.core.Issuer;
 import org.opensaml.saml2.core.Response;
 import org.opensaml.saml2.core.StatusCode;
 import org.opensaml.saml2.encryption.Decrypter;
+import org.opensaml.saml2.encryption.EncryptedElementTypeEncryptedKeyResolver;
 import org.opensaml.xml.ConfigurationException;
+import org.opensaml.xml.encryption.ChainingEncryptedKeyResolver;
 import org.opensaml.xml.encryption.DecryptionException;
-import org.opensaml.xml.encryption.EncryptedKeyResolver;
 import org.opensaml.xml.encryption.InlineEncryptedKeyResolver;
 import org.opensaml.xml.io.UnmarshallingException;
 import org.opensaml.xml.security.SecurityHelper;
@ -253,7 +254,9 @@ public class SAML2LoginAPIAuthenticatorCmd extends BaseCmd implements APIAuthent
                    Credential credential = SecurityHelper.getSimpleCredential(idpMetadata.getEncryptionCertificate().getPublicKey(),
                            spMetadata.getKeyPair().getPrivate());
                    StaticKeyInfoCredentialResolver keyInfoResolver = new StaticKeyInfoCredentialResolver(credential);
-                    EncryptedKeyResolver keyResolver = new InlineEncryptedKeyResolver();
+                    ChainingEncryptedKeyResolver keyResolver = new ChainingEncryptedKeyResolver();
+                    keyResolver.getResolverChain().add(new InlineEncryptedKeyResolver());
+                    keyResolver.getResolverChain().add(new EncryptedElementTypeEncryptedKeyResolver());
                    Decrypter decrypter = new Decrypter(null, keyInfoResolver, keyResolver);
                    decrypter.setRootInNewDocument(true);
                    List<EncryptedAssertion> encryptedAssertions = processedSAMLResponse.getEncryptedAssertions();
--- 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
@ -152,7 +152,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());
        }
@ -285,7 +286,7 @@ public class SAMLUtils {
            resp.addCookie(new Cookie("timezone", URLEncoder.encode(timezone, HttpUtils.UTF_8)));
        }
        resp.addCookie(new Cookie("userfullname", URLEncoder.encode(loginResponse.getFirstName() + " " + loginResponse.getLastName(), HttpUtils.UTF_8).replace("+", "%20")));
-        resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly", ApiConstants.SESSIONKEY, loginResponse.getSessionKey()));
+        resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly;Path=/client/api", ApiConstants.SESSIONKEY, loginResponse.getSessionKey()));
    }

    /**
--- 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
@ -23,13 +23,18 @@ import java.security.KeyPair;
 import java.security.PrivateKey;
 import java.security.PublicKey;

+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 static org.assertj.core.api.Assertions.assertThat;

 public class SAMLUtilsTest extends TestCase {

@ -50,6 +55,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";
--- a/pom.xml
+++ b/pom.xml
@ -116,6 +116,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>
--- a/ui/src/api/index.js
+++ b/ui/src/api/index.js
@ -43,6 +43,9 @@ export function api (command, args = {}, method = 'GET', data = {}) {
 export function login (arg) {
  sourceToken.init()

+  // Logout before login is called to purge any duplicate sessionkey cookies
+  api('logout')
+
  const params = new URLSearchParams()
  params.append('command', 'login')
  params.append('username', arg.username || arg.email)