kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jdcry...@apache.org
Subject kudu git commit: KUDU-1964. security: avoid calling ERR_clear_error() defensively
Date Thu, 04 May 2017 21:10:51 GMT
Repository: kudu
Updated Branches:
  refs/heads/branch-1.3.x ed3fde1b1 -> 1232c893e


KUDU-1964. security: avoid calling ERR_clear_error() defensively

This changes our various code which wraps OpenSSL to avoid calling
ERR_clear_error() defensively. Instead, we now make sure to call
ERR_clear_error() after any case where we receive an error return value
from an OpenSSL library call.

For cases where we use the existing wrapper macros like
OPENSSL_RET_NOT_OK we are already ensured of this since the
GetOpenSSLErrors() call clears the error queue.

This provides a performance boost, since ERR_clear_error() ends up
taking several library-wide mutexes in OpenSSL 1.0.x (apparently
improved in OpenSSL 1.1, but that's not available on current OSes).

To ensure that we don't accidentally leave an OpenSSL error lying around
after any functions, I added a scoped helper which is active in debug
builds. This performs a DCHECK that the error queue is empty on scope
entry and exit.

To benchmark, I tested rpc-bench with encryption enabled using OpenSSL 1.0.1e
(RHEl6):

Master, SSL off:
----------------
I0404 12:49:04.811158  7250 rpc-bench.cc:84] Mode:            Async
I0404 12:49:04.811172  7250 rpc-bench.cc:88] Client reactors:  16
I0404 12:49:04.811175  7250 rpc-bench.cc:89] Call concurrency: 60
I0404 12:49:04.811178  7250 rpc-bench.cc:92] Worker threads:   1
I0404 12:49:04.811182  7250 rpc-bench.cc:93] Server reactors:  4
I0404 12:49:04.811183  7250 rpc-bench.cc:94] ----------------------------------
I0404 12:49:04.811187  7250 rpc-bench.cc:95] Reqs/sec:         446998
I0404 12:49:04.811193  7250 rpc-bench.cc:96] User CPU per req: 9.97575us
I0404 12:49:04.811197  7250 rpc-bench.cc:97] Sys CPU per req:  12.2342us
I0404 12:49:04.811202  7250 rpc-bench.cc:98] Ctx Sw. per req:  0.604032

Master, SSL on:
--------------
I0404 12:57:10.241720  9949 rpc-bench.cc:86] Mode:            Async
I0404 12:57:10.241736  9949 rpc-bench.cc:90] Client reactors:  16
I0404 12:57:10.241739  9949 rpc-bench.cc:91] Call concurrency: 60
I0404 12:57:10.241742  9949 rpc-bench.cc:94] Worker threads:   1
I0404 12:57:10.241745  9949 rpc-bench.cc:95] Server reactors:  4
I0404 12:57:10.241747  9949 rpc-bench.cc:96] Encryption:       1
I0404 12:57:10.241760  9949 rpc-bench.cc:97] ----------------------------------
I0404 12:57:10.241762  9949 rpc-bench.cc:98] Reqs/sec:         56761.3
I0404 12:57:10.241778  9949 rpc-bench.cc:99] User CPU per req: 39.7792us
I0404 12:57:10.241783  9949 rpc-bench.cc:100] Sys CPU per req:  106.916us
I0404 12:57:10.241786  9949 rpc-bench.cc:101] Ctx Sw. per req:  5.98631

Note the high number of context switches and system CPU. I gathered stack
traces of context switches using "perf record -g -e cs":

  Overhead       Samples    Command      Shared Object                         Symbol
  ........  ............  .........  .................  .............................

   100.00%        180979  rpc-bench  [kernel.kallsyms]  [k] perf_event_task_sched_out
            |
            --- perf_event_task_sched_out
                schedule
               |
               |--83.17%-- futex_wait_queue_me
               |          futex_wait
               |          do_futex
               |          sys_futex
               |          system_call_fastpath
               |          |
               |          |--83.11%-- __lll_lock_wait
               |          |          |
               |          |          |--42.33%-- int_thread_get
               |          |          |
               |          |          |--29.36%-- CRYPTO_add_lock
               |          |          |
               |          |          |--28.28%-- int_thread_get_item
               |          |           --0.03%-- [...]
               |          |
               |          |--16.89%-- pthread_cond_wait@@GLIBC_2.3.2
               |          |          |
               |          |          |--100.00%-- kudu::rpc::ServicePool::RunThread()
               |          |          |          kudu::Thread::SuperviseThread(void*)
               |          |          |          start_thread
               |          |           --0.00%-- [...]
               |           --0.01%-- [...]
               |
               |--16.80%-- schedule_hrtimeout_range
               |          |
               |          |--100.00%-- ep_poll
               |          |          sys_epoll_wait
               |          |          system_call_fastpath
               |          |          epoll_wait
               |          |          |
               |          |           --100.00%-- ev_run
               |          |                     kudu::rpc::ReactorThread::RunThread()
               |          |                     kudu::Thread::SuperviseThread(void*)
               |          |                     start_thread
               |           --0.00%-- [...]
                --0.03%-- [...]

