Merge pull request #12440 from vespa-engine/revert-12415-bjorncs/ssl-handshake-metric

Revert "Bjorncs/ssl handshake metric"

3 files changed, 54 insertions, 170 deletions

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 7ecf1b00fc1..31ecf3ca2fc 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,12 +5,10 @@ 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;
@@ -23,8 +21,6 @@ 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;
@@ -38,8 +34,6 @@ import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 
 import javax.net.ssl.SSLContext;
-import javax.net.ssl.SSLException;
-import javax.net.ssl.SSLHandshakeException;
 import javax.security.auth.x500.X500Principal;
 import java.io.IOException;
 import java.math.BigInteger;
@@ -60,8 +54,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
 import java.util.concurrent.TimeUnit;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 import java.util.regex.Pattern;
 
 import static com.yahoo.jdisc.Response.Status.GATEWAY_TIMEOUT;
@@ -89,21 +81,15 @@ 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;
 
 /**
  * @author Oyvind Bakksjo
  * @author Simon Thoresen Hult
- * @author bjorncs
  */
 public class HttpServerTest {
 
-    private static final Logger log = Logger.getLogger(HttpServerTest.class.getName());
-
     @Rule
     public TemporaryFolder tmpFolder = new TemporaryFolder();
 
@@ -492,7 +478,7 @@ public class HttpServerTest {
         Path certificateFile = tmpFolder.newFile().toPath();
         generatePrivateKeyAndCertificate(privateKeyFile, certificateFile);
 
-        final TestDriver driver = TestDrivers.newInstanceWithSsl(new EchoRequestHandler(), certificateFile, privateKeyFile, TlsClientAuth.WANT);
+        final TestDriver driver = TestDrivers.newInstanceWithSsl(new EchoRequestHandler(), certificateFile, privateKeyFile);
         driver.client().get("/status.html")
               .expectStatusCode(is(OK));
         assertThat(driver.close(), is(true));
@@ -503,7 +489,7 @@ public class HttpServerTest {
         Path privateKeyFile = tmpFolder.newFile().toPath();
         Path certificateFile = tmpFolder.newFile().toPath();
         generatePrivateKeyAndCertificate(privateKeyFile, certificateFile);
-        TestDriver driver = TestDrivers.newInstanceWithSsl(new EchoRequestHandler(), certificateFile, privateKeyFile, TlsClientAuth.WANT);
+        TestDriver driver = TestDrivers.newInstanceWithSsl(new EchoRequestHandler(), certificateFile, privateKeyFile);
 
         SSLContext trustStoreOnlyCtx = new SslContextBuilder()
                 .withTrustStore(certificateFile)
@@ -521,7 +507,7 @@ public class HttpServerTest {
         Path privateKeyFile = tmpFolder.newFile().toPath();
         Path certificateFile = tmpFolder.newFile().toPath();
         generatePrivateKeyAndCertificate(privateKeyFile, certificateFile);
-        TestDriver driver = TestDrivers.newInstanceWithSsl(new EchoRequestHandler(), certificateFile, privateKeyFile, TlsClientAuth.WANT);
+        TestDriver driver = TestDrivers.newInstanceWithSsl(new EchoRequestHandler(), certificateFile, privateKeyFile);
 
         SSLContext trustStoreOnlyCtx = new SslContextBuilder()
                 .withTrustStore(certificateFile)
@@ -573,114 +559,6 @@ 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));
-        } catch (SSLException e) {
-            // Jetty may sometime close the connection before the apache client has fully consumed the TLS handshake frame
-            log.log(Level.WARNING, "Client failed to get a proper TLS handshake response: " + e.getMessage(), e);
-            assertThat(e.getMessage(), containsString("readHandshakeRecord")); // Only ignore this specific ssl exception
-        }
-    }
-
     private static void generatePrivateKeyAndCertificate(Path privateKeyFile, Path certificateFile) throws IOException {
         KeyPair keyPair = KeyUtils.generateKeypair(RSA, 2048);
         Files.writeString(privateKeyFile, KeyUtils.toPem(keyPair.getPrivate()));
@@ -871,18 +749,4 @@ 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); }
-    }
-
 }
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/SimpleHttpClient.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/SimpleHttpClient.java
index 8035734a76c..b0f570317d6 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/SimpleHttpClient.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/SimpleHttpClient.java
@@ -1,11 +1,12 @@
 // Copyright 2017 Yahoo Holdings. 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.http.HttpHeaders;
 import org.apache.http.Header;
 import org.apache.http.HttpEntity;
 import org.apache.http.HttpResponse;
+import org.apache.http.client.HttpClient;
 import org.apache.http.client.entity.GzipCompressingEntity;
-import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.methods.HttpPost;
 import org.apache.http.client.methods.HttpUriRequest;
@@ -18,7 +19,6 @@ import org.apache.http.entity.ByteArrayEntity;
 import org.apache.http.entity.StringEntity;
 import org.apache.http.entity.mime.FormBodyPart;
 import org.apache.http.entity.mime.MultipartEntityBuilder;
-import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.impl.conn.BasicHttpClientConnectionManager;
 import org.apache.http.util.EntityUtils;
@@ -26,11 +26,16 @@ import org.hamcrest.Matcher;
 import org.hamcrest.MatcherAssert;
 
 import javax.net.ssl.SSLContext;
+import java.io.ByteArrayOutputStream;
+import java.io.EOFException;
 import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStreamWriter;
+import java.net.Socket;
 import java.net.URI;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
-import java.util.List;
+import java.util.regex.Pattern;
 
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.not;
@@ -41,20 +46,14 @@ import static org.hamcrest.MatcherAssert.assertThat;
  * A simple http client for testing
  *
  * @author Simon Thoresen Hult
