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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
set-system-role.patchtext/x-diff; charset=US-ASCII; name=set-system-role.patchDownload
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.
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom, all,
On Wednesday, April 13, 2016, Tom Lane <tgl@sss.pgh.pa.us> 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.
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
<javascript:;>> 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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom,
On Wednesday, April 13, 2016, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Stephen Frost <sfrost@snowman.net <javascript:;>> writes:
On Wednesday, April 13, 2016, Robert Haas <robertmhaas@gmail.com
<javascript:;>> 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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* 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
On Wed, Apr 13, 2016 at 3:53 PM, Stephen Frost <sfrost@snowman.net> wrote:
* 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 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
On Fri, Apr 15, 2016 at 8:56 AM, Stephen Frost <sfrost@snowman.net> wrote:
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* 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
Attachments:
remove_default_role_checks_v1.patchtext/x-diff; charset=us-asciiDownload
From 4fae6e77eddc86360381e44f35d4da4a495cbdc1 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
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 default privileges to or from a reserved role.");
grantee_uid = get_rolespec_oid((Node *) grantee, false);
break;
}
@@ -1013,8 +1008,6 @@ ExecAlterDefaultPrivilegesStmt(AlterDefaultPrivilegesStmt *stmt)
{
RoleSpec *rolespec = lfirst(rolecell);
- check_rolespec_name((Node *) rolespec,
- "Cannot alter default privileges for reserved role.");
iacls.roleid = get_rolespec_oid((Node *) rolespec, false);
/*
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 47a5c50..4b08cb8 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -747,9 +747,6 @@ ExecAlterOwnerStmt(AlterOwnerStmt *stmt)
{
Oid newowner = get_rolespec_oid(stmt->newowner, false);
- check_rolespec_name(stmt->newowner,
- "Cannot make reserved roles owners of objects.");
-
switch (stmt->objectType)
{
case OBJECT_DATABASE:
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index 88cefb7..804bab2 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -1148,10 +1148,6 @@ CreateUserMapping(CreateUserMappingStmt *stmt)
else
useId = get_rolespec_oid(stmt->user, false);
- /* Additional check to protect reserved role names */
- check_rolespec_name(stmt->user,
- "Cannot specify reserved role as mapping user.");
-
/* Check that the server exists. */
srv = GetForeignServerByName(stmt->servername, false);
@@ -1252,10 +1248,6 @@ AlterUserMapping(AlterUserMappingStmt *stmt)
else
useId = get_rolespec_oid(stmt->user, false);
- /* Additional check to protect reserved role names */
- check_rolespec_name(stmt->user,
- "Cannot alter reserved role mapping user.");
-
srv = GetForeignServerByName(stmt->servername, false);
umId = GetSysCacheOid2(USERMAPPINGUSERSERVER,
@@ -1345,11 +1337,6 @@ RemoveUserMapping(DropUserMappingStmt *stmt)
else
{
useId = get_rolespec_oid(stmt->user, stmt->missing_ok);
-
- /* Additional check to protect reserved role names */
- check_rolespec_name(stmt->user,
- "Cannot remove reserved role mapping user.");
-
if (!OidIsValid(useId))
{
/*
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 146b36c..93d15e4 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -176,13 +176,8 @@ policy_role_list_to_array(List *roles, int *num_roles)
return role_oids;
}
else
- {
- /* Additional check to protect reserved role names */
- check_rolespec_name((Node *) spec,
- "Cannot specify reserved role as policy target");
role_oids[i++] =
ObjectIdGetDatum(get_rolespec_oid((Node *) spec, false));
- }
}
return role_oids;
diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c
index dea3299..a60ceb8 100644
--- a/src/backend/commands/schemacmds.c
+++ b/src/backend/commands/schemacmds.c
@@ -65,10 +65,6 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString)
else
owner_uid = saved_uid;
- /* Additional check to protect reserved role names */
- check_rolespec_name(stmt->authrole,
- "Cannot specify reserved role as owner.");
-
/* fill schema name with the user name if not specified */
if (!schemaName)
{
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 45a5144..86e9814 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3566,8 +3566,6 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
(List *) cmd->def, lockmode);
break;
case AT_ChangeOwner: /* ALTER OWNER */
- check_rolespec_name(cmd->newowner,
- "Cannot specify reserved role as owner.");
ATExecChangeOwner(RelationGetRelid(rel),
get_rolespec_oid(cmd->newowner, false),
false, lockmode);
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index fe7f253..7902d43 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -256,10 +256,6 @@ CreateTableSpace(CreateTableSpaceStmt *stmt)
else
ownerId = GetUserId();
- /* Additional check to protect reserved role names */
- check_rolespec_name(stmt->owner,
- "Cannot specify reserved role as owner.");
-
/* Unix-ify the offered path, and strip any trailing slashes */
location = pstrdup(stmt->location);
canonicalize_path(location);
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index cc3d564..f0ac636 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -1262,18 +1262,10 @@ GrantRole(GrantRoleStmt *stmt)
ListCell *item;
if (stmt->grantor)
- {
- check_rolespec_name(stmt->grantor,
- "Cannot specify reserved role as grantor.");
grantor = get_rolespec_oid(stmt->grantor, false);
- }
else
grantor = GetUserId();
- foreach(item, stmt->grantee_roles)
- check_rolespec_name(lfirst(item),
- "Cannot GRANT roles to a reserved role.");
-
grantee_ids = roleSpecsToIds(stmt->grantee_roles);
/* AccessShareLock is enough since we aren't modifying pg_authid */
@@ -1364,9 +1356,6 @@ ReassignOwnedObjects(ReassignOwnedStmt *stmt)
errmsg("permission denied to reassign objects")));
}
- check_rolespec_name(stmt->newrole,
- "Cannot specify reserved role as owner.");
-
/* Must have privileges on the receiving side too */
newrole = get_rolespec_oid(stmt->newrole, false);
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 05e59a6..f801faa 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -794,10 +794,6 @@ check_session_authorization(char **newval, void **extra, GucSource source)
return false;
}
- /* Do not allow setting role to a reserved role. */
- if (strncmp(*newval, "pg_", 3) == 0)
- return false;
-
/* Look up the username */
roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
if (!HeapTupleIsValid(roleTup))
@@ -858,9 +854,6 @@ check_role(char **newval, void **extra, GucSource source)
roleid = InvalidOid;
is_superuser = false;
}
- /* Do not allow setting role to a reserved role. */
- else if (strncmp(*newval, "pg_", 3) == 0)
- return false;
else
{
if (!IsTransactionState())
diff --git a/src/test/regress/expected/rolenames.out b/src/test/regress/expected/rolenames.out
index 15a97ab..a1f0394 100644
--- a/src/test/regress/expected/rolenames.out
+++ b/src/test/regress/expected/rolenames.out
@@ -816,19 +816,11 @@ LINE 1: DROP USER MAPPING IF EXISTS FOR CURRENT_ROLE SERVER sv9;
DROP USER MAPPING IF EXISTS FOR nonexistent SERVER sv9; -- error
NOTICE: role "nonexistent" does not exist, skipping
-- GRANT/REVOKE
-GRANT testrol0 TO pg_abc; -- error
-ERROR: role "pg_abc" is reserved
-DETAIL: Cannot GRANT roles to a reserved role.
-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"
-SET ROLE pg_signal_backend; --error
-ERROR: invalid value for parameter "role": "pg_signal_backend"
-CREATE SCHEMA test_schema AUTHORIZATION pg_signal_backend; --error
-ERROR: role "pg_signal_backend" is reserved
-DETAIL: Cannot specify reserved role as owner.
+GRANT testrol0 TO pg_signal_backend; -- success
+SET ROLE pg_signal_backend; --success
+RESET ROLE;
+CREATE SCHEMA test_schema AUTHORIZATION pg_signal_backend; --success
+SET ROLE testrol2;
UPDATE pg_proc SET proacl = null WHERE proname LIKE 'testagg_';
SELECT proname, proacl FROM pg_proc WHERE proname LIKE 'testagg_';
proname | proacl
diff --git a/src/test/regress/sql/rolenames.sql b/src/test/regress/sql/rolenames.sql
index b58a163..6c831b8 100644
--- a/src/test/regress/sql/rolenames.sql
+++ b/src/test/regress/sql/rolenames.sql
@@ -381,12 +381,12 @@ DROP USER MAPPING IF EXISTS FOR CURRENT_ROLE SERVER sv9; --error
DROP USER MAPPING IF EXISTS FOR nonexistent SERVER sv9; -- error
-- GRANT/REVOKE
-GRANT testrol0 TO pg_abc; -- error
-GRANT pg_abc TO pg_abcdef; -- error
+GRANT testrol0 TO pg_signal_backend; -- success
-SET ROLE pg_testrole; -- error
-SET ROLE pg_signal_backend; --error
-CREATE SCHEMA test_schema AUTHORIZATION pg_signal_backend; --error
+SET ROLE pg_signal_backend; --success
+RESET ROLE;
+CREATE SCHEMA test_schema AUTHORIZATION pg_signal_backend; --success
+SET ROLE testrol2;
UPDATE pg_proc SET proacl = null WHERE proname LIKE 'testagg_';
SELECT proname, proacl FROM pg_proc WHERE proname LIKE 'testagg_';
--
2.5.0
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert,
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
<javascript:;>> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers