Wrong security context for deferred triggers?

On Mon, 2023-11-06 at 14:23 +0100, Laurenz Albe wrote:

CREATE FUNCTION trig() RETURNS trigger
LANGUAGE plpgsql AS
$$BEGIN
RAISE NOTICE 'current_user = %', current_user;
RETURN NEW;
END;$$;

CREATE CONSTRAINT TRIGGER trig AFTER INSERT ON tab
DEFERRABLE INITIALLY IMMEDIATE
FOR EACH ROW EXECUTE FUNCTION trig();

SET ROLE duff;

BEGIN;

INSERT INTO tab VALUES (1);
NOTICE: current_user = duff

That looks ok; the current user is "duff".

SET CONSTRAINTS ALL DEFERRED;

INSERT INTO tab VALUES (2);

Become a superuser again and commit:

RESET ROLE;

COMMIT;
NOTICE: current_user = postgres

So a deferred constraint trigger does not run with the same security context
as an immediate trigger. This is somewhat nasty in combination with
SECURITY DEFINER functions: if that function performs an operation, and that
operation triggers a deferred trigger, that trigger will run in the wrong
security context.

This behavior looks buggy to me. What do you think?
I cannot imagine that it is a security problem, though.

I just realized one problem with running a deferred constraint trigger as
the triggering role: that role might have been dropped by the time the trigger
executes. But then we could still error out.

Yours,
Laurenz Albe

On Mon, 6 Nov 2023 at 11:58, Laurenz Albe <laurenz.albe@cybertec.at> wrote:

Become a superuser again and commit:

RESET ROLE;

COMMIT;
NOTICE: current_user = postgres

So a deferred constraint trigger does not run with the same security

context

as an immediate trigger. This is somewhat nasty in combination with
SECURITY DEFINER functions: if that function performs an operation, and

that

operation triggers a deferred trigger, that trigger will run in the wrong
security context.

This behavior looks buggy to me. What do you think?
I cannot imagine that it is a security problem, though.

This looks to me like another reason that triggers should run as the
trigger owner. Which role owns the trigger won’t change as a result of
constraints being deferred or not, or any role setting done during the
transaction, including that relating to security definer functions.

Right now triggers can’t do anything that those who can
INSERT/UPDATE/DELETE (i.e., cause the trigger to fire) can’t do, which in
particular breaks the almost canonical example of using triggers to log
changes — I can’t do it without also allowing users to make spurious log
entries.

Also if I cause a trigger to fire, I’ve just given the trigger owner the
opportunity to run arbitrary code as me.

I just realized one problem with running a deferred constraint trigger as
the triggering role: that role might have been dropped by the time the
trigger
executes. But then we could still error out.

This problem is also fixed by running triggers as their owners: there
should be a dependency between an object and its owner. So the
trigger-executing role can’t be dropped without dropping the trigger.

On 11/6/23 14:23, Laurenz Albe wrote:

...

This behavior looks buggy to me. What do you think?
I cannot imagine that it is a security problem, though.

How could code getting executed under the wrong role not be a security
issue? Also, does this affect just the role, or are there some other
settings that may unexpectedly change (e.g. search_path)?

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, 2023-11-06 at 18:29 +0100, Tomas Vondra wrote:

On 11/6/23 14:23, Laurenz Albe wrote:

This behavior looks buggy to me. What do you think?
I cannot imagine that it is a security problem, though.

How could code getting executed under the wrong role not be a security
issue? Also, does this affect just the role, or are there some other
settings that may unexpectedly change (e.g. search_path)?

Perhaps it is a security issue, and I am just lacking imagination.

Yes, changes to "search_path" should also have an effect.

Yours,
Laurenz Albe

On Mon, 2023-11-06 at 18:29 +0100, Tomas Vondra wrote:

On 11/6/23 14:23, Laurenz Albe wrote:

This behavior looks buggy to me. What do you think?
I cannot imagine that it is a security problem, though.

How could code getting executed under the wrong role not be a security
issue? Also, does this affect just the role, or are there some other
settings that may unexpectedly change (e.g. search_path)?

Here is a patch that fixes this problem by keeping track of the
current role in the AfterTriggerSharedData.

I have thought some more about the security aspects:

1. With the new code, you could call a SECURITY DEFINER function
that modifies data on a table with a deferred trigger, then
modify the trigger function before you commit and have your
code run with elevated privileges.
But I think that we need not worry about that. If a
superuser performs DML on a table that an untrusted user
controls, all bets are off anyway. The attacker might as
well put the bad code into the trigger *before* calling the
SECURITY DEFINER function.

