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

Robert Haas <robertmhaas@gmail.com> writes:

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?

Ugh, I think you are right. It's been a long time of course, but it sure
looks like that was copied-and-pasted without recognizing that it was
wrong in this function because of the need to check the admin_option flag.
And then in the later security discussion we didn't realize that the
problematic behavior was a flat-out thinko, so we narrowed it as much as
we could instead of just taking it out.

Does anything interesting break if you do just take it out?

regards, tom lane

On Mon, Mar 7, 2022 at 1:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Ugh, I think you are right. It's been a long time of course, but it sure
looks like that was copied-and-pasted without recognizing that it was
wrong in this function because of the need to check the admin_option flag.
And then in the later security discussion we didn't realize that the
problematic behavior was a flat-out thinko, so we narrowed it as much as
we could instead of just taking it out.

Does anything interesting break if you do just take it out?

That is an excellent question, but I haven't had time yet to
investigate the matter.

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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

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.

Agreed.

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.

Yup, and as noted elsewhere, we are tracking it but not properly dealing
with dependencies nor are we considering the grantor when REVOKE is run.

Looking at the spec for REVOKE is quite useful when trying to understand
how this is all supposed to work (and, admittedly, isn't something I did
enough of when I did the original work on roles... sorry about that, was
early on). In particular, a REVOKE only works when it finds something
to revoke/remove, and part of that search includes basically "was it the
current role who was the grantor?"

The specific language here being: A role authorization descriptor is
said to be identified if it defines the grant of any of the specified
roles revoked to grantee with grantor A.

Basically, a role authorization descriptor isn't identified unless it's
one that this user/role had previously granted.

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.

Agreed.

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.

Yeah, that doesn't seem right and isn't great.

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.

Ugh, that's pretty grotty, certainly.

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.

I'm with Robert on this though- folks should know already that they need
to use the pg_dump of the version of PG that they want to move to and
not try to re-use older pg_dump output with newer versions, for a number
of reasons and this is just another.

Thanks,

Stephen

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

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.

Whether you choose the wording of the original thread:

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

/messages/by-id/20211005025746.GN20998@tamriel.snowman.net

Or you quote at the top of this one:

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,

This example:

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

Fails to demonstrate the boss "can revoke itself from peon" / "boss can
decide what other roles they are a member of."

You are logged in as peon when you do the revoke, not boss, so the extent
of what "boss" can or cannot do has not been shown.

boss is a member of peon, not the other way around. That the wording
"grant peon to boss" makes you think otherwise is unfortunate.

David J.

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:

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.

Yeah, that seems like something that should be fixed. Superusers should
be allowed to set GRANTED BY to whatever they feel like, and I'd argue
that a role who wants a GRANT to actually be GRANTED BY some other role
they're a member of should also be allowed to (as they could anyway by
doing a SET ROLE), provided that role also has the privileges to do the
GRANT itself, of course.

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.

I'm not quite following this bit. Where would SET ROLE come into play
when we're talking about old dump scripts and how the commands in those
scripts might be interpreted by newer versions of PG..?

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

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

I'm with Robert on this though- folks should know already that they need
to use the pg_dump of the version of PG that they want to move to and
not try to re-use older pg_dump output with newer versions, for a number
of reasons and this is just another.

Yeah, in an ideal world you'd do that, but our users don't always have
the luxury of living in an ideal world. Sometimes all you've got is
an old pg_dump file. Perhaps this behavior wouldn't mess things up
enough to make the restored database unusable, but we need to think
about (and test) that case while we're considering changes.

regards, tom lane

Stephen Frost <sfrost@snowman.net> writes:

I'm not quite following this bit. Where would SET ROLE come into play
when we're talking about old dump scripts and how the commands in those
scripts might be interpreted by newer versions of PG..?

No, the concern there is the other way around: what if you take a
script made by newer pg_dump and try to load it into an older server
that doesn't have the GRANTED BY option?

We're accustomed to saying that that doesn't work if you use a
database feature that didn't exist in the old server, but
privilege grants are hardly that. I don't want us to change the
pg_dump output in such a way that the grants can't be restored at all
to an older server, just because of a syntax choice that we could
make backwards-compatibly instead of not-backwards-compatibly.

regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

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

I'm with Robert on this though- folks should know already that they need
to use the pg_dump of the version of PG that they want to move to and
not try to re-use older pg_dump output with newer versions, for a number
of reasons and this is just another.

Yeah, in an ideal world you'd do that, but our users don't always have
the luxury of living in an ideal world. Sometimes all you've got is
an old pg_dump file. Perhaps this behavior wouldn't mess things up
enough to make the restored database unusable, but we need to think
about (and test) that case while we're considering changes.

I agree it's something to consider and deal with if we're able to do so
sanely, but I disagree that we should be beholden to old dump files when
considering how to move the project forward. Further, they can surely
build and install the version of PG that goes with that dump file in a
great many cases and then dump the data out using a newer version of
pg_dump. For 5 years they could do that with a completely supported
version of PG, but we've recently agreed to make an effort to do more
here by supporting the building of even older versions on modern
systems.

Thanks,

Stephen

On Mon, Mar 7, 2022 at 1:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Stephen Frost <sfrost@snowman.net> writes:

I'm not quite following this bit. Where would SET ROLE come into play
when we're talking about old dump scripts and how the commands in those
scripts might be interpreted by newer versions of PG..?

No, the concern there is the other way around: what if you take a
script made by newer pg_dump and try to load it into an older server
that doesn't have the GRANTED BY option?

We're accustomed to saying that that doesn't work if you use a
database feature that didn't exist in the old server, but
privilege grants are hardly that. I don't want us to change the
pg_dump output in such a way that the grants can't be restored at all
to an older server, just because of a syntax choice that we could
make backwards-compatibly instead of not-backwards-compatibly.

Are you absolutely positive that it's that simple? I mean, what if the
SET ROLE command has other side effects, or if the GRANT command
behaves differently in some way as a result of the SET ROLE having
been done? I feel like a solution that involves explicitly specifying
the behavior that we want (i.e. GRANTED BY) is likely to be more
reliable and more secure than a solution which involves absorbing a
key value from a session property (i.e. the role established by SET
ROLE). Even if we decide that SET ROLE is the way to go for
compatibility reasons, I would personally say that it's an inferior
hack only worth accepting for that reason than a truly desirable
design.

See CVE-2018-1058 for an example of what I'm talking about. The
prevailing search_path turned out to affect not only the creation
schema, as intended, but also the resolution of references to other
objects mentioned in the CREATE COMMAND, as not intended. I don't see
a similar hazard here, but I'm worried that there might be one.
Declarative syntax is a very powerful tool for avoiding those kinds of
mishaps, and I think we should make as much use of it as we can.

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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

I'm not quite following this bit. Where would SET ROLE come into play
when we're talking about old dump scripts and how the commands in those
scripts might be interpreted by newer versions of PG..?

No, the concern there is the other way around: what if you take a
script made by newer pg_dump and try to load it into an older server
that doesn't have the GRANTED BY option?

Wow. No, I really don't think I can agree that we need to care about
this.

We're accustomed to saying that that doesn't work if you use a
database feature that didn't exist in the old server, but
privilege grants are hardly that. I don't want us to change the
pg_dump output in such a way that the grants can't be restored at all
to an older server, just because of a syntax choice that we could
make backwards-compatibly instead of not-backwards-compatibly.

GRANTED BY is clearly such a feature that exists in the newer version
and doesn't exist in the older and I can't agree that we should
complicate things for ourselves and bend over backwards to try and make
it work to take a dump from a newer version of PG and make it work on
random older versions.

Folks are also able to exclude privileges from dumps if they want to.

Where do we document that we are going to put in effort to make these
kinds of things work? What other guarantees are we supposed to be
providing regarding using output from a newer pg_dump against older
servers? What about newer custom format dumps? Surely you're not
suggesting that we need to back-patch support for them to released
versions of pg_restore.

Thanks,

Stephen

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

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.

You mean the one that was based upon your "ooops"...I discounted that
immediately because members cannot revoke their own membership in a group
unless they were given WITH ADMIN OPTION on that group.

The mere fact that the pg_hba.conf concern raised there hasn't been
reported as a live issue suggests the lack of any meaningful design flaw
here.

That isn't to say that having a LOGIN role get an automatic temporary WITH
ADMIN OPTION on itself is a good thing - but there isn't any privilege
escalation vector here to be squashed. There is just a "DBAs should treat
LOGIN roles as leaf nodes" expectation in which case there would be no
superuser granted memberships to be removed.

David J.

On Mon, Mar 7, 2022 at 2:29 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

You mean the one that was based upon your "ooops"...I discounted that immediately because members cannot revoke their own membership in a group unless they were given WITH ADMIN OPTION on that group.

Oh, hmm. That example might be backwards from the case I'm talking about.

The mere fact that the pg_hba.conf concern raised there hasn't been reported as a live issue suggests the lack of any meaningful design flaw here.

Not really. The system is full of old bugs, just as all software
system are, and the particular role self-administration behavior that
is at issue here appears to be something that was accidentally
introduced 16 years years ago in a commit that did something else and
never scrutinized from a design perspective since then.

Personally, I've been shocked by the degree to which this entire area
seems to be full of design flaws and half-baked code. I mean, just the
fact that the pg_auth_members.grantor can be left pointing to a role
OID that no longer exists is pretty crazy, right? I don't think anyone
today would consider something with that kind of wart committable.

That isn't to say that having a LOGIN role get an automatic temporary WITH ADMIN OPTION on itself is a good thing - but there isn't any privilege escalation vector here to be squashed. There is just a "DBAs should treat LOGIN roles as leaf nodes" expectation in which case there would be no superuser granted memberships to be removed.

Well, we may not have found one yet, but that doesn't prove none
exists. In any case, if we can agree that it's not necessarily a
desirable behavior, that's good enough for me.

(I still disagree with the idea that LOGIN roles have to be leaf
nodes. We could have a system where that's true, but that's not how
the system we actually have is designed.)

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

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

Does anything interesting break if you do just take it out?

 SET SESSION AUTHORIZATION regress_priv_group2;
 GRANT regress_priv_group2 TO regress_priv_user5; -- ok: a role can self-admin
-NOTICE:  role "regress_priv_user5" is already a member of role "regress_priv_group2"
+ERROR:  must have admin option on role "regress_priv_group2"

This test failure is just a manifestation of the intended change, but assuming we make no other changes, the error message would clearly need to be updated, because it suggests the role should have admin_option on itself, a situation which is not currently supported.

Perhaps we should support that, though, by adding a reflexive aclitem[] to pg_authid (meaning it tracks which privileges a role has on itself) with tracking of who granted it, so that revocation can be handled properly. The aclitem could start out null, meaning the role has by default the traditional limited self-admin which the code comments discuss:

/*
* A role can admin itself when it matches the session user and we're
* outside any security-restricted operation, SECURITY DEFINER or
* similar context. SQL-standard roles cannot self-admin. However,
* SQL-standard users are distinct from roles, and they are not
* grantable like roles: PostgreSQL's role-user duality extends the
* standard. Checking for a session user match has the effect of
* letting a role self-admin only when it's conspicuously behaving
* like a user. Note that allowing self-admin under a mere SET ROLE
* would make WITH ADMIN OPTION largely irrelevant; any member could
* SET ROLE to issue the otherwise-forbidden command.
*
* Withholding self-admin in a security-restricted operation prevents
* object owners from harnessing the session user identity during
* administrative maintenance. Suppose Alice owns a database, has
* issued "GRANT alice TO bob", and runs a daily ANALYZE. Bob creates
* an alice-owned SECURITY DEFINER function that issues "REVOKE alice
* FROM carol". If he creates an expression index calling that
* function, Alice will attempt the REVOKE during each ANALYZE.
* Checking InSecurityRestrictedOperation() thwarts that attack.
*
* Withholding self-admin in SECURITY DEFINER functions makes their
* behavior independent of the calling user. There's no security or
* SQL-standard-conformance need for that restriction, though.
*
* A role cannot have actual WITH ADMIN OPTION on itself, because that
* would imply a membership loop. Therefore, we're done either way.
*/

For non-null aclitem[], we could support REVOKE ADMIN OPTION FOR joe FROM joe, and for explicit re-grants, we could track who granted it, such that further revocations could properly refuse if the revoker doesn't have sufficient privileges vis-a-vis the role that granted it in the first place.

I have not yet tried to implement this, and might quickly hit problems with the idea, but will take a stab at a proof-of-concept patch unless you suggest a better approach.

Thoughts?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Mar 7, 2022 at 2:59 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote:

This test failure is just a manifestation of the intended change, but assuming we make no other changes, the error message would clearly need to be updated, because it suggests the role should have admin_option on itself, a situation which is not currently supported.

It's been pointed out upthread that this would have undesirable
security implications, because the admin option would be inherited,
and the implicit permission isn't.

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

On Mar 7, 2022, at 12:01 PM, Robert Haas <robertmhaas@gmail.com> wrote:

It's been pointed out upthread that this would have undesirable
security implications, because the admin option would be inherited,
and the implicit permission isn't.

Right, but with a reflexive self-admin-option, we could document that it works in a non-inherited way. We'd just be saying the current hard-coded behavior is an option which can be revoked rather than something you're stuck with.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mar 7, 2022, at 12:03 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

Right, but with a reflexive self-admin-option, we could document that it works in a non-inherited way. We'd just be saying the current hard-coded behavior is an option which can be revoked rather than something you're stuck with.

We could also say that the default is to not have admin option on yourself, with that being something grantable, but that is a larger change from the historical behavior and might have more consequences for dump/restore, etc.

My concern about just nuking self-admin is that there may be sites which use self-admin and we'd be leaving them without a simple work-around after upgrade, because they couldn't restore the behavior by executing a grant. They'd have to more fundamentally restructure their role relationships to not depend on self-admin, something which might be harder for them to do. Perhaps nobody is using self-admin, or very few people are using it, and I'm being overly concerned.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Mark Dilger <mark.dilger@enterprisedb.com> writes:

On Mar 7, 2022, at 12:01 PM, Robert Haas <robertmhaas@gmail.com> wrote:

It's been pointed out upthread that this would have undesirable
security implications, because the admin option would be inherited,
and the implicit permission isn't.

Right, but with a reflexive self-admin-option, we could document that it works in a non-inherited way. We'd just be saying the current hard-coded behavior is an option which can be revoked rather than something you're stuck with.

After reflection, I think that role self-admin is probably a bad idea that
we should stay away from. It could perhaps be reasonable given some other
system design and/or syntax than what SQL gives us, but we're dealing in
SQL. It doesn't make sense to GRANT a role to itself, and therefore it
likewise doesn't make sense to GRANT WITH ADMIN OPTION.

Based on Robert's archaeological dig, it now seems that the fact that
we have any such behavior at all was just a mistake. What would be
lost if we drop it?

