Обсуждение: CREATE ROLE IF NOT EXISTS

> On 19 Oct 2021, at 22:12, David Christensen <david.christensen@crunchydata.com> wrote:
>
> Greetings -hackers,
>
> Enclosed is a patch that implements CREATE ROLE IF NOT EXISTS (along with the same support for USER/GROUP).  This is
afairly straightforward approach in that we do no validation of anything other than existence, with the user needing to
ensurethat permissions/grants are set up in the proper way. 
>
> Comments?

This fails the roleattributes test in "make check", with what seems to be a
trivial change in the output.  Can you please submit a rebased version fixing
the test?

--
Daniel Gustafsson        https://vmware.com/

David Christensen <david.christensen@crunchydata.com> writes:
> Updated version attached.

I'm generally pretty down on IF NOT EXISTS semantics in all cases,
but it seems particularly dangerous for something as fundamental
to privilege checks as a role.  It's not hard at all to conjure up
scenarios in which this permits privilege escalation.  That is,
Alice wants to create role Bob and give it some privileges, but
she's lazy and writes a quick-and-dirty script using CREATE ROLE
IF NOT EXISTS.  Meanwhile Charlie sneaks in and creates Bob first,
and then grants it to himself.  Now Alice's script is giving away
all sorts of privilege to Charlie.  (Admittedly, Charlie must have
CREATEROLE privilege already, but that doesn't mean he has every
privilege that Alice has --- especially not as we continue working
to slice the superuser salami ever more finely.)

Do we really need this?

            regards, tom lane

> On 3 Nov 2021, at 23:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I'm generally pretty down on IF NOT EXISTS semantics in all cases,
> but it seems particularly dangerous for something as fundamental
> to privilege checks as a role.  It's not hard at all to conjure up
> scenarios in which this permits privilege escalation.  That is,
> Alice wants to create role Bob and give it some privileges, but
> she's lazy and writes a quick-and-dirty script using CREATE ROLE
> IF NOT EXISTS.  Meanwhile Charlie sneaks in and creates Bob first,
> and then grants it to himself.  Now Alice's script is giving away
> all sorts of privilege to Charlie.  (Admittedly, Charlie must have
> CREATEROLE privilege already, but that doesn't mean he has every
> privilege that Alice has --- especially not as we continue working
> to slice the superuser salami ever more finely.)

I agree with this take, I don't think the convenience outweighs the risk in
this case.

--
Daniel Gustafsson        https://vmware.com/

Greetings,

* Daniel Gustafsson (daniel@yesql.se) wrote:
> > On 3 Nov 2021, at 23:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I'm generally pretty down on IF NOT EXISTS semantics in all cases,
> > but it seems particularly dangerous for something as fundamental
> > to privilege checks as a role.  It's not hard at all to conjure up
> > scenarios in which this permits privilege escalation.  That is,
> > Alice wants to create role Bob and give it some privileges, but
> > she's lazy and writes a quick-and-dirty script using CREATE ROLE
> > IF NOT EXISTS.  Meanwhile Charlie sneaks in and creates Bob first,
> > and then grants it to himself.  Now Alice's script is giving away
> > all sorts of privilege to Charlie.  (Admittedly, Charlie must have
> > CREATEROLE privilege already, but that doesn't mean he has every
> > privilege that Alice has --- especially not as we continue working
> > to slice the superuser salami ever more finely.)
>
> I agree with this take, I don't think the convenience outweighs the risk in
> this case.

I don't quite follow this.  The entire point of Alice writing a script
that uses IF NOT EXISTS is to have that command not fail if, indeed,
that role already exists, but for the rest of the script to be run.
That there's some potential attacker with CREATEROLE running around
creating roles that they think someone *else* might create is really
stretching things to a very questionable level- especially with
CREATEROLE where Charlie could just CREATE a new role which is a member
of Bob anyway after the fact and then GRANT that role to themselves.

The reason this thread was started is that it's a pretty clearly useful
thing to be able to use IF NOT EXISTS for CREATE ROLE and I don't agree
with the justification that we shouldn't allow it because someone might
use it carelessly.  For one, I really doubt that's actually a risk at
all, but more importantly there's a lot of very good use-cases where
it'll be used correctly and not having it means having to do other ugly
things like write a pl/pgsql function which checks pg_roles and would
end up having the exact same risk but be a lot more clunky.  And, yes,
people are already doing that.  Let's give them useful tools and
document that they be careful with them, not make them jump through
hoops.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> I don't quite follow this.  The entire point of Alice writing a script
> that uses IF NOT EXISTS is to have that command not fail if, indeed,
> that role already exists, but for the rest of the script to be run.
> That there's some potential attacker with CREATEROLE running around
> creating roles that they think someone *else* might create is really
> stretching things to a very questionable level- especially with
> CREATEROLE where Charlie could just CREATE a new role which is a member
> of Bob anyway after the fact and then GRANT that role to themselves.

