Thread: allowing for control over SET ROLE

Hi,

There are two ways in which a role can exercise the privileges of some
other role which has been granted to it. First, it can implicitly
inherit the privileges of the granted role. Second, it can assume the
identity of the granted role using the SET ROLE command. It is
possible to control the former behavior, but not the latter. In v15
and prior release, we had a role-level [NO]INHERIT property which
controlled whether a role automatically inherited the privileges of
any role granted to it. This was all-or-nothing. Beginning in
e3ce2de09d814f8770b2e3b3c152b7671bcdb83f, the inheritance behavior of
role-grants can be overridden for individual grants, so that some
grants are inherited and others are not. However, there is no similar
facility for controlling whether a role can SET ROLE to some other
role of which it is a member. At present, if role A is a member of
role B, then A can SET ROLE B, and that's it.

In some circumstances, it may be desirable to control this behavior.
For example, if we GRANT pg_read_all_settings TO seer, we do want the
seer to be able to read all the settings, else we would not have
granted the role. But we might not want the seer to be able to do
this:

You are now connected to database "rhaas" as user "seer".
rhaas=> set role pg_read_all_settings;
SET
rhaas=> create table artifact (a int);
CREATE TABLE
rhaas=> \d
                List of relations
 Schema |   Name   | Type  |        Owner
--------+----------+-------+----------------------
 public | artifact | table | pg_read_all_settings
(1 row)

I have attached a rather small patch which makes it possible to
control this behavior:

You are now connected to database "rhaas" as user "rhaas".
rhaas=# grant pg_read_all_settings to seer with set false;
GRANT ROLE
rhaas=# \c - seer
You are now connected to database "rhaas" as user "seer".
rhaas=> set role pg_read_all_settings;
ERROR:  permission denied to set role "pg_read_all_settings"

I think that this behavior is generally useful, and not just for the
predefined roles that we ship as part of PostgreSQL. I don't think
it's too hard to imagine someone wanting to use some locally created
role as a container for privileges but not wanting the users who
possess this role to run around creating new objects owned by it. To
some extent that can be controlled by making sure the role in question
doesn't have any excess privileges, but that's not really sufficient:
all you need is one schema anywhere in the system that grants CREATE
to PUBLIC. You could avoid creating such a schema, which might be a
good idea for other reasons anyway, but it feels like approaching the
problem from the wrong end. What you really want is to allow the users
to inherit the privileges of the role but not use SET ROLE to become
that role, so that's what this patch lets you do.

There's one other kind of case in which this sort of thing might be
somewhat useful, although it's more dubious. Suppose you have an
oncall group where you regularly add and remove members according to
who is on call. Naturally, you have an on-call bot which performs this
task automatically. The on-call bot has the ability to manage
memberships in the oncall group, but should not have the ability to
access any of its privileges, either by inheritance or via SET ROLE.
This patch KIND OF lets you accomplish this:

rhaas=# create role oncall;
CREATE ROLE
rhaas=# create role oncallbot login;
CREATE ROLE
rhaas=# grant oncall to oncallbot with inherit false, set false, admin true;
GRANT ROLE
rhaas=# create role anna;
CREATE ROLE
rhaas=# create role eliza;
CREATE ROLE
rhaas=# \c - oncallbot
You are now connected to database "rhaas" as user "oncallbot".
rhaas=> grant oncall to anna;
GRANT ROLE
rhaas=> revoke oncall from anna;
REVOKE ROLE
rhaas=> grant oncall to eliza;
GRANT ROLE
rhaas=> set role oncall;
ERROR:  permission denied to set role "oncall"

The problem here is that if a nasty evil hacker takes over the
oncallbot role, nothing whatsoever prevents them from executing "grant
oncall to oncallbot with set true" after which they can then "SET ROLE
oncall" using the privileges they just granted themselves. And even if
under some theory we blocked that, they could still maliciously grant
the sought-after on-call privileges to some other role i.e. "grant
oncall to accomplice". It's fundamentally difficult to allow people to
administer a set of privileges without giving them the ability to
usurp those privileges, and I wouldn't like to pretend that this patch
is in any way sufficient to accomplish such a thing. Nevertheless, I
think there's some chance it might be useful to someone building such
a system, in combination with other safeguards. Or maybe not: this
isn't the main reason I'm interested in this, and it's just an added
benefit if it turns out that someone can do something like this with
it.

In order to apply this patch, we'd need to reach a conclusion about
the matters mentioned in
http://postgr.es/m/CA+TgmobhEYYnW9vrHvoLvD8ODsPBJuU9CbK6tms6Owd70hFMTw@mail.gmail.com
-- and thinking about this patch might shed some light on what we'd
want to do over there.

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

On Wed, Aug 31, 2022 at 08:56:31AM -0400, Robert Haas wrote:
> In some circumstances, it may be desirable to control this behavior.
> For example, if we GRANT pg_read_all_settings TO seer, we do want the
> seer to be able to read all the settings, else we would not have
> granted the role. But we might not want the seer to be able to do
> this:
> 
> You are now connected to database "rhaas" as user "seer".
> rhaas=> set role pg_read_all_settings;
> SET
> rhaas=> create table artifact (a int);
> CREATE TABLE
> rhaas=> \d
>                 List of relations
>  Schema |   Name   | Type  |        Owner
> --------+----------+-------+----------------------
>  public | artifact | table | pg_read_all_settings
> (1 row)

+1

> The problem here is that if a nasty evil hacker takes over the
> oncallbot role, nothing whatsoever prevents them from executing "grant
> oncall to oncallbot with set true" after which they can then "SET ROLE
> oncall" using the privileges they just granted themselves. And even if
> under some theory we blocked that, they could still maliciously grant
> the sought-after on-call privileges to some other role i.e. "grant
> oncall to accomplice". It's fundamentally difficult to allow people to
> administer a set of privileges without giving them the ability to
> usurp those privileges, and I wouldn't like to pretend that this patch
> is in any way sufficient to accomplish such a thing. Nevertheless, I
> think there's some chance it might be useful to someone building such
> a system, in combination with other safeguards. Or maybe not: this
> isn't the main reason I'm interested in this, and it's just an added
> benefit if it turns out that someone can do something like this with
> it.

Yeah, if you have ADMIN for a role, you would effectively have SET.  I'm
tempted to suggest that ADMIN roles should be restricted from granting SET
unless they have it themselves.  However, that seems like it'd create a
weird discrepancy.  If you have ADMIN but not INHERIT or SET, you'd still
be able to grant membership with or without INHERIT, but you wouldn't be
able to grant SET.  In the end, I guess I agree with you that it's
"fundamentally difficult to allow people to administer a set of privileges
without giving them the ability to usurp those privileges..."

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Robert Haas:
> Beginning in
> e3ce2de09d814f8770b2e3b3c152b7671bcdb83f, the inheritance behavior of
> role-grants can be overridden for individual grants, so that some
> grants are inherited and others are not.

That's a great thing to have!

> However, there is no similar
> facility for controlling whether a role can SET ROLE to some other
> role of which it is a member. At present, if role A is a member of
> role B, then A can SET ROLE B, and that's it.
> 
> In some circumstances, it may be desirable to control this behavior.

+1

> rhaas=# grant oncall to oncallbot with inherit false, set false, admin true;

Looking at the syntax here, I'm not sure whether adding more WITH 
options is the best way to do this. From a user perspective WITH SET 
TRUE looks more like a privilege granted on how to use this database 
object (role). Something like this would be more consistent with the 
other GRANT variants:

GRANT SET ON ROLE oncall TO oncallbot WITH GRANT OPTION;

This is obviously not exactly the same as the command above, because 
oncallbot would be able to use SET ROLE directly. But as discussed, this 
is more cosmetic anyway, because they could GRANT it to themselves.

The full syntax could look like this:

GRANT { INHERIT | SET | ALL [ PRIVILEGES ] }
   ON ROLE role_name [, ...]
   TO role_specification [, ...] WITH GRANT OPTION
   [ GRANTED BY role_specification ]

With this new syntax, the existing

GRANT role_name TO role_specification [WITH ADMIN OPTION];

would be the same as

GRANT ALL ON role_name TO role_specification [WITH GRANT OPTION];

This would slightly change the way INHERIT works: As a privilege, it 
would not override the member's role INHERIT attribute, but would 
control whether that attribute is applied. This means:

- INHERIT attribute + INHERIT granted -> inheritance (same)
- INHERIT attribute + INHERIT not granted -> no inheritance (different!)
- NOINHERIT attribute  + INHERIT not granted -> no inheritance (same)
- NOINHERIT attribute  + INHERIT granted -> no inheritance (different!)

This would allow us to do the following:

GRANT INHERIT ON ROLE pg_read_all_settings TO seer_bot WITH GRANT OPTION;

seer_bot would now be able to GRANT pg_read_all_settings to other users, 
too - but without the ability to use or grant SET ROLE to anyone. As 
long as seer_bot has the NOINHERIT attribute set, they wouldn't use that 
privilege, though - which might be desired for the bot.

Similary, it would be possible for the oncallbot in the example above to 
be able to grant SET ROLE only - and not INHERIT.

I realize that there has been a lot of discussion about roles and 
privileges in the past year. I have tried to follow those discussions, 
but it's likely that I missed some good arguments against my proposal above.

Best

Wolfgang

