Todd Lipcon has posted comments on this change. Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/7662/3/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: Line 149: if (!info) return nullptr; in the case of a bad PEM file format, I think this would end up leaving a pending OpenSSL error, and then the SCOPED_OPENSSL_NO_PENDING_ERRORS would crash. I'm not convinced that the SCOPED_OPENSSL_NO_PENDING_ERRORS is appropriate here, since this is function is supposed to have an SSL-like calling convention (return NULL and leave a pending error on failure). The pending error should be handled (converted to Status) by the code in FromBIO(...) in openssl_util_bio.h. Line 156: if (chain_len == 0) return nullptr; similar to above, I think this will result in returning null but with no pending errors on the OpenSSL error stack. So when FromBIO(...) calls GetOpenSSLErrors() it's going to get an empty string and this will then return a RuntimeError with no explanation. Perhaps it's better to remove this check and change the check to be in the code which consumes the STACK_OF(X509) (ie Cert::FromBIO) PS3, Line 165: free() call below the free call is no longer below. maybe just say 'the ScopedCleanup' Line 175: SCOPED_OPENSSL_NO_PENDING_ERRORS; same Line 188: SCOPED_OPENSSL_NO_PENDING_ERRORS; same Line 202: SCOPED_OPENSSL_NO_PENDING_ERRORS; same Line 212: SCOPED_OPENSSL_NO_PENDING_ERRORS; same -- 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: 3 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
