From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
To: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: allow_system_table_mods stuff
Date: Wed, 27 Nov 2019 17:01:49 +0100
Message-ID: <f48637b7-ab24-e48b-0b30-519b1170e65c@2ndquadrant.com> (raw)
In-Reply-To: <20191127082619.GB237562@paquier.xyz>
References: <a7455db0-2fc0-4ddc-4d8f-e4c860957a34@2ndquadrant.com>
<20190913213940.GA20917@alvherre.pgsql>
<20191127082619.GB237562@paquier.xyz>
On 2019-11-27 09:26, Michael Paquier wrote:
> On Fri, Sep 13, 2019 at 06:39:40PM -0300, Alvaro Herrera wrote:
>> I think this patch is at a point where it merits closer review from
>> fellow committers, so I marked it RfC for now. I hope non-committers
>> would also look at it some more, though.
>
> I guess so. The patch has conflicts in the serial and parallel
> schedules, so I have moved it to next CF, waiting on author for a
> rebase.
>
> Peter, are you planning to look at that again? Note: the patch has no
> reviewers registered.
Here is an updated patch series.
After re-reading the discussion again, I have kept the existing name of
the option. I have also moved the tests to the "unsafe_tests" suite,
which seems like a better place. And I have split the patch into three.
Other than those cosmetic changes, I think everything here has been
discussed and agreed to, so unless anyone expresses any concern or a
wish to do more review, I think this is ready to commit.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:download all (3 files, .tar.gz)v3-0001-Remove-any-user-DML-capability-from-allow_system_.patch (text/plain, 1.8K) download
From fbe9dd7d9fb4674cf2ca25b6cd4f05556c201d89 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 27 Nov 2019 16:32:50 +0100
Subject: [PATCH v3 1/3] Remove any-user DML capability from
allow_system_table_mods
Previously, allow_system_table_mods allowed a non-superuser to do DML
on a system table without further permission checks. This has been
removed, as it was quite inconsistent with the rest of the meaning of
this setting. (Since allow_system_table_mods was previously only
accessible with a server restart, it is unlikely that anyone was using
this possibility.)
Discussion: https://www.postgresql.org/message-id/flat/8b00ea5e-28a7-88ba-e848-21528b632354%402ndquadrant.com
---
src/backend/catalog/aclchk.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.cindex bed10f9409..ea5666ebb8 100644--- a/src/backend/catalog/aclchk.c+++ b/src/backend/catalog/aclchk.c@@ -3851,7 +3851,7 @@ pg_class_aclmask(Oid table_oid, Oid roleid,
/*
* Deny anyone permission to update a system catalog unless
- * pg_authid.rolsuper is set. Also allow it if allowSystemTableMods.+ * pg_authid.rolsuper is set.
*
* As of 7.4 we have some updatable system views; those shouldn't be
* protected in this way. Assume the view rules can take care of
@@ -3860,8 +3860,7 @@ pg_class_aclmask(Oid table_oid, Oid roleid,
if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE)) &&
IsSystemClass(table_oid, classForm) &&
classForm->relkind != RELKIND_VIEW &&
- !superuser_arg(roleid) &&- !allowSystemTableMods)+ !superuser_arg(roleid))
{
#ifdef ACLDEBUG
elog(DEBUG2, "permission denied for system catalog update");
base-commit: ca266a069a20c32a8f0a1df982a5a67d9483bcb3
--
2.24.0
v3-0002-Make-allow_system_table_mods-settable-at-run-time.patch (text/plain, 2.2K) download
From 3a78625fbe62ae78279357e534825a53cb11051a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 27 Nov 2019 16:34:27 +0100
Subject: [PATCH v3 2/3] Make allow_system_table_mods settable at run time
Make allow_system_table_mods settable at run time by superusers. It
was previously postmaster start only.
We don't want to make system catalog DDL wide-open, but there are
occasionally useful things to do like setting reloptions or statistics
on a busy system table, and blocking those doesn't help anyone. Also,
this enables the possibility of writing a test suite for this setting.
Discussion: https://www.postgresql.org/message-id/flat/8b00ea5e-28a7-88ba-e848-21528b632354%402ndquadrant.com
---
doc/src/sgml/config.sgml | 9 ++++++---
src/backend/utils/misc/guc.c | 2 +-
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgmlindex d4d1fe45cc..34fd759862 100644--- a/doc/src/sgml/config.sgml+++ b/doc/src/sgml/config.sgml@@ -9501,9 +9501,12 @@ <title>Developer Options</title>
</term>
<listitem>
<para>
- Allows modification of the structure of system tables.- This is used by <command>initdb</command>.- This parameter can only be set at server start.+ Allows modification of the structure of system tables as well as+ certain other risky actions on system tables. This is otherwise not+ allowed even for superusers. This is used by+ <command>initdb</command>. Inconsiderate use of this setting can+ cause irretrievable data loss or seriously corrupt the database+ system. Only superusers can change this setting.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.cindex ba4edde71a..5fccc9683e 100644--- a/src/backend/utils/misc/guc.c+++ b/src/backend/utils/misc/guc.c@@ -1777,7 +1777,7 @@ static struct config_bool ConfigureNamesBool[] =
},
{
- {"allow_system_table_mods", PGC_POSTMASTER, DEVELOPER_OPTIONS,+ {"allow_system_table_mods", PGC_SUSET, DEVELOPER_OPTIONS,
gettext_noop("Allows modifications of the structure of system tables."),
NULL,
GUC_NOT_IN_SAMPLE
--
2.24.0
v3-0003-Add-a-regression-test-for-allow_system_table_mods.patch (text/plain, 14.6K) download
From e07165d97290a882b30a6180c142d749fd72a16f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 27 Nov 2019 16:39:38 +0100
Subject: [PATCH v3 3/3] Add a regression test for allow_system_table_mods
Add a regression test file that exercises the kinds of commands that
allow_system_table_mods allows.
This is put in the "unsafe_tests" suite, so it won't accidentally
create a mess if someone runs the normal regression tests against an
instance that they care about.
Discussion: https://www.postgresql.org/message-id/flat/8b00ea5e-28a7-88ba-e848-21528b632354%402ndquadrant.com
---
src/test/modules/unsafe_tests/Makefile | 2 +-
.../expected/alter_system_table.out | 168 ++++++++++++++++
.../unsafe_tests/sql/alter_system_table.sql | 185 ++++++++++++++++++
src/test/regress/expected/alter_table.out | 4 -
src/test/regress/sql/alter_table.sql | 4 -
5 files changed, 354 insertions(+), 9 deletions(-)
create mode 100644 src/test/modules/unsafe_tests/expected/alter_system_table.out
create mode 100644 src/test/modules/unsafe_tests/sql/alter_system_table.sql
diff --git a/src/test/modules/unsafe_tests/Makefile b/src/test/modules/unsafe_tests/Makefileindex 321252f8d5..3ecf5fcfc5 100644--- a/src/test/modules/unsafe_tests/Makefile+++ b/src/test/modules/unsafe_tests/Makefile@@ -1,6 +1,6 @@
# src/test/modules/unsafe_tests/Makefile
-REGRESS = rolenames+REGRESS = rolenames alter_system_table
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/src/test/modules/unsafe_tests/expected/alter_system_table.out b/src/test/modules/unsafe_tests/expected/alter_system_table.outnew file mode 100644index 0000000000..09a557f837--- /dev/null+++ b/src/test/modules/unsafe_tests/expected/alter_system_table.out@@ -0,0 +1,168 @@+--+-- Tests for things affected by allow_system_table_mods+--+-- We run the same set of commands once with allow_system_table_mods+-- off and then again with on.+--+-- The "on" tests should where possible be wrapped in BEGIN/ROLLBACK+-- blocks so as to not leave a mess around.+CREATE USER regress_user_ast;+SET allow_system_table_mods = off;+-- create new table in pg_catalog+CREATE TABLE pg_catalog.test (a int);+ERROR: permission denied to create "pg_catalog.test"+DETAIL: System catalog modifications are currently disallowed.+-- anyarray column+CREATE TABLE t1x (a int, b anyarray);+ERROR: column "b" has pseudo-type anyarray+-- index on system catalog+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;+ERROR: permission denied: "pg_namespace" is a system catalog+-- write to system catalog table as superuser+-- (allowed even without allow_system_table_mods)+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 0, 'foo');+-- write to system catalog table as normal user+GRANT INSERT ON pg_description TO regress_user_ast;+SET ROLE regress_user_ast;+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 1, 'foo');+ERROR: permission denied for table pg_description+RESET ROLE;+-- policy on system catalog+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');+ERROR: permission denied: "pg_description" is a system catalog+-- reserved schema name+CREATE SCHEMA pg_foo;+ERROR: unacceptable schema name "pg_foo"+DETAIL: The prefix "pg_" is reserved for system schemas.+-- drop system table+DROP TABLE pg_description;+ERROR: permission denied: "pg_description" is a system catalog+-- truncate of system table+TRUNCATE pg_description;+ERROR: permission denied: "pg_description" is a system catalog+-- rename column of system table+ALTER TABLE pg_description RENAME COLUMN description TO comment;+ERROR: permission denied: "pg_description" is a system catalog+-- ATSimplePermissions()+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;+ERROR: permission denied: "pg_description" is a system catalog+-- SET STATISTICS+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;+ERROR: permission denied: "pg_description" is a system catalog+-- foreign key referencing catalog+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);+ERROR: permission denied: "pg_description" is a system catalog+-- RangeVarCallbackOwnsRelation()+CREATE INDEX pg_descripton_test_index ON pg_description (description);+ERROR: permission denied: "pg_description" is a system catalog+-- RangeVarCallbackForAlterRelation()+ALTER TABLE pg_description RENAME TO pg_comment;+ERROR: permission denied: "pg_description" is a system catalog+ALTER TABLE pg_description SET SCHEMA public;+ERROR: permission denied: "pg_description" is a system catalog+-- reserved tablespace name+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';+ERROR: unacceptable tablespace name "pg_foo"+DETAIL: The prefix "pg_" is reserved for system tablespaces.+-- triggers+CREATE FUNCTION tf1() RETURNS trigger+LANGUAGE plpgsql+AS $$+BEGIN+ RETURN NULL;+END $$;+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();+ERROR: permission denied: "pg_description" is a system catalog+ALTER TRIGGER t1 ON pg_description RENAME TO t2;+ERROR: permission denied: "pg_description" is a system catalog+--DROP TRIGGER t2 ON pg_description;+-- rules+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;+ERROR: permission denied: "pg_description" is a system catalog+ALTER RULE r1 ON pg_description RENAME TO r2;+ERROR: permission denied: "pg_description" is a system catalog+--DROP RULE r2 ON pg_description;+SET allow_system_table_mods = on;+-- create new table in pg_catalog+BEGIN;+CREATE TABLE pg_catalog.test (a int);+ROLLBACK;+-- anyarray column+BEGIN;+CREATE TABLE t1 (a int, b anyarray);+ROLLBACK;+-- index on system catalog+BEGIN;+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;+ROLLBACK;+-- write to system catalog table as superuser+BEGIN;+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 2, 'foo');+ROLLBACK;+-- write to system catalog table as normal user+-- (not allowed)+SET ROLE regress_user_ast;+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 3, 'foo');+ERROR: permission denied for table pg_description+RESET ROLE;+-- policy on system catalog+BEGIN;+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');+ROLLBACK;+-- reserved schema name+BEGIN;+CREATE SCHEMA pg_foo;+ROLLBACK;+-- drop system table+-- (This will fail anyway because it's pinned.)+BEGIN;+DROP TABLE pg_description;+ERROR: cannot drop table pg_description because it is required by the database system+ROLLBACK;+-- truncate of system table+BEGIN;+TRUNCATE pg_description;+ROLLBACK;+-- rename column of system table+BEGIN;+ALTER TABLE pg_description RENAME COLUMN description TO comment;+ROLLBACK;+-- ATSimplePermissions()+BEGIN;+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;+ROLLBACK;+-- SET STATISTICS+BEGIN;+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;+ROLLBACK;+-- foreign key referencing catalog+BEGIN;+ALTER TABLE pg_description ADD PRIMARY KEY USING INDEX pg_description_o_c_o_index;+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);+ROLLBACK;+-- RangeVarCallbackOwnsRelation()+BEGIN;+CREATE INDEX pg_descripton_test_index ON pg_description (description);+ROLLBACK;+-- RangeVarCallbackForAlterRelation()+BEGIN;+ALTER TABLE pg_description RENAME TO pg_comment;+ROLLBACK;+BEGIN;+ALTER TABLE pg_description SET SCHEMA public;+ROLLBACK;+-- reserved tablespace name+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';+ERROR: directory "/no/such/location" does not exist+-- triggers+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();+ALTER TRIGGER t1 ON pg_description RENAME TO t2;+DROP TRIGGER t2 ON pg_description;+-- rules+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;+ALTER RULE r1 ON pg_description RENAME TO r2;+DROP RULE r2 ON pg_description;+-- cleanup+REVOKE ALL ON pg_description FROM regress_user_ast;+DROP USER regress_user_ast;+DROP FUNCTION tf1;diff --git a/src/test/modules/unsafe_tests/sql/alter_system_table.sql b/src/test/modules/unsafe_tests/sql/alter_system_table.sqlnew file mode 100644index 0000000000..60769c8d51--- /dev/null+++ b/src/test/modules/unsafe_tests/sql/alter_system_table.sql@@ -0,0 +1,185 @@+--+-- Tests for things affected by allow_system_table_mods+--+-- We run the same set of commands once with allow_system_table_mods+-- off and then again with on.+--+-- The "on" tests should where possible be wrapped in BEGIN/ROLLBACK+-- blocks so as to not leave a mess around.++CREATE USER regress_user_ast;++SET allow_system_table_mods = off;++-- create new table in pg_catalog+CREATE TABLE pg_catalog.test (a int);++-- anyarray column+CREATE TABLE t1x (a int, b anyarray);++-- index on system catalog+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;++-- write to system catalog table as superuser+-- (allowed even without allow_system_table_mods)+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 0, 'foo');++-- write to system catalog table as normal user+GRANT INSERT ON pg_description TO regress_user_ast;+SET ROLE regress_user_ast;+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 1, 'foo');+RESET ROLE;++-- policy on system catalog+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');++-- reserved schema name+CREATE SCHEMA pg_foo;++-- drop system table+DROP TABLE pg_description;++-- truncate of system table+TRUNCATE pg_description;++-- rename column of system table+ALTER TABLE pg_description RENAME COLUMN description TO comment;++-- ATSimplePermissions()+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;++-- SET STATISTICS+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;++-- foreign key referencing catalog+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);++-- RangeVarCallbackOwnsRelation()+CREATE INDEX pg_descripton_test_index ON pg_description (description);++-- RangeVarCallbackForAlterRelation()+ALTER TABLE pg_description RENAME TO pg_comment;+ALTER TABLE pg_description SET SCHEMA public;++-- reserved tablespace name+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';++-- triggers+CREATE FUNCTION tf1() RETURNS trigger+LANGUAGE plpgsql+AS $$+BEGIN+ RETURN NULL;+END $$;++CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();+ALTER TRIGGER t1 ON pg_description RENAME TO t2;+--DROP TRIGGER t2 ON pg_description;++-- rules+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;+ALTER RULE r1 ON pg_description RENAME TO r2;+--DROP RULE r2 ON pg_description;+++SET allow_system_table_mods = on;++-- create new table in pg_catalog+BEGIN;+CREATE TABLE pg_catalog.test (a int);+ROLLBACK;++-- anyarray column+BEGIN;+CREATE TABLE t1 (a int, b anyarray);+ROLLBACK;++-- index on system catalog+BEGIN;+ALTER TABLE pg_namespace ADD UNIQUE USING INDEX pg_namespace_oid_index;+ROLLBACK;++-- write to system catalog table as superuser+BEGIN;+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 2, 'foo');+ROLLBACK;++-- write to system catalog table as normal user+-- (not allowed)+SET ROLE regress_user_ast;+INSERT INTO pg_description (objoid, classoid, objsubid, description) VALUES (0, 0, 3, 'foo');+RESET ROLE;++-- policy on system catalog+BEGIN;+CREATE POLICY foo ON pg_description FOR SELECT USING (description NOT LIKE 'secret%');+ROLLBACK;++-- reserved schema name+BEGIN;+CREATE SCHEMA pg_foo;+ROLLBACK;++-- drop system table+-- (This will fail anyway because it's pinned.)+BEGIN;+DROP TABLE pg_description;+ROLLBACK;++-- truncate of system table+BEGIN;+TRUNCATE pg_description;+ROLLBACK;++-- rename column of system table+BEGIN;+ALTER TABLE pg_description RENAME COLUMN description TO comment;+ROLLBACK;++-- ATSimplePermissions()+BEGIN;+ALTER TABLE pg_description ALTER COLUMN description SET NOT NULL;+ROLLBACK;++-- SET STATISTICS+BEGIN;+ALTER TABLE pg_description ALTER COLUMN description SET STATISTICS -1;+ROLLBACK;++-- foreign key referencing catalog+BEGIN;+ALTER TABLE pg_description ADD PRIMARY KEY USING INDEX pg_description_o_c_o_index;+CREATE TABLE foo (a oid, b oid, c int, FOREIGN KEY (a, b, c) REFERENCES pg_description);+ROLLBACK;++-- RangeVarCallbackOwnsRelation()+BEGIN;+CREATE INDEX pg_descripton_test_index ON pg_description (description);+ROLLBACK;++-- RangeVarCallbackForAlterRelation()+BEGIN;+ALTER TABLE pg_description RENAME TO pg_comment;+ROLLBACK;+BEGIN;+ALTER TABLE pg_description SET SCHEMA public;+ROLLBACK;++-- reserved tablespace name+CREATE TABLESPACE pg_foo LOCATION '/no/such/location';++-- triggers+CREATE TRIGGER t1 BEFORE INSERT ON pg_description EXECUTE FUNCTION tf1();+ALTER TRIGGER t1 ON pg_description RENAME TO t2;+DROP TRIGGER t2 ON pg_description;++-- rules+CREATE RULE r1 AS ON INSERT TO pg_description DO INSTEAD NOTHING;+ALTER RULE r1 ON pg_description RENAME TO r2;+DROP RULE r2 ON pg_description;+++-- cleanup+REVOKE ALL ON pg_description FROM regress_user_ast;+DROP USER regress_user_ast;+DROP FUNCTION tf1;diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.outindex 5189fd8188..f65611a62c 100644--- a/src/test/regress/expected/alter_table.out+++ b/src/test/regress/expected/alter_table.out@@ -3309,10 +3309,6 @@ WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;
-- Checks on creating and manipulation of user defined relations in
-- pg_catalog.
------ XXX: It would be useful to add checks around trying to manipulate--- catalog tables, but that might have ugly consequences when run--- against an existing server with allow_system_table_mods = on.
SHOW allow_system_table_mods;
allow_system_table_mods
-------------------------
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sqlindex 14d06c116f..abe7be3223 100644--- a/src/test/regress/sql/alter_table.sql+++ b/src/test/regress/sql/alter_table.sql@@ -2079,10 +2079,6 @@ CREATE TEMP TABLE filenode_mapping AS
-- Checks on creating and manipulation of user defined relations in
-- pg_catalog.
------ XXX: It would be useful to add checks around trying to manipulate--- catalog tables, but that might have ugly consequences when run--- against an existing server with allow_system_table_mods = on.
SHOW allow_system_table_mods;
-- disallowed because of search_path issues with pg_dump
--
2.24.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@2ndquadrant.com, michael@paquier.xyz, alvherre@2ndquadrant.com
Subject: Re: allow_system_table_mods stuff
In-Reply-To: <f48637b7-ab24-e48b-0b30-519b1170e65c@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox