KEYCLOAK-12654: Data to sign is incorrect in redirect binding when URI has parameters

saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java

@@ -355,8 +355,9 @@ public class BaseSAML2BindingBuilder<T extends BaseSAML2BindingBuilder> {
 
 
     public URI generateRedirectUri(String samlParameterName, String redirectUri, Document document) throws ConfigurationException, ProcessingException, IOException {
-        KeycloakUriBuilder builder = KeycloakUriBuilder.fromUri(redirectUri)
+        KeycloakUriBuilder builder = KeycloakUriBuilder.fromUri(redirectUri);
-                .queryParam(samlParameterName, base64Encoded(document));
+        int pos = builder.getQuery() == null? 0 : builder.getQuery().length();
+        builder.queryParam(samlParameterName, base64Encoded(document));
         if (relayState != null) {
             builder.queryParam("RelayState", relayState);
         }
@@ -365,6 +366,10 @@ public class BaseSAML2BindingBuilder<T extends BaseSAML2BindingBuilder> {
             builder.queryParam(GeneralConstants.SAML_SIG_ALG_REQUEST_KEY, signatureAlgorithm.getXmlSignatureMethod());
             URI uri = builder.build();
             String rawQuery = uri.getRawQuery();
+            if (pos > 0) {
+                // just set in the signature the added SAML parameters
+                rawQuery = rawQuery.substring(pos + 1);
+            }
             Signature signature = signatureAlgorithm.createSignature();
             byte[] sig = new byte[0];
             try {

services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java

@@ -128,11 +128,14 @@ public class SamlProtocolUtils {
 
     public static void verifyRedirectSignature(SAMLDocumentHolder documentHolder, KeyLocator locator, UriInfo uriInformation, String paramKey) throws VerificationException {
         MultivaluedMap<String, String> encodedParams = uriInformation.getQueryParameters(false);
+        verifyRedirectSignature(documentHolder, locator, encodedParams, paramKey);
+    }
+
+    public static void verifyRedirectSignature(SAMLDocumentHolder documentHolder, KeyLocator locator, MultivaluedMap<String, String> encodedParams, String paramKey) throws VerificationException {
         String request = encodedParams.getFirst(paramKey);
         String algorithm = encodedParams.getFirst(GeneralConstants.SAML_SIG_ALG_REQUEST_KEY);
         String signature = encodedParams.getFirst(GeneralConstants.SAML_SIGNATURE_REQUEST_KEY);
         String relayState = encodedParams.getFirst(GeneralConstants.RELAY_STATE);
-        String decodedAlgorithm = uriInformation.getQueryParameters(true).getFirst(GeneralConstants.SAML_SIG_ALG_REQUEST_KEY);
 
         if (request == null) throw new VerificationException("SAM was null");
         if (algorithm == null) throw new VerificationException("SigAlg was null");
@@ -153,6 +156,7 @@ public class SamlProtocolUtils {
         try {
             byte[] decodedSignature = RedirectBindingUtil.urlBase64Decode(signature);
 
+            String decodedAlgorithm = RedirectBindingUtil.urlDecode(encodedParams.getFirst(GeneralConstants.SAML_SIG_ALG_REQUEST_KEY));
             SignatureAlgorithm signatureAlgorithm = SignatureAlgorithm.getFromXmlMethod(decodedAlgorithm);
             Signature validator = signatureAlgorithm.createSignature(); // todo plugin signature alg
             Key key = locator.getKey(keyId);

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SamlClient.java

@@ -49,18 +49,26 @@ import javax.ws.rs.core.Response;
 import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.net.URI;
+import java.security.Key;
+import java.security.KeyManagementException;
 import java.security.PrivateKey;
 import java.security.PublicKey;
 import java.util.Arrays;
 import java.util.LinkedList;
 import java.util.List;
-import java.util.Optional;
 import java.util.UUID;
+import javax.ws.rs.core.MultivaluedHashMap;
+import javax.ws.rs.core.MultivaluedMap;
 
 import org.jboss.logging.Logger;
 import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+import org.keycloak.common.VerificationException;
+import org.keycloak.protocol.saml.SamlProtocolUtils;
+import org.keycloak.rotation.KeyLocator;
 import static org.keycloak.saml.common.constants.GeneralConstants.RELAY_STATE;
+import org.keycloak.saml.processing.web.util.RedirectBindingUtil;
 import static org.keycloak.testsuite.util.Matchers.statusCodeIsHC;
 
 /**
@@ -106,7 +114,7 @@ public class SamlClient {
     public enum Binding {
         POST {
             @Override
-            public SAMLDocumentHolder extractResponse(CloseableHttpResponse response) throws IOException {
+            public SAMLDocumentHolder extractResponse(CloseableHttpResponse response, String realmPublicKey) throws IOException {
                 assertThat(response, statusCodeIsHC(Response.Status.OK));
                 String responsePage = EntityUtils.toString(response.getEntity(), "UTF-8");
                 response.close();
@@ -194,11 +202,11 @@ public class SamlClient {
 
         REDIRECT {
             @Override
-            public SAMLDocumentHolder extractResponse(CloseableHttpResponse response) throws IOException {
+            public SAMLDocumentHolder extractResponse(CloseableHttpResponse response, String realmPublicKey) throws IOException {
                 assertThat(response, statusCodeIsHC(Response.Status.FOUND));
                 String location = response.getFirstHeader("Location").getValue();
                 response.close();
-                return extractSamlResponseFromRedirect(location);
+                return extractSamlResponseFromRedirect(location, realmPublicKey);
             }
 
             @Override
@@ -264,12 +272,28 @@ public class SamlClient {
             }
 
             @Override
-            public HttpUriRequest createSamlSignedRequest(URI samlEndpoint, String relayState, Document samlRequest, String realmPrivateKey, String realmPublicKey) {
+            public HttpUriRequest createSamlSignedRequest(URI samlEndpoint, String relayState, Document samlRequest, String privateKeyStr, String publicKeyStr) {
-                throw new UnsupportedOperationException("Not implemented yet.");
+                try {
+                    BaseSAML2BindingBuilder binding = new BaseSAML2BindingBuilder().relayState(relayState);
+                    if (privateKeyStr != null && publicKeyStr != null) {
+                        PrivateKey privateKey = org.keycloak.testsuite.util.KeyUtils.privateKeyFromString(privateKeyStr);
+                        PublicKey publicKey = org.keycloak.testsuite.util.KeyUtils.publicKeyFromString(publicKeyStr);
+                        binding.signatureAlgorithm(SignatureAlgorithm.RSA_SHA256)
+                                .signWith(KeyUtils.createKeyId(privateKey), privateKey, publicKey)
+                                .signDocument();
+                    }
+                    return new HttpGet(binding.redirectBinding(samlRequest).requestURI(samlEndpoint.toString()));
+                } catch (IOException | ConfigurationException | ProcessingException ex) {
+                    throw new RuntimeException(ex);
+                }
             }
         };
 
-        public abstract SAMLDocumentHolder extractResponse(CloseableHttpResponse response) throws IOException;
+        public abstract SAMLDocumentHolder extractResponse(CloseableHttpResponse response, String realmPublicKey) throws IOException;
+
+        public SAMLDocumentHolder extractResponse(CloseableHttpResponse response) throws IOException {
+            return extractResponse(response, null);
+        }
 
         public abstract HttpUriRequest createSamlUnsignedRequest(URI samlEndpoint, String relayState, Document samlRequest);
 
@@ -337,24 +361,63 @@ public class SamlClient {
                 .findFirst().map(NameValuePair::getValue).orElse(null);
     }
 
+    public static MultivaluedMap<String, String> parseEncodedQueryParameters(String queryString) throws IOException {
+        MultivaluedMap<String, String> encodedParams = new MultivaluedHashMap<>();
+        if (queryString != null) {
+            String[] params = queryString.split("&");
+            for (String param : params) {
+                if (param.indexOf('=') >= 0) {
+                    String[] nv = param.split("=", 2);
+                    encodedParams.add(RedirectBindingUtil.urlDecode(nv[0]), nv.length > 1 ? nv[1] : "");
+                } else {
+                    encodedParams.add(RedirectBindingUtil.urlDecode(param), "");
+                }
+            }
+        }
+        return encodedParams;
+    }
+
     /**
      * Extracts and parses value of SAMLResponse query parameter from the given URI.
+     * If the realmPublicKey parameter is passed the response signature is
+     * validated.
      *
-     * @param responseUri
+     * @param responseUri The redirect URI to use
+     * @param realmPublicKey The public realm key for validating signature in REDIRECT query parameters
      * @return
      */
-    public static SAMLDocumentHolder extractSamlResponseFromRedirect(String responseUri) {
+    public static SAMLDocumentHolder extractSamlResponseFromRedirect(String responseUri, String realmPublicKey) throws IOException {
-        List<NameValuePair> params = URLEncodedUtils.parse(URI.create(responseUri), "UTF-8");
+        MultivaluedMap<String, String> encodedParams = parseEncodedQueryParameters(URI.create(responseUri).getRawQuery());
 
-        String samlDoc = null;
+        String samlResponse = encodedParams.getFirst(GeneralConstants.SAML_RESPONSE_KEY);
-        for (NameValuePair param : params) {
+        String samlRequest = encodedParams.getFirst(GeneralConstants.SAML_REQUEST_KEY);
-            if ("SAMLResponse".equals(param.getName()) || "SAMLRequest".equals(param.getName())) {
+        assertTrue("Only one SAMLRequest/SAMLResponse check", (samlResponse != null && samlRequest == null)
-                assertThat("Only one SAMLRequest/SAMLResponse check", samlDoc, nullValue());
+                || (samlResponse == null && samlRequest != null));
-                samlDoc = param.getValue();
+
+        String samlDoc = RedirectBindingUtil.urlDecode(samlResponse != null? samlResponse : samlRequest);
+        SAMLDocumentHolder documentHolder = SAMLRequestParser.parseResponseRedirectBinding(samlDoc);
+
+        if (realmPublicKey != null) {
+            // if the public key is passed verify the signature of the redirect URI
+            try {
+                KeyLocator locator = new KeyLocator() {
+                    @Override
+                    public Key getKey(String kid) throws KeyManagementException {
+                        return org.keycloak.testsuite.util.KeyUtils.publicKeyFromString(realmPublicKey);
+                    }
+
+                    @Override
+                    public void refreshKeyCache() {
+                    }
+                };
+                SamlProtocolUtils.verifyRedirectSignature(documentHolder, locator, encodedParams,
+                        samlResponse != null? GeneralConstants.SAML_RESPONSE_KEY : GeneralConstants.SAML_REQUEST_KEY);
+            } catch (VerificationException e) {
+                throw new IOException(e);
             }
         }
 
-        return SAMLRequestParser.parseResponseRedirectBinding(samlDoc);
+        return documentHolder;
     }
 
     /**

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SamlRedirectBindingTest.java

@@ -16,22 +16,26 @@
  */
 package org.keycloak.testsuite.saml;
 
+import java.io.IOException;
 import org.keycloak.dom.saml.v2.protocol.AuthnRequestType;
-import org.keycloak.saml.common.exceptions.ProcessingException;
-import org.keycloak.saml.common.util.DocumentUtil;
 import org.keycloak.saml.processing.api.saml.v2.request.SAML2Request;
 import org.keycloak.testsuite.util.SamlClient;
 import org.keycloak.testsuite.util.SamlClient.Binding;
-import org.keycloak.testsuite.util.SamlClientBuilder;
 import org.apache.http.client.methods.HttpUriRequest;
-import org.apache.http.util.EntityUtils;
 import org.junit.Test;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 import static org.junit.Assert.assertThat;
+import org.keycloak.dom.saml.v2.protocol.ResponseType;
+import org.keycloak.saml.common.constants.JBossSAMLURIConstants;
+import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder;
 import static org.keycloak.testsuite.saml.AbstractSamlTest.REALM_NAME;
 import static org.keycloak.testsuite.saml.AbstractSamlTest.SAML_ASSERTION_CONSUMER_URL_SALES_POST;
 import static org.keycloak.testsuite.saml.AbstractSamlTest.SAML_CLIENT_ID_SALES_POST;
+import static org.keycloak.testsuite.util.Matchers.isSamlStatusResponse;
+import org.keycloak.testsuite.util.SamlClientBuilder;
 
 /**
  *
@@ -50,4 +54,28 @@ public class SamlRedirectBindingTest extends AbstractSamlTest {
         assertThat(url, not(containsString("\r")));
         assertThat(url, not(containsString("\t")));
     }
+
+    @Test
+    public void testQueryParametersInSamlProcessingUriRedirectWithSignature() throws Exception {
+        SamlClient samlClient = new SamlClientBuilder()
+                .authnRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST_SIG,
+                        SAML_ASSERTION_CONSUMER_URL_SALES_POST_SIG + "?param1=value1&param2=value2",
+                        Binding.REDIRECT)
+                .signWith(SAML_CLIENT_SALES_POST_SIG_PRIVATE_KEY, SAML_CLIENT_SALES_POST_SIG_PUBLIC_KEY)
+                .build()
+                .login().user(bburkeUser).build().doNotFollowRedirects()
+                .execute(hr -> {
+                    try {
+                        // obtain the document validating the signature (it should be valid)
+                        SAMLDocumentHolder doc = Binding.REDIRECT.extractResponse(hr, REALM_PUBLIC_KEY);
+                        // assert doc is OK and the destination really has the extra parameters
+                        assertThat(doc.getSamlObject(), isSamlStatusResponse(JBossSAMLURIConstants.STATUS_SUCCESS));
+                        assertThat(doc.getSamlObject(), instanceOf(ResponseType.class));
+                        ResponseType res = (ResponseType) doc.getSamlObject();
+                        assertThat(res.getDestination(), is(SAML_ASSERTION_CONSUMER_URL_SALES_POST_SIG + "?param1=value1&param2=value2"));
+                    } catch (IOException e) {
+                        throw new IllegalStateException(e);
+                    }
+                });
+    }
 }