Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Fri, May 6, 2016 at 12:12 PM, Stephen Frost  wrote:
> I've been thinking the same, my flight just arrived into DC and I'll be
> pushing it all shortly after I get home.

To be clear, I wasn't simply saying that you should commit these
patches today instead of tomorrow, although I'm glad you did that and
fully endorse that decision.  I was saying they should have been
committed last week or sooner instead of one business day before beta1
wraps.  I also would have preferred to see each patch go in as it got
done rather than pushing a whole stack of commits all at once.  I
realize that there's nothing you can do about any of this at this
point short of developing a time machine, but I'm mentioning it for
next time.

Thanks for committing, and thanks for watching the BF.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, May 5, 2016 at 10:41 AM, Stephen Frost  wrote:
> > Barring objections or concerns, I'll push this sometime tomorrow
> > (probably after I get back to DC).
> 
> It really would have been good to get this stuff done sooner.  By the
> time you push this, there will barely be enough time for a buildfarm
> cycle, let alone time for any testing people may be doing against
> latest master to find problems.

Alright, I've pushed all of my pending patches.  If anyone needs me,
I'll be busy reloading the buildfarm page.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

Robert,

On Friday, May 6, 2016, Robert Haas  wrote:

> On Thu, May 5, 2016 at 10:41 AM, Stephen Frost  > wrote:
> > Barring objections or concerns, I'll push this sometime tomorrow
> > (probably after I get back to DC).
>
> It really would have been good to get this stuff done sooner.  By the
> time you push this, there will barely be enough time for a buildfarm
> cycle, let alone time for any testing people may be doing against
> latest master to find problems.
>

I've been thinking the same, my flight just arrived into DC and I'll be
pushing it all shortly after I get home.

Thanks!

Stephen

Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Thu, May 5, 2016 at 10:41 AM, Stephen Frost  wrote:
> Barring objections or concerns, I'll push this sometime tomorrow
> (probably after I get back to DC).

It really would have been good to get this stuff done sooner.  By the
time you push this, there will barely be enough time for a buildfarm
cycle, let alone time for any testing people may be doing against
latest master to find problems.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

* Stephen Frost (sfr...@snowman.net) wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
> > On Tue, Apr 26, 2016 at 7:39 PM, Robert Haas  wrote:
> > > On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost  wrote:
> > >> Based on our discussion at PGConf.US and the comments up-thread from
> > >> Tom, I'll work up a patch to remove those checks around SET ROLE and
> > >> friends which were trying to prevent default roles from possibly being
> > >> made to own objects.
> > >>
> > >> Should the checks, which have been included since nearly the start of
> > >> this version of the patch, to prevent users from GRANT'ing other rights
> > >> to the default roles remain?  Or should those also be removed?  I
> > >> *think* pg_dump/pg_upgrade would be fine with rights being added, and if
> > >> we aren't preventing ownership of objects then we aren't going to be
> > >> able to remove such roles in any case.
> > >
> > > It'd be good to test that that works.  If it does, I think we may as
> > > well allow it.
> > >
> > >> Of course, with these default roles, users can't REVOKE the rights which
> > >> are granted to them as that happens in C code, outside of the GRANT
> > >> system.
> > >
> > > I think you mean that they can't revoke the special magic rights, but
> > > they could revoke any additional privileges which were granted.
> > >
> > >> Working up a patch to remove these checks should be pretty quickly done
> > >> (iirc, I've actually got an independent patch around from when I added
> > >> them, just need to find it and then go through the committed patches to
> > >> make sure I take care of everything), but would like to make sure that
> > >> we're now all on the same page and that *all* of these checks should be
> > >> removed, making default roles just exactly like "regular" roles, except
> > >> that they're created at initdb time and have "special" rights provided
> > >> by C-level code checks.
> > >
> > > That's what I'm thinking.  I would welcome other views.
> > 
> > Ping!
> 
> Thanks.  I'm planning to post a patch tomorrow to remove these checks.

Apologies about not getting to this yesterday, was pretty busy finding
pre-existing issues in pg_dump.

Attached is a patch which removes the various special checks that I had
added to prevent using default roles like regular roles.  As noted in
the commit message, users are still prevented from creating roles in the
"pg_" namespace and from ALTER'ing those roles, but otherwise they're
very much like regular roles.

I've adjusted the regression tests accordingly, but I'm going to do more
testing to make sure that pg_dump handles them correctly (and will be
adding cases to my pg_dump TAP test suite to ensure that they stay
working) over the next day or so.

Barring objections or concerns, I'll push this sometime tomorrow
(probably after I get back to DC).

Thanks!

Stephen
From 4fae6e77eddc86360381e44f35d4da4a495cbdc1 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 5 May 2016 09:52:26 -0400
Subject: [PATCH] Remove various special checks around default roles

Default roles really should be like regular roles, for the most part.
This removes a number of checks that were trying to make default roles
extra special by not allowing them to be used as regular roles.

We still prevent users from creating roles in the "pg_" namespace or
from altering roles which exist in that namespace via ALTER ROLE, as
we can't preserve such changes, but otherwise the roles are very much
like regular roles.

Based on discussion with Robert and Tom.
---
 src/backend/catalog/aclchk.c|  7 ---
 src/backend/commands/alter.c|  3 ---
 src/backend/commands/foreigncmds.c  | 13 -
 src/backend/commands/policy.c   |  5 -
 src/backend/commands/schemacmds.c   |  4 
 src/backend/commands/tablecmds.c|  2 --
 src/backend/commands/tablespace.c   |  4 
 src/backend/commands/user.c | 11 ---
 src/backend/commands/variable.c |  7 ---
 src/test/regress/expected/rolenames.out | 18 +-
 src/test/regress/sql/rolenames.sql  | 10 +-
 11 files changed, 10 insertions(+), 74 deletions(-)

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7d656d5..d074e85 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -423,9 +423,6 @@ ExecuteGrantStmt(GrantStmt *stmt)
 grantee_uid = ACL_ID_PUBLIC;
 break;
 			default:
-if (!IsBootstrapProcessingMode())
-	check_rolespec_name((Node *) grantee,
-			"Cannot GRANT or REVOKE privileges to or from a reserved role.");
 grantee_uid = get_rolespec_oid((Node *) grantee, false);
 break;
 		}
@@ -921,8 +918,6 @@ ExecAlterDefaultPrivilegesStmt(AlterDefaultPrivilegesStmt *stmt)
 grantee_uid = ACL_ID_PUBLIC;
 break;
 			default:
