Support proxy protocol for https connectors

4 files changed, 150 insertions, 10 deletions

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 3b4475dd11a..5423200a94a 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..90bc34d0042 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,9 @@ import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
+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 +97,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;
@@ -655,6 +665,92 @@ 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));
+        assertThat(accessLogMock.logEntries, hasSize(2));
+        assertLogEntryHasRemote(accessLogMock.logEntries.get(0), proxiedRemoteAddress, proxiedRemotePort);
+        assertLogEntryHasRemote(accessLogMock.logEntries.get(1), proxiedRemoteAddress, proxiedRemotePort);
+        client.stop();
+        assertThat(driver.close(), is(true));
+    }
+
+    @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);
+
+        sendJettyClientRequest(driver, client, null);
+        assertThat(accessLogMock.logEntries, hasSize(1));
+        assertLogEntryHasRemote(accessLogMock.logEntries.get(0), "127.0.0.1", 0);
+
+        String proxiedRemoteAddress = "192.168.0.100";
+        sendJettyClientRequest(driver, client, new V2.Tag(proxiedRemoteAddress, 12345));
+        assertThat(accessLogMock.logEntries, hasSize(2));
+        assertLogEntryHasRemote(accessLogMock.logEntries.get(1), proxiedRemoteAddress, 0);
+
+        client.stop();
+        assertThat(driver.close(), is(true));
+    }
+
+    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(