Thread: New default role- 'pg_read_all_data'
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?
Will add to the September CF.
Thanks,
Stephen
Attachment
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?
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
Attachment
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
Attachment
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)
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.
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
Attachment
‐‐‐‐‐‐‐ 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
abovedid 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
operationbut will permit the insert operation? If no, should there be one?
I hope I am clearer now.
Thank you again for your patience.
>
> > 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
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
GRANTcommand?
> >
> > 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
abovedid 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
operationbut 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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
‐‐‐‐‐‐‐ 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 subthreadbellow: >> My expectation was not met since in my manual test (unless I made a mistake which is entirely possible), the SELECT abovedid 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 operationbut 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 thecommunity 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 ratherclear 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 > > Thanks! > > Stephen
Attachment
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 thecommunity 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 shouldnot 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
Attachment
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, * 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
Attachment
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
Attachment
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
Attachment
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
permissiondenied 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> 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