New default role- 'pg_read_all_data'

On Thu, Aug 27, 2020 at 5:30 PM Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

There's no shortage of requests and responses regarding how to have a
'read all of the data' role in PG, with various hacks involving "GRANT
ALL" and "ALTER DEFAULT PRIVILEGES" to "solve" this, neither of which
really works long term ("GRANT ALL" is one-time, and "ALTER DEFAULT"
only helps for the roles that exist today).

Now that we have the default role system, we can provide a proper
solution to this oft-requested capability.

This patch adds a default role to meet specifically that use-case, in
the long-term, by explicitly allowing SELECT rights on all relations,
and USAGE rights on all schemas, for roles who are members of the new
'pg_read_all_data' role.

No effort is made to prevent a user who has this role from writing data-
that's up to the admin, but this will allow someone to use pg_dump or
pg_dumpall in a much more reliable manner to make sure that the entire
database is able to be exported for the purpose of backups, upgrades, or
other common use-cases, without having to have that same user be a PG
superuser.

This role is given the Bypass RLS right, though to use it effectively, a
user would need to pass '--role=pg_read_all_data' to pg_dump/pg_dumpall,
since role attributes are not checked as part of role membership.

This will be much appreciated from an app developers perspective. It makes
life so much easier to "do the right thing" in terms of giving read only
webapps the right permissions.

On Fri, Aug 28, 2020 at 2:30 AM Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

There's no shortage of requests and responses regarding how to have a
'read all of the data' role in PG, with various hacks involving "GRANT
ALL" and "ALTER DEFAULT PRIVILEGES" to "solve" this, neither of which
really works long term ("GRANT ALL" is one-time, and "ALTER DEFAULT"
only helps for the roles that exist today).

Now that we have the default role system, we can provide a proper
solution to this oft-requested capability.

This patch adds a default role to meet specifically that use-case, in
the long-term, by explicitly allowing SELECT rights on all relations,
and USAGE rights on all schemas, for roles who are members of the new
'pg_read_all_data' role.

No effort is made to prevent a user who has this role from writing data-
that's up to the admin, but this will allow someone to use pg_dump or
pg_dumpall in a much more reliable manner to make sure that the entire
database is able to be exported for the purpose of backups, upgrades, or
other common use-cases, without having to have that same user be a PG
superuser.

This role is given the Bypass RLS right, though to use it effectively, a
user would need to pass '--role=pg_read_all_data' to pg_dump/pg_dumpall,
since role attributes are not checked as part of role membership.

Thoughts?

Without having actually looked at the code, definite +1 for this feature.
It's much requested...

But, should we also have a pg_write_all_data to go along with it?

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

Thank you for the patch.

My high level review comment:
The patch seems to be implementing a useful and requested feature.
The patch applies cleanly and passes the basic regress tests. Also the commitfest bot is happy.

A first pass at the code, has not revealed any worthwhile comments.
Please allow me for a second and more thorough pass. The commitfest has hardly started after all.

Also allow me a series of genuine questions:

What would the behaviour be with REVOKE?
In a sequence similar to:
GRANT ALL ON ...
REVOKE pg_read_all_data FROM ...
What privileges would the user be left with? Would it be possible to end up in the same privilege only with a GRANT command?
Does the above scenario even make sense?

Regards,

Greetings,

* 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?

Doesn't seem like you could just declare it to be 'allow pg_restore'
either, as that might include creating untrusted functions, et al.

Thanks,

Stephen

Greetings,

* Georgios Kokolatos (gkokolatos@protonmail.com) wrote:

The patch seems to be implementing a useful and requested feature.
The patch applies cleanly and passes the basic regress tests. Also the commitfest bot is happy.

A first pass at the code, has not revealed any worthwhile comments.
Please allow me for a second and more thorough pass. The commitfest has hardly started after all.

Great, thanks!

Also allow me a series of genuine questions:

What would the behaviour be with REVOKE?
In a sequence similar to:
GRANT ALL ON ...

GRANT ALL would be independently GRANT'ing rights to some role and
therefore unrelated.

REVOKE pg_read_all_data FROM ...

This would simply REVOKE that role from the user. Privileges
independently GRANT'd directly to the user wouldn't be affected. Nor
would other role membership.

What privileges would the user be left with? Would it be possible to end up in the same privilege only with a GRANT command?

