You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/01/12 17:59:02 UTC
[kudu-CR] rpc: allow using encrypted private key for SSL
Hello Dan Burkert,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5692
to review the following change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
rpc: allow using encrypted private key for SSL
* Adds a new flag for a "password command" for the RPC private key
* Consolidates the test keys used for the webserver and for RPC code to
use the same key material. I used the one from the RPC test code,
since that one had a 'CN' attribute whereas the webserver one did not.
Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
---
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/test/test_certs.cc
A src/kudu/security/util.cc
A src/kudu/security/util.h
M src/kudu/server/webserver.cc
M src/kudu/server/webserver_options.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/net/ssl_factory.cc
M src/kudu/util/net/ssl_factory.h
12 files changed, 208 insertions(+), 152 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5692/1
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] rpc: allow using encrypted private key for SSL
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5692/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
PS1, Line 72: Trailing whitespace will be trimmed "
: "before it is used to decrypt the private key.
I'm worried this is going to result in issues with randomly generated passwords containing legitimate whitespace characters. Is there a strong reason to do the stripping? I don't thing the behavior of 'echo' is compelling enough.
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow using encrypted private key for SSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
Patch Set 1:
> Hey Sailesh. I don't think we have any urgent need for this patch
> from the Kudu perspective, since we are using internal PKI. Also
> the patch is quite stale at this point, I think.
>
> Do you still need something like this or should i abandon the
> review?
I have a new working patch that does this, I was going to submit it for review after https://gerrit.cloudera.org/#/c/6594/ got in.
Would you still be willing to take the newer patch?
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] rpc: allow using encrypted private key for SSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5692/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
PS1, Line 72: Trailing whitespace will be trimmed "
: "before it is used to decrypt the private key.
> > I think the likelihood is that any user-provided script is going to havin
Sorry, I mean that any user-provided script is going to output the password with a trailing newline. At least I imagine if I were the one implementing such a script I'd probably end up using 'echo' or python 'print' or something else that prints a newline by default.
Moreover, for compatibility with other components I think we have to do this. For example, Hue strips newlines and their example emits them: http://gethue.com/storing-passwords-in-script-rather-than-hue-ini-files/
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow using encrypted private key for SSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
Patch Set 1:
Sure, I'll abandon this one and await yours.
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] rpc: allow using encrypted private key for SSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has abandoned this change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] rpc: allow using encrypted private key for SSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
Patch Set 1:
Just checking in on the ETA for this patch.
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] rpc: allow using encrypted private key for SSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5692/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
PS1, Line 72: Trailing whitespace will be trimmed "
: "before it is used to decrypt the private key.
> I'm worried this is going to result in issues with randomly generated passw
I think the likelihood is that any user-provided script is going to having trailing newlines.
I think it's reasonable to say that we'd only strip newlines, and not all whitespace though.
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow using encrypted private key for SSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
Patch Set 1:
For Kudu's usage, we're a little less interested in this specifically and more interested in the ability to programatically pass in our new 'Key' and 'Cert' wrappers into the RPC system. That is being done in this WIP patch:
https://gerrit.cloudera.org/#/c/5808/
After the above is complete, I think it's a matter of hooking up some code which can load a password-protected key into an internal 'Key' object.
I'm guessing you're asking about this from the Impala perspective - I guess the question I'd have in response is whether you are planning on pulling a new snapshot of krpc in the near term, or continuing to work off the most recent snapshot you have? Because a few of these security-related APIs are very much in flux right now.
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] rpc: allow using encrypted private key for SSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
Patch Set 1:
> For Kudu's usage, we're a little less interested in this
> specifically and more interested in the ability to programatically
> pass in our new 'Key' and 'Cert' wrappers into the RPC system. That
> is being done in this WIP patch:
> https://gerrit.cloudera.org/#/c/5808/
>
> After the above is complete, I think it's a matter of hooking up
> some code which can load a password-protected key into an internal
> 'Key' object.
>
> I'm guessing you're asking about this from the Impala perspective -
> I guess the question I'd have in response is whether you are
> planning on pulling a new snapshot of krpc in the near term, or
> continuing to work off the most recent snapshot you have? Because a
> few of these security-related APIs are very much in flux right now.
We will be pulling in a newer version then the one we're working with right now, but we will probably not get the latest snapshot so as to avoid having some of the security features incomplete. We will probably cherry-pick what's necessary after that for the near-term.
For eg: We will probably not be requiring the WIP patch mentioned (https://gerrit.cloudera.org/#/c/5808/), but will probably cherry-pick this patch when it goes in. But again, this is short term, and in the near future, we probably will go back to KRPC as it will be at that point.
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] rpc: allow using encrypted private key for SSL
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5692/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
PS1, Line 72: Trailing whitespace will be trimmed "
: "before it is used to decrypt the private key.
> Sorry, I mean that any user-provided script is going to output the password
OK fair enough.
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow using encrypted private key for SSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
Patch Set 1:
Hey Sailesh. I don't think we have any urgent need for this patch from the Kudu perspective, since we are using internal PKI. Also the patch is quite stale at this point, I think.
Do you still need something like this or should i abandon the review?
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] rpc: allow using encrypted private key for SSL
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: rpc: allow using encrypted private key for SSL
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5692/1/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
PS1, Line 72: Trailing whitespace will be trimmed "
: "before it is used to decrypt the private key.
> I think the likelihood is that any user-provided script is going to having
> I think the likelihood is that any user-provided script is going to having trailing newlines.
Isn't it the output of the command which is being stripped?
The issue here is that someone who is randomly generating the password now has to throw out passwords with trailing newlines or whitespace.
--
To view, visit http://gerrit.cloudera.org:8080/5692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes