ZOOKEEPER-236: SSL Support for Atomic Broadcast protocol #184

afine · 2017-03-06T23:19:06Z

This is a work in progress, I wanted to get some feedback from the community while I worked on this. Please do not merge yet. Tests, documentation, and some cleanup still coming.

This is a first pass at ssl support for the zookeeper quorum. It supports encrypting both leader election and normal operation.

Rolling upgrades are supported via port unification (portUnification=true). This should only be used while performing a rolling upgrade.

Some open questions:

Anyone have any ideas for better names for the configuration options (sslQuorum and portUnification currently).
I am using the same configuration that points to the truststore/keystore used for server <-> client ssl. Do they need to be separate?
Is port unification the correct approach for rolling upgrades? Is the impact from the use of BufferedSockets during the upgrade acceptable? See: http://stackoverflow.com/questions/25637039/detecting-ssl-connection-and-converting-socket-to-sslsocket http://stackoverflow.com/questions/6559859/is-it-possible-to-change-plain-socket-to-sslsocket
server <-> client ssl is implemented with netty. I did not feel that rewriting our server <-> server logic with netty was necessary given how easy ssl was to implement with standard java SSLSockets. Any arguments to the contrary?

Thanks,
Abe

geek101 · 2017-03-30T03:43:18Z

src/java/main/org/apache/zookeeper/common/X509Util.java

