From be6f2634b5fe4a0433d610a29e996e05d12cdd56 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Fri, 23 Sep 2016 14:09:41 -0600 Subject: [PATCH 1/4] Allow OkHttpClient to be customized. Moved common functionality to AbstractLoadBalancingClient --- .../apache/RibbonLoadBalancingHttpClient.java | 29 +++---------- .../okhttp/OkHttpLoadBalancingClient.java | 25 ++++++----- .../support/AbstractLoadBalancingClient.java | 42 ++++++++++++++++++- 3 files changed, 58 insertions(+), 38 deletions(-) diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClient.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClient.java index daa1e831..b623652a 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClient.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/apache/RibbonLoadBalancingHttpClient.java @@ -23,13 +23,11 @@ import org.apache.http.client.HttpClient; import org.apache.http.client.config.RequestConfig; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.impl.client.HttpClientBuilder; -import org.springframework.cloud.netflix.ribbon.DefaultServerIntrospector; import org.springframework.cloud.netflix.ribbon.ServerIntrospector; import org.springframework.cloud.netflix.ribbon.support.AbstractLoadBalancingClient; import org.springframework.web.util.UriComponentsBuilder; import com.netflix.client.config.CommonClientConfigKey; -import com.netflix.client.config.DefaultClientConfigImpl; import com.netflix.client.config.IClientConfig; import com.netflix.loadbalancer.ILoadBalancer; import com.netflix.loadbalancer.Server; @@ -42,40 +40,27 @@ import static org.springframework.cloud.netflix.ribbon.RibbonUtils.updateToHttps //TODO: rename (ie new class that extends this in Dalston) to ApacheHttpLoadBalancingClient public class RibbonLoadBalancingHttpClient extends - AbstractLoadBalancingClient { - private final HttpClient delegate; - private final IClientConfig config; - private final ServerIntrospector serverIntrospector; + AbstractLoadBalancingClient { @Deprecated public RibbonLoadBalancingHttpClient() { - this(new DefaultClientConfigImpl(), new DefaultServerIntrospector()); + super(); } @Deprecated public RibbonLoadBalancingHttpClient(final ILoadBalancer lb) { super(lb); - this.config = new DefaultClientConfigImpl(); - this.delegate = createHttpClient(this.config); - this.serverIntrospector = new DefaultServerIntrospector(); - initWithNiwsConfig(config); } public RibbonLoadBalancingHttpClient(IClientConfig config, ServerIntrospector serverIntrospector) { - this.delegate = createHttpClient(config); - this.config = config; - this.serverIntrospector = serverIntrospector; - initWithNiwsConfig(config); + super(config, serverIntrospector); } public RibbonLoadBalancingHttpClient(HttpClient delegate, IClientConfig config, ServerIntrospector serverIntrospector) { - this.delegate = delegate; - this.config = config; - this.serverIntrospector = serverIntrospector; - initWithNiwsConfig(config); + super(delegate, config, serverIntrospector); } - protected HttpClient createHttpClient(IClientConfig config) { + protected HttpClient createDelegate(IClientConfig config) { return HttpClientBuilder.create() // already defaults to 0 in builder, so resetting to 0 won't hurt .setMaxConnTotal(config.getPropertyAsInteger(CommonClientConfigKey.MaxTotalConnections, 0)) @@ -86,10 +71,6 @@ public class RibbonLoadBalancingHttpClient .build(); } - protected HttpClient getDelegate() { - return this.delegate; - } - @Override public RibbonApacheHttpResponse execute(RibbonApacheHttpRequest request, final IClientConfig configOverride) throws Exception { diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/okhttp/OkHttpLoadBalancingClient.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/okhttp/OkHttpLoadBalancingClient.java index 001e57e2..33851327 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/okhttp/OkHttpLoadBalancingClient.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/okhttp/OkHttpLoadBalancingClient.java @@ -19,13 +19,11 @@ package org.springframework.cloud.netflix.ribbon.okhttp; import java.net.URI; import java.util.concurrent.TimeUnit; -import org.springframework.cloud.netflix.ribbon.DefaultServerIntrospector; import org.springframework.cloud.netflix.ribbon.ServerIntrospector; import org.springframework.cloud.netflix.ribbon.support.AbstractLoadBalancingClient; import org.springframework.web.util.UriComponentsBuilder; import com.netflix.client.config.CommonClientConfigKey; -import com.netflix.client.config.DefaultClientConfigImpl; import com.netflix.client.config.IClientConfig; import com.netflix.loadbalancer.ILoadBalancer; import com.netflix.loadbalancer.Server; @@ -40,30 +38,31 @@ import static org.springframework.cloud.netflix.ribbon.RibbonUtils.updateToHttps * @author Spencer Gibb */ public class OkHttpLoadBalancingClient - extends AbstractLoadBalancingClient { - private final OkHttpClient delegate = new OkHttpClient(); - private final IClientConfig config; - private final ServerIntrospector serverIntrospector; + extends AbstractLoadBalancingClient { @Deprecated public OkHttpLoadBalancingClient() { super(); - config = new DefaultClientConfigImpl(); - serverIntrospector = new DefaultServerIntrospector(); } @Deprecated public OkHttpLoadBalancingClient(final ILoadBalancer lb) { super(lb); - config = new DefaultClientConfigImpl(); - serverIntrospector = new DefaultServerIntrospector(); } public OkHttpLoadBalancingClient(IClientConfig config, ServerIntrospector serverIntrospector) { - this.config = config; - this.serverIntrospector = serverIntrospector; - initWithNiwsConfig(config); + super(config, serverIntrospector); + } + + public OkHttpLoadBalancingClient(OkHttpClient delegate, IClientConfig config, + ServerIntrospector serverIntrospector) { + super(delegate, config, serverIntrospector); + } + + @Override + protected OkHttpClient createDelegate(IClientConfig config) { + return new OkHttpClient(); } @Override diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/support/AbstractLoadBalancingClient.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/support/AbstractLoadBalancingClient.java index 8805f216..db376f57 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/support/AbstractLoadBalancingClient.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/support/AbstractLoadBalancingClient.java @@ -17,6 +17,9 @@ package org.springframework.cloud.netflix.ribbon.support; +import org.springframework.cloud.netflix.ribbon.DefaultServerIntrospector; +import org.springframework.cloud.netflix.ribbon.ServerIntrospector; + import com.netflix.client.AbstractLoadBalancerAwareClient; import com.netflix.client.IResponse; import com.netflix.client.RequestSpecificRetryHandler; @@ -29,7 +32,7 @@ import com.netflix.loadbalancer.ILoadBalancer; /** * @author Spencer Gibb */ -public abstract class AbstractLoadBalancingClient extends +public abstract class AbstractLoadBalancingClient extends AbstractLoadBalancerAwareClient { protected int connectTimeout; @@ -42,16 +45,47 @@ public abstract class AbstractLoadBalancingClient Date: Mon, 3 Oct 2016 14:35:30 -0400 Subject: [PATCH 2/4] Support '+' in encoded query string when rebuilding the URI to https RibbonUtils now replace the encoded space character '+' in the query string in favor of the equivalent '%20' when rewriting the URI in https. This is due to UriComponentsBuilder verifying the allowed characters in the encoded URI when building to the URI java.net class. The '+' is considered illegal but is widely used in the field. (#1367) --- .../cloud/netflix/ribbon/RibbonUtils.java | 23 ++++++---- .../RibbonClientConfigurationTests.java | 11 +++++ .../netflix/ribbon/RibbonUtilsTests.java | 44 ++++++++++++++++--- 3 files changed, 64 insertions(+), 14 deletions(-) diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java index 711b665b..fb172027 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java @@ -1,18 +1,19 @@ package org.springframework.cloud.netflix.ribbon; +import java.net.URI; + +import org.springframework.web.util.UriComponentsBuilder; + import com.netflix.client.config.CommonClientConfigKey; import com.netflix.client.config.IClientConfig; -import com.netflix.client.config.IClientConfigKey; import com.netflix.config.ConfigurationManager; import com.netflix.config.DynamicPropertyFactory; import com.netflix.config.DynamicStringProperty; import com.netflix.loadbalancer.Server; -import org.springframework.web.util.UriComponentsBuilder; - -import java.net.URI; /** * @author Spencer Gibb + * @author Jacques-Etienne Beaudet */ public class RibbonUtils { @@ -67,13 +68,19 @@ public class RibbonUtils { * @param server * @return */ - public static URI updateToHttpsIfNeeded(URI uri, IClientConfig config, ServerIntrospector serverIntrospector, Server server) { + public static URI updateToHttpsIfNeeded(URI uri, IClientConfig config, ServerIntrospector serverIntrospector, + Server server) { String scheme = uri.getScheme(); if (!"https".equals(scheme) && isSecure(config, serverIntrospector, server)) { - return UriComponentsBuilder.fromUri(uri).scheme("https").build(true) - .toUri(); + UriComponentsBuilder uriComponentsBuilder = UriComponentsBuilder.fromUri(uri).scheme("https"); + if (uri.getRawQuery() != null) { + // When building the URI, UriComponentsBuilder verify the allowed characters and does not + // support the '+' so we replace it for its equivalent '%20'. + // See issue https://jira.spring.io/browse/SPR-10172 + uriComponentsBuilder.replaceQuery(uri.getRawQuery().replace("+", "%20")); + } + return uriComponentsBuilder.build(true).toUri(); } return uri; } - } diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfigurationTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfigurationTests.java index e6d05f7c..be0bbe45 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfigurationTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfigurationTests.java @@ -127,6 +127,17 @@ public class RibbonClientConfigurationTests { } } + @Test + public void testPlusInQueryStringGetsRewrittenWhenServerIsSecure() throws Exception { + Server server = new Server("foo", 7777); + when(this.inspector.isSecure(server)).thenReturn(true); + + for (AbstractLoadBalancerAwareClient client : clients()) { + URI uri = client.reconstructURIWithServer(server, new URI("http://foo/%20bar?hello=1+2")); + assertThat(uri, is(new URI("https://foo:7777/%20bar?hello=1%202"))); + } + } + private List clients() { ArrayList clients = new ArrayList<>(); clients.add(new OverrideRestClient(this.config, this.inspector)); diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java index f8e8204f..53b1bd8d 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java @@ -17,20 +17,23 @@ package org.springframework.cloud.netflix.ribbon; -import com.netflix.client.config.CommonClientConfigKey; +import static org.hamcrest.Matchers.is; +import static org.springframework.cloud.netflix.ribbon.RibbonUtils.*; + +import java.net.URI; +import java.net.URISyntaxException; +import java.util.Map; + import org.junit.Assert; import org.junit.Test; +import com.netflix.client.config.CommonClientConfigKey; import com.netflix.client.config.DefaultClientConfigImpl; import com.netflix.loadbalancer.Server; -import java.util.Map; - -import static org.hamcrest.Matchers.is; -import static org.springframework.cloud.netflix.ribbon.RibbonUtils.isSecure; - /** * @author Spencer Gibb + * @author Jacques-Etienne Beaudet */ public class RibbonUtilsTests { @@ -77,6 +80,35 @@ public class RibbonUtilsTests { Assert.assertThat("isSecure was wrong", secure, is(false)); } + @Test + public void uriIsNotChangedWhenServerIsNotSecured() throws URISyntaxException { + URI original = new URI("http://foo"); + URI updated = updateToHttpsIfNeeded(original, NON_SECURE_CONFIG, NON_SECURE_INTROSPECTOR, SERVER); + Assert.assertThat("URI should not have been updated since server is not secured.", original, is(updated)); + } + + @Test + public void uriIsNotChangedWhenServerIsSecuredAndUriAlreadyInHttps() throws URISyntaxException { + URI original = new URI("https://foo"); + URI updated = updateToHttpsIfNeeded(original, SECURE_CONFIG, SECURE_INTROSPECTOR, SERVER); + Assert.assertThat("URI should not have been updated since uri is already in https.", original, is(updated)); + } + + @Test + public void shouldUpgradeUriToHttpsWhenServerIsSecureAndUriNotInHttps() throws URISyntaxException { + URI original = new URI("http://foo"); + URI updated = updateToHttpsIfNeeded(original, SECURE_CONFIG, SECURE_INTROSPECTOR, SERVER); + Assert.assertThat("URI should have been updated to https.", updated, is(new URI("https://foo"))); + } + + @Test + public void shouldSubstitutePlusInQueryParam() throws URISyntaxException { + URI original = new URI("http://foo/%20bar?hello=1+2"); + URI updated = updateToHttpsIfNeeded(original, SECURE_CONFIG, SECURE_INTROSPECTOR, SERVER); + Assert.assertThat("URI should have had its plus sign replaced in query string.", updated, is(new URI( + "https://foo/%20bar?hello=1%202"))); + } + static DefaultClientConfigImpl getConfig(boolean value) { DefaultClientConfigImpl config = new DefaultClientConfigImpl(); config.setProperty(CommonClientConfigKey.IsSecure, value); From 4ba34112928eaae77cb659d46baaff99c921bd03 Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Mon, 3 Oct 2016 13:44:02 -0700 Subject: [PATCH 3/4] Clarified zuul.stripPrefix usage Helps with the confusion seen in #1365 --- docs/src/main/asciidoc/spring-cloud-netflix.adoc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/src/main/asciidoc/spring-cloud-netflix.adoc b/docs/src/main/asciidoc/spring-cloud-netflix.adoc index 5768bf6c..8fe0017e 100644 --- a/docs/src/main/asciidoc/spring-cloud-netflix.adoc +++ b/docs/src/main/asciidoc/spring-cloud-netflix.adoc @@ -1305,6 +1305,9 @@ the service-specific prefix from individual routes, e.g. stripPrefix: false ---- +NOTE: `zuul.stripPrefix` only applies to the prefix set in `zuul.prefix`. It does have any effect on prefixes +defined within a given route's `path`. + In this example, requests to "/myusers/101" will be forwarded to "/myusers/101" on the "users" service. The `zuul.routes` entries actually bind to an object of type `ZuulProperties`. If you From e109093b834993f57a2e4a93307f2358fc1be339 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20D=C3=BCsterh=C3=B6ft?= Date: Tue, 4 Oct 2016 05:30:17 +0200 Subject: [PATCH 4/4] add config option to not ignore security headers (#1354) * add config option to not ignore security headers * fix field documentation of ignoreSecurityHeaders * add documenation for zuul.ignoreSecurityHeaders fixes gh-1096 --- docs/src/main/asciidoc/spring-cloud-netflix.adoc | 3 +++ .../cloud/netflix/zuul/filters/ZuulProperties.java | 13 +++++++++++-- .../netflix/zuul/filters/ZuulPropertiesTests.java | 9 +++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/docs/src/main/asciidoc/spring-cloud-netflix.adoc b/docs/src/main/asciidoc/spring-cloud-netflix.adoc index 8fe0017e..53961116 100644 --- a/docs/src/main/asciidoc/spring-cloud-netflix.adoc +++ b/docs/src/main/asciidoc/spring-cloud-netflix.adoc @@ -1413,6 +1413,8 @@ need to set it unless you want it to be different. N.B. this is new in Spring Cloud Netflix 1.1 (in 1.0 the user had no control over headers and all cookies flow in both directions). +=== Ignored Headers + In addition to the per-route sensitive headers, you can set a global value for `zuul.ignoredHeaders` for values that should be discarded (both request and response) during interactions with downstream @@ -1421,6 +1423,7 @@ classpath, and otherwise they are initialized to a set of well-known "security" headers (e.g. involving caching) as specified by Spring Security. The assumption in this case is that the downstream services might add these headers too, and we want the values from the proxy. +To not discard these well known security headers in case Spring Security is on the classpath you can set `zuul.ignoreSecurityHeaders` to `false`. This can be useful if you disabled the HTTP Security response headers in Spring Security and want the values provided by downstream services === The Routes Endpoint diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/ZuulProperties.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/ZuulProperties.java index 9d3278df..b621f59d 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/ZuulProperties.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/ZuulProperties.java @@ -42,6 +42,7 @@ import static com.netflix.hystrix.HystrixCommandProperties.ExecutionIsolationStr /** * @author Spencer Gibb * @author Dave Syer + * @author Mathias Düsterhöft */ @Data @ConfigurationProperties("zuul") @@ -50,7 +51,7 @@ public class ZuulProperties { /** * Headers that are generally expected to be added by Spring Security, and hence often * duplicated if the proxy and the backend are secured with Spring. By default they - * are added to the ignored headers if Spring Security is present. + * are added to the ignored headers if Spring Security is present and ignoreSecurityHeaders = true. */ public static final List SECURITY_HEADERS = Arrays.asList("Pragma", "Cache-Control", "X-Frame-Options", "X-Content-Type-Options", @@ -101,6 +102,14 @@ public class ZuulProperties { */ private Set ignoredHeaders = new LinkedHashSet<>(); + /** + * SECURITY_HEADERS are added to ignored headers if spring security is on the classpath and ignoreSecurityHeaders = true + * By setting ignoreSecurityHeaders to false we can switch off this default behaviour. This should be used together with + * disabling the default spring security headers + * see https://docs.spring.io/spring-security/site/docs/current/reference/html/headers.html#default-security-headers + */ + private boolean ignoreSecurityHeaders = true; + /** * Path to install Zuul as a servlet (not part of Spring MVC). The servlet is more * memory efficient for requests with large bodies, e.g. file uploads. @@ -148,7 +157,7 @@ public class ZuulProperties { Set ignoredHeaders = new LinkedHashSet<>(this.ignoredHeaders); if (ClassUtils.isPresent( "org.springframework.security.config.annotation.web.WebSecurityConfigurer", - null) && Collections.disjoint(ignoredHeaders, SECURITY_HEADERS)) { + null) && Collections.disjoint(ignoredHeaders, SECURITY_HEADERS) && ignoreSecurityHeaders) { // Allow Spring Security in the gateway to control these headers ignoredHeaders.addAll(SECURITY_HEADERS); } diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/ZuulPropertiesTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/ZuulPropertiesTests.java index 60e25406..9ea87b50 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/ZuulPropertiesTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/ZuulPropertiesTests.java @@ -29,6 +29,7 @@ import static org.junit.Assert.assertTrue; /** * @author Dave Syer + * @author Mathias Düsterhöft */ public class ZuulPropertiesTests { @@ -46,10 +47,18 @@ public class ZuulPropertiesTests { @Test public void defaultIgnoredHeaders() { + assertTrue(this.zuul.isIgnoreSecurityHeaders()); assertTrue(this.zuul.getIgnoredHeaders() .containsAll(ZuulProperties.SECURITY_HEADERS)); } + @Test + public void securityHeadersNotIgnored() { + zuul.setIgnoreSecurityHeaders(false); + + assertTrue(this.zuul.getIgnoredHeaders().isEmpty()); + } + @Test public void addIgnoredHeaders() { this.zuul.setIgnoredHeaders(Collections.singleton("x-foo"));