I'm not sure what's being asked here.

Does the above scenario even make sense?

I definitely believe it makes sense for a given role/user to be a member
of pg_read_all_data and to be a member of other roles, or to have other
privileges GRANT'd directly to them.

Thanks,

Stephen

On Fri, Aug 28, 2020 at 2:38 PM Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

* 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.

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 :)

(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)

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On Fri, 28 Aug 2020 at 08:43, Stephen Frost <sfrost@snowman.net> wrote:

This would simply REVOKE that role from the user. Privileges
independently GRANT'd directly to the user wouldn't be affected. Nor
would other role membership.

What privileges would the user be left with? Would it be possible to end

up in the same privilege only with a GRANT command?

What about:

REVOKE SELECT ON [table] FROM pg_read_all_data;

I guess what I’m really asking is whether pg_read_all_data is automatically
granted SELECT on all newly-created relations, or if the permission
checking system always returns TRUE when asked if pg_read_all_data can
select from a relation? I’m guessing it’s the latter so that it would be
ineffective to revoke select privilege as I think this is more useful, but
I’d like to be sure and the documentation should be explicit on this point.

On Fri, Aug 28, 2020 at 2:43 PM Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

* Georgios Kokolatos (gkokolatos@protonmail.com) wrote:

The patch seems to be implementing a useful and requested feature.
The patch applies cleanly and passes the basic regress tests. Also the

commitfest bot is happy.

A first pass at the code, has not revealed any worthwhile comments.
Please allow me for a second and more thorough pass. The commitfest has

hardly started after all.

Great, thanks!

Also allow me a series of genuine questions:

What would the behaviour be with REVOKE?
In a sequence similar to:
GRANT ALL ON ...

GRANT ALL would be independently GRANT'ing rights to some role and
therefore unrelated.

REVOKE pg_read_all_data FROM ...

This would simply REVOKE that role from the user. Privileges
independently GRANT'd directly to the user wouldn't be affected. Nor
would other role membership.

What privileges would the user be left with? Would it be possible to end

up in the same privilege only with a GRANT command?

I'm not sure what's being asked here.

I think the core thing to remember here is that SQL permissions are always
additive, that's what confuses some cases.

That is, REVOKE something FROM role only removes this particular additive
permission. It doesn't make sure the role doesn't have the same permission
*through some other means*.

Sometime it would be really useful to be able to do e.g. "DENY DELETE ON
importanttable FROM sfrost", which would then override any DELETE
permissions he'd be getting from anywhere else. To be able to say
"everybody except x". But that's not something that's in SQL permissions
AFAIK :)

Does the above scenario even make sense?

I definitely believe it makes sense for a given role/user to be a member
of pg_read_all_data and to be a member of other roles, or to have other
privileges GRANT'd directly to them.

Yeah, for example another role might have a combination of read and write
permissions, and those would then remain for the user if pg_read_all_data
is removed.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

Greetings,

* Isaac Morland (isaac.morland@gmail.com) wrote:

On Fri, 28 Aug 2020 at 08:43, Stephen Frost <sfrost@snowman.net> wrote:

This would simply REVOKE that role from the user. Privileges
independently GRANT'd directly to the user wouldn't be affected. Nor
would other role membership.

What privileges would the user be left with? Would it be possible to end

up in the same privilege only with a GRANT command?

What about:

REVOKE SELECT ON [table] FROM pg_read_all_data;

Wouldn't have any effect, and I think that's correct.

I guess what I’m really asking is whether pg_read_all_data is automatically
granted SELECT on all newly-created relations, or if the permission
checking system always returns TRUE when asked if pg_read_all_data can
select from a relation? I’m guessing it’s the latter so that it would be
ineffective to revoke select privilege as I think this is more useful, but
I’d like to be sure and the documentation should be explicit on this point.

Yes, it's the latter. I'm not really sure about the documentation
change you're contemplating- have a specific suggestion?

Thanks,

Stephen

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, 28 August 2020 15:43, Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

- Georgios Kokolatos (gkokolatos@protonmail.com) wrote:

The patch seems to be implementing a useful and requested feature.
The patch applies cleanly and passes the basic regress tests. Also the commitfest bot is happy.
A first pass at the code, has not revealed any worthwhile comments.
Please allow me for a second and more thorough pass. The commitfest has hardly started after all.

Great, thanks!

Also allow me a series of genuine questions:
What would the behaviour be with REVOKE?
In a sequence similar to:
GRANT ALL ON ...

GRANT ALL would be independently GRANT'ing rights to some role and
therefore unrelated.

REVOKE pg_read_all_data FROM ...

This would simply REVOKE that role from the user. Privileges
independently GRANT'd directly to the user wouldn't be affected. Nor
would other role membership.

What privileges would the user be left with? Would it be possible to end up in the same privilege only with a GRANT command?

I'm not sure what's being asked here.

You are correct. My phrasing is not clear. Please be patient and allow me to try again.

I was playing around with the code and I was trying a bit the opposite of what you have submitted in the test file.

You have, (snipped):

GRANT pg_read_all_data TO regress_priv_user6;

SET SESSION AUTHORIZATION regress_priv_user6;
SELECT * FROM atest1; -- ok
INSERT INTO atest2 VALUES ('foo', true); -- fail

I was expecting:
REVOKE pg_read_all_data FROM regress_priv_user6;

SET SESSION AUTHORIZATION regress_priv_user6;
SELECT * FROM atest1; -- fail
INSERT INTO atest2 VALUES ('foo', true); -- ok

My expectation was not met since in my manual test (unless I made a mistake which is entirely possible), the SELECT above did not fail. The insert did succeed though.

The first question: Was my expectation wrong?
The second question: Is there a privilege that can be granted to regress_priv_user6 that will not permit the select operation but will permit the insert operation? If no, should there be one?

I hope I am clearer now.

Thank you again for your patience.

Greetings,

* gkokolatos@pm.me (gkokolatos@pm.me) wrote:

On Friday, 28 August 2020 15:43, Stephen Frost <sfrost@snowman.net> wrote:

What privileges would the user be left with? Would it be possible to end up in the same privilege only with a GRANT command?

I'm not sure what's being asked here.

You are correct. My phrasing is not clear. Please be patient and allow me to try again.

I was playing around with the code and I was trying a bit the opposite of what you have submitted in the test file.

You have, (snipped):

GRANT pg_read_all_data TO regress_priv_user6;

SET SESSION AUTHORIZATION regress_priv_user6;
SELECT * FROM atest1; -- ok
INSERT INTO atest2 VALUES ('foo', true); -- fail

I was expecting:
REVOKE pg_read_all_data FROM regress_priv_user6;

Are you sure this REVOKE was successful..?

SET SESSION AUTHORIZATION regress_priv_user6;
SELECT * FROM atest1; -- fail
INSERT INTO atest2 VALUES ('foo', true); -- ok

=# create role r1;
CREATE ROLE
=*# grant pg_read_all_data to r1;
GRANT ROLE
=*# create table t1 (c1 int);
CREATE TABLE
=*# set role r1;
=*> select * from t1;
c1
----
(0 rows)
=*> reset role;
RESET
=*# revoke pg_read_all_data from r1;
REVOKE ROLE
=*# set role r1;
SET
=*> select * from t1;
ERROR: permission denied for table t1

Seems to be working as expected here.

My expectation was not met since in my manual test (unless I made a mistake which is entirely possible), the SELECT above did not fail. The insert did succeed though.

That the INSERT worked seems pretty odd- could you post the exact
changes you've made to the regression tests, or the exact script where
you aren't seeing what you expect? I've not been able to reproduce the
GRANT allowing a user to INSERT into a table.

The first question: Was my expectation wrong?

If there aren't any other privileges involved, then REVOKE'ing the role
from a user should prevent that user from being able to SELECT from the
table.

The second question: Is there a privilege that can be granted to regress_priv_user6 that will not permit the select operation but will permit the insert operation? If no, should there be one?

GRANT INSERT ON atest1 TO regress_prive_user6; would allow just
INSERT'ing.

Magnus also brought up the idea of a 'write_all_data' role, but that's
pretty independent of this, imv. Not against adding it, if we can agree
as to what it means, exactly, but we should probably discuss over in
that sub-thread.

Thanks,

Stephen

On Fri, 28 Aug 2020 at 08:54, Stephen Frost <sfrost@snowman.net> wrote:

Yes, it's the latter. I'm not really sure about the documentation
change you're contemplating- have a specific suggestion?