-check_rolespec_name((Node *) grantee,
-	"Cannot GRANT or REVOKE 

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Apr 26, 2016 at 7:39 PM, Robert Haas  wrote:
> > On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost  wrote:
> >> Based on our discussion at PGConf.US and the comments up-thread from
> >> Tom, I'll work up a patch to remove those checks around SET ROLE and
> >> friends which were trying to prevent default roles from possibly being
> >> made to own objects.
> >>
> >> Should the checks, which have been included since nearly the start of
> >> this version of the patch, to prevent users from GRANT'ing other rights
> >> to the default roles remain?  Or should those also be removed?  I
> >> *think* pg_dump/pg_upgrade would be fine with rights being added, and if
> >> we aren't preventing ownership of objects then we aren't going to be
> >> able to remove such roles in any case.
> >
> > It'd be good to test that that works.  If it does, I think we may as
> > well allow it.
> >
> >> Of course, with these default roles, users can't REVOKE the rights which
> >> are granted to them as that happens in C code, outside of the GRANT
> >> system.
> >
> > I think you mean that they can't revoke the special magic rights, but
> > they could revoke any additional privileges which were granted.
> >
> >> Working up a patch to remove these checks should be pretty quickly done
> >> (iirc, I've actually got an independent patch around from when I added
> >> them, just need to find it and then go through the committed patches to
> >> make sure I take care of everything), but would like to make sure that
> >> we're now all on the same page and that *all* of these checks should be
> >> removed, making default roles just exactly like "regular" roles, except
> >> that they're created at initdb time and have "special" rights provided
> >> by C-level code checks.
> >
> > That's what I'm thinking.  I would welcome other views.
> 
> Ping!

Thanks.  I'm planning to post a patch tomorrow to remove these checks.

Thanks again!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Tue, Apr 26, 2016 at 7:39 PM, Robert Haas  wrote:
> On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost  wrote:
>> Based on our discussion at PGConf.US and the comments up-thread from
>> Tom, I'll work up a patch to remove those checks around SET ROLE and
>> friends which were trying to prevent default roles from possibly being
>> made to own objects.
>>
>> Should the checks, which have been included since nearly the start of
>> this version of the patch, to prevent users from GRANT'ing other rights
>> to the default roles remain?  Or should those also be removed?  I
>> *think* pg_dump/pg_upgrade would be fine with rights being added, and if
>> we aren't preventing ownership of objects then we aren't going to be
>> able to remove such roles in any case.
>
> It'd be good to test that that works.  If it does, I think we may as
> well allow it.
>
>> Of course, with these default roles, users can't REVOKE the rights which
>> are granted to them as that happens in C code, outside of the GRANT
>> system.
>
> I think you mean that they can't revoke the special magic rights, but
> they could revoke any additional privileges which were granted.
>
>> Working up a patch to remove these checks should be pretty quickly done
>> (iirc, I've actually got an independent patch around from when I added
>> them, just need to find it and then go through the committed patches to
>> make sure I take care of everything), but would like to make sure that
>> we're now all on the same page and that *all* of these checks should be
>> removed, making default roles just exactly like "regular" roles, except
>> that they're created at initdb time and have "special" rights provided
>> by C-level code checks.
>
> That's what I'm thinking.  I would welcome other views.

Ping!

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Alvaro Herrera]

Robert Haas wrote:
> On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost  wrote:

> > Working up a patch to remove these checks should be pretty quickly done
> > (iirc, I've actually got an independent patch around from when I added
> > them, just need to find it and then go through the committed patches to
> > make sure I take care of everything), but would like to make sure that
> > we're now all on the same page and that *all* of these checks should be
> > removed, making default roles just exactly like "regular" roles, except
> > that they're created at initdb time and have "special" rights provided
> > by C-level code checks.
> 
> That's what I'm thinking.  I would welcome other views.

I agree with that view, assuming pg_dump correctly produces GRANTs to
replicate what was done in a database regarding those roles (and same
with pg_dumpall -g).  I think it already works that way, but I may be
mistaken.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost  wrote:
> Based on our discussion at PGConf.US and the comments up-thread from
> Tom, I'll work up a patch to remove those checks around SET ROLE and
> friends which were trying to prevent default roles from possibly being
> made to own objects.
>
> Should the checks, which have been included since nearly the start of
> this version of the patch, to prevent users from GRANT'ing other rights
> to the default roles remain?  Or should those also be removed?  I
> *think* pg_dump/pg_upgrade would be fine with rights being added, and if
> we aren't preventing ownership of objects then we aren't going to be
> able to remove such roles in any case.

It'd be good to test that that works.  If it does, I think we may as
well allow it.

> Of course, with these default roles, users can't REVOKE the rights which
> are granted to them as that happens in C code, outside of the GRANT
> system.

I think you mean that they can't revoke the special magic rights, but
they could revoke any additional privileges which were granted.

> Working up a patch to remove these checks should be pretty quickly done
> (iirc, I've actually got an independent patch around from when I added
> them, just need to find it and then go through the committed patches to
> make sure I take care of everything), but would like to make sure that
> we're now all on the same page and that *all* of these checks should be
> removed, making default roles just exactly like "regular" roles, except
> that they're created at initdb time and have "special" rights provided
> by C-level code checks.

That's what I'm thinking.  I would welcome other views.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

Robert, all,

[... comments elsewhere made me realize I hadn't actually sent this when
I thought I had, my apologies on that ...]

* Robert Haas (robertmh...@gmail.com) wrote:
> Great.  But there's no particular use case served by a lot of things
> which are natural outgrowths of the rest of the system which we permit
> anyway because it's too awkward otherwise - like zero-column tables.

Based on our discussion at PGConf.US and the comments up-thread from
Tom, I'll work up a patch to remove those checks around SET ROLE and
friends which were trying to prevent default roles from possibly being
made to own objects.

Should the checks, which have been included since nearly the start of
this version of the patch, to prevent users from GRANT'ing other rights
to the default roles remain?  Or should those also be removed?  I
*think* pg_dump/pg_upgrade would be fine with rights being added, and if
we aren't preventing ownership of objects then we aren't going to be
able to remove such roles in any case.

Of course, with these default roles, users can't REVOKE the rights which
are granted to them as that happens in C code, outside of the GRANT
system.

Working up a patch to remove these checks should be pretty quickly done
(iirc, I've actually got an independent patch around from when I added
them, just need to find it and then go through the committed patches to
make sure I take care of everything), but would like to make sure that
we're now all on the same page and that *all* of these checks should be
removed, making default roles just exactly like "regular" roles, except
that they're created at initdb time and have "special" rights provided
by C-level code checks.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Fri, Apr 15, 2016 at 11:56 AM, Stephen Frost  wrote:
> > Perhaps it would be helpful to return to the initial goal of these
> > default roles.
> >
> > We've identified certain privileges which are currently superuser-only
> > and we would like to be able to be GRANT those to non-superuser roles.
> > Where our GRANT system is able to provide the granularity desired, we
> > have simply removed the superuser checks, set up appropriate default
> > values and, through the "pg_dump dump catalog ACLs" patch, allowed
> > administrators to make the changes to those objects via the
> > 'GRANT privilege ON object TO role' system.
> >
> > For those cases where our GRANT system is unable to provide the
> > granularity desired and it isn't sensible to extend the GRANT system to
> > cover the exact granularity we wish to provide, we needed to come up
> > with an alternative approach.  That approach is the use of special
> > default roles, where we can allow exactly the level of granularity
> > desired and give administrators a way to grant those privileges to
> > users.
> >
> > What this means in a nutshell is that we've collectivly decided that
> > we'd rather support:
> >
> > /* uses the 'GRANT role TO role' system */
> > GRANT pg_signal_backend TO test_user;
> >
> > than:
> >
> > /*
> >  * uses the 'GRANT privilege ON object TO role' system
> >  * seems like cluster-level is actually the right answer here, but
> >  * we don't support such an object type currently, so using database
> >  * for the example
> >  */
> > GRANT SIGNAL BACKEND ON DATABASE my_database TO test_user;
> >
> > The goal being that the result of the two commands is identical (where
> > the second command is only hypothetical at this point, but hopefully the
> > meaning is conveyed).
> 
> I quite understand all of that.  The problem isn't that I don't
> understand what you did.  The problem is that I don't agree with it.

Ok.  Your prior comment was "I don't get it."  That's why I was
trying to explain the intent and the reasoning behind it.  I guess I
misunderstodd what you meant with that comment.

> I don't think that the way you implemented is a good idea, nor do I
> think it reflects the consensus that was reached on list.  There was
> agreement to create some special magic roles.  There was not agreement
> around the special magic you've sprinkled on those roles that makes
> them work unlike all other roles in the system, and I think that
> special magic is a bad idea.

The concern was raised by Noah about these users being able to own
objects here: 20160221022639.ga486...@tornado.leadboat.com.  Admittedly,
he merely brought it up as a potential concern without specifying a
preference, but after considering it, I realized that there are issues
with default roles being able to own objects and attempted to address
that issue.  I don't think it would have been appropriate to ignore
these issues.

> > However, GRANT'ing a role to a user traditionally also allows the user
> > to 'SET ROLE' to that user, to create objects as that user, and other
> > privileges.  This results in two issues, as I see it:
> >
> > 1) The administrator who is looking to grant the specific 'signal
> >backend' privilege to a user is unlikely to intend or desire for that
> >user to also be able to SET ROLE to the role, or for that user to be
> >able to create objects as the default role.
> 
> I don't think being able to SET ROLE to that role is something that we
> should be trying to prevent.  You're already discovering why.  You
> tried to create this new special kind of role that you can't become,
> and there are already various reports of cases that you've missed.
> You will keep finding more for a while, I predict.  If that role is
> minimally privileged, who cares if users can become it?

Primairly because objects could be created as that user.  I agree that
in well run systems, it's unlikely that will happen, but we can't simply
trust that if we wish to be able to remove or change these roles down
the road.

> > 2) If the default role ends up owning objects then we have much less
> >flexibility in making changes to that role because we must ensure
> >that those objects are preserved across upgrades, etc.
> 
> Tom already said his piece on this point, and I think he's right.  You
> re-stating the argument doesn't change that.  In any case, if we want
> to prevent this, I bet there's some less buggy and invasive way in
> which it could be done than what you've actually chosen.

I re-stated the concern because I don't think it should be missed or
overlooked lightly because it implies requirements on us down the road,
particularly as I had understood from your prior response that it
wasn't clear what the concern was.

I don't mind removing the checks which were added to attempt to prevent
these roles from owning objects, to be clear.  It'd certainly simplify
things, today.  

Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Fri, Apr 15, 2016 at 11:56 AM, Stephen Frost  wrote:
> Perhaps it would be helpful to return to the initial goal of these
> default roles.
>
> We've identified certain privileges which are currently superuser-only
> and we would like to be able to be GRANT those to non-superuser roles.
> Where our GRANT system is able to provide the granularity desired, we
> have simply removed the superuser checks, set up appropriate default
> values and, through the "pg_dump dump catalog ACLs" patch, allowed
> administrators to make the changes to those objects via the
> 'GRANT privilege ON object TO role' system.
>
> For those cases where our GRANT system is unable to provide the
> granularity desired and it isn't sensible to extend the GRANT system to
> cover the exact granularity we wish to provide, we needed to come up
> with an alternative approach.  That approach is the use of special
> default roles, where we can allow exactly the level of granularity
> desired and give administrators a way to grant those privileges to
> users.
>
> What this means in a nutshell is that we've collectivly decided that
> we'd rather support:
>
> /* uses the 'GRANT role TO role' system */
> GRANT pg_signal_backend TO test_user;
>
> than:
>
> /*
>  * uses the 'GRANT privilege ON object TO role' system
>  * seems like cluster-level is actually the right answer here, but
>  * we don't support such an object type currently, so using database
>  * for the example
>  */
> GRANT SIGNAL BACKEND ON DATABASE my_database TO test_user;
>
> The goal being that the result of the two commands is identical (where
> the second command is only hypothetical at this point, but hopefully the
> meaning is conveyed).

I quite understand all of that.  The problem isn't that I don't
understand what you did.  The problem is that I don't agree with it.
I don't think that the way you implemented is a good idea, nor do I
think it reflects the consensus that was reached on list.  There was
agreement to create some special magic roles.  There was not agreement
around the special magic you've sprinkled on those roles that makes
them work unlike all other roles in the system, and I think that
special magic is a bad idea.

> However, GRANT'ing a role to a user traditionally also allows the user
> to 'SET ROLE' to that user, to create objects as that user, and other
> privileges.  This results in two issues, as I see it:
>
> 1) The administrator who is looking to grant the specific 'signal
>backend' privilege to a user is unlikely to intend or desire for that
>user to also be able to SET ROLE to the role, or for that user to be
>able to create objects as the default role.

I don't think being able to SET ROLE to that role is something that we
should be trying to prevent.  You're already discovering why.  You
tried to create this new special kind of role that you can't become,
and there are already various reports of cases that you've missed.
You will keep finding more for a while, I predict.  If that role is
minimally privileged, who cares if users can become it?

