From 20b29db5123544b9c92902530d02a0d59dae670a Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Thu, 1 Oct 2015 15:02:09 -0700 Subject: [PATCH] Unify ServerProperties X-Forwarded-For support Add a new `server.use-forward-headers` property which can be used to switch on X-Forwarded-For header support in all supported embedded servlet containers. This commit reverts the decision to enable `RemoteIpValve` with Tomcat by default (gh-3782) and requires that either `user-forward-headers` is set to true or that `server.tomcat.protocol-header` or `server.tomcat.remote-ip-header` are set. See gh-4018 See gh-3782 --- .../autoconfigure/web/ServerProperties.java | 77 ++++++++++++++----- .../web/ServerPropertiesTests.java | 45 +++++++++-- 2 files changed, 96 insertions(+), 26 deletions(-) diff --git a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java index b0c9a813c8..5335a4940c 100644 --- a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java +++ b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java @@ -47,6 +47,7 @@ import org.springframework.boot.context.embedded.InitParameterConfiguringServlet import org.springframework.boot.context.embedded.JspServlet; import org.springframework.boot.context.embedded.ServletContextInitializer; import org.springframework.boot.context.embedded.Ssl; +import org.springframework.boot.context.embedded.jetty.JettyEmbeddedServletContainerFactory; import org.springframework.boot.context.embedded.tomcat.TomcatConnectorCustomizer; import org.springframework.boot.context.embedded.tomcat.TomcatContextCustomizer; import org.springframework.boot.context.embedded.tomcat.TomcatEmbeddedServletContainerFactory; @@ -102,6 +103,11 @@ public class ServerProperties implements EmbeddedServletContainerCustomizer, Ord */ private final Map contextParameters = new HashMap(); + /** + * If X-Forwarded-* headers should be applied to the HttpRequest. + */ + private boolean useForwardHeaders; + private Session session = new Session(); @NestedConfigurationProperty @@ -115,6 +121,8 @@ public class ServerProperties implements EmbeddedServletContainerCustomizer, Ord private final Tomcat tomcat = new Tomcat(); + private final Jetty jetty = new Jetty(); + private final Undertow undertow = new Undertow(); @Override @@ -150,11 +158,16 @@ public class ServerProperties implements EmbeddedServletContainerCustomizer, Ord container.setCompression(getCompression()); } if (container instanceof TomcatEmbeddedServletContainerFactory) { - getTomcat() - .customizeTomcat((TomcatEmbeddedServletContainerFactory) container); + getTomcat().customizeTomcat(this, + (TomcatEmbeddedServletContainerFactory) container); + } + if (container instanceof JettyEmbeddedServletContainerFactory) { + getJetty().customizeJetty(this, + (JettyEmbeddedServletContainerFactory) container); } + if (container instanceof UndertowEmbeddedServletContainerFactory) { - getUndertow().customizeUndertow( + getUndertow().customizeUndertow(this, (UndertowEmbeddedServletContainerFactory) container); } container.addInitializers(new SessionConfiguringInitializer(this.session)); @@ -267,6 +280,14 @@ public class ServerProperties implements EmbeddedServletContainerCustomizer, Ord return this.contextParameters; } + public boolean isUseForwardHeaders() { + return this.useForwardHeaders; + } + + public void setUseForwardHeaders(boolean useForwardHeaders) { + this.useForwardHeaders = useForwardHeaders; + } + /** * Get the session timeout. * @return the session timeout @@ -320,6 +341,10 @@ public class ServerProperties implements EmbeddedServletContainerCustomizer, Ord return this.tomcat; } + private Jetty getJetty() { + return this.jetty; + } + public Undertow getUndertow() { return this.undertow; } @@ -488,9 +513,8 @@ public class ServerProperties implements EmbeddedServletContainerCustomizer, Ord /** * Header that holds the incoming protocol, usually named "X-Forwarded-Proto". - * Configures a RemoteIpValve only if remoteIpHeader is also set. */ - private String protocolHeader = "x-forwarded-proto"; + private String protocolHeader; /** * Value of the protocol header that indicates that the incoming request uses SSL. @@ -500,13 +524,12 @@ public class ServerProperties implements EmbeddedServletContainerCustomizer, Ord /** * Name of the HTTP header used to override the original port value. */ - private String portHeader = "x-forwarded-port"; + private String portHeader = "X-Forwarded-Port"; /** - * Name of the http header from which the remote ip is extracted. Configures a - * RemoteIpValve only if protocolHeader is also set. + * Name of the http header from which the remote ip is extracted.. */ - private String remoteIpHeader = "x-forwarded-for"; + private String remoteIpHeader; /** * Tomcat base directory. If not specified a temporary directory will be used. @@ -659,12 +682,13 @@ public class ServerProperties implements EmbeddedServletContainerCustomizer, Ord this.uriEncoding = uriEncoding; } - void customizeTomcat(TomcatEmbeddedServletContainerFactory factory) { + void customizeTomcat(ServerProperties serverProperties, + TomcatEmbeddedServletContainerFactory factory) { if (getBasedir() != null) { factory.setBaseDirectory(getBasedir()); } customizeBackgroundProcessorDelay(factory); - customizeHeaders(factory); + customizeRemoteIpValve(serverProperties, factory); if (this.maxThreads > 0) { customizeMaxThreads(factory); } @@ -691,14 +715,20 @@ public class ServerProperties implements EmbeddedServletContainerCustomizer, Ord }); } - private void customizeHeaders(TomcatEmbeddedServletContainerFactory factory) { - String remoteIpHeader = getRemoteIpHeader(); + private void customizeRemoteIpValve(ServerProperties properties, + TomcatEmbeddedServletContainerFactory factory) { String protocolHeader = getProtocolHeader(); - if (StringUtils.hasText(remoteIpHeader) - && StringUtils.hasText(protocolHeader)) { + String remoteIpHeader = getRemoteIpHeader(); + // For back compatibility the valve is also enabled if protocol-header is set + if (StringUtils.hasText(protocolHeader) + || StringUtils.hasText(remoteIpHeader) + || properties.isUseForwardHeaders()) { RemoteIpValve valve = new RemoteIpValve(); - valve.setRemoteIpHeader(remoteIpHeader); - valve.setProtocolHeader(protocolHeader); + valve.setProtocolHeader(StringUtils.hasLength(protocolHeader) ? protocolHeader + : "X-Forwarded-Proto"); + if (StringUtils.hasLength(remoteIpHeader)) { + valve.setRemoteIpHeader(remoteIpHeader); + } // The internal proxies default to a white list of "safe" internal IP // addresses valve.setInternalProxies(getInternalProxies()); @@ -822,6 +852,15 @@ public class ServerProperties implements EmbeddedServletContainerCustomizer, Ord } + private static class Jetty { + + void customizeJetty(ServerProperties serverProperties, + JettyEmbeddedServletContainerFactory factory) { + factory.setUseForwardHeaders(serverProperties.isUseForwardHeaders()); + } + + } + public static class Undertow { /** @@ -958,7 +997,8 @@ public class ServerProperties implements EmbeddedServletContainerCustomizer, Ord getAccesslog().setDir(accessLogDir); } - void customizeUndertow(UndertowEmbeddedServletContainerFactory factory) { + void customizeUndertow(ServerProperties serverProperties, + UndertowEmbeddedServletContainerFactory factory) { factory.setBufferSize(this.bufferSize); factory.setBuffersPerRegion(this.buffersPerRegion); factory.setIoThreads(this.ioThreads); @@ -967,6 +1007,7 @@ public class ServerProperties implements EmbeddedServletContainerCustomizer, Ord factory.setAccessLogDirectory(this.accesslog.dir); factory.setAccessLogPattern(this.accesslog.pattern); factory.setAccessLogEnabled(this.accesslog.enabled); + factory.setUseForwardHeaders(serverProperties.isUseForwardHeaders()); } public static class Accesslog { diff --git a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/ServerPropertiesTests.java b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/ServerPropertiesTests.java index ec50a64e3e..298e6bdbf1 100644 --- a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/ServerPropertiesTests.java +++ b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/ServerPropertiesTests.java @@ -39,7 +39,9 @@ import org.springframework.beans.MutablePropertyValues; import org.springframework.boot.bind.RelaxedDataBinder; import org.springframework.boot.context.embedded.ConfigurableEmbeddedServletContainer; import org.springframework.boot.context.embedded.ServletContextInitializer; +import org.springframework.boot.context.embedded.jetty.JettyEmbeddedServletContainerFactory; import org.springframework.boot.context.embedded.tomcat.TomcatEmbeddedServletContainerFactory; +import org.springframework.boot.context.embedded.undertow.UndertowEmbeddedServletContainerFactory; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; @@ -50,6 +52,7 @@ import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; /** @@ -252,27 +255,38 @@ public class ServerPropertiesTests { map.put("server.tomcat.remote_ip_header", ""); map.put("server.tomcat.protocol_header", ""); bindProperties(map); - TomcatEmbeddedServletContainerFactory container = new TomcatEmbeddedServletContainerFactory(); this.properties.customize(container); - assertEquals(0, container.getValves().size()); } @Test public void defaultTomcatRemoteIpValve() throws Exception { - // Since 1.3.0 no need to explicitly set header names + Map map = new HashMap(); + // Since 1.1.7 you need to specify at least the protocol + map.put("server.tomcat.protocol_header", "X-Forwarded-Proto"); + map.put("server.tomcat.remote_ip_header", "X-Forwarded-For"); + bindProperties(map); + testRemoteIpValveConfigured(); + } + + @Test + public void setUseForwardHeadersTomcat() throws Exception { + // Since 1.3.0 no need to explicitly set header names if use-forward-header=true + this.properties.setUseForwardHeaders(true); + testRemoteIpValveConfigured(); + } + + private void testRemoteIpValveConfigured() { TomcatEmbeddedServletContainerFactory container = new TomcatEmbeddedServletContainerFactory(); this.properties.customize(container); - assertEquals(1, container.getValves().size()); Valve valve = container.getValves().iterator().next(); assertThat(valve, instanceOf(RemoteIpValve.class)); RemoteIpValve remoteIpValve = (RemoteIpValve) valve; - assertEquals("x-forwarded-proto", remoteIpValve.getProtocolHeader()); + assertEquals("X-Forwarded-Proto", remoteIpValve.getProtocolHeader()); assertEquals("https", remoteIpValve.getProtocolHeaderHttpsValue()); - assertEquals("x-forwarded-for", remoteIpValve.getRemoteIpHeader()); - + assertEquals("X-Forwarded-For", remoteIpValve.getRemoteIpHeader()); String expectedInternalProxies = "10\\.\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}|" // 10/8 + "192\\.168\\.\\d{1,3}\\.\\d{1,3}|" // 192.168/16 + "169\\.254\\.\\d{1,3}\\.\\d{1,3}|" // 169.254/16 @@ -280,7 +294,6 @@ public class ServerPropertiesTests { + "172\\.1[6-9]{1}\\.\\d{1,3}\\.\\d{1,3}|" // 172.16/12 + "172\\.2[0-9]{1}\\.\\d{1,3}\\.\\d{1,3}|" + "172\\.3[0-1]{1}\\.\\d{1,3}\\.\\d{1,3}"; - assertEquals(expectedInternalProxies, remoteIpValve.getInternalProxies()); } @@ -308,6 +321,22 @@ public class ServerPropertiesTests { assertEquals("192.168.0.1", remoteIpValve.getInternalProxies()); } + @Test + public void setUseForwardHeadersUndertow() throws Exception { + this.properties.setUseForwardHeaders(true); + UndertowEmbeddedServletContainerFactory container = spy(new UndertowEmbeddedServletContainerFactory()); + this.properties.customize(container); + verify(container).setUseForwardHeaders(true); + } + + @Test + public void setUseForwardHeadersJetty() throws Exception { + this.properties.setUseForwardHeaders(true); + JettyEmbeddedServletContainerFactory container = spy(new JettyEmbeddedServletContainerFactory()); + this.properties.customize(container); + verify(container).setUseForwardHeaders(true); + } + private void bindProperties(Map map) { new RelaxedDataBinder(this.properties, "server").bind(new MutablePropertyValues( map));