Re: [PoC] Federated Authn/z with OAUTHBEARER

Hi,

On 2025-04-07 09:41:25 -0700, Jacob Champion wrote:
> On Mon, Apr 7, 2025 at 7:21 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2025-04-04 17:27:46 -0700, Jacob Champion wrote:
> > I think otherwise
> > we run into the danger of the wrong library version being loaded in some
> > cases. Imagine a program being told with libpq to use via rpath. But then we
> > load the oauth module via a dlopen without a specified path - it'll just
> > search the global library locations.
>
> Ah, you mean if the RPATH'd build doesn't have a flow, but the
> globally installed one (with a different ABI) does? Yeah, that would
> be a problem.

That and more: Even if the RPATH libpq does have oauth support, the
libpq-oauth won't necessarily be at the front of the global library search
path. So afaict you'll often get a different libpq-oauth.

Except perhaps on macos, where all this stuff works differently again. But I
managed to unload the required knowledge out of my brain and don't want to
further think about that :)

> > > +# src/interfaces/libpq-oauth/exports.txt
> > > +pg_fe_run_oauth_flow 1
> > > +pg_fe_cleanup_oauth_flow 2
> > > +pg_g_threadlock 3
> >
> > The pg_g_threadlock thing seems pretty ugly. Can't we just pass that to the
> > relevant functions?
>
> We can do it however we want, honestly, especially since the ABI isn't
> public/stable. I chose this way just to ease review.

I found it rather confusing that libpq looks up a symbol and then sets
libpq-oauth's symbol to a pointer in libpq's namespace.

> > But more fundamentally, are we actually using
> > pg_g_threadlock anywhere? I removed all the releant code and the tests still
> > seem to pass?
>
> If you have an older Curl installation, it is used. Newer libcurls
> don't need it.

Oh, wow. We hide the references to pg_g_threadlock behind a friggin macro?
That's just ...

Not your fault, I know...

> A future simplification could be to pull the use of the threadlock
> back into libpq, and have it perform a one-time
> dlopen-plus-Curl-initialization under the lock... That would also get
> rid of the dlerror() thread safety problems. But that's an awful lot
> of moving parts under a mutex, which makes me a little nervous.

I still think we should simply reject at configure time if curl init isn't
threadsafe ;)

> On Mon, Apr 7, 2025 at 7:43 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > Yes, this is correct. We want a shared module, not a shared library, in
> > > meson parlance.
> >
> > It's not entirely obvious to me that we do want that.
> >
> > There recently was a breakage of building with PG on macos with meson, due to
> > the meson folks implementing a feature request to move away from using
> > bundles, as
> > 1) bundles apparently aren't supported on iOS
> > 2) there apparently aren't any restrictions left that require using bundles,
> > and there haven't been for a while.
>
> Could you explain how this is related to .app bundles? I thought I was
> just building a standard dylib.

The other kind of bundles (what on earth apple was thinking with the naming
here I don't know). Stuff liked with ld -bundle.

> > > I don't know whether we need an exports file for libpq-oauth. The other
> > > shared modules don't provide an export file, and I'm not sure whether this
> > > is even supported for shared modules. I guess it doesn't hurt?
> >
> > It does seem just using PGDLLEXPORT would suffice here.
>
> My motivation was to strictly identify the ABI that we intend libpq to
> use, to try to future-proof things for everybody. Especially since
> we're duplicating functions from libpq that we'd rather not be. (The
> use of RTLD_LOCAL maybe makes that more of a belt-and-suspenders
> thing.)

PGDLLEXPORT serves that purpose too, fwiw. These days we use compiler flags
that restrict function visibility of everything not annotated with
PGDLLEXPORT.

However - that's all in c.h, port/win32.h,port/cygwin.h, , which libpq headers
might not want to include.

> Are there any downsides to the exports file?

I think it's fine either way.

Greetings,

Andres Freund