ZOOKEEPER-3160: Custom User SSLContext #728

arankin-irl · 2018-12-03T11:15:07Z

This is a master branch version of: #654

The previous PR was for branch 3.5, and couldn't be merged as that branch is closed for new features.

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:

The user has a hardware keystore, loaded in using PKCS11 or something similar.
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.

Master Merge

arankin-irl · 2018-12-11T10:57:56Z

@nkalmar - just drawing your attention to this after our discussion on #654

nkalmar · 2018-12-11T11:09:36Z

Thanks @arankin-irl , I will review it (I'm guessing it's the same as for 3.5, so it will be quick), but I am not a committer, I won't be able to merge your PR :(

nkalmar

8000

LGTM, already reviewed for 3.5 previously. Thanks!

anmolnar

Nice catch @arankin-irl !

One tiny suggestion: instead of wrapping the original behaviour in an if-else branch, I'd rather create a new class which implements your interface ZKClientSSLContext and call it for example DefaultClientSSLContext.

In the createSSLContext method you can try to instantiate the class from the new property value and if it's not provided, use the "default" class.

If the property value is provided, but cannot be instantiated for some reason, throw an error, don't fallback to the default impl. (which you already do correctly)

anmolnar · 2018-12-11T12:54:14Z

@ivmaykov You might be interested to review this patch too.

arankin-irl · 2018-12-11T13:47:20Z

One tiny suggestion: instead of wrapping the original behaviour in an if-else branch, I'd rather create a new class which implements your interface ZKClientSSLContext and call it for example DefaultClientSSLContext.

@anmolnar - I did have a look at doing something similar to that, but the X509Util methods and variables feature heavily in the creation of the SSLContext. Moving these to a DefaultClientSSLContext looks like it'd be a fair rewrite of the class.

Then again, I'm not really a fan of the current if/else situation - however, maybe an acceptable solution would be to move the else branch to a new method, like createSSLContextFromConfig()?

anmolnar · 2018-12-11T19:21:20Z

@arankin-irl Got it, makes sense. I like the new method idea.

arankin-irl · 2018-12-12T12:42:18Z

@anmolnar - I've updated the PR with the extracted method.

anmolnar

+1 Thanks @arankin-irl !

anmolnar · 2019-01-03T15:21:34Z

@arankin-irl Please rebase on top of current master.
@ivmaykov @hanm Would you please take a quick a look at this patch? I'd like to get your support to confidently merge it.

Merging in latest master.

# Conflicts: # zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java

arankin-irl · 2019-01-03T16:18:05Z

@anmolnar - should be up to date now.

ivmaykov · 2019-01-12T00:24:03Z

I can review this early next week, please wait for that before merging :)

ivmaykov

Mostly looks good to me, have some style suggestions.

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

zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKClientSSLContext.java

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

zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java

zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTestClientSSLContext.java

Merging latest master

…operty

arankin-irl · 2019-01-17T19:36:54Z

@ivmaykov - Updated the PR to resolve the comments. I stuck with the nested classes in the tests since package-private classes couldn't be instantiated with reflection.

ivmaykov

A few more nits.

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

zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java

zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java

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

zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java

ivmaykov · 2019-01-17T21:41:35Z

+1 (non-binding), but please address nits from my last review pass. Great work :)

arankin-irl · 2019-01-18T09:32:57Z

@ivmaykov @anmolnar - Addressed the comments and updated the PR.

ivmaykov

+1 (non-binding) with some minor nits

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

zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java

anmolnar · 2019-01-21T17:47:27Z

@arankin-irl Please let me know once you've finished polishing your patch.

arankin-irl · 2019-01-22T07:49:40Z

@ivmaykov @anmolnar - PR updated with the requested changes.

ivmaykov · 2019-01-24T18:52:05Z

@arankin-irl you will need to rebase now that #681 landed. Let me know if you run into hard-to-resolve merge conflicts.

Merging Latest Master

# Conflicts: # zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java

arankin-irl · 2019-01-25T11:36:31Z

@ivmaykov @anmolnar - PR has been updated to resolve merge conflicts. I've removed the Null SSLContext test, as the SSLContextOptions now checks if the SSLContext is null (and there's a test around that already).

anmolnar · 2019-01-25T13:32:58Z

Committed to master branch.
Thanks @arankin-irl !

anmolnar · 2019-01-25T13:34:23Z

@arankin-irl You should be assigned to the Jira.
Would you please share your Jira username and I can add it to the contributors list and assign Jira to you?

arankin-irl · 2019-01-25T13:42:15Z

@arankin-irl You should be assigned to the Jira.
Would you please share your Jira username and I can add it to the contributors list and assign Jira to you?

My Jira username is Kenco.

This is a master branch version of: apache#654 The previous PR was for branch 3.5, and couldn't be merged as that branch is closed for new features. 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. Author: Alex Rankin <davelister@gmail.com> Author: Alex Rankin <alex.rankin@mastercard.com> Reviewers: andor@apache.org Closes apache#728 from arankin-irl/ZOOKEEPER-3160 and squashes the following commits: a20c62f [Alex Rankin] Merge branch 'master' into ZOOKEEPER-3160 5a9b8fc [Alex Rankin] Merge pull request #7 from apache/master 3c3dfdd [Alex Rankin] Re-ordering imports. 69e0b6c [Alex Rankin] Updating custom SSLContext supplier with review comments 874529b [Alex Rankin] Using supplier interface instead of custom interface, and renaming property ec27260 [Alex Rankin] Merge branch 'master' into ZOOKEEPER-3160 75a010e [Alex Rankin] Merge pull request #6 from apache/master 838f61c [Alex Rankin] Merge branch 'master' into ZOOKEEPER-3160 f85d7e5 [Alex Rankin] Merge pull request #5 from apache/master 31d8dd5 [Alex Rankin] Extracting SSLContext creation from config to new method. 400839a [Alex Rankin] Adding ability to specify custom SSLContext for client 7ae7485 [Alex Rankin] Merge pull request #4 from apache/master

arankin-irl added 2 commits December 3, 2018 10:27

Merge pull request #4 from apache/master

7ae7485

Master Merge

Adding ability to specify custom SSLContext for client

400839a

arankin-irl closed this Dec 3, 2018

arankin-irl reopened this Dec 3, 2018

arankin-irl closed this Dec 3, 2018

arankin-irl reopened this Dec 3, 2018

arankin-irl closed this Dec 3, 2018

arankin-irl reopened this Dec 3, 2018

nkalmar approved these changes Dec 11, 2018

View reviewed changes

anmolnar requested changes Dec 11, 2018

View reviewed changes

Extracting SSLContext creation from config to new method.

31d8dd5

anmolnar approved these changes Dec 12, 2018

View reviewed changes

arankin-irl added 2 commits January 3, 2019 15:30

Merge pull request #5 from apache/master

f85d7e5

Merging in latest master.

Merge branch 'master' into ZOOKEEPER-3160

838f61c

# Conflicts: # zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java

ivmaykov reviewed Jan 17, 2019

View reviewed changes

arankin-irl added 3 commits January 17, 2019 10:07

Merge pull request #6 from apache/master

75a010e

Merging latest master

Merge branch 'master' into ZOOKEEPER-3160

ec27260

Using supplier interface instead of custom interface, and renaming pr…

874529b

…operty

ivmaykov reviewed Jan 17, 2019

View reviewed changes

Updating custom SSLContext supplier with review comments

69e0b6c

ivmaykov approved these changes Jan 19, 2019

View reviewed changes

Re-ordering imports.

3c3dfdd

arankin-irl added 2 commits January 25, 2019 10:29

Merge pull request #7 from apache/master

5a9b8fc

Merging Latest Master

Merge branch 'master' into ZOOKEEPER-3160

a20c62f

# Conflicts: # zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java

asfgit closed this in 045833b Jan 25, 2019

ZOOKEEPER-3160: Custom User SSLContext #728

ZOOKEEPER-3160: Custom User SSLContext #728

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!