From f2310216a8f6e028bd333e0be69af89213290185 Mon Sep 17 00:00:00 2001 From: Mats Kindahl Date: Tue, 7 Nov 2023 14:16:39 +0100 Subject: [PATCH] Repair relacl on extension 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 when updating the extension by modifying the `relacl` for relations and remove any user that do not have an entry in `pg_authid`. A repair function `_timescaledb_functions.repair_relation_acls` is added that will perform the job. A `makeaclitem` from PG16 that accepts a list of comma and used as part of the repair is also added as `_timescaledb_functions.makeaclitem`. --- .unreleased/fix_6290 | 1 + sql/maintenance_utils.sql | 40 ++++++++++ sql/updates/post-update.sql | 3 + sql/updates/reverse-dev.sql | 3 + src/utils.c | 104 +++++++++++++++++++++++++ test/expected/repair.out | 74 ++++++++++++++++++ test/sql/CMakeLists.txt | 1 + test/sql/repair.sql | 61 +++++++++++++++ test/sql/updates/post.repair.sql | 4 + test/sql/updates/setup.repair.sql | 13 ++++ tsl/test/shared/expected/extension.out | 2 + 11 files changed, 306 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 000000000..a1ae57435 --- /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 23a77a95a..6562686e9 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/post-update.sql b/sql/updates/post-update.sql index f739eb857..5e211828b 100644 --- a/sql/updates/post-update.sql +++ b/sql/updates/post-update.sql @@ -194,3 +194,6 @@ BEGIN END; $$; +-- Repair relations that have relacl entries for users that do not +-- exist in pg_authid +CALL _timescaledb_functions.repair_relation_acls(); diff --git a/sql/updates/reverse-dev.sql b/sql/updates/reverse-dev.sql index 647e567d6..0b638966e 100644 --- a/sql/updates/reverse-dev.sql +++ b/sql/updates/reverse-dev.sql @@ -191,3 +191,6 @@ CREATE FUNCTION @extschema@.show_chunks( newer_than "any" = NULL ) RETURNS SETOF REGCLASS AS '@MODULE_PATHNAME@', 'ts_chunk_show_chunks' LANGUAGE C STABLE PARALLEL SAFE; + +DROP PROCEDURE IF EXISTS _timescaledb_functions.repair_relation_acls(); +DROP FUNCTION IF EXISTS _timescaledb_functions.makeaclitem(regrole, regrole, text, bool); diff --git a/src/utils.c b/src/utils.c index ab07add33..a005c086d 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,100 @@ 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 +ts_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; +} + +/* + * This is copied from PostgreSQL 16.0 since versions before 16.0 does not + * support lists for privileges but we need that. + */ +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 = ts_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 000000000..1b4db956c --- /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/sql/CMakeLists.txt b/test/sql/CMakeLists.txt index 928f3d419..c7f3b4def 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 000000000..4bcd19b87 --- /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/updates/post.repair.sql b/test/sql/updates/post.repair.sql index b63c7b64c..acce4bf57 100644 --- a/test/sql/updates/post.repair.sql +++ b/test/sql/updates/post.repair.sql @@ -24,3 +24,7 @@ WHERE extname = 'timescaledb' \gset \ir post.repair.hierarchical_cagg.sql \endif +\z repair_test_int +\z repair_test_extra + + diff --git a/test/sql/updates/setup.repair.sql b/test/sql/updates/setup.repair.sql index 975ca01f0..3d12fbe2e 100644 --- a/test/sql/updates/setup.repair.sql +++ b/test/sql/updates/setup.repair.sql @@ -15,6 +15,9 @@ SELECT extversion >= '2.10.0' AS has_cagg_joins FROM pg_extension WHERE extname = 'timescaledb' \gset +CREATE USER wizard; +CREATE USER "Random L User"; + CREATE TABLE repair_test_int(time integer not null, temp float8, tag integer, color integer); CREATE TABLE repair_test_timestamptz(time timestamptz not null, temp float8, tag integer, color integer); CREATE TABLE repair_test_extra(time timestamptz not null, temp float8, tag integer, color integer); @@ -65,6 +68,16 @@ INSERT INTO repair_test_date VALUES ALTER TABLE _timescaledb_catalog.chunk_constraint DROP CONSTRAINT chunk_constraint_dimension_slice_id_fkey; +-- Grant privileges to some tables above. All should be repaired. +GRANT ALL ON repair_test_int TO wizard; +GRANT ALL ON repair_test_extra TO wizard; +GRANT SELECT, INSERT ON repair_test_int TO "Random L User"; +GRANT INSERT ON repair_test_extra TO "Random L User"; + +-- Break the relacl of the table by deleting users directly from +-- pg_authid table. +DELETE FROM pg_authid WHERE rolname IN ('wizard', 'Random L User'); + \if :test_repair_dimension CREATE VIEW slices AS ( diff --git a/tsl/test/shared/expected/extension.out b/tsl/test/shared/expected/extension.out index edc9142d6..cc7825ec7 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)