[kudu-CR] KUDU-1929: [rpc] Allow using encrypted private keys for TLS

["Sailesh Mukil (Code Review)" <ge...@cloudera.org>]

Sailesh Mukil has posted comments on this change.

Change subject: KUDU-1929: [rpc] Allow using encrypted private keys for TLS
......................................................................


Patch Set 4:

(11 comments)

Apologies for sitting on this for this long. I got very busy over the last couple months. Thanks for the review, I addressed the comments and have a few questions as well.

http://gerrit.cloudera.org:8080/#/c/6635/4/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:

Line 93: DEFINE_string(rpc_private_key_password_cmd, "", "A Unix command whose output "
> what do you think about moving these flags into programmatic APIs in Messen
I did post a WIP patch a while back to do just that. I left it as a WIP since I wasn't sure if I did it the "right" way:
https://gerrit.cloudera.org/#/c/6520/

I can rebase that and work on it as a separate patch, or do you think it would be better to merge both into one patch?


PS4, Line 269:               WARN_NOT_OK(security::GetPasswordFromShellCommand(
             :                   FLAGS_rpc_private_key_password_cmd, &ret),
             :                   "could not get RPC password from configured command");
             :  
> why are we warning instead of returning an error? if the password command f
You're right. Do you think it would be better to return a Status instead and the password as an out param?
Just want to confirm before I go ahead and do it.


http://gerrit.cloudera.org:8080/#/c/6635/4/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

Line 159:   // Clear flags.
> no need to do this - we already wrap all test execution in a FlagSaver whic
Done


http://gerrit.cloudera.org:8080/#/c/6635/4/src/kudu/security/crypto.cc
File src/kudu/security/crypto.cc:

PS4, Line 188: const_cast<PasswordCallback*>(&cb))
> see elsewhere, seems weird to pass in a pointer here, and especially with a
Done


http://gerrit.cloudera.org:8080/#/c/6635/4/src/kudu/security/crypto.h
File src/kudu/security/crypto.h:

PS4, Line 78: const PasswordCallback& cb = nullptr
> it's odd to see '= nullptr' here, since it's a reference argument. I guess 
Makes sense. I changed it to the default constructor.


http://gerrit.cloudera.org:8080/#/c/6635/4/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

Line 222:     return Status::RuntimeError("failed to run private key password command", stderr);
> is there no value in the actual 's' status message?
Oops, added it to the status.


http://gerrit.cloudera.org:8080/#/c/6635/1/src/kudu/security/openssl_util_bio.h
File src/kudu/security/openssl_util_bio.h:

Line 60:   string pw = (*cb)();
> warning: function 'TLSPasswordCB' defined in a header file; function defini
Done


http://gerrit.cloudera.org:8080/#/c/6635/4/src/kudu/security/openssl_util_bio.h
File src/kudu/security/openssl_util_bio.h:

PS4, Line 58: int size
> is 'size' the maximum length of the password, or the size of 'buf'? If the 
It's the size of buf. I've modified the 'if' condition on L61 to make sure that there's always space for the NULL terminator.


Line 71: Status FromBIO(BIO* bio, DataFormat format, c_unique_ptr<TYPE>* ret, void* cb = nullptr) {
> given TLSPasswordCB is always assuming that the callback is a PasswordCallb
Done


http://gerrit.cloudera.org:8080/#/c/6635/4/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:

Line 427:   RETURN_NOT_OK(k.FromFile(key_path, DataFormat::PEM, cb));
> would be good to use PREPEND on both of this and above to indicate whether 
Done


PS4, Line 428:   is_external_cert_ = true;
             :   return UseCertificateAndKey(c, k);
> if UseCertificateAndKey fails, we don't want to set is_external_cert_. Shou
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6635
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd6369581fa426ceab11e4a10441658c7da47e81
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes