ctubbsii commented on code in PR #1919:
URL: https://github.com/apache/zookeeper/pull/1919#discussion_r1336181672
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java:
##########
@@ -81,7 +87,32 @@ public abstract class X509Util implements Closeable,
AutoCloseable {
}
}
- public static final String DEFAULT_PROTOCOL = "TLSv1.2";
+ public static final String DEFAULT_PROTOCOL = defaultTlsProtocol();
+
+ /**
+ * 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 = "TLSv1.2";
+ List<String> supported = new ArrayList<>();
+ try {
+ supported =
Arrays.asList(SSLContext.getDefault().getSupportedSSLParameters().getProtocols());
+ if (supported.contains("TLSv1.3")) {
+ defaultProtocol = "TLSv1.3";
+ }
+ } catch (NoSuchAlgorithmException e) {
+ // Ignore.
+ }
+ LOG.info("Default TLS protocol is {}, supported TLS protocols are {}",
defaultProtocol, supported);
+ 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"};
+ }
Review Comment:
It kind of feels wrong that ZK is hard-coding lists of ciphers to use. It
seems that users should be able to control their preferred ciphers in their
normal Java security configuration settings. One reason a user might want to do
that is to enforce a system-wide security policy.
Filtering out these lists by what is supported by the JDK also limits the
user to what is in these lists, rather than being able to use something newer
that might be available in their JDK.
##########
zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java:
##########
@@ -102,6 +106,19 @@ public void testCreateSSLContextWithoutCustomProtocol(
init(caKeyType, certKeyType, keyPassword, paramIndex);
SSLContext sslContext = x509Util.getDefaultSSLContext();
assertEquals(X509Util.DEFAULT_PROTOCOL, sslContext.getProtocol());
+
+ // Check that TLSv1.3 is selected in JDKs that support it (OpenJDK
8u272 and later).
+ List<String> supported =
Arrays.asList(SSLContext.getDefault().getSupportedSSLParameters().getProtocols());
+ if (supported.contains("TLSv1.3")) {
+ // SSLContext protocol.
+ assertEquals("TLSv1.3", sslContext.getProtocol());
+ // Enabled protocols.
+
assertThat(Arrays.asList(sslContext.getDefaultSSLParameters().getProtocols()),
+ Matchers.containsInAnyOrder(new String[] {"TLSv1.2",
"TLSv1.3"}));
Review Comment:
You don't actually need to use the hamcrest matcher here. It'd be better to
avoid it, to help phase out its use, which probably isn't needed at all with
JUnit5.
```suggestion
List<String> protos =
Arrays.asList(sslContext.getDefaultSSLParameters().getProtocols());
assertTrue(protos.contains("TLSv1.2"));
assertTrue(protos.contains("TLSv1.3"));
```
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java:
##########
@@ -90,18 +121,30 @@ 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"};
}
- 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;
+ /**
+ * Returns a filtered set of ciphers, where ciphers not supported by the
JDK are removed.
+ */
+ private static String[] getSupportedCiphers(String[]... cipherLists) {
Review Comment:
This method returns an array. It'd be a bigger change to return an
unmodifiable list, but it might be worth it in a future change, because this
gets converted back to a list later, and it's probably better to keep it as a
list here, then only convert it to an array when needed, rather than convert it
to an array now and then convert it back to a list later. That could probably
be done in a subsequent change, though, rather than making that change in this
PR.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java:
##########
@@ -81,7 +87,32 @@ public abstract class X509Util implements Closeable,
AutoCloseable {
}
}
- public static final String DEFAULT_PROTOCOL = "TLSv1.2";
+ public static final String DEFAULT_PROTOCOL = defaultTlsProtocol();
+
+ /**
+ * 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 = "TLSv1.2";
Review Comment:
There are a lot of places where "TLSv1.2" and "TLSv1.3" are used as String
literals. It'd be good to make those constants somewhere, to avoid typos.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java:
##########
@@ -90,18 +121,30 @@ 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"};
}
- 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;
+ /**
+ * Returns a filtered set of ciphers, where ciphers not supported by the
JDK are removed.
+ */
+ private static String[] getSupportedCiphers(String[]... cipherLists) {
+ Set<String> supported = new HashSet<>(Arrays.asList(
+ ((SSLServerSocketFactory)
SSLServerSocketFactory.getDefault()).getSupportedCipherSuites()));
Review Comment:
This list doesn't need to be placed inside a set. The `.contains` check will
work on the list.
```suggestion
List<String> supported = Arrays.asList(
((SSLServerSocketFactory)
SSLServerSocketFactory.getDefault()).getSupportedCipherSuites());
```
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java:
##########
@@ -90,18 +121,30 @@ 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"};
}
- 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;
+ /**
+ * Returns a filtered set of ciphers, where ciphers not supported by the
JDK are removed.
+ */
+ private static String[] getSupportedCiphers(String[]... cipherLists) {
+ Set<String> supported = new HashSet<>(Arrays.asList(
+ ((SSLServerSocketFactory)
SSLServerSocketFactory.getDefault()).getSupportedCipherSuites()));
+
+ List<String> chosen = new ArrayList<>();
+ for (String[] ciphers : cipherLists) {
+ for (String cipher : ciphers) {
+ if (supported.contains(cipher)) {
+ chosen.add(cipher);
+ }
+ }
+ }
+ return chosen.toArray(new String[0]);
Review Comment:
This is a pretty simple Stream operation that concatenates the lists,
filters on whether the elements exist in the supported list, and then collects
them into the list, all in one line (2 when wrapped):
```suggestion
return
Arrays.stream(cipherLists).flatMap(Arrays::stream).filter(supported::contains)
.collect(Collectors.toList()).toArray(new String[0]);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