Having said that, one thing that I find fishy is that it's not clear
where the admin privilege for a role originates. After "CREATE ROLE
alice", alice has no members, therefore none that have admin privilege,
therefore the only way that the first member could be added is via
superuser deus ex machina. This does not seem clean. If we recorded
which user created the role, we could act as though that user has
admin privilege (whether or not it's a member). Perhaps I'm
reinventing something that was already discussed upthread. I wonder
what the SQL spec has to say on this point, too.

regards, tom lane

On Mon, Mar 7, 2022 at 1:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Based on Robert's archaeological dig, it now seems that the fact that
we have any such behavior at all was just a mistake. What would be
lost if we drop it?

Probably nothing that couldn't be replaced, and with a better model, but I
do have a concern that there are setups in the wild inadvertently using
this behavior. Enough so that I would vote to change it but include a
migration GUC to restore the current behavior, probably with a deprecation
warning. Kinda depends on the post-change dump/restore mechanics. But
just tearing it out wouldn't seem extraordinary for us.

Having said that, one thing that I find fishy is that it's not clear
where the admin privilege for a role originates.

I do not see a problem with there being no inherent admin privilege for a
role. A superuser or CREATEROLE user holds admin privilege on all roles in
the cluster. They can delegate the privilege to administer a role to yet
another role in the system. The necessitates creating two roles - the one
being administered and the one being delegated to. I don't see a benefit
to saving which specific superuser or CREATEROLE user "owns" the role that
is to be administered. Not unless non-owner CREATEROLE users are prevented
from exercising admin privileges on the role. That all said, I'd accept
the choice to include such ownership information as a requirement for
meeting the auditing needs of DBAs. But I would argue that such auditing
probably needs to be external to the working system - the fact that
ownership can be changed reduces the benefit of an in-database value.

If we recorded

which user created the role, we could act as though that user has

admin privilege (whether or not it's a member).

I suppose we could record the current owner of a role but that seems
unnecessary. I dislike using the "created" concept by virtue of the fact
that, for routines, "security definer" implies creator but it actually
means "security owner".

David J.

On Mar 7, 2022, at 12:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

What would be
lost if we drop it?

I looked into this a bit. Removing that bit of code, the only regression test changes for "check-world" are the expected ones, with nothing else breaking. Running installcheck+pg_upgrade to the patched version of HEAD from each of versions 11, 12, 13 and 14 doesn't turn up anything untoward. The change I used (for reference) is attached:

On 07.03.22 19:18, Robert Haas wrote:

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.

Do we have subtractive permissions today?

On Wed, Mar 9, 2022 at 7:55 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

Do we have subtractive permissions today?

Not in the GRANT/REVOKE sense, I think, but you can put a user in a
group and then mention that group in pg_hba.conf. And that line might
be "reject" or whatever.

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

On Mon, Mar 7, 2022 at 11:14 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Mar 7, 2022, at 12:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
What would be
lost if we drop it?

I looked into this a bit. Removing that bit of code, the only regression test changes for "check-world" are the expected ones, with nothing else breaking. Running installcheck+pg_upgrade to the patched version of HEAD from each of versions 11, 12, 13 and 14 doesn't turn up anything untoward.

I looked into this a bit, too. I attach a draft patch for removing the
self-admin exception.

I found that having is_admin_of_role() return true matters in three
ways: (1) It lets you grant membership in the role to some other role.
(2) It lets you revoke membership in the role from some other role.
(3) It changes the return value of pg_role_aclcheck(), which is used
in the implementation of various SQL-callable functions all invoked
via the name pg_has_role(). We've mostly been discussing (2) as an
issue, but (1) and (3) are pretty interesting too. Regarding (3),
there is a comment in the code indicating that Noah considered the
self-admin exception something of a wart as far as pg_has_role() is
concerned. As to (1), I discovered that today you can do this:

rhaas=# create user foo;
CREATE ROLE
rhaas=# create user bar;
CREATE ROLE
rhaas=# \q
[rhaas ~]$ psql -U foo rhaas
psql (15devel)
Type "help" for help.

rhaas=> grant foo to bar with admin option;
GRANT ROLE

I don't know why I didn't realize that before. It's a natural result
of treating the logged-in user as if they had admin option. But it's
weird that you can't even be granted WITH ADMIN OPTION on your own
login role, but at the same time without having it you can grant it to
someone else!

I believe there are three other points worth some consideration here.

First, in the course of my investigation I re-discovered what Tom
already did a good job articulating:

tgl> Having said that, one thing that I find fishy is that it's not clear
tgl> where the admin privilege for a role originates. After "CREATE ROLE
tgl> alice", alice has no members, therefore none that have admin privilege,
tgl> therefore the only way that the first member could be added is via
tgl> superuser deus ex machina. This does not seem clean.

I agree with that, but I don't think it's a sufficient reason for
keeping the self-admin exception, because the same problem exists for
non-login roles. I don't even think it's the right idea conceptually
to suppose that the power to administer a role originates from the
role itself. If that were so, then it would be inherited by all
members of the role along with all the rest of the role's privileges,
which is so clearly not right that we've already prohibited a role
from having WITH ADMIN OPTION on itself. In my opinion, the right to
administer a role - regardless of whether or not it is a login role -
most naturally vests in the role that created it, or something in that
direction at least, if not that exact thing. Today, that means the
superuser or a CREATEROLE user who could hack superuser if they
wished. In the future, I hope for other alternatives, as recently
argued on other threads. But we need not resolve the question of how
that should work exactly in order to agree (as I hope we do) that
doubling down on the self-administration exception is not the answer.

Second, it occured to me to wonder what implications a change like
this might have for dump and restore. If privilege restoration somehow
relied on this behavior, then we'd have a problem. But I don't think
it does, because (a) pg_dump can SET ROLE but can't change the session
user without reconnecting, so it's unclear how we could be relying on
it; (b) it wouldn't work for non-login roles, and it's unlikely that
we would treat login and non-login roles different in terms of
restoring privileges, and (c) when I execute the example shown above
and then run pg_dump, there's no attempt to change the current user,
it just dumps "GRANT foo TO bar WITH ADMIN OPTION GRANTED BY foo".

Third, it occurred to me to wonder whether some users might be using
and relying upon this behavior. It's certainly possible, and it does
suck that we'd be removing it without providing a workable substitute.
But it's probably not a LOT of users because most people who have
commented on this topic on this mailing list seem to find granting
membership in a login role a super-weird thing to do, because a lot of
people really seem to want every role to be a user or a group, and a
login role with members feels like it's blurring that line. I'm
inclined to think that the small number of people who may be unhappy
is an acceptable price to pay for removing this wart, but it's a
judgement call and if someone has information to suggest that I'm
wrong, it'd be good to hear about that.

Thanks,

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

Robert Haas <robertmhaas@gmail.com> writes:

On Mar 7, 2022, at 12:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
tgl> Having said that, one thing that I find fishy is that it's not clear
tgl> where the admin privilege for a role originates. After "CREATE ROLE
tgl> alice", alice has no members, therefore none that have admin privilege,
tgl> therefore the only way that the first member could be added is via
tgl> superuser deus ex machina. This does not seem clean.

I agree with that, but I don't think it's a sufficient reason for
keeping the self-admin exception, because the same problem exists for
non-login roles. I don't even think it's the right idea conceptually
to suppose that the power to administer a role originates from the
role itself.

Actually, that's the same thing I was trying to say. But if it doesn't
originate from the role itself, where does it originate from?

In my opinion, the right to
administer a role - regardless of whether or not it is a login role -
most naturally vests in the role that created it, or something in that
direction at least, if not that exact thing.

This seems like a reasonable answer to me too: the creating role has admin
option implicitly, and can then choose to grant that to other roles.
Obviously some work needs to be done to make that happen (and we should
see whether the SQL spec has some different idea).

regards, tom lane

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

In my opinion, the right to
administer a role - regardless of whether or not it is a login role -
most naturally vests in the role that created it, or something in that
direction at least, if not that exact thing.

This seems like a reasonable answer to me too: the creating role has admin
option implicitly, and can then choose to grant that to other roles.
Obviously some work needs to be done to make that happen (and we should
see whether the SQL spec has some different idea).

Well, the problem is that as far as I can see, the admin option is an
optional feature of membership. You can grant someone membership
without admin option, or with admin option, but you can't grant them
the admin option without membership, just like you can't purchase an
upgrade to first class without the underlying plane ticket. What would
the syntax look even like for this? GRANT foo TO bar WITH ADMIN OPTION
BUT WITHOUT MEMBERSHIP? Yikes.

But do we really have to solve this problem before we can clean up
this session exception? I hope not, because I think that's a much
bigger can of worms than this is.

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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Mar 7, 2022, at 12:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
tgl> Having said that, one thing that I find fishy is that it's not clear
tgl> where the admin privilege for a role originates. After "CREATE ROLE
tgl> alice", alice has no members, therefore none that have admin privilege,
tgl> therefore the only way that the first member could be added is via
tgl> superuser deus ex machina. This does not seem clean.

I agree with that, but I don't think it's a sufficient reason for
keeping the self-admin exception, because the same problem exists for
non-login roles. I don't even think it's the right idea conceptually
to suppose that the power to administer a role originates from the
role itself.

Actually, that's the same thing I was trying to say. But if it doesn't
originate from the role itself, where does it originate from?

In my opinion, the right to
administer a role - regardless of whether or not it is a login role -
most naturally vests in the role that created it, or something in that
direction at least, if not that exact thing.

This seems like a reasonable answer to me too: the creating role has admin
option implicitly, and can then choose to grant that to other roles.

I agree that this has some appeal, but it's not desirable in all cases
and so I wouldn't want it to be fully baked into the system ala the role
'owner' concept.

Obviously some work needs to be done to make that happen (and we should
see whether the SQL spec has some different idea).

Agreed on this, though I don't recall it having much to say on it.

Thanks,

Stephen

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:

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

In my opinion, the right to
administer a role - regardless of whether or not it is a login role -
most naturally vests in the role that created it, or something in that
direction at least, if not that exact thing.

This seems like a reasonable answer to me too: the creating role has admin
option implicitly, and can then choose to grant that to other roles.
Obviously some work needs to be done to make that happen (and we should
see whether the SQL spec has some different idea).

Well, the problem is that as far as I can see, the admin option is an
optional feature of membership. You can grant someone membership
without admin option, or with admin option, but you can't grant them
the admin option without membership, just like you can't purchase an
upgrade to first class without the underlying plane ticket. What would
the syntax look even like for this? GRANT foo TO bar WITH ADMIN OPTION
BUT WITHOUT MEMBERSHIP? Yikes.

I've been meaning to reply to your other email regarding this, but I
don't really agree that the syntax ends up being so terrible or
difficult to deal with, considering we have these same general things
for ALTER ROLE already and there hasn't been all that much complaining.
That is, we have LOGIN and NOLOGIN, CREATEROLE and NOCREATEROLE, and we
could have MEMBERSHIP and NOMEMBERSHIP pretty easily here if we wanted
to.

But do we really have to solve this problem before we can clean up
this session exception? I hope not, because I think that's a much
bigger can of worms than this is.

I do believe we can deal with the above independently and at a later
time and go ahead and clean up the session excepton bit without dealing
with the above at the same time.

Thanks,

Stephen

I wrote:

This seems like a reasonable answer to me too: the creating role has admin
option implicitly, and can then choose to grant that to other roles.
Obviously some work needs to be done to make that happen (and we should
see whether the SQL spec has some different idea).

Ah, here we go: it's buried under CREATE ROLE. SQL:2021 12.4 <role
definition> saith that when role A executes CREATE ROLE <role name>,
then

1) A grantable role authorization descriptor is created whose role name
is <role name>, whose grantor is "_SYSTEM", and whose grantee is A.

Since nobody is _SYSTEM, this grant can't be deleted except by dropping
the new role (or, maybe, dropping A?). So that has nearly the same
end result as "the creating role has admin option implicitly". The main
difference I can see is that it also means the creating role is a *member*
implicitly, which is something I'd argue we don't want to enforce. This
is analogous to the way we let an object owner revoke her own ordinary
permissions, which the SQL model doesn't allow since those permissions
were granted to her by _SYSTEM.

regards, tom lane

Robert Haas <robertmhaas@gmail.com> writes:

Well, the problem is that as far as I can see, the admin option is an
optional feature of membership. You can grant someone membership
without admin option, or with admin option, but you can't grant them
the admin option without membership, just like you can't purchase an
upgrade to first class without the underlying plane ticket. What would
the syntax look even like for this? GRANT foo TO bar WITH ADMIN OPTION
BUT WITHOUT MEMBERSHIP? Yikes.

I don't think we need syntax to describe it. As I just said in my
other reply, we have a perfectly good precedent for this already
in ordinary object permissions. That is: an object owner always,
implicitly, has GRANT OPTION for all the object's privileges, even
if she revoked the corresponding plain privilege from herself.

Yeah, this does mean that we're effectively deciding that the creator
of a role is its owner. What's the problem with that?

But do we really have to solve this problem before we can clean up
this session exception?

I think we need a plan for where we're going. I don't see "clean up
the session exception" as an end in itself; it's part of re-examining
how all of this ought to work. I don't say that we have to have a
complete patch right away, only that we need a coherent end goal.

regards, tom lane

On Wed, Mar 9, 2022 at 2:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Well, the problem is that as far as I can see, the admin option is an
optional feature of membership. You can grant someone membership
without admin option, or with admin option, but you can't grant them
the admin option without membership, just like you can't purchase an
upgrade to first class without the underlying plane ticket. What would
the syntax look even like for this? GRANT foo TO bar WITH ADMIN OPTION
BUT WITHOUT MEMBERSHIP? Yikes.

I don't think we need syntax to describe it. As I just said in my
other reply, we have a perfectly good precedent for this already
in ordinary object permissions. That is: an object owner always,
implicitly, has GRANT OPTION for all the object's privileges, even
if she revoked the corresponding plain privilege from herself.

So CREATE ROLE will assign ownership of AND membership in the newly created
role to the session_user UNLESS the OWNER clause is present in which case
the named role, so long as the session_user can SET ROLE to the named role,
becomes the owner & member. Subsequent to that the owner can issue: REVOKE
new_role FROM role_name where role_name is again the session_user role or
one that can be SET ROLE to.

Yeah, this does mean that we're effectively deciding that the creator
of a role is its owner. What's the problem with that?

I'm fine with this. It does introduce an OWNER concept to roles and so at
minimum we need to add:

ALTER ROLE foo OWNER TO { new_owner | CURRENT_ROLE | CURRENT_USER |
SESSION_USER }

And similar for CREATE ROLE
And keep the USER alias commands in sync.
GROUP commands are only present for backward compatibility and so don't get
updated with new features by design.

Obviously a superuser can change ownership.

Playing with table ownership I find this behavior:
-- superuser
CREATE ROLE tblowner;
CREATE TABLE tblowner_test (id serial primary key);
ALTER TABLE tblowner_test OWNER TO tblowner;

CREATE ROLE boss;
GRANT boss TO tblowner;

SET SESSION AUTHORIZATION tblowner;
ALTER TABLE tblowner_test OWNER TO boss; --works

So tblowner can push their ownership attribute to any group they are a
member of. Is that the behavior we want for roles as well?

David J.

"David G. Johnston" <david.g.johnston@gmail.com> writes:

So CREATE ROLE will assign ownership of AND membership in the newly created
role to the session_user

I would NOT have it automatically assign membership in the new role,
even though the SQL spec says so. We've not done that historically
and it doesn't seem desirable. In particular, it's *really* not
desirable for a user (role with LOGIN).

I'm fine with this. It does introduce an OWNER concept to roles and so at
minimum we need to add:
ALTER ROLE foo OWNER TO { new_owner | CURRENT_ROLE | CURRENT_USER |
SESSION_USER }

Agreed.

regards, tom lane

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

I don't think we need syntax to describe it. As I just said in my
other reply, we have a perfectly good precedent for this already
in ordinary object permissions. That is: an object owner always,
implicitly, has GRANT OPTION for all the object's privileges, even
if she revoked the corresponding plain privilege from herself.

Yeah, this does mean that we're effectively deciding that the creator
of a role is its owner. What's the problem with that?

I don't think that's entirely the wrong concept, but it doesn't make a
lot of sense in a world where the creator has to be a superuser. If
alice, bob, and charlie are superusers who take turns creating new
users, and then we let charlie go due to budget cuts, forcing alice
and bob to change the owner of all the users he created to some other
superuser as a condition of dropping his account is a waste of
everyone's time. They can do exactly the same things to every account
on the system after we change the role owner as before.

But wait, I hear you cry, what about CREATEROLE? Well, CREATEROLE is
generally agreed to be broken right now, and if you don't agree with
that, consider that it can grant pg_execute_server_programs to a
newly-created account and then explain to me how it's functionally
different from superuser. The whole area needs a rethink. I believe
everyone involved in the discussion on the other threads agrees that
some reform of CREATEROLE is necessary, and more generally with the
idea that it's useful for non-superusers to be able to create roles.
But the reasons why people want that vary.

I want that because I want mini-superusers, where alice can administer
the users that alice creates just as if she were a superuser,
including having their permissions implicitly and dropping them when
she wants them gone, but where alice cannot break out to the operating
system as a true superuser could do. I want this because the lack of
meaningful privilege separation that led to CVE-2019-9193 being filed
spuriously is a very real problem. It's a thing a lot of people want,
and I want to give it to them. David Steele, on the other hand, wants
to build a user-creating bot that can create accounts but otherwise
conforms to the principle of least privilege: the bot can stand up
accounts, can grant them membership in a defined set of groups, but
cannot exercise the privileges of those accounts (or hack superuser
either). Other people may well want other things.

And that's why I'm not sure it's really the right idea to say that we
don't need syntax for this admin-without-member concept. If we just
want to bolt role ownership onto the existing framework without really
changing anything else, we can do that without extra syntax and, as
you say here, make it an implicit property of role ownership. But I
don't see that as has having much value; we just end up with a bunch
of superuser owners. Whatever. Now Stephen made the argument that we
ought to actually have admin-without-member as a first class concept,
something that could be assigned to arbitrary users. Actually, I think
he wanted it even more fine grained with that. And I think that could
make the concept a lot more useful, but then it needs some kind of
understandable syntax.

There's a lot of moving parts here. It's not just about coming up with
something that sounds generally logical, but about creating a system
that has some real-world utility.

But do we really have to solve this problem before we can clean up
this session exception?

I think we need a plan for where we're going. I don't see "clean up
the session exception" as an end in itself; it's part of re-examining
how all of this ought to work. I don't say that we have to have a
complete patch right away, only that we need a coherent end goal.

I'd like to have a plan, too, but if this behavior is accidental, I
still think we can remove it without making big decisions about future
direction. The perfect is the enemy of the good.

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

On Thu, Mar 10, 2022 at 7:46 AM Robert Haas <robertmhaas@gmail.com> wrote:

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

I don't think we need syntax to describe it. As I just said in my
other reply, we have a perfectly good precedent for this already
in ordinary object permissions. That is: an object owner always,
implicitly, has GRANT OPTION for all the object's privileges, even
if she revoked the corresponding plain privilege from herself.

Yeah, this does mean that we're effectively deciding that the creator
of a role is its owner. What's the problem with that?

I don't think that's entirely the wrong concept, but it doesn't make a
lot of sense in a world where the creator has to be a superuser. If
alice, bob, and charlie are superusers who take turns creating new
users, and then we let charlie go due to budget cuts, forcing alice
and bob to change the owner of all the users he created to some other
superuser as a condition of dropping his account is a waste of
everyone's time. They can do exactly the same things to every account
on the system after we change the role owner as before.

Then maybe we should just implement the idea that if a superuser would
become the owner we instead substitute in the bootstrap user. Or give the
DBA the choice whether they want to retain knowledge of specific roles -
and thus are willing to accept the "waste of time".

But wait, I hear you cry, what about CREATEROLE? Well, CREATEROLE is
generally agreed to be broken right now, and if you don't agree with
that, consider that it can grant pg_execute_server_programs to a
newly-created account and then explain to me how it's functionally
different from superuser.

CREATEROLE has long been defined as basically having "with admin option" on
every role in the system. The failure to special-case the roles that grant
different aspects of superuser-ness to its members doesn't make CREATEROLE
itself broken, it makes the implementation of pg_execute_server_programs
broken. Only superusers should be considered to have with admin option on
these roles. They can delegate through the usual membership+admin mechanism
to a CREATEROLE role if they desire.

The whole area needs a rethink. I believe
everyone involved in the discussion on the other threads agrees that
some reform of CREATEROLE is necessary, and more generally with the
idea that it's useful for non-superusers to be able to create roles.

As the documentation says, using SUPERUSER for day-to-day administration is
contrary to good security practices. Role management is considered to be a
day-to-day administration activity. I agree with this principle. It was
designed to neither be a superuser nor grant superuser, so removing the
ability to grant the pg_* role memberships remains consistent with its
original intent.

I want that because I want mini-superusers, where alice can administer
the users that alice creates just as if she were a superuser,
including having their permissions implicitly and dropping them when
she wants them gone, but where alice cannot break out to the operating
system as a true superuser could do.

CREATEROLE (once the pg_* with admin rules are fixed) + Ownership and rules
restricting interfering with another role's objects (unless superuser)
seems to handle this.

the bot can stand up
accounts, can grant them membership in a defined set of groups, but
cannot exercise the privileges of those accounts (or hack superuser
either).

The bot should be provided a security definer procedure that encapsulates
all of this rather than us trying to hack the permission system. This
isn't a user permission concern, it is an unauthorized privilege escalation
concern. Anyone with the bot's credentials can trivially overcome the
third restriction by creating a role with the desired membership and then
logging in as that role - and there is nothing the system can do to prevent
that while also allowing the other two permissions.

And that's why I'm not sure it's really the right idea to say that we
don't need syntax for this admin-without-member concept.

We already have this syntax in the form of CREATEROLE. But we do need a
fix, just on the group side. We need a way to define a group as having no
ADMINS.

ALTER ROLE pg_superuser WITH [NO] ADMIN;

Then adding a role membership including the WITH ADMIN OPTION can be
rejected, as can the non-superuser situation. Setting WITH NO ADMIN should
fail if any existing members have admin. You must be a superuser to
execute WITH ADMIN (maybe WITH NO ADMIN as well...). And possibly even a
new pg_* role that grants this ability (and maybe some others) for use by a
backup/restore user.

Or just special-case pg_* roles.

The advantage of exposing this to the DBA is that they can then package
pg_* roles into a custom group and still have the benefit of superuser only
administration. In the special-case implementation the presence of a pg_*
role in a group hierarchy would then preclude a non-superuser from having
admin on the entire tree (the pg_* roles are all roots, or in the case of
pg_monitor, directly emanate from a root role).

David J.

David J.

Greetings,

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Thu, Mar 10, 2022 at 7:46 AM Robert Haas <robertmhaas@gmail.com> wrote:

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

I don't think we need syntax to describe it. As I just said in my
other reply, we have a perfectly good precedent for this already
in ordinary object permissions. That is: an object owner always,
implicitly, has GRANT OPTION for all the object's privileges, even
if she revoked the corresponding plain privilege from herself.

Yeah, this does mean that we're effectively deciding that the creator
of a role is its owner. What's the problem with that?

I don't think that's entirely the wrong concept, but it doesn't make a
lot of sense in a world where the creator has to be a superuser. If
alice, bob, and charlie are superusers who take turns creating new
users, and then we let charlie go due to budget cuts, forcing alice
and bob to change the owner of all the users he created to some other
superuser as a condition of dropping his account is a waste of
everyone's time. They can do exactly the same things to every account
on the system after we change the role owner as before.

Then maybe we should just implement the idea that if a superuser would
become the owner we instead substitute in the bootstrap user. Or give the
DBA the choice whether they want to retain knowledge of specific roles -
and thus are willing to accept the "waste of time".

This doesn't strike me as going in the right direction. Falling back to
the bootstrap superuser is generally a hack and not a great one. I'll
also point out that the SQL spec hasn't got a concept of role ownership
either.

But wait, I hear you cry, what about CREATEROLE? Well, CREATEROLE is
generally agreed to be broken right now, and if you don't agree with
that, consider that it can grant pg_execute_server_programs to a
newly-created account and then explain to me how it's functionally
different from superuser.

CREATEROLE has long been defined as basically having "with admin option" on
every role in the system. The failure to special-case the roles that grant
different aspects of superuser-ness to its members doesn't make CREATEROLE
itself broken, it makes the implementation of pg_execute_server_programs
broken. Only superusers should be considered to have with admin option on
these roles. They can delegate through the usual membership+admin mechanism
to a CREATEROLE role if they desire.

No, CREATEROLE having admin option on every role in the system is broken
and always has been. It's not just an issue for predefined roles like
pg_execute_server_program, it's an issue for any role that could become
a superuser either directly or indirectly and that extends beyond the
predefined ones. As this issue with CREATEROLE existed way before
predefined roles were added to PG, claiming that it's an issue with
predefined roles doesn't make a bit of sense.

The whole area needs a rethink. I believe
everyone involved in the discussion on the other threads agrees that
some reform of CREATEROLE is necessary, and more generally with the
idea that it's useful for non-superusers to be able to create roles.

As the documentation says, using SUPERUSER for day-to-day administration is
contrary to good security practices. Role management is considered to be a
day-to-day administration activity. I agree with this principle. It was
designed to neither be a superuser nor grant superuser, so removing the
ability to grant the pg_* role memberships remains consistent with its
original intent.

That would not be sufficient to make CREATEROLE safe. Far, far from it.

I want that because I want mini-superusers, where alice can administer
the users that alice creates just as if she were a superuser,
including having their permissions implicitly and dropping them when
she wants them gone, but where alice cannot break out to the operating
system as a true superuser could do.

CREATEROLE (once the pg_* with admin rules are fixed) + Ownership and rules
restricting interfering with another role's objects (unless superuser)
seems to handle this.

This is not sufficient- roles can be not-superuser themselves but have
the ability to become superuser if GRANT'd a superuser role and
therefore we can't have a system where CREATEROLE allows arbitrary
GRANT'ing of roles to each other. I'm a bit confused too as anything
where we are curtailing what CREATEROLE roles are able to do in a manner
that means they're only able to modify some subset of roles should
equally apply to predefined roles too- that is, CREATEROLE shouldn't be
the determining factor in the question of if a role can GRANT a
predefined (or any other role) to some other role- that should be
governed by the admin option on that role, and that should work exactly
the same for predefined roles as it does for any other.

I disagree that ownership is needed that's not what the spec calls for
either. What we need is more flexibility when it comes to the
relationships which are allowed to be created between roles and what
privileges come with them. To that end, I'd argue that we should be
extending pg_auth_members, first by separating out membership itself
into an explicitly tracked attribute (instead of being implicit in the
existance of a row in the table) and then adding on what other
privileges we see fit to add, such as the ability to DROP a role. We
do need to remove the ability for a role who hasn't been explicitly
given the admin right on another role to modify that role's membership
too, as was originally proposed here. This also seems to more closely
follow the spec's expectation, something that role ownership doesn't.

the bot can stand up
accounts, can grant them membership in a defined set of groups, but
cannot exercise the privileges of those accounts (or hack superuser
either).

The bot should be provided a security definer procedure that encapsulates
all of this rather than us trying to hack the permission system. This
isn't a user permission concern, it is an unauthorized privilege escalation
concern. Anyone with the bot's credentials can trivially overcome the
third restriction by creating a role with the desired membership and then
logging in as that role - and there is nothing the system can do to prevent
that while also allowing the other two permissions.

Falling back to security definer functions may be one approach but it's
not a great one and it only works if it's possible to end up with the
catalogs having what is actually desired- for example, ADMIN option
without membership isn't something the catalogs today can understand
because existance in pg_auth_members implies membership and you can't
have ADMIN without having that row. The same issue would exist with
ownership if ownership implied the same- that's not improving things.

And that's why I'm not sure it's really the right idea to say that we
don't need syntax for this admin-without-member concept.

We already have this syntax in the form of CREATEROLE. But we do need a
fix, just on the group side. We need a way to define a group as having no
ADMINS.

We don't have this syntax today nor do we have a way to store such a
concept in the catalogs either, so I'm pretty baffled by this. Defining
a group without admins is, in fact, what we actually have support for
today in the catalogs- it's just a case where there aren't any rows in
pg_auth_members which have 'admin_option' as true. The opposite is what
we're talking about here- rows which have 'admin_option' as true but
don't have membership, and that can't be the case today because
existance in the table itself implies membership.

ALTER ROLE pg_superuser WITH [NO] ADMIN;

Then adding a role membership including the WITH ADMIN OPTION can be
rejected, as can the non-superuser situation. Setting WITH NO ADMIN should
fail if any existing members have admin. You must be a superuser to
execute WITH ADMIN (maybe WITH NO ADMIN as well...). And possibly even a
new pg_* role that grants this ability (and maybe some others) for use by a
backup/restore user.

I'm not following this in general or how it helps. Surely we don't want
to limit WITH ADMIN to superusers. As for if we should migrate
CREATEROLE to a new predefined role, maybe, but that seems like a
different question.

Or just special-case pg_* roles.

As I hopefully made clear above, this isn't actually a solution, nor do
pg_* roles need to be treated somehow differently in this aspect.

The advantage of exposing this to the DBA is that they can then package
pg_* roles into a custom group and still have the benefit of superuser only
administration. In the special-case implementation the presence of a pg_*
role in a group hierarchy would then preclude a non-superuser from having
admin on the entire tree (the pg_* roles are all roots, or in the case of
pg_monitor, directly emanate from a root role).

We are very much trying to move away from 'superuser only
administration'.

Thanks,

Stephen

On Mar 10, 2022, at 7:56 AM, David G. Johnston <david.g.johnston@gmail.com> wrote:

I want that because I want mini-superusers, where alice can administer
the users that alice creates just as if she were a superuser,
including having their permissions implicitly and dropping them when
she wants them gone, but where alice cannot break out to the operating
system as a true superuser could do.

CREATEROLE (once the pg_* with admin rules are fixed) + Ownership and rules restricting interfering with another role's objects (unless superuser) seems to handle this.

What if one of alice's subordinates also owns roles? Can alice interfere with *that* role's objects? I don't see that a simple rule restricting roles from interfering with another role's objects is quite enough. That raises the question of whether role ownership is transitive, and whether we need a concept similar to inherit/noinherit for ownership.

There is also the problem that CREATEROLE currently allows a set of privileges to be granted to created roles, and that set of privileges is hard-coded. You've suggested changing the hard-coded rules to remove pg_* roles from the list of grantable privileges, but that's still an inflexible set of hardcoded privileges. Wouldn't it make more sense for the grantor to need GRANT OPTION on any privilege they give to roles they create?

the bot can stand up
accounts, can grant them membership in a defined set of groups, but
cannot exercise the privileges of those accounts (or hack superuser
either).

The bot should be provided a security definer procedure that encapsulates all of this rather than us trying to hack the permission system. This isn't a user permission concern, it is an unauthorized privilege escalation concern. Anyone with the bot's credentials can trivially overcome the third restriction by creating a role with the desired membership and then logging in as that role - and there is nothing the system can do to prevent that while also allowing the other two permissions.

Doesn't this assume password authentication? If the server uses ldap authentication, for example, wouldn't the bot need valid ldap credentials for at least one user for this attack to work? And if CREATEROLE has been made more configurable, wouldn't the bot only be able to grant that ldap user the limited set of privileges that the bot's database user has been granted ADMIN OPTION for?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Mar 10, 2022 at 11:19 AM Stephen Frost <sfrost@snowman.net> wrote:

I disagree that ownership is needed that's not what the spec calls for
either. What we need is more flexibility when it comes to the
relationships which are allowed to be created between roles and what
privileges come with them. To that end, I'd argue that we should be
extending pg_auth_members, first by separating out membership itself
into an explicitly tracked attribute (instead of being implicit in the
existance of a row in the table) and then adding on what other
privileges we see fit to add, such as the ability to DROP a role. We
do need to remove the ability for a role who hasn't been explicitly
given the admin right on another role to modify that role's membership
too, as was originally proposed here. This also seems to more closely
follow the spec's expectation, something that role ownership doesn't.

I do not have a problem with more fine-grained kinds of authorization
even though I think there are syntactic issues to work out, but I
strongly disagree with the idea that we can't or shouldn't also have
role ownership. Marc invented it. Now Tom has invented it
independently. All sorts of other objects have it already. Trying to
make it out like this is some kind of kooky idea is not believable.
Yeah, it's not the most sophisticated or elegant model and that's why
it's good for us to also have other things, but for simple cases it is
easy to understand and works great.

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

On Thu, Mar 10, 2022 at 9:19 AM Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Thu, Mar 10, 2022 at 7:46 AM Robert Haas <robertmhaas@gmail.com>

wrote:

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

I don't think we need syntax to describe it. As I just said in my
other reply, we have a perfectly good precedent for this already
in ordinary object permissions. That is: an object owner always,
implicitly, has GRANT OPTION for all the object's privileges, even
if she revoked the corresponding plain privilege from herself.

Yeah, this does mean that we're effectively deciding that the creator
of a role is its owner. What's the problem with that?

I don't think that's entirely the wrong concept, but it doesn't make a
lot of sense in a world where the creator has to be a superuser. If
alice, bob, and charlie are superusers who take turns creating new
users, and then we let charlie go due to budget cuts, forcing alice
and bob to change the owner of all the users he created to some other
superuser as a condition of dropping his account is a waste of
everyone's time. They can do exactly the same things to every account
on the system after we change the role owner as before.

Then maybe we should just implement the idea that if a superuser would
become the owner we instead substitute in the bootstrap user. Or give

the

DBA the choice whether they want to retain knowledge of specific roles -
and thus are willing to accept the "waste of time".

This doesn't strike me as going in the right direction. Falling back to
the bootstrap superuser is generally a hack and not a great one. I'll
also point out that the SQL spec hasn't got a concept of role ownership
either.

But wait, I hear you cry, what about CREATEROLE? Well, CREATEROLE is
generally agreed to be broken right now, and if you don't agree with
that, consider that it can grant pg_execute_server_programs to a
newly-created account and then explain to me how it's functionally
different from superuser.

CREATEROLE has long been defined as basically having "with admin option"

on

every role in the system. The failure to special-case the roles that

grant

different aspects of superuser-ness to its members doesn't make

CREATEROLE

itself broken, it makes the implementation of pg_execute_server_programs
broken. Only superusers should be considered to have with admin option

on

these roles. They can delegate through the usual membership+admin

mechanism

to a CREATEROLE role if they desire.

No, CREATEROLE having admin option on every role in the system is broken
and always has been. It's not just an issue for predefined roles like
pg_execute_server_program,

it's an issue for any role that could become
a superuser either directly or indirectly and that extends beyond the
predefined ones.

The only indirect way for a role to become superuser is to have been
granted membership in a superuser group, then SET ROLE. Non-superusers
cannot do this. If a superuser does this I consider the outcome to be no
different than if they go and do:

SET allow_system_table_mods TO true;
DROP pg_catalog.pg_class;

In short, having a CREATEROLE user issuing:
GRANT pg_read_all_stats TO davidj;
should result in the same outcome as them issuing:
GRANT postgres TO davidj;
-- ERROR: must be superuser to alter superusers

Superusers can break their system and we don't go to great effort to stop
them. I see no difference here, so arguments of this nature aren't all
that compelling to me.

CREATEROLE shouldn't be

the determining factor in the question of if a role can GRANT a
predefined (or any other role) to some other role- that should be
governed by the admin option on that role, and that should work exactly
the same for predefined roles as it does for any other.

Never granting the CREATEROLE attribute to anyone will give you this
outcome today.

ADMIN option
without membership isn't something the catalogs today can understand

Today, they don't need to in order for the system to function within its
existing design specs.

ALTER ROLE pg_superuser WITH [NO] ADMIN;

Then adding a role membership including the WITH ADMIN OPTION can be
rejected, as can the non-superuser situation. Setting WITH NO ADMIN

should

fail if any existing members have admin. You must be a superuser to
execute WITH ADMIN (maybe WITH NO ADMIN as well...). And possibly even a
new pg_* role that grants this ability (and maybe some others) for use

by a

backup/restore user.

I'm not following this in general or how it helps. Surely we don't want
to limit WITH ADMIN to superusers.

Today a non-superuser cannot "grant postgres to someuser;"

The point of this attribute is to allow the superuser to apply that rule to
other roles that aren't superuser. In particular, the predefined pg_*
roles. But it could extend to any other role the superuser would like to
limit. It means, for that for named role, ADMIN privileges cannot be
delegated to other roles - thus all administration of that role's
membership roster must happen by a superuser.

In particular, this means CREATEROLE roles cannot assign membership in the
marked roles; just like they cannot assign membership in superuser roles
today.

For me, because the SUPERUSER cannot have its role become a group without a
superuser making that choice, and by default the default pg_* roles will
all have this property as well, and any newly superuser created roles that
may be members of either superuser or pg_* can have the property defined as
well, gives full control to the superuser as to how superuser abilities are
doled out and so the design itself allows for what many of you are
considering to be "safe usage". That "unsafe configurations" are possible
is due to the policy that superusers are unrestricted in what they can do,
including making unsafe and destructive choices.

In short, removing the self-administration rule solves the "login roles
should not be automatically considered groups administered by themselves"
problem - or at least a feature we really don't need.
And defining a "superuser administration only" attribute to a role solves
the indirect superuser privileges and assignment thereof by non-superusers
problem.

I can see value in adding a feature whereby we allow the DBA to define a
group as a schema-like container and then assign roles to that group with a
fine-grained permissions model. My take is this proposal is a new feature
while the two problems noted above can be solved more readily and with less
risk with the two suggested changes.

David J.

On Thu, Mar 10, 2022 at 12:11 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 10, 2022 at 11:19 AM Stephen Frost <sfrost@snowman.net> wrote:

I disagree that ownership is needed that's not what the spec calls for
either. What we need is more flexibility when it comes to the
relationships which are allowed to be created between roles and what
privileges come with them. To that end, I'd argue that we should be
extending pg_auth_members, first by separating out membership itself
into an explicitly tracked attribute (instead of being implicit in the
existance of a row in the table) and then adding on what other
privileges we see fit to add, such as the ability to DROP a role. We
do need to remove the ability for a role who hasn't been explicitly
given the admin right on another role to modify that role's membership
too, as was originally proposed here. This also seems to more closely
follow the spec's expectation, something that role ownership doesn't.

I do not have a problem with more fine-grained kinds of authorization
even though I think there are syntactic issues to work out, but I
strongly disagree with the idea that we can't or shouldn't also have
role ownership. Marc invented it. Now Tom has invented it
independently. All sorts of other objects have it already. Trying to
make it out like this is some kind of kooky idea is not believable.
Yeah, it's not the most sophisticated or elegant model and that's why
it's good for us to also have other things, but for simple cases it is
easy to understand and works great.

Ownership implies DAC, the ability to grant others rights to an
object. It's not "kooky" to see roles as owned objects, but it isn't
required either. For example most objects on a UNIX system are owned
and subject to DAC but users aren't.

Stephen's, and now my, issue with ownership is that, since it implies
DAC, most checks will be bypassed for the owner. We would both prefer
for everyone to be subject to the grants, including whoever created
the role.

Rather, we'd like to see a "creators of roles get this set of grants
against the role by default" and "as a superuser I can revoke grants
from creators against roles they created"

Greetings,

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Thu, Mar 10, 2022 at 9:19 AM Stephen Frost <sfrost@snowman.net> wrote:

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Thu, Mar 10, 2022 at 7:46 AM Robert Haas <robertmhaas@gmail.com>

wrote:

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

I don't think we need syntax to describe it. As I just said in my
other reply, we have a perfectly good precedent for this already
in ordinary object permissions. That is: an object owner always,
implicitly, has GRANT OPTION for all the object's privileges, even
if she revoked the corresponding plain privilege from herself.

Yeah, this does mean that we're effectively deciding that the creator
of a role is its owner. What's the problem with that?

I don't think that's entirely the wrong concept, but it doesn't make a
lot of sense in a world where the creator has to be a superuser. If
alice, bob, and charlie are superusers who take turns creating new
users, and then we let charlie go due to budget cuts, forcing alice
and bob to change the owner of all the users he created to some other
superuser as a condition of dropping his account is a waste of
everyone's time. They can do exactly the same things to every account
on the system after we change the role owner as before.

Then maybe we should just implement the idea that if a superuser would
become the owner we instead substitute in the bootstrap user. Or give

the

DBA the choice whether they want to retain knowledge of specific roles -
and thus are willing to accept the "waste of time".

This doesn't strike me as going in the right direction. Falling back to
the bootstrap superuser is generally a hack and not a great one. I'll
also point out that the SQL spec hasn't got a concept of role ownership
either.

But wait, I hear you cry, what about CREATEROLE? Well, CREATEROLE is
generally agreed to be broken right now, and if you don't agree with
that, consider that it can grant pg_execute_server_programs to a
newly-created account and then explain to me how it's functionally
different from superuser.

CREATEROLE has long been defined as basically having "with admin option"

on

every role in the system. The failure to special-case the roles that

grant

different aspects of superuser-ness to its members doesn't make

CREATEROLE

itself broken, it makes the implementation of pg_execute_server_programs
broken. Only superusers should be considered to have with admin option

on

these roles. They can delegate through the usual membership+admin

mechanism

to a CREATEROLE role if they desire.

No, CREATEROLE having admin option on every role in the system is broken
and always has been. It's not just an issue for predefined roles like
pg_execute_server_program,

it's an issue for any role that could become
a superuser either directly or indirectly and that extends beyond the
predefined ones.

The only indirect way for a role to become superuser is to have been
granted membership in a superuser group, then SET ROLE. Non-superusers
cannot do this. If a superuser does this I consider the outcome to be no
different than if they go and do:

A non-superuser absolutely can be GRANT'd membership in a superuser role
and then SET ROLE to that user thus becoming a superuser. Giving users
a regular role to log in as and then membership in a role that can
become a superuser is akin to having a sudoers group in Unix and is good
practice, not something that everyone should have to be super-dooper
careful to not do, lest a CREATEROLE user be able to leverage that.

SET allow_system_table_mods TO true;
DROP pg_catalog.pg_class;

I don't equate these in the least.

In short, having a CREATEROLE user issuing:
GRANT pg_read_all_stats TO davidj;
should result in the same outcome as them issuing:
GRANT postgres TO davidj;
-- ERROR: must be superuser to alter superusers

No, what should matter is if the role doing the GRANT has admin rights
on pg_read_all_stats, or on the postgres role. That also happens to be
what the spec says.

Superusers can break their system and we don't go to great effort to stop
them. I see no difference here, so arguments of this nature aren't all
that compelling to me.

That you don't feel they're compelling don't make them somehow not real,
nor even particularly uncommon, nor do I view ignoring that possibility
as somehow creating a strong authentication system.

CREATEROLE shouldn't be

the determining factor in the question of if a role can GRANT a
predefined (or any other role) to some other role- that should be
governed by the admin option on that role, and that should work exactly
the same for predefined roles as it does for any other.

Never granting the CREATEROLE attribute to anyone will give you this
outcome today.

... which is why CREATEROLE is broken.

ADMIN option
without membership isn't something the catalogs today can understand

Today, they don't need to in order for the system to function within its
existing design specs.

Eh? Your argument here is "don't use CREATEROLE"? While I agree with
that being a generally good idea today, it hardly makes sense to suggest
it in a thread where we're talking about how to make CREATEROLE, or
something like it, be useful.

ALTER ROLE pg_superuser WITH [NO] ADMIN;

Then adding a role membership including the WITH ADMIN OPTION can be
rejected, as can the non-superuser situation. Setting WITH NO ADMIN

should

fail if any existing members have admin. You must be a superuser to
execute WITH ADMIN (maybe WITH NO ADMIN as well...). And possibly even a
new pg_* role that grants this ability (and maybe some others) for use

by a

backup/restore user.

I'm not following this in general or how it helps. Surely we don't want
to limit WITH ADMIN to superusers.

Today a non-superuser cannot "grant postgres to someuser;"

No, but a role can be created like 'admin', which a superuser GRANT's
'postgres' to and then that role can be GRANT'd to anyone by anyone who
has CREATEROLE rights. That's not sane.

The point of this attribute is to allow the superuser to apply that rule to
other roles that aren't superuser. In particular, the predefined pg_*
roles. But it could extend to any other role the superuser would like to
limit. It means, for that for named role, ADMIN privileges cannot be
delegated to other roles - thus all administration of that role's
membership roster must happen by a superuser.

The whole "X can't modify a superuser role without being a superuser"
concept is just broken and was a poor choice when it was originally
done specifically because it only looks at individual roles and their
specific rolsuper bit, completely ignoring the fact that role membership
exists as a thing that we should handle sanely, including a
non-superuser role being grant'd a superuser role. Predefined roles
haven't got anything to do with any of this, they only make it more
obvious to people who didn't understand how the system worked before
they came along.

I disagree entirely with the idea that we must have some roles who can
only ever be administered by a superuser. If anything, we should be
moving away (as we have, in fact, been doing), from anything being the
exclusive purview of the superuser.

In particular, this means CREATEROLE roles cannot assign membership in the
marked roles; just like they cannot assign membership in superuser roles
today.

I disagree with the idea that we need to mark some roles as only being
able to be modified by the superuser- why invent this? We have the
ADMIN option already and that can be applied to allow any role X to have
the ability to modify the members of role Y. That's a whole lot better
than some explicit flag that says "only superusers can modify this
role". If an admin wants that, they can set things up that way already
today, as long as they don't use the current CREATEROLE attribute.
Ideally, we'd modify CREATEROLE, or remove it and replace it with
something better, which still maintains that same flexibility. What you
seem to be arguing for here is to rip out the ADMIN functionality, which
is defined by spec and not even exclusively by PG, and replace it with a
single per-role flag that says if that role can only be modified by
superusers. That seems entirely backwards to me.

For me, because the SUPERUSER cannot have its role become a group without a
superuser making that choice, and by default the default pg_* roles will
all have this property as well, and any newly superuser created roles that
may be members of either superuser or pg_* can have the property defined as
well, gives full control to the superuser as to how superuser abilities are
doled out and so the design itself allows for what many of you are
considering to be "safe usage". That "unsafe configurations" are possible
is due to the policy that superusers are unrestricted in what they can do,
including making unsafe and destructive choices.

I disagree that it's an 'unsafe configuration' for there to ever exist a
non-superuser role that has been granted a superuser role. The only
thing that makes this unsafe is the existance of CREATEROLE.

Why are we making this all about superusers though? In what you're
proposing, you're suggesting that it's perfectly fine for any role which
has CREATEROLE to be able to take over any other role in the entire
system, excluding predefined roles and superusers. How is that sane, or
truely much less than what the superuser has in terms of ability? The
short answer is that it's not- which is why we have documented
CREATEROLE as being 'superuser light'. The goal here is to get rid of
that.

In short, removing the self-administration rule solves the "login roles
should not be automatically considered groups administered by themselves"
problem - or at least a feature we really don't need.
And defining a "superuser administration only" attribute to a role solves
the indirect superuser privileges and assignment thereof by non-superusers
problem.

But it doesn't *actually* make CREATEROLE something that you can give
out to folks on a general basis because anyone with CREATEROLE would
still be able to take over every single non-superuser and non-predefined
role in the system. We do *not* want that.

I can see value in adding a feature whereby we allow the DBA to define a
group as a schema-like container and then assign roles to that group with a
fine-grained permissions model. My take is this proposal is a new feature
while the two problems noted above can be solved more readily and with less
risk with the two suggested changes.

Yes, we're talking about a new feature- one intended to replace the
broken way that CREATEROLE works, which your proposal doesn't.

Thanks,

Stephen

On Thu, Mar 10, 2022 at 12:26 PM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:

Ownership implies DAC, the ability to grant others rights to an
object. It's not "kooky" to see roles as owned objects, but it isn't
required either. For example most objects on a UNIX system are owned
and subject to DAC but users aren't.

I have no issue with anything you write in this paragraph.

Stephen's, and now my, issue with ownership is that, since it implies
DAC, most checks will be bypassed for the owner. We would both prefer
for everyone to be subject to the grants, including whoever created
the role.

That sounds like MAC, which is usually something that sits on top of
DAC and is enforced in addition to DAC, not a reason for DAC to not
exist.

Rather, we'd like to see a "creators of roles get this set of grants
against the role by default" and "as a superuser I can revoke grants
from creators against roles they created"

If you create a table, you own it. You get a set of default
permissions on the table which can be revoked either by you or by
someone else, and you also have certain intrinsic rights over the
object as owner which cannot be revoked - including the ability to
re-grant yourself any previously-revoked permissions. I am not against
the idea of trying to clean things up so that everything you can do
with a table is a revocable privilege and you can be the owner without
having any rights at all, including the right to give yourself other
rights back, but I cannot believe that the idea of removing table
ownership as a concept would ever gain consensus on this list.
Therefore, I also do not think it is reasonable to say that we
shouldn't introduce a similar concept for object types that don't have
it yet, such as roles.

But that's not to say that we couldn't decide to do something else
instead, and that other thing might well be better. Do you want to
sketch out a full proposal, even just what the syntax would look like,
and share that here? And if you could explain how I could use it to
create the mini-superusers that I'm trying to get out of this thing,
even better.

Thanks,

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

On 09.03.22 14:02, Robert Haas wrote:

On Wed, Mar 9, 2022 at 7:55 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

Do we have subtractive permissions today?

Not in the GRANT/REVOKE sense, I think, but you can put a user in a
group and then mention that group in pg_hba.conf. And that line might
be "reject" or whatever.

Well, you can always build an external system that looks at roles and
does nonsensical things with it. But the privilege system itself seems
to be additive only. Personally, I agree with the argument that there
should not be any subtractive permissions. The mental model where
permissions are sort of keys to doors or boxes just doesn't work for that.

On Thu, Mar 10, 2022 at 2:05 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 09.03.22 14:02, Robert Haas wrote:

On Wed, Mar 9, 2022 at 7:55 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

Do we have subtractive permissions today?

Not in the GRANT/REVOKE sense, I think, but you can put a user in a
group and then mention that group in pg_hba.conf. And that line might
be "reject" or whatever.

Well, you can always build an external system that looks at roles and
does nonsensical things with it. But the privilege system itself seems
to be additive only. Personally, I agree with the argument that there
should not be any subtractive permissions. The mental model where
permissions are sort of keys to doors or boxes just doesn't work for that.

I mean, I didn't design pg_hba.conf, but I think it's part of the
database doing a reasonable thing, not an external system doing a
nonsensical thing.

I am not sure that I (or anyone) would endorse a system where you can
say something like GRANT NOT SELECT ON TABLE foo TO bar, essentially
putting a negative ACL into the system dictating that, regardless of
any other grants that may exist, foo should not be able to SELECT from
that table. But I think it's reasonable to use groups as a way of
referencing a defined collection of users for some purpose. The
pg_hba.conf thing is an example of that. You put all the users that
you want to be treated in a certain way for authentication purposes
into a group, and then you mention the group in the file, and it just
works. I don't find that an unreasonable design at all. We could've
created some other kind of grouping mechanism for such purposes that
is separate from the role system, but we didn't choose to do that. I
don't know if that was the absolute best possible decision or not, but
it doesn't seem like an especially bad choice.

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

On Thu, Mar 10, 2022 at 11:05 AM Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Thu, Mar 10, 2022 at 9:19 AM Stephen Frost <sfrost@snowman.net>

wrote:

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Thu, Mar 10, 2022 at 7:46 AM Robert Haas <robertmhaas@gmail.com>

wrote:

The only indirect way for a role to become superuser is to have been
granted membership in a superuser group, then SET ROLE. Non-superusers
cannot do this. If a superuser does this I consider the outcome to be no
different than if they go and do:

A non-superuser absolutely can be GRANT'd membership in a superuser role
and then SET ROLE to that user thus becoming a superuser.

A non-superuser cannot grant a non-superuser membership in a superuser
role. A superuser granting a user membership in a superuser role makes
that user a superuser. This seems sane.

If a superuser grants a non-superuser membership in a superuser role then
today a non-superuser can grant a user membership in that intermediate
role, thus having a non-superuser make another user a superuser. This is
arguably a bug that needs to be fixed.

My desired fix is to just require the superuser to mark (or have it marked
by default ideally) the role inheriting superuser and put the
responsibility on the superuser. I agree this is not ideal, but it is
probably quick and low risk.

I'll let someone else describe the details of the alternative option. I
suspect it will end up being a better option in terms of design. But
depending on time and risk even knowing that we want the better design
eventually doesn't preclude getting the easier fix in now.

No, what should matter is if the role doing the GRANT has admin rights
on pg_read_all_stats, or on the postgres role. That also happens to be
what the spec says.

Yes, and superusers implicitly have that right, while CREATEROLE users
implicitly have that right on the pg_* role but not on superuser roles. I
just want to plug that hole and include the pg_* roles (or any role for
that matter) in being able to be denied implied ADMIN rights for
non-superusers.

Today a non-superuser cannot "grant postgres to someuser;"

No, but a role can be created like 'admin', which a superuser GRANT's
'postgres' to and then that role can be GRANT'd to anyone by anyone who
has CREATEROLE rights. That's not sane.

I agree. And I've suggested a minimal fix, adding an attribute to the role
that prohibits non-superusers from granting it to others, that removes the
insane behavior.

I'm on board for a hard-coded fix as well - if a superuser is in the
membership chain of a role then non-superusers cannot grant membership in
that role to others.

Neither of those really solves the pg_* roles problem. We still need to
indicate that they are somehow special. Whether it is a nice matrix or
roles and permissions or a simple attribute that makes them behave like
they are superuser roles.

I disagree entirely with the idea that we must have some roles who can
only ever be administered by a superuser.

I don't think this is a must have. I think that since we do have it today
that fixes that leverage the status quo in order to be done more easily are
perfectly valid solutions.

If anything, we should be
moving away (as we have, in fact, been doing), from anything being the
exclusive purview of the superuser.

I totally agree.

In particular, this means CREATEROLE roles cannot assign membership in

the

marked roles; just like they cannot assign membership in superuser roles
today.

I disagree with the idea that we need to mark some roles as only being
able to be modified by the superuser- why invent this?

Because CREATEUSER is a thing and people want to prevent roles with that
attribute from assigning membership to the predefined superuser-aspect
roles. If I've misunderstood that desire and the scope of delegation given
by the superuser to CREATEUSER roles is acceptable, then no change here is
needed.

What you

seem to be arguing for here is to rip out the ADMIN functionality, which
is defined by spec and not even exclusively by PG, and replace it with a
single per-role flag that says if that role can only be modified by
superusers.

I made the observation that being able to manage the membership of a group
without having the ability to create new users seems like a half a loaf of
a feature. That's it. I would presume that any redesign of the
permissions system here would address this adequately.

The

short answer is that it's not- which is why we have documented

CREATEROLE as being 'superuser light'. The goal here is to get rid of
that.

Now you tell me. Robert should have led with that goal upfront.

Yes, we're talking about a new feature- one intended to replace the
broken way that CREATEROLE works, which your proposal doesn't.

That is correct, I was trying to figure out minimally invasive fixes to
what are arguably being called bugs.

David J.

Greetings,

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Thu, Mar 10, 2022 at 11:05 AM Stephen Frost <sfrost@snowman.net> wrote:

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Thu, Mar 10, 2022 at 9:19 AM Stephen Frost <sfrost@snowman.net> wrote:

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Thu, Mar 10, 2022 at 7:46 AM Robert Haas <robertmhaas@gmail.com> wrote:

The only indirect way for a role to become superuser is to have been
granted membership in a superuser group, then SET ROLE. Non-superusers
cannot do this. If a superuser does this I consider the outcome to be no
different than if they go and do:

A non-superuser absolutely can be GRANT'd membership in a superuser role
and then SET ROLE to that user thus becoming a superuser.

A non-superuser cannot grant a non-superuser membership in a superuser
role. A superuser granting a user membership in a superuser role makes
that user a superuser. This seems sane.

If a superuser grants a non-superuser membership in a superuser role then
today a non-superuser can grant a user membership in that intermediate
role, thus having a non-superuser make another user a superuser. This is
arguably a bug that needs to be fixed.

My desired fix is to just require the superuser to mark (or have it marked
by default ideally) the role inheriting superuser and put the
responsibility on the superuser. I agree this is not ideal, but it is
probably quick and low risk.

I'll let someone else describe the details of the alternative option. I
suspect it will end up being a better option in terms of design. But
depending on time and risk even knowing that we want the better design
eventually doesn't preclude getting the easier fix in now.

No, what should matter is if the role doing the GRANT has admin rights
on pg_read_all_stats, or on the postgres role. That also happens to be
what the spec says.

Yes, and superusers implicitly have that right, while CREATEROLE users
implicitly have that right on the pg_* role but not on superuser roles. I
just want to plug that hole and include the pg_* roles (or any role for
that matter) in being able to be denied implied ADMIN rights for
non-superusers.

CREATEROLE users implicitly have that right on *all non-superuser
roles*. Not just the pg_* ones, which is why the pg_* ones aren't any
different in this regard.

Today a non-superuser cannot "grant postgres to someuser;"

No, but a role can be created like 'admin', which a superuser GRANT's
'postgres' to and then that role can be GRANT'd to anyone by anyone who
has CREATEROLE rights. That's not sane.

I agree. And I've suggested a minimal fix, adding an attribute to the role
that prohibits non-superusers from granting it to others, that removes the
insane behavior.

I disagree that this is a minimal fix as I don't see it as a fix to the
actual issue, which is the ability for CREATEROLE users to GRANT role
membership to all non-superuser roles on the system. CREATEROLE
shouldn't be allowing that.

I'm on board for a hard-coded fix as well - if a superuser is in the
membership chain of a role then non-superusers cannot grant membership in
that role to others.

Why not just look at the admin_option field of pg_auth_members...? I
don't get why that isn't an even more minimal fix than this idea you
have of adding a column to pg_authid and then propagating around "this
user could become a superuser" or writing code that has to go check "is
there some way for this role to become a superuser, either directly or
through some subset of pg_* roles?"

Neither of those really solves the pg_* roles problem. We still need to
indicate that they are somehow special. Whether it is a nice matrix or
roles and permissions or a simple attribute that makes them behave like
they are superuser roles.

I disagree that they should be considered special when it comes to role
membership and management. They're just roles, like any other.

I disagree entirely with the idea that we must have some roles who can
only ever be administered by a superuser.

I don't think this is a must have. I think that since we do have it today
that fixes that leverage the status quo in order to be done more easily are
perfectly valid solutions.

We have a half-way-implemented attempt at this, not something that's
actually effective, and therefore I don't agree that we really have it
today or that we should keep it. I'd much prefer to throw out nearly
everything in the system that's doing an explicit check of "does this
role have a superuser bit set on it?"

If anything, we should be
moving away (as we have, in fact, been doing), from anything being the
exclusive purview of the superuser.

I totally agree.

Great.

In particular, this means CREATEROLE roles cannot assign membership in

the

marked roles; just like they cannot assign membership in superuser roles
today.

I disagree with the idea that we need to mark some roles as only being
able to be modified by the superuser- why invent this?

Because CREATEUSER is a thing and people want to prevent roles with that
attribute from assigning membership to the predefined superuser-aspect
roles. If I've misunderstood that desire and the scope of delegation given
by the superuser to CREATEUSER roles is acceptable, then no change here is
needed.

We can do that by using the admin_option in pg_auth_members instead
though and limiting everyone to using that.

What you

seem to be arguing for here is to rip out the ADMIN functionality, which
is defined by spec and not even exclusively by PG, and replace it with a
single per-role flag that says if that role can only be modified by
superusers.

I made the observation that being able to manage the membership of a group
without having the ability to create new users seems like a half a loaf of
a feature. That's it. I would presume that any redesign of the
permissions system here would address this adequately.

If the new design ideas that are being thrown around don't address what
you're thinking they should, it'd be great to point that out.

The

short answer is that it's not- which is why we have documented

CREATEROLE as being 'superuser light'. The goal here is to get rid of
that.

Now you tell me. Robert should have led with that goal upfront.

... blink.

Yes, we're talking about a new feature- one intended to replace the
broken way that CREATEROLE works, which your proposal doesn't.

That is correct, I was trying to figure out minimally invasive fixes to
what are arguably being called bugs.

What's been proposed here doesn't strike me as minimally invasive,
though I suppose I'm looking at it more from the database system
perspective and less from the end-user side of things for people who
actually use CREATEROLE, but in this particular case, that's the side
I'm on.

Thanks,

Stephen

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:

But that's not to say that we couldn't decide to do something else
instead, and that other thing might well be better. Do you want to
sketch out a full proposal, even just what the syntax would look like,
and share that here? And if you could explain how I could use it to
create the mini-superusers that I'm trying to get out of this thing,
even better.

It'd be useful to have a better definition of exactly what a
'mini-superuser' is, but at least for the moment when it comes to roles,
let's look at what the spec says:

CREATE ROLE
- Who is allowed to run CREATE ROLE is implementation-defined
- After creation, this is effictively run:
GRANT new_role TO creator_role WITH ADMIN, GRANTOR "_SYSTEM"

DROP ROLE
- Any user who has been GRANT'd a role with ADMIN option is able to
DROP that role.

GRANT ROLE
- No cycles allowed
- A role must have ADMIN rights on the role to be able to GRANT it to
another role.

ALTER ROLE
- Doesn't exist

This actually looks to me like more-or-less what you're looking for, it
just isn't what we have today because CREATEROLE brings along with it a
bunch of other stuff, some of which we want and some that we don't, and
some things that the SQL spec says ADMIN should be allowed to do (DROP
ROLE) we don't allow today.

It's also not quite what I want because it requires that membership and
ADMIN go together where I'd like to be able to have those be
independently GRANT'able- and then some.

I don't think we're that far from having all of these though. To start
with, we remove from CREATEROLE the random things that it does which go
beyond what folks tend to expect- remove the whole 'grant any role to
any other' stuff, remove the 'drop role' exception, remove the
'alter role' stuff. Do make it so that when you create a role, however,
the above GRANT is effectively done. Now, for the items above where we
removed the checks against have_createrole_privilege() we go back and
add in checks using is_admin_of_role(). Of course, also remove the role
self-administration bug.

That's step #1, but it gets us more-or-less what you're looking for, I
think, and brings us a lot closer to what the spec has.

Step #2 is also in-line with the spec: track GRANTORs and care about
them, for everything. We really should have been doing this all along.
Note that I'm not saying that an owner of a table can't REVOKE some
right that was GRANT'd on that table, but rather that a user who was
GRANT'd ADMIN rights on a table and then GRANT'd that right to some
other user shouldn't have some other user who only has ADMIN rights on
the table be able to remove that GRANT. Same goes for roles, meaning
that you could GRANT rights in a role with ADMIN option and not have to
be afraid that the role you just gave that to will be able to remove
*your* ADMIN rights on that role. In general, I don't think this
would actually have a very large impact on users because most users
don't, today, use the ADMIN option much.

Step #3 starts going in the direction of what I'd like to see, which
would be to break out membership in a role as a separate thing from
admin rights on that role. This is also what would help with the 'bot'
use-case that Joshua (not David Steele, btw) brought up.

Step #4 then breaks the 'admin' option on roles into pieces- a 'drop
role' right, a 'reset password' right, maybe separate rights for
different role attributes, etc. We would likely still keep the
'admin_option' column in pg_auth_members and just check that first
and then check the individual rights (similar to table-level vs.
column-level privileges) so that we stay in line with the spec's
expectation here and with what users are used to.

In some hyptothetical world, there's even a later step #5 which allows
us to define user profiles and then grant the ability for a user to
create a role with a certain profile (but not any arbitrary profile),
thus making things like the 'bot' even more constrained in terms of
what it's able to do (maybe it can then create a role that's a member of
a role without itself being a member of that role or explicitly having
admin rights in that role, as an example).

