You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by arankin-irl <gi...@git.apache.org> on 2018/10/02 16:15:58 UTC
[GitHub] zookeeper pull request #654: ZOOKEEPER-3160: Custom User SSLContext
GitHub user arankin-irl opened a pull request:
https://github.com/apache/zookeeper/pull/654
ZOOKEEPER-3160: Custom User SSLContext
The Zookeeper libraries currently allow you to set up your SSL Context via system properties such as "zookeeper.ssl.keyStore.location" in the X509Util. This covers most simple use cases, where users have software keystores on their harddrive.
There are, however, a few additional scenarios that this doesn't cover. Two possible ones would be:
1. The user has a hardware keystore, loaded in using PKCS11 or something similar.
2. The user has no access to the software keystore, but can retrieve an already-constructed SSLContext from their container.
For this, I would propose that the X509Util be extended to allow a user to set a property "zookeeper.ssl.client.context" to provide a class which supplies a custom SSL context. This gives a lot more flexibility to the ZK client, and allows the user to construct the SSLContext in whatever way they please (which also future proofs the implementation somewhat).
I added a few simple tests to this class around setting the SSLContext, and setting an invalid one. I'm not testing the actual functionality of the SSLContext, etc.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Mastercard/zookeeper ZOOKEEPER-3160
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/654.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 #654
----
commit a5b93cc70b867adca542d79d17126d30b2afbd27
Author: Alex Rankin <da...@...>
Date: 2018-10-02T15:52:12Z
Adding ability to specify custom SSLContext for client
----
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/654
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2329/
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by arankin-irl <gi...@git.apache.org>.
Github user arankin-irl commented on the issue:
https://github.com/apache/zookeeper/pull/654
@eolivelli - looks like the Jenkins build is failing due to the audit warnings, but I get a 404 when attempting to view the audit findings file.
---
[GitHub] zookeeper pull request #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/654#discussion_r222023353
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -85,60 +85,73 @@ public static SSLContext createSSLContext() throws SSLContextException {
}
public static SSLContext createSSLContext(ZKConfig config) throws SSLContextException {
- KeyManager[] keyManagers = null;
- TrustManager[] trustManagers = null;
+ if (config.getProperty(ZKConfig.SSL_CLIENT_CONTEXT) != null) {
+ LOG.debug("Loading SSLContext from property '" + ZKConfig.SSL_CLIENT_CONTEXT + "'");
+ String sslClientContextClass = config.getProperty(ZKConfig.SSL_CLIENT_CONTEXT);
+ try {
+ Class<?> sslContextClass = Class.forName(sslClientContextClass);
+ ZKClientSSLContext sslContext = (ZKClientSSLContext) sslContextClass.newInstance();
--- End diff --
newInstance is deprecated.
Can we use getConstructor().newInstance() ?
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on the issue:
https://github.com/apache/zookeeper/pull/654
Yes please. But it should be fairly easy, most of the time 3.5 and master are compatible. So just cherry-picking your commits on master should work, hopefully without merge conflicts.
If you need any help, let me know!
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by arankin-irl <gi...@git.apache.org>.
Github user arankin-irl commented on the issue:
https://github.com/apache/zookeeper/pull/654
Addressed merge conflicts.
@nkalmar - Just wondering if there's anything else I can do to help this get merged?
---
[GitHub] zookeeper pull request #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by arankin-irl <gi...@git.apache.org>.
Github user arankin-irl commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/654#discussion_r222028424
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -85,60 +85,73 @@ public static SSLContext createSSLContext() throws SSLContextException {
}
public static SSLContext createSSLContext(ZKConfig config) throws SSLContextException {
- KeyManager[] keyManagers = null;
- TrustManager[] trustManagers = null;
+ if (config.getProperty(ZKConfig.SSL_CLIENT_CONTEXT) != null) {
+ LOG.debug("Loading SSLContext from property '" + ZKConfig.SSL_CLIENT_CONTEXT + "'");
+ String sslClientContextClass = config.getProperty(ZKConfig.SSL_CLIENT_CONTEXT);
+ try {
+ Class<?> sslContextClass = Class.forName(sslClientContextClass);
+ ZKClientSSLContext sslContext = (ZKClientSSLContext) sslContextClass.newInstance();
--- End diff --
No problem - I've updated the PR.
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on the issue:
https://github.com/apache/zookeeper/pull/654
Unfortunately the link at the Jenkins build are wrong in some cases. Just navigate manually from build artifacts in the jenkins build.
findbugs for example:
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2645/artifact/build/test/findbugs/newPatchFindbugsWarnings.html
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by arankin-irl <gi...@git.apache.org>.
Github user arankin-irl commented on the issue:
https://github.com/apache/zookeeper/pull/654
Updated again to resolve merge conflicts.
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/654
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2471/
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by arankin-irl <gi...@git.apache.org>.
Github user arankin-irl commented on the issue:
https://github.com/apache/zookeeper/pull/654
Ah, I hadn't realised that 3.5 was closed for stabilisation. So I should recreate this PR for the master branch then?
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on the issue:
https://github.com/apache/zookeeper/pull/654
I just realized this is for 3.5. Did you create a PR for the master branch? 3.5 is pretty much closed for new features, in order to stabilize it for the stable release.
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by arankin-irl <gi...@git.apache.org>.
Github user arankin-irl commented on the issue:
https://github.com/apache/zookeeper/pull/654
Merged the latest 3.5 branch in - looks like there was a significant structure rework, along with some changes to the ZKConfig properties - moved the additional property in with the others in X509Util, and updated the tests, javadocs, etc.
Just curious - what would be the average timeline to get this merged in?
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/654
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2328/
---
[GitHub] zookeeper pull request #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by arankin-irl <gi...@git.apache.org>.
Github user arankin-irl closed the pull request at:
https://github.com/apache/zookeeper/pull/654
---
[GitHub] zookeeper pull request #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/654#discussion_r233916605
--- Diff: zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTestClientSSLContext.java ---
@@ -0,0 +1,12 @@
+package org.apache.zookeeper.common;
--- End diff --
Add apache header
---
[GitHub] zookeeper pull request #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/654#discussion_r233916877
--- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKClientSSLContext.java ---
@@ -0,0 +1,18 @@
+package org.apache.zookeeper.common;
--- End diff --
add Apache header
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by arankin-irl <gi...@git.apache.org>.
Github user arankin-irl commented on the issue:
https://github.com/apache/zookeeper/pull/654
Thanks @nkalmar - found the [audit problems](https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2645/artifact/patchprocess/patchReleaseAuditProblems.txt) file and resolved the missing license headers.
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by arankin-irl <gi...@git.apache.org>.
Github user arankin-irl commented on the issue:
https://github.com/apache/zookeeper/pull/654
That's grand then. I'll close this PR and reopen a new one for master then 👍
---
[GitHub] zookeeper issue #654: ZOOKEEPER-3160: Custom User SSLContext
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/654
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2470/
---