Re: CREATEROLE and role ownership hierarchies

On Mon, Feb 28, 2022 at 2:09 PM Stephen Frost <sfrost@snowman.net> wrote:

I'm generally in support of changing CREATEROLE to only allow roles that
the role with CREATEROLE is an admin of to be allowed as part of the
command (throwing an error in other cases). That doesn't solve other
use-cases which would certainly be nice to solve but it would at least
reduce the shock people have when they discover how CREATEROLE actually
works (that is, the way we document it to work, but that's ultimately
not what people expect).

So I'm 100% good with that because it does exactly what I want, but my
understanding of the situation is that it breaks the userbot case that
Joshua is talking about. Right now, with stock PostgreSQL in any
released version, his userbot can have CREATEROLE and give out roles
that it doesn't itself possess. If we restrict CREATEROLE in the way
described here, that's no longer possible.

Now it's possible that you and/or he would take the position that
we're still coming out ahead despite that functional regression,
because as I now understand after reading Joshua's latest email, he
doesn't want the userbot to be able to grant ANY role, just the
'employees' role - and today he can't get that. So in a modified
universe where we restrict the privileges of CREATEROLE, then on the
one hand he GAINS the ability to have a userbot that can grant some
roles but not others, but on the other hand, he's forced to give the
userbot the roles he wants it to be able to hand out. Is that better
overall or worse?

To really give him EXACTLY what he wants, we need a way of specifying
administration without membership. See my last reply to the thread for
my concerns about that.

I don't think it's a great plan to limit who is allowed to DROP roles to
be just those that a given role created. I also don't like the idea of
introducing a single field for owner/manager/tenant/whatever to the role
system- instead we should add other ways that roles can be associated to
each other by extending the existing system that we have for that, which
is role membership. Role membership today is pretty limited but I don't
see any reason why we couldn't improve on that in a way that's flexible
and allows us to define new associations in the future. The biggest
difference between a 'tenant' or such as proposed vs. a role association
is in where the information is tracked and what exactly it means.
Saying "I want a owner" or such is easy because it's basically punting
on the complciated bit of asking the question: what does that *mean*
when it comes to what rights that includes vs. doesn't? What if I only
want some of those rights to be given away but not all of them? We have
that system for tables/schemas/etc, and it hasn't been great as we've
seen through the various requests to add things like GRANT TRUNCATE.

Well, there's no accounting for taste, but I guess I see this pretty
much opposite to the way you do. I think GRANT TRUNCATE is nice and
simple and clear. It does one thing and it's easy to understand what
that thing is, and it has very few surprising or undocumented side
effects. On the other hand, role membership is a mess, and it's not at
all clear how to sort that mess out. I guess I agree with you that it
would be nice if it could be done, but the list of problems is pretty
substantial. Like, membership implies the right to SET ROLE, and also
the right to implicitly exercise the privileges of the role, and
you've complained about that fuzziness. And ADMIN OPTION implies
membership, and you don't like that either. And elsewhere it's been
raised that nobody would expect to have a table end up owned by
'pg_execute_server_programs', or a user logged in directly as
'employees' rather than as some particular employee, but all that
stuff can happen, and some of it can't even be effectively prevented
with good configuration. 'toe' can be a member of 'foot' while, which
makes sense to everybody, and at the same time, 'foot' can be a member
of 'toe', which doesn't make any sense at all. And because both
directions are possible even experienced PostgreSQL users and hackers
get confused, as demonstrated by Joshua's having just got the
revoke-from-role case backwards.

Of those four problems, the last two are clearly the result of
conflating users with groups - and really also with capabilities - and
having a unified role concept that encompasses all of those things. I
think we would be better off if we had not done that, both in the
sense that I think the system would be less confusing to understand,
and also in the sense that we would likely have fewer security bugs.
And similarly I agree with you that it would be better if the right to
administer a role were clearly separated from membership in a role,
and if the right to use the privileges of a role were separated from
the ability to SET ROLE to it. However, unlike you, I see the whole
'role membership' concept as the problem, not the solution. We
conflate a bunch of different kinds of things together and call them
all 'roles' and a bunch of other things together and call them
'membership' and then we end up with an awkward mess. That's how I see
it, anyway.

The ability of a role to revoke itself from some other role is just
something we need to accept as being a change that needs to be made, and
I do believe that such a change is supported by the standard, in that a
REVOKE will only work if you have the right to make it as the user who
performed the GRANT in the first place.

Great. I propose that we sever that issue and discuss it on a new
thread to avoid confusion. I believe there is some debate to be had
about exactly what we want the behavior to be in this area, but if we
can reach consensus on that point, this shouldn't be too hard to knock
out. I will take it as an action item to get that thread going, if
that works for you.

So that leads to these questions: (2A) Do you care about restricting
which roles the userbot can drop? (2B) If yes, do you endorse
restricting the ability of roles to revoke themselves from other
roles?

As with Joshua, and as hopefully came across from the above discussion,
I'm also a 'yes and yes' on these two.

Great.