> 2) If the default role ends up owning objects then we have much less
>flexibility in making changes to that role because we must ensure
>that those objects are preserved across upgrades, etc.

Tom already said his piece on this point, and I think he's right.  You
re-stating the argument doesn't change that.  In any case, if we want
to prevent this, I bet there's some less buggy and invasive way in
which it could be done than what you've actually chosen.

> Further, there seems to be no particular use-case which is satisfied by
> allowing SET ROLE'ing to the default roles or for those roles to own
> objects; indeed, it seems more likely to simply spark confusion and
> ultimately may prevent administrators from making use of this system for
> granting privileges as they are unable to GRANT just the specific
> privilege they wish to.

Great.  But there's no particular use case served by a lot of things
which are natural outgrowths of the rest of the system which we permit
anyway because it's too awkward otherwise - like zero-column tables.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[David G. Johnston]

On Fri, Apr 15, 2016 at 8:56 AM, Stephen Frost  wrote:

> Robert,
>
> * Robert Haas (robertmh...@gmail.com) wrote:
> > On Wed, Apr 13, 2016 at 6:53 PM, Stephen Frost 
> wrote:
> > > Requiring that SET ROLE be allowed will mean that many more paths must
> > > be checked and adjusted, such as in all of the CreateObject statements
> > > and potentionally many other paths that I'm not thinking of here, not
> > > the least of which are all of the *existing* checks near
> > > get_rolespec_oid/tuple which will require additional checks for the
> > > CURRENT_USER case to see if the current user is a default role.
> >
> > I don't get it.  I think that these new roles should work just like
> > any other roles, except for existing at initdb time.  I don't see why
> > they would require checking or adjusting any code-paths at all.  It
> > would presumably have made the patch you committed smaller and
> > simpler.  The only thing you'd need to do is (approximately) not dump
> > them.
>
> Perhaps it would be helpful to return to the initial goal of these
> default roles.
>
> We've identified certain privileges which are currently superuser-only
> and we would like to be able to be GRANT those to non-superuser roles.
> Where our GRANT system is able to provide the granularity desired, we
> have simply removed the superuser checks, set up appropriate default
> values and, through the "pg_dump dump catalog ACLs" patch, allowed
> administrators to make the changes to those objects via the
> 'GRANT privilege ON object TO role' system.
>
> For those cases where our GRANT system is unable to provide the
> granularity desired and it isn't sensible to extend the GRANT system to
> cover the exact granularity we wish to provide, we needed to come up
> with an alternative approach.  That approach is the use of special
> default roles, where we can allow exactly the level of granularity
> desired and give administrators a way to grant those privileges to
> users.
>

​I didn't fully comprehend this before, but now I understand why additional
checks beyond simple "has inherited the necessary grant" are needed.  The
checks being performed are not for itemized granted capabilities

Further, there seems to be no particular use-case which is satisfied by
> allowing SET ROLE'ing to the default roles or for those roles to own
> objects; indeed, it seems more likely to simply spark confusion and
> ultimately may prevent administrators from making use of this system for
> granting privileges as they are unable to GRANT just the specific
> privilege they wish to.
>
>
​And I'm have trouble imaging what kind of corner-case could result from
not allowing a role to assume ownership of a role it is a member of.  While
we do have the capability to required SET ROLE it is optional: any code
paths dealing with grants have to assume that the current role receives
permission via inheritance - and all we are doing here is ensuring that is
the situation.

It now occurs to me that perhaps we can improve on this situation in the
> future by formally supporting a role attribute that is "do not allow SET
> ROLE to this user" and then changing the default roles to have that
> attribute set (and disallowing users from changing it).  That's just
> additional flexibility over what the current patch does, however, but
> perhaps it helps illustrate that there are cases where only the
> privileges of the role are intended to be GRANT'd and not the ability to
> SET ROLE to the role or to create objects as that role.
>

​That is where my first response was heading but it was definitely scope
creep so I didn't bring it up.  Basically, an "INHERITONLY" ​modifier in
addition to INHERIT and NOINHERIT.

I had stated the having a role that neither provides inheritance nor
changing-to is pointless but I am now unsure if that is true.  It would
depend upon whether a LOGIN role is one that is considered having been SET
ROLE to or not.  If not, then a "LOGINONLY" combination would work such
that a role with that combination would only have whatever grants are given
to it and is not allowed to be changed to by a different role - i.e., it
could only be used by someone logging into the system specifically as that
role.  It likewise complements "NOLOGIN + LOGIN".

It wouldn't directly preclude said role from itself SET ROLE'ing to a
different role though.​

And, for all that, I do realize I'm using these terms somewhat contrary to
reality since INHERIT is done on the receiver and not the giver.  The
wording would have to change to reflect the POV of the giver.  That is,
INHERITONLY should be something done to a role that prevents it from being
SET TO as opposed to NOINHERIT which forces a role to use SET TO to access
the permissions of all roles in which it is a member.

​David J.
​

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Wed, Apr 13, 2016 at 6:53 PM, Stephen Frost  wrote:
> > Requiring that SET ROLE be allowed will mean that many more paths must
> > be checked and adjusted, such as in all of the CreateObject statements
> > and potentionally many other paths that I'm not thinking of here, not
> > the least of which are all of the *existing* checks near
> > get_rolespec_oid/tuple which will require additional checks for the
> > CURRENT_USER case to see if the current user is a default role.
> 
> I don't get it.  I think that these new roles should work just like
> any other roles, except for existing at initdb time.  I don't see why
> they would require checking or adjusting any code-paths at all.  It
> would presumably have made the patch you committed smaller and
> simpler.  The only thing you'd need to do is (approximately) not dump
> them.

Perhaps it would be helpful to return to the initial goal of these
default roles.

We've identified certain privileges which are currently superuser-only
and we would like to be able to be GRANT those to non-superuser roles.
Where our GRANT system is able to provide the granularity desired, we
have simply removed the superuser checks, set up appropriate default
values and, through the "pg_dump dump catalog ACLs" patch, allowed
administrators to make the changes to those objects via the
'GRANT privilege ON object TO role' system.

For those cases where our GRANT system is unable to provide the
granularity desired and it isn't sensible to extend the GRANT system to
cover the exact granularity we wish to provide, we needed to come up
with an alternative approach.  That approach is the use of special
default roles, where we can allow exactly the level of granularity
desired and give administrators a way to grant those privileges to
users.

What this means in a nutshell is that we've collectivly decided that
we'd rather support:

/* uses the 'GRANT role TO role' system */
GRANT pg_signal_backend TO test_user;

