diff options
author | Bjørn Christian Seime <bjorncs@verizonmedia.com> | 2020-03-02 13:09:24 +0100 |
---|---|---|
committer | Bjørn Christian Seime <bjorncs@verizonmedia.com> | 2020-03-03 16:15:30 +0100 |
commit | 8a1dbed65e4794ca7c25c481c61c13505eeaa7de (patch) | |
tree | a70462fc2e0c5c940b5f3321f6352f60959c2031 /jdisc_http_service | |
parent | 3b68d4c694fe879f6faa609431b291d8039f269a (diff) |
Report SSL handshake failures in metric
Diffstat (limited to 'jdisc_http_service')
4 files changed, 224 insertions, 7 deletions
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 01d6fa02d6e..2796ee2d271 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 @@ -42,7 +42,7 @@ public class ConnectorFactory { public ServerConnector createConnector(final Metric metric, final Server server, final ServerSocketChannel ch) { ServerConnector connector = new JDiscServerConnector( - connectorConfig, metric, server, ch, createConnectionFactories().toArray(ConnectionFactory[]::new)); + connectorConfig, metric, server, ch, createConnectionFactories(metric).toArray(ConnectionFactory[]::new)); connector.setPort(connectorConfig.listenPort()); connector.setName(connectorConfig.name()); connector.setAcceptQueueSize(connectorConfig.acceptQueueSize()); @@ -51,14 +51,14 @@ public class ConnectorFactory { return connector; } - private List<ConnectionFactory> createConnectionFactories() { + private List<ConnectionFactory> createConnectionFactories(Metric metric) { HttpConnectionFactory httpConnectionFactory = newHttpConnectionFactory(); if (connectorConfig.healthCheckProxy().enable()) { return List.of(httpConnectionFactory); } else if (connectorConfig.ssl().enabled()) { - return List.of(newSslConnectionFactory(), httpConnectionFactory); + return List.of(newSslConnectionFactory(metric), httpConnectionFactory); } else if (TransportSecurityUtils.isTransportSecurityEnabled()) { - SslConnectionFactory sslConnectionsFactory = newSslConnectionFactory(); + SslConnectionFactory sslConnectionsFactory = newSslConnectionFactory(metric); switch (TransportSecurityUtils.getInsecureMixedMode()) { case TLS_CLIENT_MIXED_SERVER: case PLAINTEXT_CLIENT_MIXED_SERVER: @@ -88,9 +88,11 @@ public class ConnectorFactory { return new HttpConnectionFactory(httpConfig); } - private SslConnectionFactory newSslConnectionFactory() { - SslContextFactory factory = sslContextFactoryProvider.getInstance(connectorConfig.name(), connectorConfig.listenPort()); - return new SslConnectionFactory(factory, HttpVersion.HTTP_1_1.asString()); + private SslConnectionFactory newSslConnectionFactory(Metric metric) { + SslContextFactory ctxFactory = sslContextFactoryProvider.getInstance(connectorConfig.name(), connectorConfig.listenPort()); + SslConnectionFactory connectionFactory = new SslConnectionFactory(ctxFactory, HttpVersion.HTTP_1_1.asString()); + connectionFactory.addBean(new SslHandshakeFailedListener(metric, connectorConfig.name(), connectorConfig.listenPort())); + return connectionFactory; } private OptionalSslConnectionFactory newOptionalSslConnectionFactory(SslConnectionFactory sslConnectionsFactory) { diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java index 140feb75026..1c4421859f1 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JettyHttpServer.java @@ -114,6 +114,12 @@ public class JettyHttpServer extends AbstractServerProvider { String URI_LENGTH = "jdisc.http.request.uri_length"; String CONTENT_SIZE = "jdisc.http.request.content_size"; + + String SSL_HANDSHAKE_FAILURE_MISSING_CLIENT_CERT = "jdisc.http.ssl.handshake-failure.missing-client-cert"; + String SSL_HANDSHAKE_FAILURE_INVALID_CLIENT_CERT = "jdisc.http.ssl.handshake-failure.invalid-client-cert"; + String SSL_HANDSHAKE_FAILURE_INCOMPATIBLE_PROTOCOLS = "jdisc.http.ssl.handshake-failure.incompatible-protocols"; + String SSL_HANDSHAKE_FAILURE_INCOMPATIBLE_CIPHERS = "jdisc.http.ssl.handshake-failure.incompatible-ciphers"; + String SSL_HANDSHAKE_FAILURE_UNKNOWN = "jdisc.http.ssl.handshake-failure.unknown"; } private final static Logger log = Logger.getLogger(JettyHttpServer.class.getName()); diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/SslHandshakeFailedListener.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/SslHandshakeFailedListener.java new file mode 100644 index 00000000000..84f4fd118cc --- /dev/null +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/SslHandshakeFailedListener.java @@ -0,0 +1,83 @@ +// Copyright 2020 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.jdisc.http.server.jetty; + +import com.yahoo.jdisc.Metric; +import com.yahoo.jdisc.http.server.jetty.JettyHttpServer.Metrics; +import org.eclipse.jetty.io.ssl.SslHandshakeListener; + +import javax.net.ssl.SSLHandshakeException; +import java.util.Map; +import java.util.Optional; +import java.util.function.Predicate; +import java.util.logging.Level; +import java.util.logging.Logger; +import java.util.regex.Pattern; + +/** + * A {@link SslHandshakeListener} that reports metrics for SSL handshake failures. + * + * @author bjorncs + */ +class SslHandshakeFailedListener implements SslHandshakeListener { + + private final static Logger log = Logger.getLogger(SslHandshakeFailedListener.class.getName()); + + private final Metric metric; + private final String connectorName; + private final int listenPort; + + SslHandshakeFailedListener(Metric metric, String connectorName, int listenPort) { + this.metric = metric; + this.connectorName = connectorName; + this.listenPort = listenPort; + } + + @Override + public void handshakeFailed(Event event, Throwable throwable) { + log.log(Level.FINE, throwable, () -> "Ssl handshake failed: " + throwable.getMessage()); + String metricName = SslHandshakeFailure.fromSslHandshakeException((SSLHandshakeException) throwable) + .map(SslHandshakeFailure::metricName) + .orElse(Metrics.SSL_HANDSHAKE_FAILURE_UNKNOWN); + metric.add(metricName, 1L, metric.createContext(createDimensions())); + } + + private Map<String, Object> createDimensions() { + return Map.of(Metrics.NAME_DIMENSION, connectorName, Metrics.PORT_DIMENSION, listenPort); + } + + private enum SslHandshakeFailure { + INCOMPATIBLE_PROTOCOLS( + Metrics.SSL_HANDSHAKE_FAILURE_INCOMPATIBLE_PROTOCOLS, + "(Client requested protocol \\S+? is not enabled or supported in server context" + + "|The client supported protocol versions \\[\\S+?\\] are not accepted by server preferences \\[\\S+?\\])"), + INCOMPATIBLE_CIPHERS( + Metrics.SSL_HANDSHAKE_FAILURE_INCOMPATIBLE_CIPHERS, + "no cipher suites in common"), + MISSING_CLIENT_CERT( + Metrics.SSL_HANDSHAKE_FAILURE_MISSING_CLIENT_CERT, + "Empty server certificate chain"), + INVALID_CLIENT_CERT( + Metrics.SSL_HANDSHAKE_FAILURE_INVALID_CLIENT_CERT, + "PKIX path (building|validation) failed: .+"); + + private final String metricName; + private final Predicate<String> messageMatcher; + + SslHandshakeFailure(String metricName, String messagePattern) { + this.metricName = metricName; + this.messageMatcher = Pattern.compile(messagePattern).asMatchPredicate(); + } + + String metricName() { return metricName; } + + static Optional<SslHandshakeFailure> fromSslHandshakeException(SSLHandshakeException exception) { + String message = exception.getMessage(); + for (SslHandshakeFailure failure : values()) { + if (failure.messageMatcher.test(message)) { + return Optional.of(failure); + } + } + return Optional.empty(); + } + } +} 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 0e7bdd409e1..3b926225b19 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 @@ -5,10 +5,12 @@ import com.google.inject.AbstractModule; import com.google.inject.Module; import com.yahoo.container.logging.AccessLog; import com.yahoo.container.logging.AccessLogEntry; +import com.yahoo.jdisc.Metric; import com.yahoo.jdisc.References; import com.yahoo.jdisc.Request; import com.yahoo.jdisc.Response; import com.yahoo.jdisc.application.BindingSetSelector; +import com.yahoo.jdisc.application.MetricConsumer; import com.yahoo.jdisc.handler.AbstractRequestHandler; import com.yahoo.jdisc.handler.CompletionHandler; import com.yahoo.jdisc.handler.ContentChannel; @@ -21,6 +23,8 @@ import com.yahoo.jdisc.http.Cookie; import com.yahoo.jdisc.http.HttpRequest; import com.yahoo.jdisc.http.HttpResponse; import com.yahoo.jdisc.http.ServerConfig; +import com.yahoo.jdisc.http.server.jetty.JettyHttpServer.Metrics; +import com.yahoo.jdisc.http.server.jetty.TestDrivers.TlsClientAuth; import com.yahoo.jdisc.service.BindingSetNotFoundException; import com.yahoo.security.KeyUtils; import com.yahoo.security.SslContextBuilder; @@ -34,6 +38,7 @@ import org.junit.Test; import org.junit.rules.TemporaryFolder; import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLHandshakeException; import javax.security.auth.x500.X500Principal; import java.io.IOException; import java.math.BigInteger; @@ -81,7 +86,10 @@ 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.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.anyMap; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; /** @@ -560,6 +568,110 @@ public class HttpServerTest { assertThat(driver.close(), is(true)); } + @Test + public void requireThatMetricIsIncrementedWhenClientIsMissingCertificateOnHandshake() throws IOException { + Path privateKeyFile = tmpFolder.newFile().toPath(); + Path certificateFile = tmpFolder.newFile().toPath(); + generatePrivateKeyAndCertificate(privateKeyFile, certificateFile); + var metricConsumer = new MetricConsumerMock(); + TestDriver driver = createSslTestDriver(certificateFile, privateKeyFile, metricConsumer); + + SSLContext clientCtx = new SslContextBuilder() + .withTrustStore(certificateFile) + .build(); + assertHttpsRequestTriggersSslHandshakeException( + driver, clientCtx, null, null, "Received fatal alert: bad_certificate"); + verify(metricConsumer.mockitoMock()) + .add(Metrics.SSL_HANDSHAKE_FAILURE_MISSING_CLIENT_CERT, 1L, MetricConsumerMock.STATIC_CONTEXT); + assertThat(driver.close(), is(true)); + } + + @Test + public void requireThatMetricIsIncrementedWhenClientUsesIncompatibleTlsVersion() throws IOException { + Path privateKeyFile = tmpFolder.newFile().toPath(); + Path certificateFile = tmpFolder.newFile().toPath(); + generatePrivateKeyAndCertificate(privateKeyFile, certificateFile); + var metricConsumer = new MetricConsumerMock(); + TestDriver driver = createSslTestDriver(certificateFile, privateKeyFile, metricConsumer); + + SSLContext clientCtx = new SslContextBuilder() + .withTrustStore(certificateFile) + .withKeyStore(privateKeyFile, certificateFile) + .build(); + + assertHttpsRequestTriggersSslHandshakeException( + driver, clientCtx, "TLSv1.1", null, "Received fatal alert: protocol_version"); + verify(metricConsumer.mockitoMock()) + .add(Metrics.SSL_HANDSHAKE_FAILURE_INCOMPATIBLE_PROTOCOLS, 1L, MetricConsumerMock.STATIC_CONTEXT); + assertThat(driver.close(), is(true)); + } + + @Test + public void requireThatMetricIsIncrementedWhenClientUsesIncompatibleCiphers() throws IOException { + Path privateKeyFile = tmpFolder.newFile().toPath(); + Path certificateFile = tmpFolder.newFile().toPath(); + generatePrivateKeyAndCertificate(privateKeyFile, certificateFile); + var metricConsumer = new MetricConsumerMock(); + TestDriver driver = createSslTestDriver(certificateFile, privateKeyFile, metricConsumer); + + SSLContext clientCtx = new SslContextBuilder() + .withTrustStore(certificateFile) + .withKeyStore(privateKeyFile, certificateFile) + .build(); + + assertHttpsRequestTriggersSslHandshakeException( + driver, clientCtx, null, "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA", "Received fatal alert: handshake_failure"); + verify(metricConsumer.mockitoMock()) + .add(Metrics.SSL_HANDSHAKE_FAILURE_INCOMPATIBLE_CIPHERS, 1L, MetricConsumerMock.STATIC_CONTEXT); + assertThat(driver.close(), is(true)); + } + + @Test + public void requireThatMetricIsIncrementedWhenClientUsesInvalidCertificateInHandshake() throws IOException { + Path serverPrivateKeyFile = tmpFolder.newFile().toPath(); + Path serverCertificateFile = tmpFolder.newFile().toPath(); + generatePrivateKeyAndCertificate(serverPrivateKeyFile, serverCertificateFile); + var metricConsumer = new MetricConsumerMock(); + TestDriver driver = createSslTestDriver(serverCertificateFile, serverPrivateKeyFile, metricConsumer); + + Path clientPrivateKeyFile = tmpFolder.newFile().toPath(); + Path clientCertificateFile = tmpFolder.newFile().toPath(); + generatePrivateKeyAndCertificate(clientPrivateKeyFile, clientCertificateFile); + + SSLContext clientCtx = new SslContextBuilder() + .withKeyStore(clientPrivateKeyFile, clientCertificateFile) + .withTrustStore(serverCertificateFile) + .build(); + + assertHttpsRequestTriggersSslHandshakeException( + driver, clientCtx, null, null, "Received fatal alert: certificate_unknown"); + verify(metricConsumer.mockitoMock()) + .add(Metrics.SSL_HANDSHAKE_FAILURE_INVALID_CLIENT_CERT, 1L, MetricConsumerMock.STATIC_CONTEXT); + assertThat(driver.close(), is(true)); + } + + private static TestDriver createSslTestDriver( + Path serverCertificateFile, Path serverPrivateKeyFile, MetricConsumerMock metricConsumer) throws IOException { + return TestDrivers.newInstanceWithSsl( + new EchoRequestHandler(), serverCertificateFile, serverPrivateKeyFile, TlsClientAuth.NEED, metricConsumer.asGuiceModule()); + } + + private static void assertHttpsRequestTriggersSslHandshakeException( + TestDriver testDriver, + SSLContext sslContext, + String protocolOverride, + String cipherOverride, + String expectedExceptionSubstring) throws IOException { + List<String> protocols = protocolOverride != null ? List.of(protocolOverride) : null; + List<String> ciphers = cipherOverride != null ? List.of(cipherOverride) : null; + try (var client = new SimpleHttpClient(sslContext, protocols, ciphers, testDriver.server().getListenPort(), false)) { + client.get("/status.html"); + fail("SSLHandshakeException expected"); + } catch (SSLHandshakeException e) { + assertThat(e.getMessage(), containsString(expectedExceptionSubstring)); + } + } + private static void generatePrivateKeyAndCertificate(Path privateKeyFile, Path certificateFile) throws IOException { KeyPair keyPair = KeyUtils.generateKeypair(RSA, 2048); Files.writeString(privateKeyFile, KeyUtils.toPem(keyPair.getPrivate())); @@ -750,4 +862,18 @@ public class HttpServerTest { return lhs.getName().compareTo(rhs.getName()); } } + + private static class MetricConsumerMock { + static final Metric.Context STATIC_CONTEXT = new Metric.Context() {}; + + private final MetricConsumer mockitoMock = mock(MetricConsumer.class); + + MetricConsumerMock() { + when(mockitoMock.createContext(anyMap())).thenReturn(STATIC_CONTEXT); + } + + MetricConsumer mockitoMock() { return mockitoMock; } + Module asGuiceModule() { return binder -> binder.bind(MetricConsumer.class).toInstance(mockitoMock); } + } + } |