This is an automated email from the ASF dual-hosted git repository.
abukor pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new 6f0e858 KUDU-3090 Restrict changing ownership of a table
6f0e858 is described below
commit 6f0e858ba929deacf297a116e1d99a678a3d231c
Author: Attila Bukor <[email protected]>
AuthorDate: Fri Jul 10 16:05:54 2020 +0200
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 <[email protected]>
Tested-by: Kudu Jenkins
---
src/kudu/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 9cbbd53..63d6050 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<KuduClient>& client) {
+ unique_ptr<KuduTableAlterer> alterer(
+ client->NewTableAlterer(p.table_name));
+
+ return alterer->SetOwner(kTestUser)->Alter();
+ }
+
static Status IsAlterTableDone(const OperationParams& p,
const shared_ptr<KuduClient>& client) {
bool in_progress = false;
@@ -328,6 +336,9 @@ class MasterAuthzITestHarness {
const
unique_ptr<ExternalMiniCluster>& cluster) = 0;
virtual Status GrantAlterTablePrivilege(const PrivilegeParams& p,
const
unique_ptr<ExternalMiniCluster>& cluster) = 0;
+ virtual Status GrantAlterWithGrantTablePrivilege(
+ const PrivilegeParams& p,
+ const unique_ptr<ExternalMiniCluster>& cluster) = 0;
virtual Status GrantRenameTablePrivilege(const PrivilegeParams& p,
const
unique_ptr<ExternalMiniCluster>& 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<ExternalMiniCluster>& 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<ExternalMiniCluster>&
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<KuduTableAlterer>
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<KuduTableAlterer>
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<KuduTableAlterer>
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<std::tuple<HarnessEnum,
std::string>> {
@@ -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 1c98b15..401b4ed 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 706346b..96f687e 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 84da23e..2e8554c 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 25cdd06..156cef4 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 8966f7b..973acb5 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