I think that we don't have any great problems here, at least as far as
this very specific issue is concerned, if either the answer to (2A) is
no or the answer to (2B) is yes. However, if the answer to (2A) is yes
and the answer to (2B) is no, there are difficulties. Evidently in
that case we need some new kind of thing that behaves mostly likes a
group of roles but isn't actually a group of roles -- and that thing
needs to prohibit self-revocation. Given what I've written above, you
may be able to guess my preferred solution: let's call it a TENANT.
Then, my pseudo-super-user can have permission to (i) create roles in
that tenant, (ii) drop roles in that tenant, and (iii) assume the
privileges of roles in that tenant -- and your userbot can have
privileges to do (i) and (ii) but not (iii). All we need do is add a
roltenant column to pg_authid and find three bits someplace
corresponding to (i)-(iii), and we are home.

Where are those bits going to go though..? I don't think they should go
into pg_authid, nor do I feel that this 'tenant' or such should go there
either because pg_authid is about describing individual roles, not about
role associations. Instead, I'd suggest that those bits go into
pg_auth_members in the form of additional columns to describe the role
associations. That is, instead of the existance of a row in
pg_auth_members meaning that one role has membership in another role, we
give users the choice of if that's the case or not with a separate
column. That would then neatly give us a way for a role to have admin
rights over another role but not membership in that role. We could then
further extend this by adding other columns to pg_auth_members for other
rights as users decide they need them- such as the ability for a role to
DROP some set of roles.

What I had in mind is to add a pg_tenant catalog (tenid, tenname) and
add some columns to the pg_authid catalog (roltenant, roltenantrights,
or something like that). See above for why I am not excited about
piggybacking more things onto role membership.

=> revoke joshua from admin;
REVOKE ROLE

=*> \du
List of roles
Role name | Attributes | Member of
-----------+------------------------------------------------------------+-------------
admin | Cannot login | {}
employees | Cannot login | {}
joshua | | {employees}
sfrost | Superuser, Create role, Create DB, Replication, Bypass RLS | {}

Even though, in this case, it was 'sfrost' (a superuser) who GRANT'd
joshua to admin.

Quite so.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Feb 28, 2022 at 2:09 PM Stephen Frost <sfrost@snowman.net> wrote:

The ability of a role to revoke itself from some other role is just
something we need to accept as being a change that needs to be made, and
I do believe that such a change is supported by the standard, in that a
REVOKE will only work if you have the right to make it as the user who
performed the GRANT in the first place.

Moving this part of the discussion to a new thread to reduce confusion
and hopefully get broader input on this topic. It seems like Stephen
and I agree in principle that some change here is a good idea. If
anyone else thinks it's a bad idea, then this would be a great time to
mention that, ideally with reasons. If you agree that it's a good
idea, then it would be great to have your views on the follow-up
questions which I shall pose below. To the extent that it is
reasonably possible to do so, I would like to try to keep focused on
specific design questions rather than getting tangled up in general
discussion of long-term direction. First, a quick overview of the
issue for those who have not followed the earlier threads in their
grueling entirety:

rhaas=# create user boss;
CREATE ROLE
rhaas=# create user peon;
CREATE ROLE
rhaas=# grant peon to boss;
GRANT ROLE
rhaas=# \c - peon
You are now connected to database "rhaas" as user "peon".
rhaas=> revoke peon from boss; -- i don't like being bossed around!
REVOKE ROLE

I argue (and Stephen seems to agree) that the peon shouldn't be able
to undo the superuser's GRANT. Furthermore, we also seem to agree that
you don't necessarily have to be the exact user who performed the
grant. For example, it would be shocking if one superuser couldn't
remove a grant made by another superuser, or for that matter if a
superuser couldn't remove a grant made by a non-superuser. But there
are a few open questions in my mind:

1. What should be the exact rule for whether A can remove a grant made
by B? Is it has_privs_of_role()? is_member_of_role()? Something else?

2. What happens if the same GRANT is enacted by multiple users? For
example, suppose peon does "GRANT peon to boss" and then the superuser
does the same thing afterwards, or vice versa? One design would be to
try to track those as two separate grants, but I'm not sure if we want
to add that much complexity, since that's not how we do it now and it
would, for example, implicate the choice of PK on the pg_auth_members
table. An idea that occurs to me is to say that the first GRANT works
and becomes the grantor of record, and any duplicate GRANT that
happens later issues a NOTICE without changing anything. If the user
performing the later GRANT has sufficient privileges and wishes to do
so, s/he can REVOKE first and then re-GRANT. On the other hand, for
other types of grants, like table privileges, we do track multiple
grants by different users, so maybe we should do the same thing here:

rhaas=# create table example (a int, b int);
CREATE TABLE
rhaas=# grant select on table example to foo with grant option;
GRANT
rhaas=# grant select on table example to bar with grant option;
GRANT
rhaas=# \c - foo
You are now connected to database "rhaas" as user "foo".
rhaas=> grant select on table example to exemplar;
GRANT
rhaas=> \c - bar
You are now connected to database "rhaas" as user "bar".
rhaas=> grant select on table example to exemplar;
GRANT
rhaas=> select relacl from pg_class where relname = 'example';
relacl
-------------------------------------------------------------------------------
{rhaas=arwdDxt/rhaas,foo=r*/rhaas,bar=r*/rhaas,exemplar=r/foo,exemplar=r/bar}
(1 row)

