From 7a3998870ca71ffdf94b491f7f105dfc7f24649b Mon Sep 17 00:00:00 2001 From: Stefan Guilhen Date: Fri, 14 Feb 2020 15:51:19 -0300 Subject: [PATCH] [KEYCLOAK-12612][KEYCLOAK-12944] Fix validation of SAML destination URLs - no longer compare them to the server absolutePath; instead use the base URI to build the validation URL --- .../keycloak/protocol/saml/SamlService.java | 21 ++- .../main/java/org/keycloak/services/Urls.java | 5 + .../testsuite/saml/SamlReverseProxyTest.java | 146 ++++++++++++++++++ 3 files changed, 168 insertions(+), 4 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SamlReverseProxyTest.java diff --git a/services/src/main/java/org/keycloak/protocol/saml/SamlService.java b/services/src/main/java/org/keycloak/protocol/saml/SamlService.java index 18b2593eec..76d15ab7da 100755 --- a/services/src/main/java/org/keycloak/protocol/saml/SamlService.java +++ b/services/src/main/java/org/keycloak/protocol/saml/SamlService.java @@ -41,6 +41,7 @@ import org.keycloak.models.AuthenticatedClientSessionModel; import org.keycloak.models.ClientModel; import org.keycloak.models.KeyManager; import org.keycloak.models.KeycloakSession; +import org.keycloak.models.KeycloakUriInfo; import org.keycloak.models.RealmModel; import org.keycloak.models.UserSessionModel; import org.keycloak.protocol.AuthorizationEndpointBase; @@ -55,6 +56,7 @@ import org.keycloak.saml.common.constants.GeneralConstants; import org.keycloak.saml.common.constants.JBossSAMLURIConstants; import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder; import org.keycloak.services.ErrorPage; +import org.keycloak.services.Urls; import org.keycloak.services.managers.AuthenticationManager; import org.keycloak.services.messages.Messages; import org.keycloak.services.resources.RealmsResource; @@ -92,7 +94,6 @@ import org.keycloak.saml.processing.core.util.KeycloakKeySamlExtensionGenerator; import org.keycloak.saml.validators.DestinationValidator; import org.keycloak.sessions.AuthenticationSessionModel; import java.nio.charset.StandardCharsets; -import java.util.logging.Level; /** * Resource class for the saml connect token service @@ -151,7 +152,7 @@ public class SamlService extends AuthorizationEndpointBase { StatusResponseType statusResponse = (StatusResponseType) holder.getSamlObject(); // validate destination - if (! destinationValidator.validate(session.getContext().getUri().getAbsolutePath(), statusResponse.getDestination())) { + if (! destinationValidator.validate(this.getExpectedDestinationUri(session), statusResponse.getDestination())) { event.detail(Details.REASON, "invalid_destination"); event.error(Errors.INVALID_SAML_LOGOUT_RESPONSE); return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); @@ -288,7 +289,7 @@ public class SamlService extends AuthorizationEndpointBase { event.error(Errors.INVALID_SAML_AUTHN_REQUEST); return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); } - if (! destinationValidator.validate(session.getContext().getUri().getAbsolutePath(), requestAbstractType.getDestination())) { + if (! destinationValidator.validate(this.getExpectedDestinationUri(session), requestAbstractType.getDestination())) { event.detail(Details.REASON, "invalid_destination"); event.error(Errors.INVALID_SAML_AUTHN_REQUEST); return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); @@ -402,7 +403,7 @@ public class SamlService extends AuthorizationEndpointBase { event.error(Errors.INVALID_SAML_LOGOUT_REQUEST); return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); } - if (! destinationValidator.validate(logoutRequest.getDestination(), session.getContext().getUri().getAbsolutePath())) { + if (! destinationValidator.validate(this.getExpectedDestinationUri(session), logoutRequest.getDestination())) { event.detail(Details.REASON, "invalid_destination"); event.error(Errors.INVALID_SAML_LOGOUT_REQUEST); return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST); @@ -517,6 +518,18 @@ public class SamlService extends AuthorizationEndpointBase { else return handleSamlResponse(samlResponse, relayState); } + + /** + * KEYCLOAK-12616, KEYCLOAK-12944: construct the expected destination URI using the configured base URI. + * + * @param session a reference to the {@link KeycloakSession}. + * @return the constructed {@link URI}. + */ + protected URI getExpectedDestinationUri(final KeycloakSession session) { + final String realmName = session.getContext().getRealm().getName(); + final URI baseUri = session.getContext().getUri().getBaseUri(); + return Urls.samlRequestEndpoint(baseUri, realmName); + } } protected class PostBindingProtocol extends BindingProtocol { diff --git a/services/src/main/java/org/keycloak/services/Urls.java b/services/src/main/java/org/keycloak/services/Urls.java index 49cb044405..5630ed2135 100755 --- a/services/src/main/java/org/keycloak/services/Urls.java +++ b/services/src/main/java/org/keycloak/services/Urls.java @@ -20,6 +20,7 @@ import org.keycloak.common.Version; import org.keycloak.models.Constants; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.protocol.oidc.OIDCLoginProtocolService; +import org.keycloak.protocol.saml.SamlProtocol; import org.keycloak.services.resources.account.AccountFormService; import org.keycloak.services.resources.IdentityBrokerService; import org.keycloak.services.resources.LoginActionsService; @@ -268,4 +269,8 @@ public class Urls { private static UriBuilder themeBase(URI baseUri) { return UriBuilder.fromUri(baseUri).path(ThemeResource.class); } + + public static URI samlRequestEndpoint(final URI baseUri, final String realmName) { + return realmBase(baseUri).path(RealmsResource.class, "getProtocol").build(realmName, SamlProtocol.LOGIN_PROTOCOL); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SamlReverseProxyTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SamlReverseProxyTest.java new file mode 100644 index 0000000000..16d2f66251 --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SamlReverseProxyTest.java @@ -0,0 +1,146 @@ +/* + * Copyright 2020 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.keycloak.testsuite.saml; + +import java.net.URI; +import java.util.HashMap; + +import javax.ws.rs.core.Response; +import javax.ws.rs.core.UriBuilder; + +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpUriRequest; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClientBuilder; +import org.apache.http.util.EntityUtils; +import org.hamcrest.Matcher; +import org.junit.ClassRule; +import org.junit.Test; +import org.keycloak.protocol.saml.SamlProtocol; +import org.keycloak.representations.idm.RealmRepresentation; +import org.keycloak.saml.SAML2LogoutRequestBuilder; +import org.keycloak.saml.processing.api.saml.v2.request.SAML2Request; +import org.keycloak.services.resources.RealmsResource; +import org.keycloak.testsuite.util.ReverseProxy; +import org.keycloak.testsuite.util.SamlClient; +import org.w3c.dom.Document; + +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertThat; +import static org.keycloak.testsuite.util.Matchers.statusCodeIsHC; + +/** + * SAML tests using a {@code frontendUrl} that points to a reverse proxy. The SAML request destination should be validated + * against the proxy address and any redirection should also have the proxy as target. + * + * @author Stefan Guilhen + */ +public class SamlReverseProxyTest extends AbstractSamlTest { + + @ClassRule + public static ReverseProxy proxy = new ReverseProxy(); + + /** + * KEYCLOAK-12612 + * + * Tests sending a SAML {@code AuthnRequest} through a reverse proxy. In this scenario the SAML {@code AuthnRequest} + * has a destination that matches the proxy server, but the request is forwarded to a keycloak server running in a + * different address. + * + * Validation of the destination and subsequent redirection to the login screen only work if the proxy server is configured + * as the {@code frontendUrl} of the realm. + * + * @throws Exception if an error occurs while running the test. + */ + @Test + public void testAuthnRequestWithReverseProxy() throws Exception { + // send an authn request without defining the frontendUrl for the realm - should get a BAD_REQUEST response + Document document = SAML2Request.convert(SamlClient.createLoginRequestDocument(SAML_CLIENT_ID_SALES_POST, + SAML_ASSERTION_CONSUMER_URL_SALES_POST, this.buildSamlProtocolUrl(proxy.getUrl()))); + testSendSamlRequest(document, Response.Status.BAD_REQUEST, containsString("Invalid Request")); + + // set the frontendUrl pointing to the reverse proxy + RealmRepresentation rep = adminClient.realm(REALM_NAME).toRepresentation(); + try { + if (rep.getAttributes() == null) { + rep.setAttributes(new HashMap<>()); + } + rep.getAttributes().put("frontendUrl", proxy.getUrl()); + adminClient.realm(REALM_NAME).update(rep); + + // resend the authn request - should succeed this time + testSendSamlRequest(document, Response.Status.OK, containsString("login")); + } finally { + // restore the state of the realm (unset the frontendUrl) + rep.getAttributes().remove("frontendUrl"); + adminClient.realm(REALM_NAME).update(rep); + } + } + + /** + * KEYCLOAK-12944 + * + * Tests sending a SAML {@code LogoutRequest} through a reverse proxy. In this scenario the SAML {@code LogoutRequest} + * has a destination that matches the proxy server, but the request is forwarded to a keycloak server running in a + * different address. + * + * Validation of the destination and any subsequent redirection only work if the proxy server is configured as the + * {@code frontendUrl} of the realm. + * + * @throws Exception if an error occurs while running the test. + */ + @Test + public void testLogoutRequestWithReverseProxy() throws Exception { + // send a logout request without defining the frontendUrl for the realm - should get a BAD_REQUEST response + Document document = new SAML2LogoutRequestBuilder().destination( + this.buildSamlProtocolUrl(proxy.getUrl()).toString()).issuer(SAML_CLIENT_ID_SALES_POST).buildDocument(); + testSendSamlRequest(document, Response.Status.BAD_REQUEST, containsString("Invalid Request")); + + // set the frontendUrl pointing to the reverse proxy + RealmRepresentation rep = adminClient.realm(REALM_NAME).toRepresentation(); + try { + if (rep.getAttributes() == null) { + rep.setAttributes(new HashMap<>()); + } + rep.getAttributes().put("frontendUrl", proxy.getUrl()); + adminClient.realm(REALM_NAME).update(rep); + + // resend the logout request - should succeed this time (we are actually not logging out anyone, just checking the request is properly validated + testSendSamlRequest(document, Response.Status.OK, containsString("login")); + } finally { + // restore the state of the realm (unset the frontendUrl) + rep.getAttributes().remove("frontendUrl"); + adminClient.realm(REALM_NAME).update(rep); + } + } + + private void testSendSamlRequest(final Document doc, final Response.Status expectedHttpCode, final Matcher pageTextMatcher) throws Exception { + HttpUriRequest post = + SamlClient.Binding.POST.createSamlUnsignedRequest(this.buildSamlProtocolUrl(proxy.getUrl()), null, doc); + try (CloseableHttpClient client = HttpClientBuilder.create().setSSLHostnameVerifier((s, sslSession) -> true). + setRedirectStrategy(new SamlClient.RedirectStrategyWithSwitchableFollowRedirect()).build(); + CloseableHttpResponse response = client.execute(post)) { + assertThat(response, statusCodeIsHC(expectedHttpCode)); + assertThat(EntityUtils.toString(response.getEntity(), "UTF-8"), pageTextMatcher); + } + } + + private URI buildSamlProtocolUrl(final String baseUri) { + return RealmsResource.protocolUrl(UriBuilder.fromUri(baseUri)).build(REALM_NAME, SamlProtocol.LOGIN_PROTOCOL); + } + +}