Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: [oauth] Stabilize the libpq-oauth ABI (and allow alternative
 implementations?)
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAOYmi+mrGg+n_X2MOLgeWcj3v_M00gR8uz_D7mM8z=dX1JYVbg@mail.gmail.com>
Date: Fri, 12 Dec 2025 15:42:25 +0800
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <3720B2E1-0B96-4063-8D63-B5AE6AFEA159@gmail.com>
References: 
 <CAOYmi+mrGg+n_X2MOLgeWcj3v_M00gR8uz_D7mM8z=dX1JYVbg@mail.gmail.com>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Archived-At: 
 <https://www.postgresql.org/message-id/3720B2E1-0B96-4063-8D63-B5AE6AFEA159%40gmail.com>
Precedence: bulk


> On Dec 10, 2025, at 05:00, Jacob Champion =
<jacob.champion@enterprisedb.com> wrote:
>=20
> Hi everybody,
>=20
> We introduced the libpq-oauth module late in the cycle for PG18, and
> to put it mildly, its interface isn't great. The original
> implementation depended on libpq internals, and we had to make sure
> that we didn't start crashing during major or minor version upgrades.
> So there were a bunch of compromises made to keep things safe,
> including the restriction that the module name has to change every
> major release.
>=20
> Separately, but closely related: PG18's OAuth support allows you to
> customize the client flow via a libpq hook function. Third-party
> applications can make use of that, but our own utilities can still
> only use the builtin device flow. That's annoying.
>=20
> I've been working to replace the internal ABI with a stable one,
> hopefully to solve both problems at the same time. A dlopen() is a
> pretty clear seam for other people to use to modify and extend.
> Unfortunately my first attempt (not pictured) ended up in a rabbit
> hole, because I tried to tackle the third-party use case first. My
> second attempt, attached, focuses on the ABI stabilization instead,
> which I think is more likely to succeed.
>=20
> (This took enough thinking that I'm really glad we didn't try this for
> PG18. Thanks for letting me take on some technical debt for a bit.)
>=20
> =3D Design =3D
>=20
> Here's the train of thought behind the core changes (which are in =
patch 0004):
>=20
> The builtin-flow code in fe-auth-oauth.c is similar to the custom-flow
> code, but it's just ever-so-slightly different. I'd like to unify the
> two, so I want libpq-oauth to make use of the public
> PGoauthBearerRequest API, and that means that almost all of the
> injections made in the PG18 ABI need to be replaced.
>=20
> Most of those injections are simply subsumed by the hook API
> (hooray!). A couple of others can be replaced by PQconninfo(). Four
> are left over:
> - pgthreadlock_t
> - libpq_gettext
> - conn->errorMessage
> - conn->oauth_issuer_id
>=20
> I think we should keep injecting libpq_gettext; no third-party
> implementations would be able to use that. And application hooks are
> presumably capable of figuring out top-level locking already, since
> the application has to have called PQregisterThreadLock() if it needed
> to coordinate with libpq.
>=20
> That leaves error messages and the issuer identifier. I think both
> would be useful for hooks to have, too, so I'd like to add them to
> PQAUTHDATA_OAUTH_BEARER_TOKEN.
>=20
> =3D PQAUTHDATA_OAUTH_BEARER_TOKEN, version 2 =3D
>=20
> My original plan for authdata extensions was to add new members to the
> end of the structs that libpq passes into the hook. Applications would
> gate on a feature macro during compilation to see whether they could
> use the new members. That should work fine for an application hook;
> you're not allowed to downgrade libpq past the version that your
> applications are compiled against, lest you lose symbols (or other
> feature-flag functionality) you're relying on.
>=20
> Plugins, unfortunately, can't rely on the feature macro. As we found
> out during the libpq-oauth split [1], we have to handle a long-running
> application with an old libpq that loads an upgraded libpq-oauth, even
> if the OS package dependencies suggest otherwise. (A plugin
> architecture flips the direction of the runtime dependency arrow.)
>=20
> There are a couple ways we could handle this. For this draft, I've
> implemented what I think is a middle ground between verbosity and
> type-safety: introduce a new V2 struct that "inherits" the V1 struct
> and can be down-cast in the callbacks, kinda similar to our Node
> hierarchy. We could go even more verbose, and duplicate the entire
> PGoauthBearerToken struct -- matching the callback parameter types for
> maximum safety -- but I'm not convinced that this would be a good use
> of maintenance effort. The ability to cast between the two means we
> can share v1 and v2 implementations in our tests.
>=20
> We could also just add the new members at the end, say that you're
> only allowed to use them if the V2 hook type is in use, and scribble
> on them in V1 hooks to try to get misbehaving implementations to crash
> outright. This arguably has the same amount of type-safety as the
> downcast, and the resulting code looks nicer. (The libcurl API we use
> does something similar with curl_version_info().) But it is definitely
> more "magic".
>=20
> Also of note: this adds a PQExpBuffer to libpq-fe.h. Technically, that
> type is "internal". But... is it really, though? It doesn't seem
> possible for us to make incompatible changes there without crashing
> earlier psqls, in which case I would like to make use of it too. Maybe
> this deserves its own minithread.
>=20
> Okay, on to the full patchset.
>=20
> =3D Roadmap: Prep =3D
>=20
> The first few patches are bugfixes I intend to backpatch to 18.
>=20
> - 0001: I stomped on the SOCKTYPE name in libpq-fe.h, but that's not
> in our namespace and it's conceivable that it might collide with
> someone else. (It collided with my own test code during my work on
> this.)
> - 0002 fixes a copy-paste bug in meson.build, which luckily hadn't
> caused problems yet.
> - 0003 ports Tom's debug2 fix for Test::Cluster::connect_fails() over
> to 002_client.pl. (Currently, log checks in this test aren't made
> after connection failures, but I don't really want to chase that down
> later after I've forgotten about it.)
>=20
> =3D Roadmap: Implementation =3D
>=20
> Next three patches are the core implementation, which stabilizes the
> ABI for libpq-oauth. I feel fairly confident that this, or something
> close to it, could land in PG19.
>=20
> - 0004 introduces the new PQAUTHDATA_OAUTH_BEARER_TOKEN_V2 API.
>=20
> As described above.
>=20
> - 0005 makes use of the new API in libpq-oauth.
>=20
> This removes more code than it introduces, which is exciting.
>=20
> Now we can rename libpq-oauth-19.so to just libpq-oauth.so, since we
> no longer depend on anything that could change between major versions.
> (We still need lock and locale support from libpq, as mentioned above,
> so they continue to be decoupled via injection at init time.)
>=20
> Some of the code in this patch overlaps with the translation fixes
> thread [2], which I need to get to first. I'm hoping some additional
> simplifications can be made after those localization bugs are fixed.
>=20
> I think I'd also like to get the threadlock into the module API (but
> not the hook API). A third-party flow might need to perform its own
> one-time initialization, and if it relies on an old version of Curl
> (or worse, Kerberos [3]), it'll need to use the same lock that the
> top-level application has registered for libpq. So I imagine I'll need
> to break out an initialization function here. Alternatively, I could
> introduce an API into libpq to retrieve the threadlock function in
> use?
>=20
> - 0006 removes a potential ABI-compatibility pitfall for future devs.
>=20
> libpq-oauth needs to use the shared-library variant of libpgcommon,
> but it can no longer assume that the encoding support exported by
> libpq is compatible. So it must not accidentally link against those
> functions (see [4]). I don't imagine anyone will try adding code that
> does this in practice; we're pretty UTF8-centric in OAuth. But just to
> be safe, define USE_PRIVATE_ENCODING_FUNCS so that anyone who tries
> will fail the build.
>=20
> =3D Roadmap: Plugins? =3D
>=20
> So now we have a stable ABI, which technically means that any
> enterprising developer who wants to play games with LD_LIBRARY_PATH
> could implement their own version of an OAuth flow, and have our
> utilities make use of it into the future.
>=20
> That brings us to patch 0007, which experimentally promotes the stable
> API to a public header, and introduces a really janky environment
> variable so that people don't have to play games. It will be obvious
> from the code that this is not well-baked yet.
>=20
> I hope the ability to dlopen() a custom flow can make it for 19; I
> just don't know that this envvar approach is any good. The ideal
> situation, IMO, is for a flow to be selected in the connection string.
> But we have to lock that down, similarly to how we protect
> local_preload_libraries, to prevent horrible exploits. At which point
> we'll have essentially designed a generic libpq plugin system. Not
> necessarily a terrible thing, but I don't think I have time to take it
> on for PG19.
>=20
> WDYT?
> --Jacob
>=20
> [1] https://postgr.es/m/aAkJnDQq3mOUvmQV%40msg.df7cb.de
> [2] =
https://postgr.es/m/TY4PR01MB1690746DB91991D1E9A47F57E94CDA%40TY4PR01MB169=
07.jpnprd01.prod.outlook.com
> [3] https://postgr.es/m/aSSp03wmNMngi/Oe%40ubby
> [4] https://postgr.es/c/b6c7cfac8
> =
<0001-libpq-fe.h-Don-t-claim-SOCKTYPE-in-the-global-namesp.patch><0002-lib=
pq-oauth-use-correct-c_args-in-meson.build.patch><0003-oauth_validator-Avo=
id-races-in-log_check.patch><0004-libpq-Introduce-PQAUTHDATA_OAUTH_BEARER_=
TOKEN_V2.patch><0005-libpq-oauth-Use-the-PGoauthBearerRequestV2-API.patch>=
<0006-libpq-oauth-Never-link-against-libpq-s-encoding-func.patch><0007-WIP=
-Introduce-third-party-OAuth-flow-plugins.patch>

