You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/07/19 23:48:00 UTC

[GitHub] [zookeeper] sankalpbhatia opened a new pull request #1405: ZOOKEEPER-3893: Enhance documentation for property ssl.clientAuth

sankalpbhatia opened a new pull request #1405:
URL: https://github.com/apache/zookeeper/pull/1405


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] asfgit closed pull request #1405: ZOOKEEPER-3893: Enhance documentation for property ssl.clientAuth

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1405:
URL: https://github.com/apache/zookeeper/pull/1405


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] sankalpbhatia commented on a change in pull request #1405: ZOOKEEPER-3893: Enhance documentation for property ssl.clientAuth

Posted by GitBox <gi...@apache.org>.
sankalpbhatia commented on a change in pull request #1405:
URL: https://github.com/apache/zookeeper/pull/1405#discussion_r456972251



##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -1588,8 +1588,14 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
 
 * *ssl.clientAuth* and *ssl.quorum.clientAuth* :
     (Java system properties: **zookeeper.ssl.clientAuth** and **zookeeper.ssl.quorum.clientAuth**)
-    **New in 3.5.5:**
-    TBD
+    **New in 3.5.7:**

Review comment:
       I changed this to 3.5.7 because I believe this property was ignored until that version. 
   Please correct me if wrong. https://issues.apache.org/jira/browse/ZOOKEEPER-3674




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] sankalpbhatia commented on a change in pull request #1405: ZOOKEEPER-3893: Enhance documentation for property ssl.clientAuth

Posted by GitBox <gi...@apache.org>.
sankalpbhatia commented on a change in pull request #1405:
URL: https://github.com/apache/zookeeper/pull/1405#discussion_r461911837



##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -1588,8 +1588,14 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
 
 * *ssl.clientAuth* and *ssl.quorum.clientAuth* :
     (Java system properties: **zookeeper.ssl.clientAuth** and **zookeeper.ssl.quorum.clientAuth**)
-    **New in 3.5.5:**
-    TBD
+    **New in 3.5.7:**

Review comment:
       Thanks for your comment @symat . Let me try this out for 3.5.5 and 3.5.6 and will get back. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] sankalpbhatia commented on a change in pull request #1405: ZOOKEEPER-3893: Enhance documentation for property ssl.clientAuth

Posted by GitBox <gi...@apache.org>.
sankalpbhatia commented on a change in pull request #1405:
URL: https://github.com/apache/zookeeper/pull/1405#discussion_r471163076



##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -1588,8 +1588,14 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
 
 * *ssl.clientAuth* and *ssl.quorum.clientAuth* :
     (Java system properties: **zookeeper.ssl.clientAuth** and **zookeeper.ssl.quorum.clientAuth**)
-    **New in 3.5.5:**
-    TBD
+    **New in 3.5.7:**

