[kudu] 02/03: [security] KUDU-2695 fix CheckOpenSSLInitialized()

[ad...@apache.org]

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

adar pushed a commit to branch branch-1.9.x
in repository https://gitbox.apache.org/repos/asf/kudu.git

commit 14e50e38d80d50be5785a0576e6b86a90b0a77f5
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Mon Feb 11 12:15:12 2019 -0800

    [security] KUDU-2695 fix CheckOpenSSLInitialized()
    
    With OpenSSL 1.1.0, the CRYPTO_get_locking_callback() and the whole old
    threading API has been removed.  With the new threading API in OpenSSL,
    there is no need to set locking callbacks.  In other words, starting
    with OpenSSL 1.1.0, the library is initialized to be multithread-safe,
    and that's exactly what Kudu needs from the OpenSSL's initialization.
    
    Prior to this patch, the sample C++ Kudu client application from
      $KUDU_ROOT/examples/cpp
    with added call of
      KUDU_CHECK_OK(kudu::client::DisableOpenSSLInitialization());
    would fail on Ubuntu 18.04 with error message like below:
      Bad status: Runtime error: Locking callback not initialized
    
    Prior to this patch, the sample Python Kudu client application from
    $KUDU_HOME/examples/python/basic-python-example/basic_example.py
    was failing exactly as reported in KUDU-2695.
    
    With this patch, the same modified C++ Kudu client application
    works fine at Ubuntu 18.04 (OpenSSL 1.1.0g, with packages
    libssl-dev:amd64@1.1.0g-2ubuntu4.3, libssl1.1:amd64@1.1.0g-2ubuntu4.3).
    The above mentioned Python example also works as intended with this fix.
    I also verified that the kudu CLI utility works fine and uses TLS wire
    encryption with this patch.
    
    Change-Id: Ica7cf22ef81bbeffd25ef2826d925c41b97dc2d8
    Reviewed-on: http://gerrit.cloudera.org:8080/12445
    Reviewed-by: Adar Dembo <ad...@cloudera.com>
    Tested-by: Kudu Jenkins
    (cherry picked from commit 1b138a06a4222ad822a2b5cf05dcd0bf988371ef)
    Reviewed-on: http://gerrit.cloudera.org:8080/12464
    Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
 src/kudu/client/client-unittest.cc | 17 ++++++++++++++---
 src/kudu/security/openssl_util.cc  | 34 +++++++++++++++++++++++++++++-----
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/src/kudu/client/client-unittest.cc b/src/kudu/client/client-unittest.cc
index bd8126a..3112ca8 100644
--- a/src/kudu/client/client-unittest.cc
+++ b/src/kudu/client/client-unittest.cc
@@ -26,6 +26,7 @@
 #include <boost/bind.hpp>
 #include <boost/function.hpp>
 #include <gtest/gtest.h>
+#include <openssl/opensslv.h>
 
 #include "kudu/client/client-internal.h"
 #include "kudu/client/error_collector.h"
@@ -168,10 +169,20 @@ TEST(ClientUnitTest, TestSchemaBuilder_CompoundKey_BadColumnName) {
 }
 
 TEST(ClientUnitTest, TestDisableSslFailsIfNotInitialized) {
-  // If we try to disable SSL initialization without setting up SSL properly,
-  // it should return an error.
-  Status s = DisableOpenSSLInitialization();
+  const auto s = DisableOpenSSLInitialization();
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+  // With the pre-1.1.0 OpenSSL library, if we try to disable SSL
+  // initialization without setting up SSL properly, it should return an error.
   ASSERT_STR_MATCHES(s.ToString(), "Locking callback not initialized");
+#else
+  // Starting with OpenSSL 1.1.0, the library can be implicitly initialized
+  // upon calling the relevant methods of the API (e.g. SSL_CTX_new()) and
+  // overall there is no reliable non-intrusive way to determine that the
+  // library has already been initialized. So, the requirement to have
+  // the library initialized before calling DisableOpenSSLInitialization()
+  // is gone since OpenSSL 1.1.0.
+  ASSERT_TRUE(s.ok()) << s.ToString();
+#endif
 }
 
 namespace {
diff --git a/src/kudu/security/openssl_util.cc b/src/kudu/security/openssl_util.cc
index 1576ea8..e96fcb3 100644
--- a/src/kudu/security/openssl_util.cc
+++ b/src/kudu/security/openssl_util.cc
@@ -82,25 +82,49 @@ void LockingCB(int mode, int type, const char* /*file*/, int /*line*/) {
 #endif
 
 Status CheckOpenSSLInitialized() {
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+  // Starting with OpenSSL 1.1.0, the old thread API became obsolete
+  // (see changelist 2e52e7df5 in the OpenSSL upstream repo), and
+  // CRYPTO_get_locking_callback() always returns nullptr. Also, the library
+  // always initializes its internals for multi-threaded usage.
+  // Another point is that starting with version 1.1.0, SSL_CTX_new()
+  // initializes the OpenSSL library under the hood, so SSL_CTX_new() would
+  // not return nullptr unless there was an error during the initialization
+  // of the library. That makes this code in CheckOpenSSLInitialized() obsolete
+  // starting with OpenSSL version 1.1.0.
+  //
+  // Starting with OpenSSL 1.1.0, there isn't a straightforward way to
+  // determine whether the library has already been initialized if using just
+  // the API (well, there is CRYPTO_secure_malloc_initialized() but that's just
+  // for the crypto library and it's implementation-dependent). But from the
+  // other side, the whole idea that this method should check whether the
+  // library has already been initialized is not relevant anymore: even if it's
+  // not yet initialized, the first call to SSL_CTX_new() (from, say,
+  // TlsContext::Init()) will initialize the library under the hood, so the
+  // library will be ready for multi-thread usage by Kudu.
   if (!CRYPTO_get_locking_callback()) {
     return Status::RuntimeError("Locking callback not initialized");
   }
   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::RuntimeError(
+        "SSL library appears uninitialized (cannot create SSL_CTX)");
   }
+#endif
   return Status::OK();
 }
 
 void DoInitializeOpenSSL() {
-#if OPENSSL_VERSION_NUMBER > 0x10100000L
+#if OPENSSL_VERSION_NUMBER >= 0x10100000L
   // The OPENSSL_init_ssl manpage [1] says "As of version 1.1.0 OpenSSL will
   // automatically allocate all resources it needs so no explicit initialisation
   // is required." However, eliding library initialization leads to a memory
-  // leak in some versions of OpenSSL 1.1 when the first OpenSSL is
-  // ERR_peek_error [2]. In Kudu this is often the
-  // case due to prolific application of SCOPED_OPENSSL_NO_PENDING_ERRORS.
+  // leak in some versions of OpenSSL 1.1 when the first OpenSSL call is
+  // ERR_peek_error (see [2] for details; the issue was addressed in OpenSSL
+  // 1.1.0i (OPENSSL_VERSION_NUMBER 0x1010009f)). In Kudu this is often the
+  // case due to prolific application of the SCOPED_OPENSSL_NO_PENDING_ERRORS
+  // macro.
   //
   // Rather than determine whether this particular OpenSSL instance is
   // leak-free, we'll initialize the library explicitly.