From 6f0e858ba929deacf297a116e1d99a678a3d231c Mon Sep 17 00:00:00 2001 From: Attila Bukor Date: Fri, 10 Jul 2020 16:05:54 +0200 Subject: [PATCH] KUDU-3090 Restrict changing ownership of a table Before this patch changing the ownership required ALTER permissions on a table. This patch adds a new method to authorization provider to authorize ownership changes and makes catalog manager call it if the owner is changed on an alter. Change-Id: I75a8b24364572a84f93826ad670c543abd407bb1 Reviewed-on: http://gerrit.cloudera.org:8080/16113 Reviewed-by: Andrew Wong Tested-by: Kudu Jenkins --- .../integration-tests/master_authz-itest.cc | 70 +++++++++++++++++++ src/kudu/master/authz_provider.h | 10 ++- src/kudu/master/catalog_manager.cc | 12 +++- src/kudu/master/default_authz_provider.h | 6 ++ src/kudu/master/ranger_authz_provider.cc | 24 +++++++ src/kudu/master/ranger_authz_provider.h | 4 ++ 6 files changed, 123 insertions(+), 3 deletions(-) diff --git a/src/kudu/integration-tests/master_authz-itest.cc b/src/kudu/integration-tests/master_authz-itest.cc index 9cbbd53121..63d605059c 100644 --- a/src/kudu/integration-tests/master_authz-itest.cc +++ b/src/kudu/integration-tests/master_authz-itest.cc @@ -190,6 +190,14 @@ class MasterAuthzITestHarness { return alterer->DropColumn("int32_val")->Alter(); } + static Status ChangeOwner(const OperationParams& p, + const shared_ptr& client) { + unique_ptr alterer( + client->NewTableAlterer(p.table_name)); + + return alterer->SetOwner(kTestUser)->Alter(); + } + static Status IsAlterTableDone(const OperationParams& p, const shared_ptr& client) { bool in_progress = false; @@ -328,6 +336,9 @@ class MasterAuthzITestHarness { const unique_ptr& cluster) = 0; virtual Status GrantAlterTablePrivilege(const PrivilegeParams& p, const unique_ptr& cluster) = 0; + virtual Status GrantAlterWithGrantTablePrivilege( + const PrivilegeParams& p, + const unique_ptr& cluster) = 0; virtual Status GrantRenameTablePrivilege(const PrivilegeParams& p, const unique_ptr& cluster) = 0; virtual Status GrantGetMetadataTablePrivilege(const PrivilegeParams& p, @@ -411,6 +422,17 @@ class RangerITestHarness : public MasterAuthzITestHarness { return RefreshAuthzPolicies(cluster); } + Status GrantAlterWithGrantTablePrivilege( + const PrivilegeParams& p, + const unique_ptr& cluster) override { + AuthorizationPolicy policy; + policy.databases.emplace_back(p.db_name); + policy.tables.emplace_back(p.table_name); + policy.items.emplace_back(PolicyItem({p.user_name}, {ActionPB::ALTER}, true)); + RETURN_NOT_OK(ranger_->AddPolicy(move(policy))); + return RefreshAuthzPolicies(cluster); + } + Status GrantRenameTablePrivilege(const PrivilegeParams& p, const unique_ptr& cluster) override { AuthorizationPolicy policy_new_table; @@ -604,6 +626,10 @@ class MasterAuthzITestBase : public ExternalMiniClusterITestBase { return harness_->GrantGetMetadataTablePrivilege(p, cluster_); } + Status GrantAlterWithGrantTablePrivilege(const PrivilegeParams& p) { + return harness_->GrantAlterWithGrantTablePrivilege(p, cluster_); + } + Status GrantAllTablePrivilege(const PrivilegeParams& p) { return harness_->GrantAllTablePrivilege(p, cluster_); } @@ -644,6 +670,10 @@ class MasterAuthzITestBase : public ExternalMiniClusterITestBase { return harness_->IsAlterTableDone(p, client_); } + Status ChangeOwner(const OperationParams& p) { + return harness_->ChangeOwner(p, client_); + } + Status RenameTable(const OperationParams& p) { return harness_->RenameTable(p, client_); } @@ -889,6 +919,37 @@ TEST_P(MasterAuthzITest, TestAuthzGiveAwayOwnership) { } } +TEST_P(MasterAuthzITest, TestChangeOwnerWithoutDelegateAdmin) { + this->GrantAllDatabasePrivilege({kDatabaseName, kTableName}); + const string table_name = Substitute("$0.$1", kDatabaseName, kTableName); + + unique_ptr alterer(this->client_->NewTableAlterer(table_name)); + Status s = alterer->SetOwner(kSecondUser)->Alter(); + ASSERT_TRUE(s.IsNotAuthorized()); +} + +TEST_P(MasterAuthzITest, TestChangeOwnerWithoutAll) { + this->GrantAlterWithGrantTablePrivilege({kDatabaseName, kTableName}); + const string table_name = Substitute("$0.$1", kDatabaseName, kTableName); + + unique_ptr alterer(this->client_->NewTableAlterer(table_name)); + Status s = alterer->SetOwner(kSecondUser)->Alter(); + ASSERT_TRUE(s.IsNotAuthorized()); +} + +TEST_P(MasterAuthzITest, TestAlterAndChangeOwner) { + this->GrantAlterTablePrivilege({kDatabaseName, kTableName}); + const string table_name = Substitute("$0.$1", kDatabaseName, kTableName); + + unique_ptr alterer(this->client_->NewTableAlterer(table_name)); + alterer->SetOwner(kSecondUser)->DropColumn("int8_val"); + Status s = alterer->Alter(); + ASSERT_TRUE(s.IsNotAuthorized()); + + this->GrantAllWithGrantTablePrivilege({kDatabaseName, kTableName}); + ASSERT_OK(alterer->Alter()); +} + class MasterAuthzOwnerITest : public MasterAuthzITestBase, public ::testing::WithParamInterface> { @@ -1113,6 +1174,15 @@ static const AuthzDescriptor kAuthzCombinations[] = { kTableName, "" }, + { + { + &MasterAuthzITestBase::ChangeOwner, + &MasterAuthzITestBase::GrantAllWithGrantTablePrivilege, + "ChangeOwner", + }, + kDatabaseName, + kTableName, + }, }; INSTANTIATE_TEST_CASE_P( AuthzCombinations, TestAuthzTable, diff --git a/src/kudu/master/authz_provider.h b/src/kudu/master/authz_provider.h index 1c98b15872..401b4ed1c6 100644 --- a/src/kudu/master/authz_provider.h +++ b/src/kudu/master/authz_provider.h @@ -33,7 +33,6 @@ class TablePrivilegePB; } // namespace security namespace master { - // An interface for handling authorizations on Kudu operations. class AuthzProvider { public: @@ -113,6 +112,15 @@ class AuthzProvider { const SchemaPB& schema_pb, security::TablePrivilegePB* pb) WARN_UNUSED_RESULT = 0; + // Checks if changing the owner of the table is authorized for the given user. + // 'is_owner' indicates whether 'user' is the current owner of the table. + // + // If the operation is not authorized, returns Status::NotAuthorized(). + // Otherwise, may return other Status error codes depend on actual errors. + virtual Status AuthorizeChangeOwner(const std::string& table_name, + const std::string& user, + bool is_owner) WARN_UNUSED_RESULT = 0; + // Refreshes policies in the authorization provider plugin. virtual Status RefreshPolicies() WARN_UNUSED_RESULT = 0; diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc index 706346b35c..96f687ee1b 100644 --- a/src/kudu/master/catalog_manager.cc +++ b/src/kudu/master/catalog_manager.cc @@ -2612,10 +2612,19 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB& req, const string& owner) { const string new_table = req.has_new_table_name() ? NormalizeTableName(req.new_table_name()) : table_name; + // Change owner requires higher level of privilege (ALL WITH GRANT OPTION, + // or ALL + delegate admin) than other types of alter operations, so if a + // single alter contains an owner change as well as other changes, it's + // sufficient to authorize only the owner change. + if (req.has_new_table_owner()) { + return SetupError(authz_provider_->AuthorizeChangeOwner(table_name, username, + username == owner), + resp, MasterErrorPB::NOT_AUTHORIZED); + } + return SetupError(authz_provider_->AuthorizeAlterTable(table_name, new_table, username, username == owner), resp, MasterErrorPB::NOT_AUTHORIZED); - }; RETURN_NOT_OK(FindLockAndAuthorizeTable(req, resp, LockMode::WRITE, authz_func, user, &table, &l)); @@ -2701,7 +2710,6 @@ Status CatalogManager::AlterTable(const AlterTableRequestPB& req, }); // 5. Alter the table owner. - // TODO(abukor): Only the owner (or superuser) can alter the owner? if (req.has_new_table_owner()) { RETURN_NOT_OK(SetupError( ValidateOwner(req.new_table_owner()).CloneAndAppend("invalid owner name"), diff --git a/src/kudu/master/default_authz_provider.h b/src/kudu/master/default_authz_provider.h index 84da23ee77..2e8554c907 100644 --- a/src/kudu/master/default_authz_provider.h +++ b/src/kudu/master/default_authz_provider.h @@ -78,6 +78,12 @@ class DefaultAuthzProvider : public AuthzProvider { return Status::OK(); } + Status AuthorizeChangeOwner(const std::string& /*table_name*/, + const std::string& /*user*/, + bool /*is_owner*/) override WARN_UNUSED_RESULT { + return Status::OK(); + } + Status FillTablePrivilegePB(const std::string& /*table_name*/, const std::string& /*user*/, bool /*is_owner*/, diff --git a/src/kudu/master/ranger_authz_provider.cc b/src/kudu/master/ranger_authz_provider.cc index 25cdd069cf..156cef4ab6 100644 --- a/src/kudu/master/ranger_authz_provider.cc +++ b/src/kudu/master/ranger_authz_provider.cc @@ -345,6 +345,30 @@ Status RangerAuthzProvider::RefreshPolicies() { return client_.RefreshPolicies(); } +Status RangerAuthzProvider::AuthorizeChangeOwner(const string& table_name, + const string& user, + bool is_owner) { + if (IsTrustedUser(user)) { + return Status::OK(); + } + + string db; + string tbl; + + RETURN_NOT_OK(ParseTableIdentifier(table_name, &db, &tbl)); + + bool authorized; + RETURN_NOT_OK(client_.AuthorizeAction(user, ActionPB::ALL, db, tbl, is_owner, + /*requires_delegate_admin=*/true, &authorized)); + + if (PREDICT_FALSE(!authorized)) { + LOG(WARNING) << Substitute("User $0 is not authorized to change owner of $1", user, table_name); + return Status::NotAuthorized(kUnauthorizedAction); + } + + return Status::OK(); +} + bool RangerAuthzProvider::IsEnabled() { return !FLAGS_ranger_config_path.empty(); } diff --git a/src/kudu/master/ranger_authz_provider.h b/src/kudu/master/ranger_authz_provider.h index 8966f7b690..973acb5e94 100644 --- a/src/kudu/master/ranger_authz_provider.h +++ b/src/kudu/master/ranger_authz_provider.h @@ -81,6 +81,10 @@ class RangerAuthzProvider : public AuthzProvider { const SchemaPB& schema_pb, security::TablePrivilegePB* pb) override WARN_UNUSED_RESULT; + Status AuthorizeChangeOwner(const std::string& table_name, + const std::string& user, + bool is_owner) override WARN_UNUSED_RESULT; + Status RefreshPolicies() override WARN_UNUSED_RESULT; // Returns true if 'ranger_policy_server' flag is set indicating Ranger