Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3776.700.51.11.1\))
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <CAOYmi+=UsSnLM4K+hYkhrezpQROQ5jr=72feAkQ0Te8GJf3Fbg@mail.gmail.com>
Date: Wed, 30 Apr 2025 14:55:07 +0200
Cc: Christoph Berg <myon@debian.org>,
 Jelte Fennema-Nio <postgres@jeltef.nl>,
 Peter Eisentraut <peter@eisentraut.org>,
 Andres Freund <andres@anarazel.de>,
 Tom Lane <tgl@sss.pgh.pa.us>,
 Bruce Momjian <bruce@momjian.us>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Thomas Munro <thomas.munro@gmail.com>,
 Nazir Bilal Yavuz <byavuz81@gmail.com>,
 Antonin Houska <ah@cybertec.at>,
 Wolfgang Walther <walther@technowledgy.de>,
 =?utf-8?B?RGV2cmltIEfDvG5kw7x6?= <devrim@gunduz.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <8210C830-DDBB-4CD7-AB39-F5F59C36D547@yesql.se>
References: <Z_ayCLeAEinXvSxL@msg.df7cb.de>
 <CAOYmi+n9DHS_xUatuuspdC8tjtaMzY8P11Y9y5Fz+2pjikkL9g@mail.gmail.com>
 <f47ce2cb-4076-46f3-be2e-60fa4dd70f41@eisentraut.org>
 <CAOYmi+=2cArQoSSRdAYwx2TBSHL5r0UhX28Q-5TP=0g-sxBXCA@mail.gmail.com>
 <CAGECzQShbNtypPqabVv2eKJP=0Ejox=6f8k8G1E-2GN6Ag1kAA@mail.gmail.com>
 <CAOYmi+=j9nLQFjQ8z0vyQmuhNMwsFbzvne_2S2pTbBGir4q6EQ@mail.gmail.com>
 <aAOREVWMFTuWvJ1l@msg.df7cb.de>
 <CAOYmi+k9uN2OQ_sXjztZYPSpPbfGmkkv4yhtPhyjNQ6vy7SSfA@mail.gmail.com>
 <aAkJnDQq3mOUvmQV@msg.df7cb.de>
 <CAOYmi+=WA4ZfyHWt+4=yFV7VAXQWvNKsn4R=3PwLd4GfmSTiBA@mail.gmail.com>
 <aAkXb1wdZUDycyWi@msg.df7cb.de>
 <CAOYmi+=cs+gDTAH0L+3JzpRLeHPOXDQxu7MOfsbjah9SkAP_kw@mail.gmail.com>
 <CAOYmi+=UsSnLM4K+hYkhrezpQROQ5jr=72feAkQ0Te8GJf3Fbg@mail.gmail.com>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Archived-At: <https://www.postgresql.org/message-id/8210C830-DDBB-4CD7-AB39-F5F59C36D547%40yesql.se>
Precedence: bulk

> On 29 Apr 2025, at 02:10, Jacob Champion =
<jacob.champion@enterprisedb.com> wrote:
>=20
> On Wed, Apr 23, 2025 at 10:46=E2=80=AFAM Jacob Champion
> <jacob.champion@enterprisedb.com> 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?)
>=20
> 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