3. What happens if a user is dropped after being recorded as a
grantor? We actually have a grantor column in pg_auth_members today,
but it's not properly maintained. If the grantor is dropped the OID
remains in the table, and could eventually end up pointing to some
other user if the OID counter wraps around and a new role is created
with the same OID. That's completely unacceptable for something we
want to use for any serious purpose. I suggest that what ought to
happen is the role should acquire a dependency on the grant, such that
DROP fails and the GRANT is listed as something to be dropped, and
DROP OWNED BY drops the GRANT. I think this would require adding an
OID column to pg_auth_members so that a dependency can point to it,
which sounds like a significant infrastructure change that would need
to be carefully validated for adverse side effects, but not a huge
crazy problem that we can't get past.

4. Should we apply this rule to other types of grants, rather than
just to role membership? Why or why not? Consider this:

rhaas=# create user accountant;
CREATE ROLE
rhaas=# create user auditor;
CREATE ROLE
rhaas=# create table money (a int, b text);
CREATE TABLE
rhaas=# alter table money owner to accountant;
ALTER TABLE
rhaas=# grant select on table money to auditor;
GRANT
rhaas=# \c - accountant
You are now connected to database "rhaas" as user "accountant".
rhaas=> revoke select on table money from auditor;
REVOKE

I would argue that's exactly the same problem. The superuser has
decreed that the auditor gets to select from the money table owned by
the accountant. The fact that the accountant may not be not in favor
of the auditor seeing what the accountant is doing with the money is
precisely the reason why we have auditors. That said, if we apply this
to all object types, it's a much bigger change. Unlike role
membership, we do record dependencies on table privileges, which makes
any change here a bit simpler, and you can't drop a role without
removing the associated grants first. However, when the superuser
performs the GRANT as in the above example, the grantor is recorded as
the table owner, not the superuser! So if we really want role
membersip and other kinds of grants to behave in the same way, we have
our work cut out for us here.

Please note that it is not really my intention to try to shove
anything into v15 here. If it so happens that we quickly agree on
something that already exists in the patches Mark's already written,
and we also agree that those patches are in good enough shape that we
can commit something in the next few weeks, fantastic, but I'm not
necessarily expecting that. What I do want to do is agree on a plan so
that, if somebody does the work to implement said plan, we do not then
end up relitigating the whole thing and coming to a different
conclusion the second time. This being a community whose membership
varies from time to time and the opinions of whose members vary from
time to time, such misadventure can never be entirely ruled out.
However, I would like to minimize the chances of such an outcome as
much as we can.

Thanks,

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

1. What should be the exact rule for whether A can remove a grant made
by B? Is it has_privs_of_role()? is_member_of_role()? Something else?

No strong opinion here, but I'd lean slightly to the more restrictive
option.

2. What happens if the same GRANT is enacted by multiple users? For
example, suppose peon does "GRANT peon to boss" and then the superuser
does the same thing afterwards, or vice versa? One design would be to
try to track those as two separate grants, but I'm not sure if we want
to add that much complexity, since that's not how we do it now and it
would, for example, implicate the choice of PK on the pg_auth_members
table.

As you note later, we *do* track such grants separately in ordinary
ACLs, and I believe this is clearly required by the SQL spec.
It says (for privileges on objects):

Each privilege is represented by a privilege descriptor.
A privilege descriptor contains:
— The identification of the object on which the privilege is granted.
— The <authorization identifier> of the grantor of the privilege.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
— The <authorization identifier> of the grantee of the privilege.
— Identification of the action that the privilege allows.
— An indication of whether or not the privilege is grantable.
— An indication of whether or not the privilege has the WITH HIERARCHY OPTION specified.

Further down (4.42.3 in SQL:2021), the granting of roles is described,
and that says:

Each role authorization is described by a role authorization descriptor.
A role authorization descriptor includes:
— The role name of the role.
— The authorization identifier of the grantor.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
— The authorization identifier of the grantee.
— An indication of whether or not the role authorization is grantable.

If we are not tracking the grantors of role authorizations,
then we are doing it wrong and we ought to fix that.

3. What happens if a user is dropped after being recorded as a
grantor?

Should work the same as it does now for ordinary ACLs, ie, you
gotta drop the grant first.

4. Should we apply this rule to other types of grants, rather than
just to role membership?

I am not sure about the reasoning behind the existing rule that
superuser-granted privileges are recorded as being granted by the
object owner. It does feel more like a wart than something we want.
It might have been a hack to deal with the lack of GRANTED BY
options in GRANT/REVOKE back in the day.

Changing it could have some bad compatibility consequences though.
In particular, I believe it would break existing pg_dump files,
in that after restore all privileges would be attributed to the
restoring superuser, and there'd be no very easy way to clean that
up.

Please note that it is not really my intention to try to shove
anything into v15 here.

Agreed, this is not something to move on quickly. We might want
to think about adjusting pg_dump to use explicit GRANTED BY
options in GRANT/REVOKE a release or two before making incompatible
changes.

