[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

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 {

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);
+    }
 }

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 <a href="mailto:sguilhen@redhat.com">Stefan Guilhen</a>
+ */
+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<String> 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);
+    }
+
+}