This is an automated email from the ASF dual-hosted git repository. tarmstrong pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 746c968ac372938d64298372a0790ec18135c889 Author: Thomas Tauber-Marshall <[email protected]> AuthorDate: Tue Nov 3 14:22:08 2020 -0800 IMPALA-6861: Fix OpenSSL initialization Impalads currently initialize OpenSSL twice: once when initializing Thrift and once when initializing KRPC. The initialization is theoretically idempotent but not thread-safe, so its better to clean this up. This patch disables the Thrift version as its older (last updated in 2015) and the KRPC version contains logic specific to more recent versions of OpenSSL. The catalogd and statestored also now use the KRPC version instead of the Thrift version. It also improves debuggability by adding some additional startup logging. Testing: - Passed existing SSL tests. Change-Id: I35b1362d40c8a12082cc8b531a38b4a485bac0e7 Reviewed-on: http://gerrit.cloudera.org:8080/16704 Reviewed-by: Impala Public Jenkins <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- be/src/rpc/authentication.cc | 6 +++++- be/src/rpc/authentication.h | 10 +--------- be/src/rpc/thrift-server.cc | 5 +++++ 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc index 1b3f82c..1d7acca 100644 --- a/be/src/rpc/authentication.cc +++ b/be/src/rpc/authentication.cc @@ -43,6 +43,7 @@ #include "kudu/rpc/sasl_common.h" #include "kudu/security/gssapi.h" #include "kudu/security/init.h" +#include "kudu/security/openssl_util.h" #include "rpc/auth-provider.h" #include "rpc/authentication-util.h" #include "rpc/thrift-server.h" @@ -1112,7 +1113,10 @@ AuthManager::AuthManager() {} AuthManager::~AuthManager() {} Status AuthManager::Init() { - ssl_socket_factory_.reset(new TSSLSocketFactory(TLSv1_0)); + // Tell Thrift not to initialize SSL for us, as we use Kudu's SSL initializtion. + TSSLSocketFactory::setManualOpenSSLInitialization(true); + kudu::security::InitializeOpenSSL(); + LOG(INFO) << "Initialized " << OPENSSL_VERSION_TEXT; bool use_ldap = false; diff --git a/be/src/rpc/authentication.h b/be/src/rpc/authentication.h index 96d091b..f48fdcd 100644 --- a/be/src/rpc/authentication.h +++ b/be/src/rpc/authentication.h @@ -52,7 +52,7 @@ class AuthManager { ~AuthManager(); /// Set up internal and external AuthProvider classes. This also initializes SSL (via - /// the creation of ssl_socket_factory_). + /// kudu::security::InitializeOpenSSL()). Status Init(); /// Returns the authentication provider to use for "external" communication @@ -81,14 +81,6 @@ class AuthManager { boost::scoped_ptr<AuthProvider> internal_auth_provider_; boost::scoped_ptr<AuthProvider> external_auth_provider_; - /// A thrift SSL socket factory must be created and live the lifetime of the process to - /// ensure that the thrift OpenSSL initialization code runs at Init(), and is not - /// unregistered (which thrift will do when the refcount of TSSLSocketFactory objects - /// reach 0), see IMPALA-4933. For simplicity, and because Kudu will expect SSL to be - /// initialized, this will be created regardless of whether or not SSL credentials are - /// specified. This factory isn't otherwise used. - boost::scoped_ptr<TSSLSocketFactory> ssl_socket_factory_; - /// Used to authenticate usernames and passwords to LDAP. std::unique_ptr<ImpalaLdap> ldap_; }; diff --git a/be/src/rpc/thrift-server.cc b/be/src/rpc/thrift-server.cc index abc3fcf..7d5a8c1 100644 --- a/be/src/rpc/thrift-server.cc +++ b/be/src/rpc/thrift-server.cc @@ -310,6 +310,11 @@ class ImpalaSslSocketFactory : public TSSLSocketFactory { : TSSLSocketFactory(version), password_(password) {} void ciphers(const string& enable) override { + if (ctx_.get() == nullptr) { + throw new TSSLException("ImpalaSslSocketFactory was not properly initialized."); + } + LOG(INFO) << "Enabling the following ciphers for the ImpalaSslSocketFactory: " + << enable; SCOPED_OPENSSL_NO_PENDING_ERRORS; TSSLSocketFactory::ciphers(enable);
