You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by tm...@apache.org on 2019/10/30 01:47:34 UTC
[impala] 02/03: IMPALA-7504/KUDU-2979 ParseKerberosPrincipal()
should use krb5_parse_name() instead
This is an automated email from the ASF dual-hosted git repository.
tmarshall pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit bb1229411c2b19e1258450e6dc2003f9676ee236
Author: xiaomeng <xi...@cloudera.com>
AuthorDate: Mon Oct 14 14:49:14 2019 -0700
IMPALA-7504/KUDU-2979 ParseKerberosPrincipal() should use krb5_parse_name() instead
We want to use krb5_parse_name() to parse the principal instead of
using custom code.
When kerberos is initialized in Impala's copy of Kudu code, it stores a
global context which is used when accessing the Krb5 library. To use
this global context the code that parses the principal name is moved
into the Impala Kudu code. This new code is then called from the
existing ParseKerberosPrincipal method.
Test done:
Add two tests to authentication-test, one to verify parsing a principal
containing a special character. The other to verify exception when
parsing bad format principal, new error code is 2 instead of original 112
which is BAD_PRINCIPAL_FORMAT error code.
Run impala-private-parameterized tests.
Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Reviewed-on: http://gerrit.cloudera.org:8080/14520
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Reviewed-on: http://gerrit.cloudera.org:8080/14433
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/src/kudu/security/init.cc | 18 ++++++++++++++++++
be/src/kudu/security/init.h | 6 ++++++
be/src/kudu/security/test/mini_kdc-test.cc | 16 ++++++++++++++++
be/src/rpc/authentication-test.cc | 17 +++++++++++++++++
be/src/util/auth-util.cc | 19 ++++---------------
5 files changed, 61 insertions(+), 15 deletions(-)
diff --git a/be/src/kudu/security/init.cc b/be/src/kudu/security/init.cc
index 200411f..d0b2226 100644
--- a/be/src/kudu/security/init.cc
+++ b/be/src/kudu/security/init.cc
@@ -452,6 +452,24 @@ boost::optional<string> GetLoggedInUsernameFromKeytab() {
return g_kinit_ctx->username_str();
}
+Status Krb5ParseName(const string& principal, string* service_name,
+ string* hostname, string* realm) {
+ krb5_principal princ;
+ KRB5_RETURN_NOT_OK_PREPEND(krb5_parse_name(g_krb5_ctx, principal.c_str(), &princ),
+ "could not parse principal");
+ SCOPED_CLEANUP({
+ krb5_free_principal(g_krb5_ctx, princ);
+ });
+ if (princ->length != 2) {
+ return Status::InvalidArgument(Substitute("$0: principal should include "
+ "service name, hostname and realm", principal));
+ }
+ realm->assign(princ->realm.data, princ->realm.length);
+ service_name->assign(princ->data[0].data, princ->data[0].length);
+ hostname->assign(princ->data[1].data, princ->data[1].length);
+ return Status::OK();
+}
+
Status InitKerberosForServer(const std::string& raw_principal, const std::string& keytab_file,
const std::string& krb5ccname, bool disable_krb5_replay_cache) {
if (keytab_file.empty()) return Status::OK();
diff --git a/be/src/kudu/security/init.h b/be/src/kudu/security/init.h
index 8b1519a..80074b3 100644
--- a/be/src/kudu/security/init.h
+++ b/be/src/kudu/security/init.h
@@ -35,6 +35,12 @@ namespace security {
// pick up credentials from test cases or any other daemon.
static const std::string kKrb5CCName = "MEMORY:kudu";
+// Parses the given Kerberos principal into service name, hostname, and realm.
+Status Krb5ParseName(const std::string& principal,
+ std::string* service_name,
+ std::string* hostname,
+ std::string* realm);
+
// Initializes Kerberos for a server. In particular, this processes
// the '--keytab_file' command line flag.
// 'raw_principal' is the principal to Kinit with after calling GetConfiguredPrincipal()
diff --git a/be/src/kudu/security/test/mini_kdc-test.cc b/be/src/kudu/security/test/mini_kdc-test.cc
index e0ba455..ec137eb 100644
--- a/be/src/kudu/security/test/mini_kdc-test.cc
+++ b/be/src/kudu/security/test/mini_kdc-test.cc
@@ -75,6 +75,22 @@ TEST_F(MiniKdcTest, TestBasicOperation) {
ASSERT_OK(security::InitKerberosForServer(kSPN, kt_path));
ASSERT_EQ("kudu/foo.example.com@KRBTEST.COM", *security::GetLoggedInPrincipalFromKeytab());
+ // Test parse krb5 principal.
+ string service_name;
+ string hostname;
+ string realm;
+ for (const auto& principal : { "kudu/foo.example.com@KRBTEST.COM", "kudu/foo.example.com" }) {
+ ASSERT_OK(security::Krb5ParseName(principal, &service_name, &hostname, &realm));
+ ASSERT_EQ("kudu", service_name);
+ ASSERT_EQ("foo.example.com", hostname);
+ ASSERT_EQ("KRBTEST.COM", realm);
+ }
+
+ // Test bad format principal.
+ ASSERT_TRUE(security::Krb5ParseName("", &service_name, &hostname, &realm).IsInvalidArgument());
+ ASSERT_TRUE(security::Krb5ParseName("kudu@KRBTEST.COM", &service_name,
+ &hostname, &realm).IsInvalidArgument());
+
// Test principal canonicalization.
string princ = "foo";
ASSERT_OK(security::CanonicalizeKrb5Principal(&princ));
diff --git a/be/src/rpc/authentication-test.cc b/be/src/rpc/authentication-test.cc
index 7b5198e..52c5833 100644
--- a/be/src/rpc/authentication-test.cc
+++ b/be/src/rpc/authentication-test.cc
@@ -184,8 +184,25 @@ TEST(Auth, KerbAndSslEnabled) {
sa_external.InitKerberos("service_name/_HOST@some.realm", "/etc/hosts"));
}
+// Test principal with slash in hostname
+TEST(Auth, InternalPrincipalWithSlash) {
+ SecureAuthProvider sa(false); // false means it's external
+ ASSERT_OK(sa.InitKerberos("service_name/local\\/host@some.realm", "/etc/hosts"));
+ ASSERT_OK(sa.Start());
+ ASSERT_EQ("service_name", sa.service_name());
+ ASSERT_EQ("local/host", sa.hostname());
+ ASSERT_EQ("some.realm", sa.realm());
}
+// 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("service_name@some.realm", "/etc/hosts"), 2);
+ EXPECT_ERROR(sa.InitKerberos("service_name/localhost", "/etc/hosts"), 2);
+}
+
+}
int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);
impala::InitCommonRuntime(argc, argv, true, impala::TestInfo::BE_TEST);
diff --git a/be/src/util/auth-util.cc b/be/src/util/auth-util.cc
index 218c211..3085156 100644
--- a/be/src/util/auth-util.cc
+++ b/be/src/util/auth-util.cc
@@ -22,6 +22,8 @@
#include "service/client-request-state.h"
#include "util/network-util.h"
#include "gen-cpp/ImpalaInternalService_types.h"
+#include "kudu/security/init.h"
+#include "exec/kudu-util.h"
using namespace std;
using boost::algorithm::is_any_of;
@@ -86,21 +88,8 @@ Status GetInternalKerberosPrincipal(string* out_principal) {
Status ParseKerberosPrincipal(const string& principal, string* service_name,
string* hostname, string* realm) {
- // TODO: IMPALA-7504: replace this with krb5_parse_name().
- vector<string> names;
-
- split(names, principal, is_any_of("/"));
- if (names.size() != 2) return Status(TErrorCode::BAD_PRINCIPAL_FORMAT, principal);
-
- *service_name = names[0];
-
- string remaining_principal = names[1];
- split(names, remaining_principal, is_any_of("@"));
- if (names.size() != 2) return Status(TErrorCode::BAD_PRINCIPAL_FORMAT, principal);
-
- *hostname = names[0];
- *realm = names[1];
-
+ KUDU_RETURN_IF_ERROR(kudu::security::Krb5ParseName(principal, service_name,
+ hostname, realm), strings::Substitute("bad principal format $0", principal));
return Status::OK();
}