Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16631 )
Change subject: Fix order of clearing openssl error and printing it ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/16631/1//COMMIT_MSG Commit Message: PS1: > What was the error we saw here? Is there any way to test this so we don't r +1 It would be great to add a test scenario which reproduces the issue. You can take a look at available cert-related cases in src/kudu/security/test/test_certs.cc, in particular comments for the CreateTestSSLCertSignedByChain() function. I guess it might be possible to generate a certificate chain that has similar issue. http://gerrit.cloudera.org:8080/#/c/16631/1/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: http://gerrit.cloudera.org:8080/#/c/16631/1/src/kudu/security/tls_context.cc@248 PS1, Line 248: ERR_clear_error(); // in case it left anything on the queue. > nit: Can you also note that it's important to call this before X509NameToSt Maybe, it's also worth outputting the collected error information along with cert_details when returning RuntimeError at line 256? It's about calling GetOpenSSLErrors() (which does extracts all errors from the error stack as well) after X509_verify_cert() storing the result in string, and the using the string when forming the RuntimeError message. int rc = X509_verify_cert(store_ctx.get()); if (rc != 1) { const auto verify_err_msg = GetOpenSSLErrors(); ... } -- To view, visit http://gerrit.cloudera.org:8080/16631 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f78bdedce7a976a6e8117bb8683032dd917c626 Gerrit-Change-Number: 16631 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 23 Oct 2020 00:29:00 +0000 Gerrit-HasComments: Yes
