From 617ecf11ec18fcba8a94d8754ae8b84546a77d3d Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Mon, 9 May 2016 11:47:41 +0100 Subject: [PATCH] Track customization of sensitive headers so route can be cleared When the users explicitly sets the sensitive headers of a rroute to empty, it means they should be empty, not the global defaults. Fixes gh-1012 --- .../cloud/netflix/zuul/filters/Route.java | 7 + .../zuul/filters/SimpleRouteLocator.java | 10 +- .../netflix/zuul/filters/ZuulProperties.java | 36 ++++- .../zuul/filters/pre/PreDecorationFilter.java | 54 ++++--- .../filters/pre/PreDecorationFilterTests.java | 142 +++++++++++------- 5 files changed, 161 insertions(+), 88 deletions(-) diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/Route.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/Route.java index 947ed22e..63c0d7d7 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/Route.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/Route.java @@ -36,6 +36,7 @@ public class Route { this.retryable = retryable; this.sensitiveHeaders = new LinkedHashSet<>(); if (ignoredHeaders != null) { + this.customSensitiveHeaders = true; for (String header : ignoredHeaders) { this.sensitiveHeaders.add(header.toLowerCase()); } @@ -56,4 +57,10 @@ public class Route { private Set sensitiveHeaders = new LinkedHashSet<>(); + private boolean customSensitiveHeaders; + + public boolean isCustomSensitiveHeaders() { + return this.customSensitiveHeaders; + } + } \ No newline at end of file diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/SimpleRouteLocator.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/SimpleRouteLocator.java index f9265290..48466b6b 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/SimpleRouteLocator.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/SimpleRouteLocator.java @@ -90,9 +90,10 @@ public class SimpleRouteLocator implements RouteLocator { log.debug("servletPath=" + this.dispatcherServletPath); log.debug("zuulServletPath=" + this.zuulServletPath); - log.debug("RequestUtils.isDispatcherServletRequest()=" + RequestUtils.isDispatcherServletRequest()); - log.debug("RequestUtils.isZuulServletRequest()=" + RequestUtils.isZuulServletRequest()); - + log.debug("RequestUtils.isDispatcherServletRequest()=" + + RequestUtils.isDispatcherServletRequest()); + log.debug("RequestUtils.isZuulServletRequest()=" + + RequestUtils.isZuulServletRequest()); String adjustedPath = adjustPath(path); @@ -135,7 +136,8 @@ public class SimpleRouteLocator implements RouteLocator { retryable = route.getRetryable(); } return new Route(route.getId(), targetPath, route.getLocation(), prefix, - retryable, route.getSensitiveHeaders()); + retryable, + route.isCustomSensitiveHeaders() ? route.getSensitiveHeaders() : null); } /** 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 8e5049c1..8853f2db 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 @@ -116,12 +116,11 @@ public class ZuulProperties { private boolean removeSemicolonContent = true; /** - * List of sensitive headers that are not passed to downstream requests. Defaults - * to a "safe" set of headers that commonly contain user credentials. It's OK to - * remove those from the list if the downstream service is part of the same system - * as the proxy, so they are sharing authentication data. If using a physical URL - * outside your own domain, then generally it would be a bad idea to leak user - * credentials. + * List of sensitive headers that are not passed to downstream requests. Defaults to a + * "safe" set of headers that commonly contain user credentials. It's OK to remove + * those from the list if the downstream service is part of the same system as the + * proxy, so they are sharing authentication data. If using a physical URL outside + * your own domain, then generally it would be a bad idea to leak user credentials. */ private Set sensitiveHeaders = new LinkedHashSet<>( Arrays.asList("Cookie", "Set-Cookie", "Authorization")); @@ -158,7 +157,6 @@ public class ZuulProperties { } @Data - @AllArgsConstructor @NoArgsConstructor public static class ZuulRoute { @@ -206,6 +204,19 @@ public class ZuulProperties { */ private Set sensitiveHeaders = new LinkedHashSet<>(); + private boolean customSensitiveHeaders = false; + + public ZuulRoute(String id, String path, String serviceId, String url, + boolean stripPrefix, Boolean retryable, Set sensitiveHeaders) { + this.id = id; + this.path = path; + this.serviceId = serviceId; + this.url = url; + this.stripPrefix = stripPrefix; + this.retryable = retryable; + this.sensitiveHeaders = sensitiveHeaders; + } + public ZuulRoute(String text) { String location = null; String path = text; @@ -254,7 +265,16 @@ public class ZuulProperties { public Route getRoute(String prefix) { return new Route(this.id, this.path, getLocation(), prefix, this.retryable, - this.sensitiveHeaders); + isCustomSensitiveHeaders() ? this.sensitiveHeaders : null); + } + + public void setSensitiveHeaders(Set headers) { + this.customSensitiveHeaders = true; + this.sensitiveHeaders = new LinkedHashSet<>(headers); + } + + public boolean isCustomSensitiveHeaders() { + return this.customSensitiveHeaders; } } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/PreDecorationFilter.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/PreDecorationFilter.java index e1ff53b7..39dc918b 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/PreDecorationFilter.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/PreDecorationFilter.java @@ -43,15 +43,15 @@ public class PreDecorationFilter extends ZuulFilter { private ZuulProperties properties; private UrlPathHelper urlPathHelper = new UrlPathHelper(); - + private ProxyRequestHelper proxyRequestHelper; - public PreDecorationFilter(RouteLocator routeLocator, - String dispatcherServletPath, ZuulProperties properties, - ProxyRequestHelper proxyRequestHelper) { + public PreDecorationFilter(RouteLocator routeLocator, String dispatcherServletPath, + ZuulProperties properties, ProxyRequestHelper proxyRequestHelper) { this.routeLocator = routeLocator; this.properties = properties; - this.urlPathHelper.setRemoveSemicolonContent(properties.isRemoveSemicolonContent()); + this.urlPathHelper + .setRemoveSemicolonContent(properties.isRemoveSemicolonContent()); this.dispatcherServletPath = dispatcherServletPath; this.proxyRequestHelper = proxyRequestHelper; } @@ -70,7 +70,8 @@ public class PreDecorationFilter extends ZuulFilter { public boolean shouldFilter() { RequestContext ctx = RequestContext.getCurrentContext(); return !ctx.containsKey("forward.to") // a filter has already forwarded - && !ctx.containsKey("serviceId"); // a filter has already determined serviceId + && !ctx.containsKey("serviceId"); // a filter has already determined + // serviceId } @Override @@ -84,10 +85,13 @@ public class PreDecorationFilter extends ZuulFilter { if (location != null) { ctx.put("requestURI", route.getPath()); ctx.put("proxy", route.getId()); - if (route.getSensitiveHeaders().isEmpty()) { - proxyRequestHelper.addIgnoredHeaders(this.properties.getSensitiveHeaders().toArray(new String[0])); - } else { - proxyRequestHelper.addIgnoredHeaders(route.getSensitiveHeaders().toArray(new String[0])); + if (!route.isCustomSensitiveHeaders()) { + this.proxyRequestHelper.addIgnoredHeaders( + this.properties.getSensitiveHeaders().toArray(new String[0])); + } + else { + this.proxyRequestHelper.addIgnoredHeaders( + route.getSensitiveHeaders().toArray(new String[0])); } if (route.getRetryable() != null) { @@ -118,18 +122,22 @@ public class PreDecorationFilter extends ZuulFilter { ctx.addZuulRequestHeader(ZuulHeaders.X_FORWARDED_PROTO, ctx.getRequest().getScheme()); if (StringUtils.hasText(route.getPrefix())) { - String existingPrefix = ctx.getRequest().getHeader("X-Forwarded-Prefix"); + String existingPrefix = ctx.getRequest() + .getHeader("X-Forwarded-Prefix"); StringBuilder newPrefixBuilder = new StringBuilder(); if (StringUtils.hasLength(existingPrefix)) { - if (existingPrefix.endsWith("/") && route.getPrefix().startsWith("/")) { - newPrefixBuilder.append(existingPrefix, 0, existingPrefix.length() - 1); + if (existingPrefix.endsWith("/") + && route.getPrefix().startsWith("/")) { + newPrefixBuilder.append(existingPrefix, 0, + existingPrefix.length() - 1); } else { newPrefixBuilder.append(existingPrefix); } } newPrefixBuilder.append(route.getPrefix()); - ctx.addZuulRequestHeader("X-Forwarded-Prefix", newPrefixBuilder.toString()); + ctx.addZuulRequestHeader("X-Forwarded-Prefix", + newPrefixBuilder.toString()); } String xforwardedfor = ctx.getRequest().getHeader("X-Forwarded-For"); String remoteAddr = ctx.getRequest().getRemoteAddr(); @@ -147,17 +155,21 @@ public class PreDecorationFilter extends ZuulFilter { log.warn("No route found for uri: " + requestURI); String fallBackUri = requestURI; - String fallbackPrefix = dispatcherServletPath; //default fallback servlet is DispatcherServlet + String fallbackPrefix = this.dispatcherServletPath; // default fallback + // servlet is + // DispatcherServlet if (RequestUtils.isZuulServletRequest()) { - //remove the Zuul servletPath from the requestUri + // remove the Zuul servletPath from the requestUri log.debug("zuulServletPath=" + this.properties.getServletPath()); - fallBackUri = fallBackUri.replaceFirst(this.properties.getServletPath(), ""); + fallBackUri = fallBackUri.replaceFirst(this.properties.getServletPath(), + ""); log.debug("Replaced Zuul servlet path:" + fallBackUri); - } else { - //remove the DispatcherServlet servletPath from the requestUri - log.debug("dispatcherServletPath=" + dispatcherServletPath); - fallBackUri = fallBackUri.replaceFirst(dispatcherServletPath, ""); + } + else { + // remove the DispatcherServlet servletPath from the requestUri + log.debug("dispatcherServletPath=" + this.dispatcherServletPath); + fallBackUri = fallBackUri.replaceFirst(this.dispatcherServletPath, ""); log.debug("Replaced DispatcherServlet servlet path:" + fallBackUri); } if (!fallBackUri.startsWith("/")) { diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/pre/PreDecorationFilterTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/pre/PreDecorationFilterTests.java index 016802d1..cf9cfe6a 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/pre/PreDecorationFilterTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/pre/PreDecorationFilterTests.java @@ -55,7 +55,7 @@ public class PreDecorationFilterTests { private DiscoveryClientRouteLocator routeLocator; private MockHttpServletRequest request = new MockHttpServletRequest(); - + private ProxyRequestHelper proxyRequestHelper = new ProxyRequestHelper(); @Before @@ -64,7 +64,8 @@ public class PreDecorationFilterTests { this.properties = new ZuulProperties(); this.routeLocator = new DiscoveryClientRouteLocator("/", this.discovery, this.properties); - this.filter = new PreDecorationFilter(this.routeLocator, "/", this.properties, proxyRequestHelper); + this.filter = new PreDecorationFilter(this.routeLocator, "/", this.properties, + this.proxyRequestHelper); RequestContext ctx = RequestContext.getCurrentContext(); ctx.clear(); ctx.setRequest(this.request); @@ -105,7 +106,8 @@ public class PreDecorationFilterTests { assertEquals("80", ctx.getZuulRequestHeaders().get("x-forwarded-port")); assertEquals("http", ctx.getZuulRequestHeaders().get("x-forwarded-proto")); assertEquals("/api", ctx.getZuulRequestHeaders().get("x-forwarded-prefix")); - assertEquals("1.2.3.4, 5.6.7.8", ctx.getZuulRequestHeaders().get("x-forwarded-for")); + assertEquals("1.2.3.4, 5.6.7.8", + ctx.getZuulRequestHeaders().get("x-forwarded-for")); assertEquals("foo", getHeader(ctx.getOriginResponseHeaders(), "x-zuul-serviceid")); } @@ -126,8 +128,10 @@ public class PreDecorationFilterTests { assertEquals("localhost", ctx.getZuulRequestHeaders().get("x-forwarded-host")); assertEquals("80", ctx.getZuulRequestHeaders().get("x-forwarded-port")); assertEquals("http", ctx.getZuulRequestHeaders().get("x-forwarded-proto")); - assertEquals("/prefix/api", ctx.getZuulRequestHeaders().get("x-forwarded-prefix")); - assertEquals("1.2.3.4, 5.6.7.8", ctx.getZuulRequestHeaders().get("x-forwarded-for")); + assertEquals("/prefix/api", + ctx.getZuulRequestHeaders().get("x-forwarded-prefix")); + assertEquals("1.2.3.4, 5.6.7.8", + ctx.getZuulRequestHeaders().get("x-forwarded-for")); assertEquals("foo", getHeader(ctx.getOriginResponseHeaders(), "x-zuul-serviceid")); } @@ -169,101 +173,98 @@ public class PreDecorationFilterTests { assertEquals("foo", getHeader(ctx.getOriginResponseHeaders(), "x-zuul-serviceid")); } - + @Test public void routeNotFound() throws Exception { this.properties.setPrefix("/api"); - this.properties.setStripPrefix(true); + this.properties.setStripPrefix(true); this.routeLocator.addRoute( new ZuulRoute("foo", "/foo/**", null, "forward:/foo", true, null, null)); - + this.request.setRequestURI("/api/bar/1"); this.filter.run(); RequestContext ctx = RequestContext.getCurrentContext(); assertEquals("/api/bar/1", ctx.get("forward.to")); } - + @Test public void routeNotFoundDispatcherServletSpecialPath() throws Exception { this.properties.setPrefix("/api"); - this.properties.setStripPrefix(true); + this.properties.setStripPrefix(true); this.properties.setAddProxyHeaders(true); this.routeLocator.addRoute( new ZuulRoute("foo", "/foo/**", null, "forward:/foo", true, null, null)); - - this.filter = new PreDecorationFilter(this.routeLocator, - "/special", this.properties, proxyRequestHelper); - + + this.filter = new PreDecorationFilter(this.routeLocator, "/special", + this.properties, this.proxyRequestHelper); + this.request.setRequestURI("/api/bar/1"); this.filter.run(); RequestContext ctx = RequestContext.getCurrentContext(); assertEquals("/special/api/bar/1", ctx.get("forward.to")); - } - + } + @Test public void routeNotFoundZuulRequest() throws Exception { setTestRequestContext(); RequestContext ctx = RequestContext.getCurrentContext(); - ctx.getCurrentContext().setZuulEngineRan(); + RequestContext.getCurrentContext().setZuulEngineRan(); this.request.setRequestURI("/zuul/api/bar/1"); ctx.setRequest(this.request); - + this.properties.setPrefix("/api"); this.properties.setStripPrefix(true); this.properties.setServletPath("/zuul"); this.routeLocator.addRoute( new ZuulRoute("foo", "/foo/**", null, "forward:/foo", true, null, null)); - - + this.filter.run(); assertEquals("/api/bar/1", ctx.get("forward.to")); - } - + } + @Test public void routeNotFoundZuulRequestDispatcherServletSpecialPath() throws Exception { setTestRequestContext(); RequestContext ctx = RequestContext.getCurrentContext(); - ctx.getCurrentContext().setZuulEngineRan(); + RequestContext.getCurrentContext().setZuulEngineRan(); this.request.setRequestURI("/zuul/api/bar/1"); ctx.setRequest(this.request); - + this.properties.setPrefix("/api"); this.properties.setStripPrefix(true); this.properties.setServletPath("/zuul"); this.properties.setAddProxyHeaders(true); this.routeLocator.addRoute( new ZuulRoute("foo", "/foo/**", null, "forward:/foo", true, null, null)); - this.filter = new PreDecorationFilter(this.routeLocator, - "/special", this.properties, proxyRequestHelper); - - + this.filter = new PreDecorationFilter(this.routeLocator, "/special", + this.properties, this.proxyRequestHelper); + this.filter.run(); assertEquals("/special/api/bar/1", ctx.get("forward.to")); - } - - + } + @Test public void routeNotFoundZuulRequestZuulHomeMapping() throws Exception { setTestRequestContext(); RequestContext ctx = RequestContext.getCurrentContext(); - ctx.getCurrentContext().setZuulEngineRan(); + RequestContext.getCurrentContext().setZuulEngineRan(); this.request.setRequestURI("/api/bar/1"); ctx.setRequest(this.request); - + this.properties.setPrefix("/api"); this.properties.setStripPrefix(true); this.properties.setServletPath("/"); this.properties.setAddProxyHeaders(true); this.routeLocator.addRoute( new ZuulRoute("foo", "/foo/**", null, "forward:/foo", true, null, null)); - - this.filter = new PreDecorationFilter(this.routeLocator, - "/special", this.properties, proxyRequestHelper); - + + this.filter = new PreDecorationFilter(this.routeLocator, "/special", + this.properties, this.proxyRequestHelper); + this.filter.run(); assertEquals("/special/api/bar/1", ctx.get("forward.to")); @@ -281,9 +282,30 @@ public class PreDecorationFilterTests { this.filter.run(); RequestContext ctx = RequestContext.getCurrentContext(); @SuppressWarnings("unchecked") - Set sensitiveHeaders = (Set) ctx.get(ProxyRequestHelper.IGNORED_HEADERS); - assertTrue("sensitiveHeaders is wrong", sensitiveHeaders.containsAll(Collections.singletonList("x-foo"))); - assertFalse("sensitiveHeaders is wrong", sensitiveHeaders.contains("Cookie")); + Set sensitiveHeaders = (Set) ctx + .get(ProxyRequestHelper.IGNORED_HEADERS); + assertTrue("sensitiveHeaders is wrong: " + sensitiveHeaders, + sensitiveHeaders.containsAll(Collections.singletonList("x-foo"))); + assertFalse("sensitiveHeaders is wrong: " + sensitiveHeaders, + sensitiveHeaders.contains("Cookie")); + } + + @Test + public void sensitiveHeadersOverrideEmpty() throws Exception { + this.properties.setPrefix("/api"); + this.properties.setStripPrefix(true); + this.properties.setSensitiveHeaders(Collections.singleton("x-bar")); + this.request.setRequestURI("/api/foo/1"); + ZuulRoute route = new ZuulRoute("/foo/**", "foo"); + route.setSensitiveHeaders(Collections. emptySet()); + this.routeLocator.addRoute(route); + this.filter.run(); + RequestContext ctx = RequestContext.getCurrentContext(); + @SuppressWarnings("unchecked") + Set sensitiveHeaders = (Set) ctx + .get(ProxyRequestHelper.IGNORED_HEADERS); + assertTrue("sensitiveHeaders is wrong: " + sensitiveHeaders, + sensitiveHeaders.isEmpty()); } @Test @@ -296,11 +318,13 @@ public class PreDecorationFilterTests { this.filter.run(); RequestContext ctx = RequestContext.getCurrentContext(); @SuppressWarnings("unchecked") - Set sensitiveHeaders = (Set) ctx.get(ProxyRequestHelper.IGNORED_HEADERS); - assertTrue("sensitiveHeaders is wrong", sensitiveHeaders.containsAll(Collections.singletonList("x-bar"))); + Set sensitiveHeaders = (Set) ctx + .get(ProxyRequestHelper.IGNORED_HEADERS); + assertTrue("sensitiveHeaders is wrong: " + sensitiveHeaders, + sensitiveHeaders.containsAll(Collections.singletonList("x-bar"))); assertFalse("sensitiveHeaders is wrong", sensitiveHeaders.contains("Cookie")); } - + @Test public void sensitiveHeadersCaseInsensitive() throws Exception { this.properties.setPrefix("/api"); @@ -311,10 +335,12 @@ public class PreDecorationFilterTests { this.filter.run(); RequestContext ctx = RequestContext.getCurrentContext(); @SuppressWarnings("unchecked") - Set sensitiveHeaders = (Set) ctx.get(ProxyRequestHelper.IGNORED_HEADERS); - assertTrue("sensitiveHeaders is wrong", sensitiveHeaders.containsAll(Collections.singletonList("x-bar"))); + Set sensitiveHeaders = (Set) ctx + .get(ProxyRequestHelper.IGNORED_HEADERS); + assertTrue("sensitiveHeaders is wrong: " + sensitiveHeaders, + sensitiveHeaders.containsAll(Collections.singletonList("x-bar"))); } - + @Test public void sensitiveHeadersOverrideCaseInsensitive() throws Exception { this.properties.setPrefix("/api"); @@ -327,23 +353,29 @@ public class PreDecorationFilterTests { this.filter.run(); RequestContext ctx = RequestContext.getCurrentContext(); @SuppressWarnings("unchecked") - Set sensitiveHeaders = (Set) ctx.get(ProxyRequestHelper.IGNORED_HEADERS); - assertTrue("sensitiveHeaders is wrong", sensitiveHeaders.containsAll(Collections.singletonList("x-foo"))); + Set sensitiveHeaders = (Set) ctx + .get(ProxyRequestHelper.IGNORED_HEADERS); + assertTrue("sensitiveHeaders is wrong: " + sensitiveHeaders, + sensitiveHeaders.containsAll(Collections.singletonList("x-foo"))); } - + @Test - public void ignoredHeadersAlreadySetInRequestContextDontGetOverridden() throws Exception { + public void ignoredHeadersAlreadySetInRequestContextDontGetOverridden() + throws Exception { this.properties.setPrefix("/api"); this.properties.setStripPrefix(true); this.properties.setSensitiveHeaders(Collections.singleton("x-bar")); this.request.setRequestURI("/api/foo/1"); this.routeLocator.addRoute("/foo/**", "foo"); RequestContext ctx = RequestContext.getCurrentContext(); - ctx.set(ProxyRequestHelper.IGNORED_HEADERS, new HashSet<>(Arrays.asList("x-foo"))); + ctx.set(ProxyRequestHelper.IGNORED_HEADERS, + new HashSet<>(Arrays.asList("x-foo"))); this.filter.run(); @SuppressWarnings("unchecked") - Set sensitiveHeaders = (Set) ctx.get(ProxyRequestHelper.IGNORED_HEADERS); - assertTrue("sensitiveHeaders is wrong", sensitiveHeaders.containsAll(Arrays.asList("x-bar","x-foo"))); + Set sensitiveHeaders = (Set) ctx + .get(ProxyRequestHelper.IGNORED_HEADERS); + assertTrue("sensitiveHeaders is wrong: " + sensitiveHeaders, + sensitiveHeaders.containsAll(Arrays.asList("x-bar", "x-foo"))); } private Object getHeader(List> headers, String key) { @@ -356,11 +388,11 @@ public class PreDecorationFilterTests { } return value; } - + private void setTestRequestContext() { RequestContext context = new RequestContext(); RequestContext.testSetCurrentContext(context); - } + } }