With this patch
---------------
I0404 14:13:59.529119 27892 rpc-bench.cc:86] Mode:            Async
I0404 14:13:59.529134 27892 rpc-bench.cc:90] Client reactors:  16
I0404 14:13:59.529137 27892 rpc-bench.cc:91] Call concurrency: 60
I0404 14:13:59.529140 27892 rpc-bench.cc:94] Worker threads:   1
I0404 14:13:59.529142 27892 rpc-bench.cc:95] Server reactors:  4
I0404 14:13:59.529145 27892 rpc-bench.cc:96] Encryption:       1
I0404 14:13:59.529155 27892 rpc-bench.cc:97] ----------------------------------
I0404 14:13:59.529158 27892 rpc-bench.cc:98] Reqs/sec:         280819
I0404 14:13:59.529173 27892 rpc-bench.cc:99] User CPU per req: 18.2016us
I0404 14:13:59.529177 27892 rpc-bench.cc:100] Sys CPU per req:  19.9601us
I0404 14:13:59.529181 27892 rpc-bench.cc:101] Ctx Sw. per req:  1.20247
(about 5x improved throughput)

The SSL-related context switches no longer show up:

   100.00%        186653  rpc-bench  [kernel.kallsyms]  [k] perf_event_task_sched_out
            |
            --- perf_event_task_sched_out
                schedule
               |
               |--81.87%-- schedule_hrtimeout_range
               |          |
               |          |--100.00%-- ep_poll
               |          |          sys_epoll_wait
               |          |          system_call_fastpath
               |          |          epoll_wait
               |          |          |
               |          |           --100.00%-- ev_run
               |          |                     kudu::rpc::ReactorThread::RunThread()
               |          |                     kudu::Thread::SuperviseThread(void*)
               |          |                     start_thread
               |           --0.00%-- [...]
               |
               |--18.11%-- futex_wait_queue_me
               |          futex_wait
               |          do_futex
               |          sys_futex
               |          system_call_fastpath
               |          |
               |          |--98.68%-- pthread_cond_wait@@GLIBC_2.3.2
               |          |          |
               |          |          |--100.00%-- kudu::rpc::ServicePool::RunThread()
               |          |          |          kudu::Thread::SuperviseThread(void*)
               |          |          |          start_thread
               |          |           --0.00%-- [...]
               |          |
               |          |--1.02%-- base::internal::SpinLockDelay(int volatile*, int, int)
               |          |          |
               |          |          |--99.82%-- base::SpinLock::SlowLock()
               |          |          |          |
               |          |          |          |--98.09%-- kudu::rpc::LifoServiceQueue::BlockingGet(std::unique_ptr<kudu::rpc::InboundCall,
std::default_
               |          |          |          |          kudu::rpc::ServicePool::RunThread()
               |          |          |          |          kudu::Thread::SuperviseThread(void*)
               |          |          |          |          start_thread
               |          |          |          |
               |          |          |           --1.91%-- kudu::rpc::LifoServiceQueue::Put(kudu::rpc::InboundCall*,
boost::optional<kudu::rpc::InboundCal
               |          |          |                     kudu::rpc::ServicePool::QueueInboundCall(gscoped_ptr<kudu::rpc::InboundCall,
kudu::DefaultDelet
               |          |          |                     kudu::rpc::Messenger::QueueInboundCall(gscoped_ptr<kudu::rpc::InboundCall,
kudu::DefaultDeleter
               |          |          |                     kudu::rpc::Connection::HandleIncomingCall(gscoped_ptr<kudu::rpc::InboundTransfer,
kudu::Default
               |          |          |                     void ev::base<ev_io, ev::io>::method_thunk<kudu::rpc::Connection,
&(kudu::rpc::Connection::Read
               |          |          |                     ev_invoke_pending
               |          |          |                     ev_run
               |          |          |                     kudu::rpc::ReactorThread::RunThread()
               |          |          |                     kudu::Thread::SuperviseThread(void*)
               |          |          |                     start_thread
               |          |           --0.18%-- [...]
               |           --0.29%-- [...]
                --0.03%-- [...]

Change-Id: I3b4421f4aae4d0e5a2d938881f9eea4e07ff2b10
Reviewed-on: http://gerrit.cloudera.org:8080/6552
Reviewed-by: Alexey Serbin <aserbin@cloudera.com>
Tested-by: Kudu Jenkins
(cherry picked from commit 5f1ca4f3948a61b22946255e4ada895c77bc6adf)
Reviewed-on: http://gerrit.cloudera.org:8080/6801
Reviewed-by: Jean-Daniel Cryans <jdcryans@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/1232c893
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/1232c893
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/1232c893

Branch: refs/heads/branch-1.3.x
Commit: 1232c893ebcdb6230bb947de492554cd3dbe6822
Parents: ed3fde1
Author: Todd Lipcon <todd@apache.org>
Authored: Tue Apr 4 12:43:27 2017 -0700
Committer: Jean-Daniel Cryans <jdcryans@apache.org>
Committed: Thu May 4 21:10:13 2017 +0000

----------------------------------------------------------------------
 src/kudu/security/ca/cert_management.cc | 23 +++++++++------
 src/kudu/security/crypto.cc             | 12 ++++++--
 src/kudu/security/openssl_util.cc       |  8 ++++++
 src/kudu/security/openssl_util.h        | 42 +++++++++++++++++++++++++++-
 src/kudu/security/tls_context.cc        | 16 ++++++++++-
 src/kudu/security/tls_handshake.cc      | 21 ++++++++++++--
 src/kudu/security/tls_socket.cc         |  8 +++---
 7 files changed, 111 insertions(+), 19 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/1232c893/src/kudu/security/ca/cert_management.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/ca/cert_management.cc b/src/kudu/security/ca/cert_management.cc
index 4bdca21..978dd54 100644
--- a/src/kudu/security/ca/cert_management.cc
+++ b/src/kudu/security/ca/cert_management.cc
@@ -70,6 +70,7 @@ CertRequestGenerator::~CertRequestGenerator() {
 
 Status CertRequestGeneratorBase::GenerateRequest(const PrivateKey& key,
                                                  CertSignRequest* ret) const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(ret);
   CHECK(Initialized());
   auto req = ssl_make_unique(X509_REQ_new());
@@ -108,11 +109,10 @@ Status CertRequestGeneratorBase::GenerateRequest(const PrivateKey&
key,
 
 Status CertRequestGeneratorBase::PushExtension(stack_st_X509_EXTENSION* st,
                                                int32_t nid, StringPiece value) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   auto ex = ssl_make_unique(
       X509V3_EXT_conf_nid(nullptr, nullptr, nid, const_cast<char*>(value.data())));
-  if (!ex) {
-    return Status::RuntimeError("error configuring extension");
-  }
+  OPENSSL_RET_IF_NULL(ex, "error configuring extension");
   OPENSSL_RET_NOT_OK(sk_X509_EXTENSION_push(st, ex.release()),
       "error pushing extension into the stack");
   return Status::OK();
@@ -124,6 +124,7 @@ CertRequestGenerator::CertRequestGenerator(Config config)
 
 Status CertRequestGenerator::Init() {
   InitializeOpenSSL();
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
 
   CHECK(!is_initialized_);
   if (config_.cn.empty()) {
@@ -192,6 +193,7 @@ CaCertRequestGenerator::~CaCertRequestGenerator() {
 
 Status CaCertRequestGenerator::Init() {
   InitializeOpenSSL();
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
 
   lock_guard<simple_spinlock> guard(lock_);
   if (is_initialized_) {
@@ -279,6 +281,7 @@ CertSigner::CertSigner(const Cert* ca_cert,
 }
 
 Status CertSigner::Sign(const CertSignRequest& req, Cert* ret) const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   InitializeOpenSSL();
   CHECK(ret);
 
@@ -300,6 +303,7 @@ Status CertSigner::Sign(const CertSignRequest& req, Cert* ret) const
{
 // This is modeled after code in copy_extensions() function from
 // $OPENSSL_ROOT/apps/apps.c with OpenSSL 1.0.2.
 Status CertSigner::CopyExtensions(X509_REQ* req, X509* x) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(req);
   CHECK(x);
   STACK_OF(X509_EXTENSION)* exts = X509_REQ_get_extensions(req);
@@ -325,6 +329,7 @@ Status CertSigner::CopyExtensions(X509_REQ* req, X509* x) {
 }
 
 Status CertSigner::FillCertTemplateFromRequest(X509_REQ* req, X509* tmpl) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(req);
   if (!req->req_info ||
       !req->req_info->pubkey ||
@@ -333,15 +338,15 @@ Status CertSigner::FillCertTemplateFromRequest(X509_REQ* req, X509*
tmpl) {
     return Status::RuntimeError("corrupted CSR: no public key");
   }
   auto pub_key = ssl_make_unique(X509_REQ_get_pubkey(req));
-  if (!pub_key) {
-    return Status::RuntimeError("error unpacking public key from CSR");
-  }
+  OPENSSL_RET_IF_NULL(pub_key, "error unpacking public key from CSR");
   const int rc = X509_REQ_verify(req, pub_key.get());
   if (rc < 0) {
-    return Status::RuntimeError("CSR signature verification error");
+    return Status::RuntimeError("CSR signature verification error",
+                                GetOpenSSLErrors());
   }
   if (rc == 0) {
-    return Status::RuntimeError("CSR signature mismatch");
+    return Status::RuntimeError("CSR signature mismatch",
+                                GetOpenSSLErrors());
   }
   OPENSSL_RET_NOT_OK(X509_set_subject_name(tmpl, X509_REQ_get_subject_name(req)),
       "error setting cert subject name");
@@ -357,6 +362,7 @@ Status CertSigner::DigestSign(const EVP_MD* md, EVP_PKEY* pkey, X509*
x) {
 }
 
 Status CertSigner::GenerateSerial(c_unique_ptr<ASN1_INTEGER>* ret) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   auto btmp = ssl_make_unique(BN_new());
   OPENSSL_RET_NOT_OK(BN_pseudo_rand(btmp.get(), 64, 0, 0),
       "error generating random number");
@@ -371,6 +377,7 @@ Status CertSigner::GenerateSerial(c_unique_ptr<ASN1_INTEGER>* ret)
{
 
 Status CertSigner::DoSign(const EVP_MD* digest, int32_t exp_seconds,
                           X509* ret) const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(ret);
 
   // Version 3 (v3) of X509 certificates. The integer value is one less

http://git-wip-us.apache.org/repos/asf/kudu/blob/1232c893/src/kudu/security/crypto.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/crypto.cc b/src/kudu/security/crypto.cc
index 304c4c2..1aab6b1 100644
--- a/src/kudu/security/crypto.cc
+++ b/src/kudu/security/crypto.cc
@@ -122,6 +122,7 @@ Status PublicKey::FromBIO(BIO* bio, DataFormat format) {
 Status PublicKey::VerifySignature(DigestType digest,
                                   const std::string& data,
                                   const std::string& signature) const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   const EVP_MD* md = GetMessageDigest(digest);
   auto md_ctx = ssl_make_unique(EVP_MD_CTX_create());
 
@@ -141,10 +142,12 @@ Status PublicKey::VerifySignature(DigestType digest,
   const int rc = EVP_DigestVerifyFinal(md_ctx.get(), sig_data, signature.size());
   if (rc < 0 || rc > 1) {
     return Status::RuntimeError(
-        Substitute("error verifying data signature: $0",
-                   GetOpenSSLErrors()));
+        Substitute("error verifying data signature: $0", GetOpenSSLErrors()));
   }
   if (rc == 0) {
+    // No sense stringifying the internal OpenSSL error, since a bad verification
+    // is self-explanatory.
+    ERR_clear_error();
     return Status::Corruption("data signature verification failed");
   }
 
@@ -152,6 +155,7 @@ Status PublicKey::VerifySignature(DigestType digest,
 }
 
 Status PublicKey::Equals(const PublicKey& other, bool* equals) const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   int cmp = EVP_PKEY_cmp(data_.get(), other.data_.get());
   switch (cmp) {
     case -2:
@@ -187,6 +191,7 @@ Status PrivateKey::FromFile(const std::string& fpath, DataFormat format)
{
 // corresponding functionality to read public part from RSA private/public
 // keypair.
 Status PrivateKey::GetPublicKey(PublicKey* public_key) const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(public_key);
   auto rsa = ssl_make_unique(EVP_PKEY_get1_RSA(CHECK_NOTNULL(data_.get())));
   if (PREDICT_FALSE(!rsa)) {
@@ -207,6 +212,7 @@ Status PrivateKey::GetPublicKey(PublicKey* public_key) const {
 Status PrivateKey::MakeSignature(DigestType digest,
                                  const std::string& data,
                                  std::string* signature) const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(signature);
   const EVP_MD* md = GetMessageDigest(digest);
   auto md_ctx = ssl_make_unique(EVP_MD_CTX_create());
@@ -227,6 +233,7 @@ Status PrivateKey::MakeSignature(DigestType digest,
 }
 
 Status GeneratePrivateKey(int num_bits, PrivateKey* ret) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(ret);
   InitializeOpenSSL();
   auto key = ssl_make_unique(EVP_PKEY_new());
@@ -246,6 +253,7 @@ Status GeneratePrivateKey(int num_bits, PrivateKey* ret) {
 }
 
 Status GenerateNonce(string* s) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK_NOTNULL(s);
   unsigned char buf[kNonceSize];
   OPENSSL_RET_NOT_OK(RAND_bytes(buf, sizeof(buf)), "failed to generate nonce");

http://git-wip-us.apache.org/repos/asf/kudu/blob/1232c893/src/kudu/security/openssl_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/openssl_util.cc b/src/kudu/security/openssl_util.cc
index 8c26db3..f5ce822 100644
--- a/src/kudu/security/openssl_util.cc
+++ b/src/kudu/security/openssl_util.cc
@@ -77,12 +77,16 @@ Status CheckOpenSSLInitialized() {
   }
   auto ctx = ssl_make_unique(SSL_CTX_new(SSLv23_method()));
   if (!ctx) {
+    ERR_clear_error();
     return Status::RuntimeError("SSL library appears uninitialized (cannot create SSL_CTX)");
   }
   return Status::OK();
 }
 
 void DoInitializeOpenSSL() {
+  // In case the user's thread has left some error around, clear it.
+  ERR_clear_error();
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   if (g_disable_ssl_init) {
     VLOG(2) << "Not initializing OpenSSL (disabled by application)";
     return;
@@ -102,6 +106,10 @@ void DoInitializeOpenSSL() {
     // which we check before overriding. They aren't thread-safe, however -- that's why
     // we try to get embedding applications to do the right thing here rather than risk a
     // potential initialization race.
+  } else {
+    // As expected, SSL is not initialized, so SSL_CTX_new() failed. Make sure
+    // it didn't leave anything in our error queue.
+    ERR_clear_error();
   }
 
   SSL_load_error_strings();

http://git-wip-us.apache.org/repos/asf/kudu/blob/1232c893/src/kudu/security/openssl_util.h
----------------------------------------------------------------------
diff --git a/src/kudu/security/openssl_util.h b/src/kudu/security/openssl_util.h
index 1dd5859..db85a85 100644
--- a/src/kudu/security/openssl_util.h
+++ b/src/kudu/security/openssl_util.h
@@ -21,6 +21,7 @@
 #include <memory>
 #include <string>
 
+#include <openssl/err.h>
 #include <openssl/pem.h>
 #include <openssl/ssl.h>
 #include <openssl/x509.h>
@@ -49,6 +50,22 @@ typedef struct x509_st X509;
     return Status::RuntimeError((msg), GetOpenSSLErrors()); \
   }
 
+// Scoped helper which DCHECKs that on both scope entry and exit, there are no
+// pending OpenSSL errors for the current thread.
+//
+// This allows us to avoid calling ERR_clear_error() defensively before every
+// OpenSSL call, but rather call it only when we get an error code indicating
+// there may be some pending error.
+//
+// Example usage:
+//
+//    void MyFunc() {
+//      SCOPED_OPENSSL_NO_PENDING_ERRORS;
+//      ... use OpenSSL APIs ...
+//    }
+#define SCOPED_OPENSSL_NO_PENDING_ERRORS \
+  kudu::security::internal::ScopedCheckNoPendingSSLErrors _no_ssl_errors(__PRETTY_FUNCTION__)
+
 namespace kudu {
 namespace security {
 
@@ -79,7 +96,6 @@ std::string GetOpenSSLErrors();
 // See man(3) SSL_get_error for more discussion.
 std::string GetSSLErrorDescription(int error_code);
 
-
 // A generic wrapper for OpenSSL structures.
 template <typename T>
 using c_unique_ptr = std::unique_ptr<T, std::function<void(T*)>>;
@@ -146,5 +162,29 @@ class RawDataWrapper {
   c_unique_ptr<RawDataType> data_;
 };
 
+
+namespace internal {
+
+// Implementation of SCOPED_OPENSSL_NO_PENDING_ERRORS. Use the macro form
+// instead of directly instantiating the implementation class.
+struct ScopedCheckNoPendingSSLErrors {
+ public:
+  explicit ScopedCheckNoPendingSSLErrors(const char* func)
+      : func_(func) {
+    DCHECK_EQ(ERR_peek_error(), 0)
+        << "Expected no pending OpenSSL errors on " << func_
+        << " entry, but had: " << GetOpenSSLErrors();
+  }
+  ~ScopedCheckNoPendingSSLErrors() {
+    DCHECK_EQ(ERR_peek_error(), 0)
+        << "Expected no pending OpenSSL errors on " << func_
+        << " exit, but had: " << GetOpenSSLErrors();
+  }
+
+ private:
+  const char* const func_;
+};
+
+} // namespace internal
 } // namespace security
 } // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/1232c893/src/kudu/security/tls_context.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc
index 17b7f69..09ab577 100644
--- a/src/kudu/security/tls_context.cc
+++ b/src/kudu/security/tls_context.cc
@@ -96,6 +96,7 @@ TlsContext::TlsContext()
 }
 
 Status TlsContext::Init() {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(!ctx_);
 
   // NOTE: 'SSLv23 method' sounds like it would enable only SSLv2 and SSLv3, but in fact
@@ -170,6 +171,7 @@ Status TlsContext::Init() {
 }
 
 Status TlsContext::VerifyCertChainUnlocked(const Cert& cert) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   X509_STORE* store = SSL_CTX_get_cert_store(ctx_.get());
   auto store_ctx = ssl_make_unique<X509_STORE_CTX>(X509_STORE_CTX_new());
 
@@ -180,6 +182,7 @@ Status TlsContext::VerifyCertChainUnlocked(const Cert& cert) {
     int err = X509_STORE_CTX_get_error(store_ctx.get());
     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();
     }
 
@@ -192,6 +195,7 @@ 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),
         X509_verify_cert_error_string(err));
@@ -200,6 +204,7 @@ Status TlsContext::VerifyCertChainUnlocked(const Cert& cert) {
 }
 
 Status TlsContext::UseCertificateAndKey(const Cert& cert, const PrivateKey& key)
{
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   // Verify that the cert and key match.
   RETURN_NOT_OK(cert.CheckKeyMatch(key));
 
@@ -221,6 +226,7 @@ Status TlsContext::UseCertificateAndKey(const Cert& cert, const PrivateKey&
key)
 }
 
 Status TlsContext::AddTrustedCertificate(const Cert& cert) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   VLOG(2) << "Trusting certificate " << cert.SubjectName();
 
   {
@@ -240,7 +246,6 @@ Status TlsContext::AddTrustedCertificate(const Cert& cert) {
   }
 
   unique_lock<RWMutex> lock(lock_);
-  ERR_clear_error();
   auto* cert_store = SSL_CTX_get_cert_store(ctx_.get());
   int rc = X509_STORE_add_cert(cert_store, cert.GetRawData());
   if (rc <= 0) {
@@ -259,6 +264,7 @@ Status TlsContext::AddTrustedCertificate(const Cert& cert) {
 }
 
 Status TlsContext::DumpTrustedCerts(vector<string>* cert_ders) const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   shared_lock<RWMutex> lock(lock_);
 
   vector<string> ret;
@@ -284,6 +290,7 @@ Status TlsContext::DumpTrustedCerts(vector<string>* cert_ders) const
{
 
 namespace {
 Status SetCertAttributes(CertRequestGenerator::Config* config) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   RETURN_NOT_OK_PREPEND(GetFQDN(&config->cn), "could not determine FQDN for CSR");
 
   // If the server has logged in from a keytab, then we have a 'real' identity,
@@ -307,6 +314,7 @@ Status SetCertAttributes(CertRequestGenerator::Config* config) {
 } // anonymous namespace
 
 Status TlsContext::GenerateSelfSignedCertAndKey() {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CertRequestGenerator::Config config;
   RETURN_NOT_OK(SetCertAttributes(&config));
   // Step 1: generate the private key to be self signed.
@@ -336,6 +344,7 @@ Status TlsContext::GenerateSelfSignedCertAndKey() {
   // get cached, so we don't hit the race later. 'VerifyCertChain' also has the
   // effect of triggering the racy codepath.
   ignore_result(X509_check_ca(cert.GetRawData()));
+  ERR_clear_error(); // in case it left anything on the queue.
 
   // Step 4: Adopt the new key and cert.
   unique_lock<RWMutex> lock(lock_);
@@ -350,6 +359,7 @@ Status TlsContext::GenerateSelfSignedCertAndKey() {
 }
 
 boost::optional<CertSignRequest> TlsContext::GetCsrIfNecessary() const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   shared_lock<RWMutex> lock(lock_);
   if (csr_) {
     return csr_->Clone();
@@ -358,6 +368,7 @@ boost::optional<CertSignRequest> TlsContext::GetCsrIfNecessary()
const {
 }
 
 Status TlsContext::AdoptSignedCert(const Cert& cert) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   unique_lock<RWMutex> lock(lock_);
 
   // Verify that the appropriate CA certs have been loaded into the context
@@ -397,6 +408,7 @@ Status TlsContext::AdoptSignedCert(const Cert& cert) {
 
 Status TlsContext::LoadCertificateAndKey(const string& certificate_path,
                                          const string& key_path) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   Cert c;
   RETURN_NOT_OK(c.FromFile(certificate_path, DataFormat::PEM));
   PrivateKey k;
@@ -405,6 +417,7 @@ Status TlsContext::LoadCertificateAndKey(const string& certificate_path,
 }
 
 Status TlsContext::LoadCertificateAuthority(const string& certificate_path) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   Cert c;
   RETURN_NOT_OK(c.FromFile(certificate_path, DataFormat::PEM));
   return AddTrustedCertificate(c);
@@ -412,6 +425,7 @@ Status TlsContext::LoadCertificateAuthority(const string& certificate_path)
{
 
 Status TlsContext::InitiateHandshake(TlsHandshakeType handshake_type,
                                      TlsHandshake* handshake) const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(ctx_);
   CHECK(!handshake->ssl_);
   {

http://git-wip-us.apache.org/repos/asf/kudu/blob/1232c893/src/kudu/security/tls_handshake.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/tls_handshake.cc b/src/kudu/security/tls_handshake.cc
index f57f24f..530fc59 100644
--- a/src/kudu/security/tls_handshake.cc
+++ b/src/kudu/security/tls_handshake.cc
@@ -25,6 +25,7 @@
 #include <openssl/x509.h>
 #include <openssl/x509v3.h>
 
+#include "kudu/gutil/strings/substitute.h"
 #include "kudu/security/cert.h"
 #include "kudu/security/tls_socket.h"
 #include "kudu/util/net/sockaddr.h"
@@ -37,11 +38,13 @@
 
 using std::string;
 using std::unique_ptr;
+using strings::Substitute;
 
 namespace kudu {
 namespace security {
 
 void TlsHandshake::SetSSLVerify() {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(ssl_);
   CHECK(!has_started_);
   int ssl_mode = 0;
@@ -77,12 +80,12 @@ void TlsHandshake::SetSSLVerify() {
 }
 
 Status TlsHandshake::Continue(const string& recv, string* send) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   if (!has_started_) {
     SetSSLVerify();
     has_started_ = true;
   }
   CHECK(ssl_);
-  ERR_clear_error();
 
   BIO* rbio = SSL_get_rbio(ssl_.get());
   int n = BIO_write(rbio, recv.data(), recv.size());
@@ -96,6 +99,9 @@ Status TlsHandshake::Continue(const string& recv, string* send) {
     if (ssl_err != SSL_ERROR_WANT_READ && ssl_err != SSL_ERROR_WANT_WRITE) {
       return Status::RuntimeError("TLS Handshake error", GetSSLErrorDescription(ssl_err));
     }
+    // In the case that we got SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE,
+    // the OpenSSL implementation guarantees that there is no error entered into
+    // the ERR error queue, so no need to ERR_clear_error() here.
   }
 
   BIO* wbio = SSL_get_wbio(ssl_.get());
@@ -116,6 +122,7 @@ Status TlsHandshake::Continue(const string& recv, string* send) {
 }
 
 Status TlsHandshake::Verify(const Socket& socket) const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   DCHECK(SSL_is_init_finished(ssl_.get()));
   CHECK(ssl_);
 
@@ -124,10 +131,11 @@ Status TlsHandshake::Verify(const Socket& socket) const {
   }
   DCHECK(verification_mode_ == TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST);
 
-  ERR_clear_error();
   int rc = SSL_get_verify_result(ssl_.get());
   if (rc != X509_V_OK) {
-    return Status::NotAuthorized("SSL_get_verify_result()", X509_verify_cert_error_string(rc));
+    return Status::NotAuthorized(Substitute("SSL cert verification failed: $0",
+                                            X509_verify_cert_error_string(rc)),
+                                 GetOpenSSLErrors());
   }
 
   // Get the peer certificate.
@@ -174,6 +182,7 @@ Status TlsHandshake::Verify(const Socket& socket) const {
 }
 
 Status TlsHandshake::GetCerts() {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   X509* cert = SSL_get_certificate(ssl_.get());
   if (cert) {
     // For whatever reason, SSL_get_certificate (unlike SSL_get_peer_certificate)
@@ -189,6 +198,7 @@ Status TlsHandshake::GetCerts() {
 }
 
 Status TlsHandshake::Finish(unique_ptr<Socket>* socket) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   RETURN_NOT_OK(GetCerts());
   RETURN_NOT_OK(Verify(**socket));
 
@@ -208,11 +218,13 @@ Status TlsHandshake::Finish(unique_ptr<Socket>* socket) {
 }
 
 Status TlsHandshake::FinishNoWrap(const Socket& socket) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   RETURN_NOT_OK(GetCerts());
   return Verify(socket);
 }
 
 Status TlsHandshake::GetLocalCert(Cert* cert) const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   if (!local_cert_.GetRawData()) {
     return Status::RuntimeError("no local certificate");
   }
@@ -221,6 +233,7 @@ Status TlsHandshake::GetLocalCert(Cert* cert) const {
 }
 
 Status TlsHandshake::GetRemoteCert(Cert* cert) const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   if (!remote_cert_.GetRawData()) {
     return Status::RuntimeError("no remote certificate");
   }
@@ -229,11 +242,13 @@ Status TlsHandshake::GetRemoteCert(Cert* cert) const {
 }
 
 string TlsHandshake::GetCipherSuite() const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(has_started_);
   return SSL_get_cipher_name(ssl_.get());
 }
 
 string TlsHandshake::GetProtocol() const {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(has_started_);
   return SSL_get_version(ssl_.get());
 }

http://git-wip-us.apache.org/repos/asf/kudu/blob/1232c893/src/kudu/security/tls_socket.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/tls_socket.cc b/src/kudu/security/tls_socket.cc
index 77f633f..54c1f14 100644
--- a/src/kudu/security/tls_socket.cc
+++ b/src/kudu/security/tls_socket.cc
@@ -40,6 +40,7 @@ TlsSocket::~TlsSocket() {
 
 Status TlsSocket::Write(const uint8_t *buf, int32_t amt, int32_t *nwritten) {
   CHECK(ssl_);
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
 
   if (PREDICT_FALSE(amt == 0)) {
     // Writing an empty buffer is a no-op. This happens occasionally, eg in the
@@ -49,7 +50,6 @@ Status TlsSocket::Write(const uint8_t *buf, int32_t amt, int32_t *nwritten)
{
     return Status::OK();
   }
 
-  ERR_clear_error();
   errno = 0;
   int32_t bytes_written = SSL_write(ssl_.get(), buf, amt);
   if (bytes_written <= 0) {
@@ -67,8 +67,8 @@ Status TlsSocket::Write(const uint8_t *buf, int32_t amt, int32_t *nwritten)
{
 }
 
 Status TlsSocket::Writev(const struct ::iovec *iov, int iov_len, int32_t *nwritten) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   CHECK(ssl_);
-  ERR_clear_error();
   int32_t total_written = 0;
   // Allows packets to be aggresively be accumulated before sending.
   RETURN_NOT_OK(SetTcpCork(1));
@@ -86,10 +86,10 @@ Status TlsSocket::Writev(const struct ::iovec *iov, int iov_len, int32_t
*nwritt
 }
 
 Status TlsSocket::Recv(uint8_t *buf, int32_t amt, int32_t *nread) {
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   const char* kErrString = "failed to read from TLS socket";
 
   CHECK(ssl_);
-  ERR_clear_error();
   errno = 0;
   int32_t bytes_read = SSL_read(ssl_.get(), buf, amt);
   int save_errno = errno;
@@ -127,7 +127,7 @@ Status TlsSocket::Recv(uint8_t *buf, int32_t amt, int32_t *nread) {
 }
 
 Status TlsSocket::Close() {
-  ERR_clear_error();
+  SCOPED_OPENSSL_NO_PENDING_ERRORS;
   errno = 0;
 
   if (!ssl_) {


Mime
View raw message