Thread: SET ROLE and reserved roles
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
On Wed, Apr 13, 2016 at 5:58 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
Attachment
On Wed, Apr 13, 2016 at 8:49 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Wed, Apr 13, 2016 at 5:58 PM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> 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
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
Tom, all,
On Wednesday, April 13, 2016, Tom Lane <tgl@sss.pgh.pa.us> wrote:
On Wednesday, April 13, 2016, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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
On Wed, Apr 13, 2016 at 1:10 PM, Stephen Frost <sfrost@snowman.net> 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
On Wednesday, April 13, 2016, Robert Haas <robertmhaas@gmail.com> wrote:
Thanks!
On Wed, Apr 13, 2016 at 1:10 PM, Stephen Frost <sfrost@snowman.net> 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.
Stephen
On Wed, Apr 13, 2016 at 1:24 PM, Stephen Frost <sfrost@snowman.net> wrote: > On Wednesday, April 13, 2016, Robert Haas <robertmhaas@gmail.com> wrote: >> >> On Wed, Apr 13, 2016 at 1:10 PM, Stephen Frost <sfrost@snowman.net> 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
Stephen Frost <sfrost@snowman.net> writes:
> On Wednesday, April 13, 2016, Robert Haas <robertmhaas@gmail.com> 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
Tom,
On Wednesday, April 13, 2016, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Checks are included in that code path to disallow it.
On Wednesday, April 13, 2016, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Stephen Frost <sfrost@snowman.net> writes:
> On Wednesday, April 13, 2016, Robert Haas <robertmhaas@gmail.com> 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.
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
Stephen Frost <sfrost@snowman.net> writes:
> On Wednesday, April 13, 2016, Tom Lane <tgl@sss.pgh.pa.us> 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
On Wed, Apr 13, 2016 at 2:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Stephen Frost <sfrost@snowman.net> writes: >> On Wednesday, April 13, 2016, Tom Lane <tgl@sss.pgh.pa.us> 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
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > On Wednesday, April 13, 2016, Tom Lane <tgl@sss.pgh.pa.us> 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
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > On Wednesday, April 13, 2016, Tom Lane <tgl@sss.pgh.pa.us> 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 "INHERIT ONLY" (e.g., the third logical result after NOINHERIT & INHERIT(+SET); the fourth being a role that neither inherits nor can be set to - which would be pointless) should not be an acceptable combination of ROLE attributes.
Arguing that the bootstrap user has some combination of properties doesn't seem like a compelling line of argument. The whole point of this exercise is to institute more fine-grained authorizations and to provide built-in roles to access those built-in features. The bootstrap user still owns all of the system objects but delegates access and usage of them to these newly created roles. There isn't an internal need for the internal users to own things - we have the bootstrap user - and while its quite possible other people might devise a reason to utilize these roles in that way it doesn't seem like a necessary thing but rather likely done out of convenience. Call it parental involvement if you'd like but why allow situations to arise where these system roles are doing anything more than providing access to existing system objects thus increasign the number of unknowable dependencies that these system roles have when doing things like pg_upgrade?
I have not seen a strong argument for this need and while we might want to tighten up security by adding checks having the internals assume that the pg_* roles are never "current_role" should result in a more secure system and less code.
David J.
* David G. Johnston (david.g.johnston@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
Hi Stephen, On 2016/04/14 2:10, Stephen Frost wrote: >> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp <javascript:;>> 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 "<reserved-role> 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 <reserved-role-spec>, I would +1 Michael's proposed GUC_check_err*()-based patch. Thanks, Amit
Amit, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > On 2016/04/14 2:10, Stephen Frost wrote: > >> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp <javascript:;>> 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 "<reserved-role> > 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 > <reserved-role-spec>, 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
On Wed, Apr 13, 2016 at 6:53 PM, Stephen Frost <sfrost@snowman.net> 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
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Wed, Apr 13, 2016 at 6:53 PM, Stephen Frost <sfrost@snowman.net> 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 ordesire 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 defaultrole. 2) If the default role ends up owning objects then we have much less flexibility in making changes to that role becausewe 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
Robert,
* Robert Haas (robertmhaas@gmail.com) wrote:
> On Wed, Apr 13, 2016 at 6:53 PM, Stephen Frost <sfrost@snowman.net> 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.
On Fri, Apr 15, 2016 at 11:56 AM, Stephen Frost <sfrost@snowman.net> 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
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Fri, Apr 15, 2016 at 11:56 AM, Stephen Frost <sfrost@snowman.net> 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.GA486055@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. My concern, and the reason they were added is simply that it'll complicate things down the road. Thanks! Stephen
Robert, all, [... comments elsewhere made me realize I hadn't actually sent this when I thought I had, my apologies on that ...] * Robert Haas (robertmhaas@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
On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost <sfrost@snowman.net> 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
Robert Haas wrote: > On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost <sfrost@snowman.net> 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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Apr 26, 2016 at 7:39 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost <sfrost@snowman.net> 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
* Robert Haas (robertmhaas@gmail.com) wrote: > On Tue, Apr 26, 2016 at 7:39 PM, Robert Haas <robertmhaas@gmail.com> wrote: > > On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost <sfrost@snowman.net> 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
* Stephen Frost (sfrost@snowman.net) wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: > > On Tue, Apr 26, 2016 at 7:39 PM, Robert Haas <robertmhaas@gmail.com> wrote: > > > On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost <sfrost@snowman.net> 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
Attachment
On Thu, May 5, 2016 at 10:41 AM, Stephen Frost <sfrost@snowman.net> 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
Robert,
On Friday, May 6, 2016, Robert Haas <robertmhaas@gmail.com> wrote:
On Friday, May 6, 2016, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, May 5, 2016 at 10:41 AM, Stephen Frost <sfrost@snowman.net> 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
* Robert Haas (robertmhaas@gmail.com) wrote: > On Thu, May 5, 2016 at 10:41 AM, Stephen Frost <sfrost@snowman.net> 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
On Fri, May 6, 2016 at 12:12 PM, Stephen Frost <sfrost@snowman.net> 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