Github user ivmaykov commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/184#discussion_r194234222
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -18,64 +18,119 @@
package org.apache.zookeeper.common;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.CertPathTrustManagerParameters;
import javax.net.ssl.KeyManager;
import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLParameters;
+import javax.net.ssl.SSLServerSocket;
+import javax.net.ssl.SSLSocket;
import javax.net.ssl.TrustManager;
import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509ExtendedTrustManager;
import javax.net.ssl.X509KeyManager;
import javax.net.ssl.X509TrustManager;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
+import java.net.Socket;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.KeyManagementException;
import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.Security;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.PKIXBuilderParameters;
+import java.security.cert.X509CertSelector;
+import java.util.Arrays;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-import static
org.apache.zookeeper.common.X509Exception.KeyManagerException;
-import static
org.apache.zookeeper.common.X509Exception.SSLContextException;
-import static
org.apache.zookeeper.common.X509Exception.TrustManagerException;
+import org.apache.zookeeper.common.X509Exception.KeyManagerException;
+import org.apache.zookeeper.common.X509Exception.SSLContextException;
+import org.apache.zookeeper.common.X509Exception.TrustManagerException;
/**
* Utility code for X509 handling
*/
-public class X509Util {
+public abstract class X509Util {
private static final Logger LOG =
LoggerFactory.getLogger(X509Util.class);
- /**
- * @deprecated Use {@link ZKConfig#SSL_KEYSTORE_LOCATION}
- * instead.
- */
- @Deprecated
- public static final String SSL_KEYSTORE_LOCATION =
"zookeeper.ssl.keyStore.location";
- /**
- * @deprecated Use {@link ZKConfig#SSL_KEYSTORE_PASSWD}
- * instead.
- */
- @Deprecated
- public static final String SSL_KEYSTORE_PASSWD =
"zookeeper.ssl.keyStore.password";
- /**
- * @deprecated Use {@link ZKConfig#SSL_TRUSTSTORE_LOCATION}
- * instead.
- */
- @Deprecated
- public static final String SSL_TRUSTSTORE_LOCATION =
"zookeeper.ssl.trustStore.location";
- /**
- * @deprecated Use {@link ZKConfig#SSL_TRUSTSTORE_PASSWD}
- * instead.
- */
- @Deprecated
- public static final String SSL_TRUSTSTORE_PASSWD =
"zookeeper.ssl.trustStore.password";
- /**
- * @deprecated Use {@link ZKConfig#SSL_AUTHPROVIDER}
- * instead.
- */
- @Deprecated
- public static final String SSL_AUTHPROVIDER =
"zookeeper.ssl.authProvider";
-
- public static SSLContext createSSLContext() throws SSLContextException
{
- /**
+ static final String DEFAULT_PROTOCOL = "TLSv1.2";
+
+ private String sslProtocolProperty = getConfigPrefix() + "protocol";
+ private String cipherSuitesProperty = getConfigPrefix() +
"ciphersuites";
+ private String sslKeystoreLocationProperty = getConfigPrefix() +
"keyStore.location";
+ private String sslKeystorePasswdProperty = getConfigPrefix() +
"keyStore.password";
+ private String sslTruststoreLocationProperty = getConfigPrefix() +
"trustStore.location";
+ private String sslTruststorePasswdProperty = getConfigPrefix() +
"trustStore.password";
+ private String sslHostnameVerificationEnabledProperty =
getConfigPrefix() + "hostnameVerification";
+ private String sslCrlEnabledProperty = getConfigPrefix() + "crl";
+ private String sslOcspEnabledProperty = getConfigPrefix() + "ocsp";
+
+ private String[] cipherSuites;
--- End diff --
since we control all the server and client code, it should be safe to just
set the cipher suites to a small number of good defaults. I suggest the
following 4 suites:
- "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256"
- "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256"
- "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256"
- "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256"
The ECDHE suites have good performance and security, and AES symmetric
encryption is really the only mode that should be considered.
I also did some perf testing and found that the GCM suites perform poorly
on Java 8 under load, but perform better than CBC on java 9. In fact, GCM
overhead on java 9 was negligible compared to plaintext. So it might make sense
to list the suites in different orders based on java version. Something like:
```
if (java version < 9) {
cipherSuites = new String[]{
"TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256",
"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256",
"TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256",
"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256"};
} else {
cipherSuites = new String[]{
"TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256",
"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
"TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256",
"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256"};
}
```
---
