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:
https://www.postgresql.org/message-id/flat/81C10FFB-5ADC-4956-9337-FA248A4CC20D%40enterprisedb.com#77738d12b82c9a403ea2c56ed09949a3
)
Best regards,
Steve Chavez
From 84965d3ad70c4794f93473b939f5437c0d154826 Mon Sep 17 00:00:00 2001
From: steve-chavez <stevechavez...@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
