This is an automated email from the ASF dual-hosted git repository. lmccay pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/knox.git
The following commit(s) were added to refs/heads/master by this push: new 11a9b6e KNOX-2467 - Enable Jetty's X-Forwarded Header Support (#382) 11a9b6e is described below commit 11a9b6e47465ddc5d374d2fcd444b915cb1bd32d Author: lmccay <lmc...@apache.org> AuthorDate: Sun Nov 1 20:12:09 2020 -0500 KNOX-2467 - Enable Jetty's X-Forwarded Header Support (#382) * KNOX-2467 - Enable Jetty's X-Forwarded Header Support --- .../provider/federation/jwt/JWTMessages.java | 3 + .../jwt/filter/SSOCookieFederationFilter.java | 49 +++++++------ .../provider/federation/SSOCookieProviderTest.java | 82 ++++++++++++++++++++-- .../org/apache/knox/gateway/GatewayServer.java | 4 ++ .../gateway/config/impl/GatewayConfigImpl.java | 6 ++ .../apache/knox/gateway/config/GatewayConfig.java | 7 ++ .../org/apache/knox/gateway/GatewayTestConfig.java | 4 ++ .../apache/knox/gateway/GatewayBasicFuncTest.java | 19 ++--- 8 files changed, 135 insertions(+), 39 deletions(-) diff --git a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java index 72696c0..e5ce601 100644 --- a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java +++ b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java @@ -58,4 +58,7 @@ public interface JWTMessages { @Message( level = MessageLevel.INFO, text = "Path {0} is configured as unauthenticated path, letting the request {1} through" ) void unauthenticatedPathBypass(String path, String uri); + + @Message( level = MessageLevel.WARN, text = "Unable to vderive authentication provider url: {0}" ) + void failedToDeriveAuthenticationProviderUrl(@StackTrace( level = MessageLevel.ERROR) Exception e); } diff --git a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/SSOCookieFederationFilter.java b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/SSOCookieFederationFilter.java index db56f55..7cf2804 100644 --- a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/SSOCookieFederationFilter.java +++ b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/SSOCookieFederationFilter.java @@ -38,6 +38,8 @@ import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import java.io.IOException; +import java.net.MalformedURLException; +import java.net.URL; import java.nio.charset.StandardCharsets; import java.text.ParseException; import java.util.HashSet; @@ -220,14 +222,18 @@ public class SSOCookieFederationFilter extends AbstractJWTFilter { * @return url to use as login url for redirect */ protected String constructLoginURL(HttpServletRequest request) { + String providerURL = null; String delimiter = "?"; if (authenticationProviderUrl == null) { - authenticationProviderUrl = deriveDefaultAuthenticationProviderUrl(request); + providerURL = deriveDefaultAuthenticationProviderUrl(request); } - if (authenticationProviderUrl.contains("?")) { + else { + providerURL = authenticationProviderUrl; + } + if (providerURL.contains("?")) { delimiter = "&"; } - return authenticationProviderUrl + delimiter + return providerURL + delimiter + ORIGINAL_URL_QUERY_PARAM + request.getRequestURL().append(getOriginalQueryString(request)); } @@ -239,31 +245,28 @@ public class SSOCookieFederationFilter extends AbstractJWTFilter { * @return url that is based on KnoxSSO endpoint */ public String deriveDefaultAuthenticationProviderUrl(HttpServletRequest request) { + String providerURL = null; String scheme; String host; int port; - if (!beingProxied(request)) { - scheme = request.getScheme(); - host = request.getServerName(); - port = request.getServerPort(); - } - else { - scheme = request.getHeader(X_FORWARDED_PROTO); - host = request.getHeader(X_FORWARDED_HOST); - port = Integer.parseInt(request.getHeader(X_FORWARDED_PORT)); - } - StringBuilder sb = new StringBuilder(scheme); - sb.append("://").append(host); - if (!host.contains(":")) { - sb.append(':').append(port); - } - sb.append('/').append(gatewayPath).append("/knoxsso/api/v1/websso"); + try { + URL url = new URL(request.getRequestURL().toString()); + scheme = url.getProtocol(); + host = url.getHost(); + port = url.getPort(); - return sb.toString(); - } + StringBuilder sb = new StringBuilder(scheme); + sb.append("://").append(host); + if (!host.contains(":") && port != -1) { + sb.append(':').append(port); + } + sb.append('/').append(gatewayPath).append("/knoxsso/api/v1/websso"); + providerURL = sb.toString(); + } catch (MalformedURLException e) { + LOGGER.failedToDeriveAuthenticationProviderUrl(e); + } - private boolean beingProxied(HttpServletRequest request) { - return (request.getHeader(X_FORWARDED_HOST) != null); + return providerURL; } private String getOriginalQueryString(HttpServletRequest request) { diff --git a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/SSOCookieProviderTest.java b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/SSOCookieProviderTest.java index 720ed06..b2a49db 100644 --- a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/SSOCookieProviderTest.java +++ b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/SSOCookieProviderTest.java @@ -173,21 +173,26 @@ public class SSOCookieProviderTest extends AbstractJWTFilterTest { String providerURL = ((TestSSOCookieFederationProvider) handler).deriveDefaultAuthenticationProviderUrl(request); Assert.assertNotNull("LoginURL should not be null.", providerURL); - Assert.assertEquals(providerURL, "https://localhost:8443/gateway/knoxsso/api/v1/websso"); + Assert.assertEquals(providerURL, "https://localhost:8888/gateway/knoxsso/api/v1/websso"); String loginURL = ((TestSSOCookieFederationProvider) handler).constructLoginURL(request); Assert.assertNotNull("LoginURL should not be null.", loginURL); - Assert.assertEquals(loginURL, "https://localhost:8443/gateway/knoxsso/api/v1/websso?originalUrl=" + SERVICE_URL); + Assert.assertEquals(loginURL, "https://localhost:8888/gateway/knoxsso/api/v1/websso?originalUrl=" + SERVICE_URL); } @Test public void testProxiedDefaultAuthenticationProviderURL() throws Exception { + // after KNOX-2467 enables jetty's xforwarded support this test has been + // changed to expect the X-Forwarded Headers to be resolved by + // httpRequest.getRequestURL instead of explicitly checking the headers + // ourselves. Leaving the headers set to show this is a proxied request + // but they really have no bearing on the results. Properties props = new Properties(); props.setProperty("gateway.path", "gateway"); handler.init(new TestFilterConfig(props)); HttpServletRequest request = EasyMock.createNiceMock(HttpServletRequest.class); - EasyMock.expect(request.getRequestURL()).andReturn(new StringBuffer(SERVICE_URL)).anyTimes(); + EasyMock.expect(request.getRequestURL()).andReturn(new StringBuffer("https://remotehost:8443/resource")).anyTimes(); EasyMock.expect(request.getHeader(SSOCookieFederationFilter.X_FORWARDED_PROTO)).andReturn("https").anyTimes(); EasyMock.expect(request.getHeader(SSOCookieFederationFilter.X_FORWARDED_HOST)).andReturn("remotehost:8443").anyTimes(); EasyMock.expect(request.getHeader(SSOCookieFederationFilter.X_FORWARDED_PORT)).andReturn("8443").anyTimes(); @@ -199,17 +204,22 @@ public class SSOCookieProviderTest extends AbstractJWTFilterTest { String loginURL = ((TestSSOCookieFederationProvider) handler).constructLoginURL(request); Assert.assertNotNull("LoginURL should not be null.", loginURL); - Assert.assertEquals(loginURL, "https://remotehost:8443/gateway/knoxsso/api/v1/websso?originalUrl=" + SERVICE_URL); + Assert.assertEquals(loginURL, "https://remotehost:8443/gateway/knoxsso/api/v1/websso?originalUrl=" + "https://remotehost:8443/resource"); } @Test - public void testProxiedDefaultAuthenticationProviderURLWithoutPortInHostHeader() throws Exception { + public void testProxiedDefaultAuthenticationProviderURLWithoutNonGatewayAppPath() throws Exception { + // after KNOX-2467 enables jetty's xforwarded support this test has been + // changed to expect the X-Forwarded Headers to be resolved by + // httpRequest.getRequestURL instead of explicitly checking the headers + // ourselves. Leaving the headers set to show this is a proxied request + // but they really have no bearing on the results. Properties props = new Properties(); props.setProperty("gateway.path", "notgateway"); handler.init(new TestFilterConfig(props)); HttpServletRequest request = EasyMock.createNiceMock(HttpServletRequest.class); - EasyMock.expect(request.getRequestURL()).andReturn(new StringBuffer(SERVICE_URL)).anyTimes(); + EasyMock.expect(request.getRequestURL()).andReturn(new StringBuffer("https://remotehost:8443/resource")).anyTimes(); EasyMock.expect(request.getHeader(SSOCookieFederationFilter.X_FORWARDED_PROTO)).andReturn("https").anyTimes(); EasyMock.expect(request.getHeader(SSOCookieFederationFilter.X_FORWARDED_HOST)).andReturn("remotehost").anyTimes(); EasyMock.expect(request.getHeader(SSOCookieFederationFilter.X_FORWARDED_PORT)).andReturn("8443").anyTimes(); @@ -221,7 +231,65 @@ public class SSOCookieProviderTest extends AbstractJWTFilterTest { String loginURL = ((TestSSOCookieFederationProvider) handler).constructLoginURL(request); Assert.assertNotNull("LoginURL should not be null.", loginURL); - Assert.assertEquals(loginURL, "https://remotehost:8443/notgateway/knoxsso/api/v1/websso?originalUrl=" + SERVICE_URL); + Assert.assertEquals(loginURL, "https://remotehost:8443/notgateway/knoxsso/api/v1/websso?originalUrl=" + "https://remotehost:8443/resource"); + } + + @Test + public void testProxiedDefaultAuthenticationProviderURLWithoutPortInHostHeader() throws Exception { + // after KNOX-2467 enables jetty's xforwarded support this test has been + // changed to expect the X-Forwarded Headers to be resolved by + // httpRequest.getRequestURL instead of explicitly checking the headers + // ourselves. Leaving the headers set to show this is a proxied request + // but they really have no bearing on the results. + Properties props = new Properties(); + props.setProperty("gateway.path", "notgateway"); + handler.init(new TestFilterConfig(props)); + + HttpServletRequest request = EasyMock.createNiceMock(HttpServletRequest.class); + EasyMock.expect(request.getRequestURL()).andReturn(new StringBuffer("https://remotehost/resource")).anyTimes(); + EasyMock.expect(request.getHeader(SSOCookieFederationFilter.X_FORWARDED_PROTO)).andReturn("https").anyTimes(); + EasyMock.expect(request.getHeader(SSOCookieFederationFilter.X_FORWARDED_HOST)).andReturn("remotehost").anyTimes(); + EasyMock.replay(request); + + String providerURL = ((TestSSOCookieFederationProvider) handler).deriveDefaultAuthenticationProviderUrl(request); + Assert.assertNotNull("LoginURL should not be null.", providerURL); + Assert.assertEquals(providerURL, "https://remotehost/notgateway/knoxsso/api/v1/websso"); + + String loginURL = ((TestSSOCookieFederationProvider) handler).constructLoginURL(request); + Assert.assertNotNull("LoginURL should not be null.", loginURL); + Assert.assertEquals(loginURL, "https://remotehost/notgateway/knoxsso/api/v1/websso?originalUrl=" + "https://remotehost/resource"); + } + + @Test + public void testProxiedDefaultAuthenticationProviderURLWithoutMismatchInXForwardedHeader() throws Exception { + // after KNOX-2467 enables jetty's xforwarded support this test has been + // changed to expect the X-Forwarded Headers to be resolved by + // httpRequest.getRequestURL instead of explicitly checking the headers + // ourselves. Leaving the headers set to show this is a proxied request + // but they really have no bearing on the results. + + // this is an odd test but we want to make sure that the removed code + // that explicitly handled incoming xforwarded headers in the redirect + // url is actuall removed and all handling of xforwarded is handled by + // servlet container. + Properties props = new Properties(); + props.setProperty("gateway.path", "notgateway"); + handler.init(new TestFilterConfig(props)); + + HttpServletRequest request = EasyMock.createNiceMock(HttpServletRequest.class); + EasyMock.expect(request.getRequestURL()).andReturn(new StringBuffer("https://remotehost/resource")).anyTimes(); + EasyMock.expect(request.getHeader(SSOCookieFederationFilter.X_FORWARDED_PROTO)).andReturn("http").anyTimes(); + EasyMock.expect(request.getHeader(SSOCookieFederationFilter.X_FORWARDED_HOST)).andReturn("larryhost").anyTimes(); + EasyMock.expect(request.getHeader(SSOCookieFederationFilter.X_FORWARDED_PORT)).andReturn("7777").anyTimes(); + EasyMock.replay(request); + + String providerURL = ((TestSSOCookieFederationProvider) handler).deriveDefaultAuthenticationProviderUrl(request); + Assert.assertNotNull("LoginURL should not be null.", providerURL); + Assert.assertEquals(providerURL, "https://remotehost/notgateway/knoxsso/api/v1/websso"); + + String loginURL = ((TestSSOCookieFederationProvider) handler).constructLoginURL(request); + Assert.assertNotNull("LoginURL should not be null.", loginURL); + Assert.assertEquals(loginURL, "https://remotehost/notgateway/knoxsso/api/v1/websso?originalUrl=" + "https://remotehost/resource"); } @Override diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServer.java b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServer.java index f92e2d7..dab2710 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServer.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServer.java @@ -386,6 +386,10 @@ public class GatewayServer { for (ConnectionFactory x : networkConnector.getConnectionFactories()) { if (x instanceof HttpConnectionFactory) { ((HttpConnectionFactory) x).getHttpConfiguration().setSendServerVersion(config.isGatewayServerHeaderEnabled()); + if (config.isGatewayServerIncomingXForwardedSupportEnabled()) { + // Add support for X-Forwarded headers + ((HttpConnectionFactory) x).getHttpConfiguration().addCustomizer(new org.eclipse.jetty.server.ForwardedRequestCustomizer()); + } } } if (networkConnector.getName() == null) { diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java b/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java index f2c33f3..4efcd13 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java @@ -266,6 +266,7 @@ public class GatewayConfigImpl extends Configuration implements GatewayConfig { private static final String KNOX_HOMEPAGE_HIDDEN_TOPOLOGIES = "knox.homepage.hidden.topologies"; private static final Set<String> KNOX_HOMEPAGE_HIDDEN_TOPOLOGIES_DEFAULT = new HashSet<>(Arrays.asList("admin", "manager", "knoxsso", "metadata", "homepage")); private static final String KNOX_HOMEPAGE_LOGOUT_ENABLED = "knox.homepage.logout.enabled"; + private static final String KNOX_INCOMING_XFORWARDED_ENABLED = "gateway.incoming.xforwarded.enabled"; public GatewayConfigImpl() { init(); @@ -1193,4 +1194,9 @@ public class GatewayConfigImpl extends Configuration implements GatewayConfig { public long getKeystoreCacheEntryTimeToLiveInMinutes() { return getLong(KEYSTORE_CACHE_ENTRY_TTL, DEFAULT_KEYSTORE_CACHE_ENTRY_TTL); } + + @Override + public boolean isGatewayServerIncomingXForwardedSupportEnabled() { + return getBoolean(KNOX_INCOMING_XFORWARDED_ENABLED, true); + } } diff --git a/gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java b/gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java index 124d968..41b2071 100644 --- a/gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java +++ b/gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java @@ -717,4 +717,11 @@ public interface GatewayConfig { * @return the time - in minutes - an entry should be live (i.e. must not expire) in keystore cache */ long getKeystoreCacheEntryTimeToLiveInMinutes(); + + /** + * Indicates whether the embedded Jetty Server support for X-Forwarded Headers should + * be enabled. + * @return true if incoming X-Forwarded headers are enabled + */ + boolean isGatewayServerIncomingXForwardedSupportEnabled(); } diff --git a/gateway-test-release-utils/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java b/gateway-test-release-utils/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java index 6b5b466..2b50450 100644 --- a/gateway-test-release-utils/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java +++ b/gateway-test-release-utils/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java @@ -833,4 +833,8 @@ public class GatewayTestConfig extends Configuration implements GatewayConfig { return 0; } + @Override + public boolean isGatewayServerIncomingXForwardedSupportEnabled() { + return true; + } } diff --git a/gateway-test/src/test/java/org/apache/knox/gateway/GatewayBasicFuncTest.java b/gateway-test/src/test/java/org/apache/knox/gateway/GatewayBasicFuncTest.java index a77524c..d395427 100644 --- a/gateway-test/src/test/java/org/apache/knox/gateway/GatewayBasicFuncTest.java +++ b/gateway-test/src/test/java/org/apache/knox/gateway/GatewayBasicFuncTest.java @@ -3463,10 +3463,6 @@ public class GatewayBasicFuncTest { String port = "8889"; String scheme = "https"; - InetSocketAddress gatewayAddress = driver.gateway.getAddresses()[0]; - String gatewayHostName = gatewayAddress.getHostName(); - String gatewayAddrName = InetAddress.getByName( gatewayHostName ).getHostAddress(); - //Test rewriting of body with X-Forwarded headers (using storm) String resourceName = "storm/topology-id.json"; String path = "/api/v1/topology/WordCount-1-1424792039"; @@ -3478,7 +3474,12 @@ public class GatewayBasicFuncTest { .header("X-Forwarded-Proto", scheme) .header("X-Forwarded-Port", port) .header("X-Forwarded-Context", "/gateway/cluster") - .header("X-Forwarded-Server", Matchers.is(oneOf( gatewayHostName, gatewayAddrName ) )) + // Since KNOX-2467 enables Jetty's X-Forwarded handling + // the following is no longer true and while possibly accurate + // in terms of the most recent proxy hostname, it should + // represent the host of the X-Forwarded-Host and does not here + //.header("X-Forwarded-Server", Matchers.is(oneOf( gatewayHostName, gatewayAddrName ) )) + .header("X-Forwarded-Server", host) .header("X-Forwarded-For", Matchers.containsString("what, boo")) .pathInfo(path) .queryParam("user.name", username) @@ -3518,7 +3519,7 @@ public class GatewayBasicFuncTest { .header("X-Forwarded-Proto", scheme) .header("X-Forwarded-Port", port) .header("X-Forwarded-Context", "/gateway/cluster") - .header("X-Forwarded-Server", Matchers.is(oneOf( gatewayHostName, gatewayAddrName ) )) + .header("X-Forwarded-Server", host) .header("X-Forwarded-For", Matchers.containsString("what, boo")) .pathInfo(path) .queryParam("user.name", username) @@ -3534,7 +3535,7 @@ public class GatewayBasicFuncTest { .header("X-Forwarded-Host", host) .header("X-Forwarded-Proto", scheme) .header("X-Forwarded-Port", port) - .header("X-Forwarded-Server", "what") + .header("X-Forwarded-Server", host) .header("X-Forwarded-For", "what, boo") .then() // .log().all() @@ -3561,7 +3562,7 @@ public class GatewayBasicFuncTest { .header("X-Forwarded-Proto", scheme) .header("X-Forwarded-Port", port) .header("X-Forwarded-Context", "/gateway/cluster") - .header("X-Forwarded-Server", Matchers.is(oneOf( gatewayHostName, gatewayAddrName ) )) + .header("X-Forwarded-Server", host) .header("X-Forwarded-For", Matchers.containsString("what, boo")) .queryParam("op", "CREATE") .queryParam( "user.name", username ) @@ -3575,7 +3576,7 @@ public class GatewayBasicFuncTest { .header("X-Forwarded-Host", host) .header("X-Forwarded-Proto", scheme) .header("X-Forwarded-Port", port) - .header("X-Forwarded-Server", "what") + .header("X-Forwarded-Server", host) .header("X-Forwarded-For", "what, boo") .queryParam( "op", "CREATE" ) .then()