This is an automated email from the ASF dual-hosted git repository. tarmstrong pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit f66874e67b6895e2d939a0602e9633d8f3cc1cb2 Author: Tim Armstrong <[email protected]> AuthorDate: Mon Mar 2 23:37:52 2020 -0800 IMPALA-9456: allow disabling kerberos selectively There are specific use cases where we need to talk to kerberized services (HMS, etc) and want to keep our TGT up to date using Impala's kinit infrastructure, but don't want to kerberize all connections. Adds --skip_internal_kerberos_auth and --skip_external_kerberos_auth to disable Kerberos authentication for incoming connections even if --principal is set. The daemon does a kinit and keeps tickets up-to-date with the background thread even if kerberos is disabled for all incoming connections. Behaviour only changes when those flags are toggled. The change required restructuring the code a bit, specifically pulling the call to InitKerberosForServer() out of SecureAuthProvider, which I think is a net improvement that makes the control flow clearer. Testing: Add unit tests to: * confirm that the kinit occurs even with auth disabled. * confirm that incoming KRPC connections do not require authentication. I would have liked to add automated tests for thrift interfaces but did not have the infrastructure to unit-test it. I think the changes I made are fairly low risk because they do not increase the number of code paths in AuthManager::Init() and can be verified by inspection. The tests I would have liked to add are: * confirm that incoming external connections do not require auth * confirm that internal thrift connections do not require auth. Manually started kerberized minicluster with internal/external kerberos disabled, e.g. with the command line: start-impala-cluster.py --impalad_args=--skip_external_kerberos_auth=true --impalad_args=--skip_internal_kerberos_auth=true --state_store_args=--skip_internal_kerberos_auth=true --catalogd_args=--skip_internal_kerberos_auth=true Confirmed that impala-shell connected without -k when --skip_external_kerberos_auth=true and requires -k otherwise. Confirmed that we could run queries against HDFS tables even with internal and external auth disabled. Checked logs to see that tickets were reacquired. I0303 08:33:49.319911 16079 init.cc:303] Successfully reacquired a new kerberos TGT Tested that a partially kerberised minicluster worked as expected, i.e. that impalad <-> catalog/statestore connections can have auth disabled so that the impalad can authenticate even if it does not have the right principal set. The first start-impala-cluster.py command below succeeds but the second and third fail because they cannot authenticate with the catalog and statestore respectively because the processes did not kinit as the impala principal. kinit -kt impala.keytab tarmstrong/[email protected] start-impala-cluster.py --impalad_args='--principal="" --be_principal="" --keytab_file="" --krb5_ccname="/tmp/krb5cc_1000"' --state_store_args=--skip_internal_kerberos_auth=true --catalogd_args=--skip_internal_kerberos_auth=true kinit -kt impala.keytab tarmstrong/[email protected] start-impala-cluster.py --impalad_args='--principal="" --be_principal="" --keytab_file="" --krb5_ccname="/tmp/krb5cc_1000"' --state_store_args=--skip_internal_kerberos_auth=false --catalogd_args=--skip_internal_kerberos_auth=true kinit -kt impala.keytab tarmstrong/[email protected] start-impala-cluster.py --impalad_args='--principal="" --be_principal="" --keytab_file="" --krb5_ccname="/tmp/krb5cc_1000"' --state_store_args=--skip_internal_kerberos_auth=false --catalogd_args=--skip_internal_kerberos_auth=true Change-Id: I3b1c641e05e588287e4d9d9cd8389d96fc71cf74 Reviewed-on: http://gerrit.cloudera.org:8080/15351 Reviewed-by: Tim Armstrong <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- be/src/common/global-flags.cc | 10 +++++ be/src/rpc/auth-provider.h | 14 ++----- be/src/rpc/authentication-test.cc | 18 ++++----- be/src/rpc/authentication.cc | 71 ++++++++++++++++++++--------------- be/src/rpc/rpc-mgr-kerberized-test.cc | 63 +++++++++++++++++++++++++++++++ be/src/rpc/rpc-mgr.cc | 4 +- be/src/util/auth-util.h | 12 ++++++ 7 files changed, 138 insertions(+), 54 deletions(-) diff --git a/be/src/common/global-flags.cc b/be/src/common/global-flags.cc index fb65bf3..429c87b 100644 --- a/be/src/common/global-flags.cc +++ b/be/src/common/global-flags.cc @@ -56,6 +56,16 @@ DEFINE_string(krb5_ccname, "/tmp/krb5cc_impala_internal", "Absolute path to the DEFINE_string(krb5_conf, "", "Absolute path to Kerberos krb5.conf if in a non-standard " "location. Does not normally need to be set."); DEFINE_string(krb5_debug_file, "", "Turn on Kerberos debugging and output to this file"); +DEFINE_bool(skip_internal_kerberos_auth, false, + "(Advanced) skip kerberos authentication for incoming internal connections from " + "other daemons within the Impala cluster (i.e. impalads, statestored, catalogd). " + "Must be set to the same value across all daemons. Only has an effect if --principal " + "is set, i.e. Kerberos is enabled."); +DEFINE_bool(skip_external_kerberos_auth, false, + "(Advanced) skip kerberos authentication for incoming external connections to " + "this daemon, e.g. clients connecting to the HS2 interface. Only has an effect " + "if --principal is set, i.e. Kerberos is enabled."); + static const string mem_limit_help_msg = "Limit on process memory consumption. " "Includes the JVM's memory consumption only if --mem_limit_includes_jvm is true. " diff --git a/be/src/rpc/auth-provider.h b/be/src/rpc/auth-provider.h index 9179505..4587946 100644 --- a/be/src/rpc/auth-provider.h +++ b/be/src/rpc/auth-provider.h @@ -83,9 +83,9 @@ class AuthProvider { class SecureAuthProvider : public AuthProvider { public: SecureAuthProvider(bool is_internal) - : has_ldap_(false), is_internal_(is_internal), needs_kinit_(false) {} + : has_ldap_(false), is_internal_(is_internal) {} - /// Performs initialization of external state. Kinit if configured to use kerberos. + /// Performs initialization of external state. /// If we're using ldap, set up appropriate certificate usage. virtual Status Start(); @@ -123,7 +123,7 @@ class SecureAuthProvider : public AuthProvider { /// Initializes kerberos items and checks for sanity. Failures can occur on a /// malformed principal or when setting some environment variables. Called /// prior to Start(). - Status InitKerberos(const std::string& principal, const std::string& keytab_path); + Status InitKerberos(const std::string& principal); /// Initializes ldap - just record that we're going to use it. Called prior to /// Start(). @@ -153,9 +153,6 @@ class SecureAuthProvider : public AuthProvider { /// supplied, this is --be_principal. In all other cases this is --principal. std::string principal_; - /// The full path to the keytab where the above principal can be found. - std::string keytab_file_; - /// The service name, deduced from the principal. Used by servers to indicate /// what service a principal must have a ticket for in order to be granted /// access to this service. @@ -164,11 +161,6 @@ class SecureAuthProvider : public AuthProvider { /// Principal's realm, again derived from principal. std::string realm_; - /// True if tickets for this principal should be obtained. This is true if - /// we're an auth provider for an "internal" connection, because we may - /// function as a client. - bool needs_kinit_; - /// Used to generate and verify signatures for cookies. AuthenticationHash hash_; diff --git a/be/src/rpc/authentication-test.cc b/be/src/rpc/authentication-test.cc index 52c5833..7f2d1c6 100644 --- a/be/src/rpc/authentication-test.cc +++ b/be/src/rpc/authentication-test.cc @@ -60,7 +60,7 @@ TEST(Auth, PrincipalSubstitution) { string principal; ASSERT_OK(GetExternalKerberosPrincipal(&principal)); - ASSERT_OK(sa.InitKerberos(principal, "/etc/hosts")); + ASSERT_OK(sa.InitKerberos(principal)); ASSERT_OK(sa.Start()); ASSERT_EQ(string::npos, sa.principal().find("_HOST")); ASSERT_NE(string::npos, sa.principal().find(hostname)); @@ -84,7 +84,7 @@ void AuthFails(const string& name, SecureAuthProvider* sa) { TEST(Auth, AuthorizeInternalPrincipals) { SecureAuthProvider sa(true); // false means it's external - ASSERT_OK(sa.InitKerberos("service_name/[email protected]", "/etc/hosts")); + ASSERT_OK(sa.InitKerberos("service_name/[email protected]")); AuthOk("service_name/[email protected]", &sa); AuthFails("unknown/[email protected]", &sa); @@ -177,17 +177,15 @@ TEST(Auth, KerbAndSslEnabled) { FLAGS_ssl_private_key = "some_path"; ASSERT_TRUE(IsInternalTlsConfigured()); SecureAuthProvider sa_internal(true); - ASSERT_OK( - sa_internal.InitKerberos("service_name/[email protected]", "/etc/hosts")); + ASSERT_OK(sa_internal.InitKerberos("service_name/[email protected]")); SecureAuthProvider sa_external(false); - ASSERT_OK( - sa_external.InitKerberos("service_name/[email protected]", "/etc/hosts")); + ASSERT_OK(sa_external.InitKerberos("service_name/[email protected]")); } // Test principal with slash in hostname TEST(Auth, InternalPrincipalWithSlash) { SecureAuthProvider sa(false); // false means it's external - ASSERT_OK(sa.InitKerberos("service_name/local\\/[email protected]", "/etc/hosts")); + ASSERT_OK(sa.InitKerberos("service_name/local\\/[email protected]")); ASSERT_OK(sa.Start()); ASSERT_EQ("service_name", sa.service_name()); ASSERT_EQ("local/host", sa.hostname()); @@ -197,9 +195,9 @@ TEST(Auth, InternalPrincipalWithSlash) { // Test bad principal format exception TEST(Auth, BadPrincipalFormat) { SecureAuthProvider sa(false); // false means it's external - EXPECT_ERROR(sa.InitKerberos("", "/etc/hosts"), 2); - EXPECT_ERROR(sa.InitKerberos("[email protected]", "/etc/hosts"), 2); - EXPECT_ERROR(sa.InitKerberos("service_name/localhost", "/etc/hosts"), 2); + EXPECT_ERROR(sa.InitKerberos(""), 2); + EXPECT_ERROR(sa.InitKerberos("[email protected]"), 2); + EXPECT_ERROR(sa.InitKerberos("service_name/localhost"), 2); } } diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc index 9fde648..d126c6e 100644 --- a/be/src/rpc/authentication.cc +++ b/be/src/rpc/authentication.cc @@ -74,6 +74,8 @@ using namespace apache::thrift::transport; using namespace boost::filesystem; // for is_regular(), is_absolute() using namespace strings; +DECLARE_bool(skip_external_kerberos_auth); +DECLARE_bool(skip_internal_kerberos_auth); DECLARE_string(keytab_file); DECLARE_string(principal); DECLARE_string(be_principal); @@ -698,7 +700,7 @@ Status InitAuth(const string& appname) { // Other than the general callbacks, we only setup other SASL things as required. if (FLAGS_enable_ldap_auth || IsKerberosEnabled()) { - if (!FLAGS_principal.empty()) { + if (IsKerberosEnabled()) { // Callbacks for when we're a Kerberos Sasl internal connection. Just do logging. KERB_INT_CALLBACKS.resize(3); @@ -813,13 +815,8 @@ Status CheckReplayCacheDirPermissions() { return Status::OK(); } -Status SecureAuthProvider::InitKerberos( - const string& principal, const string& keytab_file) { +Status SecureAuthProvider::InitKerberos(const string& principal) { principal_ = principal; - keytab_file_ = keytab_file; - // The logic here is that needs_kinit_ is false unless we are the internal - // auth provider and we support kerberos. - needs_kinit_ = is_internal_; RETURN_IF_ERROR(ParseKerberosPrincipal( principal_, &service_name_, &hostname_, &realm_)); @@ -947,19 +944,6 @@ Status AuthManager::InitKerberosEnv() { } Status SecureAuthProvider::Start() { - // True for kerberos internal use - if (needs_kinit_) { - DCHECK(is_internal_); - DCHECK(!principal_.empty()); - // IMPALA-8154: Disable any Kerberos auth_to_local mappings. - FLAGS_use_system_auth_to_local = false; - // 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_, - FLAGS_krb5_ccname, false), "Could not init kerberos"); - LOG(INFO) << "Kerberos ticket granted to " << principal_; - } - if (has_ldap_) { DCHECK(!is_internal_); if (!FLAGS_ldap_ca_certificate.empty()) { @@ -1244,47 +1228,53 @@ Status AuthManager::Init() { // for clients that are external - that is, they aren't daemons - like the // impala shell, odbc, jdbc, etc. // + // Note that Kerberos and LDAP are enabled when --principal and --enable_ldap_auth are + // set, respectively. + // // Flags | Internal | External // --------- | -------- | -------- // None | NoAuth | NoAuth // LDAP only | NoAuth | Sasl(ldap) // Kerb only | Sasl(be) | Sasl(fe) // Both | Sasl(be) | Sasl(fe+ldap) - + // + // --skip_internal_kerberos_auth and --skip_external_kerberos_auth disable Kerberos + // auth for the Internal and External columns respectively. + // // Set up the internal auth provider as per above. Since there's no LDAP on // the client side, this is just a check for the "back end" kerberos // principal. - bool use_kerberos = IsKerberosEnabled(); // When acting as a client, or as a server on internal connections: string kerberos_internal_principal; // When acting as a server on external connections: string kerberos_external_principal; - if (use_kerberos) { + if (IsKerberosEnabled()) { RETURN_IF_ERROR(GetInternalKerberosPrincipal(&kerberos_internal_principal)); RETURN_IF_ERROR(GetExternalKerberosPrincipal(&kerberos_external_principal)); DCHECK(!kerberos_internal_principal.empty()); DCHECK(!kerberos_external_principal.empty()); + } + if (IsInternalKerberosEnabled()) { + // Initialize the auth provider first, in case validation of the principal fails. SecureAuthProvider* sap = NULL; internal_auth_provider_.reset(sap = new SecureAuthProvider(true)); - RETURN_IF_ERROR(sap->InitKerberos(kerberos_internal_principal, - FLAGS_keytab_file)); + RETURN_IF_ERROR(sap->InitKerberos(kerberos_internal_principal)); LOG(INFO) << "Internal communication is authenticated with Kerberos"; } else { internal_auth_provider_.reset(new NoAuthProvider()); LOG(INFO) << "Internal communication is not authenticated"; } - RETURN_IF_ERROR(internal_auth_provider_->Start()); + bool external_kerberos_enabled = IsExternalKerberosEnabled(); // Set up the external auth provider as per above. Either a "front end" // principal or ldap tells us to use a SecureAuthProvider, and we fill in // details from there. - if (use_ldap || use_kerberos) { + if (use_ldap || external_kerberos_enabled) { SecureAuthProvider* sap = NULL; external_auth_provider_.reset(sap = new SecureAuthProvider(false)); - if (use_kerberos) { - RETURN_IF_ERROR(sap->InitKerberos(kerberos_external_principal, - FLAGS_keytab_file)); + if (external_kerberos_enabled) { + RETURN_IF_ERROR(sap->InitKerberos(kerberos_external_principal)); LOG(INFO) << "External communication is authenticated with Kerberos"; } if (use_ldap) { @@ -1295,8 +1285,27 @@ Status AuthManager::Init() { external_auth_provider_.reset(new NoAuthProvider()); LOG(INFO) << "External communication is not authenticated"; } - RETURN_IF_ERROR(external_auth_provider_->Start()); + // Acquire a kerberos ticket and start the background renewal thread before starting + // the auth providers. Do this after the InitKerberos() calls above which validate the + // principal format so that we don't try to do anything before the flags have been + // validated. + if (IsKerberosEnabled()) { + // IMPALA-8154: Disable any Kerberos auth_to_local mappings. + FLAGS_use_system_auth_to_local = false; + // Starts a thread that periodically does a 'kinit'. The thread lives as long as the + // process does. We only need to kinit as the internal principal, because that is the + // identity we will use for authentication with both other Impala services (impalad, + // statestore, catalogd) and other services lower in the stack (HMS, HDFS, Kudu, etc). + // We do not need a TGT for the external principal because we do not create any + // connections using that principal. + KUDU_RETURN_IF_ERROR(kudu::security::InitKerberosForServer( + kerberos_internal_principal, FLAGS_keytab_file, + FLAGS_krb5_ccname, false), "Could not init kerberos"); + LOG(INFO) << "Kerberos ticket granted to " << kerberos_internal_principal; + } + RETURN_IF_ERROR(internal_auth_provider_->Start()); + RETURN_IF_ERROR(external_auth_provider_->Start()); return Status::OK(); } diff --git a/be/src/rpc/rpc-mgr-kerberized-test.cc b/be/src/rpc/rpc-mgr-kerberized-test.cc index 61fd8e3..f8df52a 100644 --- a/be/src/rpc/rpc-mgr-kerberized-test.cc +++ b/be/src/rpc/rpc-mgr-kerberized-test.cc @@ -27,6 +27,8 @@ #include "util/filesystem-util.h" #include "util/scope-exit-trigger.h" +DECLARE_bool(skip_internal_kerberos_auth); +DECLARE_bool(skip_external_kerberos_auth); DECLARE_string(be_principal); DECLARE_string(hostname); DECLARE_string(keytab_file); @@ -215,6 +217,67 @@ TEST_F(RpcMgrKerberizedTest, DisabledKerberosConfigs) { jvm_flags, "-Djava.security.krb5.conf=/tmp/DisabledKerberosConfigsConf"); } +// Test that we kinit even with --skip_internal_kerberos_auth and +// --skip_external_kerberos_auth set. We do this indirectly by checking for +// kinit success/failure. +TEST_F(RpcMgrKerberizedTest, KinitWhenIncomingAuthDisabled) { + auto ia = + ScopedFlagSetter<bool>::Make(&FLAGS_skip_internal_kerberos_auth, true); + auto ea = + ScopedFlagSetter<bool>::Make(&FLAGS_skip_external_kerberos_auth, true); + EXPECT_OK(InitAuth(CURRENT_EXECUTABLE_PATH)); + + FLAGS_principal = FLAGS_be_principal = "non-existent-principal/host@realm"; + // Kinit should fail because of principal not in keytab. + Status status = InitAuth(CURRENT_EXECUTABLE_PATH); + EXPECT_FALSE(status.ok()); + EXPECT_STR_CONTAINS(status.GetDetail(), "Could not init kerberos: Runtime error: " + "unable to kinit: unable to login from keytab: Keytab contains no suitable keys " + "for non-existent-principal/host@realm"); + + FLAGS_principal = FLAGS_be_principal = "MALFORMEDPRINCIPAL//@"; + // We should fail before attempting kinit because of malformed principal. + status = InitAuth(CURRENT_EXECUTABLE_PATH); + EXPECT_FALSE(status.ok()); + EXPECT_STR_CONTAINS(status.GetDetail(), "Could not init kerberos: Runtime error: " + "unable to kinit: unable to login from keytab:"); +} + +// This test confirms that auth is bypassed on KRPC services when +// --skip_external_kerberos_auth=true +TEST_F(RpcMgrKerberizedTest, InternalAuthorizationSkip) { + auto ia = + ScopedFlagSetter<bool>::Make(&FLAGS_skip_internal_kerberos_auth, true); + GeneratedServiceIf* ping_impl = + TakeOverService(make_unique<PingServiceImpl>(&rpc_mgr_)); + const int num_service_threads = 10; + const int queue_size = 10; + ASSERT_OK(rpc_mgr_.RegisterService(num_service_threads, queue_size, ping_impl, + static_cast<PingServiceImpl*>(ping_impl)->mem_tracker())); + FLAGS_num_acceptor_threads = 2; + FLAGS_num_reactor_threads = 10; + ASSERT_OK(rpc_mgr_.StartServices()); + + // Switch over to a credentials cache which only contains the dummy credential. + // Kinit done in InitAuth() uses a different credentials cache. + DCHECK_NE(FLAGS_krb5_ccname, kdc_ccname); + discard_result(setenv("KRB5CCNAME", kdc_ccname.c_str(), 1)); + + RpcController controller; + Status rpc_status; + + // PingService would expect FLAGS_be_principal as principal name, which + // we don't have in our dummy credential cache. So this only succeeds if + // auth is disabled. + unique_ptr<PingServiceProxy> ping_proxy; + ASSERT_OK(static_cast<PingServiceImpl*>(ping_impl)->GetProxy(krpc_address_, + FLAGS_hostname, &ping_proxy)); + PingRequestPB ping_request; + PingResponsePB ping_response; + controller.Reset(); + EXPECT_OK(FromKuduStatus(ping_proxy->Ping(ping_request, &ping_response, &controller))); +} + } // namespace impala using impala::Status; diff --git a/be/src/rpc/rpc-mgr.cc b/be/src/rpc/rpc-mgr.cc index 1e16f67..3e112a5 100644 --- a/be/src/rpc/rpc-mgr.cc +++ b/be/src/rpc/rpc-mgr.cc @@ -124,7 +124,7 @@ Status RpcMgr::Init(const TNetworkAddress& address) { // connections is also racy and leads to query failures. bld.set_connection_keepalive_time(MonoDelta::FromMilliseconds(-1)); - if (IsKerberosEnabled()) { + if (IsInternalKerberosEnabled()) { string internal_principal; RETURN_IF_ERROR(GetInternalKerberosPrincipal(&internal_principal)); string service_name, unused_hostname, unused_realm; @@ -174,7 +174,7 @@ Status RpcMgr::RegisterService(int32_t num_service_threads, int32_t service_queu bool RpcMgr::Authorize(const string& service_name, RpcContext* context, MemTracker* mem_tracker) const { // Authorization is enforced iff Kerberos is enabled. - if (!IsKerberosEnabled()) return true; + if (!IsInternalKerberosEnabled()) return true; // Check if the mapped username matches that of the kinit'ed principal. const RemoteUser& remote_user = context->remote_user(); diff --git a/be/src/util/auth-util.h b/be/src/util/auth-util.h index f7dcd86..45c9620 100644 --- a/be/src/util/auth-util.h +++ b/be/src/util/auth-util.h @@ -24,6 +24,8 @@ #include "gutil/strings/substitute.h" #include "service/impala-server.h" +DECLARE_bool(skip_external_kerberos_auth); +DECLARE_bool(skip_internal_kerberos_auth); DECLARE_string(principal); namespace impala { @@ -70,4 +72,14 @@ Status ParseKerberosPrincipal(const std::string& principal, std::string* service inline bool IsKerberosEnabled() { return !FLAGS_principal.empty(); } + +/// Returns true if kerberos is enabled for incoming connections on internal services. +inline bool IsInternalKerberosEnabled() { + return IsKerberosEnabled() && !FLAGS_skip_internal_kerberos_auth; +} + +/// Returns true if kerberos is enabled for incoming connections on external services. +inline bool IsExternalKerberosEnabled() { + return IsKerberosEnabled() && !FLAGS_skip_external_kerberos_auth; +} } // namespace impala