On Fri, Sep 2, 2022 at 3:20 AM Wolfgang Walther <walther@technowledgy.de> wrote:
> The full syntax could look like this:
>
> GRANT { INHERIT | SET | ALL [ PRIVILEGES ] }
>    ON ROLE role_name [, ...]
>    TO role_specification [, ...] WITH GRANT OPTION
>    [ GRANTED BY role_specification ]
>
> With this new syntax, the existing
>
> GRANT role_name TO role_specification [WITH ADMIN OPTION];
>
> would be the same as
>
> GRANT ALL ON role_name TO role_specification [WITH GRANT OPTION];

This would be a pretty significant rework. Right now, there's only one
ADMIN OPTION on a role, and you either have it or you don't. Changing
things around so that you can have each individual privilege with or
without grant option would be a fair amount of work. I don't think
it's completely crazy, but I'm not very sold on the idea, either,
because giving somebody *either* the ability to grant INHERIT option
*or* the ability to grant SET option is largely equivalent from a
security point of view. Either way, the grantees will be able to
access the privileges of the role in some fashion. This is different
from table privileges, where SELECT and INSERT are clearly distinct
rights that do not overlap, and thus separating the ability to
administer one of those things from the ability to administer the
other one has more utility.

The situation might look different in the future if we added more role
options and if each of those were clearly severable rights. For
instance, if we had a DROP option on a role grant that conferred the
right to drop the role, that would be distinct from SET and INHERIT
and it might make sense to allow someone to administer SET and/or
INHERIT but not DROP. However, I don't have any current plans to add
such an option, and TBH I find it a little hard to come up with a
compelling list of things that would be worth adding as separate
permissions here. There are a bunch of things that one role can do to
another using ALTER ROLE, and right now you have to be SUPERUSER or
have CREATEROLE to do that stuff. In
theory, you could turn that into a big list of individual rights so
that you can e.g. GRANT CHANGE PASSWORD ON role1 TO role2 WITH GRANT
OPTION.

However, I really don't see a lot of utility in slicing things up at
that level of granularity. There isn't in my view a lot of use case
for giving a user the right to change some other user's password but
not giving them the right to set the connection limit for that same
other user -- and there's even less use case for giving some user the
ability to grant one of those rights but not the other.

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

On Wed, 2022-08-31 at 08:56 -0400, Robert Haas wrote:
> In some circumstances, it may be desirable to control this behavior.
> For example, if we GRANT pg_read_all_settings TO seer, we do want the
> seer to be able to read all the settings, else we would not have
> granted the role. But we might not want the seer to be able to do
> this:
>
> You are now connected to database "rhaas" as user "seer".
> rhaas=> set role pg_read_all_settings;
> SET
> rhaas=> create table artifact (a int);
> CREATE TABLE
> rhaas=> \d
>                 List of relations
>  Schema |   Name   | Type  |        Owner
> --------+----------+-------+----------------------
>  public | artifact | table | pg_read_all_settings
> (1 row)

Interesting case.

> I have attached a rather small patch which makes it possible to
> control this behavior:
>
> You are now connected to database "rhaas" as user "rhaas".
> rhaas=# grant pg_read_all_settings to seer with set false;
> GRANT ROLE

You've defined this in terms of the mechanics -- allow SET ROLE or not
-- but I assume you intend it as a security feature to allow/forbid
some capabilities.

Is this only about the capability to create objects owned by a role
you're a member of? Or are there other implications?

Regards,
    Jeff Davis

On Sat, Sep 3, 2022 at 3:46 PM Jeff Davis <pgsql@j-davis.com> wrote:
> > You are now connected to database "rhaas" as user "rhaas".
> > rhaas=# grant pg_read_all_settings to seer with set false;
> > GRANT ROLE
>
> You've defined this in terms of the mechanics -- allow SET ROLE or not
> -- but I assume you intend it as a security feature to allow/forbid
> some capabilities.
>
> Is this only about the capability to create objects owned by a role
> you're a member of? Or are there other implications?

I think there are some other implications, but I don't think they're
anything super-dramatic. For example, you could create a group that's
just for purposes of pg_hba.conf matching and make the grants both SET
FALSE and INHERIT FALSE, with the idea that the members shouldn't have
any access to the role; it's just there for grouping purposes. I
mentioned one other possible scenario, with oncallbot, in the original
post.

I'm not sure whether thinking about this in terms of security
capabilities is the most helpful way to view it. My view was, as you
say, more mechanical. I think sometimes you grant somebody a role and
you want them to be able to use SET ROLE to assume the privileges of
the target role, and sometimes you don't. I think that primarily
depends on the reason why you made the grant. In the case of a
predefined role, you're almost certainly granting membership so that
the privileges of the predefined role can be inherited. In other
cases, you may be doing it so that the member can SET ROLE to the
target role, or you may be doing it so that the member can administer
the role (because you give them ADMIN OPTION), or you may even be
doing it for pg_hba.conf matching.

And because of this, I think it follows that there may be some
capabilities conferred by role membership that you don't really want
to convey in particular cases, so I think it makes sense to have a way
to avoid conveying the ones that aren't necessary for the grant to
fulfill its purpose. I'm not exactly sure how far that gets you in
terms of building a system that is more secure than what you could
build otherwise, but it feels like a useful capability regardless.

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

On Tue, 2022-09-06 at 10:42 -0400, Robert Haas wrote:
> I think there are some other implications, but I don't think they're
> anything super-dramatic. For example, you could create a group that's
> just for purposes of pg_hba.conf matching and make the grants both
> SET
> FALSE and INHERIT FALSE, with the idea that the members shouldn't
> have
> any access to the role; it's just there for grouping purposes. I
> mentioned one other possible scenario, with oncallbot, in the
> original
> post.

Interesting. All of those seem like worthwhile use cases to me.

> I'm not sure whether thinking about this in terms of security
> capabilities is the most helpful way to view it. My view was, as you
> say, more mechanical. I think sometimes you grant somebody a role and
> you want them to be able to use SET ROLE to assume the privileges of
> the target role, and sometimes you don't.

By denying the ability to "SET ROLE pg_read_all_settings", I assumed
that we'd deny the ability to create objects owned by that
pg_read_all_settings. But on closer inspection:

  grant all privileges on schema public to public;
  create user u1;
  grant pg_read_all_settings to u1 with set false;
  \c - u1
  create table foo(i int);
  set role pg_read_all_settings;
  ERROR:  permission denied to set role "pg_read_all_settings"
  alter table foo owner to pg_read_all_settings;
  \d
                List of relations
   Schema | Name | Type  |        Owner
  --------+------+-------+----------------------
   public | foo  | table | pg_read_all_settings
  (1 row)


Users will reasonably interpret any feature of GRANT to be a security
feature that allows or prevents certain users from causing certain
outcomes. But here, I was initially fooled, and the outcome is still
possible.

So I believe we do need to think in terms of what capabilities we are
really restricting with this feature rather than solely the mechanics.

Regards,
    Jeff Davis

On Tue, Sep 6, 2022 at 2:45 PM Jeff Davis <pgsql@j-davis.com> wrote:
> > I'm not sure whether thinking about this in terms of security
> > capabilities is the most helpful way to view it. My view was, as you
> > say, more mechanical. I think sometimes you grant somebody a role and
> > you want them to be able to use SET ROLE to assume the privileges of
> > the target role, and sometimes you don't.
>
> By denying the ability to "SET ROLE pg_read_all_settings", I assumed
> that we'd deny the ability to create objects owned by that
> pg_read_all_settings. But on closer inspection:
>
>   grant all privileges on schema public to public;
>   create user u1;
>   grant pg_read_all_settings to u1 with set false;
>   \c - u1
>   create table foo(i int);
>   set role pg_read_all_settings;
>   ERROR:  permission denied to set role "pg_read_all_settings"
>   alter table foo owner to pg_read_all_settings;
>   \d
>                 List of relations
>    Schema | Name | Type  |        Owner
>   --------+------+-------+----------------------
>    public | foo  | table | pg_read_all_settings
>   (1 row)

Yeah. Please note this paragraph in my original post:

"In order to apply this patch, we'd need to reach a conclusion about
the matters mentioned in
http://postgr.es/m/CA+TgmobhEYYnW9vrHvoLvD8ODsPBJuU9CbK6tms6Owd70hFMTw@mail.gmail.com
-- and thinking about this patch might shed some light on what we'd
want to do over there."

I hadn't quite gotten around to updating that thread based on posting
this, but this scenario was indeed on my mind.

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

On 31.08.22 14:56, Robert Haas wrote:
> In some circumstances, it may be desirable to control this behavior.
> For example, if we GRANT pg_read_all_settings TO seer, we do want the
> seer to be able to read all the settings, else we would not have
> granted the role. But we might not want the seer to be able to do
> this:
> 
> You are now connected to database "rhaas" as user "seer".
> rhaas=> set role pg_read_all_settings;
> SET
> rhaas=> create table artifact (a int);
> CREATE TABLE
> rhaas=> \d
>                  List of relations
>   Schema |   Name   | Type  |        Owner
> --------+----------+-------+----------------------
>   public | artifact | table | pg_read_all_settings
> (1 row)

I think this is because we have (erroneously) make SET ROLE to be the 
same as SET SESSION AUTHORIZATION.  If those two were separate (i.e., 
there is a current user and a separate current role, as in the SQL 
standard), then this would be more straightforward.

