Execute event trigger under superuser

[steve-chavez]

Problem

When supautils is enabled for superusers, this will cause them to skip event triggers. This is to prevent privilege escalation due to user defined event triggers (#98), which is desirable.

However this will also happen for superuser owned event triggers. So if we're logged in as superuser, and we create an event trigger it won't fire. Which is surprising behavior.

Solution

We already differentiate between user-owned event triggers and superuser-owned event triggers. An solution would be:

• superuser event triggers will execute for both superusers and regular users.
• user event triggers will only execute for non-superusers. Superusers will skip them. Ensuring no privesc can happen, same as before.

Implementation

Due to PostgreSQL C API limitations, it's not possible to know inside the fmgr_hook which event trigger fired the function. So we cannot get the "owner" of the event trigger by intercepting the function execution.

However, we can enforce that:

• Superuser event triggers must execute superuser-owned functions.
• User event triggers must execute non-superuser-owned functions.

This would be enough to implement the above solution in the fmgr_hook.

Blocker

While the above should work, the Supabase plataform currently has a bug which would be incompatible with the above design , see grant_pg_cron_access on supabase/postgres#1437 (essentially a superuser event trigger is executing a user function). Once that is fixed, this can be implemented safely.

Workaround

A workaround is to not enable supautils on superusers. Although in most cases supautils will be applied cluster-wide.

---

@soedirgo Any thoughts?

[soedirgo]

How about handling this via needs_fmgr_hook? So if the current role is a superuser we return false? won't work, we want the fmgr_hook to run for user-defined event triggers

[steve-chavez]

I don't think that would work.. we need to skip user-defined event triggers for superusers and reserved_roles. Returning false from needs_fmgr_hook would not apply that logic.

[soedirgo]

Yeah let's go with your approach 👍 I'd even go further and enforce event triggers to have the same owner as the event trigger function.

We'd also need to enforce it on ALTER { FUNCTION | EVENT TRIGGER } ... OWNER TO ... so users can't change the event trigger function owner after the fact.

[steve-chavez]

Ok cool. Since we have a blocker for that solution as mentioned above, how about if we just release without solving this issue for now? I think event triggers should still be useful in the current form.

[soedirgo]

release without solving this issue for now

That might be problematic? Some of our internal event triggers would get skipped (pg_graphql, pg_net, pg_cron). Actually I just realized the skipping event trigger execution on CREATE EXTENSION would interfere with that too.

Let's test this on a custom AMI first - we may be able to use an alternate mechanism for these internal event triggers (e.g. using extensions' after-create script)

[steve-chavez]

That might be problematic? Some of our internal event triggers would get skipped (pg_graphql, pg_net, pg_cron).

No, those should work fine. Right now the superuser event triggers execute normal for regular users. They're only skipped for superusers (which customers never use).

Let's test this on a custom AMI first - we may be able to use an alternate mechanism for these internal event triggers (e.g. using extensions' after-create script)

If we do want to solve this issue now, wouldn't it be better to fix supabase/postgres#1437 instead? It seems like a serious issue that needs to be resolved anyway.

Actually I just realized the skipping event trigger execution on CREATE EXTENSION would interfere with that too.

Could you elaborate on this? It looked OK to me to skip event triggers on CREATE EXTENSION, this from the perspective of pg_graphql and postgREST evtrigs which reload their schema cache and don't need to look at extension objects.

[soedirgo]

It looked OK to me to skip event triggers on CREATE EXTENSION, this from the perspective of pg_graphql and postgREST evtrigs which reload their schema cache and don't need to look at extension objects

Skipping pg_graphql event trigger would be a problem because that's where we replace the placeholder function and perform grants. Same with pg_net and pg_cron.

[steve-chavez]

Implemented the above on #115 as agreed but without:

I'd even go further and enforce event triggers to have the same owner as the event trigger function.

While it does make sense, I'm not sure if it's really needed.