Re: role self-revocation - Mailing list pgsql-hackers

Greetings,

* David G. Johnston (david.g.johnston@gmail.com) wrote:
> On Thu, Mar 10, 2022 at 11:05 AM Stephen Frost <sfrost@snowman.net> wrote:
> > * David G. Johnston (david.g.johnston@gmail.com) wrote:
> > > On Thu, Mar 10, 2022 at 9:19 AM Stephen Frost <sfrost@snowman.net> wrote:
> > > > * David G. Johnston (david.g.johnston@gmail.com) wrote:
> > > > > On Thu, Mar 10, 2022 at 7:46 AM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > > The only indirect way for a role to become superuser is to have been
> > > granted membership in a superuser group, then SET ROLE.  Non-superusers
> > > cannot do this.  If a superuser does this I consider the outcome to be no
> > > different than if they go and do:
> >
> > A non-superuser absolutely can be GRANT'd membership in a superuser role
> > and then SET ROLE to that user thus becoming a superuser.
>
> A non-superuser cannot grant a non-superuser membership in a superuser
> role.  A superuser granting a user membership in a superuser role makes
> that user a superuser.  This seems sane.
>
> If a superuser grants a non-superuser membership in a superuser role then
> today a non-superuser can grant a user membership in that intermediate
> role, thus having a non-superuser make another user a superuser.  This is
> arguably a bug that needs to be fixed.
>
> My desired fix is to just require the superuser to mark (or have it marked
> by default ideally) the role inheriting superuser and put the
> responsibility on the superuser.  I agree this is not ideal, but it is
> probably quick and low risk.
>
> I'll let someone else describe the details of the alternative option.  I
> suspect it will end up being a better option in terms of design.  But
> depending on time and risk even knowing that we want the better design
> eventually doesn't preclude getting the easier fix in now.
>
> > No, what should matter is if the role doing the GRANT has admin rights
> > on pg_read_all_stats, or on the postgres role.  That also happens to be
> > what the spec says.
>
> Yes, and superusers implicitly have that right, while CREATEROLE users
> implicitly have that right on the pg_* role but not on superuser roles.  I
> just want to plug that hole and include the pg_* roles (or any role for
> that matter) in being able to be denied implied ADMIN rights for
> non-superusers.

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

> > Today a non-superuser cannot "grant postgres to someuser;"
> >
> > No, but a role can be created like 'admin', which a superuser GRANT's
> > 'postgres' to and then that role can be GRANT'd to anyone by anyone who
> > has CREATEROLE rights.  That's not sane.
>
> I agree.  And I've suggested a minimal fix, adding an attribute to the role
> that prohibits non-superusers from granting it to others, that removes the
> insane behavior.

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

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

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

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

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

> > I disagree entirely with the idea that we must have some roles who can
> > only ever be administered by a superuser.
>
> I don't think this is a must have.  I think that since we do have it today
> that fixes that leverage the status quo in order to be done more easily are
> perfectly valid solutions.

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

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

Great.

> > > In particular, this means CREATEROLE roles cannot assign membership in
> > the
> > > marked roles; just like they cannot assign membership in superuser roles
> > > today.
> >
> > I disagree with the idea that we need to mark some roles as only being
> > able to be modified by the superuser- why invent this?
>
> Because CREATEUSER is a thing and people want to prevent roles with that
> attribute from assigning membership to the predefined superuser-aspect
> roles.  If I've misunderstood that desire and the scope of delegation given
> by the superuser to CREATEUSER roles is acceptable, then no change here is
> needed.

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

> What you
> > seem to be arguing for here is to rip out the ADMIN functionality, which
> > is defined by spec and not even exclusively by PG, and replace it with a
> > single per-role flag that says if that role can only be modified by
> > superusers.
>
> I made the observation that being able to manage the membership of a group
> without having the ability to create new users seems like a half a loaf of
> a feature.  That's it.  I would presume that any redesign of the
> permissions system here would address this adequately.

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

>  The
> >
> short answer is that it's not- which is why we have documented
> > CREATEROLE as being 'superuser light'.  The goal here is to get rid of
> > that.
>
> Now you tell me.  Robert should have led with that goal upfront.

... blink.

> > Yes, we're talking about a new feature- one intended to replace the
> > broken way that CREATEROLE works, which your proposal doesn't.
>
> That is correct, I was trying to figure out minimally invasive fixes to
> what are arguably being called bugs.

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

Thanks,

Stephen