2. The more serious concern is that the old code constitutes
a narrow security hazard: a superuser could temporarily
assume an unprivileged role to avoid risks while performing
DML on a table controlled by an untrusted user, but for
some reason resume being a superuser *before* COMMIT.
Then a deferred trigger would inadvertently run with
superuser privileges.

That seems like a very unlikely scenario (who would RESET ROLE
before committing in that situation?). Moreover, it seems
like the current buggy behavior has been in place for decades
without anybody noticing.

I am not against backpatching this (the fix is simple enough),
but I am not convinced that it is necessary.

Yours,
Laurenz Albe

Hi,

I see that this patch is marked as ready for review, so I thought I
would attempt to review it. This is my first review, so please take it
with a grain of salt.

So a deferred constraint trigger does not run with the same security

context

as an immediate trigger.

It sounds like the crux of your argument is that the current behavior
is that triggers are executed with the role and security context of the
session at the time of execution. Instead, the trigger should be
executed with the role and security context of the session at the time
time of queuing (i.e. the same context as the action that triggered the
trigger). While I understand that the current behavior can be
surprising in some scenarios, it's not clear to me why this behavior is
wrong. It seems that the whole point of deferring a trigger to commit
time is that the context that the trigger is executed in is different
than the context that it was triggered in. Tables may have changed,
permissions may have changed, session configuration variables may have
changed, roles may have changed, etc. So why should the executing role
be treated differently and restored to the value at the time of
triggering. Perhaps you can expand on why you feel that the current
behavior is wrong?

This is somewhat nasty in combination with
SECURITY DEFINER functions: if that function performs an operation, and

that

operation triggers a deferred trigger, that trigger will run in the wrong
security context.

...

The more serious concern is that the old code constitutes
a narrow security hazard: a superuser could temporarily
assume an unprivileged role to avoid risks while performing
DML on a table controlled by an untrusted user, but for
some reason resume being a superuser *before* COMMIT.
Then a deferred trigger would inadvertently run with
superuser privileges.

I find these examples to be surprising, but not necessarily wrong
(as per my comment above). If someone wants the triggers to be executed
as the triggering role, then they can run `SET CONSTRAINTS ALL
IMMEDIATE`. If deferring a trigger to commit time and executing it as
the triggering role is desirable, then maybe we should add a modifier
to triggers that can control this behavior. Something like
`SECURITY INVOKER | SECURITY TRIGGERER` (modeled after the modifiers in
`CREATE FUNCTION`) that control which role is used.

This looks to me like another reason that triggers should run as the
trigger owner. Which role owns the trigger won’t change as a result of
constraints being deferred or not, or any role setting done during the
transaction, including that relating to security definer functions.

Right now triggers can’t do anything that those who can
INSERT/UPDATE/DELETE (i.e., cause the trigger to fire) can’t do, which in
particular breaks the almost canonical example of using triggers to log
changes — I can’t do it without also allowing users to make spurious log
entries.

Also if I cause a trigger to fire, I’ve just given the trigger owner the
opportunity to run arbitrary code as me.

I just realized one problem with running a deferred constraint trigger as
the triggering role: that role might have been dropped by the time the
trigger
executes. But then we could still error out.

This problem is also fixed by running triggers as their owners: there
should be a dependency between an object and its owner. So the
trigger-executing role can’t be dropped without dropping the trigger.

+1, this approach would remove all of the surprising/wrong behavior and
in my opinion is more obvious. I'd like to add some more reasons why
this behavior makes sense:

- The documentation [0]https://www.postgresql.org/docs/16/sql-createtrigger.html indicates that to create a trigger, the
creating role must have the `EXECUTE` privilege on the trigger
function. In fact this check is skipped for the role that triggers
trigger.

-- Create trig_owner role and function. Grant execute on function
-- to role.
test=# CREATE ROLE trig_owner;
CREATE ROLE
test=# GRANT CREATE ON SCHEMA public TO trig_owner;
GRANT
test=# CREATE OR REPLACE FUNCTION f() RETURNS trigger
LANGUAGE plpgsql AS
$$BEGIN
RAISE NOTICE 'current_user = %', current_user;
RETURN NEW;
END;$$;
CREATE FUNCTION
test=# REVOKE EXECUTE ON FUNCTION f FROM PUBLIC;
REVOKE
test=# GRANT EXECUTE ON FUNCTION f TO trig_owner;
GRANT

