From 0baa2b5562380535be4e8a03287c909e48cd87c8 Mon Sep 17 00:00:00 2001 From: Sanju98 Date: Tue, 26 May 2026 18:16:22 +0530 Subject: [PATCH 1/4] ZOOKEEPER-4415 + ZOOKEEPER-4912: enable TLSv1.3 and remove default cipher overrides Combined backport of two upstream commits to branch-3.6: - ZOOKEEPER-4415 (Apache 66771be4d, in 3.9.2+): adds TLSv1.3 support to the server by picking TLSv1.3 as the default SSLContext protocol when the running JDK supports it, and returning the JDK's default enabled protocols (TLSv1.3 + TLSv1.2 on JDK 11+; TLSv1.2-only on older JDKs) when `ssl.enabledProtocols` is unset. - ZOOKEEPER-4912 (Apache master 2aaeff840, not yet in any 3.9.x release): removes the hardcoded TLSv1.2-only default cipher list from X509Util. SSLContextAndOptions.getCipherSuites now returns null when `ssl.ciphersuites` is unset, which lets the JDK supply its own default cipher list at SSLEngine construction time. Motivation ========== With branch-3.6 today, the server installs a fixed TLSv1.2-only cipher set on every SSLEngine even when the JDK supports more recent ciphers and protocols. Any attempt to enable TLSv1.3 from configuration runs into two problems: 1. The hardcoded cipher list contains no TLSv1.3 ciphers, so TLSv1.3 handshakes have no overlap on the server side and fail. 2. The default protocol is pinned to TLSv1.2 regardless of what the JDK supports. The MP-side workaround of shipping `ssl.ciphersuites` with explicit TLSv1.3 cipher names is brittle against JDK drift: if the running JDK does not recognise a cipher name (e.g. TLS_CHACHA20_POLY1305_SHA256 on OpenJDK 11.0.8, which only added it in 11.0.11), SSLEngine throws `IllegalArgumentException: Unsupported CipherSuite` at engine setup and every handshake fails. With this backport, neither X509Util nor any downstream config has to carry a cipher list at all. The JDK is the single source of truth for both protocol selection and cipher selection, so JDK upgrades automatically pull in new ciphers and protocols without an MP change. Per-deployment overrides via `ssl.enabledProtocols` / `ssl.ciphersuites` continue to work exactly as before. Changes ======= zookeeper-server/.../X509Util.java - Add TLS_1_1 / TLS_1_2 / TLS_1_3 string constants. - Replace `DEFAULT_PROTOCOL = "TLSv1.2"` with `defaultTlsProtocol()`, which returns TLSv1.3 when the JDK supports it, TLSv1.2 otherwise. - Delete getGCMCiphers / getCBCCiphers / concatArrays / DEFAULT_CIPHERS_JAVA8 / DEFAULT_CIPHERS_JAVA9 / getDefaultCipherSuites / getDefaultCipherSuitesForJavaVersion. - Drop the now-irrelevant "Default cipher suites" class doc-comment and the unused `Objects` import. zookeeper-server/.../SSLContextAndOptions.java - getCipherSuites: return null when `ssl.ciphersuites` is unset instead of falling back to X509Util.getDefaultCipherSuites(). - Constructor: null-guard cipherSuitesAsList (Collections.asList of null would NPE; configureSslParameters and createNettyJdkSslContext both already accept null = "use JDK defaults"). - getEnabledProtocols: return sslContext.getDefaultSSLParameters() .getProtocols() (the JDK's curated default list for the chosen protocol) instead of new String[]{sslContext.getProtocol()}. zookeeper-server/.../X509UtilTest.java - Augment testCreateSSLContextWithoutCustomProtocol to assert TLSv1.3 is selected on JDKs that support it (and the enabled protocol list contains both TLSv1.2 and TLSv1.3); otherwise assert TLSv1.2-only. - Switch "TLSv1.1" string literal to X509Util.TLS_1_1 constant. - Pin testClientRenegotiationFails to TLSv1.2 (renegotiation is not a TLSv1.3 feature, so this test must explicitly force 1.2 to remain meaningful once 1.3 becomes the default). - Delete the six obsolete testGetDefaultCipherSuites* tests that exercised the now-deleted Java-version-aware cipher selector. zookeeper-docs/.../zookeeperAdmin.md - Document the new defaults: TLSv1.3 when JDK supports it; JDK default protocol/cipher list when properties are unset. Local verification ================== X509UtilTest: 304/304 pass, including the new TLSv1.3 assertions. QuorumSSLTest: 12/13 pass; testOCSP failure is pre-existing on branch-3.6 (verified against unmodified HEAD) and unrelated to TLS protocol/cipher logic. Full zookeeper-server module compiles (384 source files, BUILD SUCCESS). --- .../main/resources/markdown/zookeeperAdmin.md | 6 +- .../common/SSLContextAndOptions.java | 12 +++- .../org/apache/zookeeper/common/X509Util.java | 69 +++++++------------ .../apache/zookeeper/common/X509UtilTest.java | 61 +++++----------- 4 files changed, 55 insertions(+), 93 deletions(-) diff --git a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md index 3032c3e7411..045bac6eb90 100644 --- a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md +++ b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md @@ -1453,19 +1453,19 @@ 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. * *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**) 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 01e97c3f594..8a3a0f5aa4e 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 @@ -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,7 +170,10 @@ 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 + return sslContext.getDefaultSSLParameters().getProtocols(); } return enabledProtocolsInput.split(","); } @@ -178,7 +181,10 @@ 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(); + // 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(","); } 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 52cb5fe6f0c..9ce71f5ab4d 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 @@ -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 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; 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 05d216b2b3b..3a1ab781b13 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 @@ -29,7 +29,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,11 +120,23 @@ 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 supported = Arrays.asList(SSLContext.getDefault().getSupportedSSLParameters().getProtocols()); + if (supported.contains(X509Util.TLS_1_3)) { + assertEquals(X509Util.TLS_1_3, sslContext.getProtocol()); + List 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()); @@ -531,7 +545,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 +565,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 +608,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]); From db01e68efc7368d7b179dc6b1a5db3b13d82d993 Mon Sep 17 00:00:00 2001 From: Sanju98 Date: Tue, 26 May 2026 18:30:21 +0530 Subject: [PATCH 2/4] QuorumSSLTest: adopt X509Util.TLS_1_x constants for protocol literals testProtocolVersion still used the raw "TLSv1.2" / "TLSv1.1" strings even after the ZOOKEEPER-4415 backport introduced the TLS_1_1 / TLS_1_2 constants on X509Util. Switch the two System.setProperty calls to use the constants for consistency with the new convention. No behaviour change. Verified by running testProtocolVersion locally (1/1 pass). --- .../org/apache/zookeeper/server/quorum/QuorumSSLTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java index 593edbd9245..92785ab399d 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java +++ b/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); From e685ea44762d69a4155024875007b34cba23d534 Mon Sep 17 00:00:00 2001 From: Sanju98 Date: Wed, 27 May 2026 11:04:39 +0530 Subject: [PATCH 3/4] X509UtilTest: pin null-cipher path in SSLContextAndOptions constructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a focused regression guard for the null-handling chain introduced by the ZOOKEEPER-4912 backport. testCreateSSLContextWithoutCustomProtocol exercises the unset-ssl.ciphersuites path implicitly, but does not pin the assertion to that condition — a future regression that reintroduces a hardcoded default cipher list would still let it pass. testCreateSSLContextWithoutCipherSuites explicitly clears the property, rebuilds the X509Util, and asserts SSLContextAndOptions is constructed without NPE. Pre-fix, the constructor would fail on Collections.unmodifiableList(Arrays.asList(null)) before returning. --- .../org/apache/zookeeper/common/X509UtilTest.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) 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 3a1ab781b13..e8a061002b6 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 @@ -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; @@ -142,6 +143,19 @@ public void testCreateSSLContextWithCustomProtocol() throws Exception { 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. + @Test(timeout = 5000) + public void testCreateSSLContextWithoutCipherSuites() throws Exception { + System.clearProperty(x509Util.getCipherSuitesProperty()); + x509Util.close(); + x509Util = new ClientX509Util(); + SSLContext sslContext = x509Util.getDefaultSSLContext(); + assertNotNull(sslContext); + } + @Test(timeout = 5000) public void testCreateSSLContextWithoutKeyStoreLocation() throws Exception { System.clearProperty(x509Util.getSslKeystoreLocationProperty()); From cf39e49b5422ba9a90b0bab108f8334fc1368110 Mon Sep 17 00:00:00 2001 From: Sanju98 Date: Fri, 29 May 2026 10:34:43 +0530 Subject: [PATCH 4/4] Address review feedback: extend cipher-suites regression guard + doc note Address two non-blocking review comments from @adityaagg09 on PR #143. * X509UtilTest.testCreateSSLContextWithoutCipherSuites: loop the regression guard over both ClientX509Util (zookeeper.ssl.* prefix) and QuorumX509Util (zookeeper.ssl.quorum.* prefix) so a future change to either subclass's config prefix or defaults path is caught. The null-handling lives in the shared SSLContextAndOptions, but exercising both subclasses removes the prior duplication where all eight parametrized invocations ran the identical ClientX509Util path. Verified locally: X509UtilTest 312/312 pass on JDK 17. * SSLContextAndOptions.getEnabledProtocols and zookeeperAdmin.md: document that setting only ssl.protocol 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). Operators who want strict single-protocol pinning must set ssl.enabledProtocols explicitly. The behavior matches upstream ZOOKEEPER-4415; this just makes the implication explicit in the operator-facing docs and in the source comment so future readers understand the widening. No functional change. --- .../main/resources/markdown/zookeeperAdmin.md | 1 + .../common/SSLContextAndOptions.java | 3 +++ .../apache/zookeeper/common/X509UtilTest.java | 22 ++++++++++++++----- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md index 045bac6eb90..d015079d7dc 100644 --- a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md +++ b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md @@ -1454,6 +1454,7 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp **New in 3.5.5:** Specifies to protocol to be used in client and quorum TLS negotiation. 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**) 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 8a3a0f5aa4e..82ea024c745 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 @@ -173,6 +173,9 @@ private String[] getEnabledProtocols(final ZKConfig config, final SSLContext ssl // 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(","); 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 e8a061002b6..30da3234e56 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 @@ -147,13 +147,25 @@ public void testCreateSSLContextWithCustomProtocol() throws Exception { // 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 { - System.clearProperty(x509Util.getCipherSuitesProperty()); - x509Util.close(); - x509Util = new ClientX509Util(); - SSLContext sslContext = x509Util.getDefaultSSLContext(); - assertNotNull(sslContext); + 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)