Sorry, I was discussing this as if it was an abstract idea, not a concrete
patch. I've just taken a look at the patch and I think the documentation in
doc/src/sgml/user-manag.sgml is OK.

Greetings,

* 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.

Thanks,

Stephen

Le 28/08/2020 � 15:26, Stephen Frost a �crit�:

Greetings,

* 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.

Thanks,

Stephen

Hi,

Looking at this thread I was thinking about the FDW. Does role
pg_read_all_data will allow to execute pg_dump with
--include-foreign-data option? If this is the case how about priviledge
on fdw and foreign server? If this is the behavior we want I guess that
the modification should be applied to pg_foreign_data_wrapper_aclmask()
and pg_foreign_server_aclmask() too.

Best regards,

--
Gilles Darold

Greetings,

* Gilles Darold (gilles@darold.net) wrote:

Le 28/08/2020 à 15:26, Stephen Frost a écrit :

* 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.

Looking at this thread I was thinking about the FDW. Does role
pg_read_all_data will allow to execute pg_dump with --include-foreign-data
option? If this is the case how about priviledge on fdw and foreign server?
If this is the behavior we want I guess that the modification should be
applied to pg_foreign_data_wrapper_aclmask() and pg_foreign_server_aclmask()
too.

Using an FDW will often also require having a user mapping and there's
no way for that to be accomplished through only GRANT'ing a default
role, so I don't think we should mix this specific role with the FDW
permissions system.

Thanks,

Stephen

Le 28/08/2020 � 16:52, Stephen Frost a �crit�:

Greetings,

* Gilles Darold (gilles@darold.net) wrote:

Le 28/08/2020 � 15:26, Stephen Frost a �crit�:

* 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.

Looking at this thread I was thinking about the FDW. Does role
pg_read_all_data will allow to execute pg_dump with --include-foreign-data
option? If this is the case how about priviledge on fdw and foreign server?
If this is the behavior we want I guess that the modification should be
applied to pg_foreign_data_wrapper_aclmask() and pg_foreign_server_aclmask()
too.

Using an FDW will often also require having a user mapping and there's
no way for that to be accomplished through only GRANT'ing a default
role, so I don't think we should mix this specific role with the FDW
permissions system.

I'm fine with that, perhaps it should be mentioned in the documentation
that foreign tables are not covered by this role.

--
Gilles Darold

Greetings,

* Gilles Darold (gilles@darold.net) wrote:

Le 28/08/2020 à 16:52, Stephen Frost a écrit :

Using an FDW will often also require having a user mapping and there's
no way for that to be accomplished through only GRANT'ing a default
role, so I don't think we should mix this specific role with the FDW
permissions system.

I'm fine with that, perhaps it should be mentioned in the documentation that
foreign tables are not covered by this role.

We could say it doesn't GRANT CONNECT rights on databases, or EXECUTE on
functions too, but that doesn't seem like a terribly good approach for
the documentation to take- instead we document specifically what IS
included, which seems sufficiently clear to me.

Thanks,

Stephen

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

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, 31 August 2020 02:20, Stephen Frost <sfrost@snowman.net> wrote:

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.

Thank you for the updated patch!

Had a quick look on it and nothing stands out.

Also this sub-thread seems to have clearly responded on my early thoughts regarding invoking. Adding part of that subthread bellow:

My expectation was not met since in my manual test (unless I made a mistake which is entirely possible), the SELECT above did not fail. The insert did succeed though.

That the INSERT worked seems pretty odd- could you post the exact
changes you've made to the regression tests, or the exact script where
you aren't seeing what you expect? I've not been able to reproduce the
GRANT allowing a user to INSERT into a table.

The first question: Was my expectation wrong?

If there aren't any other privileges involved, then REVOKE'ing the role
from a user should prevent that user from being able to SELECT from the
table.

The second question: Is there a privilege that can be granted to regress_priv_user6 that will not permit the select operation but will permit the insert operation? If no, should there be one?

As discussed above, while I was struggling to formulate the thought, Magnus had already proposed pg_write_all_data and the community had reached a consensus on what it actually means.

Please find attached a minimal test case and the output of it.

It is obvious that I was confused and added confusion to the thread. Permissions are additive and autonomous. Now it is rather clear to me what the expectations are and how the patch should behave.