-- Create the trigger as trig_owner.
test=# SET ROLE trig_owner;
SET
test=> CREATE TABLE t (a INT);
CREATE TABLE
test=> CREATE CONSTRAINT TRIGGER trig AFTER INSERT ON t
DEFERRABLE INITIALLY IMMEDIATE
FOR EACH ROW EXECUTE FUNCTION f();
CREATE TRIGGER

-- Trigger the trigger with a role that doesn't have execute
-- privileges on the trigger function and also call the function
-- directly. The trigger succeeds but the function call fails.
test=> RESET ROLE;
RESET
test=# CREATE ROLE r1;
CREATE ROLE
test=# GRANT INSERT ON t TO r1;
GRANT
test=# SET ROLE r1;
SET
test=> INSERT INTO t VALUES (1);
NOTICE: current_user = r1
INSERT 0 1
test=> SELECT f();
ERROR: permission denied for function f

So the execute privilege check on the trigger function is done using
the trigger owner role, why shouldn't all privilege checks use the
trigger owner?

- The set of triggers that will execute as a result of any statement
is not obvious. Therefore it is non-trivial to figure out if you're
role will have the privileges necessary to execute a given statement.
Switching to the trigger owner role makes this check trivial.

- This is consistent with how view privileges work. When querying a
view, the privileges of the view owner is checked against the view
definition. Similarly when executing a trigger, the trigger owner's
privileges should be checked against the trigger definition.

However, I do worry that this is too much of a breaking change and
fundamentally changes how triggers work. Another draw back is that if
the trigger owner loses the required privileges, then no one can modify
to the table.

Here is a patch that fixes this problem by keeping track of the

current role in the AfterTriggerSharedData.

I skimmed the code and haven't looked at in depth. Whichever direction
we go, I think it's worth updating the documentation to make the
behavior around triggers and roles clear.

Additionally, I applied your patch to master and re-ran the example and
didn't notice any behavior change.

test=# CREATE TABLE tab (i integer);
CREATE TABLE
test=# CREATE FUNCTION trig() RETURNS trigger
LANGUAGE plpgsql AS
$$BEGIN
RAISE NOTICE 'current_user = %', current_user;
RETURN NEW;
END;$$;
CREATE FUNCTION
test=# CREATE CONSTRAINT TRIGGER trig AFTER INSERT ON tab
DEFERRABLE INITIALLY IMMEDIATE
FOR EACH ROW EXECUTE FUNCTION trig();
CREATE TRIGGER
test=# CREATE ROLE duff;
CREATE ROLE
test=# GRANT INSERT ON tab TO duff;
GRANT
test=# SET ROLE duff;
SET
test=> BEGIN;
BEGIN
test=*> INSERT INTO tab VALUES (1);
NOTICE: current_user = duff
INSERT 0 1
test=*> SET CONSTRAINTS ALL DEFERRED;
SET CONSTRAINTS
test=*> INSERT INTO tab VALUES (2);
INSERT 0 1
test=*> RESET ROLE;
RESET
test=*# COMMIT;
NOTICE: current_user = joe
COMMIT

Though maybe I'm just doing something wrong.

Thanks,
Joe Koshakow

P.S. Since this is my first review, feel free to give me meta-comments
critiquing the review.

[0]: https://www.postgresql.org/docs/16/sql-createtrigger.html

On Sat, Jun 8, 2024 at 5:36 PM Joseph Koshakow <koshy44@gmail.com> wrote:

Additionally, I applied your patch to master and re-ran the example and
didn't notice any behavior change.

test=# CREATE TABLE tab (i integer);
CREATE TABLE
test=# CREATE FUNCTION trig() RETURNS trigger
LANGUAGE plpgsql AS
$$BEGIN
RAISE NOTICE 'current_user = %', current_user;
RETURN NEW;
END;$$;
CREATE FUNCTION
test=# CREATE CONSTRAINT TRIGGER trig AFTER INSERT ON tab
DEFERRABLE INITIALLY IMMEDIATE
FOR EACH ROW EXECUTE FUNCTION trig();
CREATE TRIGGER
test=# CREATE ROLE duff;
CREATE ROLE
test=# GRANT INSERT ON tab TO duff;
GRANT
test=# SET ROLE duff;
SET
test=> BEGIN;
BEGIN
test=*> INSERT INTO tab VALUES (1);
NOTICE: current_user = duff
INSERT 0 1
test=*> SET CONSTRAINTS ALL DEFERRED;
SET CONSTRAINTS
test=*> INSERT INTO tab VALUES (2);
INSERT 0 1
test=*> RESET ROLE;
RESET
test=*# COMMIT;
NOTICE: current_user = joe
COMMIT

