Re: [PATCH] Expose port->authn_id to extensions and triggers

Greetings,

* Michael Paquier (michael(at)paquier(dot)xyz) wrote:
> On Mon, Feb 28, 2022 at 04:00:36PM -0500, Stephen Frost wrote:
> > * Jacob Champion (pchampion(at)vmware(dot)com) wrote:
> >> On Fri, Feb 25, 2022 at 01:23:49PM -0800, Andres Freund wrote:
> >>> Looks to me like authn_id isn't synchronized to parallel workers right now. So
> >>> the function will return the wrong thing when executed as part of a parallel
> >>> query.
> >>
> >> Thanks for the catch. It looks like MyProcPort is left empty, and other
> >> functions that rely on like inet_server_addr() are marked parallel-
> >> restricted, so I've done the same in v4.
> >
> > That's probably alright.
>
> I'd say as well that this is right as-is. If it happens that there is
> a use-case for making this parallel aware in the future, it could be
> done. Now, it may be a bit weird to make parallel workers inherit the
> authn ID of the parent as these did not go through authentication, no?
> Letting this function being run only by the leader feels intuitive.

I'm not really sure why we're arguing about this, but clearly the authn
ID of the leader process is what should be used because that's the
authentication under which the parallel worker is running, just as much
as the effective role is the authorization. Having this be available in
worker processes would certainly be good as it would allow more query
plans to be considered when these functions are used. At this time, I
don't think that outweighs the complications around having it and I'm
not suggesting that Jacob needs to go do that, but surely it would be
better.

> >> Subject: [PATCH v4] Add API to retrieve authn_id from SQL
> >>
> >> The authn_id field in MyProcPort is currently only accessible to the
> >> backend itself. Add a SQL function, pg_session_authn_id(), to expose
> >> the field to triggers that may want to make use of it.
> >
> > Only did a quick look but generally looks reasonable to me.
>
> The function and the test are fine, pgperltidy complains a bit about
> the format of the tests.
>
> Ayway, this function needs to be documented. I think that you should
> just add that in "Session Information Functions" in func.sgml, same
> area as current_user(). The last time we talked about the authn ID,
> one thing we discussed about was how to describe that in a good way to
> the user, which is why the section of log_connections was reworked a
> bit. And we don't have yet any references to what an authenticated
> identity is in the docs.

Agreed that it should be documented and that location seems reasonable
to me.

> There is no need to update catversion.h in the patch, committers
> usually take care of that and that's an area of the code that
> conflicts a lot.

Yeah, best to let committers handle catversion bumps.

Thanks,

Stephen