From 3bb63b93b474f9971d1f6eebec5b13c6e7cb216e Mon Sep 17 00:00:00 2001 From: Bjørn Christian Seime Date: Fri, 6 Mar 2020 16:23:27 +0100 Subject: Support proxy protocol for https connectors --- jdisc_http_service/pom.xml | 6 ++ .../jdisc/http/server/jetty/ConnectorFactory.java | 52 +++++++++--- .../configdefinitions/jdisc.http.connector.def | 6 ++ .../jdisc/http/server/jetty/HttpServerTest.java | 96 ++++++++++++++++++++++ 4 files changed, 150 insertions(+), 10 deletions(-) (limited to 'jdisc_http_service') 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 @@ -85,6 +85,12 @@ httpmime test + + org.eclipse.jetty + jetty-client + ${jetty.version} + test + org.cthul cthul-matchers 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 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 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( -- cgit v1.2.3