Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9934 )
Change subject: KUDU-2401: External TLS certificate with Intermediate CA in server cert file fails ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/9934/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9934/1//COMMIT_MSG@11 PS1, Line 11: cert.pem has 2 certificates in it: I couldn't find any documentation on whether this is allowed or not. On the one hand one might think that cert.pem has to contain the host certificate and that the supporting chain must be in the truststore. On the other hand we seem to accept any partition between the two. I think it would be good to formally document which cases we accept and which we don't. http://gerrit.cloudera.org:8080/#/c/9934/1//COMMIT_MSG@28 PS1, Line 28: P.S: A majority of the change is testing related and the core code change : is pretty small. nit: remove http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/test/test_certs.h File src/kudu/security/test/test_certs.h: http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/test/test_certs.h@78 PS1, Line 78: // Same as the CreateTestSSLCertWithPlainKey() except that the 'cert_file' is ca_cert_file is also different between the two. See my comment in the cc file. http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/test/test_certs.cc File src/kudu/security/test/test_certs.cc: http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/test/test_certs.cc@766 PS1, Line 766: // signed by the root CA. Maybe you could add some ascii-art to outline how these are signed and distributed across the files. E.g. A->B->C where -> means signs and then cert_file contains (A, B) in that order, and ca_cert_file contains (C). Can we merge this method with the previous one and pass the distribution across the two files (ca, cert)? We should also consider adding a 4th certificate to the tests. http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: http://gerrit.cloudera.org:8080/#/c/9934/1/src/kudu/security/tls_context.cc@197 PS1, Line 197: for (int i = 0; i < cert.chain_len(); ++i) { > I pretty much completely paged these APIs out so I started looking through I think this might work as expected, but we should double check that we don't validate the cert against itself when passing the full chain. Alternatively we could remove the cert from the chain before passing it. -- To view, visit http://gerrit.cloudera.org:8080/9934 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If4af35e97ec6f91c1d9ed902128bd7f4e260f0f4 Gerrit-Change-Number: 9934 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Thu, 05 Apr 2018 21:52:43 +0000 Gerrit-HasComments: Yes
