From aab9f623eb172acd0e90b5d064539800421a00f5 Mon Sep 17 00:00:00 2001 From: Marco Slot Date: Fri, 16 Nov 2018 23:27:34 +0100 Subject: [PATCH 1/3] Check table ownership in upgrade_to_reference_table --- .../distributed/utils/reference_table_utils.c | 3 ++- src/test/regress/expected/multi_multiuser.out | 12 ++++++++++++ src/test/regress/sql/multi_multiuser.sql | 8 ++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/backend/distributed/utils/reference_table_utils.c b/src/backend/distributed/utils/reference_table_utils.c index a6b0b29f0..f5b1608a2 100644 --- a/src/backend/distributed/utils/reference_table_utils.c +++ b/src/backend/distributed/utils/reference_table_utils.c @@ -60,8 +60,9 @@ upgrade_to_reference_table(PG_FUNCTION_ARGS) uint64 shardId = INVALID_SHARD_ID; DistTableCacheEntry *tableEntry = NULL; - EnsureCoordinator(); CheckCitusVersion(ERROR); + EnsureCoordinator(); + EnsureTableOwner(relationId); if (!IsDistributedTable(relationId)) { diff --git a/src/test/regress/expected/multi_multiuser.out b/src/test/regress/expected/multi_multiuser.out index d040f3e3a..a86ea2e44 100644 --- a/src/test/regress/expected/multi_multiuser.out +++ b/src/test/regress/expected/multi_multiuser.out @@ -21,6 +21,14 @@ SELECT create_distributed_table('test', 'id'); (1 row) +SET citus.shard_count TO 1; +CREATE TABLE singleshard (id integer, val integer); +SELECT create_distributed_table('singleshard', 'id'); + create_distributed_table +-------------------------- + +(1 row) + -- turn off propagation to avoid Enterprise processing the following section SET citus.enable_ddl_propagation TO off; CREATE USER full_access; @@ -212,6 +220,9 @@ ERROR: permission denied for table test ABORT; SELECT * FROM citus_stat_statements_reset(); ERROR: permission denied for function citus_stat_statements_reset +-- should not be allowed to upgrade to reference table +SELECT upgrade_to_reference_table('singleshard'); +ERROR: must be owner of table singleshard -- table owner should be the same on the shards, even when distributing the table as superuser SET ROLE full_access; CREATE TABLE my_table (id integer, val integer); @@ -231,6 +242,7 @@ SELECT result FROM run_command_on_workers($$SELECT tableowner FROM pg_tables WHE DROP TABLE my_table; DROP TABLE test; +DROP TABLE singleshard; DROP USER full_access; DROP USER read_access; DROP USER no_access; diff --git a/src/test/regress/sql/multi_multiuser.sql b/src/test/regress/sql/multi_multiuser.sql index 43306f09c..478e02925 100644 --- a/src/test/regress/sql/multi_multiuser.sql +++ b/src/test/regress/sql/multi_multiuser.sql @@ -16,6 +16,10 @@ SET citus.shard_replication_factor TO 1; CREATE TABLE test (id integer, val integer); SELECT create_distributed_table('test', 'id'); +SET citus.shard_count TO 1; +CREATE TABLE singleshard (id integer, val integer); +SELECT create_distributed_table('singleshard', 'id'); + -- turn off propagation to avoid Enterprise processing the following section SET citus.enable_ddl_propagation TO off; @@ -138,6 +142,9 @@ ABORT; SELECT * FROM citus_stat_statements_reset(); +-- should not be allowed to upgrade to reference table +SELECT upgrade_to_reference_table('singleshard'); + -- table owner should be the same on the shards, even when distributing the table as superuser SET ROLE full_access; CREATE TABLE my_table (id integer, val integer); @@ -147,6 +154,7 @@ SELECT result FROM run_command_on_workers($$SELECT tableowner FROM pg_tables WHE DROP TABLE my_table; DROP TABLE test; +DROP TABLE singleshard; DROP USER full_access; DROP USER read_access; DROP USER no_access; From 18acd00553b15fba532c562b0fdd98007228cb23 Mon Sep 17 00:00:00 2001 From: Marco Slot Date: Fri, 16 Nov 2018 23:53:37 +0100 Subject: [PATCH 2/3] Check permissions in lock_relation_if_exists --- src/backend/distributed/utils/resource_lock.c | 94 +++++++++++++++++-- src/test/regress/expected/multi_multiuser.out | 11 +++ .../regress/expected/multi_multiuser_0.out | 35 ++++++- src/test/regress/sql/multi_multiuser.sql | 6 ++ 4 files changed, 136 insertions(+), 10 deletions(-) diff --git a/src/backend/distributed/utils/resource_lock.c b/src/backend/distributed/utils/resource_lock.c index fce2de7a0..6a0e1f2e1 100644 --- a/src/backend/distributed/utils/resource_lock.c +++ b/src/backend/distributed/utils/resource_lock.c @@ -19,6 +19,7 @@ #include "access/xact.h" #include "catalog/namespace.h" +#include "commands/tablecmds.h" #include "distributed/colocation_utils.h" #include "distributed/listutils.h" #include "distributed/master_metadata_utility.h" @@ -36,6 +37,7 @@ #include "distributed/version_compat.h" #include "storage/lmgr.h" #include "utils/builtins.h" +#include "utils/lsyscache.h" #if (PG_VERSION_NUM >= 100000) #include "utils/varlena.h" #endif @@ -73,6 +75,9 @@ static void LockShardListResources(List *shardIntervalList, LOCKMODE lockMode); static void LockShardListResourcesOnFirstWorker(LOCKMODE lockmode, List *shardIntervalList); static bool IsFirstWorkerNode(); +static void CitusRangeVarCallbackForLockTable(const RangeVar *rangeVar, Oid relationId, + Oid oldRelationId, void *arg); +static AclResult CitusLockTableAclCheck(Oid relationId, LOCKMODE lockmode, Oid userId); /* exports for SQL callable functions */ @@ -742,16 +747,11 @@ lock_relation_if_exists(PG_FUNCTION_ARGS) List *relationNameList = NIL; RangeVar *relation = NULL; LOCKMODE lockMode = NoLock; + bool relationExists = false; /* ensure that we're in a transaction block */ RequireTransactionBlock(true, "lock_relation_if_exists"); - relationId = ResolveRelationId(relationName, true); - if (!OidIsValid(relationId)) - { - PG_RETURN_BOOL(false); - } - /* get the lock mode */ lockMode = LockModeTextToLockMode(lockModeCString); @@ -760,7 +760,85 @@ lock_relation_if_exists(PG_FUNCTION_ARGS) relation = makeRangeVarFromNameList(relationNameList); /* lock the relation with the lock mode */ - RangeVarGetRelid(relation, lockMode, false); + relationId = RangeVarGetRelidInternal(relation, lockMode, RVR_MISSING_OK, + CitusRangeVarCallbackForLockTable, + (void *) &lockMode); + relationExists = OidIsValid(relationId); - PG_RETURN_BOOL(true); + PG_RETURN_BOOL(relationExists); +} + + +/* + * CitusRangeVarCallbackForLockTable is a callback for RangeVarGetRelidExtended used + * to check whether the user has permission to lock a table in a particular mode. + * + * This function is a copy of RangeVarCallbackForLockTable in lockcmds.c adapted to + * Citus code style. + */ +static void +CitusRangeVarCallbackForLockTable(const RangeVar *rangeVar, Oid relationId, + Oid oldRelationId, void *arg) +{ + LOCKMODE lockmode = *(LOCKMODE *) arg; + AclResult aclResult; + + if (!OidIsValid(relationId)) + { + /* table doesn't exist, so no permissions check */ + return; + } + + /* we only allow tables and views to be locked */ + if (!RegularTable(relationId)) + { + ereport(ERROR, (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("\"%s\" is not a table", rangeVar->relname))); + } + + /* check permissions */ + aclResult = CitusLockTableAclCheck(relationId, lockmode, GetUserId()); + if (aclResult != ACLCHECK_OK) + { +#if (PG_VERSION_NUM >= 110000) + aclcheck_error(aclResult, get_relkind_objtype(get_rel_relkind(relationId)), + rangeVar->relname); +#else + + aclcheck_error(aclResult, ACL_KIND_CLASS, rangeVar->relname); +#endif + } +} + + +/* + * CitusLockTableAclCheck checks whether a user has permission to lock a relation + * in the given lock mode. + * + * This function is a copy of LockTableAclCheck in lockcmds.c adapted to Citus + * code style. + */ +static AclResult +CitusLockTableAclCheck(Oid relationId, LOCKMODE lockmode, Oid userId) +{ + AclResult aclResult; + AclMode aclMask; + + /* verify adequate privilege */ + if (lockmode == AccessShareLock) + { + aclMask = ACL_SELECT; + } + else if (lockmode == RowExclusiveLock) + { + aclMask = ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE; + } + else + { + aclMask = ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE; + } + + aclResult = pg_class_aclcheck(relationId, userId, aclMask); + + return aclResult; } diff --git a/src/test/regress/expected/multi_multiuser.out b/src/test/regress/expected/multi_multiuser.out index a86ea2e44..fb94d9917 100644 --- a/src/test/regress/expected/multi_multiuser.out +++ b/src/test/regress/expected/multi_multiuser.out @@ -165,6 +165,17 @@ SELECT count(*) FROM test a JOIN test b ON (a.val = b.val) WHERE a.id = 1 AND b. COPY "postgresql.conf" TO STDOUT WITH (format transmit); ERROR: operation is not allowed HINT: Run the command with a superuser. +-- should not be allowed to take aggressive locks on table +BEGIN; +SELECT lock_relation_if_exists('test', 'ACCESS SHARE'); + lock_relation_if_exists +------------------------- + t +(1 row) + +SELECT lock_relation_if_exists('test', 'EXCLUSIVE'); +ERROR: permission denied for table test +ABORT; SET citus.task_executor_type TO 'real-time'; -- check no permission SET ROLE no_access; diff --git a/src/test/regress/expected/multi_multiuser_0.out b/src/test/regress/expected/multi_multiuser_0.out index 037a31381..3088baa74 100644 --- a/src/test/regress/expected/multi_multiuser_0.out +++ b/src/test/regress/expected/multi_multiuser_0.out @@ -21,6 +21,21 @@ SELECT create_distributed_table('test', 'id'); (1 row) +CREATE TABLE test_coloc (id integer, val integer); +SELECT create_distributed_table('test_coloc', 'id', colocate_with := 'none'); + create_distributed_table +-------------------------- + +(1 row) + +SET citus.shard_count TO 1; +CREATE TABLE singleshard (id integer, val integer); +SELECT create_distributed_table('singleshard', 'id'); + create_distributed_table +-------------------------- + +(1 row) + -- turn off propagation to avoid Enterprise processing the following section SET citus.enable_ddl_propagation TO off; CREATE USER full_access; @@ -157,6 +172,17 @@ SELECT count(*) FROM test a JOIN test b ON (a.val = b.val) WHERE a.id = 1 AND b. COPY "postgresql.conf" TO STDOUT WITH (format transmit); ERROR: operation is not allowed HINT: Run the command with a superuser. +-- should not be allowed to take aggressive locks on table +BEGIN; +SELECT lock_relation_if_exists('test', 'ACCESS SHARE'); + lock_relation_if_exists +------------------------- + t +(1 row) + +SELECT lock_relation_if_exists('test', 'EXCLUSIVE'); +ERROR: permission denied for relation test +ABORT; SET citus.task_executor_type TO 'real-time'; -- check no permission SET ROLE no_access; @@ -212,6 +238,12 @@ ERROR: permission denied for relation test ABORT; SELECT * FROM citus_stat_statements_reset(); ERROR: permission denied for function citus_stat_statements_reset +-- should not be allowed to upgrade to reference table +SELECT upgrade_to_reference_table('singleshard'); +ERROR: must be owner of relation singleshard +-- should not be allowed to co-located tables +SELECT mark_tables_colocated('test', ARRAY['test_coloc'::regclass]); +ERROR: must be owner of relation test -- table owner should be the same on the shards, even when distributing the table as superuser SET ROLE full_access; CREATE TABLE my_table (id integer, val integer); @@ -229,8 +261,7 @@ SELECT result FROM run_command_on_workers($$SELECT tableowner FROM pg_tables WHE full_access (2 rows) -DROP TABLE my_table; -DROP TABLE test; +DROP TABLE my_table, singleshard, test, test_coloc; DROP USER full_access; DROP USER read_access; DROP USER no_access; diff --git a/src/test/regress/sql/multi_multiuser.sql b/src/test/regress/sql/multi_multiuser.sql index 478e02925..160a74c7f 100644 --- a/src/test/regress/sql/multi_multiuser.sql +++ b/src/test/regress/sql/multi_multiuser.sql @@ -106,6 +106,12 @@ SELECT count(*) FROM test a JOIN test b ON (a.val = b.val) WHERE a.id = 1 AND b. -- should not be able to transmit directly COPY "postgresql.conf" TO STDOUT WITH (format transmit); +-- should not be allowed to take aggressive locks on table +BEGIN; +SELECT lock_relation_if_exists('test', 'ACCESS SHARE'); +SELECT lock_relation_if_exists('test', 'EXCLUSIVE'); +ABORT; + SET citus.task_executor_type TO 'real-time'; -- check no permission From e17025e1d4484c2c16fbe4d1149792bb2062c15e Mon Sep 17 00:00:00 2001 From: Marco Slot Date: Sat, 17 Nov 2018 00:01:14 +0100 Subject: [PATCH 3/3] Check table ownership in mark_tables_colocated --- src/backend/distributed/utils/colocation_utils.c | 6 +++++- src/test/regress/expected/multi_multiuser.out | 14 +++++++++++--- src/test/regress/sql/multi_multiuser.sql | 10 +++++++--- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/backend/distributed/utils/colocation_utils.c b/src/backend/distributed/utils/colocation_utils.c index d7c0e9a2a..ff273a99c 100644 --- a/src/backend/distributed/utils/colocation_utils.c +++ b/src/backend/distributed/utils/colocation_utils.c @@ -78,8 +78,9 @@ mark_tables_colocated(PG_FUNCTION_ARGS) "operation"))); } - EnsureCoordinator(); CheckCitusVersion(ERROR); + EnsureCoordinator(); + EnsureTableOwner(sourceRelationId); relationIdDatumArray = DeconstructArrayObject(relationIdArrayObject); @@ -87,6 +88,9 @@ mark_tables_colocated(PG_FUNCTION_ARGS) { Oid nextRelationOid = DatumGetObjectId(relationIdDatumArray[relationIndex]); + /* we require that the user either owns all tables or is superuser */ + EnsureTableOwner(nextRelationOid); + MarkTablesColocated(sourceRelationId, nextRelationOid); } diff --git a/src/test/regress/expected/multi_multiuser.out b/src/test/regress/expected/multi_multiuser.out index fb94d9917..5d04ddfa1 100644 --- a/src/test/regress/expected/multi_multiuser.out +++ b/src/test/regress/expected/multi_multiuser.out @@ -21,6 +21,13 @@ SELECT create_distributed_table('test', 'id'); (1 row) +CREATE TABLE test_coloc (id integer, val integer); +SELECT create_distributed_table('test_coloc', 'id', colocate_with := 'none'); + create_distributed_table +-------------------------- + +(1 row) + SET citus.shard_count TO 1; CREATE TABLE singleshard (id integer, val integer); SELECT create_distributed_table('singleshard', 'id'); @@ -234,6 +241,9 @@ ERROR: permission denied for function citus_stat_statements_reset -- should not be allowed to upgrade to reference table SELECT upgrade_to_reference_table('singleshard'); ERROR: must be owner of table singleshard +-- should not be allowed to co-located tables +SELECT mark_tables_colocated('test', ARRAY['test_coloc'::regclass]); +ERROR: must be owner of table test -- table owner should be the same on the shards, even when distributing the table as superuser SET ROLE full_access; CREATE TABLE my_table (id integer, val integer); @@ -251,9 +261,7 @@ SELECT result FROM run_command_on_workers($$SELECT tableowner FROM pg_tables WHE full_access (2 rows) -DROP TABLE my_table; -DROP TABLE test; -DROP TABLE singleshard; +DROP TABLE my_table, singleshard, test, test_coloc; DROP USER full_access; DROP USER read_access; DROP USER no_access; diff --git a/src/test/regress/sql/multi_multiuser.sql b/src/test/regress/sql/multi_multiuser.sql index 160a74c7f..6b9529c33 100644 --- a/src/test/regress/sql/multi_multiuser.sql +++ b/src/test/regress/sql/multi_multiuser.sql @@ -16,6 +16,9 @@ SET citus.shard_replication_factor TO 1; CREATE TABLE test (id integer, val integer); SELECT create_distributed_table('test', 'id'); +CREATE TABLE test_coloc (id integer, val integer); +SELECT create_distributed_table('test_coloc', 'id', colocate_with := 'none'); + SET citus.shard_count TO 1; CREATE TABLE singleshard (id integer, val integer); SELECT create_distributed_table('singleshard', 'id'); @@ -151,6 +154,9 @@ SELECT * FROM citus_stat_statements_reset(); -- should not be allowed to upgrade to reference table SELECT upgrade_to_reference_table('singleshard'); +-- should not be allowed to co-located tables +SELECT mark_tables_colocated('test', ARRAY['test_coloc'::regclass]); + -- table owner should be the same on the shards, even when distributing the table as superuser SET ROLE full_access; CREATE TABLE my_table (id integer, val integer); @@ -158,9 +164,7 @@ RESET ROLE; SELECT create_distributed_table('my_table', 'id'); SELECT result FROM run_command_on_workers($$SELECT tableowner FROM pg_tables WHERE tablename LIKE 'my_table_%' LIMIT 1$$); -DROP TABLE my_table; -DROP TABLE test; -DROP TABLE singleshard; +DROP TABLE my_table, singleshard, test, test_coloc; DROP USER full_access; DROP USER read_access; DROP USER no_access;