regards, tom lane

On Fri, Mar 4, 2022 at 1:50 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Feb 28, 2022 at 2:09 PM Stephen Frost <sfrost@snowman.net> wrote:

The ability of a role to revoke itself from some other role is just
something we need to accept as being a change that needs to be made, and
I do believe that such a change is supported by the standard, in that a
REVOKE will only work if you have the right to make it as the user who
performed the GRANT in the first place.

First, a quick overview of the
issue for those who have not followed the earlier threads in their
grueling entirety:

rhaas=# create user boss;
CREATE ROLE
rhaas=# create user peon;
CREATE ROLE
rhaas=# grant peon to boss;
GRANT ROLE
rhaas=# \c - peon
You are now connected to database "rhaas" as user "peon".
rhaas=> revoke peon from boss; -- i don't like being bossed around!
REVOKE ROLE

The wording for this example is hurting my brain.
GRANT admin TO joe;
\c admin
REVOKE admin FROM joe;

I argue (and Stephen seems to agree) that the peon shouldn't be able
to undo the superuser's GRANT.

I think I disagree. Or, at least, the superuser has full control of
dictating how role membership is modified and that seems sufficient.

The example above works because of:

"A role is not considered to hold WITH ADMIN OPTION on itself, but it may
grant or revoke membership in itself from a database session where the
session user matches the role."

If a superuser doesn't want "admin" to modify its own membership then they
can prevent anyone but a superuser from being able to have a session_user
of "admin". If that happens then the only way a non-superuser can modify
group membership is by being added to said group WITH ADMIN OPTION.

Now, if two people and a superuser are all doing membership management on
the same group, and we want to add permission checks and multiple grants as
tools, instead of having them just communicate with each other, then by all
means let us do so. In that case, in answer to questions 2 and 3, we
should indeed track which session_user made the grant and only allow the
same session_user or the superuser to revoke it (we'd want to stop
"ignoring" the GRANTED BY clause of REVOKE ROLE FROM so the superuser at
least could remove grants made via WITH ADMIN OPTION).

4. Should we apply this rule to other types of grants, rather than

just to role membership? Why or why not? Consider this:

The fact that the accountant may not be not in favor
of the auditor seeing what the accountant is doing with the money is
precisely the reason why we have auditors.

[...]

However, when the superuser
performs the GRANT as in the above example, the grantor is recorded as
the table owner, not the superuser! So if we really want role
membersip and other kinds of grants to behave in the same way, we have
our work cut out for us here.

Yes, this particular choice seems unfortunate, but also not something that
I think it is necessarily mandatory for us to improve. If the accountant
is the owner then yes they get to decide permissions. In the presence of
an auditor role either you trust the accountant role to keep the
permissions in place or you define a superior authority to both the auditor
and accountant to be the owner. Or let the superuser manage everything by
witholding login and WITH ADMIN OPTION privileges from the ownership role.

If we do extend role membership tracking I suppose the design question is
whether the new role grantor dependency tracking will have a superuser be
the recorded grantor instead of some owner. Given that roles don't
presently have an owner concept, consistency with existing permissions in
this manner would be trickier. Because of this, I would probably leave
role grantor tracking at the session_user level while database objects
continue to emanate from the object owner. The global vs database
differences seem like a sufficient theoretical justification for the
difference in implementation.

David J.

On Fri, Mar 4, 2022 at 5:20 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

I think I disagree. Or, at least, the superuser has full control of dictating how role membership is modified and that seems sufficient.

The point is that the superuser DOES NOT have full control. The
superuser cannot prevent relatively low-privileged users from undoing
things that the superuser did intentionally and doesn't want reversed.

The choice of names in my example wasn't accidental. If the granted
role is a login role, then the superuser's intention was to vest the
privileges of that role in some other role, and it is surely not right
for that role to be able to decide that it doesn't want it's
privileges to be so granted. That's why I chose the name "peon". In
your example, where you chose the name "admin", the situation is less
clear. If we imagine the granted role as a container for a bundle of
privileges, giving it the ability to administer itself feels more
reasonable. However, I am very much unconvinced that it's correct even
there. Suppose the superuser grants "admin" to both "joe" and "sally".
Now "joe" can SET ROLE to "admin" and revoke it from "sally", and the
superuser has no tool to prevent this.

Now you can imagine a situation where the superuser is totally OK with
either "joe" or "sally" having the ability to lock the other one out,
but I don't think it's right to say that this will be true in all
cases.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Mar 4, 2022 at 4:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

If we are not tracking the grantors of role authorizations,
then we are doing it wrong and we ought to fix that.

Hmm, so maybe that's the place to start. We are tracking it in the
sense that we record an OID in the catalog, but nothing that happens
after that makes a lot of sense.

3. What happens if a user is dropped after being recorded as a
grantor?

Should work the same as it does now for ordinary ACLs, ie, you
gotta drop the grant first.

OK, that makes sense to me.

Changing it could have some bad compatibility consequences though.
In particular, I believe it would break existing pg_dump files,
in that after restore all privileges would be attributed to the
restoring superuser, and there'd be no very easy way to clean that
up.

