On Wed, Apr 30, 2025 at 5:55 AM Daniel Gustafsson <dan...@yesql.se> wrote: > > 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.
Yeah, it moves us much closer to the long-term goal. > We should either clarify that it was never shipped as part of libpq core, or > remove this altogether. Done in v11, with your suggested wording. > I think this explanatory paragraph should come before the function prototype. Done. > 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"? 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 <dan...@yesql.se>
Reviewed-by: Andres Freund <and...@anarazel.de>
Reviewed-by: Christoph Berg <m...@debian.org>
+ Reviewed-by: Daniel Gustafsson <dan...@yesql.se>
Reviewed-by: Jelte Fennema-Nio <postg...@jeltef.nl>
Reviewed-by: Peter Eisentraut <pe...@eisentraut.org>
Reviewed-by: Wolfgang Walther <walt...@technowledgy.de>
@@ 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
+
+/*
v11-0001-oauth-Move-the-builtin-flow-into-a-separate-modu.patch
Description: Binary data
