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 e858aba5d21793f134ff25da74a30992395fda62 Author: Tim Armstrong <[email protected]> AuthorDate: Mon Mar 2 16:47:32 2020 -0800 IMPALA-9430: always pass through kerberos configs The behaviour of kerberos-related command line flags is changed so that their values are always passed through to underlying libraries, even if Kerberos isn't enabled for internal communication in Impala. This is good because: * Various libraries that communicate with external systems may use kerberos for outgoing connections, if *incoming* connections are not authenticated. e.g. it might just be enabled for HMS. Having them pick up different kerberos settings for outgoing connections if kerberos is disabled for incoming connections is a little weird. This is a safer default that reduces chances of inadvertant misconfigurations. * It matches the documentation of the flags. Some validations are still disabled when --principal is not set, e.g. we don't check the replay cache directory. This is to avoid any potential regressions or startup failures on non-kerberised clusters. Testing: Added unit tests for flag validation and env var setting on the code paths that I touched. Change-Id: If4bb311c7ab7173232aab36c5ed801f93f38f5b9 Reviewed-on: http://gerrit.cloudera.org:8080/15340 Reviewed-by: Impala Public Jenkins <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- be/src/common/global-flags.cc | 8 +++- be/src/rpc/authentication.cc | 88 +++++++++++++++++++---------------- be/src/rpc/authentication.h | 5 +- be/src/rpc/rpc-mgr-kerberized-test.cc | 67 ++++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 44 deletions(-) diff --git a/be/src/common/global-flags.cc b/be/src/common/global-flags.cc index dea6e27..fb65bf3 100644 --- a/be/src/common/global-flags.cc +++ b/be/src/common/global-flags.cc @@ -42,8 +42,12 @@ DEFINE_int32(krpc_port, 27000, // Kerberos is enabled if and only if principal is set. DEFINE_string(principal, "", "Kerberos principal. If set, both client and backend " - "network connections will use Kerberos encryption and authentication. Kerberos will " - "not be used for internal or external connections if this is not set."); + "network connections will use Kerberos encryption and authentication and the daemon " + "will acquire a Kerberos TGT (i.e. do the equivalent of the kinit command) and keep " + "it refreshed for the lifetime of the daemon. If this is not set the TGT ticket " + "will not be acquired and incoming connections will not be authenticated or " + "encrypted using Kerberos. However, the TGT and other settings may be inherited from " + "the environment and used by client libraries in certain cases."); DEFINE_string(be_principal, "", "Kerberos principal for backend network connections only," "overriding --principal if set. Must not be set if --principal is not set."); DEFINE_string(keytab_file, "", "Absolute path to Kerberos keytab file"); diff --git a/be/src/rpc/authentication.cc b/be/src/rpc/authentication.cc index af2808c..9fde648 100644 --- a/be/src/rpc/authentication.cc +++ b/be/src/rpc/authentication.cc @@ -793,6 +793,7 @@ Status InitAuth(const string& appname) { // to debug. We do this using direct stat() calls because boost doesn't support the // detail we need. Status CheckReplayCacheDirPermissions() { + DCHECK(IsKerberosEnabled()); struct stat st; if (stat("/var/tmp", &st) < 0) { @@ -864,33 +865,43 @@ static Status EnvAppend(const string& attr, const string& thing, const string& t } Status AuthManager::InitKerberosEnv() { - DCHECK(!FLAGS_principal.empty()); - - RETURN_IF_ERROR(CheckReplayCacheDirPermissions()); - - if (!is_regular(FLAGS_keytab_file)) { - return Status(Substitute("Bad --keytab_file value: The file $0 is not a " - "regular file", FLAGS_keytab_file)); + if (IsKerberosEnabled()) { + RETURN_IF_ERROR(CheckReplayCacheDirPermissions()); + if (FLAGS_keytab_file.empty()) { + return Status("--keytab_file must be configured if kerberos is enabled"); + } + if (FLAGS_krb5_ccname.empty()) { + return Status("--krb5_ccname must be configured if kerberos is enabled"); + } } - // Set the keytab name in the environment so that Sasl Kerberos and kinit can - // find and use it. - if (setenv("KRB5_KTNAME", FLAGS_keytab_file.c_str(), 1)) { - return Status(Substitute("Kerberos could not set KRB5_KTNAME: $0", - GetStrErrMsg())); + if (!FLAGS_keytab_file.empty()) { + if (!is_regular(FLAGS_keytab_file)) { + return Status(Substitute("Bad --keytab_file value: The file $0 is not a " + "regular file", FLAGS_keytab_file)); + } + + // Set the keytab name in the environment so that Sasl Kerberos and kinit can + // find and use it. + if (setenv("KRB5_KTNAME", FLAGS_keytab_file.c_str(), 1)) { + return Status(Substitute("Kerberos could not set KRB5_KTNAME: $0", + GetStrErrMsg())); + } } - // We want to set a custom location for the impala credential cache. - // Usually, it's /tmp/krb5cc_xxx where xxx is the UID of the process. This - // is normally fine, but if you're not running impala daemons as user - // 'impala', the kinit we perform is going to blow away credentials for the - // current user. Not setting this isn't technically fatal, so ignore errors. - const path krb5_ccname_path(FLAGS_krb5_ccname); - if (!krb5_ccname_path.is_absolute()) { - return Status(Substitute("Bad --krb5_ccname value: $0 is not an absolute file path", - FLAGS_krb5_ccname)); + if (!FLAGS_krb5_ccname.empty()) { + // We want to set a custom location for the impala credential cache. + // Usually, it's /tmp/krb5cc_xxx where xxx is the UID of the process. This + // is normally fine, but if you're not running impala daemons as user + // 'impala', the kinit we perform is going to blow away credentials for the + // current user. Not setting this isn't technically fatal, so ignore errors. + const path krb5_ccname_path(FLAGS_krb5_ccname); + if (!krb5_ccname_path.is_absolute()) { + return Status(Substitute("Bad --krb5_ccname value: $0 is not an absolute file path", + FLAGS_krb5_ccname)); + } + discard_result(setenv("KRB5CCNAME", FLAGS_krb5_ccname.c_str(), 1)); } - discard_result(setenv("KRB5CCNAME", FLAGS_krb5_ccname.c_str(), 1)); // If an alternate krb5_conf location is supplied, set both KRB5_CONFIG and // JAVA_TOOL_OPTIONS in the environment. @@ -918,13 +929,13 @@ Status AuthManager::InitKerberosEnv() { if (!FLAGS_krb5_debug_file.empty()) { bool krb5_debug_fail = false; if (setenv("KRB5_TRACE", FLAGS_krb5_debug_file.c_str(), 1) < 0) { - LOG(WARNING) << "Failed to set KRB5_TRACE; --krb5_debuf_file not enabled for " - "back-end code"; + LOG(WARNING) << "Failed to set KRB5_TRACE; --krb5_debug_file not enabled for " + "back-end code"; krb5_debug_fail = true; } if (!EnvAppend("JAVA_TOOL_OPTIONS", "sun.security.krb5.debug", "true").ok()) { - LOG(WARNING) << "Failed to set JAVA_TOOL_OPTIONS; --krb5_debuf_file not enabled " - "for front-end code"; + LOG(WARNING) << "Failed to set JAVA_TOOL_OPTIONS; --krb5_debug_file not enabled " + "for front-end code"; krb5_debug_fail = true; } if (!krb5_debug_fail) { @@ -1225,20 +1236,7 @@ Status AuthManager::Init() { "is used in internal (back-end) communication."); } - // 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; - - bool use_kerberos = IsKerberosEnabled(); - if (use_kerberos) { - RETURN_IF_ERROR(GetInternalKerberosPrincipal(&kerberos_internal_principal)); - RETURN_IF_ERROR(GetExternalKerberosPrincipal(&kerberos_external_principal)); - DCHECK(!kerberos_internal_principal.empty()); - DCHECK(!kerberos_external_principal.empty()); - - RETURN_IF_ERROR(InitKerberosEnv()); - } + RETURN_IF_ERROR(InitKerberosEnv()); // This is written from the perspective of the daemons - thus "internal" // means "I am used for communication with other daemons, both as a client @@ -1256,7 +1254,17 @@ Status AuthManager::Init() { // 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) { + RETURN_IF_ERROR(GetInternalKerberosPrincipal(&kerberos_internal_principal)); + RETURN_IF_ERROR(GetExternalKerberosPrincipal(&kerberos_external_principal)); + DCHECK(!kerberos_internal_principal.empty()); + DCHECK(!kerberos_external_principal.empty()); + SecureAuthProvider* sap = NULL; internal_auth_provider_.reset(sap = new SecureAuthProvider(true)); RETURN_IF_ERROR(sap->InitKerberos(kerberos_internal_principal, diff --git a/be/src/rpc/authentication.h b/be/src/rpc/authentication.h index 5bc3138..7b65233 100644 --- a/be/src/rpc/authentication.h +++ b/be/src/rpc/authentication.h @@ -62,8 +62,9 @@ class AuthManager { AuthProvider* GetInternalAuthProvider(); private: - /// One-time kerberos-specific environment variable setup. Called by Init() if Kerberos - /// is enabled. + /// One-time kerberos-specific environment variable setup. Sets variables like + /// KRB5CCNAME and friends so that command-line flags take effect in the C++ and + /// Java Kerberos implementations. Called by Init() whether or not Kerberos is enabled. Status InitKerberosEnv(); static AuthManager* auth_manager_; diff --git a/be/src/rpc/rpc-mgr-kerberized-test.cc b/be/src/rpc/rpc-mgr-kerberized-test.cc index 62b15e4..61fd8e3 100644 --- a/be/src/rpc/rpc-mgr-kerberized-test.cc +++ b/be/src/rpc/rpc-mgr-kerberized-test.cc @@ -17,15 +17,22 @@ #include "rpc/rpc-mgr-test.h" +#include <boost/filesystem/operations.hpp> + #include "exec/kudu-util.h" #include "rpc/auth-provider.h" #include "service/fe-support.h" #include "testutil/mini-kdc-wrapper.h" +#include "testutil/scoped-flag-setter.h" +#include "util/filesystem-util.h" +#include "util/scope-exit-trigger.h" DECLARE_string(be_principal); DECLARE_string(hostname); DECLARE_string(keytab_file); DECLARE_string(krb5_ccname); +DECLARE_string(krb5_conf); +DECLARE_string(krb5_debug_file); DECLARE_string(principal); DECLARE_string(ssl_client_ca_certificate); DECLARE_string(ssl_server_certificate); @@ -146,6 +153,66 @@ TEST_F(RpcMgrKerberizedTest, BadCredentialsCachePath) { ASSERT_TRUE(!status.ok()); EXPECT_EQ(status.GetDetail(), "Bad --krb5_ccname value: ~/foo is not an absolute file path\n"); + + FLAGS_krb5_ccname = ""; + status = InitAuth(CURRENT_EXECUTABLE_PATH); + ASSERT_TRUE(!status.ok()); + EXPECT_EQ( + status.GetDetail(), "--krb5_ccname must be configured if kerberos is enabled\n"); +} + +// Test cases in which bad keytab path is specified. +TEST_F(RpcMgrKerberizedTest, BadKeytabPath) { + FLAGS_keytab_file = "non_existent_file_for_testing"; + Status status = InitAuth(CURRENT_EXECUTABLE_PATH); + ASSERT_TRUE(!status.ok()); + EXPECT_EQ(status.GetDetail(), + "Bad --keytab_file value: The file " + "non_existent_file_for_testing is not a regular file\n"); + + FLAGS_keytab_file = ""; + status = InitAuth(CURRENT_EXECUTABLE_PATH); + ASSERT_TRUE(!status.ok()); + EXPECT_EQ( + status.GetDetail(), "--keytab_file must be configured if kerberos is enabled\n"); +} + +// Test that configurations are passed through via env variables even if kerberos +// is disabled for internal auth (i.e. --principal is not set). +TEST_F(RpcMgrKerberizedTest, DisabledKerberosConfigs) { + // These flags are reset in Setup, so just overwrite them. + FLAGS_principal = FLAGS_be_principal = ""; + FLAGS_keytab_file = "/tmp/DisabledKerberosConfigsKeytab"; + FLAGS_krb5_ccname = "/tmp/DisabledKerberosConfigsCC"; + // These flags are not reset in Setup, so used ScopedFlagSetter. + auto k5c = ScopedFlagSetter<string>::Make( + &FLAGS_krb5_conf, "/tmp/DisabledKerberosConfigsConf"); + auto k5dbg = ScopedFlagSetter<string>::Make( + &FLAGS_krb5_debug_file, "/tmp/DisabledKerberosConfigsDebug"); + + // Unset JAVA_TOOL_OPTIONS before more gets appended to it. + EXPECT_EQ(0, setenv("JAVA_TOOL_OPTIONS", "", 1)); + + // Create dummy files to satisfy validations. + auto file_cleanup = MakeScopeExitTrigger([&]() { + boost::filesystem::remove(FLAGS_keytab_file); + boost::filesystem::remove(FLAGS_krb5_conf); + }); + EXPECT_OK(FileSystemUtil::CreateFile(FLAGS_keytab_file)); + EXPECT_OK(FileSystemUtil::CreateFile(FLAGS_krb5_conf)); + + EXPECT_OK(InitAuth(CURRENT_EXECUTABLE_PATH)); + + // Check that the above changes went into the appropriate env variables where + // they can be picked up by libkrb5 and the JVM Kerberos libraries. + EXPECT_EQ("/tmp/DisabledKerberosConfigsKeytab", string(getenv("KRB5_KTNAME"))); + EXPECT_EQ("/tmp/DisabledKerberosConfigsCC", string(getenv("KRB5CCNAME"))); + EXPECT_EQ("/tmp/DisabledKerberosConfigsConf", string(getenv("KRB5_CONFIG"))); + EXPECT_EQ("/tmp/DisabledKerberosConfigsDebug", string(getenv("KRB5_TRACE"))); + string jvm_flags = getenv("JAVA_TOOL_OPTIONS"); + EXPECT_STR_CONTAINS(jvm_flags, "-Dsun.security.krb5.debug=true"); + EXPECT_STR_CONTAINS( + jvm_flags, "-Djava.security.krb5.conf=/tmp/DisabledKerberosConfigsConf"); } } // namespace impala