Though maybe I'm just doing something wrong.

Sorry, there's definitely something wrong with my environment. You can
ignore this.

Thanks,
Joe Koshakow

On Sat, 8 Jun 2024 at 17:37, Joseph Koshakow <koshy44@gmail.com> wrote:

However, I do worry that this is too much of a breaking change and
fundamentally changes how triggers work. Another draw back is that if
the trigger owner loses the required privileges, then no one can modify
to the table.

I worry about breaking changes too. The more I think about it, though, the
more I think the existing behaviour doesn’t make sense.

Speaking as a table owner, when I set a trigger on it, I expect that when
the specified actions occur my trigger will fire and will do what I
specify, without regard to the execution environment of the caller
(search_path in particular); and my trigger should be able to do anything
that I can do. For the canonical case of a logging table the trigger has to
be able to do stuff the caller can't do. I don't expect to be able to do
stuff that the caller can do.

Speaking as someone making an update on a table, I don't expect to have it
fail because my execution environment (search_path in particular) is wrong
for the trigger implementation, and I consider it a security violation if
the table owner is able to do stuff as me as a result, especially if I am
an administrator making an update as superuser.

In effect, I want the action which fires the trigger to be like a system
call, and the trigger, plus the underlying action, to be like what the OS
does in response to the system call.

I'm not sure how to evaluate what problems with existing implementations
would be caused by changing what role executes triggers, but I think it's
pretty clear the existing behaviour is the wrong choice in every other way
than backward compatibility. I welcome examples to the contrary, where the
existing behaviour is not just OK but actually wanted.

On Sat, Jun 8, 2024 at 10:13 PM Isaac Morland <isaac.morland@gmail.com>
wrote:

Speaking as a table owner, when I set a trigger on it, I expect that when

the specified actions occur my trigger will fire and will do what I
specify, without regard to the execution environment of the caller
(search_path in particular); and my trigger should be able to do anything
that I can do. For the canonical case of a logging table the trigger has to
be able to do stuff the caller can't do. I don't expect to be able to do
stuff that the caller can do.

Speaking as someone making an update on a table, I don't expect to have

it fail because my execution environment (search_path in particular) is
wrong for the trigger implementation, and I consider it a security
violation if the table owner is able to do stuff as me as a result,
especially if I am an administrator making an update as superuser.

Can you expand on this a bit? When a trigger executes should the
execution environment match:

- The execution environment of the trigger owner at the time of
trigger creation?
- The execution environment of the function owner at the time of
function creation?
- An execution environment built from the trigger owner's default
configuration parameters?
- Something else?

While I am convinced that privileges should be checked using the
trigger owner's role, I'm less convinced of other configuration
parameters. For the search_path example, that can be resolved by
either fully qualifying object names or setting the search_path in the
function itself. Similar approaches can be taken with other
configuration parameters.

I also worry that it would be a source of confusion that the execution
environment of triggers come from the trigger/function owner, but the
execution environment of function calls come from the caller.

I think it's pretty clear the existing behaviour is the wrong choice in

every other way than backward compatibility. I welcome examples to the
contrary, where the existing behaviour is not just OK but actually wanted.

This is perhaps a contrived example, but here's one. Suppose I create a
trigger that raises a notice that includes the current timestamp. I
would probably want to use the timezone of the caller, not the
trigger owner.

Thanks,
Joe Koshakow

On Sat, 2024-06-08 at 17:36 -0400, Joseph Koshakow wrote:

I see that this patch is marked as ready for review, so I thought I
would attempt to review it. This is my first review, so please take it
with a grain of salt.

Thank you. Your review is valuable and much to the point.

It sounds like the crux of your argument is that the current behavior
is that triggers are executed with the role and security context of the
session at the time of execution. Instead, the trigger should be
executed with the role and security context of the session at the time
time of queuing (i.e. the same context as the action that triggered the
trigger). While I understand that the current behavior can be
surprising in some scenarios, it's not clear to me why this behavior is
wrong.

Since neither the documentation nor any source comment cover this case,
it is to some extent a matter of taste if the current behavior is
correct ot not. An alternative to my patch would be to document the
current behavior rather than change it.

Like you, I was surprised by the current behavior. There is a design
principle that PostgreSQL tries to follow, called the "Principle of
least astonishment". Things should behave like a moderately skilled
user would expect them to. In my opinion, the current behavior violates
that principle. Tomas seems to agree with that point of view.