Thanks,

Stephen

On Thu, Mar 10, 2022 at 12:45 PM Stephen Frost <sfrost@snowman.net> wrote:

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Thu, Mar 10, 2022 at 11:05 AM Stephen Frost <sfrost@snowman.net>

wrote:

Why not just look at the admin_option field of pg_auth_members...? I

don't get why that isn't an even more minimal fix than this idea you
have of adding a column to pg_authid and then propagating around "this
user could become a superuser" or writing code that has to go check "is
there some way for this role to become a superuser, either directly or
through some subset of pg_* roles?"

Indeed, maybe I am wrong on the scope of the patch. But at least for the
explicit attribute it should be no more difficult than changing:

if (grouprole_is_superuser and current_role_is_not_superuser) then error:
to be
if ((grouoprole_is_superuser OR !groupuser_has_adminattr) AND
current_role_is_not_superuser) then error;

I have to imagine that given how fundamental inheritance is to our
permissions system than doing a similar check up the tree wouldn't be
difficult, but I truly don't know with a strong degree of certainty.

Assuming we don't actually rip out CREATEROLE when this change goes in...do
you propose to prohibit a CREATEROLE user from altering the membership
roster of any group which itself is not a member of and also those which it
is a member of but where admin_option is false?

I don't personally have a problem with the current state where CREATEROLE
is an admin for, but not a member of, every non-superuser(-related) role in
the system. If the consensus is to change that then I suppose this becomes
the minimally invasive fix that accomplishes that goal as well. It seems
incomplete though, since you still need superuser to create a group and add
the initial WITH ADMIN member to it. So this seems to work in the "avoid
using superuser" sense if you've also added something that has what
CREATEROLE provides today - admin without membership - but that would have
the benefit of not carrying around all the baggage that CREATEROLE has.

I made the observation that being able to manage the membership of a

group

without having the ability to create new users seems like a half a loaf

of

a feature. That's it. I would presume that any redesign of the
permissions system here would address this adequately.

If the new design ideas that are being thrown around don't address what
you're thinking they should, it'd be great to point that out.

I mean, you need a Create Role permission in some form, even if it's
deprecating the attribute and making it a predefined role. I picked this
thread up because it seemed like a limited scope that I could get my head
around with the time I have, with the main goal to try to understand this
aspect of the system better. I haven't gone and looked into the main
thread yet.

David J.

On Thu, Mar 10, 2022 at 2:58 PM Stephen Frost <sfrost@snowman.net> wrote:

It'd be useful to have a better definition of exactly what a
'mini-superuser' is, but at least for the moment when it comes to roles,
let's look at what the spec says:

Gosh, I feel like I've spelled that out approximately 463,121 times
already. That estimate might be slightly off though; I've been known
to make mistakes from time to time....

CREATE ROLE
- Who is allowed to run CREATE ROLE is implementation-defined
- After creation, this is effictively run:
GRANT new_role TO creator_role WITH ADMIN, GRANTOR "_SYSTEM"

DROP ROLE
- Any user who has been GRANT'd a role with ADMIN option is able to
DROP that role.

