Re: Wrong security context for deferred triggers?

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
https://postgr.es/m/e89e8dd9-7143-4db8-ac19-b2951cb0c0da%40gmail.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