Todd Lipcon has posted comments on this change. Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu ......................................................................
Patch Set 1: (20 comments) http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.cc File src/kudu/security/cert.cc: Line 71: X509* Cert::GetEndOfChainCert() const { worth a CHECK_GT(chain_len_, 0); or an if statement and return nullptr in the case of an empty stack. PS1, Line 72: static_cast<X509*>(sk_value could you use sk_X509_value(data_.get(), chain_len_ - 1) here? Line 78: if (s.ok()) chain_len_ = sk_num(reinterpret_cast<const _STACK*>(data_.get())); sk_X509_num? http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.h File src/kudu/security/cert.h: PS1, Line 63: this the end-user cert? PS1, Line 89: GetEndOfChainCert nit: think this should be named GetEndOfChainX509 since it returns an X509* instead of a Cert wrapper Line 93: int chain_len_ = 0; why not just use sk_X509_num(data_.get()) where necessary? is caching the value necessary? 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, > warning: function 'PEM_read_STACK_OF_X509' defined in a header file; functi yea, I think this is better off in the .cc file anyway since it's too large to inline. PS1, Line 131: STACK_OF(X509_INFO)* info; : info = PEM_X509_INFO_read_bio(bio, nullptr, nullptr, nullptr); : combine onto one line Line 138: sk_X509_INFO_pop_free(info, X509_INFO_free); can you use a ScopedCleanup to make this automatic right after allocating the 'info' and avoid having it in three places? PS1, Line 143: TACK_OF(X509)* sk; : sk = sk_X509_new_null(); combine on one line? Line 147: X509_INFO *stack_item = nullptr; move into the loop body since it's only used in that scope? Line 162: unsigned chain_len = sk_num(reinterpret_cast<const _STACK*>(obj)); sk_X509_num? use 'int' instead of 'unsigned' to make unsigned overflow bugs less likely Line 164: X509* cert_item = nullptr; move inside loop PS1, Line 167: int ret; : ret = PEM_write_bio_X509(bio, cert_item); : combine (it looks like a bunch of this code has this style - I'm guessing you borrowed it from some C language example. Is there any copyright notice we need to retain? is the license OK?) Line 176: // We don't support chain certificates written in DER format. is such a thing actually commonplace out there? can we at least detect it and LOG(FATAL) or otherwise return some warning instead of silently ignoring? Line 177: X509* x = d2i_X509_bio(bio, nullptr); what if x is null? 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? http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/test/test_certs.h File src/kudu/security/test/test_certs.h: Line 74: std::string* cert_file, nit: indentation http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: Line 255: X509* inner_cert = sk_X509_value Line 270: trusted_cert_count_ += 1; should you add chain_len() 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