Review comment:
       Sorry for the delay @symat. I was able to take some time and verify that this doesn't work with versions 3.5.5 and 3.5.6
   
   Here are the steps we can use to reproduce it with the following zk cfg
   
   ```
   dataDir=/usr/local/var/lib/zookeeper
   secureClientPort=2182
   clientPort=2181
   maxClientCnxns=0
   admin.enableServer=false
   serverCnxnFactory=org.apache.zookeeper.server.NettyServerCnxnFactory
   
   ssl.clientAuth=none
   ssl.keyStore.location=/path/to/keystore
   ssl.keyStore.password=password
   ssl.trustStore.location=/path/to/truststore
   ssl.trustStore.password=password
   serverCnxnFactory=org.apache.zookeeper.server.NettyServerCnxnFactory
   ```
   
   
   
   1) build the 3.5.5 source using command 
   ``
   git checkout release-3.5.5 && mvn clean install -DskipTests
   ``
   
   
   2) Start zk server 
   
   ``bin/zkServer.sh start-foreground /usr/local/Cellar/kafka/2.4.0/.bottle/etc/kafka/zookeeper.properties ``
   
   
   
   3) Try connecting using client 
   ```
   /Volumes/workplace/zookeeper/bin(f0fdd529) » export CLIENT_JVMFLAGS="
   -Dzookeeper.clientCnxnSocket=org.apache.zookeeper.ClientCnxnSocketNetty
   -Dzookeeper.client.secure=true
   -Dzookeeper.ssl.hostnameVerification=false
   -Dzookeeper.ssl.trustStore.location=/path/to/truststore
   -Dzookeeper.ssl.trustStore.password=changeit"
   ```
   
   ```
   ./zkCli.sh -server localhost:2182
   ```
   fails with exception  
   ```
   Exception caught
   io.netty.handler.codec.DecoderException: javax.net.ssl.SSLHandshakeException: null cert chain
   ```
   
   
   We get the same error for building with 3.5.6. Building and running the 3.5.7 source however, works with the same steps. 
   
   I did a bit of deep dive on the code and verified it was [ZOOKEEPER-3388](https://issues.apache.org/jira/browse/ZOOKEEPER-3388) which fixed this issue indeed. 
   
   The initSSL() method prior to ZK-3388 was always setting clientAuth=required through the code ```        sslEngine.setNeedClientAuth(true);```([Check PR for ZK-3388](https://github.com/apache/zookeeper/pull/944/files#diff-079ae166eaea30d08f04c1424e016a44L343-R471)). The pull request changed it to use the property derived from the config file. 
   
   To summarise, I think it is safe to say that this works only from 3.5.7 onwards only. 
   
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] symat commented on a change in pull request #1405: ZOOKEEPER-3893: Enhance documentation for property ssl.clientAuth

Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1405:
URL: https://github.com/apache/zookeeper/pull/1405#discussion_r461459949



##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -1588,8 +1588,14 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
 
 * *ssl.clientAuth* and *ssl.quorum.clientAuth* :
     (Java system properties: **zookeeper.ssl.clientAuth** and **zookeeper.ssl.quorum.clientAuth**)
-    **New in 3.5.5:**
-    TBD
+    **New in 3.5.7:**

Review comment:
       actually the original feature was present since 3.5.5, it was added in: https://issues.apache.org/jira/browse/ZOOKEEPER-3176
   
   In ZOOKEEPER-3674 it is said to not work. But I don't know if it was actually true. I found ZOOKEEPER-3388 what was actually released in 5.7 and touched this part. Maybe this is why it is said to be fixed in 3.5.7? I really don't know.
   
   With regards to the documentation, I'm happy to keep 3.5.5, unless someone can try with 3.5.5 and 3.5.6 to see if it is really broken.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] sankalpbhatia commented on a change in pull request #1405: ZOOKEEPER-3893: Enhance documentation for property ssl.clientAuth

Posted by GitBox <gi...@apache.org>.
sankalpbhatia commented on a change in pull request #1405:
URL: https://github.com/apache/zookeeper/pull/1405#discussion_r471171561



##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -1588,8 +1588,14 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
 
 * *ssl.clientAuth* and *ssl.quorum.clientAuth* :
     (Java system properties: **zookeeper.ssl.clientAuth** and **zookeeper.ssl.quorum.clientAuth**)
-    **New in 3.5.5:**
-    TBD
+    **New in 3.5.7:**

Review comment:
       Thanks for the suggestion. Changed
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] symat commented on a change in pull request #1405: ZOOKEEPER-3893: Enhance documentation for property ssl.clientAuth

Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1405:
URL: https://github.com/apache/zookeeper/pull/1405#discussion_r462217897



##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -1588,8 +1588,14 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
 
 * *ssl.clientAuth* and *ssl.quorum.clientAuth* :
     (Java system properties: **zookeeper.ssl.clientAuth** and **zookeeper.ssl.quorum.clientAuth**)
-    **New in 3.5.5:**
-    TBD
+    **New in 3.5.7:**

Review comment:
       that would be really valuable, thank you for taking the time on these tests!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1405: ZOOKEEPER-3893: Enhance documentation for property ssl.clientAuth

Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1405:
URL: https://github.com/apache/zookeeper/pull/1405#discussion_r471164510



##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -1588,8 +1588,14 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
 
 * *ssl.clientAuth* and *ssl.quorum.clientAuth* :
     (Java system properties: **zookeeper.ssl.clientAuth** and **zookeeper.ssl.quorum.clientAuth**)
-    **New in 3.5.5:**
-    TBD
+    **New in 3.5.7:**

Review comment:
       To make the historical record perfectly clear, you could have something like:
   
   ```suggestion
       **Added in 3.5.5, but broken until 3.5.7:**
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] symat commented on pull request #1405: ZOOKEEPER-3893: Enhance documentation for property ssl.clientAuth

Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1405:
URL: https://github.com/apache/zookeeper/pull/1405#issuecomment-678955222


   great, thanks @sankalpbhatia, I'll merge this today / soon


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org