[6/8] impala git commit: IMPALA-6256: Incorrect principal will be used for internal connections if FLAGS_be_principal is set

[ta...@apache.org]

IMPALA-6256: Incorrect principal will be used for internal connections if FLAGS_be_principal is set

In Impala, we have FLAGS_principal and FLAGS_be_principal flags.
If only FLAGS_principal is set, we use it as both the internal
and external principals.

If both FLAGS_principal and FLAGS_be_principal are set, we use
FLAGS_be_principal as the internal principal and FLAGS_principal
as the external principal.

However, in Kudu, they only source the internal principal from
FLAGS_principal and aren't aware of a flag called FLAGS_be_principal.
So as of the time IMPALA-5129 went in, if FLAGS_be_principal is
explicitly set to something different from FLAGS_principal, we would
be using the external principal as the internal principal, which is
incorrect.

This is fixed on the Kudu side by the following commit:
https://github.com/apache/kudu/commit/d42c2916467b83347f064ddea59f7a65202f7247

This is now cherry-picked to Impala and we now use the new API to
fix this problem.

Testing: Made the MiniKdc explicitly set FLAGS_principal and
FLAGS_be_principal. Confirmed that the tests fail without this
patch and with FLAGS_be_principal set.

Change-Id: If5af4398467857da09878075439b6612a04d7a01
Reviewed-on: http://gerrit.cloudera.org:8080/8761
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Reviewed-by: Michael Ho <kw...@cloudera.com>
Tested-by: Impala Public Jenkins


Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/e79f6446
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/e79f6446
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/e79f6446

Branch: refs/heads/master
Commit: e79f6446b1eb9647ecbf61ed6b8fe7e992142de1
Parents: e27b01f
Author: Sailesh Mukil <sa...@apache.org>
Authored: Mon Dec 4 22:31:52 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Tue Dec 5 23:24:08 2017 +0000

----------------------------------------------------------------------
 be/src/rpc/authentication.cc        | 4 ++--
 be/src/testutil/mini-kdc-wrapper.cc | 8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/e79f6446/be/src/rpc/authentication.cc
----------------------------------------------------------------------
diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc
index 151dad0..29fa8f7 100644
--- a/be/src/rpc/authentication.cc
+++ b/be/src/rpc/authentication.cc
@@ -833,8 +833,8 @@ Status SaslAuthProvider::Start() {
     if (FLAGS_use_kudu_kinit) {
       // Starts a thread that periodically does a 'kinit'. The thread lives as long as the
       // process does.
-      KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer(KRB5CCNAME_PATH, false),
-          "Could not init kerberos");
+      KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer(principal_,
+          KRB5CCNAME_PATH, false), "Could not init kerberos");
     } else {
       Promise<Status> first_kinit;
       stringstream thread_name;

http://git-wip-us.apache.org/repos/asf/impala/blob/e79f6446/be/src/testutil/mini-kdc-wrapper.cc
----------------------------------------------------------------------
diff --git a/be/src/testutil/mini-kdc-wrapper.cc b/be/src/testutil/mini-kdc-wrapper.cc
index b4e0f25..d378ea4 100644
--- a/be/src/testutil/mini-kdc-wrapper.cc
+++ b/be/src/testutil/mini-kdc-wrapper.cc
@@ -34,6 +34,7 @@ DECLARE_bool(use_kudu_kinit);
 
 DECLARE_string(keytab_file);
 DECLARE_string(principal);
+DECLARE_string(be_principal);
 DECLARE_string(krb5_conf);
 
 Status MiniKdcWrapper::StartKdc(string keytab_dir) {
@@ -79,7 +80,11 @@ Status MiniKdcWrapper::SetupAndStartMiniKDC(KerberosSwitch k) {
     // Set the appropriate flags based on how we've set up the kerberos environment.
     FLAGS_krb5_conf = strings::Substitute("$0/$1", keytab_dir, "krb5.conf");
     FLAGS_keytab_file = kt_path;
-    FLAGS_principal = strings::Substitute("$0@$1", spn_, realm_);
+
+    // We explicitly set 'principal' and 'be_principal' even though 'principal' won't be
+    // used to test IMPALA-6256.
+    FLAGS_principal = "dummy-service/host@realm";
+    FLAGS_be_principal = strings::Substitute("$0@$1", spn_, realm_);
   }
   return Status::OK();
 }
@@ -91,6 +96,7 @@ Status MiniKdcWrapper::TearDownMiniKDC(KerberosSwitch k) {
     // Clear the flags so we don't step on other tests that may run in the same process.
     FLAGS_keytab_file.clear();
     FLAGS_principal.clear();
+    FLAGS_be_principal.clear();
     FLAGS_krb5_conf.clear();
 
     // Remove test directory.