[kudu] branch master updated: Fix order of clearing and printing openssl error

[abukor]

This is an automated email from the ASF dual-hosted git repository.

abukor pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git


The following commit(s) were added to refs/heads/master by this push:
     new 0e86d4f  Fix order of clearing and printing openssl error
0e86d4f is described below

commit 0e86d4fce538fd2ed1fb21ac07c073eea15aaa39
Author: Attila Bukor <abu...@apache.org>
AuthorDate: Wed Oct 21 17:19:47 2020 +0200

    Fix order of clearing and printing openssl error
    
    When verifying the certificate chain fails with an error other than
    self-signed certificate, we try to get the subject and issuer to print
    in the error message. Unfortunately X509NameToString(), the method doing
    the conversion, also checks that there are no leftover OpenSSL errors,
    so it fails immediately on call. This commit changes the behavior to
    clear the errors *before* calling X509NameToString().
    
    I ran into this problem while debugging test failures on a host where
    the OpenSSL was provided by CryptoComply SafeLogic:
    
    F1020 12:06:13.327023 25579 openssl_util.h:210] Check failed: 
ERR_peek_error() == 0 (67567722 vs. 0) Expected no pending OpenSSL errors on 
std::string kudu::security::X509NameToString(X509_NAME*) entry, but had: 
error:0407006A:rsa routines:RSA_padding_check_PKCS1_type_1:block type is not 
01:rsa_pk1.c:102 error:04067072:rsa routines:RSA_EAY_PUBLIC_DECRYPT:padding 
check failed:rsa_eay.c:786 error:0D0C5006:asn1 encoding 
routines:ASN1_item_verify:EVP lib:a_verify.c:218
    
    Unfortunately, I couldn't reproduce it in other OpenSSL versions and
    distributions, so I can't add a regression test, at least for now.
    
    Change-Id: I3f78bdedce7a976a6e8117bb8683032dd917c626
    Reviewed-on: http://gerrit.cloudera.org:8080/16631
    Reviewed-by: Alexey Serbin <aser...@cloudera.com>
    Tested-by: Kudu Jenkins
    Reviewed-by: Grant Henke <granthe...@apache.org>
---
 src/kudu/security/tls_context.cc | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc
index 8a637f6..6f4d19c 100644
--- a/src/kudu/security/tls_context.cc
+++ b/src/kudu/security/tls_context.cc
@@ -31,6 +31,7 @@
 #include <mutex>
 #include <ostream>
 #include <string>
+#include <type_traits>
 #include <vector>
 
 #include <boost/algorithm/string/predicate.hpp>
@@ -245,9 +246,13 @@ Status TlsContext::VerifyCertChainUnlocked(const Cert& 
cert) {
   int rc = X509_verify_cert(store_ctx.get());
   if (rc != 1) {
     int err = X509_STORE_CTX_get_error(store_ctx.get());
+
+    // This also clears the errors. It's important to do this before we call
+    // X509NameToString(), as it expects the error stack to be empty and it
+    // calls SCOPED_OPENSSL_NO_PENDING_ERRORS.
+    const auto error_msg = GetOpenSSLErrors();
     if (err == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT) {
       // It's OK to provide a self-signed cert.
-      ERR_clear_error(); // in case it left anything on the queue.
       return Status::OK();
     }
 
@@ -260,9 +265,8 @@ Status TlsContext::VerifyCertChainUnlocked(const Cert& 
cert) {
                                 
X509NameToString(X509_get_issuer_name(cur_cert)));
     }
 
-    ERR_clear_error(); // in case it left anything on the queue.
     return Status::RuntimeError(
-        Substitute("could not verify certificate chain$0", cert_details),
+        Substitute("could not verify certificate chain$0: $1", cert_details, 
error_msg),
         X509_verify_cert_error_string(err));
   }
   return Status::OK();