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 <sail...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
