Repository: incubator-impala Updated Branches: refs/heads/master f2cd5bd51 -> 4592ed445
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 Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/4592ed44 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/4592ed44 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/4592ed44 Branch: refs/heads/master Commit: 4592ed445e5cd946efbec8e40c73ed3e8162f3b2 Parents: f2cd5bd Author: Sailesh Mukil <[email protected]> Authored: Tue Mar 28 15:56:26 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Fri Oct 27 00:19:44 2017 +0000 ---------------------------------------------------------------------- CMakeLists.txt | 4 + be/src/exec/kudu-util.h | 7 ++ be/src/kudu/security/CMakeLists.txt | 8 ++ be/src/kudu/security/test/mini_kdc.cc | 9 +- be/src/rpc/CMakeLists.txt | 3 + be/src/rpc/auth-provider.h | 4 +- be/src/rpc/authentication.cc | 37 +++++-- be/src/rpc/thrift-server-test.cc | 136 ++++++++++++++++++++++++-- bin/bootstrap_system.sh | 9 +- cmake_modules/FindKerberosPrograms.cmake | 38 +++++++ 10 files changed, 234 insertions(+), 21 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4592ed44/CMakeLists.txt ---------------------------------------------------------------------- diff --git a/CMakeLists.txt b/CMakeLists.txt index 865f7b2..916c249 100644 --- a/CMakeLists.txt +++ b/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 http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4592ed44/be/src/exec/kudu-util.h ---------------------------------------------------------------------- diff --git a/be/src/exec/kudu-util.h b/be/src/exec/kudu-util.h index 6113401..5de55fe 100644 --- a/be/src/exec/kudu-util.h +++ b/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 http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4592ed44/be/src/kudu/security/CMakeLists.txt ---------------------------------------------------------------------- diff --git a/be/src/kudu/security/CMakeLists.txt b/be/src/kudu/security/CMakeLists.txt index 0dc7d0f..4c88fe3 100644 --- a/be/src/kudu/security/CMakeLists.txt +++ b/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 http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4592ed44/be/src/kudu/security/test/mini_kdc.cc ---------------------------------------------------------------------- diff --git a/be/src/kudu/security/test/mini_kdc.cc b/be/src/kudu/security/test/mini_kdc.cc index 4b84bf2..0d23f27 100644 --- a/be/src/kudu/security/test/mini_kdc.cc +++ b/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"; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4592ed44/be/src/rpc/CMakeLists.txt ---------------------------------------------------------------------- diff --git a/be/src/rpc/CMakeLists.txt b/be/src/rpc/CMakeLists.txt index 95be43f..326b806 100644 --- a/be/src/rpc/CMakeLists.txt +++ b/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) http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4592ed44/be/src/rpc/auth-provider.h ---------------------------------------------------------------------- diff --git a/be/src/rpc/auth-provider.h b/be/src/rpc/auth-provider.h index 9286154..63d2fe1 100644 --- a/be/src/rpc/auth-provider.h +++ b/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 http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4592ed44/be/src/rpc/authentication.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc index 275963f..b973f20 100644 --- a/be/src/rpc/authentication.cc +++ b/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_; } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4592ed44/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 fbb00ef..c1ad4fc 100644 --- a/be/src/rpc/thrift-server-test.cc +++ b/be/src/rpc/thrift-server-test.cc @@ -16,15 +16,21 @@ // 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" @@ -32,6 +38,10 @@ 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(¤t_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) { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4592ed44/bin/bootstrap_system.sh ---------------------------------------------------------------------- diff --git a/bin/bootstrap_system.sh b/bin/bootstrap_system.sh index 2229c24..5a47722 100755 --- a/bin/bootstrap_system.sh +++ b/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 http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4592ed44/cmake_modules/FindKerberosPrograms.cmake ---------------------------------------------------------------------- diff --git a/cmake_modules/FindKerberosPrograms.cmake b/cmake_modules/FindKerberosPrograms.cmake new file mode 100644 index 0000000..27b181a --- /dev/null +++ b/cmake_modules/FindKerberosPrograms.cmake @@ -0,0 +1,38 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# - Find Kerberos Binaries +# This module ensures that the Kerberos binaries depended on by tests are +# present on the system. + +include(FindPackageHandleStandardArgs) +set(bins kadmin.local kdb5_util kdestroy kinit klist krb5kdc) + +foreach(bin ${bins}) + find_program(${bin} ${bin} PATHS + # Linux install location. + /usr/sbin + # Homebrew install location. + /usr/local/opt/krb5/sbin + # Macports install location. + /opt/local/sbin + # SLES + /usr/lib/mit/sbin) +endforeach(bin) + +find_package_handle_standard_args(Kerberos REQUIRED_VARS ${bins} + FAIL_MESSAGE "Kerberos binaries not found: security tests will fail")