I kind of wonder whether we ought to attribute all privileges granted
by any superuser to the bootstrap superuser. That doesn't seem to have
any meaningful downside, and it could avoid a lot of annoying
dependencies that serve no real purpose.

Agreed, this is not something to move on quickly. We might want
to think about adjusting pg_dump to use explicit GRANTED BY
options in GRANT/REVOKE a release or two before making incompatible
changes.

Uggh. I really want to make some meaningful progress here before the
heat death of the universe, and I'm not sure that this manner of
proceeding is really going in that direction. That said, I do entirely
see your point. Are you thinking we'd actually add a GRANTED BY clause
to GRANT/REVOKE, vs. just wrapping it in SET ROLE incantations of some
sort?

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Mar 4, 2022 at 4:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Agreed, this is not something to move on quickly. We might want
to think about adjusting pg_dump to use explicit GRANTED BY
options in GRANT/REVOKE a release or two before making incompatible
changes.

Uggh. I really want to make some meaningful progress here before the
heat death of the universe, and I'm not sure that this manner of
proceeding is really going in that direction. That said, I do entirely
see your point. Are you thinking we'd actually add a GRANTED BY clause
to GRANT/REVOKE, vs. just wrapping it in SET ROLE incantations of some
sort?

I was thinking the former ... however, after a bit of experimentation
I see that we accept "grant foo to bar granted by baz" a VERY long
way back, but the "granted by" option for object privileges is
(a) pretty new and (b) apparently restrictively implemented:

regression=# grant delete on alices_table to bob granted by alice;
ERROR: grantor must be current user

That's ... surprising. I guess whoever put that in was only
interested in pro-forma SQL syntax compliance and not in making
a usable feature.

So if we decide to extend this change into object privileges
it would be advisable to use SET ROLE, else we'd be giving up
an awful lot of backwards compatibility in dump scripts.
But if we're only talking about role grants then I think
GRANTED BY would work fine.

regards, tom lane

On Sun, Mar 6, 2022 at 10:19 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Mar 4, 2022 at 5:20 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

I think I disagree. Or, at least, the superuser has full control of dictating how role membership is modified and that seems sufficient.

The point is that the superuser DOES NOT have full control. The
superuser cannot prevent relatively low-privileged users from undoing
things that the superuser did intentionally and doesn't want reversed.

The choice of names in my example wasn't accidental. If the granted
role is a login role, then the superuser's intention was to vest the
privileges of that role in some other role, and it is surely not right
for that role to be able to decide that it doesn't want it's
privileges to be so granted. That's why I chose the name "peon". In
your example, where you chose the name "admin", the situation is less
clear. If we imagine the granted role as a container for a bundle of
privileges, giving it the ability to administer itself feels more
reasonable. However, I am very much unconvinced that it's correct even
there. Suppose the superuser grants "admin" to both "joe" and "sally".
Now "joe" can SET ROLE to "admin" and revoke it from "sally", and the
superuser has no tool to prevent this.

Now you can imagine a situation where the superuser is totally OK with
either "joe" or "sally" having the ability to lock the other one out,
but I don't think it's right to say that this will be true in all
cases.

Another example here is usage of groups in pg_hba.conf, if the admin
has a group of users with stronger authentication requirements: e.g.,

hostssl all +certonlyusers all cert map=certmap clientcert=1

and one can remove their membership, they can change their
authentication requirements.

Robert Haas <robertmhaas@gmail.com> writes:

... Suppose the superuser grants "admin" to both "joe" and "sally".
Now "joe" can SET ROLE to "admin" and revoke it from "sally", and the
superuser has no tool to prevent this.

Really?

regression=# grant admin to joe;
GRANT ROLE
regression=# grant admin to sally;
GRANT ROLE
regression=# \c - joe
You are now connected to database "regression" as user "joe".
regression=> revoke admin from sally;
ERROR: must have admin option on role "admin"
regression=> set role admin;
SET
regression=> revoke admin from sally;
ERROR: must have admin option on role "admin"

I think there is an issue here around exactly what the admin option
means, but if it doesn't grant you the ability to remove grants
made by other people, it's pretty hard to see what it's for.

regards, tom lane

On Sun, Mar 6, 2022 at 9:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

... Suppose the superuser grants "admin" to both "joe" and "sally".
Now "joe" can SET ROLE to "admin" and revoke it from "sally", and the
superuser has no tool to prevent this.

Really?

regression=# grant admin to joe;
GRANT ROLE
regression=# grant admin to sally;
GRANT ROLE
regression=# \c - joe
You are now connected to database "regression" as user "joe".
regression=> revoke admin from sally;
ERROR: must have admin option on role "admin"
regression=> set role admin;
SET
regression=> revoke admin from sally;
ERROR: must have admin option on role "admin"

I think there is an issue here around exactly what the admin option
means, but if it doesn't grant you the ability to remove grants
made by other people, it's pretty hard to see what it's for.

Precisely.

The current system, with the session_user exception, basically guides a
superuser to define two kinds of roles.

Groups: No login, permission grants
Users: Login, inherits permissions from groups, can manage group membership
if given WITH ADMIN OPTION.

