IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.

This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.

Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.

TODO: Since the setting up and tearing down of our security code
isn't idempotent, we can run only any one test in a process with
Kerberos now (IMPALA-6085).

Updated bin/bootstrap_system.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Also fixed a bug that didn't transfer the environment into 'sudo'
in bin/bootstrap_system.sh.

Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.

Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Reviewed-on: http://gerrit.cloudera.org:8080/7938
Reviewed-by: Sailesh Mukil <[email protected]>
Tested-by: Impala Public Jenkins

CMakeLists.txt

@@ -301,6 +301,10 @@ IMPALA_ADD_THIRDPARTY_LIB(breakpad_client ${BREAKPAD_INCLUDE_DIR} ${BREAKPAD_STA
 find_package(Kerberos REQUIRED)
 IMPALA_ADD_THIRDPARTY_LIB(krb5 ${KERBEROS_INCLUDE_DIR} "" ${KERBEROS_LIBRARY})
 
+# We require certain binaries from the kerberos project for our automated kerberos
+# testing.
+find_package(KerberosPrograms REQUIRED)
+
 ###################################################################
 
 # System dependencies

be/src/exec/kudu-util.h

@@ -39,6 +39,13 @@ namespace impala {
     } \
   } while (0)
 
+
+#define KUDU_ASSERT_OK(status)                                     \
+  do {                                                             \
+    const Status& status_ = FromKuduStatus(status);                \
+    ASSERT_TRUE(status_.ok()) << "Error: " << status_.GetDetail(); \
+  } while (0)
+
 class TimestampValue;
 
 /// Returns false when running on an operating system that Kudu doesn't support. If this

be/src/kudu/security/CMakeLists.txt

@@ -84,6 +84,14 @@ ADD_EXPORTABLE_LIBRARY(security
   SRCS ${SECURITY_SRCS}
   DEPS ${SECURITY_LIBS})
 
+# Since Kudu tests are explicitly disabled, we want to expose some of their sources
+# to Impala using another variable.
+set(SECURITY_TEST_SRCS_FOR_IMPALA test/mini_kdc.cc)
+add_library(security-test-for-impala ${SECURITY_TEST_SRCS_FOR_IMPALA})
+target_link_libraries(security-test-for-impala
+  gutil
+  kudu_util
+  security)
 
 ##############################
 # security-test

be/src/kudu/security/test/mini_kdc.cc

@@ -37,7 +37,10 @@
 #include "kudu/util/path_util.h"
 #include "kudu/util/stopwatch.h"
 #include "kudu/util/subprocess.h"
-#include "kudu/util/test_util.h"
+
+// test_util.h has a dependancy on gmock which Impala doesn't depend on, so we rewrite
+// parts of this code that use test_util members.
+//#include "kudu/util/test_util.h"
 
 using std::map;
 using std::string;
@@ -60,7 +63,9 @@ MiniKdc::MiniKdc(const MiniKdcOptions& options)
     options_.realm = "KRBTEST.COM";
   }
   if (options_.data_root.empty()) {
-    options_.data_root = JoinPathSegments(GetTestDataDirectory(), "krb5kdc");
+    // We hardcode "/tmp" here since the original function which initializes a random test
+    // directory (GetTestDataDirectory()), depends on gmock.
+    options_.data_root = JoinPathSegments("/tmp", "krb5kdc");
   }
   if (options_.renew_lifetime.empty()) {
     options_.renew_lifetime = "7d";

be/src/rpc/CMakeLists.txt

@@ -38,6 +38,9 @@ add_dependencies(Rpc gen-deps)
 
 ADD_BE_TEST(thrift-util-test)
 ADD_BE_TEST(thrift-server-test)
+# The thrift-server-test uses some utilites from the Kudu security test code.
+target_link_libraries(thrift-server-test security-test-for-impala)
+
 ADD_BE_TEST(authentication-test)
 
 ADD_BE_TEST(rpc-mgr-test)

be/src/rpc/auth-provider.h

@@ -140,7 +140,9 @@ class SaslAuthProvider : public AuthProvider {
   /// function as a client.
   bool needs_kinit_;
 
-  /// Runs "RunKinit" below if needs_kinit_ is true.
+  /// Runs "RunKinit" below if needs_kinit_ is true and FLAGS_use_kudu_kinit 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

be/src/rpc/authentication.cc

@@ -37,6 +37,7 @@
 #include <ldap.h>
 
 #include "exec/kudu-util.h"
+#include "kudu/security/init.h"
 #include "rpc/auth-provider.h"
 #include "rpc/thrift-server.h"
 #include "transport/TSaslClientTransport.h"
@@ -70,8 +71,11 @@ 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.");
+    "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,
@@ -101,6 +105,12 @@ 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 in a compatibility-breaking release.
+// (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.");
+
 namespace impala {
 
 // Sasl callbacks.  Why are these here?  Well, Sasl isn't that bright, and
@@ -119,6 +129,10 @@ static vector<sasl_callback_t> KERB_INT_CALLBACKS;  // Internal kerberos connect
 static vector<sasl_callback_t> KERB_EXT_CALLBACKS;  // External kerberos connections
 static vector<sasl_callback_t> LDAP_EXT_CALLBACKS;  // External LDAP connections
 
+// Path to the file based credential cache that we pass to the KRB5CCNAME environment
+// variable.
+static const string KRB5CCNAME_PATH = "/tmp/krb5cc_impala_internal";
+
 // Pattern for hostname substitution.
 static const string HOSTNAME_PATTERN = "_HOST";
 
@@ -832,13 +846,20 @@ Status SaslAuthProvider::Start() {
   if (needs_kinit_) {
     DCHECK(is_internal_);
     DCHECK(!principal_.empty());
-    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());
+    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");
+    } 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());
+    }
     LOG(INFO) << "Kerberos ticket granted to " << principal_;
   }
 

be/src/rpc/thrift-server-test.cc

@@ -16,22 +16,32 @@
 // under the License.
 
 #include <atomic>
+#include <boost/filesystem.hpp>
 #include <string>
 
+#include "exec/kudu-util.h"
 #include "gen-cpp/StatestoreService.h"
 #include "gutil/strings/substitute.h"
+#include "kudu/util/env.h"
+#include "kudu/security/test/mini_kdc.h"
+#include "rpc/authentication.h"
 #include "rpc/thrift-client.h"
 #include "service/fe-support.h"
 #include "service/impala-server.h"
 #include "testutil/gtest-util.h"
 #include "testutil/scoped-flag-setter.h"
+#include "util/filesystem-util.h"
 
 #include "common/names.h"
 
 using namespace impala;
 using namespace strings;
 using namespace apache::thrift;
 using apache::thrift::transport::SSLProtocol;
+namespace filesystem = boost::filesystem;
+using filesystem::path;
+
+DECLARE_bool(use_kudu_kinit);
 
 DECLARE_string(ssl_client_ca_certificate);
 DECLARE_string(ssl_cipher_list);
@@ -42,6 +52,10 @@ DECLARE_int32(state_store_port);
 DECLARE_int32(be_port);
 DECLARE_int32(beeswax_port);
 
+DECLARE_string(keytab_file);
+DECLARE_string(principal);
+DECLARE_string(krb5_conf);
+
 string IMPALA_HOME(getenv("IMPALA_HOME"));
 const string& SERVER_CERT =
     Substitute("$0/be/src/testutil/server-cert.pem", IMPALA_HOME);
@@ -79,7 +93,105 @@ int GetServerPort() {
   return port;
 }
 