On the other hand, changing current behavior carries the risk of
backward compatibility problems. I don't know how much of a problem
that would be, but I have the impression that few people use deferred
triggers in combination with SET ROLE or SECURITY DEFINER functions,
which makes the problem rather exotic, so hopefully the pain caused by
the compatibility break will be moderate.

I didn't find this strange behavior myself: it was one of our customers
who uses security definer functions for data modifications and has
problems with the current behavior, and I am trying to improve the
situation on their behalf.

It seems that the whole point of deferring a trigger to commit
time is that the context that the trigger is executed in is different
than the context that it was triggered in. Tables may have changed,
permissions may have changed, session configuration variables may have
changed, roles may have changed, etc. So why should the executing role
be treated differently and restored to the value at the time of
triggering. Perhaps you can expand on why you feel that the current
behavior is wrong?

True, somebody could change permissions or parameters between the
DML statement and COMMIT, but that feels like external influences to me.
Those changes would be explicit.

But I feel that the database user that runs the trigger should be the
same user that ran the triggering SQL statement. Even though I cannot
put my hand on a case where changing this user would constitute a real
security problem, it feels wrong.

I am aware that that is rather squishy argumentation, but I have no
better one. Both my and Thomas' gut reaction seems to have been "the
current behavior is wrong".

I find these examples to be surprising, but not necessarily wrong
(as per my comment above). If someone wants the triggers to be executed
as the triggering role, then they can run `SET CONSTRAINTS ALL
IMMEDIATE`. If deferring a trigger to commit time and executing it as
the triggering role is desirable, then maybe we should add a modifier
to triggers that can control this behavior. Something like
`SECURITY INVOKER | SECURITY TRIGGERER` (modeled after the modifiers in
`CREATE FUNCTION`) that control which role is used.

SECURITY INVOKER and SECURITY TRIGGERER seem too confusing. I would say
that the triggerer is the one who invokes the trigger...

It would have to be a switch like EXECUTE DEFERRED TRIGGER AS INVOKER|COMMITTER
or so, but I think that special SQL syntax for this exotic corner case
is a little too much. And then: is there anybody who feels that the current
behavior is desirable?

Isaac Morland wrote:

This looks to me like another reason that triggers should run as the
trigger owner. Which role owns the trigger won’t change as a result of
constraints being deferred or not, or any role setting done during the
transaction, including that relating to security definer functions.

+1, this approach would remove all of the surprising/wrong behavior and
in my opinion is more obvious. I'd like to add some more reasons why
this behavior makes sense: [...]

However, I do worry that this is too much of a breaking change and
fundamentally changes how triggers work.

Yes. This might be the right thing to do if we designed triggers as a
new feature, but changing the behavior like that would certainly break
a lot of cases.

People who want that behavior use a SECURITY DEFINER trigger function.

I skimmed the code and haven't looked at in depth. Whichever direction
we go, I think it's worth updating the documentation to make the
behavior around triggers and roles clear.

I agree: adding a sentence somewhere won't hurt.
I'll do that once the feedback has given me the feeling that I am on
the right track.

Yours,
Laurenz Albe

On Mon, Jun 10, 2024 at 1:00 PM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:

Like you, I was surprised by the current behavior. There is a design
principle that PostgreSQL tries to follow, called the "Principle of
least astonishment". Things should behave like a moderately skilled
user would expect them to. In my opinion, the current behavior

violates

that principle. Tomas seems to agree with that point of view.

I worry that both approaches violate this principle in different ways.
For example consider the following sequence of events:

SET ROLE r1;
BEGIN;
SET CONSTRAINTS ALL DEFERRED;
INSERT INTO ...;
SET ROLE r2;
SET search_path = '...';
COMMIT;

I think that it would be reasonable to expect that the triggers execute
with r2 and not r1, since the triggers were explicitly deferred and the
role was explicitly set. It would likely be surprising that the search
path was updated for the trigger but not the role. With your proposed
approach it would be impossible for someone to trigger a trigger with
one role and execute it with another, if that's a desirable feature.

I didn't find this strange behavior myself: it was one of our customers
who uses security definer functions for data modifications and has
problems with the current behavior, and I am trying to improve the
situation on their behalf.

Would it be possible to share more details about this use case? For
example, What are their current problems? Are they not able to set
constraints to immediate? Or can they update the trigger function
itself be a security definer function? That might help illuminate why
the current behavior is wrong.

But I feel that the database user that runs the trigger should be the
same user that ran the triggering SQL statement. Even though I cannot
put my hand on a case where changing this user would constitute a real
security problem, it feels wrong.