-                            ((X509ExtendedTrustManager) tm).checkClientTrusted(x509Certificates, s, socket);
+                        public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket) throws CertificateException {
+                            if (hostnameVerificationEnabled && shouldVerifyClientHostname) {
+                                hostnameVerifier.verify(socket.getInetAddress().getHostName(), ((SSLSocket) socket).getSession());


Will the hostname be available on the server side to verify?. How will the server know who this client is, one way is to implement a special HostnameVerifier that can be provide a set of peer hostnames or refer back to QuorumPeer to verify using the QuorumVerifier?.

Yes, the reverse dns lookup is done on the performHostnameVerification... line

implement a special HostnameVerifier that can be provide a set of peer hostnames or refer back to QuorumPeer to verify using the QuorumVerifier

I'm a little confused by this. Do you mean that the users needs to provide the mapping from IP addresses to hostnames in advance?

geek101 · 2017-03-30T03:43:23Z

src/java/main/org/apache/zookeeper/common/X509Util.java

                    return new X509ExtendedTrustManager() {
-                        HostnameChecker hostnameChecker = HostnameChecker.getInstance(HostnameChecker.TYPE_TLS);
+                        X509ExtendedTrustManager x509ExtendedTrustManager = (X509ExtendedTrustManager) tm;


Do you think extending X509ExtendedTrustManager outside will help us test it better?.

Not sure yet. The createTrustManager method might give us all the exposure to this implementation that we need. I think this will become more obvious as we begin expanding the tests.

hanm · 2017-04-06T00:32:13Z

src/java/main/org/apache/zookeeper/common/X509UtilTest.java

@@ -0,0 +1,231 @@
+/**


This is supposed to be a test file, right? If so, test file should be under src/test folder. Does this even build?

Whoops... Fixed.

hanm · 2017-04-06T00:33:56Z

src/java/main/org/apache/zookeeper/common/X509UtilTest.java

+        Security.setProperty("com.sun.security.enableCRLDP", "false");
+    }
+
+    @Test


Just in case if any of these test method run for a long time (which I doubt) then it'll be good to specify a timeout value annotation.

Agreed.

The QuorumSSLTest needs them too.

hanm · 2017-04-06T01:05:57Z

src/java/main/org/apache/zookeeper/server/quorum/Learner.java

@@ -193,21 +198,18 @@ void request(Request request) throws IOException {
    /**
     * Returns the address of the node we think is the leader.
     */
-    protected InetSocketAddress findLeader() {
+    protected QuorumServer findLeader() {


Not sure why findLeader is returning a QuorumServer instead of InetSocketAddress?

Egah, this is a change from an earlier iteration that I forgot to remove. Fixed now.

hanm · 2017-04-06T01:07:00Z

src/java/main/org/apache/zookeeper/server/quorum/Learner.java

@@ -253,7 +255,10 @@ protected void connectToLeader(InetSocketAddress addr)
                    throw new IOException("initLimit exceeded on retries.");
                }

-                sockConnect(sock, addr, Math.min(self.tickTime * self.syncLimit, remainingInitLimitTime));
+                sockConnect(sock, leader.addr, Math.min(self.tickTime * self.syncLimit, remainingInitLimitTime));


leader could be null in leader.addr right? Still wondering why change the leader type from an address to a QuorumServer.

This should be fixed.

hanm · 2017-04-06T01:09:12Z

src/java/main/org/apache/zookeeper/server/quorum/Learner.java

-        sock.setSoTimeout(self.tickTime * self.initLimit);
+    protected void connectToLeader(QuorumServer leader)
+    throws IOException, InterruptedException, X509Exception {
+        QuorumX509Util quorumX509Util = new QuorumX509Util();


We now create QuorumX509Util whenever connectToLeader is called, even if ssl is not enabled. Could this be improved so the SSL related code path is only involved when ssl is enabled?

Agreed. Fixed

geek101 · 2017-04-06T06:31:46Z

ivy.xml

@@ -63,6 +67,9 @@
    <dependency org="commons-collections" name="commons-collections" 
                rev="3.2.2" conf="test->default"/>

+    <dependency org="org.bouncycastle" name="bcprov-jdk15on" rev="1.56" conf="test->default"/>
+    <dependency org="org.bouncycastle" name="bcpkix-jdk15on" rev="1.56" conf="test->default"/>


is this specified twice?

One is bcprov-jdk15on and the other is bcpkix-jdk15on.

geek101 · 2017-04-06T06:48:25Z

src/java/main/org/apache/zookeeper/server/quorum/Leader.java

        this.self = self;
        try {
-            if (self.getQuorumListenOnAllIPs()) {
-                ss = new ServerSocket(self.getQuorumAddress().getPort());
+            if (self.shouldUsePortUnification()) {


Is it possible to move all this code away from here to a different class like SSLSocketFactory which can help keep Leader code have similar number of branches if not more.

This is a tough call IMO. I think you are correct that moving this out of the Leader would make the leader code simpler but on the other hand this is logic that I think "belongs" to the leader and is not really relevant anywhere else.

What do you think about the compromise of moving this to a static method in Leader where it can be tested and prevents cluttering up the constructor logic?

I think some level of refactoring that makes the constructor less clogged would be valuable here..

geek101 · 2017-04-06T06:51:51Z

src/java/main/org/apache/zookeeper/common/X509Util.java

+                        @Override
+                        public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket) throws CertificateException {
+                            if (hostnameVerificationEnabled && shouldVerifyClientHostname) {
+                                performHostnameVerification(socket.getInetAddress().getHostName(), chain[0]);


For Quorum server connection I am not entirely sure if getInetAddres().getHostName() is a good idea when on server side this will force a reverse DNS lookup. When customer has only provided a ip address as config perhaps using hostname is not correct. And if customer has provided a hostname performing reverse dns lookup is not necessary and can be argued as not safe. Since the trust anchor is the user provided config and not the DNS service. Let me know what you think.

I agree that we should not do reverse DNS lookups when we do not need to.

I changed the host verification logic to only perform a reverse dns lookup when host verification fails using the ip address. I think it is still fair to try the reverse dns lookup at that point as the value given in the altname of the certificate is not necessarily tied to the zookeeper configuration.

Let me know if you think this is reasonable.

geek101 · 2017-04-06T06:58:45Z

src/java/test/org/apache/zookeeper/test/QuorumSSLTest.java

+import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
+import static org.apache.zookeeper.test.ClientBase.createTmpDir;
+
+public class QuorumSSLTest extends QuorumPeerTestBase {


This is very nice great stuff :)

geek101 · 2017-04-06T06:59:54Z

src/java/main/org/apache/zookeeper/common/X509Util.java

+    public static final String DEFAULT_PROTOCOL = "TLSv1";
+
+    private String sslProtocolProperty = getConfigPrefix() + "protocol";
+    private String cipherSuitesProperty = getConfigPrefix() + "ciphersuites";


hanm · 2017-04-13T20:47:25Z

src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java

@@ -21,6 +21,7 @@
 import org.apache.zookeeper.ClientCnxn.EndOfStreamException;
 import org.apache.zookeeper.ClientCnxn.Packet;
 import org.apache.zookeeper.client.ZKClientConfig;
+import org.apache.zookeeper.common.ClientX509Util;
 import org.apache.zookeeper.common.X509Util;


Unused import.

hanm · 2017-04-13T20:47:51Z

src/java/main/org/apache/zookeeper/client/FourLetterWordMain.java

@@ -31,6 +31,7 @@
 import javax.net.ssl.SSLSocket;
 import javax.net.ssl.SSLSocketFactory;

+import org.apache.zookeeper.common.ClientX509Util;
 import org.apache.zookeeper.common.X509Exception.SSLContextException;
 import org.apache.zookeeper.common.X509Util;


Unused import.

hanm · 2017-04-13T20:49:35Z

src/java/main/org/apache/zookeeper/common/X509Util.java

-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 static org.apache.zookeeper.common.X509Exception.*;


Use explicit imports instead of wildcards imports - I guess here IDE tried to be clever..

hanm · 2017-04-13T20:53:45Z

src/java/main/org/apache/zookeeper/common/X509Util.java

@@ -150,7 +196,7 @@ public static X509KeyManager createKeyManager(String keyStoreLocation, String ke
            KeyStore ks = KeyStore.getInstance("JKS");
            inputStream = new FileInputStream(keyStoreFile);
            ks.load(inputStream, keyStorePasswordChars);
-            KeyManagerFactory kmf = KeyManagerFactory.getInstance("SunX509");
+            KeyManagerFactory kmf = KeyManagerFactory.getInstance("PKIX");


Any comments on what this change is for?

A comment has been added where pkix is used for the trustmanager factory. We don't actually need this change for the keymanager, but i preferred to be consistent.

hanm · 2017-04-13T20:59:09Z

src/java/main/org/apache/zookeeper/common/ZKTrustManager.java

+
+    private void performHostVerification(InetAddress inetAddress, X509Certificate certificate) throws CertificateException {
+        try {
+            hostnameVerifier.verify(inetAddress.getHostAddress(), certificate);


IIUC, this only verifies that the host name matches what's stored in the certificates, right? If so, how do we verify that same subject is a valid peer we intended to connect to, rather than any random server which happens to have a valid cert? Or is this already taken care of by other parts of code?

For reference, in ZOOKEEPER-1045 what we did for similar verification is by comparing the name of the server with the name retrieved from Kerberos principal. The name of the server is retrieved from the zoo.cfg and because of this we require if users want this verification they need encode the server as FQDN in zoo.cfg rather than using IP address. Maybe we can do something similar here.

So we have a few hostname checks that we could consider. Please let me know if I am missing any.

Check that the address/hostname of the machine matches what is listed on the certificate

Check that the address/hostname of the machine matches what is expected from zoo.cfg (this may also require reading the sid from the packet sent when a connection is established).

Check that the address/hostname listed on the certificate matches what is expected from zoo.cfg

This patch only performs check 1. I think that check 3 is something @geek101 mentioned earlier and we agreed this can be moved to another patch as it is more complicated.

I think it is also possible to implement check 2. This can exist entirely outside of tls/ssl support and is something we should consider based on its own merits.

I think having checks 1 and 2 would be plenty secure and we would not need check 3.

Let me know what you think @hanm.

check 1 and check 2 together do what check 3 does, no? So I think 1+2 is enough. The point is to make sure that:

The peer is what it claims to be (check 1 - authentication).

The peer is authorized to join quorum (check 2 - authorization).

I agree. It could also be argued that authorization is handled by this patch in its current state as well, by only accepting peers that have trusted certificates. In other words, the CA limits which machines get certificates, which limits which machines get to join the quorum. So check 2 is not "needed" if the CA is trusted.

I also think that check 2 has value for deployments that want to reuse the same truststore for multiple services and for deployments that are not using SSL/TLS at all. So I think it is a check worth doing and it is deserving of its own patch. Would be interested in knowing what other members of the community think.

hanm · 2017-04-13T21:00:00Z

src/java/main/org/apache/zookeeper/server/quorum/Follower.java

@@ -71,7 +71,7 @@ void followLeader() throws InterruptedException {
        self.end_fle = 0;
        fzk.registerJMX(new FollowerBean(this, zk), self.jmxLocalPeerBean);
        try {
-            InetSocketAddress addr = findLeader();            
+            InetSocketAddress addr = findLeader();


Unnecessary change here - this file can be removed from the change list.

hanm · 2017-04-13T21:01:28Z

src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java

@@ -51,6 +53,8 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

+import javax.net.ssl.SSLSocket;
+


Looks like these imports are not used. Another look is this file can also be removed from change list because nothing changed.

hanm · 2017-04-13T21:02:57Z

src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java

@@ -383,6 +387,7 @@ public void run() {
                        + leader.self.getView().get(this.sid).toString());
            } else {
                LOG.info("Follower sid: " + this.sid + " not in the current config " + Long.toHexString(leader.self.getQuorumVerifier().getVersion()));
+                //TODO: Hostname verification possible


Suggest to remove this TODO as it appertains to no code.

hanm · 2017-04-13T21:06:03Z

src/java/test/org/apache/zookeeper/test/QuorumSSLTest.java

+    }
+
+    @Test(timeout = 300000)
+    public void testRollingUpgrade() throws Exception {


This test is pretty flaky - please check internal Jenkins to diagnose.

hanm · 2017-04-25T18:36:27Z

src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java

@@ -63,6 +66,10 @@ public void shutdown() {
    }

    public static class MainThread implements Runnable {
+        public File getConfFile() {


Can't find a call to this getConfFile. Is this needed?

https://github.com/apache/zookeeper/pull/184/files#diff-41ff8b5e930c780549068c1e027290a4R449

enixon · 2018-09-17T23:57:46Z

+1 for @anmolnar 's plan

Afaik, with portUnification=false there shouldn't be any problems.

@ivmaykov does this work for you?

ivmaykov · 2018-09-18T01:34:51Z

I think as long as we keep portUnification=false there should not be hangs/crashes. However, that means it's not possible to safely upgrade a cluster from plaintext quorum to TLS quorum without downtime. @hanm mentioned "mitigations" but there really isn't a way to mitigate the issues w/ UnifiedServerSocket in #184 (other than "don't use it"). One option is to keep the unified server socket code, but don't parse the portUnification option in QuorumPeerConfig so there is no way to use the feature. Or we could document the issues and have a clear warning ("portUnification has known problems and may cause your ensemble to enter a bad state that requires reboots, use at your own risk"), and let people take the risk if they like.

One issue that we found is the >10% perf regression in plaintext mode when the apache httpclient library dependency is added. We never figured out why it caused the perf regression, but it could be a potential blocker. Unlike the port unification bugs, this perf regression cannot be worked around - it was present even when all the SSL features were disabled. The fix for that is isolated to one file and basically involves copy/pasting the hostname verifier code from httpclient directly into ZK, so it would be pretty easy to backport to #184 if desired.

Let me just do another pass over the differences between the two PRs and see if anything else jumps out at me.

anmolnar · 2018-09-25T10:56:45Z

@ivmaykov

portUnification
Let's just disable the feature completely in this patch: don't parse the config option to be on the safe side. Rolling upgrade will be supported once we consider it stable.

perf regression
What exactly the code snippet which needs to be copied over and where? Sounds like an easy fix to this patch.

@hanm Please start reviewing the code if you have some free cycles.

anmolnar · 2018-09-25T11:02:13Z

@eolivelli Looks like I triggered the build with my latest comment. Maybe my comment to @hanm :)

asfgit · 2018-09-25T11:16:51Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2259/

hanm · 2018-09-26T02:17:58Z

@anmolnar sure, will sign this off before next monday.

hanm

had one pass on code, just a few nits on styling issues, and one question about the conditions on host name verification. will have another pass on test code tomorrow.

hanm · 2018-09-26T21:40:48Z

src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java

-            setSockOpts(sock);
-            sock.connect(electionAddr, cnxTO);
-            LOG.debug("Connected to server " + sid);
+             LOG.debug("Opening channel to server " + sid);


nit: indentations on this line and the following lines

hanm · 2018-09-26T21:41:01Z

src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java

+                 sock = new Socket();
+                 setSockOpts(sock);
+                 sock.connect(electionAddr, cnxTO);
+


nit: remove extra line here.

hanm · 2018-09-26T21:42:51Z

src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java

+            } else { initiateConnection(sock, sid);
+            } return true;
+         } catch (UnresolvedAddressException e) {
+             // Sun doesn't include the address that causes this


nit: indentations

hanm · 2018-09-26T21:43:49Z

src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java

+             LOG.warn("Cannot open channel to " + sid
+                     + " at election address " + electionAddr, e);
+             closeSocket(sock);
+             throw e;} catch (X509Exception e) {


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

hanm · 2018-09-26T23:11:32Z

src/java/test/org/apache/zookeeper/test/QuorumSSLTest.java

+ * 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
+ * <p/>


shouldn't this be <p> (we already have a closing tag <p/>)?

hanm · 2018-09-27T22:57:42Z

src/java/main/org/apache/zookeeper/common/ZKTrustManager.java

+            try {
+                performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+            } catch (UnknownHostException e) {
+                throw new CertificateException("failed to verify host", e);


nit: Failed

hanm · 2018-09-27T22:58:12Z

src/java/main/org/apache/zookeeper/common/ZKTrustManager.java

+            hostnameVerifier.verify(hostAddress, certificate);
+        } catch (SSLException addressVerificationException) {
+            try {
+                LOG.debug("Failed to verify host address: " + hostAddress +


nit: use parameterized logging.

hanm · 2018-09-27T22:58:19Z

src/java/main/org/apache/zookeeper/common/ZKTrustManager.java

+                hostnameVerifier.verify(hostName, certificate);
+            } catch (SSLException hostnameVerificationException) {
+                LOG.error("Failed to verify host address: " + hostAddress, addressVerificationException);
+                LOG.error("Failed to verify hostname: " + hostName, hostnameVerificationException);


nit: use parameterized logging.

hanm · 2018-09-27T22:59:36Z

src/java/main/org/apache/zookeeper/common/ZKTrustManager.java

+            } catch (SSLException hostnameVerificationException) {
+                LOG.error("Failed to verify host address: " + hostAddress, addressVerificationException);
+                LOG.error("Failed to verify hostname: " + hostName, hostnameVerificationException);
+                throw new CertificateException("Failed to verify both host address and host name", hostnameVerificationException);


it might be useful to log inetAddress as well in cases where both hostName and hostAddress ends up empty because of exceptions.

Both hostAddress and hostName are logged (the 2 entities which was probed). What else should be logged?

I was thinking the inetAddress should be logged to provide additional information when both hostAddress and hostName fails to resolve.

hanm · 2018-09-27T23:17:18Z

src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java

+            } else if (key.equals("sslQuorum")){
+                sslQuorum = Boolean.parseBoolean(value);
+            } else if (key.equals("portUnification")){
+                shouldUsePortUnification = Boolean.parseBoolean(value);


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

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.

ivmaykov · 2018-09-28T20:05:54Z

@anmolnar you just need to copy ZKTrustManager.java from #627 to #184, and remove the dependency on httpclient from build.xml and ivy.xml. And comment out the 2 lines of code in QuorumPeerConfig.java that parse the "portUnification" config property and set the instance variable shouldUsePortUnification.

ivmaykov · 2018-09-28T20:06:34Z

build.xml

@@ -38,11 +38,15 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">

    <property name="netty.version" value="3.10.6.Final"/>

+    <property name="httpcomponents.version" value="4.5.3"/>


As discussed, let's remove this dependency.

ivmaykov · 2018-09-28T20:06:56Z

ivy.xml

@@ -63,6 +63,10 @@
      <artifact name="netty" type="jar" conf="default"/>
    </dependency>

+    <dependency org="org.apache.httpcomponents" name="httpclient" rev="${httpcomponents.version}">


ivmaykov · 2018-09-28T20:09:31Z

src/java/main/org/apache/zookeeper/common/ZKTrustManager.java

+ * 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 {


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 ...

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?

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?

Maybe:

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

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.

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?).

ivmaykov · 2018-09-28T20:15:02Z

src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java

+            } else if (key.equals("sslQuorum")){
+                sslQuorum = Boolean.parseBoolean(value);
+            } else if (key.equals("portUnification")){
+                shouldUsePortUnification = Boolean.parseBoolean(value);


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.

asfgit · 2018-10-01T11:44:29Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2296/

asfgit · 2018-10-01T12:55:19Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2297/

anmolnar · 2018-10-03T14:34:46Z

@ivmaykov @hanm I'm done with the changes.
Let's see if we can get a green build.

asfgit · 2018-10-03T15:00:58Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2335/

ivmaykov · 2018-10-04T21:42:23Z

I'm not a committer so my +1 doesn't count for much, but this PR looks good to me. I think we should get it in ASAP and then try to get my changes in (as several separate PRs, I will need to break them up) hopefully before 3.5 ships so people can have PEM support, cert reloading, and port unification.

hanm · 2018-10-04T22:16:42Z

I'm not a committer so my +1 doesn't count for much

It counts a lot, thanks for spending time not just review the patch but also test it in production!

hanm

LGTM with latest changes.

@anmolnar are there any remaining works you plan to do on this PR? I think we are good to land but I'd like to double check before I commit this. thanks

anmolnar

+1 no more plans, good to go.
I'll commit just to make things faster. (not my PR anyway :)

This is a work in progress, I wanted to get some feedback from the community while I worked on this. Please do not merge yet. Tests, documentation, and some cleanup still coming. This is a first pass at ssl support for the zookeeper quorum. It supports encrypting both leader election and normal operation. Rolling upgrades are supported via port unification (`portUnification=true`). This should only be used while performing a rolling upgrade. Some open questions: - Anyone have any ideas for better names for the configuration options (`sslQuorum` and `portUnification` currently). - I am using the same configuration that points to the truststore/keystore used for server <-> client ssl. Do they need to be separate? - Is port unification the correct approach for rolling upgrades? Is the impact from the use of `BufferedSocket`s during the upgrade acceptable? See: http://stackoverflow.com/questions/25637039/detecting-ssl-connection-and-converting-socket-to-sslsocket http://stackoverflow.com/questions/6559859/is-it-possible-to-change-plain-socket-to-sslsocket - server <-> client ssl is implemented with netty. I did not feel that rewriting our server <-> server logic with netty was necessary given how easy ssl was to implement with standard java `SSLSocket`s. Any arguments to the contrary? Thanks, Abe Author: Andor Molnar <andor@cloudera.com> Author: Andor Molnar <andor@apache.org> Reviewers: hanm@apache.org, andor@apache.org Closes #184 from afine/ZOOKEEPER-236 and squashes the following commits: fdcc915 [Andor Molnar] ZOOKEEPER-236. Replaced DefaultHostnameVerifier with custom impl c014a54 [Andor Molnar] ZOOKEEPER-236. Temporary disabled portUnification support e414496 [Andor Molnar] ZOOKEEPER-236. Nit code review fixes 209fbca [Andor Molnar] ZOOKEEPER-236. Added new JMX properties to expose SSL quorum related settings 1f8aab0 [Andor Molnar] ZOOKEEPER-236. Revert portUnification/sslQuorum logic a9fa698 [Andor Molnar] ZOOKEEPER-236. Code review fixes: 777f31a [Andor Molnar] ZOOKEEPER-236. Added Java8/Java9 default cipher suites e8a1729 [Andor Molnar] ZOOKEEPER-236. Reverted to use single property for hostname verification d64eb26 [Andor Molnar] ZOOKEEPER-236. Code review related changes: - server & client hostname verification can be set independently, - refactor defaultSSLContext to use AtomicReference, - some minor nitpicks 9ab476a [Andor Molnar] ZOOKEEPER-236. Trying to fix cipher suites test by changing the default protocol to TLSv1.2 and filter suitable cipher suites ed10e88 [Andor Molnar] ZOOKEEPER-236. Added cipher suite to test to run on CentOS. Timeout in constant value. Null checks c452d1b [Andor Molnar] ZOOKEEPER-236. Fixed unit test + added some extra debug logging 88b6171 [Andor Molnar] ZOOKEEPER-236: SSL Support for Atomic Broadcast protocol

This is a work in progress, I wanted to get some feedback from the community while I worked on this. Please do not merge yet. Tests, documentation, and some cleanup still coming. This is a first pass at ssl support for the zookeeper quorum. It supports encrypting both leader election and normal operation. Rolling upgrades are supported via port unification (`portUnification=true`). This should only be used while performing a rolling upgrade. Some open questions: - Anyone have any ideas for better names for the configuration options (`sslQuorum` and `portUnification` currently). - I am using the same configuration that points to the truststore/keystore used for server <-> client ssl. Do they need to be separate? - Is port unification the correct approach for rolling upgrades? Is the impact from the use of `BufferedSocket`s during the upgrade acceptable? See: http://stackoverflow.com/questions/25637039/detecting-ssl-connection-and-converting-socket-to-sslsocket http://stackoverflow.com/questions/6559859/is-it-possible-to-change-plain-socket-to-sslsocket - server <-> client ssl is implemented with netty. I did not feel that rewriting our server <-> server logic with netty was necessary given how easy ssl was to implement with standard java `SSLSocket`s. Any arguments to the contrary? Thanks, Abe Author: Andor Molnar <andor@cloudera.com> Author: Andor Molnar <andor@apache.org> Reviewers: hanm@apache.org, andor@apache.org Closes apache#184 from afine/ZOOKEEPER-236 and squashes the following commits: fdcc915 [Andor Molnar] ZOOKEEPER-236. Replaced DefaultHostnameVerifier with custom impl c014a54 [Andor Molnar] ZOOKEEPER-236. Temporary disabled portUnification support e414496 [Andor Molnar] ZOOKEEPER-236. Nit code review fixes 209fbca [Andor Molnar] ZOOKEEPER-236. Added new JMX properties to expose SSL quorum related settings 1f8aab0 [Andor Molnar] ZOOKEEPER-236. Revert portUnification/sslQuorum logic a9fa698 [Andor Molnar] ZOOKEEPER-236. Code review fixes: 777f31a [Andor Molnar] ZOOKEEPER-236. Added Java8/Java9 default cipher suites e8a1729 [Andor Molnar] ZOOKEEPER-236. Reverted to use single property for hostname verification d64eb26 [Andor Molnar] ZOOKEEPER-236. Code review related changes: - server & client hostname verification can be set independently, - refactor defaultSSLContext to use AtomicReference, - some minor nitpicks 9ab476a [Andor Molnar] ZOOKEEPER-236. Trying to fix cipher suites test by changing the default protocol to TLSv1.2 and filter suitable cipher suites ed10e88 [Andor Molnar] ZOOKEEPER-236. Added cipher suite to test to run on CentOS. Timeout in constant value. Null checks c452d1b [Andor Molnar] ZOOKEEPER-236. Fixed unit test + added some extra debug logging 88b6171 [Andor Molnar] ZOOKEEPER-236: SSL Support for Atomic Broadcast protocol

anmolnar · 2018-10-05T12:37:57Z

@ivmaykov It was great help reviewing this huge PR. Thanks a lot!
Merged to 3.5 branch, master is ongoing. I need to rebase it first, because Maven migration was committed in the meantime.
@afine Please close this PR.

This is a work in progress, I wanted to get some feedback from the community while I worked on this. Please do not merge yet. Tests, documentation, and some cleanup still coming. This is a first pass at ssl support for the zookeeper quorum. It supports encrypting both leader election and normal operation. Rolling upgrades are supported via port unification (`portUnification=true`). This should only be used while performing a rolling upgrade. Some open questions: - Anyone have any ideas for better names for the configuration options (`sslQuorum` and `portUnification` currently). - I am using the same configuration that points to the truststore/keystore used for server <-> client ssl. Do they need to be separate? - Is port unification the correct approach for rolling upgrades? Is the impact from the use of `BufferedSocket`s during the upgrade acceptable? See: http://stackoverflow.com/questions/25637039/detecting-ssl-connection-and-converting-socket-to-sslsocket http://stackoverflow.com/questions/6559859/is-it-possible-to-change-plain-socket-to-sslsocket - server <-> client ssl is implemented with netty. I did not feel that rewriting our server <-> server logic with netty was necessary given how easy ssl was to implement with standard java `SSLSocket`s. Any arguments to the contrary? Thanks, Abe Author: Andor Molnar <andor@cloudera.com> Author: Andor Molnar <andor@apache.org> Reviewers: hanm@apache.org, andor@apache.org Closes apache#184 from afine/ZOOKEEPER-236 and squashes the following commits: fdcc915 [Andor Molnar] ZOOKEEPER-236. Replaced DefaultHostnameVerifier with custom impl c014a54 [Andor Molnar] ZOOKEEPER-236. Temporary disabled portUnification support e414496 [Andor Molnar] ZOOKEEPER-236. Nit code review fixes 209fbca [Andor Molnar] ZOOKEEPER-236. Added new JMX properties to expose SSL quorum related settings 1f8aab0 [Andor Molnar] ZOOKEEPER-236. Revert portUnification/sslQuorum logic a9fa698 [Andor Molnar] ZOOKEEPER-236. Code review fixes: 777f31a [Andor Molnar] ZOOKEEPER-236. Added Java8/Java9 default cipher suites e8a1729 [Andor Molnar] ZOOKEEPER-236. Reverted to use single property for hostname verification d64eb26 [Andor Molnar] ZOOKEEPER-236. Code review related changes: - server & client hostname verification can be set independently, - refactor defaultSSLContext to use AtomicReference, - some minor nitpicks 9ab476a [Andor Molnar] ZOOKEEPER-236. Trying to fix cipher suites test by changing the default protocol to TLSv1.2 and filter suitable cipher suites ed10e88 [Andor Molnar] ZOOKEEPER-236. Added cipher suite to test to run on CentOS. Timeout in constant value. Null checks c452d1b [Andor Molnar] ZOOKEEPER-236. Fixed unit test + added some extra debug logging 88b6171 [Andor Molnar] ZOOKEEPER-236: SSL Support for Atomic Broadcast protocol

ivmaykov · 2018-10-15T21:41:05Z

@anmolnar any ETA on merging this to master? I'd like to break up #627 into several parts with associated JIRAs and start the process on getting that code upstream, but need this code in master first.

afine force-pushed the ZOOKEEPER-236 branch 2 times, most recently from 937c39a to 54f60f2 Compare March 10, 2017 22:23

afine force-pushed the ZOOKEEPER-236 branch 3 times, most recently from a192293 to bebe096 Compare March 17, 2017 22:19

geek101 reviewed Mar 30, 2017

View reviewed changes

hanm reviewed Apr 6, 2017

View reviewed changes

geek101 reviewed Apr 6, 2017

View reviewed changes

afine force-pushed the ZOOKEEPER-236 branch from b13c686 to 5e8182d Compare April 7, 2017 19:47

hanm reviewed Apr 13, 2017

View reviewed changes

afine force-pushed the ZOOKEEPER-236 branch from 078b23e to be07dfd Compare April 14, 2017 17:23

afine changed the title ~~ZOOKEEPER-236: SSL Support for Atomic Broadcast protocol [DO NOT MERGE]~~ ZOOKEEPER-236: SSL Support for Atomic Broadcast protocol Apr 14, 2017

afine force-pushed the ZOOKEEPER-236 branch 3 times, most recently from d1a150d to 08d2981 Compare April 20, 2017 23:16

hanm reviewed Apr 25, 2017

View reviewed changes

afine mentioned this pull request Apr 28, 2017

ZOOKEEPER-2750: Document SSL Support for Atomic Broadcast protocol [WIP] #245

Closed

hanm suggested changes Sep 28, 2018

View reviewed changes

ivmaykov reviewed Sep 28, 2018

View reviewed changes

ZOOKEEPER-236. Nit code review fixes

e414496

ZOOKEEPER-236. Temporary disabled portUnification support

c014a54

ZOOKEEPER-236. Replaced DefaultHostnameVerifier with custom impl

fdcc915

hanm approved these changes Oct 4, 2018

View reviewed changes

anmolnar approved these changes Oct 5, 2018

View reviewed changes

anmolnar closed this Jan 30, 2019

anmolnar deleted the ZOOKEEPER-236 branch February 21, 2019 15:11

		@@ -38,11 +38,15 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">

		<property name="netty.version" value="3.10.6.Final"/>

		<property name="httpcomponents.version" value="4.5.3"/>

ZOOKEEPER-236: SSL Support for Atomic Broadcast protocol #184

ZOOKEEPER-236: SSL Support for Atomic Broadcast protocol #184

Conversation

afine commented Mar 6, 2017

Choose a reason for hiding this comment

afine Mar 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enixon commented Sep 17, 2018

ivmaykov commented Sep 18, 2018 • edited

anmolnar commented Sep 25, 2018

anmolnar commented Sep 25, 2018

asfgit commented Sep 25, 2018

hanm commented Sep 26, 2018

hanm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmaykov commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afine Mar 31, 2017 •

edited

ivmaykov commented Sep 18, 2018 •

edited