I don't know if it's possible to untangle that at this point.

On Mon, Sep 12, 2022 at 11:41 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> I think this is because we have (erroneously) make SET ROLE to be the
> same as SET SESSION AUTHORIZATION.  If those two were separate (i.e.,
> there is a current user and a separate current role, as in the SQL
> standard), then this would be more straightforward.
>
> I don't know if it's possible to untangle that at this point.

I think that it already works as you describe:

rhaas=# create role foo;
CREATE ROLE
rhaas=# create role bar;
CREATE ROLE
rhaas=# grant bar to foo;
GRANT ROLE
rhaas=# set session authorization foo;
SET
rhaas=> set role bar;
SET
rhaas=> select current_user;
 current_user
--------------
 bar
(1 row)

rhaas=> select session_user;
 session_user
--------------
 foo
(1 row)

There may well be problems here, but this example shows that the
current_user and session_user concepts are different in PostgreSQL.
It's also true that the privileges required to execute the commands
are different: SET SESSION AUTHORIZATION requires that the session
user is a superuser, and SET ROLE requires that the identity
established via SET SESSION AUTHORIZATION has the target role granted
to it.

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

On Wed, Aug 31, 2022 at 8:56 AM Robert Haas <robertmhaas@gmail.com> wrote:
> In order to apply this patch, we'd need to reach a conclusion about
> the matters mentioned in
> http://postgr.es/m/CA+TgmobhEYYnW9vrHvoLvD8ODsPBJuU9CbK6tms6Owd70hFMTw@mail.gmail.com
> -- and thinking about this patch might shed some light on what we'd
> want to do over there.

That thread has not reached an entirely satisfying conclusion.
However, the behavior that was deemed outright buggy over there has
been fixed. The remaining question is what to do about commands that
allow you to give objects to other users (like ALTER <whatever> ..
OWNER TO) or commands that allow you to create objects owned by other
users (like CREATE DATABASE ... OWNER). I have, in this version,
adopted the proposal by Wolfgang Walther on the other thread to make
this controlled by the new SET option. This essentially takes the view
that the ability to create objects owned by another user is not
precisely a privilege, and is thus not inherited just because the
INHERIT option is set on the GRANT, but it is something you can do if
you could SET ROLE to that role, so we make it dependent on the SET
option. This logic is certainly debatable, but it does have the
practical advantage of making INHERIT TRUE, SET FALSE a useful
combination of settings for predefined roles. It's also 100%
backward-compatible, whereas if we made the behavior dependent on the
INHERIT option, users could potentially notice behavior changes after
upgrading to v16.

So I do like this behavior ... but it's definitely arguable whether
it's the best thing. At any rate, here's an updated patch that
implements it, and to which I've also added a test case.

Review appreciated.

Thanks,

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

On Fri, Sep 30, 2022 at 04:34:32PM -0400, Robert Haas wrote:
> That thread has not reached an entirely satisfying conclusion.
> However, the behavior that was deemed outright buggy over there has
> been fixed. The remaining question is what to do about commands that
> allow you to give objects to other users (like ALTER <whatever> ..
> OWNER TO) or commands that allow you to create objects owned by other
> users (like CREATE DATABASE ... OWNER). I have, in this version,
> adopted the proposal by Wolfgang Walther on the other thread to make
> this controlled by the new SET option. This essentially takes the view
> that the ability to create objects owned by another user is not
> precisely a privilege, and is thus not inherited just because the
> INHERIT option is set on the GRANT, but it is something you can do if
> you could SET ROLE to that role, so we make it dependent on the SET
> option. This logic is certainly debatable, but it does have the
> practical advantage of making INHERIT TRUE, SET FALSE a useful
> combination of settings for predefined roles. It's also 100%
> backward-compatible, whereas if we made the behavior dependent on the
> INHERIT option, users could potentially notice behavior changes after
> upgrading to v16.

I'm not sure about tying the ownership stuff to this new SET privilege.
While you noted some practical advantages, I'd expect users to find it kind
of surprising.  Also, for predefined roles, I think you need to be careful
about distributing ADMIN, as anyone with ADMIN on a predefined role can
just GRANT SET to work around the restrictions.  I don't have a better
idea, though, so perhaps neither of these things is a deal-breaker.  I was
tempted to suggest using ADMIN instead of SET for the ownership stuff, but
that wouldn't be backward-compatible, and you'd still be able to work
around it to some extent with SET (e.g., SET ROLE followed by CREATE
DATABASE).

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Greetings,

* Nathan Bossart (nathandbossart@gmail.com) wrote:
> On Fri, Sep 30, 2022 at 04:34:32PM -0400, Robert Haas wrote:
> > That thread has not reached an entirely satisfying conclusion.
> > However, the behavior that was deemed outright buggy over there has
> > been fixed. The remaining question is what to do about commands that
> > allow you to give objects to other users (like ALTER <whatever> ..
> > OWNER TO) or commands that allow you to create objects owned by other
> > users (like CREATE DATABASE ... OWNER). I have, in this version,
> > adopted the proposal by Wolfgang Walther on the other thread to make
> > this controlled by the new SET option. This essentially takes the view
> > that the ability to create objects owned by another user is not
> > precisely a privilege, and is thus not inherited just because the
> > INHERIT option is set on the GRANT, but it is something you can do if
> > you could SET ROLE to that role, so we make it dependent on the SET
> > option. This logic is certainly debatable, but it does have the
> > practical advantage of making INHERIT TRUE, SET FALSE a useful
> > combination of settings for predefined roles. It's also 100%
> > backward-compatible, whereas if we made the behavior dependent on the
> > INHERIT option, users could potentially notice behavior changes after
> > upgrading to v16.
>
> I'm not sure about tying the ownership stuff to this new SET privilege.
> While you noted some practical advantages, I'd expect users to find it kind
> of surprising.  Also, for predefined roles, I think you need to be careful
> about distributing ADMIN, as anyone with ADMIN on a predefined role can
> just GRANT SET to work around the restrictions.  I don't have a better
> idea, though, so perhaps neither of these things is a deal-breaker.  I was
> tempted to suggest using ADMIN instead of SET for the ownership stuff, but
> that wouldn't be backward-compatible, and you'd still be able to work
> around it to some extent with SET (e.g., SET ROLE followed by CREATE
> DATABASE).

As we work through splitting up the privileges and managing them in a
more fine-grained way, it seems clear that we'll need to have a similar
split for ADMIN rights on roles- that is, we'll need to be able to
say "role X is allowed to GRANT INHERIT for role Y to other roles, but
not SET".

I'm still half-tempted to say that predefined roles should just be dealt
with as a special case.. but if we split ADMIN in the manner as
described above then maybe we could get away with not having to, but it
would depend a great deal of people actually reading the documentation
and I'm concerned that's a bit too much to ask in this case.

That is- the first person who is likely to GRANT out ADMIN rights in a
predefined role is going to be a superuser.  To avoid breaking backwards
compatibility, GRANT'ing of ADMIN needs to GRANT all the partial-ADMIN
rights that exist, or at least exist today, which includes both SET and
INHERIT.  Unless we put some kind of special case for predefined roles
where we throw an error or at least a warning when a superuser
(presumably) inadvertantly does a simple GRANT ADMIN for $predefined
role, we're going to end up in the situation where folks can SET ROLE to
a predefined role and do things that they really shouldn't be allowed
to.

We could, of course, very clearly document that the way to GRANT ADMIN
rights for a predefined role is to always make sure to *only* GRANT
ADMIN/INHERIT, but again I worry that it simply wouldn't be followed in
many cases.  Perhaps we could arrange for the bootstrap superuser to
only be GRANT'd ADMIN/INHERIT for predefined roles and then not have an
explicit cut-out for superuser doing a GRANT on predefined roles or
perhaps having such be protected under allow_system_table_mods under the
general consideration that modifying of predefined roles isn't something
that folks should be doing post-initdb.

Just a few thoughts on this, not sure any of these ideas are great but
perhaps this helps move us forward.

Thanks,

Stephen

On Wed, Oct 12, 2022 at 4:59 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I'm not sure about tying the ownership stuff to this new SET privilege.
> While you noted some practical advantages, I'd expect users to find it kind
> of surprising.  Also, for predefined roles, I think you need to be careful
> about distributing ADMIN, as anyone with ADMIN on a predefined role can
> just GRANT SET to work around the restrictions.  I don't have a better
> idea, though, so perhaps neither of these things is a deal-breaker.

Right, I think if you give ADMIN away to someone, that's it: they can
grant that role to whoever they want in whatever mode they want,
including themselves. That seems more or less intentional to me,
though. Giving someone ADMIN OPTION on a role is basically making them
an administrator of that role, and then it is not surprising that they
can access its privileges.

I agree with your other caveat about it being potentially surprising,
but I think it's not worse than a lot of other somewhat surprising
things that we handle by documenting them. And I don't have a better
idea either.

> I was
> tempted to suggest using ADMIN instead of SET for the ownership stuff, but
> that wouldn't be backward-compatible, and you'd still be able to work
> around it to some extent with SET (e.g., SET ROLE followed by CREATE
> DATABASE).

I think that would be way worse. Giving ADMIN OPTION on a role is like
making someone the owner of the object, whereas giving someone INHERIT
or SET on a role is just a privilege to use the object.

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

On Sun, Oct 16, 2022 at 12:34 PM Stephen Frost <sfrost@snowman.net> wrote:
> As we work through splitting up the privileges and managing them in a
> more fine-grained way, it seems clear that we'll need to have a similar
> split for ADMIN rights on roles- that is, we'll need to be able to
> say "role X is allowed to GRANT INHERIT for role Y to other roles, but
> not SET".

I don't think this is clear at all, actually.  I see very little
advantage in splitting up ADMIN OPTION this way. I did think about
this, because it would be more consistent with what we do for table
privileges, but INHERIT and SET overlap enough from a permissions
point of view that there doesn't seem to be a lot of value in it. Now,
if we invent a bunch more per-grant options, things might look
different, but in my opinion that has dubious value. Right now, all
role privileges other than the ones that are controlled by ADMIN
OPTION, INHERIT, and what I'm proposing to make controlled by SET, are
gated by CREATEROLE or by SUPERUSER. The list looks something like
this: change the INHERIT flag on a role, change the CREATEROLE flag on
a role, change the CREATEDB flag on a role, change the connection
limit for a role, change the VALID UNTIL time for a role, change the
password for a role other than your own, drop the role.

And that's a pretty obscure list of things. I do think we need better
ways to control who can do those things, but I don't think making them
all role privileges and then on top of that giving them all separate
admin options is the right way to go. It's slicing things incredibly
finely to give alice the right to grant to some other user the right
to set only the VALID UNTIL time on role bob, but not the right to
modify role bob in any other way or the right to confer the ability to
set VALID UNTIL for any other user. I can't believe we want to go
there. It's not worth the permissions bits, and even if we had
infinite privilege bits available, it's not worth the complexity from
a user perspective. Maybe you have some less-obscure list of things
that you think should be grantable privileges on roles?

Another thing to consider is that, since ADMIN OPTION is, as I
understand it, part of the SQL specification, I think it would move us
further from the SQL specification. I think we will be better off
thinking of ADMIN OPTION on a role as roughly equivalent to being the
owner of that role, which is an indivisible privilege, rather than
thinking of it as equivalent to GRANT OPTION on each of N rights,
which could then be subdivided.

> I'm still half-tempted to say that predefined roles should just be dealt
> with as a special case.. but if we split ADMIN in the manner as
> described above then maybe we could get away with not having to, but it
> would depend a great deal of people actually reading the documentation
> and I'm concerned that's a bit too much to ask in this case.

I don't think any splitting of ADMIN would be required to solve the
predefined roles problem. Doesn't the patch I proposed do that?

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

Bump.

Discussion has trailed off here, but I still don't see that we have a
better way forward here than what I proposed on September 30th. Two
people have commented. Nathan said that he wasn't sure this was best
(neither am I) but that he didn't have a better idea either (neither
do I). Stephen proposed decomposing ADMIN OPTION, which is not my
preference, but even if it turns out that we want to pursue that
approach, I do not think it would make sense to bundle that into this
patch, because there isn't enough overlap between that change and this
change to justify that treatment.

If anyone else wants to comment, or if either of those people want to
comment further, please speak up soon. Otherwise, I am going to press
forward with committing this. If we do not, we will continue to have
no way of restricting of SET ROLE, and we will continue to have no way
of preventing the creation of objects owned by predefined roles by
users who have been granted those roles. As far as I am aware, no one
is opposed to those goals, and in fact I think everyone who has
commented thinks that it would be good to do something. If a better
idea than what I've implemented comes along, I'm happy to defer to it,
but I think this is one of those cases in which there probably isn't
any totally satisfying solution, and yet doing nothing is not a
superior alternative.

Thanks,

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

On Tue, Nov 15, 2022 at 12:07:06PM -0500, Robert Haas wrote:
> If anyone else wants to comment, or if either of those people want to
> comment further, please speak up soon. Otherwise, I am going to press
> forward with committing this.

I don't think I have any further thoughts about the approach, so I won't
balk if you proceed with this change.  It might be worth starting a new
thread to discuss whether to treat predefined roles as a special case, but
IMO that needn't hold up this patch.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, 2022-11-15 at 12:07 -0500, Robert Haas wrote:
> If anyone else wants to comment, or if either of those people want to
> comment further, please speak up soon.

Did you have some thoughts on:

https://postgr.es/m/a41d606daaaa03b629c2ef0ed274ae3b04a2c266.camel@j-davis.com

Regards,
    Jeff Davis

On Tue, Nov 15, 2022 at 7:23 PM Jeff Davis <pgsql@j-davis.com> wrote:
> On Tue, 2022-11-15 at 12:07 -0500, Robert Haas wrote:
> > If anyone else wants to comment, or if either of those people want to
> > comment further, please speak up soon.
>
> Did you have some thoughts on:
>
> https://postgr.es/m/a41d606daaaa03b629c2ef0ed274ae3b04a2c266.camel@j-davis.com

I mean, I think what we were discussing there could be done, but it's
not the approach I like best. That's partly because that was just a
back-of-the-envelope sketch of an idea, not a real proposal for
something with a clear implementation path. But I think the bigger
reason is that, in my opinion, this proposal is more generally useful,
because it takes no position on why you wish to disallow SET ROLE. You
can just disallow it in some cases and allow it in others, and that's
fine. That proposal targets a specific use case, which may make it a
better solution to that particular problem, but it makes it unworkable
as a solution to any other problem, I believe.

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

On Thu, 2022-11-17 at 16:52 -0500, Robert Haas wrote:

> But I think the bigger
> reason is that, in my opinion, this proposal is more generally
> useful,
> because it takes no position on why you wish to disallow SET ROLE.
> You
> can just disallow it in some cases and allow it in others, and that's
> fine.

I agree that the it's more flexible in the sense that it does what it
does, and administrators can use it if it's useful for them. That means
we don't need to understand the actual goals as well; but it also means
that it's harder to determine the consequences if we tweak the behavior
(or any related behavior) later.

I'll admit that I don't have an example of a likely problem here,
though.

> That proposal targets a specific use case, which may make it a
> better solution to that particular problem, but it makes it
> unworkable
> as a solution to any other problem, I believe.

Yeah, that's the flip side: "virtual" roles (for lack of a better name)
are a more narrow fix for the problem as I understand it; but it might
leave related problems unfixed. You and Stephen[2] both seemed to
consider this approach, and I happened to like it, so I wanted to make
sure that it wasn't dismissed too quickly.

But I'm fine if you'd like to move on with the SET ROLE privilege
instead, as long as we believe it grants a stable set of capabilities
(and conversely, that if the SET ROLE privilege is revoked, that it
revokes a stable set of capabilities).

[2]
https://www.postgresql.org/message-id/YzIAGCrxoXibAKOD%40tamriel.snowman.net


--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Thu, Nov 17, 2022 at 7:24 PM Jeff Davis <pgsql@j-davis.com> wrote:
> But I'm fine if you'd like to move on with the SET ROLE privilege
> instead, as long as we believe it grants a stable set of capabilities
> (and conversely, that if the SET ROLE privilege is revoked, that it
> revokes a stable set of capabilities).

OK.

Here's a rebased v3 to see what cfbot thinks.

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

On 2022-Nov-18, Robert Haas wrote:

> On Thu, Nov 17, 2022 at 7:24 PM Jeff Davis <pgsql@j-davis.com> wrote:
> > But I'm fine if you'd like to move on with the SET ROLE privilege
> > instead, as long as we believe it grants a stable set of capabilities
> > (and conversely, that if the SET ROLE privilege is revoked, that it
> > revokes a stable set of capabilities).
> 
> OK.
> 
> Here's a rebased v3 to see what cfbot thinks.

I think this hunk in dumpRoleMembership() leaves an unwanted line
behind.

    /*
-    * Previous versions of PostgreSQL also did not have a grant-level
+    * Previous versions of PostgreSQL also did not have grant-level options.
     * INHERIT option.
     */

(I was just looking at the doc part of this patch.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)

On Fri, Nov 18, 2022 at 12:50 PM Robert Haas <robertmhaas@gmail.com> wrote:
> Here's a rebased v3 to see what cfbot thinks.

cfbot is happy, so committed.

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

Op 18-11-2022 om 19:43 schreef Robert Haas:
> On Fri, Nov 18, 2022 at 12:50 PM Robert Haas <robertmhaas@gmail.com> wrote:
>> Here's a rebased v3 to see what cfbot thinks.
> 
> cfbot is happy, so committed.

In grant.sgml,

   'actualy permisions'

looks a bit unorthodox.

On Fri, Nov 18, 2022 at 1:50 PM Erik Rijkers <er@xs4all.nl> wrote:
> In grant.sgml,
>
>    'actualy permisions'
>
> looks a bit unorthodox.

Fixed that, and the other mistake Álvaro spotted, and also bumped
catversion because I forgot that earlier.

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

Op 18-11-2022 om 22:19 schreef Robert Haas:
> On Fri, Nov 18, 2022 at 1:50 PM Erik Rijkers <er@xs4all.nl> wrote:
>> In grant.sgml,
>>
>>     'actualy permisions'
>>
>> looks a bit unorthodox.
> 
> Fixed that, and the other mistake Álvaro spotted, and also bumped
> catversion because I forgot that earlier.

