You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by roodkcab <gi...@git.apache.org> on 2018/09/21 14:47:26 UTC
[GitHub] zookeeper pull request #639: ZOOKEEPER-2122: add openssl to zookeeper c clie...
GitHub user roodkcab opened a pull request:
https://github.com/apache/zookeeper/pull/639
ZOOKEEPER-2122: add openssl to zookeeper c client to support ssl
I have some problem with cmake, I was intend to find openssl as dependency using pkg_search_module, but it didn't work. so I have to set the include and lib directory in /opt/local, can anyone help me on this ? thanks.
I made a wrong pull request to branch-3.5 #625, this one is a correction.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/roodkcab/zookeeper master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/639.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #639
----
commit 7ecbcbc14f99a43f1c7b226e05acc171f72e1951
Author: buaacss <sr...@...>
Date: 2018-09-17T16:31:49Z
refine zhandle fd
commit cedd176b0189a4a4ddf5252dcb4435011ec4e8dd
Author: 陈硕实 <bi...@...>
Date: 2018-09-19T09:55:51Z
refine cli.c opt to getopt
commit 9a06457cf9f543bdd005fe5565f4a79c980ff3ea
Author: buaacss <sr...@...>
Date: 2018-09-21T14:40:13Z
refine with getopt
----
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by roodkcab <gi...@git.apache.org>.
Github user roodkcab commented on the issue:
https://github.com/apache/zookeeper/pull/639
a problem is when the network get disconnect, the client will enter into two state, one is ZOO_EXPIRED_SESSION_STATE, the other is ZOO_CONNECTING_STATE, the problem is the client didn't reconnect to server when enter into ZOO_CONNECTING_STATE. I'll have a check on this
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/639
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2258/
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/639
@eolivelli maybe, but it works. i think the trigger phrase was looking for a substring match only.
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/639
@roodkcab we could exclude some of the files for `releaseaudit` target such as cert files (or even zoo.cfg) where adding Apache License header does not make any sense. See [here](https://github.com/apache/zookeeper/blob/master/build.xml#L1722) for how to
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/639
@roodkcab Some new files are missing apache license header, which leads to release audit failure:
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2245/artifact/patchprocess/patchReleaseAuditProblems.txt
[rat:report] !????? /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build/build/zookeeper-3.6.0-SNAPSHOT/src/zookeeper-client/zookeeper-client-c/ssl/cert_creator.sh
[rat:report] !????? /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build/build/zookeeper-3.6.0-SNAPSHOT/src/zookeeper-client/zookeeper-client-c/ssl/client.crt
[rat:report] !????? /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build/build/zookeeper-3.6.0-SNAPSHOT/src/zookeeper-client/zookeeper-client-c/ssl/client.csr
[rat:report] !????? /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build/build/zookeeper-3.6.0-SNAPSHOT/src/zookeeper-client/zookeeper-client-c/ssl/client.pkcs12
[rat:report] !????? /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build/build/zookeeper-3.6.0-SNAPSHOT/src/zookeeper-client/zookeeper-client-c/ssl/root.crt
[rat:report] !????? /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build/build/zookeeper-3.6.0-SNAPSHOT/src/zookeeper-client/zookeeper-client-c/ssl/root.srl
[rat:report] !????? /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build/build/zookeeper-3.6.0-SNAPSHOT/src/zookeeper-client/zookeeper-client-c/ssl/server.crt
[rat:report] !????? /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build/build/zookeeper-3.6.0-SNAPSHOT/src/zookeeper-client/zookeeper-client-c/ssl/server.csr
[rat:report] !????? /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build/build/zookeeper-3.6.0-SNAPSHOT/src/zookeeper-client/zookeeper-client-c/ssl/server.pkcs12
[rat:report] !????? /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build/build/zookeeper-3.6.0-SNAPSHOT/src/zookeeper-client/zookeeper-client-c/tests/zoo.cfg
Lines that start with ????? in the release audit report indicate files that do not have an Apache license header.
Can you please add apache license header to these files? You can find the header in any of existing source files.
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/639
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2245/
---
[GitHub] zookeeper pull request #639: ZOOKEEPER-2122: add openssl to zookeeper c clie...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/639#discussion_r220134951
--- Diff: zookeeper-client/zookeeper-client-c/CMakeLists.txt ---
@@ -56,7 +56,7 @@ option(WANT_CPPUNIT "Enables CppUnit and tests" ${DEFAULT_WANT_CPPUNIT})
# SOCK_CLOEXEC
option(WANT_SOCK_CLOEXEC "Enables SOCK_CLOEXEC on sockets" OFF)
include(CheckSymbolExists)
-check_symbol_exists(SOCK_CLOEXEC sys/socket.h HAVE_SOCK_CLOEXEC)
+check_symbol_exists(zktest_runnerSOCK_CLOEXEC sys/socket.h HAVE_SOCK_CLOEXEC)
--- End diff --
Didn't you miss a whitespace here like `zktest_runner SOCK_CLOEXEC`?
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/639
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2257/
---
[GitHub] zookeeper pull request #639: ZOOKEEPER-2122: add openssl to zookeeper c clie...
Posted by roodkcab <gi...@git.apache.org>.
Github user roodkcab commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/639#discussion_r220168781
--- Diff: zookeeper-client/zookeeper-client-c/src/zookeeper.c ---
@@ -2213,6 +2259,72 @@ static socket_t zookeeper_connect(zhandle_t *zh,
LOG_DEBUG(LOGCALLBACK(zh), "[zk] connect()\n");
rc = connect(fd, (struct sockaddr *)addr, addr_len);
+#ifdef HAVE_OPENSSL_H
+ if (zh->fd->cert != NULL) {
+ SSL_CTX *ctx = NULL;
+ SSL *ssl = NULL;
+ const SSL_METHOD *method;
+
+ SSL_library_init();
--- End diff --
I think it's better to use OPENSSL_init_ssl(OPENSSL_INIT_LOAD_SSL_STRINGS | OPENSSL_INIT_LOAD_CRYPTO_STRINGS, NULL), SSL_library_init is for pre-1.1.0, I'll have a unit test for this and push again. Thanks!
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by roodkcab <gi...@git.apache.org>.
Github user roodkcab commented on the issue:
https://github.com/apache/zookeeper/pull/639
problem solved, I've already push the latest code into a new branch and will run more test cases until I can confirm it works.
---
[GitHub] zookeeper pull request #639: ZOOKEEPER-2122: add openssl to zookeeper c clie...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/639#discussion_r220136616
--- Diff: zookeeper-client/zookeeper-client-c/src/zookeeper.c ---
@@ -2213,6 +2259,72 @@ static socket_t zookeeper_connect(zhandle_t *zh,
LOG_DEBUG(LOGCALLBACK(zh), "[zk] connect()\n");
rc = connect(fd, (struct sockaddr *)addr, addr_len);
+#ifdef HAVE_OPENSSL_H
+ if (zh->fd->cert != NULL) {
+ SSL_CTX *ctx = NULL;
+ SSL *ssl = NULL;
+ const SSL_METHOD *method;
+
+ SSL_library_init();
--- End diff --
Which version of OpenSSL are you going to support?
According to the wiki: https://wiki.openssl.org/index.php/Library_Initialization
You need a different call to init openssl library as of version `1.1.0`
---
[GitHub] zookeeper pull request #639: ZOOKEEPER-2122: add openssl to zookeeper c clie...
Posted by roodkcab <gi...@git.apache.org>.
Github user roodkcab commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/639#discussion_r220159265
--- Diff: zookeeper-client/zookeeper-client-c/CMakeLists.txt ---
@@ -56,7 +56,7 @@ option(WANT_CPPUNIT "Enables CppUnit and tests" ${DEFAULT_WANT_CPPUNIT})
# SOCK_CLOEXEC
option(WANT_SOCK_CLOEXEC "Enables SOCK_CLOEXEC on sockets" OFF)
include(CheckSymbolExists)
-check_symbol_exists(SOCK_CLOEXEC sys/socket.h HAVE_SOCK_CLOEXEC)
+check_symbol_exists(zktest_runnerSOCK_CLOEXEC sys/socket.h HAVE_SOCK_CLOEXEC)
--- End diff --
I think this a typo actually..., I'll fix it.
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by roodkcab <gi...@git.apache.org>.
Github user roodkcab commented on the issue:
https://github.com/apache/zookeeper/pull/639
the early approach of adding ssl to the client uses a while loop to accomplish the certificate process between server and client, but it didn't check the connectivity, so when the client get a disconnect or refused by the server, it can't break the infinity loop. then I put such process after poll tells me the plain connection is ok (triway handshaking finished), and return a ZSSLCONNECTIONERROR on SSL_WANT_READ or SSL_WANT_WRITE, making zookeeper's event loop start over again, and everything works fine this time.
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/639
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2227/
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/639
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2274/
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/639
@hanm your phrase was wrong
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/639
thanks for update, @roodkcab . will take a look at this.
quick question, looks like you had prove it working [here](https://github.com/apache/zookeeper/pull/625#issuecomment-421330923). Do you think you can add a unit test? It might not be easy, but would be good if we can have a unit test around this feature for c client.
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/639
test this please
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by roodkcab <gi...@git.apache.org>.
Github user roodkcab commented on the issue:
https://github.com/apache/zookeeper/pull/639
@hanm no problem, I'll have a check later. I did find testKeyStore.jks and testTrustStore.jks in java's test directory, but I didn't find root.ca for both of them. I'd like to commit a tool for making ssl cert files and full double side cert files for both server side and client side in .jks and .crt format respectively.
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/639
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2228/
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by roodkcab <gi...@git.apache.org>.
Github user roodkcab commented on the issue:
https://github.com/apache/zookeeper/pull/639
@hanm no problem, I’ll add Apache license headers to zoo.cfg and cert_creator.sh, .crt and .csr and .pkcs12 files are certificate files, which should not add license headers, May I add a README in ssl folder and put license there?
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/639
retest this please
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/639
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2248/
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by roodkcab <gi...@git.apache.org>.
Github user roodkcab commented on the issue:
https://github.com/apache/zookeeper/pull/639
@hanm license header added for cert_creator.sh and ignore all others
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/639
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2262/
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by roodkcab <gi...@git.apache.org>.
Github user roodkcab commented on the issue:
https://github.com/apache/zookeeper/pull/639
@hanm unit test for ssl added.
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by roodkcab <gi...@git.apache.org>.
Github user roodkcab commented on the issue:
https://github.com/apache/zookeeper/pull/639
seems Jenkins says No... I’ll have a check on java’s test cert files later
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by roodkcab <gi...@git.apache.org>.
Github user roodkcab commented on the issue:
https://github.com/apache/zookeeper/pull/639
P.S. u have to compile openssl from source before u can do `cmake -DWITH_OPENSSL=$OPENSSL_INSTALL_DIRECTORY ../`. I tried to make openssl as a git submodule and let cmake to find it using find_package but didn't work, then I think it even better to make it as a option rather than make it as a submodule, since someone don't need it at all will get a binary without linking with ssl and crypto.
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/639
retest this please
---
[GitHub] zookeeper issue #639: ZOOKEEPER-2122: add openssl to zookeeper c client to s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/639
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2247/
---