GRANT ROLE
- No cycles allowed
- A role must have ADMIN rights on the role to be able to GRANT it to
another role.

ALTER ROLE
- Doesn't exist

This actually looks to me like more-or-less what you're looking for, it
just isn't what we have today because CREATEROLE brings along with it a
bunch of other stuff, some of which we want and some that we don't, and
some things that the SQL spec says ADMIN should be allowed to do (DROP
ROLE) we don't allow today.

The above is mostly fine with me, except for the part about ALTER ROLE
not existing. I think it's always good to be able to change your mind
post-CREATE.

Basically, in this sketch, ADMIN OPTION on a role involves the ability
to DROP it, which means we don't need a separate role owner concept.
It also involves membership, meaning that you can freely exercise the
privileges of the role without SET ROLE. While I'm totally down with
having other possible behaviors as options, that particular behavior
seems very useful to me, so, sounds great.

It's also not quite what I want because it requires that membership and
ADMIN go together where I'd like to be able to have those be
independently GRANT'able- and then some.

I don't think we're that far from having all of these though. To start
with, we remove from CREATEROLE the random things that it does which go
beyond what folks tend to expect- remove the whole 'grant any role to
any other' stuff, remove the 'drop role' exception, remove the
'alter role' stuff. Do make it so that when you create a role, however,
the above GRANT is effectively done. Now, for the items above where we
removed the checks against have_createrole_privilege() we go back and
add in checks using is_admin_of_role(). Of course, also remove the role
self-administration bug.

What do you mean by the 'drop role' exception?

I don't like removing 'alter role'.

The rest sounds good.

That's step #1, but it gets us more-or-less what you're looking for, I
think, and brings us a lot closer to what the spec has.

Great.

Step #2 is also in-line with the spec: track GRANTORs and care about
them, for everything. We really should have been doing this all along.
Note that I'm not saying that an owner of a table can't REVOKE some
right that was GRANT'd on that table, but rather that a user who was
GRANT'd ADMIN rights on a table and then GRANT'd that right to some
other user shouldn't have some other user who only has ADMIN rights on
the table be able to remove that GRANT. Same goes for roles, meaning
that you could GRANT rights in a role with ADMIN option and not have to
be afraid that the role you just gave that to will be able to remove
*your* ADMIN rights on that role. In general, I don't think this
would actually have a very large impact on users because most users
don't, today, use the ADMIN option much.

There are details to work out here, but in general, I like it.

Step #3 starts going in the direction of what I'd like to see, which
would be to break out membership in a role as a separate thing from
admin rights on that role. This is also what would help with the 'bot'
use-case that Joshua (not David Steele, btw) brought up.

Woops, apologies for getting the name wrong. I also said Marc earlier
when I meant Mark, because I work with people named Mark, Marc, and
Marc, and Mark's spelling got outvoted by some distant corner of my
brain.

I think this is a fine long-term direction, with the caveat that
you've not provided enough specifics here for me to really understand
how it would work. I fear the specifics might be hard to get right,
both in terms of making it understandable to users and in terms of
preserving as much backward-compatibility as we can. However, I am not
opposed to the concept.

Step #4 then breaks the 'admin' option on roles into pieces- a 'drop
role' right, a 'reset password' right, maybe separate rights for
different role attributes, etc. We would likely still keep the
'admin_option' column in pg_auth_members and just check that first
and then check the individual rights (similar to table-level vs.
column-level privileges) so that we stay in line with the spec's
expectation here and with what users are used to.

Same comments as #3, plus I wonder whether it really makes sense to
separate #3 and #4. But we can decide that when there's a fleshed-out
design for this.

In some hyptothetical world, there's even a later step #5 which allows
us to define user profiles and then grant the ability for a user to
create a role with a certain profile (but not any arbitrary profile),
thus making things like the 'bot' even more constrained in terms of
what it's able to do (maybe it can then create a role that's a member of
a role without itself being a member of that role or explicitly having
admin rights in that role, as an example).

Right. I don't object to this either, hypothetically, but I think
we're a long way from understanding how to get there, and I don't want
step #1 to get blocked behind all the rest of this. Particularly the
part where we remove the role self-administration thing.

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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Thu, Mar 10, 2022 at 2:58 PM Stephen Frost <sfrost@snowman.net> wrote:

It'd be useful to have a better definition of exactly what a
'mini-superuser' is, but at least for the moment when it comes to roles,
let's look at what the spec says:

Gosh, I feel like I've spelled that out approximately 463,121 times
already. That estimate might be slightly off though; I've been known
to make mistakes from time to time....

If there's a specific message that details it closely on the lists
somewhere, I'm happy to go review it. I admit that I didn't go back and
look for such.

CREATE ROLE
- Who is allowed to run CREATE ROLE is implementation-defined
- After creation, this is effictively run:
GRANT new_role TO creator_role WITH ADMIN, GRANTOR "_SYSTEM"

DROP ROLE
- Any user who has been GRANT'd a role with ADMIN option is able to
DROP that role.

GRANT ROLE
- No cycles allowed
- A role must have ADMIN rights on the role to be able to GRANT it to
another role.

ALTER ROLE
- Doesn't exist

This actually looks to me like more-or-less what you're looking for, it
just isn't what we have today because CREATEROLE brings along with it a
bunch of other stuff, some of which we want and some that we don't, and
some things that the SQL spec says ADMIN should be allowed to do (DROP
ROLE) we don't allow today.

The above is mostly fine with me, except for the part about ALTER ROLE
not existing. I think it's always good to be able to change your mind
post-CREATE.

Errr, just to be clear, ALTER ROLE doesn't exist *in the spec*. I
wasn't suggesting that we get rid of it, just that it doesn't exist in
the spec and therefore the spec doesn't have anything to say about it.

Basically, in this sketch, ADMIN OPTION on a role involves the ability
to DROP it, which means we don't need a separate role owner concept.

Right. The above doesn't include any specifics about what to do with
ALTER ROLE, but my thought would be to have it also be under ADMIN
OPTION rather than under CREATEROLE, as I tried to outline (though not
very well, I'll admit) below.

It also involves membership, meaning that you can freely exercise the
privileges of the role without SET ROLE. While I'm totally down with
having other possible behaviors as options, that particular behavior
seems very useful to me, so, sounds great.

Well, yes and no- by default you're right, presuming everything is set
as inheirited, but I'd wish for us to keep the option of creating roles
which are noinherit and having that work just as it does today.

It's also not quite what I want because it requires that membership and
ADMIN go together where I'd like to be able to have those be
independently GRANT'able- and then some.

I don't think we're that far from having all of these though. To start
with, we remove from CREATEROLE the random things that it does which go
beyond what folks tend to expect- remove the whole 'grant any role to
any other' stuff, remove the 'drop role' exception, remove the
'alter role' stuff. Do make it so that when you create a role, however,
the above GRANT is effectively done. Now, for the items above where we
removed the checks against have_createrole_privilege() we go back and
add in checks using is_admin_of_role(). Of course, also remove the role
self-administration bug.

What do you mean by the 'drop role' exception?

'ability' was probably a better word there. What I'm talking about is
changing in DropRole:

if (!have_createrole_privilege())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to drop role")));

to be, more or less:

if (!is_admin_of_role(role))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to drop role")));

I don't like removing 'alter role'.

Ditto above but for AlterRole. Taking it away from users with
CREATEROLE being able to run those commands on $anyrole and instead
making it so that the role running DROP ROLE or ALTER ROLE needs to have
ADMIN on the role they're messing with. I do think we may also need to
make some adjustments in terms of what a regular user WITH ADMIN on a
given role is able to do when it comes to ALTER ROLE, in particular, I
don't think we'll want to remove the existing is-superuser checks
against a user settings bypassrls or replication or superuser on some
other role. Maybe we can provide a way for a non-superuser to be given
the ability to set those attributes for roles they create, but that
would be a separate thing.

The rest sounds good.

Great.

That's step #1, but it gets us more-or-less what you're looking for, I
think, and brings us a lot closer to what the spec has.

Great.

Awesome.

Step #2 is also in-line with the spec: track GRANTORs and care about
them, for everything. We really should have been doing this all along.
Note that I'm not saying that an owner of a table can't REVOKE some
right that was GRANT'd on that table, but rather that a user who was
GRANT'd ADMIN rights on a table and then GRANT'd that right to some
other user shouldn't have some other user who only has ADMIN rights on
the table be able to remove that GRANT. Same goes for roles, meaning
that you could GRANT rights in a role with ADMIN option and not have to
be afraid that the role you just gave that to will be able to remove
*your* ADMIN rights on that role. In general, I don't think this
would actually have a very large impact on users because most users
don't, today, use the ADMIN option much.

There are details to work out here, but in general, I like it.

Cool. Note that superusers would still be able to do $anything,
including removing someone's ADMIN rights on a role even if that
superuser didn't GRANT it (at least, that's my thinking on this).

Step #3 starts going in the direction of what I'd like to see, which
would be to break out membership in a role as a separate thing from
admin rights on that role. This is also what would help with the 'bot'
use-case that Joshua (not David Steele, btw) brought up.

Woops, apologies for getting the name wrong. I also said Marc earlier
when I meant Mark, because I work with people named Mark, Marc, and
Marc, and Mark's spelling got outvoted by some distant corner of my
brain.

Hah, no worries.

I think this is a fine long-term direction, with the caveat that
you've not provided enough specifics here for me to really understand
how it would work. I fear the specifics might be hard to get right,
both in terms of making it understandable to users and in terms of
preserving as much backward-compatibility as we can. However, I am not
opposed to the concept.

We can perhaps debate the specifics around this later.

Step #4 then breaks the 'admin' option on roles into pieces- a 'drop
role' right, a 'reset password' right, maybe separate rights for
different role attributes, etc. We would likely still keep the
'admin_option' column in pg_auth_members and just check that first
and then check the individual rights (similar to table-level vs.
column-level privileges) so that we stay in line with the spec's
expectation here and with what users are used to.

Same comments as #3, plus I wonder whether it really makes sense to
separate #3 and #4. But we can decide that when there's a fleshed-out
design for this.

Ditto. I don't know that they need to be independent either.

In some hyptothetical world, there's even a later step #5 which allows
us to define user profiles and then grant the ability for a user to
create a role with a certain profile (but not any arbitrary profile),
thus making things like the 'bot' even more constrained in terms of
what it's able to do (maybe it can then create a role that's a member of
a role without itself being a member of that role or explicitly having
admin rights in that role, as an example).

Right. I don't object to this either, hypothetically, but I think
we're a long way from understanding how to get there, and I don't want
step #1 to get blocked behind all the rest of this. Particularly the
part where we remove the role self-administration thing.

Sure.

Thanks,

Stephen

On Thu, Mar 10, 2022 at 12:58 PM Stephen Frost <sfrost@snowman.net> wrote:

I don't think we're that far from having all of these though. To start
with, we remove from CREATEROLE the random things that it does which go
beyond what folks tend to expect- remove the whole 'grant any role to
any other' stuff, remove the 'drop role' exception, remove the
'alter role' stuff. Do make it so that when you create a role, however,
the above GRANT is effectively done. Now, for the items above where we
removed the checks against have_createrole_privilege() we go back and
add in checks using is_admin_of_role(). Of course, also remove the role
self-administration bug.

That's step #1, but it gets us more-or-less what you're looking for, I
think, and brings us a lot closer to what the spec has.

That still leaves attribute specification in place: e.g., REPLICATION,
CREATEROLE, CREATEDB, etc... (I see BYPASSRLS already is SUPERUSER only)

I dislike changing the documented behavior of CREATEROLE to the degree
suggested here. However, there are three choices here, only one of which
can be chosen:

1. Leave CREATEROLE alone entirely
2. Make it so CREATEROLE cannot assign membership to the predefined roles
or superuser (inheritance included), but leave the rest alone. This would
be the hard-coded version, not the role attribute one.
3. Make it so CREATEROLE can only assign membership to roles for which it
has been made an admin; as well as the other things mentioned

Moving forward I'd prefer options 1 or 2, leaving the ability to
create/alter/drop a role to be vested via predefined roles.

The rest seems fine at an initial glance.

David J.

On Thu, Mar 10, 2022 at 3:41 PM Stephen Frost <sfrost@snowman.net> wrote:

Gosh, I feel like I've spelled that out approximately 463,121 times
already. That estimate might be slightly off though; I've been known
to make mistakes from time to time....

If there's a specific message that details it closely on the lists
somewhere, I'm happy to go review it. I admit that I didn't go back and
look for such.

Probably easier to just say it again: I want to have users that can
create roles and then have superuser-like powers with respect to those
roles. They can freely exercise the privileges of those roles, and
they can do all the things that a superuser can do but only with
respect to those roles. They cannot break out to the OS. I think it's
pretty similar to what you are describing, with a couple of possible
exceptions. For example, would you imagine that being an admin of a
login role would let you change that user's password? Because that
would be desirable behavior from where I sit.

Errr, just to be clear, ALTER ROLE doesn't exist *in the spec*. I
wasn't suggesting that we get rid of it, just that it doesn't exist in
the spec and therefore the spec doesn't have anything to say about it.

Oh, OK.

Basically, in this sketch, ADMIN OPTION on a role involves the ability
to DROP it, which means we don't need a separate role owner concept.

Right. The above doesn't include any specifics about what to do with
ALTER ROLE, but my thought would be to have it also be under ADMIN
OPTION rather than under CREATEROLE, as I tried to outline (though not
very well, I'll admit) below.

This sentence really confused me at first, but I think you're saying
that the right to alter a role would be dependent on having ADMIN
OPTION on the role rather than on having the CREATEROLE attribute.
That seems like a reasonable idea to me.

It also involves membership, meaning that you can freely exercise the
privileges of the role without SET ROLE. While I'm totally down with
having other possible behaviors as options, that particular behavior
seems very useful to me, so, sounds great.

Well, yes and no- by default you're right, presuming everything is set
as inheirited, but I'd wish for us to keep the option of creating roles
which are noinherit and having that work just as it does today.

Hmm, so if I have membership WITH ADMIN OPTION in a role, but my role
is marked NOINHERIT, that means I can't exercise the privileges of
that role without SET ROLE. But, can I still do other things to that
role, such as dropping it? Given the current coding of
roles_is_member_of(), it seems like I can't. I don't like that, but
then I don't like much of anything about NOINHERIT. Do you have any
suggestions for how this could be improved?

To make this more concrete, suppose the superuser does "CREATE USER
alice CREATEROLE". Alice will have INHERIT, so she'll have control
over any roles she creates. But if she does "CREATE USER bob
CREATEROLE NOINHERIT" then neither she nor Bob will be able to control
the roles bob creates. I'd like to have a way to make it so that
neither Alice nor any other CREATEROLE users she spins up can create
roles over which they no longer have control. Because otherwise people
will do dumb stuff like that and then have to call the superuser to
sort it out, and the superuser won't like that because s/he is a super
busy person.

What do you mean by the 'drop role' exception?

'ability' was probably a better word there. What I'm talking about is
changing in DropRole:

to be, more or less:

if (!is_admin_of_role(role))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to drop role")));

Sounds good.

I don't like removing 'alter role'.

Ditto above but for AlterRole. Taking it away from users with
CREATEROLE being able to run those commands on $anyrole and instead
making it so that the role running DROP ROLE or ALTER ROLE needs to have
ADMIN on the role they're messing with. I do think we may also need to
make some adjustments in terms of what a regular user WITH ADMIN on a
given role is able to do when it comes to ALTER ROLE, in particular, I
don't think we'll want to remove the existing is-superuser checks
against a user settings bypassrls or replication or superuser on some
other role. Maybe we can provide a way for a non-superuser to be given
the ability to set those attributes for roles they create, but that
would be a separate thing.

This too.

Step #2 is also in-line with the spec: track GRANTORs and care about
them, for everything. We really should have been doing this all along.

There are details to work out here, but in general, I like it.

Cool. Note that superusers would still be able to do $anything,
including removing someone's ADMIN rights on a role even if that
superuser didn't GRANT it (at least, that's my thinking on this).

Agree. I also think that it would be a good idea to attribute grants
performed by any superuser to the bootstrap superuser, or leave them
unattributed somehow. Because otherwise dropping superusers becomes a
pain in the tail for no good reason.

We might also need to think carefully about what happens if for
example the table owner is changed. If bob owns the table and we
change the owner to mary, but bob's previous grants are still
attributed to bob, I'm not sure that's going to be very convenient.
Possibly if the table owner changes we also change the owner of all
grants attributed to the old table owner to be attributed to the new
table owner?

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

On Thu, Mar 10, 2022 at 02:22:05PM -0500, Robert Haas wrote:

I mean, I didn't design pg_hba.conf, but I think it's part of the
database doing a reasonable thing, not an external system doing a
nonsensical thing.

FYI, I think pg_hba.conf gets away with having negative/reject
permissions only because it is strictly ordered.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

On Thu, Mar 10, 2022 at 4:00 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

I dislike changing the documented behavior of CREATEROLE to the degree suggested here. However, there are three choices here, only one of which can be chosen:

1. Leave CREATEROLE alone entirely
2. Make it so CREATEROLE cannot assign membership to the predefined roles or superuser (inheritance included), but leave the rest alone. This would be the hard-coded version, not the role attribute one.
3. Make it so CREATEROLE can only assign membership to roles for which it has been made an admin; as well as the other things mentioned

Moving forward I'd prefer options 1 or 2, leaving the ability to create/alter/drop a role to be vested via predefined roles.

