Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 30 Apr 2025 14:55:07 +0200

> On 29 Apr 2025, at 02:10, Jacob Champion <[email-masked]> wrote:
> 
> On Wed, Apr 23, 2025 at 10:46 AM Jacob Champion
> <[email-masked]> wrote:
>> Are there any readers who feel like an internal ABI version for
>> `struct pg_conn`, bumped during breaking backports, would be
>> acceptable? (More definitively: are there any readers who would veto
>> that?)
> 
> To keep things moving: I assume this is unacceptable. So v10 redirects
> every access to a PGconn struct member through a shim, similarly to
> how conn->errorMessage was translated in v9. This adds plenty of new
> boilerplate, but not a whole lot of complexity. To try to keep us
> honest, libpq-int.h has been removed from the libpq-oauth includes.

That admittedly seems like a win regardless.

> This will now handle in-place minor version upgrades that swap pg_conn
> internals around, so I've gone back to -MAJOR versioning alone.
> fe_oauth_state is still exported; it now has an ABI warning above it.
> (I figure that's easier to draw a line around during backports,
> compared to everything in PGconn. We can still break things there
> during major version upgrades.)

While I'm far from the expert on this subject (luckily there are such in this
thread), I am unable to see any sharp edges from reading and testing this
version of the patch. A few small comments:

+libpq-oauth is an optional module implementing the Device Authorization flow for
+OAuth clients (RFC 8628). It was originally developed as part of libpq core and
+later split out as its own shared library in order to isolate its dependency on
+libcurl. (End users who don't want the Curl dependency can simply choose not to
+install this module.)

We should either clarify that it was never shipped as part of libpq core, or
remove this altogether.  I would vote for the latter since we typically don't
document changes that happen during the devcycle.  How about something like:

+libpq-oauth is an optional module implementing the Device Authorization flow for
+OAuth clients (RFC 8628). It is maintained as its own shared library in order to
+isolate its dependency on libcurl. (End users who don't want the Curl dependency
+can simply choose not to install this module.)