Hi Jacob,

This is a solid patch set. Only a few small comments:

1 - 0001
```
 /* for PGoauthBearerRequest.async() */
 #ifdef _WIN32
-#define SOCKTYPE uintptr_t		/* avoids depending on =
winsock2.h for SOCKET */
+#define PQ_SOCKTYPE uintptr_t	/* avoids depending on winsock2.h for =
SOCKET */
 #else
-#define SOCKTYPE int
+#define PQ_SOCKTYPE int
 #endif
```

The commit message has explained why SOCKTYPE is temporary and the =
reason why adding prefix =E2=80=9CPG_=E2=80=9D is to avoid collisions. =
But I don=E2=80=99t think code readers will always read commit messages, =
given the macro is a local and temporary, why adding a prefix starting =
with a underscore, like =E2=80=9C_PQ_SOCKTYPE=E2=80=9D, which explicitly =
indicates the macro is kinda private.

=3D=3D=3D
0002 & 0003 Looks good.
=3D=3D=3D

2 - 0004
```
+ * Helper for handling user flow failures. If the implementation put =
anything
+ * into request->error, it's added to conn->errorMessage here.
```

Typo: put -> puts

3 - 0004
```
+# Make sure the v1 hook continues to work. */
+test(
```

=E2=80=9C*/=E2=80=9C in the end of the comment line seems a typo.

4 - 0005
```
+	PQAUTHDATA_OAUTH_BEARER_TOKEN,	/* server requests an OAuth =
Bearer token
+									 =
* (prefer v2, below, instead) */
```
"(prefer v2, below, instead)" looks confusing to me, though I can =
understand what it means. Maybe make it clearer, like =E2=80=9C(v2 is =
preferred; see below)"


=3D=3D=3D
0006 Looks good.
=3D=3D=3D

=3D=3D=3D
Not reviewing 0007 as it marks with WIP.
=3D=3D=3D


Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/