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+197-10
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+82-43
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+156-43
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+156-43
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+147-43
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.