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 <sail...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to