Repository: incubator-impala Updated Branches: refs/heads/master 3aeb2aeb5 -> b41c2114f
IMPALA-5041: Allow AuthManager::Init() to be called more than once We use a global variable to track the completion of the kerberos environment setup which shouldn't be the case since we would want to re-setup the environment if we call AuthManager::Init() with a different configuration. Currently, if we call it again, some of our new configuration get applied, whereas some don't. Also, the global variable 'env_setup_complete_' was used so that we don't set the environment twice (once for internal transport and once for external). This patch ensures that it's still the case. Change-Id: I12cc210aa422f077446ea728ebf76921351417b0 Reviewed-on: http://gerrit.cloudera.org:8080/6333 Reviewed-by: Sailesh Mukil <[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/69510304 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/69510304 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/69510304 Branch: refs/heads/master Commit: 69510304f52c062dcbb291e37e96831ad0df6264 Parents: 3aeb2ae Author: Sailesh Mukil <[email protected]> Authored: Wed Mar 8 13:38:23 2017 -0800 Committer: Impala Public Jenkins <[email protected]> Committed: Fri Mar 17 10:14:24 2017 +0000 ---------------------------------------------------------------------- be/src/rpc/auth-provider.h | 3 --- be/src/rpc/authentication.cc | 19 +++++++------------ be/src/rpc/authentication.h | 7 +++++++ 3 files changed, 14 insertions(+), 15 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/69510304/be/src/rpc/auth-provider.h ---------------------------------------------------------------------- diff --git a/be/src/rpc/auth-provider.h b/be/src/rpc/auth-provider.h index 1a9ed7b..2b5b747 100644 --- a/be/src/rpc/auth-provider.h +++ b/be/src/rpc/auth-provider.h @@ -150,9 +150,6 @@ class SaslAuthProvider : public AuthProvider { /// Additionally, if the first attempt fails, this method will return. void RunKinit(Promise<Status>* first_kinit); - /// We use this to ensure that we only set up environment variables one time. - static bool env_setup_complete_; - /// One-time kerberos-specific environment variable setup. Called by InitKerberos(). Status InitKerberosEnv(); }; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/69510304/be/src/rpc/authentication.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc index a42a1a4..765ab5d 100644 --- a/be/src/rpc/authentication.cc +++ b/be/src/rpc/authentication.cc @@ -134,7 +134,6 @@ static const string LDAPS_URI_PREFIX = "ldaps://"; // to log messages about the start of authentication. This is that plugin's name. static const string IMPALA_AUXPROP_PLUGIN = "impala-auxprop"; -bool SaslAuthProvider::env_setup_complete_ = false; AuthManager* AuthManager::auth_manager_ = new AuthManager(); // This Sasl callback is called when the underlying cyrus-sasl layer has @@ -720,9 +719,6 @@ Status SaslAuthProvider::InitKerberos(const string& principal, hostname_ = names[1]; realm_ = names[2]; - RETURN_IF_ERROR(CheckReplayCacheDirPermissions()); - RETURN_IF_ERROR(InitKerberosEnv()); - LOG(INFO) << "Using " << (is_internal_ ? "internal" : "external") << " kerberos principal \"" << service_name_ << "/" << hostname_ << "@" << realm_ << "\""; @@ -763,20 +759,19 @@ static Status EnvAppend(const string& attr, const string& thing, const string& t return Status::OK(); } -Status SaslAuthProvider::InitKerberosEnv() { - DCHECK(!principal_.empty()); +Status AuthManager::InitKerberosEnv() { + DCHECK(!FLAGS_principal.empty()); - // Called only during setup; no locking required. - if (env_setup_complete_) return Status::OK(); + RETURN_IF_ERROR(CheckReplayCacheDirPermissions()); - if (!is_regular(keytab_file_)) { + if (!is_regular(FLAGS_keytab_file)) { return Status(Substitute("Bad --keytab_file value: The file $0 is not a " - "regular file", keytab_file_)); + "regular file", FLAGS_keytab_file)); } // Set the keytab name in the environment so that Sasl Kerberos and kinit can // find and use it. - if (setenv("KRB5_KTNAME", keytab_file_.c_str(), 1)) { + if (setenv("KRB5_KTNAME", FLAGS_keytab_file.c_str(), 1)) { return Status(Substitute("Kerberos could not set KRB5_KTNAME: $0", GetStrErrMsg())); } @@ -829,7 +824,6 @@ Status SaslAuthProvider::InitKerberosEnv() { } } - env_setup_complete_ = true; return Status::OK(); } @@ -1038,6 +1032,7 @@ Status AuthManager::Init() { } else { kerberos_internal_principal = FLAGS_be_principal; } + RETURN_IF_ERROR(InitKerberosEnv()); } // This is written from the perspective of the daemons - thus "internal" // means "I am used for communication with other daemons, both as a client http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/69510304/be/src/rpc/authentication.h ---------------------------------------------------------------------- diff --git a/be/src/rpc/authentication.h b/be/src/rpc/authentication.h index a89d945..5bc3138 100644 --- a/be/src/rpc/authentication.h +++ b/be/src/rpc/authentication.h @@ -39,6 +39,9 @@ namespace impala { /// System-wide authentication manager responsible for initialising authentication systems, /// including SSL, Sasl and Kerberos, and for providing auth-enabled Thrift structures to /// servers and clients. +/// There should only be one AuthManager instantiated at a time. +/// If Init() is called more than once, all the setup state will be overwritten (Currently +/// only used for testing purposes to validate different security configurations) class AuthManager { public: static AuthManager* GetInstance() { return AuthManager::auth_manager_; } @@ -59,6 +62,10 @@ class AuthManager { AuthProvider* GetInternalAuthProvider(); private: + /// One-time kerberos-specific environment variable setup. Called by Init() if Kerberos + /// is enabled. + Status InitKerberosEnv(); + static AuthManager* auth_manager_; /// These are provided for convenience, so that demon<->demon and client<->demon services
