From d39dfd86888d292ebdafd6b5b98be2436b64846c Mon Sep 17 00:00:00 2001 From: rmartinc Date: Wed, 15 Jan 2020 13:49:33 +0100 Subject: [PATCH] KEYCLOAK-12654: Data to sign is incorrect in redirect binding when URI has parameters --- .../saml/BaseSAML2BindingBuilder.java | 9 +- .../protocol/saml/SamlProtocolUtils.java | 6 +- .../keycloak/testsuite/util/SamlClient.java | 95 +++++++++++++++---- .../saml/SamlRedirectBindingTest.java | 36 ++++++- 4 files changed, 123 insertions(+), 23 deletions(-) diff --git a/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java b/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java index 9a417522f6..da366a33f0 100755 --- a/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java +++ b/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java @@ -355,8 +355,9 @@ public class BaseSAML2BindingBuilder { public URI generateRedirectUri(String samlParameterName, String redirectUri, Document document) throws ConfigurationException, ProcessingException, IOException { - KeycloakUriBuilder builder = KeycloakUriBuilder.fromUri(redirectUri) - .queryParam(samlParameterName, base64Encoded(document)); + KeycloakUriBuilder builder = KeycloakUriBuilder.fromUri(redirectUri); + 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 { 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 { diff --git a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java b/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java index 825a31e94b..dd5eb7aaf6 100755 --- a/services/src/main/java/org/keycloak/protocol/saml/SamlProtocolUtils.java +++ b/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 encodedParams = uriInformation.getQueryParameters(false); + verifyRedirectSignature(documentHolder, locator, encodedParams, paramKey); + } + + public static void verifyRedirectSignature(SAMLDocumentHolder documentHolder, KeyLocator locator, MultivaluedMap 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); diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SamlClient.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SamlClient.java index 06ed39a749..a9d28bc0f2 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SamlClient.java +++ b/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) { - throw new UnsupportedOperationException("Not implemented yet."); + public HttpUriRequest createSamlSignedRequest(URI samlEndpoint, String relayState, Document samlRequest, String privateKeyStr, String publicKeyStr) { + 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 parseEncodedQueryParameters(String queryString) throws IOException { + MultivaluedMap 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) { - List params = URLEncodedUtils.parse(URI.create(responseUri), "UTF-8"); + public static SAMLDocumentHolder extractSamlResponseFromRedirect(String responseUri, String realmPublicKey) throws IOException { + MultivaluedMap encodedParams = parseEncodedQueryParameters(URI.create(responseUri).getRawQuery()); - String samlDoc = null; - for (NameValuePair param : params) { - if ("SAMLResponse".equals(param.getName()) || "SAMLRequest".equals(param.getName())) { - assertThat("Only one SAMLRequest/SAMLResponse check", samlDoc, nullValue()); - samlDoc = param.getValue(); + String samlResponse = encodedParams.getFirst(GeneralConstants.SAML_RESPONSE_KEY); + String samlRequest = encodedParams.getFirst(GeneralConstants.SAML_REQUEST_KEY); + assertTrue("Only one SAMLRequest/SAMLResponse check", (samlResponse != null && samlRequest == null) + || (samlResponse == null && samlRequest != null)); + + 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; } /** diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SamlRedirectBindingTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SamlRedirectBindingTest.java index 9cd401b39a..47ba92f8ac 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SamlRedirectBindingTest.java +++ b/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¶m2=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¶m2=value2")); + } catch (IOException e) { + throw new IllegalStateException(e); + } + }); + } }