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