The original example using only users is not all that compelling to me.
IMO, DBAs should not be setting up their system that way.

Two questions remain:

1. Are we willing to get rid of the session_user exception?

2. Do we want to track who the grantor is for role membership grants and
institute a requirement that non-superusers can only revoke the grants that
they personally made?

I'm personally in favor of getting rid of the session_user exception, which
nicely prevents the problem at the beginning of this thread and further
encourages the DBA to define groups and roles with a greater
separation-of-concerns design. WITH ADMIN OPTION is sufficient.

I think tracking grantor information for role membership would allow for
greater auditing capabilities and a better degree of control in the
permissions system.

In short, I am in favor of both options. The grantor tracking seems to be
headed for acceptance.

So, do we really want to treat every single login role as a potential group
by keeping the session_user exception?

David J.

On Sun, Mar 6, 2022 at 8:19 AM Robert Haas <robertmhaas@gmail.com> wrote:

The choice of names in my example wasn't accidental. If the granted
role is a login role, then the superuser's intention was to vest the
privileges of that role in some other role, and it is surely not right
for that role to be able to decide that it doesn't want it's
privileges to be so granted. That's why I chose the name "peon".

rhaas [as peon] => revoke peon from boss; -- i don't like being bossed

around!

Well, the peon is not getting bossed around, the boss is getting peoned
around and the peon has decided that they like boss too much and don't need
to do that anymore.

When you grant a group "to" a role you place the role under the group - and
inheritance flows downward.

In the original thread Stephen wrote:

"This is because we allow 'self administration' of roles, meaning that
they can decide what other roles they are a member of.:

The example, which you moved here, then attempts to demonstrate this "fact"
but gets it wrong. Boss became a member of peon so if you want to
demonstrate self-administration of a role's membership in a different group
you have to login as boss, not peon. Doing that, and then revoking peon
from boss, yields "ERROR: must have admin option on role "peon"".

So no, without "WITH ADMIN OPTION" a role cannot decide what other roles
they are a member of.

I don't necessarily have an issue changing self-administration but if the
motivating concern is that all these new pg_* roles we are creating are
something a normal user can opt-out of/revoke that simply isn't the case
today, unless they are added to the pg_* role WITH ADMIN OPTION.

That all said, permissions SHOULD BE strictly additive. If boss doesn't
want to be a member of pg_read_all_files allowing them to revoke themself
from that role seems like it should be acceptable. If there is fear in
allowing someone to revoke (not add) themselves as a member of a different
role that suggests we have a design issue in another feature of the
system. Today, they neither grant nor revoke, and the self-revocation
doesn't seem that important to add.

David J.

On Sun, Mar 6, 2022 at 11:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I was thinking the former ... however, after a bit of experimentation
I see that we accept "grant foo to bar granted by baz" a VERY long
way back, but the "granted by" option for object privileges is
(a) pretty new and (b) apparently restrictively implemented:

regression=# grant delete on alices_table to bob granted by alice;
ERROR: grantor must be current user

That's ... surprising. I guess whoever put that in was only
interested in pro-forma SQL syntax compliance and not in making
a usable feature.

It appears so: /messages/by-id/2073b6a9-7f79-5a00-5f26-cd19589a52c7@2ndquadrant.com

It doesn't seem like that would be hard to fix. Maybe we should just do that.

So if we decide to extend this change into object privileges
it would be advisable to use SET ROLE, else we'd be giving up
an awful lot of backwards compatibility in dump scripts.
But if we're only talking about role grants then I think
GRANTED BY would work fine.

OK.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Sun, Mar 6, 2022 at 11:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Really?

regression=# grant admin to joe;
GRANT ROLE
regression=# grant admin to sally;
GRANT ROLE
regression=# \c - joe
You are now connected to database "regression" as user "joe".
regression=> revoke admin from sally;
ERROR: must have admin option on role "admin"
regression=> set role admin;
SET
regression=> revoke admin from sally;
ERROR: must have admin option on role "admin"

Oops. I stand corrected.

I think there is an issue here around exactly what the admin option
means, but if it doesn't grant you the ability to remove grants
made by other people, it's pretty hard to see what it's for.

Hmm. I think the real issue is what David Johnson calls the session
user exception. I hadn't quite understood how that played into this.
According to the documentation: "If WITH ADMIN OPTION is specified,
the member can in turn grant membership in the role to others, and
revoke membership in the role as well. Without the admin option,
ordinary users cannot do that. A role is not considered to hold WITH
ADMIN OPTION on itself, but it may grant or revoke membership in
itself from a database session where the session user matches the
role."

Is there some use case for the behavior described in that last
sentence? If that exception is the only case in which an unprivileged
user can revoke a grant made by someone else, then getting rid of it
seems pretty appealing from where I sit. I can't speak to the
standards compliance end of things, but it doesn't intrinsically seem
bothersome that having "WITH ADMIN OPTION" on a role lets you control
who has membership in said role. And certainly it's not bothersome
that the superuser can change whatever they want. The problem here is
just that a user with NO special privileges on any role, including
their own, can make changes that more privileged users might not like.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Sun, Mar 6, 2022 at 2:09 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

