This is an automated email from the ASF dual-hosted git repository. laszlog pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 5164a8c4196bc6bb3372706acf3658507ce0d5c9 Author: Michael Smith <[email protected]> AuthorDate: Thu Feb 6 15:56:21 2025 -0800 IMPALA-13687: Support shared secret key for cookies Adds support for a shared secret key across coordinators for cookie validation. The key is used with Thrift servers using HTTPS to support sharing the same cookie across sessions with different coordinators. It's also used when authentication is enabled on the web UI. The key path is configured with '--cookie_secret_file=<path>'. Adds an inotify watcher on the cookie secret key to reload when it's updated. Inotify failures during startup will cause Impala to exit with an error. Inotify errors at runtime are fatal and will cause Impala to exit. Failures reloading the key will be logged as errors for hash_reload_grace_period_s (default=300) seconds, after which they will become fatal and cause Impala to exit. Testing: - Adds new LdapHS2Test, LdapWebserverTest, and LdapImpalaShellTest cases for shared cookie. - Adds AuthenticationHash and AuthenticationHashFromFile unit tests. - Drops webserver test for new HMAC because the hash in AuthManager is now re-used. HMAC re-use is better tested in new cases. Generated-by: Github Copilot (GPT-4.1) Change-Id: Ie2e2345f771608069407e9dcf7ed4697fc0214e7 Reviewed-on: http://gerrit.cloudera.org:8080/22462 Reviewed-by: Joe McDonnell <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- be/src/rpc/auth-provider.h | 3 - be/src/rpc/authentication.cc | 37 ++++-- be/src/rpc/authentication.h | 9 +- be/src/util/openssl-util-test.cc | 130 +++++++++++++++++++++ be/src/util/openssl-util.cc | 127 ++++++++++++++++++++ be/src/util/openssl-util.h | 49 +++++++- be/src/util/webserver-test.cc | 9 -- be/src/util/webserver.cc | 10 +- be/src/util/webserver.h | 3 - docs/topics/impala_client.xml | 34 +++++- .../apache/impala/customcluster/LdapHS2Test.java | 111 ++++++++++++++++++ .../impala/customcluster/LdapImpalaShellTest.java | 63 ++++++++++ .../LdapSearchBindImpalaShellTest.java | 13 +++ .../LdapSimpleBindImpalaShellTest.java | 11 ++ .../impala/customcluster/LdapWebserverTest.java | 50 ++++++++ .../java/org/apache/impala/testutil/TestUtils.java | 31 +++++ .../java/org/apache/impala/testutil/WebClient.java | 7 ++ 17 files changed, 659 insertions(+), 38 deletions(-) diff --git a/be/src/rpc/auth-provider.h b/be/src/rpc/auth-provider.h index 9f1a1420a..de48315cd 100644 --- a/be/src/rpc/auth-provider.h +++ b/be/src/rpc/auth-provider.h @@ -175,9 +175,6 @@ class SecureAuthProvider : public AuthProvider { /// Principal's realm, again derived from principal. std::string realm_; - /// Used to generate and verify signatures for cookies. - AuthenticationHash hash_; - /// One-time kerberos-specific environment variable setup. Called by InitKerberos(). Status InitKerberosEnv() WARN_UNUSED_RESULT; }; diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc index a282ac1fe..df47e92dc 100644 --- a/be/src/rpc/authentication.cc +++ b/be/src/rpc/authentication.cc @@ -95,6 +95,9 @@ DECLARE_bool(use_system_auth_to_local); DECLARE_int64(max_cookie_lifetime_s); +DEFINE_string(cookie_secret_file, "", "Path to file containing cookie secret used to " + "construct an HMAC for verification."); + DECLARE_int32(hs2_http_port); DEFINE_string(sasl_path, "", "Colon separated list of paths to look for SASL " @@ -1569,6 +1572,7 @@ void SecureAuthProvider::SetupConnectionContext( THttpServer* http_input_transport = down_cast<THttpServer*>(input_transport); THttpServer* http_output_transport = down_cast<THttpServer*>(output_transport); THttpServer::HttpCallbacks callbacks; + const AuthenticationHash& hash = AuthManager::GetInstance()->GetAuthHash(); string saml_path; if (has_saml_) { @@ -1580,16 +1584,16 @@ void SecureAuthProvider::SetupConnectionContext( std::placeholders::_2, std::placeholders::_3); callbacks.return_headers_fn = std::bind(ReturnHeaders, connection_ptr.get()); callbacks.cookie_auth_fn = - std::bind(CookieAuth, connection_ptr.get(), hash_, std::placeholders::_1); + std::bind(CookieAuth, connection_ptr.get(), hash, std::placeholders::_1); callbacks.trusted_domain_check_fn = std::bind(TrustedDomainCheck, - connection_ptr.get(), hash_, std::placeholders::_1, std::placeholders::_2); + connection_ptr.get(), hash, std::placeholders::_1, std::placeholders::_2); if (has_ldap_) { callbacks.basic_auth_fn = - std::bind(BasicAuth, connection_ptr.get(), hash_, std::placeholders::_1); + std::bind(BasicAuth, connection_ptr.get(), hash, std::placeholders::_1); } if (!principal_.empty()) { callbacks.negotiate_auth_fn = std::bind(NegotiateAuth, connection_ptr.get(), - hash_, std::placeholders::_1, std::placeholders::_2); + hash, std::placeholders::_1, std::placeholders::_2); } if (has_saml_) { callbacks.init_wrapped_http_request_fn = std::bind( @@ -1597,21 +1601,21 @@ void SecureAuthProvider::SetupConnectionContext( callbacks.get_saml_redirect_fn = std::bind(GetSaml2Redirect, connection_ptr.get()); callbacks.validate_saml2_authn_response_fn = - std::bind(ValidateSaml2AuthnResponse, connection_ptr.get(), hash_); + std::bind(ValidateSaml2AuthnResponse, connection_ptr.get(), hash); callbacks.validate_saml2_bearer_fn = - std::bind(ValidateSaml2Bearer, connection_ptr.get(), hash_); + std::bind(ValidateSaml2Bearer, connection_ptr.get(), hash); } if (has_jwt_ ) { - callbacks.jwt_token_auth_fn = - std::bind(JWTTokenAuth, connection_ptr.get(), hash_, std::placeholders::_1); + callbacks.jwt_token_auth_fn = std::bind( + JWTTokenAuth, connection_ptr.get(), hash, std::placeholders::_1); } if (has_oauth_) { - callbacks.oauth_token_auth_fn = - std::bind(OAuthTokenAuth, connection_ptr.get(), hash_, std::placeholders::_1); + callbacks.oauth_token_auth_fn = std::bind( + OAuthTokenAuth, connection_ptr.get(), hash, std::placeholders::_1); } if (!FLAGS_trusted_auth_header.empty()) { callbacks.trusted_auth_header_handle_fn = std::bind( - HandleTrustedAuthHeader, connection_ptr.get(), hash_, std::placeholders::_1); + HandleTrustedAuthHeader, connection_ptr.get(), hash, std::placeholders::_1); } callbacks.set_http_origin_fn = std::bind(SetOrigin, connection_ptr.get(), std::placeholders::_1); @@ -1755,6 +1759,17 @@ Status AuthManager::Init() { ImpalaLdap::CreateLdap(&ldap_, FLAGS_ldap_user_filter, FLAGS_ldap_group_filter)); } + unique_ptr<AuthenticationHash> hash; + if (!FLAGS_cookie_secret_file.empty()) { + LOG(INFO) << "Using cookie secret file: " << FLAGS_cookie_secret_file; + hash.reset(new AuthenticationHashFromFile(FLAGS_cookie_secret_file)); + } else { + LOG(INFO) << "Using auto-generated cookie secret"; + hash.reset(new AuthenticationHash()); + } + RETURN_IF_ERROR(hash->Init()); + hash_.reset(hash.release()); + if (FLAGS_principal.empty() && !FLAGS_be_principal.empty()) { return Status("A back end principal (--be_principal) was supplied without " "also supplying a regular principal (--principal). Either --principal " diff --git a/be/src/rpc/authentication.h b/be/src/rpc/authentication.h index 313d5d0d7..0f5e8f16e 100644 --- a/be/src/rpc/authentication.h +++ b/be/src/rpc/authentication.h @@ -78,7 +78,11 @@ class AuthManager { /// connection this applies to would be backend <-> statestore. AuthProvider* GetInternalAuthProvider(); - ImpalaLdap* GetLdap() { return ldap_.get(); } + ImpalaLdap* GetLdap() const { return ldap_.get(); } + + /// Returns the AuthenticationHash used to generate and verify signatures for cookies. + /// The returned reference is valid as long as the AuthManager instance is valid. + const AuthenticationHash& GetAuthHash() const { return *hash_.get(); } private: /// One-time kerberos-specific environment variable setup. Sets variables like @@ -99,6 +103,9 @@ class AuthManager { /// Used to authenticate usernames and passwords to LDAP. std::unique_ptr<ImpalaLdap> ldap_; + + /// Used to generate and verify signatures for cookies. + std::unique_ptr<const AuthenticationHash> hash_; }; diff --git a/be/src/util/openssl-util-test.cc b/be/src/util/openssl-util-test.cc index 29f26437e..8ada6deb0 100644 --- a/be/src/util/openssl-util-test.cc +++ b/be/src/util/openssl-util-test.cc @@ -15,6 +15,7 @@ // specific language governing permissions and limitations // under the License. +#include <fstream> #include <random> #include <glog/logging.h> @@ -28,14 +29,18 @@ #include "kudu/util/env.h" #include "kudu/util/faststring.h" #include "kudu/util/status.h" +#include "testutil/death-test-util.h" #include "testutil/gtest-util.h" #include "util/openssl-util.h" +#include "util/time.h" using std::uniform_int_distribution; using std::mt19937_64; using strings::Substitute; using std::string; +DECLARE_int32(hash_reload_grace_period_s); + namespace impala { class OpenSSLUtilTest : public ::testing::Test { @@ -63,6 +68,26 @@ class OpenSSLUtilTest : public ::testing::Test { } } + struct KeyFile { + ~KeyFile() { + unlink(path.c_str()); + } + string path; + }; + + void WriteRandomKeyToFile(const string& path, int key_len) { + vector<uint8_t> key(key_len); + std::ofstream key_file(path, std::ios::binary); + GenerateRandomBytes(key.data(), key_len); + key_file.write(reinterpret_cast<const char*>(key.data()), key.size()); + } + + KeyFile MakeKeyFile(int key_len = AuthenticationHash::HashLen()) { + string key_path = Substitute("/tmp/auth_key_$0", getpid()); + WriteRandomKeyToFile(key_path, key_len); + return KeyFile{move(key_path)}; + } + void TestEncryptionDecryption(const int64_t buffer_size) { vector<uint8_t> original(buffer_size); // Scratch buffer for in-place encryption. @@ -463,4 +488,109 @@ TEST_F(OpenSSLUtilTest, ValidatePemBundleOneExpiredOneFutureDated) { read_cert(EXPIRED_CERT))), _msg_time(1, 1)); } +TEST_F(OpenSSLUtilTest, AuthenticationHash) { + // Create an AuthenticationHash and verify we can Compute a signature and Verify it. + const int buffer_size = 1024 * 1024; + vector<uint8_t> buf(buffer_size); + GenerateRandomData(buf.data(), buffer_size); + + AuthenticationHash auth_hash; + uint8_t signature[AuthenticationHash::HashLen()]; + ASSERT_OK(auth_hash.Compute(buf.data(), buffer_size, signature)); + + // Should verify with the same data + EXPECT_TRUE(auth_hash.Verify(buf.data(), buffer_size, signature)); + + // Should not verify with different data + buf[0] ^= 0xFF; + EXPECT_FALSE(auth_hash.Verify(buf.data(), buffer_size, signature)); +} + +TEST_F(OpenSSLUtilTest, AuthenticationHashFromFile) { + // Create a temporary file with a random key. + KeyFile key = MakeKeyFile(); + + // Create an AuthenticationHashFromFile and verify we can Compute a signature and Verify + // it. + const int buffer_size = 1024 * 1024; + vector<uint8_t> buf(buffer_size); + GenerateRandomData(buf.data(), buffer_size); + + // On exit the destructor will halt and join with the reload thread. + AuthenticationHashFromFile auth_hash(key.path); + ASSERT_OK(auth_hash.Init()); + uint8_t signature[AuthenticationHash::HashLen()]; + ASSERT_OK(auth_hash.Compute(buf.data(), buffer_size, signature)); + + // Should verify with the same data + EXPECT_TRUE(auth_hash.Verify(buf.data(), buffer_size, signature)); + + // Overwrite the temporary file with a bad value. + WriteRandomKeyToFile(key.path, 1); // Too short + // Wait for the reload thread to pick up the new key. + SleepForMs(500); + // Should still verify with the original signature, since the key is cached. + EXPECT_TRUE(auth_hash.Verify(buf.data(), buffer_size, signature)); + // Note that a single read error isn't fatal. + + // Now overwrite the file with a new valid key. + WriteRandomKeyToFile(key.path, AuthenticationHash::HashLen()); + // Retry for a few seconds until the reload thread picks up the new key. + const int max_retries = 20; + int retries = 0; + while (retries++ < max_retries) { + SleepForMs(100); + if (!auth_hash.Verify(buf.data(), buffer_size, signature)) break; + } + ASSERT_LT(retries, max_retries) << "Timed out waiting for key reload"; + + // Should not verify with the original signature, since the key has changed. + EXPECT_FALSE(auth_hash.Verify(buf.data(), buffer_size, signature)); +} + +TEST_F(OpenSSLUtilTest, MissingAuthenticationHashFromFile) { + // Try to create an AuthenticationHashFromFile with a non-existent key file. + string missing_key_path = Substitute("/tmp/auth_key_missing_$0", getpid()); + // Ensure the file does not exist. + unlink(missing_key_path.c_str()); + AuthenticationHashFromFile missing_hash(missing_key_path); + ASSERT_ERROR_MSG(missing_hash.Init(), + Substitute("Unable to load authentication hash from $0: " + "No such file or directory", missing_key_path)); +} + +TEST_F(OpenSSLUtilTest, ShortAuthenticationHashFromFile) { + // Create a temporary file with a key that's too short. + KeyFile short_key = MakeKeyFile(AuthenticationHash::HashLen()-1); + AuthenticationHashFromFile short_hash(short_key.path); + ASSERT_ERROR_MSG(short_hash.Init(), + Substitute("Authentication hash from $0 did not contain 32 bytes", short_key.path)); +} + +TEST_F(OpenSSLUtilTest, EmptyAuthenticationHashFromFile) { + // Create a temporary file with an empty key. + KeyFile empty_key{Substitute("/tmp/auth_key_$0", getpid())}; + { + std::ofstream key_file(empty_key.path, std::ios::binary); + } + AuthenticationHashFromFile empty_hash(empty_key.path); + ASSERT_ERROR_MSG(empty_hash.Init(), + Substitute("Authentication hash from $0 did not contain 32 bytes", empty_key.path)); +} + +TEST_F(OpenSSLUtilTest, AuthenticationHashFatal) { + // Create a temporary file with a random key. + KeyFile key = MakeKeyFile(); + + // Initialize the hash, then remove the key file. + FLAGS_hash_reload_grace_period_s = 1; // Shorten grace period for test. + AuthenticationHashFromFile auth_hash(key.path); + [[maybe_unused]] auto runReloadThreadAndWait = [&auth_hash, &key]() { + ASSERT_OK(auth_hash.Init()); + chmod(key.path.c_str(), 0); // Remove all permissions to create an unreadable file. + SleepForMs(2500); + }; + IMPALA_ASSERT_DEBUG_DEATH(runReloadThreadAndWait(), ""); +} + } // namespace impala diff --git a/be/src/util/openssl-util.cc b/be/src/util/openssl-util.cc index 80d9f23c5..4f1a9227d 100644 --- a/be/src/util/openssl-util.cc +++ b/be/src/util/openssl-util.cc @@ -21,8 +21,10 @@ #include <string_view> #include <limits.h> +#include <mutex> #include <sstream> #include <iostream> +#include <sys/inotify.h> #include <glog/logging.h> #include <openssl/bio.h> @@ -43,12 +45,17 @@ #include "common/names.h" #include "cpu-info.h" #include "kudu/util/openssl_util.h" +#include "util/time.h" DECLARE_string(ssl_client_ca_certificate); DECLARE_string(ssl_server_certificate); DECLARE_string(ssl_private_key); DECLARE_string(ssl_cipher_list); +DEFINE_int32(hash_reload_grace_period_s, 300, "The time in seconds to allow " + "authentication hash reload to fail before terminating Impala. A negative value " + "indicates to never terminate Impala."); + /// OpenSSL 1.0.1d #define OPENSSL_VERSION_1_0_1D 0x1000104fL @@ -268,6 +275,126 @@ bool AuthenticationHash::Verify( return memcmp(signature, out, HashLen()) == 0; } +AuthenticationHashFromFile::~AuthenticationHashFromFile() { + // Signal the reload thread to stop. + if (stop_pipe_write_fd_ >= 0) { + write(stop_pipe_write_fd_, "x", 1); // Write a byte to signal shutdown. + close(stop_pipe_write_fd_); + } + if (reload_thread_.joinable()) { + reload_thread_.join(); + } +} + +Status AuthenticationHashFromFile::Init() { + RETURN_IF_ERROR(LoadKey()); + + // Start a thread to handle inotify-triggered reload. + int pipefd[2]; + if (pipe(pipefd) != 0) { + return Status(Substitute("Failed to create pipe for reload thread: $0", + strerror(errno))); + } + stop_pipe_read_fd_ = pipefd[0]; + stop_pipe_write_fd_ = pipefd[1]; + + int fd = inotify_init1(IN_NONBLOCK); + if (fd < 0) { + close(stop_pipe_read_fd_); + return Status( + Substitute("Failed to initialize inotify for $0: $1", path_, strerror(errno))); + } + + int wd = inotify_add_watch( + fd, path_.c_str(), IN_MODIFY | IN_CLOSE_WRITE | IN_MOVE_SELF | IN_ATTRIB); + if (wd < 0) { + close(stop_pipe_read_fd_); + close(fd); + return Status( + Substitute("Failed to add inotify watch for $0: $1", path_, strerror(errno))); + } + + reload_thread_ = std::thread([this, fd, wd] { ReloadMonitor(fd, wd); }); + return Status::OK(); +} + +Status AuthenticationHashFromFile::Compute( + const uint8_t* data, int64_t len, uint8_t* out) const { + DCHECK(reload_thread_.joinable()) << "Compute() called before Init()"; + // Ensure key_ isn't updated during HMAC calculation. + std::shared_lock<std::shared_mutex> l(key_lock_); + return AuthenticationHash::Compute(data, len, out); +} + +Status AuthenticationHashFromFile::LoadKey() { + FILE* hashfile = fopen(path_.c_str(), "rb"); + if (!hashfile) { + return Status(Substitute("Unable to load authentication hash from $0: $1", + path_, strerror(errno))); + } + + uint8_t new_key[SHA256_DIGEST_LENGTH]; + size_t n = fread(new_key, sizeof(uint8_t), SHA256_DIGEST_LENGTH, hashfile); + fclose(hashfile); + if (n != SHA256_DIGEST_LENGTH) { + return Status(Substitute("Authentication hash from $0 did not contain $1 bytes", + path_, SHA256_DIGEST_LENGTH)); + } + + LOG(INFO) << "Loaded authentication key from " << path_; + std::unique_lock<std::shared_mutex> l(key_lock_); + memcpy(key_, new_key, SHA256_DIGEST_LENGTH); + return Status::OK(); +} + +void AuthenticationHashFromFile::ReloadMonitor(int fd, int wd) { + LOG(INFO) << "Started reload thread for authentication key at " << path_; + char buf[sizeof(struct inotify_event) + PATH_MAX + 1] + __attribute__((aligned(__alignof__(struct inotify_event)))); + while (true) { + fd_set readfds; + FD_ZERO(&readfds); + FD_SET(fd, &readfds); + FD_SET(stop_pipe_read_fd_, &readfds); + int maxfd = std::max(fd, stop_pipe_read_fd_) + 1; + int ret = select(maxfd, &readfds, nullptr, nullptr, nullptr); + if (ret < 0) { + if (errno == EINTR) continue; + LOG(FATAL) << "select() failed in reload thread for authentication key at " + << path_ << ": " << strerror(errno); + } + if (FD_ISSET(stop_pipe_read_fd_, &readfds)) { + // Stop signal received. + LOG(INFO) << "Stopping reload thread for authentication key at " << path_; + break; + } + + if (FD_ISSET(fd, &readfds)) { + ssize_t len = read(fd, buf, sizeof(buf)); + if (len > 0) { + // Reload key on any event. + int64_t start = MonotonicSeconds(); + Status stat; + while (!(stat = LoadKey()).ok()) { + if (FLAGS_hash_reload_grace_period_s >= 0 && + MonotonicSeconds() >= start + FLAGS_hash_reload_grace_period_s) { + LOG(FATAL) << "Authentication key reload failed for more than " + << FLAGS_hash_reload_grace_period_s + << " seconds; terminating Impala. Last error: " << stat; + } + LOG(ERROR) << stat; + SleepForMs(1000); + } + } else if (len < 0 && errno != EAGAIN) { + LOG(FATAL) << "Failed to read inotify event: " << strerror(errno); + } + } + } + inotify_rm_watch(fd, wd); + close(stop_pipe_read_fd_); + close(fd); +} + int GetKeyLenForMode(AES_CIPHER_MODE m) { switch (m) { case AES_CIPHER_MODE::AES_128_ECB: diff --git a/be/src/util/openssl-util.h b/be/src/util/openssl-util.h index b64ef8281..189262f80 100644 --- a/be/src/util/openssl-util.h +++ b/be/src/util/openssl-util.h @@ -17,7 +17,9 @@ #pragma once +#include <shared_mutex> #include <string_view> +#include <thread> #include <openssl/aes.h> #include <openssl/crypto.h> @@ -129,26 +131,65 @@ class IntegrityHash { /// authentication, eg. checking signatures of cookies. A SHA256 hash is used internally. class AuthenticationHash { public: + /// Generates a random key_ for authentication. AuthenticationHash(); + /// Default destructor for override in derived classes. + virtual ~AuthenticationHash() = default; + + virtual Status Init() { return Status::OK(); } + /// Computes the HMAC of 'data', which has length 'len', and stores it in 'out', which /// must already be allocated with a length of HashLen() bytes. Returns an error if /// computing the hash was unsuccessful. - Status Compute(const uint8_t* data, int64_t len, uint8_t* out) const WARN_UNUSED_RESULT; + virtual Status Compute(const uint8_t* data, int64_t len, uint8_t* out) + const WARN_UNUSED_RESULT; /// Computes the HMAC of 'data', which has length 'len', and returns true if it matches /// 'signature', which is expected to have length HashLen(). - bool Verify(const uint8_t* data, int64_t len, - const uint8_t* signature) const WARN_UNUSED_RESULT; + bool Verify(const uint8_t* data, int64_t len, const uint8_t* signature) + const WARN_UNUSED_RESULT; /// Returns the length in bytes of the generated hashes. Currently we always use SHA256. static int HashLen() { return SHA256_DIGEST_LENGTH; } - private: + protected: /// An AES 256-bit key. uint8_t key_[SHA256_DIGEST_LENGTH]; }; +class AuthenticationHashFromFile : public AuthenticationHash { + public: + /// Initializes key_ from a file. + AuthenticationHashFromFile(std::string path) : path_(std::move(path)) {} + + /// Destructor to shut down the reload thread and signal it to stop. + ~AuthenticationHashFromFile() override; + + Status Init() override; + + Status Compute(const uint8_t* data, int64_t len, uint8_t* out) + const override WARN_UNUSED_RESULT; + + private: + Status LoadKey(); + + // Thread function to monitor file changes and reload key_. + void ReloadMonitor(int fd, int wd); + + // Path to read and watch for updates. + std::string path_; + + // Write pipe to signal the reload thread to stop. + int stop_pipe_read_fd_{-1}, stop_pipe_write_fd_{-1}; + + // Thread to reload key_ from path_ on file changes. + std::thread reload_thread_; + + // Mutex guarding updates to key_. Allow holding lock in read-only const functions. + mutable std::shared_mutex key_lock_; +}; + /// The key and initialization vector (IV) required to encrypt and decrypt a buffer of /// data. This should be regenerated for each buffer of data. /// diff --git a/be/src/util/webserver-test.cc b/be/src/util/webserver-test.cc index c9c308146..e249459ab 100644 --- a/be/src/util/webserver-test.cc +++ b/be/src/util/webserver-test.cc @@ -599,15 +599,6 @@ TEST(Webserver, TestGetWithSpnego) { CheckAuthMetrics(&metrics, 1, (curl_7_64_or_above ? 1 : 2), 1, 0); // Validate authentication mechanism stored in the cookie ASSERT_EQ(cookie.auth_mech(), "SPNEGO"); - - webserver.Stop(); - MetricGroup metrics2("webserver-test"); - Webserver webserver2("", FLAGS_webserver_port, &metrics2); - ASSERT_OK(webserver2.Start()); - // Run the command again. We should get a failed cookie attempt because the new - // webserver uses a different HMAC key. See above note about curl 7.64.0 or above. - system(curl_cmd.c_str()); - CheckAuthMetrics(&metrics2, 1, (curl_7_64_or_above ? 0 : 1), 0, 1); } else { cout << "Skipping test, curl was not present or did not have the required " << "features: " << curl_output << endl; diff --git a/be/src/util/webserver.cc b/be/src/util/webserver.cc index 4cd21f2dd..3415cf100 100644 --- a/be/src/util/webserver.cc +++ b/be/src/util/webserver.cc @@ -159,6 +159,7 @@ DECLARE_bool(enable_ldap_auth); DECLARE_string(hostname); DECLARE_bool(is_coordinator); DECLARE_int64(max_cookie_lifetime_s); +DECLARE_string(cookie_secret_file); DECLARE_string(ssl_minimum_version); DECLARE_string(ssl_cipher_list); DECLARE_string(tls_ciphersuites); @@ -819,9 +820,9 @@ sq_callback_result_t Webserver::BeginRequestCallback(struct sq_connection* conne string username; string auth_mech; if (cookie_header != nullptr) { - Status cookie_status = - AuthenticateCookie(hash_, cookie_header, &username, &auth_mech, - &cookie_rand_value); + const AuthenticationHash& hash = AuthManager::GetInstance()->GetAuthHash(); + Status cookie_status = AuthenticateCookie( + hash, cookie_header, &username, &auth_mech, &cookie_rand_value); if (cookie_status.ok()) { authenticated = true; cookie_authenticated = true; @@ -1253,8 +1254,9 @@ void Webserver::AddCookie(const char* user, vector<string>* response_headers, response_headers->erase(it); } // Generate a cookie to return. + const AuthenticationHash& hash = AuthManager::GetInstance()->GetAuthHash(); response_headers->push_back(Substitute("Set-Cookie: $0", - GenerateCookie(user, hash_, authMech, cookie_rand_value))); + GenerateCookie(user, hash, authMech, cookie_rand_value))); } } diff --git a/be/src/util/webserver.h b/be/src/util/webserver.h index df92d6132..b7a528e22 100644 --- a/be/src/util/webserver.h +++ b/be/src/util/webserver.h @@ -285,9 +285,6 @@ class Webserver { /// Catch-all handler for error messages UrlHandler error_handler_; - /// Used to generate and verify signatures for cookies. - AuthenticationHash hash_; - /// If true and SPNEGO is in use, cookies will be used for authentication. bool use_cookies_; diff --git a/docs/topics/impala_client.xml b/docs/topics/impala_client.xml index 4c8ee6e2d..da78c9ba8 100644 --- a/docs/topics/impala_client.xml +++ b/docs/topics/impala_client.xml @@ -308,12 +308,40 @@ under the License. <p> Each <codeph>impalad</codeph> uses its own key to generate the signature, so clients that reconnect to a different <codeph>impalad</codeph> have to - re-authenticate. + re-authenticate. On a single <codeph>impalad</codeph>, cookies are valid + across sessions and connections. See <codeph>--cookie_secret_file</codeph> + to configure a consistent secret across <codeph>impalad</codeph> instances. </p> + </dd> + + </dlentry> + <dlentry> + + <dt> + --cookie_secret_file + </dt> + + <dd> + Starting in Impala 5.0.0, the secret used to construct an HMAC for cookie + verification for both HS2-HTTP clients and the Impala Web User Interface can + be shared across instances and service restart rather than being generated on + startup. Use the <codeph>--cookie_secret_file</codeph> startup flag to provide + the path to a file containing a 256-bit (32-byte) secret in binary format. + <p> + The default value of an empty string indicates that Impala should generate a + random secret on startup and store it in memory. + </p> <p> - On a single <codeph>impalad</codeph>, cookies are valid across sessions and - connections. + If a file path is provided, Impala reads the secret from the file on + startup. It then starts a thread to monitor the file for changes and reloads + the secret if the file is modified. Any errors in the monitor thread will + be logged as an error; after 300 seconds of failure to reload the secret, + the error will become fatal and Impala will exit to avoid incorrectly + authenticated access. The 300 second grace period can be changed using the + <codeph>--hash_reload_grace_period_s</codeph> flag, or disabled with a + negative value. Successful reloads will log an informational message + "Loaded authentication key from <file_path>". </p> </dd> diff --git a/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java b/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java index eb76f9c75..7b2b26235 100644 --- a/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java +++ b/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java @@ -18,7 +18,10 @@ package org.apache.impala.customcluster; import static org.apache.impala.testutil.LdapUtil.*; +import static org.apache.impala.testutil.TestUtils.retryUntilSuccess; +import static org.apache.impala.testutil.TestUtils.writeCookieSecret; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -35,12 +38,19 @@ import org.apache.directory.server.annotations.CreateTransport; import org.apache.directory.server.core.annotations.ApplyLdifFiles; import org.apache.directory.server.core.integ.CreateLdapServerRule; import org.apache.hive.service.rpc.thrift.*; +import org.apache.http.HttpResponse; +import org.apache.http.HttpResponseInterceptor; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClients; +import org.apache.http.protocol.HttpContext; import org.apache.impala.testutil.WebClient; import org.apache.thrift.transport.THttpClient; import org.apache.thrift.protocol.TBinaryProtocol; import org.junit.After; import org.junit.ClassRule; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TemporaryFolder; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -59,6 +69,11 @@ public class LdapHS2Test { @ClassRule public static CreateLdapServerRule serverRule = new CreateLdapServerRule(); + // Temp folder where the config files are copied so we can modify them in place. + // The JUnit @Rule creates and removes the temp folder between every test. + @Rule + public TemporaryFolder tempFolder = new TemporaryFolder(); + WebClient client_ = new WebClient(); public void setUp(String extraArgs) throws Exception { @@ -958,4 +973,100 @@ public class LdapHS2Test { // Two successful authentications for each ExecAndFetch(). verifyMetrics(23, 0); } + + /** + * Tests that cookie can be shared across multiple coordinators. + */ + @Test + public void testHiveserver2SharedCookie() throws Exception { + // Write a temporary key file for the cookie secret. + File keyFile = tempFolder.newFile(); + writeCookieSecret(keyFile); + + setUp(String.format("--cookie_secret_file=%s", keyFile.getCanonicalPath())); + verifyMetrics(0, 0); + + // Use HttpClient so we can access the returned cookie. Final array is used + // so we can store a cookie from an anonymous function. + final String[] cookie = new String[1]; + HttpResponseInterceptor cookieSaver = new HttpResponseInterceptor() { + @Override public void process(HttpResponse response, HttpContext context) { + if (response.containsHeader("Set-Cookie")) { + cookie[0] = response.getFirstHeader("Set-Cookie").getValue(); + } else { + assertNotNull("First response is expected to Set-Cookie", cookie[0]); + } + } + }; + + try (CloseableHttpClient clientImpl = + HttpClients.custom().addInterceptorFirst(cookieSaver).build(); + THttpClient transport = new THttpClient("http://localhost:28000";, clientImpl)) { + Map<String, String> headers = new HashMap<String, String>(); + // Authenticate as 'Test1Ldap' with password '12345' + headers.put("Authorization", "Basic VGVzdDFMZGFwOjEyMzQ1"); + transport.setCustomHeaders(headers); + transport.open(); + TCLIService.Iface client = new TCLIService.Client(new TBinaryProtocol(transport)); + + // Open a session which will get username 'Test1Ldap'. + TOpenSessionReq openReq = new TOpenSessionReq(); + TOpenSessionResp openResp = client.OpenSession(openReq); + // One successful authentication. + verifyMetrics(1, 0); + + // Running a query with only the cookie should succeed. + headers.remove("Authorization"); + headers.put("Cookie", cookie[0]); + transport.setCustomHeaders(headers); + execAndFetch( + client, openResp.getSessionHandle(), "select logged_in_user()", "Test1Ldap"); + // Two successful authentications - for the Exec() and the Fetch(). + verifyCookieMetrics(2, 0); + } + + // Cookie should be re-usable with another server. + try (WebClient webClient = new WebClient(25001); + THttpClient transport = new THttpClient("http://localhost:28001";)) { + transport.setCustomHeader("Cookie", cookie[0]); + transport.open(); + TCLIService.Iface client = new TCLIService.Client(new TBinaryProtocol(transport)); + + // Open a session which will get username 'Test1Ldap'. + TOpenSessionReq openReq = new TOpenSessionReq(); + TOpenSessionResp openResp = client.OpenSession(openReq); + // One successful cookie authentication. + long actualCookieAuthSuccess = (long) webClient.getMetric( + "impala.thrift-server.hiveserver2-http-frontend.total-cookie-auth-success"); + assertEquals(1, actualCookieAuthSuccess); + + execAndFetch( + client, openResp.getSessionHandle(), "select logged_in_user()", "Test1Ldap"); + // Two more successful authentications - for the Exec() and the Fetch(). + actualCookieAuthSuccess = (long) webClient.getMetric( + "impala.thrift-server.hiveserver2-http-frontend.total-cookie-auth-success"); + assertEquals(3, actualCookieAuthSuccess); + } + + // Cookie should be reloaded with the new key. + writeCookieSecret(keyFile); + String respMessage = retryUntilSuccess(() -> { + try (THttpClient transport = new THttpClient("http://localhost:28000";)) { + transport.setCustomHeader("Cookie", cookie[0]); + transport.open(); + TCLIService.Iface client = new TCLIService.Client(new TBinaryProtocol(transport)); + + // Open a session which will get username 'Test1Ldap'. Failure is expected since + // the cookie_secret_file has changed. + TOpenSessionReq openReq = new TOpenSessionReq(); + try { + client.OpenSession(openReq); + throw new Exception("Expected failure due to changed cookie secret."); + } catch (Exception e) { + return e.getMessage(); + } + } + }, 20, 100); + assertEquals(respMessage, "HTTP Response code: 401"); + } } diff --git a/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java b/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java index d1e96b270..c7bfe8ce6 100644 --- a/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java +++ b/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java @@ -18,12 +18,15 @@ package org.apache.impala.customcluster; import static org.apache.impala.testutil.LdapUtil.*; +import static org.apache.impala.testutil.TestUtils.retryUntilSuccess; +import static org.apache.impala.testutil.TestUtils.writeCookieSecret; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import com.google.common.collect.Range; +import java.io.File; import java.io.IOException; import java.util.Arrays; import java.util.ArrayList; @@ -37,6 +40,8 @@ import org.apache.impala.testutil.WebClient; import org.junit.After; import org.junit.Assume; import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.rules.TemporaryFolder; /** * Impala shell connectivity tests for LDAP authentication. This class contains the common @@ -49,6 +54,9 @@ public class LdapImpalaShellTest { @ClassRule public static CreateLdapServerRule serverRule = new CreateLdapServerRule(); + @Rule + public TemporaryFolder tempFolder = new TemporaryFolder(); + // The cluster will be set up to allow TEST_USER_1 to act as a proxy for delegateUser_. // Includes a special character to test HTTP path encoding. protected static final String delegateUser_ = "proxyUser$"; @@ -282,6 +290,61 @@ public class LdapImpalaShellTest { RunShellCommand.Run(command, /* shouldSucceed */ true, delegateUser_, ""); } + /** + * Used to configure and return a temporary cookie secret file. This is needed to set + * --cookie_secret_file=file.getCanonicalPath() and pass to testCookieRefreshImpl. + */ + protected File getCookieSecretFile() throws Exception { + // Write a temporary key file for the cookie secret. + File keyFile = tempFolder.newFile(); + writeCookieSecret(keyFile); + return keyFile; + } + + /** + * Tests cookie rotation during a query does not interrupt the session. + */ + protected void testCookieRefreshImpl(File keyFile) throws Exception { + String query = "select sleep(3000)"; + String[] command = + buildCommand(query, "hs2-http", TEST_USER_1, TEST_PASSWORD_1, "/cliservice"); + final RunShellCommand.Output[] resultHolder = new RunShellCommand.Output[1]; + Thread thread = new Thread(() -> { + try { + resultHolder[0] = RunShellCommand.Run(command, + /* shouldSucceed */ true, /* sleep returns true */ "true", + "Starting Impala Shell with LDAP-based authentication"); + } catch (Throwable e) { + resultHolder[0] = new RunShellCommand.Output("", e.getMessage()); + } + }); + thread.start(); + + // Poll the metric until authentication succeeds, then update the cookie secret file. + // Success can be seen from timing: client authenticates, key is updated, client + // continues and finishes the query. The relevant log lines are: + // Loaded authentication key from ... + // Opened session + // Loaded authentication key from ... + // Invalid cookie provided + // Closed session + retryUntilSuccess(() -> { + long success = (long) client_.getMetric( + "impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-success"); + if (success < 1L) throw new Exception("Authentication not yet succeeded."); + return null; + }, 20, 100); + + writeCookieSecret(keyFile); + thread.join(); + RunShellCommand.Output result = resultHolder[0]; + assertTrue(result.stderr, + result.stderr.contains("Preserving cookies: impala.auth")); + // Cookie auth should fail once due to key change, requiring two basic auths. + // Cookie auth is expected to succeed at least once, possibly many times. + verifyMetrics(Range.closed(2L, 2L), zero, Range.atLeast(1L), one); + } + /** * Tests the LDAP user and group filter configs. */ diff --git a/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java b/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java index 29389cf2b..6e9e0992f 100644 --- a/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java +++ b/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java @@ -30,6 +30,7 @@ import org.junit.rules.TemporaryFolder; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; +import java.io.File; import java.io.IOException; /** @@ -113,6 +114,18 @@ public class LdapSearchBindImpalaShellTest extends LdapImpalaShellTest { testHttpImpersonationImpl(); } + /** + * Tests cookie rotation during a query does not interrupt the session. + */ + @Test + public void testCookieRefresh() throws Exception { + File cookieSecretFile = getCookieSecretFile(); + setUp(String.format("--ldap_user_search_basedn=dc=myorg,dc=com " + + "--ldap_user_filter=(&(objectClass=person)(cn={0})) " + + "--cookie_secret_file=%s", cookieSecretFile.getCanonicalPath())); + testCookieRefreshImpl(cookieSecretFile); + } + /** * Tests the LDAP user and group filter configs. */ diff --git a/fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java b/fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java index e165a3ef9..39a4780fb 100644 --- a/fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java +++ b/fe/src/test/java/org/apache/impala/customcluster/LdapSimpleBindImpalaShellTest.java @@ -30,6 +30,7 @@ import org.junit.rules.TemporaryFolder; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; +import java.io.File; import java.io.IOException; /** @@ -109,6 +110,16 @@ public class LdapSimpleBindImpalaShellTest extends LdapImpalaShellTest { testHttpImpersonationImpl(); } + /** + * Tests cookie rotation during a query does not interrupt the session. + */ + @Test + public void testCookieRefresh() throws Exception { + File cookieSecretFile = getCookieSecretFile(); + setUp(String.format("--cookie_secret_file=%s", cookieSecretFile.getCanonicalPath())); + testCookieRefreshImpl(cookieSecretFile); + } + /** * Tests the LDAP user and group filter configs. */ diff --git a/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java b/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java index 7850c254e..fdcd9fa5d 100644 --- a/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java +++ b/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java @@ -18,6 +18,8 @@ package org.apache.impala.customcluster; import static org.apache.impala.testutil.LdapUtil.*; +import static org.apache.impala.testutil.TestUtils.retryUntilSuccess; +import static org.apache.impala.testutil.TestUtils.writeCookieSecret; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -53,7 +55,9 @@ import org.apache.thrift.transport.THttpClient; import org.json.simple.JSONObject; import org.junit.After; import org.junit.ClassRule; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TemporaryFolder; @CreateDS(name = "myDS", partitions = { @CreatePartition(name = "test", suffix = "dc=myorg,dc=com") }) @@ -66,6 +70,11 @@ public class LdapWebserverTest { private static final Range<Long> zero = Range.closed(0L, 0L); + // Temp folder where the config files are copied so we can modify them in place. + // The JUnit @Rule creates and removes the temp folder between every test. + @Rule + public TemporaryFolder tempFolder = new TemporaryFolder(); + WebClient client_ = new WebClient(TEST_USER_1, TEST_PASSWORD_1); public void setUp(String extraArgs, String startArgs, String catalogdArgs, @@ -166,6 +175,47 @@ public class LdapWebserverTest { } } + @Test + public void testWebserverSharedCookie() throws Exception { + // Write a temporary key file for the cookie secret. + File keyFile = tempFolder.newFile(); + writeCookieSecret(keyFile); + + setUp(String.format("--cookie_secret_file=%s", keyFile.getCanonicalPath()), + "", "", "", ""); + // start-impala-cluster contacts the webui to confirm the impalads have started, so + // there will already be some successful auth attempts. + verifyMetrics(Range.atLeast(1L), zero, Range.atLeast(1L), zero); + + // Attempt to access the webserver without a username/password, only the cookie from + // the verifyMetrics calls. + try (WebClient cookieOnly = new WebClient("", "", 25000, client_.getCookies())) { + String result = cookieOnly.readContent("/"); + assertTrue(result, result.contains("<title>Apache Impala</title>")); + // Check that there were no unsuccessful auth attempts. + verifyMetrics(Range.atLeast(1L), zero, Range.atLeast(1L), zero); + } + + // Access a different webserver with the same cookie, should succeed. + try (WebClient cookieOnly = new WebClient("", "", 25001, client_.getCookies())) { + String result = cookieOnly.readContent("/"); + assertTrue(result, result.contains("<title>Apache Impala</title>")); + + // Cookie should be reloaded with the new key. + writeCookieSecret(keyFile); + result = retryUntilSuccess(() -> { + String res = cookieOnly.readContent("/"); + if (res.contains("<title>Apache Impala</title>")) { + throw new Exception("Cookie should be invalid after key change."); + } + return res; + }, 20, 100); + assertTrue(result, result.contains("Must authenticate with Basic authentication.")); + // Check that there is an unsuccessful cookie attempt. + verifyMetrics(Range.atLeast(1L), zero, Range.atLeast(1L), Range.closed(1L, 1L)); + } + } + /** * Tests the webserver specific LDAP user and group filter configs. */ diff --git a/fe/src/test/java/org/apache/impala/testutil/TestUtils.java b/fe/src/test/java/org/apache/impala/testutil/TestUtils.java index 124ad9571..8d0714020 100644 --- a/fe/src/test/java/org/apache/impala/testutil/TestUtils.java +++ b/fe/src/test/java/org/apache/impala/testutil/TestUtils.java @@ -19,6 +19,8 @@ package org.apache.impala.testutil; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Preconditions; +import java.io.File; +import java.io.FileOutputStream; import java.io.IOException; import java.io.StringReader; import java.io.StringWriter; @@ -35,6 +37,8 @@ import java.util.Map; import java.util.Scanner; import java.util.TimeZone; import java.util.regex.Pattern; +import java.util.concurrent.Callable; +import java.util.concurrent.ThreadLocalRandom; import javax.json.Json; import javax.json.JsonObject; @@ -511,4 +515,31 @@ public class TestUtils { Preconditions.checkState(result >= 8); return result; } + + public static void writeCookieSecret(File keyFile) throws IOException { + byte[] key = new byte[32]; + // Used for testing, so prefer performance over strong randomness. + ThreadLocalRandom.current().nextBytes(key); + try (FileOutputStream keyWriter = new FileOutputStream(keyFile)) { + keyWriter.write(key); + } + } + + public static <T> T retryUntilSuccess(Callable<T> callable, int maxAttempts, + long waitMillis) throws InterruptedException { + int attempts = 0; + while (true) { + try { + return callable.call(); + } catch (Exception e) { + attempts++; + if (attempts >= maxAttempts) { + throw new RuntimeException( + String.format("Failed after %d attempts", attempts), e); + } + LOG.warn("Attempt {} failed: {}", attempts, e.getMessage()); + Thread.sleep(waitMillis); + } + } + } } diff --git a/fe/src/test/java/org/apache/impala/testutil/WebClient.java b/fe/src/test/java/org/apache/impala/testutil/WebClient.java index 8f71acae4..3e5e122ab 100644 --- a/fe/src/test/java/org/apache/impala/testutil/WebClient.java +++ b/fe/src/test/java/org/apache/impala/testutil/WebClient.java @@ -74,6 +74,13 @@ public class WebClient implements AutoCloseable { cookieStore_ = new BasicCookieStore(); } + public WebClient(String username, String password, int port, List<Cookie> cookies) { + this(username, password, port); + for (Cookie cookie: cookies) { + cookieStore_.addCookie(cookie); + } + } + public void close() throws IOException { httpClient_.close(); } public List<Cookie> getCookies() { return cookieStore_.getCookies(); }
