Horton

Re: Wrong security context for deferred triggers?

Pavel Stehule <pavel.stehule@gmail.com>

View thread
From: Pavel Stehule <pavel.stehule@gmail.com>
To: Laurenz Albe <laurenz.albe@cybertec.at>
Cc: pgsql-hackers@lists.postgresql.org
Date: 2024-10-19T04:17:27Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Doc: improve description of which role runs a trigger.

  2. Change role names used in trigger test.

  3. Ensure that AFTER triggers run as the instigating user.

  4. Reverse the search order in afterTriggerAddEvent().

Hi

pá 18. 10. 2024 v 15:24 odesílatel Laurenz Albe <laurenz.albe@cybertec.at>
napsal:

> On Fri, 2024-10-18 at 11:32 +0200, Pavel Stehule wrote:
> > pá 18. 10. 2024 v 10:20 odesílatel Laurenz Albe <
> laurenz.albe@cybertec.at> napsal:
> > > On Fri, 2024-10-18 at 07:47 +0200, Pavel Stehule wrote:
> > > > Without deeper checks I don't like using GetUserNameFromId for
> checking the validity of a role.
> > > >
> > > > Maybe it is better to use own read of syscache or wrap
> SetUserIdAndSecContext to do this check.
> > >
> > > I agree; it was just the simplest way I could make it happen.  It is
> ugly to allocate and
> > > return the user name, since we don't really need it.
> > >
> > > I could write a dedicated function to check the existence of a user.
> >
> > +1
>
> The check is so simple that I didn't write a dedicated function.
> Instead, I put the catalog search into the code directly.
>

ok


>
> > >
> > > The trigger queue exists only in memory, and PostgreSQL tracks
> dependencies
> > > only between persisted objects.  Do you think that I should add a
> sentence
> > > like that to the comment?
> >
> > yes, please. I think so it is not too intuitive. Inside a context of
> this patch
> > it is ok, but without knowledge of this context, can be strange, why
> some role used
> > for trigger can be invalid, although the transaction is not fully
> finished yet.
>
> I tried to improve the patch along these lines.
>
> Attached is a new version.
>

The compilation is without any problems, make check-world is ok

I played with this little bit and I didn't find any problems. This patch
introduces a possible compatibility issue, but from a security perspective
it is a clear improvement.

I did a quick search of documentation and I didn't find any sentence about
identity inside trigger execution.

Maybe we should document so the trigger is executed with the identity used
by DML command always, when the trigger function has no SECURITY DEFINER
flag?

Regards

Pavel





>
> Thanks for the review!
>
> Yours,
> Laurenz Albe
>