diff options
author | Bjørn Christian Seime <bjorncs@verizonmedia.com> | 2020-04-30 14:36:16 +0200 |
---|---|---|
committer | Bjørn Christian Seime <bjorncs@verizonmedia.com> | 2020-04-30 14:36:19 +0200 |
commit | 9543e2c3f7ae725f0829306e0d94b1aee01ea58f (patch) | |
tree | b060b982ccdf1aa4df079010987a411b849471f2 /jdisc_http_service | |
parent | 485c6a41c13d4a386da63e082c185c81da798d21 (diff) |
Ignore local port reported from proxy-protocol header
Replace usage of ServletRequest.getLocalPort() with equivalent from
ServerConnector. The latter will not be overridden by the proxy-protocol
header if proxy-protocol is enabled for that connector.
Diffstat (limited to 'jdisc_http_service')
8 files changed, 58 insertions, 8 deletions
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/core/HttpServletRequestUtils.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/core/HttpServletRequestUtils.java index 1a559de1f1e..9fda60cfe6b 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/core/HttpServletRequestUtils.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/core/HttpServletRequestUtils.java @@ -2,6 +2,7 @@ package com.yahoo.jdisc.http.core; import org.eclipse.jetty.server.HttpConnection; +import org.eclipse.jetty.server.ServerConnector; import javax.servlet.http.HttpServletRequest; @@ -15,4 +16,13 @@ public class HttpServletRequestUtils { return (HttpConnection)request.getAttribute("org.eclipse.jetty.server.HttpConnection"); } + /** + * Note: {@link HttpServletRequest#getLocalPort()} may return the local port of the load balancer / reverse proxy if proxy-protocol is enabled. + * @return the actual local port of the underlying Jetty connector + */ + public static int getConnectorLocalPort(HttpServletRequest request) { + ServerConnector jettyConnector = (ServerConnector) getConnection(request).getConnector(); + return jettyConnector.getLocalPort(); + } + } diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java index 0074f5cfe89..d8b649c9db8 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/AccessLogRequestLog.java @@ -18,6 +18,8 @@ import java.util.Optional; import java.util.logging.Level; import java.util.logging.Logger; +import static com.yahoo.jdisc.http.core.HttpServletRequestUtils.getConnectorLocalPort; + /** * This class is a bridge between Jetty's {@link org.eclipse.jetty.server.handler.RequestLogHandler} * and our own configurable access logging in different formats provided by {@link AccessLog}. @@ -80,7 +82,7 @@ public class AccessLogRequestLog extends AbstractLifeCycle implements RequestLog } accessLogEntry.setHttpVersion(request.getProtocol()); accessLogEntry.setScheme(request.getScheme()); - accessLogEntry.setLocalPort(request.getLocalPort()); + accessLogEntry.setLocalPort(getConnectorLocalPort(request)); Principal principal = (Principal) request.getAttribute(ServletRequest.JDISC_REQUEST_PRINCIPAL); if (principal != null) { accessLogEntry.setUserPrincipal(principal); diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HealthCheckProxyHandler.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HealthCheckProxyHandler.java index 9dc3380baac..d93d738bb91 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HealthCheckProxyHandler.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HealthCheckProxyHandler.java @@ -36,6 +36,7 @@ import java.util.logging.Level; import java.util.logging.Logger; import static com.yahoo.jdisc.Response.Status.NOT_FOUND; +import static com.yahoo.jdisc.http.core.HttpServletRequestUtils.getConnectorLocalPort; /** * A handler that proxies status.html health checks @@ -84,7 +85,7 @@ class HealthCheckProxyHandler extends HandlerWrapper { @Override public void handle(String target, Request request, HttpServletRequest servletRequest, HttpServletResponse servletResponse) throws IOException, ServletException { - ProxyTarget proxyTarget = portToProxyTargetMapping.get(request.getLocalPort()); + ProxyTarget proxyTarget = portToProxyTargetMapping.get(getConnectorLocalPort(servletRequest)); if (proxyTarget != null) { if (servletRequest.getRequestURI().equals(HEALTH_CHECK_PATH)) { try (CloseableHttpResponse proxyResponse = proxyTarget.requestStatusHtml()) { diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactory.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactory.java index 1306d72a618..515753c88f0 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactory.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactory.java @@ -15,6 +15,7 @@ import java.security.cert.X509Certificate; import java.util.Enumeration; import static com.yahoo.jdisc.http.core.HttpServletRequestUtils.getConnection; +import static com.yahoo.jdisc.http.core.HttpServletRequestUtils.getConnectorLocalPort; /** * @author Simon Thoresen Hult @@ -38,11 +39,11 @@ class HttpRequestFactory { } } - // Implementation based on org.eclipse.jetty.server.Request.getRequestURL(), but with getLocalPort() as port + // Implementation based on org.eclipse.jetty.server.Request.getRequestURL(), but with the connector's local port instead public static URI getUri(HttpServletRequest servletRequest) { try { StringBuffer builder = new StringBuffer(128); - URIUtil.appendSchemeHostPort(builder, servletRequest.getScheme(), servletRequest.getServerName(), servletRequest.getLocalPort()); + URIUtil.appendSchemeHostPort(builder, servletRequest.getScheme(), servletRequest.getServerName(), getConnectorLocalPort(servletRequest)); builder.append(servletRequest.getRequestURI()); String query = servletRequest.getQueryString(); if (query != null) { diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/SecuredRedirectHandler.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/SecuredRedirectHandler.java index 7798b5e6ae3..6590b76f1ec 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/SecuredRedirectHandler.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/SecuredRedirectHandler.java @@ -14,6 +14,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import static com.yahoo.jdisc.http.core.HttpServletRequestUtils.getConnectorLocalPort; + /** * A secure redirect handler inspired by {@link org.eclipse.jetty.server.handler.SecuredRedirectHandler}. * @@ -31,7 +33,7 @@ class SecuredRedirectHandler extends HandlerWrapper { @Override public void handle(String target, Request request, HttpServletRequest servletRequest, HttpServletResponse servletResponse) throws IOException, ServletException { - int localPort = servletRequest.getLocalPort(); + int localPort = getConnectorLocalPort(servletRequest); if (!redirectMap.containsKey(localPort)) { _handler.handle(target, request, servletRequest, servletResponse); return; diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/TlsClientAuthenticationEnforcer.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/TlsClientAuthenticationEnforcer.java index 6ad38747091..3a8de717b79 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/TlsClientAuthenticationEnforcer.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/TlsClientAuthenticationEnforcer.java @@ -16,6 +16,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import static com.yahoo.jdisc.http.core.HttpServletRequestUtils.getConnectorLocalPort; + /** * A Jetty handler that enforces TLS client authentication with configurable white list. * @@ -59,7 +61,7 @@ class TlsClientAuthenticationEnforcer extends HandlerWrapper { } private boolean isRequestToWhitelistedBinding(HttpServletRequest servletRequest) { - int localPort = servletRequest.getLocalPort(); + int localPort = getConnectorLocalPort(servletRequest); List<String> whiteListedPaths = getWhitelistedPathsForPort(localPort); if (whiteListedPaths == null) { return true; // enforcer not enabled diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactoryTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactoryTest.java index 564475b5603..752e4bed119 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactoryTest.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactoryTest.java @@ -10,6 +10,7 @@ import com.yahoo.jdisc.handler.RequestHandler; import com.yahoo.jdisc.http.HttpRequest; import com.yahoo.jdisc.service.CurrentContainer; import org.eclipse.jetty.server.HttpConnection; +import org.eclipse.jetty.server.ServerConnector; import org.junit.Test; import javax.servlet.http.HttpServletRequest; @@ -67,7 +68,10 @@ public class HttpRequestFactoryTest { private static HttpServletRequest createMockRequest(String scheme, String serverName, String path, String queryString) { HttpServletRequest request = mock(HttpServletRequest.class); HttpConnection connection = mock(HttpConnection.class); + ServerConnector connector = mock(ServerConnector.class); + when(connector.getLocalPort()).thenReturn(LOCAL_PORT); when(connection.getCreatedTimeStamp()).thenReturn(System.currentTimeMillis()); + when(connection.getConnector()).thenReturn(connector); when(request.getAttribute("org.eclipse.jetty.server.HttpConnection")).thenReturn(connection); when(request.getProtocol()).thenReturn("HTTP/1.1"); when(request.getScheme()).thenReturn(scheme); diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java index dfa4fe37ef3..37f717437ee 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java @@ -103,6 +103,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.anyMap; import static org.mockito.Mockito.mock; @@ -733,12 +734,36 @@ public class HttpServerTest { assertLogEntryHasRemote(accessLogMock.logEntries.get(1), proxiedRemoteAddress, 0); } - private void sendJettyClientRequest(TestDriver testDriver, HttpClient client, Object tag) + @Test + public void requireThatJdiscLocalPortPropertyIsNotOverriddenByProxyProtocol() throws Exception { + Path privateKeyFile = tmpFolder.newFile().toPath(); + Path certificateFile = tmpFolder.newFile().toPath(); + generatePrivateKeyAndCertificate(privateKeyFile, certificateFile); + AccessLogMock accessLogMock = new AccessLogMock(); + TestDriver driver = createSslWithProxyProtocolTestDriver(certificateFile, privateKeyFile, accessLogMock, /*mixedMode*/false); + HttpClient client = createJettyHttpClient(certificateFile); + + String proxiedRemoteAddress = "192.168.0.100"; + int proxiedRemotePort = 12345; + String proxyLocalAddress = "10.0.0.10"; + int proxyLocalPort = 23456; + V2.Tag v2Tag = new V2.Tag(V2.Tag.Command.PROXY, null, V2.Tag.Protocol.STREAM, + proxiedRemoteAddress, proxiedRemotePort, proxyLocalAddress, proxyLocalPort, null); + ContentResponse response = sendJettyClientRequest(driver, client, v2Tag); + client.stop(); + assertThat(driver.close(), is(true)); + + int clientPort = Integer.parseInt(response.getHeaders().get("Jdisc-Local-Port")); + assertNotEquals(proxyLocalPort, clientPort); + } + + private ContentResponse sendJettyClientRequest(TestDriver testDriver, HttpClient client, Object tag) throws InterruptedException, ExecutionException, TimeoutException { ContentResponse response = client.newRequest(URI.create("https://localhost:" + testDriver.server().getListenPort() + "/")) .tag(tag) .send(); assertEquals(200, response.getStatus()); + return response; } // Using Jetty's http client as Apache httpclient does not support the proxy-protocol v1/v2. @@ -963,7 +988,10 @@ public class HttpServerTest { @Override public ContentChannel handleRequest(final Request request, final ResponseHandler handler) { - return handler.handleResponse(new Response(OK)); + int port = request.getUri().getPort(); + Response response = new Response(OK); + response.headers().put("Jdisc-Local-Port", Integer.toString(port)); + return handler.handleResponse(response); } } |