Merge pull request #4090 from vespa-engine/bjorncs/custom-ssl-context

Bjorncs/custom ssl context

6 files changed, 157 insertions, 136 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 54338c64c1e..7ec51f35b74 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
@@ -1,7 +1,6 @@
 // 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.google.common.base.Preconditions;
 import com.google.inject.Inject;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.jdisc.http.ConnectorConfig;
@@ -10,31 +9,20 @@ import com.yahoo.jdisc.http.ConnectorConfig.Ssl.PemKeyStore;
 import com.yahoo.jdisc.http.SecretStore;
 import com.yahoo.jdisc.http.ssl.pem.PemSslKeyStore;
 import org.eclipse.jetty.http.HttpVersion;
-import org.eclipse.jetty.server.ConnectionFactory;
 import org.eclipse.jetty.server.HttpConfiguration;
 import org.eclipse.jetty.server.HttpConnectionFactory;
 import org.eclipse.jetty.server.SecureRequestCustomizer;
 import org.eclipse.jetty.server.Server;
-import org.eclipse.jetty.server.ServerConnectionStatistics;
 import org.eclipse.jetty.server.ServerConnector;
 import org.eclipse.jetty.server.SslConnectionFactory;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 
-import javax.servlet.ServletRequest;
 import java.io.IOException;
 import java.io.UncheckedIOException;
-import java.lang.reflect.Field;
-import java.net.Socket;
-import java.net.SocketException;
 import java.nio.channels.ServerSocketChannel;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.security.KeyStore;
