From commits-return-6718-archive-asf-public=cust-asf.ponee.io@kudu.apache.org Tue Nov 6 02:17:40 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id E27D818067A for ; Tue, 6 Nov 2018 02:17:38 +0100 (CET) Received: (qmail 70571 invoked by uid 500); 6 Nov 2018 01:17:38 -0000 Mailing-List: contact commits-help@kudu.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@kudu.apache.org Delivered-To: mailing list commits@kudu.apache.org Received: (qmail 70562 invoked by uid 99); 6 Nov 2018 01:17:38 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 06 Nov 2018 01:17:38 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 75177E0058; Tue, 6 Nov 2018 01:17:37 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit From: awong@apache.org To: commits@kudu.apache.org Date: Tue, 06 Nov 2018 01:17:38 -0000 Message-Id: In-Reply-To: References: X-Mailer: ASF-Git Admin Mailer Subject: [2/3] kudu git commit: [sentry] add AuthzProvider [sentry] add AuthzProvider This commit adds a high-level abstraction which handles authorizations on Kudu operations, called AuthzProvider. It has a default implementation which always allow any operations for any users, and a SentryAuthzProvider implementation which connects to the Sentry service for authorization metadata. AuthzProvider, along with its implementations, is placed in the master module. The idea is to decouple it from Sentry since in the future, other authorization implementations might be introduced. A follow-up commit will integrate the AuthzProvider into CatalogManager to perform authorization checks on Master RPCs. Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Reviewed-on: http://gerrit.cloudera.org:8080/11659 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin Reviewed-by: Andrew Wong Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/000f2cde Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/000f2cde Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/000f2cde Branch: refs/heads/master Commit: 000f2cde16a5d318927c9180333bb6937b04cb6e Parents: 086d8a0 Author: Hao Hao Authored: Wed Oct 10 22:29:46 2018 -0700 Committer: Hao Hao Committed: Mon Nov 5 19:21:05 2018 +0000 ---------------------------------------------------------------------- src/kudu/hms/hms_catalog.cc | 37 ++- src/kudu/master/CMakeLists.txt | 8 +- src/kudu/master/authz_provider.h | 75 +++++ src/kudu/master/default_authz_provider.h | 60 ++++ src/kudu/master/sentry_authz_provider-test.cc | 296 ++++++++++++++++++++ src/kudu/master/sentry_authz_provider.cc | 307 +++++++++++++++++++++ src/kudu/master/sentry_authz_provider.h | 100 +++++++ src/kudu/sentry/sentry-test-base.h | 83 ++++++ src/kudu/sentry/sentry_action-test.cc | 18 +- src/kudu/sentry/sentry_action.cc | 2 +- src/kudu/sentry/sentry_action.h | 2 +- src/kudu/sentry/sentry_client-test.cc | 115 +++----- src/kudu/sentry/sentry_client.cc | 4 +- src/kudu/sentry/sentry_client.h | 4 +- 14 files changed, 994 insertions(+), 117 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/hms/hms_catalog.cc ---------------------------------------------------------------------- diff --git a/src/kudu/hms/hms_catalog.cc b/src/kudu/hms/hms_catalog.cc index 589fe57..9749582 100644 --- a/src/kudu/hms/hms_catalog.cc +++ b/src/kudu/hms/hms_catalog.cc @@ -81,36 +81,31 @@ DEFINE_int32(hive_metastore_retry_count, 1, "encountering retriable failures, such as network errors."); TAG_FLAG(hive_metastore_retry_count, advanced); TAG_FLAG(hive_metastore_retry_count, experimental); -TAG_FLAG(hive_metastore_retry_count, runtime); -DEFINE_int32(hive_metastore_send_timeout, 60, +DEFINE_int32(hive_metastore_send_timeout_seconds, 60, "Configures the socket send timeout, in seconds, for Thrift " "connections to the Hive Metastore."); -TAG_FLAG(hive_metastore_send_timeout, advanced); -TAG_FLAG(hive_metastore_send_timeout, experimental); -TAG_FLAG(hive_metastore_send_timeout, runtime); +TAG_FLAG(hive_metastore_send_timeout_seconds, advanced); +TAG_FLAG(hive_metastore_send_timeout_seconds, experimental); -DEFINE_int32(hive_metastore_recv_timeout, 60, +DEFINE_int32(hive_metastore_recv_timeout_seconds, 60, "Configures the socket receive timeout, in seconds, for Thrift " "connections to the Hive Metastore."); -TAG_FLAG(hive_metastore_recv_timeout, advanced); -TAG_FLAG(hive_metastore_recv_timeout, experimental); -TAG_FLAG(hive_metastore_recv_timeout, runtime); +TAG_FLAG(hive_metastore_recv_timeout_seconds, advanced); +TAG_FLAG(hive_metastore_recv_timeout_seconds, experimental); -DEFINE_int32(hive_metastore_conn_timeout, 60, +DEFINE_int32(hive_metastore_conn_timeout_seconds, 60, "Configures the socket connect timeout, in seconds, for Thrift " "connections to the Hive Metastore."); -TAG_FLAG(hive_metastore_conn_timeout, advanced); -TAG_FLAG(hive_metastore_conn_timeout, experimental); -TAG_FLAG(hive_metastore_conn_timeout, runtime); +TAG_FLAG(hive_metastore_conn_timeout_seconds, advanced); +TAG_FLAG(hive_metastore_conn_timeout_seconds, experimental); -DEFINE_int32(hive_metastore_max_message_size, 100 * 1024 * 1024, +DEFINE_int32(hive_metastore_max_message_size_bytes, 100 * 1024 * 1024, "Maximum size of Hive Metastore objects that can be received by the " "HMS client in bytes. Should match the metastore.server.max.message.size " "configuration."); -TAG_FLAG(hive_metastore_max_message_size, advanced); -TAG_FLAG(hive_metastore_max_message_size, experimental); -TAG_FLAG(hive_metastore_max_message_size, runtime); +TAG_FLAG(hive_metastore_max_message_size_bytes, advanced); +TAG_FLAG(hive_metastore_max_message_size_bytes, experimental); namespace kudu { namespace hms { @@ -128,12 +123,12 @@ Status HmsCatalog::Start() { RETURN_NOT_OK(ParseUris(FLAGS_hive_metastore_uris, &addresses)); thrift::ClientOptions options; - options.send_timeout = MonoDelta::FromSeconds(FLAGS_hive_metastore_send_timeout); - options.recv_timeout = MonoDelta::FromSeconds(FLAGS_hive_metastore_recv_timeout); - options.conn_timeout = MonoDelta::FromSeconds(FLAGS_hive_metastore_conn_timeout); + options.send_timeout = MonoDelta::FromSeconds(FLAGS_hive_metastore_send_timeout_seconds); + options.recv_timeout = MonoDelta::FromSeconds(FLAGS_hive_metastore_recv_timeout_seconds); + options.conn_timeout = MonoDelta::FromSeconds(FLAGS_hive_metastore_conn_timeout_seconds); options.enable_kerberos = FLAGS_hive_metastore_sasl_enabled; options.service_principal = FLAGS_hive_metastore_kerberos_principal; - options.max_buf_size = FLAGS_hive_metastore_max_message_size; + options.max_buf_size = FLAGS_hive_metastore_max_message_size_bytes; options.retry_count = FLAGS_hive_metastore_retry_count; return ha_client_.Start(std::move(addresses), std::move(options)); http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/master/CMakeLists.txt ---------------------------------------------------------------------- diff --git a/src/kudu/master/CMakeLists.txt b/src/kudu/master/CMakeLists.txt index 1333eaf..9b536b7 100644 --- a/src/kudu/master/CMakeLists.txt +++ b/src/kudu/master/CMakeLists.txt @@ -42,6 +42,7 @@ set(MASTER_SRCS master_service.cc mini_master.cc placement_policy.cc + sentry_authz_provider.cc sys_catalog.cc ts_descriptor.cc ts_manager.cc) @@ -54,6 +55,8 @@ target_link_libraries(master kserver kudu_common kudu_hms + kudu_sentry + kudu_thrift kudu_util master_proto rpc_header_proto @@ -70,13 +73,16 @@ SET_KUDU_TEST_LINK_LIBS( kudu_curl_util master master_proto - mini_hms) + mini_hms + mini_kdc + mini_sentry) ADD_KUDU_TEST(catalog_manager-test) ADD_KUDU_TEST(hms_notification_log_listener-test) ADD_KUDU_TEST(master-test RESOURCE_LOCK "master-web-port") ADD_KUDU_TEST(mini_master-test RESOURCE_LOCK "master-web-port") ADD_KUDU_TEST(placement_policy-test) +ADD_KUDU_TEST(sentry_authz_provider-test) ADD_KUDU_TEST(sys_catalog-test RESOURCE_LOCK "master-web-port") ADD_KUDU_TEST(ts_descriptor-test DATA_FILES ../scripts/first_argument.sh) http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/master/authz_provider.h ---------------------------------------------------------------------- diff --git a/src/kudu/master/authz_provider.h b/src/kudu/master/authz_provider.h new file mode 100644 index 0000000..417dd55 --- /dev/null +++ b/src/kudu/master/authz_provider.h @@ -0,0 +1,75 @@ +// 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. + +#pragma once + +#include + +#include "kudu/gutil/port.h" +#include "kudu/util/status.h" + +namespace kudu { +namespace master { + +// An interface for handling authorizations on Kudu operations. +class AuthzProvider { + public: + + // Starts the AuthzProvider instance. + virtual Status Start() = 0; + + // Stops the AuthzProvider instance. + virtual void Stop() = 0; + + // Checks if the table creation is authorized for the given user. + // If the table is being created with a different owner than the user, + // then more strict privilege is required. + // + // If the operation is not authorized, returns Status::NotAuthorized(). + // Otherwise, may return other Status error codes depend on actual errors. + virtual Status AuthorizeCreateTable(const std::string& table_name, + const std::string& user, + const std::string& owner) WARN_UNUSED_RESULT = 0; + + // Checks if the table deletion is authorized for the given user. + // + // If the operation is not authorized, returns Status::NotAuthorized(). + // Otherwise, may return other Status error codes depend on actual errors. + virtual Status AuthorizeDropTable(const std::string& table_name, + const std::string& user) WARN_UNUSED_RESULT = 0; + + // Checks if the table alteration is authorized for the given user. + // + // If the operation is not authorized, returns Status::NotAuthorized(). + // Otherwise, may return other Status error codes depend on actual errors. + virtual Status AuthorizeAlterTable(const std::string& old_table, + const std::string& new_table, + const std::string& user) WARN_UNUSED_RESULT = 0; + + // Checks if retrieving metadata about the table is authorized for the + // given user. For example, when checking for table presence or locations. + // + // If the operation is not authorized, returns Status::NotAuthorized(). + // Otherwise, may return other Status error codes depend on actual errors. + virtual Status AuthorizeGetTableMetadata(const std::string& table_name, + const std::string& user) WARN_UNUSED_RESULT = 0; + + virtual ~AuthzProvider() {} +}; + +} // namespace master +} // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/master/default_authz_provider.h ---------------------------------------------------------------------- diff --git a/src/kudu/master/default_authz_provider.h b/src/kudu/master/default_authz_provider.h new file mode 100644 index 0000000..6c512f6 --- /dev/null +++ b/src/kudu/master/default_authz_provider.h @@ -0,0 +1,60 @@ +// 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. + +#pragma once + +#include + +#include "kudu/master/authz_provider.h" +#include "kudu/util/status.h" + +namespace kudu { +namespace master { + +// Default AuthzProvider which always authorizes any operations. +class DefaultAuthzProvider : public AuthzProvider { + public: + + Status Start() override WARN_UNUSED_RESULT { return Status::OK(); } + + void Stop() override {}; + + Status AuthorizeCreateTable(const std::string& /*table_name*/, + const std::string& /*user*/, + const std::string& /*owner*/) override WARN_UNUSED_RESULT { + return Status::OK(); + } + + Status AuthorizeDropTable(const std::string& /*table_name*/, + const std::string& /*user*/) override WARN_UNUSED_RESULT { + return Status::OK(); + } + + Status AuthorizeAlterTable(const std::string& /*old_table*/, + const std::string& /*new_table*/, + const std::string& /*user*/) override WARN_UNUSED_RESULT { + return Status::OK(); + } + + Status AuthorizeGetTableMetadata(const std::string& /*table_name*/, + const std::string& /*user*/) override WARN_UNUSED_RESULT { + return Status::OK(); + } +}; + +} // namespace master +} // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/master/sentry_authz_provider-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/sentry_authz_provider-test.cc b/src/kudu/master/sentry_authz_provider-test.cc new file mode 100644 index 0000000..9267048 --- /dev/null +++ b/src/kudu/master/sentry_authz_provider-test.cc @@ -0,0 +1,296 @@ +// 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. + +#include "kudu/master/sentry_authz_provider.h" + +#include +#include +#include + +#include +#include + +#include "kudu/sentry/mini_sentry.h" +#include "kudu/sentry/sentry-test-base.h" +#include "kudu/sentry/sentry_client.h" +#include "kudu/sentry/sentry_policy_service_types.h" +#include "kudu/util/net/net_util.h" +#include "kudu/util/status.h" +#include "kudu/util/test_macros.h" +#include "kudu/util/test_util.h" + +DECLARE_int32(sentry_service_recv_timeout_seconds); +DECLARE_int32(sentry_service_send_timeout_seconds); +DECLARE_string(sentry_service_rpc_addresses); +DECLARE_string(sentry_service_security_mode); +DECLARE_string(server_name); + +using sentry::TAlterSentryRoleAddGroupsRequest; +using sentry::TAlterSentryRoleAddGroupsResponse; +using sentry::TAlterSentryRoleGrantPrivilegeRequest; +using sentry::TAlterSentryRoleGrantPrivilegeResponse; +using sentry::TCreateSentryRoleRequest; +using sentry::TSentryGrantOption; +using sentry::TSentryGroup; +using sentry::TSentryPrivilege; +using std::unique_ptr; +using std::set; +using std::string; + +namespace kudu { + +using sentry::SentryTestBase; + +namespace master { + +// Class for SentryAuthzProvider tests. Parameterized by whether +// Kerberos should be enabled. +class SentryAuthzProviderTest : public SentryTestBase { + public: + const char* const kAdminUser = "test-admin"; + const char* const kUserGroup = "user"; + const char* const kTestUser = "test-user"; + + void SetUp() override { + SentryTestBase::SetUp(); + + // Configure the SentryAuthzProvider flags. + FLAGS_sentry_service_security_mode = kerberos_enabled_ ? "kerberos" : "none"; + FLAGS_sentry_service_rpc_addresses = sentry_->address().ToString(); + sentry_authz_provider_.reset(new SentryAuthzProvider()); + ASSERT_OK(sentry_authz_provider_->Start()); + } + + Status StopSentry() { + RETURN_NOT_OK(sentry_client_->Stop()); + RETURN_NOT_OK(sentry_->Stop()); + return Status::OK(); + } + + Status StartSentry() { + RETURN_NOT_OK(sentry_->Start()); + RETURN_NOT_OK(sentry_client_->Start()); + return Status::OK(); + } + + Status CreateRoleAndAddToGroups(const string& role_name, const string& group_name) { + TCreateSentryRoleRequest role_req; + role_req.__set_requestorUserName(kAdminUser); + role_req.__set_roleName(role_name); + RETURN_NOT_OK(sentry_client_->CreateRole(role_req)); + + TSentryGroup group; + group.groupName = group_name; + set groups; + groups.insert(group); + TAlterSentryRoleAddGroupsRequest group_request; + TAlterSentryRoleAddGroupsResponse group_response; + group_request.__set_requestorUserName(kAdminUser); + group_request.__set_roleName(role_name); + group_request.__set_groups(groups); + return sentry_client_->AlterRoleAddGroups(group_request, &group_response); + } + + Status AlterRoleGrantPrivilege(const string& role_name, const TSentryPrivilege& privilege) { + TAlterSentryRoleGrantPrivilegeRequest privilege_request; + TAlterSentryRoleGrantPrivilegeResponse privilege_response; + privilege_request.__set_requestorUserName(kAdminUser); + privilege_request.__set_roleName(role_name); + privilege_request.__set_privilege(privilege); + return sentry_client_->AlterRoleGrantPrivilege(privilege_request, &privilege_response); + } + + // Returns a database level TSentryPrivilege with the given database name, action + // and grant option. + TSentryPrivilege GetDatabasePrivilege(const string& db_name, + const string& action, + const TSentryGrantOption::type& grant_option) { + TSentryPrivilege privilege; + privilege.__set_privilegeScope("DATABASE"); + privilege.__set_serverName(FLAGS_server_name); + privilege.__set_dbName(db_name); + privilege.__set_action(action); + privilege.__set_grantOption(grant_option); + return privilege; + } + + // Returns a table level TSentryPrivilege with the given table name, database name, + // action and grant option. + TSentryPrivilege GetTablePrivilege(const string& db_name, + const string& table_name, + const string& action, + const TSentryGrantOption::type& grant_option) { + TSentryPrivilege privilege = GetDatabasePrivilege(db_name, action, grant_option); + privilege.__set_tableName(table_name); + return privilege; + } + + protected: + unique_ptr sentry_authz_provider_; +}; + +INSTANTIATE_TEST_CASE_P(KerberosEnabled, SentryAuthzProviderTest, ::testing::Bool()); + +TEST_P(SentryAuthzProviderTest, TestAuthorizeCreateTable) { + // Don't authorize create table on a non-existent user. + Status s = sentry_authz_provider_->AuthorizeCreateTable("db.table", + "non-existent-user", + "non-existent-user"); + ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); + + // Don't authorize create table on a user without any privileges. + s = sentry_authz_provider_->AuthorizeCreateTable("db.table", kTestUser, kTestUser); + ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); + + // Don't authorize create table on a user without required privileges. + const string role_name = "developer"; + ASSERT_OK(CreateRoleAndAddToGroups(role_name, kUserGroup)); + TSentryPrivilege privilege = GetDatabasePrivilege("db", "DROP", TSentryGrantOption::FALSE); + ASSERT_OK(AlterRoleGrantPrivilege(role_name, privilege)); + s = sentry_authz_provider_->AuthorizeCreateTable("db.table", kTestUser, kTestUser); + ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); + + // Authorize create table on a user with proper privileges. + privilege = GetDatabasePrivilege("db", "CREATE", TSentryGrantOption::FALSE); + ASSERT_OK(AlterRoleGrantPrivilege(role_name, privilege)); + ASSERT_OK(sentry_authz_provider_->AuthorizeCreateTable("db.table", kTestUser, kTestUser)); + + // Table creation with a different owner than the user + // requires the creating user have 'ALL on DATABASE' with grant. + s = sentry_authz_provider_->AuthorizeCreateTable("db.table", kTestUser, "diff-user"); + ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); + privilege = GetDatabasePrivilege("db", "ALL", TSentryGrantOption::TRUE); + ASSERT_OK(AlterRoleGrantPrivilege(role_name, privilege)); + ASSERT_OK(sentry_authz_provider_->AuthorizeCreateTable("db.table", kTestUser, "diff-user")); +} + +TEST_P(SentryAuthzProviderTest, TestAuthorizeDropTable) { + // Don't authorize delete table on a user without required privileges. + const string role_name = "developer"; + ASSERT_OK(CreateRoleAndAddToGroups(role_name, kUserGroup)); + TSentryPrivilege privilege = GetDatabasePrivilege("db", "SELECT", TSentryGrantOption::FALSE); + ASSERT_OK(AlterRoleGrantPrivilege(role_name, privilege)); + Status s = sentry_authz_provider_->AuthorizeDropTable("db.table", kTestUser); + ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); + + // Authorize delete table on a user with proper privileges. + privilege = GetDatabasePrivilege("db", "DROP", TSentryGrantOption::FALSE); + ASSERT_OK(AlterRoleGrantPrivilege(role_name, privilege)); + ASSERT_OK(sentry_authz_provider_->AuthorizeDropTable("db.table", kTestUser)); +} + +TEST_P(SentryAuthzProviderTest, TestAuthorizeAlterTable) { + // Don't authorize alter table on a user without required privileges. + const string role_name = "developer"; + ASSERT_OK(CreateRoleAndAddToGroups(role_name, kUserGroup)); + TSentryPrivilege db_privilege = GetDatabasePrivilege("db", "SELECT", TSentryGrantOption::FALSE); + ASSERT_OK(AlterRoleGrantPrivilege(role_name, db_privilege)); + Status s = sentry_authz_provider_->AuthorizeAlterTable("db.table", "db.table", kTestUser); + ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); + + // Authorize alter table without rename on a user with proper privileges. + db_privilege = GetDatabasePrivilege("db", "ALTER", TSentryGrantOption::FALSE); + ASSERT_OK(AlterRoleGrantPrivilege(role_name, db_privilege)); + ASSERT_OK(sentry_authz_provider_->AuthorizeAlterTable("db.table", "db.table", kTestUser)); + + // Table alteration with rename requires 'ALL ON TABLE ' and + // 'CREATE ON DATABASE ' + s = sentry_authz_provider_->AuthorizeAlterTable("db.table", "new_db.new_table", kTestUser); + ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); + + // Authorize alter table without rename on a user with proper privileges. + db_privilege = GetDatabasePrivilege("new_db", "CREATE", TSentryGrantOption::FALSE); + ASSERT_OK(AlterRoleGrantPrivilege(role_name, db_privilege)); + TSentryPrivilege table_privilege = GetTablePrivilege("db", "table", "ALL", + TSentryGrantOption::FALSE); + ASSERT_OK(AlterRoleGrantPrivilege(role_name, table_privilege)); + ASSERT_OK(sentry_authz_provider_->AuthorizeAlterTable("db.table", + "new_db.new_table", + kTestUser)); +} + +TEST_P(SentryAuthzProviderTest, TestAuthorizeGetTableMetadata) { + // Don't authorize delete table on a user without required privileges. + const string role_name = "developer"; + ASSERT_OK(CreateRoleAndAddToGroups(role_name, kUserGroup)); + Status s = sentry_authz_provider_->AuthorizeGetTableMetadata("db.table", kTestUser); + ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); + + // Authorize delete table on a user with proper privileges. + TSentryPrivilege privilege = GetDatabasePrivilege("db", "SELECT", TSentryGrantOption::FALSE); + ASSERT_OK(AlterRoleGrantPrivilege(role_name, privilege)); + ASSERT_OK(sentry_authz_provider_->AuthorizeGetTableMetadata("db.table", kTestUser)); +} + +// Checks that the SentryAuthzProvider handles reconnecting to Sentry after a connection failure, +// or service being too busy. +TEST_P(SentryAuthzProviderTest, TestReconnect) { + + // Restart SentryAuthzProvider with configured timeout to reduce the run time of this test. + NO_FATALS(sentry_authz_provider_->Stop()); + FLAGS_sentry_service_security_mode = kerberos_enabled_ ? "kerberos" : "none"; + FLAGS_sentry_service_rpc_addresses = sentry_->address().ToString(); + FLAGS_sentry_service_send_timeout_seconds = AllowSlowTests() ? 5 : 2; + FLAGS_sentry_service_recv_timeout_seconds = AllowSlowTests() ? 5 : 2; + sentry_authz_provider_.reset(new SentryAuthzProvider()); + ASSERT_OK(sentry_authz_provider_->Start()); + + const string role_name = "developer"; + ASSERT_OK(CreateRoleAndAddToGroups(role_name, kUserGroup)); + TSentryPrivilege privilege = GetDatabasePrivilege("db", "METADATA", TSentryGrantOption::FALSE); + ASSERT_OK(AlterRoleGrantPrivilege(role_name, privilege)); + ASSERT_OK(sentry_authz_provider_->AuthorizeGetTableMetadata("db.table", kTestUser)); + + // Shutdown Sentry and try a few operations. + ASSERT_OK(StopSentry()); + + Status s = sentry_authz_provider_->AuthorizeDropTable("db.table", kTestUser); + EXPECT_TRUE(s.IsNetworkError()) << s.ToString(); + + s = sentry_authz_provider_->AuthorizeCreateTable("db.table", kTestUser, "diff-user"); + EXPECT_TRUE(s.IsNetworkError()) << s.ToString(); + + // Start Sentry back up and ensure that the same operations succeed. + ASSERT_OK(StartSentry()); + ASSERT_EVENTUALLY([&] { + ASSERT_OK(sentry_authz_provider_->AuthorizeGetTableMetadata( + "db.table", kTestUser)); + }); + + privilege = GetDatabasePrivilege("db", "DROP", TSentryGrantOption::FALSE); + ASSERT_OK(AlterRoleGrantPrivilege(role_name, privilege)); + ASSERT_OK(sentry_authz_provider_->AuthorizeDropTable("db.table", kTestUser)); + + // Pause Sentry and try a few operations. + ASSERT_OK(sentry_->Pause()); + + s = sentry_authz_provider_->AuthorizeDropTable("db.table", kTestUser); + EXPECT_TRUE(s.IsTimedOut()) << s.ToString(); + + s = sentry_authz_provider_->AuthorizeGetTableMetadata("db.table", kTestUser); + EXPECT_TRUE(s.IsTimedOut()) << s.ToString(); + + // Resume Sentry and ensure that the same operations succeed. + ASSERT_OK(sentry_->Resume()); + ASSERT_EVENTUALLY([&] { + ASSERT_OK(sentry_authz_provider_->AuthorizeDropTable( + "db.table", kTestUser)); + }); +} + +} // namespace master +} // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/master/sentry_authz_provider.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/sentry_authz_provider.cc b/src/kudu/master/sentry_authz_provider.cc new file mode 100644 index 0000000..6a9357d --- /dev/null +++ b/src/kudu/master/sentry_authz_provider.cc @@ -0,0 +1,307 @@ +// 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. + +#include "kudu/master/sentry_authz_provider.h" + +#include +#include +#include + +#include +#include +#include + +#include "kudu/common/table_util.h" +#include "kudu/gutil/macros.h" +#include "kudu/gutil/strings/substitute.h" +#include "kudu/sentry/sentry_action.h" +#include "kudu/sentry/sentry_client.h" +#include "kudu/sentry/sentry_policy_service_types.h" +#include "kudu/thrift/client.h" +#include "kudu/util/flag_tags.h" +#include "kudu/util/monotime.h" +#include "kudu/util/net/net_util.h" +#include "kudu/util/slice.h" + +using sentry::TListSentryPrivilegesRequest; +using sentry::TListSentryPrivilegesResponse; +using sentry::TSentryAuthorizable; +using sentry::TSentryGrantOption; +using std::string; +using std::vector; + +DEFINE_string(sentry_service_rpc_addresses, "", + "Comma-separated list of RPC addresses of the Sentry service(s). When " + "set, Sentry integration is enabled, fine-grained access control is " + "enforced in the master, and clients are issued authorization tokens. " + "Must match the value of the sentry.service.client.server.rpc-addresses " + "option in the Sentry server configuration."); +TAG_FLAG(sentry_service_rpc_addresses, experimental); + +DEFINE_string(server_name, "server1", + "Configures which server namespace the Kudu instance belongs to for defining " + "server-level privileges in Sentry. Used to distinguish a particular Kudu " + "cluster in case of a multi-cluster setup. Must match the value of the " + "hive.sentry.server option in the HiveServer2 configuration, and the value " + "of the --server_name in Impala configuration."); +TAG_FLAG(server_name, experimental); + +DEFINE_string(kudu_service_name, "kudu", + "The service name of the Kudu server. Must match the service name " + "used for Kudu server of sentry.service.admin.group option in the " + "Sentry server configuration."); +TAG_FLAG(kudu_service_name, experimental); + +DEFINE_string(sentry_service_kerberos_principal, "sentry", + "The service principal of the Sentry server. Must match the primary " + "(user) portion of sentry.service.server.principal option in the " + "Sentry server configuration."); +TAG_FLAG(sentry_service_kerberos_principal, experimental); + +DEFINE_string(sentry_service_security_mode, "kerberos", + "Configures whether Thrift connections to the Sentry server use " + "SASL (Kerberos) security. Must match the value of the " + "‘sentry.service.security.mode’ option in the Sentry server " + "configuration."); +TAG_FLAG(sentry_service_security_mode, experimental); + +DEFINE_int32(sentry_service_retry_count, 1, + "The number of times that Sentry operations will retry after " + "encountering retriable failures, such as network errors."); +TAG_FLAG(sentry_service_retry_count, advanced); +TAG_FLAG(sentry_service_retry_count, experimental); + +DEFINE_int32(sentry_service_send_timeout_seconds, 60, + "Configures the socket send timeout, in seconds, for Thrift " + "connections to the Sentry server."); +TAG_FLAG(sentry_service_send_timeout_seconds, advanced); +TAG_FLAG(sentry_service_send_timeout_seconds, experimental); + +DEFINE_int32(sentry_service_recv_timeout_seconds, 60, + "Configures the socket receive timeout, in seconds, for Thrift " + "connections to the Sentry server."); +TAG_FLAG(sentry_service_recv_timeout_seconds, advanced); +TAG_FLAG(sentry_service_recv_timeout_seconds, experimental); + +DEFINE_int32(sentry_service_conn_timeout_seconds, 60, + "Configures the socket connect timeout, in seconds, for Thrift " + "connections to the Sentry server."); +TAG_FLAG(sentry_service_conn_timeout_seconds, advanced); +TAG_FLAG(sentry_service_conn_timeout_seconds, experimental); + +DEFINE_int32(sentry_service_max_message_size_bytes, 100 * 1024 * 1024, + "Maximum size of Sentry objects that can be received by the " + "Sentry client in bytes. Must match the value of the " + "sentry.policy.client.thrift.max.message.size option in the " + "Sentry server configuration."); +TAG_FLAG(sentry_service_max_message_size_bytes, advanced); +TAG_FLAG(sentry_service_max_message_size_bytes, experimental); + +using strings::Substitute; + +namespace kudu { + +using sentry::SentryAction; +using sentry::SentryClient; + +namespace master { + +// Validates the sentry_service_rpc_addresses gflag. +static bool ValidateAddresses(const char* flag_name, const string& addresses) { + vector host_ports; + Status s = HostPort::ParseStringsWithScheme(addresses, + SentryClient::kDefaultSentryPort, + &host_ports); + if (!s.ok()) { + LOG(ERROR) << "invalid flag " << flag_name << ": " << s.ToString(); + } + return s.ok(); +} +DEFINE_validator(sentry_service_rpc_addresses, &ValidateAddresses); + +SentryAuthzProvider::~SentryAuthzProvider() { + Stop(); +} + +Status SentryAuthzProvider::Start() { + vector addresses; + RETURN_NOT_OK(HostPort::ParseStringsWithScheme(FLAGS_sentry_service_rpc_addresses, + SentryClient::kDefaultSentryPort, + &addresses)); + + thrift::ClientOptions options; + options.enable_kerberos = boost::iequals(FLAGS_sentry_service_security_mode, "kerberos"); + options.service_principal = FLAGS_sentry_service_kerberos_principal; + options.send_timeout = MonoDelta::FromSeconds(FLAGS_sentry_service_send_timeout_seconds); + options.recv_timeout = MonoDelta::FromSeconds(FLAGS_sentry_service_recv_timeout_seconds); + options.conn_timeout = MonoDelta::FromSeconds(FLAGS_sentry_service_conn_timeout_seconds); + options.max_buf_size = FLAGS_sentry_service_max_message_size_bytes; + options.retry_count = FLAGS_sentry_service_retry_count; + return ha_client_.Start(std::move(addresses), std::move(options)); +} + +void SentryAuthzProvider::Stop() { + ha_client_.Stop(); +} + +namespace { + +// Returns an authorizable based on the table name and the given scope. +Status GetAuthorizable(const string& table_name, + SentryAuthzProvider::AuthorizableScope scope, + TSentryAuthorizable* authorizable) { + Slice database; + Slice table; + switch (scope) { + case SentryAuthzProvider::AuthorizableScope::TABLE: + RETURN_NOT_OK(ParseHiveTableIdentifier(table_name, &database, &table)); + authorizable->__set_table(table.ToString()); + FALLTHROUGH_INTENDED; + case SentryAuthzProvider::AuthorizableScope::DATABASE: + if (database.empty() && table.empty()) { + RETURN_NOT_OK(ParseHiveTableIdentifier(table_name, &database, &table)); + } + authorizable->__set_db(database.ToString()); + FALLTHROUGH_INTENDED; + case SentryAuthzProvider::AuthorizableScope::SERVER: + authorizable->__set_server(FLAGS_server_name); + break; + default: + LOG(FATAL) << "unsupported authorizable scope"; + break; + } + + return Status::OK(); +} + +} // anonymous namespace + +Status SentryAuthzProvider::AuthorizeCreateTable(const string& table_name, + const string& user, + const string& owner) { + // If the table is being created with a different owner than the user, + // then the creating user must have 'ALL ON DATABASE' with grant. See + // design doc in [SENTRY-2151](https://issues.apache.org/jira/browse/SENTRY-2151). + // + // Otherwise, table creation requires 'CREATE ON DATABASE' privilege. + TSentryAuthorizable authorizable; + RETURN_NOT_OK(GetAuthorizable(table_name, AuthorizableScope::DATABASE, &authorizable)); + SentryAction action; + bool grant_option; + if (user == owner) { + action = SentryAction(SentryAction::Action::CREATE); + grant_option = false; + } else { + action = SentryAction(SentryAction::Action::ALL); + grant_option = true; + } + return Authorize(authorizable, action, user, grant_option); +} + +Status SentryAuthzProvider::AuthorizeDropTable(const string& table_name, + const string& user) { + // Table deletion requires 'DROP ON TABLE' privilege. + TSentryAuthorizable authorizable; + RETURN_NOT_OK(GetAuthorizable(table_name, AuthorizableScope::TABLE, &authorizable)); + SentryAction action = SentryAction(SentryAction::Action::DROP); + return Authorize(authorizable, action, user); +} + +Status SentryAuthzProvider::AuthorizeAlterTable(const string& old_table, + const string& new_table, + const string& user) { + // For table alteration (without table rename) requires 'ALTER ON TABLE' + // privilege; + // For table alteration (with table rename) requires + // 1. 'ALL ON TABLE ', + // 2. 'CREATE ON DATABASE '. + // See [SENTRY-2264](https://issues.apache.org/jira/browse/SENTRY-2264). + // TODO(hao): add inline hierarchy validation to avoid multiple RPCs. + TSentryAuthorizable table_authorizable; + RETURN_NOT_OK(GetAuthorizable(old_table, AuthorizableScope::TABLE, &table_authorizable)); + if (old_table == new_table) { + SentryAction action = SentryAction(SentryAction::Action::ALTER); + return Authorize(table_authorizable, action, user); + } + + SentryAction table_action = SentryAction(SentryAction::Action::ALL); + RETURN_NOT_OK(Authorize(table_authorizable, table_action, user)); + TSentryAuthorizable db_authorizable; + RETURN_NOT_OK(GetAuthorizable(new_table, AuthorizableScope::DATABASE, &db_authorizable)); + SentryAction db_action = SentryAction(SentryAction::Action::CREATE); + return Authorize(db_authorizable, db_action, user); +} + +Status SentryAuthzProvider::AuthorizeGetTableMetadata(const std::string& table_name, + const std::string& user) { + // Retrieving table metadata requires 'METADATA ON TABLE' privilege. + TSentryAuthorizable authorizable; + RETURN_NOT_OK(GetAuthorizable(table_name, AuthorizableScope::TABLE, &authorizable)); + SentryAction action = SentryAction(SentryAction::Action::METADATA); + return Authorize(authorizable, action, user); +} + +Status SentryAuthzProvider::Authorize(const TSentryAuthorizable& authorizable, + const SentryAction& action, + const string& user, + bool grant_option) { + + // In general, a privilege implies another when the authorizable from + // the former implies the authorizable from the latter, the action + // from the former implies the action from the latter, and grant option + // from the former implies the grant option from the latter. + // + // ListPrivilegesByUser returns all privileges granted to the user that + // imply the given authorizable. Therefore, we only need to validate if + // the granted actions can imply the required one. + + TListSentryPrivilegesRequest request; + request.__set_requestorUserName(FLAGS_kudu_service_name); + request.__set_principalName(user); + request.__set_authorizableHierarchy(authorizable); + TListSentryPrivilegesResponse response; + + RETURN_NOT_OK(ha_client_.Execute( + [&] (SentryClient* client) { + return client->ListPrivilegesByUser(request, &response); + })); + + for (const auto& privilege : response.privileges) { + // A grant option cannot imply the other if the former is set + // but the latter is not. + if (grant_option && privilege.grantOption != TSentryGrantOption::TRUE) { + continue; + } + + SentryAction granted_action; + Status s = SentryAction::FromString(privilege.action, &granted_action); + WARN_NOT_OK(s, Substitute("failed to construct sentry action from $0", privilege.action)); + if (s.ok() && granted_action.Implies(action)) { + return Status::OK(); + } + } + + // Logs an error if the action is not authorized for debugging purpose, and + // only returns generic error back to the users to avoid side channel leak, + // e.g. 'whether table A exists'. + LOG(ERROR) << "Action <" << action.action() << "> on authorizable <" + << authorizable << "> is not permitted for user <" << user << ">"; + return Status::NotAuthorized("unauthorized action"); +} + +} // namespace master +} // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/master/sentry_authz_provider.h ---------------------------------------------------------------------- diff --git a/src/kudu/master/sentry_authz_provider.h b/src/kudu/master/sentry_authz_provider.h new file mode 100644 index 0000000..0ac82d3 --- /dev/null +++ b/src/kudu/master/sentry_authz_provider.h @@ -0,0 +1,100 @@ +// 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. + +#pragma once + +#include + +#include "kudu/gutil/port.h" +#include "kudu/master/authz_provider.h" +#include "kudu/sentry/sentry_client.h" +#include "kudu/thrift/client.h" +#include "kudu/util/status.h" + +namespace sentry { +class TSentryAuthorizable; +} // namespace sentry + +namespace kudu { + +namespace sentry { +class SentryAction; +} // namespace sentry + +namespace master { + +// An implementation of AuthzProvider that connects to the Sentry service +// for authorization metadata and allows or denies the actions performed by +// users based on the metadata. +// +// This class is thread-safe after Start() is called. +class SentryAuthzProvider : public AuthzProvider { + public: + + enum class AuthorizableScope { + SERVER, + DATABASE, + TABLE, + }; + + ~SentryAuthzProvider(); + + // Start SentryAuthzProvider instance which connects to the Sentry service. + Status Start() override WARN_UNUSED_RESULT; + + void Stop() override; + + // The following authorizing methods will fail if: + // - the operation is not authorized + // - the Sentry service is unreachable + // - Sentry fails to resolve the group mapping of the user + // - the specified '--kudu_service_name' is a non-admin user in Sentry + // TODO(hao): add early failure recognition when SENTRY-2440 is done. + + Status AuthorizeCreateTable(const std::string& table_name, + const std::string& user, + const std::string& owner) override WARN_UNUSED_RESULT; + + Status AuthorizeDropTable(const std::string& table_name, + const std::string& user) override WARN_UNUSED_RESULT; + + // Note that the caller should normalize the table name if case sensitivity is not + // enforced for naming. e.g. when HMS integration is enabled. + Status AuthorizeAlterTable(const std::string& old_table, + const std::string& new_table, + const std::string& user) override WARN_UNUSED_RESULT; + + Status AuthorizeGetTableMetadata(const std::string& table_name, + const std::string& user) override WARN_UNUSED_RESULT; + + // Validates the sentry_service_rpc_addresses gflag. + static bool ValidateAddresses(const char* flag_name, const std::string& addresses); + + private: + + // Checks if the user can perform an action on the given authorizable. + // If the operation is not authorized, returns Status::NotAuthorized(). + Status Authorize(const ::sentry::TSentryAuthorizable& authorizable, + const sentry::SentryAction& action, + const std::string& user, + bool grant_option = false); + + thrift::HaClient ha_client_; +}; + +} // namespace master +} // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/sentry/sentry-test-base.h ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/sentry-test-base.h b/src/kudu/sentry/sentry-test-base.h new file mode 100644 index 0000000..c18bc79 --- /dev/null +++ b/src/kudu/sentry/sentry-test-base.h @@ -0,0 +1,83 @@ +// 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. + +#pragma once + +#include + +#include "kudu/rpc/sasl_common.h" +#include "kudu/security/test/mini_kdc.h" +#include "kudu/sentry/mini_sentry.h" +#include "kudu/sentry/sentry_client.h" +#include "kudu/thrift/client.h" +#include "kudu/util/test_macros.h" +#include "kudu/util/test_util.h" + +namespace kudu { +namespace sentry { + +class SentryTestBase : public KuduTest, + public ::testing::WithParamInterface { + public: + + void SetUp() override { + KuduTest::SetUp(); + thrift::ClientOptions sentry_client_opts; + sentry_.reset(new MiniSentry()); + if (kerberos_enabled_) { + kdc_.reset(new MiniKdc(MiniKdcOptions())); + ASSERT_OK(kdc_->Start()); + + // Create a service principal for the Sentry, and configure it to use it. + std::string spn = "sentry/127.0.0.1@KRBTEST.COM"; + std::string ktpath; + ASSERT_OK(kdc_->CreateServiceKeytab("sentry/127.0.0.1", &ktpath)); + + sentry_->EnableKerberos(kdc_->GetEnvVars()["KRB5_CONFIG"], + spn, + ktpath); + + ASSERT_OK(rpc::SaslInit()); + // Create a principal 'kudu' for the SentryAuthzProvider, and configure it + // to use it. + ASSERT_OK(kdc_->CreateUserPrincipal("kudu")); + ASSERT_OK(kdc_->Kinit("kudu")); + ASSERT_OK(kdc_->SetKrb5Environment()); + sentry_client_opts.enable_kerberos = true; + sentry_client_opts.service_principal = "sentry"; + } + + ASSERT_OK(sentry_->Start()); + sentry_client_.reset(new SentryClient(sentry_->address(), sentry_client_opts)); + ASSERT_OK(sentry_client_->Start()); + } + + void TearDown() override { + ASSERT_OK(sentry_client_->Stop()); + ASSERT_OK(sentry_->Stop()); + KuduTest::TearDown(); + } + + protected: + const bool kerberos_enabled_ = GetParam(); + std::unique_ptr kdc_; + std::unique_ptr sentry_; + std::unique_ptr sentry_client_; +}; + +} // namespace sentry +} // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/sentry/sentry_action-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/sentry_action-test.cc b/src/kudu/sentry/sentry_action-test.cc index 4679537..1f1644d 100644 --- a/src/kudu/sentry/sentry_action-test.cc +++ b/src/kudu/sentry/sentry_action-test.cc @@ -45,28 +45,28 @@ TEST(SentryActionTest, TestImplyAction) { SentryAction owner(SentryAction::Action::OWNER); // Different action cannot imply each other. - ASSERT_FALSE(insert.Imply(select)); - ASSERT_FALSE(select.Imply(insert)); + ASSERT_FALSE(insert.Implies(select)); + ASSERT_FALSE(select.Implies(insert)); vector actions({ all, select, insert, update, del, alter, create, drop, owner }); // Any action subsumes METADATA, not vice versa. for (const auto& action : actions) { - ASSERT_TRUE(action.Imply(metadata)); - ASSERT_FALSE(metadata.Imply(action)); + ASSERT_TRUE(action.Implies(metadata)); + ASSERT_FALSE(metadata.Implies(action)); } actions.emplace_back(metadata); for (const auto& action : actions) { // Action ALL implies all other actions. - ASSERT_TRUE(all.Imply(action)); + ASSERT_TRUE(all.Implies(action)); // Action OWNER equals to ALL, which implies all other actions. - ASSERT_TRUE(owner.Imply(action)); + ASSERT_TRUE(owner.Implies(action)); // Any action implies itself. - ASSERT_TRUE(action.Imply(action)); + ASSERT_TRUE(action.Implies(action)); } } @@ -75,8 +75,8 @@ TEST(SentryActionTest, TestFromString) { SentryAction wildcard; ASSERT_OK(SentryAction::FromString(SentryAction::kWildCard, &wildcard)); SentryAction all(SentryAction::Action::ALL); - ASSERT_TRUE(all.Imply(wildcard)); - ASSERT_TRUE(wildcard.Imply(all)); + ASSERT_TRUE(all.Implies(wildcard)); + ASSERT_TRUE(wildcard.Implies(all)); // Unsupported action, such as '+', throws invalid argument error. SentryAction invalid_action; http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/sentry/sentry_action.cc ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/sentry_action.cc b/src/kudu/sentry/sentry_action.cc index 73d314a..5512423 100644 --- a/src/kudu/sentry/sentry_action.cc +++ b/src/kudu/sentry/sentry_action.cc @@ -94,7 +94,7 @@ Status SentryAction::FromString(const string& str, SentryAction* action) { return Status::OK(); } -bool SentryAction::Imply(const SentryAction& other) const { +bool SentryAction::Implies(const SentryAction& other) const { // Every action must be initialized. CHECK_NE(action(), Action::UNINITIALIZED); CHECK_NE(other.action(), Action::UNINITIALIZED); http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/sentry/sentry_action.h ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/sentry_action.h b/src/kudu/sentry/sentry_action.h index 313868a..b12715c 100644 --- a/src/kudu/sentry/sentry_action.h +++ b/src/kudu/sentry/sentry_action.h @@ -77,7 +77,7 @@ class SentryAction { // and any action implies METADATA. // // See org.apache.sentry.policy.common.CommonPrivilege.impliesAction. - bool Imply(const SentryAction& other) const; + bool Implies(const SentryAction& other) const; private: Action action_; http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/sentry/sentry_client-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/sentry_client-test.cc b/src/kudu/sentry/sentry_client-test.cc index 10a4134..58af6fc 100644 --- a/src/kudu/sentry/sentry_client-test.cc +++ b/src/kudu/sentry/sentry_client-test.cc @@ -17,7 +17,6 @@ #include "kudu/sentry/sentry_client.h" -#include #include #include #include @@ -25,15 +24,13 @@ #include -#include "kudu/rpc/sasl_common.h" -#include "kudu/security/test/mini_kdc.h" #include "kudu/sentry/mini_sentry.h" +#include "kudu/sentry/sentry-test-base.h" #include "kudu/sentry/sentry_policy_service_types.h" #include "kudu/thrift/client.h" #include "kudu/util/net/net_util.h" #include "kudu/util/status.h" #include "kudu/util/test_macros.h" -#include "kudu/util/test_util.h" using sentry::TAlterSentryRoleAddGroupsRequest; using sentry::TAlterSentryRoleAddGroupsResponse; @@ -48,59 +45,12 @@ using sentry::TSentryGroup; using sentry::TSentryPrivilege; using std::set; using std::string; -using std::unique_ptr; using std::vector; namespace kudu { namespace sentry { -class SentryClientTest : public KuduTest, - public ::testing::WithParamInterface { - public: - bool KerberosEnabled() const { - return GetParam(); - } - - void SetUp() override { - bool enable_kerberos = KerberosEnabled(); - - thrift::ClientOptions sentry_client_opts; - - sentry_.reset(new MiniSentry()); - if (enable_kerberos) { - kdc_.reset(new MiniKdc()); - ASSERT_OK(kdc_->Start()); - - // Create a service principal for Sentry, and configure it to use it. - string spn = "sentry/127.0.0.1@KRBTEST.COM"; - string ktpath; - ASSERT_OK(kdc_->CreateServiceKeytab("sentry/127.0.0.1", &ktpath)); - - ASSERT_OK(rpc::SaslInit()); - sentry_->EnableKerberos(kdc_->GetEnvVars()["KRB5_CONFIG"], spn, ktpath); - - ASSERT_OK(kdc_->CreateUserPrincipal("kudu")); - ASSERT_OK(kdc_->Kinit("kudu")); - ASSERT_OK(kdc_->SetKrb5Environment()); - sentry_client_opts.enable_kerberos = true; - sentry_client_opts.service_principal = "sentry"; - } - ASSERT_OK(sentry_->Start()); - - sentry_client_.reset(new SentryClient(sentry_->address(), - sentry_client_opts)); - ASSERT_OK(sentry_client_->Start()); - } - - void TearDown() override { - ASSERT_OK(sentry_client_->Stop()); - ASSERT_OK(sentry_->Stop()); - } - - protected: - unique_ptr sentry_client_; - unique_ptr sentry_; - unique_ptr kdc_; +class SentryClientTest : public SentryTestBase { }; INSTANTIATE_TEST_CASE_P(KerberosEnabled, SentryClientTest, ::testing::Bool()); @@ -108,7 +58,7 @@ TEST_P(SentryClientTest, TestMiniSentryLifecycle) { // Create an HA Sentry client and ensure it automatically reconnects after service interruption. thrift::HaClient client; thrift::ClientOptions sentry_client_opts; - if (KerberosEnabled()) { + if (kerberos_enabled_) { sentry_client_opts.enable_kerberos = true; sentry_client_opts.service_principal = "sentry"; } @@ -189,18 +139,22 @@ TEST_P(SentryClientTest, TestCreateDropRole) { // Similar to above test to verify that the client can communicate with the // Sentry service to list privileges, and errors are converted to Status // instances. -TEST_P(SentryClientTest, TestListPrivilege) { - // Attempt to access Sentry privileges by a non admin user. +TEST_P(SentryClientTest, TestListPrivileges) { + // Attempt to access Sentry privileges without setting the user/principal name. TSentryAuthorizable authorizable; authorizable.server = "server"; authorizable.db = "db"; authorizable.table = "table"; TListSentryPrivilegesRequest request; - request.requestorUserName = "joe-interloper"; - request.authorizableHierarchy = authorizable; - request.__set_principalName("viewer"); + request.__set_requestorUserName("joe-interloper"); + request.__set_authorizableHierarchy(authorizable); TListSentryPrivilegesResponse response; Status s = sentry_client_->ListPrivilegesByUser(request, &response); + ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString(); + + // Attempt to access Sentry privileges by a non admin user. + request.__set_principalName("viewer"); + s = sentry_client_->ListPrivilegesByUser(request, &response); ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); // Attempt to access Sentry privileges by a user without @@ -220,9 +174,10 @@ TEST_P(SentryClientTest, TestListPrivilege) { } // Similar to above test to verify that the client can communicate with the -// Sentry service to grant roles, and errors are converted to Status +// Sentry service to add roles to groups (this allows the users from the +// groups have all privilege the role has), and errors are converted to Status // instances. -TEST_P(SentryClientTest, TestGrantRole) { +TEST_P(SentryClientTest, TestAlterRoleAddGroups) { // Attempt to alter role by a non admin user. TSentryGroup group; @@ -232,22 +187,22 @@ TEST_P(SentryClientTest, TestGrantRole) { TAlterSentryRoleAddGroupsRequest group_request; TAlterSentryRoleAddGroupsResponse group_response; - group_request.requestorUserName = "joe-interloper"; - group_request.roleName = "viewer"; - group_request.groups = groups; + group_request.__set_requestorUserName("joe-interloper"); + group_request.__set_roleName("viewer"); + group_request.__set_groups(groups); Status s = sentry_client_->AlterRoleAddGroups(group_request, &group_response); ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); // Attempt to alter a non-exist role. - group_request.requestorUserName = "test-admin"; + group_request.__set_requestorUserName("test-admin"); s = sentry_client_->AlterRoleAddGroups(group_request, &group_response); ASSERT_TRUE(s.IsNotFound()) << s.ToString(); // Alter role 'viewer' to add group 'user'. TCreateSentryRoleRequest role_request; - role_request.requestorUserName = "test-admin"; - role_request.roleName = "viewer"; + role_request.__set_requestorUserName("test-admin"); + role_request.__set_roleName("viewer"); ASSERT_OK(sentry_client_->CreateRole(role_request)); ASSERT_OK(sentry_client_->AlterRoleAddGroups(group_request, &group_response)); } @@ -265,37 +220,37 @@ TEST_P(SentryClientTest, TestGrantPrivilege) { TAlterSentryRoleAddGroupsRequest group_request; TAlterSentryRoleAddGroupsResponse group_response; - group_request.requestorUserName = "test-admin"; - group_request.roleName = "viewer"; - group_request.groups = groups; + group_request.__set_requestorUserName("test-admin"); + group_request.__set_roleName("viewer"); + group_request.__set_groups(groups); TCreateSentryRoleRequest role_request; - role_request.requestorUserName = "test-admin"; - role_request.roleName = "viewer"; + role_request.__set_requestorUserName("test-admin"); + role_request.__set_roleName("viewer"); ASSERT_OK(sentry_client_->CreateRole(role_request)); ASSERT_OK(sentry_client_->AlterRoleAddGroups(group_request, &group_response)); // Attempt to alter role by a non admin user. TAlterSentryRoleGrantPrivilegeRequest privilege_request; TAlterSentryRoleGrantPrivilegeResponse privilege_response; - privilege_request.requestorUserName = "joe-interloper"; - privilege_request.roleName = "viewer"; + privilege_request.__set_requestorUserName("joe-interloper"); + privilege_request.__set_roleName("viewer"); TSentryPrivilege privilege; - privilege.serverName = "server"; - privilege.dbName = "db"; - privilege.tableName = "table"; - privilege.action = "SELECT"; + privilege.__set_serverName("server"); + privilege.__set_dbName("db"); + privilege.__set_tableName("table"); + privilege.__set_action("SELECT"); privilege_request.__set_privilege(privilege); Status s = sentry_client_->AlterRoleGrantPrivilege(privilege_request, &privilege_response); ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); // Attempt to alter a non-exist role. - privilege_request.requestorUserName = "test-admin"; - privilege_request.roleName = "not-exist"; + privilege_request.__set_requestorUserName("test-admin"); + privilege_request.__set_roleName("not-exist"); s = sentry_client_->AlterRoleGrantPrivilege(privilege_request, &privilege_response); ASSERT_TRUE(s.IsNotFound()) << s.ToString(); - privilege_request.roleName = "viewer"; + privilege_request.__set_roleName("viewer"); ASSERT_OK(sentry_client_->AlterRoleGrantPrivilege(privilege_request, &privilege_response)); } http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/sentry/sentry_client.cc ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/sentry_client.cc b/src/kudu/sentry/sentry_client.cc index 919fb07..dacc3a9 100644 --- a/src/kudu/sentry/sentry_client.cc +++ b/src/kudu/sentry/sentry_client.cc @@ -167,9 +167,9 @@ Status SentryClient::DropRole(const TDropSentryRoleRequest& request) { Status SentryClient::ListPrivilegesByUser(const TListSentryPrivilegesRequest& request, TListSentryPrivilegesResponse* response) { SCOPED_LOG_SLOW_EXECUTION(WARNING, kSlowExecutionWarningThresholdMs, - "list Sentry privilege by user"); + "list Sentry privileges by user"); SENTRY_RET_NOT_OK(client_.list_sentry_privileges_by_user_and_itsgroups(*response, request), - response->status, "failed to list Sentry privilege by user"); + response->status, "failed to list Sentry privileges by user"); return Status::OK(); } http://git-wip-us.apache.org/repos/asf/kudu/blob/000f2cde/src/kudu/sentry/sentry_client.h ---------------------------------------------------------------------- diff --git a/src/kudu/sentry/sentry_client.h b/src/kudu/sentry/sentry_client.h index 41d5f87..27cc266 100644 --- a/src/kudu/sentry/sentry_client.h +++ b/src/kudu/sentry/sentry_client.h @@ -98,9 +98,9 @@ class SentryClient { // Alter role to grant privileges in Sentry. Status AlterRoleGrantPrivilege(const ::sentry::TAlterSentryRoleGrantPrivilegeRequest& request, - ::sentry::TAlterSentryRoleGrantPrivilegeResponse* response) WARN_UNUSED_RESULT; + ::sentry::TAlterSentryRoleGrantPrivilegeResponse* response) WARN_UNUSED_RESULT; -private: + private: ::sentry::SentryPolicyServiceClient client_; };