-TEST(ThriftServer, Connectivity) {
+static int kdc_port = GetServerPort();
+
+enum KerberosSwitch {
+  KERBEROS_OFF,
+  USE_KUDU_KERBEROS,    // FLAGS_use_kudu_kinit = true
+  USE_IMPALA_KERBEROS   // FLAGS_use_kudu_kinit = false
+};
+
+template <class T> class ThriftTestBase : public T {
+  virtual void SetUp() {}
+  virtual void TearDown() {}
+};
+
+// This class allows us to run all the tests that derive from this in the modes enumerated
+// in 'KerberosSwitch'.
+// If the mode is USE_KUDU_KERBEROS or USE_IMPALA_KERBEROS, the MiniKdc which is a wrapper
+// around the 'krb5kdc' process, is configured and started. We then configure our
+// thrift transports to speak Kebreros and verify that it functionally works.
+// TODO: Since the setting up and tearing down of our security code isn't idempotent, we
+// can run only any one test in a process with Kerberos now (IMPALA-6085).
+class ThriftParamsTest : public ThriftTestBase<testing::TestWithParam<KerberosSwitch> > {
+  virtual void SetUp() {
+    if (GetParam() > KERBEROS_OFF) {
+      FLAGS_use_kudu_kinit = GetParam() == USE_KUDU_KERBEROS;
+      // Check if the unique directory already exists, and create it if it doesn't.
+      ASSERT_OK(FileSystemUtil::RemoveAndCreateDirectory(unique_test_dir_.string()));
+      string keytab_dir = unique_test_dir_.string() + "/krb5kdc";
+      string realm = "KRBTEST.COM";
+      string ticket_lifetime = "24h";
+      string renew_lifetime = "7d";
+      FLAGS_krb5_conf = Substitute("$0/$1", keytab_dir, "krb5.conf");
+
+      StartKdc(realm, keytab_dir, ticket_lifetime, renew_lifetime);
+
+      string spn = "impala-test/localhost";
+      string kt_path;
+      CreateServiceKeytab(spn, &kt_path);
+
+      FLAGS_keytab_file = kt_path;
+      FLAGS_principal = Substitute("$0@$1", spn, realm);
+
+    }
+    string current_executable_path;
+    KUDU_ASSERT_OK(kudu::Env::Default()->GetExecutablePath(&current_executable_path));
+    ASSERT_OK(InitAuth(current_executable_path));
+  }
+
+  virtual void TearDown() {
+    if (GetParam() > KERBEROS_OFF) {
+      StopKdc();
+      FLAGS_keytab_file.clear();
+      FLAGS_principal.clear();
+      FLAGS_krb5_conf.clear();
+      EXPECT_OK(FileSystemUtil::RemovePaths({unique_test_dir_.string()}));
+    }
+  }
+
+ private:
+  boost::scoped_ptr<kudu::MiniKdc> kdc_;
+  // Create a unique directory for this test to store its files in.
+  filesystem::path unique_test_dir_ = filesystem::unique_path();
+
+  void StartKdc(string realm, string keytab_dir, string ticket_lifetime,
+      string renew_lifetime);
+  void StopKdc();
+  void CreateServiceKeytab(const string& spn, string* kt_path);
+};
+
+void ThriftParamsTest::StartKdc(string realm, string keytab_dir, string ticket_lifetime,
+    string renew_lifetime) {
+  kudu::MiniKdcOptions options;
+  options.realm = realm;
+  options.data_root = keytab_dir;
+  options.ticket_lifetime = ticket_lifetime;
+  options.renew_lifetime = renew_lifetime;
+  options.port = kdc_port;
+
+  DCHECK(kdc_.get() == nullptr);
+  kdc_.reset(new kudu::MiniKdc(options));
+  DCHECK(kdc_.get() != nullptr);
+  KUDU_ASSERT_OK(kdc_->Start());
+  KUDU_ASSERT_OK(kdc_->SetKrb5Environment());
+}
+
+void ThriftParamsTest::CreateServiceKeytab(const string& spn, string* kt_path) {
+  KUDU_ASSERT_OK(kdc_->CreateServiceKeytab(spn, kt_path));
+}
+
+void ThriftParamsTest::StopKdc() {
+  KUDU_ASSERT_OK(kdc_->Stop());
+}
+
+INSTANTIATE_TEST_CASE_P(KerberosOnAndOff,
+                        ThriftParamsTest,
+                        ::testing::Values(KERBEROS_OFF,
+                                          USE_KUDU_KERBEROS,
+                                          USE_IMPALA_KERBEROS));
+
+TEST(ThriftTestBase, Connectivity) {
   int port = GetServerPort();
   ThriftClient<StatestoreServiceClientWrapper> wrong_port_client(
       "localhost", port, "", nullptr, false);
@@ -93,7 +205,7 @@ TEST(ThriftServer, Connectivity) {
   ASSERT_OK(wrong_port_client.Open());
 }
 
-TEST(SslTest, Connectivity) {
+TEST_P(ThriftParamsTest, SslConnectivity) {
   int port = GetServerPort();
   // Start a server using SSL and confirm that an SSL client can connect, while a non-SSL
   // client cannot.
@@ -118,10 +230,22 @@ TEST(SslTest, Connectivity) {
   // Disable SSL for this client.
   ThriftClient<StatestoreServiceClientWrapper> non_ssl_client(
       "localhost", port, "", nullptr, false);
-  ASSERT_OK(non_ssl_client.Open());
-  send_done = false;
-  EXPECT_THROW(non_ssl_client.iface()->RegisterSubscriber(
-      resp, TRegisterSubscriberRequest(), &send_done), TTransportException);
+
+  if (GetParam() == KERBEROS_OFF) {
+    // When Kerberos is OFF, Open() succeeds as there's no data transfer over the wire.
+    ASSERT_OK(non_ssl_client.Open());
+    send_done = false;
+    // Verify that data transfer over the wire is not possible.
+    EXPECT_THROW(non_ssl_client.iface()->RegisterSubscriber(
+        resp, TRegisterSubscriberRequest(), &send_done), TTransportException);
+  } else {
+    // When Kerberos is ON, the SASL negotiation happens inside Open(). We expect that to
+    // fail beacuse the server expects the client to negotiate over an encrypted
+    // connection.
+    EXPECT_STR_CONTAINS(non_ssl_client.Open().GetDetail(),
+        "No more data to read");
+  }
+
 }
 
 TEST(SslTest, BadCertificate) {

bin/bootstrap_system.sh

@@ -79,7 +79,7 @@ REAL_APT_GET=$(which apt-get)
 function apt-get {
   for ITER in $(seq 1 20); do
     echo "ATTEMPT: ${ITER}"
-    if sudo "${REAL_APT_GET}" "$@"
+    if sudo -E "${REAL_APT_GET}" "$@"
     then
       return 0
     fi
@@ -103,9 +103,10 @@ SET_IMPALA_HOME="export IMPALA_HOME=$(pwd)"
 echo "$SET_IMPALA_HOME" >> ~/.bashrc
 eval "$SET_IMPALA_HOME"
 
-apt-get --yes install ccache g++ gcc libffi-dev liblzo2-dev libkrb5-dev libsasl2-dev \
-        libssl-dev make maven ninja-build ntp ntpdate python-dev python-setuptools \
-        postgresql ssh wget vim-common psmisc
+apt-get --yes install ccache g++ gcc libffi-dev liblzo2-dev libkrb5-dev \
+        krb5-admin-server krb5-kdc krb5-user libsasl2-dev libsasl2-modules \
+        libsasl2-modules-gssapi-mit libssl-dev make maven ninja-build ntp \
+        ntpdate python-dev python-setuptools postgresql ssh wget vim-common psmisc
 
 if ! { service --status-all | grep -E '^ \[ \+ \]  ssh$'; }
 then