Alexey Serbin has posted comments on this change. Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu ......................................................................
Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: PS1, Line 162: CHECK_OK ASSERT_OK() ? PS1, Line 171: LOG(INFO) << "Connecting to " << server_addr.ToString(); Is this crucial to have this info line in the test? As I understand, this might be useful only if test fails -- if really necessary, consider using SCOPED_TRACE() with the server_addr.ToString(). PS1, Line 178: for (int i = 0; i < 10; i++) { nit: just curious why calling it once is not enough http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.cc File src/kudu/security/cert.cc: Line 214: void Cert::AdoptAndAddRefRawData(STACK_OF(X509)* data) { Why not just to use X509_chain_up_ref() and remove the constraint on data_chain_len? http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.h File src/kudu/security/cert.h: PS1, Line 76: STACK_OF(X509) nit: this is RawDataType typedef http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/openssl_util.h File src/kudu/security/openssl_util.h: Line 128: STACK_OF(X509)* PEM_read_STACK_OF_X509(BIO* bio, void* unused1, pem_password_cb* unused2, > yea, I think this is better off in the .cc file anyway since it's too large once moved into the .cc file, consider adding SCOPED_OPENSSL_NO_PENDING_ERRORS PS1, Line 178: STACK_OF(X509)* sk = sk_X509_new_null(); : if (sk_X509_push(sk, x) == 0) return nullptr; There might be a memory leak if the 'sk' is allocated but xk_X509_push() failed. Consider using ssl_make_unique() construct (you could look around for examples). http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/test/test_certs.cc File src/kudu/security/test/test_certs.cc: Line 499: Status CreateTestSSLCertSignedByChain(const string& dir, > add a comment explaining how this was generated? nit: indentation for the last 3 parameters http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/tls_handshake.cc File src/kudu/security/tls_handshake.cc: PS1, Line 194: if (cert) remote_cert_.AdoptX509(cert); nit: consider keeping the original brace style here -- To view, visit http://gerrit.cloudera.org:8080/7662 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