I am aware that that is rather squishy argumentation, but I have no
better one. Both my and Thomas' gut reaction seems to have been "the
current behavior is wrong".

I understand the gut reaction, and I even have the same gut reaction,
but since we would be treating roles exceptionally compared to the rest
of the execution context, I would feel better if we had a more concrete
reason.

I also took a look at the code. It doesn't apply cleanly to master, so
I took the liberty of rebasing and attaching it.

+ /*
+ * The role could have been dropped since the trigger was queued.
+ * In that case, give up and error out.
+ */
+ pfree(GetUserNameFromId(evtshared->ats_rolid, false));

It feels a bit wasteful to allocate and copy the role name when we
never actually use it. Is it possible to check that the role exists
without copying the name?

Everything else looked good, and the code does what it says it will.

Thanks,
Joe Koshakow

On Sat, Jun 8, 2024 at 2:37 PM Joseph Koshakow <koshy44@gmail.com> wrote:

Something like

`SECURITY INVOKER | SECURITY TRIGGERER` (modeled after the modifiers in
`CREATE FUNCTION`) that control which role is used.

I'm inclined toward this option (except invoker and triggerer are the same
entity, we need owner|definer). I'm having trouble accepting changing the
existing behavior here but agree that having a mode whereby the owner of
the trigger/table executes the trigger function in an initially clean
environment (server/database defaults; the owner role isn't considered as
having logged in so their personalized configurations do not take effect)
(maybe add a SET clause to create trigger too). Security invoker would be
the default, retaining current behavior for upgrade/dump+restore.

Security definer on the function would take precedence as would its set
clause.

David J.

On Sat, Jun 22, 2024 at 6:23 PM David G. Johnston <
david.g.johnston@gmail.com> wrote:

except invoker and triggerer are the same entity

Maybe "executor" would have been a better term than 'invoker". In this
specific example they are not the same entity. The trigger is
triggered and queued by one role and executed by a different role,
hence the confusion. Though I agree with Laurenz, special SQL syntax
for this exotic corner case is a little too much.

Security definer on the function would take precedence as would its set

clause.

These trigger options seem a bit redundant with the equivalent options
on the function that is executed by the trigger. What would be the
advantages or differences of setting these options on the trigger
versus the function?

Thanks,
Joe Koshakow

On Sat, Jun 22, 2024 at 7:21 PM Joseph Koshakow <koshy44@gmail.com> wrote:

On Sat, Jun 22, 2024 at 6:23 PM David G. Johnston <
david.g.johnston@gmail.com> wrote:

except invoker and triggerer are the same entity

Maybe "executor" would have been a better term than 'invoker". In this
specific example they are not the same entity. The trigger is
triggered and queued by one role and executed by a different role,
hence the confusion.

No matter what we label the keyword it would be represent the default and
existing behavior whereby the environment at trigger resolution time, not
trigger enqueue time, is used.

I suppose there is an argument for capturing and reapplying the trigger
enqueue time environment and giving that a keyword as well. But fewer
options has value and security definer seems like the strictly superior
option.

Though I agree with Laurenz, special SQL syntax
for this exotic corner case is a little too much.

It doesn't seem like a corner case if we want to institute a new
recommended practice that all triggers should be created with security
definer. We seem to be discussing that without giving the dba a choice in
the matter - but IMO we do want to provide the choice and leave the default.

Security definer on the function would take precedence as would its set

clause.

These trigger options seem a bit redundant with the equivalent options
on the function that is executed by the trigger. What would be the
advantages or differences of setting these options on the trigger
versus the function?

At least security definer needs to take precedence as the function owner is
fully expecting their role to be the one executing the function, not
whomever the trigger owner might be.

If putting a set clause on the trigger is a thing then the same thing goes
- the function author, if they also did that, expects their settings to be
in place. Whether it really makes sense to have trigger owner set
configuration when they attach the function is arguable but also the most
flexible option.

David J.

On Sat, 2024-06-22 at 20:13 -0700, David G. Johnston wrote:

[bikeshedding discussion about SQL syntax]

Sure, something like CREATE TRIGGER ... USING {INVOKER|CURRENT} ROLE
orsimilar would work, but think that this discussion is premature
at this point. If we have syntax to specify the behavior
of deferred triggers, that needs a new column in "pg_trigger", support
in pg_get_triggerdef(), pg_dump, pg_upgrade etc.

All that is possible, but keep in mind that we are talking about corner
case behavior. To the best of my knowledge, nobody has even noticed the
difference in behavior up to now.

