From 7bf78a431ee83badb6284eb86f2cb97e4db1b1f1 Mon Sep 17 00:00:00 2001 From: "Fitzgerald, Andrew" Date: Sat, 13 Jan 2018 23:35:39 -0500 Subject: [PATCH 1/2] Update `RemoteAddrRoutePredicateFactory` to optionally respect the `X-Forwarded-For` header. Fixes gh-155. --- .../RemoteAddrRoutePredicateFactory.java | 14 +++++- .../DefaultRemoteAddressResolver.java | 16 +++++++ .../ipresolver/RemoteAddressResolver.java | 13 +++++ .../XForwardedRemoteAddressResolver.java | 34 ++++++++++++++ .../XForwardedRemoteAddressResolverTest.java | 47 +++++++++++++++++++ 5 files changed, 122 insertions(+), 2 deletions(-) create mode 100644 spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/DefaultRemoteAddressResolver.java create mode 100644 spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/RemoteAddressResolver.java create mode 100644 spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolver.java create mode 100644 spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolverTest.java diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/handler/predicate/RemoteAddrRoutePredicateFactory.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/handler/predicate/RemoteAddrRoutePredicateFactory.java index f8cd892fd3..de0750aee5 100644 --- a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/handler/predicate/RemoteAddrRoutePredicateFactory.java +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/handler/predicate/RemoteAddrRoutePredicateFactory.java @@ -25,10 +25,12 @@ import java.util.function.Predicate; import javax.validation.constraints.NotEmpty; +import javax.validation.constraints.NotNull; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.jetbrains.annotations.NotNull; +import org.springframework.cloud.gateway.support.ipresolver.DefaultRemoteAddressResolver; +import org.springframework.cloud.gateway.support.ipresolver.RemoteAddressResolver; import org.springframework.validation.annotation.Validated; import org.springframework.web.server.ServerWebExchange; @@ -72,7 +74,7 @@ public Predicate apply(Config config) { List sources = convert(config.sources); return exchange -> { - InetSocketAddress remoteAddress = exchange.getRequest().getRemoteAddress(); + InetSocketAddress remoteAddress = config.remoteAddressResolver.resolve(exchange); if (remoteAddress != null) { String hostAddress = remoteAddress.getAddress().getHostAddress(); String host = exchange.getRequest().getURI().getHost(); @@ -109,6 +111,9 @@ public static class Config { @NotEmpty private List sources = new ArrayList<>(); + @NotNull + private RemoteAddressResolver remoteAddressResolver = new DefaultRemoteAddressResolver(); + public List getSources() { return sources; } @@ -123,5 +128,10 @@ public Config setSources(String... sources) { return this; } + + public Config setRemoteAddressResolver(RemoteAddressResolver remoteAddressResolver) { + this.remoteAddressResolver = remoteAddressResolver; + return this; + } } } diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/DefaultRemoteAddressResolver.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/DefaultRemoteAddressResolver.java new file mode 100644 index 0000000000..63e82e70c8 --- /dev/null +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/DefaultRemoteAddressResolver.java @@ -0,0 +1,16 @@ +package org.springframework.cloud.gateway.support.ipresolver; + +import java.net.InetSocketAddress; + +import org.springframework.web.server.ServerWebExchange; + +/** + * @author Andrew Fitzgerald + */ +public class DefaultRemoteAddressResolver implements RemoteAddressResolver { + + @Override + public InetSocketAddress resolve(ServerWebExchange exchange) { + return exchange.getRequest().getRemoteAddress(); + } +} diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/RemoteAddressResolver.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/RemoteAddressResolver.java new file mode 100644 index 0000000000..4bb630f9bc --- /dev/null +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/RemoteAddressResolver.java @@ -0,0 +1,13 @@ +package org.springframework.cloud.gateway.support.ipresolver; + +import java.net.InetSocketAddress; + +import org.springframework.web.server.ServerWebExchange; + +/** + * @author Andrew Fitzgerald + */ +public interface RemoteAddressResolver { + + InetSocketAddress resolve(ServerWebExchange exchange); +} diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolver.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolver.java new file mode 100644 index 0000000000..c13395e95f --- /dev/null +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolver.java @@ -0,0 +1,34 @@ +package org.springframework.cloud.gateway.support.ipresolver; + +import java.net.InetSocketAddress; +import java.util.List; + +import org.springframework.http.server.reactive.ServerHttpRequest; +import org.springframework.web.server.ServerWebExchange; + +/** + * Parses the client address from the X-Forwarded-For header. + * If header is not present, falls back to {@link DefaultRemoteAddressResolver} and {@link ServerHttpRequest#getRemoteAddress()}. + * Note that this implementation is potentially vulnerable to spoofing, + * as an untrusted client could manually set an initial value of the X-Forwarded-For header. + * + * @see X-Forwarded-For reference + * @author Andrew Fitzgerald + */ +public class XForwardedRemoteAddressResolver implements RemoteAddressResolver { + + public static final String X_FORWARDED_FOR = "X-Forwarded-For"; + + private final DefaultRemoteAddressResolver defaultRemoteIpResolver = new DefaultRemoteAddressResolver(); + + @Override + public InetSocketAddress resolve(ServerWebExchange exchange) { + List xForwardedValues = exchange.getRequest().getHeaders() + .get(X_FORWARDED_FOR); + if (xForwardedValues != null && xForwardedValues.size() != 0) { + String remoteAddress = xForwardedValues.get(0).split(", ")[0]; + return InetSocketAddress.createUnresolved(remoteAddress, 0); + } + return defaultRemoteIpResolver.resolve(exchange); + } +} diff --git a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolverTest.java b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolverTest.java new file mode 100644 index 0000000000..e49e695558 --- /dev/null +++ b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolverTest.java @@ -0,0 +1,47 @@ +package org.springframework.cloud.gateway.support.ipresolver; + +import org.junit.Test; +import org.springframework.mock.http.server.reactive.MockServerHttpRequest; +import org.springframework.mock.web.server.MockServerWebExchange; + +import java.net.InetSocketAddress; + +import static org.assertj.core.api.Assertions.assertThat; + +public class XForwardedRemoteAddressResolverTest { + + + private final InetSocketAddress remote0000Address = InetSocketAddress.createUnresolved("0.0.0.0", 1234); + + XForwardedRemoteAddressResolver resolver = new XForwardedRemoteAddressResolver(); + + @Test + public void resolvesFirstForwardedAddress() { + MockServerHttpRequest request = MockServerHttpRequest.get("someUrl") + .remoteAddress(remote0000Address) + .header("X-Forwarded-For", "0.0.0.1, 0.0.0.2, 0.0.0.3").build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); + InetSocketAddress actualIp = resolver.resolve(exchange); + + assertThat(actualIp.getHostName()).isEqualTo("0.0.0.1"); + } + + @Test + public void fallsBackToRemoteAddress() { + MockServerHttpRequest request = MockServerHttpRequest.get("someUrl") + .remoteAddress(remote0000Address).build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); + InetSocketAddress actualIp = resolver.resolve(exchange); + + assertThat(actualIp.getHostName()).isEqualTo("0.0.0.0"); + } + + @Test + public void returnsNullIfNoForwardedOrRemoteAddress() { + MockServerHttpRequest request = MockServerHttpRequest.get("someUrl").build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); + InetSocketAddress actualIp = resolver.resolve(exchange); + + assertThat(actualIp).isEqualTo(null); + } +} \ No newline at end of file From f167bc360ec314ac0e6b9b4496e5441288f8f5a5 Mon Sep 17 00:00:00 2001 From: "Fitzgerald, Andrew" Date: Tue, 13 Feb 2018 21:13:32 -0500 Subject: [PATCH 2/2] Make function to extract client IP configurable. Add additional method for extracting client IP with is safer from spoofing of X-Forwarded-For header. --- .../XForwardedRemoteAddressResolver.java | 103 +++++++++++-- .../XForwardedRemoteAddressResolverTest.java | 137 +++++++++++++++--- 2 files changed, 207 insertions(+), 33 deletions(-) diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolver.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolver.java index c13395e95f..e36e2c5f38 100644 --- a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolver.java +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolver.java @@ -1,34 +1,117 @@ package org.springframework.cloud.gateway.support.ipresolver; import java.net.InetSocketAddress; +import java.util.Arrays; +import java.util.Collections; import java.util.List; +import org.apache.logging.log4j.util.Strings; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.http.server.reactive.ServerHttpRequest; +import org.springframework.util.Assert; import org.springframework.web.server.ServerWebExchange; /** - * Parses the client address from the X-Forwarded-For header. - * If header is not present, falls back to {@link DefaultRemoteAddressResolver} and {@link ServerHttpRequest#getRemoteAddress()}. - * Note that this implementation is potentially vulnerable to spoofing, - * as an untrusted client could manually set an initial value of the X-Forwarded-For header. + * Parses the client address from the X-Forwarded-For header. If header is not present, + * falls back to {@link DefaultRemoteAddressResolver} and + * {@link ServerHttpRequest#getRemoteAddress()}. Use the static constructor methods which + * meets your security requirements. * - * @see X-Forwarded-For reference + * @see X-Forwarded-For + * reference * @author Andrew Fitzgerald */ public class XForwardedRemoteAddressResolver implements RemoteAddressResolver { public static final String X_FORWARDED_FOR = "X-Forwarded-For"; - + private static final Logger log = LoggerFactory + .getLogger(XForwardedRemoteAddressResolver.class); private final DefaultRemoteAddressResolver defaultRemoteIpResolver = new DefaultRemoteAddressResolver(); + private final int maxTrustedIndex; + + private XForwardedRemoteAddressResolver(int maxTrustedIndex) { + this.maxTrustedIndex = maxTrustedIndex; + } + + /** + * @return a {@link XForwardedRemoteAddressResolver} which always extracts the first + * IP address found in the X-Forwarded-For header (when present). Equivalent to + * calling {@link #maxTrustedIndexXForwardedRemoteAddressResolver(int)} with a + * {@link #maxTrustedIndex} of {@link Integer#MAX_VALUE}. This configuration is + * vulnerable to spoofing via manually setting the X-Forwarded-For header. If the + * resulting IP address is used for security purposes, use + * {@link #maxTrustedIndexXForwardedRemoteAddressResolver(int)} instead. + */ + public static XForwardedRemoteAddressResolver trustAllXForwardedRemoteAddressResolver() { + return new XForwardedRemoteAddressResolver(Integer.MAX_VALUE); + } + + /** + * @return a {@link XForwardedRemoteAddressResolver} which extracts the last + * trusted IP address found in the X-Forwarded-For header (when present). + * This configuration exists to prevent a malicious actor from spoofing the value of + * the X-Forwarded-For header. If you know that your gateway application is only + * accessible from a a trusted load balancer, then you can trust that the load + * balancer will append a valid client IP address to the X-Forwarded-For header, and + * should use a value of `1` for the `maxTrustedIndex`. + * + * + * Given the X-Forwarded-For value of [0.0.0.1, 0.0.0.2, 0.0.0.3]: + * + * 
+	 * maxTrustedIndex -> result
+	 *
+	 * [MIN_VALUE,0] -> IllegalArgumentException
+	 * 1 -> 0.0.0.3
+	 * 2 -> 0.0.0.2
+	 * 3 -> 0.0.0.1
+	 * [4, MAX_VALUE] -> 0.0.0.1
+	 *
+ * + * @param maxTrustedIndex correlates to the number of trusted proxies expected in + * front of Spring Cloud Gateway (index starts at 1). + */ + public static XForwardedRemoteAddressResolver maxTrustedIndexXForwardedRemoteAddressResolver( + int maxTrustedIndex) { + Assert.isTrue(maxTrustedIndex > 0, "An index greater than 0 is required"); + return new XForwardedRemoteAddressResolver(maxTrustedIndex); + } + + /** + * The X-Forwarded-For header contains a comma separated list of IP addresses. This + * method parses those IP addresses into a list. If no X-Forwarded-For header is + * found, an empty list is returned. If multiple X-Forwarded-For headers are found, an + * empty list is returned out of caution. + * @return The parsed values of the X-Forwarded-Header. + */ @Override public InetSocketAddress resolve(ServerWebExchange exchange) { + List xForwardedValues = extractXForwardedValues(exchange); + Collections.reverse(xForwardedValues); + if (xForwardedValues.size() != 0) { + int index = Math.min(xForwardedValues.size(), maxTrustedIndex) - 1; + return InetSocketAddress.createUnresolved(xForwardedValues.get(index), 0); + } + return defaultRemoteIpResolver.resolve(exchange); + } + + private List extractXForwardedValues(ServerWebExchange exchange) { List xForwardedValues = exchange.getRequest().getHeaders() .get(X_FORWARDED_FOR); - if (xForwardedValues != null && xForwardedValues.size() != 0) { - String remoteAddress = xForwardedValues.get(0).split(", ")[0]; - return InetSocketAddress.createUnresolved(remoteAddress, 0); + if (xForwardedValues == null || xForwardedValues.size() == 0) { + return Collections.emptyList(); } - return defaultRemoteIpResolver.resolve(exchange); + if (xForwardedValues.size() > 1) { + log.warn("Multiple X-Forwarded-For headers found, discarding all"); + return Collections.emptyList(); + } + List values = Arrays.asList(xForwardedValues.get(0).split(", ")); + if (values.size() == 1 && Strings.isBlank(values.get(0))) { + return Collections.emptyList(); + } + return values; } } diff --git a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolverTest.java b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolverTest.java index e49e695558..b851581ca4 100644 --- a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolverTest.java +++ b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/support/ipresolver/XForwardedRemoteAddressResolverTest.java @@ -1,47 +1,138 @@ package org.springframework.cloud.gateway.support.ipresolver; +import static org.assertj.core.api.Assertions.assertThat; + +import java.net.InetSocketAddress; + import org.junit.Test; import org.springframework.mock.http.server.reactive.MockServerHttpRequest; import org.springframework.mock.web.server.MockServerWebExchange; +import org.springframework.web.server.ServerWebExchange; -import java.net.InetSocketAddress; +public class XForwardedRemoteAddressResolverTest { -import static org.assertj.core.api.Assertions.assertThat; + private final InetSocketAddress remote0000Address = InetSocketAddress + .createUnresolved("0.0.0.0", 1234); -public class XForwardedRemoteAddressResolverTest { + private final XForwardedRemoteAddressResolver trustOne = XForwardedRemoteAddressResolver + .maxTrustedIndexXForwardedRemoteAddressResolver(1); + + private final XForwardedRemoteAddressResolver trustAll = XForwardedRemoteAddressResolver + .trustAllXForwardedRemoteAddressResolver(); + + @Test + public void maxIndexOneReturnsLastForwardedIp() { + ServerWebExchange exchange = buildExchange(oneTwoThreeBuilder()); + + InetSocketAddress address = trustOne.resolve(exchange); + + assertThat(address.getHostName()).isEqualTo("0.0.0.3"); + } + + @Test + public void maxIndexOneFallsBackToRemoteIp() { + ServerWebExchange exchange = buildExchange(remoteAddressOnlyBuilder()); + + InetSocketAddress address = trustOne.resolve(exchange); + + assertThat(address.getHostName()).isEqualTo("0.0.0.0"); + } + + @Test + public void maxIndexOneReturnsNullIfNoForwardedOrRemoteIp() { + ServerWebExchange exchange = buildExchange(emptyBuilder()); + + InetSocketAddress address = trustOne.resolve(exchange); + + assertThat(address).isEqualTo(null); + } + + @Test + public void trustOneFallsBackOnEmptyHeader() { + ServerWebExchange exchange = buildExchange( + remoteAddressOnlyBuilder().header("X-Forwarded-For", "")); + + InetSocketAddress address = trustOne.resolve(exchange); + assertThat(address.getHostName()).isEqualTo("0.0.0.0"); - private final InetSocketAddress remote0000Address = InetSocketAddress.createUnresolved("0.0.0.0", 1234); + } + + @Test + public void trustOneFallsBackOnMultipleHeaders() { + ServerWebExchange exchange = buildExchange( + remoteAddressOnlyBuilder().header("X-Forwarded-For", "0.0.0.1") + .header("X-Forwarded-For", "0.0.0.2")); + + InetSocketAddress address = trustOne.resolve(exchange); + + assertThat(address.getHostName()).isEqualTo("0.0.0.0"); + } + + @Test + public void trustAllReturnsFirstForwardedIp() { + ServerWebExchange exchange = buildExchange(oneTwoThreeBuilder()); + + InetSocketAddress address = trustAll.resolve(exchange); - XForwardedRemoteAddressResolver resolver = new XForwardedRemoteAddressResolver(); + assertThat(address.getHostName()).isEqualTo("0.0.0.1"); + } @Test - public void resolvesFirstForwardedAddress() { - MockServerHttpRequest request = MockServerHttpRequest.get("someUrl") - .remoteAddress(remote0000Address) - .header("X-Forwarded-For", "0.0.0.1, 0.0.0.2, 0.0.0.3").build(); - MockServerWebExchange exchange = MockServerWebExchange.from(request); - InetSocketAddress actualIp = resolver.resolve(exchange); + public void trustAllFinalFallsBackToRemoteIp() { + ServerWebExchange exchange = buildExchange(remoteAddressOnlyBuilder()); + + InetSocketAddress address = trustAll.resolve(exchange); - assertThat(actualIp.getHostName()).isEqualTo("0.0.0.1"); + assertThat(address.getHostName()).isEqualTo("0.0.0.0"); } @Test - public void fallsBackToRemoteAddress() { - MockServerHttpRequest request = MockServerHttpRequest.get("someUrl") - .remoteAddress(remote0000Address).build(); - MockServerWebExchange exchange = MockServerWebExchange.from(request); - InetSocketAddress actualIp = resolver.resolve(exchange); + public void trustAllReturnsNullIfNoForwardedOrRemoteIp() { + ServerWebExchange exchange = buildExchange(emptyBuilder()); - assertThat(actualIp.getHostName()).isEqualTo("0.0.0.0"); + InetSocketAddress address = trustAll.resolve(exchange); + + assertThat(address).isEqualTo(null); } @Test - public void returnsNullIfNoForwardedOrRemoteAddress() { - MockServerHttpRequest request = MockServerHttpRequest.get("someUrl").build(); - MockServerWebExchange exchange = MockServerWebExchange.from(request); - InetSocketAddress actualIp = resolver.resolve(exchange); + public void trustAllFallsBackOnEmptyHeader() { + ServerWebExchange exchange = buildExchange( + remoteAddressOnlyBuilder().header("X-Forwarded-For", "")); + + InetSocketAddress address = trustAll.resolve(exchange); + + assertThat(address.getHostName()).isEqualTo("0.0.0.0"); + + } + + @Test + public void trustAllFallsBackOnMultipleHeaders() { + ServerWebExchange exchange = buildExchange( + remoteAddressOnlyBuilder().header("X-Forwarded-For", "0.0.0.1") + .header("X-Forwarded-For", "0.0.0.2")); + + InetSocketAddress address = trustAll.resolve(exchange); + + assertThat(address.getHostName()).isEqualTo("0.0.0.0"); + } + + private MockServerHttpRequest.BaseBuilder emptyBuilder() { + return MockServerHttpRequest.get("someUrl"); + } + + private MockServerHttpRequest.BaseBuilder remoteAddressOnlyBuilder() { + return MockServerHttpRequest.get("someUrl").remoteAddress(remote0000Address); + } + + private MockServerHttpRequest.BaseBuilder oneTwoThreeBuilder() { + return MockServerHttpRequest.get("someUrl").remoteAddress(remote0000Address) + .header("X-Forwarded-For", "0.0.0.1, 0.0.0.2, 0.0.0.3"); + } - assertThat(actualIp).isEqualTo(null); + private ServerWebExchange buildExchange( + MockServerHttpRequest.BaseBuilder requestBuilder) { + return MockServerWebExchange.from(requestBuilder.build()); } } \ No newline at end of file