than:

/*
 * uses the 'GRANT privilege ON object TO role' system
 * seems like cluster-level is actually the right answer here, but
 * we don't support such an object type currently, so using database
 * for the example
 */
GRANT SIGNAL BACKEND ON DATABASE my_database TO test_user;

The goal being that the result of the two commands is identical (where
the second command is only hypothetical at this point, but hopefully the
meaning is conveyed).

However, GRANT'ing a role to a user traditionally also allows the user
to 'SET ROLE' to that user, to create objects as that user, and other
privileges.  This results in two issues, as I see it:

1) The administrator who is looking to grant the specific 'signal
   backend' privilege to a user is unlikely to intend or desire for that
   user to also be able to SET ROLE to the role, or for that user to be
   able to create objects as the default role.

2) If the default role ends up owning objects then we have much less
   flexibility in making changes to that role because we must ensure
   that those objects are preserved across upgrades, etc.

Further, there seems to be no particular use-case which is satisfied by
allowing SET ROLE'ing to the default roles or for those roles to own
objects; indeed, it seems more likely to simply spark confusion and
ultimately may prevent administrators from making use of this system for
granting privileges as they are unable to GRANT just the specific
privilege they wish to.

It now occurs to me that perhaps we can improve on this situation in the
future by formally supporting a role attribute that is "do not allow SET
ROLE to this user" and then changing the default roles to have that
attribute set (and disallowing users from changing it).  That's just
additional flexibility over what the current patch does, however, but
perhaps it helps illustrate that there are cases where only the
privileges of the role are intended to be GRANT'd and not the ability to
SET ROLE to the role or to create objects as that role.

I hope that helps.  I'll be in NY next week also, so perhaps that would
be a good opportunity to have an interactive discussion on this topic.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Wed, Apr 13, 2016 at 6:53 PM, Stephen Frost  wrote:
> Requiring that SET ROLE be allowed will mean that many more paths must
> be checked and adjusted, such as in all of the CreateObject statements
> and potentionally many other paths that I'm not thinking of here, not
> the least of which are all of the *existing* checks near
> get_rolespec_oid/tuple which will require additional checks for the
> CURRENT_USER case to see if the current user is a default role.

I don't get it.  I think that these new roles should work just like
any other roles, except for existing at initdb time.  I don't see why
they would require checking or adjusting any code-paths at all.  It
would presumably have made the patch you committed smaller and
simpler.  The only thing you'd need to do is (approximately) not dump
them.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> On 2016/04/14 2:10, Stephen Frost wrote:
> >> Amit Langote > writes:
> >>> I observe this:
> >>
> >>> postgres=# SET ROLE TO NONE;
> >>> SET
> >>> postgres=# SET ROLE TO nonexistent;
> >>> ERROR:  role "nonexistent" does not exist
> >>> postgres=# SET ROLE TO pg_signal_backend;
> >>> ERROR:  invalid value for parameter "role": "pg_signal_backend"
> >>
> >>> Is that behavior deliberate? Might it be better to handle the case
> >>> specially much as setting to "none" works?
> > 
> > I don't think it makes sense to say the role doesn't exist when it does, in
> > fact, exist.
> 
> Sorry, I didn't mean to say that we should error with "
> does not exist" on such SET ROLE attempts.  Like Michael, I was a bit
> surprised to find that it output "invalid value for parameter".

No problem.  I realized that when I read your email, but when I was
added to the CC (which results in the email showing up on my phone,
where I had been responding from earlier), I only saw the subset which
was quoted.  Your comments make more sense now that I've caught up on
-hackers email.

> So, if consensus emerges that we should indeed disallow SET ROLE
> , I would +1 Michael's proposed GUC_check_err*()-based
> patch.

I've not looked it over carefully, but I generally agree with improving
the error messages.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] SET ROLE and reserved roles

[Amit Langote]

Hi Stephen,

On 2016/04/14 2:10, Stephen Frost wrote:
>> Amit Langote > writes:
>>> I observe this:
>>
>>> postgres=# SET ROLE TO NONE;
>>> SET
>>> postgres=# SET ROLE TO nonexistent;
>>> ERROR:  role "nonexistent" does not exist
>>> postgres=# SET ROLE TO pg_signal_backend;
>>> ERROR:  invalid value for parameter "role": "pg_signal_backend"
>>
>>> Is that behavior deliberate? Might it be better to handle the case
>>> specially much as setting to "none" works?
> 
> I don't think it makes sense to say the role doesn't exist when it does, in
> fact, exist.

Sorry, I didn't mean to say that we should error with "
does not exist" on such SET ROLE attempts.  Like Michael, I was a bit
surprised to find that it output "invalid value for parameter".

So, if consensus emerges that we should indeed disallow SET ROLE
, I would +1 Michael's proposed GUC_check_err*()-based
patch.

Thanks,
Amit




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

* David G. Johnston (david.g.johns...@gmail.com) wrote:
> From what I've read here I'm thinking Stephen has the right idea.

Thanks.  Additionally, your comments make me realize an existing issue,
which is superuser-only but I'll address shortly anyway (we have far too
many users running around as superuser)- SET SESSION AUTHORIZATION.

> Lets be conservative in what we allow with these new roles and let
> experience guide us as to whether we need to open things up more - or just
> fix oversights.

Agreed.

I would further point out that allowing users to SET ROLE to a system
role means they can "give away" objects to that role, which is quite
unlikely what an administrator intended to allow.

Consider the 'pg_signal_backend' role, in particular.  You may wish to
give that to your test users, who are running crazy tests and who need
to be able to cancel crazy backend queries that get kicked off due to
their crazy testing.  Those users shouldn't be allowed to give away
objects they create to a system role, yet that's difficult to prevent,
if we allow users to SET ROLE to system roles.  I don't think that most
admins would really want users to be able to SET ROLE to the system
users they've been granted.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] SET ROLE and reserved roles

[David G. Johnston]

On Wed, Apr 13, 2016 at 3:53 PM, Stephen Frost  wrote:

> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> > Stephen Frost  writes:
> > > On Wednesday, April 13, 2016, Tom Lane  wrote:
> > >> If you want to prevent that, I think it needs to be done somewhere
> else
> > >> than here.  What about "ALTER OWNER TO pg_signal_backend", for
> instance?
> >
> > > Checks are included in that code path to disallow it.
> >
> > If there are such low-level checks, why do we need to disallow SET ROLE?
> > It seems like the wrong place to be inserting such defenses.
>
> The low-level checks were placed all along the paths which used
> get_rolespace_oid/tuple.  SET ROLE is the only place where a user could
> change to another role and then do things which result in objects being
> owned by that role without going through one of those paths.
>
> Requiring that SET ROLE be allowed will mean that many more paths must
> be checked and adjusted, such as in all of the CreateObject statements
> and potentionally many other paths that I'm not thinking of here, not
> the least of which are all of the *existing* checks near
> get_rolespec_oid/tuple which will require additional checks for the
> CURRENT_USER case to see if the current user is a default role.
>
> > >> But perhaps more to the point, why is it a strange corner case for one
> > >> of these roles to own objects?
> >
> > > If the default roles can own objects and otherwise be used for other
> > > purposes then we can never, ever, be rid of any which we create.
> >
> > This argument seems quite bogus to me, because granted privileges are
> > pretty darn close to being objects.  Certainly, if you have some
> > "GRANT pg_signal_backend TO joe_user" commands in a pg_dump script,
> > those will fail for lack of the role just as surely as ALTER OWNER
> > commands would.  So we would need some kind of special hack in pg_dump
> > either way, unless we make it incumbent on users to clean up before
> > upgrading (which doesn't seem out of the question to me ...)
>
> I don't think we would have any concerns with a hack in pg_dump to
> remove those GRANTs if that default role goes away.  There's certainly
> no way we're going to do that for tables which have data in them,
> however.  Therefore, the user would have to address any such cases
> before being able to an upgrade, and I don't see why we want to allow
> such a case.
>
> As for present-vs-future cruft, we can't put this genie back in the
> bottle once we allow a default role to own objects, which is why I felt
> it was prudent to address that concern from the get-go.
>
> > I think you're replacing hypothetical future cruft with actual present
> > cruft, and it doesn't seem like a very good tradeoff.
>
> If we don't care about default roles being able to own objects, then we
> can remove the check in SET ROLE and simply accept that we can never
> remove those roles without user intervention.  There's a number of other
> checks in the tree which we can debate (should a default role be allowed
> to have a USER MAPPING?  What about additional privileges beyond those
> we define for it?  Perhaps DEFAULT privileges?).  If we're going to
> allow default roles to own objects, it seems like we could just about
> remove all those other checks also.
>
> If we don't want default roles to be able to own objects, but we do want
> users to be able to SET ROLE to them, then there's a whole slew of
> *additional* checks which have to be added, which is surely a
> net-increase in code.
>
> I'm happy to do my best to implement the concensus opinion, just wanted
> to be clear on what the options are.  If I could get responses regarding
> everyone's preferences on the above, then we can get to what the
> desired behavior should be and I'll implement it.
>

>From what I've read here I'm thinking Stephen has the right idea.

Lets be conservative in what we allow with these new roles​ and let
experience guide us as to whether we need to open things up more - or just
fix oversights.

We have chosen to give up "defense in depth" here since if by some other
means the value of current_user gets set to one of these roles having the
sole protection point at SET ROLE will seem like a bad choice.  Aside from
that it will allow for fewer changes for code that chooses to rely on that
assertion instead of ensuring that it is true.

If we are wrong the obvious work-around is that all roles that would be
granted a given pg_* role would instead be granted a normal role which
itself would be granted the pg_* role.  Any of those roles would be then be
able emulate SET ROLE pg_* by instead switching to the normal role.

I don't think we'd be inclined to make these login roles so most external
tools are likely going to simply rely on the fact that whatever user they
are told to login with already has the necessary grants setup.

Stephen's entire response is enough for me to want to require an
justification for why 

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > On Wednesday, April 13, 2016, Tom Lane  wrote:
> >> If you want to prevent that, I think it needs to be done somewhere else
> >> than here.  What about "ALTER OWNER TO pg_signal_backend", for instance?
> 
> > Checks are included in that code path to disallow it.
> 
> If there are such low-level checks, why do we need to disallow SET ROLE?
> It seems like the wrong place to be inserting such defenses.

The low-level checks were placed all along the paths which used
get_rolespace_oid/tuple.  SET ROLE is the only place where a user could
change to another role and then do things which result in objects being
owned by that role without going through one of those paths.

Requiring that SET ROLE be allowed will mean that many more paths must
be checked and adjusted, such as in all of the CreateObject statements
and potentionally many other paths that I'm not thinking of here, not
the least of which are all of the *existing* checks near
get_rolespec_oid/tuple which will require additional checks for the
CURRENT_USER case to see if the current user is a default role.

> >> But perhaps more to the point, why is it a strange corner case for one
> >> of these roles to own objects?
> 
> > If the default roles can own objects and otherwise be used for other
> > purposes then we can never, ever, be rid of any which we create.
> 
> This argument seems quite bogus to me, because granted privileges are
> pretty darn close to being objects.  Certainly, if you have some
> "GRANT pg_signal_backend TO joe_user" commands in a pg_dump script,
> those will fail for lack of the role just as surely as ALTER OWNER
> commands would.  So we would need some kind of special hack in pg_dump
> either way, unless we make it incumbent on users to clean up before
> upgrading (which doesn't seem out of the question to me ...)

I don't think we would have any concerns with a hack in pg_dump to
remove those GRANTs if that default role goes away.  There's certainly
no way we're going to do that for tables which have data in them,
however.  Therefore, the user would have to address any such cases
before being able to an upgrade, and I don't see why we want to allow
such a case.

As for present-vs-future cruft, we can't put this genie back in the
bottle once we allow a default role to own objects, which is why I felt
it was prudent to address that concern from the get-go.

> I think you're replacing hypothetical future cruft with actual present
> cruft, and it doesn't seem like a very good tradeoff.

If we don't care about default roles being able to own objects, then we
can remove the check in SET ROLE and simply accept that we can never
remove those roles without user intervention.  There's a number of other
checks in the tree which we can debate (should a default role be allowed
to have a USER MAPPING?  What about additional privileges beyond those
we define for it?  Perhaps DEFAULT privileges?).  If we're going to
allow default roles to own objects, it seems like we could just about
remove all those other checks also.

If we don't want default roles to be able to own objects, but we do want
users to be able to SET ROLE to them, then there's a whole slew of
*additional* checks which have to be added, which is surely a
net-increase in code.

I'm happy to do my best to implement the concensus opinion, just wanted
to be clear on what the options are.  If I could get responses regarding
everyone's preferences on the above, then we can get to what the
desired behavior should be and I'll implement it.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Wed, Apr 13, 2016 at 2:10 PM, Tom Lane  wrote:
> Stephen Frost  writes:
>> On Wednesday, April 13, 2016, Tom Lane  wrote:
>>> If you want to prevent that, I think it needs to be done somewhere else
>>> than here.  What about "ALTER OWNER TO pg_signal_backend", for instance?
>
>> Checks are included in that code path to disallow it.
>
> If there are such low-level checks, why do we need to disallow SET ROLE?
> It seems like the wrong place to be inserting such defenses.
>
>
>>> But perhaps more to the point, why is it a strange corner case for one
>>> of these roles to own objects?
>
>> If the default roles can own objects and otherwise be used for other
>> purposes then we can never, ever, be rid of any which we create.
>
> This argument seems quite bogus to me, because granted privileges are
> pretty darn close to being objects.  Certainly, if you have some
> "GRANT pg_signal_backend TO joe_user" commands in a pg_dump script,
> those will fail for lack of the role just as surely as ALTER OWNER
> commands would.  So we would need some kind of special hack in pg_dump
> either way, unless we make it incumbent on users to clean up before
> upgrading (which doesn't seem out of the question to me ...)
>
> I think you're replacing hypothetical future cruft with actual present
> cruft, and it doesn't seem like a very good tradeoff.

That's my feeling as well.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Tom Lane]

Stephen Frost  writes:
> On Wednesday, April 13, 2016, Tom Lane  wrote:
>> If you want to prevent that, I think it needs to be done somewhere else
>> than here.  What about "ALTER OWNER TO pg_signal_backend", for instance?

> Checks are included in that code path to disallow it.

If there are such low-level checks, why do we need to disallow SET ROLE?
It seems like the wrong place to be inserting such defenses.


>> But perhaps more to the point, why is it a strange corner case for one
>> of these roles to own objects?

> If the default roles can own objects and otherwise be used for other
> purposes then we can never, ever, be rid of any which we create.

This argument seems quite bogus to me, because granted privileges are
pretty darn close to being objects.  Certainly, if you have some
"GRANT pg_signal_backend TO joe_user" commands in a pg_dump script,
those will fail for lack of the role just as surely as ALTER OWNER
commands would.  So we would need some kind of special hack in pg_dump
either way, unless we make it incumbent on users to clean up before
upgrading (which doesn't seem out of the question to me ...)

I think you're replacing hypothetical future cruft with actual present
cruft, and it doesn't seem like a very good tradeoff.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

Tom,

On Wednesday, April 13, 2016, Tom Lane  wrote:

> Stephen Frost > writes:
> > On Wednesday, April 13, 2016, Robert Haas  > wrote:
> >> Well ... yeah.  But that doesn't mean it should be impossible to SET
> >> to that role itself.  I'm a little worried that could create strange
> >> corner cases.
>
> > Being able to create objects owned by a default role was one of those
> > strange corner cases I was trying to avoid.
>
> If you want to prevent that, I think it needs to be done somewhere else
> than here.  What about "ALTER OWNER TO pg_signal_backend", for instance?


Checks are included in that code path to disallow it.


> But perhaps more to the point, why is it a strange corner case for one
> of these roles to own objects?  Isn't it *more* of a strange corner case
> to try to prohibit it?  Certainly the bootstrap superuser owns lots of
> objects, and I don't see why these roles can't.
>

Specifically because these are purpose built roles for managing access to
privileges which previously were only available to a superuser.

As an example, what if we decide that rights for a given capability, such
as "signal backend" really make sense as grant-able on a per-database
basis, and we work out the issues with the limited number of bits we
currently have, and some future version of PG has support for "GRANT SIGNAL
BACKEND TO user1 ON DATABASE db1;". We would want to transition the
existing users of pg_signal_backend to that system and then drop the
pg_signal_backend default role. I'm not looking to go in such a direction,
but it could certainly happen. Another case would be if we add a capability
and a default role to manage access to it and then later remove that entire
capability, what then?

If the default roles can own objects and otherwise be used for other
purposes then we can never, ever, be rid of any which we create.  The
argument you use for the bootstrap user isn't the same as it will
absolutely always be required to exist, so allowing a user to do what they
wish with it is perfectly fine.

The default roles are for the system to provide and manage, with admins
using them to grant access to other users, not for general usage, as
designed.

Thanks!

Stephen

Re: [HACKERS] SET ROLE and reserved roles

[Tom Lane]

Stephen Frost  writes:
> On Wednesday, April 13, 2016, Robert Haas  wrote:
>> Well ... yeah.  But that doesn't mean it should be impossible to SET
>> to that role itself.  I'm a little worried that could create strange
>> corner cases.

> Being able to create objects owned by a default role was one of those
> strange corner cases I was trying to avoid.

If you want to prevent that, I think it needs to be done somewhere else
than here.  What about "ALTER OWNER TO pg_signal_backend", for instance?

But perhaps more to the point, why is it a strange corner case for one
of these roles to own objects?  Isn't it *more* of a strange corner case
to try to prohibit it?  Certainly the bootstrap superuser owns lots of
objects, and I don't see why these roles can't.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Wed, Apr 13, 2016 at 1:24 PM, Stephen Frost  wrote:
> On Wednesday, April 13, 2016, Robert Haas  wrote:
>>
>> On Wed, Apr 13, 2016 at 1:10 PM, Stephen Frost  wrote:
>> >> What I'd like to know is why it rejects that at all.  What's the point
>> >> of having roles you can't SET to?
>> >
>> > To use them to GRANT access to other roles, which was the goal of the
>> > default roles system to begin with.
>>
>> Well ... yeah.  But that doesn't mean it should be impossible to SET
>> to that role itself.  I'm a little worried that could create strange
>> corner cases.
>
> Being able to create objects owned by a default role was one of those
> strange corner cases I was trying to avoid.
>
> What's the use-case for setting to the role..?  I would generally argue that
> it's actually to create objects as that role, which is something I believe
> we specifically do not want for default roles, and in some limited cases to
> drop or gain additional privileges, when using noinherit roles (which are
> not the default).  The latter can still be accomplished, of course, by
> creating a role which is noinherit and using that.

I don't know that there is a use case for it, but it seems like a
weird inconsistency.  It may be fine; it just seems odd.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

On Wednesday, April 13, 2016, Robert Haas  wrote:

> On Wed, Apr 13, 2016 at 1:10 PM, Stephen Frost  > wrote:
> >> What I'd like to know is why it rejects that at all.  What's the point
> >> of having roles you can't SET to?
> >
> > To use them to GRANT access to other roles, which was the goal of the
> > default roles system to begin with.
>
> Well ... yeah.  But that doesn't mean it should be impossible to SET
> to that role itself.  I'm a little worried that could create strange
> corner cases.
>

Being able to create objects owned by a default role was one of those
strange corner cases I was trying to avoid.

What's the use-case for setting to the role..?  I would generally argue
that it's actually to create objects as that role, which is something I
believe we specifically do not want for default roles, and in some limited
cases to drop or gain additional privileges, when using noinherit roles
(which are not the default).  The latter can still be accomplished, of
course, by creating a role which is noinherit and using that.

Thanks!

Stephen

Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Wed, Apr 13, 2016 at 1:10 PM, Stephen Frost  wrote:
>> What I'd like to know is why it rejects that at all.  What's the point
>> of having roles you can't SET to?
>
> To use them to GRANT access to other roles, which was the goal of the
> default roles system to begin with.

Well ... yeah.  But that doesn't mean it should be impossible to SET
to that role itself.  I'm a little worried that could create strange
corner cases.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Stephen Frost]

Tom, all,

On Wednesday, April 13, 2016, Tom Lane  wrote:

> Amit Langote > writes:
> > I observe this:
>
> > postgres=# SET ROLE TO NONE;
> > SET
> > postgres=# SET ROLE TO nonexistent;
> > ERROR:  role "nonexistent" does not exist
> > postgres=# SET ROLE TO pg_signal_backend;
> > ERROR:  invalid value for parameter "role": "pg_signal_backend"
>
> > Is that behavior deliberate? Might it be better to handle the case
> > specially much as setting to "none" works?


I don't think it makes sense to say the role doesn't exist when it does, in
fact, exist.


> What I'd like to know is why it rejects that at all.  What's the point
> of having roles you can't SET to?
>

To use them to GRANT access to other roles, which was the goal of the
default roles system to begin with.

GRANT pg_signal_backend TO user1;

User1 can then send (certain) signals to other backends which it isn't a
role member of.

That also avoids the issue of a default role ending up owning objects,
which I don't think is desirable.

Thanks!

Stephen

Re: [HACKERS] SET ROLE and reserved roles

[Tom Lane]

Amit Langote  writes:
> I observe this:

> postgres=# SET ROLE TO NONE;
> SET
> postgres=# SET ROLE TO nonexistent;
> ERROR:  role "nonexistent" does not exist
> postgres=# SET ROLE TO pg_signal_backend;
> ERROR:  invalid value for parameter "role": "pg_signal_backend"

> Is that behavior deliberate? Might it be better to handle the case
> specially much as setting to "none" works?

What I'd like to know is why it rejects that at all.  What's the point
of having roles you can't SET to?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Wed, Apr 13, 2016 at 8:49 AM, Michael Paquier
 wrote:
> On Wed, Apr 13, 2016 at 5:58 PM, Amit Langote
>  wrote:
>> Is that behavior deliberate? Might it be better to handle the case
>> specially much as setting to "none" works?  Such as:
>>
>> ERROR: cannot set to reserved role "pg_signal_backend"
>>
>> Sorry if I have missed any discussion where such a choice was deliberately
>> made.
>
> I agree that this is a bit surprising. We could do something like the
> attached, and switch the error code to ERRCODE_RESERVED_NAME as well
> without caring much if a system role exists or not, this does not seem
> worth doing a catalog lookup:
> =# set role to pg_test;
> ERROR:  42939: role "pg_test" is reserved
> LOCATION:  call_string_check_hook, guc.c:9788
> =# set role to pg_signal_backend;
> ERROR:  42939: role "pg_signal_backend" is reserved
> LOCATION:  call_string_check_hook, guc.c:9788

But is it even intended behavior that you can't set to these reserved roles?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] SET ROLE and reserved roles

[Michael Paquier]

On Wed, Apr 13, 2016 at 5:58 PM, Amit Langote
 wrote:
> Is that behavior deliberate? Might it be better to handle the case
> specially much as setting to "none" works?  Such as:
>
> ERROR: cannot set to reserved role "pg_signal_backend"
>
> Sorry if I have missed any discussion where such a choice was deliberately
> made.

I agree that this is a bit surprising. We could do something like the
attached, and switch the error code to ERRCODE_RESERVED_NAME as well
without caring much if a system role exists or not, this does not seem
worth doing a catalog lookup:
=# set role to pg_test;
ERROR:  42939: role "pg_test" is reserved
LOCATION:  call_string_check_hook, guc.c:9788
=# set role to pg_signal_backend;
ERROR:  42939: role "pg_signal_backend" is reserved
LOCATION:  call_string_check_hook, guc.c:9788
-- 
Michael
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 57da014..7772cbe 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -856,7 +856,11 @@ check_role(char **newval, void **extra, GucSource source)
 	}
 	/* Do not allow setting role to a reserved role. */
 	else if (strncmp(*newval, "pg_", 3) == 0)
+	{
+		GUC_check_errcode(ERRCODE_RESERVED_NAME);
+		GUC_check_errmsg("role \"%s\" is reserved", *newval);
 		return false;
+	}
 	else
 	{
 		if (!IsTransactionState())
diff --git a/src/test/regress/expected/rolenames.out b/src/test/regress/expected/rolenames.out
index 15a97ab..12d1e76 100644
--- a/src/test/regress/expected/rolenames.out
+++ b/src/test/regress/expected/rolenames.out
@@ -823,9 +823,9 @@ GRANT pg_abc TO pg_abcdef; -- error
 ERROR:  role "pg_abcdef" is reserved
 DETAIL:  Cannot GRANT roles to a reserved role.
 SET ROLE pg_testrole; -- error
-ERROR:  invalid value for parameter "role": "pg_testrole"
+ERROR:  role "pg_testrole" is reserved
 SET ROLE pg_signal_backend; --error
-ERROR:  invalid value for parameter "role": "pg_signal_backend"
+ERROR:  role "pg_signal_backend" is reserved
 CREATE SCHEMA test_schema AUTHORIZATION pg_signal_backend; --error
 ERROR:  role "pg_signal_backend" is reserved
 DETAIL:  Cannot specify reserved role as owner.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] SET ROLE and reserved roles

[Amit Langote]

Hi,

I observe this:

postgres=# SET ROLE TO NONE;
SET

postgres=# SET ROLE TO nonexistent;
ERROR:  role "nonexistent" does not exist

postgres=# SET ROLE TO pg_signal_backend;
ERROR:  invalid value for parameter "role": "pg_signal_backend"

Is that behavior deliberate? Might it be better to handle the case
specially much as setting to "none" works?  Such as:

ERROR: cannot set to reserved role "pg_signal_backend"

Sorry if I have missed any discussion where such a choice was deliberately
made.

Thanks,
Amit





-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers