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/12/03 11:15:12 UTC
[GitHub] zookeeper pull request #728: ZOOKEEPER-3160: Custom User SSLContext
GitHub user arankin-irl opened a pull request:
https://github.com/apache/zookeeper/pull/728
ZOOKEEPER-3160: Custom User SSLContext
This is a master branch version of: https://github.com/apache/zookeeper/pull/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.
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/728.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 #728
----
commit 7ae74851b8e14bcae80d4eaa1141e076e3953fa6
Author: Alex Rankin <da...@...>
Date: 2018-12-03T10:27:35Z
Merge pull request #4 from apache/master
Master Merge
commit 400839a60ff3bd5a4af60710fbd07ce4ae5601a0
Author: Alex Rankin <da...@...>
Date: 2018-12-03T11:12:19Z
Adding ability to specify custom SSLContext for client
----
---
[GitHub] zookeeper pull request #728: 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/728
---
[GitHub] zookeeper pull request #728: 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/728
---
[GitHub] zookeeper pull request #728: ZOOKEEPER-3160: Custom User SSLContext
Posted by arankin-irl <gi...@git.apache.org>.
GitHub user arankin-irl reopened a pull request:
https://github.com/apache/zookeeper/pull/728
ZOOKEEPER-3160: Custom User SSLContext
This is a master branch version of: https://github.com/apache/zookeeper/pull/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.
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/728.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 #728
----
commit 7ae74851b8e14bcae80d4eaa1141e076e3953fa6
Author: Alex Rankin <da...@...>
Date: 2018-12-03T10:27:35Z
Merge pull request #4 from apache/master
Master Merge
commit 400839a60ff3bd5a4af60710fbd07ce4ae5601a0
Author: Alex Rankin <da...@...>
Date: 2018-12-03T11:12:19Z
Adding ability to specify custom SSLContext for client
----
---
[GitHub] zookeeper issue #728: 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/728
@anmolnar - I've updated the PR with the extracted method.
---
[GitHub] zookeeper issue #728: 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/728
@nkalmar - just drawing your attention to this after our discussion on https://github.com/apache/zookeeper/pull/654
---
[GitHub] zookeeper pull request #728: ZOOKEEPER-3160: Custom User SSLContext
Posted by arankin-irl <gi...@git.apache.org>.
GitHub user arankin-irl reopened a pull request:
https://github.com/apache/zookeeper/pull/728
ZOOKEEPER-3160: Custom User SSLContext
This is a master branch version of: https://github.com/apache/zookeeper/pull/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.
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/728.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 #728
----
commit 7ae74851b8e14bcae80d4eaa1141e076e3953fa6
Author: Alex Rankin <da...@...>
Date: 2018-12-03T10:27:35Z
Merge pull request #4 from apache/master
Master Merge
commit 400839a60ff3bd5a4af60710fbd07ce4ae5601a0
Author: Alex Rankin <da...@...>
Date: 2018-12-03T11:12:19Z
Adding ability to specify custom SSLContext for client
----
---
[GitHub] zookeeper pull request #728: 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/728
---
[GitHub] zookeeper issue #728: 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/728
> 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()`?
---
[GitHub] zookeeper pull request #728: ZOOKEEPER-3160: Custom User SSLContext
Posted by arankin-irl <gi...@git.apache.org>.
GitHub user arankin-irl reopened a pull request:
https://github.com/apache/zookeeper/pull/728
ZOOKEEPER-3160: Custom User SSLContext
This is a master branch version of: https://github.com/apache/zookeeper/pull/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.
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/728.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 #728
----
commit 7ae74851b8e14bcae80d4eaa1141e076e3953fa6
Author: Alex Rankin <da...@...>
Date: 2018-12-03T10:27:35Z
Merge pull request #4 from apache/master
Master Merge
commit 400839a60ff3bd5a4af60710fbd07ce4ae5601a0
Author: Alex Rankin <da...@...>
Date: 2018-12-03T11:12:19Z
Adding ability to specify custom SSLContext for client
----
---
[GitHub] zookeeper issue #728: ZOOKEEPER-3160: Custom User SSLContext
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/728
@ivmaykov You might be interested to review this patch too.
---
[GitHub] zookeeper issue #728: 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/728
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 :(
---