Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23097 )
Change subject: IMPALA-13235: [Patch 2 of 5] - Add OpenSSL Utility Function to Validate PEM Bundles ...................................................................... Patch Set 9: (8 comments) http://gerrit.cloudera.org:8080/#/c/23097/9/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: http://gerrit.cloudera.org:8080/#/c/23097/9/be/src/util/openssl-util-test.cc@253 PS9, Line 253: static std::string IMPALA_HOME(getenv("IMPALA_HOME")); Why not to move this into the read_cert() function that's the only place IMPALA_HOME is used in this file? http://gerrit.cloudera.org:8080/#/c/23097/9/be/src/util/openssl-util-test.cc@254 PS9, Line 254: std::string nit: having 'using std::string;' would allow removing the 'std::' prefix from any 'std::string' in the code below http://gerrit.cloudera.org:8080/#/c/23097/9/be/src/util/openssl-util-test.cc@272 PS9, Line 272: EXPECT Why to have EXPECT here instead of ASSERT (or even CHECK)? If !s.ok(), it doesn't make sense to run the rest of the code below anyway, right? http://gerrit.cloudera.org:8080/#/c/23097/9/be/src/util/openssl-util-test.cc@316 PS9, Line 316: strings::Substitute nit: you could add 'using strings::Substitute;' and drop the 'strings::' namespace prefix everywhere in the code below http://gerrit.cloudera.org:8080/#/c/23097/9/be/src/util/openssl-util.h File be/src/util/openssl-util.h: http://gerrit.cloudera.org:8080/#/c/23097/9/be/src/util/openssl-util.h@73 PS9, Line 73: Validates a PEM-encoded certificate bundle It would be great to provide more information on what 'validation' here means. Is this just reading in X509 structure and checking for the validity date? This isn't going to validate the certificate chain or validate certificate signatures against some cert store, right? http://gerrit.cloudera.org:8080/#/c/23097/9/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/23097/9/be/src/util/openssl-util.cc@174 PS9, Line 174: X509_cmp_current_time() only returns 0 : // if it passed a NULL pointer I guess that's version-dependent. In some versions, it just crashes with SIGSEGV because of an attempt of null pointer dereferencing. At least that seems to be so for OpenSSL 1.1.1 and even current(?) versions: https://github.com/openssl/openssl/blob/2d978786f3e97a2701d5f62c26a4baab4a224e69/crypto/x509/x509_vfy.c#L2004-L2033 http://gerrit.cloudera.org:8080/#/c/23097/9/be/src/util/openssl-util.cc@199 PS9, Line 199: Status ret; : : if (UNLIKELY(invalid_cert_count > 0 || cert_count == invalid_cert_count)) { : ret = Status("PEM bundle contains at least 1 invalid certificate"); : } else if (UNLIKELY(cert_count == 0)) { : ret = Status("PEM bundle contains no valid certificates"); : } else if (UNLIKELY(ERR_GET_REASON(ERR_peek_error()) != PEM_R_NO_START_LINE)) { : // The final PEM_read_bio_X509 always sets the openssl error. : ret = OpenSSLErr("PEM_read_bio_X509", "unknown error while reading PEM bundle"); : } else { : ret = Status::OK(); : } : : // Clear the error left by the final PEM_read_bio_X509 call when it returns nullptr. : ERR_clear_error(); : BIO_free(bio); : : return ret; Since I saw kudu::ReadFileToString() in the test scenario, I'm curious whether it makes sense to use Kudu's smart pointer wrappers for OpenSSL objects here instead. In other words, using OpenSSL wrappers one could use c_unique_ptr/ssl_make_unique and return from the function right away when an error detected, while the clean-up of the allocated objects would be done automatically. http://gerrit.cloudera.org:8080/#/c/23097/9/be/src/util/openssl-util.cc@219 PS9, Line 219: return Status("bundle is empty"); nit: for better readability, wrap this into corresponding 'if()' clause and move into the very beginning of the function -- To view, visit http://gerrit.cloudera.org:8080/23097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e9f12e854b679ccf3e94a38c2688a7431460a7a Gerrit-Change-Number: 23097 Gerrit-PatchSet: 9 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Wed, 02 Jul 2025 02:41:06 +0000 Gerrit-HasComments: Yes
