Re: [PoC] Federated Authn/z with OAUTHBEARER

Hi all,

Thanks for all the feedback! I'll combine them all into one email:

On Mon, Apr 7, 2025 at 6:59 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> Looks mostly ok. I need the following patch to get it to build on macOS:
> [...]
> (The second change is not strictly required, but it disables the use of
> -bundle_loader postgres, since this is not a backend-loadable module.)

Hm, okay. I'll take a closer look at this, thanks.

> The PKG_CONFIG_REQUIRES_PRIVATE setting in libpq-oauth/Makefile is
> meaningless and can be removed.

Ah, right. Will do.

> In fe-auth-oauth.c, I note that dlerror() is not necessarily
> thread-safe. Since there isn't really an alternative, I guess it's ok
> to leave it like that, but I figured it should be mentioned.

Yeah. The XXX comments there are a reminder to me to lock the stderr
printing behind debug mode, since I hope most non-packaging people are
not going to be troubleshooting load-time errors. But see the
threadlock discussions below.

> Not sure, the code looks correct at first glance. However, you could
> also just keep the libpq-oauth strings in the libpq catalog. There
> isn't really a need to make a separate one, since the versions you end
> up installing are locked to each other. So you could for example in
> libpq's nls.mk just add
>
> ../libpq-oauth/oauth-curl.c
>
> etc. to the files.

Oh, that's an interesting idea. Thanks, I'll give it a try.

> Maybe it would also make sense to make libpq-oauth a subdirectory of the
> libpq directory instead of a peer.

Works for me.

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:
> > +This module ABI is an internal implementation detail, so it's subject to change
> > +without warning, even during minor releases (however unlikely). The compiled
> > +version of libpq-oauth should always match the compiled version of libpq.
>
> Shouldn't we then include the *minor* version in the soname?

No objection here.

> 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.

> Which actually makes me wonder if we ought to instead load the library from a
> specific location...

We could hardcode the disk location for version 1, I guess. I kind of
liked giving packagers the flexibility they're used to having from the
ld.so architecture, though. See below.

> > +# 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.

> 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.

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.

> > diff --git a/src/interfaces/libpq-oauth/meson.build b/src/interfaces/libpq-oauth/meson.build
> > +if not libcurl.found() or host_system == 'windows'
> > + subdir_done()
> > +endif
>
> Why does this not work on windows? I don't see similar code in the removed
> lines?

The Device Authorization flow is not currently implemented on Windows.

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.

> Afaict this library doesn't have unresolved symbols, due to just linking to
> libpq. So I don't think we really need this to be a shared module?

Correct, no unresolved symbols. My naive understanding was that
distros were going to impose restrictions on an SONAME'd library that
we may not want to deal with.

> > But the installation directory of a shared module should not be directly
> > libdir.
>
> Agreed. However, it seems like relocatability would be much more important for
> something like libpq than server modules... Particularly on windows it'll
> often just be shipped alongside the executable - which won't work if we load
> from pklibdir or such.

Yeah, I really like the simplicity of "use the standard runtime
loader, just on-demand." Seems more friendly to the ecosystem.

Are there technical downsides of putting it into $libdir? I understand
there are "people" downsides, since we don't really want to signal
that this is a publicly linkable thing... but surely if you go through
the process of linking our library (which has no SONAME, includes the
major/minor version in its -l option, and has no pkgconfig) and
shoving a private pointer to a threadlock into it, you can keep all
the pieces when they break?

> > 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.)

Are there any downsides to the exports file?

Thanks,
--Jacob