It sounds like you prefer a behavior where CREATEROLE gives power over
all non-superusers, but that seems pretty limiting to me. Why can't
someone want to create a user with power over some users but not
others? For example, the superuser might want to give alice the
ability to set up new users in the accounting department, but NOT give
alice the right to tinker with the backup user (who is not a
superuser, but doesn't have the replication privilege). How would they
accomplish that in your view?

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

On Thu, Mar 10, 2022 at 5:00 PM Bruce Momjian <bruce@momjian.us> wrote:

On Thu, Mar 10, 2022 at 02:22:05PM -0500, Robert Haas wrote:

I mean, I didn't design pg_hba.conf, but I think it's part of the
database doing a reasonable thing, not an external system doing a
nonsensical thing.

FYI, I think pg_hba.conf gets away with having negative/reject
permissions only because it is strictly ordered.

I agree.

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

On Thu, Mar 10, 2022 at 3:01 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 10, 2022 at 4:00 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

I dislike changing the documented behavior of CREATEROLE to the degree

suggested here. However, there are three choices here, only one of which
can be chosen:

1. Leave CREATEROLE alone entirely
2. Make it so CREATEROLE cannot assign membership to the predefined

roles or superuser (inheritance included), but leave the rest alone. This
would be the hard-coded version, not the role attribute one.

3. Make it so CREATEROLE can only assign membership to roles for which

it has been made an admin; as well as the other things mentioned

Moving forward I'd prefer options 1 or 2, leaving the ability to

create/alter/drop a role to be vested via predefined roles.

It sounds like you prefer a behavior where CREATEROLE gives power over
all non-superusers, but that seems pretty limiting to me.

Doh! I edited out the part where I made clear I considered options 1 and 2
as basically being done for a limited period of time while deprecating the
CREATEROLE attribute altogether in favor of the fine-grained and predefined
role based permission granting. I don't want to nerf CREATEROLE as part of
adding this new feature, instead leave it as close to status quo as
reasonable so as not to mess up existing setups that make use of it. We
can note in the release notes and documentation that we consider CREATEROLE
to be deprecated and that the new predefined role should be used to give a
user the ability to create/alter/drop roles, etc... DBAs should consider
revoking CREATEROLE from their users and granting them proper memberships
in the predefined roles and the groups those roles should be administering.

David J.

Robert Haas <robertmhaas@gmail.com> writes:

Probably easier to just say it again: I want to have users that can
create roles and then have superuser-like powers with respect to those
roles. They can freely exercise the privileges of those roles, and
they can do all the things that a superuser can do but only with
respect to those roles.

This seems reasonable in isolation, but

(1) it implies a persistent relationship between creating and created
roles. Whether you want to call that ownership or not, it sure walks
and quacks like ownership.

(2) it seems exactly contradictory to your later point that

Agree. I also think that it would be a good idea to attribute grants
performed by any superuser to the bootstrap superuser, or leave them
unattributed somehow. Because otherwise dropping superusers becomes a
pain in the tail for no good reason.

Either there's a persistent relationship or there's not. I don't
think it's sensible to treat superusers differently here.

I think that this argument about the difficulty of dropping superusers
may in fact be the motivation for the existing behavior that object-
permissions GRANTs done by superusers are attributed to the object
owner; something you were unhappy about upthread.

In the end these requirements seem mutually contradictory. Either
we can have a persistent ownership relationship or not, but I don't
think we can have it apply in some cases and not others without
creating worse problems than we solve. I'm inclined to toss overboard
the requirement that superusers need to be an easy thing to drop.
Why is that important, anyway?

We might also need to think carefully about what happens if for
example the table owner is changed. If bob owns the table and we
change the owner to mary, but bob's previous grants are still
attributed to bob, I'm not sure that's going to be very convenient.

That's already handled, is it not?

regression=# create user alice;
CREATE ROLE
regression=# create user bob;
CREATE ROLE
regression=# create user charlie;
CREATE ROLE
regression=# \c - alice
You are now connected to database "regression" as user "alice".
regression=> create table alices_table (f1 int);
CREATE TABLE
regression=> grant select on alices_table to bob;
GRANT
regression=> \c - postgres
You are now connected to database "regression" as user "postgres".
regression=# alter table alices_table owner to charlie;
ALTER TABLE
regression=# \dp alices_table
Access privileges
Schema | Name | Type | Access privileges | Column privileges | Policies
--------+--------------+-------+-------------------------+-------------------+----------
public | alices_table | table | charlie=arwdDxt/charlie+| |
| | | bob=r/charlie | |
(1 row)

I'm a bit disturbed that parts of this discussion seem to be getting
conducted with little understanding of the system's existing behaviors.
We should not be reinventing things we already have perfectly good
solutions for in the object-privileges domain.

regards, tom lane

On Mar 10, 2022, at 2:01 PM, Robert Haas <robertmhaas@gmail.com> wrote:

It sounds like you prefer a behavior where CREATEROLE gives power over
all non-superusers, but that seems pretty limiting to me. Why can't
someone want to create a user with power over some users but not
others?

I agree with Robert on this.

Over at [1]/messages/by-id/53C7DF4C-8463-4647-9DFD-779B5E1861C4@amazon.com, I introduced a patch series to (a) change CREATEROLE and (b) introduce role ownership. Part (a) wasn't that controversial. The patch series failed to make it for postgres 15 on account of (b). The patch didn't go quite far enough, but with it applied, this is an example of a min-superuser "lord" operating within database "fiefdom":

fiefdom=# -- mini-superuser who can create roles and write all data
fiefdom=# CREATE ROLE lord
fiefdom-# WITH CREATEROLE
fiefdom-# IN ROLE pg_write_all_data;
CREATE ROLE
fiefdom=#
fiefdom=# -- group which "lord" belongs to
fiefdom=# CREATE GROUP squire
fiefdom-# ROLE lord;
CREATE ROLE
fiefdom=#
fiefdom=# -- group which "lord" has no connection to
fiefdom=# CREATE GROUP paladin;
CREATE ROLE
fiefdom=#
fiefdom=# SET SESSION AUTHORIZATION lord;
SET
fiefdom=>
fiefdom=> -- fail, merely a member of "squire"
fiefdom=> CREATE ROLE peon IN ROLE squire;
ERROR: must have admin option on role "squire"
fiefdom=>
fiefdom=> -- fail, no privilege to grant CREATEDB
fiefdom=> CREATE ROLE peon CREATEDB;
ERROR: must have createdb privilege to create createdb users
fiefdom=>
fiefdom=> RESET SESSION AUTHORIZATION;
RESET
fiefdom=#
fiefdom=# -- grant admin over "squire" to "lord"
fiefdom=# GRANT squire
fiefdom-# TO lord
fiefdom-# WITH ADMIN OPTION;
GRANT ROLE
fiefdom=#
fiefdom=# SET SESSION AUTHORIZATION lord;
SET
fiefdom=>
fiefdom=> -- ok, have both "CREATEROLE" and admin option for "squire"
fiefdom=> CREATE ROLE peon IN ROLE squire;
CREATE ROLE
fiefdom=>
fiefdom=> -- fail, no privilege to grant CREATEDB
fiefdom=> CREATE ROLE peasant CREATEDB IN ROLE squire;
ERROR: must have createdb privilege to create createdb users
fiefdom=>
fiefdom=> RESET SESSION AUTHORIZATION;
RESET
fiefdom=#
fiefdom=# -- Give lord the missing privilege
fiefdom=# GRANT CREATEDB TO lord;
ERROR: role "createdb" does not exist
fiefdom=#
fiefdom=# RESET SESSION AUTHORIZATION;
RESET
fiefdom=#
fiefdom=# -- ok, have "CREATEROLE", "CREATEDB", and admin option for "squire"
fiefdom=# CREATE ROLE peasant CREATEDB IN ROLE squire;
CREATE ROLE

The problem with this is that "lord" needs CREATEDB to grant CREATEDB, but really it should need something like grant option on "CREATEDB". But that's hard to do with the existing system, given the way these privilege bits are represented. If we added a few more built-in pg_* roles, such as pg_create_db, it would just work. CREATEROLE itself could be reimagined as pg_create_role, and then users could be granted into this role with or without admin option, meaning they could/couldn't further give it away. I think that would be a necessary component to Joshua's "bot" use-case, since the bot must itself have the privilege to create roles, but shouldn't necessarily be trusted with the privilege to create additional roles who have it.

[1]: /messages/by-id/53C7DF4C-8463-4647-9DFD-779B5E1861C4@amazon.com

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Mar 10, 2022 at 5:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

This seems reasonable in isolation, but

(1) it implies a persistent relationship between creating and created
roles. Whether you want to call that ownership or not, it sure walks
and quacks like ownership.

I agree. It's been obvious to me from the beginning that we needed
such a persistent relationship, and also that it needed to be a
relationship from which the created role couldn't simply walk away.
Yet, more than six months after the first discussions of this topic,
we still don't have any kind of agreement on what that thing should be
called. I like my TENANT idea best, but I'm perfectly willing to call
it ownership as you seem to prefer or WITH ADMIN OPTION as Stephen
seems to prefer if one of those ideas gains consensus. But we've
managed to waste all hope of making any significant progress here for
an entire release cycle for lack of ability to agree on spelling. I
think that's unfair to Mark, who put a lot of work into this area and
got nothing out of it, and I think it sucks for users of PostgreSQL,
too.

(2) it seems exactly contradictory to your later point that

Agree. I also think that it would be a good idea to attribute grants
performed by any superuser to the bootstrap superuser, or leave them
unattributed somehow. Because otherwise dropping superusers becomes a
pain in the tail for no good reason.

Either there's a persistent relationship or there's not. I don't
think it's sensible to treat superusers differently here.

I think that this argument about the difficulty of dropping superusers
may in fact be the motivation for the existing behavior that object-
permissions GRANTs done by superusers are attributed to the object
owner; something you were unhappy about upthread.

In the end these requirements seem mutually contradictory. Either
we can have a persistent ownership relationship or not, but I don't
think we can have it apply in some cases and not others without
creating worse problems than we solve. I'm inclined to toss overboard
the requirement that superusers need to be an easy thing to drop.
Why is that important, anyway?

Well, I think you're looking at it the wrong way. Compared to getting
useful functionality, the relative ease of dropping users is
completely unimportant. I'm happy to surrender it in exchange for
something else. I just don't see why we should give it up for nothing.
If Alice creates non-superusers Bob and Charlie, and Charlie creates
Doug, we need the persistent relationship to know that Charlie is
allowed to drop Doug and Bob is not. But if Charlie is a superuser
anyway, then the persistent relationship is of no use. I don't see the
point of cluttering up the system with such dependencies. Will I do it
that way, if that's what it takes to get the patch accepted? Sure. But
I can't imagine any end-user actually liking it.

I'm a bit disturbed that parts of this discussion seem to be getting
conducted with little understanding of the system's existing behaviors.
We should not be reinventing things we already have perfectly good
solutions for in the object-privileges domain.

I did wonder whether that might be the existing behavior, but stopping
to check right at that moment didn't seem that important to me. Maybe
I should have taken the time, but it's not like we're writing the
final patch for commit next Tuesday at this point. It's more important
at this point to get agreement on the principles. That said, I do
agree that there have been times when we haven't thought hard enough
about the existing behavior in proposing new behavior. On the third
hand, though, part of the problem here is that neither Stephen nor I
are entirely happy with the existing behavior, if for somewhat
different reasons. It really isn't "perfectly good." On the one hand,
from a purely technical standpoint, a lot of the behavior around roles
in particular seems well below the standard that anyone would consider
committable today. On the other hand, even the parts of the code that
are in reasonable shape from a code quality point of view don't
actually do the things that we think users want done.

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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Thu, Mar 10, 2022 at 5:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

This seems reasonable in isolation, but

(1) it implies a persistent relationship between creating and created
roles. Whether you want to call that ownership or not, it sure walks
and quacks like ownership.

I agree that there would be a recorded relationship (that is, one that
we write into the catalog and keep around until and unless it's removed
by an admin) between creating and created roles and that's probably the
default when CREATE ROLE is run but, unlike tables or such objects in
the system, I don't agree that we should require this to exist at
absolutely all times for every role (what would it be for the bootstrap
superuser..?). At least today, that's distinct from how ownership in
the system works. I also don't believe that this is necessarily an
issue for Robert's use-case, as long as there are appropriate
restrictions around who is allowed to remove or modify these
relationships.

I agree. It's been obvious to me from the beginning that we needed
such a persistent relationship, and also that it needed to be a
relationship from which the created role couldn't simply walk away.
Yet, more than six months after the first discussions of this topic,
we still don't have any kind of agreement on what that thing should be
called. I like my TENANT idea best, but I'm perfectly willing to call
it ownership as you seem to prefer or WITH ADMIN OPTION as Stephen
seems to prefer if one of those ideas gains consensus. But we've
managed to waste all hope of making any significant progress here for
an entire release cycle for lack of ability to agree on spelling. I
think that's unfair to Mark, who put a lot of work into this area and
got nothing out of it, and I think it sucks for users of PostgreSQL,
too.

Well ... one of those actually already exists and also happens to be in
the SQL spec. I don't necessarily agree that we should absolutely
require that the system always enforce that this relationship exist (I'd
like a superuser to be able to get rid of it and to be able to change it
too if they want) and that seems a bit saner than having the bootstrap
superuser be special in some way here as would seem to otherwise be
required. I also feel that it would be generally useful to have more
than one of these relationships, if the user wishes, and that's
something that ownership doesn't (directly) support today. Further,
that's supported and expected by the SQL spec too. Even if we invented
some concept of ownership of roles, it seems like we should make most of
the other changes discussed here to bring us closer to what the spec
says about CREATE ROLE, DROP ROLE, GRANT, REVOKE, etc. At that point
though, what's the point of having ownership?

(2) it seems exactly contradictory to your later point that

Agree. I also think that it would be a good idea to attribute grants
performed by any superuser to the bootstrap superuser, or leave them
unattributed somehow. Because otherwise dropping superusers becomes a
pain in the tail for no good reason.

Either there's a persistent relationship or there's not. I don't
think it's sensible to treat superusers differently here.

I think that this argument about the difficulty of dropping superusers
may in fact be the motivation for the existing behavior that object-
permissions GRANTs done by superusers are attributed to the object
owner; something you were unhappy about upthread.

In the end these requirements seem mutually contradictory. Either
we can have a persistent ownership relationship or not, but I don't
think we can have it apply in some cases and not others without
creating worse problems than we solve. I'm inclined to toss overboard
the requirement that superusers need to be an easy thing to drop.
Why is that important, anyway?

Well, I think you're looking at it the wrong way. Compared to getting
useful functionality, the relative ease of dropping users is
completely unimportant. I'm happy to surrender it in exchange for
something else. I just don't see why we should give it up for nothing.
If Alice creates non-superusers Bob and Charlie, and Charlie creates
Doug, we need the persistent relationship to know that Charlie is
allowed to drop Doug and Bob is not. But if Charlie is a superuser
anyway, then the persistent relationship is of no use. I don't see the
point of cluttering up the system with such dependencies. Will I do it
that way, if that's what it takes to get the patch accepted? Sure. But
I can't imagine any end-user actually liking it.

We need to know that Charlie is allowed to drop Doug and Bob isn't but
that doesn't make it absolutely required that this be tracked
permanently or that Alice can't decide later to make it such that Doug
can't be dropped by Charlie for whatever reason she has. Also, I don't
think it would be such an issue to have a CASCADE for DROP ROLE which
would handle this case if we want it (and pg_auth_members is shared, so
there isn't an issue with multi-database concerns). We could also call
it something else if people feel CASCADE would be confusing since it
wouldn't cascade to owned objects. Or we could consider extending GRANT
to make this situation something that could be handled more easily.

Thanks,

Stephen

On Fri, Mar 11, 2022 at 6:55 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 10, 2022 at 5:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

This seems reasonable in isolation, but

(1) it implies a persistent relationship between creating and created
roles. Whether you want to call that ownership or not, it sure walks
and quacks like ownership.

I like my TENANT idea best, but I'm perfectly willing to call
it ownership as you seem to prefer or WITH ADMIN OPTION as Stephen
seems to prefer if one of those ideas gains consensus.

If WITH ADMIN OPTION is sufficient to meet our immediate goals I do not see
the benefit of adding an ownership concept where there is not one today.
If added, I'd much rather have it be ownership as to fit in with the rest
of the existing system rather than introduce an entirely new term.

If Alice creates non-superusers Bob and Charlie, and Charlie creates
Doug, we need the persistent relationship to know that Charlie is
allowed to drop Doug and Bob is not

The interesting question seems to be whether Alice can drop Doug, not
whether Bob can.

It's more important
at this point to get agreement on the principles.

What are the principles you want to get agreement on and how do they differ
from what we have in place today? What are the proposed changes you would
make to enforce the new principles. Which principles are now obsolete and
what do you want to do about the features that were built to enforce them
(including backward compatibility concerns)?

David J.

Greetings,

* David G. Johnston (david.g.johnston@gmail.com) wrote:

On Thu, Mar 10, 2022 at 3:01 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 10, 2022 at 4:00 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

I dislike changing the documented behavior of CREATEROLE to the degree

suggested here. However, there are three choices here, only one of which
can be chosen:

1. Leave CREATEROLE alone entirely
2. Make it so CREATEROLE cannot assign membership to the predefined

roles or superuser (inheritance included), but leave the rest alone. This
would be the hard-coded version, not the role attribute one.

3. Make it so CREATEROLE can only assign membership to roles for which

it has been made an admin; as well as the other things mentioned

Moving forward I'd prefer options 1 or 2, leaving the ability to

create/alter/drop a role to be vested via predefined roles.

It sounds like you prefer a behavior where CREATEROLE gives power over
all non-superusers, but that seems pretty limiting to me.

Doh! I edited out the part where I made clear I considered options 1 and 2
as basically being done for a limited period of time while deprecating the
CREATEROLE attribute altogether in favor of the fine-grained and predefined
role based permission granting. I don't want to nerf CREATEROLE as part of
adding this new feature, instead leave it as close to status quo as
reasonable so as not to mess up existing setups that make use of it. We
can note in the release notes and documentation that we consider CREATEROLE
to be deprecated and that the new predefined role should be used to give a
user the ability to create/alter/drop roles, etc... DBAs should consider
revoking CREATEROLE from their users and granting them proper memberships
in the predefined roles and the groups those roles should be administering.

I disagree entirely with the idea that we should push the out however
many years it'd take to get through some deprecation period. We are
absolutely terrible when it comes to that and what we're talking about
here, at this point anyway, is making changes that get us closer to what
the spec says. I agree that we can't back-patch these changes, but I
don't think we need a deprecation period. If we were just getting rid
of CREATEROLE, I don't think we'd have a deprecation period. If we need
to get rid of CREATEROLE and introduce something new that more-or-less
means the same thing, to make it so that people's scripts break in a
more obvious way, maybe we can consider that, but I don't really think
that's actually the case here. Such scripts as will break will still
break in a pretty clear way with a clear answer as to how to fix them
and I don't think there's some kind of data corruption or something that
would happen.

Thanks,

Stephen

On Fri, Mar 11, 2022 at 8:32 AM Stephen Frost <sfrost@snowman.net> wrote:

Such scripts as will break will still
break in a pretty clear way with a clear answer as to how to fix them
and I don't think there's some kind of data corruption or something that
would happen.

I largely agree and am perfectly fine with going with the majority on this
point. My vote would just fall on the conservative side. But as so far no
one else seems to be overly concerned, nerfing CREATEROLE seems to be the
path forward.

David J.

On Fri, Mar 11, 2022 at 10:27 AM Stephen Frost <sfrost@snowman.net> wrote:

I agree that there would be a recorded relationship (that is, one that
we write into the catalog and keep around until and unless it's removed
by an admin) between creating and created roles and that's probably the
default when CREATE ROLE is run but, unlike tables or such objects in
the system, I don't agree that we should require this to exist at
absolutely all times for every role (what would it be for the bootstrap
superuser..?). At least today, that's distinct from how ownership in
the system works. I also don't believe that this is necessarily an
issue for Robert's use-case, as long as there are appropriate
restrictions around who is allowed to remove or modify these
relationships.

I agree.

I agree. [ but we need to get consensus ]

Well ... [ how about we do it my way? ]

Repeating the same argument over again isn't necessarily going to help
anything here. I read your argument and I can believe there could be a
solution along those lines, although you haven't addressed my concern
about NOINHERIT. Tom is apparently less convinced, and you know, I
think that's OK. Not everybody has to agree with the way you want to
do it.

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

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Mar 11, 2022 at 10:27 AM Stephen Frost <sfrost@snowman.net> wrote:

I agree that there would be a recorded relationship (that is, one that
we write into the catalog and keep around until and unless it's removed
by an admin) between creating and created roles and that's probably the
default when CREATE ROLE is run but, unlike tables or such objects in
the system, I don't agree that we should require this to exist at
absolutely all times for every role (what would it be for the bootstrap
superuser..?). At least today, that's distinct from how ownership in
the system works. I also don't believe that this is necessarily an
issue for Robert's use-case, as long as there are appropriate
restrictions around who is allowed to remove or modify these
relationships.

I agree.

The bootstrap superuser clearly must be a special case in some way.
I'm not convinced that that means there should be other special
cases. Maybe there is a use-case for other "unowned" roles, but in
exactly what way would that be different from deeming such roles
to be owned by the bootstrap superuser?

regards, tom lane

On Fri, Mar 11, 2022 at 10:37 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

I largely agree and am perfectly fine with going with the majority on this point. My vote would just fall on the conservative side. But as so far no one else seems to be overly concerned, nerfing CREATEROLE seems to be the path forward.

This kind of thing is always a judgement call. If we were talking
about breaking 'SELECT * from table', I'm sure it would be hard to
convince anybody to agree to do that at all, let alone with no
deprecation period. Fortunately, CREATEROLE is less used, so breaking
it will inconvenience fewer people. Moreover, unlike 'SELECT * FROM
table', CREATEROLE is kinda broken, and it's less scary to make
changes to behavior that sucks in the first place than it is to make
changes to the behavior of things that are working well. For all of
that, there's no hard-and-fast rule that we couldn't keep the existing
behavior around, introduce a substitute, and eventually drop the old
thing. I'm just not clear that it's really worth it in this case. It'd
certainly be interesting to hear from anyone who is finding some
utility in the current system. It looks pretty crap to me, but it's
easy to bring too much of one's own bias to such judgements.

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

On Mar 11, 2022, at 7:58 AM, Robert Haas <robertmhaas@gmail.com> wrote:

This kind of thing is always a judgement call. If we were talking
about breaking 'SELECT * from table', I'm sure it would be hard to
convince anybody to agree to do that at all, let alone with no
deprecation period. Fortunately, CREATEROLE is less used, so breaking
it will inconvenience fewer people.

This issue of how much backwards compatibility breakage we're willing to tolerate is just as important as questions about how we would want roles to work in a green-field development project. The sense I got a year ago, on this list, was that changing CREATEROLE was acceptable, but changing other parts of the system, such as how ADMIN OPTION works, would go too far.

Role ownership did not yet exist, and that was a big motivation in introducing that concept, because you couldn't credibly say it broke other existing features. It introduces the new notion that when a superuser creates a role, the superuser owns it, which is identical to how things implicitly work today; and when a CREATEROLE non-superuser creates a role, that role owns the new role, which is different from how it works today, arguably breaking CREATEROLE's prior behavior. *But it doesn't break anything else*.

If we're going to change how ADMIN OPTION works, or how role membership works, or how inherit/noinherit works, let's first be clear that we are willing to accept whatever backwards incompatibility that entails. This is not a green-field development project. The constant spinning around with regard to how much compatibility we need to preserve is giving me vertigo.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Mar 11, 2022 at 10:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

The bootstrap superuser clearly must be a special case in some way.
I'm not convinced that that means there should be other special
cases. Maybe there is a use-case for other "unowned" roles, but in
exactly what way would that be different from deeming such roles
to be owned by the bootstrap superuser?

I think that just boils down to how many useless catalog entries you
want to make.

If we implement the link between the creating role and the created
role as role ownership, then we are surely just going to add a
rolowner column to pg_authid, and when the role is owned by nobody, I
think we should always just store a valid OID in it, rather than
sometimes storing 0. It just seems simpler. Any time we would store 0,
store the bootstrap superuser's pg_authid.oid value instead. That way
the OID is always valid, which probably lets us get by with fewer
special cases in the code.

If we implement the link between the creating role and the created
role as an automatically-granted WITH ADMIN OPTION, then we could
choose to put (CREATOR_OID, CREATED_OID, whatever, TRUE) into
pg_auth_members for the creating superuser or, indeed, every superuser
in the system. Or we can leave it out. The result will be exactly the
same. Here, I would favor leaving it out, because extra catalog
entries that don't do anything are usually a thing that we do not
want. See a49d081235997c67e8aab7a523b17e8d1cb93184, for example.

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

Robert Haas <robertmhaas@gmail.com> writes:

If we implement the link between the creating role and the created
role as role ownership, then we are surely just going to add a
rolowner column to pg_authid, and when the role is owned by nobody, I
think we should always just store a valid OID in it, rather than
sometimes storing 0. It just seems simpler. Any time we would store 0,
store the bootstrap superuser's pg_authid.oid value instead. That way
the OID is always valid, which probably lets us get by with fewer
special cases in the code.

+1.

Note that either case would also involve making entries in pg_shdepend;
although for the case of roles owned by/granted to the bootstrap
superuser, we could omit those on the usual grounds that we don't need
to record dependencies on pinned objects.

regards, tom lane

On Fri, Mar 11, 2022 at 11:12 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

This issue of how much backwards compatibility breakage we're willing to tolerate is just as important as questions about how we would want roles to work in a green-field development project. The sense I got a year ago, on this list, was that changing CREATEROLE was acceptable, but changing other parts of the system, such as how ADMIN OPTION works, would go too far.

Role ownership did not yet exist, and that was a big motivation in introducing that concept, because you couldn't credibly say it broke other existing features. It introduces the new notion that when a superuser creates a role, the superuser owns it, which is identical to how things implicitly work today; and when a CREATEROLE non-superuser creates a role, that role owns the new role, which is different from how it works today, arguably breaking CREATEROLE's prior behavior. *But it doesn't break anything else*.

If we're going to change how ADMIN OPTION works, or how role membership works, or how inherit/noinherit works, let's first be clear that we are willing to accept whatever backwards incompatibility that entails. This is not a green-field development project. The constant spinning around with regard to how much compatibility we need to preserve is giving me vertigo.

I mean, I agree that the backward compatibility ramifications of every
idea need to be considered, but I agree even more that the amount of
spinning around here is pretty insane. My feeling is that neither role
owners nor tenants introduce any real concerns about
backward-compatibility or, for that matter, SQL standards compliance,
nonwithstanding Stephen's argument to the contrary. Every vendor
extends the standard with their own stuff, and we've done that as
well, as we can do it in more places.

On the other hand, changing ADMIN OPTION does have compatibility and
spec-compliance ramifications. I think Stephen is arguing that we can
solve this problem while coming closer to the spec, and I think we
usually consider getting closer to the spec to be a sufficient reason
for breaking backward compatibility (cf. standard_conforming_strings).
But I don't know whether he is correct when he argues that the spec
makes admin option on a role sufficient to drop the role. I've never
had any luck understanding what the SQL specification is saying about
any topic.

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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Robert Haas <robertmhaas@gmail.com> writes:

If we implement the link between the creating role and the created
role as role ownership, then we are surely just going to add a
rolowner column to pg_authid, and when the role is owned by nobody, I
think we should always just store a valid OID in it, rather than
sometimes storing 0. It just seems simpler. Any time we would store 0,
store the bootstrap superuser's pg_authid.oid value instead. That way
the OID is always valid, which probably lets us get by with fewer
special cases in the code.

We haven't got any particularly special cases in the code today for what
happens if we run up the role hierarchy to a point that it ends and so
I'm not sure why adding in a whole new concept around role ownership,
which doesn't exist in the spec, would somehow leave us with fewer such
special cases.

+1.

Note that either case would also involve making entries in pg_shdepend;
although for the case of roles owned by/granted to the bootstrap
superuser, we could omit those on the usual grounds that we don't need
to record dependencies on pinned objects.

That we aren't discussing the issues with the current GRANT ... WITH
ADMIN OPTION and how we deviate from what the spec calls for when it
comes to DROP ROLE, which seems to be the largest thing that's
'solved' with this ownership concept, is concerning to me.

If we go down the route of adding role ownership, are we going to
document that we explicitly go against the SQL standard when it comes to
how DROP ROLE works? Or are we going to fix DROP ROLE? I'd much prefer
the latter, but doing so then largely negates the point of this role
ownership concept. I don't see how it makes sense to do both.

Thanks,

Stephen

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Fri, Mar 11, 2022 at 11:12 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

This issue of how much backwards compatibility breakage we're willing to tolerate is just as important as questions about how we would want roles to work in a green-field development project. The sense I got a year ago, on this list, was that changing CREATEROLE was acceptable, but changing other parts of the system, such as how ADMIN OPTION works, would go too far.

That we deviate as far as we do when it comes to the SQL spec is
something that I don't feel like I had a good handle on when discussing
this previously (that the spec doesn't talk about 'admin option' really
but rather 'grantable authorization identifiers' or whatever it is
doesn't help... but still, that's on me, sorry about that).

Role ownership did not yet exist, and that was a big motivation in introducing that concept, because you couldn't credibly say it broke other existing features. It introduces the new notion that when a superuser creates a role, the superuser owns it, which is identical to how things implicitly work today; and when a CREATEROLE non-superuser creates a role, that role owns the new role, which is different from how it works today, arguably breaking CREATEROLE's prior behavior. *But it doesn't break anything else*.

If we're going to change how ADMIN OPTION works, or how role membership works, or how inherit/noinherit works, let's first be clear that we are willing to accept whatever backwards incompatibility that entails. This is not a green-field development project. The constant spinning around with regard to how much compatibility we need to preserve is giving me vertigo.

I agree that it would have an impact on backwards compatibility to
change how WITH ADMIN works- but it would also get us more in line with
what the SQL standard says for how WITH ADMIN is supposed to work and
that seems worth the change to me.

On the other hand, changing ADMIN OPTION does have compatibility and
spec-compliance ramifications. I think Stephen is arguing that we can
solve this problem while coming closer to the spec, and I think we
usually consider getting closer to the spec to be a sufficient reason
for breaking backward compatibility (cf. standard_conforming_strings).

Indeed.

But I don't know whether he is correct when he argues that the spec
makes admin option on a role sufficient to drop the role. I've never
had any luck understanding what the SQL specification is saying about
any topic.

I'm happy to point you to what the spec says and to discuss it further
if that would be helpful, or to get other folks to comment on it. I
agree that it's definitely hard to grok at times. In this particular
case what I'm looking at is, under DROP ROLE / Access Rules, there's
only one sentence:

There shall exist at least one grantable role authorization descriptor
whose role name is R and whose grantee is an enabled authorization
identifier.

A bit of decoding: 'grantable role authorization descriptor' is a GRANT
of a role WITH ADMIN OPTION. The role name 'R' is the role specified.
The 'grantee' is who that role R was GRANT'd to, and 'enabled
authorization identifier' is basically "has_privs_of_role()" (note that
you can in the spec hvae roles that you're a member of but which are
*not* currently enabled).

Hopefully that helps.

Thanks,

Stephen

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

Note that either case would also involve making entries in pg_shdepend;
although for the case of roles owned by/granted to the bootstrap
superuser, we could omit those on the usual grounds that we don't need
to record dependencies on pinned objects.

That makes sense to me, but it still doesn't solve the problem of
agreeing on role ownership vs. WITH ADMIN OPTION vs. something else.

I find it ironic (and frustrating) that Mark implemented what I think
is basically what you're arguing for, it got stuck because Stephen
didn't like it, we then said OK so let's try to find out what Stephen
would like, only to have you show up and say that it's right the way
he already had it. I'm not saying that you're wrong, or for that
matter that he's wrong. I'm just saying that if both of you are
absolutely bent on having it the way you want it, either one of you is
going to be sad, or we're not going to make any progress.

Never mind the fact that neither of you seem interested in even giving
a hearing to my preferred way of doing it. :-(

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

On Mar 11, 2022, at 8:48 AM, Stephen Frost <sfrost@snowman.net> wrote:

I agree that it would have an impact on backwards compatibility to
change how WITH ADMIN works- but it would also get us more in line with
what the SQL standard says for how WITH ADMIN is supposed to work and
that seems worth the change to me.

I'm fine with giving up some backwards compatibility to get some SQL standard compatibility, as long as we're clear that is what we're doing. What you say about the SQL spec isn't great, though, because too much power is vested in "ADMIN". I see "ADMIN" as at least three separate privileges together. Maybe it would be spec compliant to implement "ADMIN" as a synonym for a set of separate privileges?

On Mar 11, 2022, at 8:41 AM, Stephen Frost <sfrost@snowman.net> wrote:

That we aren't discussing the issues with the current GRANT ... WITH
ADMIN OPTION and how we deviate from what the spec calls for when it
comes to DROP ROLE, which seems to be the largest thing that's
'solved' with this ownership concept, is concerning to me.

Sure, let's discuss that a bit more. Here is my best interpretation of your post about the spec, when applied to postgres with an eye towards not doing any more damage than necessary:

On Mar 10, 2022, at 11:58 AM, Stephen Frost <sfrost@snowman.net> wrote:

let's look at what the spec says:

CREATE ROLE
- Who is allowed to run CREATE ROLE is implementation-defined

This should be anyone with membership in pg_create_role.

- After creation, this is effictively run:
GRANT new_role TO creator_role WITH ADMIN, GRANTOR "_SYSTEM"

This should internally be implemented as three separate privileges, one which means you can grant the role, another which means you can drop the role, and a third that means you're a member of the role. That way, they can be independently granted and revoked. We could make "WITH ADMIN" a short-hand for "WITH G, D, M" where G, D, and M are whatever we name the independent privileges Grant, Drop, and Member-of.

Splitting G and D helps with backwards compatibility, because it gives people who want the traditional postgres "admin" a way to get there, by granting "G+M". Splitting M from G and D makes it simpler to implement the "bot" idea, since the bot shouldn't have M. But it does raise a question about always granting G+D+M to the creator, since the bot is the creator and we don't want the bot to have M. This isn't a problem I've invented from thin air, mind you, as G+D+M is just the definition of ADMIN per the SQL spec, if I've understood you correctly. So we need to think a bit more about the pg_create_role built-in role and whether that needs to be further refined to distinguish those who can get membership in roles they create vs. those who cannot. This line of reasoning takes me in the direction of what I think you were calling #5 upthread, but you'd have to elaborate on that, and how it interacts with the spec, for us to have a useful conversation about it.

DROP ROLE
- Any user who has been GRANT'd a role with ADMIN option is able to
DROP that role.

Change this to "Any role who has D on the role". That's spec compliant, because anyone granted ADMIN necessarily has D.

GRANT ROLE
- No cycles allowed
- A role must have ADMIN rights on the role to be able to GRANT it to
another role.

Change this to "Any role who has G on the role". That's spec compliant, because anyone grant ADMIN necessarily has G.

We should also fix the CREATE ROLE command to require the grantor have G on a role in order to give it to the new role as part of the command. Changing the CREATEROLE, CREATEDB, REPLICATION, and BYPASSRLS attributes into pg_create_role, pg_create_db, pg_replication, and pg_bypassrls, the creator could only give them to the created role if the creator has G on the roles. If we do this, we could keep the historical privilege bits and their syntax support for backward compatibility, or we could rip them out, but the decision between those two options is independent of the rest of the design.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

On Fri, Mar 11, 2022 at 12:32 Mark Dilger <mark.dilger@enterprisedb.com>
wrote:

On Mar 11, 2022, at 8:48 AM, Stephen Frost <sfrost@snowman.net> wrote:

I agree that it would have an impact on backwards compatibility to
change how WITH ADMIN works- but it would also get us more in line with
what the SQL standard says for how WITH ADMIN is supposed to work and
that seems worth the change to me.

I'm fine with giving up some backwards compatibility to get some SQL
standard compatibility, as long as we're clear that is what we're doing.
What you say about the SQL spec isn't great, though, because too much power
is vested in "ADMIN". I see "ADMIN" as at least three separate privileges
together. Maybe it would be spec compliant to implement "ADMIN" as a
synonym for a set of separate privileges?

I do think that’s reasonable … and believe I suggested it about 3 messages
ago in this thread. ;) (step #3 I think it was? Or maybe 4).

On Mar 11, 2022, at 8:41 AM, Stephen Frost <sfrost@snowman.net> wrote:

That we aren't discussing the issues with the current GRANT ... WITH
ADMIN OPTION and how we deviate from what the spec calls for when it
comes to DROP ROLE, which seems to be the largest thing that's
'solved' with this ownership concept, is concerning to me.

Sure, let's discuss that a bit more. Here is my best interpretation of
your post about the spec, when applied to postgres with an eye towards not
doing any more damage than necessary:

On Mar 10, 2022, at 11:58 AM, Stephen Frost <sfrost@snowman.net> wrote:

let's look at what the spec says:

CREATE ROLE
- Who is allowed to run CREATE ROLE is implementation-defined

This should be anyone with membership in pg_create_role.

That could be the case if we wished to go that route. I’d think in such
case we’d then also remove CREATEROLE as otherwise the documentation feels
like it’d be quite confusing.

- After creation, this is effictively run:

GRANT new_role TO creator_role WITH ADMIN, GRANTOR "_SYSTEM"

This should internally be implemented as three separate privileges, one
which means you can grant the role, another which means you can drop the
role, and a third that means you're a member of the role. That way, they
can be independently granted and revoked. We could make "WITH ADMIN" a
short-hand for "WITH G, D, M" where G, D, and M are whatever we name the
independent privileges Grant, Drop, and Member-of.

I mean, sure, we can get there, and possibly add more like if you’re
allowed to change or reset that role’s password and other things, but I
don’t see that this piece is required as part of the very first change in
this area. Further, WITH ADMIN already gives grant and member today, so
you’re saying the only thing this change does that makes “WITH ADMIN” too
powerful is adding DROP to it, yet that’s explicitly what the spec calls
for. In short, I disagree that moving the DROP ROLE right from CREATEROLE
roles having that across the entire system (excluding superusers) to WITH
ADMIN where the role who has that right can:

A) already become that role and drop all their objects
B) already GRANT that role to some other role

