ZOOKEEPER-4912 Remove default TLS cipher overrides #2239
Conversation
| @@ -80,7 +80,10 @@ public SslContext createNettySslContextForClient(ZKConfig config) | |||
| } | |||
|
|
|||
| sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty())); | |||
| sslContextBuilder.protocols(getEnabledProtocols(config)); | |||
| String[] enabledProtocols = getEnabledProtocols(config); | |||
There was a problem hiding this comment.
Is this change necessary? It seems that SslContextBuilder#protocols is null-safe:
There was a problem hiding this comment.
You're right, it's not necessary.
However, I think that the if improves readability by handling the null value explicitly, and showing that we leave the defaults in this case.
I can remove it if you prefer.
There was a problem hiding this comment.
Readability is a good reason to keep this as it is. No change necessary. Thanks!
There was a problem hiding this comment.
Setting it to null explicitly and not setting it are two different things. What's the default value?
With this behaviour we will keep the default value which I think makes sense in this case.
Yes, I do. |
|
I have updated the admin guide, @cnauroth @anmolnar . We could go into more detail (like the full cipher lists for Java 8/9, or more information on how to influence that cipher selection), but I couldn't find a good place to put that information. I also couldn't find a focused doc for the relevant JVM config properties, as it's a large topic, as well as being provider dependent. Please let me know if more documentation is needed, and where I should add that. |
|
From my perspective, this looks like sufficient documentation. |
There was a problem hiding this comment.
+1
I will also follow up on the related discussion on the mailing list:
https://lists.apache.org/thread/3k5m8bx6vb4kp7j042vqlx5vqn8dwh9b
| Default: Enabled cipher suites depend on the Java runtime version being used. | ||
| Default: None, the JVM defaults are used (3.10.0+), | ||
| Enabled cipher suites are hard coded, with the ordering dependent on whether Java 8, or Java 9+ is used. | ||
| 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). |
There was a problem hiding this comment.
Shall we list the cipher suites explicitly here which were previously in the code?
I know source control preserves it for the future, but I think it could be beneficial here as well for future doc readers.
There was a problem hiding this comment.
The format of this list doesn't really allow for long readable explanations.
We could add it in a separate section maybe ?
There was a problem hiding this comment.
Sounds good to me, thanks.
There was a problem hiding this comment.
I've added a new section, @anmolnar .
| @@ -80,7 +80,10 @@ public SslContext createNettySslContextForClient(ZKConfig config) | |||
| } | |||
|
|
|||
| sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty())); | |||
| sslContextBuilder.protocols(getEnabledProtocols(config)); | |||
| String[] enabledProtocols = getEnabledProtocols(config); | |||
There was a problem hiding this comment.
Setting it to null explicitly and not setting it are two different things. What's the default value?
With this behaviour we will keep the default value which I think makes sense in this case.
No description provided.