Sailesh Mukil has posted comments on this change. Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu ......................................................................
Patch Set 2: (41 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: ASSERT_O > ASSERT_OK() ? Done PS1, Line 171: SCOPED_TRACE(strings::Substitute("Connecting to $0", ser > Is this crucial to have this info line in the test? As I understand, this I changed it to a scoped trace, so that we can check if the test server started or not easily. PS1, Line 178: ASSERT_OK(DoTestSyncCall(p, Ge > nit: just curious why calling it once is not enough I think it should be fine, but I copied the above TestCall() test. I guess the point is to see if it works reliably. But since we're only testing if the certificates work, I think we can remove it. http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.cc File src/kudu/security/cert.cc: Line 71: X509* Cert::GetEndOfChainX509() const { > worth a CHECK_GT(chain_len_, 0); or an if statement and return nullptr in t I added a CHECK_GT() since we only use it after populating Cert object. PS1, Line 72: T(chain_len(), 0); > could you use sk_X509_value(data_.get(), chain_len_ - 1) here? Done Line 78: } > sk_X509_num? Done Line 214: DCHECK(cert); > Why not just to use X509_chain_up_ref() and remove the constraint on data_c Unfortunately, X509_chain_up_ref() is only available from OpenSSL 1.1.0. Should I add another #ifdef or just leave it as it is? http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.h File src/kudu/security/cert.h: Line 45: // TODO(unknown): Currently, there isn't a mechanism to add to the chain. Implement it when needed. > warning: missing username/bug in TODO [google-readability-todo] Done PS1, Line 63: the e > the end-user cert? Done PS1, Line 76: RawDataType* d > nit: this is RawDataType typedef Done PS1, Line 89: GetEndOfChainX509 > nit: think this should be named GetEndOfChainX509 since it returns an X509* Done Line 93: public: > why not just use sk_X509_num(data_.get()) where necessary? is caching the v Not really. I initially thought the sk_num() function might be large, but I looked at the sk_X509_num() function and it's very small, so we might as well not re-track it. http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/openssl_util.h File src/kudu/security/openssl_util.h: Line 125: // SslTypeTraits functions for Type STACK_OF(X509) > warning: do not use unnamed namespaces in header files [google-build-namesp Done Line 128: int PEM_write_STACK_OF_X509(BIO* bio, STACK_OF(X509)* obj); > warning: parameter 'unused2' is unused [misc-unused-parameters] Done Line 128: int PEM_write_STACK_OF_X509(BIO* bio, STACK_OF(X509)* obj); > warning: parameter 'unused1' is unused [misc-unused-parameters] Done Line 128: int PEM_write_STACK_OF_X509(BIO* bio, STACK_OF(X509)* obj); > once moved into the .cc file, consider adding SCOPED_OPENSSL_NO_PENDING_ERR Done Line 128: int PEM_write_STACK_OF_X509(BIO* bio, STACK_OF(X509)* obj); > warning: function 'PEM_read_STACK_OF_X509' defined in a header file; functi Done Line 128: int PEM_write_STACK_OF_X509(BIO* bio, STACK_OF(X509)* obj); > yea, I think this is better off in the .cc file anyway since it's too large Done Line 129: STACK_OF(X509)* DER_read_STACK_OF_X509(BIO* bio, void* /* unused */); > warning: parameter 'unused3' is unused [misc-unused-parameters] Done PS1, Line 131: void free_STACK_OF_X509(STACK_OF(X509)* sk); : : t > combine onto one line Done Line 138: static constexpr auto kWriteDerFunc = &DER_write_STACK_OF_X509; > can you use a ScopedCleanup to make this automatic right after allocating t Done PS1, Line 143: plate<> struct SslTypeTraits<X509_REQ> { : static constexpr auto kF > combine on one line? Done Line 147: static constexpr auto kWritePemFunc = &PEM_write_bio_X509_REQ; > move into the loop body since it's only used in that scope? Done Line 161: > warning: function 'PEM_write_STACK_OF_X509' defined in a header file; funct Done Line 162: // Acceptable formats for keys, X509 certificates and X509 CSRs. > sk_X509_num? Done Line 164: DER = 0, // DER/ASN1 format (binary): for representing object on the wire > move inside loop Done PS1, Line 167: : // Data format representation as a string. : c > combine Not really, I looked at some tutorials and some stack overflow examples to understand the use of these APIs and wrote this part pretty quickly, so I sort of dropped the ball on the style part. Sorry about that. Line 175: public: > warning: parameter 'unused' is unused [misc-unused-parameters] Done Line 175: public: > warning: function 'DER_read_STACK_OF_X509' defined in a header file; functi Done Line 176: typedef Type RawDataType; > is such a thing actually commonplace out there? can we at least detect it a Do you mean chain certificates in DER format? It is possible to do it if the certificate uses the PKCS7 standard. The d2i_X509_bio() call below only reads single certificates anyway, so if a chain DER certificate is presented, I would expect that call would fail and return nullptr. Line 177: > what if x is null? Done PS1, Line 178: RawDataType* GetRawData() const { : return data_.get(); > There might be a memory leak if the 'sk' is allocated but xk_X509_push() fa Done Line 184: } > warning: function 'DER_write_STACK_OF_X509' defined in a header file; funct Done Line 192: > warning: function 'free_STACK_OF_X509' defined in a header file; function d Done Line 196: public: > warning: anonymous namespace not terminated with a closing comment [google- Done 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: // > add a comment explaining how this was generated? I followed a tutorial online which is fairly long and involves configurations files (so not just using commands). So I just added a link to the tutorial. Let me know if you think it'll be better to add the actual commands and the conf file contents itself. Line 499: // > nit: indentation for the last 3 parameters Done 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 Done 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(cert.GetRawData(), i); > sk_X509_value Done Line 270: return Status::OK(); > should you add chain_len() here? I was thinking about that earlier and I know this part gets a bit confusing. IIUC, the TLS context can trust multiple CAs not related to each other right? In that case, wouldn't leaving it as +=1 be the right thing to do? 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) { > nit: consider keeping the original brace style here Done -- 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: 2 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: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
