SSL/TLS: Properly test for client auth and allow to require

* Fixed issue, where client auth was never needed for HTTP.
* Changed parameter name to `require.client.auth`
* Added tests, removed useless code in SslIntegrationTests

Original commit: elastic/x-pack-elasticsearch@ea424e0ae4

src/main/java/org/elasticsearch/shield/ssl/SSLConfig.java

@@ -30,19 +30,15 @@ public class SSLConfig {
     private SSLContext sslContext;
     private String[] ciphers;
 
-    public SSLConfig(Settings componentSettings) {
+    public SSLConfig(Settings componentSettings, Settings defaultSettings) {
-        this(componentSettings, ImmutableSettings.EMPTY);
+        this.clientAuth = componentSettings.getAsBoolean("require.client.auth", defaultSettings.getAsBoolean("require.client.auth", true));
-    }
+        String keyStore = componentSettings.get("keystore", defaultSettings.get("keystore", System.getProperty("javax.net.ssl.keyStore")));
-
+        String keyStorePassword = componentSettings.get("keystore_password", defaultSettings.get("keystore_password", System.getProperty("javax.net.ssl.keyStorePassword")));
-    public SSLConfig(Settings componentSettings, Settings settings) {
+        String keyStoreAlgorithm = componentSettings.get("keystore_algorithm", defaultSettings.get("keystore_algorithm", System.getProperty("ssl.KeyManagerFactory.algorithm")));
-        this.clientAuth = componentSettings.getAsBoolean("client.auth", settings.getAsBoolean("shield.ssl.client.auth", true));
+        String trustStore = componentSettings.get("truststore", defaultSettings.get("truststore", System.getProperty("javax.net.ssl.trustStore")));
-        String keyStore = componentSettings.get("keystore", settings.get("shield.ssl.keystore", System.getProperty("javax.net.ssl.keyStore")));
+        String trustStorePassword = componentSettings.get("truststore_password", defaultSettings.get("truststore_password", System.getProperty("javax.net.ssl.trustStorePassword")));
-        String keyStorePassword = componentSettings.get("keystore_password", settings.get("shield.ssl.keystore_password", System.getProperty("javax.net.ssl.keyStorePassword")));
+        String trustStoreAlgorithm = componentSettings.get("truststore_algorithm", defaultSettings.get("truststore_algorithm", System.getProperty("ssl.TrustManagerFactory.algorithm")));
-        String keyStoreAlgorithm = componentSettings.get("keystore_algorithm", settings.get("shield.ssl.keystore_algorithm", System.getProperty("ssl.KeyManagerFactory.algorithm")));
+        this.ciphers = componentSettings.getAsArray("ciphers", defaultSettings.getAsArray("ciphers", DEFAULT_CIPHERS));
-        String trustStore = componentSettings.get("truststore", settings.get("shield.ssl.truststore", System.getProperty("javax.net.ssl.trustStore")));
-        String trustStorePassword = componentSettings.get("truststore_password", settings.get("shield.ssl.truststore_password", System.getProperty("javax.net.ssl.trustStorePassword")));
-        String trustStoreAlgorithm = componentSettings.get("truststore_algorithm", settings.get("shield.ssl.truststore_algorithm", System.getProperty("ssl.TrustManagerFactory.algorithm")));
-        this.ciphers = componentSettings.getAsArray("ciphers", settings.getAsArray("shield.ssl.ciphers", DEFAULT_CIPHERS));
 
         if (keyStore == null) {
             throw new ElasticsearchException("SSL Enabled, but keystore unconfigured");
@@ -101,7 +97,7 @@ public class SSLConfig {
 
         // Initialize sslContext
         try {
-            String algorithm = componentSettings.get("context_algorithm", settings.get("shield.ssl.context_algorithm", "TLS"));
+            String algorithm = componentSettings.get("context_algorithm", defaultSettings.get("shield.ssl.context_algorithm", "TLS"));
             sslContext = SSLContext.getInstance(algorithm);
             sslContext.init(kmf.getKeyManagers(), trustManagers, null);
         } catch (Exception e) {

src/main/java/org/elasticsearch/shield/ssl/netty/NettySSLHttpServerTransport.java

@@ -15,6 +15,7 @@ import org.elasticsearch.common.util.BigArrays;
 import org.elasticsearch.http.netty.NettyHttpServerTransport;
 import org.elasticsearch.shield.n2n.N2NNettyUpstreamHandler;
 import org.elasticsearch.shield.ssl.SSLConfig;
+import org.elasticsearch.transport.netty.NettyTransport;
 
 import javax.net.ssl.SSLEngine;
 
@@ -46,7 +47,7 @@ public class NettySSLHttpServerTransport extends NettyHttpServerTransport {
         public HttpSslChannelPipelineFactory(NettyHttpServerTransport transport) {
             super(transport);
             if (ssl) {
-                sslConfig = new SSLConfig(settings.getByPrefix("shield.http.ssl."));
+                sslConfig = new SSLConfig(settings.getByPrefix("shield.http.ssl."), settings.getByPrefix("shield.ssl."));
                 // try to create an SSL engine, so that exceptions lead to early exit
                 sslConfig.createSSLEngine();
             } else {
@@ -61,8 +62,6 @@ public class NettySSLHttpServerTransport extends NettyHttpServerTransport {
             if (ssl) {
                 SSLEngine engine = sslConfig.createSSLEngine();
                 engine.setUseClientMode(false);
-                // TODO MAKE ME CONFIGURABLE
-                engine.setNeedClientAuth(false);
                 pipeline.addFirst("ssl", new SslHandler(engine));
             }
             return pipeline;

src/main/java/org/elasticsearch/shield/ssl/netty/NettySSLTransport.java

@@ -53,7 +53,7 @@ public class NettySSLTransport extends NettyTransport {
         public SslServerChannelPipelineFactory(NettyTransport nettyTransport) {
             super(nettyTransport);
             if (ssl) {
-                sslConfig = new SSLConfig(settings.getByPrefix("shield.transport.ssl."));
+                sslConfig = new SSLConfig(settings.getByPrefix("shield.transport.ssl."), settings.getByPrefix("shield.ssl."));
                 // try to create an SSL engine, so that exceptions lead to early exit
                 sslConfig.createSSLEngine();
             } else {
@@ -83,7 +83,7 @@ public class NettySSLTransport extends NettyTransport {
         public SslClientChannelPipelineFactory(NettyTransport transport) {
             super(transport);
             if (ssl) {
-                sslConfig = new SSLConfig(settings.getByPrefix("shield.transport.ssl."));
+                sslConfig = new SSLConfig(settings.getByPrefix("shield.transport.ssl."), settings.getByPrefix("shield.ssl."));
                 // try to create an SSL engine, so that exceptions lead to early exit
                 sslConfig.createSSLEngine();
             } else {

src/test/java/org/elasticsearch/shield/ssl/SSLConfigTests.java

@@ -33,11 +33,11 @@ public class SSLConfigTests extends ElasticsearchTestCase {
         try {
             new SSLConfig(ImmutableSettings.EMPTY,
                     settingsBuilder()
-                            .put("shield.ssl.context_algorithm", "non-existing")
+                            .put("context_algorithm", "non-existing")
-                            .put("shield.ssl.keystore", testnodeStore.getPath())
+                            .put("keystore", testnodeStore.getPath())
-                            .put("shield.ssl.keystore_password", "testnode")
+                            .put("keystore_password", "testnode")
-                            .put("shield.ssl.truststore", testnodeStore.getPath())
+                            .put("truststore", testnodeStore.getPath())
-                            .put("shield.ssl.truststore_password", "testnode")
+                            .put("truststore_password", "testnode")
                         .build());
         } catch (ElasticsearchSSLException e) {
             assertThat(e.getRootCause(), instanceOf(NoSuchAlgorithmException.class));
@@ -46,6 +46,7 @@ public class SSLConfigTests extends ElasticsearchTestCase {
 
     @Test
     public void testThatExactConfigOverwritesDefaultConfig() throws Exception {
+        //
         Settings concreteSettings = settingsBuilder()
                 .put("ciphers", "TLS_RSA_WITH_AES_128_CBC_SHA")
                 .build();
@@ -58,7 +59,7 @@ public class SSLConfigTests extends ElasticsearchTestCase {
                 .put("shield.ssl.truststore_password", "testnode")
                 .build();
 
-        SSLConfig sslConfig = new SSLConfig(concreteSettings, genericSettings);
+        SSLConfig sslConfig = new SSLConfig(concreteSettings, genericSettings.getByPrefix("shield.ssl."));
         SSLEngine sslEngine = sslConfig.createSSLEngine();
         assertThat(sslEngine.getEnabledCipherSuites().length, is(1));
     }

src/test/java/org/elasticsearch/shield/ssl/SslIntegrationTests.java

@@ -29,6 +29,7 @@ import org.elasticsearch.test.junit.annotations.TestLogging;
 import org.elasticsearch.transport.Transport;
 import org.elasticsearch.transport.TransportModule;
 import org.junit.*;
+import org.junit.rules.TemporaryFolder;
 
 import javax.net.ssl.*;
 import java.io.File;
@@ -46,17 +47,15 @@ import static org.hamcrest.Matchers.*;
 @ClusterScope(scope = Scope.SUITE, numDataNodes = 1, transportClientRatio = 0.0, numClientNodes = 0)
 public class SslIntegrationTests extends ElasticsearchIntegrationTest {
 
-    private static File ipFilterFile = null;
+    @ClassRule
+    public static TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+    private static File ipFilterFile;
 
     @BeforeClass
-    public static void writeAllowAllIpFilterFile() {
+    public static void writeAllowAllIpFilterFile() throws Exception {
-        try {
+        ipFilterFile = temporaryFolder.newFile();
-            ipFilterFile = File.createTempFile("elasticsearch", "ipfilter");
-            ipFilterFile.deleteOnExit();
         Files.write("allow: all\n".getBytes(com.google.common.base.Charsets.UTF_8), ipFilterFile);
-        } catch (IOException e) {
-            throw new ElasticsearchException("error creating temp file", e);
-        }
     }
 
     @Override
@@ -72,6 +71,8 @@ public class SslIntegrationTests extends ElasticsearchIntegrationTest {
         return ImmutableSettings.settingsBuilder()
                 .put(super.nodeSettings(nodeOrdinal))
                 .put("discovery.zen.ping.multicast.ping.enabled", false)
+                //
+                .put("shield.authz.file.roles", "not/existing")
                 // needed to ensure that netty transport is started
                 .put("node.mode", "network")
                 .put("shield.transport.ssl", true)
@@ -80,6 +81,7 @@ public class SslIntegrationTests extends ElasticsearchIntegrationTest {
                 .put("shield.transport.ssl.truststore", testnodeStore.getPath())
                 .put("shield.transport.ssl.truststore_password", "testnode")
                 .put("shield.http.ssl", true)
+                .put("shield.http.ssl.require.client.auth", false)
                 .put("shield.http.ssl.keystore", testnodeStore.getPath())
                 .put("shield.http.ssl.keystore_password", "testnode")
                 .put("shield.http.ssl.truststore", testnodeStore.getPath())
@@ -92,16 +94,6 @@ public class SslIntegrationTests extends ElasticsearchIntegrationTest {
                 .build();
     }
 
-    @Before
-    public void setup() {
-        System.setProperty("javax.net.debug", "all");
-    }
-
-    @After
-    public void teardown() {
-        System.clearProperty("javax.net.debug");
-    }
-
     @Test
     @TestLogging("_root:INFO,org.elasticsearch.test:TRACE, org.elasticsearch.client.transport:DEBUG,org.elasticsearch.shield:TRACE")
     public void testThatTransportClientCanConnectToNodeViaSsl() throws Exception {

src/test/java/org/elasticsearch/shield/ssl/SslRequireAuthTests.java

@@ -0,0 +1,187 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+package org.elasticsearch.shield.ssl;
+
+import com.carrotsearch.ant.tasks.junit4.dependencies.com.google.common.base.Charsets;
+import com.google.common.io.Files;
+import com.google.common.net.InetAddresses;
+import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.common.io.Streams;
+import org.elasticsearch.common.settings.ImmutableSettings;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.transport.InetSocketTransportAddress;
+import org.elasticsearch.common.transport.TransportAddress;
+import org.elasticsearch.http.HttpServerTransport;
+import org.elasticsearch.shield.plugin.SecurityPlugin;
+import org.elasticsearch.shield.ssl.netty.NettySSLHttpServerTransportModule;
+import org.elasticsearch.shield.ssl.netty.NettySSLTransportModule;
+import org.elasticsearch.test.ElasticsearchIntegrationTest;
+import org.elasticsearch.test.junit.annotations.TestLogging;
+import org.elasticsearch.transport.TransportModule;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import javax.net.ssl.*;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.net.HttpURLConnection;
+import java.net.URL;
+import java.security.KeyStore;
+import java.security.SecureRandom;
+import java.util.Locale;
+
+import static org.hamcrest.Matchers.*;
+
+/**
+ *
+ */
+@ElasticsearchIntegrationTest.ClusterScope(scope = ElasticsearchIntegrationTest.Scope.SUITE, numDataNodes = 1, transportClientRatio = 0.0, numClientNodes = 0)
+public class SslRequireAuthTests extends ElasticsearchIntegrationTest {
+
+    public static final HostnameVerifier HOSTNAME_VERIFIER = new HostnameVerifier() {
+        @Override
+        public boolean verify(String s, SSLSession sslSession) {
+            return true;
+        }
+    };
+
+    @ClassRule
+    public static TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+    private static File ipFilterFile;
+
+    @BeforeClass
+    public static void writeAllowAllIpFilterFile() throws Exception {
+        ipFilterFile = temporaryFolder.newFile();
+        Files.write("allow: all\n".getBytes(com.google.common.base.Charsets.UTF_8), ipFilterFile);
+    }
+
+    @Override
+    protected Settings nodeSettings(int nodeOrdinal) {
+        File testnodeStore;
+        try {
+            testnodeStore = new File(getClass().getResource("/certs/simple/testnode.jks").toURI());
+            assertThat(testnodeStore.exists(), is(true));
+        } catch (Exception e) {
+            throw new RuntimeException(e);
+        }
+
+        return ImmutableSettings.settingsBuilder()
+                .put(super.nodeSettings(nodeOrdinal))
+                .put("discovery.zen.ping.multicast.ping.enabled", false)
+                // prevents exception until parsing has been fixed in PR
+                .put("shield.authz.file.roles", "not/existing")
+                // needed to ensure that netty transport is started
+                .put("node.mode", "network")
+                .put("shield.transport.ssl", true)
+                .put("shield.transport.ssl.require.client.auth", true)
+                .put("shield.transport.ssl.keystore", testnodeStore.getPath())
+                .put("shield.transport.ssl.keystore_password", "testnode")
+                .put("shield.transport.ssl.truststore", testnodeStore.getPath())
+                .put("shield.transport.ssl.truststore_password", "testnode")
+                .put("shield.http.ssl", true)
+                .put("shield.http.ssl.require.client.auth", true)
+                .put("shield.http.ssl.keystore", testnodeStore.getPath())
+                .put("shield.http.ssl.keystore_password", "testnode")
+                .put("shield.http.ssl.truststore", testnodeStore.getPath())
+                .put("shield.http.ssl.truststore_password", "testnode")
+                // SSL SETUP
+                .put("http.type", NettySSLHttpServerTransportModule.class.getName())
+                .put(TransportModule.TRANSPORT_TYPE_KEY, NettySSLTransportModule.class.getName())
+                .put("plugin.types", SecurityPlugin.class.getName())
+                .put("shield.n2n.file", ipFilterFile.getPath())
+                .build();
+    }
+
+
+    @Test(expected = SSLHandshakeException.class)
+    public void testThatRequireClientAuthRejectsWithoutCert() throws Exception {
+        TrustManager[] trustAllCerts = new TrustManager[]{
+                new X509TrustManager() {
+                    public java.security.cert.X509Certificate[] getAcceptedIssuers() {
+                        return null;
+                    }
+
+                    public void checkClientTrusted(
+                            java.security.cert.X509Certificate[] certs, String authType) {
+                    }
+
+                    public void checkServerTrusted(
+                            java.security.cert.X509Certificate[] certs, String authType) {
+                    }
+                }
+        };
+
+        setupTrustManagers(trustAllCerts);
+
+        TransportAddress transportAddress = internalCluster().getInstance(HttpServerTransport.class).boundAddress().boundAddress();
+        assertThat(transportAddress, is(instanceOf(InetSocketTransportAddress.class)));
+        InetSocketTransportAddress inetSocketTransportAddress = (InetSocketTransportAddress) transportAddress;
+        String url = String.format(Locale.ROOT, "https://%s:%s/", InetAddresses.toUriString(inetSocketTransportAddress.address().getAddress()), inetSocketTransportAddress.address().getPort());
+
+        HttpURLConnection connection = (HttpURLConnection) new URL(url).openConnection();
+        connection.connect();
+    }
+
+    @Test
+    @TestLogging("_root:DEBUG")
+    public void testThatConnectionToHTTPWorks() throws Exception {
+        File store = new File(getClass().getResource("/certs/simple/testnode.jks").toURI());
+
+        KeyStore ks;
+        KeyManagerFactory kmf;
+        try (FileInputStream in = new FileInputStream(store)){
+            // Load KeyStore
+            ks = KeyStore.getInstance("jks");
+            ks.load(in, "testnode".toCharArray());
+
+            // Initialize KeyManagerFactory
+            kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+            kmf.init(ks, "testnode".toCharArray());
+        }
+
+        TrustManagerFactory trustFactory;
+        try (FileInputStream in = new FileInputStream(store)) {
+            // Load TrustStore
+            ks.load(in, "testnode".toCharArray());
+
+            // Initialize a trust manager factory with the trusted store
+            trustFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+            trustFactory.init(ks);
+
+            // Retrieve the trust managers from the factory
+        }
+        setupTrustManagers(kmf.getKeyManagers(), trustFactory.getTrustManagers());
+
+        TransportAddress transportAddress = internalCluster().getInstance(HttpServerTransport.class).boundAddress().boundAddress();
+        assertThat(transportAddress, is(instanceOf(InetSocketTransportAddress.class)));
+        InetSocketTransportAddress inetSocketTransportAddress = (InetSocketTransportAddress) transportAddress;
+        String url = String.format(Locale.ROOT, "https://%s:%s/", InetAddresses.toUriString(inetSocketTransportAddress.address().getAddress()), inetSocketTransportAddress.address().getPort());
+
+        HttpURLConnection connection = (HttpURLConnection) new URL(url).openConnection();
+        connection.connect();
+
+        assertThat(connection.getResponseCode(), is(200));
+        String data = Streams.copyToString(new InputStreamReader(connection.getInputStream(), Charsets.UTF_8));
+        assertThat(data, containsString("You Know, for Search"));
+    }
+
+    private void setupTrustManagers(KeyManager[] keyManagers, TrustManager[] trustManagers) throws Exception {
+        SSLContext sc = SSLContext.getInstance("TLS");
+        sc.init(keyManagers, trustManagers, new SecureRandom());
+        HttpsURLConnection.setDefaultSSLSocketFactory(sc.getSocketFactory());
+        // totally secure
+        HttpsURLConnection.setDefaultHostnameVerifier(HOSTNAME_VERIFIER);
+    }
+
+    private void setupTrustManagers(TrustManager[] trustManagers) throws Exception {
+        setupTrustManagers(null, trustManagers);
+    }
+}