You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by mj...@apache.org on 2017/02/17 23:17:09 UTC
[6/9] incubator-impala git commit: IMPALA-4933,
IMPALA-4931: Simplify SSL initialization on startup
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 <sa...@cloudera.com>
Reviewed-by: Matthew Jacobs <mj...@cloudera.com>
Reviewed-by: Henry Robinson <he...@cloudera.com>
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 <mj...@cloudera.com>
Authored: Tue Feb 14 15:41:30 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
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());