Re: [oauth] Stabilize the libpq-oauth ABI (and allow alternative implementations?)

+ * TODO: have to think about _all_ the security ramifications of this. What
+ * existing protections in LD_LIBRARY_PATH (and/or SIP) are we potentially
+ * bypassing? Should we check the permissions of the file somehow...?
+ * TODO: maybe disallow anything not underneath LIBDIR? or PKGLIBDIR?
+ * Should it have a naming convention?
+ */
+ const char *env = getenv("PGOAUTHMODULE");

and

> That's exactly the use case. (Also, library clients of postgres that
> should not install application hooks, other monolithic utilities that
> link against libpq, etc.) Who wants to recompile their clients just to
> switch how they get a token?

How would this work with library clients exactly (and even multiple
library clients)?

* Language bindings: there's already ongoing work on adding support
for the hooks into these libraries, so that's not really a use case
(and: 1. there's no recompilation cost for many of them 2. even if
there is, you can't easily use the same language for these plugins,
which is again a downside). And some bindings don't use libpq at all.
* postgres CLI tools: That's one use case, but it could work without
generic user-facing plugin support, limited to these tools.
* postgres_fwd/dblink and similar: I'm not sure how that would work,
what if different extensions require different plugins? This already
seems like a tricky question with the hooks

And from a configuration/security risk point:

* Restricting to system paths could work, but it limits who and how
can add these plugins, and removes the possibility of modifying a
specific application without system permissions. I can't just download
a binary application, and a separate oauth plugin, and use them
together, I need admin permission - that seems strange to me. (Unless
I compile libpq myself in a different prefix?)
* On the other hand requiring applications to specify allowed paths or
plugins directly, without environment variables goes back to the
previous configuration question

> What existing protections in LD_LIBRARY_PATH (and/or SIP) are we potentially bypassing?

From the ld.so.8 manpage:

> In  secure-execution  mode, preload pathnames containing slashes are ignored.  Furthermore, shared objects are preloaded only from the standard search directories and only if they have set-user-ID mode bit enabled (which  is  not typical).

An easy solution could be using secure_getenv instead of getenv, which
would at least improve the situation?


And a few specific comments about the patches:

+oom:
+ request->error = libpq_gettext("out of memory");
+ return -1;

Shouldn't this also free conninfo if it is allocated?

+/* Features added in PostgreSQL v19: */
+/* Indicates presence of PQgetThreadLock */
+#define LIBPQ_HAS_GET_THREAD_LOCK

Should this be defined to 1?


+ /*
+ * We need to inject necessary function pointers into the module. This
+ * only needs to be done once -- even if the pointers are constant,
+ * assigning them while another thread is executing the flows feels
+ * like tempting fate.
+ */
+ if ((lockerr = pthread_mutex_lock(&init_mutex)) != 0)
+ {
+ /* Should not happen... but don't continue if it does. */
+ Assert(false);

- libpq_append_conn_error(conn, "failed to lock mutex (%d)", lockerr);
- return 0;
- }
+ libpq_append_conn_error(conn, "failed to lock mutex (%d)", lockerr);
+ return 0;
+ }


Shouldn't this path return -1, and also dlclose the library?

+
+ dlclose(state->flow_module);
+

Shouldn't dlcloses also reset state->flow_module after?

-free_async_ctx(PGconn *conn, struct async_ctx *actx)
+free_async_ctx(PGoauthBearerRequestV2 *req, struct async_ctx *actx)

req (or conn) doesn't seem to be used in this function, does it need
that parameter?


+ env = strchr(env, '\x01');
+ *env++ = '\0';

Isn't mutating environment variables UB?