Support event trigger for Neon users

[knizhnik]

Problem

#7570

Even triggers are supported only for superusers.

Summary of changes

Temporary switch to superuser when even trigger is created and disable execution of user's even triggers under superuser.

[github-actions]

7425 tests run: 7072 passed, 0 failed, 353 skipped (full report)

---

Flaky tests (2)

Postgres 17

• test_scrubber_physical_gc_ancestors[2]: release-arm64-without-lfc

Postgres 16

• test_lr_with_slow_safekeeper: release-x86-64-with-lfc

Code coverage* (full report)

• functions: 33.3% (8589 of 25823 functions)
• lines: 49.1% (72305 of 147256 lines)

* collected from Rust tests only

---

_{The comment gets automatically updated with the latest test results
720fe00 at 2025-02-06T20:53:26.459Z :recycle:}

[kelvich]

few quick thoughts:

• Placing this code in control_plane_connector.c seems a bit off. It follows established pattern though, so may be we need to rename file?
• Some comments would be nice. Even supa source had them
• We follow Supa approach here, let's mention that as well and put a link to their PR

[knizhnik]

1. control_plane_connector is renamed to neon_ddl_handler
2. GUC is renamed to enable_event_triggers_for_superuser with default value off
3. Comments added
4. Reference to Supa PR added
5. Now even if event triggers are not enabled for superuser, they still can be fired if trigger procedure is owned by superuser. It allows to pass regression tests without explicit setting of this GUC and seems to be correct in general case: it should be safe to call procedure created by superuser with admin permissions.

[kelvich]

I have a hard time doing mental negation for variables that has inverted meaning. Wouldn't it be easier with allow_event_triggers_for_superuser?

[knizhnik]

Done

[hlinnaka]

Hoo boy, this is going to be tough to get right. That's why when event triggers were introduced in the first place, they were made superuser only..

1. One classic problem with login triggers is that it's easy to accidentally lock yourself out:

neondb=> create function deny()
    returns event_trigger
    language plpgsql as
$$
begin raise  'NOPE!'; end
$$;
CREATE FUNCTION
neondb=> create event trigger denylogin on login execute function deny();
CREATE EVENT TRIGGER

Postgres has an escape hatch for that: you can temporarily set event_triggers=off in the config file, or as a connection option. However, we don't let customers edit the config file directly, and because event_triggers is PGC_SUSET, neon_superuser role cannot set that in the connection string either. So that escape hatch doesn't work for non-superusers.

2. The rules for when to fire the trigger and when not seem a bit fragile and difficult to grasp. For example, the triggers are not normally fired for a superuser. But they are fired for a superuser if the function is defined as SECURITY DEFINER:

eondb=> create function trigfunc_secdef()
    returns event_trigger
    language plpgsql as
$$
begin raise notice 'TRIGGER FIRED as %', current_user; end
$$ security definer;
CREATE FUNCTION
neondb=> create event trigger logintrig on login execute function trigfunc_secdef();
CREATE EVENT TRIGGER

This trigger will fire when you log in as superuser. It runs as the user who created the function; however, it can still lay some nasty surprises for the session, like change search_path, or create temporary tables that shadow other tables.

3. If I'm reading neon_fmgr_hook() correctly, it doesn't check what kind of a function is being executed. That's only checked in neon_needs_fmgr_hook(). If another extension was installed that also set an fmgr_hook, it might cause our hook to also be called for all functions, not just event triggers. And our hook would then replace other function calls with noops too. I don't think we install any other such extensions at the moment, but it's an accident waiting to happen.

4. The switched_to_superuser flag is not cleared correctly on error:

neondb=> create event trigger denylogin on login execute function bogus();     // func doesn't exit
ERROR:  function bogus() does not exist
neondb=> create event trigger denylogin on login execute function bogus();     // try the same again
ERROR:  permission denied to create event trigger "denylogin"
HINT:  Must be superuser to create an event trigger.

[knizhnik]

1. One classic problem with login triggers is that it's easy to accidentally lock yourself out:

We should definitely allow user to set event_triggers=off - not a big deal IMHO.

[kelvich]

Nice, thank you @hlinnaka for a prompt review.

I've checked how RDS behaves in cases you've described:

One classic problem with login triggers is that it's easy to accidentally lock yourself out:

Looks like no special protection there:

postgres=> create function deny()
    returns event_trigger
    language plpgsql as
$$
begin raise  'NOPE!'; end
$$;
CREATE FUNCTION
postgres=> create event trigger denylogin on login execute function deny();
CREATE EVENT TRIGGER