- * @author bjorncs
  */
-public class SimpleHttpClient implements AutoCloseable {
+public class SimpleHttpClient {
 
-    private final CloseableHttpClient delegate;
+    private final HttpClient delegate;
     private final String scheme;
     private final int listenPort;
 
-    public SimpleHttpClient(SSLContext sslContext, int listenPort, boolean useCompression) {
-        this(sslContext, null, null, listenPort, useCompression);
-    }
-
-    public SimpleHttpClient(SSLContext sslContext, List<String> enabledProtocols, List<String> enabledCiphers,
-                            int listenPort, boolean useCompression) {
+    public SimpleHttpClient(final SSLContext sslContext, final int listenPort, final boolean useCompression) {
         HttpClientBuilder builder = HttpClientBuilder.create();
         if (!useCompression) {
             builder.disableContentCompression();
@@ -62,8 +61,6 @@ public class SimpleHttpClient implements AutoCloseable {
         if (sslContext != null) {
             SSLConnectionSocketFactory sslConnectionFactory = new SSLConnectionSocketFactory(
                     sslContext,
-                    toArray(enabledProtocols),
-                    toArray(enabledCiphers),
                     new DefaultHostnameVerifier());
             builder.setSSLSocketFactory(sslConnectionFactory);
 
@@ -79,10 +76,6 @@ public class SimpleHttpClient implements AutoCloseable {
         this.listenPort = listenPort;
     }
 
-    private static String[] toArray(List<String> list) {
-        return list != null ? list.toArray(new String[0]) : null;
-    }
-
     public URI newUri(final String path) {
         return URI.create(scheme + "://localhost:" + listenPort + path);
     }
@@ -107,9 +100,40 @@ public class SimpleHttpClient implements AutoCloseable {
         return newGet(path).execute();
     }
 
-    @Override
-    public void close() throws IOException {
-        delegate.close();
+    public String raw(final String request) throws IOException {
+        final Socket socket = new Socket("localhost", listenPort);
+        final OutputStreamWriter out = new OutputStreamWriter(socket.getOutputStream(), StandardCharsets.UTF_8);
+        out.write(request);
+        out.flush();
+
+        final ByteArrayOutputStream buf = new ByteArrayOutputStream();
+        final InputStream in = socket.getInputStream();
+        final int[] TERMINATOR = { '\r', '\n', '\r', '\n' };
+        for (int pos = 0; pos < TERMINATOR.length; ++pos) {
+            final int b = in.read();
+            if (b < 0) {
+                throw new EOFException();
+            }
+            if (b != TERMINATOR[pos]) {
+                pos = -1;
+            }
+            buf.write(b);
+        }
+        final String response = buf.toString(StandardCharsets.UTF_8.name());
+        final java.util.regex.Matcher matcher = Pattern.compile(HttpHeaders.Names.CONTENT_LENGTH + ": (.+)\r\n").matcher(response);
+        if (matcher.find()) {
+            final int len = Integer.valueOf(matcher.group(1));
+            for (int i = 0; i < len; ++i) {
+                final int b = in.read();
+                if (b < 0) {
+                    throw new EOFException();
+                }
+                buf.write(b);
+            }
+        }
+
+        socket.close();
+        return buf.toString(StandardCharsets.UTF_8.name());
     }
 
     public class RequestExecutor {
@@ -153,9 +177,7 @@ public class SimpleHttpClient implements AutoCloseable {
             if (entity != null) {
                 ((HttpPost)request).setEntity(entity);
             }
-            try (CloseableHttpResponse response = delegate.execute(request)){
-                return new ResponseValidator(response);
-            }
+            return new ResponseValidator(delegate.execute(request));
         }
     }
 
@@ -196,5 +218,9 @@ public class SimpleHttpClient implements AutoCloseable {
             return this;
         }
 
+        public ResponseValidator expectTrailer(final String trailerName, final Matcher<String> matcher) {
+            // TODO: check trailer, not header
+            return expectHeader(trailerName, matcher);
+        }
     }
 }
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDrivers.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDrivers.java
index 4908da2ba75..e0933ac485e 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDrivers.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDrivers.java
@@ -20,7 +20,6 @@ import java.nio.file.Path;
 
 /**
  * @author Simon Thoresen Hult
- * @author bjorncs
  */
 public class TestDrivers {
 
@@ -46,12 +45,9 @@ public class TestDrivers {
                 ));
     }
 
-    public enum TlsClientAuth { NEED, WANT }
-
     public static TestDriver newInstanceWithSsl(final RequestHandler requestHandler,
                                                 Path certificateFile,
                                                 Path privateKeyFile,
-                                                TlsClientAuth tlsClientAuth,
                                                 final Module... guiceModules) throws IOException {
         return TestDriver.newInstance(
                 JettyHttpServer.class,
@@ -65,9 +61,7 @@ public class TestDrivers {
                                                 .pathWhitelist("/status.html"))
                                 .ssl(new ConnectorConfig.Ssl.Builder()
                                              .enabled(true)
-                                             .clientAuth(tlsClientAuth == TlsClientAuth.NEED
-                                                                 ? ConnectorConfig.Ssl.ClientAuth.Enum.NEED_AUTH
-                                                                 : ConnectorConfig.Ssl.ClientAuth.Enum.WANT_AUTH)
+                                             .clientAuth(ConnectorConfig.Ssl.ClientAuth.Enum.WANT_AUTH)
                                              .privateKeyFile(privateKeyFile.toString())
                                              .certificateFile(certificateFile.toString())
                                              .caCertificateFile(certificateFile.toString())),