This is an automated email from the ASF dual-hosted git repository. jasonmfehr pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 0cd240a1cbd25ef097f392cbc02a70ad0b5f11d6 Author: jasonmfehr <[email protected]> AuthorDate: Thu Aug 14 13:02:10 2025 -0700 IMPALA-14312: Fix Issues with ssl_minimum_version Flag Validator The validator for the ssl_minimum_version flag has several issues that are fixed. Allows flag to be empty as long as both internal and external TLS is not configured. Fixes allowed value for TLS v1 to be tlsv1 instead of the incorrect value tlsv1.0. Removes "tlsv1.3" as an allowed value since Thrift does not support that value as the minimum TLS version. Testing accomplished by new ctests and manual testing. Change-Id: I6493852b581e26c203b6b46b97be76100bcc534b Reviewed-on: http://gerrit.cloudera.org:8080/23300 Tested-by: Impala Public Jenkins <[email protected]> Reviewed-by: Jason Fehr <[email protected]> --- be/src/rpc/thrift-server-test.cc | 2 +- be/src/rpc/thrift-server.cc | 9 +-- be/src/rpc/thrift-server.h | 3 +- be/src/service/impala-server-test.cc | 113 +++++++++++++++++++++++++++++++++++ be/src/service/impala-server.cc | 25 +++++--- be/src/util/openssl-util.cc | 11 ++++ be/src/util/openssl-util.h | 14 +++++ 7 files changed, 164 insertions(+), 13 deletions(-) diff --git a/be/src/rpc/thrift-server-test.cc b/be/src/rpc/thrift-server-test.cc index 960a81cd4..d2cd27783 100644 --- a/be/src/rpc/thrift-server-test.cc +++ b/be/src/rpc/thrift-server-test.cc @@ -449,7 +449,7 @@ TEST(SslTest, TLSVersionControl) { for (auto client_version : SSLProtoVersions::PROTO_MAP) { auto s = ScopedFlagSetter<string>::Make( - &FLAGS_ssl_minimum_version, client_version.first); + &FLAGS_ssl_minimum_version, string(client_version.first)); ThriftClient<StatestoreServiceClientWrapper> ssl_client( "localhost", port, "", nullptr, true); if (!SSLProtoVersions::IsSupported(client_version.second)) { diff --git a/be/src/rpc/thrift-server.cc b/be/src/rpc/thrift-server.cc index f8cab8e48..b35ac08c8 100644 --- a/be/src/rpc/thrift-server.cc +++ b/be/src/rpc/thrift-server.cc @@ -16,6 +16,7 @@ // under the License. #include <mutex> +#include <string_view> #include <boost/algorithm/string.hpp> #include <boost/filesystem.hpp> #include <boost/uuid/uuid_io.hpp> @@ -71,10 +72,10 @@ namespace impala { // Specifies the allowed set of values for --ssl_minimum_version. To keep consistent with // Apache Kudu, specifying a single version enables all versions including and succeeding // that one (e.g. TLSv1.1 enables v1.1 and v1.2). -map<string, SSLProtocol> SSLProtoVersions::PROTO_MAP = { - {"tlsv1.2", TLSv1_2}, - {"tlsv1.1", TLSv1_1}, - {"tlsv1", TLSv1_0}}; +map<std::string_view, SSLProtocol> SSLProtoVersions::PROTO_MAP = { + {TLSVersions::TLSV1_2, TLSv1_2}, + {TLSVersions::TLSV1_1, TLSv1_1}, + {TLSVersions::TLSV1_0, TLSv1_0}}; // A generic wrapper for OpenSSL structures. template <typename T> diff --git a/be/src/rpc/thrift-server.h b/be/src/rpc/thrift-server.h index a6fe327a7..2fe01138e 100644 --- a/be/src/rpc/thrift-server.h +++ b/be/src/rpc/thrift-server.h @@ -18,6 +18,7 @@ #pragma once #include <mutex> +#include <string_view> #include <boost/scoped_ptr.hpp> #include <boost/shared_ptr.hpp> #include <boost/unordered_map.hpp> @@ -606,7 +607,7 @@ class ThriftServerBuilder { /// Contains a map from string for --ssl_minimum_version to Thrift's SSLProtocol. struct SSLProtoVersions { - static std::map<std::string, apache::thrift::transport::SSLProtocol> PROTO_MAP; + static std::map<std::string_view, apache::thrift::transport::SSLProtocol> PROTO_MAP; /// Given a string, find a corresponding SSLProtocol from PROTO_MAP. Returns an error if /// one cannot be found. Matching is case-insensitive. diff --git a/be/src/service/impala-server-test.cc b/be/src/service/impala-server-test.cc index 10dd22b5c..d31229b38 100644 --- a/be/src/service/impala-server-test.cc +++ b/be/src/service/impala-server-test.cc @@ -15,6 +15,7 @@ // specific language governing permissions and limitations // under the License. +#include <gflags/gflags.h> #include <gutil/strings/substitute.h> #include <sstream> #include <vector> @@ -70,3 +71,115 @@ TEST(ImpalaServerTest, PopulateAuthorizedProxyConfig) { EXPECT_EQ(proxy_map.end(), proxy_map.find("doesnotexist")); } } + +// ssl_minimum_version flag validation tests. +const auto VALID_TLS_VERSIONS = {"tlsv1", "tlsv1.1", "tlsv1.2"}; +TEST(ImpalaServerTest, FlagSSLMinimumVersionValidValues) { + for (const auto& tls_ver : VALID_TLS_VERSIONS) { + gflags::FlagSaver s; + EXPECT_EQ(Substitute("ssl_minimum_version set to $0\n", tls_ver), + gflags::SetCommandLineOption("ssl_minimum_version", tls_ver)); + } +} + +TEST(ImpalaServerTest, FlagSSLMinimumVersionInvalidValues) { + { + gflags::FlagSaver s; + EXPECT_TRUE(gflags::SetCommandLineOption("ssl_minimum_version", "tlsv1.0").empty()); + } + + { + gflags::FlagSaver s; + EXPECT_TRUE(gflags::SetCommandLineOption("ssl_minimum_version", "TLSv1").empty()); + } + + { + gflags::FlagSaver s; + EXPECT_TRUE(gflags::SetCommandLineOption("ssl_minimum_version", "TLSv1.1").empty()); + } + + { + gflags::FlagSaver s; + EXPECT_TRUE(gflags::SetCommandLineOption("ssl_minimum_version", "TLSv1.2").empty()); + } + + { + gflags::FlagSaver s; + EXPECT_TRUE(gflags::SetCommandLineOption("ssl_minimum_version", "tlsv1.3").empty()); + } + + { + gflags::FlagSaver s; + EXPECT_TRUE(gflags::SetCommandLineOption("ssl_minimum_version", "TLSv1.3").empty()); + } + + { + gflags::FlagSaver s; + EXPECT_TRUE(gflags::SetCommandLineOption("ssl_minimum_version", " ").empty()); + } +} + +TEST(ImpalaServerTest, FlagSSLMinimumVersionEmpty) { + { + // Flag can be empty if TLS is not configured. + gflags::FlagSaver s; + EXPECT_EQ("ssl_minimum_version set to \n", + gflags::SetCommandLineOption("ssl_minimum_version", "")); + } + + { + // Flag cannot be empty if external TLS is configured. + gflags::FlagSaver s; + gflags::SetCommandLineOption("ssl_server_certificate", "some_cert.pem"); + gflags::SetCommandLineOption("ssl_private_key", "some_key.pem"); + EXPECT_TRUE(gflags::SetCommandLineOption("ssl_minimum_version", "").empty()); + } + + { + // Flag cannot be empty if internal TLS is configured. + gflags::FlagSaver s; + gflags::SetCommandLineOption("ssl_client_ca_certificate", "some_ca_cert.pem"); + gflags::SetCommandLineOption("ssl_server_certificate", "some_cert.pem"); + gflags::SetCommandLineOption("ssl_private_key", "some_key.pem"); + EXPECT_TRUE(gflags::SetCommandLineOption("ssl_minimum_version", "").empty()); + } +} + +TEST(ImpalaServerTest, FlagSSLMinimumVersionExternalTLSConfigured) { + for (const auto& tls_ver : VALID_TLS_VERSIONS) { + // Flag validation passes if external TLS is configured. + gflags::FlagSaver s; + gflags::SetCommandLineOption("ssl_server_certificate", "some_cert.pem"); + gflags::SetCommandLineOption("ssl_private_key", "some_key.pem"); + EXPECT_EQ(Substitute("ssl_minimum_version set to $0\n", tls_ver), + gflags::SetCommandLineOption("ssl_minimum_version", tls_ver)); + } + + { + // Flag validation fails if external TLS is configured and flag has an invalid value. + gflags::FlagSaver s; + gflags::SetCommandLineOption("ssl_server_certificate", "some_cert.pem"); + gflags::SetCommandLineOption("ssl_private_key", "some_key.pem"); + EXPECT_TRUE(gflags::SetCommandLineOption("ssl_minimum_version", "foo").empty()); + } +} + +TEST(ImpalaServerTest, FlagSSLMinimumVersionInternalTLSConfigured) { + for (const auto& tls_ver : VALID_TLS_VERSIONS) { + // Flag validation passes if internal TLS is configured. + gflags::FlagSaver s; + gflags::SetCommandLineOption("ssl_server_certificate", "some_cert.pem"); + gflags::SetCommandLineOption("ssl_private_key", "some_key.pem"); + EXPECT_EQ(Substitute("ssl_minimum_version set to $0\n", tls_ver), + gflags::SetCommandLineOption("ssl_minimum_version", tls_ver)); + } + + { + // Flag validation fails if internal TLS is configured and flag has an invalid value. + gflags::FlagSaver s; + gflags::SetCommandLineOption("ssl_client_ca_certificate", "some_ca_cert.pem"); + gflags::SetCommandLineOption("ssl_server_certificate", "some_cert.pem"); + gflags::SetCommandLineOption("ssl_private_key", "some_key.pem"); + EXPECT_TRUE(gflags::SetCommandLineOption("ssl_minimum_version", "foo").empty()); + } +} \ No newline at end of file diff --git a/be/src/service/impala-server.cc b/be/src/service/impala-server.cc index ccd9a189b..645679bab 100644 --- a/be/src/service/impala-server.cc +++ b/be/src/service/impala-server.cc @@ -21,6 +21,7 @@ #include <algorithm> #include <exception> #include <sstream> +#include <string_view> #ifdef CALLONCEHACK #include <calloncehack.h> #endif @@ -255,15 +256,25 @@ DEFINE_string(tls_ciphersuites, 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 " - "TLSv1.0, TLSv1.1 and TLSv1.2 (as long as the system OpenSSL library supports them)"; + "TLSv1, TLSv1.1, and TLSv1.2 (as long as the system OpenSSL library supports them)"; DEFINE_string(ssl_minimum_version, "tlsv1.2", SSL_MIN_VERSION_HELP.c_str()); DEFINE_validator(ssl_minimum_version, [](const char* flagname, const string& value) { - const std::string trimmed = boost::algorithm::trim_copy(value); - return boost::iequals(trimmed, "tlsv1") - || boost::iequals(trimmed, "tlsv1.0") - || boost::iequals(trimmed, "tlsv1.1") - || boost::iequals(trimmed, "tlsv1.2") - || boost::iequals(trimmed, "tlsv1.3"); + // empty is allowed if TLS is not configured + if (value.empty()) { + if (impala::IsExternalTlsConfigured() || impala::IsInternalTlsConfigured()) { + return false; + } + + return true; + } + + for (const std::string_view& valid_value : impala::TLSVersions::VALUES) { + if (value == valid_value) { + return true; + } + } + + return false; }); DEFINE_int32(idle_session_timeout, 0, "The time, in seconds, that a session may be idle" diff --git a/be/src/util/openssl-util.cc b/be/src/util/openssl-util.cc index cad078565..80d9f23c5 100644 --- a/be/src/util/openssl-util.cc +++ b/be/src/util/openssl-util.cc @@ -83,6 +83,17 @@ static const int RNG_RESEED_INTERVAL = 128; // Number of bytes of entropy to add at RNG_RESEED_INTERVAL. static const int RNG_RESEED_BYTES = 512; +// Valid strings for TLS versions. +const std::string_view TLSVersions::TLSV1_0 = "tlsv1"; +const std::string_view TLSVersions::TLSV1_1 = "tlsv1.1"; +const std::string_view TLSVersions::TLSV1_2 = "tlsv1.2"; + +inline const std::vector<std::string_view> TLSVersions::VALUES = { + TLSVersions::TLSV1_0, + TLSVersions::TLSV1_1, + TLSVersions::TLSV1_2 +}; + int MaxSupportedTlsVersion() { #if OPENSSL_VERSION_NUMBER < 0x10100000L return SSLv23_method()->version; diff --git a/be/src/util/openssl-util.h b/be/src/util/openssl-util.h index b52c0710a..b64ef8281 100644 --- a/be/src/util/openssl-util.h +++ b/be/src/util/openssl-util.h @@ -45,6 +45,20 @@ namespace impala { #define TLS1_2_VERSION 0x0303 #endif +/// Valid strings for minimum TLS versions. Does not include TLSv1.3 because Thrift does +/// not yet support that value as a minimum TLS version. +/// TODO: Add TLSV1_3 when ssl_minimum_version=tlsv1.3 is supported. +class TLSVersions final { +public: + static const std::string_view TLSV1_0; + static const std::string_view TLSV1_1; + static const std::string_view TLSV1_2; + + static const std::vector<std::string_view> VALUES; + + TLSVersions() = delete; +}; + /// Returns the maximum supported TLS version available in the linked OpenSSL library. int MaxSupportedTlsVersion();