So, do we really want to treat every single login role as a potential group by keeping the session_user exception?

I think that we DO want to continue to treat login roles as
potentially grantable privileges. That feels fundamentally useful to
me. The superuser is essentially granted the privileges of all users
on the system, and has all the rights they have, including the right
to drop tables owned by those users as if they were the owner of those
tables. If it's useful for the superuser to implicitly have the rights
of all users on the system, why should it not be useful for some
non-superuser to implicitly have the rights of some other users on the
system? I think it pretty clearly is. If one of my colleagues leaves
the company, the DBA can say "grant jdoe to rhaas" and let me mess
around with this stuff. Or, the DBA can grant me the privileges of all
my direct reports even when they're not leaving so that I can sort out
anything I need to do without superuser involvement. That all seems
cool and OK to me.

Now I think it is fair to say that we could have chosen a different
design, and MAYBE that would have been better. Nobody forced us to
conflate users and groups into a unified thing called roles, and I
think there's pretty good evidence that it's confusing and
counterintuitive in some ways. There's also no intrinsic reason why
the superuser has to be able to directly exercise the privileges of
every role rather than, say, having a way to become any given role.
But at this point, those design decisions are pretty well baked into
the system design, and I don't really think it's likely that we want
to change them. To put that another way, just because you don't like
the idea of granting one login role to another login role, that
doesn't mean that the feature doesn't exist, and as long as that
feature does exist, trying to make it work better or differently is
fair game.

But I think that's separate from your other question about whether we
should remove the session user exception. That looks tempting to me at
first glance, because we have exchanged several hundred, and it really
feels more like several million, emails on this list about how much of
a problem it is that an unprivileged user can just log in and run a
REVOKE. It breaks the idea that the people WITH ADMIN OPTION on a role
are the ones who control membership in that role. Joshua Brindle's
note upthread about the interaction of this with pg_hba.conf is
another example of that, and I think there are more. Any idea that a
role is a general-purpose way of designating a group of users for some
security critical purpose is threatened if people can make changes to
the membership of that group without being specifically authorized to
do so.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Mar 7, 2022 at 8:37 AM Robert Haas <robertmhaas@gmail.com> wrote:

A role is not considered to hold WITH
ADMIN OPTION on itself, but it may grant or revoke membership in
itself from a database session where the session user matches the
role."

Is there some use case for the behavior described in that last
sentence?

I can imagine, in particular combined with CREATEROLE, that this allows for
any user to delegate their personal permissions to a separate newly created
user. Like an assistant. I'm not all that sure whether CREATEROLE is
presently safe enough to give to a normal user in order to make this use
case work but it seems reasonable.

I would be concerned about changing the behavior at this point. But I
would be in favor of at least removing the hard-coded exception and linking
it to a role attribute. That attribute can default to "SELFADMIN" to match
the existing behavior but then "NOSELFADMIN" would exist to disable that
behavior on the per-role basis. Still tied to session_user as opposed to
current_user.

David J.

P.S.

create role selfadmin admin selfadmin; -- ERROR: role "selfadmin" is a
member of role "selfadmin"

create role selfadmin;
grant selfadmin to selfadmin with admin option; -- ERROR: role "selfadmin"
is a member of role "selfadmin"

The error message seems odd. I tried this because instead of a "SELFADMIN"
attribute adding a role to itself WITH ADMIN OPTION could be defined to
have the same effect. You cannot change WITH ADMIN OPTION independently of
the adding of the role to the group.

Robert Haas <robertmhaas@gmail.com> writes:

Hmm. I think the real issue is what David Johnson calls the session
user exception. I hadn't quite understood how that played into this.
According to the documentation: "If WITH ADMIN OPTION is specified,
the member can in turn grant membership in the role to others, and
revoke membership in the role as well. Without the admin option,
ordinary users cannot do that. A role is not considered to hold WITH
ADMIN OPTION on itself, but it may grant or revoke membership in
itself from a database session where the session user matches the
role."

Is there some use case for the behavior described in that last
sentence?

Good question. You might try figuring out when that text was added
and then see if there's relevant discussion in the archives.

Just looking at it now, without having done any historical research,
I wonder why it is that we don't attach significance to WITH ADMIN
OPTION being granted to the role itself. It seems like the second
part of that sentence is effectively saying that a role DOES have
admin option on itself, contradicting the first part.

regards, tom lane

On Mon, Mar 7, 2022 at 9:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Just looking at it now, without having done any historical research,

I wonder why it is that we don't attach significance to WITH ADMIN

OPTION being granted to the role itself. It seems like the second
part of that sentence is effectively saying that a role DOES have
admin option on itself, contradicting the first part.

WITH ADMIN OPTION is inheritable which is really bad if the group has WITH
ADMIN OPTION on itself. The session_user exception temporarily grants WITH
ADMIN OPTION to the group but it is done in such a way so that it is not
inheritable.

There is no possible way to even assign WITH ADMIN OPTION on a role to
itself since pg_auth_members doesn't record a self-relationship and
admin_option only exists there.

David J.

P.S. Feature request; modify \du+ to show which "Member of" roles a given
role has the WITH ADMIN OPTION privilege on.

