From 052ba21b19100ee7eec04bd71ad3e24076d78694 Mon Sep 17 00:00:00 2001 From: Onder Kalaci Date: Tue, 13 Nov 2018 18:27:01 +0300 Subject: [PATCH] Make sure to prevent unauthorized users to drop sequences in Citus MX --- .../master/master_delete_protocol.c | 37 ++++- .../master/master_metadata_utility.c | 15 ++ .../distributed/master_metadata_utility.h | 1 + src/include/distributed/version_compat.h | 4 + .../regress/expected/multi_mx_metadata.out | 156 +++++++++++++++--- src/test/regress/sql/multi_mx_metadata.sql | 95 ++++++++++- 6 files changed, 271 insertions(+), 37 deletions(-) diff --git a/src/backend/distributed/master/master_delete_protocol.c b/src/backend/distributed/master/master_delete_protocol.c index 91a0b70a3..ce39f1cef 100644 --- a/src/backend/distributed/master/master_delete_protocol.c +++ b/src/backend/distributed/master/master_delete_protocol.c @@ -58,6 +58,9 @@ #include "utils/elog.h" #include "utils/errcodes.h" #include "utils/lsyscache.h" +#if (PG_VERSION_NUM >= 100000) +#include "utils/varlena.h" +#endif /* Local functions forward declarations */ @@ -265,29 +268,51 @@ master_drop_sequences(PG_FUNCTION_ARGS) { ArrayType *sequenceNamesArray = PG_GETARG_ARRAYTYPE_P(0); ArrayIterator sequenceIterator = NULL; - Datum sequenceText = 0; + Datum sequenceNameDatum = 0; bool isNull = false; StringInfo dropSeqCommand = makeStringInfo(); - bool coordinator = IsCoordinator(); CheckCitusVersion(ERROR); - /* do nothing if DDL propagation is switched off or this is not the coordinator */ - if (!EnableDDLPropagation || !coordinator) + /* + * Do nothing if DDL propagation is switched off or we're not on + * the coordinator. Here we prefer to not error out on the workers + * because this function is called on every dropped sequence and + * we don't want to mess up the sequences that are not associated + * with distributed tables. + */ + if (!EnableDDLPropagation || !IsCoordinator()) { PG_RETURN_VOID(); } /* iterate over sequence names to build single command to DROP them all */ sequenceIterator = array_create_iterator(sequenceNamesArray, 0, NULL); - while (array_iterate(sequenceIterator, &sequenceText, &isNull)) + while (array_iterate(sequenceIterator, &sequenceNameDatum, &isNull)) { + text *sequenceNameText = NULL; + Oid sequenceOid = InvalidOid; + if (isNull) { ereport(ERROR, (errmsg("unexpected NULL sequence name"), errcode(ERRCODE_INVALID_PARAMETER_VALUE))); } + sequenceNameText = DatumGetTextP(sequenceNameDatum); + sequenceOid = ResolveRelationId(sequenceNameText, true); + if (OidIsValid(sequenceOid)) + { + /* + * This case (e.g., OID is valid) could only happen when a user manually calls + * the UDF. So, ensure that the user has right to drop the sequence. + * + * In case the UDF is called via the DROP trigger, the OID wouldn't be valid since + * the trigger is called after DROP happens. + */ + EnsureSequenceOwner(sequenceOid); + } + /* append command portion if we haven't added any sequence names yet */ if (dropSeqCommand->len == 0) { @@ -299,7 +324,7 @@ master_drop_sequences(PG_FUNCTION_ARGS) appendStringInfoChar(dropSeqCommand, ','); } - appendStringInfo(dropSeqCommand, " %s", TextDatumGetCString(sequenceText)); + appendStringInfo(dropSeqCommand, " %s", TextDatumGetCString(sequenceNameText)); } if (dropSeqCommand->len != 0) diff --git a/src/backend/distributed/master/master_metadata_utility.c b/src/backend/distributed/master/master_metadata_utility.c index 8b9d042e2..134d40eb2 100644 --- a/src/backend/distributed/master/master_metadata_utility.c +++ b/src/backend/distributed/master/master_metadata_utility.c @@ -1346,6 +1346,21 @@ EnsureTableOwner(Oid relationId) } +/* + * Check that the current user has owner rights to sequenceRelationId, error out if + * not. Superusers are regarded as owners. + */ +void +EnsureSequenceOwner(Oid sequenceOid) +{ + if (!pg_class_ownercheck(sequenceOid, GetUserId())) + { + aclcheck_error(ACLCHECK_NOT_OWNER, ACLCHECK_OBJECT_SEQUENCE, + get_rel_name(sequenceOid)); + } +} + + /* * EnsureSuperUser check that the current user is a superuser and errors out if not. */ diff --git a/src/include/distributed/master_metadata_utility.h b/src/include/distributed/master_metadata_utility.h index 5f63392b4..1461fd473 100644 --- a/src/include/distributed/master_metadata_utility.h +++ b/src/include/distributed/master_metadata_utility.h @@ -155,6 +155,7 @@ extern void CreateTruncateTrigger(Oid relationId); extern char * TableOwner(Oid relationId); extern void EnsureTablePermissions(Oid relationId, AclMode mode); extern void EnsureTableOwner(Oid relationId); +extern void EnsureSequenceOwner(Oid sequenceOid); extern void EnsureSuperUser(void); extern void EnsureReplicationSettings(Oid relationId, char replicationModel); extern bool RegularTable(Oid relationId); diff --git a/src/include/distributed/version_compat.h b/src/include/distributed/version_compat.h index f7e1e09e0..5d75089fa 100644 --- a/src/include/distributed/version_compat.h +++ b/src/include/distributed/version_compat.h @@ -67,6 +67,8 @@ struct QueryEnvironment; /* forward-declare to appease compiler */ #define ACLCHECK_OBJECT_TABLE ACL_KIND_CLASS #define ACLCHECK_OBJECT_SCHEMA ACL_KIND_NAMESPACE #define ACLCHECK_OBJECT_INDEX ACL_KIND_CLASS +#define ACLCHECK_OBJECT_SEQUENCE ACL_KIND_CLASS + static inline int BasicOpenFilePerm(FileName fileName, int fileFlags, int fileMode) @@ -158,6 +160,8 @@ canonicalize_qual_compat(Expr *qual, bool is_check) #define ACLCHECK_OBJECT_TABLE OBJECT_TABLE #define ACLCHECK_OBJECT_SCHEMA OBJECT_SCHEMA #define ACLCHECK_OBJECT_INDEX OBJECT_INDEX +#define ACLCHECK_OBJECT_SEQUENCE OBJECT_SEQUENCE + #define ConstraintRelidIndexId ConstraintRelidTypidNameIndexId diff --git a/src/test/regress/expected/multi_mx_metadata.out b/src/test/regress/expected/multi_mx_metadata.out index 9e8df19c4..4bab9a2af 100644 --- a/src/test/regress/expected/multi_mx_metadata.out +++ b/src/test/regress/expected/multi_mx_metadata.out @@ -7,12 +7,25 @@ SELECT pg_reload_conf(); t (1 row) +-- pg 10 and pg 11 has different error messages for acl checks +-- so catch them and print only one type of error message to prevent +-- multiple output test files +CREATE OR REPLACE FUNCTION raise_failed_aclcheck(query text) RETURNS void AS $$ +BEGIN + EXECUTE query; + EXCEPTION WHEN OTHERS THEN + IF SQLERRM LIKE 'must be owner of%' THEN + RAISE 'must be owner of the object'; + END IF; +END; +$$LANGUAGE plpgsql; -- get rid of the previously created entries in pg_dist_transaction -- for the sake of getting consistent results in this test file TRUNCATE pg_dist_transaction; CREATE TABLE distributed_mx_table ( key text primary key, - value jsonb + value jsonb, + some_val bigserial ); CREATE INDEX ON distributed_mx_table USING GIN (value); SET citus.shard_replication_factor TO 1; @@ -47,11 +60,12 @@ SELECT count(*) FROM pg_dist_transaction; \c - - - :worker_1_port SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass; - Column | Type | Modifiers ---------+-------+----------- - key | text | not null - value | jsonb | -(2 rows) + Column | Type | Modifiers +----------+--------+------------------------------------------------------------------------- + key | text | not null + value | jsonb | + some_val | bigint | not null default nextval('distributed_mx_table_some_val_seq'::regclass) +(3 rows) SELECT "Column", "Type", "Definition" FROM index_attrs WHERE relid = 'distributed_mx_table_pkey'::regclass; @@ -83,11 +97,12 @@ WHERE logicalrelid = 'distributed_mx_table'::regclass; \c - - - :worker_2_port SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass; - Column | Type | Modifiers ---------+-------+----------- - key | text | not null - value | jsonb | -(2 rows) + Column | Type | Modifiers +----------+--------+------------------------------------------------------------------------- + key | text | not null + value | jsonb | + some_val | bigint | not null default nextval('distributed_mx_table_some_val_seq'::regclass) +(3 rows) SELECT "Column", "Type", "Definition" FROM index_attrs WHERE relid = 'distributed_mx_table_pkey'::regclass; @@ -299,25 +314,120 @@ SELECT run_command_on_workers($$CREATE USER no_access_mx;$$); (2 rows) SET ROLE no_access_mx; -DROP TABLE distributed_mx_table; -ERROR: must be owner of table distributed_mx_table -SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); -ERROR: must be owner of table distributed_mx_table -SELECT master_drop_all_shards('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); -ERROR: must be owner of table distributed_mx_table -SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); -ERROR: must be owner of table distributed_mx_table +SELECT raise_failed_aclcheck($$ + DROP TABLE distributed_mx_table; +$$); +ERROR: must be owner of the object +CONTEXT: PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE +SELECT raise_failed_aclcheck($$ + SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); + $$); +ERROR: must be owner of the object +CONTEXT: PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE +SELECT raise_failed_aclcheck($$ + SELECT master_drop_all_shards('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); +$$); +ERROR: must be owner of the object +CONTEXT: PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE +SELECT raise_failed_aclcheck($$ + SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); +$$); +ERROR: must be owner of the object +CONTEXT: PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE +SELECT raise_failed_aclcheck($$ + SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq']); +$$); +ERROR: must be owner of the object +CONTEXT: PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE +SELECT raise_failed_aclcheck($$ + SELECT master_drop_sequences(ARRAY['distributed_mx_table_some_val_seq']); +$$); +ERROR: must be owner of the object +CONTEXT: PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE +SELECT master_drop_sequences(ARRAY['non_existing_schema.distributed_mx_table_some_val_seq']); + master_drop_sequences +----------------------- + +(1 row) + +SELECT master_drop_sequences(ARRAY['']); +ERROR: invalid name syntax +SELECT master_drop_sequences(ARRAY['public.']); +ERROR: invalid name syntax +SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq_not_existing']); + master_drop_sequences +----------------------- + +(1 row) + +-- make sure that we can drop unrelated tables/sequences +CREATE TABLE unrelated_table(key serial); +DROP TABLE unrelated_table; +-- doesn't error out but it has no effect, so no need to error out +SELECT master_drop_sequences(NULL); + master_drop_sequences +----------------------- + +(1 row) + +\c - postgres - :master_port +-- finally make sure that the sequence remains +SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass; + Column | Type | Modifiers +----------+--------+------------------------------------------------------------------------- + key | text | not null + value | jsonb | + some_val | bigint | not null default nextval('distributed_mx_table_some_val_seq'::regclass) +(3 rows) + \c - no_access_mx - :worker_1_port -DROP TABLE distributed_mx_table; -ERROR: must be owner of table distributed_mx_table -SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); -ERROR: must be owner of table distributed_mx_table +-- see the comment in the top of the file +CREATE OR REPLACE FUNCTION raise_failed_aclcheck(query text) RETURNS void AS $$ +BEGIN + EXECUTE query; + EXCEPTION WHEN OTHERS THEN + IF SQLERRM LIKE 'must be owner of%' THEN + RAISE 'must be owner of the object'; + END IF; +END; +$$LANGUAGE plpgsql; +SELECT raise_failed_aclcheck($$ + DROP TABLE distributed_mx_table; +$$); +ERROR: must be owner of the object +CONTEXT: PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE +SELECT raise_failed_aclcheck($$ + SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); + $$); +ERROR: must be owner of the object +CONTEXT: PL/pgSQL function raise_failed_aclcheck(text) line 6 at RAISE +SELECT raise_failed_aclcheck($$ + SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq']); +$$); + raise_failed_aclcheck +----------------------- + +(1 row) + SELECT master_drop_all_shards('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); ERROR: operation is not allowed on this node HINT: Connect to the coordinator and run it again. SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); ERROR: operation is not allowed on this node HINT: Connect to the coordinator and run it again. +-- make sure that we can drop unrelated tables/sequences +CREATE TABLE unrelated_table(key serial); +DROP TABLE unrelated_table; +\c - postgres - :worker_1_port +-- finally make sure that the sequence remains +SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass; + Column | Type | Modifiers +----------+--------+------------------------------------------------------------------------- + key | text | not null + value | jsonb | + some_val | bigint | not null default nextval('distributed_mx_table_some_val_seq'::regclass) +(3 rows) + -- Resume ordinary recovery \c - postgres - :master_port ALTER SYSTEM RESET citus.recover_2pc_interval; diff --git a/src/test/regress/sql/multi_mx_metadata.sql b/src/test/regress/sql/multi_mx_metadata.sql index 1ec1a420b..9d87500c1 100644 --- a/src/test/regress/sql/multi_mx_metadata.sql +++ b/src/test/regress/sql/multi_mx_metadata.sql @@ -4,13 +4,27 @@ ALTER SYSTEM SET citus.recover_2pc_interval TO -1; SELECT pg_reload_conf(); +-- pg 10 and pg 11 has different error messages for acl checks +-- so catch them and print only one type of error message to prevent +-- multiple output test files +CREATE OR REPLACE FUNCTION raise_failed_aclcheck(query text) RETURNS void AS $$ +BEGIN + EXECUTE query; + EXCEPTION WHEN OTHERS THEN + IF SQLERRM LIKE 'must be owner of%' THEN + RAISE 'must be owner of the object'; + END IF; +END; +$$LANGUAGE plpgsql; + -- get rid of the previously created entries in pg_dist_transaction -- for the sake of getting consistent results in this test file TRUNCATE pg_dist_transaction; CREATE TABLE distributed_mx_table ( key text primary key, - value jsonb + value jsonb, + some_val bigserial ); CREATE INDEX ON distributed_mx_table USING GIN (value); @@ -181,16 +195,81 @@ CREATE USER no_access_mx; SELECT run_command_on_workers($$CREATE USER no_access_mx;$$); SET ROLE no_access_mx; -DROP TABLE distributed_mx_table; -SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); + +SELECT raise_failed_aclcheck($$ + DROP TABLE distributed_mx_table; +$$); + +SELECT raise_failed_aclcheck($$ + SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); + $$); + +SELECT raise_failed_aclcheck($$ + SELECT master_drop_all_shards('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); +$$); +SELECT raise_failed_aclcheck($$ + SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); +$$); +SELECT raise_failed_aclcheck($$ + SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq']); +$$); +SELECT raise_failed_aclcheck($$ + SELECT master_drop_sequences(ARRAY['distributed_mx_table_some_val_seq']); +$$); + +SELECT master_drop_sequences(ARRAY['non_existing_schema.distributed_mx_table_some_val_seq']); +SELECT master_drop_sequences(ARRAY['']); +SELECT master_drop_sequences(ARRAY['public.']); +SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq_not_existing']); + +-- make sure that we can drop unrelated tables/sequences +CREATE TABLE unrelated_table(key serial); +DROP TABLE unrelated_table; + +-- doesn't error out but it has no effect, so no need to error out +SELECT master_drop_sequences(NULL); + +\c - postgres - :master_port + +-- finally make sure that the sequence remains +SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass; + +\c - no_access_mx - :worker_1_port + +-- see the comment in the top of the file +CREATE OR REPLACE FUNCTION raise_failed_aclcheck(query text) RETURNS void AS $$ +BEGIN + EXECUTE query; + EXCEPTION WHEN OTHERS THEN + IF SQLERRM LIKE 'must be owner of%' THEN + RAISE 'must be owner of the object'; + END IF; +END; +$$LANGUAGE plpgsql; + +SELECT raise_failed_aclcheck($$ + DROP TABLE distributed_mx_table; +$$); + +SELECT raise_failed_aclcheck($$ + SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); + $$); + +SELECT raise_failed_aclcheck($$ + SELECT master_drop_sequences(ARRAY['public.distributed_mx_table_some_val_seq']); +$$); + SELECT master_drop_all_shards('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); -\c - no_access_mx - :worker_1_port -DROP TABLE distributed_mx_table; -SELECT master_remove_distributed_table_metadata_from_workers('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); -SELECT master_drop_all_shards('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); -SELECT master_remove_partition_metadata('distributed_mx_table'::regclass, 'public', 'distributed_mx_table'); +-- make sure that we can drop unrelated tables/sequences +CREATE TABLE unrelated_table(key serial); +DROP TABLE unrelated_table; + +\c - postgres - :worker_1_port + +-- finally make sure that the sequence remains +SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='distributed_mx_table'::regclass; -- Resume ordinary recovery \c - postgres - :master_port