From cc694b6bcfb13d02aa00ba467acf356b61d612a1 Mon Sep 17 00:00:00 2001 From: aykut-bozkurt <51649454+aykut-bozkurt@users.noreply.github.com> Date: Tue, 9 Aug 2022 20:59:30 +0300 Subject: [PATCH] we consider stat object as invalid if it is not owned by current user (#6130) --- .../distributed/commands/utility_hook.c | 19 +-- .../distributed/utils/citus_depended_object.c | 111 +++++++++++++++++- .../distributed/citus_depended_object.h | 2 + .../grant_on_function_propagation.out | 8 +- .../sql/grant_on_function_propagation.sql | 3 +- 5 files changed, 126 insertions(+), 17 deletions(-) diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 6e1ff984a..bf1077534 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -549,11 +549,11 @@ ProcessUtilityInternal(PlannedStmt *pstmt, * unwanted citus related warnings or early error logs related to invalid address. * Therefore, we first check if any address in the given statement is valid. * Then, we do not execute qualify and preprocess if none of the addresses are valid - * to prevent before-mentioned citus related messages. PG will complain about the - * invalid address, so we are safe to not execute qualify and preprocess. Also - * note that we should not guard any step after standardProcess_Utility with - * the enum state distOpsValidationState because PG would have already failed the - * transaction. + * or any address violates ownership rules to prevent before-mentioned citus related + * messages. PG will complain about the invalid address or ownership violation, so we + * are safe to not execute qualify and preprocess. Also note that we should not guard + * any step after standardProcess_Utility with the enum state distOpsValidationState + * because PG would have already failed the transaction. */ distOpsValidationState = DistOpsValidityState(parsetree, ops); @@ -565,15 +565,16 @@ ProcessUtilityInternal(PlannedStmt *pstmt, * and fill them out how postgres would resolve them. This makes subsequent * deserialize calls for the statement portable to other postgres servers, the * workers in our case. - * If there are no valid objects, let's skip the qualify and - * preprocess, and do not diverge from Postgres in terms of error messages. + * If there are no valid objects or any object violates ownership, let's skip + * the qualify and preprocess, and do not diverge from Postgres in terms of + * error messages. */ - if (ops && ops->qualify && distOpsValidationState != HasNoneValidObject) + if (ops && ops->qualify && DistOpsInValidState(distOpsValidationState)) { ops->qualify(parsetree); } - if (ops && ops->preprocess && distOpsValidationState != HasNoneValidObject) + if (ops && ops->preprocess && DistOpsInValidState(distOpsValidationState)) { ddlJobs = ops->preprocess(parsetree, queryString, context); } diff --git a/src/backend/distributed/utils/citus_depended_object.c b/src/backend/distributed/utils/citus_depended_object.c index 83062ccda..b911b1c35 100644 --- a/src/backend/distributed/utils/citus_depended_object.c +++ b/src/backend/distributed/utils/citus_depended_object.c @@ -39,7 +39,10 @@ #include "nodes/makefuncs.h" #include "nodes/nodeFuncs.h" #include "nodes/parsenodes.h" +#include "parser/parse_type.h" +#include "storage/large_object.h" #include "utils/lsyscache.h" +#include "utils/syscache.h" /* * GUC hides any objects, which depends on citus extension, from pg meta class queries, @@ -50,6 +53,8 @@ bool HideCitusDependentObjects = false; static Node * CreateCitusDependentObjectExpr(int pgMetaTableVarno, int pgMetaTableOid); static List * GetCitusDependedObjectArgs(int pgMetaTableVarno, int pgMetaTableOid); static bool AlterRoleSetStatementContainsAll(Node *node); +static bool HasDropCommandViolatesOwnership(Node *node); +static bool AnyObjectViolatesOwnership(DropStmt *dropStmt); /* * IsPgLocksTable returns true if RTE is pg_locks table. @@ -334,13 +339,20 @@ DistOpsValidityState(Node *node, const DistributeObjectOps *ops) */ return NoAddressResolutionRequired; } + else if (HasDropCommandViolatesOwnership(node)) + { + /* + * found object with an invalid ownership, PG will complain if there is any object + * with an invalid ownership. + */ + return HasObjectWithInvalidOwnership; + } if (ops && ops->address) { bool missingOk = true; bool isPostprocess = false; List *objectAddresses = ops->address(node, missingOk, isPostprocess); - ObjectAddress *objectAddress = NULL; foreach_ptr(objectAddress, objectAddresses) { @@ -362,6 +374,18 @@ DistOpsValidityState(Node *node, const DistributeObjectOps *ops) } +/* + * DistOpsInValidState returns true if given state is valid to execute + * preprocess and qualify steps. + */ +bool +DistOpsInValidState(DistOpsValidationState distOpsValidationState) +{ + return distOpsValidationState == HasAtLeastOneValidObject || distOpsValidationState == + NoAddressResolutionRequired; +} + + /* * AlterRoleSetStatementContainsAll returns true if the statement is a * ALTER ROLE ALL (SET / RESET). @@ -384,3 +408,88 @@ AlterRoleSetStatementContainsAll(Node *node) return false; } + + +/* + * HasDropCommandViolatesOwnership returns true if any object in the given + * statement violates object ownership. + * + * Currently there is only one test which fails due to object ownership. + * The command that is failing is DROP. If in the future we hit other + * commands like this, we should expand this function. + */ +static bool +HasDropCommandViolatesOwnership(Node *node) +{ + if (!IsA(node, DropStmt)) + { + return false; + } + + DropStmt *dropStmt = castNode(DropStmt, node); + if (AnyObjectViolatesOwnership(dropStmt)) + { + return true; + } + + return false; +} + + +/* + * AnyObjectViolatesOwnership return true if given object in stmt violates ownership. + */ +static bool +AnyObjectViolatesOwnership(DropStmt *dropStmt) +{ + volatile ObjectAddress objectAddress = { 0 }; + Relation relation = NULL; + bool objectViolatesOwnership = false; + ObjectType objectType = dropStmt->removeType; + bool missingOk = dropStmt->missing_ok; + + Node *object = NULL; + foreach_ptr(object, dropStmt->objects) + { + PG_TRY(); + { + objectAddress = get_object_address(objectType, object, + &relation, AccessShareLock, missingOk); + + + if (OidIsValid(objectAddress.objectId)) + { + /* + * if object violates ownership, check_object_ownership will throw error. + */ + check_object_ownership(GetUserId(), + objectType, + objectAddress, + object, relation); + } + } + PG_CATCH(); + { + if (OidIsValid(objectAddress.objectId)) + { + /* ownership violation */ + objectViolatesOwnership = true; + } + } + PG_END_TRY(); + + if (relation != NULL) + { + relation_close(relation, AccessShareLock); + relation = NULL; + } + + /* we found ownership violation, so can return here */ + if (objectViolatesOwnership) + { + return true; + } + } + + return false; +} diff --git a/src/include/distributed/citus_depended_object.h b/src/include/distributed/citus_depended_object.h index 72ecf322c..b52018411 100644 --- a/src/include/distributed/citus_depended_object.h +++ b/src/include/distributed/citus_depended_object.h @@ -23,6 +23,7 @@ typedef enum DistOpsValidationState { HasAtLeastOneValidObject, HasNoneValidObject, + HasObjectWithInvalidOwnership, NoAddressResolutionRequired } DistOpsValidationState; @@ -33,5 +34,6 @@ extern bool HideCitusDependentObjectsOnQueriesOfPgMetaTables(Node *node, void *c extern bool IsPgLocksTable(RangeTblEntry *rte); extern DistOpsValidationState DistOpsValidityState(Node *node, const DistributeObjectOps *ops); +extern bool DistOpsInValidState(DistOpsValidationState distOpsValidationState); #endif /* CITUS_DEPENDED_OBJECT_H */ diff --git a/src/test/regress/expected/grant_on_function_propagation.out b/src/test/regress/expected/grant_on_function_propagation.out index d7700d3b2..2e4501541 100644 --- a/src/test/regress/expected/grant_on_function_propagation.out +++ b/src/test/regress/expected/grant_on_function_propagation.out @@ -828,11 +828,7 @@ SELECT proname, pronargs, proacl FROM pg_proc WHERE proname IN ('dist_float_avg' \c - - - :master_port SET search_path TO grant_on_function, public; DROP AGGREGATE dist_float_avg(float8), dist_float_avg_2(float8), no_dist_float_avg(float8), not_propagated_aggregate_user_test(float8); -SELECT run_command_on_coordinator_and_workers('DROP SCHEMA grant_on_function CASCADE'); - run_command_on_coordinator_and_workers ---------------------------------------------------------------------- - -(1 row) - +DROP SCHEMA grant_on_function CASCADE; DROP USER function_user_1, function_user_2, function_user_3, not_propagated_function_user_4; DROP USER procedure_user_1, procedure_user_2, procedure_user_3, not_propagated_procedure_user_4; +DROP USER aggregate_user_1, aggregate_user_2, aggregate_user_3, not_propagated_aggregate_user_4; diff --git a/src/test/regress/sql/grant_on_function_propagation.sql b/src/test/regress/sql/grant_on_function_propagation.sql index 007150d3e..3f7a4bd5d 100644 --- a/src/test/regress/sql/grant_on_function_propagation.sql +++ b/src/test/regress/sql/grant_on_function_propagation.sql @@ -492,6 +492,7 @@ SET search_path TO grant_on_function, public; DROP AGGREGATE dist_float_avg(float8), dist_float_avg_2(float8), no_dist_float_avg(float8), not_propagated_aggregate_user_test(float8); -SELECT run_command_on_coordinator_and_workers('DROP SCHEMA grant_on_function CASCADE'); +DROP SCHEMA grant_on_function CASCADE; DROP USER function_user_1, function_user_2, function_user_3, not_propagated_function_user_4; DROP USER procedure_user_1, procedure_user_2, procedure_user_3, not_propagated_procedure_user_4; +DROP USER aggregate_user_1, aggregate_user_2, aggregate_user_3, not_propagated_aggregate_user_4;