Merge pull request #13123 from vespa-engine/bjorncs/connector-local-port

Ignore local port reported from proxy-protocol header

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);
         }
     }