IMPALA-4933, IMPALA-4931: Simplify SSL initialization on startup OpenSSL initialization functions are not threadsafe and are currently called by both thrift, squeasel, and the Kudu client. This change forces thrift to initialize OpenSSL on startup by adding a TSSLSocketFactory to the AuthManager which initializes OpenSSL upon creation and lives the lifetime of the process. Then, squeasel is configured to skip the OpenSSL initialization.
TODO: When the Kudu client supports a flag to disable its initialization path (KUDU-1738), Impala will call that. In the meantime, there will continue to be some small likelihood of a race. Also updates Squeasel in thirdparty to c304d3f3481b07bf153979155f02e0aab24d01de This is necessary to configure squeasel not to init OpenSSL. Change-Id: I245a8a001103ddca7f07349faa82bbb5b9fe3ab0 Reviewed-on: http://gerrit.cloudera.org:8080/6027 Reviewed-by: Sailesh Mukil <[email protected]> Reviewed-by: Matthew Jacobs <[email protected]> Reviewed-by: Henry Robinson <[email protected]> Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/b7a76361 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/b7a76361 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/b7a76361 Branch: refs/heads/master Commit: b7a76361bbab7ea4e3c4df7574755cf3f6bc8cae Parents: 661921b Author: Matthew Jacobs <[email protected]> Authored: Tue Feb 14 15:41:30 2017 -0800 Committer: Impala Public Jenkins <[email protected]> Committed: Fri Feb 17 04:08:32 2017 +0000 ---------------------------------------------------------------------- be/src/rpc/authentication.cc | 2 ++ be/src/rpc/authentication.h | 15 ++++++-- be/src/thirdparty/squeasel/squeasel.c | 57 +++++++++++++++++++----------- be/src/util/CMakeLists.txt | 5 --- be/src/util/webserver.cc | 4 +++ 5 files changed, 54 insertions(+), 29 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b7a76361/be/src/rpc/authentication.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc index db0e608..665a68c 100644 --- a/be/src/rpc/authentication.cc +++ b/be/src/rpc/authentication.cc @@ -955,6 +955,8 @@ Status NoAuthProvider::WrapClientTransport(const string& hostname, } Status AuthManager::Init() { + ssl_socket_factory_.reset(new TSSLSocketFactory()); + bool use_ldap = false; const string excl_msg = "--$0 and --$1 are mutually exclusive " "and should not be set together"; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b7a76361/be/src/rpc/authentication.h ---------------------------------------------------------------------- diff --git a/be/src/rpc/authentication.h b/be/src/rpc/authentication.h index 858b9ea..a89d945 100644 --- a/be/src/rpc/authentication.h +++ b/be/src/rpc/authentication.h @@ -21,6 +21,7 @@ #include <string> #include <thrift/transport/TTransport.h> +#include <thrift/transport/TSSLSocket.h> #include "rpc/auth-provider.h" #include "sasl/sasl.h" @@ -36,14 +37,14 @@ using namespace ::apache::thrift::transport; namespace impala { /// System-wide authentication manager responsible for initialising authentication systems, -/// including Sasl and Kerberos, and for providing auth-enabled Thrift structures to +/// including SSL, Sasl and Kerberos, and for providing auth-enabled Thrift structures to /// servers and clients. class AuthManager { public: static AuthManager* GetInstance() { return AuthManager::auth_manager_; } - /// Set up internal and external AuthProvider classes. This does a bunch of flag - /// checking and calls each AuthProvider->Start(). + /// Set up internal and external AuthProvider classes. This also initializes SSL (via + /// the creation of ssl_socket_factory_). Status Init(); /// Returns the authentication provider to use for "external" communication @@ -64,6 +65,14 @@ class AuthManager { /// don't have to check the auth flags to figure out which auth provider to use. 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_; }; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b7a76361/be/src/thirdparty/squeasel/squeasel.c ---------------------------------------------------------------------- diff --git a/be/src/thirdparty/squeasel/squeasel.c b/be/src/thirdparty/squeasel/squeasel.c index 7cdcad2..3d27d2d 100644 --- a/be/src/thirdparty/squeasel/squeasel.c +++ b/be/src/thirdparty/squeasel/squeasel.c @@ -211,8 +211,8 @@ enum { ACCESS_LOG_FILE, ENABLE_DIRECTORY_LISTING, ERROR_LOG_FILE, GLOBAL_PASSWORDS_FILE, INDEX_FILES, ENABLE_KEEP_ALIVE, ACCESS_CONTROL_LIST, EXTRA_MIME_TYPES, LISTENING_PORTS, DOCUMENT_ROOT, SSL_CERTIFICATE, SSL_PRIVATE_KEY, - SSL_PRIVATE_KEY_PASSWORD, NUM_THREADS, RUN_AS_USER, REWRITE, HIDE_FILES, - REQUEST_TIMEOUT, NUM_OPTIONS + SSL_PRIVATE_KEY_PASSWORD, SSL_GLOBAL_INIT, NUM_THREADS, RUN_AS_USER, REWRITE, + HIDE_FILES, REQUEST_TIMEOUT, NUM_OPTIONS }; static const char *config_options[] = { @@ -238,6 +238,7 @@ static const char *config_options[] = { "ssl_certificate", NULL, "ssl_private_key", NULL, "ssl_private_key_password", NULL, + "ssl_global_init", "yes", "num_threads", "50", "run_as_user", NULL, "url_rewrite_patterns", NULL, @@ -4193,11 +4194,38 @@ static int set_ssl_option(struct sq_context *ctx) { const char *private_key = ctx->config[SSL_PRIVATE_KEY]; if (private_key == NULL) private_key = pem; - // Initialize SSL library - SSL_library_init(); - SSL_load_error_strings(); + // Initialize SSL library, unless the user has disabled this. + int should_init_ssl = (sq_strcasecmp(ctx->config[SSL_GLOBAL_INIT], "yes") == 0); + if (should_init_ssl) { + SSL_library_init(); + SSL_load_error_strings(); + // Initialize locking callbacks, needed for thread safety. + // http://www.openssl.org/support/faq.html#PROG1 + size = sizeof(pthread_mutex_t) * CRYPTO_num_locks(); + if ((ssl_mutexes = (pthread_mutex_t *) malloc((size_t)size)) == NULL) { + cry(fc(ctx), "%s: cannot allocate mutexes: %s", __func__, ssl_error()); + return 0; + } + + for (i = 0; i < CRYPTO_num_locks(); i++) { + pthread_mutex_init(&ssl_mutexes[i], NULL); + } + + CRYPTO_set_locking_callback(&ssl_locking_callback); + CRYPTO_set_id_callback(&ssl_id_callback); + } if ((ctx->ssl_ctx = SSL_CTX_new(SSLv23_server_method())) == NULL) { + unsigned long err_code = ERR_peek_error(); + // If it looks like the error is due to SSL not being initialized, + // provide a better error. + if (!should_init_ssl && + ERR_GET_LIB(err_code) == ERR_LIB_SSL && + ERR_GET_REASON(err_code) == SSL_R_LIBRARY_HAS_NO_CIPHERS) { + cry(fc(ctx), "SSL_CTX_new failed: %s was disabled: OpenSSL must " + "be initialized before starting squeasel", + config_options[SSL_GLOBAL_INIT * 2]); + } cry(fc(ctx), "SSL_CTX_new (server) error: %s", ssl_error()); return 0; } @@ -4223,32 +4251,18 @@ static int set_ssl_option(struct sq_context *ctx) { (void) SSL_CTX_use_certificate_chain_file(ctx->ssl_ctx, pem); } - // Initialize locking callbacks, needed for thread safety. - // http://www.openssl.org/support/faq.html#PROG1 - size = sizeof(pthread_mutex_t) * CRYPTO_num_locks(); - if ((ssl_mutexes = (pthread_mutex_t *) malloc((size_t)size)) == NULL) { - cry(fc(ctx), "%s: cannot allocate mutexes: %s", __func__, ssl_error()); - return 0; - } - - for (i = 0; i < CRYPTO_num_locks(); i++) { - pthread_mutex_init(&ssl_mutexes[i], NULL); - } - - CRYPTO_set_locking_callback(&ssl_locking_callback); - CRYPTO_set_id_callback(&ssl_id_callback); return 1; } static void uninitialize_ssl(struct sq_context *ctx) { int i; - if (ctx->ssl_ctx != NULL) { + if (ctx->ssl_ctx != NULL && + sq_strcasecmp(ctx->config[SSL_GLOBAL_INIT], "yes") == 0) { CRYPTO_set_locking_callback(NULL); for (i = 0; i < CRYPTO_num_locks(); i++) { pthread_mutex_destroy(&ssl_mutexes[i]); } - CRYPTO_set_locking_callback(NULL); CRYPTO_set_id_callback(NULL); } } @@ -4501,6 +4515,7 @@ static int consume_socket(struct sq_context *ctx, struct socket *sp) { clock_get_time(cclock, &mts); mach_port_deallocate(mach_task_self(), cclock); timeout.tv_sec = mts.tv_sec; + timeout.tv_nsec = (long) mts.tv_nsec; #endif ctx->num_free_threads++; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b7a76361/be/src/util/CMakeLists.txt ---------------------------------------------------------------------- diff --git a/be/src/util/CMakeLists.txt b/be/src/util/CMakeLists.txt index 6656a38..8039b31 100644 --- a/be/src/util/CMakeLists.txt +++ b/be/src/util/CMakeLists.txt @@ -18,11 +18,6 @@ set(SQUEASEL_SRC_DIR "${CMAKE_SOURCE_DIR}/be/src/thirdparty/squeasel") set(MUSTACHE_SRC_DIR "${CMAKE_SOURCE_DIR}/be/src/thirdparty/mustache") -# Without this option Squeasel looks up the SSL library at run-time -# and may not guess the correct name on some distributions -SET_SOURCE_FILES_PROPERTIES(${SQUEASEL_SRC_DIR}/squeasel.c PROPERTIES - COMPILE_FLAGS -DNO_SSL_DL) - # where to put generated libraries set(LIBRARY_OUTPUT_PATH "${BUILD_OUTPUT_ROOT_DIRECTORY}/util") http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b7a76361/be/src/util/webserver.cc ---------------------------------------------------------------------- diff --git a/be/src/util/webserver.cc b/be/src/util/webserver.cc index d971852..61ef3ea 100644 --- a/be/src/util/webserver.cc +++ b/be/src/util/webserver.cc @@ -253,6 +253,10 @@ Status Webserver::Start() { string key_password; if (IsSecure()) { + // Impala initializes OpenSSL (see authentication.h). + options.push_back("ssl_global_init"); + options.push_back("false"); + options.push_back("ssl_certificate"); options.push_back(FLAGS_webserver_certificate_file.c_str());
