impala-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sail...@apache.org
Subject [1/4] incubator-impala git commit: IMPALA-5041: Allow AuthManager::Init() to be called more than once
Date Sat, 18 Mar 2017 00:28:50 GMT
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 <sailesh@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/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 <sailesh@cloudera.com>
Authored: Wed Mar 8 13:38:23 2017 -0800
Committer: Impala Public Jenkins <impala-public-jenkins@gerrit.cloudera.org>
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


Mime
View raw message