ZOOKEEPER-4415 + ZOOKEEPER-4912: enable TLSv1.3 and remove default cipher overrides

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

@@ -1453,19 +1453,20 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
     (Java system properties: **zookeeper.ssl.protocol** and **zookeeper.ssl.quorum.protocol**)
     **New in 3.5.5:**
     Specifies to protocol to be used in client and quorum TLS negotiation.
-    Default: TLSv1.2
+    Default: TLSv1.3 when the running JDK supports it, TLSv1.2 otherwise.
+    Setting only this property selects the `SSLContext` protocol but does not by itself restrict the enabled-protocol set — the JDK's default enabled list for the chosen context applies. To pin an ensemble to a single TLS protocol (e.g. TLSv1.2 only), set `ssl.enabledProtocols` (or `ssl.quorum.enabledProtocols`) explicitly.
 
 * *ssl.enabledProtocols* and *ssl.quorum.enabledProtocols* :
     (Java system properties: **zookeeper.ssl.enabledProtocols** and **zookeeper.ssl.quorum.enabledProtocols**)
     **New in 3.5.5:**
     Specifies the enabled protocols in client and quorum TLS negotiation.
-    Default: value of `protocol` property
+    Default: when unset, the JDK's default enabled protocols for the configured `protocol` are used. On a JDK that supports TLSv1.3 this is `TLSv1.3, TLSv1.2`; otherwise it is `TLSv1.2`.
 
 * *ssl.ciphersuites* and *ssl.quorum.ciphersuites* :
     (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: when unset, the JDK's default cipher suites are used. This adapts automatically to the JDK in use and avoids freezing a cipher list that may not match the running JDK.
 
 * *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**)

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

@@ -67,7 +67,7 @@ public class SSLContextAndOptions {
         this.enabledProtocols = getEnabledProtocols(requireNonNull(config), sslContext);
         String[] ciphers = getCipherSuites(config);
         this.cipherSuites = ciphers;
-        this.cipherSuitesAsList = Collections.unmodifiableList(Arrays.asList(ciphers));
+        this.cipherSuitesAsList = ciphers == null ? null : Collections.unmodifiableList(Arrays.asList(ciphers));
         this.clientAuth = getClientAuth(config);
         this.handshakeDetectionTimeoutMillis = getHandshakeDetectionTimeoutMillis(config);
     }
