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
>From fbe9dd7d9fb4674cf2ca25b6cd4f05556c201d89 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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.c
index 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
>From 3a78625fbe62ae78279357e534825a53cb11051a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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.sgml
index 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.c
index 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
>From e07165d97290a882b30a6180c142d749fd72a16f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@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/Makefile
index 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.out
new file mode 100644
index 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.sql
new file mode 100644
index 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.out
index 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.sql
index 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