I think that we should have some consensus about the following before
we discuss syntax:

- Does anybody depend on the current behavior and would be hurt if
my current patch went in as it is?

- Is this worth changing at all or had we better document the current
behavior and leave it as it is?

Concerning the latter, I am hoping for a detailed description of our
customer's use case some time soon.

Yours,
Laurenz Albe

On Wed, Jun 26, 2024 at 2:02 AM Laurenz Albe <laurenz.albe@cybertec.at>
wrote:

I think that we should have some consensus about the following before
we discuss syntax:

- Does anybody depend on the current behavior and would be hurt if
my current patch went in as it is?

- Is this worth changing at all or had we better document the current
behavior and leave it as it is?

Concerning the latter, I am hoping for a detailed description of our
customer's use case some time soon.

We have a few choices then:
1. Status quo + documentation backpatch
2. Change v18 narrowly + documentation backpatch
3. Backpatch narrowly (one infers the new behavior after reading the
existing documentation)
4. Option 1, plus a new v18 owner-execution mode in lieu of the narrow
change to fix the POLA violation

I've been presenting option 4.

Pondering further, I see now that having the owner-execution mode be the
only way to avoid the POLA violation in deferred triggers isn't great since
many triggers benefit from the implied security of being able to run in the
invoker's execution context - especially if the trigger doesn't do anything
that PUBLIC cannot already do.

So, I'm on board with option 2 at this point.

David J.

On Wed, 2024-06-26 at 07:38 -0700, David G. Johnston wrote:

We have a few choices then:
1. Status quo + documentation backpatch
2. Change v18 narrowly + documentation backpatch
3. Backpatch narrowly (one infers the new behavior after reading the existing documentation)
4. Option 1, plus a new v18 owner-execution mode in lieu of the narrow change to fix the POLA violation

I've been presenting option 4.

Pondering further, I see now that having the owner-execution mode be the only way to avoid
the POLA violation in deferred triggers isn't great since many triggers benefit from the
implied security of being able to run in the invoker's execution context - especially if
the trigger doesn't do anything that PUBLIC cannot already do.

So, I'm on board with option 2 at this point.

Nice.

I think we can safely rule out option 3.
Even if it is a bug, it is not one that has bothered anybody so far that a backpatch
is indicated.

Yours,
Laurenz Albe

Hi,

Allow me to provide some background on how we came across this.

(This is my first time posting to a pgsql list so hopefully I've got
everything set up correctly.)

We have a db with a big legacy section that we're in the process of
modernizing. To compensate for some of the shortcomings we have designed
a layer of writable views to better represent the underlying data and
make working with it more convenient. The following should illustrate
what we're doing:

    -- this is the schema containing the view layer.
    create schema api;
    -- and this user is granted access to the api, but not the rest of
the legacy db.
    create role apiuser;
    grant usage on schema api to apiuser;

    -- some dummy objects in the legacy db - poorly laid out and poorly
named.
    create schema legacy;
    create table legacy.stock_base (
        id serial primary key
      , lbl text not null unique
      , num int not null
      -- etc
    );
    create table legacy.stock_extra (
        id int not null unique references legacy.stock_base (id)
      , man text
      -- etc
    );

    -- create the stock view which better names and logically groups
the data.
    create view api.stock as
      select sb.id
           , sb.lbl as description
           , sb.num as quantity
           , se.man as manufacturer
        from legacy.stock_base sb
          left join legacy.stock_extra se using (id);
    -- make it writable so it is easier to work with. use security
definer to allow access to legacy sections.
    create function api.stock_cud() returns trigger language plpgsql
security definer as $$
    begin
        -- insert/update legacy.stock_base and legacy.stock_extra
depending on trigger action, modified fields, etc.
        assert tg_op = 'INSERT'; -- assume insert for example's sake.
        insert into legacy.stock_base (lbl, num) values
(new.description, new.quantity) returning id into new.id;
        insert into legacy.stock_extra (id, man) values (new.id,
new.manufacturer);
        return new;
    end;
    $$;
    create trigger stock_cud
        instead of insert or update or delete on api.stock
        for each row execute function api.stock_cud();

    -- grant the apiuser permission to work with the view.
    grant insert, update, delete on api.stock to apiuser;

    -- insert as superuser - works as expected.
    insert into api.stock (description, quantity, manufacturer) values
('item1', 10, 'manufacturer1');
    -- insert as apiuser - works as expected.
    set role apiuser;
    insert into api.stock (description, quantity, manufacturer) values
