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

Reply via email to