I agree that as things stand, CREATEROLE is powerful enough that Charlie
doesn't need any subterfuge to become a member of the Bob role.  However,
in view of other work that's going on, I think we shouldn't design the
system on the assumption that it'll always be that way.  As soon as
there exist roles that can create roles but cannot make arbitrary
privilege grants, this becomes an interesting security question.
Do you really think that's never going to happen?

My concern here is basically that the semantics of CINE --- ie, that
you don't really know the initial properties of the target object ---
seem far more dangerous for a role than for any other sort of object.
The possibility of unexpected grants on or to that role means
that you may be giving away privileges unintentionally.

> The reason this thread was started is that it's a pretty clearly useful
> thing to be able to use IF NOT EXISTS for CREATE ROLE and I don't agree
> with the justification that we shouldn't allow it because someone might
> use it carelessly.

I'm not buying the argument that it's a "clearly useful thing".
I think it's a foot-gun, and I repeat the point that nobody's
actually provided a concrete use-case.

            regards, tom lane

> On Nov 8, 2021, at 10:38 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
> I don't quite follow this.  The entire point of Alice writing a script
> that uses IF NOT EXISTS is to have that command not fail if, indeed,
> that role already exists, but for the rest of the script to be run.
> That there's some potential attacker with CREATEROLE running around
> creating roles that they think someone *else* might create is really
> stretching things to a very questionable level- especially with
> CREATEROLE where Charlie could just CREATE a new role which is a member
> of Bob anyway after the fact and then GRANT that role to themselves.

I don't see why this is "stretching things to a very questionable level".  It might help this discussion if you could
providepseudo-code or similar for adding roles which is well-written and secure, and which benefits from this syntax.
Iwould expect the amount of locking and checking for pre-existing roles that such logic would require would make the IF
NOTEXIST option useless.  Perhaps I'm wrong? 

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

> On Nov 9, 2021, at 7:36 AM, David Christensen <david.christensen@crunchydata.com> wrote:
>
> If CINE semantics are at issue, what about the CREATE OR REPLACE semantics with some sort of merge into the existing
role? I don't care strongly about which approach is taken, just think the overall "make this role exist in this form"
withoutan error is useful in my own work, and CINE was easier to implement as a first pass. 

CREATE OR REPLACE might be a better option, not with the "merge into the existing role" part, but rather as
drop+create. If a malicious actor has already added other roles to the role, or created a table with a malicious
triggerdefinition, the drop part will fail, which is good from a security viewpoint.  Of course, the drop portion will
alsofail under other conditions which don't entail any security concerns, but maybe they could be addressed in a series
offollow-on patches? 

I understand this idea is not as useful for creating idempotent scripts, but maybe it gets you part of the way there?

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

Greetings,

* David Christensen (david.christensen@crunchydata.com) wrote:
> On Mon, Nov 8, 2021 at 1:22 PM Mark Dilger <mark.dilger@enterprisedb.com>
> wrote:
>
> > > On Nov 8, 2021, at 10:38 AM, Stephen Frost <sfrost@snowman.net> wrote:
> >
> > > I don't quite follow this.  The entire point of Alice writing a script
> > > that uses IF NOT EXISTS is to have that command not fail if, indeed,
> > > that role already exists, but for the rest of the script to be run.
> > > That there's some potential attacker with CREATEROLE running around
> > > creating roles that they think someone *else* might create is really
> > > stretching things to a very questionable level- especially with
> > > CREATEROLE where Charlie could just CREATE a new role which is a member
> > > of Bob anyway after the fact and then GRANT that role to themselves.
> >
> > I don't see why this is "stretching things to a very questionable level".
> > It might help this discussion if you could provide pseudo-code or similar
> > for adding roles which is well-written and secure, and which benefits from
> > this syntax.  I would expect the amount of locking and checking for
> > pre-existing roles that such logic would require would make the IF NOT
> > EXIST option useless.  Perhaps I'm wrong?
> >
>
> The main motivator for me writing this was trying to increase idempotency
> for things like scripting, where you want to be able to minimize the effort
> required to get things into a particular state.  I agree with Stephen that
> whether or not this is a best practices approach, this is something that is
> being done in the wild via DO blocks or similar, so providing a tool to
> handle this better seems useful on its own.

Agreed.

