Skip to content

Commit cf051ad

Browse files
stotystoty
committed
ZOOKEEPER-4912 Remove default TLS cipher overrides
1 parent 2d2820e commit cf051ad

File tree

5 files changed

+16
-133
lines changed

5 files changed

+16
-133
lines changed

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1738,7 +1738,9 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
17381738
(Java system properties: **zookeeper.ssl.ciphersuites** and **zookeeper.ssl.quorum.ciphersuites**)
17391739
**New in 3.5.5:**
17401740
Specifies the enabled cipher suites to be used in client and quorum TLS negotiation.
1741-
Default: Enabled cipher suites depend on the Java runtime version being used.
1741+
Default: None, the JVM defaults are used (3.10.0+),
1742+
Enabled cipher suites are hard coded, with the ordering dependent on whether Java 8, or Java 9+ is used.
1743+
For Java 8 the list begins with the TLSv1.2 CBC ciphers, while for Java 9+ it begins with the TLSv1.2 CBM ciphers (3.5.5-3.9.x).
17421744

17431745
* *ssl.context.supplier.class* and *ssl.quorum.context.supplier.class* :
17441746
(Java system properties: **zookeeper.ssl.context.supplier.class** and **zookeeper.ssl.quorum.context.supplier.class**)

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

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,10 @@ public SslContext createNettySslContextForClient(ZKConfig config)
8080
}
8181

