Sailesh Mukil has posted comments on this change.

Change subject: KUDU-2091: Certificates with intermediate CA's do not work with 

Patch Set 2:

File src/kudu/rpc/

PS1, Line 162: ASSERT_O

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.
File src/kudu/security/

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?

Line 78: }
> sk_X509_num?

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?
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]

PS1, Line 63: the e
> the end-user cert?

PS1, Line 76: RawDataType* d
> nit: this is RawDataType typedef

PS1, Line 89: GetEndOfChainX509
> nit: think this should be named GetEndOfChainX509 since it returns an X509*

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.
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

Line 128: int PEM_write_STACK_OF_X509(BIO* bio, STACK_OF(X509)* obj);
> warning: parameter 'unused2' is unused [misc-unused-parameters]

Line 128: int PEM_write_STACK_OF_X509(BIO* bio, STACK_OF(X509)* obj);
> warning: parameter 'unused1' is unused [misc-unused-parameters]

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

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

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

Line 129: STACK_OF(X509)* DER_read_STACK_OF_X509(BIO* bio, void* /* unused */);
> warning: parameter 'unused3' is unused [misc-unused-parameters]

PS1, Line 131: void free_STACK_OF_X509(STACK_OF(X509)* sk);
             : t
> combine onto one line

Line 138:   static constexpr auto kWriteDerFunc = &DER_write_STACK_OF_X509;
> can you use a ScopedCleanup to make this automatic right after allocating t

PS1, Line 143: plate<> struct SslTypeTraits<X509_REQ> {
             :   static constexpr auto kF
> combine on one line?

Line 147:   static constexpr auto kWritePemFunc = &PEM_write_bio_X509_REQ;
> move into the loop body since it's only used in that scope?

Line 161: 
> warning: function 'PEM_write_STACK_OF_X509' defined in a header file; funct

Line 162: // Acceptable formats for keys, X509 certificates and X509 CSRs.
> sk_X509_num?

Line 164:   DER = 0,    // DER/ASN1 format (binary): for representing object on 
the wire
> move inside loop

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]

Line 175:  public:
> warning: function 'DER_read_STACK_OF_X509' defined in a header file; functi

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?

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

Line 184:   }
> warning: function 'DER_write_STACK_OF_X509' defined in a header file; funct

Line 192: 
> warning: function 'free_STACK_OF_X509' defined in a header file; function d

Line 196:  public:
> warning: anonymous namespace not terminated with a closing comment [google-
File src/kudu/security/test/

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
File src/kudu/security/test/test_certs.h:

Line 74:                                       std::string* cert_file,
> nit: indentation
File src/kudu/security/

Line 255:     X509* inner_cert = sk_X509_value(cert.GetRawData(), i);
> sk_X509_value

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?
File src/kudu/security/

PS1, Line 194:   if (cert) {
> nit: consider keeping the original brace style here

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to