From 231ffee0a13804e440c00c1fe077e38b4ab4a29f Mon Sep 17 00:00:00 2001 From: Istvan Toth Date: Sun, 11 May 2025 04:16:38 +0200 Subject: [PATCH] ZOOKEEPER-5048: Remove default TLS cipher overrides Reviewers: cnauroth, anmolnar, kezhuw Author: stoty Closes #2239 from stoty/ZOOKEEPER-4912 (cherry picked from commit 2aaeff840e8e5b61a30530b261b8c05c181d078f) --- .../main/resources/markdown/zookeeperAdmin.md | 16 ++++- .../zookeeper/common/ClientX509Util.java | 17 +++-- .../common/SSLContextAndOptions.java | 2 +- .../org/apache/zookeeper/common/X509Util.java | 60 +---------------- .../apache/zookeeper/common/X509UtilTest.java | 66 ------------------- 5 files changed, 28 insertions(+), 133 deletions(-) diff --git a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md index b27d637b39d..0fa3b9e78ae 100644 --- a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md +++ b/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) @@ -1734,7 +1735,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**) @@ -1887,6 +1888,19 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp Default: **true** (3.9.0+), **false** (3.8.x) + + +##### 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. + #### Experimental Options/Features diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java b/zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java index f1d1b164bcf..6994334b42d 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java @@ -86,7 +86,10 @@ public SslContext createNettySslContextForClient(ZKConfig config) } handleTcnativeOcspStapling(sslContextBuilder, config); - sslContextBuilder.protocols(getEnabledProtocols(config)); + String[] enabledProtocols = getEnabledProtocols(config); + if (enabledProtocols != null) { + sslContextBuilder.protocols(enabledProtocols); + } Iterable enabledCiphers = getCipherSuites(config); if (enabledCiphers != null) { sslContextBuilder.ciphers(enabledCiphers); @@ -127,7 +130,10 @@ public SslContext createNettySslContextForServer(ZKConfig config, KeyManager key } handleTcnativeOcspStapling(sslContextBuilder, config); - sslContextBuilder.protocols(getEnabledProtocols(config)); + String[] enabledProtocols = getEnabledProtocols(config); + if (enabledProtocols != null) { + sslContextBuilder.protocols(enabledProtocols); + } sslContextBuilder.clientAuth(getClientAuth(config).toNettyClientAuth()); Iterable enabledCiphers = getCipherSuites(config); if (enabledCiphers != null) { @@ -172,7 +178,7 @@ protected void initEngine(SSLEngine sslEngine) { 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(","); } @@ -184,10 +190,7 @@ private X509Util.ClientAuth getClientAuth(final ZKConfig config) { private Iterable 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(",")); } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/common/SSLContextAndOptions.java b/zookeeper-server/src/main/java/org/apache/zookeeper/common/SSLContextAndOptions.java index 3a254255292..c712f6acb6c 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/common/SSLContextAndOptions.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/common/SSLContextAndOptions.java @@ -160,7 +160,7 @@ private String[] getEnabledProtocols(final ZKConfig config, final SSLContext ssl private String[] getCipherSuites(final ZKConfig config) { String cipherSuitesInput = config.getProperty(x509Util.getSslCipherSuitesProperty()); if (cipherSuitesInput == null) { - return X509Util.getDefaultCipherSuites(); + return null; } else { return cipherSuitesInput.split(","); } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java b/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java index 9cfa79bc1cb..9620dfbd484 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java @@ -39,17 +39,14 @@ import java.util.Arrays; import java.util.HashSet; import java.util.List; -import java.util.Objects; import java.util.Set; 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; @@ -66,11 +63,6 @@ /** * 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 { @@ -106,6 +98,8 @@ private static String defaultTlsProtocol() { List 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; } @@ -116,36 +110,6 @@ private static String defaultTlsProtocol() { 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 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; /** @@ -677,26 +641,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; diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java index b5ac140ff5e..1c5104e784b 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java @@ -831,72 +831,6 @@ public void handshakeCompleted(HandshakeCompletedEvent handshakeCompletedEvent) }); } - @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]);