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

> On Jul 23, 2021, at 9:20 AM, Stephen Frost <sfrost@snowman.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
todo so, and it seems too coarse grained to either skip all of them or none of them.  It's perfectly reasonable to want
toskip 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-servicecase.  The superuser wants to be able to grant pg_database_security to the customer, but doesn't
wantthe 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_securityusable feature, then I think we need to solve the problems a naive approach would create for
serviceproviders. 

>> Instead of a GUC, how about checking permissions inside event triggers for both the user firing the trigger *and*
thetrigger owner.  That's a backward compatibility break, but maybe not a bad one, since until now only superusers have
beenallowed 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
thinkit is better than the GUC based solution.  Event triggers owned by a superuser continue to work as they do now.
Eventtriggers owned by a non-superuser cannot be used to force a privileged user to run a command that the event
triggerowner 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
privilegeduser into running a function that the less privileged user can't directly run.  Certainly such a user cannot
GRANTprivilege 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
SetUserIdAndSecContextto a new type of context similar to SECURITY_LOCAL_USERID_CHANGE but where instead of simply
changingto the owner of the event trigger, we'd be changing to a virtual user who is defined to only have the
privilegesof the intersection of the current user and the event trigger owner.  That entails at least two problems,
thoughI don't see that they are insoluble.  First, all places in the code that check permissions need to check in a way
thatworks in this mode.  We might not be able to call GetUserId() as part of aclchecks any longer, and instead have to
callsome new function GetVirtualUserId() as part of aclchecks, reserving GetUserId() just for cases where you're not
tryingto perform a permissions check.  Second, since event triggers can cause other event triggers to fire, we'd need
thesevirtual 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'dhave to think carefully about how to handle errors, since GetUserIdAndSecContext and SetUserIdAndSecContext are
calledinside transaction start and end, where errors must not be raised.  But since transaction start and end would
neverwant to set or reset the state to a virtual user, those should never throw, and the calls elsewhere are at liberty
tothrow 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
triggerswhile having a mode that behaves in this new way, which means that privileged users could be a bit more
cavalierthan they can now about DML against user defined tables.  This mode might become the standard operating mode
forservice provider scripts, whether running as superuser, as pg_network_security, or whatever.  This might also be
usedto secure operations on indexes over user defined functions.  If the index operation is run in this mode, the index
operationcould 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