subject:"\[GitHub\] zookeeper pull request #184\: ZOOKEEPER\-236\: SSL Support for Atomic Broadcast..."

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-04 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r222842677
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+ZKTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager, 
boolean serverHostnameVerificationEnabled,
+   boolean clientHostnameVerificationEnabled) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.serverHostnameVerificationEnabled = 
serverHostnameVerificationEnabled;
+this.clientHostnameVerificationEnabled = 
clientHostnameVerificationEnabled;
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
+if (clientHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkServerTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkServerTrusted(chain, authType, 
socket);
+if (serverHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, SSLEngine engine) throws

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-04 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r222841188
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
--- End diff --

I am +1 on @ivmaykov 's approach.

We should create a JIRA and put a comment in code links with JIRA so this 
can be cleaned up later, though this sounds a mysterious issue by the 
description so far (that performance degrades with new code path not hit?).


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-03 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r23427
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
--- End diff --

We don't have a dependency on Hadoop, so we would need to copy-and-paste 
this class too. Maybe your original approach is the quickest way to get this 
done.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-03 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r23001
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
--- End diff --

Maybe:


https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/SSLHostnameVerifier.java


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-02 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r222111859
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -84,61 +163,61 @@ public static SSLContext createSSLContext() throws 
SSLContextException {
 return createSSLContext(config);
 }
 
-public static SSLContext createSSLContext(ZKConfig config) throws 
SSLContextException {
+public SSLContext createSSLContext(ZKConfig config) throws 
SSLContextException {
 KeyManager[] keyManagers = null;
 TrustManager[] trustManagers = null;
 
-String keyStoreLocationProp = 
config.getProperty(ZKConfig.SSL_KEYSTORE_LOCATION);
-String keyStorePasswordProp = 
config.getProperty(ZKConfig.SSL_KEYSTORE_PASSWD);
+String keyStoreLocationProp = 
config.getProperty(sslKeystoreLocationProperty);
+String keyStorePasswordProp = 
config.getProperty(sslKeystorePasswdProperty);
 
 // There are legal states in some use cases for null KeyManager or 
TrustManager.
 // But if a user wanna specify one, location and password are 
required.
 
 if (keyStoreLocationProp == null && keyStorePasswordProp == null) {
-LOG.warn("keystore not specified for client connection");
+LOG.warn(getSslKeystoreLocationProperty() + " not specified");
 } else {
 if (keyStoreLocationProp == null) {
-throw new SSLContextException("keystore location not 
specified for client connection");
+throw new 
SSLContextException(getSslKeystoreLocationProperty() + " not specified");
 }
 if (keyStorePasswordProp == null) {
-throw new SSLContextException("keystore password not 
specified for client connection");
+throw new 
SSLContextException(getSslKeystorePasswdProperty() + " not specified");
 }
 try {
 keyManagers = new KeyManager[]{
 createKeyManager(keyStoreLocationProp, 
keyStorePasswordProp)};
-} catch (KeyManagerException e) {
-throw new SSLContextException("Failed to create 
KeyManager", e);
+} catch (KeyManagerException keyManagerException) {
+throw new SSLContextException("Failed to create 
KeyManager", keyManagerException);
 }
 }
 
-String trustStoreLocationProp = 
config.getProperty(ZKConfig.SSL_TRUSTSTORE_LOCATION);
-String trustStorePasswordProp = 
config.getProperty(ZKConfig.SSL_TRUSTSTORE_PASSWD);
+String trustStoreLocationProp = 
config.getProperty(sslTruststoreLocationProperty);
+String trustStorePasswordProp = 
config.getProperty(sslTruststorePasswdProperty);
 
-if (trustStoreLocationProp == null && trustStorePasswordProp == 
null) {
-LOG.warn("Truststore not specified for client connection");
+boolean sslCrlEnabled = 
config.getBoolean(this.sslCrlEnabledProperty);
+boolean sslOcspEnabled = 
config.getBoolean(this.sslOcspEnabledProperty);
+boolean sslServerHostnameVerificationEnabled = 
config.getBoolean(this.getSslHostnameVerificationEnabledProperty(), true);
+boolean sslClientHostnameVerificationEnabled = 
sslServerHostnameVerificationEnabled && shouldVerifyClientHostname();
--- End diff --

the local variable `sslServerHostnameVerificationEnabled` is read from the 
config option `getSslHostnameVerificationEnabledProperty` - the config option 
is not server-specific. So I think the confusion is from local variable naming. 
Change it to something like:

  boolean hostnameVerificationEnabled = 
config.getBoolean(this.getSslHostnameVerificationEnabledProperty(), true);
  // If hostname verification is enabled, we always verify servers
  boolean sslServerHostnameVerificationEnabled = 
hostnameVerificationEnabled;
  // We verify clients only if hostname verification is enabled in the 
config,
  // and `shouldVerifyClientHostname()` returns true.
  boolean sslClientHostnameVerificationEnabled = 
shouldVerifyClientHostname() && hostnameVerificationEnabled;


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-02 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r222110824
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
--- End diff --

We haven't checked against 4.5.6, but given that the code path wasn't even 
being taken I doubt a later version would help. I don't know enough about JVM 
performance quirks to even have a theory for how including an unused jar leads 
to a perf regression. Maybe something to do with the class loader?

Anyway, including a fully featured HTTP client library in Zookeeper's 
server code base seems weird in any case, since ZK doesn't actually use the 
HTTP client functionality for anything. If you don't want to copy-paste code, 
do you know of some smaller open source library that just provides the hostname 
verification that we could use?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-01 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221596357
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -84,61 +163,61 @@ public static SSLContext createSSLContext() throws 
SSLContextException {
 return createSSLContext(config);
 }
 
-public static SSLContext createSSLContext(ZKConfig config) throws 
SSLContextException {
+public SSLContext createSSLContext(ZKConfig config) throws 
SSLContextException {
 KeyManager[] keyManagers = null;
 TrustManager[] trustManagers = null;
 
-String keyStoreLocationProp = 
config.getProperty(ZKConfig.SSL_KEYSTORE_LOCATION);
-String keyStorePasswordProp = 
config.getProperty(ZKConfig.SSL_KEYSTORE_PASSWD);
+String keyStoreLocationProp = 
config.getProperty(sslKeystoreLocationProperty);
+String keyStorePasswordProp = 
config.getProperty(sslKeystorePasswdProperty);
 
 // There are legal states in some use cases for null KeyManager or 
TrustManager.
 // But if a user wanna specify one, location and password are 
required.
 
 if (keyStoreLocationProp == null && keyStorePasswordProp == null) {
-LOG.warn("keystore not specified for client connection");
+LOG.warn(getSslKeystoreLocationProperty() + " not specified");
 } else {
 if (keyStoreLocationProp == null) {
-throw new SSLContextException("keystore location not 
specified for client connection");
+throw new 
SSLContextException(getSslKeystoreLocationProperty() + " not specified");
 }
 if (keyStorePasswordProp == null) {
-throw new SSLContextException("keystore password not 
specified for client connection");
+throw new 
SSLContextException(getSslKeystorePasswdProperty() + " not specified");
 }
 try {
 keyManagers = new KeyManager[]{
 createKeyManager(keyStoreLocationProp, 
keyStorePasswordProp)};
-} catch (KeyManagerException e) {
-throw new SSLContextException("Failed to create 
KeyManager", e);
+} catch (KeyManagerException keyManagerException) {
+throw new SSLContextException("Failed to create 
KeyManager", keyManagerException);
 }
 }
 
-String trustStoreLocationProp = 
config.getProperty(ZKConfig.SSL_TRUSTSTORE_LOCATION);
-String trustStorePasswordProp = 
config.getProperty(ZKConfig.SSL_TRUSTSTORE_PASSWD);
+String trustStoreLocationProp = 
config.getProperty(sslTruststoreLocationProperty);
+String trustStorePasswordProp = 
config.getProperty(sslTruststorePasswdProperty);
 
-if (trustStoreLocationProp == null && trustStorePasswordProp == 
null) {
-LOG.warn("Truststore not specified for client connection");
+boolean sslCrlEnabled = 
config.getBoolean(this.sslCrlEnabledProperty);
+boolean sslOcspEnabled = 
config.getBoolean(this.sslOcspEnabledProperty);
+boolean sslServerHostnameVerificationEnabled = 
config.getBoolean(this.getSslHostnameVerificationEnabledProperty(), true);
+boolean sslClientHostnameVerificationEnabled = 
sslServerHostnameVerificationEnabled && shouldVerifyClientHostname();
--- End diff --

Very good question. I have no idea. :(
Sounds like it only makes sense this way: why would you verify client certs 
and not the servers?
@ivmaykov Does it ring a bell?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-01 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221595992
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
--- End diff --

This is a bit strange to me (copy-pasting code from other project to remove 
dependency) and needs additional review, but I don't want to block this PR 
really.

Have you checked the latest version of `HttpClient` 4.5.6? I've seen a 
number of bugfixes included in the maintenance releases. Might help.

@hanm what do you think?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-01 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221584542
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
--- End diff --

I would just simplify the comment and remove description of this logic. It 
should rather go to the documentation, because it's essentially the behaviour 
of a configuration setting.

Logic is placed in `createSSLContext`, but that doesn't have any related 
parameter.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-01 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221583890
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
--- End diff --

Your first comment makes sense, I fixed the text.
Second, place of this comment isn't the best I think. The logic actually 
implemented in `X509Util` class that creates ZKTrustManager.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-01 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221568118
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+ZKTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager, 
boolean serverHostnameVerificationEnabled,
+   boolean clientHostnameVerificationEnabled) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.serverHostnameVerificationEnabled = 
serverHostnameVerificationEnabled;
+this.clientHostnameVerificationEnabled = 
clientHostnameVerificationEnabled;
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
+if (clientHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkServerTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkServerTrusted(chain, authType, 
socket);
+if (serverHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, SSLEngine engine) throws

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-01 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221567377
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+ZKTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager, 
boolean serverHostnameVerificationEnabled,
+   boolean clientHostnameVerificationEnabled) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.serverHostnameVerificationEnabled = 
serverHostnameVerificationEnabled;
+this.clientHostnameVerificationEnabled = 
clientHostnameVerificationEnabled;
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
+if (clientHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkServerTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkServerTrusted(chain, authType, 
socket);
+if (serverHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
--- End diff --

No DNS resolution happens here, so that exception cannot be thrown.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-01 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221567351
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+ZKTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager, 
boolean serverHostnameVerificationEnabled,
+   boolean clientHostnameVerificationEnabled) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.serverHostnameVerificationEnabled = 
serverHostnameVerificationEnabled;
+this.clientHostnameVerificationEnabled = 
clientHostnameVerificationEnabled;
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
+if (clientHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
--- End diff --

No DNS resolution happens here, so that exception cannot be thrown.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-10-01 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221566036
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -160,43 +239,130 @@ public static X509KeyManager createKeyManager(String 
keyStoreLocation, String ke
 }
 throw new KeyManagerException("Couldn't find X509KeyManager");
 
-} catch (Exception e) {
-throw new KeyManagerException(e);
+} catch 
(IOException|CertificateException|UnrecoverableKeyException|NoSuchAlgorithmException|KeyStoreException
+keyManagerCreationException) {
+throw new KeyManagerException(keyManagerCreationException);
 } finally {
 if (inputStream != null) {
 try {
 inputStream.close();
-} catch (IOException e) {}
+} catch (IOException ioException) {
+LOG.info("Failed to close key store input stream", 
ioException);
+}
 }
 }
 }
 
-public static X509TrustManager createTrustManager(String 
trustStoreLocation, String trustStorePassword)
+public static X509TrustManager createTrustManager(String 
trustStoreLocation, String trustStorePassword,
+  boolean crlEnabled, 
boolean ocspEnabled,
+  final boolean 
serverHostnameVerificationEnabled,
+  final boolean 
clientHostnameVerificationEnabled)
 throws TrustManagerException {
 FileInputStream inputStream = null;
 try {
-char[] trustStorePasswordChars = 
trustStorePassword.toCharArray();
 File trustStoreFile = new File(trustStoreLocation);
 KeyStore ts = KeyStore.getInstance("JKS");
 inputStream = new FileInputStream(trustStoreFile);
-ts.load(inputStream, trustStorePasswordChars);
-TrustManagerFactory tmf = 
TrustManagerFactory.getInstance("SunX509");
-tmf.init(ts);
+if (trustStorePassword != null) {
+char[] trustStorePasswordChars = 
trustStorePassword.toCharArray();
+ts.load(inputStream, trustStorePasswordChars);
+} else {
+ts.load(inputStream, null);
+}
+
+PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, 
new X509CertSelector());
+if (crlEnabled || ocspEnabled) {
+pbParams.setRevocationEnabled(true);
+System.setProperty("com.sun.net.ssl.checkRevocation", 
"true");
+System.setProperty("com.sun.security.enableCRLDP", "true");
+if (ocspEnabled) {
+Security.setProperty("ocsp.enable", "true");
+}
+
+} else {
+pbParams.setRevocationEnabled(false);
+}
 
-for (TrustManager tm : tmf.getTrustManagers()) {
-if (tm instanceof X509TrustManager) {
-return (X509TrustManager) tm;
+// Revocation checking is only supported with the PKIX 
algorithm
+TrustManagerFactory tmf = 
TrustManagerFactory.getInstance("PKIX");
+tmf.init(new CertPathTrustManagerParameters(pbParams));
+
+for (final TrustManager tm : tmf.getTrustManagers()) {
+if (tm instanceof X509ExtendedTrustManager) {
+return new ZKTrustManager((X509ExtendedTrustManager) 
tm,
+serverHostnameVerificationEnabled, 
clientHostnameVerificationEnabled);
 }
 }
 throw new TrustManagerException("Couldn't find 
X509TrustManager");
-} catch (Exception e) {
-throw new TrustManagerException(e);
+} catch 
(IOException|CertificateException|NoSuchAlgorithmException|InvalidAlgorithmParameterException|KeyStoreException
+ trustManagerCreationException) {
+throw new TrustManagerException(trustManagerCreationException);
 } finally {
 if (inputStream != null) {
 try {
 inputStream.close();
-} catch (IOException e) {}
+} catch (IOException ioException) {
+LOG.info("failed to close TrustStore input stream", 
ioException);
+}
 }
 }
 }
-}
\ No newline at end of file
+
+public SSLSocket createSSLSocket() throws

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-28 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221368066
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
--- End diff --

replace the entire contents of this file with the same file from #627. It's 
the same code, but the `org.apache.http.conn.ssl.DefaultHostnameVerifier` class 
is copied into the file as a private inner class and renamed to 
`ZKHostnameVerifier`. Two other apache utility classes from httpclient 
(`SubjectName` and `InetAddressUtils`) are also copied in.

This resolved a >10% perf regression in our internal testing at FB. We 
don't actually know why including httpclient caused such a large perf 
regression (and strangely, the regression was present even with SSL configs 
disabled), but it's a pretty easy fix ...


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-28 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221369391
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java ---
@@ -303,6 +305,10 @@ public void parseProperties(Properties zkProp)
 } else {
 throw new ConfigException("Invalid option " + value + 
" for reconfigEnabled flag. Choose 'true' or 'false.'");
 }
+} else if (key.equals("sslQuorum")){
+sslQuorum = Boolean.parseBoolean(value);
+} else if (key.equals("portUnification")){
+shouldUsePortUnification = Boolean.parseBoolean(value);
--- End diff --

I agree with @hanm - comment out these two lines, and add a comment:

`// TODO: UnifiedServerSocket is currently buggy, will be fixed when 
@ivmaykov's PRs are merged. Disable port unification until then.`


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-28 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221367412
  
--- Diff: ivy.xml ---
@@ -63,6 +63,10 @@
   
 
 
+
--- End diff --

remove


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-28 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221367305
  
--- Diff: build.xml ---
@@ -38,11 +38,15 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
 
 
 
+
--- End diff --

As discussed, let's remove this dependency.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221101825
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+ZKTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager, 
boolean serverHostnameVerificationEnabled,
+   boolean clientHostnameVerificationEnabled) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.serverHostnameVerificationEnabled = 
serverHostnameVerificationEnabled;
+this.clientHostnameVerificationEnabled = 
clientHostnameVerificationEnabled;
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
+if (clientHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkServerTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkServerTrusted(chain, authType, 
socket);
+if (serverHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, SSLEngine engine) throws

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r220732673
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -632,37 +639,49 @@ synchronized private boolean connectOne(long sid, 
InetSocketAddress electionAddr
 
 Socket sock = null;
 try {
-LOG.debug("Opening channel to server " + sid);
-sock = new Socket();
-setSockOpts(sock);
-sock.connect(electionAddr, cnxTO);
-LOG.debug("Connected to server " + sid);
+ LOG.debug("Opening channel to server " + sid);
+ if (self.isSslQuorum()) {
+ SSLSocket sslSock = x509Util.createSSLSocket();
+ setSockOpts(sslSock);
+ sslSock.connect(electionAddr, cnxTO);
+ sslSock.startHandshake();
+ sock = sslSock;
+ } else {
+ sock = new Socket();
+ setSockOpts(sock);
+ sock.connect(electionAddr, cnxTO);
+
--- End diff --

nit: remove extra line here.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r220755770
  
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Learner.java ---
@@ -242,9 +247,8 @@ protected void sockConnect(Socket sock, 
InetSocketAddress addr, int timeout)
  * @throws InterruptedException
  */
 protected void connectToLeader(InetSocketAddress addr, String hostname)
-throws IOException, ConnectException, InterruptedException {
-sock = new Socket();
-sock.setSoTimeout(self.tickTime * self.initLimit);
+throws IOException, InterruptedException, X509Exception {
--- End diff --

nit: indentation


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221095784
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -160,43 +239,130 @@ public static X509KeyManager createKeyManager(String 
keyStoreLocation, String ke
 }
 throw new KeyManagerException("Couldn't find X509KeyManager");
 
-} catch (Exception e) {
-throw new KeyManagerException(e);
+} catch 
(IOException|CertificateException|UnrecoverableKeyException|NoSuchAlgorithmException|KeyStoreException
+keyManagerCreationException) {
+throw new KeyManagerException(keyManagerCreationException);
 } finally {
 if (inputStream != null) {
 try {
 inputStream.close();
-} catch (IOException e) {}
+} catch (IOException ioException) {
+LOG.info("Failed to close key store input stream", 
ioException);
+}
 }
 }
 }
 
-public static X509TrustManager createTrustManager(String 
trustStoreLocation, String trustStorePassword)
+public static X509TrustManager createTrustManager(String 
trustStoreLocation, String trustStorePassword,
+  boolean crlEnabled, 
boolean ocspEnabled,
+  final boolean 
serverHostnameVerificationEnabled,
+  final boolean 
clientHostnameVerificationEnabled)
 throws TrustManagerException {
 FileInputStream inputStream = null;
 try {
-char[] trustStorePasswordChars = 
trustStorePassword.toCharArray();
 File trustStoreFile = new File(trustStoreLocation);
 KeyStore ts = KeyStore.getInstance("JKS");
 inputStream = new FileInputStream(trustStoreFile);
-ts.load(inputStream, trustStorePasswordChars);
-TrustManagerFactory tmf = 
TrustManagerFactory.getInstance("SunX509");
-tmf.init(ts);
+if (trustStorePassword != null) {
+char[] trustStorePasswordChars = 
trustStorePassword.toCharArray();
+ts.load(inputStream, trustStorePasswordChars);
+} else {
+ts.load(inputStream, null);
+}
+
+PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, 
new X509CertSelector());
+if (crlEnabled || ocspEnabled) {
+pbParams.setRevocationEnabled(true);
+System.setProperty("com.sun.net.ssl.checkRevocation", 
"true");
+System.setProperty("com.sun.security.enableCRLDP", "true");
+if (ocspEnabled) {
+Security.setProperty("ocsp.enable", "true");
+}
+
+} else {
+pbParams.setRevocationEnabled(false);
+}
 
-for (TrustManager tm : tmf.getTrustManagers()) {
-if (tm instanceof X509TrustManager) {
-return (X509TrustManager) tm;
+// Revocation checking is only supported with the PKIX 
algorithm
+TrustManagerFactory tmf = 
TrustManagerFactory.getInstance("PKIX");
+tmf.init(new CertPathTrustManagerParameters(pbParams));
+
+for (final TrustManager tm : tmf.getTrustManagers()) {
+if (tm instanceof X509ExtendedTrustManager) {
+return new ZKTrustManager((X509ExtendedTrustManager) 
tm,
+serverHostnameVerificationEnabled, 
clientHostnameVerificationEnabled);
 }
 }
 throw new TrustManagerException("Couldn't find 
X509TrustManager");
-} catch (Exception e) {
-throw new TrustManagerException(e);
+} catch 
(IOException|CertificateException|NoSuchAlgorithmException|InvalidAlgorithmParameterException|KeyStoreException
+ trustManagerCreationException) {
+throw new TrustManagerException(trustManagerCreationException);
 } finally {
 if (inputStream != null) {
 try {
 inputStream.close();
-} catch (IOException e) {}
+} catch (IOException ioException) {
+LOG.info("failed to close TrustStore input stream", 
ioException);
+}
 }
 }
 }
-}
\ No newline at end of file
+
+public SSLSocket createSSLSocket() throws X509Exception,

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221101696
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+ZKTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager, 
boolean serverHostnameVerificationEnabled,
+   boolean clientHostnameVerificationEnabled) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.serverHostnameVerificationEnabled = 
serverHostnameVerificationEnabled;
+this.clientHostnameVerificationEnabled = 
clientHostnameVerificationEnabled;
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
+if (clientHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkServerTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkServerTrusted(chain, authType, 
socket);
+if (serverHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, SSLEngine engine) throws

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221102292
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
--- End diff --

also, the comment is saying client host verification depends on both 
`clientHostnameVerificationEnabled` and another condition, but the verification 
code later in this file only checks `clientHostnameVerificationEnabled`.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r220754469
  
--- Diff: src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 
---
@@ -50,6 +50,7 @@
 import org.apache.zookeeper.Watcher;
 import org.apache.zookeeper.Watcher.Event.EventType;
 import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.common.X509Exception;
--- End diff --

unused import - so this file can be excluded from the change list.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221087073
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -84,61 +163,61 @@ public static SSLContext createSSLContext() throws 
SSLContextException {
 return createSSLContext(config);
 }
 
-public static SSLContext createSSLContext(ZKConfig config) throws 
SSLContextException {
+public SSLContext createSSLContext(ZKConfig config) throws 
SSLContextException {
 KeyManager[] keyManagers = null;
 TrustManager[] trustManagers = null;
 
-String keyStoreLocationProp = 
config.getProperty(ZKConfig.SSL_KEYSTORE_LOCATION);
-String keyStorePasswordProp = 
config.getProperty(ZKConfig.SSL_KEYSTORE_PASSWD);
+String keyStoreLocationProp = 
config.getProperty(sslKeystoreLocationProperty);
+String keyStorePasswordProp = 
config.getProperty(sslKeystorePasswdProperty);
 
 // There are legal states in some use cases for null KeyManager or 
TrustManager.
 // But if a user wanna specify one, location and password are 
required.
 
 if (keyStoreLocationProp == null && keyStorePasswordProp == null) {
-LOG.warn("keystore not specified for client connection");
+LOG.warn(getSslKeystoreLocationProperty() + " not specified");
 } else {
 if (keyStoreLocationProp == null) {
-throw new SSLContextException("keystore location not 
specified for client connection");
+throw new 
SSLContextException(getSslKeystoreLocationProperty() + " not specified");
 }
 if (keyStorePasswordProp == null) {
-throw new SSLContextException("keystore password not 
specified for client connection");
+throw new 
SSLContextException(getSslKeystorePasswdProperty() + " not specified");
 }
 try {
 keyManagers = new KeyManager[]{
 createKeyManager(keyStoreLocationProp, 
keyStorePasswordProp)};
-} catch (KeyManagerException e) {
-throw new SSLContextException("Failed to create 
KeyManager", e);
+} catch (KeyManagerException keyManagerException) {
+throw new SSLContextException("Failed to create 
KeyManager", keyManagerException);
 }
 }
 
-String trustStoreLocationProp = 
config.getProperty(ZKConfig.SSL_TRUSTSTORE_LOCATION);
-String trustStorePasswordProp = 
config.getProperty(ZKConfig.SSL_TRUSTSTORE_PASSWD);
+String trustStoreLocationProp = 
config.getProperty(sslTruststoreLocationProperty);
+String trustStorePasswordProp = 
config.getProperty(sslTruststorePasswdProperty);
 
-if (trustStoreLocationProp == null && trustStorePasswordProp == 
null) {
-LOG.warn("Truststore not specified for client connection");
+boolean sslCrlEnabled = 
config.getBoolean(this.sslCrlEnabledProperty);
+boolean sslOcspEnabled = 
config.getBoolean(this.sslOcspEnabledProperty);
+boolean sslServerHostnameVerificationEnabled = 
config.getBoolean(this.getSslHostnameVerificationEnabledProperty(), true);
+boolean sslClientHostnameVerificationEnabled = 
sslServerHostnameVerificationEnabled && shouldVerifyClientHostname();
--- End diff --

is there a particular reason that `sslClientHostnameVerificationEnabled` 
depends on `sslServerHostnameVerificationEnabled`, instead of just depends on 
`shouldVerifyClientHostname()` ?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221104823
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java ---
@@ -303,6 +305,10 @@ public void parseProperties(Properties zkProp)
 } else {
 throw new ConfigException("Invalid option " + value + 
" for reconfigEnabled flag. Choose 'true' or 'false.'");
 }
+} else if (key.equals("sslQuorum")){
+sslQuorum = Boolean.parseBoolean(value);
+} else if (key.equals("portUnification")){
+shouldUsePortUnification = Boolean.parseBoolean(value);
--- End diff --

i think we need to disable the parsing code here for this patch, right?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r220750938
  
--- Diff: src/java/test/org/apache/zookeeper/test/QuorumSSLTest.java ---
@@ -0,0 +1,743 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
--- End diff --

shouldn't this be `` (we already have a closing tag ``)? 


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221102101
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+ZKTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager, 
boolean serverHostnameVerificationEnabled,
+   boolean clientHostnameVerificationEnabled) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.serverHostnameVerificationEnabled = 
serverHostnameVerificationEnabled;
+this.clientHostnameVerificationEnabled = 
clientHostnameVerificationEnabled;
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
+if (clientHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkServerTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkServerTrusted(chain, authType, 
socket);
+if (serverHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, SSLEngine engine) throws

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r220751460
  
--- Diff: src/java/test/org/apache/zookeeper/test/QuorumSSLTest.java ---
@@ -0,0 +1,743 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.test;
+
+import com.sun.net.httpserver.Headers;
+import com.sun.net.httpserver.HttpHandler;
+import com.sun.net.httpserver.HttpServer;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.client.ZKClientConfig;
+import org.apache.zookeeper.common.QuorumX509Util;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
+import org.bouncycastle.asn1.ocsp.OCSPResponse;
+import org.bouncycastle.asn1.ocsp.OCSPResponseStatus;
+import org.bouncycastle.asn1.x500.X500Name;
+import org.bouncycastle.asn1.x500.X500NameBuilder;
+import org.bouncycastle.asn1.x500.style.BCStyle;
+import org.bouncycastle.asn1.x509.AuthorityInformationAccess;
+import org.bouncycastle.asn1.x509.BasicConstraints;
+import org.bouncycastle.asn1.x509.CRLDistPoint;
+import org.bouncycastle.asn1.x509.CRLNumber;
+import org.bouncycastle.asn1.x509.CRLReason;
+import org.bouncycastle.asn1.x509.DistributionPoint;
+import org.bouncycastle.asn1.x509.DistributionPointName;
+import org.bouncycastle.asn1.x509.Extension;
+import org.bouncycastle.asn1.x509.GeneralName;
+import org.bouncycastle.asn1.x509.GeneralNames;
+import org.bouncycastle.asn1.x509.KeyUsage;
+import org.bouncycastle.asn1.x509.SubjectPublicKeyInfo;
+import org.bouncycastle.asn1.x509.X509ObjectIdentifiers;
+import org.bouncycastle.cert.X509CRLHolder;
+import org.bouncycastle.cert.X509CertificateHolder;
+import org.bouncycastle.cert.X509ExtensionUtils;
+import org.bouncycastle.cert.X509v2CRLBuilder;
+import org.bouncycastle.cert.X509v3CertificateBuilder;
+import org.bouncycastle.cert.bc.BcX509ExtensionUtils;
+import org.bouncycastle.cert.jcajce.JcaX509CertificateConverter;
+import org.bouncycastle.cert.jcajce.JcaX509CertificateHolder;
+import org.bouncycastle.cert.jcajce.JcaX509ExtensionUtils;
+import org.bouncycastle.cert.jcajce.JcaX509v2CRLBuilder;
+import org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder;
+import org.bouncycastle.cert.ocsp.BasicOCSPResp;
+import org.bouncycastle.cert.ocsp.BasicOCSPRespBuilder;
+import org.bouncycastle.cert.ocsp.CertificateID;
+import org.bouncycastle.cert.ocsp.CertificateStatus;
+import org.bouncycastle.cert.ocsp.OCSPException;
+import org.bouncycastle.cert.ocsp.OCSPReq;
+import org.bouncycastle.cert.ocsp.OCSPResp;
+import org.bouncycastle.cert.ocsp.OCSPRespBuilder;
+import org.bouncycastle.cert.ocsp.Req;
+import org.bouncycastle.cert.ocsp.UnknownStatus;
+import org.bouncycastle.cert.ocsp.jcajce.JcaBasicOCSPRespBuilder;
+import org.bouncycastle.cert.ocsp.jcajce.JcaCertificateID;
+import org.bouncycastle.crypto.util.PublicKeyFactory;
+import org.bouncycastle.crypto.util.SubjectPublicKeyInfoFactory;
+import org.bouncycastle.jce.provider.BouncyCastleProvider;
+import org.bouncycastle.openssl.MiscPEMGenerator;
+import org.bouncycastle.operator.ContentSigner;
+import org.bouncycastle.operator.DigestCalculator;
+import org.bouncycastle.operator.OperatorException;
+import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder;
+import org.bouncycastle.operator.jcajce.JcaDigestCalculatorProviderBuilder;
+import org.bouncycastle.util.io.pem.PemWriter;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+
+import javax.net.ssl.SSLServerSocketFactory;
+import java.io.FileOutputStream;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.math.BigInteger;
+import

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r220754714
  
--- Diff: 
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerConfigTest.java ---
@@ -26,6 +26,8 @@
 import java.io.IOException;
 import java.util.Properties;
 
+import org.apache.zookeeper.common.ClientX509Util;
+import org.apache.zookeeper.common.X509Util;
 import org.apache.zookeeper.common.ZKConfig;
--- End diff --

`X509Util` and `ZKConfig` can be removed.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r220733105
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -632,37 +639,49 @@ synchronized private boolean connectOne(long sid, 
InetSocketAddress electionAddr
 
 Socket sock = null;
 try {
-LOG.debug("Opening channel to server " + sid);
-sock = new Socket();
-setSockOpts(sock);
-sock.connect(electionAddr, cnxTO);
-LOG.debug("Connected to server " + sid);
+ LOG.debug("Opening channel to server " + sid);
+ if (self.isSslQuorum()) {
+ SSLSocket sslSock = x509Util.createSSLSocket();
+ setSockOpts(sslSock);
+ sslSock.connect(electionAddr, cnxTO);
+ sslSock.startHandshake();
+ sock = sslSock;
+ } else {
+ sock = new Socket();
+ setSockOpts(sock);
+ sock.connect(electionAddr, cnxTO);
+
+ }
+ LOG.debug("Connected to server " + sid);
 // Sends connection request asynchronously if the quorum
 // sasl authentication is enabled. This is required because
 // sasl server authentication process may take few seconds to
 // finish, this may delay next peer connection requests.
 if (quorumSaslAuthEnabled) {
 initiateConnectionAsync(sock, sid);
-} else {
-initiateConnection(sock, sid);
-}
-return true;
-} catch (UnresolvedAddressException e) {
-// Sun doesn't include the address that causes this
-// exception to be thrown, also UAE cannot be wrapped cleanly
-// so we log the exception in order to capture this critical
-// detail.
-LOG.warn("Cannot open channel to " + sid
-+ " at election address " + electionAddr, e);
-closeSocket(sock);
-throw e;
-} catch (IOException e) {
-LOG.warn("Cannot open channel to " + sid
-+ " at election address " + electionAddr,
-e);
+} else { initiateConnection(sock, sid);
+} return true;
+ } catch (UnresolvedAddressException e) {
+ // Sun doesn't include the address that causes this
--- End diff --

nit: indentations 


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r220755547
  
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -225,19 +227,36 @@ public boolean isQuorumSynced(QuorumVerifier qv) {
 
 private final ServerSocket ss;
 
-Leader(QuorumPeer self,LeaderZooKeeperServer zk) throws IOException {
+Leader(QuorumPeer self,LeaderZooKeeperServer zk) throws IOException, 
X509Exception {
 this.self = self;
 this.proposalStats = new BufferStats();
 try {
-if (self.getQuorumListenOnAllIPs()) {
-ss = new ServerSocket(self.getQuorumAddress().getPort());
+if (self.shouldUsePortUnification()) {
+if (self.getQuorumListenOnAllIPs()) {
+ss = new UnifiedServerSocket(new QuorumX509Util(), 
self.getQuorumAddress().getPort());
+} else {
+ss = new UnifiedServerSocket(new QuorumX509Util());
+}
+} else if (self.isSslQuorum()) {
+if (self.getQuorumListenOnAllIPs()) {
+ss = new 
QuorumX509Util().createSSLServerSocket(self.getQuorumAddress().getPort());
+} else {
+ss = new QuorumX509Util().createSSLServerSocket();
+}
 } else {
-ss = new ServerSocket();
+if (self.getQuorumListenOnAllIPs()) {
+ss = new 
ServerSocket(self.getQuorumAddress().getPort());
+} else {
+ss = new ServerSocket();
+}
 }
 ss.setReuseAddress(true);
 if (!self.getQuorumListenOnAllIPs()) {
 ss.bind(self.getQuorumAddress());
 }
+} catch (X509Exception e) {
+LOG.error("failed to setup ssl server socket", e);
--- End diff --

nit: `Failed to` 


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221101711
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+ZKTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager, 
boolean serverHostnameVerificationEnabled,
+   boolean clientHostnameVerificationEnabled) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.serverHostnameVerificationEnabled = 
serverHostnameVerificationEnabled;
+this.clientHostnameVerificationEnabled = 
clientHostnameVerificationEnabled;
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
+if (clientHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkServerTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkServerTrusted(chain, authType, 
socket);
+if (serverHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, SSLEngine engine) throws

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221101593
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+ZKTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager, 
boolean serverHostnameVerificationEnabled,
+   boolean clientHostnameVerificationEnabled) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.serverHostnameVerificationEnabled = 
serverHostnameVerificationEnabled;
+this.clientHostnameVerificationEnabled = 
clientHostnameVerificationEnabled;
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
+if (clientHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
--- End diff --

should we catch `UnknownHostException` here, similar as what it's doing in 
the overloaded version?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r220733326
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -632,37 +639,49 @@ synchronized private boolean connectOne(long sid, 
InetSocketAddress electionAddr
 
 Socket sock = null;
 try {
-LOG.debug("Opening channel to server " + sid);
-sock = new Socket();
-setSockOpts(sock);
-sock.connect(electionAddr, cnxTO);
-LOG.debug("Connected to server " + sid);
+ LOG.debug("Opening channel to server " + sid);
+ if (self.isSslQuorum()) {
+ SSLSocket sslSock = x509Util.createSSLSocket();
+ setSockOpts(sslSock);
+ sslSock.connect(electionAddr, cnxTO);
+ sslSock.startHandshake();
+ sock = sslSock;
+ } else {
+ sock = new Socket();
+ setSockOpts(sock);
+ sock.connect(electionAddr, cnxTO);
+
+ }
+ LOG.debug("Connected to server " + sid);
 // Sends connection request asynchronously if the quorum
 // sasl authentication is enabled. This is required because
 // sasl server authentication process may take few seconds to
 // finish, this may delay next peer connection requests.
 if (quorumSaslAuthEnabled) {
 initiateConnectionAsync(sock, sid);
-} else {
-initiateConnection(sock, sid);
-}
-return true;
-} catch (UnresolvedAddressException e) {
-// Sun doesn't include the address that causes this
-// exception to be thrown, also UAE cannot be wrapped cleanly
-// so we log the exception in order to capture this critical
-// detail.
-LOG.warn("Cannot open channel to " + sid
-+ " at election address " + electionAddr, e);
-closeSocket(sock);
-throw e;
-} catch (IOException e) {
-LOG.warn("Cannot open channel to " + sid
-+ " at election address " + electionAddr,
-e);
+} else { initiateConnection(sock, sid);
+} return true;
+ } catch (UnresolvedAddressException e) {
+ // Sun doesn't include the address that causes this
+ // exception to be thrown, also UAE cannot be wrapped cleanly
+ // so we log the exception in order to capture this critical
+ // detail.
+ LOG.warn("Cannot open channel to " + sid
+ + " at election address " + electionAddr, e);
+ closeSocket(sock);
+ throw e;} catch (X509Exception e) {
--- End diff --

nit: start a new line for `} catch (X509Exception e) {`


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221096682
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
--- End diff --

what is `verifySSLServerHostname`? should this be 
`serverHostnameVerificationEnabled`?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221095909
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -160,43 +239,130 @@ public static X509KeyManager createKeyManager(String 
keyStoreLocation, String ke
 }
 throw new KeyManagerException("Couldn't find X509KeyManager");
 
-} catch (Exception e) {
-throw new KeyManagerException(e);
+} catch 
(IOException|CertificateException|UnrecoverableKeyException|NoSuchAlgorithmException|KeyStoreException
+keyManagerCreationException) {
+throw new KeyManagerException(keyManagerCreationException);
 } finally {
 if (inputStream != null) {
 try {
 inputStream.close();
-} catch (IOException e) {}
+} catch (IOException ioException) {
+LOG.info("Failed to close key store input stream", 
ioException);
+}
 }
 }
 }
 
-public static X509TrustManager createTrustManager(String 
trustStoreLocation, String trustStorePassword)
+public static X509TrustManager createTrustManager(String 
trustStoreLocation, String trustStorePassword,
+  boolean crlEnabled, 
boolean ocspEnabled,
+  final boolean 
serverHostnameVerificationEnabled,
+  final boolean 
clientHostnameVerificationEnabled)
 throws TrustManagerException {
 FileInputStream inputStream = null;
 try {
-char[] trustStorePasswordChars = 
trustStorePassword.toCharArray();
 File trustStoreFile = new File(trustStoreLocation);
 KeyStore ts = KeyStore.getInstance("JKS");
 inputStream = new FileInputStream(trustStoreFile);
-ts.load(inputStream, trustStorePasswordChars);
-TrustManagerFactory tmf = 
TrustManagerFactory.getInstance("SunX509");
-tmf.init(ts);
+if (trustStorePassword != null) {
+char[] trustStorePasswordChars = 
trustStorePassword.toCharArray();
+ts.load(inputStream, trustStorePasswordChars);
+} else {
+ts.load(inputStream, null);
+}
+
+PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, 
new X509CertSelector());
+if (crlEnabled || ocspEnabled) {
+pbParams.setRevocationEnabled(true);
+System.setProperty("com.sun.net.ssl.checkRevocation", 
"true");
+System.setProperty("com.sun.security.enableCRLDP", "true");
+if (ocspEnabled) {
+Security.setProperty("ocsp.enable", "true");
+}
+
+} else {
+pbParams.setRevocationEnabled(false);
+}
 
-for (TrustManager tm : tmf.getTrustManagers()) {
-if (tm instanceof X509TrustManager) {
-return (X509TrustManager) tm;
+// Revocation checking is only supported with the PKIX 
algorithm
+TrustManagerFactory tmf = 
TrustManagerFactory.getInstance("PKIX");
+tmf.init(new CertPathTrustManagerParameters(pbParams));
+
+for (final TrustManager tm : tmf.getTrustManagers()) {
+if (tm instanceof X509ExtendedTrustManager) {
+return new ZKTrustManager((X509ExtendedTrustManager) 
tm,
+serverHostnameVerificationEnabled, 
clientHostnameVerificationEnabled);
 }
 }
 throw new TrustManagerException("Couldn't find 
X509TrustManager");
-} catch (Exception e) {
-throw new TrustManagerException(e);
+} catch 
(IOException|CertificateException|NoSuchAlgorithmException|InvalidAlgorithmParameterException|KeyStoreException
+ trustManagerCreationException) {
+throw new TrustManagerException(trustManagerCreationException);
 } finally {
 if (inputStream != null) {
 try {
 inputStream.close();
-} catch (IOException e) {}
+} catch (IOException ioException) {
+LOG.info("failed to close TrustStore input stream", 
ioException);
+}
 }
 }
 }
-}
\ No newline at end of file
+
+public SSLSocket createSSLSocket() throws X509Exception,

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r220752510
  
--- Diff: src/java/test/org/apache/zookeeper/test/QuorumSSLTest.java ---
@@ -0,0 +1,743 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.test;
+
+import com.sun.net.httpserver.Headers;
+import com.sun.net.httpserver.HttpHandler;
+import com.sun.net.httpserver.HttpServer;
+import org.apache.zookeeper.PortAssignment;
+import org.apache.zookeeper.client.ZKClientConfig;
+import org.apache.zookeeper.common.QuorumX509Util;
+import org.apache.zookeeper.server.ServerCnxnFactory;
+import org.apache.zookeeper.server.quorum.QuorumPeerTestBase;
+import org.bouncycastle.asn1.ocsp.OCSPResponse;
+import org.bouncycastle.asn1.ocsp.OCSPResponseStatus;
+import org.bouncycastle.asn1.x500.X500Name;
+import org.bouncycastle.asn1.x500.X500NameBuilder;
+import org.bouncycastle.asn1.x500.style.BCStyle;
+import org.bouncycastle.asn1.x509.AuthorityInformationAccess;
+import org.bouncycastle.asn1.x509.BasicConstraints;
+import org.bouncycastle.asn1.x509.CRLDistPoint;
+import org.bouncycastle.asn1.x509.CRLNumber;
+import org.bouncycastle.asn1.x509.CRLReason;
+import org.bouncycastle.asn1.x509.DistributionPoint;
+import org.bouncycastle.asn1.x509.DistributionPointName;
+import org.bouncycastle.asn1.x509.Extension;
+import org.bouncycastle.asn1.x509.GeneralName;
+import org.bouncycastle.asn1.x509.GeneralNames;
+import org.bouncycastle.asn1.x509.KeyUsage;
+import org.bouncycastle.asn1.x509.SubjectPublicKeyInfo;
+import org.bouncycastle.asn1.x509.X509ObjectIdentifiers;
+import org.bouncycastle.cert.X509CRLHolder;
+import org.bouncycastle.cert.X509CertificateHolder;
+import org.bouncycastle.cert.X509ExtensionUtils;
+import org.bouncycastle.cert.X509v2CRLBuilder;
+import org.bouncycastle.cert.X509v3CertificateBuilder;
+import org.bouncycastle.cert.bc.BcX509ExtensionUtils;
+import org.bouncycastle.cert.jcajce.JcaX509CertificateConverter;
+import org.bouncycastle.cert.jcajce.JcaX509CertificateHolder;
+import org.bouncycastle.cert.jcajce.JcaX509ExtensionUtils;
+import org.bouncycastle.cert.jcajce.JcaX509v2CRLBuilder;
+import org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder;
+import org.bouncycastle.cert.ocsp.BasicOCSPResp;
+import org.bouncycastle.cert.ocsp.BasicOCSPRespBuilder;
+import org.bouncycastle.cert.ocsp.CertificateID;
+import org.bouncycastle.cert.ocsp.CertificateStatus;
+import org.bouncycastle.cert.ocsp.OCSPException;
+import org.bouncycastle.cert.ocsp.OCSPReq;
+import org.bouncycastle.cert.ocsp.OCSPResp;
+import org.bouncycastle.cert.ocsp.OCSPRespBuilder;
+import org.bouncycastle.cert.ocsp.Req;
+import org.bouncycastle.cert.ocsp.UnknownStatus;
+import org.bouncycastle.cert.ocsp.jcajce.JcaBasicOCSPRespBuilder;
+import org.bouncycastle.cert.ocsp.jcajce.JcaCertificateID;
+import org.bouncycastle.crypto.util.PublicKeyFactory;
+import org.bouncycastle.crypto.util.SubjectPublicKeyInfoFactory;
+import org.bouncycastle.jce.provider.BouncyCastleProvider;
+import org.bouncycastle.openssl.MiscPEMGenerator;
+import org.bouncycastle.operator.ContentSigner;
+import org.bouncycastle.operator.DigestCalculator;
+import org.bouncycastle.operator.OperatorException;
+import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder;
+import org.bouncycastle.operator.jcajce.JcaDigestCalculatorProviderBuilder;
+import org.bouncycastle.util.io.pem.PemWriter;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.Timeout;
+
+import javax.net.ssl.SSLServerSocketFactory;
+import java.io.FileOutputStream;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.math.BigInteger;
+import

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221069516
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -534,14 +543,12 @@ private void handleConnection(Socket sock, 
DataInputStream din)
 LOG.info("Setting arbitrary identifier to observer: " + 
sid);
 }
 } catch (IOException e) {
+LOG.warn("Exception reading or writing challenge: {}", e);
 closeSocket(sock);
-LOG.warn("Exception reading or writing challenge: {}", 
e.toString());
 return;
 }
-
--- End diff --

nit: keep this empty line.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r220732624
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -632,37 +639,49 @@ synchronized private boolean connectOne(long sid, 
InetSocketAddress electionAddr
 
 Socket sock = null;
 try {
-LOG.debug("Opening channel to server " + sid);
-sock = new Socket();
-setSockOpts(sock);
-sock.connect(electionAddr, cnxTO);
-LOG.debug("Connected to server " + sid);
+ LOG.debug("Opening channel to server " + sid);
--- End diff --

nit: indentations on this line and the following lines


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221101806
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+ZKTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager, 
boolean serverHostnameVerificationEnabled,
+   boolean clientHostnameVerificationEnabled) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.serverHostnameVerificationEnabled = 
serverHostnameVerificationEnabled;
+this.clientHostnameVerificationEnabled = 
clientHostnameVerificationEnabled;
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
+if (clientHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkServerTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkServerTrusted(chain, authType, 
socket);
+if (serverHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, SSLEngine engine) throws

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221088959
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -160,43 +239,130 @@ public static X509KeyManager createKeyManager(String 
keyStoreLocation, String ke
 }
 throw new KeyManagerException("Couldn't find X509KeyManager");
 
-} catch (Exception e) {
-throw new KeyManagerException(e);
+} catch 
(IOException|CertificateException|UnrecoverableKeyException|NoSuchAlgorithmException|KeyStoreException
+keyManagerCreationException) {
+throw new KeyManagerException(keyManagerCreationException);
 } finally {
 if (inputStream != null) {
 try {
 inputStream.close();
-} catch (IOException e) {}
+} catch (IOException ioException) {
+LOG.info("Failed to close key store input stream", 
ioException);
+}
 }
 }
 }
 
-public static X509TrustManager createTrustManager(String 
trustStoreLocation, String trustStorePassword)
+public static X509TrustManager createTrustManager(String 
trustStoreLocation, String trustStorePassword,
+  boolean crlEnabled, 
boolean ocspEnabled,
+  final boolean 
serverHostnameVerificationEnabled,
+  final boolean 
clientHostnameVerificationEnabled)
 throws TrustManagerException {
 FileInputStream inputStream = null;
 try {
-char[] trustStorePasswordChars = 
trustStorePassword.toCharArray();
 File trustStoreFile = new File(trustStoreLocation);
 KeyStore ts = KeyStore.getInstance("JKS");
 inputStream = new FileInputStream(trustStoreFile);
-ts.load(inputStream, trustStorePasswordChars);
-TrustManagerFactory tmf = 
TrustManagerFactory.getInstance("SunX509");
-tmf.init(ts);
+if (trustStorePassword != null) {
+char[] trustStorePasswordChars = 
trustStorePassword.toCharArray();
+ts.load(inputStream, trustStorePasswordChars);
+} else {
+ts.load(inputStream, null);
+}
+
+PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, 
new X509CertSelector());
+if (crlEnabled || ocspEnabled) {
+pbParams.setRevocationEnabled(true);
+System.setProperty("com.sun.net.ssl.checkRevocation", 
"true");
+System.setProperty("com.sun.security.enableCRLDP", "true");
+if (ocspEnabled) {
+Security.setProperty("ocsp.enable", "true");
+}
+
--- End diff --

nit: remove extra line.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-09-27 Thread hanm

Github user hanm commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r221101612
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean serverHostnameVerificationEnabled;
+private boolean clientHostnameVerificationEnabled;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled  If true, this 
TrustManager should verify hostnames of servers that this
+ * instance connects to.
+ * @param clientHostnameVerificationEnabled  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+ZKTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager, 
boolean serverHostnameVerificationEnabled,
+   boolean clientHostnameVerificationEnabled) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.serverHostnameVerificationEnabled = 
serverHostnameVerificationEnabled;
+this.clientHostnameVerificationEnabled = 
clientHostnameVerificationEnabled;
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
+if (clientHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+}
+
+@Override
+public void checkServerTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+x509ExtendedTrustManager.checkServerTrusted(chain, authType, 
socket);
+if (serverHostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
--- End diff --

should we catch `UnknownHostException` here, similar as what it's doing in 
the overloaded version?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-17 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195935031
  
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -227,19 +229,36 @@ public boolean isQuorumSynced(QuorumVerifier qv) {
 
 private final ServerSocket ss;
 
-Leader(QuorumPeer self,LeaderZooKeeperServer zk) throws IOException {
+Leader(QuorumPeer self,LeaderZooKeeperServer zk) throws IOException, 
X509Exception {
 this.self = self;
 this.proposalStats = new ProposalStats();
 try {
-if (self.getQuorumListenOnAllIPs()) {
-ss = new ServerSocket(self.getQuorumAddress().getPort());
+if (self.shouldUsePortUnification()) {
+if (self.getQuorumListenOnAllIPs()) {
+ss = new UnifiedServerSocket(new QuorumX509Util(), 
self.getQuorumAddress().getPort());
+} else {
+ss = new UnifiedServerSocket(new QuorumX509Util());
+}
+} else if (self.isSslQuorum()) {
--- End diff --

Don't worry too much. It was useful to get a clear understanding on what's 
going on. Documenting it would be great of course. There's a separate PR for 
the documentation, we'll cover that once this one is in.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-15 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195891467
  
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -227,19 +229,36 @@ public boolean isQuorumSynced(QuorumVerifier qv) {
 
 private final ServerSocket ss;
 
-Leader(QuorumPeer self,LeaderZooKeeperServer zk) throws IOException {
+Leader(QuorumPeer self,LeaderZooKeeperServer zk) throws IOException, 
X509Exception {
 this.self = self;
 this.proposalStats = new ProposalStats();
 try {
-if (self.getQuorumListenOnAllIPs()) {
-ss = new ServerSocket(self.getQuorumAddress().getPort());
+if (self.shouldUsePortUnification()) {
+if (self.getQuorumListenOnAllIPs()) {
+ss = new UnifiedServerSocket(new QuorumX509Util(), 
self.getQuorumAddress().getPort());
+} else {
+ss = new UnifiedServerSocket(new QuorumX509Util());
+}
+} else if (self.isSslQuorum()) {
--- End diff --

Is the behavior of `sslQuorum` and `portUnification` documented? If not, we 
should document it somewhere, as well as the upgrade procedure for taking a 
plaintext cluster and making it use SSL.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-15 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195891451
  
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -227,19 +229,36 @@ public boolean isQuorumSynced(QuorumVerifier qv) {
 
 private final ServerSocket ss;
 
-Leader(QuorumPeer self,LeaderZooKeeperServer zk) throws IOException {
+Leader(QuorumPeer self,LeaderZooKeeperServer zk) throws IOException, 
X509Exception {
 this.self = self;
 this.proposalStats = new ProposalStats();
 try {
-if (self.getQuorumListenOnAllIPs()) {
-ss = new ServerSocket(self.getQuorumAddress().getPort());
+if (self.shouldUsePortUnification()) {
+if (self.getQuorumListenOnAllIPs()) {
+ss = new UnifiedServerSocket(new QuorumX509Util(), 
self.getQuorumAddress().getPort());
+} else {
+ss = new UnifiedServerSocket(new QuorumX509Util());
+}
+} else if (self.isSslQuorum()) {
--- End diff --

Ok, makes sense. Sorry about wasting your time with a bad code review 
proposal.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-15 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195779559
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java ---
@@ -64,6 +66,8 @@
 
 protected InetSocketAddress clientPortAddress;
 protected InetSocketAddress secureClientPortAddress;
+protected boolean sslQuorum = false;
+protected boolean shouldUsePortUnification = false;
--- End diff --

@breed Do you think it would be useful to set these properties via JMX or 
should it be just read-only?

I think it would be also great to expose the properties via Jetty `stat` 
command as well. It makes it easier to figure out which instance is already 
SSL/Unification enabled.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-15 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195724314
  
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -227,19 +229,36 @@ public boolean isQuorumSynced(QuorumVerifier qv) {
 
 private final ServerSocket ss;
 
-Leader(QuorumPeer self,LeaderZooKeeperServer zk) throws IOException {
+Leader(QuorumPeer self,LeaderZooKeeperServer zk) throws IOException, 
X509Exception {
 this.self = self;
 this.proposalStats = new ProposalStats();
 try {
-if (self.getQuorumListenOnAllIPs()) {
-ss = new ServerSocket(self.getQuorumAddress().getPort());
+if (self.shouldUsePortUnification()) {
+if (self.getQuorumListenOnAllIPs()) {
+ss = new UnifiedServerSocket(new QuorumX509Util(), 
self.getQuorumAddress().getPort());
+} else {
+ss = new UnifiedServerSocket(new QuorumX509Util());
+}
+} else if (self.isSslQuorum()) {
--- End diff --

I just tried to change it in the way you suggested and it got broken. The 
idea is the following:

- when you turn on `sslQuorum`, it means that the peer initiates SSL 
connection when trying to connect other quorum members and at the same time 
accepts SSL connections *only*,
- when you turn on `portUnification` it means that peer *accepts* both SSL 
and non-SSL connections, but still tries to initiate non-SSL connections,
- if both of them false, peer initiates and accepts non-SSL connection only.

Upgrade path is the following:
1. Quorum runs with non-SSL connections,
2. Rolling upgrade nodes by adding `portUnification=true`: peers still 
communicate without SSL, but able to accept SSL connections as well,
3. Rolling upgrade nodes by adding `sslQuorum=true`: peers accept both 
connections, but initiates with SSL,
4. Rolling upgrade nodes by removing `portUnification`: peers accept and 
initiate SSL connections only.

`QuorumSSLTest.testRollingUpgrade` covers the scenario.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-15 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195667693
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKConfig.java ---
@@ -107,14 +99,33 @@ private void init() {
  * this configuration.
  */
 protected void handleBackwardCompatibility() {
-properties.put(SSL_KEYSTORE_LOCATION, 
System.getProperty(SSL_KEYSTORE_LOCATION));
-properties.put(SSL_KEYSTORE_PASSWD, 
System.getProperty(SSL_KEYSTORE_PASSWD));
-properties.put(SSL_TRUSTSTORE_LOCATION, 
System.getProperty(SSL_TRUSTSTORE_LOCATION));
-properties.put(SSL_TRUSTSTORE_PASSWD, 
System.getProperty(SSL_TRUSTSTORE_PASSWD));
-properties.put(SSL_AUTHPROVIDER, 
System.getProperty(SSL_AUTHPROVIDER));
 properties.put(JUTE_MAXBUFFER, System.getProperty(JUTE_MAXBUFFER));
 properties.put(KINIT_COMMAND, System.getProperty(KINIT_COMMAND));
 properties.put(JGSS_NATIVE, System.getProperty(JGSS_NATIVE));
+
+ClientX509Util clientX509Util = new ClientX509Util();
--- End diff --

That's correct. So it basically turned out that `ClientX509Util` is 
somewhat related to client communication too. Later, in a separate ticket, we 
can either pull everything together or make the separation more strict, but I 
wouldn't touch it for now.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195587480
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/UnifiedServerSocket.java ---
@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import org.apache.zookeeper.common.X509Exception;
+import org.apache.zookeeper.common.X509Util;
+import org.jboss.netty.buffer.ChannelBuffers;
+import org.jboss.netty.handler.ssl.SslHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLSocket;
+import java.io.IOException;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.net.SocketException;
+
+public class UnifiedServerSocket extends ServerSocket {
--- End diff --

I will figure out a way to extract my implementation and provide it to you. 
Give me a day or two to figure out how.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195587293
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKConfig.java ---
@@ -107,14 +99,33 @@ private void init() {
  * this configuration.
  */
 protected void handleBackwardCompatibility() {
-properties.put(SSL_KEYSTORE_LOCATION, 
System.getProperty(SSL_KEYSTORE_LOCATION));
-properties.put(SSL_KEYSTORE_PASSWD, 
System.getProperty(SSL_KEYSTORE_PASSWD));
-properties.put(SSL_TRUSTSTORE_LOCATION, 
System.getProperty(SSL_TRUSTSTORE_LOCATION));
-properties.put(SSL_TRUSTSTORE_PASSWD, 
System.getProperty(SSL_TRUSTSTORE_PASSWD));
-properties.put(SSL_AUTHPROVIDER, 
System.getProperty(SSL_AUTHPROVIDER));
 properties.put(JUTE_MAXBUFFER, System.getProperty(JUTE_MAXBUFFER));
 properties.put(KINIT_COMMAND, System.getProperty(KINIT_COMMAND));
 properties.put(JGSS_NATIVE, System.getProperty(JGSS_NATIVE));
+
+ClientX509Util clientX509Util = new ClientX509Util();
--- End diff --

Hmm, I don't know if we necessarily want to use the same SSL settings for 
that, but maybe we can figure it out later. Is `ClientCnxnSocketNetty` used for 
connections between ensemble servers and ZK clients?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195586921
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -79,7 +91,7 @@
 public X509Util() {
 String cipherSuitesInput = 
System.getProperty(cipherSuitesProperty);
 if (cipherSuitesInput == null) {
-cipherSuites = null;
+cipherSuites = getDefaultCipherSuites();
--- End diff --

@anmolnar sounds good


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195567746
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/UnifiedServerSocket.java ---
@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import org.apache.zookeeper.common.X509Exception;
+import org.apache.zookeeper.common.X509Util;
+import org.jboss.netty.buffer.ChannelBuffers;
+import org.jboss.netty.handler.ssl.SslHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLSocket;
+import java.io.IOException;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.net.SocketException;
+
+public class UnifiedServerSocket extends ServerSocket {
--- End diff --

I'm struggling to get this right, so really curious about your 
implementation. The SSL side works fine, I can create the special socket 
combined with the InputStream as suggested to channel back already consumed 
data, but how can I do the same with non-SSL socket?

Modified `UnifiedServerSocket` like this:
```java
final Socket normalSocket = new Socket();
implAccept(normalSocket);

byte[] litmus = new byte[5];
int bytesRead = normalSocket.getInputStream().read(litmus, 0, 5);

if (bytesRead == 5 && 
SslHandler.isEncrypted(ChannelBuffers.wrappedBuffer(litmus))) {
LOG.info(getInetAddress() + " attempting to connect over ssl");
SSLSocket sslSocket;
try {
sslSocket = x509Util.createSSLSocket(normalSocket, new 
ByteArrayInputStream(litmus));
} catch (X509Exception e) {
throw new IOException("failed to create SSL context", e);
}
sslSocket.setUseClientMode(false);
return sslSocket;
} else {
LOG.info(getInetAddress() + " attempting to connect without 
ssl");
return normalSocket;
}
```
Last return statement lacks of channeling back the litmus, hence it cannot 
be read on server side.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195537620
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -79,7 +91,7 @@
 public X509Util() {
 String cipherSuitesInput = 
System.getProperty(cipherSuitesProperty);
 if (cipherSuitesInput == null) {
-cipherSuites = null;
+cipherSuites = getDefaultCipherSuites();
--- End diff --

Providing null as cipher suite list throws exception, but empty list is 
accepted. In which case SSL won't work, because there's no enabled cipher suite 
at all. Similarly if enabled suites and supported suites doesn't have anything 
in common, SSL is broken.

I wouldn't intersect and wouldn't default on empty list. Just use whatever 
the user provided and don't do magic which otherwise has to be documented 
properly.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread dain

Github user dain commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195534358
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -79,7 +91,7 @@
 public X509Util() {
 String cipherSuitesInput = 
System.getProperty(cipherSuitesProperty);
 if (cipherSuitesInput == null) {
-cipherSuites = null;
+cipherSuites = getDefaultCipherSuites();
--- End diff --

I would say that if a user manually selected a cipher suite and it is not 
present on the JVM it should be an error.  Otherwise they are not getting what 
they explicitly asked for.  That said, I'm not sure you can check this 
explicitly (I don't know the apis that well either... especially when multiple 
providers are installed).


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195530931
  
--- 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 --

I won't touch it then.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195529315
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKConfig.java ---
@@ -107,14 +99,33 @@ private void init() {
  * this configuration.
  */
 protected void handleBackwardCompatibility() {
-properties.put(SSL_KEYSTORE_LOCATION, 
System.getProperty(SSL_KEYSTORE_LOCATION));
-properties.put(SSL_KEYSTORE_PASSWD, 
System.getProperty(SSL_KEYSTORE_PASSWD));
-properties.put(SSL_TRUSTSTORE_LOCATION, 
System.getProperty(SSL_TRUSTSTORE_LOCATION));
-properties.put(SSL_TRUSTSTORE_PASSWD, 
System.getProperty(SSL_TRUSTSTORE_PASSWD));
-properties.put(SSL_AUTHPROVIDER, 
System.getProperty(SSL_AUTHPROVIDER));
 properties.put(JUTE_MAXBUFFER, System.getProperty(JUTE_MAXBUFFER));
 properties.put(KINIT_COMMAND, System.getProperty(KINIT_COMMAND));
 properties.put(JGSS_NATIVE, System.getProperty(JGSS_NATIVE));
+
+ClientX509Util clientX509Util = new ClientX509Util();
--- End diff --

Not sure that would be better either. Looks like `ClientCnxnSocketNetty` 
uses it too to create SSLContext.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195518790
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/UnifiedServerSocket.java ---
@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import org.apache.zookeeper.common.X509Exception;
+import org.apache.zookeeper.common.X509Util;
+import org.jboss.netty.buffer.ChannelBuffers;
+import org.jboss.netty.handler.ssl.SslHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLSocket;
+import java.io.IOException;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.net.SocketException;
+
+public class UnifiedServerSocket extends ServerSocket {
--- End diff --

Sure, that would be awesome.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195517105
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -79,7 +91,7 @@
 public X509Util() {
 String cipherSuitesInput = 
System.getProperty(cipherSuitesProperty);
 if (cipherSuitesInput == null) {
-cipherSuites = null;
+cipherSuites = getDefaultCipherSuites();
--- End diff --

Should we also intersect the selected cipher suites with enabled cipher 
suites (after this if/else so it covers both branches)? I'm not an expert on 
Java SSL APIs so I'm not sure if it's necessary. Maybe @dain or @electrum can 
chime in.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195517786
  
--- 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;
+
+private volatile SSLContext defaultSSLContext;
+
+public X509Util() {
+String cipherSuitesInput = 
System.getProperty(cipherSuitesProperty);
+if (cipherSuitesInput == null) {
+cipherSuites = null;
+} else {
+cipherSuites = cipherSuitesInput.split(",");
+}
+}
+
+protected abstract String getConfigPrefix();
+protected

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195514675
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -339,4 +351,20 @@ private void configureSSLServerSocket(SSLServerSocket 
sslServerSocket) {
 
 sslServerSocket.setSSLParameters(sslParameters);
 }
+
+private String[] getDefaultCipherSuites() {
+String javaVersion = System.getProperty("java.version");
--- End diff --

Couple minor suggestions:
- use the "java.specification.version" property instead, it returns a 
string w/o the minor version (such as "1.8" or "9"), easier to deal with.
- Add some comments that explain why we branch the ciphers based on the 
java version. Something like "perf 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."


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195517918
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKConfig.java ---
@@ -107,14 +99,33 @@ private void init() {
  * this configuration.
  */
 protected void handleBackwardCompatibility() {
-properties.put(SSL_KEYSTORE_LOCATION, 
System.getProperty(SSL_KEYSTORE_LOCATION));
-properties.put(SSL_KEYSTORE_PASSWD, 
System.getProperty(SSL_KEYSTORE_PASSWD));
-properties.put(SSL_TRUSTSTORE_LOCATION, 
System.getProperty(SSL_TRUSTSTORE_LOCATION));
-properties.put(SSL_TRUSTSTORE_PASSWD, 
System.getProperty(SSL_TRUSTSTORE_PASSWD));
-properties.put(SSL_AUTHPROVIDER, 
System.getProperty(SSL_AUTHPROVIDER));
 properties.put(JUTE_MAXBUFFER, System.getProperty(JUTE_MAXBUFFER));
 properties.put(KINIT_COMMAND, System.getProperty(KINIT_COMMAND));
 properties.put(JGSS_NATIVE, System.getProperty(JGSS_NATIVE));
+
+ClientX509Util clientX509Util = new ClientX509Util();
--- End diff --

@anmolnar good idea. Or maybe rename the class? Something like 
`QuorumClientX509Util`?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195516340
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -79,7 +91,7 @@
 public X509Util() {
 String cipherSuitesInput = 
System.getProperty(cipherSuitesProperty);
 if (cipherSuitesInput == null) {
-cipherSuites = null;
+cipherSuites = getDefaultCipherSuites();
--- End diff --

nit: add "|| cipherSuitsInput.isEmpty()" to the if condition? Is there any 
time we would want the cipher suites to be an empty array? I think that would 
make SSL fail so I don't think we should allow it. Unless the SSL socket will 
use some system default if you pass in an empty array, do you know?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195517644
  
--- 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 --

I don't think we ever want to use anything earlier than "TLSv1.2", so I 
wouldn't worry about it too much.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195481800
  
--- 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 --

Java-specific default cipher suites has been added. I still need to work on 
protocol restrictions. Do you know what's the correlation between the default 
protocol set here:
```java
String protocol = System.getProperty(sslProtocolProperty, DEFAULT_PROTOCOL);
try {
SSLContext sslContext = SSLContext.getInstance(protocol);
sslContext.init(keyManagers, trustManagers, null);
```
and

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-12 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194715953
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKConfig.java ---
@@ -107,14 +99,33 @@ private void init() {
  * this configuration.
  */
 protected void handleBackwardCompatibility() {
-properties.put(SSL_KEYSTORE_LOCATION, 
System.getProperty(SSL_KEYSTORE_LOCATION));
-properties.put(SSL_KEYSTORE_PASSWD, 
System.getProperty(SSL_KEYSTORE_PASSWD));
-properties.put(SSL_TRUSTSTORE_LOCATION, 
System.getProperty(SSL_TRUSTSTORE_LOCATION));
-properties.put(SSL_TRUSTSTORE_PASSWD, 
System.getProperty(SSL_TRUSTSTORE_PASSWD));
-properties.put(SSL_AUTHPROVIDER, 
System.getProperty(SSL_AUTHPROVIDER));
 properties.put(JUTE_MAXBUFFER, System.getProperty(JUTE_MAXBUFFER));
 properties.put(KINIT_COMMAND, System.getProperty(KINIT_COMMAND));
 properties.put(JGSS_NATIVE, System.getProperty(JGSS_NATIVE));
+
+ClientX509Util clientX509Util = new ClientX509Util();
--- End diff --

I'm thinking of adding a short comment to these classes for clarification.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194539816
  
--- 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 --

I think it's ok to keep it configurable, just give it good defaults so most 
people don't need to. For example, if someone wants to use ZK with DSA-signed 
certs, they would need to specify other cipher suites.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194527693
  
--- 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 --

Okay. I'll remove the configuration of both enabled cipher suites and 
protocols. Default protocol remains TLSv1.2, enabled cipher suites will be the 
list you provided. Thanks.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194523369
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKConfig.java ---
@@ -107,14 +99,33 @@ private void init() {
  * this configuration.
  */
 protected void handleBackwardCompatibility() {
-properties.put(SSL_KEYSTORE_LOCATION, 
System.getProperty(SSL_KEYSTORE_LOCATION));
-properties.put(SSL_KEYSTORE_PASSWD, 
System.getProperty(SSL_KEYSTORE_PASSWD));
-properties.put(SSL_TRUSTSTORE_LOCATION, 
System.getProperty(SSL_TRUSTSTORE_LOCATION));
-properties.put(SSL_TRUSTSTORE_PASSWD, 
System.getProperty(SSL_TRUSTSTORE_PASSWD));
-properties.put(SSL_AUTHPROVIDER, 
System.getProperty(SSL_AUTHPROVIDER));
 properties.put(JUTE_MAXBUFFER, System.getProperty(JUTE_MAXBUFFER));
 properties.put(KINIT_COMMAND, System.getProperty(KINIT_COMMAND));
 properties.put(JGSS_NATIVE, System.getProperty(JGSS_NATIVE));
+
+ClientX509Util clientX509Util = new ClientX509Util();
--- End diff --

The naming confused me a bit too, but yeah, this is quorum security only.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread breed

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194501452
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java ---
@@ -64,6 +66,8 @@
 
 protected InetSocketAddress clientPortAddress;
 protected InetSocketAddress secureClientPortAddress;
+protected boolean sslQuorum = false;
+protected boolean shouldUsePortUnification = false;
--- End diff --

it would be nice to expose these through JMX


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread breed

Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194498695
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKConfig.java ---
@@ -107,14 +99,33 @@ private void init() {
  * this configuration.
  */
 protected void handleBackwardCompatibility() {
-properties.put(SSL_KEYSTORE_LOCATION, 
System.getProperty(SSL_KEYSTORE_LOCATION));
-properties.put(SSL_KEYSTORE_PASSWD, 
System.getProperty(SSL_KEYSTORE_PASSWD));
-properties.put(SSL_TRUSTSTORE_LOCATION, 
System.getProperty(SSL_TRUSTSTORE_LOCATION));
-properties.put(SSL_TRUSTSTORE_PASSWD, 
System.getProperty(SSL_TRUSTSTORE_PASSWD));
-properties.put(SSL_AUTHPROVIDER, 
System.getProperty(SSL_AUTHPROVIDER));
 properties.put(JUTE_MAXBUFFER, System.getProperty(JUTE_MAXBUFFER));
 properties.put(KINIT_COMMAND, System.getProperty(KINIT_COMMAND));
 properties.put(JGSS_NATIVE, System.getProperty(JGSS_NATIVE));
+
+ClientX509Util clientX509Util = new ClientX509Util();
--- End diff --

should we have client stuff in this diff? we are just doing quorum 
security. right?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194460371
  
--- 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 --

Again, since we control the client and server side of quorum connections, I 
think it's fine to default to `TLSv1.2`. Someday in the future we'll want to 
migrate to 1.3, but Java doesn't have support for it yet (indeed, most software 
does not).


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194460585
  
--- 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;
+
+private volatile SSLContext defaultSSLContext;
+
+public X509Util() {
+String cipherSuitesInput = 
System.getProperty(cipherSuitesProperty);
+if (cipherSuitesInput == null) {
+cipherSuites = null;
+} else {
+cipherSuites = cipherSuitesInput.split(",");
+}
+}
+
+protected abstract String getConfigPrefix();
+protected

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194461322
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -160,43 +214,120 @@ public static X509KeyManager createKeyManager(String 
keyStoreLocation, String ke
 }
 throw new KeyManagerException("Couldn't find X509KeyManager");
 
-} catch (Exception e) {
-throw new KeyManagerException(e);
+} catch 
(IOException|CertificateException|UnrecoverableKeyException|NoSuchAlgorithmException|KeyStoreException
+keyManagerCreationException) {
+throw new KeyManagerException(keyManagerCreationException);
 } finally {
 if (inputStream != null) {
 try {
 inputStream.close();
-} catch (IOException e) {}
+} catch (IOException ioException) {
+LOG.info("Failed to close key store input stream", 
ioException);
+}
 }
 }
 }
 
-public static X509TrustManager createTrustManager(String 
trustStoreLocation, String trustStorePassword)
+public static X509TrustManager createTrustManager(String 
trustStoreLocation, String trustStorePassword,
+  boolean crlEnabled, 
boolean ocspEnabled,
+  final boolean 
hostnameVerificationEnabled,
+  final boolean 
shouldVerifyClientHostname)
 throws TrustManagerException {
 FileInputStream inputStream = null;
 try {
-char[] trustStorePasswordChars = 
trustStorePassword.toCharArray();
 File trustStoreFile = new File(trustStoreLocation);
 KeyStore ts = KeyStore.getInstance("JKS");
 inputStream = new FileInputStream(trustStoreFile);
-ts.load(inputStream, trustStorePasswordChars);
-TrustManagerFactory tmf = 
TrustManagerFactory.getInstance("SunX509");
-tmf.init(ts);
+if (trustStorePassword != null) {
+char[] trustStorePasswordChars = 
trustStorePassword.toCharArray();
+ts.load(inputStream, trustStorePasswordChars);
+} else {
+ts.load(inputStream, null);
+}
 
-for (TrustManager tm : tmf.getTrustManagers()) {
-if (tm instanceof X509TrustManager) {
-return (X509TrustManager) tm;
+PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, 
new X509CertSelector());
+if (crlEnabled || ocspEnabled) {
+pbParams.setRevocationEnabled(true);
+System.setProperty("com.sun.net.ssl.checkRevocation", 
"true");
+System.setProperty("com.sun.security.enableCRLDP", "true");
+if (ocspEnabled) {
+Security.setProperty("ocsp.enable", "true");
+}
+
+} else {
+pbParams.setRevocationEnabled(false);
+}
+
+// Revocation checking is only supported with the PKIX 
algorithm
+TrustManagerFactory tmf = 
TrustManagerFactory.getInstance("PKIX");
+tmf.init(new CertPathTrustManagerParameters(pbParams));
+
+for (final TrustManager tm : tmf.getTrustManagers()) {
+if (tm instanceof X509ExtendedTrustManager) {
+return new ZKTrustManager((X509ExtendedTrustManager) 
tm, hostnameVerificationEnabled, shouldVerifyClientHostname);
 }
 }
 throw new TrustManagerException("Couldn't find 
X509TrustManager");
-} catch (Exception e) {
-throw new TrustManagerException(e);
+} catch 
(IOException|CertificateException|NoSuchAlgorithmException|InvalidAlgorithmParameterException|KeyStoreException
+ trustManagerCreationException) {
+throw new TrustManagerException(trustManagerCreationException);
 } finally {
 if (inputStream != null) {
 try {
 inputStream.close();
-} catch (IOException e) {}
+} catch (IOException ioException) {
+LOG.info("failed to close TrustStore input stream", 
ioException);
+}
 }
 }
 }
-}
\ No newline at end of file
+
+public SSLSocket createSSLSocket() throws X509Exception, IOException {
+SSLSocket sslSocket =

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194458091
  
--- 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 --

@ivmaykov Yep, it does make sense indeed. I've just got an understanding of 
client-side TLS has been implemented completely different from this and it's 
built inside Netty.

How about protocol restrictions here?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194456601
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,144 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean hostnameVerificationEnabled;
+private boolean shouldVerifyClientHostname;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param verifySSLServerHostname  If true, this TrustManager should 
verify hostnames of servers that this
+ * instance connects to.
+ * @param verifySSLClientHostname  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+public ZKTrustManager(X509ExtendedTrustManager 
x509ExtendedTrustManager, boolean verifySSLServerHostname,
+  boolean verifySSLClientHostname) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.hostnameVerificationEnabled = verifySSLServerHostname;
+this.shouldVerifyClientHostname = verifySSLClientHostname;
+
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+if (hostnameVerificationEnabled && shouldVerifyClientHostname) {
--- End diff --

This is done.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194451677
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,144 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean hostnameVerificationEnabled;
+private boolean shouldVerifyClientHostname;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param verifySSLServerHostname  If true, this TrustManager should 
verify hostnames of servers that this
+ * instance connects to.
+ * @param verifySSLClientHostname  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+public ZKTrustManager(X509ExtendedTrustManager 
x509ExtendedTrustManager, boolean verifySSLServerHostname,
+  boolean verifySSLClientHostname) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.hostnameVerificationEnabled = verifySSLServerHostname;
+this.shouldVerifyClientHostname = verifySSLClientHostname;
+
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+if (hostnameVerificationEnabled && shouldVerifyClientHostname) {
--- End diff --

Sorry, just noticed your previous comment. Let's see if I understand you 
properly:
- keep one configuration property: `hostnameVerificationEnabled`
- set 2 distinct properties on instantiation: if config is enabled, use 
original behaviour, if config is disabled, disable both.
I'll give it a try.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194442041
  
--- 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 --

I meant quorum only, at least for now. We might need to support more wide 
variety of suites for clients (I don't plan to start working on client-side TLS 
until July or August, so I'm not sure yet).


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194441963
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,144 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean hostnameVerificationEnabled;
+private boolean shouldVerifyClientHostname;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param verifySSLServerHostname  If true, this TrustManager should 
verify hostnames of servers that this
+ * instance connects to.
+ * @param verifySSLClientHostname  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+public ZKTrustManager(X509ExtendedTrustManager 
x509ExtendedTrustManager, boolean verifySSLServerHostname,
+  boolean verifySSLClientHostname) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.hostnameVerificationEnabled = verifySSLServerHostname;
+this.shouldVerifyClientHostname = verifySSLClientHostname;
+
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+if (hostnameVerificationEnabled && shouldVerifyClientHostname) {
--- End diff --

I ended up creating 2 distinct properties for the settings, but client 
hostname verification is a bit strange to me: when enabled it should have 
different behaviours when used in `QuorumX509Util` and `ClientX509Util`; true 
and false accordingly.

Now you have basically 3 options:
- property enabled: both enabled,
- property disabled: both disabled,
- property not present: the original, 2-fold behaviour

Purpose of having separate Client/Quorum X509Utils need to be clarified 
first to get this right.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194438668
  
--- 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 --

Do you think we should follow the same approach with enabled protocols?

Currently there're 2 settings we're dealing with in this patch regarding 
protocols:
1. a default protocol setting which is configurable and set when we create 
default SSL context,
2. restriction of enabled protocols which should be set similarly to 
enabled cipher suites when creating the socket.

The latter one is missing

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194438427
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,144 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean hostnameVerificationEnabled;
+private boolean shouldVerifyClientHostname;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param verifySSLServerHostname  If true, this TrustManager should 
verify hostnames of servers that this
+ * instance connects to.
+ * @param verifySSLClientHostname  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+public ZKTrustManager(X509ExtendedTrustManager 
x509ExtendedTrustManager, boolean verifySSLServerHostname,
+  boolean verifySSLClientHostname) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.hostnameVerificationEnabled = verifySSLServerHostname;
+this.shouldVerifyClientHostname = verifySSLClientHostname;
+
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+if (hostnameVerificationEnabled && shouldVerifyClientHostname) {
--- End diff --

How about keeping one configurable property, but deriving the two 
independent properties from it in this code? Makes this code more readable and 
you can still disable both verifications with one change in the config file.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194437578
  
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Learner.java ---
@@ -292,8 +298,20 @@ protected void connectToLeader(InetSocketAddress addr, 
String hostname)
 sock.getInputStream()));
 bufferedOutput = new BufferedOutputStream(sock.getOutputStream());
 leaderOs = BinaryOutputArchive.getArchive(bufferedOutput);
-}   
-
+}
+
+private void createSocket() throws X509Exception, IOException {
--- End diff --

I prefer pure functions whenever possible. Since they don't touch global 
state they are trivially thread-safe, and they are also easier to test. Like I 
said, it's a minor style thing.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194405375
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/PrependableSocket.java ---
@@ -0,0 +1,49 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.SequenceInputStream;
+import java.net.Socket;
+import java.net.SocketImpl;
+
+public class PrependableSocket extends Socket {
--- End diff --

Master/3.5 branches have already been upgraded to Java 8.
I don't see any barrier to use your proposal.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194404893
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java ---
@@ -348,20 +347,20 @@ public ChannelPipeline getPipeline() throws Exception 
{
 return p;
 }
 });
+x509Util = new ClientX509Util();
 }
 
 private synchronized void initSSL(ChannelPipeline p)
 throws X509Exception, KeyManagementException, 
NoSuchAlgorithmException {
-String authProviderProp = 
System.getProperty(ZKConfig.SSL_AUTHPROVIDER);
--- End diff --

I referred this one in my previous comment.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194404621
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -160,43 +214,120 @@ public static X509KeyManager createKeyManager(String 
keyStoreLocation, String ke
 }
 throw new KeyManagerException("Couldn't find X509KeyManager");
 
-} catch (Exception e) {
-throw new KeyManagerException(e);
+} catch 
(IOException|CertificateException|UnrecoverableKeyException|NoSuchAlgorithmException|KeyStoreException
+keyManagerCreationException) {
+throw new KeyManagerException(keyManagerCreationException);
 } finally {
 if (inputStream != null) {
 try {
 inputStream.close();
-} catch (IOException e) {}
+} catch (IOException ioException) {
+LOG.info("Failed to close key store input stream", 
ioException);
+}
 }
 }
 }
 
-public static X509TrustManager createTrustManager(String 
trustStoreLocation, String trustStorePassword)
+public static X509TrustManager createTrustManager(String 
trustStoreLocation, String trustStorePassword,
+  boolean crlEnabled, 
boolean ocspEnabled,
+  final boolean 
hostnameVerificationEnabled,
+  final boolean 
shouldVerifyClientHostname)
 throws TrustManagerException {
 FileInputStream inputStream = null;
 try {
-char[] trustStorePasswordChars = 
trustStorePassword.toCharArray();
 File trustStoreFile = new File(trustStoreLocation);
 KeyStore ts = KeyStore.getInstance("JKS");
 inputStream = new FileInputStream(trustStoreFile);
-ts.load(inputStream, trustStorePasswordChars);
-TrustManagerFactory tmf = 
TrustManagerFactory.getInstance("SunX509");
-tmf.init(ts);
+if (trustStorePassword != null) {
+char[] trustStorePasswordChars = 
trustStorePassword.toCharArray();
+ts.load(inputStream, trustStorePasswordChars);
+} else {
+ts.load(inputStream, null);
+}
 
-for (TrustManager tm : tmf.getTrustManagers()) {
-if (tm instanceof X509TrustManager) {
-return (X509TrustManager) tm;
+PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, 
new X509CertSelector());
+if (crlEnabled || ocspEnabled) {
+pbParams.setRevocationEnabled(true);
+System.setProperty("com.sun.net.ssl.checkRevocation", 
"true");
+System.setProperty("com.sun.security.enableCRLDP", "true");
+if (ocspEnabled) {
+Security.setProperty("ocsp.enable", "true");
+}
+
+} else {
+pbParams.setRevocationEnabled(false);
+}
+
+// Revocation checking is only supported with the PKIX 
algorithm
+TrustManagerFactory tmf = 
TrustManagerFactory.getInstance("PKIX");
+tmf.init(new CertPathTrustManagerParameters(pbParams));
+
+for (final TrustManager tm : tmf.getTrustManagers()) {
+if (tm instanceof X509ExtendedTrustManager) {
+return new ZKTrustManager((X509ExtendedTrustManager) 
tm, hostnameVerificationEnabled, shouldVerifyClientHostname);
 }
 }
 throw new TrustManagerException("Couldn't find 
X509TrustManager");
-} catch (Exception e) {
-throw new TrustManagerException(e);
+} catch 
(IOException|CertificateException|NoSuchAlgorithmException|InvalidAlgorithmParameterException|KeyStoreException
+ trustManagerCreationException) {
+throw new TrustManagerException(trustManagerCreationException);
 } finally {
 if (inputStream != null) {
 try {
 inputStream.close();
-} catch (IOException e) {}
+} catch (IOException ioException) {
+LOG.info("failed to close TrustStore input stream", 
ioException);
+}
 }
 }
 }
-}
\ No newline at end of file
+
+public SSLSocket createSSLSocket() throws X509Exception, IOException {
+SSLSocket sslSocket =

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194395807
  
--- 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;
+
+private volatile SSLContext defaultSSLContext;
+
+public X509Util() {
+String cipherSuitesInput = 
System.getProperty(cipherSuitesProperty);
+if (cipherSuitesInput == null) {
+cipherSuites = null;
+} else {
+cipherSuites = cipherSuitesInput.split(",");
+}
+}
+
+protected abstract String getConfigPrefix();
+protected

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194378274
  
--- 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 --

I think this makes a lot of sense and actually was thinking about something 
similar previously. There might be other clients which use ZooKeeper interface 
directly, but I think that should not be a problem and safety is more important 
in this case.

Are you talking about restricting the quorum communication only or set both 
for server and client connections?

In the latter case we could remove related

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194372536
  
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Learner.java ---
@@ -292,8 +298,20 @@ protected void connectToLeader(InetSocketAddress addr, 
String hostname)
 sock.getInputStream()));
 bufferedOutput = new BufferedOutputStream(sock.getOutputStream());
 leaderOs = BinaryOutputArchive.getArchive(bufferedOutput);
-}   
-
+}
+
+private void createSocket() throws X509Exception, IOException {
--- End diff --

This is also done. Would you please elaborate a little bit on what are the 
benefits?


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194371329
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -632,37 +639,46 @@ synchronized private boolean connectOne(long sid, 
InetSocketAddress electionAddr
 
 Socket sock = null;
 try {
-LOG.debug("Opening channel to server " + sid);
-sock = new Socket();
-setSockOpts(sock);
-sock.connect(electionAddr, cnxTO);
-LOG.debug("Connected to server " + sid);
+ LOG.debug("Opening channel to server " + sid);
+ if (self.isSslQuorum()) {
+ SSLSocket sslSock = x509Util.createSSLSocket();
+ setSockOpts(sslSock);
+ sslSock.connect(electionAddr, cnxTO);
+ sslSock.startHandshake();
+ sock = sslSock;
+ } else {sock = new Socket();
+ setSockOpts(sock);
--- End diff --

Done.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194369924
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,144 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean hostnameVerificationEnabled;
+private boolean shouldVerifyClientHostname;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param verifySSLServerHostname  If true, this TrustManager should 
verify hostnames of servers that this
+ * instance connects to.
+ * @param verifySSLClientHostname  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+public ZKTrustManager(X509ExtendedTrustManager 
x509ExtendedTrustManager, boolean verifySSLServerHostname,
+  boolean verifySSLClientHostname) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.hostnameVerificationEnabled = verifySSLServerHostname;
+this.shouldVerifyClientHostname = verifySSLClientHostname;
+
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+if (hostnameVerificationEnabled && shouldVerifyClientHostname) {
--- End diff --

With the current implementation the 2nd argument (which now became the 
`clientHostnameVerificationEnabled` property) is hardcoded and false for client 
connections and true for quorum connections.

One cannot disable hostname verification for the quorum protocol in this 
case unless we make it both configurable.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194368166
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,144 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean hostnameVerificationEnabled;
+private boolean shouldVerifyClientHostname;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param verifySSLServerHostname  If true, this TrustManager should 
verify hostnames of servers that this
+ * instance connects to.
+ * @param verifySSLClientHostname  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+public ZKTrustManager(X509ExtendedTrustManager 
x509ExtendedTrustManager, boolean verifySSLServerHostname,
+  boolean verifySSLClientHostname) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.hostnameVerificationEnabled = verifySSLServerHostname;
+this.shouldVerifyClientHostname = verifySSLClientHostname;
+
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+if (hostnameVerificationEnabled && shouldVerifyClientHostname) {
--- End diff --

I agree completely that it would be more clearer. A little bit cumbersome 
to do the change, but I'll do the refactoring.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194366505
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,144 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean hostnameVerificationEnabled;
+private boolean shouldVerifyClientHostname;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param verifySSLServerHostname  If true, this TrustManager should 
verify hostnames of servers that this
+ * instance connects to.
+ * @param verifySSLClientHostname  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+public ZKTrustManager(X509ExtendedTrustManager 
x509ExtendedTrustManager, boolean verifySSLServerHostname,
+  boolean verifySSLClientHostname) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.hostnameVerificationEnabled = verifySSLServerHostname;
+this.shouldVerifyClientHostname = verifySSLClientHostname;
+
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+if (hostnameVerificationEnabled && shouldVerifyClientHostname) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
+}
+
+@Override
+public void checkServerTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+if (hostnameVerificationEnabled) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+x509ExtendedTrustManager.checkServerTrusted(chain, authType, 
socket);
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, SSLEngine engine) throws CertificateException {
+if

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194365408
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,144 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean hostnameVerificationEnabled;
+private boolean shouldVerifyClientHostname;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param verifySSLServerHostname  If true, this TrustManager should 
verify hostnames of servers that this
+ * instance connects to.
+ * @param verifySSLClientHostname  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+public ZKTrustManager(X509ExtendedTrustManager 
x509ExtendedTrustManager, boolean verifySSLServerHostname,
+  boolean verifySSLClientHostname) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.hostnameVerificationEnabled = verifySSLServerHostname;
+this.shouldVerifyClientHostname = verifySSLClientHostname;
--- End diff --

Done.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194365379
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,144 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean hostnameVerificationEnabled;
+private boolean shouldVerifyClientHostname;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param verifySSLServerHostname  If true, this TrustManager should 
verify hostnames of servers that this
+ * instance connects to.
+ * @param verifySSLClientHostname  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+public ZKTrustManager(X509ExtendedTrustManager 
x509ExtendedTrustManager, boolean verifySSLServerHostname,
+  boolean verifySSLClientHostname) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.hostnameVerificationEnabled = verifySSLServerHostname;
+this.shouldVerifyClientHostname = verifySSLClientHostname;
+
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+if (hostnameVerificationEnabled && shouldVerifyClientHostname) {
+performHostVerification(socket.getInetAddress(), chain[0]);
+}
+x509ExtendedTrustManager.checkClientTrusted(chain, authType, 
socket);
--- End diff --

Makes sense, I'll do the change.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-11 Thread anmolnar

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194365328
  
--- Diff: src/java/main/org/apache/zookeeper/common/ClientX509Util.java ---
@@ -0,0 +1,38 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+public class ClientX509Util extends X509Util {
+
+private String sslAuthProviderProperty = getConfigPrefix() + 
"authProvider";
--- End diff --

Fixed.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-09 Thread ivmaykov

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,

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-09 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194234308
  
--- 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;
+
+private volatile SSLContext defaultSSLContext;
+
+public X509Util() {
+String cipherSuitesInput = 
System.getProperty(cipherSuitesProperty);
+if (cipherSuitesInput == null) {
+cipherSuites = null;
+} else {
+cipherSuites = cipherSuitesInput.split(",");
+}
+}
+
+protected abstract String getConfigPrefix();
+protected

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-09 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194234667
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/UnifiedServerSocket.java ---
@@ -0,0 +1,79 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import org.apache.zookeeper.common.X509Exception;
+import org.apache.zookeeper.common.X509Util;
+import org.jboss.netty.buffer.ChannelBuffers;
+import org.jboss.netty.handler.ssl.SslHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLSocket;
+import java.io.IOException;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.net.SocketException;
+
+public class UnifiedServerSocket extends ServerSocket {
--- End diff --

See my other comment in `PrependableSocket`. It's possible to implement 
port unification without creating socket subclasses if we require Java 8. I 
have an implementation that I could share as an example if you want to go that 
route.


---

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-09 Thread ivmaykov

Github user ivmaykov commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r194233492
  
--- Diff: src/java/main/org/apache/zookeeper/common/ZKTrustManager.java ---
@@ -0,0 +1,144 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.zookeeper.common;
+
+import org.apache.http.conn.ssl.DefaultHostnameVerifier;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+
+/**
+ * A custom TrustManager that supports hostname verification via 
org.apache.http.conn.ssl.DefaultHostnameVerifier.
+ *
+ * We attempt to perform verification using just the IP address first and 
if that fails will attempt to perform a
+ * reverse DNS lookup and verify using the hostname.
+ */
+public class ZKTrustManager extends X509ExtendedTrustManager {
+
+private static final Logger LOG = 
LoggerFactory.getLogger(ZKTrustManager.class);
+
+private X509ExtendedTrustManager x509ExtendedTrustManager;
+private boolean hostnameVerificationEnabled;
+private boolean shouldVerifyClientHostname;
+
+private DefaultHostnameVerifier hostnameVerifier;
+
+/**
+ * Instantiate a new ZKTrustManager.
+ *
+ * @param x509ExtendedTrustManager The trustmanager to use for 
checkClientTrusted/checkServerTrusted logic
+ * @param verifySSLServerHostname  If true, this TrustManager should 
verify hostnames of servers that this
+ * instance connects to.
+ * @param verifySSLClientHostname  If true, and 
verifySSLServerHostname is true, the hostname of a client
+ * connecting to this machine will be 
verified in addition to the servers that this
+ * instance connects to. If false, and 
verifySSLServerHostname is true, only
+ * the hostnames of servers that this 
instance connects to will be verified. If
+ * verifySSLServerHostname is false, 
this argument is ignored.
+ */
+public ZKTrustManager(X509ExtendedTrustManager 
x509ExtendedTrustManager, boolean verifySSLServerHostname,
+  boolean verifySSLClientHostname) {
+this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+this.hostnameVerificationEnabled = verifySSLServerHostname;
+this.shouldVerifyClientHostname = verifySSLClientHostname;
+
+hostnameVerifier = new DefaultHostnameVerifier();
+}
+
+@Override
+public X509Certificate[] getAcceptedIssuers() {
+return x509ExtendedTrustManager.getAcceptedIssuers();
+}
+
+@Override
+public void checkClientTrusted(X509Certificate[] chain, String 
authType, Socket socket) throws CertificateException {
+if (hostnameVerificationEnabled && shouldVerifyClientHostname) {
--- End diff --

might be simpler to just have separate booleans for verifying server 
hostname and verifying client hostname. That way one or the other or both can 
be enabled. Right now, it looks like to enable client hostname verification, 
server hostname verification must be enabled as well. What do you think?


---

1 2 >

1 - 100 of 174 matches

Mail list logo