Non-superuser event trigger owners

Over in [1]/messages/by-id/F9408A5A-B20B-42D2-9E7F-49CD3D1547BC@enterprisedb.com — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company, you wrote:

On Oct 20, 2021, at 11:27 AM, Jeff Davis <pgsql@j-davis.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]: /messages/by-id/F9408A5A-B20B-42D2-9E7F-49CD3D1547BC@enterprisedb.com — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company