> This originally came from me looking into the failures to load certain
> `pg_dump` or `pg_dumpall` output when generated with the `--clean` flag,
> which necessarily cannot work, as it fails with the error `current user
> cannot be dropped`.  Not that I am promoting the use of `pg_dumpall
> --clean`, as there are clearly better solutions here, but something which
> generates unusable output does not seem that useful.  Instead, you could
> generate `CREATE ROLE IF NOT EXISTS username` statements and emit `ALTER
> ROLE ...`, which is what it is already doing (modulo `IF NOT EXISTS`).

The other very common case that I've seen is where the role ends up
owning objects and therefore can't be dropped without also dropping
those objects- possibly just GRANTs but may also be tables or other
things.  In other words, a script like this:

DROP ROLE IF EXISTS r1;
CREATE ROLE r1;
CREATE SCHEMA IF NOT EXISTS r1 AUTHORIZATION r1;

isn't able to be re-run, while this is able to be:

CREATE ROLE IF NOT EXISTS r1;
CREATE SCHEMA IF NOT EXISTS r1 AUTHORIZATION r1;

> This seems to introduce no further security vectors compared to field work
> and increases utility in some cases, so seems generally useful to me.

Yeah.

> If CINE semantics are at issue, what about the CREATE OR REPLACE semantics
> with some sort of merge into the existing role?  I don't care strongly
> about which approach is taken, just think the overall "make this role exist
> in this form" without an error is useful in my own work, and CINE was
> easier to implement as a first pass.

I don't really see how we could do CREATE OR REPLACE here, at least for
the cases that I'm thinking about.  How would that work with existing
GRANTs, for example?  Perhaps it'd be alright if we limited it to just
what can be specified in the CREATE ROLE and then left anything else in
place.  I do generally like the idea of being able to explicitly say how
the role should look in one shot.

Thanks,

Stephen

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Nov 9, 2021, at 7:36 AM, David Christensen <david.christensen@crunchydata.com> wrote:
> > If CINE semantics are at issue, what about the CREATE OR REPLACE semantics with some sort of merge into the
existingrole?  I don't care strongly about which approach is taken, just think the overall "make this role exist in
thisform" without an error is useful in my own work, and CINE was easier to implement as a first pass. 
>
> CREATE OR REPLACE might be a better option, not with the "merge into the existing role" part, but rather as
drop+create. If a malicious actor has already added other roles to the role, or created a table with a malicious
triggerdefinition, the drop part will fail, which is good from a security viewpoint.  Of course, the drop portion will
alsofail under other conditions which don't entail any security concerns, but maybe they could be addressed in a series
offollow-on patches? 
>
> I understand this idea is not as useful for creating idempotent scripts, but maybe it gets you part of the way there?

If it's actually drop+create then, no, that isn't really useful because
it'll fail when that role owns objects (see my other email).  If we can
avoid that issue then CREATE OR REPLACE might work, we just need to make
sure that we document what is, and isn't, done in such a case.

Thanks,

Stephen

Greetings,

* David Christensen (david.christensen@crunchydata.com) wrote:
> On Tue, Nov 9, 2021 at 9:55 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> > > On Nov 9, 2021, at 7:36 AM, David Christensen <
> > david.christensen@crunchydata.com> wrote:
> > > If CINE semantics are at issue, what about the CREATE OR REPLACE
> > semantics with some sort of merge into the existing role?  I don't care
> > strongly about which approach is taken, just think the overall "make this
> > role exist in this form" without an error is useful in my own work, and
> > CINE was easier to implement as a first pass.
> >
> > CREATE OR REPLACE might be a better option, not with the "merge into the
> > existing role" part, but rather as drop+create.  If a malicious actor has
> > already added other roles to the role, or created a table with a malicious
> > trigger definition, the drop part will fail, which is good from a security
> > viewpoint.  Of course, the drop portion will also fail under other
> > conditions which don't entail any security concerns, but maybe they could
> > be addressed in a series of follow-on patches?
> >
> > I understand this idea is not as useful for creating idempotent scripts,
> > but maybe it gets you part of the way there?
>
> Well, the CREATE OR REPLACE via just setting the role's attributes
> explicitly based on what you passed it could work (not strictly DROP +
> CREATE, in that you're keeping existing ownerships, etc, and can avoid
> cross-db permissions/ownership checks).  Seems like some sort of merge
> logic could be in order, as you wouldn't really want to lose existing
> permissions granted to a role, but you want to ensure that /at least/ the
> permissions granted exist for this role.

What happens with role attributes that aren't explicitly mentioned
though?  Do those get reset to 'default' or are they left as-is?

