IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0 (Prerequisite knowledge: Impala must support building and linking against OpenSSL 1.0.0 and 1.0.1. 1.0.0 only supports TLS1.0. 1.0.1 supports TLS1.1 and 1.2 as well).
Rather than check for OpenSSL w/TLSv1.1+ support at compile time, we altered Thrift-0.9.0-p11 to compile on all versions, and fail with an error if an unsupported version is requested at runtime. This patch is the Impala-side counterpart to that work, and also works around a bug. The bug is in OpenSSL. When disabling, say, TLS1.1 we pass an option bitmask to SSL_CTX_set_options() that includes SSL_OP_NO_TLSv1_1. In OpenSSL 1.0.1 this is defined as 0x10000000L but in 1.0.0 it is not defined. So to support compilation on all OpenSSL versions, we added definitions of that constant to Thrift if it was not found at compile time. However, in OpenSSL 1.0.0 *another* option for SSL_CTX_set_options() has the *same* value (0x10000000L). This means that passing SSL_OP_NO_TLSv1_1 to OpenSSL 1.0.0 actually does something completely different. This lack of backwards compatibility is the bug. To work around it, we use the SSLeay() function, which, although cryptically named, returns the value of OPENSSL_VERSION_NUMBER in the version of OpenSSL currently linked against (I have tested that it behaves differently when dynamically linked against different versions of OpenSSL, but compiled against the same one). We use that method to tell us which of the TLS protocol versions are actually supported, and raise errors if they are not. An alternative approach to doing this inside ThriftClient and ThriftServer would be to do so in Thrift; this might be a reasonable future option but for now it is too unwieldy to test a toolchain build that is linked against different OpenSSL versions. To reduce risk, and the number of ways things can go wrong, we do all the protocol version whitelisting in the Impala code. To simplify the code further, we also remove the values of SSLProtoVersions that allowed us to restrict the TLS protocol to exactly *one* value, rather than specify the minimum (e.g. we remove TLSv1_1 but retain TLSv1_1_plus). The more restrictive options were not intended for production use, and using them now will raise an error. Error messages -------------- Passing a valid, but unsupported version to --ssl_minimum_version: "TLS (6) version not supported (linked OpenSSL version is 1234567)" Passing an invalid version to --ssl_minimum_version: "Unknown TLS version: 'tls_4_2'" Testing ------- We assume that tests will be run on the same machine that built Impala, so still execute slightly different tests depending on the detected OpenSSL version. For thrift-server-test, we check to see if a protocol version is supported at runtime, and use that to determine the expected behaviour of a test which starts all combinations of clients and server versions. For the webserver-test, we determine at compile-time the set of supported TLS protocols, and change the test's expected behaviour based on that. Tests have been run when compiling and running against OpenSSL 1.0.0 and OpenSSL 1.0.1. Also include corresponding changes to Squeasel from this commit: https://github.com/henryr/squeasel/commit/eded53 Testing: New webserver and thrift-server tests. Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d Reviewed-on: http://gerrit.cloudera.org:8080/7866 Tested-by: Impala Public Jenkins Reviewed-by: Henry Robinson <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/ac8a72f3 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/ac8a72f3 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/ac8a72f3 Branch: refs/heads/master Commit: ac8a72f3041688316a20c9a85eda5a5cb9af1578 Parents: e4a2d22 Author: Henry Robinson <[email protected]> Authored: Fri Aug 25 11:39:20 2017 -0700 Committer: Henry Robinson <[email protected]> Committed: Sat Sep 2 18:39:21 2017 +0000 ---------------------------------------------------------------------- be/src/rpc/thrift-client.cc | 6 ++++++ be/src/rpc/thrift-server-test.cc | 32 +++++++++++++----------------- be/src/rpc/thrift-server.cc | 21 +++++++++++++++----- be/src/rpc/thrift-server.h | 6 +++++- be/src/service/impala-server.cc | 6 +----- be/src/thirdparty/squeasel/squeasel.c | 28 ++++++++++++++++++++++---- be/src/util/webserver-test.cc | 9 +++++++++ bin/impala-config.sh | 4 ++-- 8 files changed, 77 insertions(+), 35 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/ac8a72f3/be/src/rpc/thrift-client.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/thrift-client.cc b/be/src/rpc/thrift-client.cc index f8da136..d4d246b 100644 --- a/be/src/rpc/thrift-client.cc +++ b/be/src/rpc/thrift-client.cc @@ -44,6 +44,12 @@ ThriftClientImpl::ThriftClientImpl(const std::string& ipaddress, int port, bool SSLProtocol version; init_status_ = SSLProtoVersions::StringToProtocol(FLAGS_ssl_minimum_version, &version); + if (init_status_.ok() && !SSLProtoVersions::IsSupported(version)) { + string err = + Substitute("TLS ($0) version not supported (linked OpenSSL version is $1)", + version, SSLeay()); + init_status_ = Status(err); + } if (!init_status_.ok()) return; ssl_factory_.reset(new TSSLSocketFactory(version)); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/ac8a72f3/be/src/rpc/thrift-server-test.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/thrift-server-test.cc b/be/src/rpc/thrift-server-test.cc index 67e149a..6ceaefd 100644 --- a/be/src/rpc/thrift-server-test.cc +++ b/be/src/rpc/thrift-server-test.cc @@ -263,21 +263,12 @@ TEST(SslTest, MismatchedCiphers) { // equivalents. TEST(SslTest, StringToProtocol) { SSLProtocol version; -#if OPENSSL_VERSION_NUMBER < 0x10001000L - // No TLSv1.1+ support in OpenSSL v1.0.0. - EXPECT_FALSE(SSLProtoVersions::StringToProtocol("tlsv1.2", &version).ok()); - EXPECT_FALSE(SSLProtoVersions::StringToProtocol("tlsv1.1", &version).ok()); - EXPECT_OK(SSLProtoVersions::StringToProtocol("tlsv1", &version)); - EXPECT_EQ(TLSv1_0_plus, version); -#else - map<string, SSLProtocol> TEST_CASES = {{"tlsv1", TLSv1_0_plus}, - {"tlsv1.1", TLSv1_1_plus}, {"tlsv1.2", TLSv1_2_plus}, {"tlsv1_only", TLSv1_0}, - {"tlsv1.1_only", TLSv1_1}, {"tlsv1.2_only", TLSv1_2}}; + map<string, SSLProtocol> TEST_CASES = { + {"tlsv1", TLSv1_0_plus}, {"tlsv1.1", TLSv1_1_plus}, {"tlsv1.2", TLSv1_2_plus}}; for (auto p : TEST_CASES) { EXPECT_OK(SSLProtoVersions::StringToProtocol(p.first, &version)); EXPECT_EQ(p.second, version) << "TLS version: " << p.first; } -#endif } TEST(SslTest, TLSVersionControl) { @@ -293,10 +284,9 @@ TEST(SslTest, TLSVersionControl) { set<SSLProtocol> whitelist; }; -#if OPENSSL_VERSION_NUMBER < 0x10001000L - vector<Config> configs = { - {TLSv1_0, {TLSv1_0, TLSv1_0_plus}}, {TLSv1_0_plus, {TLSv1_0, TLSv1_0_plus}}}; -#else + // Test all configurations supported by Thrift, even if some won't work with the linked + // OpenSSL(). We catch those by checking IsSupported() for both the client and ther + // server. vector<Config> configs = {{TLSv1_0, {TLSv1_0, TLSv1_0_plus}}, {TLSv1_0_plus, {TLSv1_0, TLSv1_1, TLSv1_2, TLSv1_0_plus, TLSv1_1_plus, TLSv1_2_plus}}, @@ -304,7 +294,6 @@ TEST(SslTest, TLSVersionControl) { {TLSv1_1_plus, {TLSv1_1, TLSv1_2, TLSv1_0_plus, TLSv1_1_plus, TLSv1_2_plus}}, {TLSv1_2, {TLSv1_2, TLSv1_0_plus, TLSv1_1_plus, TLSv1_2_plus}}, {TLSv1_2_plus, {TLSv1_2, TLSv1_0_plus, TLSv1_1_plus, TLSv1_2_plus}}}; -#endif for (const auto& config : configs) { // For each config, start a server with the requested protocol spec, and then try to @@ -317,17 +306,24 @@ TEST(SslTest, TLSVersionControl) { .ssl(SERVER_CERT, PRIVATE_KEY) .ssl_version(config.server_version) .Build(&server)); - EXPECT_OK(server->Start()); + if (!SSLProtoVersions::IsSupported(config.server_version)) { + EXPECT_FALSE(server->Start().ok()); + continue; + } + ASSERT_OK(server->Start()); for (auto client_version : SSLProtoVersions::PROTO_MAP) { auto s = ScopedFlagSetter<string>::Make( &FLAGS_ssl_minimum_version, client_version.first); ThriftClient<StatestoreServiceClientWrapper> ssl_client( "localhost", port, "", nullptr, true); + if (!SSLProtoVersions::IsSupported(client_version.second)) { + EXPECT_FALSE(ssl_client.Open().ok()); + continue; + } EXPECT_OK(ssl_client.Open()); bool send_done = false; TRegisterSubscriberResponse resp; - if (config.whitelist.find(client_version.second) == config.whitelist.end()) { EXPECT_THROW(ssl_client.iface()->RegisterSubscriber( resp, TRegisterSubscriberRequest(), &send_done), http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/ac8a72f3/be/src/rpc/thrift-server.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/thrift-server.cc b/be/src/rpc/thrift-server.cc index 35a1852..467c004 100644 --- a/be/src/rpc/thrift-server.cc +++ b/be/src/rpc/thrift-server.cc @@ -77,11 +77,7 @@ namespace impala { // that one (e.g. TLSv1.1 enables v1.1 and v1.2). Specifying TLSv1.1_only enables only // v1.1. map<string, SSLProtocol> SSLProtoVersions::PROTO_MAP = { -#if OPENSSL_VERSION_NUMBER >= 0x10001000L - {"tlsv1.2", TLSv1_2_plus}, {"tlsv1.2_only", TLSv1_2}, {"tlsv1.1", TLSv1_1_plus}, - {"tlsv1.1_only", TLSv1_1}, -#endif - {"tlsv1", TLSv1_0_plus}, {"tlsv1_only", TLSv1_0}}; + {"tlsv1.2", TLSv1_2_plus}, {"tlsv1.1", TLSv1_1_plus}, {"tlsv1", TLSv1_0_plus}}; Status SSLProtoVersions::StringToProtocol(const string& in, SSLProtocol* protocol) { for (const auto& proto : SSLProtoVersions::PROTO_MAP) { @@ -94,6 +90,16 @@ Status SSLProtoVersions::StringToProtocol(const string& in, SSLProtocol* protoco return Status(Substitute("Unknown TLS version: '$0'", in)); } +#define OPENSSL_MIN_VERSION_WITH_TLS_1_1 0x10001000L + +bool SSLProtoVersions::IsSupported(const SSLProtocol& protocol) { + bool is_openssl_1_0_0_or_lower = (SSLeay() < OPENSSL_MIN_VERSION_WITH_TLS_1_1); + if (is_openssl_1_0_0_or_lower) return (protocol == TLSv1_0_plus); + + // All other versions supported by OpenSSL 1.0.1 and later. + return true; +} + bool EnableInternalSslConnections() { // Enable SSL between servers only if both the client validation certificate and the // server certificate are specified. 'Client' here means clients that are used by Impala @@ -374,6 +380,11 @@ class ImpalaSslSocketFactory : public TSSLSocketFactory { } Status ThriftServer::CreateSocket(boost::shared_ptr<TServerTransport>* socket) { if (ssl_enabled()) { + if (!SSLProtoVersions::IsSupported(version_)) { + return Status(TErrorCode::SSL_SOCKET_CREATION_FAILED, + Substitute("TLS ($0) version not supported (linked OpenSSL version is $1)", + version_, SSLeay())); + } try { // This 'factory' is only called once, since CreateSocket() is only called from // Start(). The c'tor may throw if there is an error initializing the SSL context. http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/ac8a72f3/be/src/rpc/thrift-server.h ---------------------------------------------------------------------- diff --git a/be/src/rpc/thrift-server.h b/be/src/rpc/thrift-server.h index e486e37..2002f7f 100644 --- a/be/src/rpc/thrift-server.h +++ b/be/src/rpc/thrift-server.h @@ -340,7 +340,7 @@ class ThriftServerBuilder { bool enable_ssl_ = false; apache::thrift::transport::SSLProtocol version_ = - apache::thrift::transport::SSLProtocol::TLSv1_0; + apache::thrift::transport::SSLProtocol::TLSv1_0_plus; std::string certificate_; std::string private_key_; std::string pem_password_cmd_; @@ -355,6 +355,10 @@ struct SSLProtoVersions { /// one cannot be found. Matching is case-insensitive. static Status StringToProtocol( const std::string& in, apache::thrift::transport::SSLProtocol* protocol); + + /// Returns true if 'protocol' is supported by the version of OpenSSL this binary is + /// linked to. + static bool IsSupported(const apache::thrift::transport::SSLProtocol& protocol); }; // Returns true if, per the process configuration flags, server<->server communications http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/ac8a72f3/be/src/service/impala-server.cc ---------------------------------------------------------------------- diff --git a/be/src/service/impala-server.cc b/be/src/service/impala-server.cc index 4d55c85..18582da 100644 --- a/be/src/service/impala-server.cc +++ b/be/src/service/impala-server.cc @@ -183,11 +183,7 @@ DEFINE_string(ssl_cipher_list, "", const string SSL_MIN_VERSION_HELP = "The minimum SSL/TLS version that Thrift " "services should use for both client and server connections. Supported versions are " -#if OPENSSL_VERSION_NUMBER >= 0x10001000L - "TLSv1.0, TLSv1.1 and TLSv1.2"; -#else - "TLSv1.0"; -#endif + "TLSv1.0, TLSv1.1 and TLSv1.2 (as long as the system OpenSSL library supports them)"; DEFINE_string(ssl_minimum_version, "tlsv1", SSL_MIN_VERSION_HELP.c_str()); DEFINE_int32(idle_session_timeout, 0, "The time, in seconds, that a session may be idle" http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/ac8a72f3/be/src/thirdparty/squeasel/squeasel.c ---------------------------------------------------------------------- diff --git a/be/src/thirdparty/squeasel/squeasel.c b/be/src/thirdparty/squeasel/squeasel.c index 055a959..d461b24 100644 --- a/be/src/thirdparty/squeasel/squeasel.c +++ b/be/src/thirdparty/squeasel/squeasel.c @@ -150,10 +150,20 @@ typedef int socklen_t; static const char *http_500_error = "Internal Server Error"; +#include <openssl/crypto.h> #include <openssl/ssl.h> #include <openssl/err.h> -#define OPENSSL_VERSION_HAS_TLS_1_1 0x10001000L +// If these constants aren't defined, we still need them to compile and maintain backwards +// compatibility with pre-1.0.1 OpenSSL. +#ifndef SSL_OP_NO_TLSv1 +#define SSL_OP_NO_TLSv1 0x04000000U +#endif +#ifndef SSL_OP_NO_TLSv1_1 +#define SSL_OP_NO_TLSv1_1 0x10000000U +#endif + +#define OPENSSL_MIN_VERSION_WITH_TLS_1_1 0x10001000L static const char *month_names[] = { "Jan", "Feb", "Mar", "Apr", "May", "Jun", @@ -4224,12 +4234,18 @@ static int set_ssl_option(struct sq_context *ctx) { int options = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3; if (sq_strcasecmp(ssl_version, "tlsv1") == 0) { // No-op - don't exclude any TLS protocols. -#if OPENSSL_VERSION_NUMBER >= OPENSSL_VERSION_HAS_TLS_1_1 } else if (sq_strcasecmp(ssl_version, "tlsv1.1") == 0) { + if (SSLeay() < OPENSSL_MIN_VERSION_WITH_TLS_1_1) { + cry(fc(ctx), "Unsupported TLS version: %s", ssl_version); + return 0; + } options |= SSL_OP_NO_TLSv1; } else if (sq_strcasecmp(ssl_version, "tlsv1.2") == 0) { + if (SSLeay() < OPENSSL_MIN_VERSION_WITH_TLS_1_1) { + cry(fc(ctx), "Unsupported TLS version: %s", ssl_version); + return 0; + } options |= (SSL_OP_NO_TLSv1 | SSL_OP_NO_TLSv1_1); -#endif } else { cry(fc(ctx), "%s: unknown SSL version: %s", __func__, ssl_version); return 0; @@ -4251,7 +4267,11 @@ static int set_ssl_option(struct sq_context *ctx) { return 0; } - SSL_CTX_set_options(ctx->ssl_ctx, options); + if ((SSL_CTX_set_options(ctx->ssl_ctx, options) & options) != options) { + cry(fc(ctx), "SSL_CTX_set_options (server) error: could not set options (%d)", + options); + return 0; + } if (ctx->config[SSL_PRIVATE_KEY_PASSWORD] != NULL) { SSL_CTX_set_default_passwd_cb(ctx->ssl_ctx, ssl_password_callback); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/ac8a72f3/be/src/util/webserver-test.cc ---------------------------------------------------------------------- diff --git a/be/src/util/webserver-test.cc b/be/src/util/webserver-test.cc index ba6f276..78934c6 100644 --- a/be/src/util/webserver-test.cc +++ b/be/src/util/webserver-test.cc @@ -297,8 +297,10 @@ TEST(Webserver, SslGoodTlsVersion) { #if OPENSSL_VERSION_NUMBER >= 0x10001000L auto versions = {"tlsv1", "tlsv1.1", "tlsv1.2"}; + vector<string> unsupported_versions = {}; #else auto versions = {"tlsv1"}; + auto unsupported_versions = {"tlsv1.1", "tlsv1.2"}; #endif for (auto v: versions) { auto ssl_version = ScopedFlagSetter<string>::Make( @@ -307,6 +309,13 @@ TEST(Webserver, SslGoodTlsVersion) { Webserver webserver(FLAGS_webserver_port); ASSERT_OK(webserver.Start()); } + + for (auto v : unsupported_versions) { + auto ssl_version = ScopedFlagSetter<string>::Make(&FLAGS_ssl_minimum_version, v); + + Webserver webserver(FLAGS_webserver_port); + EXPECT_FALSE(webserver.Start().ok()) << "Version: " << v; + } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/ac8a72f3/bin/impala-config.sh ---------------------------------------------------------------------- diff --git a/bin/impala-config.sh b/bin/impala-config.sh index 15951fd..f3354f1 100755 --- a/bin/impala-config.sh +++ b/bin/impala-config.sh @@ -72,7 +72,7 @@ fi # moving to a different build of the toolchain, e.g. when a version is bumped or a # compile option is changed. The build id can be found in the output of the toolchain # build jobs, it is constructed from the build number and toolchain git hash prefix. -export IMPALA_TOOLCHAIN_BUILD_ID=457-22902e9fb8 +export IMPALA_TOOLCHAIN_BUILD_ID=459-0157f69796 # Versions of toolchain dependencies. # ----------------------------------- export IMPALA_AVRO_VERSION=1.7.4-p4 @@ -107,7 +107,7 @@ export IMPALA_SQUEASEL_VERSION=3.3 # TPC utilities used for test/benchmark data generation. export IMPALA_TPC_DS_VERSION=2.1.0 export IMPALA_TPC_H_VERSION=2.17.0 -export IMPALA_THRIFT_VERSION=0.9.0-p10 +export IMPALA_THRIFT_VERSION=0.9.0-p11 export IMPALA_THRIFT_JAVA_VERSION=0.9.0 export IMPALA_ZLIB_VERSION=1.2.8