-> psql 'postgres://postgres:<...>@stas-triggers-test-3.<...>.eu-central-1.rds.amazonaws.com/postgres'
psql: error: connection to server at "stas-triggers-test-3.<...>.eu-central-1.rds.amazonaws.com" (<...>), port 5432 failed: FATAL:  NOPE!
CONTEXT:  PL/pgSQL function deny() line 2 at RAISE

I've tested that setting event_triggers=off works as expected.

The rules for when to fire the trigger and when not seem a bit fragile and difficult to grasp. For example, the triggers are not normally fired for a superuser. But they are fired for a superuser if the function is defined as SECURITY DEFINER

That one is more interesting. I've changed this trigger a bit and watched logs

CREATE OR REPLACE FUNCTION print_login_user()
returns event_trigger
LANGUAGE plpgsql
SECURITY DEFINER
AS $$
DECLARE
    login_user TEXT;
    current_exec_user TEXT;
BEGIN
    login_user := session_user;
    current_exec_user := current_user;
    RAISE 'Login User: %, Current User: %', login_user, current_exec_user;
END;
$$;

some observations:

• i do see my own login attempts in logs: 2025-02-04 11:41:37 UTC:81.4.182.210(55217):stas@postgres:[1115]:FATAL: Login User: stas, Current User: postgres
• i do see rdsadmin session in pg_stat_activity, but no traces of login events firing for that user in logs. Even if I restart the database
• i also did database S3 export while having this trigger, no issues, no traces of trigger running in logs

So looks like RDS has changed secdef trigger behavior.

If I'm reading neon_fmgr_hook() correctly, it doesn't check what kind of a function is being executed. That's only checked in neon_needs_fmgr_hook().

The switched_to_superuser flag is not cleared correctly on error:

Looks like we should fix that? Let's fix

[knizhnik]

The rules for when to fire the trigger and when not seem a bit fragile and difficult to grasp

I have committed version which prohibit execution of any trigger procedure for superuser including ones created as security definers. As the results user will not be able to declare trigger procedures as security definers. Nit sure if it is significant limitation.

It will be nice if it is possible to somehow determine the original user (before changed by security definer function invocation). Do not know how to do it, do you?

Alternatively we can allow execution of user trigger procedures defined as security definers.
Yes, in this case user's can try to somehow fool superuser: "change search_path, or create temporary tables that shadow other tables". But we can consider this risk...

In any case, I will be glad to hear your proposal.

3. If I'm reading neon_fmgr_hook() correctly, it doesn't check what kind of a function is being executed.

I have added check for trigger return type. But I am not sure that fn_oid is always defined in FmgrInfo.
Alternatively I can add assert that no other mgr hooks are registered.
Or can you propose better solution?

4. The switched_to_superuser flag is not cleared correctly on error:

Fixed

[hlinnaka]

It will be nice if it is possible to somehow determine the original user (before changed by security definer function invocation). Do not know how to do it, do you?

There are a bunch of functions in miscinit.c: GetSystemUser(), GetAuthenticatedUserId(), GetSessionUserId(), GetOuterUserId(), GetUserId(). I don't remember which is which, but perhaps one of them is what you want :-).

[hlinnaka]

The rules for when to fire the trigger and when not seem a bit fragile and difficult to grasp

I have committed version which prohibit execution of any trigger procedure for superuser including ones created as security definers. As the results user will not be able to declare trigger procedures as security definers.

To be precise, they will now be able to create such triggers, but they will not fire.

In any case, I will be glad to hear your proposal.

I don't really have a proposal. I can poke holes at what's proposed, but I don't have a model in mind on how it should work. Whatever the proposed model is, I'd love to see a short RFC or other doc that explains how it's supposed to work, what the threat model is, what things you can and cannot do with this, compared to having real superuser rights.

Perhaps start by writing user documentation for the proposed behaviour, and work from there?

[knizhnik]

There are a bunch of functions in miscinit.c: GetSystemUser(), GetAuthenticatedUserId(), GetSessionUserId(), GetOuterUserId(), GetUserId(). I don't remember which is which, but perhaps one of them is what you want :-).

Thank you!
I have committed version which uses GetSessionUserIsSuperuser() to disable execution of all event trigger functions (including security definers) for superusers.

[cermakondrej]

Supabase also opted in for temporary switch to superuser ( supabase/supautils#98) while skipping trigger execution. Should be safe imo

[kelvich]

I'd love to see a short RFC or other doc that explains how it's supposed to work, what the threat model is, what things you can and cannot do with this, compared to having real superuser rights.

+1, let's do that. Would be nice to discuss too:

• that part: FIXME: now neon_superuser is also ab;e to drop event triggers created by superuser
• can we / should we make a patch to postgres to make triggers management ability exposed as a pre-defined role like https://www.postgresql.org/docs/devel/predefined-roles.html