Todd Lipcon has posted comments on this change. Change subject: KUDU-1929: [rpc] Allow using encrypted private keys for TLS ......................................................................
Patch Set 4: (10 comments) 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 MessengerOptions to be able to pass in a key/CA? Given we aren't currently using them in Kudu, it seems like it might be better for KRPC to just act as a library, and for the flags to live in Impala's code which sets up the Messenger? 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 fails, seems like it would be more clear if the error message propagated directly instead of ending up in a warning log somewhere 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 which restores the values to defaults after each test 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 const cast 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 it's triggering the std::function<>(function pointer) constructor, but I think it might be cleaner to use the default constructor? = PasswordCallback? Or make it a boost::optional, which might be a litle easier to follow? 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? 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 latter, then we need to be more careful below to make sure there's 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 PasswordCallback, I think it's best for this argument to just be a 'const PasswordCallback&' instance, and do the pointer construction inline in this function, rather than at the caller. 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 the issue was loading the key or the cert PS4, Line 428: is_external_cert_ = true; : return UseCertificateAndKey(c, k); if UseCertificateAndKey fails, we don't want to set is_external_cert_. Should probably swap the order of these -- 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 <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
