ZOOKEEPER-4912: Remove default TLS cipher overrides

Reviewers: cnauroth, anmolnar, kezhuw
Author: stoty
Closes #2239 from stoty/ZOOKEEPER-4912

zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md

@@ -43,6 +43,7 @@ limitations under the License.
         * [Advanced Configuration](#sc_advancedConfiguration)
         * [Cluster Options](#sc_clusterOptions)
         * [Encryption, Authentication, Authorization Options](#sc_authOptions)
+            * [TLS Cipher Suites](#sc_tls_cipher_suites)
         * [Experimental Options/Features](#Experimental+Options%2FFeatures)
         * [Unsafe Options](#Unsafe+Options)
         * [Disabling data directory autocreation](#Disabling+data+directory+autocreation)
@@ -1738,7 +1739,7 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
     (Java system properties: **zookeeper.ssl.ciphersuites** and **zookeeper.ssl.quorum.ciphersuites**)
     **New in 3.5.5:**
     Specifies the enabled cipher suites to be used in client and quorum TLS negotiation.
-    Default: Enabled cipher suites depend on the Java runtime version being used.
+    Default: JDK defaults since 3.10.0, and hard coded cipher suites for 3.9 and earlier versions. See [TLS Cipher Suites](#sc_tls_cipher_suites).
 
 * *ssl.context.supplier.class* and *ssl.quorum.context.supplier.class* :
     (Java system properties: **zookeeper.ssl.context.supplier.class** and **zookeeper.ssl.quorum.context.supplier.class**)
@@ -1877,6 +1878,19 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
     
     Default: **true** (3.9.0+), **false** (3.8.x)
 
+<a name="sc_tls_cipher_suites"></a>
+
+##### TLS Cipher Suites
+
+From 3.5.5 to 3.9 a hard coded default cipher list was used, with the ordering
+dependent on whether it is run Java 8 or a later version.
+
+The list on Java 8 includes TLSv1.2 CBC, GCM and TLSv1.3 ciphers in ordering: *TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384, TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, TLS_AES_256_GCM_SHA384,TLS_AES_128_GCM_SHA256, TLS_CHACHA20_POLY1305_SHA256*
+
+The list on Java 9+ includes TLSv1.2 GCM, CBC and TLSv1.3 ciphers in ordering: *TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384, TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, TLS_AES_256_GCM_SHA384,TLS_AES_128_GCM_SHA256, TLS_CHACHA20_POLY1305_SHA256*
+
+Since 3.10 there is no hardcoded list, and the JDK defaults are used.
+
 <a name="Experimental+Options%2FFeatures"></a>
 
 #### Experimental Options/Features

zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java

@@ -80,7 +80,10 @@ public class ClientX509Util extends X509Util {
         }
 
         sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
-        sslContextBuilder.protocols(getEnabledProtocols(config));
+        String[] enabledProtocols = getEnabledProtocols(config);
+        if (enabledProtocols != null) {
+            sslContextBuilder.protocols(enabledProtocols);
+        }
         Iterable<String> enabledCiphers = getCipherSuites(config);
         if (enabledCiphers != null) {
             sslContextBuilder.ciphers(enabledCiphers);
@@ -121,7 +124,10 @@ public class ClientX509Util extends X509Util {
         }
 
         sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
-        sslContextBuilder.protocols(getEnabledProtocols(config));
+        String[] enabledProtocols = getEnabledProtocols(config);
+        if (enabledProtocols != null) {
+            sslContextBuilder.protocols(enabledProtocols);
+        }
         sslContextBuilder.clientAuth(getClientAuth(config).toNettyClientAuth());
         Iterable<String> enabledCiphers = getCipherSuites(config);
         if (enabledCiphers != null) {
@@ -155,7 +161,7 @@ public class ClientX509Util extends X509Util {
     private String[] getEnabledProtocols(final ZKConfig config) {
         String enabledProtocolsInput = config.getProperty(getSslEnabledProtocolsProperty());
         if (enabledProtocolsInput == null) {
-            return new String[]{ config.getProperty(getSslProtocolProperty(), DEFAULT_PROTOCOL) };
+            return null;
         }
         return enabledProtocolsInput.split(",");
     }
@@ -167,10 +173,7 @@ public class ClientX509Util extends X509Util {
     private Iterable<String> getCipherSuites(final ZKConfig config) {
         String cipherSuitesInput = config.getProperty(getSslCipherSuitesProperty());
         if (cipherSuitesInput == null) {
-            if (getSslProvider(config) != SslProvider.JDK) {
-                return null;
-            }
-            return Arrays.asList(X509Util.getDefaultCipherSuites());
+            return null;
         } else {
             return Arrays.asList(cipherSuitesInput.split(","));
         }

zookeeper-server/src/main/java/org/apache/zookeeper/common/SSLContextAndOptions.java

@@ -160,7 +160,7 @@ public class SSLContextAndOptions {
     private String[] getCipherSuites(final ZKConfig config) {
         String cipherSuitesInput = config.getProperty(x509Util.getSslCipherSuitesProperty());
         if (cipherSuitesInput == null) {
-            return X509Util.getDefaultCipherSuites();
+            return null;
         } else {
             return cipherSuitesInput.split(",");
         }

zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java

@@ -36,16 +36,13 @@ import java.security.cert.X509CertSelector;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
-import java.util.Objects;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Supplier;
-import java.util.stream.Collectors;
 import javax.net.ssl.CertPathTrustManagerParameters;
 import javax.net.ssl.KeyManager;
 import javax.net.ssl.KeyManagerFactory;
 import javax.net.ssl.SSLContext;
 import javax.net.ssl.SSLServerSocket;
-import javax.net.ssl.SSLServerSocketFactory;
 import javax.net.ssl.SSLSocket;
 import javax.net.ssl.TrustManager;
 import javax.net.ssl.TrustManagerFactory;
@@ -62,11 +59,6 @@ import org.slf4j.LoggerFactory;
 
 /**
  * Utility code for X509 handling
- *
- * Default cipher suites:
- *
- *   Performance testing done by Facebook engineers shows that on Intel x86_64 machines, Java9 performs better with
- *   GCM and Java8 performs better with CBC, so these seem like reasonable defaults.
  */
 public abstract class X509Util implements Closeable, AutoCloseable {
 
@@ -102,6 +94,8 @@ public abstract class X509Util implements Closeable, AutoCloseable {
         List<String> supported = new ArrayList<>();
         try {
             supported = Arrays.asList(SSLContext.getDefault().getSupportedSSLParameters().getProtocols());
+            // We cannot use the default protocols directly, because the SSLContext factory methods
+            // only accept a single protocol
             if (supported.contains(TLS_1_3)) {
                 defaultProtocol = TLS_1_3;
             }
@@ -112,36 +106,6 @@ public abstract class X509Util implements Closeable, AutoCloseable {
         return defaultProtocol;
     }
 
-    // ChaCha20 was introduced in OpenJDK 11.0.15 and it is not supported by JDK8.
-    private static String[] getTLSv13Ciphers() {
-        return new String[]{"TLS_AES_256_GCM_SHA384", "TLS_AES_128_GCM_SHA256", "TLS_CHACHA20_POLY1305_SHA256"};
-    }
-
-    private static String[] getGCMCiphers() {
-        return new String[]{"TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"};
-    }
-
-    private static String[] getCBCCiphers() {
-        return new String[]{"TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256", "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256", "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA", "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA", "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384", "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384", "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA", "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA"};
-    }
-
-    /**
-     * Returns a filtered set of ciphers, where ciphers not supported by the JDK are removed.
-     */
-    private static String[] getSupportedCiphers(String[]... cipherLists) {
-        List<String> supported = Arrays.asList(
-            ((SSLServerSocketFactory) SSLServerSocketFactory.getDefault()).getSupportedCipherSuites());
-
-        return Arrays.stream(cipherLists).flatMap(Arrays::stream).filter(supported::contains).collect(Collectors.toList()).toArray(new String[0]);
-    }
-
-    // On Java 8, prefer CBC ciphers since AES-NI support is lacking and GCM is slower than CBC.
-    private static final String[] DEFAULT_CIPHERS_JAVA8 = getSupportedCiphers(getCBCCiphers(), getGCMCiphers(), getTLSv13Ciphers());
-    // On Java 9 and later, prefer GCM ciphers due to improved AES-NI support.
-    // Note that this performance assumption might not hold true for architectures other than x86_64.
-    // TLSv1.3 ciphers can be added at the end of the list without impacting the priority of TLSv1.3 vs TLSv1.2.
-    private static final String[] DEFAULT_CIPHERS_JAVA9 = getSupportedCiphers(getGCMCiphers(), getCBCCiphers(), getTLSv13Ciphers());
-
     public static final int DEFAULT_HANDSHAKE_DETECTION_TIMEOUT_MILLIS = 5000;
 
     /**
@@ -636,26 +600,6 @@ public abstract class X509Util implements Closeable, AutoCloseable {
         return getDefaultSSLContextAndOptions().createSSLServerSocket(port);
     }
 
-    static String[] getDefaultCipherSuites() {
-        return getDefaultCipherSuitesForJavaVersion(System.getProperty("java.specification.version"));
-    }
-
-    static String[] getDefaultCipherSuitesForJavaVersion(String javaVersion) {
-        Objects.requireNonNull(javaVersion);
-        if (javaVersion.matches("\\d+")) {
-            // Must be Java 9 or later
-            LOG.debug("Using Java9+ optimized cipher suites for Java version {}", javaVersion);
-            return DEFAULT_CIPHERS_JAVA9;
-        } else if (javaVersion.startsWith("1.")) {
-            // Must be Java 1.8 or earlier
-            LOG.debug("Using Java8 optimized cipher suites for Java version {}", javaVersion);
-            return DEFAULT_CIPHERS_JAVA8;
-        } else {
-            LOG.debug("Could not parse java version {}, using Java8 optimized cipher suites", javaVersion);
-            return DEFAULT_CIPHERS_JAVA8;
-        }
-    }
-
     private FileChangeWatcher newFileChangeWatcher(String fileLocation) throws IOException {
         if (fileLocation == null || fileLocation.isEmpty()) {
             return null;

zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java

@@ -829,72 +829,6 @@ public class X509UtilTest extends BaseX509ParameterizedTestCase {
         });
     }
 
-    @ParameterizedTest
-    @MethodSource("data")
-    public void testGetDefaultCipherSuitesJava8(
-            X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
-            throws Exception {
-        init(caKeyType, certKeyType, keyPassword, paramIndex);
-        String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("1.8");
-        // Java 8 default should have the CBC suites first
-        assertTrue(cipherSuites[0].contains("CBC"));
-    }
-
-    @ParameterizedTest
-    @MethodSource("data")
-    public void testGetDefaultCipherSuitesJava9(
-            X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
-            throws Exception {
-        init(caKeyType, certKeyType, keyPassword, paramIndex);
-        String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("9");
-        // Java 9+ default should have the GCM suites first
-        assertTrue(cipherSuites[0].contains("GCM"));
-    }
-
-    @ParameterizedTest
-    @MethodSource("data")
-    public void testGetDefaultCipherSuitesJava10(
-            X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
-            throws Exception {
-        init(caKeyType, certKeyType, keyPassword, paramIndex);
-        String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("10");
-        // Java 9+ default should have the GCM suites first
-        assertTrue(cipherSuites[0].contains("GCM"));
-    }
-
-    @ParameterizedTest
-    @MethodSource("data")
-    public void testGetDefaultCipherSuitesJava11(
-            X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
-            throws Exception {
-        init(caKeyType, certKeyType, keyPassword, paramIndex);
-        String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("11");
-        // Java 9+ default should have the GCM suites first
-        assertTrue(cipherSuites[0].contains("GCM"));
-    }
-
-    @ParameterizedTest
-    @MethodSource("data")
-    public void testGetDefaultCipherSuitesUnknownVersion(
-            X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
-            throws Exception {
-        init(caKeyType, certKeyType, keyPassword, paramIndex);
-        String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("notaversion");
-        // If version can't be parsed, use the more conservative Java 8 default
-        assertTrue(cipherSuites[0].contains("CBC"));
-    }
-
-    @ParameterizedTest
-    @MethodSource("data")
-    public void testGetDefaultCipherSuitesNullVersion(
-            X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
-            throws Exception {
-        init(caKeyType, certKeyType, keyPassword, paramIndex);
-        assertThrows(NullPointerException.class, () -> {
-            X509Util.getDefaultCipherSuitesForJavaVersion(null);
-        });
-    }
-
     // Warning: this will reset the x509Util
     private void setCustomCipherSuites() {
         System.setProperty(x509Util.getCipherSuitesProperty(), customCipherSuites[0] + "," + customCipherSuites[1]);