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 13: (6 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@272 PS9, Line 272: > Why to have EXPECT here instead of ASSERT (or even CHECK)? If !s.ok(), it Did you miss addressing this one? Or, maybe, the idea is to pass along an empty string if ReadFileToString() returns non-OK status? If the latter, maybe add a comment to explain that such behavior is intentional; otherwise, this looks like a bug. http://gerrit.cloudera.org:8080/#/c/23097/13/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: http://gerrit.cloudera.org:8080/#/c/23097/13/be/src/util/openssl-util-test.cc@439 PS13, Line 439: std::cerr << "OPENSSL ERROR: " << ERR_peek_error() << std::endl; A remnant from debugging sessions? Should this be removed? http://gerrit.cloudera.org:8080/#/c/23097/13/be/src/util/openssl-util.h File be/src/util/openssl-util.h: http://gerrit.cloudera.org:8080/#/c/23097/13/be/src/util/openssl-util.h@75 PS13, Line 75: Does not validate each certificate's signature chain nit: this is most likely out of the scope of this pre-validation, but you might consider adding extra validation steps even if not validating the signature chain, such as trying to decode public key of the issuer by calling X509_get0_pubkey(), etc. A reference: https://github.com/openssl/openssl/blob/51ce5499f9bd1f12cf08f511faaf163b0c4448bb/crypto/x509/x509_vfy.c#L1886 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: : // Check certificate validity (not > I checked the docs, but apparently they do not match the code. That's a good call: by removing that you also got rid of two memory leaks for non-OK control paths :) I also took a look at http://gerrit.cloudera.org:8080/#/c/23097/9/be/src/util/openssl-util.cc@199 PS9, Line 199: // The final PEM_read_bio_X509 always sets the openssl error to PEM_R_NO_START_LINE, : // if the ssl error is set to anything else, return it. : 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(); : : return ret; : } // function ValidatePemBundle : : void IntegrityHash::Compute(const uint8_t* data, int64_t len) { : // Explicitly ignore the return value from SHA256(); it can't fail. : (void)SHA256(data, len, hash_); : DCHECK_EQ(ERR_peek_error(), 0) << "Did not clear OpenSSL error queue"; : } > I dug into this, and using those functions doesn't save code or improve cla Well, from my point of view those do save code and make it simpler by improving clarity, and help preventing memory leaks. They are very handy when using with OPENSSL_RET_NOT_OK() and OPENSSL_RET_IF_NULL() macros because they automatically handle errors left on the OpenSSL's error stack properly. FWIW, the code in PS9 has at least two memory leaks: at line 178 and at line 190. Anyway, I'm fine with your decision for this particular instance: PS13 doesn't have the memory leaks that PS9 had, and I'm not a maintainer of this code anyway :) http://gerrit.cloudera.org:8080/#/c/23097/13/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/23097/13/be/src/util/openssl-util.cc@207 PS13, Line 207: ERR_clear_error(); If clearing the error in the error stack unconditionally like this, does it make sense to add a DCHECK() to make sure nothing was on the error stack at the entrance in this ValidatePemBundle function? Otherwise, this might clear unrelated errors left by sloppy code elsewhere, hide various issues, and make it much harder to troubleshoot. Adding such a DCHECK() will also protect this code against phantom errors returned from line 201 as of PS13 in such condition. As for more comprehensive solution to detect/prevent phantom errors being picked up from the OpenSSL's error stack, consider using SCOPED_OPENSSL_NO_PENDING_ERRORS from be/src/kudu/util/openssl_util.h -- 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: 13 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, 09 Jul 2025 19:23:27 +0000 Gerrit-HasComments: Yes