Sorry to be nagging but

   'permisions'  should be
   'permissions'

as well.


And as I'm nagging anyway: I also wondered whether the word order could 
improve:

- Word order as it stands:
However, the actual permissions conferred depend on the options 
associated with the grant.

-- maybe better:
However, the permissions actually conferred depend on the options 
associated with the grant.

But I'm not sure.


Thanks,

Erik

Thanks,

Erik

On Fri, Nov 18, 2022 at 04:19:15PM -0500, Robert Haas wrote:
> Fixed that, and the other mistake Álvaro spotted, and also bumped
> catversion because I forgot that earlier.

I was looking at this code yesterday, to see today that psql's
completion should be completed with this new clause, similary to ADMIN
and INHERIT.
--
Michael

On Fri, Nov 18, 2022 at 04:19:15PM -0500, Robert Haas wrote:
> On Fri, Nov 18, 2022 at 1:50 PM Erik Rijkers <er@xs4all.nl> wrote:
> > In grant.sgml,
> >
> >    'actualy permisions'
> >
> > looks a bit unorthodox.
> 
> Fixed that, and the other mistake Álvaro spotted, and also bumped
> catversion because I forgot that earlier.

I think Erik was trying to report that both words were misspelled.  I
added to my typos to be fixed in batch if you want to wait.

On Sat, Nov 19, 2022 at 1:00 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Nov 18, 2022 at 04:19:15PM -0500, Robert Haas wrote:
> > Fixed that, and the other mistake Álvaro spotted, and also bumped
> > catversion because I forgot that earlier.
>
> I was looking at this code yesterday, to see today that psql's
> completion should be completed with this new clause, similary to ADMIN
> and INHERIT.

Seems like a good idea but I'm not sure about this hunk:

  TailMatches("GRANT|REVOKE", "ALTER", "SYSTEM") ||
- TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "ALTER", "SYSTEM"))
+ TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "ALTER", "SYSTEM") ||
+ TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "SET"))

That might be a correct change for other reasons, but it doesn't seem
related to this patch. The rest looks good.

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

идея и её реализация насчёт Параллельного чтения - как вам? Мне 
показалось, интересно и полезно. Но это, думаю, одноразовая акция. 
Времени и сил на это довольно много ухлопал, хотя вроде дело нехитрое 
:)  Стоило?

15.11.2022 20:07, Robert Haas пишет:
> Bump.
>
> Discussion has trailed off here, but I still don't see that we have a
> better way forward here than what I proposed on September 30th. Two
> people have commented. Nathan said that he wasn't sure this was best
> (neither am I) but that he didn't have a better idea either (neither
> do I). Stephen proposed decomposing ADMIN OPTION, which is not my
> preference, but even if it turns out that we want to pursue that
> approach, I do not think it would make sense to bundle that into this
> patch, because there isn't enough overlap between that change and this
> change to justify that treatment.
>
> If anyone else wants to comment, or if either of those people want to
> comment further, please speak up soon. Otherwise, I am going to press
> forward with committing this. If we do not, we will continue to have
> no way of restricting of SET ROLE, and we will continue to have no way
> of preventing the creation of objects owned by predefined roles by
> users who have been granted those roles. As far as I am aware, no one
> is opposed to those goals, and in fact I think everyone who has
> commented thinks that it would be good to do something. If a better
> idea than what I've implemented comes along, I'm happy to defer to it,
> but I think this is one of those cases in which there probably isn't
> any totally satisfying solution, and yet doing nothing is not a
> superior alternative.
>
> Thanks,
>

On Mon, Nov 21, 2022 at 10:45:53AM -0500, Robert Haas wrote:
> Seems like a good idea but I'm not sure about this hunk:
>
>   TailMatches("GRANT|REVOKE", "ALTER", "SYSTEM") ||
> - TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "ALTER", "SYSTEM"))
> + TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "ALTER", "SYSTEM") ||
> + TailMatches("REVOKE", "GRANT", "OPTION", "FOR", "SET"))
>
> That might be a correct change for other reasons, but it doesn't seem
> related to this patch. The rest looks good.

(Forgot to press "Send" a few days ago..)

Hmm, right, I see your point.  I have just moved that to reorder the
terms alphabetically, but moving the check on REVOKE GRANT OPTION FOR
SET is not mandatory.  I have moved it back in its previous
position, leading to less noise in the diffs, and applied the rest as
of 9d0cf57.
Thanks!
--
Michael

On Thu, Nov 17, 2022 at 04:24:24PM -0800, Jeff Davis wrote:
> On Thu, 2022-11-17 at 16:52 -0500, Robert Haas wrote:
> > But I think the bigger reason is that, in my opinion, this proposal is
> > more generally useful, because it takes no position on why you wish to
> > disallow SET ROLE.  You can just disallow it in some cases and allow it in
> > others, and that's fine.

In this commit 3d14e17, the documentation takes the above "no position".  The
implementation does not, in that WITH SET FALSE has undocumented ability to
block ALTER ... OWNER TO, not just SET ROLE.  Leaving that undocumented feels
weird to me, but documenting it would take the position that WITH SET FALSE is
relevant to the security objective of preventing object creation like the
example in the original post of this thread.  How do you weigh those
documentation trade-offs?

> I agree that the it's more flexible in the sense that it does what it
> does, and administrators can use it if it's useful for them. That means
> we don't need to understand the actual goals as well; but it also means
> that it's harder to determine the consequences if we tweak the behavior
> (or any related behavior) later.

I have similar concerns.  For the original post's security objective, the role
must also own no objects of certain types.  Otherwise, WITH SET FALSE members
can use operations like CREATE OR REPLACE FUNCTION or CREATE INDEX to escalate
to full role privileges:

create user unpriv;
grant pg_maintain to unpriv with set false;
create schema maint authorization pg_maintain
  create table t (c int);
create or replace function maint.f() returns int language sql as 'select 1';
alter function maint.f() owner to pg_maintain;
set session authorization unpriv;
create or replace function maint.f() returns int language sql security definer as 'select 1';
create index on maint.t(c);

On Sat, Dec 31, 2022 at 1:16 AM Noah Misch <noah@leadboat.com> wrote:
> On Thu, Nov 17, 2022 at 04:24:24PM -0800, Jeff Davis wrote:
> > On Thu, 2022-11-17 at 16:52 -0500, Robert Haas wrote:
> > > But I think the bigger reason is that, in my opinion, this proposal is
> > > more generally useful, because it takes no position on why you wish to
> > > disallow SET ROLE.  You can just disallow it in some cases and allow it in
> > > others, and that's fine.
>
> In this commit 3d14e17, the documentation takes the above "no position".  The
> implementation does not, in that WITH SET FALSE has undocumented ability to
> block ALTER ... OWNER TO, not just SET ROLE.  Leaving that undocumented feels
> weird to me, but documenting it would take the position that WITH SET FALSE is
> relevant to the security objective of preventing object creation like the
> example in the original post of this thread.  How do you weigh those
> documentation trade-offs?

In general, I favor trying to make the documentation clearer and more
complete. Intentionally leaving things undocumented doesn't seem like
the right course of action to me. That said, the pre-existing
documentation in this area is so incomplete that it's sometimes hard
to figure out where to add new information - and it made no mention of
the privileges required for ALTER .. OWNER TO. I didn't immediately
know where to add that, so did nothing. Maybe I should have tried
harder, though.

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

On Tue, Jan 03, 2023 at 02:43:10PM -0500, Robert Haas wrote:
> On Sat, Dec 31, 2022 at 1:16 AM Noah Misch <noah@leadboat.com> wrote:
> > On Thu, Nov 17, 2022 at 04:24:24PM -0800, Jeff Davis wrote:
> > > On Thu, 2022-11-17 at 16:52 -0500, Robert Haas wrote:
> > > > But I think the bigger reason is that, in my opinion, this proposal is
> > > > more generally useful, because it takes no position on why you wish to
> > > > disallow SET ROLE.  You can just disallow it in some cases and allow it in
> > > > others, and that's fine.
> >
> > In this commit 3d14e17, the documentation takes the above "no position".  The
> > implementation does not, in that WITH SET FALSE has undocumented ability to
> > block ALTER ... OWNER TO, not just SET ROLE.  Leaving that undocumented feels
> > weird to me, but documenting it would take the position that WITH SET FALSE is
> > relevant to the security objective of preventing object creation like the
> > example in the original post of this thread.  How do you weigh those
> > documentation trade-offs?
> 
> In general, I favor trying to make the documentation clearer and more
> complete. Intentionally leaving things undocumented doesn't seem like
> the right course of action to me.

For what it's worth, I like to leave many things undocumented, but not this.

> That said, the pre-existing
> documentation in this area is so incomplete that it's sometimes hard
> to figure out where to add new information - and it made no mention of
> the privileges required for ALTER .. OWNER TO. I didn't immediately
> know where to add that, so did nothing.

I'd start with locations where the patch already added documentation.  In the
absence of documentation otherwise, a reasonable person could think WITH SET
controls just SET ROLE.  The documentation of WITH SET is a good place to list
what else you opted for it to control.  If the documentation can explain the
set of principles that would be used to decide whether WITH SET should govern
another thing in the future, that would provide extra value.

On Tue, Jan 3, 2023 at 5:03 PM Noah Misch <noah@leadboat.com> wrote:
> I'd start with locations where the patch already added documentation.  In the
> absence of documentation otherwise, a reasonable person could think WITH SET
> controls just SET ROLE.  The documentation of WITH SET is a good place to list
> what else you opted for it to control.  If the documentation can explain the
> set of principles that would be used to decide whether WITH SET should govern
> another thing in the future, that would provide extra value.

From the point of view of the code, we currently have four different
functions that make inquiries about role membership:
has_privs_of_role, is_member_of_role, is_member_of_role_nosuper, and
member_can_set_role.

I spent a while looking at how has_privs_of_role() is used. Basically,
there are three main patterns. First, in some places, you must have
the privileges of a certain role (typically, either a predefined role
or the role that owns some object) or the operation will fail with an
error indicating that you don't have sufficient permissions. Second,
there are places where having the privileges of a certain role exempts
you from some other permissions check; if you have neither, you'll get
an error. An example is that having the permissions of
pg_read_all_data substitutes for a select privilege. And third, there
are cases where you definitely won't get an error, but the behavior
will vary depending on whether you have the privileges of some role.
For instance, you can see more data in pg_stat_replication,
pg_stat_wal_receiver, and other stats views if you have
pg_read_all_stats. The GUC values reported in EXPLAIN output will
exclude superuser-only values unless you have pg_read_all_settings. It
looks like some maintenance commands like CLUSTER and VACUUM
completely skip over, or just warn about, cases where permission is
lacking. And weirdest of all, having the privileges of a role means
that the RLS policies applied to that role also apply to you. That's
odd because it makes permissions not strictly additive.

member_can_set_role() controls (a) whether you can SET ROLE to some
other role, (b) whether you can alter the owner of an existing object
to that role, and (c) whether you can create an object owned by some
other user in cases where the CREATE command has an option for that,
like CREATE DATABASE ... OWNER.

is_member_of_role_nosuper() is used to prevent creation of role
membership loops, and for pg_hba.conf matching.

The only remaining call to is_member_of_role() is in
pg_role_aclcheck(), which just supports the SQL-callable
pg_has_role(). has_privs_of_role() and member_can_set_role() are used
here, too.

How much of this should we document, do you think? If we're going to
go into the details, I sort of feel like it would be good to somehow
contrast what is attached to membership with what is attached to the
INHERIT option or the SET option. I think it would be slightly
surprising not to mention the way that RLS rules are triggered by
privilege inheritance yet include the fact that the SET option affects
ALTER ... OWNER TO, but maybe I've got the wrong idea. An exhaustive
concordance of what depends on what might be too much, or maybe it
isn't, but it's probably good if the level of detail is pretty
uniform.

Your thoughts?

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

On Wed, Jan 04, 2023 at 03:56:34PM -0500, Robert Haas wrote:
> On Tue, Jan 3, 2023 at 5:03 PM Noah Misch <noah@leadboat.com> wrote:
> > I'd start with locations where the patch already added documentation.  In the
> > absence of documentation otherwise, a reasonable person could think WITH SET
> > controls just SET ROLE.  The documentation of WITH SET is a good place to list
> > what else you opted for it to control.  If the documentation can explain the
> > set of principles that would be used to decide whether WITH SET should govern
> > another thing in the future, that would provide extra value.
> 
> From the point of view of the code, we currently have four different
> functions that make inquiries about role membership:
> has_privs_of_role, is_member_of_role, is_member_of_role_nosuper, and
> member_can_set_role.
> 
> I spent a while looking at how has_privs_of_role() is used. Basically,
> there are three main patterns. First, in some places, you must have
> the privileges of a certain role (typically, either a predefined role
> or the role that owns some object) or the operation will fail with an
> error indicating that you don't have sufficient permissions. Second,
> there are places where having the privileges of a certain role exempts
> you from some other permissions check; if you have neither, you'll get
> an error. An example is that having the permissions of
> pg_read_all_data substitutes for a select privilege. And third, there
> are cases where you definitely won't get an error, but the behavior
> will vary depending on whether you have the privileges of some role.
> For instance, you can see more data in pg_stat_replication,
> pg_stat_wal_receiver, and other stats views if you have
> pg_read_all_stats. The GUC values reported in EXPLAIN output will
> exclude superuser-only values unless you have pg_read_all_settings. It
> looks like some maintenance commands like CLUSTER and VACUUM
> completely skip over, or just warn about, cases where permission is
> lacking. And weirdest of all, having the privileges of a role means
> that the RLS policies applied to that role also apply to you. That's
> odd because it makes permissions not strictly additive.
> 
> member_can_set_role() controls (a) whether you can SET ROLE to some
> other role, (b) whether you can alter the owner of an existing object
> to that role, and (c) whether you can create an object owned by some
> other user in cases where the CREATE command has an option for that,
> like CREATE DATABASE ... OWNER.
> 
> is_member_of_role_nosuper() is used to prevent creation of role
> membership loops, and for pg_hba.conf matching.
> 
> The only remaining call to is_member_of_role() is in
> pg_role_aclcheck(), which just supports the SQL-callable
> pg_has_role(). has_privs_of_role() and member_can_set_role() are used
> here, too.
> 
> How much of this should we document, do you think?

Rough thoughts:

Do document:
- For pg_read_all_stats, something like s/Read all pg_stat_/See all rows of all pg_stat_/
- At CREATE POLICY and/or similar places, explain the semantics used to judge
  the applicability of role_name to a given query.

Don't document:
- Mechanism for preventing membership loops.

Already documented adequately:
- "First, in some places, you must have the privileges of a certain role" is
  documented through language like "You must own the table".
- pg_read_all_data
- EXPLAIN.  I'm not seeing any setting that's both GUC_SUPERUSER_ONLY and
  GUC_EXPLAIN.
- SQL-level pg_has_role().

Unsure:
- At INHERIT, cover the not-strictly-additive RLS consequences.

> If we're going to
> go into the details, I sort of feel like it would be good to somehow
> contrast what is attached to membership with what is attached to the
> INHERIT option or the SET option.

Works for me.

> I think it would be slightly
> surprising not to mention the way that RLS rules are triggered by
> privilege inheritance yet include the fact that the SET option affects
> ALTER ... OWNER TO, but maybe I've got the wrong idea.

The CREATE POLICY syntax and docs show the role_name parameter, though they
don't detail how exactly the server determines whether a given role applies at
a given moment.  The docs are silent on the SET / OWNER TO connection.  Hence,
I think the doc gap around SET / OWNER TO is more acute than the doc gap
around this RLS behavior.

Thanks,
nm

On Fri, 2022-12-30 at 22:16 -0800, Noah Misch wrote:
> create user unpriv;
> grant pg_maintain to unpriv with set false;
> create schema maint authorization pg_maintain
>   create table t (c int);
> create or replace function maint.f() returns int language sql as
> 'select 1';
> alter function maint.f() owner to pg_maintain;
> set session authorization unpriv;
> create or replace function maint.f() returns int language sql
> security definer as 'select 1';
> create index on maint.t(c);

I dug into this case, as well as some mirror-image risks associated
with SECURITY INVOKER. This goes on a bit of a tangent and I'm sure I'm
retreading what others already know.

The risks of SECURITY DEFINER are about ownership: by owning something
with code attached, you're responsible to make sure the code is safe
and can only be run by the right users. Additionally, there are a
number of ways someone might come to own some code other than by
defining it themselves. Robert addressed the SET ROLE, CREATE ... OWNER
and the OWNER TO paths; but that still leaves the replace-function path
and the create index paths that you illustrated. As I said earlier I'm
not 100% satisfied with SET ROLE as a privilege; but I'm more
comfortable that it has a defined scope: the SET ROLE privilege should
control paths that can "gift" code to that user.

The risks of SECURITY INVOKER are more serious. It inherently means
that one user is writing code, and another is executing it. And in the
SQL world of triggers, views, expression indexes and logical
replication, the invoker often doesn't know what they are invoking.
There are search path risks, risks associated with resolving the right
function/operator/cast, risks of concurrent DDL (i.e. changing a
function definition right before a superuser executes it), etc. It
severely limits the kinds of trust models you can use in logical
replication. And SECURITY INVOKER weirdly inverts the trust
relationship of a GRANT: if A grants to B, then B must *completely*
trust A in order to exercise that new privilege because A can inject
arbitrary SECURITY INVOKER code in front of the object.

UNIX basically operates on a SECURITY INVOKER model, so I guess that
means that it can work. But then again, grepping a file doesn't execute
arbitrary code from inside that file (though there are bugs
sometimes... see [1]). It just seems like the wrong model for SQL.

[ Aside: that probably explains why the SQL spec defaults to SECURITY
DEFINER. ]

Brainstorming, I think we can do more to mitigate the risks of SECURITY
INVOKER:

* If running a command that would invoke a SECURITY INVOKER function
that is not owned by superuser or a member of the invoker's role, throw
an error instead. We could control this with a GUC for compatibility.

* Have SECURITY PUBLIC which executes with minimal privileges, which
would be good for convenience functions that might be used in an index
expression or view.

* Another idea is to separate out read privileges -- a SECURITY INVOKER
that is read-only is sounds less dangerous (though not without some
risk).

* Prevent extension scripts from running SECURITY INVOKER functions.


[1]
https://lcamtuf.blogspot.com/2014/10/psa-dont-run-strings-on-untrusted-files.html


--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Sat, Jan 7, 2023 at 12:00 AM Noah Misch <noah@leadboat.com> wrote:
> The docs are silent on the SET / OWNER TO connection.  Hence,

Reviewing the documentation again today, I realized that the
documentation describes the rules for changing the ownership of an
object in a whole bunch of places which this patch failed to update.
Here's a patch to update all of the places I found.

I suspect that these changes will mean that we don't also need to
adjust the discussion of the SET option itself, but let me know what
you think.

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

On Tue, Jan 10, 2023 at 2:28 AM Jeff Davis <pgsql@j-davis.com> wrote:
> The risks of SECURITY INVOKER are more serious. It inherently means
> that one user is writing code, and another is executing it. And in the
> SQL world of triggers, views, expression indexes and logical
> replication, the invoker often doesn't know what they are invoking.
> There are search path risks, risks associated with resolving the right
> function/operator/cast, risks of concurrent DDL (i.e. changing a
> function definition right before a superuser executes it), etc. It
> severely limits the kinds of trust models you can use in logical
> replication. And SECURITY INVOKER weirdly inverts the trust
> relationship of a GRANT: if A grants to B, then B must *completely*
> trust A in order to exercise that new privilege because A can inject
> arbitrary SECURITY INVOKER code in front of the object.

Yes. I think it's extremely difficult to operate a PostgreSQL database
with mutually untrusting users. If the high-privilege users don't
trust the regular users, they must also make very little use of the
database and only in carefully circumscribed ways. If not, the whole
security model unravels really fast. It would certainly be nice to do
better here.

> UNIX basically operates on a SECURITY INVOKER model, so I guess that
> means that it can work. But then again, grepping a file doesn't execute
> arbitrary code from inside that file (though there are bugs
> sometimes... see [1]). It just seems like the wrong model for SQL.

I often think about the UNIX model to better understand the problems
we have in PostgreSQL. I don't think that there's any real theoretical
difference between the cases, but there are practical differences
nearly all of which are unfavorable to PostgreSQL. For example, when
you log into your UNIX account, you have a home directory which is
pre-created. Your path is likely configured to contain only root-owned
directories and perhaps directories within your home directory that
are controlled by you, and the permissions on the root-owned
directories are locked down tight. That's because people figured out
in the 1970s and 1980s that if other people could write executable
code into a path you were likely to search, your account was probably
going to get compromised.

Now, in PostgreSQL, the equivalent of a home directory is a user
schema. We set things up to search those by default if they exist, but
we don't create them by default. We also put the public schema in the
default search path and, up until very recently, it was writeable by
default. In practice, many users probably put back write permission on
that schema, partly because if they don't, unprivileged users can't
create database objects anywhere at all. The practical effect of this
is that, when you log into a UNIX system, you're strongly encouraged
to access only things that are owned by you or root, and any new stuff
you create will be in a location where nobody but you is likely to
touch it. On the other hand, when you log into a PostgreSQL system,
you're set up by default to access objects created by other
unprivileged users and you may have nowhere to put your own objects
where those users won't also be accessing your stuff.

So the risks, which in theory are all very similar, are in practice
far greater in the PostgreSQL context, basically because our default
setup is about 40 years behind the times in terms of implementing best
practices. At least we've locked down write permission on pg_catalog.
I think some early UNIX systems didn't even do that, or not well. But
that's about the end of the good things that I have to say about what
we're doing in this area.

To be fair, I think many security people also consider it wise to
assume that a local unprivileged UNIX user can probably find a way to
escalate to root. There are a lot of setuid binaries on a
normally-configured UNIX system, and you only need to find one of them
that has an exploitable vulnerability. Those are the equivalent of
SECURITY DEFINER privileges, and I don't think we ship any of those in
a default configuration. In that regard, we're perhaps better-secured
than UNIX. Unfortunately, I think it is probably still wise to assume
that an unprivileged PostgreSQL user can find some way of getting
superuser if they want -- not only because of Trojan horse attacks
based on leaving security-invoker functions or procedures or operators
lying around, but also because I strongly suspect there are more
escalate-to-superuser bugs in the code than we've found yet. Those
we've not found, or have found but have not fixed, may still be known
to bad actors.

> [ Aside: that probably explains why the SQL spec defaults to SECURITY
> DEFINER. ]

I doubt that SECURITY DEFINER is safer in general than SECURITY
INVOKER. That'd be the equivalent of having binaries installed setuid
by default, which would be insane. I think it is right to regard
SECURITY DEFINER as the bigger threat by far. The reason it doesn't
always seem that way with PostgreSQL, at least in my view, is because
we make it so atrociously easy to accidentally invoke executable code
somewhere. If you start by assuming that you're probably going to
execute some random other user's code by accident, well then in that
world yes you would prefer to at least have it be running as them, not
you. But that's not really safe anyway. Sure, if the code runs as
them, they can't so easily usurp your privileges, but they can still
log everything you do, or make it fail, or make it take forever. Those
things are less serious than outright account takeover, but nobody
stands up a web site and hopes that it only gets DDOS'd rather than
vandalized. What you want is for it to stay up.

> Brainstorming, I think we can do more to mitigate the risks of SECURITY
> INVOKER:
>
> * If running a command that would invoke a SECURITY INVOKER function
> that is not owned by superuser or a member of the invoker's role, throw
> an error instead. We could control this with a GUC for compatibility.
>
> * Have SECURITY PUBLIC which executes with minimal privileges, which
> would be good for convenience functions that might be used in an index
> expression or view.
>
> * Another idea is to separate out read privileges -- a SECURITY INVOKER
> that is read-only is sounds less dangerous (though not without some
> risk).
>
> * Prevent extension scripts from running SECURITY INVOKER functions.

It might be best to repost some of these ideas on a new thread with a
relevant subject line, but I agree that there's some potential here.
Your first idea reminds me a lot of the proposal Tom made in
https://www.postgresql.org/message-id/19327.1533748538@sss.pgh.pa.us
-- except that his mechanism is more general, since you can say whose
code you trust and whose code you don't trust. Noah had a competing
version of that patch, too. But we never settled on an approach. I
still think something like this would be a good idea, and the fact
that you've apparently-independently come up with a similar notion
just reinforces that.

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

On Tue, 2023-01-10 at 11:45 -0500, Robert Haas wrote:
> So the risks, which in theory are all very similar, are in practice
> far greater in the PostgreSQL context, basically because our default
> setup is about 40 years behind the times in terms of implementing
> best
> practices.

I agree that huge improvements could be made with improvements to best
practices/defaults.

But there are some differences that are harder to fix that way. In
postgres, one can attach arbitrary code to pretty much anything, so you
need to trust everything you touch. There is no safe postgres
equivalent to grepping an untrusted file.


> It might be best to repost some of these ideas on a new thread with a
> relevant subject line, but I agree that there's some potential here.
> Your first idea reminds me a lot of the proposal Tom made in
> https://www.postgresql.org/message-id/19327.1533748538@sss.pgh.pa.us
> -- except that his mechanism is more general, since you can say whose
> code you trust and whose code you don't trust. Noah had a competing
> version of that patch, too. But we never settled on an approach. I
> still think something like this would be a good idea, and the fact
> that you've apparently-independently come up with a similar notion
> just reinforces that.

Will do, thank you for the reference.


--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Tue, Jan 10, 2023 at 11:06:52AM -0500, Robert Haas wrote:
> On Sat, Jan 7, 2023 at 12:00 AM Noah Misch <noah@leadboat.com> wrote:
> > The docs are silent on the SET / OWNER TO connection.  Hence,
> 
> Reviewing the documentation again today, I realized that the
> documentation describes the rules for changing the ownership of an
> object in a whole bunch of places which this patch failed to update.
> Here's a patch to update all of the places I found.

A "git grep 'direct or indirect mem'" found a few more:

doc/src/sgml/ref/alter_collation.sgml:42:   To alter the owner, you must also be a direct or indirect member of the
new
doc/src/sgml/ref/create_database.sgml:92:        role, you must be a direct or indirect member of that role,
doc/src/sgml/ref/create_schema.sgml:92:        owned by another role, you must be a direct or indirect member of

I wondered if the new recurring phrase "must be able to SET ROLE" should be
more specific, e.g. one of "must have
{permission,authorization,authority,right} to SET ROLE".  But then I stopped
wondering and figured "be able to" is sufficient.

> I suspect that these changes will mean that we don't also need to
> adjust the discussion of the SET option itself, but let me know what
> you think.

I still think docs for the SET option itself should give a sense of the
diversity of things it's intended to control.  It could be simple.  A bunch of
the sites you're modifying are near text like "These restrictions enforce that
altering the owner doesn't do anything you couldn't do by dropping and
recreating the aggregate function."  Perhaps the main SET doc could say
something about how it restricts other things that would yield equivalent
outcomes.  (Incidentally, DROP is another case of something one likely doesn't
want the WITH SET FALSE member using.  I think that reinforces a point I wrote
upthread.  To achieve the original post's security objective, the role must
own no objects whatsoever.)