To add to my embarrassment, the REVOKE operation emitted a warning which I had clearly missed.

Apologies.

//Georgios

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

Version 2 of the patch, implements a useful feature. Based on the mailing list discussion, it is also a feature that the community desires.

The code seems to be correct and it follows the style. The patch comes complete with tests and documentation.

As a non native English speaker, I did not notice any syntactical or grammatical errors in the documentation. Yet it should not mean a lot.

As far as I am concerned, this version of the patch is ready for a committer.

Please feel free to contest my review, if you think I am wrong.

The new status of this patch is: Ready for Committer

Hi,

this patch is in "Ready for committer" state and the Cfbot is happy.

Is there any committer that is available for taking a look at it?

Cheers,
//Georgios - CFM 2020-11

Greetings,

* Georgios Kokolatos (gkokolatos@protonmail.com) wrote:

this patch is in "Ready for committer" state and the Cfbot is happy.

Glad that's still the case. :)

Is there any committer that is available for taking a look at it?

If there aren't any objections or further comments, I'll take another
look through it and will commit it during the upcoming CF.

Thanks!

Stephen

On 29.10.2020 17:19, Stephen Frost wrote:

Greetings,

* Georgios Kokolatos (gkokolatos@protonmail.com) wrote:

this patch is in "Ready for committer" state and the Cfbot is happy.

Glad that's still the case. :)

Is there any committer that is available for taking a look at it?

If there aren't any objections or further comments, I'll take another
look through it and will commit it during the upcoming CF.

Thanks!

Stephen

CFM reminder. Just in case you forgot about this thread)
The commitfest is heading to the end. And there was a plenty of time for
anyone to object.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Greetings,

* Anastasia Lubennikova (a.lubennikova@postgrespro.ru) wrote:

On 29.10.2020 17:19, Stephen Frost wrote:

* Georgios Kokolatos (gkokolatos@protonmail.com) wrote:

this patch is in "Ready for committer" state and the Cfbot is happy.

Glad that's still the case. :)

Is there any committer that is available for taking a look at it?

If there aren't any objections or further comments, I'll take another
look through it and will commit it during the upcoming CF.

Thanks!

Stephen

CFM reminder. Just in case you forgot about this thread)
The commitfest is heading to the end. And there was a plenty of time for
anyone to object.

Thanks, I've not forgotten, but it's a bit complicated given that I've
another patch in progress to rename default roles to be predefined
roles which conflicts with this one. Hopefully we'll have a few
comments on that and I can get it committed and this one updated with
the new naming. I'd rather not commit this one and then immediately
commit changes over top of it.

Thanks again!

Stephen

Hi,

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, November 23, 2020 11:31 PM, Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

- Anastasia Lubennikova (a.lubennikova@postgrespro.ru) wrote:

On 29.10.2020 17:19, Stephen Frost wrote:

- Georgios Kokolatos (gkokolatos@protonmail.com) wrote:

this patch is in "Ready for committer" state and the Cfbot is happy.
Glad that's still the case. :)

Is there any committer that is available for taking a look at it?
If there aren't any objections or further comments, I'll take another
look through it and will commit it during the upcoming CF.

Thanks!
Stephen

CFM reminder. Just in case you forgot about this thread)
The commitfest is heading to the end. And there was a plenty of time for
anyone to object.

Thanks, I've not forgotten, but it's a bit complicated given that I've
another patch in progress to rename default roles to be predefined
roles which conflicts with this one. Hopefully we'll have a few
comments on that and I can get it committed and this one updated with
the new naming. I'd rather not commit this one and then immediately
commit changes over top of it.

May I enquire about the status of the current?

//Georgios

Greetings,

* gkokolatos@pm.me (gkokolatos@pm.me) wrote:

On Monday, November 23, 2020 11:31 PM, Stephen Frost <sfrost@snowman.net> wrote:

- Anastasia Lubennikova (a.lubennikova@postgrespro.ru) wrote:

On 29.10.2020 17:19, Stephen Frost wrote:

- Georgios Kokolatos (gkokolatos@protonmail.com) wrote:

this patch is in "Ready for committer" state and the Cfbot is happy.
Glad that's still the case. :)

Is there any committer that is available for taking a look at it?
If there aren't any objections or further comments, I'll take another
look through it and will commit it during the upcoming CF.

