diff --git a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md index 3032c3e7411..d015079d7dc 100644 --- a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md +++ b/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**) 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..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 @@ -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,13 @@ 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(","); } @@ -178,7 +184,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..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 @@ -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 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()); } + // 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]); 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);