Re: Delegating superuser tasks to new security roles (Was: Granting control of SUSET gucs to non-superusers)

Greetings,

* Mark Dilger (mark(dot)dilger(at)enterprisedb(dot)com) wrote:
> > On Jul 22, 2021, at 1:01 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > It's awkward. I think that we can't afford to create a separate
> > predefined role for every single thing that someone could
> > theoretically want to sever, because then we'll have a zillion of them
> > and it will be unmaintainable. So the idea was to try to break up
> > everything someone might want to do either via DDL or by setting GUCs
> > into one of three categories: internal to the database
> > (pg_database_security), facing outward toward the network
> > (pg_network_security), and facing inward toward the host
> > (pg_host_security). If we didn't have any predefined security-related
> > roles already, this would still have complications, but as things
> > stand it has more, because as you point out, pg_read_server_files
> > overlaps with pg_host_security. But what do we do about that?
>
> I gave up on the idea of splitting all superuser functions into three roles.

I can't say that I blame you for that. :) For my 2c, at least, the ones
proposed never really felt like they were very directly tied to specific
capabilities, which I think was one of the issues with that approach.

> Patch v5-0001 refactors the guc code to allow non-superuser roles to be associated with guc variables. Any such role can then set the variable, including via "alter system set". The patch stops short of creating any new roles or assigning any roles to any guc variable.

Haven't looked at the patch yet but this does generally seem like an
interesting approach.

> Patches v5-0002 through v5-0005 create four new roles for managing host resource settings, vacuum settings, autovacuum settings, and logging settings. That last one excludes "where to log" settings, because we don't want the role to be able to write to arbitrary locations on the server. Remaining guc variables not in these four categories continue to belong to the superuser.

We do have a role today who is allowed to write to arbitrary locations
on the server, so I wonder if for those log settings we'd include a
requirement for the user to have both of those roles instead..?

> Patches v5-0006 and v5-0007 allow non-superusers to own event triggers, and limit the event triggers to only running for events triggered by roles that the event trigger owner belongs to. This is backward compatible, because event triggers have historically belonged only to superusers, and superusers have implicit membership in all groups.

While I generally agree that this doesn't end up opening up security
issues, it's going to certainly be a change in how event triggers work
as they'll no longer *always* fire, and that seems quite at odds with
how triggers are generally thought of. So much so that I worry about
mis-use due to this. Then again, if we're going to go down this route
at all, I can't think of any particular way to avoid the security issues
of running the trigger for everyone when it's owned by a non-superuser.

> Patches v5-0008 through v5-0010 allow non-superusers to own subscriptions while restricting the tablesync and apply workers to only work on tables that the subscription owner has permissions on. This is almost backward compatible, because subscriptions have historically belonged only to superusers, as above, except for unlikely scenarios where superusers have given ownership to non-superusers. In those cases, the new code will refuse to apply in situations where the old code would blindly apply changes. Does anybody see a problem with this?

This doesn't particularly bother me, at least.

> Patch v5-0011 is a bug fix posted elsewhere that hasn't been committed yet but which must be committed in preparation for v5-0012.

No idea what it is as I hadn't looked yet, but if it's a bug fix then
shouldn't it be separated and back-patched..?

> Patch v5-0012 creates a new role, pg_manage_database_objects, which can do anything with an object that the owner could do with it, as long as the owner is not a superuser. This role is intended as a "tenant" role, and is in some sense a less powerful replacement for the pg_database_security role previously proposed.

This I have to object to pretty strongly- we have got to get away from
the idea that just because X isn't a superuser or isn't owned by a
superuser that it's fine to allow some non-superuser to mess with it.
In particlar, just because a role isn't explicitly marked as a superuser
doesn't mean that the role can't *become* a superuser, or that it hasn't
got privileged access to the system in other ways, such as by being a
member of other predefined roles that perhaps the role who is a member
of pg_manage_database_objects doesn't have. Such a check against
modifying of "superuser owned" objects implies that it's providing some
kind of protection against the role being able to become a superuser
when it doesn't actually provide that protection in any kind of reliable
fashion and instead ends up fooling the user.

This is the issue with CREATEROLE and we definitely shouldn't be
doubling-down on that mistake, and also brings up the point that I, at
least, had certainly hoped that part of this effort would include a way
for roles to be created by a user with an appropriate predefined role,
and w/o CREATEROLE (which would then be deprecated or, ideally, just
outright removed). I get that this doesn't have to be in the first
patch or even patch set going down this road but the lack of discussion
or of any coordination between this effort and the other one that is
trying to address the CREATEROLE issue seems likely to land us in a bad
place with two distinct approaches being used.

> I doubt that I will create any replacement for the pg_host_security role previously proposed, as I think that role is just synonymous with "superuser", so it serves no purpose.
>
> I am uncertain about creating a role similar to the pg_network_security role previously proposed, as the changes to how publications and subscriptions work in patches v5-0008 through v5-0010 may be enough. In any event, I'd like feedback on those patches before designing one or more additional roles for this.

"Able to create network connections" sure seems like a useful
capabilitiy to be able to delegate and which would cover postgres_fdw
and dblink use-cases also.

Thanks,

Stephen