is a big issue.

Splitting G and D helps with backwards compatibility, because it gives

people who want the traditional postgres "admin" a way to get there, by
granting "G+M". Splitting M from G and D makes it simpler to implement the
"bot" idea, since the bot shouldn't have M. But it does raise a question
about always granting G+D+M to the creator, since the bot is the creator
and we don't want the bot to have M. This isn't a problem I've invented
from thin air, mind you, as G+D+M is just the definition of ADMIN per the
SQL spec, if I've understood you correctly. So we need to think a bit more
about the pg_create_role built-in role and whether that needs to be further
refined to distinguish those who can get membership in roles they create
vs. those who cannot. This line of reasoning takes me in the direction of
what I think you were calling #5 upthread, but you'd have to elaborate on
that, and how it interacts with the spec, for us to have a useful
conversation about it.

All that said, as I said before, I’m in favor of splitting things up and so
if you want to do that as part of this initial work, sure. Idk that it’s
absolutely required as part of this but I’m not going to complain if it’s
included either. I agree that would allow folks to get something similar
to what they could get today if they want.

I agree that the split up helps with the “bot” idea, as we could at least
then create a security definer function that the bot runs and which creates
roles that the bot then has G for but not M or D. Even better would be to
also provide a way for the “bot” to be able to create roles without the
need for a security definer function where it doesn’t automatically get all
three, and that was indeed what I was thinking about with the template
idea. The general thought there being that an admin could define a template
along the lines of:

CREATE TEMPLATE employee_template
CREATOR WITH ADMIN, NOMEMBERSHIP
ROLE IN employee;

And then provide a way for the bot to be given the right to use this
template. Thinking on it a bit further, I’m guessing that we wouldn’t
actually give the bot pg_create_role in this case and instead would leave
that to mean “able to create arbitrary roles and have all privs in that”
similar to what we are talking about where ADMIN implies the full set of
rights.

DROP ROLE

- Any user who has been GRANT'd a role with ADMIN option is able to
DROP that role.

Change this to "Any role who has D on the role". That's spec compliant,
because anyone granted ADMIN necessarily has D.

Yeah.

GRANT ROLE

- No cycles allowed
- A role must have ADMIN rights on the role to be able to GRANT it to
another role.

Change this to "Any role who has G on the role". That's spec compliant,
because anyone grant ADMIN necessarily has G.

Sure.

We should also fix the CREATE ROLE command to require the grantor have G on

a role in order to give it to the new role as part of the command.

… or just get rid of it, which seems saner to me.

Changing the CREATEROLE, CREATEDB, REPLICATION, and BYPASSRLS attributes

into pg_create_role, pg_create_db, pg_replication, and pg_bypassrls, the
creator could only give them to the created role if the creator has G on
the roles. If we do this, we could keep the historical privilege bits and
their syntax support for backward compatibility, or we could rip them out,
but the decision between those two options is independent of the rest of
the design.

Yeah, turning those into predefined roles which an admin can then decide to
give out (and to allow ADMIN on them to be given to folks who could then
pass that along if they wanted) is another thought I’ve had though one
that’s somewhat independent of the rest of this, but also shows how we
could make those be things that a superuser could choose to give out, or
not, to some set of roles who would then be able to create roles of their
own with those privileges.

On the whole, using predefined roles as the source of certain capabilities,
and the options discussed here which would allow an admin to grant those
capabilities out with or without the ability to grant them further, plus
the splitting out of the individual role-relationship rights (membership,
grantable, drop, etc) strikes me as being quite flexible and extendable and
generally in the direction that we’ve been trending and which seems to be
reasonably successful so far.

Thanks,

Stephen

On Mar 11, 2022, at 2:46 PM, Stephen Frost <sfrost@snowman.net> wrote:

I do think that’s reasonable … and believe I suggested it about 3 messages ago in this thread. ;) (step #3 I think it was? Or maybe 4).

Yes, and you mentioned it to me off-list.

I'm soliciting a more concrete specification for what you are proposing. To me, that means understanding how the SQL spec behavior that you champion translates into specific changes. You specified some of this in steps #1 through #5, but I'd like a clearer indication of how many of those (#1 alone, both #1 and #2, or what?) constitute a competing idea to the idea of role ownership, and greater detail about how each of those steps translate into specific behavior changes in postgres. Your initial five-step email seems to be claiming that #1 by itself is competitive, but to me it seems at least #1 and #2 would be required.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

On Fri, Mar 11, 2022 at 19:03 Mark Dilger <mark.dilger@enterprisedb.com>
wrote:

On Mar 11, 2022, at 2:46 PM, Stephen Frost <sfrost@snowman.net> wrote:

I do think that’s reasonable … and believe I suggested it about 3

messages ago in this thread. ;) (step #3 I think it was? Or maybe 4).

Yes, and you mentioned it to me off-list.

Indeed.

I'm soliciting a more concrete specification for what you are proposing.

To me, that means understanding how the SQL spec behavior that you champion
translates into specific changes. You specified some of this in steps #1
through #5, but I'd like a clearer indication of how many of those (#1
alone, both #1 and #2, or what?) constitute a competing idea to the idea of
role ownership, and greater detail about how each of those steps translate
into specific behavior changes in postgres. Your initial five-step email
seems to be claiming that #1 by itself is competitive, but to me it seems
at least #1 and #2 would be required.

First … I outlined a fair bit of further description in the message you
just responded to but neglected to include in your response, which strikes
me as odd that you’re now asking for further explanation. When it comes to
completing the idea of role ownership- I didn’t come up with that idea nor
champion it and therefore I’m not really sure how many of the steps are
required to fully support that concept..? For my part, I would think that
those steps necessary to satisfy the spec would get us pretty darn close to
what true folks advocating for role ownership are asking for, but that
doesn’t include the superuser-only alter role attributes piece. Is that
included in role ownership? I wouldn’t think so, but some might argue
otherwise, and I don’t know that it is actually useful to divert into a
discussion about what is or isn’t in that.

If we agree that the role attribute bits are independent then I think I
agree that we need 1 and 2 to get the capabilities that the folks asking
for role ownership want, as 2 is where we make sure that one admin of a
role can’t revoke another admin’s rights over that role. Perhaps 2 isn’t
strictly necessary in a carefully managed environment where no one else is
given admin rights over the mini-superuser roles, but I’d rather not have
folks depending on that. I’d still push back though and ask those
advocating for this if it meets what they’re asking for. I got the
impression that it did but maybe I misunderstood.

In terms of exactly how things would work with these changes… I thought I
explained it pretty clearly, so it’s kind of hard to answer that further
without a specific question to answer. Did you have something specific in
mind? Perhaps I could answer a specific question and provide more clarity
that way.

Thanks,

Stephen

On Mar 11, 2022, at 4:56 PM, Stephen Frost <sfrost@snowman.net> wrote:

First … I outlined a fair bit of further description in the message you just responded to but neglected to include in your response, which strikes me as odd that you’re now asking for further explanation.

When it comes to completing the idea of role ownership- I didn’t come up with that idea nor champion it

Sorry, not "completing", but "competing". It seems we're discussing different ways to fix how roles and CREATEROLE work, and we have several ideas competing against each other. (This differs from *people* competing against each other, as I don't necessarily like the patch I wrote better than I like your idea.)

and therefore I’m not really sure how many of the steps are required to fully support that concept..?

There are problems that the ownership concepts solve. I strongly suspect that your proposal could also solve those same problems, and just trying to identify the specific portions of your proposal necessary to do so.

For my part, I would think that those steps necessary to satisfy the spec would get us pretty darn close to what true folks advocating for role ownership are asking for

I have little idea what "true folks" means in this context. As for "advocating for role ownership", I'm not in that group. Whether role ownership or something else, I just want some solution to a set of problems, mostly to do with needing superuser to do role management tasks.

, but that doesn’t include the superuser-only alter role attributes piece. Is that included in role ownership? I wouldn’t think so, but some might argue otherwise, and I don’t know that it is actually useful to divert into a discussion about what is or isn’t in that.

Introducing the idea of role ownership doesn't fix that. But a patch which introduces role ownership is useless unless CREATEROLE is also fixed. There isn't any point having non-superusers create and own roles if, to do so, they need a privilege which can break into superuser. But that argument is no different with a patch along the lines of what you are proposing. CREATEROLE needs fixing either way.

If we agree that the role attribute bits are independent

Yes.

then I think I agree that we need 1 and 2 to get the capabilities that the folks asking for role ownership want

Yes.

as 2 is where we make sure that one admin of a role can’t revoke another admin’s rights over that role.

Exactly, so #2 is part of the competing proposal. (I get the sense you might not see these as competing proposals, but I find that framing useful for deciding which approach to pursue.)

Perhaps 2 isn’t strictly necessary in a carefully managed environment where no one else is given admin rights over the mini-superuser roles, but I’d rather not have folks depending on that.

I think it is necessary, and for the reason you say.

I’d still push back though and ask those advocating for this if it meets what they’re asking for. I got the impression that it did but maybe I misunderstood.

In terms of exactly how things would work with these changes… I thought I explained it pretty clearly, so it’s kind of hard to answer that further without a specific question to answer. Did you have something specific in mind? Perhaps I could answer a specific question and provide more clarity that way.

Your emails contained a lot of "we could do this or that depending on what people want, and maybe this other thing, but that isn't really necessary, and ...." which left me unclear on the proposal. I don't mean to disparage your communication style; it's just that when trying to distill technical details, high level conversation can be hard to grok.

I have the sense that you aren't going to submit a patch, so I wanted this thread to contain enough detail for somebody else to do so. Thanks.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:

On Mar 11, 2022, at 4:56 PM, Stephen Frost <sfrost@snowman.net> wrote:
First … I outlined a fair bit of further description in the message you just responded to but neglected to include in your response, which strikes me as odd that you’re now asking for further explanation.

When it comes to completing the idea of role ownership- I didn’t come up with that idea nor champion it

Sorry, not "completing", but "competing". It seems we're discussing different ways to fix how roles and CREATEROLE work, and we have several ideas competing against each other. (This differs from *people* competing against each other, as I don't necessarily like the patch I wrote better than I like your idea.)

and therefore I’m not really sure how many of the steps are required to fully support that concept..?

There are problems that the ownership concepts solve. I strongly suspect that your proposal could also solve those same problems, and just trying to identify the specific portions of your proposal necessary to do so.

I'm happy to help try to identify those, but it seems we'd need to have
the exact problems that ownership solves defined first. Robert defined
what he's looking for as:

Robert Haas <robertmhaas@gmail.com> wrote:

Probably easier to just say it again: I want to have users that can
create roles and then have superuser-like powers with respect to those
roles. They can freely exercise the privileges of those roles, and
they can do all the things that a superuser can do but only with
respect to those roles. They cannot break out to the OS. I think it's
pretty similar to what you are describing, with a couple of possible
exceptions. For example, would you imagine that being an admin of a
login role would let you change that user's password? Because that
would be desirable behavior from where I sit.

Which sure sounds like it's just about covered in step #1 of what I
outlined before, except that the above description implies that one
can't "get away" from the user who created their role, in which case we
do need step #2 also.

For my part, I would think that those steps necessary to satisfy the spec would get us pretty darn close to what true folks advocating for role ownership are asking for

I have little idea what "true folks" means in this context. As for "advocating for role ownership", I'm not in that group. Whether role ownership or something else, I just want some solution to a set of problems, mostly to do with needing superuser to do role management tasks.

... I'm not entirely sure what I meant there either, my hunch is that
'true' was actually just a leftover word from some other framing of that
sentence and I had meant to remove it. Apologies for that. What I was
trying to get at there is that steps #1 & #2 are the ones that I view as
getting us closer to spec compliance and that doing so would get us to
where Robert's ask above would be answered.

, but that doesn’t include the superuser-only alter role attributes piece. Is that included in role ownership? I wouldn’t think so, but some might argue otherwise, and I don’t know that it is actually useful to divert into a discussion about what is or isn’t in that.

Introducing the idea of role ownership doesn't fix that. But a patch which introduces role ownership is useless unless CREATEROLE is also fixed. There isn't any point having non-superusers create and own roles if, to do so, they need a privilege which can break into superuser. But that argument is no different with a patch along the lines of what you are proposing. CREATEROLE needs fixing either way.

There's a few ways to have the 'CREATEROLE' role attribute be fixed-

- Remove it entirely (replacing with pg_create_role or such)
- Remove its ability to GRANT out rights that the role running it
doesn't have
- Make it superfluous (leave it as-is, but add in pg_create_role which
allows a role to create another role but doesn't include the magic
GRANT whatever-role TO whatever-role that CREATEROLE has)

I agree that we need to do something here to allow roles to create other
roles while not having or being able to trivially get superuser
themselves.

If we agree that the role attribute bits are independent

Yes.

Great.

then I think I agree that we need 1 and 2 to get the capabilities that the folks asking for role ownership want

Yes.

Ok.

as 2 is where we make sure that one admin of a role can’t revoke another admin’s rights over that role.

Exactly, so #2 is part of the competing proposal. (I get the sense you might not see these as competing proposals, but I find that framing useful for deciding which approach to pursue.)

... and is also part of getting us closer to the spec.

Perhaps 2 isn’t strictly necessary in a carefully managed environment where no one else is given admin rights over the mini-superuser roles, but I’d rather not have folks depending on that.

I think it is necessary, and for the reason you say.

Great.

I’d still push back though and ask those advocating for this if it meets what they’re asking for. I got the impression that it did but maybe I misunderstood.

In terms of exactly how things would work with these changes… I thought I explained it pretty clearly, so it’s kind of hard to answer that further without a specific question to answer. Did you have something specific in mind? Perhaps I could answer a specific question and provide more clarity that way.

Your emails contained a lot of "we could do this or that depending on what people want, and maybe this other thing, but that isn't really necessary, and ...." which left me unclear on the proposal. I don't mean to disparage your communication style; it's just that when trying to distill technical details, high level conversation can be hard to grok.

Feel free to quote me explicitly in such places that you're looking for
clarification and I'd be happy to drill down on those.

I have the sense that you aren't going to submit a patch, so I wanted this thread to contain enough detail for somebody else to do so. Thanks.

So ... do you feel like that's now the case? Or were you looking for
more?

Thanks,

Stephen

On Mar 14, 2022, at 7:38 AM, Stephen Frost <sfrost@snowman.net> wrote:

So ... do you feel like that's now the case? Or were you looking for
more?

I don't have any more questions at the moment. Thanks!

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Mar 11, 2022 at 11:51 AM Robert Haas <robertmhaas@gmail.com> wrote:

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

Note that either case would also involve making entries in pg_shdepend;
although for the case of roles owned by/granted to the bootstrap
superuser, we could omit those on the usual grounds that we don't need
to record dependencies on pinned objects.

That makes sense to me, but it still doesn't solve the problem of
agreeing on role ownership vs. WITH ADMIN OPTION vs. something else.

Notwithstanding the lack of agreement on that point, I believe that
what we should do for v15 is remove the session user
self-administration exception. We have pretty much established that it
was originally introduced in error. It later was found to be a
security vulnerability, and that resulted in the exception being
narrowed without removing it altogether. While there are differences
of opinion on what the larger plan here ought to be, nobody's proposal
plan involves retaining that exception. Neither has anyone offered a
plausible use case for the current behavior, so there's no reason to
think that removing it would break anything.

However, it might. And if it does, I think it would be best if
removing that exception were the *only* change in this area made by
that release. If for v16 or v17 or v23 we implement Plan Tom or Plan
Stephen or Plan Robert or something else, and along the way we remove
that self-administration exception, we're going to have a real fire
drill if it turns out that the self-administration exception was
important for some reason we're not seeing right now. If, on the other
hand, we remove that exception in v15, then if anything breaks, it'll
be a lot easier to deal with. Worst case scenario we just revert the
removal of that exception, which will be a very localized change if
nothing else has been done that depends heavily on its having been
removed.

So I propose to commit something like what I posted here:

/messages/by-id/CA+TgmobgeK0JraOwQVPqhSXcfBdFitXSomoebHMMMhmJ4gLonw@mail.gmail.com

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

Robert Haas <robertmhaas@gmail.com> writes:

Notwithstanding the lack of agreement on that point, I believe that
what we should do for v15 is remove the session user
self-administration exception. We have pretty much established that it
was originally introduced in error.

Agreed.

However, it might. And if it does, I think it would be best if
removing that exception were the *only* change in this area made by
that release.

Good idea, especially since it's getting to be too late to consider
anything more invasive anyway.

So I propose to commit something like what I posted here:
/messages/by-id/CA+TgmobgeK0JraOwQVPqhSXcfBdFitXSomoebHMMMhmJ4gLonw@mail.gmail.com

+1, although the comments might need some more work. In particular,
I'm not sure that this bit is well stated:

+	 * A role cannot have WITH ADMIN OPTION on itself, because that would
+	 * imply a membership loop.

We already do consider a role to be a member of itself:

regression=# create role r;
CREATE ROLE
regression=# grant r to r;
ERROR: role "r" is a member of role "r"
regression=# grant r to r with admin option;
ERROR: role "r" is a member of role "r"

It might be better to just say "By policy, a role cannot have WITH ADMIN
OPTION on itself". But if you want to write a defense of that policy,
this isn't a very good one.

regards, tom lane

On Thu, Mar 24, 2022 at 1:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

However, it might. And if it does, I think it would be best if
removing that exception were the *only* change in this area made by
that release.

Good idea, especially since it's getting to be too late to consider
anything more invasive anyway.

I'd say it's definitely too late at this point.

So I propose to commit something like what I posted here:
/messages/by-id/CA+TgmobgeK0JraOwQVPqhSXcfBdFitXSomoebHMMMhmJ4gLonw@mail.gmail.com

+1, although the comments might need some more work. In particular,
I'm not sure that this bit is well stated:

+        * A role cannot have WITH ADMIN OPTION on itself, because that would
+        * imply a membership loop.

We already do consider a role to be a member of itself:

regression=# create role r;
CREATE ROLE
regression=# grant r to r;
ERROR: role "r" is a member of role "r"
regression=# grant r to r with admin option;
ERROR: role "r" is a member of role "r"

It might be better to just say "By policy, a role cannot have WITH ADMIN
OPTION on itself". But if you want to write a defense of that policy,
this isn't a very good one.

That sentence is present in the current code, along with a bunch of
other sentences, which the patch renders irrelevant. So I just deleted
all of the other stuff and kept the sentence that is still relevant to
the revised code. I think your proposed replacement is an improvement,
but let's be careful not to get sucked into too much of a wordsmithing
exercise in a patch that's here to make a functional change.

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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Thu, Mar 24, 2022 at 1:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

However, it might. And if it does, I think it would be best if
removing that exception were the *only* change in this area made by
that release.

Good idea, especially since it's getting to be too late to consider
anything more invasive anyway.

I'd say it's definitely too late at this point.

Agreed.

So I propose to commit something like what I posted here:
/messages/by-id/CA+TgmobgeK0JraOwQVPqhSXcfBdFitXSomoebHMMMhmJ4gLonw@mail.gmail.com

+1, although the comments might need some more work. In particular,
I'm not sure that this bit is well stated:

Also +1 on this.

Thanks,

Stephen

On Mon, Mar 28, 2022 at 10:51 AM Stephen Frost <sfrost@snowman.net> wrote:

So I propose to commit something like what I posted here:
/messages/by-id/CA+TgmobgeK0JraOwQVPqhSXcfBdFitXSomoebHMMMhmJ4gLonw@mail.gmail.com

+1, although the comments might need some more work. In particular,
I'm not sure that this bit is well stated:

Also +1 on this.

OK, done using Tom's proposed wording.

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

We are definitely doing it wrong. It's not that we aren't doing it at
all, but we are doing it incorrectly. If user foo executes "GRANT foo
TO bar GRANTED BY quux", pg_auth_members.grantor gets the OID of role
"quux". Without the "GRANTED BY" clause, it gets the OID of role
"foo". But no dependency is created; therefore, the OID of that column
can point to a role that no longer exists, or potentially to a role
that did exist at one point, was dropped, and was later replaced by
some other role that happens to get the same OID. pg_dump handles this
by dumping "GRANTED BY whoever" if pg_auth_members.grantor is a
still-extant role and omitting the clause if not. This would be a
security vulnerability if there were any logic in the backend that
actually did anything with pg_auth_members.grantor, because if the
original grantor is removed and replaced by another role with the same
OID, a dump/restore could change the notional grantor. Since there is
no such logic, I don't think it's insecure, but it's still really
lame.

So let's talk about how we could fix this. In a vacuum I'd say this is
just a feature that never got finished and we should rip the whole
thing out. That is, remove pg_auth_members.grantor entirely and at
most keep some do-nothing syntax around for backward compatibility.
However, what Tom is saying in the text quoted above is that we ought
to have something that actually works, which is more challenging.
Apparently, the desired behavior here is for this to work like grants
on non-role objects, where executing "GRANT SELECT ON TABLE s1 TO foo"
under two different user accounts bar and baz that both have
permissions to grant that privilege creates two independent grants
that can be independently revoked. To get there, we'll have to change
a good few things -- not only will we need a dependency to prevent a
grantor from being dropped without revoking the grant, but we need to
change the primary key of pg_auth_members from (roleid, member) to
(roleid, member, grantor). Then we'll also have to change the behavior
of the GRANT and REVOKE commands at the SQL level, and also the
behavior of pg_dump, which will need to dump and restore all grants.

I'm open to other proposals, but my thought is that it might be
simplest to try to clean this up in two steps. In step one, the only
goal would be to make pg_auth_members.grantor reliably sane. In other
words, we'd add a dependency on the grantor when a role is granted to
another role. You could still only have one grant of role A to role B,
but the notional grantor C would always be a user that actually
exists. I suspect it would be a really good idea to also patch pg_dump
to not ever dump the grantor when working from an older release,
because the information is not necessarily reliable and I fear that
propagating it forward could lead to broken stuff or maybe even
security hazards as noted above. Then, in step two, we change things
around to allow multiple grants of the same role to the same other
role, one per grantor. Now you've achieved parity between the behavior
we have for roles and the behavior we have for permissions on other
kinds of SQL objects.

There may be other improvements we want to make in this area -
previous discussions have suggested various ideas - but it seems to me
that making the behavior sane and consistent with other types of
objects would be a good start. That way, if we decide we do want to
change anything else, we will be starting from a firm foundation,
rather than building on sand.

Thoughts?

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

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

So let's talk about how we could fix this. In a vacuum I'd say this is
just a feature that never got finished and we should rip the whole
thing out. That is, remove pg_auth_members.grantor entirely and at
most keep some do-nothing syntax around for backward compatibility.
However, what Tom is saying in the text quoted above is that we ought
to have something that actually works, which is more challenging.
Apparently, the desired behavior here is for this to work like grants
on non-role objects, where executing "GRANT SELECT ON TABLE s1 TO foo"
under two different user accounts bar and baz that both have
permissions to grant that privilege creates two independent grants
that can be independently revoked.

Maybe. What I was pointing out is that this is SQL-standard syntax
and there are SQL-standard semantics that it ought to be implementing.
Probably those semantics match what you describe here, but we ought
to dive into the spec and make sure before we spend a lot of effort.
It's not quite clear to me whether the spec defines any particular
unique key (identity) for the set of role authorizations.

regards, tom lane

On Thu, Jun 2, 2022 at 3:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Maybe. What I was pointing out is that this is SQL-standard syntax
and there are SQL-standard semantics that it ought to be implementing.
Probably those semantics match what you describe here, but we ought
to dive into the spec and make sure before we spend a lot of effort.
It's not quite clear to me whether the spec defines any particular
unique key (identity) for the set of role authorizations.

I sort of thought /messages/by-id/3981966.1646429663@sss.pgh.pa.us
constituted a completed investigation of this sort. No?

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

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Jun 2, 2022 at 3:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Maybe. What I was pointing out is that this is SQL-standard syntax
and there are SQL-standard semantics that it ought to be implementing.
Probably those semantics match what you describe here, but we ought
to dive into the spec and make sure before we spend a lot of effort.
It's not quite clear to me whether the spec defines any particular
unique key (identity) for the set of role authorizations.

I sort of thought /messages/by-id/3981966.1646429663@sss.pgh.pa.us
constituted a completed investigation of this sort. No?

I didn't think so. It's clear that the spec expects us to track the
grantor, but I didn't chase down what it expects us to *do* with that
information, nor what it thinks the rules are for merging multiple
authorizations.

regards, tom lane

On Thu, Jun 2, 2022 at 3:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I sort of thought /messages/by-id/3981966.1646429663@sss.pgh.pa.us
constituted a completed investigation of this sort. No?

I didn't think so. It's clear that the spec expects us to track the
grantor, but I didn't chase down what it expects us to *do* with that
information, nor what it thinks the rules are for merging multiple
authorizations.

Hmm, OK. Well, one problem is that I've never had any luck
interpreting what the spec says about anything, and I've sort of given
up. But even if that were not so, I'm a little unclear what other
conclusion is possible here. The spec either wants the same behavior
that we already have for other object types, which is what I am here
proposing that we do, or it wants something different. If it wants
something different, it probably wants that for all object types, not
just roles. Since I doubt we would want the behavior for roles to be
inconsistent with what we do for all other object types, in that case
we would probably either change the behavior for all other object
types to something new, and then clean up the role stuff afterwards,
or else first do what I proposed here and then later change it all at
once. In which case the proposal that I've made is as good a way to
start as any.