From: Stephen Frost <sfrost@snowman.net>
To: Magnus Hagander <magnus@hagander.net>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: New default role- 'pg_read_all_data'
Date: Sun, 30 Aug 2020 19:20:01 -0400
Message-ID: <20200830232001.GM29590@tamriel.snowman.net> (raw)
In-Reply-To: <20200828132645.GZ29590@tamriel.snowman.net>
References: <20200828003023.GU29590@tamriel.snowman.net>
<CABUevEy9QAnx5h_BmSk4j6K9AfS9=co15RUFda0kknMHO9NJOg@mail.gmail.com>
<20200828123847.GV29590@tamriel.snowman.net>
<CABUevEx5akfNeSrHsinCRWfXzSYFNRTnBc4vHKMgum8hLrr=yg@mail.gmail.com>
<20200828132645.GZ29590@tamriel.snowman.net>
Greetings,
* Stephen Frost (sfrost@snowman.net) wrote:
> * Magnus Hagander (magnus@hagander.net) wrote:
> > On Fri, Aug 28, 2020 at 2:38 PM Stephen Frost <sfrost@snowman.net> wrote:
> > > * Magnus Hagander (magnus@hagander.net) wrote:
> > > > Without having actually looked at the code, definite +1 for this feature.
> > > > It's much requested...
> > >
> > > Thanks.
> > >
> > > > But, should we also have a pg_write_all_data to go along with it?
> > >
> > > Perhaps, but could certainly be a different patch, and it'd need to be
> > > better defined, it seems to me... read_all is pretty straight-forward
> > > (the general goal being "make pg_dumpall/pg_dump work"), what would
> > > write mean? INSERT? DELETE? TRUNCATE? ALTER TABLE? System catalogs?
> >
> > Well, it's pg_write_all_*data*, so it certainly wouldn't be alter table or
> > system catalogs.
> >
> > I'd say insert/update/delete yes.
> >
> > TRUNCATE is always an outlier.Given it's generally classified as DDL, I
> > wouldn't include it.
>
> Alright, that seems like it'd be pretty easy. We already have a check
> in pg_class_aclmask to disallow modification of system catalogs w/o
> being a superuser, so we should be alright to add a similar check for
> insert/update/delete just below where I added the SELECT check.
>
> > > Doesn't seem like you could just declare it to be 'allow pg_restore'
> > > either, as that might include creating untrusted functions, et al.
> >
> > No definitely not. That wouldn't be the usecase at all :)
>
> Good. :)
>
> > (and fwiw to me the main use case for read_all_data also isn't pg_dump,
> > because most people using pg_dump are already db owner or higher in my
> > experience. But it is nice that it helps with that too)
>
> Glad to have confirmation that there's other use-cases this helps with.
>
> I'll post an updated patch with that added in a day or two.
Here's that updated patch, comments welcome.
Thanks!
Stephen
Attachments:pg_rorole_v2.patch (text/x-diff, 9.0K) downloaddiff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgmlindex 829decd883..1213125bfd 100644--- a/doc/src/sgml/user-manag.sgml+++ b/doc/src/sgml/user-manag.sgml@@ -517,6 +517,20 @@ DROP ROLE doomed_role;
</row>
</thead>
<tbody>
+ <row>+ <entry>pg_read_all_data</entry>+ <entry>Read all data (tables, views, sequences), as if having SELECT+ rights on those objects, and USAGE rights on all schemas, even without+ having it explicitly. This role also has <literal>BYPASSRLS</literal>+ set for it.</entry>+ </row>+ <row>+ <entry>pg_write_all_data</entry>+ <entry>Write all data (tables, views, sequences), as if having INSERT,+ UPDATE, and DELETE rights on those objects, and USAGE rights on all+ schemas, even without having it explicitly. This role also has+ <literal>BYPASSRLS</literal> set for it.</entry>+ </row>
<row>
<entry>pg_read_all_settings</entry>
<entry>Read all configuration variables, even those normally visible only to
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.cindex c626161408..3e6d060554 100644--- a/src/backend/catalog/aclchk.c+++ b/src/backend/catalog/aclchk.c@@ -3849,6 +3849,26 @@ pg_class_aclmask(Oid table_oid, Oid roleid,
ReleaseSysCache(tuple);
+ /*+ * Check if ACL_SELECT is being checked and, if so, and not set already+ * as part of the result, then check if the user is a member of the+ * pg_read_all_data role, which allows read access to all relations.+ */+ if (mask & ACL_SELECT && !(result & ACL_SELECT) &&+ has_privs_of_role(roleid, DEFAULT_ROLE_READ_ALL_DATA))+ result |= ACL_SELECT;++ /*+ * Check if ACL_INSERT, ACL_UPDATE, or ACL_DELETE is being checked+ * and, if so, and not set already as part of the result, then check+ * if the user is a member of the pg_write_all_data role, which+ * allows INSERT/UPDATE/DELETE access to all relations.+ */+ if (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE) &&+ !(result & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)) &&+ has_privs_of_role(roleid, DEFAULT_ROLE_WRITE_ALL_DATA))+ result |= (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE));+
return result;
}
@@ -4175,6 +4195,16 @@ pg_namespace_aclmask(Oid nsp_oid, Oid roleid,
ReleaseSysCache(tuple);
+ /*+ * Check if ACL_USAGE is being checked and, if so, and not set already+ * as part of the result, then check if the user is a member of the+ * pg_read_all_data or pg_write_all_data roles, which allow usage+ * access to all schemas.+ */+ if (mask & ACL_USAGE && !(result & ACL_USAGE) &&+ (has_privs_of_role(roleid, DEFAULT_ROLE_READ_ALL_DATA) ||+ has_privs_of_role(roleid, DEFAULT_ROLE_WRITE_ALL_DATA)))+ result |= ACL_USAGE;
return result;
}
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.datindex 7c08851550..3f72474146 100644--- a/src/include/catalog/pg_authid.dat+++ b/src/include/catalog/pg_authid.dat@@ -25,6 +25,16 @@
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
rolpassword => '_null_', rolvaliduntil => '_null_' },
+{ oid => '9274', oid_symbol => 'DEFAULT_ROLE_READ_ALL_DATA',+ rolname => 'pg_read_all_data', rolsuper => 'f', rolinherit => 't',+ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',+ rolreplication => 'f', rolbypassrls => 't', rolconnlimit => '-1',+ rolpassword => '_null_', rolvaliduntil => '_null_' },+{ oid => '9275', oid_symbol => 'DEFAULT_ROLE_WRITE_ALL_DATA',+ rolname => 'pg_write_all_data', rolsuper => 'f', rolinherit => 't',+ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',+ rolreplication => 'f', rolbypassrls => 't', rolconnlimit => '-1',+ rolpassword => '_null_', rolvaliduntil => '_null_' },
{ oid => '3374', oid_symbol => 'DEFAULT_ROLE_READ_ALL_SETTINGS',
rolname => 'pg_read_all_settings', rolsuper => 'f', rolinherit => 't',
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.outindex 3ec22c20ea..090e90dadb 100644--- a/src/test/regress/expected/privileges.out+++ b/src/test/regress/expected/privileges.out@@ -12,6 +12,7 @@ DROP ROLE IF EXISTS regress_priv_user3;
DROP ROLE IF EXISTS regress_priv_user4;
DROP ROLE IF EXISTS regress_priv_user5;
DROP ROLE IF EXISTS regress_priv_user6;
+DROP ROLE IF EXISTS regress_priv_user7;
SELECT lo_unlink(oid) FROM pg_largeobject_metadata WHERE oid >= 1000 AND oid < 3000 ORDER BY oid;
lo_unlink
-----------
@@ -26,6 +27,10 @@ CREATE USER regress_priv_user4;
CREATE USER regress_priv_user5;
CREATE USER regress_priv_user5; -- duplicate
ERROR: role "regress_priv_user5" already exists
+CREATE USER regress_priv_user6;+CREATE USER regress_priv_user7;+GRANT pg_read_all_data TO regress_priv_user6;+GRANT pg_write_all_data TO regress_priv_user7;
CREATE GROUP regress_priv_group1;
CREATE GROUP regress_priv_group2 WITH USER regress_priv_user1, regress_priv_user2;
ALTER GROUP regress_priv_group1 ADD USER regress_priv_user4;
@@ -129,6 +134,29 @@ SELECT * FROM atest2 WHERE ( col1 IN ( SELECT b FROM atest1 ) );
------+------
(0 rows)
+SET SESSION AUTHORIZATION regress_priv_user6;+SELECT * FROM atest1; -- ok+ a | b +---+-----+ 1 | two+ 1 | two+(2 rows)++SELECT * FROM atest2; -- ok+ col1 | col2 +------+------+(0 rows)++INSERT INTO atest2 VALUES ('foo', true); -- fail+ERROR: permission denied for table atest2+SET SESSION AUTHORIZATION regress_priv_user7;+SELECT * FROM atest1; -- fail+ERROR: permission denied for table atest1+SELECT * FROM atest2; -- fail+ERROR: permission denied for table atest2+INSERT INTO atest2 VALUES ('foo', true); -- ok+UPDATE atest2 SET col2 = true; -- ok+DELETE FROM atest2; -- ok
SET SESSION AUTHORIZATION regress_priv_user3;
SELECT session_user, current_user;
session_user | current_user
@@ -1674,6 +1702,12 @@ SELECT has_schema_privilege('regress_priv_user2', 'testns2', 'USAGE'); -- yes
t
(1 row)
+SELECT has_schema_privilege('regress_priv_user6', 'testns2', 'USAGE'); -- yes+ has_schema_privilege +----------------------+ t+(1 row)+
SELECT has_schema_privilege('regress_priv_user2', 'testns2', 'CREATE'); -- no
has_schema_privilege
----------------------
@@ -2045,7 +2079,9 @@ DROP USER regress_priv_user3;
DROP USER regress_priv_user4;
DROP USER regress_priv_user5;
DROP USER regress_priv_user6;
-ERROR: role "regress_priv_user6" does not exist+DROP USER regress_priv_user7;+DROP USER regress_priv_user8; -- does not exist+ERROR: role "regress_priv_user8" does not exist
-- permissions with LOCK TABLE
CREATE USER regress_locktable_user;
CREATE TABLE lock_table (a int);
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sqlindex 3550f61587..9919e8c490 100644--- a/src/test/regress/sql/privileges.sql+++ b/src/test/regress/sql/privileges.sql@@ -16,6 +16,7 @@ DROP ROLE IF EXISTS regress_priv_user3;
DROP ROLE IF EXISTS regress_priv_user4;
DROP ROLE IF EXISTS regress_priv_user5;
DROP ROLE IF EXISTS regress_priv_user6;
+DROP ROLE IF EXISTS regress_priv_user7;
SELECT lo_unlink(oid) FROM pg_largeobject_metadata WHERE oid >= 1000 AND oid < 3000 ORDER BY oid;
@@ -29,6 +30,11 @@ CREATE USER regress_priv_user3;
CREATE USER regress_priv_user4;
CREATE USER regress_priv_user5;
CREATE USER regress_priv_user5; -- duplicate
+CREATE USER regress_priv_user6;+CREATE USER regress_priv_user7;++GRANT pg_read_all_data TO regress_priv_user6;+GRANT pg_write_all_data TO regress_priv_user7;
CREATE GROUP regress_priv_group1;
CREATE GROUP regress_priv_group2 WITH USER regress_priv_user1, regress_priv_user2;
@@ -94,6 +100,17 @@ GRANT ALL ON atest1 TO PUBLIC; -- fail
SELECT * FROM atest1 WHERE ( b IN ( SELECT col1 FROM atest2 ) );
SELECT * FROM atest2 WHERE ( col1 IN ( SELECT b FROM atest1 ) );
+SET SESSION AUTHORIZATION regress_priv_user6;+SELECT * FROM atest1; -- ok+SELECT * FROM atest2; -- ok+INSERT INTO atest2 VALUES ('foo', true); -- fail++SET SESSION AUTHORIZATION regress_priv_user7;+SELECT * FROM atest1; -- fail+SELECT * FROM atest2; -- fail+INSERT INTO atest2 VALUES ('foo', true); -- ok+UPDATE atest2 SET col2 = true; -- ok+DELETE FROM atest2; -- ok
SET SESSION AUTHORIZATION regress_priv_user3;
SELECT session_user, current_user;
@@ -988,6 +1005,7 @@ ALTER DEFAULT PRIVILEGES GRANT USAGE ON SCHEMAS TO regress_priv_user2;
CREATE SCHEMA testns2;
SELECT has_schema_privilege('regress_priv_user2', 'testns2', 'USAGE'); -- yes
+SELECT has_schema_privilege('regress_priv_user6', 'testns2', 'USAGE'); -- yes
SELECT has_schema_privilege('regress_priv_user2', 'testns2', 'CREATE'); -- no
ALTER DEFAULT PRIVILEGES REVOKE USAGE ON SCHEMAS FROM regress_priv_user2;
@@ -1211,6 +1229,8 @@ DROP USER regress_priv_user3;
DROP USER regress_priv_user4;
DROP USER regress_priv_user5;
DROP USER regress_priv_user6;
+DROP USER regress_priv_user7;+DROP USER regress_priv_user8; -- does not exist
-- permissions with LOCK TABLE
signature.asc (application/pgp-signature, 818B) download
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: sfrost@snowman.net, magnus@hagander.net, pgsql-hackers@lists.postgresql.org
Subject: Re: New default role- 'pg_read_all_data'
In-Reply-To: <20200830232001.GM29590@tamriel.snowman.net>
* 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