Merge pull request #12516 from vespa-engine/bjorncs/support-proxy-protocol-in-jdisc

Support proxy protocol for https connectors

5 files changed, 187 insertions, 13 deletions

diff --git a/jdisc_http_service/abi-spec.json b/jdisc_http_service/abi-spec.json
index 508a6a84974..bb6285ab94f 100644
--- a/jdisc_http_service/abi-spec.json
+++ b/jdisc_http_service/abi-spec.json
@@ -41,6 +41,7 @@
       "public com.yahoo.jdisc.http.ConnectorConfig$Builder ssl(com.yahoo.jdisc.http.ConnectorConfig$Ssl$Builder)",
       "public com.yahoo.jdisc.http.ConnectorConfig$Builder tlsClientAuthEnforcer(com.yahoo.jdisc.http.ConnectorConfig$TlsClientAuthEnforcer$Builder)",
       "public com.yahoo.jdisc.http.ConnectorConfig$Builder healthCheckProxy(com.yahoo.jdisc.http.ConnectorConfig$HealthCheckProxy$Builder)",
+      "public com.yahoo.jdisc.http.ConnectorConfig$Builder proxyProtocol(com.yahoo.jdisc.http.ConnectorConfig$ProxyProtocol$Builder)",
       "public final boolean dispatchGetConfig(com.yahoo.config.ConfigInstance$Producer)",
       "public final java.lang.String getDefMd5()",
       "public final java.lang.String getDefName()",
@@ -51,7 +52,8 @@
       "public com.yahoo.jdisc.http.ConnectorConfig$Throttling$Builder throttling",
       "public com.yahoo.jdisc.http.ConnectorConfig$Ssl$Builder ssl",
       "public com.yahoo.jdisc.http.ConnectorConfig$TlsClientAuthEnforcer$Builder tlsClientAuthEnforcer",
-      "public com.yahoo.jdisc.http.ConnectorConfig$HealthCheckProxy$Builder healthCheckProxy"
+      "public com.yahoo.jdisc.http.ConnectorConfig$HealthCheckProxy$Builder healthCheckProxy",
+      "public com.yahoo.jdisc.http.ConnectorConfig$ProxyProtocol$Builder proxyProtocol"
     ]
   },
   "com.yahoo.jdisc.http.ConnectorConfig$HealthCheckProxy$Builder": {
@@ -100,6 +102,37 @@
     ],
     "fields": []
   },
+  "com.yahoo.jdisc.http.ConnectorConfig$ProxyProtocol$Builder": {
+    "superClass": "java.lang.Object",
+    "interfaces": [
+      "com.yahoo.config.ConfigBuilder"
+    ],
+    "attributes": [
+      "public"
+    ],
+    "methods": [
+      "public void <init>()",
+      "public void <init>(com.yahoo.jdisc.http.ConnectorConfig$ProxyProtocol)",
+      "public com.yahoo.jdisc.http.ConnectorConfig$ProxyProtocol$Builder enabled(boolean)",
+      "public com.yahoo.jdisc.http.ConnectorConfig$ProxyProtocol$Builder mixedMode(boolean)",
+      "public com.yahoo.jdisc.http.ConnectorConfig$ProxyProtocol build()"
+    ],
+    "fields": []
+  },
+  "com.yahoo.jdisc.http.ConnectorConfig$ProxyProtocol": {
+    "superClass": "com.yahoo.config.InnerNode",
+    "interfaces": [],
+    "attributes": [
+      "public",
+      "final"
+    ],
+    "methods": [
+      "public void <init>(com.yahoo.jdisc.http.ConnectorConfig$ProxyProtocol$Builder)",
+      "public boolean enabled()",
+      "public boolean mixedMode()"
+    ],
+    "fields": []
+  },
   "com.yahoo.jdisc.http.ConnectorConfig$Ssl$Builder": {
     "superClass": "java.lang.Object",
     "interfaces": [
@@ -289,7 +322,8 @@
       "public com.yahoo.jdisc.http.ConnectorConfig$Throttling throttling()",
       "public com.yahoo.jdisc.http.ConnectorConfig$Ssl ssl()",
       "public com.yahoo.jdisc.http.ConnectorConfig$TlsClientAuthEnforcer tlsClientAuthEnforcer()",
-      "public com.yahoo.jdisc.http.ConnectorConfig$HealthCheckProxy healthCheckProxy()"
+      "public com.yahoo.jdisc.http.ConnectorConfig$HealthCheckProxy healthCheckProxy()",
+      "public com.yahoo.jdisc.http.ConnectorConfig$ProxyProtocol proxyProtocol()"
     ],
     "fields": [
       "public static final java.lang.String CONFIG_DEF_MD5",
diff --git a/jdisc_http_service/pom.xml b/jdisc_http_service/pom.xml
index c5555d5b690..bd8c77bc9cc 100644
--- a/jdisc_http_service/pom.xml
+++ b/jdisc_http_service/pom.xml
@@ -86,6 +86,12 @@
       <scope>test</scope>
     </dependency>
     <dependency>
+      <groupId>org.eclipse.jetty</groupId>
+      <artifactId>jetty-client</artifactId>
+      <version>${jetty.version}</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
       <groupId>org.cthul</groupId>
       <artifactId>cthul-matchers</artifactId>
       <scope>test</scope>
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactory.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactory.java
index 959735c4314..1a73b50bfa3 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactory.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactory.java
@@ -5,12 +5,13 @@ import com.google.inject.Inject;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.jdisc.http.ConnectorConfig;
 import com.yahoo.jdisc.http.ssl.SslContextFactoryProvider;
+import com.yahoo.security.tls.MixedMode;
 import com.yahoo.security.tls.TransportSecurityUtils;
-import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.server.ConnectionFactory;
 import org.eclipse.jetty.server.DetectorConnectionFactory;
 import org.eclipse.jetty.server.HttpConfiguration;
 import org.eclipse.jetty.server.HttpConnectionFactory;
+import org.eclipse.jetty.server.ProxyConnectionFactory;
 import org.eclipse.jetty.server.SecureRequestCustomizer;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
@@ -32,10 +33,28 @@ public class ConnectorFactory {
     @Inject
     public ConnectorFactory(ConnectorConfig connectorConfig,
                             SslContextFactoryProvider sslContextFactoryProvider) {
+        runtimeConnectorConfigValidation(connectorConfig);
         this.connectorConfig = connectorConfig;
         this.sslContextFactoryProvider = sslContextFactoryProvider;
     }
 
+    // Perform extra connector config validation that can only be performed at runtime,
+    // e.g. due to TLS configuration through environment variables.
+    private static void runtimeConnectorConfigValidation(ConnectorConfig config) {
+        validateProxyProtocolConfiguration(config);
+    }
+
+    private static void validateProxyProtocolConfiguration(ConnectorConfig config) {
+        ConnectorConfig.ProxyProtocol proxyProtocolConfig = config.proxyProtocol();
+        if (proxyProtocolConfig.enabled()) {
+            boolean sslEnabled = config.ssl().enabled() || TransportSecurityUtils.isTransportSecurityEnabled();
+            boolean tlsMixedModeEnabled = TransportSecurityUtils.getInsecureMixedMode() != MixedMode.DISABLED;
+            if (!sslEnabled || tlsMixedModeEnabled) {
+                throw new IllegalArgumentException("Proxy protocol can only be enabled if connector is effectively HTTPS only");
+            }
+        }
+    }
+
     public ConnectorConfig getConnectorConfig() {
         return connectorConfig;
     }
@@ -52,24 +71,37 @@ public class ConnectorFactory {
     }
 
     private List<ConnectionFactory> createConnectionFactories(Metric metric) {
-        HttpConnectionFactory httpConnectionFactory = newHttpConnectionFactory();
+        HttpConnectionFactory httpFactory = newHttpConnectionFactory();
         if (connectorConfig.healthCheckProxy().enable()) {
-            return List.of(httpConnectionFactory);
+            return List.of(httpFactory);
         } else if (connectorConfig.ssl().enabled()) {
-            return List.of(newSslConnectionFactory(metric), httpConnectionFactory);
+            return connectionFactoriesForHttps(metric, httpFactory);
         } else if (TransportSecurityUtils.isTransportSecurityEnabled()) {
-            SslConnectionFactory sslConnectionsFactory = newSslConnectionFactory(metric);
             switch (TransportSecurityUtils.getInsecureMixedMode()) {
                 case TLS_CLIENT_MIXED_SERVER:
                 case PLAINTEXT_CLIENT_MIXED_SERVER:
-                    return List.of(new DetectorConnectionFactory(sslConnectionsFactory), httpConnectionFactory);
+                    return List.of(new DetectorConnectionFactory(newSslConnectionFactory(metric, httpFactory)), httpFactory);
                 case DISABLED:
-                    return List.of(sslConnectionsFactory, httpConnectionFactory);
+                    return connectionFactoriesForHttps(metric, httpFactory);
                 default:
                     throw new IllegalStateException();
             }
         } else {
-            return List.of(httpConnectionFactory);
+            return List.of(httpFactory);
+        }
+    }
+
+    private List<ConnectionFactory> connectionFactoriesForHttps(Metric metric, HttpConnectionFactory httpFactory) {
+        ConnectorConfig.ProxyProtocol proxyProtocolConfig = connectorConfig.proxyProtocol();
+        SslConnectionFactory sslFactory = newSslConnectionFactory(metric, httpFactory);
+        if (proxyProtocolConfig.enabled()) {
+            if (proxyProtocolConfig.mixedMode()) {
+                return List.of(new DetectorConnectionFactory(sslFactory, new ProxyConnectionFactory(sslFactory.getProtocol())), sslFactory, httpFactory);
+            } else {
+                return List.of(new ProxyConnectionFactory(), sslFactory, httpFactory);
+            }
+        } else {
+            return List.of(sslFactory, httpFactory);
         }
     }
 
@@ -88,9 +120,9 @@ public class ConnectorFactory {
         return new HttpConnectionFactory(httpConfig);
     }
 
-    private SslConnectionFactory newSslConnectionFactory(Metric metric) {
+    private SslConnectionFactory newSslConnectionFactory(Metric metric, HttpConnectionFactory httpFactory) {
         SslContextFactory ctxFactory = sslContextFactoryProvider.getInstance(connectorConfig.name(), connectorConfig.listenPort());
-        SslConnectionFactory connectionFactory = new SslConnectionFactory(ctxFactory, HttpVersion.HTTP_1_1.asString());
+        SslConnectionFactory connectionFactory = new SslConnectionFactory(ctxFactory, httpFactory.getProtocol());
         connectionFactory.addBean(new SslHandshakeFailedListener(metric, connectorConfig.name(), connectorConfig.listenPort()));
         return connectionFactory;
     }
diff --git a/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.connector.def b/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.connector.def
index fe79ec2ffa3..8027525521c 100644
--- a/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.connector.def
+++ b/jdisc_http_service/src/main/resources/configdefinitions/jdisc.http.connector.def
@@ -100,3 +100,9 @@ healthCheckProxy.enable        bool    default=false
 
 # Which port to proxy
 healthCheckProxy.port          int     default=8080
+
+# Enable PROXY protocol V1/V2 support (only for https connectors).
+proxyProtocol.enabled          bool    default=false
+
+# Allow https in parallel with proxy protocol
+proxyProtocol.mixedMode        bool    default=false
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 a5bd0164e25..6ace9699b42 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
@@ -30,9 +30,15 @@ import com.yahoo.security.KeyUtils;
 import com.yahoo.security.SslContextBuilder;
 import com.yahoo.security.X509CertificateBuilder;
 import com.yahoo.security.X509CertificateUtils;
+import org.apache.http.conn.ssl.NoopHostnameVerifier;
 import org.apache.http.entity.ContentType;
 import org.apache.http.entity.mime.FormBodyPart;
 import org.apache.http.entity.mime.content.StringBody;
+import org.eclipse.jetty.client.HttpClient;
+import org.eclipse.jetty.client.ProxyProtocolClientConnectionFactory.V1;
+import org.eclipse.jetty.client.ProxyProtocolClientConnectionFactory.V2;
+import org.eclipse.jetty.client.api.ContentResponse;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
@@ -59,7 +65,10 @@ import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
@@ -89,6 +98,8 @@ import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.CoreMatchers.startsWith;
 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.fail;
 import static org.mockito.ArgumentMatchers.anyMap;
 import static org.mockito.Mockito.mock;
@@ -178,7 +189,7 @@ public class HttpServerTest {
 
     private static class AccessLogMock extends AccessLog {
 
-        final List<AccessLogEntry> logEntries = new ArrayList<>();
+        final List<AccessLogEntry> logEntries = new CopyOnWriteArrayList<>();
 
         AccessLogMock() { super(null); }
 
@@ -655,6 +666,91 @@ public class HttpServerTest {
         assertThat(driver.close(), is(true));
     }
 
+    @Test
+    public void requireThatProxyProtocolIsAcceptedAndActualRemoteAddressStoredInAccessLog() 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;
+        sendJettyClientRequest(driver, client, new V1.Tag(proxiedRemoteAddress, proxiedRemotePort));
+        sendJettyClientRequest(driver, client, new V2.Tag(proxiedRemoteAddress, proxiedRemotePort));
+        client.stop();
+        assertThat(driver.close(), is(true));
+
+        assertThat(accessLogMock.logEntries, hasSize(2));
+        assertLogEntryHasRemote(accessLogMock.logEntries.get(0), proxiedRemoteAddress, proxiedRemotePort);
+        assertLogEntryHasRemote(accessLogMock.logEntries.get(1), proxiedRemoteAddress, proxiedRemotePort);
+    }
+
+    @Test
+    public void requireThatConnectorWithProxyProtocolMixedEnabledAcceptsBothProxyProtocolAndHttps() 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*/true);
+        HttpClient client = createJettyHttpClient(certificateFile);
+
+        String proxiedRemoteAddress = "192.168.0.100";
+        sendJettyClientRequest(driver, client, null);
+        sendJettyClientRequest(driver, client, new V2.Tag(proxiedRemoteAddress, 12345));
+        client.stop();
+        assertThat(driver.close(), is(true));
+
+        assertThat(accessLogMock.logEntries, hasSize(2));
+        assertLogEntryHasRemote(accessLogMock.logEntries.get(0), "127.0.0.1", 0);
+        assertLogEntryHasRemote(accessLogMock.logEntries.get(1), proxiedRemoteAddress, 0);
+    }
+
+    private void 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());
+    }
+
+    // Using Jetty's http client as Apache httpclient does not support the proxy-protocol v1/v2.
+    private static HttpClient createJettyHttpClient(Path certificateFile) throws Exception {
+        SslContextFactory.Client clientSslCtxFactory = new SslContextFactory.Client();
+        clientSslCtxFactory.setHostnameVerifier(NoopHostnameVerifier.INSTANCE);
+        clientSslCtxFactory.setSslContext(new SslContextBuilder().withTrustStore(certificateFile).build());
+
+        HttpClient client = new HttpClient(clientSslCtxFactory);
+        client.start();
+        return client;
+    }
+
+    private static void assertLogEntryHasRemote(AccessLogEntry entry, String expectedAddress, int expectedPort) {
+        assertEquals(expectedAddress, entry.getPeerAddress());
+        if (expectedPort > 0) {
+            assertEquals(expectedPort, entry.getPeerPort());
+        }
+    }
+
+    private static TestDriver createSslWithProxyProtocolTestDriver(
+            Path certificateFile, Path privateKeyFile, AccessLogMock accessLogMock, boolean mixedMode) throws IOException {
+        ConnectorConfig.Builder connectorConfig = new ConnectorConfig.Builder()
+                .proxyProtocol(new ConnectorConfig.ProxyProtocol.Builder()
+                                       .enabled(true)
+                                       .mixedMode(mixedMode))
+                .ssl(new ConnectorConfig.Ssl.Builder()
+                             .enabled(true)
+                             .privateKeyFile(privateKeyFile.toString())
+                             .certificateFile(certificateFile.toString())
+                             .caCertificateFile(certificateFile.toString()));
+        return TestDrivers.newConfiguredInstance(
+                new EchoRequestHandler(),
+                new ServerConfig.Builder(),
+                connectorConfig,
+                binder -> binder.bind(AccessLog.class).toInstance(accessLogMock));
+    }
+
     private static TestDriver createSslTestDriver(
             Path serverCertificateFile, Path serverPrivateKeyFile, MetricConsumerMock metricConsumer) throws IOException {
         return TestDrivers.newInstanceWithSsl(