@@ -170,15 +170,24 @@ private void configureSslParameters(SSLParameters sslParameters, boolean isClien
     private String[] getEnabledProtocols(final ZKConfig config, final SSLContext sslContext) {
         String enabledProtocolsInput = config.getProperty(x509Util.getSslEnabledProtocolsProperty());
         if (enabledProtocolsInput == null) {
-            return new String[]{sslContext.getProtocol()};
+            // Use JDK defaults for enabled protocols:
+            //   Protocol TLSv1.3 -> enabled protocols TLSv1.3 and TLSv1.2
+            //   Protocol TLSv1.2 -> enabled protocols TLSv1.2
+            // Note: setting only ssl.protocol selects the SSLContext protocol but does
+            // not by itself restrict the enabled-protocol set. To strictly pin to a
+            // single protocol, the operator must set ssl.enabledProtocols explicitly.
+            return sslContext.getDefaultSSLParameters().getProtocols();
         }
         return enabledProtocolsInput.split(",");
     }
 
     private String[] getCipherSuites(final ZKConfig config) {
         String cipherSuitesInput = config.getProperty(x509Util.getSslCipherSuitesProperty());
         if (cipherSuitesInput == null) {
-            return X509Util.getDefaultCipherSuites();
+            // Returning null lets the JDK pick its own default cipher list at
+            // SSLEngine construction time. Avoids freezing a hardcoded list
+            // that may not match the running JDK (ZOOKEEPER-4912).
+            return null;
         } else {
             return cipherSuitesInput.split(",");
         }

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

@@ -33,7 +33,9 @@
 import java.security.Security;
 import java.security.cert.PKIXBuilderParameters;
 import java.security.cert.X509CertSelector;
-import java.util.Objects;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Supplier;
 import javax.net.ssl.CertPathTrustManagerParameters;
@@ -55,17 +57,15 @@
 
 /**
  * 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 {
 
     private static final Logger LOG = LoggerFactory.getLogger(X509Util.class);
 
     private static final String REJECT_CLIENT_RENEGOTIATION_PROPERTY = "jdk.tls.rejectClientInitiatedRenegotiation";
+    public static final String TLS_1_1 = "TLSv1.1";
+    public static final String TLS_1_2 = "TLSv1.2";
+    public static final String TLS_1_3 = "TLSv1.3";
 
     static {
         // Client-initiated renegotiation in TLS is unsafe and
@@ -79,28 +79,27 @@ public abstract class X509Util implements Closeable, AutoCloseable {
         }
     }
 
-    public static final String DEFAULT_PROTOCOL = "TLSv1.2";
-    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"};
-    }
+    public static final String DEFAULT_PROTOCOL = defaultTlsProtocol();
 
-    private static String[] concatArrays(String[] left, String[] right) {
-        String[] result = new String[left.length + right.length];
-        System.arraycopy(left, 0, result, 0, left.length);
-        System.arraycopy(right, 0, result, left.length, right.length);
-        return result;
+    /**
+     * Return TLSv1.3 or TLSv1.2 depending on Java runtime version being used.
+     * TLSv1.3 was first introduced in JDK11 and back-ported to OpenJDK 8u272.
+     */
+    private static String defaultTlsProtocol() {
+        String defaultProtocol = TLS_1_2;
+        List<String> supported = new ArrayList<>();
+        try {
+            supported = Arrays.asList(SSLContext.getDefault().getSupportedSSLParameters().getProtocols());
+            if (supported.contains(TLS_1_3)) {
+                defaultProtocol = TLS_1_3;
+            }
+        } catch (NoSuchAlgorithmException e) {
+            // Ignore. SSLContext.getDefault() should not normally fail; fall back to TLSv1.2.
+        }
+        LOG.info("Default TLS protocol is {}, supported TLS protocols are {}", defaultProtocol, supported);
+        return defaultProtocol;
     }
 
-    // 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 = concatArrays(getCBCCiphers(), getGCMCiphers());
-    // 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.
-    private static final String[] DEFAULT_CIPHERS_JAVA9 = concatArrays(getGCMCiphers(), getCBCCiphers());
-
     public static final int DEFAULT_HANDSHAKE_DETECTION_TIMEOUT_MILLIS = 5000;
 
     /**
@@ -529,26 +528,6 @@ public SSLServerSocket createSSLServerSocket(int port) throws X509Exception, IOE
         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

@@ -21,6 +21,7 @@
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import java.io.IOException;
 import java.net.InetAddress;
@@ -29,7 +30,9 @@
 import java.net.Socket;
 import java.security.NoSuchAlgorithmException;
 import java.security.Security;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.List;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
@@ -118,16 +121,53 @@ public void cleanUp() {
     public void testCreateSSLContextWithoutCustomProtocol() throws Exception {
         SSLContext sslContext = x509Util.getDefaultSSLContext();
         assertEquals(X509Util.DEFAULT_PROTOCOL, sslContext.getProtocol());
+
+        // Check that TLSv1.3 is selected on JDKs that support it (OpenJDK 8u272 and later).
+        List<String> supported = Arrays.asList(SSLContext.getDefault().getSupportedSSLParameters().getProtocols());
+        if (supported.contains(X509Util.TLS_1_3)) {
+            assertEquals(X509Util.TLS_1_3, sslContext.getProtocol());
+            List<String> protos = Arrays.asList(sslContext.getDefaultSSLParameters().getProtocols());
+            assertTrue(protos.contains(X509Util.TLS_1_2));
+            assertTrue(protos.contains(X509Util.TLS_1_3));
+        } else {
+            assertEquals(X509Util.TLS_1_2, sslContext.getProtocol());
+            assertArrayEquals(new String[]{X509Util.TLS_1_2}, sslContext.getDefaultSSLParameters().getProtocols());
+        }
     }
 
     @Test(timeout = 5000)
     public void testCreateSSLContextWithCustomProtocol() throws Exception {
-        final String protocol = "TLSv1.1";
+        final String protocol = X509Util.TLS_1_1;
         System.setProperty(x509Util.getSslProtocolProperty(), protocol);
         SSLContext sslContext = x509Util.getDefaultSSLContext();
         assertEquals(protocol, sslContext.getProtocol());
     }
 
+    // Regression guard for ZOOKEEPER-4912: when ssl.ciphersuites is unset,
+    // SSLContextAndOptions.getCipherSuites() returns null and the constructor
+    // must null-guard cipherSuitesAsList. A regression that re-wrapped null
+    // via Arrays.asList(null) would NPE before the SSLContext is returned.
+    // Exercises both ClientX509Util (zookeeper.ssl.* prefix) and
+    // QuorumX509Util (zookeeper.ssl.quorum.* prefix) so the guard catches a
+    // future change to either subclass's config prefix or defaults path.
+    @Test(timeout = 5000)
+    public void testCreateSSLContextWithoutCipherSuites() throws Exception {
+        X509Util[] utils = {new ClientX509Util(), new QuorumX509Util()};
+        try {
+            for (X509Util util : utils) {
+                x509TestContext.setSystemProperties(util, KeyStoreFileType.JKS, KeyStoreFileType.JKS);
+                System.clearProperty(util.getCipherSuitesProperty());
+                SSLContext sslContext = util.getDefaultSSLContext();
+                assertNotNull("SSLContext null for " + util.getClass().getSimpleName(), sslContext);
+                x509TestContext.clearSystemProperties(util);
+            }
+        } finally {
+            for (X509Util util : utils) {
+                util.close();
+            }
+        }
+    }
+
     @Test(timeout = 5000)
     public void testCreateSSLContextWithoutKeyStoreLocation() throws Exception {
         System.clearProperty(x509Util.getSslKeystoreLocationProperty());
@@ -531,7 +571,9 @@ private static void forceClose(ServerSocket s) {
     }
 
     // This test makes sure that client-initiated TLS renegotiation does not
-    // succeed. We explicitly disable it at the top of X509Util.java.
+    // succeed when using TLSv1.2. We explicitly disable it at the top of X509Util.java.
+    // Force TLSv1.2 since the renegotiation feature is not supported in TLSv1.3, which
+    // would make this test invalid when the JDK picks TLSv1.3 by default.
     @Test(expected = SSLHandshakeException.class)
     public void testClientRenegotiationFails() throws Throwable {
         int port = PortAssignment.unique();
@@ -549,6 +591,7 @@ public void testClientRenegotiationFails() throws Throwable {
                 @Override
                 public SSLSocket call() throws Exception {
                     SSLSocket sslSocket = (SSLSocket) listeningSocket.accept();
+                    sslSocket.setEnabledProtocols(new String[]{X509Util.TLS_1_2});
                     sslSocket.addHandshakeCompletedListener(new HandshakeCompletedListener() {
                         @Override
                         public void handshakeCompleted(HandshakeCompletedEvent handshakeCompletedEvent) {
@@ -591,46 +634,6 @@ public void handshakeCompleted(HandshakeCompletedEvent handshakeCompletedEvent)
         }
     }
 
-    @Test
-    public void testGetDefaultCipherSuitesJava8() {
-        String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("1.8");
-        // Java 8 default should have the CBC suites first
-        assertTrue(cipherSuites[0].contains("CBC"));
-    }
-
-    @Test
-    public void testGetDefaultCipherSuitesJava9() {
-        String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("9");
-        // Java 9+ default should have the GCM suites first
-        assertTrue(cipherSuites[0].contains("GCM"));
-    }
-
-    @Test
-    public void testGetDefaultCipherSuitesJava10() {
-        String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("10");
-        // Java 9+ default should have the GCM suites first
-        assertTrue(cipherSuites[0].contains("GCM"));
-    }
-
-    @Test
-    public void testGetDefaultCipherSuitesJava11() {
-        String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("11");
-        // Java 9+ default should have the GCM suites first
-        assertTrue(cipherSuites[0].contains("GCM"));
-    }
-
-    @Test
-    public void testGetDefaultCipherSuitesUnknownVersion() {
-        String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("notaversion");
-        // If version can't be parsed, use the more conservative Java 8 default
-        assertTrue(cipherSuites[0].contains("CBC"));
-    }
-
-    @Test(expected = NullPointerException.class)
-    public void testGetDefaultCipherSuitesNullVersion() {
-        X509Util.getDefaultCipherSuitesForJavaVersion(null);
-    }
-
     // Warning: this will reset the x509Util
     private void setCustomCipherSuites() {
         System.setProperty(x509Util.getCipherSuitesProperty(), customCipherSuites[0] + "," + customCipherSuites[1]);

zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java

@@ -56,6 +56,7 @@
 import org.apache.zookeeper.PortAssignment;
 import org.apache.zookeeper.client.ZKClientConfig;
 import org.apache.zookeeper.common.QuorumX509Util;
+import org.apache.zookeeper.common.X509Util;
 import org.apache.zookeeper.server.ServerCnxnFactory;
 import org.apache.zookeeper.test.ClientBase;
 import org.bouncycastle.asn1.ocsp.OCSPResponse;
@@ -879,7 +880,7 @@ public void testCipherSuites() throws Exception {
 
     @Test
     public void testProtocolVersion() throws Exception {
-        System.setProperty(quorumX509Util.getSslProtocolProperty(), "TLSv1.2");
+        System.setProperty(quorumX509Util.getSslProtocolProperty(), X509Util.TLS_1_2);
 
         q1 = new MainThread(1, clientPortQp1, quorumConfiguration, SSL_QUORUM_ENABLED);
         q2 = new MainThread(2, clientPortQp2, quorumConfiguration, SSL_QUORUM_ENABLED);
@@ -890,7 +891,7 @@ public void testProtocolVersion() throws Exception {
         assertTrue(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp1, CONNECTION_TIMEOUT));
         assertTrue(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp2, CONNECTION_TIMEOUT));
 
-        System.setProperty(quorumX509Util.getSslProtocolProperty(), "TLSv1.1");
+        System.setProperty(quorumX509Util.getSslProtocolProperty(), X509Util.TLS_1_1);
 
         // This server should fail to join the quorum as it is not using TLSv1.2
         q3 = new MainThread(3, clientPortQp3, quorumConfiguration, SSL_QUORUM_ENABLED);