+- void libpq_oauth_init(pgthreadlock_t threadlock,
+             <snip>
+At the moment, pg_fe_run_oauth_flow() relies on libpq's pg_g_threadlock and
+libpq_gettext(), which must be injected by libpq using this initialization
+function before the flow is run.

I think this explanatory paragraph should come before the function prototype.
The following paragraph on the setters/getters make sense where it is though.

+#if defined(USE_DYNAMIC_OAUTH) && defined(LIBPQ_INT_H)
+#error do not rely on libpq-int.h in libpq-oauth.so
+#endif

Nitpick, but it won't be .so everywhere.  Would this be clearar if spelled out
with something like "do not rely on libpq-int.h when building libpq-oauth as
dynamic shared lib"?

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Wed, 30 Apr 2025 10:59:43 -0700

> That admittedly seems like a win regardless.

Yeah, it moves us much closer to the long-term goal.

> remove this altogether.

Done in v11, with your suggested wording.

> I think this explanatory paragraph should come before the function prototype.

Done.

> with something like "do not rely on libpq-int.h when building libpq-oauth as

I went with "do not rely on libpq-int.h in dynamic builds of
libpq-oauth", since devs are hopefully going to be the only people who
see it. I've also fixed up an errant #endif label right above it.

I'd ideally like to get a working split in for beta. Barring
objections, I plan to get this pushed tomorrow so that the buildfarm
has time to highlight any corner cases well before the Saturday
freeze. I still see the choice of naming (with its forced-ABI break
every major version) as needing more scrutiny, and probably worth a
Revisit entry.

The CI still looks happy, and I will spend today with VMs and more
testing on the Autoconf side. I'll try to peer at Alpine and musl
libc, too; dogfish and basilisk are the Curl-enabled animals that
caught my attention most.

Thanks!
--Jacob
1:  e86e93f7ac8 ! 1:  5a1d1345919 oauth: Move the builtin flow into a separate module
    @@ Commit message
     
         Per request from Tom Lane and Bruce Momjian. Based on an initial patch
         by Daniel Gustafsson, who also contributed docs changes. The "bare"
    -    dlopen() concept came from Thomas Munro. Many many people reviewed the
    -    design and implementation; thank you!
    +    dlopen() concept came from Thomas Munro. Many people reviewed the design
    +    and implementation; thank you!
     
         Co-authored-by: Daniel Gustafsson <[email-masked]>
         Reviewed-by: Andres Freund <[email-masked]>
         Reviewed-by: Christoph Berg <[email-masked]>
    +    Reviewed-by: Daniel Gustafsson <[email-masked]>
         Reviewed-by: Jelte Fennema-Nio <[email-masked]>
         Reviewed-by: Peter Eisentraut <[email-masked]>
         Reviewed-by: Wolfgang Walther <[email-masked]>
    @@ src/interfaces/libpq-oauth/Makefile (new)
      ## src/interfaces/libpq-oauth/README (new) ##
     @@
     +libpq-oauth is an optional module implementing the Device Authorization flow for
    -+OAuth clients (RFC 8628). It was originally developed as part of libpq core and
    -+later split out as its own shared library in order to isolate its dependency on
    -+libcurl. (End users who don't want the Curl dependency can simply choose not to
    -+install this module.)
    ++OAuth clients (RFC 8628). It is maintained as its own shared library in order to
    ++isolate its dependency on libcurl. (End users who don't want the Curl dependency
    ++can simply choose not to install this module.)
     +
     +If a connection string allows the use of OAuth, and the server asks for it, and
     +a libpq client has not installed its own custom OAuth flow, libpq will attempt
    @@ src/interfaces/libpq-oauth/README (new)
     +pg_fe_run_oauth_flow and pg_fe_cleanup_oauth_flow are implementations of
     +conn->async_auth and conn->cleanup_async_auth, respectively.
     +
    ++At the moment, pg_fe_run_oauth_flow() relies on libpq's pg_g_threadlock and
    ++libpq_gettext(), which must be injected by libpq using this initialization
    ++function before the flow is run:
    ++
     +- void libpq_oauth_init(pgthreadlock_t threadlock,
     +						libpq_gettext_func gettext_impl,
     +						conn_errorMessage_func errmsg_impl,
    @@ src/interfaces/libpq-oauth/README (new)
     +						set_conn_altsock_func setaltsock_impl,
     +						set_conn_oauth_token_func settoken_impl);
     +
    -+At the moment, pg_fe_run_oauth_flow() relies on libpq's pg_g_threadlock and
    -+libpq_gettext(), which must be injected by libpq using this initialization
    -+function before the flow is run.
    -+
     +It also relies on access to several members of the PGconn struct. Not only can
     +these change positions across minor versions, but the offsets aren't necessarily
     +stable within a single minor release (conn->errorMessage, for instance, can
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c => src/interfaces/libpq-oauth/oauth-cu
     +#define set_conn_altsock(CONN, VAL) do { CONN->altsock = VAL; } while (0)
     +#define set_conn_oauth_token(CONN, VAL) do { CONN->oauth_token = VAL; } while (0)
     +
    -+#endif							/* !USE_DYNAMIC_OAUTH */
    ++#endif							/* USE_DYNAMIC_OAUTH */
     +
     +/* One final guardrail against accidental inclusion... */
     +#if defined(USE_DYNAMIC_OAUTH) && defined(LIBPQ_INT_H)
    -+#error do not rely on libpq-int.h in libpq-oauth.so
    ++#error do not rely on libpq-int.h in dynamic builds of libpq-oauth
     +#endif
      
      /*
    @@ src/interfaces/libpq-oauth/oauth-utils.c (new)
     +#endif
     +
     +#ifdef LIBPQ_INT_H
    -+#error do not rely on libpq-int.h in libpq-oauth
    ++#error do not rely on libpq-int.h in dynamic builds of libpq-oauth
     +#endif
     +
     +/*

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 30 Apr 2025 20:09:28 +0200

> On 30 Apr 2025, at 19:59, Jacob Champion <[email-masked]> wrote:

> see it. I've also fixed up an errant #endif label right above it.

That's indeed better than my suggestion.

> I'd ideally like to get a working split in for beta.

+Many

> Barring
> freeze.

I'll try to kick the tyres a bit more as well.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Thu, 1 May 2025 10:38:15 -0700

> I'll try to kick the tyres a bit more as well.

Thanks! Alpine seems to be happy with the dlopen() arrangement. And
I've thrown some more Autoconf testing at Rocky, Mac, and Ubuntu.

So, committed. Thanks everyone for all the excellent feedback!
(Further feedback is still very welcome.)

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 1 May 2025 12:24:19 -0700

On Thu, May 1, 2025 at 10:38 AM Jacob Champion
<[email-masked]> wrote:
> So, committed.

I forgot --enable-nls in my Mac testing, so indri complains about my
omission of -lintl... I'd incorrectly thought it was no longer needed
after all the gettext motion.

I'm running the attached fixup through CI now.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 1 May 2025 13:31:05 -0700

> I'm running the attached fixup through CI now.

(Pushed, and indri is happy again.)

--Jacob

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 2 May 2025 10:11:08 -0500

After commit b0635bf, I'm seeing the following meson build failures on
macOS:

	In file included from ../postgresql/src/interfaces/libpq-oauth/oauth-curl.c:51:
	../postgresql/src/interfaces/libpq/libpq-int.h:70:10: fatal error: 'openssl/ssl.h' file not found
	   70 | #include <openssl/ssl.h>
	      |          ^~~~~~~~~~~~~~~
	1 error generated.

The following patch seems to resolve it.  I'm curious if commit 4ea1254
might apply to meson, too, but FWIW I haven't noticed any related failures
on my machine.

diff --git a/meson.build b/meson.build
index 29d46c8ad01..19ad03042d3 100644
--- a/meson.build
+++ b/meson.build
@@ -3295,6 +3295,7 @@ libpq_deps += [

 libpq_oauth_deps += [
   libcurl,
+  ssl,
 ]

 subdir('src/interfaces/libpq')

-- 
nathan

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 08:46:21 -0700

> After commit b0635bf, I'm seeing the following meson build failures on

Thanks for the report, and sorry for the breakage.

>            70 | #include <openssl/ssl.h>

Hm. My test setup here is Homebrew with -Dextra_include_dirs, which
may explain why it's not failing for me. Looks like Cirrus also has
-Dextra_include_dirs...

> might apply to meson, too, but FWIW I haven't noticed any related failures

Yeah, I wonder if libintl is being similarly "cheated" on the Meson side.

> @@ -3295,6 +3295,7 @@ libpq_deps += [

Thanks! I think the include directory is the only thing needed for the
static library, not the full link dependency. But I'll try to build up
a new Meson setup, with minimal added settings, to see if I can
reproduce here. Can you share your Meson configuration?

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 08:59:18 -0700

> Yeah, I wonder if libintl is being similarly "cheated" on the Meson side.

libintl is already coming in via frontend_stlib_code, so that's fine.
So now I'm wondering if any other static clients of libpq-int.h (if
there are any) need the ssl dependency too, for correctness, or if
it's just me.

> But I'll try to build up

(Never mind -- this was pretty easy to hit in a from-scratch configuration.)

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 10:05:26 -0700

> there are any) need the ssl dependency too, for correctness, or if

Looks like it's just me. And using partial_dependency for the includes
seems like overkill, so I've kept the full ssl dependency object, but
moved it to the staticlib only, which is enough to solve the breakage
on my machine.

Nathan, if you get a chance, does the attached patch work for you?

Thanks!
--Jacob

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 2 May 2025 12:31:27 -0500

> Nathan, if you get a chance, does the attached patch work for you?

Yup, thanks!

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Fri, 02 May 2025 13:35:30 -0400

> Nathan, if you get a chance, does the attached patch work for you?

FWIW, on my Mac a meson build from HEAD goes through fine, with or
without this patch.  I'm getting openssl and libcurl from MacPorts
not Homebrew, but I don't know why that would make any difference.

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 10:42:25 -0700

> not Homebrew, but I don't know why that would make any difference.

Do your <libintl.h> and <openssl/*.h> live in the same place? Mine do,
so I had to disable NLS to get this to break.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 10:42:34 -0700

> Yup, thanks!

Great, thanks. I'll push it soon.

--Jacob

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 2 May 2025 12:59:12 -0500

> so I had to disable NLS to get this to break.

I enabled NLS and the problem disappeared for me, but that seems to be a
side effect of setting -Dextra_{include,lib}_dirs to point to my Homebrew
directories, which I needed to do to get NLS to work.

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Fri, 02 May 2025 14:25:49 -0400

> so I had to disable NLS to get this to break.

Yeah, they are both under /opt/local/include in a MacPorts setup.
But disabling NLS doesn't break it for me.  I tried

meson setup build --auto-features=disabled -Dlibcurl=enabled

to make sure that /opt/local/include wasn't getting pulled in
some other way, and it still builds.

Apropos of that: our fine manual claims that option is spelled
--auto_features, but that fails for me.  Is that a typo in our
manual, or do some meson versions accept the underscore?

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 11:46:15 -0700

> some other way, and it still builds.

Hm. If you clear out the build artifacts under
src/interfaces/libpq-oauth, and then build with

    $ ninja -v src/interfaces/libpq-oauth/libpq-oauth.a

does that help surface anything interesting?

> manual, or do some meson versions accept the underscore?

--auto_features doesn't work for me either. That looks like an
accidental mashup of --auto-features and -Dauto_features.

--Jacob

---

From: Tom Lane <[email-masked]>
Date: Fri, 02 May 2025 14:52:36 -0400

> does that help surface anything interesting?

$ rm -rf src/interfaces/libpq-oauth
$ ninja -v src/interfaces/libpq-oauth/libpq-oauth.a
[2/2] rm -f src/interfaces/libpq-oauth/libpq-oauth.a && ar csr src/interfaces/libpq-oauth/libpq-oauth.a src/interfaces/libpq-oauth/libpq-oauth.a.p/oauth-curl.c.o && ranlib -c src/interfaces/libpq-oauth/libpq-oauth.a

So it's getting -I/opt/local/include and also
-I/opt/local/libexec/openssl3/include from somewhere,
which I guess must be libcurl's pkg-config data ... yup:

$ pkg-config --cflags libcurl
-I/opt/local/include -I/opt/local/libexec/openssl3/include -I/opt/local/include

I bet Homebrew's libcurl packaging doesn't do that.

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 11:56:46 -0700

> I bet Homebrew's libcurl packaging doesn't do that.

Nope, Homebrew breaks them out into smaller pieces:

    % PKG_CONFIG_PATH=/opt/homebrew/opt/curl/lib/pkgconfig pkg-config
--cflags libcurl
    -I/opt/homebrew/Cellar/curl/8.13.0/include
-I/opt/homebrew/Cellar/brotli/1.1.0/include
-I/opt/homebrew/opt/zstd/include
-I/opt/homebrew/Cellar/libssh2/1.11.1/include
-I/opt/homebrew/Cellar/rtmpdump/2.4-20151223_3/include
-I/opt/homebrew/Cellar/openssl@3/3.5.0/include
-I/opt/homebrew/Cellar/libnghttp2/1.65.0/include

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 12:05:10 -0700

> -I/opt/homebrew/Cellar/openssl@3/3.5.0/include

Except it _is_ right there.

Oh, ha -- I'm not using Homebrew's Curl in this minimal build. Looks
like it's coming from the sysroot.

    % ls -l /Library/Developer/CommandLineTools/SDKs/MacOSX15.2.sdk/usr/include/curl
    total 208
    -rw-r--r--  1 root  wheel  129052 Nov  9 22:54 curl.h
    -rw-r--r--  1 root  wheel    3044 Nov  9 22:54 curlver.h
    ...

Well, that was fun.

--Jacob

---

From: Tom Lane <[email-masked]>
Date: Fri, 02 May 2025 15:05:26 -0400

> accidental mashup of --auto-features and -Dauto_features.

Ah, I see somebody already complained of this [1], but apparently
we did nothing about it.  I shall go fix it now.

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/172465652540.862882.17808523044292761256%40wrigleys.postgresql.org

---

From: Christoph Berg <[email-masked]>
Date: Sat, 3 May 2025 16:54:59 +0200

Re: Jacob Champion
> So, committed. Thanks everyone for all the excellent feedback!

The package split between libpq5 and libpq-oauth in Debian has already
been accepted into the experimental branch.

Thanks,
Christoph

---

From: Wolfgang Walther <[email-masked]>
Date: Sun, 4 May 2025 14:58:48 +0200

> Nathan, if you get a chance, does the attached patch work for you?

I couldn't reproduce the problem, so did not test the latest patch. But 
I tested a lot of scenarios on nixpkgs with latest master (250a718a):

- aarch64 + x86_64 architectures, both Linux and MacOS

- Autoconf and Meson

- Various features enabled / disabled in different configurations (NLS, 
OpenSSL, GSSAPI)

- And additionally some cross-compiling from x86_64 Linux to aarch64 
Linux and x86_64 FreeBSD

Worked very well.

The only inconsistency I was able to find is the autoconf-generated 
libpq.pc file, which has this:

   Requires.private: libssl, libcrypto libcurl

Note the missing "," before libcurl.

It does *not* affect functionality, though:

   pkg-config --print-requires-private libpq
   libssl
   libcrypto
   libcurl

The meson-generated libpq.pc looks like this:

   Requires.private: openssl, krb5-gssapi, libcurl >=  7.61.0

I was only able to test the latter in an end-to-end fully static build 
of a downstream dependency - works great. The final executable has all 
the expected oauth strings in it.

Best,

Wolfgang

---

From: Devrim Gündüz <[email-masked]>
Date: Mon, 05 May 2025 15:37:45 +0300

Hi,

> been accepted into the experimental branch.

RPMs will ship postgresql18-libs and postgresql18-libs-oauth. The latter
depends on the former for sure.

Regards,

-- 
Devrim Gündüz
Open Source Solution Architect, PostgreSQL Major Contributor
BlueSky: @devrim.gunduz.org , @gunduz.org

---

From: Jacob Champion <[email-masked]>
Date: Tue, 6 May 2025 10:49:31 -0700

>    Requires.private: libssl, libcrypto libcurl

Oh, I see what I did. Will fix, thanks.

> the expected oauth strings in it.

Thank you so much for all the detailed testing!

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Mon, 4 Aug 2025 16:20:29 -0700

On Wed, Apr 30, 2025 at 10:59 AM Jacob Champion
<[email-masked]> wrote:
>
> I still see the choice of naming (with its forced-ABI break

It is now time to revisit.

= The Status Quo =

The libpq-oauth module is loaded on-demand, during the first use of
OAuth authentication, so users who don't want the behavior don't have
to install it. This module is named "libpq-oauth-18.so" for the PG18
release. So libpq v18 will always load the 18 OAuth behavior, libpq
v19 will load the 19 OAuth behavior, etc. Builds on HEAD have already
switched to -19, which is not yet any different from -18.

The internal API injects some libpq internals into the libpq-oauth
module. The ABI for this is assumed to break during each major version
release, so I don't have to watch the boundary like a hawk, and other
maintainers hopefully won't be saddled with breakage reports if I get
hit by a bus. (This is another advantage to using the -MAJOR naming
scheme.) And pg_conn in particular is given more protections: we can
still change its member offsets in minor versions without any ABI
breakage.

During major-version upgrades, if a packager doesn't provide a
side-by-side installation of the -18 and -19 modules, there is a
hazard: an already-loaded v18 libpq might find that the -18 module no
longer exists on disk, which would require a restart of the affected
application to pick up the v19 libpq. This is not really a consequence
of the -MAJOR naming scheme -- it's a consequence of delay-loaded
libraries that go through an ABI version bump -- but the naming scheme
makes the problem extremely visible.

The annoying part is that, if 19 doesn't change anything in the OAuth
flow compared to 18, I will basically have made busywork for our
packagers for no reason. But my goal for v19 is to replace the
internally coupled API with a public API, so that users can swap in
their own flows for use with our utilities. As far as I know, that
work necessarily includes designing a stable ABI and figuring out a
trusted place that users can put their plugins into. If we can do
both, I think we can get rid of the -MAJOR versioning scheme entirely,
because our use case will have been subsumed by the more general
framework.

So, as we approach Beta 3: can anyone think of a way that this plan will fail?

Thanks,
--Jacob

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Tue, 5 Aug 2025 11:39:10 +0200

On Tue, 5 Aug 2025 at 01:20, Jacob Champion
<[email-masked]> wrote:
> As far as I know, that

It's not entirely clear what plan exactly you talk about here. Are you
saying you want to remove the -MAJOR suffix now for PG18? Or you want
to postpone doing that until PG19, when you would have designed a
stable API?

Based on my current understanding from what you wrote, I think that
second option would make sense, and the first option seems sketchy.
Because we don't know yet what the PG19 API will look like.

Also, the breakage during libpq major upgrades that you describe,
while unfortunate, doesn't seem completely terrible. This only impacts
people updating system packages in place on machines, which (based on
my experience) has started to become a minority in production setups.
Also this will obviously only impact oauth users, which I expect not
to be that many right away. If your goal is to remove this
during-upgrade breakage after PG19, then I'd say that seems totally
fine for a new feature.

---

From: Jacob Champion <[email-masked]>
Date: Tue, 5 Aug 2025 08:21:14 -0700

> to postpone doing that until PG19, when you would have designed a

That is a PG19 plan. I don't want to make any changes for 18 unless
someone can see a fatal flaw; this is just my mid-beta check.

> If your goal is to remove this

That's the hope, yes.

Thanks!
--Jacob

---

From: Christoph Berg <[email-masked]>
Date: Tue, 5 Aug 2025 17:44:15 +0200

Re: Jacob Champion
> someone can see a fatal flaw; this is just my mid-beta check.

FTR, fine with me.

> > during-upgrade breakage after PG19, then I'd say that seems totally

Well, at the PG19 release time it will no longer be new. But the plan
sounds like a good one.

Christoph

---

From: Jacob Champion <[email-masked]>
Date: Thu, 7 Aug 2025 13:52:53 -0700

> Well, at the PG19 release time it will no longer be new. But the plan

Excellent. Thanks everybody!

--Jacob