From 0eb0281bf222d7781dbf2bdc084a4f73f06c8bc7 Mon Sep 17 00:00:00 2001 From: Steven Hawkins Date: Fri, 22 Nov 2024 14:57:20 -0500 Subject: [PATCH] fix: returning addresses instead of hosts on the ClientConnection (#35247) also consolidates checks of whether a host or address is local closes: #35216 closes: #34671 Signed-off-by: Steve Hawkins --- .../org/keycloak/common/ClientConnection.java | 3 ++ .../keycloak/common/enums/SslRequired.java | 16 +++++++--- .../common/enums/SslRequiredTest.java | 20 ++++++++++++ .../resteasy/QuarkusClientConnection.java | 4 +-- .../it/cli/dist/ProxyHostnameV2DistTest.java | 22 +++++++++---- .../services/resources/WelcomeResource.java | 31 +++++++------------ .../keycloak/utils/SecureContextResolver.java | 30 +++++++++++++----- .../utils/SecureContextResolverTest.java | 12 +++++++ 8 files changed, 98 insertions(+), 40 deletions(-) create mode 100644 common/src/test/java/org/keycloak/common/enums/SslRequiredTest.java diff --git a/common/src/main/java/org/keycloak/common/ClientConnection.java b/common/src/main/java/org/keycloak/common/ClientConnection.java index 5afbed6741..3e6c124da8 100755 --- a/common/src/main/java/org/keycloak/common/ClientConnection.java +++ b/common/src/main/java/org/keycloak/common/ClientConnection.java @@ -25,6 +25,9 @@ package org.keycloak.common; */ public interface ClientConnection { + /** + * @return the address as a string if it is available, otherwise null + */ String getRemoteAddr(); String getRemoteHost(); int getRemotePort(); diff --git a/common/src/main/java/org/keycloak/common/enums/SslRequired.java b/common/src/main/java/org/keycloak/common/enums/SslRequired.java index 4c5519db22..c928438ec2 100644 --- a/common/src/main/java/org/keycloak/common/enums/SslRequired.java +++ b/common/src/main/java/org/keycloak/common/enums/SslRequired.java @@ -35,23 +35,29 @@ public enum SslRequired { return isRequired(connection.getRemoteAddr()); } - public boolean isRequired(String address) { + public boolean isRequired(String host) { switch (this) { case ALL: return true; case NONE: return false; case EXTERNAL: - return !isLocal(address); + // NOTE: this is sometimes using hostnames here, which require DNS resolution + // It assumes that the resolution will be the same on the client side + // - this will go away once EXTERNAL is no longer supported + return !isLocal(host); default: return true; } } - private boolean isLocal(String remoteAddress) { + private boolean isLocal(String host) { + if (host == null || host.isEmpty()) { + return false; // InetAddress.getByName returns localhost for these + } try { - InetAddress inetAddress = InetAddress.getByName(remoteAddress); - return inetAddress.isAnyLocalAddress() || inetAddress.isLoopbackAddress() || inetAddress.isSiteLocalAddress() || inetAddress.isLinkLocalAddress() || isUniqueLocal(inetAddress); + InetAddress inetAddress = InetAddress.getByName(host); + return inetAddress.isLoopbackAddress() || inetAddress.isSiteLocalAddress() || inetAddress.isLinkLocalAddress() || isUniqueLocal(inetAddress); } catch (UnknownHostException e) { return false; } diff --git a/common/src/test/java/org/keycloak/common/enums/SslRequiredTest.java b/common/src/test/java/org/keycloak/common/enums/SslRequiredTest.java new file mode 100644 index 0000000000..346a4eaade --- /dev/null +++ b/common/src/test/java/org/keycloak/common/enums/SslRequiredTest.java @@ -0,0 +1,20 @@ +package org.keycloak.common.enums; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; + +import org.junit.Test; + +public class SslRequiredTest { + + @Test + public void sslRequiredExternalTest() throws IOException { + assertFalse(SslRequired.EXTERNAL.isRequired("127.0.0.1")); + assertTrue(SslRequired.EXTERNAL.isRequired((String)null)); + assertTrue(SslRequired.EXTERNAL.isRequired("")); + assertTrue(SslRequired.EXTERNAL.isRequired("0.0.0.0")); + } + +} diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/resteasy/QuarkusClientConnection.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/resteasy/QuarkusClientConnection.java index ddbea53514..c1753640c5 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/resteasy/QuarkusClientConnection.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/resteasy/QuarkusClientConnection.java @@ -31,7 +31,7 @@ public final class QuarkusClientConnection implements ClientConnection { @Override public String getRemoteAddr() { - return request.remoteAddress().host(); + return request.remoteAddress().hostAddress(); } @Override @@ -46,7 +46,7 @@ public final class QuarkusClientConnection implements ClientConnection { @Override public String getLocalAddr() { - return request.localAddress().host(); + return request.localAddress().hostAddress(); } @Override diff --git a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ProxyHostnameV2DistTest.java b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ProxyHostnameV2DistTest.java index ab6fe05a6d..72062e64d0 100644 --- a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ProxyHostnameV2DistTest.java +++ b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ProxyHostnameV2DistTest.java @@ -18,6 +18,7 @@ package org.keycloak.it.cli.dist; import io.quarkus.test.junit.main.Launch; +import io.quarkus.test.junit.main.LaunchResult; import io.restassured.RestAssured; import io.restassured.config.RedirectConfig; import io.restassured.config.RestAssuredConfig; @@ -41,6 +42,9 @@ import static org.hamcrest.Matchers.containsString; @RawDistOnly(reason = "Containers are immutable") public class ProxyHostnameV2DistTest { + private static final String ADDRESS = "12.23.45.67"; + private static final String NOT_ADDRESS = "notaddress"; + @BeforeAll public static void onBeforeAll() { RestAssured.useRelaxedHTTPSValidation(); @@ -78,10 +82,14 @@ public class ProxyHostnameV2DistTest { } @Test - @Launch({ "start-dev", "--hostname-strict=false", "--proxy-headers=forwarded" }) - public void testForwardedProxyHeaders() { + @Launch({ "start-dev", "--hostname-strict=false", "--proxy-headers=forwarded", "--spi-event-listener-provider=jboss-logging" }) + public void testForwardedProxyHeaders(LaunchResult result) { assertForwardedHeader(); assertXForwardedHeadersAreIgnored(); + + CLIResult cliResult = (CLIResult)result; + cliResult.assertNoMessage(NOT_ADDRESS); + cliResult.assertMessage(ADDRESS); } @Test @@ -92,13 +100,15 @@ public class ProxyHostnameV2DistTest { } private void assertForwardedHeader() { - assertForwardedHeader("https://test:1234/admin"); + // trigger a login error + assertForwardedHeader("http://mykeycloak.org:8080/realms/master/protocol/openid-connect/auth?client_id=security-admin-console", "https://test:1234/admin", ADDRESS); + assertForwardedHeader("http://mykeycloak.org:8080/realms/master/protocol/openid-connect/auth?client_id=security-admin-console", "https://test:1234/admin", NOT_ADDRESS); } - private void assertForwardedHeader(String expectedUrl) { + private void assertForwardedHeader(String url, String expectedUrl, String forAddress) { given() - .header("Forwarded", "for=12.34.56.78;host=test:1234;proto=https, for=23.45.67.89") - .when().get("http://mykeycloak.org:8080") + .header("Forwarded", "for="+forAddress+";host=test:1234;proto=https, for=23.45.67.89") + .when().get(url) .then().header(HttpHeaders.LOCATION, containsString(expectedUrl)); } diff --git a/services/src/main/java/org/keycloak/services/resources/WelcomeResource.java b/services/src/main/java/org/keycloak/services/resources/WelcomeResource.java index 43f9383e66..700afe5130 100755 --- a/services/src/main/java/org/keycloak/services/resources/WelcomeResource.java +++ b/services/src/main/java/org/keycloak/services/resources/WelcomeResource.java @@ -51,13 +51,12 @@ import org.keycloak.theme.Theme; import org.keycloak.theme.freemarker.FreeMarkerProvider; import org.keycloak.urls.UrlType; import org.keycloak.utils.MediaType; +import org.keycloak.utils.SecureContextResolver; import java.io.IOException; import java.io.InputStream; -import java.net.InetAddress; import java.net.URI; import java.net.URISyntaxException; -import java.net.UnknownHostException; import java.util.HashMap; import java.util.Map; import java.util.Objects; @@ -271,25 +270,17 @@ public class WelcomeResource { } private boolean isLocal() { - try { - ClientConnection clientConnection = session.getContext().getConnection(); - InetAddress remoteInetAddress = InetAddress.getByName(clientConnection.getRemoteAddr()); - InetAddress localInetAddress = InetAddress.getByName(clientConnection.getLocalAddr()); - HttpRequest request = session.getContext().getHttpRequest(); - HttpHeaders headers = request.getHttpHeaders(); - String xForwardedFor = headers.getHeaderString("X-Forwarded-For"); - logger.debugf("Checking WelcomePage. Remote address: %s, Local address: %s, X-Forwarded-For header: %s", remoteInetAddress.toString(), localInetAddress.toString(), xForwardedFor); + ClientConnection clientConnection = session.getContext().getConnection(); + String remoteAddress = clientConnection.getRemoteAddr(); + String localAddress = clientConnection.getLocalAddr(); + HttpRequest request = session.getContext().getHttpRequest(); + HttpHeaders headers = request.getHttpHeaders(); + String xForwardedFor = headers.getHeaderString("X-Forwarded-For"); + String forwarded = headers.getHeaderString("Forwarded"); + logger.debugf("Checking WelcomePage. Remote address: %s, Local address: %s, X-Forwarded-For header: %s, Forwarded header: %s", remoteAddress.toString(), localAddress.toString(), xForwardedFor, forwarded); - // Access through AJP protocol (loadbalancer) may cause that remoteAddress is "127.0.0.1". - // So consider that welcome page accessed locally just if it was accessed really through "localhost" URL and without loadbalancer (x-forwarded-for header is empty). - return isLocalAddress(remoteInetAddress) && isLocalAddress(localInetAddress) && xForwardedFor == null; - } catch (UnknownHostException e) { - throw new WebApplicationException(e, Response.Status.INTERNAL_SERVER_ERROR); - } - } - - private boolean isLocalAddress(InetAddress inetAddress) { - return inetAddress.isAnyLocalAddress() || inetAddress.isLoopbackAddress(); + // Consider that welcome page accessed locally just if it was accessed really through "localhost" URL and without loadbalancer (x-forwarded-for and forwarded header is empty). + return xForwardedFor == null && forwarded == null && SecureContextResolver.isLocalAddress(remoteAddress) && SecureContextResolver.isLocalAddress(localAddress); } private String setCsrfCookie() { diff --git a/services/src/main/java/org/keycloak/utils/SecureContextResolver.java b/services/src/main/java/org/keycloak/utils/SecureContextResolver.java index 128c13c64c..29e9eca1cb 100644 --- a/services/src/main/java/org/keycloak/utils/SecureContextResolver.java +++ b/services/src/main/java/org/keycloak/utils/SecureContextResolver.java @@ -57,13 +57,7 @@ public class SecureContextResolver { return false; } - // The host matches a CIDR notation of ::1/128 - if (host.equals("[::1]") || host.equals("[0000:0000:0000:0000:0000:0000:0000:0001]")) { - return true; - } - - // The host matches a CIDR notation of 127.0.0.0/8 - if (LOCALHOST_IPV4.matcher(host).matches()) { + if (isLocalAddress(host)) { return true; } @@ -73,4 +67,26 @@ public class SecureContextResolver { return host.endsWith(".localhost") || host.endsWith(".localhost."); } + + /** + * Test whether the given address is the localhost + * @param address + * @return false if the address is not localhost or not an address value + */ + public static boolean isLocalAddress(String address) { + if (address == null) { + return false; + } + // The host matches a CIDR notation of ::1/128 + if (address.equals("[::1]") || address.equals("[0000:0000:0000:0000:0000:0000:0000:0001]")) { + return true; + } + + // The host matches a CIDR notation of 127.0.0.0/8 + if (LOCALHOST_IPV4.matcher(address).matches()) { + return true; + } + + return false; + } } diff --git a/services/src/test/java/org/keycloak/utils/SecureContextResolverTest.java b/services/src/test/java/org/keycloak/utils/SecureContextResolverTest.java index 55e96af86d..fdf792c5e1 100644 --- a/services/src/test/java/org/keycloak/utils/SecureContextResolverTest.java +++ b/services/src/test/java/org/keycloak/utils/SecureContextResolverTest.java @@ -4,6 +4,9 @@ import org.junit.Assert; import org.junit.Test; import org.keycloak.representations.account.DeviceRepresentation; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + import java.net.URI; import java.net.URISyntaxException; import java.util.function.Supplier; @@ -47,6 +50,7 @@ public class SecureContextResolverTest { assertSecureContext("http://[::2]", false); assertSecureContext("http://[2001:0000:130F:0000:0000:09C0:876A:130B]", false); assertSecureContext("http://::1", false); + assertSecureContext("http://[FE80:0000:130F:0000:0000:09C0:876A:130B]", false); } @Test @@ -59,6 +63,14 @@ public class SecureContextResolverTest { assertSecureContext("http://test.localhostn", false); assertSecureContext("http://test.localhost.not", false); } + + @Test + public void testIsLocalhost() { + assertTrue(SecureContextResolver.isLocalAddress("127.0.0.1")); + assertFalse(SecureContextResolver.isLocalAddress("not.an.ip")); + assertFalse(SecureContextResolver.isLocalAddress(null)); + assertFalse(SecureContextResolver.isLocalAddress("")); + } @Test public void testQuirksSafari() {