Re: Non-superuser event trigger owners

Over in [1], you wrote:

> On Oct 20, 2021, at 11:27 AM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>
> On Wed, 2021-10-20 at 10:32 -0700, Mark Dilger wrote:
>> I'd like to have a much clearer understanding of Noah's complaint
>> first. There are multiple things to consider: (1) the role which
>> owns the trigger, (2) the role which is performing an action which
>> would cause the trigger to fire, (3) the set of roles role #1 belongs
>> to, (4) the set of roles role #1 has ADMIN privilege on, (5) the set
>> of roles that role #2 belongs to, and (6) the set of roles that role
>> #2 has ADMIN privilege on. Maybe more?
>>
>> And that's before we even get into having roles own other roles,
>> which the event trigger patches *do not depend on*. In the patch set
>> associated with this thread, the event trigger stuff is in patches
>> 0014 and 0015. The changes to CREATEROLE and role ownership are not
>> until patches 0019, 0020, and 0021. (I'm presently writing another
>> set of emails to split this all into four threads/patch sets.)
>>
>> I'd like to know precisely which combinations of these six things are
>> objectionable, and why. There may be a way around the objections
>> without needing to create new user options or new privileged roles.
>
> I can't speak for Noah, but my interpretation is that it would be
> surprising if GRANT/REVOKE or membership in an ordinary role had
> effects other than "permission denied" errors. It might make sense for
> event trigger firing in all the cases we can think of, but it would
> certainly be strange if we started accumulating a collection of
> behaviors that implicitly change when you move in or out of a role.
>
> That's pretty general, so to answer your question: it seems like a
> problem to use #3-6 in the calculation about whether to fire an event
> trigger.

Right. The patch as currently written requires that the trigger owner (role #1) be a member of role #2, as determined by is_member_of_role(item->fnowner, GetUserId()). The idea is that role #1 cannot force an action to be performed as role #2 that role #1 couldn't do independently through a SET ROLE followed by the same action.

I admit that the patch has an achilles heal, in that the patch does not run SetUserIdAndSecContext with SECURITY_LOCAL_USERID_CHANGE to avoid the trigger changing role to the SessionUserId, but that issue exists all over the system with table triggers and user defined functions (including on indexes), and those don't even have the protection of requiring the function owner to be a member of the role invoking the function. As such, nailing that down is probably the work for an entirely separate patch set.

As for whether it strikes users as strange that event triggers sometimes fire and sometimes do not, depending on which role is the CurrentUserId, I think it's more a question of whether the trigger owner finds that strange. Triggers are used for things like auditing, and it's not really on behalf of the person whose actions are being audited, but rather on behalf of the auditor. Setting up the owner of the trigger to be a powerful enough user to catch everyone you mean to catch is the responsibility of whoever sets up the auditing system.

> However, if we have a concept of role *ownership*, that's something
> new. It may be less surprising to use that to determine additional
> behaviors, like whether event triggers fire.

I hadn't really thought about it that way. The two things were not all that connected, except perhaps indirectly.

> We can also consider adding some additional language to the CREATE
> EVENT TRIGGER syntax to make it more explicit what the scope is. For
> instance:
>
> CREATE EVENT TRIGGER name
> ON event
> [ FOR {ALL|OWNED} ROLES ]
> [ WHEN filter_variable IN (filter_value [, ... ]) [ AND ... ] ]
> EXECUTE { FUNCTION | PROCEDURE } function_name()
>
> For a superuser ALL and OWNED would be the same, but regular users
> would need to specify "FOR OWNED ROLES" or they'd get an error.

I'll postpone taking any position on this, as role ownership is now a separate patch set and there is no connection between when/if that one gets committed and when/if this one does.

[1] https://www.postgresql.org/message-id/flat/F9408A5A-B20B-42D2-9E7F-49CD3D1547BC%40enterprisedb.com
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company