Re: New default role- 'pg_read_all_data'

[gkokolatos]

Hi,




‐‐‐ Original Message ‐‐‐
On Monday, November 23, 2020 11:31 PM, Stephen Frost  wrote:

> Greetings,
>
> -   Anastasia Lubennikova (a.lubennik...@postgrespro.ru) wrote:
>
> > On 29.10.2020 17:19, Stephen Frost wrote:
> >
> > > -   Georgios Kokolatos (gkokola...@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

>
> Thanks again!
>
> Stephen

Re: New default role- 'pg_read_all_data'

[Stephen Frost]

Greetings,

* Anastasia Lubennikova (a.lubennik...@postgrespro.ru) wrote:
> On 29.10.2020 17:19, Stephen Frost wrote:
> >* Georgios Kokolatos (gkokola...@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


signature.asc
Description: PGP signature

Re: New default role- 'pg_read_all_data'

[Anastasia Lubennikova]

On 29.10.2020 17:19, Stephen Frost wrote:

Greetings,

* Georgios Kokolatos (gkokola...@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

Re: New default role- 'pg_read_all_data'

[Georgios Kokolatos]

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

Re: New default role- 'pg_read_all_data'

[Stephen Frost]

Greetings,

* Georgios Kokolatos (gkokola...@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


signature.asc
Description: PGP signature

Re: New default role- 'pg_read_all_data'

[Georgios Kokolatos]

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

Re: New default role- 'pg_read_all_data'

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Monday, 31 August 2020 02:20, Stephen Frost  wrote:

> Greetings,
>
> -   Stephen Frost (sfr...@snowman.net) wrote:
>
> > -   Magnus Hagander (mag...@hagander.net) wrote:
> >
> > > On Fri, Aug 28, 2020 at 2:38 PM Stephen Frost sfr...@snowman.net wrote:
> > >
> > > > -   Magnus Hagander (mag...@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

>
> Thanks!
>
> Stephen



pg_rorole_v2_minimal_test.out
Description: Binary data


pg_rorole_v2_minimal_test.sql
Description: application/sql

Re: New default role- 'pg_read_all_data'

[Stephen Frost]

Greetings,

* Stephen Frost (sfr...@snowman.net) wrote:
> * Magnus Hagander (mag...@hagander.net) wrote:
> > On Fri, Aug 28, 2020 at 2:38 PM Stephen Frost  wrote:
> > > * Magnus Hagander (mag...@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
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 829decd883..1213125bfd 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -517,6 +517,20 @@ DROP ROLE doomed_role;
   
  
  
+  
+   pg_read_all_data
+   Read all data (tables, views, sequences), as if having SELECT
+   rights on those objects, and USAGE rights on all schemas, even without
+   having it explicitly.  This role also has BYPASSRLS
+   set for it.
+  
+  
+   pg_write_all_data
+   Write all data (tables, views, sequences), as if having INSERT,
+   UPDATE, and DELETE rights on those objects, and USAGE rights on all
+   schemas, even without having it explicitly.  This role also has
+   BYPASSRLS set for it.
+  
   
pg_read_all_settings
Read all configuration variables, even those normally visible only to
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index c626161408..3e6d060554 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3849,6 +3849,26 @@ pg_class_aclmask(Oid table_oid, Oid roleid,
 
 	ReleaseSysCache(tuple);
 
+	/*
+	 * Check if ACL_SELECT is being checked and, if so, and not set already
+	 * as part of the result, then check if the user is a member of the
+	 * pg_read_all_data role, which allows read access to all relations.
+	 */
+	if (mask & ACL_SELECT && !(result & ACL_SELECT) &&
+		has_privs_of_role(roleid, DEFAULT_ROLE_READ_ALL_DATA))
+		result |= ACL_SELECT;
+
+	/*
+	 * Check if ACL_INSERT, ACL_UPDATE, or ACL_DELETE is being checked
+	 * and, if so, and not set already as part of the result, then check
+	 * if the user is a member of the pg_write_all_data role, which
+	 * allows INSERT/UPDATE/DELETE access to all relations.
+	 */
+	if (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE) &&
+	   !(result & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)) &&
+		has_privs_of_role(roleid, DEFAULT_ROLE_WRITE_ALL_DATA))
+		result |= (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE));
+
 	return result;
 }
 
@@ -4175,6 +4195,16 @@ pg_namespace_aclmask(Oid nsp_oid, Oid roleid,
 
 	ReleaseSysCache(tuple);
 
+	/*
+	 * Check if ACL_USAGE is being checked and, if so, and not set already
+	 * as part of the result, then check if the user is a member of the
+	 * pg_read_all_data or pg_write_all_data roles, which allow usage
+	 * access to all schemas.
+	 */
+	if (mask & ACL_USAGE && !(result & ACL_USAGE) &&
+		(has_privs_of_role(roleid, DEFAULT_ROLE_READ_ALL_DATA) ||
+		has_privs_of_role(roleid, DEFAULT_ROLE_WRITE_ALL_DATA)))
+		result |= ACL_USAGE;
 	return result;
 }
 
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 7c08851550..3f72474146 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -25,6 +25,16 @@
   rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
   rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
   rolpassword => '_null_', rolvaliduntil 

Re: New default role- 'pg_read_all_data'

[Stephen Frost]

Greetings,

* Gilles Darold (gil...@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


signature.asc
Description: PGP signature

Re: New default role- 'pg_read_all_data'

[Georgios Kokolatos]

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,

Re: New default role- 'pg_read_all_data'

[Gilles Darold]

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

Greetings,

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

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

* Magnus Hagander (mag...@hagander.net) wrote:

On Fri, Aug 28, 2020 at 2:38 PM Stephen Frost  wrote:

* Magnus Hagander (mag...@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

Re: New default role- 'pg_read_all_data'

[Stephen Frost]

Greetings,

* Gilles Darold (gil...@darold.net) wrote:
> Le 28/08/2020 à 15:26, Stephen Frost a écrit :
> >* Magnus Hagander (mag...@hagander.net) wrote:
> >>On Fri, Aug 28, 2020 at 2:38 PM Stephen Frost  wrote:
> >>>* Magnus Hagander (mag...@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


signature.asc
Description: PGP signature

Re: New default role- 'pg_read_all_data'

[Gilles Darold]

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

Greetings,

* Magnus Hagander (mag...@hagander.net) wrote:

On Fri, Aug 28, 2020 at 2:38 PM Stephen Frost  wrote:

* Magnus Hagander (mag...@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

Re: New default role- 'pg_read_all_data'

[Stephen Frost]

Greetings,

* Magnus Hagander (mag...@hagander.net) wrote:
> On Fri, Aug 28, 2020 at 2:38 PM Stephen Frost  wrote:
> > * Magnus Hagander (mag...@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


signature.asc
Description: PGP signature

Re: New default role- 'pg_read_all_data'

[Isaac Morland]

On Fri, 28 Aug 2020 at 08:54, Stephen Frost  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.

Re: New default role- 'pg_read_all_data'

[Stephen Frost]

Greetings,

* gkokola...@pm.me (gkokola...@pm.me) wrote:
> On Friday, 28 August 2020 15:43, Stephen Frost  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


signature.asc
Description: PGP signature

Re: New default role- 'pg_read_all_data'

[gkokolatos]

‐‐‐ Original Message ‐‐‐
On Friday, 28 August 2020 15:43, Stephen Frost  wrote:

> Greetings,
>
> -   Georgios Kokolatos (gkokola...@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.

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

Re: New default role- 'pg_read_all_data'

[Magnus Hagander]

On Fri, Aug 28, 2020 at 2:43 PM Stephen Frost  wrote:

> Greetings,
>
> * Georgios Kokolatos (gkokola...@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/ 
 Work: https://www.redpill-linpro.com/ 

Re: New default role- 'pg_read_all_data'

[Stephen Frost]

Greetings,

* Isaac Morland (isaac.morl...@gmail.com) wrote:
> On Fri, 28 Aug 2020 at 08:43, Stephen Frost  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


signature.asc
Description: PGP signature

Re: New default role- 'pg_read_all_data'

[Isaac Morland]

On Fri, 28 Aug 2020 at 08:43, Stephen Frost  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.

Re: New default role- 'pg_read_all_data'

[Magnus Hagander]

On Fri, Aug 28, 2020 at 2:38 PM Stephen Frost  wrote:

> Greetings,
>
> * Magnus Hagander (mag...@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/ 
 Work: https://www.redpill-linpro.com/ 

Re: New default role- 'pg_read_all_data'

[Stephen Frost]

Greetings,

* Georgios Kokolatos (gkokola...@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


signature.asc
Description: PGP signature

Re: New default role- 'pg_read_all_data'

[Stephen Frost]

Greetings,

* Magnus Hagander (mag...@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


signature.asc
Description: PGP signature

Re: New default role- 'pg_read_all_data'

[Magnus Hagander]

On Fri, Aug 28, 2020 at 2:30 AM Stephen Frost  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/ 
 Work: https://www.redpill-linpro.com/ 

Re: New default role- 'pg_read_all_data'

[Steven Pousty]

On Thu, Aug 27, 2020 at 5:30 PM Stephen Frost  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.

New default role- 'pg_read_all_data'

[Stephen Frost]

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
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 829decd883..09b3cd6cb8 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -517,6 +517,12 @@ DROP ROLE doomed_role;
   
  
  
+  
+   pg_read_all_data
+   Read all data (tables, sequences), even without having explicit
+   GRANT rights to the object.  Implicitly includes USAGE rights on all
+   schemas.
+  
   
pg_read_all_settings
Read all configuration variables, even those normally visible only to
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index c626161408..aca1deeca8 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3849,6 +3849,15 @@ pg_class_aclmask(Oid table_oid, Oid roleid,
 
 	ReleaseSysCache(tuple);
 
+	/*
+	 * Check if ACL_SELECT is being checked and, if so, and not set already
+	 * as part of the result, then check if the user is a member of the
+	 * pg_read_all_data role, which allows read access to all relations.
+	 */
+	if (mask & ACL_SELECT && !(result & ACL_SELECT) &&
+		has_privs_of_role(roleid, DEFAULT_ROLE_READ_ALL_DATA))
+		result |= ACL_SELECT;
+
 	return result;
 }
 
@@ -4175,6 +4184,14 @@ pg_namespace_aclmask(Oid nsp_oid, Oid roleid,
 
 	ReleaseSysCache(tuple);
 
+	/*
+	 * Check if ACL_USAGE is being checked and, if so, and not set already
+	 * as part of the result, then check if the user is a member of the
+	 * pg_read_all_data role, which allows usage access to all schemas.
+	 */
+	if (mask & ACL_USAGE && !(result & ACL_USAGE) &&
+		has_privs_of_role(roleid, DEFAULT_ROLE_READ_ALL_DATA))
+		result |= ACL_USAGE;
 	return result;
 }
 
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 7c08851550..0dc4b2baec 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -25,6 +25,11 @@
   rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
   rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
   rolpassword => '_null_', rolvaliduntil => '_null_' },
+{ oid => '3435', oid_symbol => 'DEFAULT_ROLE_READ_ALL_DATA',
+  rolname => 'pg_read_all_data', rolsuper => 'f', rolinherit => 't',
+  rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
+  rolreplication => 'f', rolbypassrls => 't', rolconnlimit => '-1',
+  rolpassword => '_null_', rolvaliduntil => '_null_' },
 { oid => '3374', oid_symbol => 'DEFAULT_ROLE_READ_ALL_SETTINGS',
   rolname => 'pg_read_all_settings', rolsuper => 'f', rolinherit => 't',
   rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 3ec22c20ea..411525bcd1 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -24,8 +24,10 @@ CREATE USER regress_priv_user2;
 CREATE USER regress_priv_user3;
 CREATE USER regress_priv_user4;
 CREATE USER regress_priv_user5;
+CREATE USER regress_priv_user6;
 CREATE USER regress_priv_user5;	-- duplicate
 ERROR:  role "regress_priv_user5" already exists
+GRANT pg_read_all_data TO regress_priv_user6;
 CREATE GROUP regress_priv_group1;
 CREATE GROUP regress_priv_group2 WITH USER regress_priv_user1, regress_priv_user2;
 ALTER GROUP regress_priv_group1 ADD USER regress_priv_user4;
@@ -129,6 +131,21 @@ SELECT * FROM atest2 WHERE ( col1 IN ( SELECT b FROM atest1 ) );
 --+--
 (0 rows)
 
+SET SESSION AUTHORIZATION regress_priv_user6;
+SELECT * FROM atest1; -- ok
+ a |  b  
+---+-
+ 1 | two
+ 1 | two
+(2 rows)
+
+SELECT * FROM atest2; -- ok
+ col1 | col2