Repository: knox Updated Branches: refs/heads/master 2717b1574 -> e844eff90
KNOX-1375 - Dispatch whitelist validation should decode URLs before testing Project: http://git-wip-us.apache.org/repos/asf/knox/repo Commit: http://git-wip-us.apache.org/repos/asf/knox/commit/e844eff9 Tree: http://git-wip-us.apache.org/repos/asf/knox/tree/e844eff9 Diff: http://git-wip-us.apache.org/repos/asf/knox/diff/e844eff9 Branch: refs/heads/master Commit: e844eff90b5993da01ab6027a93265d3f1f0077f Parents: 2717b15 Author: Phil Zampino <pzamp...@apache.org> Authored: Tue Jul 3 17:19:47 2018 -0400 Committer: Phil Zampino <pzamp...@apache.org> Committed: Tue Jul 3 17:19:47 2018 -0400 ---------------------------------------------------------------------- .../gateway/service/knoxsso/WebSSOResource.java | 24 ++++++-- .../service/knoxsso/WebSSOResourceTest.java | 60 ++++++++++++++++++++ .../gateway/dispatch/GatewayDispatchFilter.java | 11 +++- .../knox/gateway/util/WhitelistUtils.java | 3 +- .../dispatch/GatewayDispatchFilterTest.java | 39 ++++++++++++- 5 files changed, 129 insertions(+), 8 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/knox/blob/e844eff9/gateway-service-knoxsso/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOResource.java ---------------------------------------------------------------------- diff --git a/gateway-service-knoxsso/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOResource.java b/gateway-service-knoxsso/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOResource.java index f207432..cda3d48 100644 --- a/gateway-service-knoxsso/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOResource.java +++ b/gateway-service-knoxsso/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOResource.java @@ -18,8 +18,10 @@ package org.apache.knox.gateway.service.knoxsso; import java.io.IOException; +import java.io.UnsupportedEncodingException; import java.net.URI; import java.net.URISyntaxException; +import java.net.URLDecoder; import java.security.Principal; import java.util.ArrayList; import java.util.Arrays; @@ -168,8 +170,8 @@ public class WebSSOResource { } private Response getAuthenticationToken(int statusCode) { - GatewayServices services = (GatewayServices) request.getServletContext() - .getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE); + GatewayServices services = + (GatewayServices) request.getServletContext().getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE); boolean removeOriginalUrlCookie = true; String original = getCookieValue((HttpServletRequest) request, ORIGINAL_URL_COOKIE_NAME); if (original == null) { @@ -182,11 +184,25 @@ public class WebSSOResource { throw new WebApplicationException("Original URL not found in the request.", Response.Status.BAD_REQUEST); } - boolean validRedirect = (whitelist == null) || RegExUtils.checkWhitelist(whitelist, original); + boolean validRedirect = true; + + // If there is a whitelist defined, then the original URL must be validated against it. + // If there is no whitelist, then everything is valid. + if (whitelist != null) { + String decodedOriginal = null; + try { + decodedOriginal = URLDecoder.decode(original, "UTF-8"); + } catch (UnsupportedEncodingException e) { + // + } + + validRedirect = RegExUtils.checkWhitelist(whitelist, (decodedOriginal != null ? decodedOriginal : original)); + } + if (!validRedirect) { log.whiteListMatchFail(original, whitelist); throw new WebApplicationException("Original URL not valid according to the configured whitelist.", - Response.Status.BAD_REQUEST); + Response.Status.BAD_REQUEST); } } http://git-wip-us.apache.org/repos/asf/knox/blob/e844eff9/gateway-service-knoxsso/src/test/java/org/apache/knox/gateway/service/knoxsso/WebSSOResourceTest.java ---------------------------------------------------------------------- diff --git a/gateway-service-knoxsso/src/test/java/org/apache/knox/gateway/service/knoxsso/WebSSOResourceTest.java b/gateway-service-knoxsso/src/test/java/org/apache/knox/gateway/service/knoxsso/WebSSOResourceTest.java index 58877e4..19a8f03 100644 --- a/gateway-service-knoxsso/src/test/java/org/apache/knox/gateway/service/knoxsso/WebSSOResourceTest.java +++ b/gateway-service-knoxsso/src/test/java/org/apache/knox/gateway/service/knoxsso/WebSSOResourceTest.java @@ -17,6 +17,7 @@ */ package org.apache.knox.gateway.service.knoxsso; +import org.apache.http.HttpStatus; import org.apache.knox.gateway.config.GatewayConfig; import org.apache.knox.gateway.util.RegExUtils; import static org.junit.Assert.assertEquals; @@ -25,6 +26,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import java.lang.reflect.Field; +import java.net.URLEncoder; import java.security.KeyPair; import java.security.KeyPairGenerator; import java.security.NoSuchAlgorithmException; @@ -46,6 +48,7 @@ import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpServletResponseWrapper; +import javax.ws.rs.WebApplicationException; import org.apache.knox.gateway.services.GatewayServices; import org.apache.knox.gateway.services.security.token.JWTokenAuthority; @@ -645,6 +648,63 @@ public class WebSSOResourceTest { } @Test + public void testWhitelistValidationWithEncodedOriginalURL() throws Exception { + GatewayConfig config = EasyMock.createNiceMock(GatewayConfig.class); + EasyMock.expect(config.getDispatchWhitelistServices()).andReturn(Collections.emptyList()).anyTimes(); + EasyMock.expect(config.getDispatchWhitelist()).andReturn(null).anyTimes(); + EasyMock.replay(config); + + ServletContext context = EasyMock.createNiceMock(ServletContext.class); + EasyMock.expect(context.getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE)).andReturn(config).anyTimes(); + EasyMock.expect(context.getInitParameter("knoxsso.cookie.name")).andReturn(null); + EasyMock.expect(context.getInitParameter("knoxsso.cookie.secure.only")).andReturn(null); + EasyMock.expect(context.getInitParameter("knoxsso.cookie.max.age")).andReturn(null); + EasyMock.expect(context.getInitParameter("knoxsso.cookie.domain.suffix")).andReturn(null); + EasyMock.expect(context.getInitParameter("knoxsso.redirect.whitelist.regex")).andReturn(null); + EasyMock.expect(context.getInitParameter("knoxsso.token.audiences")).andReturn(null); + EasyMock.expect(context.getInitParameter("knoxsso.token.ttl")).andReturn(null); + EasyMock.expect(context.getInitParameter("knoxsso.enable.session")).andReturn(null); + + HttpServletRequest request = EasyMock.createNiceMock(HttpServletRequest.class); + EasyMock.expect(request.getParameter("originalUrl")).andReturn(URLEncoder.encode("http://disallowedhost:9080/service", + "UTF-8")); + EasyMock.expect(request.getAttribute("targetServiceRole")).andReturn("KNOXSSO").anyTimes(); + EasyMock.expect(request.getParameterMap()).andReturn(Collections.<String,String[]>emptyMap()); + EasyMock.expect(request.getServletContext()).andReturn(context).anyTimes(); + + Principal principal = EasyMock.createNiceMock(Principal.class); + EasyMock.expect(principal.getName()).andReturn("alice").anyTimes(); + EasyMock.expect(request.getUserPrincipal()).andReturn(principal).anyTimes(); + + GatewayServices services = EasyMock.createNiceMock(GatewayServices.class); + EasyMock.expect(context.getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE)).andReturn(services); + + JWTokenAuthority authority = new TestJWTokenAuthority(publicKey, privateKey); + EasyMock.expect(services.getService(GatewayServices.TOKEN_SERVICE)).andReturn(authority); + + HttpServletResponse response = EasyMock.createNiceMock(HttpServletResponse.class); + ServletOutputStream outputStream = EasyMock.createNiceMock(ServletOutputStream.class); + CookieResponseWrapper responseWrapper = new CookieResponseWrapper(response, outputStream); + + EasyMock.replay(principal, services, context, request); + + WebSSOResource webSSOResponse = new WebSSOResource(); + webSSOResponse.request = request; + webSSOResponse.response = responseWrapper; + webSSOResponse.context = context; + webSSOResponse.init(); + + try { + webSSOResponse.doGet(); + } catch (WebApplicationException e) { + // Expected + int status = e.getResponse().getStatus(); + assertEquals(HttpStatus.SC_BAD_REQUEST, status); + } + } + + + @Test public void testTopologyDefinedWhitelist() throws Exception { final String testServiceRole = "TEST"; http://git-wip-us.apache.org/repos/asf/knox/blob/e844eff9/gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/GatewayDispatchFilter.java ---------------------------------------------------------------------- diff --git a/gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/GatewayDispatchFilter.java b/gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/GatewayDispatchFilter.java index f4d8383..f23ba55 100644 --- a/gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/GatewayDispatchFilter.java +++ b/gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/GatewayDispatchFilter.java @@ -32,7 +32,9 @@ import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import java.io.IOException; +import java.io.UnsupportedEncodingException; import java.net.URISyntaxException; +import java.net.URLDecoder; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -140,7 +142,14 @@ public class GatewayDispatchFilter extends AbstractGatewayFilter { if (whitelist != null) { String requestURI = request.getRequestURI(); - isAllowed = RegExUtils.checkWhitelist(whitelist, requestURI); + String decodedURL = null; + try { + decodedURL = URLDecoder.decode(requestURI, "UTF-8"); + } catch (UnsupportedEncodingException e) { + // + } + + isAllowed = RegExUtils.checkWhitelist(whitelist, (decodedURL != null ? decodedURL : requestURI)); if (!isAllowed) { LOG.dispatchDisallowed(requestURI); http://git-wip-us.apache.org/repos/asf/knox/blob/e844eff9/gateway-spi/src/main/java/org/apache/knox/gateway/util/WhitelistUtils.java ---------------------------------------------------------------------- diff --git a/gateway-spi/src/main/java/org/apache/knox/gateway/util/WhitelistUtils.java b/gateway-spi/src/main/java/org/apache/knox/gateway/util/WhitelistUtils.java index 37df2f6..ae5e519 100644 --- a/gateway-spi/src/main/java/org/apache/knox/gateway/util/WhitelistUtils.java +++ b/gateway-spi/src/main/java/org/apache/knox/gateway/util/WhitelistUtils.java @@ -44,7 +44,8 @@ public class WhitelistUtils { public static String getDispatchWhitelist(HttpServletRequest request) { String whitelist = null; - GatewayConfig config = (GatewayConfig) request.getServletContext().getAttribute("org.apache.knox.gateway.config"); + GatewayConfig config = + (GatewayConfig) request.getServletContext().getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE); if (config != null) { List<String> whitelistedServiceRoles = new ArrayList<>(); whitelistedServiceRoles.addAll(DEFAULT_SERVICE_ROLES); http://git-wip-us.apache.org/repos/asf/knox/blob/e844eff9/gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/GatewayDispatchFilterTest.java ---------------------------------------------------------------------- diff --git a/gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/GatewayDispatchFilterTest.java b/gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/GatewayDispatchFilterTest.java index 1ad7b18..02f6fa1 100644 --- a/gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/GatewayDispatchFilterTest.java +++ b/gateway-spi/src/test/java/org/apache/knox/gateway/dispatch/GatewayDispatchFilterTest.java @@ -26,6 +26,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import java.io.IOException; import java.lang.reflect.Method; +import java.net.URLEncoder; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -88,7 +89,8 @@ public class GatewayDispatchFilterTest { doTestServiceDispatchWhitelist(Collections.singletonList(serviceRole), null, serviceRole, - "http://www.notonmylist.org:9999", false); + "http://www.notonmylist.org:9999", + false); } /** @@ -118,7 +120,8 @@ public class GatewayDispatchFilterTest { "knoxbox.test.org", null, serviceRole, - "http://www.notonmylist.org:9999", false); + "http://www.notonmylist.org:9999", + false); } /** @@ -137,6 +140,38 @@ public class GatewayDispatchFilterTest { } /** + * If the dispatch service is configured to honor the whitelist, but no whitelist is configured, then the default + * whitelist should be applied. If the dispatch URL does match the default whitelist, then the dispatch should be + * allowed. + */ + @Test + public void testServiceDispatchWhitelistNoWhiteListForRole_encodedurl_valid() throws Exception { + final String serviceRole = "TESTROLE"; + doTestServiceDispatchWhitelist(Collections.singletonList(serviceRole), + null, + serviceRole, + URLEncoder.encode("http://localhost:9999", "UTF-8"), + true); + } + + + /** + * If the dispatch service is configured to honor the whitelist, but DEFAULT whitelist is configured, then the default + * whitelist should be applied. If the dispatch URL does match the default whitelist, then the dispatch should be + * allowed. + */ + @Test + public void testServiceDispatchWhitelistNoWhiteListForRole_encodedurl_invalid() throws Exception { + final String serviceRole = "TESTROLE"; + doTestServiceDispatchWhitelist(Collections.singletonList(serviceRole), + "DEFAULT", + serviceRole, + URLEncoder.encode("http://www.notonmylist.org:9999", "UTF-8"), + false); + } + + + /** * An empty whitelist should be treated as the default whitelist. */ @Test