8282
sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
83-
sslContextBuilder.protocols(getEnabledProtocols(config));
83+
String[] enabledProtocols = getEnabledProtocols(config);
84+
if (enabledProtocols != null) {
85+
sslContextBuilder.protocols(enabledProtocols);
86+
}
8487
Iterable<String> enabledCiphers = getCipherSuites(config);
8588
if (enabledCiphers != null) {
8689
sslContextBuilder.ciphers(enabledCiphers);
@@ -121,7 +124,10 @@ public SslContext createNettySslContextForServer(ZKConfig config, KeyManager key
121124
}
122125

123126
sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
124-
sslContextBuilder.protocols(getEnabledProtocols(config));
127+
String[] enabledProtocols = getEnabledProtocols(config);
128+
if (enabledProtocols != null) {
129+
sslContextBuilder.protocols(enabledProtocols);
130+
}
125131
sslContextBuilder.clientAuth(getClientAuth(config).toNettyClientAuth());
126132
Iterable<String> enabledCiphers = getCipherSuites(config);
127133
if (enabledCiphers != null) {
@@ -155,7 +161,7 @@ protected void initEngine(SSLEngine sslEngine) {
155161
private String[] getEnabledProtocols(final ZKConfig config) {
156162
String enabledProtocolsInput = config.getProperty(getSslEnabledProtocolsProperty());
157163
if (enabledProtocolsInput == null) {
158-
return new String[]{ config.getProperty(getSslProtocolProperty(), DEFAULT_PROTOCOL) };
164+
return null;
159165
}
160166
return enabledProtocolsInput.split(",");
161167
}
@@ -167,10 +173,7 @@ private X509Util.ClientAuth getClientAuth(final ZKConfig config) {
167173
private Iterable<String> getCipherSuites(final ZKConfig config) {
168174
String cipherSuitesInput = config.getProperty(getSslCipherSuitesProperty());
169175
if (cipherSuitesInput == null) {
170-
if (getSslProvider(config) != SslProvider.JDK) {
171-
return null;
172-
}
173-
return Arrays.asList(X509Util.getDefaultCipherSuites());
176+
return null;
174177
} else {
175178
return Arrays.asList(cipherSuitesInput.split(","));
176179
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ private String[] getEnabledProtocols(final ZKConfig config, final SSLContext ssl
160160
private String[] getCipherSuites(final ZKConfig config) {
161161
String cipherSuitesInput = config.getProperty(x509Util.getSslCipherSuitesProperty());
162162
if (cipherSuitesInput == null) {
163-
return X509Util.getDefaultCipherSuites();
163+
return null;
164164
} else {
165165
return cipherSuitesInput.split(",");
166166
}

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

Lines changed: 2 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -36,16 +36,13 @@
3636
import java.util.ArrayList;
3737
import java.util.Arrays;
3838
import java.util.List;
39-
import java.util.Objects;
4039
import java.util.concurrent.atomic.AtomicReference;
4140
import java.util.function.Supplier;
42-
import java.util.stream.Collectors;
4341
import javax.net.ssl.CertPathTrustManagerParameters;
4442
import javax.net.ssl.KeyManager;
4543
import javax.net.ssl.KeyManagerFactory;
4644
import javax.net.ssl.SSLContext;
4745
import javax.net.ssl.SSLServerSocket;
48-
import javax.net.ssl.SSLServerSocketFactory;
4946
import javax.net.ssl.SSLSocket;
5047
import javax.net.ssl.TrustManager;
5148
import javax.net.ssl.TrustManagerFactory;
@@ -62,11 +59,6 @@
6259

6360
/**
6461
* Utility code for X509 handling
65-
*
66-
* Default cipher suites:
67-
*
68-
* Performance testing done by Facebook engineers shows that on Intel x86_64 machines, Java9 performs better with
69-
* GCM and Java8 performs better with CBC, so these seem like reasonable defaults.
7062
*/
7163
public abstract class X509Util implements Closeable, AutoCloseable {
7264

@@ -102,6 +94,8 @@ private static String defaultTlsProtocol() {
10294
List<String> supported = new ArrayList<>();
10395
try {
10496
supported = Arrays.asList(SSLContext.getDefault().getSupportedSSLParameters().getProtocols());
97+
// We cannot use the default protocols directly, because the SSLContext factory methods
98+
// only accept a single protocol
10599
if (supported.contains(TLS_1_3)) {
106100
defaultProtocol = TLS_1_3;
107101
}
@@ -112,36 +106,6 @@ private static String defaultTlsProtocol() {
112106
return defaultProtocol;
113107
}
114108

115-
// ChaCha20 was introduced in OpenJDK 11.0.15 and it is not supported by JDK8.
116-
private static String[] getTLSv13Ciphers() {
117-
return new String[]{"TLS_AES_256_GCM_SHA384", "TLS_AES_128_GCM_SHA256", "TLS_CHACHA20_POLY1305_SHA256"};
118-
}
119-
120-
private static String[] getGCMCiphers() {
121-
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"};
122-
}
123-
124-
private static String[] getCBCCiphers() {
125-
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"};
126-
}
127-
128-
/**
129-
* Returns a filtered set of ciphers, where ciphers not supported by the JDK are removed.
130-
*/
131-
private static String[] getSupportedCiphers(String[]... cipherLists) {
132-
List<String> supported = Arrays.asList(
133-
((SSLServerSocketFactory) SSLServerSocketFactory.getDefault()).getSupportedCipherSuites());
134-
135-
return Arrays.stream(cipherLists).flatMap(Arrays::stream).filter(supported::contains).collect(Collectors.toList()).toArray(new String[0]);
136-
}
137-
138-
// On Java 8, prefer CBC ciphers since AES-NI support is lacking and GCM is slower than CBC.
139-
private static final String[] DEFAULT_CIPHERS_JAVA8 = getSupportedCiphers(getCBCCiphers(), getGCMCiphers(), getTLSv13Ciphers());
140-
// On Java 9 and later, prefer GCM ciphers due to improved AES-NI support.
141-
// Note that this performance assumption might not hold true for architectures other than x86_64.
142-
// TLSv1.3 ciphers can be added at the end of the list without impacting the priority of TLSv1.3 vs TLSv1.2.
143-
private static final String[] DEFAULT_CIPHERS_JAVA9 = getSupportedCiphers(getGCMCiphers(), getCBCCiphers(), getTLSv13Ciphers());
144-
145109
public static final int DEFAULT_HANDSHAKE_DETECTION_TIMEOUT_MILLIS = 5000;
146110

147111
/**
@@ -636,26 +600,6 @@ public SSLServerSocket createSSLServerSocket(int port) throws X509Exception, IOE
636600
return getDefaultSSLContextAndOptions().createSSLServerSocket(port);
637601
}
638602

639-
static String[] getDefaultCipherSuites() {
640-
return getDefaultCipherSuitesForJavaVersion(System.getProperty("java.specification.version"));
641-
}
642-
643-
static String[] getDefaultCipherSuitesForJavaVersion(String javaVersion) {
644-
Objects.requireNonNull(javaVersion);
645-
if (javaVersion.matches("\\d+")) {
646-
// Must be Java 9 or later
647-
LOG.debug("Using Java9+ optimized cipher suites for Java version {}", javaVersion);
648-
return DEFAULT_CIPHERS_JAVA9;
649-
} else if (javaVersion.startsWith("1.")) {
650-
// Must be Java 1.8 or earlier
651-
LOG.debug("Using Java8 optimized cipher suites for Java version {}", javaVersion);
652-
return DEFAULT_CIPHERS_JAVA8;
653-
} else {
654-
LOG.debug("Could not parse java version {}, using Java8 optimized cipher suites", javaVersion);
655-
return DEFAULT_CIPHERS_JAVA8;
656-
}
657-
}
658-
659603
private FileChangeWatcher newFileChangeWatcher(String fileLocation) throws IOException {
660604
if (fileLocation == null || fileLocation.isEmpty()) {
661605
return null;

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

Lines changed: 0 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -829,72 +829,6 @@ public void handshakeCompleted(HandshakeCompletedEvent handshakeCompletedEvent)
829829
});
830830
}
831831

832-
@ParameterizedTest
833-
@MethodSource("data")
834-
public void testGetDefaultCipherSuitesJava8(
835-
X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
836-
throws Exception {
837-
init(caKeyType, certKeyType, keyPassword, paramIndex);
838-
String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("1.8");
839-
// Java 8 default should have the CBC suites first
840-
assertTrue(cipherSuites[0].contains("CBC"));
841-
}
842-
843-
@ParameterizedTest
844-
@MethodSource("data")
845-
public void testGetDefaultCipherSuitesJava9(
846-
X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
847-
throws Exception {
848-
init(caKeyType, certKeyType, keyPassword, paramIndex);
849-
String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("9");
850-
// Java 9+ default should have the GCM suites first
851-
assertTrue(cipherSuites[0].contains("GCM"));
852-
}
853-
854-
@ParameterizedTest
855-
@MethodSource("data")
856-
public void testGetDefaultCipherSuitesJava10(
857-
X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
858-
throws Exception {
859-
init(caKeyType, certKeyType, keyPassword, paramIndex);
860-
String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("10");
861-
// Java 9+ default should have the GCM suites first
862-
assertTrue(cipherSuites[0].contains("GCM"));
863-
}
864-
865-
@ParameterizedTest
866-
@MethodSource("data")
867-
public void testGetDefaultCipherSuitesJava11(
868-
X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
869-
throws Exception {
870-
init(caKeyType, certKeyType, keyPassword, paramIndex);
871-
String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("11");
872-
// Java 9+ default should have the GCM suites first
873-
assertTrue(cipherSuites[0].contains("GCM"));
874-
}
875-
876-
@ParameterizedTest
877-
@MethodSource("data")
878-
public void testGetDefaultCipherSuitesUnknownVersion(
879-
X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
880-
throws Exception {
881-
init(caKeyType, certKeyType, keyPassword, paramIndex);
882-
String[] cipherSuites = X509Util.getDefaultCipherSuitesForJavaVersion("notaversion");
883-
// If version can't be parsed, use the more conservative Java 8 default
884-
assertTrue(cipherSuites[0].contains("CBC"));
885-
}
886-
887-
@ParameterizedTest
888-
@MethodSource("data")
889-
public void testGetDefaultCipherSuitesNullVersion(
890-
X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
891-
throws Exception {
892-
init(caKeyType, certKeyType, keyPassword, paramIndex);
893-
assertThrows(NullPointerException.class, () -> {
894-
X509Util.getDefaultCipherSuitesForJavaVersion(null);
895-
});
896-
}
897-
898832
// Warning: this will reset the x509Util
899833
private void setCustomCipherSuites() {
900834
System.setProperty(x509Util.getCipherSuitesProperty(), customCipherSuites[0] + "," + customCipherSuites[1]);

0 commit comments

Comments
 (0)