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
kaijunwu · Jul 16, 2020 · 6f0e858 · 6f0e858
1 parent 0e94aa7
commit 6f0e858
Show file tree

Hide file tree

Showing 6 changed files with 123 additions and 3 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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