Dan Burkert has posted comments on this change. Change subject: [security] generate self-signed certs on server startup ......................................................................
Patch Set 6: (13 comments) http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/rpc/messenger.h File src/kudu/rpc/messenger.h: Line 105: MessengerBuilder& enable_inbound_tls(std::string server_uuid); > worth a doc note about what effect the UUID has Done http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: PS6, Line 110: : return Status::RuntimeError( : Substitute("could not verify certificate chain (subject=$0, issuer=$1)", : > not sure about this part of the change. The cert that failed to verify may Done PS6, Line 192: if (VLOG_IS_ON(2)) { : string s; : cert.ToString(&s, DataFormat::PEM); : VLOG(2) << s; : } > I think I accidentally left this VLOG in one of my earlier patches that you Done http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/security/tls_context.h File src/kudu/security/tls_context.h: Line 52: // cert, access the CSR, and transtition to a CA-signed cert, repectively > nit: add '.' Done PS6, Line 109: usuing > nit: typo Done Line 115: // boost::none; > typo: '.' Done Line 157: // Protexts has_cert_ and csr_, as well as ctx_ and trusted_cert_count_ when > typo Done http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/security/tls_handshake-test.cc File src/kudu/security/tls_handshake-test.cc: PS6, Line 54: expected_status_ > nit: don't end in _ Done PS6, Line 55: : client_cert(client_cert_type), : client_verification(client_verification_mode), : server_cert(server_cert_type), : server_verification(server_verification_mode), : expected_status(std::move(expected_status_)) { : } > given that we are just initializing all of the fields in the same order as Done Line 160: Status InitTlsContextCert(const PrivateKey& ca_key, > nit: don't indent namespaces Done PS6, Line 226: ASSERT_TRUE(!server_tls_.has_cert()); : ASSERT_TRUE(!server_tls_.has_signed_cert()); > nit: may read better to use ASSERT_FALSE rather than ASSERT_TRUE(!...) Done PS6, Line 293: InitTlsContextCert(ca_key, ca_cert, &client_tls_, test_case.client_cert); : InitTlsContextCert(ca_key, ca_cert, &server_tls_, test_case.server_cert); > ASSERT_OK for these? Done http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/tserver/heartbeater.cc File src/kudu/tserver/heartbeater.cc: Line 367: csr->ToString(req.mutable_csr_der(), security::DataFormat::DER); > CHECK_OK maybe? Added a RETURN_NOT_OK, let me know if I should change it to CHECK. -- To view, visit http://gerrit.cloudera.org:8080/5955 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