CFM reminder. Just in case you forgot about this thread)
The commitfest is heading to the end. And there was a plenty of time for
anyone to object.

Thanks, I've not forgotten, but it's a bit complicated given that I've
another patch in progress to rename default roles to be predefined
roles which conflicts with this one. Hopefully we'll have a few
comments on that and I can get it committed and this one updated with
the new naming. I'd rather not commit this one and then immediately
commit changes over top of it.

May I enquire about the status of the current?

The patch to rename default roles to predefined roles for v14 has gone
in, and so I've come back to this patch to add the
pg_read/write_all_data roles.

Having contemplated a bit further, I ended up deciding that it made more
sense for these predefined roles to *not* have BYPASSRLS, which gives
admins the flexibilty to choose if they actually want RLS to be
bypassed, or not, on the roles who they GRANT these roles to (if we just
always had bypassrls set, then they wouldn't have any choice but to
accept that, which doesn't seem very kind). I've updated the
documentation to make a note of that and to encourage admins who use
these roles to consider if they want to set BYPASSRLS on the actual
login role which they'll have to create in order to use these roles
(since they can't be used to login directly).

Updated patch attached. Will be playing with it a bit more but
generally feel like it's in pretty good shape. Unless there's anything
further on this, I'll likely commit it over the weekend.

Thanks!

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:

Updated patch attached. Will be playing with it a bit more but
generally feel like it's in pretty good shape. Unless there's anything
further on this, I'll likely commit it over the weekend.

Weekend ended up being a bit busy, but I've now pushed this.

Thanks!

Stephen

Hi,

On Thu, Apr 01, 2021 at 04:00:06PM -0400, Stephen Frost wrote:

diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index d171b13236..fe0bdb7599 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -518,6 +518,24 @@ 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 does not have the role attribute
+       <literal>BYPASSRLS</literal> set.  If RLS is being used, an administrator
+       may wish to set <literal>BYPASSRLS</literal> on roles which this role is
+       GRANTed to.</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 does not have the
+       role attribute <literal>BYPASSRLS</literal> set.  If RLS is being used,
+       an administrator may wish to set <literal>BYPASSRLS</literal> on roles
+       which this role is GRANTed to.</entry>
+      </row>

Shouldn't those "SELECT", "INSERT" etc. be wrapped in <command> tags?

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB M�nchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 M�nchengladbach
Gesch�ftsf�hrung: Dr. Michael Meskes, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Greetings,

* Michael Banck (michael.banck@credativ.de) wrote:

On Thu, Apr 01, 2021 at 04:00:06PM -0400, Stephen Frost wrote:

diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index d171b13236..fe0bdb7599 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -518,6 +518,24 @@ 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 does not have the role attribute
+       <literal>BYPASSRLS</literal> set.  If RLS is being used, an administrator
+       may wish to set <literal>BYPASSRLS</literal> on roles which this role is
+       GRANTed to.</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 does not have the
+       role attribute <literal>BYPASSRLS</literal> set.  If RLS is being used,
+       an administrator may wish to set <literal>BYPASSRLS</literal> on roles
+       which this role is GRANTed to.</entry>
+      </row>

Shouldn't those "SELECT", "INSERT" etc. be wrapped in <command> tags?

Yeah, good point, fixed.

Thanks!

Stephen

Hi hackers,

I have tested this new feature with PostgreSQL 14 Beta 3 environment.
I created a user granted with pg_write_all_data role and executed UPDATE and DELETE statements on tables owned by other users.
If there is no WHERE clause, it can be executed as expected, but if the WHERE clause is specified, an error of permission denied will occur.
Is this the expected behavior?
The WHERE clause is not specified in the regression test (privileges.sql).

Below is the execution log.
------------------------------------------------
postgres=# CREATE USER owner1 PASSWORD 'owner1';
CREATE ROLE
postgres=# CREATE USER write1 PASSWORD 'write1';
CREATE ROLE
postgres=# GRANT pg_write_all_data TO write1;
GRANT ROLE
postgres=# SET SESSION AUTHORIZATION owner1;
SET
postgres=> CREATE TABLE data1(c1 INT, c2 VARCHAR(10));
CREATE TABLE
postgres=> INSERT INTO data1 VALUES (generate_series(1, 10), 'data1');
INSERT 0 10
postgres=> SET SESSION AUTHORIZATION write1;
SET
postgres=> INSERT INTO data1 VALUES (0, 'data1'); -- success
INSERT 0 1
postgres=> UPDATE data1 SET c2='update' WHERE c1=0; -- fail
ERROR: permission denied for table data1
postgres=> DELETE FROM data1 WHERE c1=0; -- fail
ERROR: permission denied for table data1
postgres=> UPDATE data1 SET c2='update'; -- success
UPDATE 11
postgres=> DELETE FROM data1; -- success
DELETE 11
postgres=> SELECT version();
version
------------------------------------------------------------------------------------------------------------
PostgreSQL 14beta3 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-39), 64-bit
(1 row)
-------------

Regards,
Noriyoshi Shinoda

-----Original Message-----
From: Stephen Frost [mailto:sfrost@snowman.net]
Sent: Saturday, August 28, 2021 7:34 AM
To: Michael Banck <michael.banck@credativ.de>
Cc: gkokolatos@pm.me; Anastasia Lubennikova <a.lubennikova@postgrespro.ru>; pgsql-hackers@lists.postgresql.org
Subject: Re: New predefined roles- 'pg_read/write_all_data'

Greetings,

* Michael Banck (michael.banck@credativ.de) wrote:

On Thu, Apr 01, 2021 at 04:00:06PM -0400, Stephen Frost wrote:

diff --git a/doc/src/sgml/user-manag.sgml 
b/doc/src/sgml/user-manag.sgml index d171b13236..fe0bdb7599 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -518,6 +518,24 @@ 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 does not have the role attribute
+       <literal>BYPASSRLS</literal> set.  If RLS is being used, an administrator
+       may wish to set <literal>BYPASSRLS</literal> on roles which this role is
+       GRANTed to.</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 does not have the
+       role attribute <literal>BYPASSRLS</literal> set.  If RLS is being used,
+       an administrator may wish to set <literal>BYPASSRLS</literal> on roles
+       which this role is GRANTed to.</entry>
+      </row>

Shouldn't those "SELECT", "INSERT" etc. be wrapped in <command> tags?

Yeah, good point, fixed.

Thanks!

Stephen

Greetings,

On Sun, Sep 5, 2021 at 07:43 Shinoda, Noriyoshi (PN Japan FSIP) <
noriyoshi.shinoda@hpe.com> wrote:

I have tested this new feature with PostgreSQL 14 Beta 3 environment.
I created a user granted with pg_write_all_data role and executed UPDATE
and DELETE statements on tables owned by other users.
If there is no WHERE clause, it can be executed as expected, but if the
WHERE clause is specified, an error of permission denied will occur.
Is this the expected behavior?

A WHERE clause requires SELECT rights on the table/columns referenced and
if no SELECT rights were granted then a permission denied error is the
correct result, yes. Note that pg_write_all_data, as documented, does not
include SELECT rights.

Thanks,

Stephen

Thank you for your quick response.
I understood the specifications from your explanation.

Regards,
Noriyoshi Shinoda

From: Stephen Frost [mailto:sfrost@snowman.net]
Sent: Sunday, September 5, 2021 8:50 PM
To: Shinoda, Noriyoshi (PN Japan FSIP) <noriyoshi.shinoda@hpe.com>
Cc: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>; Michael Banck <michael.banck@credativ.de>; gkokolatos@pm.me; pgsql-hackers@lists.postgresql.org
Subject: Re: New predefined roles- 'pg_read/write_all_data'

Greetings,

On Sun, Sep 5, 2021 at 07:43 Shinoda, Noriyoshi (PN Japan FSIP) <noriyoshi.shinoda@hpe.com<mailto:noriyoshi.shinoda@hpe.com>> wrote:
I have tested this new feature with PostgreSQL 14 Beta 3 environment.
I created a user granted with pg_write_all_data role and executed UPDATE and DELETE statements on tables owned by other users.
If there is no WHERE clause, it can be executed as expected, but if the WHERE clause is specified, an error of permission denied will occur.
Is this the expected behavior?

A WHERE clause requires SELECT rights on the table/columns referenced and if no SELECT rights were granted then a permission denied error is the correct result, yes. Note that pg_write_all_data, as documented, does not include SELECT rights.

Thanks,

Stephen