I suspect that most implementations will end up just explicitly setting
all of the role attributes, of course, because they want the role to
look like how it is defined to in whatever manifest is declaring the
role, but we should still think about how we want this to work if we're
going in this direction.

In terms of least-surprise, I do tend to think that the answer is "only
care about what is explicitly put into the command"- that is, if it
isn't in the CREATE ROLE statement then it gets left as-is.  Not sure
how others feel about that though.

Thanks,

Stephen

> On Nov 9, 2021, at 8:22 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
> In terms of least-surprise, I do tend to think that the answer is "only
> care about what is explicitly put into the command"- that is, if it
> isn't in the CREATE ROLE statement then it gets left as-is.  Not sure
> how others feel about that though.

bob:  CREATE ROLE charlie;
bob:  GRANT charlie TO david;

super_alice: CREATE OR REPLACE ROLE charlie SUPERUSER;

I think this is the sort of thing Tom and I are worried about.  "david" is now a member of a superuser role, and it is
farfrom clear that "super_alice" intended that.  Even if "bob" is not malicious, having this happen by accident is
prettybad. 

If we fix the existing bug that the pg_auth_members.grantor field can end up as a dangling reference, instead making
surethat it is always accurate, then perhaps this would be ok if all roles granted into "charlie" had
grantor="super_alice". I'm not sure that is really good enough, but it is a lot closer to making this safe than
allowingthe command to succeed when role "charlie" has been granted away by someone else. 

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

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Nov 9, 2021, at 8:22 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > In terms of least-surprise, I do tend to think that the answer is "only
> > care about what is explicitly put into the command"- that is, if it
> > isn't in the CREATE ROLE statement then it gets left as-is.  Not sure
> > how others feel about that though.
>
> bob:  CREATE ROLE charlie;
> bob:  GRANT charlie TO david;
>
> super_alice: CREATE OR REPLACE ROLE charlie SUPERUSER;
>
> I think this is the sort of thing Tom and I are worried about.  "david" is now a member of a superuser role, and it
isfar from clear that "super_alice" intended that.  Even if "bob" is not malicious, having this happen by accident is
prettybad. 

I understand the concern that you and Tom have raised, I just don't see
it as such an issue that we can't give users this option.  They're
already doing it via DO blocks and that's surely not any better.
Documenting that you should care about who is able to create roles in
your system when thinking about this is certainly reasonable, but just
saying we won't add it because someone might somewhere mis-use it isn't.

> If we fix the existing bug that the pg_auth_members.grantor field can end up as a dangling reference, instead making
surethat it is always accurate, then perhaps this would be ok if all roles granted into "charlie" had
grantor="super_alice". I'm not sure that is really good enough, but it is a lot closer to making this safe than
allowingthe command to succeed when role "charlie" has been granted away by someone else. 

I agree we should fix the issue of the grantor field being a dangling
reference, that's clearly not a good thing.

I'm not sure what is meant by making sure they're always 'accurate' or
why 'accurate' in this case means that the grantor is always
'super_alice'..?  Are you suggesting that the CREATE OR REPLACE ROLE run
by super_alice would remove the GRANT that bob made of granting charlie
to david?  I would argue that it's entirely possible that super_alice
knows exactly what is going on and intends for charlie to have superuser
access and understands that any role which charlie has been GRANT'd to
would therefore be able to become charlie, that's not a surprise.

Now, bringing this around to the more general discussion about making it
possible for folks who aren't superuser to be able to create roles, I
think there's another way to address this that might satisfy everyone,
particularly with the CREATE OR REPLACE approach- to wit: if the role
create isn't one that you've got appropriate rights on, then you
shouldn't be able to CREATE OR REPLACE it.  This, perhaps, gets to a
distinction between having ADMIN rights on a role vs. the ability to
redefine the role (perhaps by virtue of being the 'owner' of that role)
that's useful.

In other words, in the case outlined above:

bob: CREATE ROLE charlie;
 -- charlie is now a role 'owned' by bob and that isn't able to be
 -- changed by bob to be some other owner, unless bob can become the
 -- role to which they want to change ownership to
bob: GRANT charlie TO david;

alice: CREATE OR REPLACE ROLE charlie;
 -- This now fails because while alice is able to create roles, alice
 -- can only 'replace' roles which alice owns.

I appreciate that the case of 'super_alice' doing things where they're
an actual superuser might still be an issue, but running around doing
things with superuser is already risky business and the point here is to
get away from doing that by splitting superuser up, ideally in a way
that privileges can be given out to non-superusers in a manner that's
safer than doing things as a superuser and where independent
non-superusers aren't able to do bad things to each other.

