Re: Wrong security context for deferred triggers? - Mailing list pgsql-hackers

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