Re: fixing CREATEROLE - Mailing list pgsql-hackers

On Wed, Nov 23, 2022 at 12:36 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> Oh, I don't mean that it will be poorly documented.  I mean that the way the command is written won't advertise what
itis going to do.  That's concerning if you fat-finger a CREATE ROLE command, then realize you need to drop and
recreatethe role, only to discover that a property you weren't thinking about, and which you are accustomed to being
setthe opposite way, is set such that you can't drop the role you just created. 

That doesn't ever happen. No matter how the properties are set, you
end up with ADMIN OPTION on the newly-created role and can drop it.
The flags control things like whether you can select from the newly
created role's tables even if you otherwise lack permissions on them
(INHERIT), and whether you can SET ROLE to it (SET). You can always
administer it, i.e. grant rights on it to others, change its password,
drop it.

> I think if you're going to create-and-disown something, you should have to say so, to make sure you mean it.

Reasonable, but not relevant, since that isn't what's happening.

> Why not make this be a permissions issue, rather than a default behavior issue?  Instead of a single CREATEROLE
privilege,grant roles privileges to CREATE-WITH-INHERIT, CREATE-WITH-ADMIN, CREATE-SANS-INHERIT, CREATE-SANS-ADMIN, and
therebylimit which forms of the command they may execute.  That way, the semantics of the command do not depend on some
propertyexternal to the command.  Users (and older scripts) will expect the traditional syntax to behave consistent
withhow CREATE  ROLE has worked in the past.  The behaviors can be specified explicitly. 

Perhaps if we get the confusion above cleared up you won't be as
concerned about this, but let me just say that this patch is
absolutely breaking backward compatibility. I don't feel bad about
that, either. I think it's a good thing in this case, because the
current behavior is abjectly broken and horrible. What we've been
doing for the last several years is shipping a product that has a
built-in exploit that a clever 10-year-old could use to escalate
privileges from CREATEROLE to SUPERUSER. We should not be OK with
that, and we should be OK with changing the behavior however much is
required to fix it. I'm personally of the opinion that this patch set
does a rather clever job minimizing that blast radius, but that might
be my own bias as the patch author. Regardless, I don't think there's
any reasonable argument for maintaining the current behavior. I don't
entirely follow exactly what you have in mind in the sentence above,
but if it involves keeping the current CREATEROLE behavior around in
any form, -1 from me.

> > But since this implicit grant has, and must have, the bootstrap
> > superuser as grantor, it is also only reasonable that superusers get
> > to determine what options are used when creating that grant, rather
> > than leaving that up to the CREATEROLE user.
>
> Yes, this all makes sense, but does it entail that the CREATE ROLE command must behave differently on the basis of a
setting?

Well, we certainly don't HAVE to add those new role-level properties;
that's why they are in a separate patch. But I think they add a lot of
useful functionality for a pretty minimal amount of extra code
complexity.

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