Thanks,

Stephen

> On Nov 9, 2021, at 8:50 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
>> If we fix the existing bug that the pg_auth_members.grantor field can end up as a dangling reference, instead making
surethat it is always accurate, then perhaps this would be ok if all roles granted into "charlie" had
grantor="super_alice". I'm not sure that is really good enough, but it is a lot closer to making this safe than
allowingthe command to succeed when role "charlie" has been granted away by someone else. 
>
> I agree we should fix the issue of the grantor field being a dangling
> reference, that's clearly not a good thing.

Just FYI, I have a patch underway to fix it.  I'm not super close to posting it, though.

> I'm not sure what is meant by making sure they're always 'accurate' or
> why 'accurate' in this case means that the grantor is always
> 'super_alice'..?

I mean that the dangling reference could point at a role that no longer exists, but if the oid gets recycled, it could
pointat the *wrong* role rather than merely at no role.  So we'd need that fixed before we could rely on the "grantor"
fieldfor anything.  I think Robert mentioned this issue already, on another thread. 

>  Are you suggesting that the CREATE OR REPLACE ROLE run
> by super_alice would remove the GRANT that bob made of granting charlie
> to david?

Suppose user "stephen.frost" owns a database and runs a script which creates roles, schemas, etc:

  CREATE OR REPLACE ROLE super_alice SUPERUSER;
  SET SESSION AUTHORIZATION super_alice;
  CREATE OR REPLACE ROLE charlie;
  CREATE OR REPLACE ROLE david IN ROLE charlie;

User "stephen.frost" runs that script again.  The system cannot tell, as things currently are implemented, that
"stephen.frost"was the original creator of role "super_alice", nor that "super_alice" was the original creator of
"charlie"and "david".  

The "grantor" field for "david"'s membership in "charlie" points at "super_alice", so we know enough to allow the "IN
ROLEcharlie" part, at least if we fix the dangling reference bug. 

If we add an "owner" (or perhaps a "creator") field to pg_authid, the first time the script runs, it could be set to
"stephen.frost"for "super_alice" and to "super_alice" for "charlie" and "david".  When the script gets re-run, the
CREATEOR REPLACE commands can succeed because that field matches. 

>  I would argue that it's entirely possible that super_alice
> knows exactly what is going on and intends for charlie to have superuser
> access and understands that any role which charlie has been GRANT'd to
> would therefore be able to become charlie, that's not a surprise.

I agree that super_alice might know that, but perhaps we can make this feature less of a foot-gun and still achieve the
goalof making idempotent role creation scripts work? 

> Now, bringing this around to the more general discussion about making it
> possible for folks who aren't superuser to be able to create roles, I
> think there's another way to address this that might satisfy everyone,
> particularly with the CREATE OR REPLACE approach- to wit: if the role
> create isn't one that you've got appropriate rights on, then you
> shouldn't be able to CREATE OR REPLACE it.

Agreed.  You shouldn't be able to CREATE OR REPLACE a role that you couldn't CREATE in the first case.

>  This, perhaps, gets to a
> distinction between having ADMIN rights on a role vs. the ability to
> redefine the role (perhaps by virtue of being the 'owner' of that role)
> that's useful.
>
> In other words, in the case outlined above:
>
> bob: CREATE ROLE charlie;
> -- charlie is now a role 'owned' by bob and that isn't able to be
> -- changed by bob to be some other owner, unless bob can become the
> -- role to which they want to change ownership to
> bob: GRANT charlie TO david;
>
> alice: CREATE OR REPLACE ROLE charlie;
> -- This now fails because while alice is able to create roles, alice
> -- can only 'replace' roles which alice owns.

This sounds reasonable.  It means, of course, implementing a role ownership system.  I thought you had other concerns
aboutdoing so. 

> I appreciate that the case of 'super_alice' doing things where they're
> an actual superuser might still be an issue, but running around doing
> things with superuser is already risky business and the point here is to
> get away from doing that by splitting superuser up, ideally in a way
> that privileges can be given out to non-superusers in a manner that's
> safer than doing things as a superuser and where independent
> non-superusers aren't able to do bad things to each other.

I'm not sure what to do about this.

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

> On 10 Nov 2021, at 18:14, David Christensen <david.christensen@crunchydata.com> wrote:

> Modulo other issues/discussions, here is a version of this patch..

This patch fails to compile since you renamed the if_not_exists member in
CreateRoleStmt but still set it in the parser.

--
Daniel Gustafsson        https://vmware.com/

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

Wouldn't using opt_or_replace rule be a better option?

The new status of this patch is: Waiting on Author