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 <dhe...@cloudera.com> Reviewed-by: Michael Ho <k...@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 <sail...@apache.org> Authored: Mon Dec 4 22:31:52 2017 -0800 Committer: Impala Public Jenkins <impala-public-jenk...@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.