-import java.util.Map;
-import java.util.Optional;
-import java.util.TreeMap;
-import java.util.function.Supplier;
-import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import static com.yahoo.jdisc.http.ConnectorConfig.Ssl.KeyStoreType.Enum.JKS;
@@ -42,6 +30,7 @@ import static com.yahoo.jdisc.http.ConnectorConfig.Ssl.KeyStoreType.Enum.PEM;
 
 /**
  * @author Einar M R Rosenvinge
+ * @author bjorncs
  */
 public class ConnectorFactory {
 
@@ -63,12 +52,30 @@ public class ConnectorFactory {
         ConnectorConfig.Ssl ssl = config.ssl();
 
         if (ssl.keyStoreType() == JKS) {
-            if (! ssl.pemKeyStore().keyPath().isEmpty() || ! ssl.pemKeyStore().certificatePath().isEmpty())
+            if (!ssl.pemKeyStore().keyPath().isEmpty() || ! ssl.pemKeyStore().certificatePath().isEmpty()) {
                 throw new IllegalArgumentException("pemKeyStore attributes can not be set when keyStoreType is JKS.");
+            }
+            if (ssl.keyDbKey().isEmpty()) {
+                throw new IllegalArgumentException("Missing password for JKS keystore");
+            }
         }
         if (ssl.keyStoreType() == PEM) {
-            if (! ssl.keyStorePath().isEmpty())
+            if (! ssl.keyStorePath().isEmpty()) {
                 throw new IllegalArgumentException("keyStorePath can not be set when keyStoreType is PEM");
+            }
+            if (!ssl.keyDbKey().isEmpty()) {
+                // TODO Make an error once there are separate passwords for truststore and keystore
+                log.warning("Encrypted PEM key stores are not supported. Password is only applied to truststore");
+            }
+            if (ssl.pemKeyStore().certificatePath().isEmpty()) {
+                throw new IllegalArgumentException("Missing certificate path.");
+            }
+            if (ssl.pemKeyStore().keyPath().isEmpty()) {
+                throw new IllegalArgumentException("Missing key path.");
+            }
+        }
+        if (!ssl.trustStorePath().isEmpty() && ssl.useTrustStorePassword() && ssl.keyDbKey().isEmpty()) {
+            throw new IllegalArgumentException("Missing password for JKS truststore");
         }
     }
 
@@ -164,25 +171,24 @@ public class ConnectorFactory {
             factory.setIncludeCipherSuites(ciphs);
         }
 
-        Optional<String> keyDbPassword = secret(sslConfig.keyDbKey());
+        String keyDbPassword = sslConfig.keyDbKey();
         switch (sslConfig.keyStoreType()) {
             case PEM:
-                factory.setKeyStore(getKeyStore(sslConfig.pemKeyStore()));
-                if (keyDbPassword.isPresent())
-                    log.warning("Encrypted PEM key stores are not supported.");
+                factory.setKeyStore(createPemKeyStore(sslConfig.pemKeyStore()));
                 break;
             case JKS:
                 factory.setKeyStorePath(sslConfig.keyStorePath());
                 factory.setKeyStoreType(sslConfig.keyStoreType().toString());
-                factory.setKeyStorePassword(keyDbPassword.orElseThrow(passwordRequiredForJKSKeyStore("key")));
+                factory.setKeyStorePassword(secretStore.getSecret(keyDbPassword));
                 break;
         }
 
         if (!sslConfig.trustStorePath().isEmpty()) {
             factory.setTrustStorePath(sslConfig.trustStorePath());
             factory.setTrustStoreType(sslConfig.trustStoreType().toString());      
-            if (sslConfig.useTrustStorePassword())
-                factory.setTrustStorePassword(keyDbPassword.orElseThrow(passwordRequiredForJKSKeyStore("trust")));
+            if (sslConfig.useTrustStorePassword()) {
+                factory.setTrustStorePassword(secretStore.getSecret(keyDbPassword));
+            }
         }
 
         factory.setKeyManagerFactoryAlgorithm(sslConfig.sslKeyManagerFactoryAlgorithm());
@@ -190,19 +196,7 @@ public class ConnectorFactory {
         return new SslConnectionFactory(factory, HttpVersion.HTTP_1_1.asString());
     }
 
-    /** Returns the secret password with the given name, or empty if the password name is null or empty */
-    private Optional<String> secret(String keyname) {
-        return Optional.of(keyname).filter(key -> !key.isEmpty()).map(secretStore::getSecret);
-    }
-    
-    @SuppressWarnings("ThrowableInstanceNeverThrown")
-    private Supplier<RuntimeException> passwordRequiredForJKSKeyStore(String type) {
-        return () -> new RuntimeException(String.format("Password is required for JKS %s store", type));
-    }
-
-    private static KeyStore getKeyStore(PemKeyStore pemKeyStore) {
-        Preconditions.checkArgument(!pemKeyStore.certificatePath().isEmpty(), "Missing certificate path.");
-        Preconditions.checkArgument(!pemKeyStore.keyPath().isEmpty(), "Missing key path.");
+    private static KeyStore createPemKeyStore(PemKeyStore pemKeyStore) {
         try {
             Path certificatePath = Paths.get(pemKeyStore.certificatePath());
             Path keyPath = Paths.get(pemKeyStore.keyPath());
@@ -215,99 +209,4 @@ public class ConnectorFactory {
         }
     }
 
-    public static class JDiscServerConnector extends ServerConnector {
-        public static final String REQUEST_ATTRIBUTE = JDiscServerConnector.class.getName();
-        private final static Logger log = Logger.getLogger(JDiscServerConnector.class.getName());
-        private final Metric.Context metricCtx;
-        private final ServerConnectionStatistics statistics;
-        private final boolean tcpKeepAlive;
-        private final boolean tcpNoDelay;
-        private final ServerSocketChannel channelOpenedByActivator;
-
-        private JDiscServerConnector(ConnectorConfig config, Metric metric, Server server, 
-                                     ServerSocketChannel channelOpenedByActivator, ConnectionFactory... factories) {
-            super(server, factories);
-            this.channelOpenedByActivator = channelOpenedByActivator;
-            this.tcpKeepAlive = config.tcpKeepAliveEnabled();
-            this.tcpNoDelay = config.tcpNoDelay();
-            this.metricCtx = createMetricContext(config, metric);
-
-            this.statistics = new ServerConnectionStatistics();
-            addBean(statistics);
-        }
-
-        private Metric.Context createMetricContext(ConnectorConfig config, Metric metric) {
-            Map<String, Object> props = new TreeMap<>();
-            props.put(JettyHttpServer.Metrics.NAME_DIMENSION, config.name());
-            props.put(JettyHttpServer.Metrics.PORT_DIMENSION, config.listenPort());
-            return metric.createContext(props);
-        }
-
-        @Override
-        protected void configure(final Socket socket) {
-            super.configure(socket);
-            try {
-                socket.setKeepAlive(tcpKeepAlive);
-                socket.setTcpNoDelay(tcpNoDelay);
-            } catch (SocketException ignored) {
-            }
-        }
-
-        @Override
-        public void open() throws IOException {
-            if (channelOpenedByActivator == null) {
-                log.log(Level.INFO, "No channel set by activator, opening channel ourselves.");
-                try {
-                    super.open();
-                } catch (RuntimeException e) {
-                    log.log(Level.SEVERE, "failed org.eclipse.jetty.server.Server open() with port "+getPort());
-                    throw e;
-                }
-                return;
-            }
-            log.log(Level.INFO, "Using channel set by activator: " + channelOpenedByActivator);
-
-            channelOpenedByActivator.socket().setReuseAddress(getReuseAddress());
-            int localPort = channelOpenedByActivator.socket().getLocalPort();
-            try {
-                uglySetLocalPort(localPort);
-            } catch (NoSuchFieldException | IllegalAccessException e) {
-                throw new RuntimeException("Could not set local port.", e);
-            }
-            if (localPort <= 0) {
-                throw new IOException("Server channel not bound");
-            }
-            addBean(channelOpenedByActivator);
-            channelOpenedByActivator.configureBlocking(true);
-            addBean(channelOpenedByActivator);
-
-            try {
-                uglySetChannel(channelOpenedByActivator);
-            } catch (NoSuchFieldException | IllegalAccessException e) {
-                throw new RuntimeException("Could not set server channel.", e);
-            }
-        }
-
-        private void uglySetLocalPort(int localPort) throws NoSuchFieldException, IllegalAccessException {
-            Field localPortField = ServerConnector.class.getDeclaredField("_localPort");
-            localPortField.setAccessible(true);
-            localPortField.set(this, localPort);
-        }
-
-        private void uglySetChannel(ServerSocketChannel channelOpenedByActivator) throws NoSuchFieldException, 
-                                                                                         IllegalAccessException {
-            Field acceptChannelField = ServerConnector.class.getDeclaredField("_acceptChannel");
-            acceptChannelField.setAccessible(true);
-            acceptChannelField.set(this, channelOpenedByActivator);
-        }
-
-        public ServerConnectionStatistics getStatistics() { return statistics; }
-
-        public Metric.Context getMetricContext() { return metricCtx; }
-
-        public static JDiscServerConnector fromRequest(ServletRequest request) {
-            return (JDiscServerConnector)request.getAttribute(REQUEST_ATTRIBUTE);
-        }
-    }
-
 }
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java
index 543cf8ab43e..27f72c7b4bf 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java
@@ -20,7 +20,6 @@ import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import static com.yahoo.jdisc.http.core.HttpServletRequestUtils.getConnection;
-import static com.yahoo.jdisc.http.server.jetty.ConnectorFactory.JDiscServerConnector;
 
 /**
  * @author Simon Thoresen Hult
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscServerConnector.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscServerConnector.java
new file mode 100644
index 00000000000..8dd50074c32
--- /dev/null
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscServerConnector.java
@@ -0,0 +1,122 @@
+// 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.Metric;
+import com.yahoo.jdisc.http.ConnectorConfig;
+import org.eclipse.jetty.server.ConnectionFactory;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.server.ServerConnectionStatistics;
+import org.eclipse.jetty.server.ServerConnector;
+
+import javax.servlet.ServletRequest;
+import java.io.IOException;
+import java.lang.reflect.Field;
+import java.net.Socket;
+import java.net.SocketException;
+import java.nio.channels.ServerSocketChannel;
+import java.util.Map;
+import java.util.TreeMap;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+/**
+ * @author bjorncs
+ */
+class JDiscServerConnector extends ServerConnector {
+    public static final String REQUEST_ATTRIBUTE = JDiscServerConnector.class.getName();
+    private final static Logger log = Logger.getLogger(JDiscServerConnector.class.getName());
+    private final Metric.Context metricCtx;
+    private final ServerConnectionStatistics statistics;
+    private final boolean tcpKeepAlive;
+    private final boolean tcpNoDelay;
+    private final ServerSocketChannel channelOpenedByActivator;
+
+    JDiscServerConnector(ConnectorConfig config, Metric metric, Server server,
+                         ServerSocketChannel channelOpenedByActivator, ConnectionFactory... factories) {
+        super(server, factories);
+        this.channelOpenedByActivator = channelOpenedByActivator;
+        this.tcpKeepAlive = config.tcpKeepAliveEnabled();
+        this.tcpNoDelay = config.tcpNoDelay();
+        this.metricCtx = createMetricContext(config, metric);
+
+        this.statistics = new ServerConnectionStatistics();
+        addBean(statistics);
+    }
+
+    private Metric.Context createMetricContext(ConnectorConfig config, Metric metric) {
+        Map<String, Object> props = new TreeMap<>();
+        props.put(JettyHttpServer.Metrics.NAME_DIMENSION, config.name());
+        props.put(JettyHttpServer.Metrics.PORT_DIMENSION, config.listenPort());
+        return metric.createContext(props);
+    }
+
+    @Override
+    protected void configure(final Socket socket) {
+        super.configure(socket);
+        try {
+            socket.setKeepAlive(tcpKeepAlive);
+            socket.setTcpNoDelay(tcpNoDelay);
+        } catch (SocketException ignored) {
+        }
+    }
+
+    @Override
+    public void open() throws IOException {
+        if (channelOpenedByActivator == null) {
+            log.log(Level.INFO, "No channel set by activator, opening channel ourselves.");
+            try {
+                super.open();
+            } catch (RuntimeException e) {
+                log.log(Level.SEVERE, "failed org.eclipse.jetty.server.Server open() with port " + getPort());
+                throw e;
+            }
+            return;
+        }
+        log.log(Level.INFO, "Using channel set by activator: " + channelOpenedByActivator);
+
+        channelOpenedByActivator.socket().setReuseAddress(getReuseAddress());
+        int localPort = channelOpenedByActivator.socket().getLocalPort();
+        try {
+            uglySetLocalPort(localPort);
+        } catch (NoSuchFieldException | IllegalAccessException e) {
+            throw new RuntimeException("Could not set local port.", e);
+        }
+        if (localPort <= 0) {
+            throw new IOException("Server channel not bound");
+        }
+        addBean(channelOpenedByActivator);
+        channelOpenedByActivator.configureBlocking(true);
+        addBean(channelOpenedByActivator);
+
+        try {
+            uglySetChannel(channelOpenedByActivator);
+        } catch (NoSuchFieldException | IllegalAccessException e) {
+            throw new RuntimeException("Could not set server channel.", e);
+        }
+    }
+
+    private void uglySetLocalPort(int localPort) throws NoSuchFieldException, IllegalAccessException {
+        Field localPortField = ServerConnector.class.getDeclaredField("_localPort");
+        localPortField.setAccessible(true);
+        localPortField.set(this, localPort);
+    }
+
+    private void uglySetChannel(ServerSocketChannel channelOpenedByActivator) throws NoSuchFieldException,
+            IllegalAccessException {
+        Field acceptChannelField = ServerConnector.class.getDeclaredField("_acceptChannel");
+        acceptChannelField.setAccessible(true);
+        acceptChannelField.set(this, channelOpenedByActivator);
+    }
+
+    public ServerConnectionStatistics getStatistics() {
+        return statistics;
+    }
+
+    public Metric.Context getMetricContext() {
+        return metricCtx;
+    }
+
+    public static JDiscServerConnector fromRequest(ServletRequest request) {
+        return (JDiscServerConnector) request.getAttribute(REQUEST_ATTRIBUTE);
+    }
+}
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 aaa213095c6..7bff685e780 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
@@ -58,8 +58,6 @@ import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
-import static com.yahoo.jdisc.http.server.jetty.ConnectorFactory.JDiscServerConnector;
-
 /**
  * @author Simon Thoresen Hult
  * @author bjorncs
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 7a13ec2485f..59893753bea 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
@@ -45,8 +45,9 @@ ssl.enabled                         bool     default=false
 
 # The name of the key to the password to the key store if in the secret store, if JKS is used.
 # Must be empty with PEM
-# By default this is also used to look up the password to the trust store. 
+# By default this is also used to look up the password to the trust store.
 ssl.keyDbKey                        string   default=""
+# TODO Rename keyDbKey to keyStorePassword after introducing custom services.xml syntax
 
 # Names of protocols to exclude.
 ssl.excludeProtocol[].name          string
@@ -74,6 +75,8 @@ ssl.trustStoreType                  enum { JKS }  default=JKS
 # JKS only - the path to the truststore.
 ssl.trustStorePath                  string   default=""
 
+# TODO Add separate config for truststore password
+
 # Whether we should use keyDbKey as password to the trust store (true, default),
 # or use no password with the trust store (false)
 ssl.useTrustStorePassword           bool   default=true
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactoryTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactoryTest.java
index 1380abc03f3..fceec51231a 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactoryTest.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactoryTest.java
@@ -59,8 +59,8 @@ public class ConnectorFactoryTest {
         try {
             ConnectorFactory factory = new ConnectorFactory(new ConnectorConfig(new ConnectorConfig.Builder()),
                                                             new ThrowingSecretStore());
-            ConnectorFactory.JDiscServerConnector connector =
-                    (ConnectorFactory.JDiscServerConnector)factory.createConnector(new DummyMetric(), server, null);
+            JDiscServerConnector connector =
+                    (JDiscServerConnector)factory.createConnector(new DummyMetric(), server, null);
             server.addConnector(connector);
             server.setHandler(new HelloWorldHandler());
             server.start();
@@ -86,7 +86,7 @@ public class ConnectorFactoryTest {
             serverChannel.socket().bind(new InetSocketAddress(0));
 
             ConnectorFactory factory = new ConnectorFactory(new ConnectorConfig(new ConnectorConfig.Builder()), new ThrowingSecretStore());
-            ConnectorFactory.JDiscServerConnector connector = (ConnectorFactory.JDiscServerConnector) factory.createConnector(new DummyMetric(), server, serverChannel);
+            JDiscServerConnector connector = (JDiscServerConnector) factory.createConnector(new DummyMetric(), server, serverChannel);
             server.addConnector(connector);
             server.setHandler(new HelloWorldHandler());
             server.start();