On Wed, Jan 11, 2023 at 10:16 AM Noah Misch <noah@leadboat.com> wrote:
> A "git grep 'direct or indirect mem'" found a few more:
>
> doc/src/sgml/ref/alter_collation.sgml:42:   To alter the owner, you must also be a direct or indirect member of the
new
> doc/src/sgml/ref/create_database.sgml:92:        role, you must be a direct or indirect member of that role,
> doc/src/sgml/ref/create_schema.sgml:92:        owned by another role, you must be a direct or indirect member of

Ah, thanks.

> I wondered if the new recurring phrase "must be able to SET ROLE" should be
> more specific, e.g. one of "must have
> {permission,authorization,authority,right} to SET ROLE".  But then I stopped
> wondering and figured "be able to" is sufficient.

I think so, too. Note the wording of the error message in check_can_set_role().

> I still think docs for the SET option itself should give a sense of the
> diversity of things it's intended to control.  It could be simple.  A bunch of
> the sites you're modifying are near text like "These restrictions enforce that
> altering the owner doesn't do anything you couldn't do by dropping and
> recreating the aggregate function."  Perhaps the main SET doc could say
> something about how it restricts other things that would yield equivalent
> outcomes.  (Incidentally, DROP is another case of something one likely doesn't
> want the WITH SET FALSE member using.  I think that reinforces a point I wrote
> upthread.  To achieve the original post's security objective, the role must
> own no objects whatsoever.)

I spent a while on this. The attached is as well I was able to figure
out how to do. What do you think?

Thanks,

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

On Wed, Jan 11, 2023 at 03:13:29PM -0500, Robert Haas wrote:
> On Wed, Jan 11, 2023 at 10:16 AM Noah Misch <noah@leadboat.com> wrote:
> > I still think docs for the SET option itself should give a sense of the
> > diversity of things it's intended to control.  It could be simple.  A bunch of
> > the sites you're modifying are near text like "These restrictions enforce that
> > altering the owner doesn't do anything you couldn't do by dropping and
> > recreating the aggregate function."  Perhaps the main SET doc could say
> > something about how it restricts other things that would yield equivalent
> > outcomes.  (Incidentally, DROP is another case of something one likely doesn't
> > want the WITH SET FALSE member using.  I think that reinforces a point I wrote
> > upthread.  To achieve the original post's security objective, the role must
> > own no objects whatsoever.)
> 
> I spent a while on this. The attached is as well I was able to figure
> out how to do. What do you think?

I think this is good to go modulo one or two things:

> Subject: [PATCH v2] More documentation update for GRANT ... WITH SET OPTION.
> 
> Update the reference pages for various ALTER commands that
> mentioned that you must be a member of role that will be the
> new owner to instead say that you must be able to SET ROLE
> to the new owner. Update ddl.sgml's generate statement on this

s/generate/general/

> --- a/doc/src/sgml/ref/grant.sgml
> +++ b/doc/src/sgml/ref/grant.sgml
> @@ -298,6 +298,20 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
>     This option defaults to <literal>TRUE</literal>.
>    </para>
>  
> +  <para>
> +   To create an object owned by another role or give ownership of an existing
> +   object to another role, you must have the ability to <literal>SET
> +   ROLE</literal> to that role; otherwise, commands such as <literal>ALTER
> +   ... OWNER TO</literal> or <literal>CREATE DATABASE ... OWNER</literal>
> +   will fail.  However, a user who inherits the privileges of a role but does
> +   not have the ability to <literal>SET ROLE</literal> to that role may be
> +   able to obtain full access to the role by manipulating existing objects
> +   owned by that role (e.g. they could redefine an existing function to act
> +   as a Trojan horse).  Therefore, if a role's privileges are to be inherited
> +   but should not be accessible via <literal>SET ROLE</literal>, it should not
> +   own any SQL objects.
> +  </para>

I recommend deleting the phrase "are to be inherited but" as superfluous.  The
earlier sentence's mention will still be there.  WITH SET FALSE + NOINHERIT is
a combination folks should not use or should use only when the role has no
known privileges.

On Thu, Jan 12, 2023 at 12:09 AM Noah Misch <noah@leadboat.com> wrote:
> I think this is good to go modulo one or two things:
>
> > Subject: [PATCH v2] More documentation update for GRANT ... WITH SET OPTION.
> >
> > Update the reference pages for various ALTER commands that
> > mentioned that you must be a member of role that will be the
> > new owner to instead say that you must be able to SET ROLE
> > to the new owner. Update ddl.sgml's generate statement on this
>
> s/generate/general/

Oops, yes.

> > --- a/doc/src/sgml/ref/grant.sgml
> > +++ b/doc/src/sgml/ref/grant.sgml
> > @@ -298,6 +298,20 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
> >     This option defaults to <literal>TRUE</literal>.
> >    </para>
> >
> > +  <para>
> > +   To create an object owned by another role or give ownership of an existing
> > +   object to another role, you must have the ability to <literal>SET
> > +   ROLE</literal> to that role; otherwise, commands such as <literal>ALTER
> > +   ... OWNER TO</literal> or <literal>CREATE DATABASE ... OWNER</literal>
> > +   will fail.  However, a user who inherits the privileges of a role but does
> > +   not have the ability to <literal>SET ROLE</literal> to that role may be
> > +   able to obtain full access to the role by manipulating existing objects
> > +   owned by that role (e.g. they could redefine an existing function to act
> > +   as a Trojan horse).  Therefore, if a role's privileges are to be inherited
> > +   but should not be accessible via <literal>SET ROLE</literal>, it should not
> > +   own any SQL objects.
> > +  </para>
>
> I recommend deleting the phrase "are to be inherited but" as superfluous.  The
> earlier sentence's mention will still be there.  WITH SET FALSE + NOINHERIT is
> a combination folks should not use or should use only when the role has no
> known privileges.

I don't think I agree with this suggestion. If the privileges aren't
going to be inherited, it doesn't matter whether the role owns SQL
objects or not. And I think that there are two notable use cases for
SET FALSE + NOINHERIT (or SET FALSE + INHERIT FALSE). First, the a
grant with SET FALSE, INHERIT FALSE, ADMIN TRUE gives you the ability
to administer a role without inheriting its privileges or being able
to SET ROLE to it. You could grant yourself those abilities if you
want, but you don't have them straight off. In fact, CREATE ROLE
issued by a non-superuser creates such a grant implicitly as of
cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb. Second, SET FALSE, INHERIT
FALSE could be used to set up groups for pg_hba.conf matching without
conferring privileges.

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

On Thu, Jan 12, 2023 at 10:21:32AM -0500, Robert Haas wrote:
> On Thu, Jan 12, 2023 at 12:09 AM Noah Misch <noah@leadboat.com> wrote:

> > > --- a/doc/src/sgml/ref/grant.sgml
> > > +++ b/doc/src/sgml/ref/grant.sgml
> > > @@ -298,6 +298,20 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
> > >     This option defaults to <literal>TRUE</literal>.
> > >    </para>
> > >
> > > +  <para>
> > > +   To create an object owned by another role or give ownership of an existing
> > > +   object to another role, you must have the ability to <literal>SET
> > > +   ROLE</literal> to that role; otherwise, commands such as <literal>ALTER
> > > +   ... OWNER TO</literal> or <literal>CREATE DATABASE ... OWNER</literal>
> > > +   will fail.  However, a user who inherits the privileges of a role but does
> > > +   not have the ability to <literal>SET ROLE</literal> to that role may be
> > > +   able to obtain full access to the role by manipulating existing objects
> > > +   owned by that role (e.g. they could redefine an existing function to act
> > > +   as a Trojan horse).  Therefore, if a role's privileges are to be inherited
> > > +   but should not be accessible via <literal>SET ROLE</literal>, it should not
> > > +   own any SQL objects.
> > > +  </para>
> >
> > I recommend deleting the phrase "are to be inherited but" as superfluous.  The
> > earlier sentence's mention will still be there.  WITH SET FALSE + NOINHERIT is
> > a combination folks should not use or should use only when the role has no
> > known privileges.
> 
> I don't think I agree with this suggestion. If the privileges aren't
> going to be inherited, it doesn't matter whether the role owns SQL
> objects or not. And I think that there are two notable use cases for
> SET FALSE + NOINHERIT (or SET FALSE + INHERIT FALSE). First, the a
> grant with SET FALSE, INHERIT FALSE, ADMIN TRUE gives you the ability
> to administer a role without inheriting its privileges or being able
> to SET ROLE to it. You could grant yourself those abilities if you
> want, but you don't have them straight off. In fact, CREATE ROLE
> issued by a non-superuser creates such a grant implicitly as of
> cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb.

That is a valid use case, but Trojan horse matters don't apply there.

> Second, SET FALSE, INHERIT
> FALSE could be used to set up groups for pg_hba.conf matching without
> conferring privileges.

That is factual, but doing this and having that role own objects shouldn't be
considered a best practice.  It's a bit like using the address of a function
as an enum value.  Instead of role own_some_objects_and_control_hba, the best
practice would be to have two roles, own_some_objects / control_hba.

Since the text is superfluous but not wrong, I won't insist.

On Fri, Jan 13, 2023 at 2:17 AM Noah Misch <noah@leadboat.com> wrote:
> Since the text is superfluous but not wrong, I won't insist.

OK, committed as I had it, then.

To me, the text isn't superfluous, because otherwise the connection to
what has been said in the previous sentence seems tenuous, which
impacts understandability. We'll see what other people think, I guess.
Perhaps there's some altogether better way to talk about this.

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