Hello hackers,
I've now modified my patch to allow regular users (not just database
owners) to create event triggers.
As mentioned by Tom before, this now relies on role membership to allowlist
the target roles of the event trigger.
A summary of the changes:
- EventTriggerCacheItem now has an owneroid
- EventTriggerCommonSetup now returns a list of EventTriggerCacheItem
instead of a list of function oids
- The cache items are used in EventTriggerInvoke to discriminate the role
memberships using the `is_member_of_role_nosuper` function
- The event_trigger.sql tests are minimally modified.
- A new file event_trigger_nosuper.sql is added for the new tests.
Any feedback is welcomed. I'll also gladly modify the docs if the patch
looks good.
Best regards,
Steve Chavez
On Sat, 8 Mar 2025 at 00:03, Steve Chavez <st...@supabase.io> wrote:
> Hi Tom,
>
> > Well, no. That still allows the database owner to commandeer any
> > non-superuser role. Even if we tightened "nosuperuser" to mean
> > "not superuser and not any built-in role", I don't think it will fly.
>
> Why would the predefined roles be taken into consideration here? The docs
> on https://www.postgresql.org/docs/current/predefined-roles.html say:
>
> " pg_read_server_files, pg_write_server_files and
> pg_execute_server_program..."
> " ..they could be used to gain superuser-level access, therefore great
> care should be taken when granting these roles to users."
>
> If a dbowner event trigger does `GRANT pg_read_server_files TO
> current_user;` inside it will fail with `ERROR: permission denied to grant
> role "pg_read_server_files"`.
> The only way for that to succeed is for a superuser to explicitly grant
> access to `pg_read_server_files` before, and that would have to be a
> conscious operation.
>
> I would appreciate any clarification here.
>
> > maybe say that an event trigger fires for queries that are run by a role
> - that the trigger's owning role is a member of?
>
> The role membership approach would work but it seems insufficient. For
> example consider `pgaudit` which installs event triggers and requires
> superuser.
>
> Let's assume `pgaudit` would try to adopt this new feature. Then it would
> need to provide some special role like `pgaudit_admin`, create the event
> triggers under this role,
> and users of this extension would have to manually grant membership to
> `pgaudit_admin` for the audit event triggers to fire.
> That is a problem because that's easy to forget when creating new roles
> and the audit event triggers won't be "enforced".
> So in that case I guess `pgaudit` developers would keep requiring
> superuser and not bother to adopt this new feature.
>
> From a PoLP perspective it would be a desirable side-effect of this
> feature to stop requiring superuser for certain extensions too.
>
> Best regards,
> Steve Chavez
>
> On Fri, 7 Mar 2025 at 15:19, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
>> Steve Chavez <st...@supabase.io> writes:
>> > This is why I thought the database owner is the right role to allow
>> evtrig
>> > creation since it won't need an explicit list of roles.
>>
>> > How about requiring explicit non-superuser execution for the database
>> owner
>> > evtrig? It would be like:
>> > CREATE EVENT TRIGGER name ... FOR NOSUPERUSER;
>>
>> Well, no. That still allows the database owner to commandeer any
>> non-superuser role. Even if we tightened "nosuperuser" to mean
>> "not superuser and not any built-in role", I don't think it will fly.
>>
>> Here is the real problem: database owners are not specially
>> privileged in Postgres. Yeah, they can drop their DB, but they
>> don't have automatic permissions to mess with other people's
>> objects inside the DB. (Much the same can be said of schema
>> owners.) So any proposal that effectively gives DB owners
>> such privileges is going to fail. I realize that some other
>> DBMSes assign more privileges to schema or DB owners, but we
>> don't and I don't think we're open to changing that.
>>
>> I think you need to be thinking of this in terms of "what sort
>> of feature can we add that can be allowed to any SQL user?"
>> The notion I proposed earlier that an event trigger only fires
>> on queries executed by roles the trigger's owner belongs to
>> is (AFAICS) safe to allow to anyone. If that's not good enough
>> for your notion of what a DB owner should be able to do, the
>> answer is to grant the DB owner membership in every role that
>> uses her database. That's effectively what the feature you're
>> suggesting would do anyway.
>>
>> regards, tom lane
>>
>
From 82a23eb69fe128eb530aaedf5ba08402a2cab585 Mon Sep 17 00:00:00 2001
From: steve-chavez <stevechavez...@gmail.com>
Date: Sun, 20 Apr 2025 19:46:00 -0500
Subject: [PATCH] Allow regular users to create event triggers
---
src/backend/commands/event_trigger.c | 33 +++-------
src/backend/utils/cache/evtcache.c | 1 +
src/include/utils/evtcache.h | 1 +
src/test/regress/expected/event_trigger.out | 13 +---
src/test/regress/parallel_schedule | 4 ++
src/test/regress/sql/event_trigger.sql | 11 +---
.../regress/sql/event_trigger_nosuper.sql | 65 +++++++++++++++++++
7 files changed, 82 insertions(+), 46 deletions(-)
create mode 100644 src/test/regress/sql/event_trigger_nosuper.sql
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index edc2c988e29..9dc813adb6a 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -126,18 +126,6 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
ListCell *lc;
List *tags = NULL;
- /*
- * It would be nice to allow database owners or even regular users to do
- * this, but there are obvious privilege escalation risks which would have
- * to somehow be plugged first.
- */
- if (!superuser())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to create event trigger \"%s\"",
- stmt->trigname),
- errhint("Must be superuser to create an event trigger.")));
-
/* Validate event name. */
if (strcmp(stmt->eventname, "ddl_command_start") != 0 &&
strcmp(stmt->eventname, "ddl_command_end") != 0 &&
@@ -545,14 +533,6 @@ AlterEventTriggerOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_EVENT_TRIGGER,
NameStr(form->evtname));
- /* New owner must be a superuser */
- if (!superuser_arg(newOwnerId))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to change owner of event trigger \"%s\"",
- NameStr(form->evtname)),
- errhint("The owner of an event trigger must be a superuser.")));
-
form->evtowner = newOwnerId;
CatalogTupleUpdate(rel, &tup->t_self, tup);
@@ -698,7 +678,7 @@ EventTriggerCommonSetup(Node *parsetree,
if (unfiltered || filter_event_trigger(tag, item))
{
/* We must plan to fire this trigger. */
- runlist = lappend_oid(runlist, item->fnoid);
+ runlist = lappend(runlist, item);
}
}
@@ -1084,11 +1064,16 @@ EventTriggerInvoke(List *fn_oid_list, EventTriggerData *trigdata)
foreach(lc, fn_oid_list)
{
LOCAL_FCINFO(fcinfo, 0);
- Oid fnoid = lfirst_oid(lc);
+ EventTriggerCacheItem *item = (EventTriggerCacheItem*) lfirst_oid(lc);
FmgrInfo flinfo;
PgStat_FunctionCallUsage fcusage;
+ Oid current_user = GetUserId();
+
+ if (!is_member_of_role_nosuper(current_user, item->owneroid)) {
+ continue;
+ }
- elog(DEBUG1, "EventTriggerInvoke %u", fnoid);
+ elog(DEBUG1, "EventTriggerInvoke %u", item->fnoid);
/*
* We want each event trigger to be able to see the results of the
@@ -1102,7 +1087,7 @@ EventTriggerInvoke(List *fn_oid_list, EventTriggerData *trigdata)
CommandCounterIncrement();
/* Look up the function */
- fmgr_info(fnoid, &flinfo);
+ fmgr_info(item->fnoid, &flinfo);
/* Call the function, passing no arguments but setting a context. */
InitFunctionCallInfoData(*fcinfo, &flinfo, 0,
diff --git a/src/backend/utils/cache/evtcache.c b/src/backend/utils/cache/evtcache.c
index ce596bf5638..1dc9a864034 100644
--- a/src/backend/utils/cache/evtcache.c
+++ b/src/backend/utils/cache/evtcache.c
@@ -175,6 +175,7 @@ BuildEventTriggerCache(void)
item = palloc0(sizeof(EventTriggerCacheItem));
item->fnoid = form->evtfoid;
item->enabled = form->evtenabled;
+ item->owneroid = form->evtowner;
/* Decode and sort tags array. */
evttags = heap_getattr(tup, Anum_pg_event_trigger_evttags,
diff --git a/src/include/utils/evtcache.h b/src/include/utils/evtcache.h
index 9d9fcb8657b..0ecd6a54c5f 100644
--- a/src/include/utils/evtcache.h
+++ b/src/include/utils/evtcache.h
@@ -31,6 +31,7 @@ typedef struct
Oid fnoid; /* function to be called */
char enabled; /* as SESSION_REPLICATION_ROLE_* */
Bitmapset *tagset; /* command tags, or NULL if empty */
+ Oid owneroid; /* owner of the event trigger */
} EventTriggerCacheItem;
extern List *EventCacheLookup(EventTriggerEvent event);
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 7b2198eac6f..0ca16ec5e38 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -84,14 +84,6 @@ create event trigger regress_event_trigger2 on ddl_command_start
execute procedure test_event_trigger();
-- OK
comment on event trigger regress_event_trigger is 'test comment';
--- drop as non-superuser should fail
-create role regress_evt_user;
-set role regress_evt_user;
-create event trigger regress_event_trigger_noperms on ddl_command_start
- execute procedure test_event_trigger();
-ERROR: permission denied to create event trigger "regress_event_trigger_noperms"
-HINT: Must be superuser to create an event trigger.
-reset role;
-- test enabling and disabling
alter event trigger regress_event_trigger disable;
-- fires _trigger2 and _trigger_end should fire, but not _trigger
@@ -168,15 +160,12 @@ create foreign data wrapper useless;
NOTICE: test_event_trigger: ddl_command_end CREATE FOREIGN DATA WRAPPER
create server useless_server foreign data wrapper useless;
NOTICE: test_event_trigger: ddl_command_end CREATE SERVER
+create role regress_evt_user;
create user mapping for regress_evt_user server useless_server;
NOTICE: test_event_trigger: ddl_command_end CREATE USER MAPPING
alter default privileges for role regress_evt_user
revoke delete on tables from regress_evt_user;
NOTICE: test_event_trigger: ddl_command_end ALTER DEFAULT PRIVILEGES
--- alter owner to non-superuser should fail
-alter event trigger regress_event_trigger owner to regress_evt_user;
-ERROR: permission denied to change owner of event trigger "regress_event_trigger"
-HINT: The owner of an event trigger must be a superuser.
-- alter owner to superuser should work
alter role regress_evt_user superuser;
alter event trigger regress_event_trigger owner to regress_evt_user;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index a424be2a6bf..fd3ef719f5e 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -130,6 +130,10 @@ test: partition_join partition_prune reloptions hash_part indexing partition_agg
# oidjoins is read-only, though, and should run late for best coverage
test: oidjoins event_trigger
+# event_trigger_nosuper cannot run concurrently
+# with other tests that runs DDL
+test: event_trigger_nosuper
+
# event_trigger_login cannot run concurrently with any other tests because
# on-login event handling could catch connection of a concurrent test.
test: event_trigger_login
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index 013546b8305..6833e991762 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -85,13 +85,6 @@ create event trigger regress_event_trigger2 on ddl_command_start
-- OK
comment on event trigger regress_event_trigger is 'test comment';
--- drop as non-superuser should fail
-create role regress_evt_user;
-set role regress_evt_user;
-create event trigger regress_event_trigger_noperms on ddl_command_start
- execute procedure test_event_trigger();
-reset role;
-
-- test enabling and disabling
alter event trigger regress_event_trigger disable;
-- fires _trigger2 and _trigger_end should fire, but not _trigger
@@ -139,13 +132,11 @@ revoke all on table event_trigger_fire1 from public;
drop table event_trigger_fire1;
create foreign data wrapper useless;
create server useless_server foreign data wrapper useless;
+create role regress_evt_user;
create user mapping for regress_evt_user server useless_server;
alter default privileges for role regress_evt_user
revoke delete on tables from regress_evt_user;
--- alter owner to non-superuser should fail
-alter event trigger regress_event_trigger owner to regress_evt_user;
-
-- alter owner to superuser should work
alter role regress_evt_user superuser;
alter event trigger regress_event_trigger owner to regress_evt_user;
diff --git a/src/test/regress/sql/event_trigger_nosuper.sql b/src/test/regress/sql/event_trigger_nosuper.sql
new file mode 100644
index 00000000000..4023575bf44
--- /dev/null
+++ b/src/test/regress/sql/event_trigger_nosuper.sql
@@ -0,0 +1,65 @@
+-- setup roles and privileges
+create role evtrig_owner;
+create role member_1;
+create role member_2;
+grant evtrig_owner to member_1;
+grant evtrig_owner to member_2;
+create role non_member;
+grant all on schema public to evtrig_owner, member_1, member_2, non_member;
+\echo
+
+-- create nonsuper event trigger
+set role evtrig_owner;
+create function show_current_user()
+ returns event_trigger
+ language plpgsql as
+$$
+begin
+ raise notice 'the event trigger is executed for %', current_user;
+end;
+$$;
+create event trigger evtrig_show_current_user_1
+ on ddl_command_start
+ execute procedure show_current_user();
+reset role;
+\echo
+
+-- create super event trigger and alter it to be nonsuper
+create event trigger evtrig_show_current_user_2
+ on ddl_command_end
+ execute procedure show_current_user();
+alter event trigger evtrig_show_current_user_2 owner to evtrig_owner;
+\echo
+
+-- evtrig should not fire for superusers
+select current_setting('is_superuser');
+create table evtrig_quux();
+\echo
+
+-- evtrig should fire for members
+set role member_1;
+create table evtrig_foo();
+\echo
+
+set role member_2;
+create table evtrig_bar();
+\echo
+
+-- evtrig should not fire for non-members
+set role non_member;
+create table evtrig_qux();
+\echo
+
+-- cleanup
+reset role;
+drop table evtrig_qux;
+drop table evtrig_bar;
+drop table evtrig_foo;
+revoke all on schema public from member_1, member_2, non_member, evtrig_owner;
+drop event trigger evtrig_show_current_user_1;
+drop event trigger evtrig_show_current_user_2;
+drop function show_current_user();
+drop role member_1;
+drop role member_2;
+drop role non_member;
+drop role evtrig_owner;
--
2.40.1
