IMPALA-5893: Remove old kinit code for Impala 3 We've gone through a couple of releases with Kudu's kinit as the default way to use kerberos and we've not come across any major issues.
Since we're going to have a major release soon, it's time to get rid of the old Kinit code that's largely unused for a while now. Testing: Made sure that our current kerberos tests continue to work without the old code. Cherry-picks: not for 2.x Change-Id: Ic78de10f3fb9ec36537de7a090916e4be123234b Reviewed-on: http://gerrit.cloudera.org:8080/9941 Reviewed-by: Sailesh Mukil <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/impala/repo Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/4dc3d340 Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/4dc3d340 Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/4dc3d340 Branch: refs/heads/master Commit: 4dc3d340d866ef9a7ef1f654cbfff402e0bc69cc Parents: 7134d81 Author: Sailesh Mukil <[email protected]> Authored: Thu Apr 5 18:19:17 2018 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Fri Apr 20 21:40:13 2018 +0000 ---------------------------------------------------------------------- be/src/common/global-flags.cc | 2 + be/src/rpc/auth-provider.h | 21 +------- be/src/rpc/authentication.cc | 82 ++---------------------------- be/src/rpc/rpc-mgr-kerberized-test.cc | 6 +-- be/src/rpc/thrift-server-test.cc | 5 +- be/src/testutil/mini-kdc-wrapper.h | 5 +- 6 files changed, 11 insertions(+), 110 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/4dc3d340/be/src/common/global-flags.cc ---------------------------------------------------------------------- diff --git a/be/src/common/global-flags.cc b/be/src/common/global-flags.cc index a15e91b..ea88b28 100644 --- a/be/src/common/global-flags.cc +++ b/be/src/common/global-flags.cc @@ -226,6 +226,7 @@ REMOVED_FLAG(enable_partitioned_aggregation); REMOVED_FLAG(enable_partitioned_hash_join); REMOVED_FLAG(enable_phj_probe_side_filtering); REMOVED_FLAG(enable_rm); +REMOVED_FLAG(kerberos_reinit_interval); REMOVED_FLAG(llama_addresses); REMOVED_FLAG(llama_callback_port); REMOVED_FLAG(llama_host); @@ -246,4 +247,5 @@ REMOVED_FLAG(rpc_cnxn_retry_interval_ms); REMOVED_FLAG(staging_cgroup); REMOVED_FLAG(suppress_unknown_disk_id_warnings); REMOVED_FLAG(use_statestore); +REMOVED_FLAG(use_kudu_kinit); REMOVED_FLAG(disable_admission_control); http://git-wip-us.apache.org/repos/asf/impala/blob/4dc3d340/be/src/rpc/auth-provider.h ---------------------------------------------------------------------- diff --git a/be/src/rpc/auth-provider.h b/be/src/rpc/auth-provider.h index 3e5517f..f3180d5 100644 --- a/be/src/rpc/auth-provider.h +++ b/be/src/rpc/auth-provider.h @@ -24,14 +24,11 @@ #include "common/status.h" #include "util/promise.h" -#include "util/thread.h" namespace sasl { class TSasl; } namespace impala { -class Thread; - /// An AuthProvider creates Thrift transports that are set up to authenticate themselves /// using a protocol such as Kerberos or PLAIN/SASL. Both server and client transports are /// provided by this class, using slightly different mechanisms (servers use a factory, @@ -70,9 +67,8 @@ class SaslAuthProvider : public AuthProvider { SaslAuthProvider(bool is_internal) : has_ldap_(false), is_internal_(is_internal), needs_kinit_(false) {} - /// Performs initialization of external state. If we're using kerberos and - /// need to kinit, start that thread. If we're using ldap, set up appropriate - /// certificate usage. + /// Performs initialization of external state. Kinit if configured to use kerberos. + /// If we're using ldap, set up appropriate certificate usage. virtual Status Start(); /// Wrap the client transport with a new TSaslClientTransport. This is only for @@ -143,19 +139,6 @@ class SaslAuthProvider : public AuthProvider { /// function as a client. bool needs_kinit_; - /// Runs "RunKinit" below if needs_kinit_ is true and FLAGS_use_kudu_kinit is false - /// and FLAGS_use_krpc is false. Once started, this thread lives as long as the process - /// does and periodically forks impalad and execs the 'kinit' process. - std::unique_ptr<Thread> kinit_thread_; - - /// Periodically (roughly once every FLAGS_kerberos_reinit_interval minutes) calls kinit - /// to get a ticket granting ticket from the kerberos server for principal_, which is - /// kept in the kerberos cache associated with this process. This ensures that we have - /// valid kerberos credentials when operating as a client. Once the first attempt to - /// obtain a ticket has completed, first_kinit is Set() with the status of the operation. - /// Additionally, if the first attempt fails, this method will return. - void RunKinit(Promise<Status>* first_kinit); - /// One-time kerberos-specific environment variable setup. Called by InitKerberos(). Status InitKerberosEnv() WARN_UNUSED_RESULT; }; http://git-wip-us.apache.org/repos/asf/impala/blob/4dc3d340/be/src/rpc/authentication.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc index 4c3df50..3eb77bb 100644 --- a/be/src/rpc/authentication.cc +++ b/be/src/rpc/authentication.cc @@ -20,7 +20,6 @@ #include <stdio.h> #include <signal.h> #include <boost/algorithm/string.hpp> -#include <boost/thread/thread.hpp> #include <boost/scoped_ptr.hpp> #include <boost/random/mersenne_twister.hpp> #include <boost/random/uniform_int.hpp> @@ -48,7 +47,6 @@ #include "util/network-util.h" #include "util/os-util.h" #include "util/promise.h" -#include "util/thread.h" #include "util/time.h" #include <sys/types.h> // for stat system call @@ -74,11 +72,6 @@ DECLARE_string(be_principal); DECLARE_string(krb5_conf); DECLARE_string(krb5_debug_file); -// TODO: Remove this flag in a compatibility-breaking release. (IMPALA-5893) -DEFINE_int32(kerberos_reinit_interval, 60, - "Interval, in minutes, between kerberos ticket renewals. " - "Only used when FLAGS_use_krpc is false"); - DEFINE_string(sasl_path, "", "Colon separated list of paths to look for SASL " "security library plugins."); DEFINE_bool(enable_ldap_auth, false, @@ -108,13 +101,6 @@ DEFINE_string(internal_principals_whitelist, "hdfs", "(Advanced) Comma-separated "'hdfs' which is the system user that in certain deployments must access " "catalog server APIs."); -// TODO: Remove this flag and the old kerberos code once we remove 'use_krpc' flag. -// (IMPALA-5893) -DEFINE_bool(use_kudu_kinit, true, "If true, Impala will programatically perform kinit " - "by calling into the libkrb5 library using the provided APIs. If false, it will fork " - "off a kinit process. If use_krpc=true, this flag is treated as true regardless of " - "what it's set to."); - namespace impala { // Sasl callbacks. Why are these here? Well, Sasl isn't that bright, and @@ -496,51 +482,6 @@ static int SaslGetPath(void* context, const char** path) { return SASL_OK; } -// When operating as a Kerberos client (internal connections only), we need to -// 'kinit' as the principal. A thread is created and calls this function for -// that purpose, and to periodically renew the ticket as well. -// -// first_kinit: Used to communicate success/failure of the initial kinit call to -// the parent thread -// Return: Only if the first call to 'kinit' fails -void SaslAuthProvider::RunKinit(Promise<Status>* first_kinit) { - - // Pass the path to the key file and the principal. - const string kinit_cmd = Substitute("kinit -k -t $0 $1 2>&1", - keytab_file_, principal_); - - bool first_time = true; - std::random_device rd; - mt19937 generator(rd()); - uniform_int<> dist(0, 300); - - while (true) { - LOG(INFO) << "Registering " << principal_ << ", keytab file " << keytab_file_; - string kinit_output; - bool success = RunShellProcess(kinit_cmd, &kinit_output); - - if (!success) { - const string& err_msg = Substitute( - "Failed to obtain Kerberos ticket for principal: $0. $1", principal_, - kinit_output); - if (first_time) { - first_kinit->Set(Status(err_msg)); - return; - } else { - LOG(ERROR) << err_msg; - } - } else if (first_time) { - first_time = false; - first_kinit->Set(Status::OK()); - } - - // Sleep for the renewal interval, minus a random time between 0-5 minutes to help - // avoid a storm at the KDC. Additionally, never sleep less than a minute to - // reduce KDC stress due to frequent renewals. - SleepForMs(1000 * max((60 * FLAGS_kerberos_reinit_interval) - dist(generator), 60)); - } -} - namespace { // SASL requires mutexes for thread safety, but doesn't implement @@ -842,25 +783,10 @@ Status SaslAuthProvider::Start() { if (needs_kinit_) { DCHECK(is_internal_); DCHECK(!principal_.empty()); - if (FLAGS_use_kudu_kinit || FLAGS_use_krpc) { - // With KRPC enabled, we always rely on the Kudu library to carry out the Kerberos - // authentication during connection negotiation. - if (!FLAGS_use_kudu_kinit) { - LOG(INFO) << "Ignoring --use_kudu_kinit=false as KRPC and Kerberos are enabled"; - } - // Starts a thread that periodically does a 'kinit'. The thread lives as long as the - // process does. - KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer(principal_, keytab_file_, - KRB5CCNAME_PATH, false), "Could not init kerberos"); - } else { - Promise<Status> first_kinit; - stringstream thread_name; - thread_name << "kinit-" << principal_; - RETURN_IF_ERROR(Thread::Create("authentication", thread_name.str(), - &SaslAuthProvider::RunKinit, this, &first_kinit, &kinit_thread_)); - LOG(INFO) << "Waiting for Kerberos ticket for principal: " << principal_; - RETURN_IF_ERROR(first_kinit.Get()); - } + // Starts a thread that periodically does a 'kinit'. The thread lives as long as the + // process does. + KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer(principal_, keytab_file_, + KRB5CCNAME_PATH, false), "Could not init kerberos"); LOG(INFO) << "Kerberos ticket granted to " << principal_; } http://git-wip-us.apache.org/repos/asf/impala/blob/4dc3d340/be/src/rpc/rpc-mgr-kerberized-test.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/rpc-mgr-kerberized-test.cc b/be/src/rpc/rpc-mgr-kerberized-test.cc index 141f359..0407308 100644 --- a/be/src/rpc/rpc-mgr-kerberized-test.cc +++ b/be/src/rpc/rpc-mgr-kerberized-test.cc @@ -18,7 +18,6 @@ #include "rpc/rpc-mgr-test-base.h" #include "service/fe-support.h" -DECLARE_bool(use_kudu_kinit); DECLARE_bool(use_krpc); DECLARE_string(be_principal); @@ -39,9 +38,7 @@ class RpcMgrKerberizedTest : public RpcMgrTestBase<testing::TestWithParam<KerberosSwitch> > { virtual void SetUp() override { - KerberosSwitch k = GetParam(); FLAGS_use_krpc = true; - FLAGS_use_kudu_kinit = k == USE_KRPC_KUDU_KERBEROS; FLAGS_principal = "dummy-service/host@realm"; FLAGS_be_principal = strings::Substitute("$0@$1", kdc_principal, kdc_realm); ASSERT_OK(InitAuth(CURRENT_EXECUTABLE_PATH)); @@ -56,8 +53,7 @@ class RpcMgrKerberizedTest : INSTANTIATE_TEST_CASE_P(KerberosOnAndOff, RpcMgrKerberizedTest, - ::testing::Values(USE_KRPC_IMPALA_KERBEROS, - USE_KRPC_KUDU_KERBEROS)); + ::testing::Values(KERBEROS_ON)); TEST_P(RpcMgrKerberizedTest, MultipleServicesTls) { // TODO: We're starting a seperate RpcMgr here instead of configuring http://git-wip-us.apache.org/repos/asf/impala/blob/4dc3d340/be/src/rpc/thrift-server-test.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/thrift-server-test.cc b/be/src/rpc/thrift-server-test.cc index f0a0bc5..03bd295 100644 --- a/be/src/rpc/thrift-server-test.cc +++ b/be/src/rpc/thrift-server-test.cc @@ -35,7 +35,6 @@ using namespace strings; using namespace apache::thrift; using apache::thrift::transport::SSLProtocol; -DECLARE_bool(use_kudu_kinit); DECLARE_bool(use_krpc); DECLARE_string(principal); @@ -110,7 +109,6 @@ class ThriftKerberizedParamsTest : FLAGS_principal.clear(); FLAGS_be_principal.clear(); } else { - FLAGS_use_kudu_kinit = k == USE_THRIFT_KUDU_KERBEROS; FLAGS_principal = "dummy-service/host@realm"; FLAGS_be_principal = strings::Substitute("$0@$1", kdc_principal, kdc_realm); } @@ -127,8 +125,7 @@ class ThriftKerberizedParamsTest : INSTANTIATE_TEST_CASE_P(KerberosOnAndOff, ThriftKerberizedParamsTest, ::testing::Values(KERBEROS_OFF, - USE_THRIFT_KUDU_KERBEROS, - USE_THRIFT_IMPALA_KERBEROS)); + KERBEROS_ON)); TEST(ThriftTestBase, Connectivity) { int port = GetServerPort(); http://git-wip-us.apache.org/repos/asf/impala/blob/4dc3d340/be/src/testutil/mini-kdc-wrapper.h ---------------------------------------------------------------------- diff --git a/be/src/testutil/mini-kdc-wrapper.h b/be/src/testutil/mini-kdc-wrapper.h index 17c174a..602c15b 100644 --- a/be/src/testutil/mini-kdc-wrapper.h +++ b/be/src/testutil/mini-kdc-wrapper.h @@ -29,10 +29,7 @@ namespace impala { enum KerberosSwitch { KERBEROS_OFF, - USE_KRPC_KUDU_KERBEROS, // FLAGS_use_kudu_kinit = true, FLAGS_use_krpc = true - USE_KRPC_IMPALA_KERBEROS, // FLAGS_use_kudu_kinit = false, FLAGS_use_krpc = true - USE_THRIFT_KUDU_KERBEROS, // FLAGS_use_kudu_kinit = true, FLAGS_use_krpc = false - USE_THRIFT_IMPALA_KERBEROS // FLAGS_use_kudu_kinit = false, FLAGS_use_krpc = false + KERBEROS_ON }; /// This class allows tests to easily start and stop a KDC and configure Impala's auth
