Allow database owners to CREATE EVENT TRIGGER
Hello hackers,
Currently PostgreSQL only allows creating event triggers for superusers,
this prevents usage on PostgreSQL service providers, which do not grant
superuser access.
This patch allows database owners to create event triggers, while
preventing privilege escalation.
Unlike superuser event triggers, which execute functions for every role,
database owner event triggers are only executed for non-superusers.
This is necessary to prevent privesc. i.e. a superuser tripping on an event
trigger containing an `ALTER ROLE dbowner SUPERUSER`.
For skipping dbowner event triggers for superusers:
- A restriction is added for superuser event triggers, the event trigger
function must be owned by a superuser.
+ While this is a breaking change, I think it's minor as the usual flow
is to "login as superuser" -> "create an evtrig function" -> "create the
evtrig". This is also proved by the existing tests, which barely change.
- A restriction is added for dbowner event triggers, the event trigger
function must not be owned by a superuser.This way we can filter dbowner event trigger functions inside
`EventTriggerInvoke`.
Tests are included in the patch, I've added a dedicated regression file for
easier review. Only a couple of error messages of the existing event
trigger regression tests are changed.
Any feedback is welcomed. I haven't added docs yet but I'll gladly add them
if the community thinks this patch makes sense.
(Previous thread that also discussed allowing event triggers for
non-superusers:
/messages/by-id/81C10FFB-5ADC-4956-9337-FA248A4CC20D@enterprisedb.com
)
Best regards,
Steve Chavez
Attachments:
0001-Allow-database-owners-to-CREATE-EVENT-TRIGGER.patchtext/x-patch; charset=US-ASCII; name=0001-Allow-database-owners-to-CREATE-EVENT-TRIGGER.patchDownload
From 84965d3ad70c4794f93473b939f5437c0d154826 Mon Sep 17 00:00:00 2001
From: steve-chavez <stevechavezast@gmail.com>
Date: Wed, 26 Feb 2025 20:17:36 -0500
Subject: [PATCH] Allow database owners to CREATE EVENT TRIGGER
---
src/backend/commands/dbcommands.c | 22 ++++
src/backend/commands/event_trigger.c | 38 ++++--
src/backend/utils/cache/lsyscache.c | 23 ++++
src/include/commands/dbcommands.h | 1 +
src/include/utils/lsyscache.h | 1 +
src/test/regress/expected/event_trigger.out | 2 +-
src/test/regress/parallel_schedule | 2 +-
.../regress/sql/event_trigger_dbowner.sql | 117 ++++++++++++++++++
8 files changed, 197 insertions(+), 9 deletions(-)
create mode 100644 src/test/regress/sql/event_trigger_dbowner.sql
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 1753b289074..4cd31dc1d90 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -3201,6 +3201,28 @@ get_database_name(Oid dbid)
return result;
}
+/*
+ * get_database_owner - given a database OID, look up the owner OID
+ *
+ * If the owner is not found, it will return InvalidOid
+ */
+Oid
+get_database_owner(Oid dbid)
+{
+ HeapTuple dbtuple;
+ Oid dbowner;
+
+ dbtuple = SearchSysCache1(DATABASEOID, ObjectIdGetDatum(dbid));
+ if (HeapTupleIsValid(dbtuple))
+ {
+ dbowner = ((Form_pg_database) GETSTRUCT(dbtuple))->datdba;
+ ReleaseSysCache(dbtuple);
+ }
+ else
+ dbowner = InvalidOid;
+
+ return dbowner;
+}
/*
* While dropping a database the pg_database row is marked invalid, but the
diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c
index edc2c988e29..62e7c7326f4 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -34,6 +34,7 @@
#include "catalog/pg_trigger.h"
#include "catalog/pg_ts_config.h"
#include "catalog/pg_type.h"
+#include "commands/dbcommands.h"
#include "commands/event_trigger.h"
#include "commands/extension.h"
#include "commands/trigger.h"
@@ -125,18 +126,17 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
Oid evtowner = GetUserId();
ListCell *lc;
List *tags = NULL;
+ bool is_database_owner = (evtowner == get_database_owner(MyDatabaseId));
+ bool owner_is_super = superuser_arg(evtowner);
+ bool function_is_owned_by_super;
- /*
- * 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())
+ if (!owner_is_super && !is_database_owner){
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.")));
+ errhint("Must be the database owner or superuser to create an event trigger.")));
+ }
/* Validate event name. */
if (strcmp(stmt->eventname, "ddl_command_start") != 0 &&
@@ -200,6 +200,24 @@ CreateEventTrigger(CreateEventTrigStmt *stmt)
errmsg("function %s must return type %s",
NameListToString(stmt->funcname), "event_trigger")));
+ function_is_owned_by_super = superuser_arg(get_func_owner(funcoid));
+
+ if(owner_is_super && !function_is_owned_by_super){
+ ereport(ERROR, (
+ errcode(ERRCODE_INVALID_OBJECT_DEFINITION)
+ , errmsg("A superuser event trigger must execute a superuser owned function")
+ , errdetail("The current user \"%s\" is a superuser and the function \"%s\" is owned by a non-superuser", GetUserNameFromId(evtowner, false), NameListToString(stmt->funcname))
+ ));
+ }
+
+ if(!owner_is_super && function_is_owned_by_super){
+ ereport(ERROR, (
+ errcode(ERRCODE_INVALID_OBJECT_DEFINITION)
+ , errmsg("A database owner event trigger must not execute a superuser owned function")
+ , errdetail("The function \"%s\" is owned by a superuser", NameListToString(stmt->funcname))
+ ));
+ }
+
/* Insert catalog entries. */
return insert_event_trigger_tuple(stmt->trigname, stmt->eventname,
evtowner, funcoid, tags);
@@ -1087,9 +1105,15 @@ EventTriggerInvoke(List *fn_oid_list, EventTriggerData *trigdata)
Oid fnoid = lfirst_oid(lc);
FmgrInfo flinfo;
PgStat_FunctionCallUsage fcusage;
+ bool function_is_owned_by_super = superuser_arg(get_func_owner(fnoid));
elog(DEBUG1, "EventTriggerInvoke %u", fnoid);
+ if (superuser() && !function_is_owned_by_super) {
+ elog(DEBUG1, "Non-superuser owned event trigger function \"%s\" is skipped for superuser \"%s\"", get_func_name(fnoid), GetUserNameFromId(GetUserId(), false));
+ continue;
+ }
+
/*
* We want each event trigger to be able to see the results of the
* previous event trigger's action. Caller is responsible for any
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 7bd476f3de7..3fc0eec9783 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -1650,6 +1650,29 @@ get_func_name(Oid funcid)
return NULL;
}
+/*
+ * get_func_owner
+ * returns the name of the function owner the given funcid
+ */
+Oid
+get_func_owner(Oid funcid)
+{
+ HeapTuple tp;
+
+ tp = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
+ if (HeapTupleIsValid(tp))
+ {
+ Form_pg_proc functup = (Form_pg_proc) GETSTRUCT(tp);
+ Oid result;
+
+ result = functup->proowner;
+ ReleaseSysCache(tp);
+ return result;
+ }
+ else
+ return InvalidOid;
+}
+
/*
* get_func_namespace
*
diff --git a/src/include/commands/dbcommands.h b/src/include/commands/dbcommands.h
index 524ac6d97e8..4d572bb53ed 100644
--- a/src/include/commands/dbcommands.h
+++ b/src/include/commands/dbcommands.h
@@ -30,6 +30,7 @@ extern ObjectAddress AlterDatabaseOwner(const char *dbname, Oid newOwnerId);
extern Oid get_database_oid(const char *dbname, bool missing_ok);
extern char *get_database_name(Oid dbid);
+extern Oid get_database_owner(Oid dbid);
extern bool have_createdb_privilege(void);
extern void check_encoding_locale_matches(int encoding, const char *collate, const char *ctype);
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 6fab7aa6009..b9d560a6ef9 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -122,6 +122,7 @@ extern Oid get_negator(Oid opno);
extern RegProcedure get_oprrest(Oid opno);
extern RegProcedure get_oprjoin(Oid opno);
extern char *get_func_name(Oid funcid);
+extern Oid get_func_owner(Oid funcid);
extern Oid get_func_namespace(Oid funcid);
extern Oid get_func_rettype(Oid funcid);
extern int get_func_nargs(Oid funcid);
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index 7b2198eac6f..f951031b4c8 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -90,7 +90,7 @@ 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.
+HINT: Must be the database owner or superuser to create an event trigger.
reset role;
-- test enabling and disabling
alter event trigger regress_event_trigger disable;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 37b6d21e1f9..6b322c76b00 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -124,7 +124,7 @@ test: partition_join partition_prune reloptions hash_part indexing partition_agg
# event_trigger depends on create_am and cannot run concurrently with
# any test that runs DDL
# oidjoins is read-only, though, and should run late for best coverage
-test: oidjoins event_trigger
+test: oidjoins event_trigger event_trigger_dbowner
# event_trigger_login cannot run concurrently with any other tests because
# on-login event handling could catch connection of a concurrent test.
diff --git a/src/test/regress/sql/event_trigger_dbowner.sql b/src/test/regress/sql/event_trigger_dbowner.sql
new file mode 100644
index 00000000000..00b62045e47
--- /dev/null
+++ b/src/test/regress/sql/event_trigger_dbowner.sql
@@ -0,0 +1,117 @@
+-- set up
+create role regress_non_owner;
+create role regress_owner login nosuperuser;
+create database regress_owner owner regress_owner;
+\connect regress_owner
+grant create on schema public to regress_non_owner;
+create function regress_super_show_cur_user()
+ returns event_trigger
+ language plpgsql as
+$$
+begin
+ raise notice 'the superuser event trigger is executed';
+end;
+$$;
+\echo
+
+-- a database owner can create event triggers
+set role regress_owner;
+\echo
+
+create function regress_show_cur_user()
+ returns event_trigger
+ language plpgsql as
+$$
+begin
+ raise notice 'the event trigger is executed for %', current_user;
+end;
+$$;
+\echo
+
+create event trigger regress_evtrig_1 on ddl_command_end
+execute procedure regress_show_cur_user();
+\echo
+
+reset role;
+\echo
+
+-- a database owner cannot create event triggers when the function is superuser owned
+set role regress_owner;
+\echo
+
+create event trigger regress_evtrig_2 on ddl_command_end
+execute procedure regress_super_show_cur_user();
+\echo
+
+reset role;
+\echo
+
+-- a superuser cannot create event triggers when the function is not superuser owned
+create event trigger regress_evtrig_3 on ddl_command_end
+execute procedure regress_show_cur_user();
+\echo
+
+-- a non-database owner cannot create event triggers
+set role regress_non_owner;
+\echo
+
+create event trigger regress_evtrig_4 on ddl_command_end
+execute procedure regress_show_cur_user();
+\echo
+
+reset role;
+\echo
+
+-- database owner event trigger will fire for database owners
+set role regress_owner;
+create table regress_foo();
+reset role;
+\echo
+
+-- database owner event trigger will fire for non-database owners
+set role regress_non_owner;
+create table regress_bar();
+reset role;
+\echo
+
+-- database owner event trigger will not fire for superusers
+create table regress_qux();
+\echo
+
+-- superuser event triggers will fire for all roles
+create event trigger regress_evtrig_5 on ddl_command_end
+execute procedure regress_super_show_cur_user();
+\echo
+
+-- will fire for superusers
+create table regress_super_foo();
+\echo
+
+-- will fire for db owners
+set role regress_owner;
+create table regress_foo_2();
+reset role;
+\echo
+
+-- will fire for non-db-owners
+set role regress_owner;
+create table regress_bar_2();
+reset role;
+\echo
+
+-- clean up
+drop event trigger regress_evtrig_5;
+drop table regress_foo;
+drop table regress_foo_2;
+drop table regress_bar;
+drop table regress_bar_2;
+drop table regress_qux;
+drop table regress_super_foo;
+revoke create on schema public from regress_non_owner;
+drop role regress_non_owner;
+drop event trigger regress_evtrig_1;
+drop function regress_show_cur_user();
+drop function regress_super_show_cur_user();
+\connect regression
+drop database regress_owner;
+drop role regress_owner;
--
2.40.1
Hi,
Unlike superuser event triggers, which execute functions for every role, database owner event triggers are only executed for non-superusers.
Even if we forget about the security aspect for a moment, personally I
have mixed feelings about the idea of adding a new type of event
trigger that looks like a regular one but works differently depending
on who creates them. Also what will happen if I promote a user to a
superuser or vice versa? All this doesn't strike me as a great UI.
Maybe you could explain your particular use case?
--
Best regards,
Aleksander Alekseev
On Wednesday, March 5, 2025, Aleksander Alekseev <aleksander@timescale.com>
wrote:
Unlike superuser event triggers, which execute functions for every role,
database owner event triggers are only executed for non-superusers.
All this doesn't strike me as a great UI.
Yeah. Seems better to make “execute_for” an attribute of the trigger and
allow both superusers and non-superusers to create them. Then enforce that
non-superusers must specify the more limited value.
Though it would seem nice to be able to exclude the pseudo-admin roles
these service providers create as well.
David J.
Many thanks for the feedback.
an attribute of the trigger and allow both superusers and non-superusers
to create them.
The above is the crux of the issue. Superuser evtrigs can target every role
but non-superusers evtrigs must apply only to a restricted set of roles to
avoid privilege escalation.
With an explicit attribute, I guess the SQL syntax should be like:
Seems better to make “execute_for” an attribute of the trigger
CREATE EVENT TRIGGER name ... FOR role1, role2;
Now say a new role is created and has usage/create on this database and we
want the evtrig to apply to it. We would need to manually update the list
of roles, it won't be done automatically. That is a problem if, for
example, we need to enforce an audit trail through event triggers.
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;
I welcome any alternative ideas.
Best regards,
Steve Chavez
On Wed, 5 Mar 2025 at 08:54, David G. Johnston <david.g.johnston@gmail.com>
wrote:
Show quoted text
On Wednesday, March 5, 2025, Aleksander Alekseev <aleksander@timescale.com>
wrote:Unlike superuser event triggers, which execute functions for every
role, database owner event triggers are only executed for non-superusers.
All this doesn't strike me as a great UI.
Yeah. Seems better to make “execute_for” an attribute of the trigger and
allow both superusers and non-superusers to create them. Then enforce that
non-superusers must specify the more limited value.Though it would seem nice to be able to exclude the pseudo-admin roles
these service providers create as well.David J.
On Wed, Mar 5, 2025 at 7:55 AM Steve Chavez <steve@supabase.io> wrote:
How about requiring explicit non-superuser execution for the database
owner evtrig? It would be like:CREATE EVENT TRIGGER name ... FOR NOSUPERUSER;
This is more what I was thinking - which works for a boolean. The ability
to have an exclusion list would make sense, or maybe just a predefined role
pg_bypass_eventtriggers.
David J.
Steve Chavez <steve@supabase.io> writes:
Currently PostgreSQL only allows creating event triggers for superusers,
this prevents usage on PostgreSQL service providers, which do not grant
superuser access.
This patch allows database owners to create event triggers, while
preventing privilege escalation.
I'm pretty down on this, at least in the form presented. While
you may have managed to keep the DB owner from sabotaging superusers,
the proposed feature still allows owning every other special role,
for example pg_write_server_files (which is something that's pretty
trivially exploitable to get superuser). Since we've generally been
working towards not requiring superuser for most routine admin tasks,
that problem is going to get worse not better over time. I don't
want to see us add a feature that creates a security reason to
avoid using those special roles in favor of using a superuser.
Or in other words: not-superuser to superuser is far from the only
type of privilege escalation that we need to prevent.
regards, tom lane
I wrote:
Or in other words: not-superuser to superuser is far from the only
type of privilege escalation that we need to prevent.
After reflecting on that for a moment: 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? That changes nothing for superuser-owned
triggers.
regards, tom lane
On Wed, 5 Mar 2025 at 10:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
Or in other words: not-superuser to superuser is far from the only
type of privilege escalation that we need to prevent.After reflecting on that for a moment: 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? That changes nothing for superuser-owned
triggers.
Can somebody remind me why triggers don't run as their owner in the first
place?
It would make triggers way more useful, and eliminate the whole issue of
trigger owners escalating to whomever tries to access the object on which
the trigger is defined.
Steve Chavez <steve@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
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 <tgl@sss.pgh.pa.us> wrote:
Show quoted text
Steve Chavez <steve@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
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 <steve@supabase.io> wrote:
Show quoted text
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 ChavezOn Fri, 7 Mar 2025 at 15:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Steve Chavez <steve@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
Attachments:
0001-Allow-regular-users-to-create-event-triggers.patchtext/x-patch; charset=US-ASCII; name=0001-Allow-regular-users-to-create-event-triggers.patchDownload
From 82a23eb69fe128eb530aaedf5ba08402a2cab585 Mon Sep 17 00:00:00 2001
From: steve-chavez <stevechavezast@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
On Sunday, April 20, 2025, Steve Chavez <steve@supabase.io> wrote:
- A new file event_trigger_nosuper.sql is added for the new tests
Expected output for the new script was not included in the commit.
Also, this looks unconventional…
EventTriggerCacheItem *item = (EventTriggerCacheItem*) lfirst_oid(lc);
David J.
Sorry, attached the output file.
Attachments:
0001-Allow-regular-users-to-create-event-triggers.patchtext/x-patch; charset=US-ASCII; name=0001-Allow-regular-users-to-create-event-triggers.patchDownload
From 2165513e858a5a6c7a620b1499b2f634c4f2ab44 Mon Sep 17 00:00:00 2001
From: steve-chavez <stevechavezast@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 +---
.../expected/event_trigger_nosuper.out | 74 +++++++++++++++++++
src/test/regress/parallel_schedule | 4 +
src/test/regress/sql/event_trigger.sql | 11 +--
.../regress/sql/event_trigger_nosuper.sql | 65 ++++++++++++++++
8 files changed, 156 insertions(+), 46 deletions(-)
create mode 100644 src/test/regress/expected/event_trigger_nosuper.out
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/expected/event_trigger_nosuper.out b/src/test/regress/expected/event_trigger_nosuper.out
new file mode 100644
index 00000000000..38c4c1d5301
--- /dev/null
+++ b/src/test/regress/expected/event_trigger_nosuper.out
@@ -0,0 +1,74 @@
+-- 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');
+ current_setting
+-----------------
+ on
+(1 row)
+
+create table evtrig_quux();
+\echo
+
+-- evtrig should fire for members
+set role member_1;
+create table evtrig_foo();
+NOTICE: the event trigger is executed for member_1
+NOTICE: the event trigger is executed for member_1
+\echo
+
+set role member_2;
+create table evtrig_bar();
+NOTICE: the event trigger is executed for member_2
+NOTICE: the event trigger is executed for member_2
+\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;
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.42.0
Also, this looks unconventional…
EventTriggerCacheItem *item = (EventTriggerCacheItem*) lfirst_oid(lc);
Just noticed the mistake there, I would have expected a compilation error.
New patch attached with the following change:
EventTriggerCacheItem *item = lfirst(lc);
On Sun, 20 Apr 2025 at 22:55, Steve Chavez <steve@supabase.io> wrote:
Show quoted text
Sorry, attached the output file.
Attachments:
0001-Allow-regular-users-to-create-event-triggers.patchtext/x-patch; charset=US-ASCII; name=0001-Allow-regular-users-to-create-event-triggers.patchDownload
From 2d0ee67eb7c2d005ca5011c2a9ae01933dc5dba4 Mon Sep 17 00:00:00 2001
From: steve-chavez <stevechavezast@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 +---
.../expected/event_trigger_nosuper.out | 74 +++++++++++++++++++
src/test/regress/parallel_schedule | 4 +
src/test/regress/sql/event_trigger.sql | 11 +--
.../regress/sql/event_trigger_nosuper.sql | 65 ++++++++++++++++
8 files changed, 156 insertions(+), 46 deletions(-)
create mode 100644 src/test/regress/expected/event_trigger_nosuper.out
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..7ae0636f7c2 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 = lfirst(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/expected/event_trigger_nosuper.out b/src/test/regress/expected/event_trigger_nosuper.out
new file mode 100644
index 00000000000..38c4c1d5301
--- /dev/null
+++ b/src/test/regress/expected/event_trigger_nosuper.out
@@ -0,0 +1,74 @@
+-- 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');
+ current_setting
+-----------------
+ on
+(1 row)
+
+create table evtrig_quux();
+\echo
+
+-- evtrig should fire for members
+set role member_1;
+create table evtrig_foo();
+NOTICE: the event trigger is executed for member_1
+NOTICE: the event trigger is executed for member_1
+\echo
+
+set role member_2;
+create table evtrig_bar();
+NOTICE: the event trigger is executed for member_2
+NOTICE: the event trigger is executed for member_2
+\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;
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.42.0
On Sunday, April 20, 2025, Steve Chavez <steve@supabase.io> wrote:
Also, this looks unconventional…
EventTriggerCacheItem *item = (EventTriggerCacheItem*) lfirst_oid(lc);Just noticed the mistake there, I would have expected a compilation error.
New patch attached with the following change:EventTriggerCacheItem *item = lfirst(lc);
On Sun, 20 Apr 2025 at 22:55, Steve Chavez <steve@supabase.io> wrote:
Sorry, attached the output file.
You can remove role member_1 and trigger..1 and “create table foo” from the
nosuper script without any loss of test coverage. Or member2 trigger2
table_bar along with the alter event trigger command which doesn’t need to
be exercised here. Ownership is all that matters. Whether come to
directly or via alter.
Actually, leave the other member around, but not granted ownership, and
both create tables, to demonstrate that a non-superuser and non-owner is
unaffected by the trigger.
David J.
alter event trigger command which doesn’t need to be exercised here
That part does need to be tested, I modified
`AlterEventTriggerOwner_internal` to allow altering owners to regular
users. Before it was only restricted to superusers.
Actually, leave the other member around, but not granted ownership, and
both create tables, to demonstrate that a non-superuser and non-owner is
unaffected by the trigger.
I've updated the tests accordingly. Please let me know if that's what you
had in mind.
Best regards,
Steve Chavez
On Sun, 20 Apr 2025 at 23:13, David G. Johnston <david.g.johnston@gmail.com>
wrote:
Show quoted text
On Sunday, April 20, 2025, Steve Chavez <steve@supabase.io> wrote:
Also, this looks unconventional…
EventTriggerCacheItem *item = (EventTriggerCacheItem*) lfirst_oid(lc);Just noticed the mistake there, I would have expected a compilation
error. New patch attached with the following change:EventTriggerCacheItem *item = lfirst(lc);
On Sun, 20 Apr 2025 at 22:55, Steve Chavez <steve@supabase.io> wrote:
Sorry, attached the output file.
You can remove role member_1 and trigger..1 and “create table foo” from
the nosuper script without any loss of test coverage. Or member2 trigger2
table_bar along with the alter event trigger command which doesn’t need to
be exercised here. Ownership is all that matters. Whether come to
directly or via alter.Actually, leave the other member around, but not granted ownership, and
both create tables, to demonstrate that a non-superuser and non-owner is
unaffected by the trigger.David J.
Attachments:
v2-0001-Allow-regular-users-to-create-event-triggers.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Allow-regular-users-to-create-event-triggers.patchDownload
From e9a73cda8145a04b8375d21e37a6e713af1bed34 Mon Sep 17 00:00:00 2001
From: steve-chavez <stevechavezast@gmail.com>
Date: Sun, 20 Apr 2025 19:46:00 -0500
Subject: [PATCH v2] 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 +---
.../expected/event_trigger_nosuper.out | 71 +++++++++++++++++++
src/test/regress/parallel_schedule | 4 ++
src/test/regress/sql/event_trigger.sql | 11 +--
.../regress/sql/event_trigger_nosuper.sql | 59 +++++++++++++++
8 files changed, 147 insertions(+), 46 deletions(-)
create mode 100644 src/test/regress/expected/event_trigger_nosuper.out
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..7ae0636f7c2 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 = lfirst(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/expected/event_trigger_nosuper.out b/src/test/regress/expected/event_trigger_nosuper.out
new file mode 100644
index 00000000000..e662d5b4f11
--- /dev/null
+++ b/src/test/regress/expected/event_trigger_nosuper.out
@@ -0,0 +1,71 @@
+-- setup roles and privileges
+create role evtrig_owner;
+create role member;
+grant evtrig_owner to member;
+create role non_member;
+grant all on schema public to evtrig_owner, member, 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
+
+-- evtrig should not fire for superusers
+select current_setting('is_superuser');
+ current_setting
+-----------------
+ on
+(1 row)
+
+create table evtrig_quux();
+\echo
+
+-- evtrig should fire for member
+set role member;
+create table evtrig_foo();
+NOTICE: the event trigger is executed for member
+\echo
+
+-- evtrig should not fire for non-member
+set role non_member;
+create table evtrig_qux();
+\echo
+
+-- altering the event trigger owner to a regular user should work
+reset role;
+select current_setting('is_superuser');
+ current_setting
+-----------------
+ on
+(1 row)
+
+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
+
+-- cleanup
+drop table evtrig_qux;
+drop table evtrig_bar;
+ERROR: table "evtrig_bar" does not exist
+drop table evtrig_foo;
+revoke all on schema public from member, 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;
+drop role non_member;
+drop role evtrig_owner;
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..0ba41089f08
--- /dev/null
+++ b/src/test/regress/sql/event_trigger_nosuper.sql
@@ -0,0 +1,59 @@
+-- setup roles and privileges
+create role evtrig_owner;
+create role member;
+grant evtrig_owner to member;
+create role non_member;
+grant all on schema public to evtrig_owner, member, 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
+
+-- evtrig should not fire for superusers
+select current_setting('is_superuser');
+create table evtrig_quux();
+\echo
+
+-- evtrig should fire for member
+set role member;
+create table evtrig_foo();
+\echo
+
+-- evtrig should not fire for non-member
+set role non_member;
+create table evtrig_qux();
+\echo
+
+-- altering the event trigger owner to a regular user should work
+reset role;
+select current_setting('is_superuser');
+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
+
+-- cleanup
+drop table evtrig_qux;
+drop table evtrig_bar;
+drop table evtrig_foo;
+revoke all on schema public from member, 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;
+drop role non_member;
+drop role evtrig_owner;
--
2.40.1
On Tuesday, April 22, 2025, Steve Chavez <steve@supabase.io> wrote:
alter event trigger command which doesn’t need to be exercised here
That part does need to be tested, I modified `AlterEventTriggerOwner_internal`
to allow altering owners to regular users. Before it was only restricted to
superusers.Actually, leave the other member around, but not granted ownership, and
both create tables, to demonstrate that a non-superuser and non-owner is
unaffected by the trigger.I've updated the tests accordingly. Please let me know if that's what you
had in mind.
Pretty much. You have a bad drop table cleanup command, and I’d drop the
entire alter event trigger owner test.
The other thing I’m wondering, but haven’t gotten around to testing, is
whether a role affected by the event trigger is able to just drop the
trigger given this implementation. I always get member/member-of dynamics
confused. Having member (possibly via set role…) trying to drop the
trigger would be good to prove that it isn’t allowed.
David J.
On Tuesday, April 22, 2025, David G. Johnston <david.g.johnston@gmail.com>
wrote:
On Tuesday, April 22, 2025, Steve Chavez <steve@supabase.io> wrote:
alter event trigger command which doesn’t need to be exercised here
That part does need to be tested, I modified
`AlterEventTriggerOwner_internal` to allow altering owners to regular
users. Before it was only restricted to superusers.
Ok. I missed this.
David J.
On Tuesday, April 22, 2025, David G. Johnston <david.g.johnston@gmail.com>
wrote:
On Tuesday, April 22, 2025, David G. Johnston <david.g.johnston@gmail.com>
wrote:On Tuesday, April 22, 2025, Steve Chavez <steve@supabase.io> wrote:
alter event trigger command which doesn’t need to be exercised here
That part does need to be tested, I modified
`AlterEventTriggerOwner_internal` to allow altering owners to regular
users. Before it was only restricted to superusers.Ok. I missed this.
Sorry for the self-reply but this nagged at me.
It’s probably not a big deal either way, but the prior test existed to
ensure that a superuser couldn’t do something they are otherwise are always
permitted to do - assign object to whomever they wish. So
event_trigger.sql had a test that errored showing this anomaly. You moved
the test and now are proving it doesn’t error. But it is not expected to
error; and immediately above you already show that a non-superuser can be
an owner. We don’t need a test to show a superuser demonstrating their
normal abilities.
IOW, select test cases around the feature as it is implemented now, not its
history. A personal one-off test to ensure that no super-user prohibitions
remained will suffice to make sure all such code that needed to be removed
is gone.
David J.
You have a bad drop table cleanup command, and I’d drop the entire alter
event trigger owner test.
My bad, removed the bad drop table and also removed the alter owner test.
The other thing I’m wondering, but haven’t gotten around to testing, is
whether a role affected by the event trigger is able to just drop the
trigger given this implementation. I always get member/member-of dynamics
confused. Having member (possibly via set role…) trying to drop the
trigger would be good to prove that it isn’t allowed.
So this one is a problem. If we do `grant evtrig_owner to member` then
`member` will be able to drop the event trigger, which is no good. There
are 2 option to solve this:
1. Do `grant evtrig_owner to member with inherit false`, then `member` will
not be able to drop the event trigger. I've updated the tests to reflect
that.
2. `create role member noinherit`, which won't let `member` drop the event
trigger with a regular `grant evtrig_owner to member`. But this change is
more invasive towards existing roles.
That being said, it's not a good default behavior to let evtrigger targets
to drop the evtrigger. Should we enforce that only granting with inherit
false will make the role members targets of the evtrigger? Are there any
other options?
On Tue, 22 Apr 2025 at 20:18, David G. Johnston <david.g.johnston@gmail.com>
wrote:
Show quoted text
On Tuesday, April 22, 2025, David G. Johnston <david.g.johnston@gmail.com>
wrote:On Tuesday, April 22, 2025, David G. Johnston <david.g.johnston@gmail.com>
wrote:On Tuesday, April 22, 2025, Steve Chavez <steve@supabase.io> wrote:
alter event trigger command which doesn’t need to be exercised here
That part does need to be tested, I modified
`AlterEventTriggerOwner_internal` to allow altering owners to regular
users. Before it was only restricted to superusers.Ok. I missed this.
Sorry for the self-reply but this nagged at me.
It’s probably not a big deal either way, but the prior test existed to
ensure that a superuser couldn’t do something they are otherwise are always
permitted to do - assign object to whomever they wish. So
event_trigger.sql had a test that errored showing this anomaly. You moved
the test and now are proving it doesn’t error. But it is not expected to
error; and immediately above you already show that a non-superuser can be
an owner. We don’t need a test to show a superuser demonstrating their
normal abilities.IOW, select test cases around the feature as it is implemented now, not
its history. A personal one-off test to ensure that no super-user
prohibitions remained will suffice to make sure all such code that needed
to be removed is gone.David J.
Attachments:
v3-0001-Allow-regular-users-to-create-event-triggers.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Allow-regular-users-to-create-event-triggers.patchDownload
From e9e294a638d55458adb6d08c9d2ce22e0c41f267 Mon Sep 17 00:00:00 2001
From: steve-chavez <stevechavezast@gmail.com>
Date: Sun, 20 Apr 2025 19:46:00 -0500
Subject: [PATCH v3] Allow regular users to create event triggers
* fix bad drop table
* remove alter owner test
* add test for not allowing member to drop evtrig
---
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 +---
.../expected/event_trigger_nosuper.out | 60 +++++++++++++++++++
src/test/regress/parallel_schedule | 4 ++
src/test/regress/sql/event_trigger.sql | 11 +---
.../regress/sql/event_trigger_nosuper.sql | 53 ++++++++++++++++
8 files changed, 130 insertions(+), 46 deletions(-)
create mode 100644 src/test/regress/expected/event_trigger_nosuper.out
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..7ae0636f7c2 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 = lfirst(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/expected/event_trigger_nosuper.out b/src/test/regress/expected/event_trigger_nosuper.out
new file mode 100644
index 00000000000..86d8f8b2da3
--- /dev/null
+++ b/src/test/regress/expected/event_trigger_nosuper.out
@@ -0,0 +1,60 @@
+-- setup roles and privileges
+create role evtrig_owner;
+create role member;
+grant evtrig_owner to member with inherit false;
+create role non_member;
+grant all on schema public to evtrig_owner, member, 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
+
+-- evtrig should not fire for superusers
+select current_setting('is_superuser');
+ current_setting
+-----------------
+ on
+(1 row)
+
+create table evtrig_quux();
+\echo
+
+-- evtrig should fire for member
+set role member;
+create table evtrig_foo();
+NOTICE: the event trigger is executed for member
+\echo
+
+-- member is not allowed to drop the evtrig
+drop event trigger evtrig_show_current_user_1;
+ERROR: must be owner of event trigger evtrig_show_current_user_1
+\echo
+
+-- evtrig should not fire for non-member
+set role non_member;
+create table evtrig_qux();
+\echo
+
+-- cleanup
+reset role;
+drop table evtrig_qux;
+drop table evtrig_foo;
+revoke all on schema public from member, non_member, evtrig_owner;
+drop event trigger evtrig_show_current_user_1;
+drop function show_current_user();
+drop role member;
+drop role non_member;
+drop role evtrig_owner;
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..70e5e82a7cb
--- /dev/null
+++ b/src/test/regress/sql/event_trigger_nosuper.sql
@@ -0,0 +1,53 @@
+-- setup roles and privileges
+create role evtrig_owner;
+create role member;
+grant evtrig_owner to member with inherit false;
+create role non_member;
+grant all on schema public to evtrig_owner, member, 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
+
+-- evtrig should not fire for superusers
+select current_setting('is_superuser');
+create table evtrig_quux();
+\echo
+
+-- evtrig should fire for member
+set role member;
+create table evtrig_foo();
+\echo
+
+-- member is not allowed to drop the evtrig
+drop event trigger evtrig_show_current_user_1;
+\echo
+
+-- evtrig should not fire for non-member
+set role non_member;
+create table evtrig_qux();
+\echo
+
+-- cleanup
+reset role;
+drop table evtrig_qux;
+drop table evtrig_foo;
+revoke all on schema public from member, non_member, evtrig_owner;
+drop event trigger evtrig_show_current_user_1;
+drop function show_current_user();
+drop role member;
+drop role non_member;
+drop role evtrig_owner;
--
2.42.0
Isaac,
Can somebody remind me why triggers don't run as their owner in the first
place?
It would make triggers way more useful, and eliminate the whole issue of
trigger owners escalating to whomever tries to access the object on which
the trigger is defined.
Just noted this is already possible when marking the event trigger function
as SECURITY DEFINER (instead of having the SECURITY INVOKER default), it
will fire for every role but keeping the privilege of the event trigger
creator.
Seeing that we have a problem with membership-based event triggers, how
about if we require that regular user event triggers can only have SECURITY
DEFINER functions? We can enforce this at `create event trigger` time.
Best regards,
Steve Chavez
On Wed, 5 Mar 2025 at 11:17, Isaac Morland <isaac.morland@gmail.com> wrote:
Show quoted text
On Wed, 5 Mar 2025 at 10:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
Or in other words: not-superuser to superuser is far from the only
type of privilege escalation that we need to prevent.After reflecting on that for a moment: 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? That changes nothing for superuser-owned
triggers.Can somebody remind me why triggers don't run as their owner in the first
place?It would make triggers way more useful, and eliminate the whole issue of
trigger owners escalating to whomever tries to access the object on which
the trigger is defined.
On Monday, April 28, 2025, Steve Chavez <steve@supabase.io> wrote:
1. Do `grant evtrig_owner to member with inherit false`, then `member`
will not be able to drop the event trigger.
I think they can still use set role…
After getting my head around this, and re-reading what Tom said, I think
you have the grant backwards. You’ve made member a member of owner, while
the specification would have to be making owner a member of member.
“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.” - Tom Lane
This might be better to do if you restrict yourself to use a role with the
createrole attribute instead of a superuser - since that is effectively
what you are trying to model anyway.
David J.