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

> On Jul 23, 2021, at 9:20 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
> These considerations were addressed with row_security by allowing the
> GUC to be set by anyone, but throwing an ERROR if RLS would have been
> required by the query instead of just allowing it. I don't see any
> obvious reason why that couldn't be the case for event triggers..?

Because a postgres-as-a-service provider may want to install their own event triggers as well as allowing the customer to do so, and it seems too coarse grained to either skip all of them or none of them. It's perfectly reasonable to want to skip your customer's event triggers while not skipping your own.

>> A superuser-only GUC for this is also a bit too heavy handed. The superuser may not want to circumvent all event triggers, just those put in place by the pg_database_security role. If that sounds arbitrary, just consider the postgres-as-a-service case. The superuser wants to be able to grant pg_database_security to the customer, but doesn't want the customer to be able to use that to trap the service provider.
>
> Having a trust system for triggers, functions, etc, where you can say
> whose triggers you're ok running might be interesting but it also seems
> like an awful lot of work and I'm not sure that it's actually really
> that much better than a GUC similar to row_security.

My first impression was that it is too much work, which is why I put event trigger creation into the pg_host_security bucket. It might be more sane to just leave it as superuser-only. But if we're going to fix this and make it a pg_database_security usable feature, then I think we need to solve the problems a naive approach would create for service providers.

>> Instead of a GUC, how about checking permissions inside event triggers for both the user firing the trigger *and* the trigger owner. That's a backward compatibility break, but maybe not a bad one, since until now only superusers have been allowed to create event triggers. Systems which create an event trigger using a role that later has superuser revoked, or which change ownership to a non-superuser, will see a behavior change. I'm not super happy with that, but I think it is better than the GUC based solution. Event triggers owned by a superuser continue to work as they do now. Event triggers owned by a non-superuser cannot be used to force a privileged user to run a command that the event trigger owner could not have run for themself.
>
> I'm not following what this suggestion is, exactly. What permissions
> are being checked inside the event trigger being run, exactly..? Who
> would get to set those permissions? Any object owner, today, can GRANT
> access to any other user in the system, we don't prevent that.

I don't think GRANT is really relevant here, as what I'm trying to avoid is a less privileged user trapping a more privileged user into running a function that the less privileged user can't directly run. Certainly such a user cannot GRANT privilege on a function that they cannot even run, else your system has a privilege escalation hazard already.

It's a substantial change to the security model, but the idea is that inside an event trigger, we'd SetUserIdAndSecContext to a new type of context similar to SECURITY_LOCAL_USERID_CHANGE but where instead of simply changing to the owner of the event trigger, we'd be changing to a virtual user who is defined to only have the privileges of the intersection of the current user and the event trigger owner. That entails at least two problems, though I don't see that they are insoluble. First, all places in the code that check permissions need to check in a way that works in this mode. We might not be able to call GetUserId() as part of aclchecks any longer, and instead have to call some new function GetVirtualUserId() as part of aclchecks, reserving GetUserId() just for cases where you're not trying to perform a permissions check. Second, since event triggers can cause other event triggers to fire, we'd need these virtual users to be able to nest, so we'd have to push a stack of users and check all of them in each case, and we'd have to think carefully about how to handle errors, since GetUserIdAndSecContext and SetUserIdAndSecContext are called inside transaction start and end, where errors must not be raised. But since transaction start and end would never want to set or reset the state to a virtual user, those should never throw, and the calls elsewhere are at liberty to throw if they like, so we'd just have to be careful to use the right version of these operations in the right places.

This all sounds a bit much, but it has knock-on benefits. We'd be able to preserve the historical behavior of table triggers while having a mode that behaves in this new way, which means that privileged users could be a bit more cavalier than they can now about DML against user defined tables. This mode might become the standard operating mode for service provider scripts, whether running as superuser, as pg_network_security, or whatever. This might also be used to secure operations on indexes over user defined functions. If the index operation is run in this mode, the index operation could throw an error rather than performing a function maliciously embedded inside the index function call.

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