('item2', 10, 'manufacturer2');

In some cases there are constraint triggers on the underlying tables to
validate certain states. It is, however, possible for a state to be
temporarily invalid between statements, so long as it is valid at tx
commit. For this reason the triggers are deferred by default. Consider
the following example:

    reset role;
    create function legacy.stock_check_state() returns trigger language
plpgsql as $$
    begin
        -- do some queries to check the state of stock based on
modified rows and error if invalid.
        raise notice 'current_user %', current_user;
        -- dummy validation code.
        perform * from legacy.stock_base sb left join
legacy.stock_extra se using (id) where sb.id = new.id;
        return new;
    end;
    $$;
    create constraint trigger stock_check_state
        after insert or update or delete on legacy.stock_base
        deferrable initially deferred
        for each row execute function legacy.stock_check_state();

Then repeat the inserts:

    -- insert as superuser - works as expected.
    reset role;
    insert into api.stock (description, quantity, manufacturer) values
('item3', 10, 'manufacturer3'); -- NOTICE:  current_user postgres
    -- insert as apiuser - fails.
    set role apiuser;
    insert into api.stock (description, quantity, manufacturer) values
('item4', 10, 'manufacturer4'); -- NOTICE:  current_user apiuser

    -- insert as apiuser, not deferred - works as expected.
    begin;
    set constraints all immediate;
    insert into api.stock (description, quantity, manufacturer) values
('item4', 10, 'manufacturer4'); -- NOTICE:  current_user postgres
    commit;

The insert as apiuser fails when the constraint trigger is deferred, but
works as expected when it is immediate.

Hopefully this helps to better paint the picture. Our workaround
currently is to just make `legacy.stock_check_state()` security definer
as well. As I told Laurenz, we're not looking to advocate for any
specific outcome here. We noticed this strange behaviour and thought it
to be a bug that should be fixed - whatever "fixed" ends up meaning.

Regards,

Bennie Swart

On Sat, 2024-06-22 at 17:50 -0400, Joseph Koshakow wrote:

On Mon, Jun 10, 2024 at 1:00 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:

Like you, I was surprised by the current behavior.  There is a design
principle that PostgreSQL tries to follow, called the "Principle of
least astonishment".  Things should behave like a moderately skilled
user would expect them to.  In my opinion, the current behavior violates
that principle.  Tomas seems to agree with that point of view.

I worry that both approaches violate this principle in different ways.
For example consider the following sequence of events:

    SET ROLE r1;
    BEGIN;
    SET CONSTRAINTS ALL DEFERRED;
    INSERT INTO ...;
    SET ROLE r2;
    SET search_path = '...';
    COMMIT;

I think that it would be reasonable to expect that the triggers execute
with r2 and not r1, since the triggers were explicitly deferred and the
role was explicitly set. It would likely be surprising that the search
path was updated for the trigger but not the role. With your proposed
approach it would be impossible for someone to trigger a trigger with
one role and execute it with another, if that's a desirable feature.

I definitely see your point, although GUC settings and the current
security context are something different.

It would definitely not be viable to put all GUC values in the trigger
state.

So if you say "all or nothing", it would be nothing, and the patch should
be rejected.

I didn't find this strange behavior myself: it was one of our customers
who uses security definer functions for data modifications and has
problems with the current behavior, and I am trying to improve the
situation on their behalf.

Would it be possible to share more details about this use case? For
example, What are their current problems? Are they not able to set
constraints to immediate? Or can they update the trigger function
itself be a security definer function? That might help illuminate why
the current behavior is wrong.

I asked them for a statement, and they were nice enough to write up
/messages/by-id/e89e8dd9-7143-4db8-ac19-b2951cb0c0da@gmail.com

They have a workaround, so the patch is not absolutely necessary for them.

I also took a look at the code. It doesn't apply cleanly to master, so
I took the liberty of rebasing and attaching it.

+ /*
+ * The role could have been dropped since the trigger was queued.
+ * In that case, give up and error out.
+ */
+ pfree(GetUserNameFromId(evtshared->ats_rolid, false));

It feels a bit wasteful to allocate and copy the role name when we
never actually use it. Is it possible to check that the role exists
without copying the name?

If that is a concern (and I can envision it to be), I can improve that.
One option is to copy the guts of GetUserNameFromId(), and another
is to factor out the common parts into a new function.

I'd wait until we have a decision whether we want the patch or not
before I make the effort, if that's ok.

Everything else looked good, and the code does what it says it will.

Thanks for the review!

Yours,
Laurenz Albe