Todd Lipcon 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 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 be the CA cert or an intermediate cert, not the cert passed in as 'cert' 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 built this off of, but I dont see it being particularly useful. do you? 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 '.' PS6, Line 109: usuing nit: typo Line 115: // boost::none; typo: '.' Line 157: // Protexts has_cert_ and csr_, as well as ctx_ and trusted_cert_count_ when typo Odd to see trusted_cert_count_ being an atomic, but protected by a lock. Explain? maybe it should just be a normal int, since iirc it's just for tests 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 _ 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 the ctor params, could we just use brace initialization? not sure the ctor is doing anything. Line 160: Status InitTlsContextCert(const PrivateKey& ca_key, nit: don't indent namespaces 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(!...) (same below) 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? 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? -- 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
