From b70650c17451dfee2ec753f66bb9ce93de2d7c8f Mon Sep 17 00:00:00 2001 From: Mats Kindahl Date: Tue, 7 Nov 2023 14:16:39 +0100 Subject: [PATCH] Repair relacl on upgrade If users have accidentally been removed from `pg_authid` as a result of bugs where dropping a user did not revoke privileges from all tables where the had privileges, it will not be possible to create new chunks since these require the user to be found when copying the privileges for the parent table (either compressed hypertable or normal hypertable). To fix the situation, we repair the `pg_class` table by updating the `relacl` for relations and remove any user that do not have an entry in `pg_authid`. --- .unreleased/fix_6290 | 1 + sql/maintenance_utils.sql | 40 ++++++++++ sql/updates/latest-dev.sql | 31 ++++++++ src/utils.c | 100 +++++++++++++++++++++++++ test/expected/repair.out | 74 ++++++++++++++++++ test/expected/util.out | 8 ++ test/sql/CMakeLists.txt | 1 + test/sql/repair.sql | 61 +++++++++++++++ test/sql/util.sql | 11 +++ tsl/test/shared/expected/extension.out | 2 + 10 files changed, 329 insertions(+) create mode 100644 .unreleased/fix_6290 create mode 100644 test/expected/repair.out create mode 100644 test/sql/repair.sql diff --git a/.unreleased/fix_6290 b/.unreleased/fix_6290 new file mode 100644 index 00000000000..a1ae57435cf --- /dev/null +++ b/.unreleased/fix_6290 @@ -0,0 +1 @@ +Fixes: #6290 Repair relacl on upgrade diff --git a/sql/maintenance_utils.sql b/sql/maintenance_utils.sql index 23a77a95ab7..6562686e9e4 100644 --- a/sql/maintenance_utils.sql +++ b/sql/maintenance_utils.sql @@ -117,3 +117,43 @@ BEGIN END CASE; END $$ LANGUAGE plpgsql; + +-- A version of makeaclitem that accepts a comma-separated list of +-- privileges rather than just a single privilege. This is copied from +-- PG16, but since we need to support earlier versions, we provide it +-- with the extension. +-- +-- This is intended for internal usage and interface might change. +CREATE FUNCTION _timescaledb_functions.makeaclitem(regrole, regrole, text, bool) +RETURNS AclItem AS '@MODULE_PATHNAME@', 'ts_makeaclitem' +LANGUAGE C STABLE PARALLEL SAFE; + +-- Repair relation ACL by removing roles that do not exist in pg_authid. +CREATE PROCEDURE _timescaledb_functions.repair_relation_acls() +LANGUAGE SQL AS $$ + WITH + badrels AS ( + SELECT oid::regclass + FROM (SELECT oid, (aclexplode(relacl)).* FROM pg_class) AS rels + WHERE rels.grantee != 0 + AND rels.grantee NOT IN (SELECT oid FROM pg_authid) + ), + pickacls AS ( + SELECT oid::regclass, + _timescaledb_functions.makeaclitem( + b.grantee, + b.grantor, + string_agg(b.privilege_type, ','), + b.is_grantable + ) AS acl + FROM (SELECT oid, (aclexplode(relacl)).* AS a FROM pg_class) AS b + WHERE b.grantee IN (SELECT oid FROM pg_authid) + GROUP BY oid, b.grantee, b.grantor, b.is_grantable + ), + cleanacls AS ( + SELECT oid, array_agg(acl) AS acl FROM pickacls GROUP BY oid + ) + UPDATE pg_class c + SET relacl = (SELECT acl FROM cleanacls n WHERE c.oid = n.oid) + WHERE oid IN (SELECT oid FROM badrels) +$$ SET search_path TO pg_catalog, pg_temp; diff --git a/sql/updates/latest-dev.sql b/sql/updates/latest-dev.sql index e743b81bd3f..ab55259edf9 100644 --- a/sql/updates/latest-dev.sql +++ b/sql/updates/latest-dev.sql @@ -250,3 +250,34 @@ CREATE FUNCTION @extschema@.show_chunks( created_after "any" = NULL ) RETURNS SETOF REGCLASS AS '@MODULE_PATHNAME@', 'ts_chunk_show_chunks' LANGUAGE C STABLE PARALLEL SAFE; + +CREATE FUNCTION _timescaledb_functions.makeaclitem(regrole, regrole, text, bool) +RETURNS AclItem AS '@MODULE_PATHNAME@', 'ts_makeaclitem' +LANGUAGE C STABLE PARALLEL SAFE; + +-- Repair relations that have relacl entries for users that do not exist in pg_authid +WITH + badrels AS ( + SELECT oid::regclass + FROM (SELECT oid, (aclexplode(relacl)).* FROM pg_catalog.pg_class) AS rels + WHERE rels.grantee != 0 + AND rels.grantee NOT IN (SELECT oid FROM pg_catalog.pg_authid) + ), + pickacls AS ( + SELECT oid::regclass, + _timescaledb_functions.makeaclitem( + b.grantee, + b.grantor, + string_agg(b.privilege_type, ','), + b.is_grantable + ) AS acl + FROM (SELECT oid, (aclexplode(relacl)).* AS a FROM pg_catalog.pg_class) AS b + WHERE b.grantee IN (SELECT oid FROM pg_catalog.pg_authid) + GROUP BY oid, b.grantee, b.grantor, b.is_grantable + ), + cleanacls AS ( + SELECT oid, array_agg(acl) AS acl FROM pickacls GROUP BY oid + ) +UPDATE pg_catalog.pg_class c + SET relacl = (SELECT acl FROM cleanacls n WHERE c.oid = n.oid) + WHERE oid IN (SELECT oid FROM badrels); diff --git a/src/utils.c b/src/utils.c index ab07add3314..6ce5b1493e2 100644 --- a/src/utils.c +++ b/src/utils.c @@ -42,7 +42,14 @@ #include "utils.h" #include "time_utils.h" +typedef struct +{ + const char *name; + AclMode value; +} priv_map; + TS_FUNCTION_INFO_V1(ts_pg_timestamp_to_unix_microseconds); +TS_FUNCTION_INFO_V1(ts_makeaclitem); /* * Convert a Postgres TIMESTAMP to BIGINT microseconds relative the UNIX epoch. @@ -1378,3 +1385,96 @@ ts_table_has_tuples(Oid table_relid, LOCKMODE lockmode) table_close(rel, lockmode); return hastuples; } + +/* + * This is copied from PostgreSQL 16.0 since versions before 16.0 does not + * support lists for privileges. + */ +static AclMode +convert_any_priv_string(text *priv_type_text, const priv_map *privileges) +{ + AclMode result = 0; + char *priv_type = text_to_cstring(priv_type_text); + char *chunk; + char *next_chunk; + + /* We rely on priv_type being a private, modifiable string */ + for (chunk = priv_type; chunk; chunk = next_chunk) + { + int chunk_len; + const priv_map *this_priv; + + /* Split string at commas */ + next_chunk = strchr(chunk, ','); + if (next_chunk) + *next_chunk++ = '\0'; + + /* Drop leading/trailing whitespace in this chunk */ + while (*chunk && isspace((unsigned char) *chunk)) + chunk++; + chunk_len = strlen(chunk); + while (chunk_len > 0 && isspace((unsigned char) chunk[chunk_len - 1])) + chunk_len--; + chunk[chunk_len] = '\0'; + + /* Match to the privileges list */ + for (this_priv = privileges; this_priv->name; this_priv++) + { + if (pg_strcasecmp(this_priv->name, chunk) == 0) + { + result |= this_priv->value; + break; + } + } + if (!this_priv->name) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("unrecognized privilege type: \"%s\"", chunk))); + } + + pfree(priv_type); + return result; +} + +Datum +ts_makeaclitem(PG_FUNCTION_ARGS) +{ + Oid grantee = PG_GETARG_OID(0); + Oid grantor = PG_GETARG_OID(1); + text *privtext = PG_GETARG_TEXT_PP(2); + bool goption = PG_GETARG_BOOL(3); + AclItem *result; + AclMode priv; + static const priv_map any_priv_map[] = { + { "SELECT", ACL_SELECT }, + { "INSERT", ACL_INSERT }, + { "UPDATE", ACL_UPDATE }, + { "DELETE", ACL_DELETE }, + { "TRUNCATE", ACL_TRUNCATE }, + { "REFERENCES", ACL_REFERENCES }, + { "TRIGGER", ACL_TRIGGER }, + { "EXECUTE", ACL_EXECUTE }, + { "USAGE", ACL_USAGE }, + { "CREATE", ACL_CREATE }, + { "TEMP", ACL_CREATE_TEMP }, + { "TEMPORARY", ACL_CREATE_TEMP }, + { "CONNECT", ACL_CONNECT }, +#if PG16_GE + { "SET", ACL_SET }, + { "ALTER SYSTEM", ACL_ALTER_SYSTEM }, +#endif + { "RULE", 0 }, /* ignore old RULE privileges */ + { NULL, 0 } + }; + + priv = convert_any_priv_string(privtext, any_priv_map); + + result = (AclItem *) palloc(sizeof(AclItem)); + + result->ai_grantee = grantee; + result->ai_grantor = grantor; + + ACLITEM_SET_PRIVS_GOPTIONS(*result, priv, (goption ? priv : ACL_NO_RIGHTS)); + + PG_RETURN_ACLITEM_P(result); +} diff --git a/test/expected/repair.out b/test/expected/repair.out new file mode 100644 index 00000000000..1b4db956c87 --- /dev/null +++ b/test/expected/repair.out @@ -0,0 +1,74 @@ +-- This file and its contents are licensed under the Apache License 2.0. +-- Please see the included NOTICE for copyright information and +-- LICENSE-APACHE for a copy of the license. +-- We are testing different repair functions here to make sure that +-- they work as expected. +\c :TEST_DBNAME :ROLE_SUPERUSER +CREATE USER wizard; +CREATE USER "Random L User"; +CREATE TABLE test_table_1(time timestamptz not null, temp float); +SELECT create_hypertable('test_table_1', by_range('time', '1 day'::interval)); + create_hypertable +------------------- + (1,t) +(1 row) + +INSERT INTO test_table_1(time,temp) +SELECT time, 100 * random() + FROM generate_series( + '2000-01-01'::timestamptz, + '2000-01-05'::timestamptz, + '1min'::interval + ) time; +CREATE TABLE test_table_2(time timestamptz not null, temp float); +SELECT create_hypertable('test_table_2', by_range('time', '1 day'::interval)); + create_hypertable +------------------- + (2,t) +(1 row) + +INSERT INTO test_table_2(time,temp) +SELECT time, 100 * random() + FROM generate_series( + '2000-01-01'::timestamptz, + '2000-01-05'::timestamptz, + '1min'::interval + ) time; +GRANT ALL ON test_table_1 TO wizard; +GRANT ALL ON test_table_2 TO wizard; +GRANT SELECT, INSERT ON test_table_1 TO "Random L User"; +GRANT INSERT ON test_table_2 TO "Random L User"; +-- Break the relacl of the table by deleting users +DELETE FROM pg_authid WHERE rolname IN ('wizard', 'Random L User'); +CREATE TABLE saved (LIKE pg_class); +INSERT INTO saved SELECT * FROM pg_class; +CALL _timescaledb_functions.repair_relation_acls(); +-- The only thing we should see here are the relations we broke and +-- the privileges we added for that user. No other relations should be +-- touched. +WITH + lhs AS (SELECT oid, aclexplode(relacl) FROM pg_class), + rhs AS (SELECT oid, aclexplode(relacl) FROM saved) +SELECT rhs.oid::regclass +FROM lhs FULL OUTER JOIN rhs ON row(lhs) = row(rhs) +WHERE lhs.oid IS NULL AND rhs.oid IS NOT NULL +GROUP BY rhs.oid; + oid +----------------------------------------- + test_table_1 + _timescaledb_internal._hyper_1_1_chunk + _timescaledb_internal._hyper_1_2_chunk + _timescaledb_internal._hyper_1_3_chunk + _timescaledb_internal._hyper_1_4_chunk + _timescaledb_internal._hyper_1_5_chunk + test_table_2 + _timescaledb_internal._hyper_2_6_chunk + _timescaledb_internal._hyper_2_7_chunk + _timescaledb_internal._hyper_2_8_chunk + _timescaledb_internal._hyper_2_9_chunk + _timescaledb_internal._hyper_2_10_chunk +(12 rows) + +DROP TABLE saved; +DROP TABLE test_table_1; +DROP TABLE test_table_2; diff --git a/test/expected/util.out b/test/expected/util.out index cbaaf466a1f..25a5edda548 100644 --- a/test/expected/util.out +++ b/test/expected/util.out @@ -2,3 +2,11 @@ -- Please see the included NOTICE for copyright information and -- LICENSE-APACHE for a copy of the license. \set ECHO errors + item +-------------------- + wizard=a/wizard + wizard=ar/wizard + wizard=a*/wizard + wizard=a*r*/wizard +(4 rows) + diff --git a/test/sql/CMakeLists.txt b/test/sql/CMakeLists.txt index 928f3d41977..c7f3b4defcc 100644 --- a/test/sql/CMakeLists.txt +++ b/test/sql/CMakeLists.txt @@ -42,6 +42,7 @@ set(TEST_FILES plan_hypertable_inline.sql relocate_extension.sql reloptions.sql + repair.sql size_utils.sql sort_optimization.sql sql_query.sql diff --git a/test/sql/repair.sql b/test/sql/repair.sql new file mode 100644 index 00000000000..4bcd19b87c0 --- /dev/null +++ b/test/sql/repair.sql @@ -0,0 +1,61 @@ +-- This file and its contents are licensed under the Apache License 2.0. +-- Please see the included NOTICE for copyright information and +-- LICENSE-APACHE for a copy of the license. + +-- We are testing different repair functions here to make sure that +-- they work as expected. + +\c :TEST_DBNAME :ROLE_SUPERUSER + +CREATE USER wizard; +CREATE USER "Random L User"; + +CREATE TABLE test_table_1(time timestamptz not null, temp float); +SELECT create_hypertable('test_table_1', by_range('time', '1 day'::interval)); + +INSERT INTO test_table_1(time,temp) +SELECT time, 100 * random() + FROM generate_series( + '2000-01-01'::timestamptz, + '2000-01-05'::timestamptz, + '1min'::interval + ) time; + +CREATE TABLE test_table_2(time timestamptz not null, temp float); +SELECT create_hypertable('test_table_2', by_range('time', '1 day'::interval)); + +INSERT INTO test_table_2(time,temp) +SELECT time, 100 * random() + FROM generate_series( + '2000-01-01'::timestamptz, + '2000-01-05'::timestamptz, + '1min'::interval + ) time; + +GRANT ALL ON test_table_1 TO wizard; +GRANT ALL ON test_table_2 TO wizard; +GRANT SELECT, INSERT ON test_table_1 TO "Random L User"; +GRANT INSERT ON test_table_2 TO "Random L User"; + +-- Break the relacl of the table by deleting users +DELETE FROM pg_authid WHERE rolname IN ('wizard', 'Random L User'); + +CREATE TABLE saved (LIKE pg_class); +INSERT INTO saved SELECT * FROM pg_class; + +CALL _timescaledb_functions.repair_relation_acls(); + +-- The only thing we should see here are the relations we broke and +-- the privileges we added for that user. No other relations should be +-- touched. +WITH + lhs AS (SELECT oid, aclexplode(relacl) FROM pg_class), + rhs AS (SELECT oid, aclexplode(relacl) FROM saved) +SELECT rhs.oid::regclass +FROM lhs FULL OUTER JOIN rhs ON row(lhs) = row(rhs) +WHERE lhs.oid IS NULL AND rhs.oid IS NOT NULL +GROUP BY rhs.oid; + +DROP TABLE saved; +DROP TABLE test_table_1; +DROP TABLE test_table_2; diff --git a/test/sql/util.sql b/test/sql/util.sql index 881b2289e6b..371406b76f9 100644 --- a/test/sql/util.sql +++ b/test/sql/util.sql @@ -4,6 +4,7 @@ \set ECHO errors \set VERBOSITY default +\c :TEST_DBNAME :ROLE_SUPERUSER DO $$ BEGIN @@ -11,3 +12,13 @@ BEGIN ASSERT( _timescaledb_functions.get_partition_for_key('dev1'::text) = 1129986420 ); ASSERT( _timescaledb_functions.get_partition_for_key('longlonglonglongpartitionkey'::text) = 1169179734); END$$; + +CREATE USER wizard; +SELECT * FROM ( + VALUES + (_timescaledb_functions.makeaclitem('wizard', 'wizard', 'insert', false)), + (_timescaledb_functions.makeaclitem('wizard', 'wizard', 'insert,select', false)), + (_timescaledb_functions.makeaclitem('wizard', 'wizard', 'insert', true)), + (_timescaledb_functions.makeaclitem('wizard', 'wizard', 'insert,select', true))) + AS t(item); +DROP USER wizard; diff --git a/tsl/test/shared/expected/extension.out b/tsl/test/shared/expected/extension.out index edc9142d60f..cc7825ec7f7 100644 --- a/tsl/test/shared/expected/extension.out +++ b/tsl/test/shared/expected/extension.out @@ -107,6 +107,7 @@ ORDER BY pronamespace::regnamespace::text COLLATE "C", p.oid::regprocedure::text _timescaledb_functions.invalidation_process_hypertable_log(integer,integer,regtype,integer[],bigint[],bigint[],text[]) _timescaledb_functions.last_combinefunc(internal,internal) _timescaledb_functions.last_sfunc(internal,anyelement,"any") + _timescaledb_functions.makeaclitem(regrole,regrole,text,boolean) _timescaledb_functions.materialization_invalidation_log_delete(integer) _timescaledb_functions.partialize_agg(anyelement) _timescaledb_functions.ping_data_node(name,interval) @@ -127,6 +128,7 @@ ORDER BY pronamespace::regnamespace::text COLLATE "C", p.oid::regprocedure::text _timescaledb_functions.recompress_chunk_segmentwise(regclass,boolean) _timescaledb_functions.relation_size(regclass) _timescaledb_functions.remote_txn_heal_data_node(oid) + _timescaledb_functions.repair_relation_acls() _timescaledb_functions.restart_background_workers() _timescaledb_functions.rxid_in(cstring) _timescaledb_functions.rxid_out(rxid)