On Mon, Mar 7, 2022 at 11:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Is there some use case for the behavior described in that last
sentence?

Good question. You might try figuring out when that text was added
and then see if there's relevant discussion in the archives.

Apparently the permission used to be broader, and commit
fea164a72a7bfd50d77ba5fb418d357f8f2bb7d0 (February 2014, Noah,
CVE-2014-0060) restricted it by requiring that (a) the user had to be
the logged-in user, rather than an identity assumed via SET ROLE (so
maybe my bogus example from before would have worked in 2013) and (b)
that we're not in a security-restricted operation at the time.

Interestingly, it appears to me that the behavior wasn't documented
prior to that commit. The previous text read simply:

If <literal>WITH ADMIN OPTION</literal> is specified, the member can
in turn grant membership in the role to others, and revoke membership
in the role as well. Without the admin option, ordinary users cannot do
that.

That doesn't give any hint that self-administration is a special case.

I reviewed the (private) discussion of this vulnerability on the
pgsql-security mailing list where various approaches were considered.
I think it's safe to share a few broad details about that conversation
publicly now, since it was many years ago and the fix has long since
been published. There was discussion of making this
self-administration behavior something that could be turned off, but
such a change was deemed too large for the back-branches. There was no
discussion that I could find about removing the behavior altogether.
It was noted that having a special case for this was different than
granting WITH ADMIN OPTION because WITH ADMIN OPTION is inherited and
being logged in as a certain user is not.

It appears to me that the actual behavior of having is_admin_of_role()
return true when member == role dates to
f9fd1764615ed5d85fab703b0ffb0c323fe7dfd5 (Tom Lane, 2005). If I'm not
reading this code wrong, prior to that commit, it seems to me that we
only searched the roles that were members of that role, directly or
indirectly, and you had to have admin_option on the last hop of the
membership chain in order to get a "true" result. But that commit,
among other changes, made member == role a special case, but the
comment just says /* Fast path for simple case */ which makes it
appear that it wasn't thought to be a behavior change at all, but it
looks to me like it was. Am I confused?

--
Robert Haas
EDB: http://www.enterprisedb.com

On Sun, Mar 6, 2022 at 11:01 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

The example, which you moved here, then attempts to demonstrate this "fact" but gets it wrong. Boss became a member of peon so if you want to demonstrate self-administration of a role's membership in a different group you have to login as boss, not peon. Doing that, and then revoking peon from boss, yields "ERROR: must have admin option on role "peon"".

This doesn't seem to me to be making a constructive argument. I showed
an example with certain names demonstrating a certain behavior that I
find problematic. You don't have to think it's problematic, and you
can show other examples that demonstrate things you want to show. But
please don't tell me that when I literally cut and paste the output
from my terminal into an email window, what I'm showing is somehow
counterfactual. The behavior as it exists today is surely a fact, and
an easily demonstrable one at that. It's not a "fact'" in quotes, and
it doesn't "get it wrong". It is the actual behavior and the example
with the names I picked demonstrates precisely what I want to
demonstrate. When you say that I should have chosen a different
example or used different identifier names or talked about it in
different way, *that* is an opinion. I believe that you are wholly
entitled to that opinion, even if (as in this case) I disagree, but I
believe that it is not right at all to make it sound as if I don't
have the right to pick the examples I care about, or as if terminal
output is not a factual representation of how things work today.

So no, without "WITH ADMIN OPTION" a role cannot decide what other roles they are a member of.

It clearly can in some limited cases, because I showed an example
demonstrating *exactly that thing*.

I don't necessarily have an issue changing self-administration but if the motivating concern is that all these new pg_* roles we are creating are something a normal user can opt-out of/revoke that simply isn't the case today, unless they are added to the pg_* role WITH ADMIN OPTION.

I agree with this, but that's not my concern, because that's a
different use case from the one that I complained about. Since the
session user exception only applies to login roles, the problem that
I'm talking about only occurs when a login role is granted to some
other role.

That all said, permissions SHOULD BE strictly additive. If boss doesn't want to be a member of pg_read_all_files allowing them to revoke themself from that role seems like it should be acceptable. If there is fear in allowing someone to revoke (not add) themselves as a member of a different role that suggests we have a design issue in another feature of the system. Today, they neither grant nor revoke, and the self-revocation doesn't seem that important to add.

I disagree with this on principle, and I also think that's not how it
works today. On the general principle, I do not see a compelling
reason why we should have two systems for maintaining groups of users,
one of which is used for additive things and one of which is used for
subtractive things. That is a lot of extra machinery for little gain,
especially given how close we are to having it sorted out so that the
same mechanism can serve both purposes. It presently appears to me
that if we either remove the session user exception OR do the
grantor-tracking thing discussed earlier, we can get to a place where
the same facility can be used for either purpose. That would, I think,
be a significant step forward over the status quo. In terms of how
things work today, see Joshua Brindle's email about the use of groups
in pg_hba.conf. That is an excellent example of how removing oneself
from a group could enable one to bypass security restrictions intended
by the DBA.

--
Robert Haas
EDB: http://www.enterprisedb.com