Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER

From: Jacob Champion <[email-masked]>
Date: Thu, 6 Mar 2025 16:35:43 -0800

On Thu, Mar 6, 2025 at 12:57 PM Jacob Champion
<[email-masked]> wrote:
> Problem 1 is a simple patch. I am working on a fix for Problem 2, but
> I got stuck trying to get a "perfect" solution working yesterday...
> Since this is a partial(?) blocker for getting NetBSD going, I'm going
> to pivot to an ugly-but-simple approach today.

Attached:
- 0001 fixes IPv6 failures,
- 0002 fixes set_timer() on Mac, and
- 0003-0005 are the existing followup patches from upthread.

Thanks!
--Jacob

---

From: Thomas Munro <[email-masked]>
Date: Fri, 7 Mar 2025 18:12:50 +1300

On Fri, Mar 7, 2025 at 9:57 AM Jacob Champion
<[email-masked]> wrote:
> 2) macOS's EVFILT_TIMER implementation seems to be different from the
> other BSDs. On Mac, when you re-add a timer to a kqueue, any existing
> timer-fired events for it are not cleared out and the kqueue might
> remain readable. This breaks a postcondition of our set_timer()
> function, which is that new timeouts are supposed to completely
> replace previous timeouts.

I don't see that behaviour on my Mac with a simple program, and that
seems like it couldn't possibly be intended.  Hmm...  <browses source
code painfully> I wonder if this atomic generation scheme has a hole
in it, under concurrency...

https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/bsd/kern/kern_event.c#L1661

The code on the other OSes just dequeues it when reprogramming the
timer, which involves a lock and no doubt a few more cycles, and is
clearly not quite as exciting but ...

---

From: Jacob Champion <[email-masked]>
Date: Fri, 7 Mar 2025 09:30:52 -0800

On Thu, Mar 6, 2025 at 9:13 PM Thomas Munro <[email-masked]> wrote:
> seems like it couldn't possibly be intended.

What version of macOS?

Just to make sure I'm not chasing ghosts, I've attached my test
program. Here are my CI results for running it:

= FreeBSD =

[      6 us] timer is set
[   1039 us] kqueue is readable
[   1050 us] timer is reset
[   1052 us] kqueue is not readable

= NetBSD =

[      3 us] timer is set
[  14993 us] kqueue is readable
[  15000 us] timer is reset
[  15002 us] kqueue is not readable

= OpenBSD =

[     24 us] timer is set
[  19660 us] kqueue is readable
[  19709 us] timer is reset
[  19712 us] kqueue is not readable

= macOS Sonoma =

[      4 us] timer is set
[   1282 us] kqueue is readable
[   1286 us] timer is reset
[   1287 us] kqueue is still readable

--Jacob

---

From: Thomas Munro <[email-masked]>
Date: Sat, 8 Mar 2025 10:51:41 +1300

> program. Here are my CI results for running it:

Ah, right, yeah I see that here too.  I thought you were saying that
kevent() could report an already triggered alarm even though we'd
replaced it (it doesn') but of course you meant poll(kq) as libpq
does.

I believe I know exactly why: kqueues are considered readable (by
poll/select/other kqueues) if there are any events queued[1].  Apple's
EVFILT_TIMER implementation is doing that trick[2] where it leaves
them queued, but filt_timerprocess() filters them out if its own
private _FIRED flag isn't set, so kevent() itself won't wake up or
return them.  That trick doesn't survive nesting.  I think I would
call that a bug.  (I think I would keep the atomic CAS piece -- it
means you don't have to drain the timer callout synchronously when
reprogramming it which is a cool trick, but I think they overshot when
they left the knote queued.)

Maybe just do the delete-and-add in one call?

    EV_SET(&ev[0], 1, EVFILT_TIMER, EV_DELETE, 0, 0, 0);
    EV_SET(&ev[1], 1, EVFILT_TIMER, EV_ADD | EV_ONESHOT, 0, timeout, 0);
    if (kevent(kq, &ev[0], 2, NULL, 0, NULL) < 0)

[1] https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/bsd/kern/kern_event.c#L1078
[2] https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/bsd/kern/kern_event.c#L1661

---

From: Jacob Champion <[email-masked]>
Date: Fri, 7 Mar 2025 15:02:50 -0800

On Fri, Mar 7, 2025 at 1:52 PM Thomas Munro <[email-masked]> wrote:
> call that a bug.

Bleh. Thank you for the analysis!

>     if (kevent(kq, &ev[0], 2, NULL, 0, NULL) < 0)

I think that requires me to copy the EV_RECEIPT handling from
register_socket(), to make sure an ENOENT is correctly ignored on
delete but doesn't mask failures from the addition. Do you prefer that
to the separate calls? (Or, better yet, is it easier than I'm making
it?)

Thanks!
--Jacob

---

From: Nazir Bilal Yavuz <[email-masked]>
Date: Mon, 17 Mar 2025 14:36:46 +0300

Hi,

I just wanted to report that the 'oauth_validator/t/001_server.pl'
test failed on FreeBSD in one of my local CI runs [1]. I looked at the
thread but could not find the same error report; if this is already
known, please excuse me.

HEAD was at 3943f5cff6 and there were no other changes. Sharing the
failure here for visibility:

[11:09:56.548] stderr:
[11:09:56.548] #   Failed test 'stress-async: stdout matches'
[11:09:56.548] #   at
/tmp/cirrus-ci-build/src/test/modules/oauth_validator/t/001_server.pl
line 409.
[11:09:56.548] #                   ''
[11:09:56.548] #     doesn't match '(?^:connection succeeded)'
[11:09:56.548] #   Failed test 'stress-async: stderr matches'
[11:09:56.548] #   at
/tmp/cirrus-ci-build/src/test/modules/oauth_validator/t/001_server.pl
line 410.
[11:09:56.548] #                   '[libcurl] * Host localhost:39251
was resolved.
[11:09:56.548] # [libcurl] * IPv6: ::1
[11:09:56.548] # [libcurl] * IPv4: 127.0.0.1
[11:09:56.548] # [libcurl] *   Trying [::1]:39251...
[11:09:56.548] # [libcurl] * Immediate connect fail for ::1: Connection refused
[11:09:56.548] # [libcurl] *   Trying 127.0.0.1:39251...
[11:09:56.548] # [libcurl] * Connected to localhost (127.0.0.1) port 39251
[11:09:56.548] # [libcurl] * using HTTP/1.x
[11:09:56.548] # [libcurl] > GET
/param/.well-known/openid-configuration HTTP/1.1
[11:09:56.548] # [libcurl] > Host: localhost:39251
[11:09:56.548] # [libcurl] >
[11:09:56.548] # [libcurl] * Request completely sent off
[11:09:56.548] # [libcurl] * HTTP 1.0, assume close after body
[11:09:56.548] # [libcurl] < HTTP/1.0 200 OK
[11:09:56.548] # [libcurl] < Server: BaseHTTP/0.6 Python/3.11.11
[11:09:56.548] # [libcurl] < Date: Mon, 17 Mar 2025 11:09:55 GMT
[11:09:56.548] # [libcurl] < Content-Type: application/json
[11:09:56.548] # [libcurl] < Content-Length: 400
[11:09:56.548] # [libcurl] <
[11:09:56.548] # [libcurl] < {"issuer":
"http://localhost:39251/param", "token_endpoint":
"http://localhost:39251/param/token", "device_authorization_endpoint":
"http://localhost:39251/param/authorize", "response_types_supported":
["token"], "subject_types_supported": ["public"],
"id_token_signing_alg_values_supported": ["RS256"],
"grant_types_supported": ["authorization_code",
"urn:ietf:params:oauth:grant-type:device_code"]}
[11:09:56.548] # [libcurl] * shutting down connection #0
[11:09:56.548] # [libcurl] * Hostname localhost was found in DNS cache
[11:09:56.548] # [libcurl] *   Trying [::1]:39251...
[11:09:56.548] # [libcurl] * Immediate connect fail for ::1: Connection refused
[11:09:56.548] # [libcurl] *   Trying 127.0.0.1:39251...
[11:09:56.548] # [libcurl] * Connected to localhost (127.0.0.1) port 39251
[11:09:56.548] # [libcurl] * using HTTP/1.x
[11:09:56.548] # [libcurl] > POST /param/authorize HTTP/1.1
[11:09:56.548] # [libcurl] > Host: localhost:39251
[11:09:56.548] # [libcurl] > Content-Length: 92
[11:09:56.548] # [libcurl] > Content-Type: application/x-www-form-urlencoded
[11:09:56.548] # [libcurl] >
[11:09:56.548] # [libcurl] >
scope=openid+postgres&client_id=eyJpbnRlcnZhbCI6MSwicmV0cmllcyI6MSwic3RhZ2UiOiJhbGwifQ%3D%3D
[11:09:56.548] # [libcurl] * upload completely sent off: 92 bytes
[11:09:56.548] # [libcurl] * HTTP 1.0, assume close after body
[11:09:56.548] # [libcurl] < HTTP/1.0 200 OK
[11:09:56.548] # [libcurl] < Server: BaseHTTP/0.6 Python/3.11.11
[11:09:56.548] # [libcurl] < Date: Mon, 17 Mar 2025 11:09:55 GMT
[11:09:56.548] # [libcurl] < Content-Type: application/json
[11:09:56.548] # [libcurl] < Content-Length: 132
[11:09:56.548] # [libcurl] <
[11:09:56.548] # [libcurl] < {"device_code": "postgres", "user_code":
"postgresuser", "verification_uri": "https://example.com/",
"expires_in": 5, "interval": 1}
[11:09:56.548] # [libcurl] * shutting down connection #1
[11:09:56.548] # [libcurl] * Hostname localhost was found in DNS cache
[11:09:56.548] # [libcurl] *   Trying [::1]:39251...
[11:09:56.548] # [libcurl] * Connected to localhost (::1) port 39251
[11:09:56.548] # [libcurl] * using HTTP/1.x
[11:09:56.548] # [libcurl] > POST /param/token HTTP/1.1
[11:09:56.548] # [libcurl] > Host: localhost:39251
[11:09:56.548] # [libcurl] > Content-Length: 157
[11:09:56.548] # [libcurl] > Content-Type: application/x-www-form-urlencoded
[11:09:56.548] # [libcurl] >
[11:09:56.548] # [libcurl] >
device_code=postgres&grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Adevice_code&client_id=eyJpbnRlcnZhbCI6MSwicmV0cmllcyI6MSwic3RhZ2UiOiJhbGwifQ%3D%3D
[11:09:56.548] # [libcurl] * upload completely sent off: 157 bytes
[11:09:56.548] # [libcurl] * Received HTTP/0.9 when not allowed
[11:09:56.548] # [libcurl] * closing connection #2
[11:09:56.548] # connection to database failed: connection to server
on socket "/tmp/xZKYtq40nL/.s.PGSQL.21400" failed: failed to obtain
access token: Unsupported protocol (libcurl: Received HTTP/0.9 when
not allowed)
[11:09:56.548] # '
[11:09:56.548] #           matches '(?^:connection to database failed)'
[11:09:56.548] # Looks like you failed 2 tests of 121.
[11:09:56.548]
[11:09:56.548] (test program exited with status code 2)

[1] https://cirrus-ci.com/task/4621590844932096

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

---

From: Jacob Champion <[email-masked]>
Date: Mon, 17 Mar 2025 08:08:30 -0700

> [11:09:56.548] # [libcurl] * Connected to localhost (::1) port 39251

Curl should not have connected to ::1 (the test server isn't listening
on IPv6). Whatever is talking on that port doesn't understand HTTP,
and we later fail with the "HTTP/0.9" error -- a slightly confusing
way to describe a protocol violation.

0001 will fix that. I think we should get that and 0002 in, ASAP. (And
the others.) Thomas has shown me a side quest to get rid of the second
kqueue instance, but so far that is not bearing fruit and we shouldn't
wait on it.

Thanks again!
--Jacob

---

From: Thomas Munro <[email-masked]>
Date: Wed, 19 Mar 2025 17:09:21 +1300

On Tue, Mar 18, 2025 at 4:08 AM Jacob Champion
<[email-masked]> wrote:
> the others.)

All pushed (wasn't sure if Daniel was going to but once I got tangled
up in all that kqueue stuff he probably quite reasonably assumed that
I would :-)).

> Thomas has shown me a side quest to get rid of the second

Cool, thanks for looking into it anyway.  (Feel free to post a
nonworking patch with a got-stuck-here-because problem statement...)

---

From: Tom Lane <[email-masked]>
Date: Wed, 19 Mar 2025 00:17:15 -0400

Thomas Munro <[email-masked]> writes:
> All pushed

You may have noticed it already, but indri reports that this
printf-like call isn't right:

fe-auth-oauth-curl.c:1392:49: error: data argument not used by format string [-Werror,-Wformat-extra-args]
 1392 |                 actx_error(actx, "deleting kqueue timer: %m", timeout);
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~  ^
fe-auth-oauth-curl.c:324:59: note: expanded from macro 'actx_error'
  324 |         appendPQExpBuffer(&(ACTX)->errbuf, libpq_gettext(FMT), ##__VA_ARGS__)
      |                                                          ~~~     ^

"timeout" isn't being used anymore.

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Wed, 19 Mar 2025 17:34:18 +1300

>  1392 |                 actx_error(actx, "deleting kqueue timer: %m", timeout);

Yeah.  Thanks, fixed.

Now I'm wondering about teaching CI to fail on compiler warnings, ie
not just the special warnings task but also in the Mac etc builds.
The reason it doesn't is because it's sort of annoying to stop the
main tests because of a format string snafu, but we must be able to
put a new step at the end after all tests that scans the build logs
for warning and then raises the alarm...

---

From: Tom Lane <[email-masked]>
Date: Wed, 19 Mar 2025 00:57:29 -0400

BTW, I was pretty seriously disheartened just now to realize that
this feature was implemented by making libpq depend on libcurl.
I'd misread the relevant commit messages to say that libcurl was
just being used as test infrastructure; but nope, it's a genuine
build and runtime dependency.  I wonder how much big-picture
thinking went into that.  I can see at least two objections:

* This represents a pretty large expansion of dependency footprint,
not just for us but for the umpteen hundred packages that depend on
libpq.  libcurl alone maybe wouldn't be so bad, but have you looked
at libcurl's dependencies?  On RHEL8,

$ ldd /usr/lib64/libcurl.so.4.5.0
        linux-vdso.so.1 (0x00007fffd3075000)
        libnghttp2.so.14 => /lib64/libnghttp2.so.14 (0x00007f992097a000)
        libidn2.so.0 => /lib64/libidn2.so.0 (0x00007f992075c000)
        libssh.so.4 => /lib64/libssh.so.4 (0x00007f99204ec000)
        libpsl.so.5 => /lib64/libpsl.so.5 (0x00007f99202db000)
        libssl.so.1.1 => /lib64/libssl.so.1.1 (0x00007f9920046000)
        libcrypto.so.1.1 => /lib64/libcrypto.so.1.1 (0x00007f991fb5b000)
        libgssapi_krb5.so.2 => /lib64/libgssapi_krb5.so.2 (0x00007f991f906000)
        libkrb5.so.3 => /lib64/libkrb5.so.3 (0x00007f991f61b000)
        libk5crypto.so.3 => /lib64/libk5crypto.so.3 (0x00007f991f404000)
        libcom_err.so.2 => /lib64/libcom_err.so.2 (0x00007f991f200000)
        libldap-2.4.so.2 => /lib64/libldap-2.4.so.2 (0x00007f991efb1000)
        liblber-2.4.so.2 => /lib64/liblber-2.4.so.2 (0x00007f991eda1000)
        libbrotlidec.so.1 => /lib64/libbrotlidec.so.1 (0x00007f991eb94000)
        libz.so.1 => /lib64/libz.so.1 (0x00007f991e97c000)
        libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f991e75c000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f991e386000)
        libunistring.so.2 => /lib64/libunistring.so.2 (0x00007f991e005000)
        librt.so.1 => /lib64/librt.so.1 (0x00007f991ddfd000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f9920e30000)
        libdl.so.2 => /lib64/libdl.so.2 (0x00007f991dbf9000)
        libkrb5support.so.0 => /lib64/libkrb5support.so.0 (0x00007f991d9e8000)
        libkeyutils.so.1 => /lib64/libkeyutils.so.1 (0x00007f991d7e4000)
        libresolv.so.2 => /lib64/libresolv.so.2 (0x00007f991d5cc000)
        libsasl2.so.3 => /lib64/libsasl2.so.3 (0x00007f991d3ae000)
        libm.so.6 => /lib64/libm.so.6 (0x00007f991d02c000)
        libbrotlicommon.so.1 => /lib64/libbrotlicommon.so.1 (0x00007f991ce0b000)
        libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f991cbe0000)
        libcrypt.so.1 => /lib64/libcrypt.so.1 (0x00007f991c9b7000)
        libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 (0x00007f991c733000)

* Given libcurl's very squishy portfolio:

  libcurl is a free and easy-to-use client-side URL transfer library, supporting
  FTP, FTPS, HTTP, HTTPS, SCP, SFTP, TFTP, TELNET, DICT, LDAP, LDAPS, FILE, IMAP,
  SMTP, POP3 and RTSP. libcurl supports SSL certificates, HTTP POST, HTTP PUT,
  FTP uploading, HTTP form based upload, proxies, cookies, user+password
  authentication (Basic, Digest, NTLM, Negotiate, Kerberos4), file transfer
  resume, http proxy tunneling and more.

it's not exactly hard to imagine them growing a desire to handle
"postgresql://" URLs, which they would surely do by invoking libpq.
Then we'll have circular build dependencies and circular runtime
dependencies, not to mention inter-library recursion at runtime.

This is not quite a hill that I wish to die on, but I will
flatly predict that we will regret this.

			regards, tom lane

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 19 Mar 2025 09:31:42 -0400

> flatly predict that we will regret this.

I regularly see curl security fixes in my Debian updates, so there is a
security issue that any serious curl bug could also make Postgres
vulnerable.  I might be willing to die on that hill.

-- 
  Bruce Momjian  <[email-masked]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 19 Mar 2025 14:38:08 +0100

> On 19 Mar 2025, at 05:57, Tom Lane <[email-masked]> wrote:
> 
> thinking went into that.

A considerable amount. 

libcurl is not a dependency for OAuth support in libpq, the support was
designed to be exensible such that clients can hook in their own flow
implementations.  This part does not require libcurl.  It is however a
dependency for the RFC 8628 implementation which is included when building with
--with-libcurl, this in order to ship something which can be used out of the
box (for actual connections *and* testing) without clients being forced to
provide their own implementation.

This obviously means that the RFC8628 part could be moved to contrib/, but I
fear we wouldn't make life easier for packagers by doing that.

>  ...
> "postgresql://" URLs,

While there is no guarantee that such a pull request wont be submitted,
speaking as a (admittedly not very active at the moment) libcurl maintainer I
consider it highly unlikely that it would be accepted.  A postgres connnection
does not fit into what libcurl/curl is and wants to be.

--
Daniel Gustafsson

---

From: Andres Freund <[email-masked]>
Date: Wed, 19 Mar 2025 09:46:50 -0400

Hi,

> for warning and then raises the alarm...

The best way would probably be to tee the output of the build to a log file
and then have a script at the end to check for errors in that.

Greetings,

Andres Freund

---

From: Thomas Munro <[email-masked]>
Date: Thu, 20 Mar 2025 09:11:43 +1300

> fear we wouldn't make life easier for packagers by doing that.

How feasible/fragile/weird would it be to dlopen() it on demand?
Looks like it'd take ~20 function pointers:

                 U curl_easy_cleanup
                 U curl_easy_escape
                 U curl_easy_getinfo
                 U curl_easy_init
                 U curl_easy_setopt
                 U curl_easy_strerror
                 U curl_free
                 U curl_global_init
                 U curl_multi_add_handle
                 U curl_multi_cleanup
                 U curl_multi_info_read
                 U curl_multi_init
                 U curl_multi_remove_handle
                 U curl_multi_setopt
                 U curl_multi_socket_action
                 U curl_multi_socket_all
                 U curl_multi_strerror
                 U curl_slist_append
                 U curl_slist_free_all
                 U curl_version_info

---

From: Tom Lane <[email-masked]>
Date: Wed, 19 Mar 2025 17:03:59 -0400

> How feasible/fragile/weird would it be to dlopen() it on demand?

FWIW, that would not really move the needle one bit so far as
my worries are concerned.  What I'm unhappy about is the very
sizable expansion of our build dependency footprint as well
as the sizable expansion of the 'package requires' footprint.
The fact that the new dependencies are mostly indirect doesn't
soften that blow at all.

To address that (without finding some less kitchen-sink-y OAuth
implementation to depend on), we'd need to shove the whole thing
into a separately-built, separately-installable package.

What I expect is likely to happen is that packagers will try to do
that themselves to avoid the dependency bloat.  AFAICT our current
setup will make that quite painful for them, and in any case I
don't believe it's work we should make them do.  If they fail to
do that, the burden of the extra dependencies will fall on end
users.  Either way, it's not going to make us look good.

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Thu, 20 Mar 2025 11:02:57 +1300

> users.  Either way, it's not going to make us look good.

It would increase the build dependencies, assuming a package
maintainer wants to enable as many features as possible, but it would
*not* increase the 'package requires' footprint, merely the 'package
suggests' footprint (as Debian calls it), and it's up to the user
whether they install suggested extra packages, no?

---

From: Thomas Munro <[email-masked]>
Date: Thu, 20 Mar 2025 11:14:29 +1300

> > > How feasible/fragile/weird would it be to dlopen() it on demand?

. o O { There may also be security reasons to reject the idea, would
need to look into that... }

---

From: Tom Lane <[email-masked]>
Date: Wed, 19 Mar 2025 18:19:23 -0400

> whether they install suggested extra packages, no?

Maybe I'm confused, but what I saw was a hard dependency on libcurl,
as well as several of its dependencies:

$ ./configure --with-libcurl
...
$ make
...
$ ldd src/interfaces/libpq/libpq.so.5.18 
        linux-vdso.so.1 (0x00007ffc145fe000)
        libcurl.so.4 => /lib64/libcurl.so.4 (0x00007f2c2fa36000)
        libm.so.6 => /lib64/libm.so.6 (0x00007f2c2f95b000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f2c2f600000)
        libnghttp2.so.14 => /lib64/libnghttp2.so.14 (0x00007f2c2f931000)
        libidn2.so.0 => /lib64/libidn2.so.0 (0x00007f2c2f910000)
        libssh.so.4 => /lib64/libssh.so.4 (0x00007f2c2f89b000)
        libpsl.so.5 => /lib64/libpsl.so.5 (0x00007f2c2f885000)
        libssl.so.3 => /lib64/libssl.so.3 (0x00007f2c2f51a000)
        libcrypto.so.3 => /lib64/libcrypto.so.3 (0x00007f2c2f000000)
        libgssapi_krb5.so.2 => /lib64/libgssapi_krb5.so.2 (0x00007f2c2f82f000)
        libkrb5.so.3 => /lib64/libkrb5.so.3 (0x00007f2c2ef26000)
        libk5crypto.so.3 => /lib64/libk5crypto.so.3 (0x00007f2c2f816000)
        libcom_err.so.2 => /lib64/libcom_err.so.2 (0x00007f2c2f80d000)
        libldap.so.2 => /lib64/libldap.so.2 (0x00007f2c2eebf000)
        liblber.so.2 => /lib64/liblber.so.2 (0x00007f2c2eead000)
        libbrotlidec.so.1 => /lib64/libbrotlidec.so.1 (0x00007f2c2ee9f000)
        libz.so.1 => /lib64/libz.so.1 (0x00007f2c2ee85000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f2c2fb43000)
        libunistring.so.2 => /lib64/libunistring.so.2 (0x00007f2c2ed00000)
        libkrb5support.so.0 => /lib64/libkrb5support.so.0 (0x00007f2c2ecef000)
        libkeyutils.so.1 => /lib64/libkeyutils.so.1 (0x00007f2c2ece8000)
        libresolv.so.2 => /lib64/libresolv.so.2 (0x00007f2c2ecd4000)
        libevent-2.1.so.7 => /lib64/libevent-2.1.so.7 (0x00007f2c2ec7b000)
        libsasl2.so.3 => /lib64/libsasl2.so.3 (0x00007f2c2ec5b000)
        libbrotlicommon.so.1 => /lib64/libbrotlicommon.so.1 (0x00007f2c2ec38000)
        libselinux.so.1 => /lib64/libselinux.so.1 (0x00007f2c2ec0b000)
        libcrypt.so.2 => /lib64/libcrypt.so.2 (0x00007f2c2ebd1000)
        libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 (0x00007f2c2eb35000)

I don't think that will be satisfied by 'package suggests'.
Even if it somehow manages to load, the result of trying to
use OAuth would be a segfault rather than any useful message.

			regards, tom lane

---

From: Thomas Munro <[email-masked]>
Date: Thu, 20 Mar 2025 11:28:50 +1300

> use OAuth would be a segfault rather than any useful message.

I was imagining that it would just error out if you try to use that
stuff and it fails to open libcurl.  Then it's up to end users: if
they want to use libpq + OAuth, they have to install both libpq5 and
libcurl packages, and if they don't their connections will just fail,
presumably with some error message explaining why.  Or something like
that...

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 19 Mar 2025 19:11:28 -0400

> presumably with some error message explaining why.  Or something like

Am I understanding that curl is being used just to honor the RFC and it
is only for testing?  That seems like a small reason to add such a
dependency.

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 19 Mar 2025 19:59:28 -0400

> fear we wouldn't make life easier for packagers by doing that.

I see it now ---- without having RFC 8628 built into the server, clients
have to implement it.  Do we know what percentage would need to do that?
The spec:

	https://datatracker.ietf.org/doc/html/rfc8628

Do we think packagers will use the --with-libcurl configure option?

It does kind of make sense for curl to handle OAUTH since curl has to
simulate a browser.  I assume we can't call a shell to invoke curl from
the command line.

---

From: Jacob Champion <[email-masked]>
Date: Thu, 20 Mar 2025 13:33:26 -0700

Hi all,

With the understanding that the patchset is no longer just "my" baby...

= Dependencies =

I like seeing risk/reward discussions. I agonized over the choice of
HTTP dependency, and I transitioned from an "easier" OAuth library
over to Curl early on because of the same tradeoffs.

That said... Tom, I think the dependency list you've presented is not
quite fair, because it doesn't show what libpq's dependency list was
before adding Curl. From your email, and my local Rocky 9 install, I
think these are the net-new dependencies that we (and packagers) need
to worry about:

        libcurl.so.4
        libnghttp2.so.14
        libidn2.so.0
        libssh.so.4
        libpsl.so.5
        libunistring.so.2
        libbrotlidec.so.1
        libbrotlicommon.so.1
        libz.so.1

That's more than I'd like, to be perfectly honest. I'm least happy
about libssh, because we're not using SFTP but we have to pay for it.
And the Deb-alikes add librtmp, which I'm not thrilled about either.

The rest are, IMO, natural dependencies of a mature HTTP client: the
HTTP/1 and HTTP/2 engines, Punycode, the Public Suffix List, UTF
handling, and common response compression types. Those are kind of
part and parcel of communicating on the web. (If we find an HTTP
client that does all those things itself, awesome, but then we have to
ask how well they did it.)

So one question for the collective is -- putting Curl itself aside --
is having a basic-but-usable OAuth flow, out of the box, worth the
costs of a generic HTTP client? A non-trivial footprint *will* be
there, whether it's one library or several, whether we delay-load it
or not, whether we have the unused SFTP/RTMP dependencies or not. But
we could still find ways to reduce that cost for people who aren't
using it, if necessary.

= Asides =

I would also like to point out: End users opt into this by
preregistering a client ID with an OAuth issuer ID, then providing
that pair of IDs in the connection string. We will not just start
crawling the web because a server tells us to. I don't want to
downplay the additional risk of having it in the address space, but
the design goal is that vulnerabilities in the HTTP logic should not
affect users who have not explicitly consented to the use of OAuth.

There were some other questions/statements made upthread that I want
to clarify too:

On Wed, Mar 19, 2025 at 4:11 PM Bruce Momjian <[email-masked]> wrote:
> is only for testing?

No. (I see you found it later, but to state clearly for the record:
it's not just for testing.) libcurl is used for the Device
Authorization flow implementation. You don't have to use Device
Authorization to use OAuth, but we don't provide any alternative flows
in-tree; you'd have to use the libpq API to insert your own flow.

> I see it now ---- without having RFC 8628 built into the server,

(libpq, not the server. We do not ship server-side plugins at all, yet.)

> clients

For version 1 of the feature, Device Authorization is the only option
for our utilities (psql et al). I can't really speculate on
percentages; it depends on what percentage want to use OAuth and don't
like (or can't use) our builtin flow. Obviously the percentage goes up
to 100% if we don't provide one. Plus we lose significant testability,
plus no one can use it from psql.

> Do we think packagers will use the --with-libcurl configure option?

Well, hopefully, yes. The tradeoffs of the builtin flow were chosen
explicitly so that existing clients could use it with minimal-to-no
code changes.

Thanks!
--Jacob

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 20 Mar 2025 16:50:57 -0400

> we could still find ways to reduce that cost for people who aren't

One observation is that security scanning tools are going to see the
curl dependency and look at any CSVs related to them and ask us, whether
they are using OAUTH or not.

---

From: Tom Lane <[email-masked]>
Date: Thu, 20 Mar 2025 17:08:54 -0400

Bruce Momjian <[email-masked]> writes:
>> costs of a generic HTTP client?

> they are using OAUTH or not.

Yes.  Also, none of this has addressed my complaint about the extent
of the build and install dependencies.  Yes, simply not selecting
--with-libcurl removes the problem ... but most packagers are under
very heavy pressure to enable all features of a package.

---

From: Andres Freund <[email-masked]>
Date: Thu, 20 Mar 2025 19:26:49 -0400

Hi,

> very heavy pressure to enable all features of a package.

How about we provide the current libpq.so without linking to curl and also a
libpq-oauth.so that has curl support? If we do it right libpq-oauth.so would
itself link to libpq.so, making libpq-oauth.so a fairly small library.

That way packagers can split libpq-oauth.so into a separate package, while
still just building once.

That'd be a bit of work on the buildsystem side, but it seems doable.

> From what's been said here, only a small minority of users are likely
> to have any interest in this feature.  So my answer to "is it worth
> the cost" is no, and would be no even if I had a lower estimate of
> the costs.

I think this is likely going to be rather widely used, way more widely than
e.g. kerberos or ldap support in libpq. My understanding is that there's a
fair bit of pressure in lots of companies to centralize authentication towards
centralized systems, even for server applications.

> I don't have any problem with making a solution available to those
> users who want it --- but I really do NOT want this to be part of
> stock libpq nor done as part of the core Postgres build.  I do not
> think that the costs of that have been fully accounted for, especially
> not the fact that almost all of those costs fall on people other than
> us.

I am on board with not having it as part of stock libpq, but I don't see what
we gain by not building it as part of postgres (if the dependencies are
available, of course).

Greetings,

Andres Freund

---

From: Andrew Dunstan <[email-masked]>
Date: Fri, 21 Mar 2025 08:40:45 -0400

> That'd be a bit of work on the buildsystem side, but it seems doable.

That certainly seems worth exploring.

> centralized systems, even for server applications.

Indeed. There is still work to do on OAUTH2 but the demand you mention 
is just going to keep increasing.

> we gain by not building it as part of postgres (if the dependencies are

+1.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 21 Mar 2025 19:02:02 +0100

> On 21 Mar 2025, at 13:40, Andrew Dunstan <[email-masked]> wrote:

> That certainly seems worth exploring.

This is being worked on.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Wed, 26 Mar 2025 12:09:14 -0700

> > That certainly seems worth exploring.

Attached is a proof of concept, with code from Daniel and myself,
which passes the CI as a starting point.

Roughly speaking, some things to debate are
- the module API itself
- how much to duplicate from libpq vs how much to export
- is this even what you had in mind

libpq-oauth.so is dlopen'd when needed. If it's not found or it
doesn't have the right symbols, builtin OAuth will not happen. Right
now we have an SO version of 1; maybe we want to remove the SO version
entirely to better indicate that it shouldn't be linked?

Two symbols are exported for the async authentication callbacks. Since
the module understands PGconn internals, maybe we could simplify this
to a single callback that manipulates the connection directly.

To keep the diff small to start, the current patch probably exports
too much. I think appendPQExpBufferVA makes sense, considering we
export much of the PQExpBuffer API already, but I imagine we won't
want to expose the pg_g_threadlock symbol. (libpq could maybe push
that pointer into the libpq-oauth module at load time, instead of
having the module pull it.) And we could probably go either way with
the PQauthDataHook; I prefer having a getter and setter for future
flexibility, but it would be simpler to just export the hook directly.

The following functions are duplicated from libpq:
- libpq_block_sigpipe
- libpq_reset_sigpipe
- libpq_binddomain
- libpq_[n]gettext
- libpq_append_conn_error
- oauth_unsafe_debugging_enabled

Those don't seem too bad to me, though maybe there's a good way to
deduplicate. But i18n needs further work. It builds right now, but I
don't think it works yet.

WDYT?

Thanks,
--Jacob

---

From: Christoph Berg <[email-masked]>
Date: Mon, 31 Mar 2025 16:06:49 +0200

Re: Andres Freund
> > very heavy pressure to enable all features of a package.

And this feature is kind of only useful if it's available anywhere. If
only half of your clients are able to use SSO, you'd probably stick
with passwords anyway. So it needs to be enabled by default.

> still just building once.

That's definitely a good plan. The blast radius of build dependencies
isn't really a problem, the install/run-time is.

Perhaps we could do the same with libldap and libgssapi? (Though
admittedly I have never seen any complaints or nagging questions from
security people about these.)

Christoph

---

From: Jacob Champion <[email-masked]>
Date: Tue, 1 Apr 2025 15:40:49 -0700

> security people about these.)

If we end up happy with how the Curl indirection works, that seems
like it'd be kind of nice in theory. I'm not sure how many people
would notice, though.

On Wed, Mar 26, 2025 at 12:09 PM Jacob Champion
<[email-masked]> wrote:
> Right

Maybe a better idea would be to ship an SONAME of
`libpq-oauth.so.0.<major>`, without any symlinks, so that there's
never any ambiguity about which module belongs with which libpq.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 3 Apr 2025 11:02:03 -0700

> up in all that kqueue stuff he probably quite reasonably assumed that

Attached are two more followups, separate from the libcurl split:
- 0001 is a patch originally proposed at [1]. Christoph pointed out
that the build fails on a platform that tries to enable Curl without
having either epoll() or kqueue(), due to a silly mistake I made in
the #ifdefs.
- 0002 should fix some timeouts in 002_client.pl reported by Andres on
Discord. I allowed a short connect_timeout to propagate into tests
which should not have it.

(The goal of 0001 is to get things building for now. After I finish
splitting the implementation into its own module, it'll make more
sense to simply not build that module on platforms that can't
implement a useful flow.)

Thanks,
--Jacob

[1] https://postgr.es/m/CAOYmi%2B%3D4898tXuTvb2LstorRo9JsAnBcn8LE%3DqrgVPiPW8ZfCw%40mail.gmail.com

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 3 Apr 2025 21:50:16 +0200

> On 3 Apr 2025, at 20:02, Jacob Champion <[email-masked]> wrote:
> 

Thanks, both LGTM so pushed.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Fri, 4 Apr 2025 17:27:46 -0700

> Thanks, both LGTM so pushed.

Thank you!

> never any ambiguity about which module belongs with which libpq.

While I was looking into this I found that Debian's going to use the
existence of an SONAME to check other things, which I assume will make
Christoph's life harder. I have switched over to
'libpq-oauth-<major>.so', without any SONAME or symlinks.

v2 simplifies quite a few things and breaks out the new duplicated
code into its own file. I pared down the exports from libpq, by having
it push the pg_g_threadlock pointer directly into the module when
needed. I think a future improvement would be to combine the dlopen
with the libcurl initialization, so that everything is done exactly
once and the module doesn't need to know about threadlocks at all.

i18n is still not working correctly on my machine. I've gotten `make
init-po` to put the files into the right places now, but if I fake a
.po file and install the generated .mo, the translations still don't
seem to be found at runtime. Is anyone able to take a quick look to
see if I'm missing something obvious?

I still need to disable the module entirely on Windows (and other
platforms without support), and potentially rename the --with-libcurl
option.

Thanks,
--Jacob

---

From: Christoph Berg <[email-masked]>
Date: Sun, 6 Apr 2025 22:48:16 +0200

> > Thanks, both LGTM so pushed.

Ack, the build there worked now. (Albeit without running any tests,
but let's not care too much about this snowflake architecture.)

> 'libpq-oauth-<major>.so', without any SONAME or symlinks.

Since this is a plugin for libpq and nothing external is linking
directly to it, using a formal SONAME wouldn't gain anything, right.

Thanks,
Christoph

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 7 Apr 2025 15:59:19 +0200

> 'libpq-oauth-<major>.so', without any SONAME or symlinks.

Yes, this is correct.  We want a shared module, not a shared library, in 
meson parlance.

But the installation directory of a shared module should not be directly 
libdir.  That is reserved for libraries that you can link at build-time. 
  Here are some examples I found of other packages that have a library 
that itself has plugins:

https://packages.debian.org/bookworm/amd64/libaspell15/filelist
https://packages.debian.org/bookworm/amd64/libkrb5-3/filelist
https://packages.debian.org/bookworm/amd64/libmagickcore-6.q16-6/filelist

> once and the module doesn't need to know about threadlocks at all.

Looks mostly ok.  I need the following patch to get it to build on macOS:

diff --git a/src/interfaces/libpq-oauth/Makefile 
b/src/interfaces/libpq-oauth/Makefile
index 461c44b59c1..d5469ca0e11 100644
--- a/src/interfaces/libpq-oauth/Makefile
+++ b/src/interfaces/libpq-oauth/Makefile
@@ -28,8 +28,9 @@ OBJS = \
     oauth-utils.o

  SHLIB_LINK_INTERNAL = $(libpq_pgport_shlib)
-SHLIB_LINK = -lcurl
+SHLIB_LINK = -lcurl $(filter -lintl, $(LIBS))
  SHLIB_PREREQS = submake-libpq
+BE_DLLLIBS =

  SHLIB_EXPORTS = exports.txt

(The second change is not strictly required, but it disables the use of 
-bundle_loader postgres, since this is not a backend-loadable module.)

I don't know whether we need an exports file for libpq-oauth.  The other 
shared modules don't provide an export file, and I'm not sure whether 
this is even supported for shared modules.  I guess it doesn't hurt?

The PKG_CONFIG_REQUIRES_PRIVATE setting in libpq-oauth/Makefile is 
meaningless and can be removed.

In fe-auth-oauth.c, I note that dlerror() is not necessarily 
thread-safe.  Since there isn't really an alternative, I guess it's ok 
to leave it like that, but I figured it should be mentioned.

> see if I'm missing something obvious?

Not sure, the code looks correct at first glance.  However, you could 
also just keep the libpq-oauth strings in the libpq catalog.  There 
isn't really a need to make a separate one, since the versions you end 
up installing are locked to each other.  So you could for example in 
libpq's nls.mk just add

../libpq-oauth/oauth-curl.c

etc. to the files.

Maybe it would also make sense to make libpq-oauth a subdirectory of the 
libpq directory instead of a peer.

---

From: Andres Freund <[email-masked]>
Date: Mon, 7 Apr 2025 10:21:33 -0400

Hi,

On 2025-04-04 17:27:46 -0700, Jacob Champion wrote:
> += Load-Time ABI =
> +
> +This module ABI is an internal implementation detail, so it's subject to change
> +without warning, even during minor releases (however unlikely). The compiled
> +version of libpq-oauth should always match the compiled version of libpq.

Shouldn't we then include the *minor* version in the soname? I think otherwise
we run into the danger of the wrong library version being loaded in some
cases. Imagine a program being told with libpq to use via rpath. But then we
load the oauth module via a dlopen without a specified path - it'll just
search the global library locations.

Which actually makes me wonder if we ought to instead load the library from a
specific location...

> +TODO
> diff --git a/src/interfaces/libpq-oauth/exports.txt b/src/interfaces/libpq-oauth/exports.txt
> new file mode 100644
> index 00000000000..3787b388e04
> --- /dev/null
> +++ b/src/interfaces/libpq-oauth/exports.txt
> @@ -0,0 +1,4 @@
> +# src/interfaces/libpq-oauth/exports.txt
> +pg_fe_run_oauth_flow      1
> +pg_fe_cleanup_oauth_flow  2
> +pg_g_threadlock           3

The pg_g_threadlock thing seems pretty ugly. Can't we just pass that to the
relevant functions?  But more fundamentally, are we actually using
pg_g_threadlock anywhere? I removed all the releant code and the tests still
seem to pass?

> diff --git a/src/interfaces/libpq-oauth/meson.build b/src/interfaces/libpq-oauth/meson.build
> new file mode 100644
> index 00000000000..1834afbf7a5
> --- /dev/null
> +++ b/src/interfaces/libpq-oauth/meson.build
> @@ -0,0 +1,32 @@
> +# Copyright (c) 2022-2025, PostgreSQL Global Development Group
> +
> +if not libcurl.found() or host_system == 'windows'
> +  subdir_done()
> +endif

Why does this not work on windows? I don't see similar code in the removed
lines?

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Mon, 7 Apr 2025 10:43:40 -0400

Hi,

> Yes, this is correct.  We want a shared module, not a shared library, in

It's not entirely obvious to me that we do want that.

There recently was a breakage of building with PG on macos with meson, due to
the meson folks implementing a feature request to move away from using
bundles, as
1) bundles apparently aren't supported on iOS
2) there apparently aren't any restrictions left that require using bundles,
   and there haven't been for a while.

They've now reverted these changes, due to the postgres build failures that
caused as well as recognizing they probably moved too fast, but the iOS
portion seems like it could be relevant for us?

Afaict this library doesn't have unresolved symbols, due to just linking to
libpq. So I don't think we really need this to be a shared module?

> libdir.

Agreed. However, it seems like relocatability would be much more important for
something like libpq than server modules... Particularly on windows it'll
often just be shipped alongside the executable - which won't work if we load
from pklibdir or such.

> shared modules don't provide an export file, and I'm not sure whether this
> is even supported for shared modules.  I guess it doesn't hurt?

It does seem just using PGDLLEXPORT would suffice here.

> In fe-auth-oauth.c, I note that dlerror() is not necessarily thread-safe.

I sometimes really really hate posix.

Greetings,

Andres Freund

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 7 Apr 2025 18:38:19 +0200

> portion seems like it could be relevant for us?

Um, interesting.  AFAICT, the change you mention was reverted from the 
1.7 branch because it was accidentally backpatched, but it remains in 
master.

(For those just catching up:

https://github.com/mesonbuild/meson/issues/14240
https://github.com/mesonbuild/meson/pull/14340
https://github.com/mesonbuild/meson/commit/fa3f7e10b47d1f2f438f216f6c44f56076a01bfc
)

Overall, this seems like a good idea, as it removes a historical 
platform-specific particularity.  (I found a historical analysis at 
<https://stackoverflow.com/questions/2339679/>.)

But it does break existing users that add -bundle_loader, because 
-bundle_loader only works with -bundle and is rejected with -dynamiclib.

To test, I patched the makefiles to use -dynamiclib instead of -bundle, 
which also required removing -bundle_loader, and it also required adding 
-Wl,-undefined,dynamic_lookup.  This built correctly and generally worked.

But then you also run into a new variant of this issue:

https://www.postgresql.org/message-id/[email-masked]

Because there is no -bundle_loader, the symbol search order appears to 
be different, and so hash_search() gets found in the OS library first.

So this is all going to be a mess at some point sooner or later. :-(

> libpq. So I don't think we really need this to be a shared module?

Apart from the hard distinction on macOS, in terms of the build system, 
the distinction between "library" and "module" is mainly whether the 
resulting library gets a soname, version symlinks, and what directory it 
is installed in, so in that sense the discussion so far indicates that 
it should be a module.  I suppose on macOS we could link it like a 
library and install it like a module, but that would effectively create 
a third category, and I don't see why that would be worth it.

---

From: Jacob Champion <[email-masked]>
Date: Mon, 7 Apr 2025 09:41:25 -0700

Hi all,

Thanks for all the feedback! I'll combine them all into one email:

On Mon, Apr 7, 2025 at 6:59 AM Peter Eisentraut <[email-masked]> wrote:
> [...]

Hm, okay. I'll take a closer look at this, thanks.

> meaningless and can be removed.

Ah, right. Will do.

> to leave it like that, but I figured it should be mentioned.

Yeah. The XXX comments there are a reminder to me to lock the stderr
printing behind debug mode, since I hope most non-packaging people are
not going to be troubleshooting load-time errors. But see the
threadlock discussions below.

> ../libpq-oauth/oauth-curl.c

Oh, that's an interesting idea. Thanks, I'll give it a try.

> libpq directory instead of a peer.

Works for me.

On Mon, Apr 7, 2025 at 7:21 AM Andres Freund <[email-masked]> wrote:
> Shouldn't we then include the *minor* version in the soname?

No objection here.

> I think otherwise

Ah, you mean if the RPATH'd build doesn't have a flow, but the
globally installed one (with a different ABI) does? Yeah, that would
be a problem.

> Which actually makes me wonder if we ought to instead load the library from a

We could hardcode the disk location for version 1, I guess. I kind of
liked giving packagers the flexibility they're used to having from the
ld.so architecture, though. See below.

> relevant functions?

We can do it however we want, honestly, especially since the ABI isn't
public/stable. I chose this way just to ease review.

> But more fundamentally, are we actually using

If you have an older Curl installation, it is used. Newer libcurls
don't need it.

A future simplification could be to pull the use of the threadlock
back into libpq, and have it perform a one-time
dlopen-plus-Curl-initialization under the lock... That would also get
rid of the dlerror() thread safety problems. But that's an awful lot
of moving parts under a mutex, which makes me a little nervous.

> Why does this not work on windows? I don't see similar code in the removed

The Device Authorization flow is not currently implemented on Windows.

>    and there haven't been for a while.

Could you explain how this is related to .app bundles? I thought I was
just building a standard dylib.

> libpq. So I don't think we really need this to be a shared module?

Correct, no unresolved symbols. My naive understanding was that
distros were going to impose restrictions on an SONAME'd library that
we may not want to deal with.

> often just be shipped alongside the executable - which won't work if we load

Yeah, I really like the simplicity of "use the standard runtime
loader, just on-demand." Seems more friendly to the ecosystem.

Are there technical downsides of putting it into $libdir? I understand
there are "people" downsides, since we don't really want to signal
that this is a publicly linkable thing... but surely if you go through
the process of linking our library (which has no SONAME, includes the
major/minor version in its -l option, and has no pkgconfig) and
shoving a private pointer to a threadlock into it, you can keep all
the pieces when they break?

> It does seem just using PGDLLEXPORT would suffice here.

My motivation was to strictly identify the ABI that we intend libpq to
use, to try to future-proof things for everybody. Especially since
we're duplicating functions from libpq that we'd rather not be. (The
use of RTLD_LOCAL maybe makes that more of a belt-and-suspenders
thing.)

Are there any downsides to the exports file?

Thanks,
--Jacob

---

From: Andres Freund <[email-masked]>
Date: Mon, 7 Apr 2025 12:52:57 -0400

Hi,

On 2025-04-07 18:38:19 +0200, Peter Eisentraut wrote:
> Um, interesting.  AFAICT, the change you mention was reverted from the 1.7
> branch because it was accidentally backpatched, but it remains in master.

I think the plan is to either redesign it in master or to revert it.

> -bundle_loader only works with -bundle and is rejected with -dynamiclib.

Seems hard to imagine that somebody would inject -bundle_loader separately
from src/makefiles/Makefile.darwin?

> Because there is no -bundle_loader, the symbol search order appears to be
> different, and so hash_search() gets found in the OS library first.
>

Yikes, that is depressing / scary.  I wonder if we ought to rename our
hash_search with some macro magic or such regardless of using -bundle or not.

> Apart from the hard distinction on macOS, in terms of the build system, the
> distinction between "library" and "module" is mainly whether the resulting
> library gets a soname, version symlinks, and what directory it is installed
> in, so in that sense the discussion so far indicates that it should be a
> module.

I don't think that happens if you don't specify a soname etc. And we'd need to
adjust the install dir either way, I think?

> I suppose on macOS we could link it like a library and install it
> like a module, but that would effectively create a third category, and I
> don't see why that would be worth it.

I think there are postgres clients for iphone, not sure if they use
libpq. Today libpq might actually cross-build successfully for iOS [1]. But if
we use shared_module() that won't be the case for libpq-oauth.

Anyway, I don't have a strong position on this, I just wanted to bring it up
for consideration.

Greetings,

Andres Freund

[1] I couldn't immediately quickly figure out how to install additional SDKs
on the commandline on macos an then gave up before attaching a monitor to my
mac mini.

---

From: Andres Freund <[email-masked]>
Date: Mon, 7 Apr 2025 13:05:43 -0400

Hi,

> globally installed one (with a different ABI) does? Yeah, that would

That and more: Even if the RPATH libpq does have oauth support, the
libpq-oauth won't necessarily be at the front of the global library search
path.  So afaict you'll often get a different libpq-oauth.

Except perhaps on macos, where all this stuff works differently again. But I
managed to unload the required knowledge out of my brain and don't want to
further think about that :)

> public/stable. I chose this way just to ease review.

I found it rather confusing that libpq looks up a symbol and then sets
libpq-oauth's symbol to a pointer in libpq's namespace.

> If you have an older Curl installation, it is used. Newer libcurls

Oh, wow. We hide the references to pg_g_threadlock behind a friggin macro?
That's just ...

Not your fault, I know...

> of moving parts under a mutex, which makes me a little nervous.

I still think we should simply reject at configure time if curl init isn't
threadsafe ;)

> just building a standard dylib.

The other kind of bundles (what on earth apple was thinking with the naming
here I don't know). Stuff liked with ld -bundle.

> use of RTLD_LOCAL maybe makes that more of a belt-and-suspenders

PGDLLEXPORT serves that purpose too, fwiw.  These days we use compiler flags
that restrict function visibility of everything not annotated with
PGDLLEXPORT.

However - that's all in c.h, port/win32.h,port/cygwin.h, , which libpq headers
might not want to include.

> Are there any downsides to the exports file?

I think it's fine either way.

Greetings,

Andres Freund

---

From: Jacob Champion <[email-masked]>
Date: Mon, 7 Apr 2025 10:32:31 -0700

> path.  So afaict you'll often get a different libpq-oauth.

ldopen() should respect RPATH, though? Either way, I agree with
pushing the minor version into the name (or else deciding that we will
keep the ABI completely stable across minor version bumps; not sure I
want to guarantee that just yet).

> libpq-oauth's symbol to a pointer in libpq's namespace.

Yeah, I think a one-time init call would make this nicer.

> I still think we should simply reject at configure time if curl init isn't

Practically speaking, I don't think that's a choice we can make. For
example, RHEL won't have threadsafe Curl until 10.

> here I don't know). Stuff liked with ld -bundle.

Ah, some new corner-case magic to learn...

> These days we use compiler flags

Hm, I missed/forgot that. That is nice. Personally I like having a
single file document the exports, so I'll keep it that way for now
unless there are objections, but it's good to know it's not necessary.

Thanks,
--Jacob

---

From: Christoph Berg <[email-masked]>
Date: Mon, 7 Apr 2025 20:47:18 +0200

Re: Jacob Champion
> > Shouldn't we then include the *minor* version in the soname?

Mmmmh. Since we are currently only talking about 3 symbols, it doesn't
sound very likely that we'd have to bump this in a major branch.
Putting the minor version into the filename would make looking at
package diffs harder when upgrading. Do we really need this as opposed
to some hardcoded number like libpq.so.5.18 ?

Perhaps reusing the number from libpq.so.5.18 also for this lib would
be the way to go?

> ld.so architecture, though. See below.

pkglibdir or a subdirectory of it might make sense.

Though for Debian, I'd actually prefer
/usr/lib/$DEB_HOST_MULTIARCH/libpq/libpq-oauth...
since the libpq packaging is independent from the major version
packaging.

> the pieces when they break?

The Debian Policy expectation is that everything in libdir is a proper
library that could be linked to, and that random private stuff should
be elsewhere. But if being able to use the default lib search path is
an important argument, we could put it there.

Christoph

---

From: Jacob Champion <[email-masked]>
Date: Mon, 7 Apr 2025 14:49:43 -0700

> sound very likely that we'd have to bump this in a major branch.

The ABI extends to the pointers we're using, though. This module uses
PGconn* internals and libpq-int.h. [1]

> Perhaps reusing the number from libpq.so.5.18 also for this lib would

That doesn't address Andres' concern, though; if multiple
installations all use libpq.so.5.18, they still can't necessarily mix
and match.

In fact you can't mix and match across different settings of
ENABLE_SSL/GSS/SSPI, either. So I guess that nudges me towards
pkglibdir/<some subdirectory>, to avoid major pain for some unlucky
end user.

> since the libpq packaging is independent from the major version

Not sure I understand this. Do you mean you'd patch our lookup for
Debian, to find it there instead of pkglibdir? I don't think we can
adopt that ourselves, for the same reasons as above; the two sides
have to be in lockstep.

> an important argument, we could put it there.

I was hoping the default lib search would make your life (and ours)
easier. If it doesn't, I can lock it down.

Thanks,
--Jacob

[1] Future work ideas in this area include allowing other people to
compile their own loadable flow plugin, so that the utilities can use
it. (Only Device Authorization can be used by psql et al, for 18.) At
that point, developers will need a limited API to twiddle the
connection handle, and our builtin flow(s?) could use the same API.
But that's not work we can tackle for 18.

---

From: Christoph Berg <[email-masked]>
Date: Mon, 7 Apr 2025 23:58:23 +0200

Re: Jacob Champion
> installations all use libpq.so.5.18, they still can't necessarily mix

Well the whole world is linking against libpq5.so and not breaking
either. Why is this module worse? (I guess the answer is internal data
structures... but does it have to be worse?)

> adopt that ourselves, for the same reasons as above; the two sides

Because pkglibdir would be something like /usr/lib/postgresql/17/lib,
even when there is only one libpq5 package for all major server
versions on Debian. So if you have postgresql-16 installed, you'd end
up with

/usr/lib/postgresql/16/{bin,lib}  everything from PG 16
/usr/lib/x86_64-linux-gnu/libpq*  libpq5
/usr/lib/postgresql/17/lib/libpq-oauth.so

... which is weird.

> But that's not work we can tackle for 18.

Perhaps keep things simple for PG18 and choose a simple filename and
location. If future extensions need something more elaborate, we can
still switch later.

Christoph

---

From: Jacob Champion <[email-masked]>
Date: Mon, 7 Apr 2025 15:26:47 -0700

On Mon, Apr 7, 2025 at 2:58 PM Christoph Berg <[email-masked]> wrote:
> Why is this module worse? (I guess the answer is internal data

It doesn't have to be, in general, and the coupling surface is small
enough (libpq_append_conn_error) that we have a relatively easy path
toward decoupling it in the future. But for 18, I suspect no one will
be happy with me if I try to turn that inside out right this instant.
The goal was just to turn an internal implementation detail into a
delay-loaded internal implementation detail.

> /usr/lib/postgresql/17/lib/libpq-oauth.so

Weird, sure -- but it's correct, right? Because you have PG17's OAuth
flow installed.

If someone comes to the list with a flow bug in three years, and I ask
them what version they have installed, and they tell me "PG16, and
it's loading /usr/lib/aarch64-linux-gnu/libpq/libpq-oauth.so." That
won't be incredibly helpful IMHO.

> location. If future extensions need something more elaborate, we can

Sounds good. Any opinions from the gallery on what a "libpq plugin
subdirectory" in pkglibdir should be called? ("client", "modules",
"plugins"...?) Is there still a good reason to put any explicit
versioning into the filename if we do that?

Thanks,
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Mon, 7 Apr 2025 19:10:10 -0700

On Mon, Apr 7, 2025 at 3:26 PM Jacob Champion
<[email-masked]> wrote:
> "plugins"...?)

Hm, one immediate consequence of hardcoding pkglibdir is that we can
no longer rely on LD_LIBRARY_PATH for pre-installation testing.
(Contrast with the server, which is able to relocate extension paths
based on its executable location.)

--Jacob

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 8 Apr 2025 10:32:21 -0400

On Tue, Apr  8, 2025 at 12:34:02PM +0200, Daniel Gustafsson wrote:
> > On 8 Apr 2025, at 04:10, Jacob Champion <[email-masked]> wrote:
> > 
> That strikes me as a signifant drawback.

Uh, where are we on the inclusion of curl in our build?  Maybe it was
explained but I have not seen it.  I still see
src/interfaces/libpq/fe-auth-oauth-curl.c.

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 08:04:22 -0700

On Tue, Apr 8, 2025 at 7:32 AM Bruce Momjian <[email-masked]> wrote:
> explained but I have not seen it.

The above is discussing a patch to split this into its own loadable
module. Andres and Christoph's feedback has been shaping where we put
that module, exactly.

Thanks,
--Jacob

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 8 Apr 2025 11:13:51 -0400

> module. Andres and Christoph's feedback has been shaping where we put

Uh, I was afraid that was the case, which is why I asked.  We have just
hit feature freeze, so it is not good that we are still "shaping" the
patch.  Should we consider reverting it?  It is true we still "adjust"
patches after feature freeze.

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 8 Apr 2025 11:22:21 -0400

On Tue, Apr  8, 2025 at 11:20:11AM -0400, Andres Freund wrote:
> Hi,
> 
> On 2025-04-08 11:13:51 -0400, Bruce Momjian wrote:
> You brought the dependency concern up well after the feature was merged, after
> it had been in development for a *long* time. It wasn't a secret that it had a
> dependency on curl.  I don't think it's fair to penalize a feature's authors
> to not finish implementing a complicated and completely new requirement within
> 17 days.

Fair point --- I was just asking.

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 8 Apr 2025 12:14:32 -0400

On Tue, Apr  8, 2025 at 06:01:42PM +0200, Wolfgang Walther wrote:
> Jacob Champion:
> Wasn't sure where to put this exactly, the thread is long and I couldn't
> find any discussion around it:
> 
> How does the proposal with a loadable module affect a static libpq.a?
> 
> I have not tried, yet, but is my assumption correct, that I could build a
> libpq.a with oauth/curl support on current HEAD?
> 
> If yes, would that still be an option after the split?

How does this patch help us avoid having to handle curl CVEs and its
curl's additional dependencies?  As I understand the patch, it makes
libpq _not_ have additional dependencies but moves the dependencies to a
special loadable library that libpq can use.

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 09:14:54 -0700

> How does the proposal with a loadable module affect a static libpq.a?

The currently proposed patch would have you package and install a
separate .so module implementing OAuth, which the staticlib would load
once when needed. Similarly to how you still have to somehow
dynamically link your static app against Curl.

As a staticlib user, how do you feel about that?

> I have not tried, yet, but is my assumption correct, that I could build
> a libpq.a with oauth/curl support on current HEAD?

Yes.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 09:17:03 -0700

> special loadable library that libpq can use.

It allows packagers to ship the OAuth library separately, so end users
that don't want the additional exposure don't have to install it at
all.

--Jacob

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 8 Apr 2025 12:33:55 -0400

> that don't want the additional exposure don't have to install it at

Okay, so how would they do that?  I understand how that would happen if
it was an external extension, but how if it is under /src or /contrib.

FYI, I see a good number of curl CVEs:

	https://curl.se/docs/security.html

Would we have to put out minor releases for curl CVEs?  I don't think we
have to for OpenSSL so would curl be the same?

I am asking these questions now so we can save time in getting this
closed.

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 09:43:01 -0700

> it was an external extension, but how if it is under /src or /contrib.

By adding the new .so to a different package. For example, RPM specs
would just let you say "hey, this .so I just built doesn't go into the
main client package, it goes into an add-on that depends on the client
package." It's the same way separate client and server packages get
generated from the same single build of Postgres.

> Would we have to put out minor releases for curl CVEs?

In general, no.

Thanks,
--Jacob

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 8 Apr 2025 12:48:15 -0400

On Tue, Apr  8, 2025 at 06:42:19PM +0200, Daniel Gustafsson wrote:
> > On 8 Apr 2025, at 18:33, Bruce Momjian <[email-masked]> wrote:
> 
> Why do you envision this being different from all other dependencies we have?
> For OpenSSL we also happily build against a version (and until recently,
> several versions) which is EOL and don't even receive security fixes.

I don't know.  I know people scan our downloads and report when the
scanners detect something, but I am unclear what those scanners are
doing.  Would they see some new risks with curl?

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 8 Apr 2025 12:49:58 -0400

> generated from the same single build of Postgres.

Do we have any idea how many packagers are interested in doing this?

> > Would we have to put out minor releases for curl CVEs?

Good.

FYI, I saw bug bounty dollar amounts next to each curl CVE:

No wonder some people ask for bounties.

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 09:50:26 -0700

On Tue, Apr 8, 2025 at 9:32 AM Wolfgang Walther <[email-masked]> wrote:
> And that should also not be a problem for distributions - they could offer a libpq and a libpq_oauth package, where only one of them can be installed at the same time, I guess? *

My outsider understanding is that maintaining this sort of thing
becomes a major headache, because of combinatorics. You don't really
want to ship a libpq and libpq-with-gss and libpq-with-oauth and
libpq-with-oauth-and-gss and ...

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 10:00:56 -0700

> Do we have any idea how many packagers are interested in doing this?

I'm not sure how to answer this. The primary drivers from the dev side
are you and Tom, I think. Christoph seems to be on board with a split
as long as we don't make his life harder. Wolfgang appears to be a
packager who would not make use of a split (and in fact cannot).

--Jacob

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 8 Apr 2025 13:02:11 -0400

On Tue, Apr  8, 2025 at 06:57:18PM +0200, Wolfgang Walther wrote:
> That would only be the case, if you were to consider those other
> dependencies as "dangerous" as cURL. But we already depend on them. So if
> it's really the case that cURL is that much worse, that we consider loading
> it as a module... then the combinatorics should not be a problem either.
> 
> However, if the other deps are considered problematic as well, then the ship
> has already sailed, and there is not point for a special case here anymore.

Yes, I think this is what I am asking too.  For me it was curl's
security reputation and whether that would taint the security reputation
of libpq. For Tom, I think it was the dependency additions.

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 8 Apr 2025 13:03:10 -0400

> packager who would not make use of a split (and in fact cannot).

Okay, I have just posted a more detailed email about my security
concern, so let's look at that.  I am ready to admit I am wrong.

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 10:04:26 -0700

On Tue, Apr 8, 2025 at 9:57 AM Wolfgang Walther <[email-masked]> wrote:
> if it's really the case that cURL is that much worse

(it is not, but I am sympathetic to the argument that if you don't use
it, you don't need it in the process space)

> However, if the other deps are considered problematic as well, then the
> ship has already sailed, and there is not point for a special case here
> anymore.

I think this line of argument is unlikely to find traction. Upthread
there were people asking if we could maybe split out other
possibly-unused dependencies in the future, like Kerberos.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 10:13:46 -0700

On Tue, Apr 8, 2025 at 10:10 AM Wolfgang Walther
<[email-masked]> wrote:
> And if that means making libpq modular at run-time, then this should be planned and built with all deps, and other use-cases (like static linking) in mind - and not like it is right now.

I think that'd be neat in concept, but specifically this thread is
discussing a PG18 open item. For future releases, if we're happy with
how Curl gets split out, maybe that would be fuel for other
delay-loaded client dependencies. I'm not sure.

--Jacob

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 8 Apr 2025 13:15:47 -0400

> delay-loaded client dependencies. I'm not sure.

Well, if we think we are going to do that, it seems we would need a
different architecture than the one being proposed for PG 18, which
could lead to a lot of user/developer API churn.

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 10:19:21 -0700

> could lead to a lot of user/developer API churn.

A major goal of the current patch proposal is to explicitly hide this
from the end-user and public APIs. So it can be changed without public
breakage. It can't be hidden from packagers, of course, but that's the
point of the feature request.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 10:36:08 -0700

> That strikes me as a signifant drawback.

Yeah, but it's one of those things that feels like it must have been
solved by the others in the space. Once it's installed, the concern
goes away (unless you demand absolute relocatability without
recompilation). I'll take a look at how libkrb/libmagick do their
testing.

If it somehow turns out to be impossible, one option might be to shove
a more detailed ABI identifier into the name. In other words, builds
without ENABLE_SSL/GSS/SSPI or whatever get different names on disk.
That doesn't scale at all, but it's a short-term option that would put
more pressure on a medium-term stable ABI.

Thanks,
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 10:44:58 -0700

On Tue, Apr 8, 2025 at 9:32 AM Wolfgang Walther <[email-masked]> wrote:
> This means that shipping another .so file will not happen with this approach. Assuming OAuth will be picked up by some of the bigger providers, that would... make me feel quite bad about it, actually.

It occurs to me that I didn't respond to this point explicitly. I
would like to avoid making your life harder.

Would anybody following along be opposed to a situation where
- dynamiclib builds go through the dlopen() shim
- staticlib builds always rely on statically linked symbols

Or do we need to be able to mix and match?

--Jacob

---

From: Andres Freund <[email-masked]>
Date: Tue, 8 Apr 2025 14:11:19 -0400

Hi,

> of libpq. For Tom, I think it was the dependency additions.

I'd say that curl's security reputation is higher than most of our other
dependencies. We have dependencies for libraries with regular security issues,
with those issues at times not getting addressed for prolonged amounts of
time.

I do agree that there's an issue increasing libpq's indirect footprint
substantially, but I don't think that's due to curl's reputation or
anything. It's just needing a significantly higher number of shared libraries,
which comes with runtime and distribution overhead.

Greetings,

Andres Freund

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 8 Apr 2025 14:25:20 -0400

> with those issues at times not getting addressed for prolonged amounts of

I see curl CVEs regularly as part of Debian minor updates, which is why
I had concerns, but if it is similar to OpenSSL, and better than other
libraries that don't even get CVEs, I guess it okay.  However, is this
true for libpq libraries or database server libraries.  Does it matter?

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 12:07:51 -0700

> recompilation). I'll take a look at how libkrb/libmagick do their

Perhaps unsurprisingly, they inject different lookup paths via
envvars. We could do the same (I have FUD about the security
characteristics)...

> a more detailed ABI identifier into the name.

...but I wonder if I can invert the dependency on
libpq_append_conn_error entirely, and remove that part of the ABI
surface, then revisit the discussion on `-<major>.so` vs
`-<major>-<minor>.so`.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 12:22:55 -0700

On Tue, Apr 8, 2025 at 11:25 AM Bruce Momjian <[email-masked]> wrote:
> However, is this

The dependency on Curl is through libpq. We have some server-side
features that pull in libpq and would transitively depend on Curl. But
for Curl to be initialized server-side, the two peers still have to
agree on the use of OAuth.

It seems unlikely that users would opt into that for, say,
postgres_fdw in PG18, because the Device Authorization flow is the
only one we currently ship, and it's intended for end users. A flow
that prints a code to stderr is not very helpful for your proxy.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 14:32:09 -0700

> Oh, that's an interesting idea. Thanks, I'll give it a try.

A consequence of this is that our copy of libpq_binddomain isn't using
the same mutex as libpq's copy to protect the "libpq-18" message
domain. We could discuss whether or not it matters, since we don't
support Windows, but it doesn't feel architecturally sound to me. If
we want to reuse the same domain, I think the module should be using
libpq's libpq_gettext(). (Which we could do, again through the magic
of dependency injection.)

> > libpq directory instead of a peer.

It does not, however, work for our $(recurse) setup in the makefiles
-- a shared library depending on a parent directory's shared library
leads to infinite recursion, with the current tools -- so I'll keep it
at the current directory level for now.

Thanks,
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 8 Apr 2025 19:57:47 -0700

On Tue, Apr 8, 2025 at 2:32 PM Jacob Champion
<[email-masked]> wrote:
> I think the module should be using

To illustrate what I mean, v3 introduces an initialization function
that names the three internal dependencies (libpq_gettext,
pg_g_threadlock, and conn->errorMessage) explicitly. I decided not to
attempt injecting the variadic libpq_append_conn_error function, and
instead focus a level below it, since we must depend directly on
libpq_gettext anyway.

This is maybe overkill, if it's decided that the two halves must
always come from the same build, but I think it should decouple the
two sides enough that this is now a question of user experience rather
than ABI correctness.

Is it acceptable/desirable for a build, which has not been configured
--with-libcurl, to still pick up a compatible OAuth implementation
installed by the distro? If so, we can go with a "bare" dlopen(). If
that's not okay, I think we will probably need to use pkglibdir or
some derivative, and introduce a way for tests (and users?) to
override that directory selection. Unless someone has a good idea on
how we can split the difference.

--Jacob

---

From: Christoph Berg <[email-masked]>
Date: Wed, 9 Apr 2025 10:14:32 +0200

Re: Jacob Champion
> how we can split the difference.

One design goal could be reproducible builds-alike, that is, have
libpq configured with or without libcurl be completely identical, and
the feature being present is simply the libpq-oauth.so file existing
or not. That might make using plain dlopen() more attractive.

Christoph

---

From: Jacob Champion <[email-masked]>
Date: Wed, 9 Apr 2025 05:50:56 -0700

> or not. That might make using plain dlopen() more attractive.

I think that's more or less what the current v3 does, but Jelte's
point (which is upthread for me but downthread for others :D) is that
making them identical might not actually be a desirable thing in this
situation, because if you don't compile --with-libcurl, when you test
that the feature is disabled you might potentially find that it is
not.

On Wed, Apr 9, 2025 at 1:39 AM Jelte Fennema-Nio <[email-masked]> wrote:
> How about ifdef-ing away the dlopen call if --with-libcurl is not specified.

This sounds like a pretty decent, simple way to go. Christoph, does
that ring any alarm bells from your perspective?

Thanks,
--Jacob

---

From: Christoph Berg <[email-masked]>
Date: Wed, 9 Apr 2025 19:44:40 +0200

Re: Jacob Champion
> that ring any alarm bells from your perspective?

Ok for me. The opposite that I said in the other mail was just a
suggestion.

Christoph

---

From: Jacob Champion <[email-masked]>
Date: Wed, 9 Apr 2025 16:08:33 -0700

> Ok for me. The opposite that I said in the other mail was just a

Cool, thanks! v4 does it that way. It also errors out at configure
time if you demand libpq-oauth on a platform that does not support it.

The Autoconf side was still polluting LIBS and CPPFLAGS with Curl
stuff. I have isolated them in v4, with some additional m4
boilerplate. IMO this makes the subtle difference between USE_LIBCURL
(which means the libpq-oauth module is enabled to build) and
HAVE_LIBCURL (which means you have libcurl installed locally) even
more confusing.

Christoph noted that this was also confusing from the packaging side,
earlier, and Daniel proposed -Doauth-client/--with-oauth-client as the
feature switch name instead. Any objections? Unfortunately it would
mean a buildfarm email is in order, so we should get it locked in.

Next up: staticlibs.

Thanks,
--Jacob

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Thu, 10 Apr 2025 07:42:28 +0800

On Thu, Apr 10, 2025, 07:08 Jacob Champion <[email-masked]>
wrote:

> feature switch name instead.
>

+1

Next up: staticlibs.

I think your suggestion of not using any .so files would best there (from w
user perspective). I'd be quite surprised if a static build still resulted
in me having to manage shared library files anyway.

---

From: Jacob Champion <[email-masked]>
Date: Thu, 10 Apr 2025 17:12:46 -0700

On Wed, Apr 9, 2025 at 4:42 PM Jelte Fennema-Nio <[email-masked]> wrote:
> I think your suggestion of not using any .so files would best there (from w user perspective). I'd be quite surprised if a static build still resulted in me having to manage shared library files anyway.

Done this way in v5. I had planned to separate the implementations by
a #define, but I ran into issues with Makefile.shlib, so I split the
shared and dynamic versions into separate files. I just now realized
that we do something about this exact problem in src/common, so I'll
see if I can copy its technique for the next go round.

In the next version, I'll try to add --with-oauth-client while keeping
--with-libcurl as an alias, to let the buildfarm migrate off of it
before it's removed.

Thanks!
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Mon, 14 Apr 2025 09:12:53 -0700

On Fri, Apr 11, 2025 at 9:21 AM Wolfgang Walther
<[email-masked]> wrote:
> I tried to apply this patch to nixpkgs' libpq build [1]. First, I pinned
> a recent commit from master (one where the v5 patch will apply cleanly
> later) and enabled --with-libcurl [2].

(The [2] link is missing, I think.)

> 2. The statically linked build fails during configure:

I'm confused by this -- the build produces staticlibs alongside the
dynamically linked ones, so that's what I've been testing against.
What different options do you pass to configure for a "statically
linked build"?

>    undefined reference to `psl_is_cookie_domain_acceptable'
>    undefined reference to `nghttp2_session_check_request_allowed'
>
> I assume the many libs listed in Libs.private in libcurl.pc are not
> added automatically for this check?

Not unless there is some magic in PKG_CHECK_MODULES I've never heard
of (which is entirely possible!). Furthermore I imagine that the
transitive dependencies of all its dependencies are not added either.

Does your build method currently work for dependency forests like
libgssapi_krb5 and libldap? (I want to make sure I'm not accidentally
doing less work than we currently support for those other deps, but
I'm also not planning to add more feature work as part of this
particular open item.)

> I tried adding "make submake-libpq-oauth", but that doesn't exist.

There is no submake for this because no other targets depend on it.
Currently I don't have any plans to add one (but -C should work).

> When I do "make -C src/interfaces/libpq-oauth", I get this error:
>
>    make: *** No rule to make target 'oauth-curl.o', needed by
> 'libpq-oauth-18.so'.  Stop.

I cannot reproduce this. The CI seems happy, too. Is this patch the
only modification you've made to our build system, or are there more
changes?

I'm about to rewrite this part somewhat, so a deep dive may not be very helpful.

Thanks,
--Jacob

---

From: Wolfgang Walther <[email-masked]>
Date: Mon, 14 Apr 2025 20:27:46 +0200

Wolfgang Walther:
 > So yes, not related to your patch. I do understand that PostgreSQL's
 > autoconf build system is not designed for "static only", I am certainly
 > not expecting you to fix that.
 >
 > I think meson will do better here, but I was not able to make that work,
 > yet.
I did a basic meson build. Full postgresql package, not libpq-only.

The static-only build just works. On master that is. Same as the regular 
build.

So yes, meson will handle the static stuff much better.

 > I just tried the same thing on the bigger postgresql package, where the
 > full build is run and not only libpq / libpq-oauth. It fails with the
 > same error. No rule for oauth-curl.o.
Applying the v5 patch to the above meson build, will give me a different 
error. This time for both the static-only and the regular build:

   src/interfaces/libpq-oauth/meson.build:18:22: ERROR: File 
oauth-curl.c does not exist.

This.. clears it up, because that file is indeed missing for me on disk. 
I assume that's because this file is tracked as a rename in the v5 
patch. I can apply this with git, but not directly in the nix build 
system. TIL, I need to use "fetchpatch2" instead of "fetchpatch" for 
that. Sure thing.

So, with the patch applied correctly, I get the following:

1. Meson regular build:

libpq-oauth-18.so
libpq.so
libpq.so.5
libpq.so.5.18

The libpq.so file has references to dlopen and libpq-auth-18.so, cool.

2. Meson static-only build:

libpq.a
libpq-oauth-18.a

The libpq.a file has no references to dlopen, but plenty of references 
to curl stuff.

I'm not sure what the libpq-oauth-18.a file is for.

3. Back to the lipq-only build with autoconf, from where I started. I 
only need to add the following line:

make -C src/interfaces/libpq-oauth install

and get this:

libpq-oauth-18.so
libpq.so
libpq.so.5
libpq.so.5.18

Sweet!

4. Of course the static-only build does not work with autoconf, but 
that's expected.

So, sorry for the noise before. Now, that I know how to apply patches 
with renames, I will try your next patch as well.

Best,

Wolfgang

---

From: Jacob Champion <[email-masked]>
Date: Mon, 14 Apr 2025 12:14:20 -0700

> This.. clears it up, because that file is indeed missing for me on disk.

Aha! Okay, glad I don't need to track that down.

> The libpq.a file has no references to dlopen, but plenty of references

Which references? libpq-oauth should be the only thing using Curl symbols:

    $ nm src/interfaces/libpq/libpq.a | grep --count curl
    0
    $ nm src/interfaces/libpq-oauth/libpq-oauth-18.a | grep --count curl
    116

> I'm not sure what the libpq-oauth-18.a file is for.

That implements the flow. You'll need to link that into your
application or it will complain about missing flow symbols. (I don't
think there's an easy way to combine the two libraries in our Autoconf
setup; the only ways I can think of right now would introduce a
circular dependency between libpq and libpq-oauth...)

Thanks!
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Mon, 14 Apr 2025 13:17:13 -0700

On Mon, Apr 14, 2025 at 12:46 PM Wolfgang Walther
<[email-masked]> wrote:
> But that means we'd need a -lpq-oauth-18 or something like that in
> Libs.private in libpq.pc, right?

I believe so. I'm in the middle of the .pc stuff right now; v6 should
have the fixes as long as I don't get stuck.

Thanks,
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Mon, 14 Apr 2025 17:13:35 -0700

> have the fixes as long as I don't get stuck.

Done in v6-0001. I think this is now architecturally complete, so if
reviewers are happy I can work on docs and the commit message. As a
summary:

- We provide a libpq-oauth-18.so module for shared builds, and a
corresponding .a for static builds, when OAuth is enabled.
- Platforms which cannot support the builtin flow now error out if you
request OAuth at configure time.
- When OAuth is enabled and there's no custom client flow, libpq.so
loads the module via dlopen(), which respects RPATH/LD_LIBRARY_PATH et
al. If it's not installed, OAuth doesn't continue.
- Static builds must link libpq-oauth-18.a explicitly. libpq.pc now
puts -lpq-oauth-18 in Libs.private, and libcurl in Requires.private.
- Internally, we compile separate versions of fe-auth-oauth.c to
handle the different cases (disabled, dynamic, static). This is
borrowed from src/common.
- The only new export from libpq is appendPQExpBufferVA. Other
internal symbols are shared with libpq-oauth via dependency injection.

v6-0002 is a WIP rename of the --with-libcurl option to
--with-oauth-client. I'm not sure I have all of the Meson corner cases
with auto_features figured out, but maybe it doesn't matter since it's
temporary (it targets a total of seven buildfarm animals, and once
they've switched we can remove the old name). I have added a separate
open item for this.

Thanks,
--Jacob

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 15 Apr 2025 14:31:50 +0200

> mean a buildfarm email is in order, so we should get it locked in.

We had that discussion early in the development, and I still think it's 
not the right choice.

The general idea, at least on the Autoconf side, is that --with-FOO 
means enable all the features that require library FOO.  For example, 
--with-ldap enables all the LDAP-related features, including 
authentication support in libpq, authentication support in the server, 
and service lookup in libpq.  --with-[open]ssl enables all the features 
that use OpenSSL, including SSL support in the client and server but 
also encryption support in pgcrypto.

The naming system you propose has problems:

First, what if we add another kind of "oauth-client" that doesn't use 
libcurl, how would you extend the set of options?

Second, what if we add some kind of oauth plugin for the server that 
uses libcurl, how would you extend the set of options?

If you used that system for options in the ldap or openssl cases, you'd 
end up with maybe six options (and packagers would forget to turn on 
half of them).  But worse, what you are hiding is the information what 
dependencies you are pulling in, which is the actual reason for the 
options.  (If there was no external dependency, there would be no option 
at all.)

This seems unnecessarily complicated and inconsistent.  Once you have 
made the choice of taking the libcurl dependency, why not build 
everything that requires it?

(Nitpick: If you go with this kind of option, it should be --enable-XXX 
on the Autoconf side.)

---

From: Christoph Berg <[email-masked]>
Date: Tue, 15 Apr 2025 17:34:19 +0200

Re: Peter Eisentraut
> But worse, what you are hiding is the information what dependencies
> you are pulling in, which is the actual reason for the options.  (If there
> was no external dependency, there would be no option at all.)
> 
> This seems unnecessarily complicated and inconsistent.  Once you have made
> the choice of taking the libcurl dependency, why not build everything that
> requires it?

I agree with this reasoning and retract my suggestion to rename the option.

Thanks for the explanation,
Christoph

---

From: Jacob Champion <[email-masked]>
Date: Tue, 15 Apr 2025 10:53:07 -0700

On Tue, Apr 15, 2025 at 5:31 AM Peter Eisentraut <[email-masked]> wrote:
> > earlier,

Since Christoph has withdrawn the request, I will drop -0002.

However, I'll go ahead and put some of my opinions down on paper here:

> means enable all the features that require library FOO.

I don't think this is particularly user-friendly if it's not obvious
what feature is enabled by FOO.

LDAP? PAM? Sure. SSL? Eh, I think the pgcrypto coupling is a little
strange -- that's not implied by "SSL" at all! -- but it's not
problematic enough to complain loudly. --with-gssapi selects... some
dependency... which may or may not come from a particular library.
--with-bsd-auth doesn't add any library dependencies at all, instead
depending on the kernel, but it makes sense.

But there's no connection between "libcurl" and "OAuth Device
Authorization flow" in anyone's mind except the people who have worked
on that feature.

If the argument is that we'd need to switch to --enable-oauth-client
rather than --with-oauth-client, that works for me. But I don't quite
understand the desire to stick to the existing configuration
methodology for something that's very different from an end-user
perspective.

> libcurl, how would you extend the set of options?

With an extension to the values that you can provide to
--with-oauth-client, similarly to what was originally proposed for
--with-ssl.

> uses libcurl, how would you extend the set of options?

With a new option.

But let me turn this around, because we currently have the opposite
problem: if someone comes in and adds a completely new feature
depending on libcurl, and you want OAuth but you do not want that new
feature -- or vice-versa -- what do you do? In other words, what if
your concern is not with libcurl, but with the feature itself?

> But worse, what you are hiding is the information what

I'm not sure I agree, either practically or philosophically. I like to
see the build dependencies, definitely, but I also like to see the
features. (Meson will make both things visible separately, for that
matter.)

> everything that requires it?

Simply because the end user or packager might not want to.

In any case -- I won't die on this particular hill, and I'm happy to
continue forward with 0001 alone.

Thanks!
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 15 Apr 2025 11:02:13 -0700

> I agree with this reasoning and retract my suggestion to rename the option.

(Thank you for chiming in; having the packager feedback has been
extremely helpful.)

While I have you, may I ask whether you're okay (from the packager
perspective) with the current division of dynamic and static
behaviors?

Dynamic: --with-libcurl builds a runtime-loadable module, and if you
don't install it, OAuth isn't supported (i.e. it's optional)
Static: --with-libcurl builds an additional linkable staticlib, which
you must link into your application (i.e. not optional)

I want to make absolutely sure the existing packager requests are not
conflicting. :D

Thanks,
--Jacob

---

From: Christoph Berg <[email-masked]>
Date: Tue, 15 Apr 2025 20:57:32 +0200

Re: Jacob Champion
> Authorization flow" in anyone's mind except the people who have worked

Fwiw that was exactly the reason I originally voiced the idea to
rename.

> your concern is not with libcurl, but with the feature itself?

What made me reconsider was Peter saying that what defines the blast
radius of some feature is usually the extra dependency pulled in. If
you don't like tracking OpenSSL problems, build without it. If you
don't like libcurl, build without it. That's the "we are going to be
hated by security scanner people" argument that brought up this
sub-thread.

Now if the feature itself were a problem, that might change how
configuration should be working. Is "libpq can now initiate oauth
requests" something people would like to be able to control?

Re: Jacob Champion
> don't install it, OAuth isn't supported (i.e. it's optional)

Ok.

> you must link into your application (i.e. not optional)

Debian does not care really about static libs. We are currently
shipping libpq.a, but if it breaks in any funny way, we might as well
remove it.

Christoph

---

From: Robert Haas <[email-masked]>
Date: Tue, 15 Apr 2025 15:21:33 -0400

> We had that discussion early in the development, and I still think it's

I strongly agree. I think it will not be long before somebody
implements a second feature depending on libcurl, and there's no
precedent for the idea that we allow those features to be enabled and
disabled individually. If that turns out to be something that is
wanted, then since this will be a separate library, a packager can
choose not to ship it, or to put it in a separate package, if they
wish. If there's REALLY a lot of demand for a separate enable/disable
switch for this feature then we can consider making this an exception
to what we do for all other dependent libraries, but I bet there won't
be. I can imagine someone not wanting libcurl on their system on the
theory that it would potentially open up the ability to download data
from arbitrary URLs which might be considered bad from a security
posture -- but I don't really see why someone would be particularly
upset about one particular way in which libcurl might be used.

I also don't mind being wrong, of course. But I think it's better to
bet on making this like other things and then change strategy if that
doesn't work out, rather than starting out by making this different
from other things.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Jacob Champion <[email-masked]>
Date: Tue, 15 Apr 2025 12:44:48 -0700

> requests" something people would like to be able to control?

Well... I'd sure like to live in a world where users thought about the
implications and risks of what they're using and why, rather than
farming a decision out to a static analysis tool. ("And as long as I'm
dreaming, I'd like a pony.")

But end users already control the initiation of OAuth requests (it's
opt-in via the connection string), so that's not really the goal.

> shipping libpq.a, but if it breaks in any funny way, we might as well

Awesome. I think we have a consensus.

Thanks!
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 15 Apr 2025 12:45:28 -0700

> doesn't work out, rather than starting out by making this different

Works for me. (And it's less work, too!)

Thanks,
--Jacob

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Tue, 15 Apr 2025 23:38:35 +0200

> your concern is not with libcurl, but with the feature itself?

After reconsidering this, I now agree with Peter and Robert that
--with-libcurl is the flag that we should be relying on. Specifically
because of the situation you're describing above: Once you have
libcurl, why wouldn't you want all the features (e.g. in some other
thread there was a suggestion about fetching the PGSERVICEFILE from a
HTTP endpoint).

It's not like we add compile time flags for other user facing features
like --enable-index-scan. All the --enable-xyz options that we have
are for developer features (like debug and asserts). Starting to add
such a flag for this feature seems unnecessary.

Regarding discoverability, I think the error message that you have
already solves that:

> libpq_append_conn_error(conn, "no custom OAuth flows are available, and libpq was not built with libcurl support");

Side-note: I think it would be good to have a different error when
libpq was build with libcurl support, but the dlopen failed. Something
like:

libpq_append_conn_error(conn, "no custom OAuth flows are available,
and libpq-oauth could not be loaded library could not be loaded. Try
installing the libpq-oauth package from the same source that you
installed libpq from");

---

From: Jacob Champion <[email-masked]>
Date: Thu, 17 Apr 2025 17:47:42 -0700

> installing the libpq-oauth package from the same source that you

Thanks! I think that's a little too prescriptive for packagers,
personally, but I agree that the current message isn't correct
anymore. I've gone with "no custom OAuth flows are available, and the
builtin flow is not installed". (I suppose packagers could patch in a
platform-specific message if they really wanted?)

--

Other changes in v7:

- The option name remains --with-libcurl.
- Daniel and I have tweaked the documentation, and a draft commit message is up
- Removed the ENABLE_NLS-mismatch assertion in oauth-utils.c; we don't
need to care anymore
- Added an initialization mutex

I was feeling paranoid about injecting dependency pointers
concurrently to their use in another thread. They're _supposed_ to be
constant... but I have no doubt that someone somewhere knows of a
platform/compiler/linker combo where that blows up anyway.
Initialization is now run once, under pthread_mutex protection.

- Fixed module load on macOS

The green CI was masking a bug with its use of DYLD_LIBRARY_PATH: we
don't make use of RPATH on macOS, so after installing libpq, it lost
the ability to find libpq-oauth. (A stale installation due to SIP
weirdness was masking this on my local machine; sorry for not catching
it before.)

I have swapped to using an absolute path on Mac only, because unlike
LD_LIBRARY_PATH on *nix, DYLD_LIBRARY_PATH can still override absolute
paths in dlopen()! Whee. I could use a sanity check from a native Mac
developer, but I believe this mirrors the expected behavior for a
"typical" runtime dependency: libraries point directly to the things
they depend on.

With those, I have no more TODOs and I believe this is ready for a
final review round.

Thanks,
--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 18 Apr 2025 10:01:17 -0700

> With those, I have no more TODOs and I believe this is ready for a

Some ABI self-review. These references to conn->errorMessage also need
the indirection treatment, which I'm working on now:

>     if (actx->errctx)
>     {
>         appendPQExpBufferStr(&conn->errorMessage,
>                              libpq_gettext(actx->errctx));
>         appendPQExpBufferStr(&conn->errorMessage, ": ");
>     ...

I was searching backwards through history to confirm that we don't
rearrange struct pg_conn in back branches; turns out that was a false
assumption. See e8f60e6fe2:

    While at it, fix some places where parameter-related infrastructure
    was added with the aid of a dartboard, or perhaps with the aid of
    the anti-pattern "add new stuff at the end".  It should be safe
    to rearrange the contents of struct pg_conn even in released
    branches, since that's private to libpq (and we'd have to move
    some fields in some builds to fix this, anyway).

So that means, I think, the name needs to go back to -<major>-<minor>,
unless anyone can think of a clever way around it. (Injecting
conn->errorMessage to avoid the messiness around ENABLE_GSS et al is
still useful, but injecting every single offset doesn't seem
maintainable to me.) Sorry, Christoph; I know that's not what you were
hoping for.

--Jacob

---

From: Christoph Berg <[email-masked]>
Date: Sat, 19 Apr 2025 14:03:29 +0200

Re: Jacob Champion
> builtin flow is not installed".

This whole oauth business is highly confusing if you aren't a web
security expert. It's a pretty long way from "the builtin flow is not
installed" to "if you want this to work, you need to install an extra
library/package on the client", so I don't think this message is
helpful.

The originally suggested message was pretty good in that regard. The
distinction about custom flows could probably be dropped.

How about this:

  No libpq OAuth flows are available. (Try installing the libpq-oauth package.)

People who have custom flows will likely know that they have to do
anyway.

Devrim: Does that match the package name you'd use?

> (I suppose packagers could patch in a

We could, but I'd prefer if we didn't have to. :*)

Christoph

---

From: Jacob Champion <[email-masked]>
Date: Mon, 21 Apr 2025 09:57:33 -0700

On Sun, Apr 20, 2025 at 10:12 AM Ivan Kush <[email-masked]> wrote:
> I'm testing OAuth Device Flow implementation on Google. Met several
> problems.

Hi Ivan, thank you for testing and reporting! Unfortunately, yeah,
Google is a known problem [1]. They've taken several liberties with
the spec, as you point out.

We have some options for dealing with them, since their documentation
instructs clients to hardcode their API entry points instead of using
discovery. (That makes it easy for us to figure out when we're talking
to Google, and potentially switch to a quirks mode.)

But! Before we do that: How do you intend to authorize tokens issued
by Google? Last I checked, they still had no way to register an
application-specific scope, making it very dangerous IMO to use a
public flow [2]. Do you have an architecture where this usage is safe,
and/or have they added custom scopes? (I deprioritized handling the
nonstandard behavior when I couldn't prove to myself that it was
possible to use the Google version of Device Authorization safely, but
I'm happy to jump back into that if we have a good use case.)

> 1) In Device Authorization Request Google returns 428 code on pending
> https://developers.google.com/identity/protocols/oauth2/limited-input-device#authorization-pending

Right. I believe there were other nonstandard errors in other corner
cases, too. :(

> I suggest to add a GUC in postgresql.conf that contains additional
> non-standard error codes for a specific service.
> oauth_add_error_codes = [
>    {
>           issuer: google
>          add_err_codes: [428],
>      },
>      {
>          issuer: someservice
>          add_err_code: [403],
>      }
> ]
> So Google can contain 400,401,428

The server config doesn't help us much, since this is a client-side
feature. Any "global" configuration is probably going to be done
through environment variables or a service file [3].

> Additionally write parsing of such json-like config-values. Will be cool
> to create serializer, that matches struct to such json-like GUC.

I'm not too excited about a separate configuration DSL. I'm guessing
most end users, if they really want Google as their Device
Authorization provider, would rather have us switch over to "Google
mode" once we notice the magic Google endpoint is in use.

> 2) Google requires client_secret only in the Device Access Token Request
> (Section 3.3 RFC-8628).
> ...
> But Postgres sends client_secret in both request, also in Device
> Authorization Request.

Yes. See 3.1 (Device Authorization Request):

   The client authentication requirements of Section 3.2.1 of [RFC6749]
   apply to requests on this endpoint, which means that confidential
   clients (those that have established client credentials) authenticate
   in the same manner as when making requests to the token endpoint, and
   public clients provide the "client_id" parameter to identify
   themselves.

> I suggest to remove send secret on Device Authorization Request.

This breaks Okta, at minimum. We can't do it across the board. (As for
Azure, I haven't figured out how to configure it to *require* a
confidential client secret for the device flow -- which makes a
certain amount of sense since the flow is public -- but its v2
endpoint doesn't mind being *sent* a secret.)

> 3) Additionally if secret exists PG sends it only using Basic Auth. But
> RFC contain only MAY word about Basic Auth. Section 2.3.1 RFC 6749,

---

From: Jacob Champion <[email-masked]>
Date: Mon, 21 Apr 2025 16:19:06 -0700

>   No libpq OAuth flows are available. (Try installing the libpq-oauth package.)

Tweaked for capitalization/punctuation rules, and removing the first
"libpq" mention (which I don't think helps a user of, say, psql):

    no OAuth flows are available (try installing the libpq-oauth package)

v8 also makes the following changes:

- Per ABI comment upthread, we are back to major-minor versioning for
the shared library (e.g. libpq-oauth-18-0.so). 0001 adds the macros
and makefile variables to make this easy, and 0002 is the bulk of the
change now.
- Since libpq-oauth.a is going to be discovered at compile time, not
runtime, I've removed the versioning from that filename. Static
clients need to match them anyway, so we don't need that additional
packaging headache.
- conn->errorMessage is now decoupled from oauth-curl.c. Separate
object file builds are made using the same technique as libpq.

Thanks,
--Jacob
-:  ----------- > 1:  5f87f11b18e Add minor-version counterpart to (PG_)MAJORVERSION
1:  942ad5391e2 ! 2:  4c9cc7f69af oauth: Move the builtin flow into a separate module
    @@ Commit message
         the search path came from a different build of Postgres.
     
         This ABI is considered "private". The module has no SONAME or version
    -    symlinks, and it's named libpq-oauth-<major>.so to avoid mixing and
    -    matching across major Postgres versions. (Future improvements may
    -    promote this "OAuth flow plugin" to a first-class concept, at which
    -    point we would need a public API to replace this anyway.)
    +    symlinks, and it's named libpq-oauth-<major>-<minor>.so to avoid mixing
    +    and matching across Postgres versions, in case internal struct order
    +    needs to change. (Future improvements may promote this "OAuth flow
    +    plugin" to a first-class concept, at which point we would need a public
    +    API to replace this anyway.)
     
         Additionally, NLS support for error messages in b3f0be788a was
         incomplete, because the new error macros weren't being scanned by
    @@ src/interfaces/libpq-oauth/Makefile (new)
     +
     +# This is an internal module; we don't want an SONAME and therefore do not set
     +# SO_MAJOR_VERSION.
    -+NAME = pq-oauth-$(MAJORVERSION)
    ++NAME = pq-oauth-$(MAJORVERSION)-$(MINORVERSION)
     +
    -+# Force the name "libpq-oauth" for both the static and shared libraries.
    ++# Force the name "libpq-oauth" for both the static and shared libraries. The
    ++# staticlib doesn't need version information in its name.
     +override shlib := lib$(NAME)$(DLSUFFIX)
    -+override stlib := lib$(NAME).a
    ++override stlib := libpq-oauth.a
     +
     +override CPPFLAGS := -I$(libpq_srcdir) -I$(top_builddir)/src/port $(LIBCURL_CPPFLAGS) $(CPPFLAGS)
     +
     +OBJS = \
    -+	$(WIN32RES) \
    -+	oauth-curl.o
    ++	$(WIN32RES)
    ++
    ++OBJS_STATIC = oauth-curl.o
     +
     +# The shared library needs additional glue symbols.
    -+$(shlib): OBJS += oauth-utils.o
    -+$(shlib): oauth-utils.o
    ++OBJS_SHLIB = \
    ++	oauth-curl_shlib.o \
    ++	oauth-utils.o \
    ++
    ++oauth-utils.o: override CPPFLAGS += -DUSE_DYNAMIC_OAUTH
    ++oauth-curl_shlib.o: override CPPFLAGS_SHLIB += -DUSE_DYNAMIC_OAUTH
    ++
    ++# Add shlib-/stlib-specific objects.
    ++$(shlib): override OBJS += $(OBJS_SHLIB)
    ++$(shlib): $(OBJS_SHLIB)
    ++
    ++$(stlib): override OBJS += $(OBJS_STATIC)
    ++$(stlib): $(OBJS_STATIC)
     +
     +SHLIB_LINK_INTERNAL = $(libpq_pgport_shlib)
     +SHLIB_LINK = $(LIBCURL_LDFLAGS) $(LIBCURL_LDLIBS)
    @@ src/interfaces/libpq-oauth/Makefile (new)
     +# Shared library stuff
     +include $(top_srcdir)/src/Makefile.shlib
     +
    ++# Use src/common/Makefile's trick for tracking dependencies of shlib-specific
    ++# objects.
    ++%_shlib.o: %.c %.o
    ++	$(CC) $(CFLAGS) $(CFLAGS_SL) $(CPPFLAGS) $(CPPFLAGS_SHLIB) -c $< -o $@
    ++
     +# Ignore the standard rules for SONAME-less installation; we want both the
     +# static and shared libraries to go into libdir.
     +install: all installdirs $(stlib) $(shlib)
    @@ src/interfaces/libpq-oauth/Makefile (new)
     +	rm -f '$(DESTDIR)$(libdir)/$(shlib)'
     +
     +clean distclean: clean-lib
    -+	rm -f $(OBJS) oauth-utils.o
    ++	rm -f $(OBJS) $(OBJS_STATIC) $(OBJS_SHLIB)
     
      ## src/interfaces/libpq-oauth/README (new) ##
     @@
    @@ src/interfaces/libpq-oauth/README (new)
     += Load-Time ABI =
     +
     +This module ABI is an internal implementation detail, so it's subject to change
    -+across major releases; the name of the module (libpq-oauth-MAJOR) reflects this.
    ++across releases; the name of the module (libpq-oauth-MAJOR-MINOR) reflects this.
     +The module exports the following symbols:
     +
     +- PostgresPollingStatusType pg_fe_run_oauth_flow(PGconn *conn);
    @@ src/interfaces/libpq-oauth/README (new)
     +
     +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 libpq to expose
    -+conn->errorMessage, via the errmsg_impl.
    ++function before the flow is run.
     +
    -+This dependency injection is done to ensure that the module ABI is decoupled
    -+from the internals of `struct pg_conn`. This way we can safely search the
    -+standard dlopen() paths (e.g. RPATH, LD_LIBRARY_PATH, the SO cache) for an
    -+implementation module to use, even if that module wasn't compiled at the same
    -+time as libpq.
    ++It also relies on libpq to expose conn->errorMessage, via the errmsg_impl. This
    ++is done to decouple the module ABI from the offset of errorMessage, which can
    ++change positions depending on configure-time options. This way we can safely
    ++search the standard dlopen() paths (e.g. RPATH, LD_LIBRARY_PATH, the SO cache)
    ++for an implementation module to use, even if that module wasn't compiled at the
    ++same time as libpq.
     +
     += Static Build =
     +
     +The static library libpq.a does not perform any dynamic loading. If the builtin
    -+flow is enabled, the application is expected to link against libpq-oauth-*.a
    ++flow is enabled, the application is expected to link against libpq-oauth.a
     +directly to provide the necessary symbols.
     
      ## src/interfaces/libpq-oauth/exports.txt (new) ##
    @@ src/interfaces/libpq-oauth/meson.build (new)
     +libpq_oauth_so_sources = files(
     +  'oauth-utils.c',
     +)
    ++libpq_oauth_so_c_args = ['-DUSE_DYNAMIC_OAUTH']
     +
     +export_file = custom_target('libpq-oauth.exports',
     +  kwargs: gen_export_kwargs,
    @@ src/interfaces/libpq-oauth/meson.build (new)
     +# port needs to be in include path due to pthread-win32.h
     +libpq_oauth_inc = include_directories('.', '../libpq', '../../port')
     +
    -+# This is an internal module; we don't want an SONAME and therefore do not set
    -+# SO_MAJOR_VERSION.
    -+libpq_oauth_name = 'libpq-oauth-@0@'.format(pg_version_major)
    -+
    -+libpq_oauth_st = static_library(libpq_oauth_name,
    ++libpq_oauth_st = static_library('libpq-oauth',
     +  libpq_oauth_sources,
     +  include_directories: [libpq_oauth_inc, postgres_inc],
     +  c_pch: pch_postgres_fe_h,
    @@ src/interfaces/libpq-oauth/meson.build (new)
     +  kwargs: default_lib_args,
     +)
     +
    ++# This is an internal module; we don't want an SONAME and therefore do not set
    ++# SO_MAJOR_VERSION.
    ++libpq_oauth_name = 'libpq-oauth-@0@-@1@'.format(pg_version_major, pg_version_minor)
    ++
     +libpq_oauth_so = shared_module(libpq_oauth_name,
     +  libpq_oauth_sources + libpq_oauth_so_sources,
     +  include_directories: [libpq_oauth_inc, postgres_inc],
    ++  c_args: libpq_so_c_args,
     +  c_pch: pch_postgres_fe_h,
     +  dependencies: [frontend_shlib_code, libpq, libpq_oauth_deps],
     +  link_depends: export_file,
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c => src/interfaces/libpq-oauth/oauth-cu
     -#include "libpq-int.h"
      #include "mb/pg_wchar.h"
     +#include "oauth-curl.h"
    ++#ifdef USE_DYNAMIC_OAUTH
     +#include "oauth-utils.h"
    ++#endif
      
      /*
       * It's generally prudent to set a maximum response size to buffer in memory,
    @@ src/interfaces/libpq-oauth/oauth-curl.c: prompt_user(struct async_ctx *actx, PGc
      
      	if (!res)
      	{
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
    + {
    + 	fe_oauth_state *state = conn->sasl_state;
    + 	struct async_ctx *actx;
    ++	PQExpBuffer errbuf;
    + 
    + 	if (!initialize_curl(conn))
    + 		return PGRES_POLLING_FAILED;
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
    + 
    + error_return:
    + 
    ++	/*
    ++	 * For the dynamic module build, we can't safely rely on the offset of
    ++	 * conn->errorMessage, since it depends on build options like USE_SSL et
    ++	 * al. libpq gives us a translator function instead.
    ++	 */
    ++#ifdef USE_DYNAMIC_OAUTH
    ++	errbuf = conn_errorMessage(conn);
    ++#else
    ++	errbuf = &conn->errorMessage;
    ++#endif
    ++
    + 	/*
    + 	 * Assemble the three parts of our error: context, body, and detail. See
    + 	 * also the documentation for struct async_ctx.
    + 	 */
    + 	if (actx->errctx)
    + 	{
    +-		appendPQExpBufferStr(&conn->errorMessage,
    +-							 libpq_gettext(actx->errctx));
    +-		appendPQExpBufferStr(&conn->errorMessage, ": ");
    ++		appendPQExpBufferStr(errbuf, libpq_gettext(actx->errctx));
    ++		appendPQExpBufferStr(errbuf, ": ");
    + 	}
    + 
    + 	if (PQExpBufferDataBroken(actx->errbuf))
    +-		appendPQExpBufferStr(&conn->errorMessage,
    +-							 libpq_gettext("out of memory"));
    ++		appendPQExpBufferStr(errbuf, libpq_gettext("out of memory"));
    + 	else
    +-		appendPQExpBufferStr(&conn->errorMessage, actx->errbuf.data);
    ++		appendPQExpBufferStr(errbuf, actx->errbuf.data);
    + 
    + 	if (actx->curl_err[0])
    + 	{
    + 		size_t		len;
    + 
    +-		appendPQExpBuffer(&conn->errorMessage,
    +-						  " (libcurl: %s)", actx->curl_err);
    ++		appendPQExpBuffer(errbuf, " (libcurl: %s)", actx->curl_err);
    + 
    + 		/* Sometimes libcurl adds a newline to the error buffer. :( */
    +-		len = conn->errorMessage.len;
    +-		if (len >= 2 && conn->errorMessage.data[len - 2] == '\n')
    ++		len = errbuf->len;
    ++		if (len >= 2 && errbuf->data[len - 2] == '\n')
    + 		{
    +-			conn->errorMessage.data[len - 2] = ')';
    +-			conn->errorMessage.data[len - 1] = '\0';
    +-			conn->errorMessage.len--;
    ++			errbuf->data[len - 2] = ')';
    ++			errbuf->data[len - 1] = '\0';
    ++			errbuf->len--;
    + 		}
    + 	}
    + 
    +-	appendPQExpBufferChar(&conn->errorMessage, '\n');
    ++	appendPQExpBufferChar(errbuf, '\n');
    + 
    + 	return PGRES_POLLING_FAILED;
    + }
     
      ## src/interfaces/libpq-oauth/oauth-curl.h (new) ##
     @@
    @@ src/interfaces/libpq-oauth/oauth-utils.c (new)
     +#include "libpq-int.h"
     +#include "oauth-utils.h"
     +
    ++#ifndef USE_DYNAMIC_OAUTH
    ++#error oauth-utils.c is not supported in static builds
    ++#endif
    ++
     +static libpq_gettext_func libpq_gettext_impl;
    -+static conn_errorMessage_func conn_errorMessage;
     +
     +pgthreadlock_t pg_g_threadlock;
    ++conn_errorMessage_func conn_errorMessage;
     +
     +/*-
     + * Initializes libpq-oauth by setting necessary callbacks.
    @@ src/interfaces/libpq-oauth/oauth-utils.h (new)
     +										 libpq_gettext_func gettext_impl,
     +										 conn_errorMessage_func errmsg_impl);
     +
    ++/* Callback to safely obtain conn->errorMessage from a PGconn. */
    ++extern conn_errorMessage_func conn_errorMessage;
    ++
     +/* Duplicated APIs, copied from libpq. */
     +extern void libpq_append_conn_error(PGconn *conn, const char *fmt,...) pg_attribute_printf(2, 3);
     +extern bool oauth_unsafe_debugging_enabled(void);
    @@ src/interfaces/libpq/Makefile: ifeq ($(with_ssl),openssl)
     +# libpq.so doesn't link against libcurl, but libpq.a needs libpq-oauth, and
     +# libpq-oauth needs libcurl. Put both into *.private.
     +PKG_CONFIG_REQUIRES_PRIVATE += libcurl
    -+%.pc: override SHLIB_LINK_INTERNAL += -lpq-oauth-$(MAJORVERSION)
    ++%.pc: override SHLIB_LINK_INTERNAL += -lpq-oauth
     +endif
     +
      all: all-lib libpq-refs-stamp
    @@ src/interfaces/libpq/fe-auth-oauth.c: cleanup_user_oauth_flow(PGconn *conn)
     +	 */
     +	const char *const module_name =
     +#if defined(__darwin__)
    -+		LIBDIR "/libpq-oauth-" PG_MAJORVERSION DLSUFFIX;
    ++		LIBDIR "/libpq-oauth-" PG_MAJORVERSION "-" PG_MINORVERSION DLSUFFIX;
     +#else
    -+		"libpq-oauth-" PG_MAJORVERSION DLSUFFIX;
    ++		"libpq-oauth-" PG_MAJORVERSION "-" PG_MINORVERSION DLSUFFIX;
     +#endif
     +
     +	state->builtin_flow = dlopen(module_name, RTLD_NOW | RTLD_LOCAL);
    @@ src/interfaces/libpq/fe-auth-oauth.c: setup_token_request(PGconn *conn, fe_oauth
     -
     -#else
     -		libpq_append_conn_error(conn, "no custom OAuth flows are available, and libpq was not built with libcurl support");
    -+		libpq_append_conn_error(conn, "no custom OAuth flows are available, and the builtin flow is not installed");
    ++		libpq_append_conn_error(conn, "no OAuth flows are available (try installing the libpq-oauth package)");
      		goto fail;
     -
     -#endif
    @@ src/interfaces/libpq/meson.build: libpq = declare_dependency(
     +  # libpq-oauth needs libcurl. Put both into *.private.
     +  private_deps += [
     +    libpq_oauth_deps,
    -+    '-lpq-oauth-@0@'.format(pg_version_major),
    ++    '-lpq-oauth',
     +  ]
     +endif
     +
    @@ src/test/modules/oauth_validator/t/002_client.pl: if ($ENV{with_libcurl} ne 'yes
      		flags => ["--no-hook"],
      		expected_stderr =>
     -		  qr/no custom OAuth flows are available, and libpq was not built with libcurl support/
    -+		  qr/no custom OAuth flows are available, and the builtin flow is not installed/
    ++		  qr/no OAuth flows are available \(try installing the libpq-oauth package\)/
      	);
      }

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 22 Apr 2025 12:02:42 +0200

> On 22 Apr 2025, at 01:19, Jacob Champion <[email-masked]> wrote:

> v8 also makes the following changes:

Thanks for this version, a few small comments:

+  if oauth_flow_supported
+    cdata.set('USE_LIBCURL', 1)
+  elif libcurlopt.enabled()
+    error('client OAuth is not supported on this platform')
+  endif
We already know that libcurlopt.enabled() is true here so maybe just doing
if-else-endif would make it more readable and save readers thinking it might
have changed?  Also, "client OAuth" reads a bit strange, how about "client-side
OAuth" or "OAuth flow module"?

-       appendPQExpBufferStr(&conn->errorMessage,
-                            libpq_gettext(actx->errctx));
-       appendPQExpBufferStr(&conn->errorMessage, ": ");
+       appendPQExpBufferStr(errbuf, libpq_gettext(actx->errctx));
+       appendPQExpBufferStr(errbuf, ": ");
I think we should take this opportunity to turn this into a appendPQExpBuffer()
with a format string instead of two calls.

+       len = errbuf->len;
+       if (len >= 2 && errbuf->data[len - 2] == '\n')
Now that the actual variable, errbuf->len, is short and very descriptive I
wonder if we shouldn't just use this as it makes the code even clearer IMO.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Tue, 22 Apr 2025 16:41:50 -0700

On Tue, Apr 22, 2025 at 3:02 AM Daniel Gustafsson <[email-masked]> wrote:
> have changed?

Features are tri-state, so libcurlopt.disabled() and
libcurlopt.enabled() can both be false. :( My intent is to fall
through nicely in the case where -Dlibcurl=auto.

(Our minimum version of Meson is too old to switch to syntax that
makes this more readable, like .allowed(), .require(), .disable_if(),
etc...)

> Also, "client OAuth" reads a bit strange, how about "client-side

All three done in v9, attached.

Thanks!
--Jacob
1:  5f87f11b18e = 1:  5f87f11b18e Add minor-version counterpart to (PG_)MAJORVERSION
2:  4c9cc7f69af ! 2:  9e37fd7c217 oauth: Move the builtin flow into a separate module
    @@ configure.ac: if test "$PORTNAME" = "win32" ; then
     +if test "$with_libcurl" = yes ; then
     +  # Error out early if this platform can't support libpq-oauth.
     +  if test "$ac_cv_header_sys_event_h" != yes -a "$ac_cv_header_sys_epoll_h" != yes; then
    -+    AC_MSG_ERROR([client OAuth is not supported on this platform])
    ++    AC_MSG_ERROR([client-side OAuth is not supported on this platform])
     +  fi
     +fi
     +
    @@ meson.build: if not libcurlopt.disabled()
     +  if oauth_flow_supported
     +    cdata.set('USE_LIBCURL', 1)
     +  elif libcurlopt.enabled()
    -+    error('client OAuth is not supported on this platform')
    ++    error('client-side OAuth is not supported on this platform')
     +  endif
     +
      else
    @@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
      	 * also the documentation for struct async_ctx.
      	 */
      	if (actx->errctx)
    - 	{
    +-	{
     -		appendPQExpBufferStr(&conn->errorMessage,
     -							 libpq_gettext(actx->errctx));
     -		appendPQExpBufferStr(&conn->errorMessage, ": ");
    -+		appendPQExpBufferStr(errbuf, libpq_gettext(actx->errctx));
    -+		appendPQExpBufferStr(errbuf, ": ");
    - 	}
    +-	}
    ++		appendPQExpBuffer(errbuf, "%s: ", libpq_gettext(actx->errctx));
      
      	if (PQExpBufferDataBroken(actx->errbuf))
     -		appendPQExpBufferStr(&conn->errorMessage,
    @@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
      
      	if (actx->curl_err[0])
      	{
    - 		size_t		len;
    - 
    +-		size_t		len;
    +-
     -		appendPQExpBuffer(&conn->errorMessage,
     -						  " (libcurl: %s)", actx->curl_err);
     +		appendPQExpBuffer(errbuf, " (libcurl: %s)", actx->curl_err);
    @@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
      		/* Sometimes libcurl adds a newline to the error buffer. :( */
     -		len = conn->errorMessage.len;
     -		if (len >= 2 && conn->errorMessage.data[len - 2] == '\n')
    -+		len = errbuf->len;
    -+		if (len >= 2 && errbuf->data[len - 2] == '\n')
    ++		if (errbuf->len >= 2 && errbuf->data[errbuf->len - 2] == '\n')
      		{
     -			conn->errorMessage.data[len - 2] = ')';
     -			conn->errorMessage.data[len - 1] = '\0';
     -			conn->errorMessage.len--;
    -+			errbuf->data[len - 2] = ')';
    -+			errbuf->data[len - 1] = '\0';
    ++			errbuf->data[errbuf->len - 2] = ')';
    ++			errbuf->data[errbuf->len - 1] = '\0';
     +			errbuf->len--;
      		}
      	}

---

From: Jacob Champion <[email-masked]>
Date: Wed, 23 Apr 2025 09:07:52 -0700

On Wed, Apr 23, 2025 at 8:39 AM Christoph Berg <[email-masked]> wrote:
> This will cause problems when programs are running while packages are
> updated on disk. That program then tries to dlopen 18-0.so when there
> is already 18-1.so installed. Relevant when the first oauth connection
> is made way after startup.

Ugh, good point. This hazard applies to the previous suggestion of
pkglibdir, too, but in that case it would have been silent...

> This is trading one problem for another, but within-a-major ABI
> changes should be much rarer than normal minor updates with
> applications restarting only later.

But the consequences are much worse for a silent ABI mismatch. Imagine
if libpq-oauth examines the wrong pointer inside PGconn for a
security-critical check.

> Alternatively, there could be a dedicated SONAME for the plugin that
> only changes when necessary, but perhaps the simple "18.so" solution
> is good enough.

I don't think SONAME helps us, does it? We're not using it in dlopen().

We could all agree to bump the second number in the filename whenever
there's an internal ABI change. That works from a technical
perspective, but it's hard to test and enforce and... just not forget.
Or, I may still be able to thread the needle with a fuller lookup
table, and remove the dependency on libpq-int.h; it's just not going
to be incredibly pretty. Thinking...

Thanks so much for your continued review!

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Wed, 23 Apr 2025 10:46:14 -0700

On Wed, Apr 23, 2025 at 9:38 AM Christoph Berg <[email-masked]> wrote:
> It's hopefully not harder than checking ABI compatibility of any other
> libpq change, just a different number. If that number is in the
> meson.build in the same directory, people should be able to connect
> the dots.

I think it is harder, simply because no one has to do it today, and
that change would sign them up to do it, forever, adding to the
backport checklist. It's one thing if there's a bunch of committers
who pile into the thread right now saying "yes, that's okay", but I
don't really feel comfortable making that decision for them right this
instant.

If we had robust ABI compatibility checks as part of the farm [1], I
think we could do that. Doesn't feel like an 18 thing, though.

> Btw, if we have that number, we might as well drop the MAJOR part as
> well... apt.pg.o is always shipping the latest libpq5, so major libpq
> upgrades while apps are running are going to happen. (But this is just
> once a year and much less problematic than minor upgrades and I'm not
> going to complain if MAJOR is kept.)

I don't want to introduce another testing matrix dimension if I can
avoid it. ("I have this bug where libpq.so.5.18 is using
libpq-oauth.so from PG20 and I had no idea it was doing that and the
problem went away when I restarted and...")

And the intent is for this to be temporary until we have a user-facing
API. If this is the solution we go with, I think it'd wise to prepare
for a -19 version of libpq-oauth, but I'm going to try my best to get
custom modules in ASAP. People are going to be annoyed that v1 of the
feature doesn't let them swap the flow for our utilities. Ideally they
only have to deal with that for a single major release.

Also: since the libpq-oauth-18 and libpq-oauth-19 packages can be
installed side-by-side safely, isn't the upgrade hazard significantly
diminished? (If a user uninstalls the previous libpq-oauth version
while they're still running that version of libpq in memory, _and_
they've somehow never used OAuth until right that instant... it's easy
enough for them to undo their mistake while the application is still
running.)

> Don't overdesign it...

Oh, I agree... but my "minimal" ABI designs have all had corner cases
so far. I may need to just bite the bullet.

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?) You're still signing up for delayed errors in the long-lived
client case, so it's not a magic bullet, but the breakage is easy to
see and it's not a crash. The client application "just" has to restart
after a libpq upgrade.

Thanks,
--Jacob

[1] https://www.postgresql.org/message-id/B142EE8A-5D38-48B9-A4BB-82D69A854B55%40justatheory.com

---

From: Jacob Champion <[email-masked]>
Date: Thu, 24 Apr 2025 10:02:25 -0700

On Wed, Apr 23, 2025 at 1:13 PM Christoph Berg <[email-masked]> wrote:
> Uhm, so far the plan was to have one "libpq-oauth" package, not several.

I think the system is overconstrained at that point. If you want to
support clients that delay-load the ABI they're compiled against,
_and_ have them continue to work seamlessly after the system has
upgraded the ABI underneath them, without restarting the client... is
there any option other than side-by-side installation?

> Since shipping a single libpq5.deb package for all PG majors has worked well
> for the past decades, I wouldn't want to complicate that now.

I'm not sure if it's possible to ship a client-side module system
without something getting more complicated, though... I'm trying hard
not to overcomplicate it for you, but I also don't think the
complexity is going to remain the same.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 25 Apr 2025 13:31:16 -0700

On Fri, Apr 25, 2025 at 2:03 AM Christoph Berg <[email-masked]> wrote:
> My point is that we should be trying to change the ABI-as-coded-in-the-
> filename as rarely as possible.

I agree, but I'm also trying to say I can't unilaterally declare
pieces of our internal structs to be covered by an ABI guarantee.
Maybe the rest of the ABI will never change because it'll be perfect,
but I point to the immediately preceding thread as evidence against
the likelihood of perfection on the first try. I'm trying to build in
air bags so we don't have to regret a mistake.

> Then side-by-side should not be required.

It's still required _during_ an ABI bump, though, if you don't want
things to break. Right?

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Mon, 28 Apr 2025 17:10:06 -0700

On Wed, Apr 23, 2025 at 10:46 AM Jacob Champion
<[email-masked]> wrote:
> that?)

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.

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.)

Thanks,
--Jacob
1:  5f87f11b18e < -:  ----------- Add minor-version counterpart to (PG_)MAJORVERSION
2:  9e37fd7c217 ! 1:  e86e93f7ac8 oauth: Move the builtin flow into a separate module
    @@ Commit message
     
         The default flow relies on some libpq internals. Some of these can be
         safely duplicated (such as the SIGPIPE handlers), but others need to be
    -    shared between libpq and libpq-oauth for thread-safety. To avoid exporting
    -    these internals to all libpq clients forever, these dependencies are
    -    instead injected from the libpq side via an initialization function.
    -    This also lets libpq communicate the offset of conn->errorMessage to
    -    libpq-oauth, so that we can function without crashing if the module on
    -    the search path came from a different build of Postgres.
    +    shared between libpq and libpq-oauth for thread-safety. To avoid
    +    exporting these internals to all libpq clients forever, these
    +    dependencies are instead injected from the libpq side via an
    +    initialization function. This also lets libpq communicate the offsets of
    +    PGconn struct members to libpq-oauth, so that we can function without
    +    crashing if the module on the search path came from a different build of
    +    Postgres. (A minor-version upgrade could swap the libpq-oauth module out
    +    from under a long-running libpq client before it does its first load of
    +    the OAuth flow.)
     
         This ABI is considered "private". The module has no SONAME or version
    -    symlinks, and it's named libpq-oauth-<major>-<minor>.so to avoid mixing
    -    and matching across Postgres versions, in case internal struct order
    -    needs to change. (Future improvements may promote this "OAuth flow
    -    plugin" to a first-class concept, at which point we would need a public
    -    API to replace this anyway.)
    +    symlinks, and it's named libpq-oauth-<major>.so to avoid mixing and
    +    matching across Postgres versions. (Future improvements may promote this
    +    "OAuth flow plugin" to a first-class concept, at which point we would
    +    need a public API to replace this anyway.)
     
         Additionally, NLS support for error messages in b3f0be788a was
         incomplete, because the new error macros weren't being scanned by
    @@ src/interfaces/libpq-oauth/Makefile (new)
     +
     +# This is an internal module; we don't want an SONAME and therefore do not set
     +# SO_MAJOR_VERSION.
    -+NAME = pq-oauth-$(MAJORVERSION)-$(MINORVERSION)
    ++NAME = pq-oauth-$(MAJORVERSION)
     +
     +# Force the name "libpq-oauth" for both the static and shared libraries. The
     +# staticlib doesn't need version information in its name.
    @@ src/interfaces/libpq-oauth/README (new)
     += Load-Time ABI =
     +
     +This module ABI is an internal implementation detail, so it's subject to change
    -+across releases; the name of the module (libpq-oauth-MAJOR-MINOR) reflects this.
    ++across major releases; the name of the module (libpq-oauth-MAJOR) reflects this.
     +The module exports the following symbols:
     +
     +- PostgresPollingStatusType pg_fe_run_oauth_flow(PGconn *conn);
    @@ src/interfaces/libpq-oauth/README (new)
     +
     +- void libpq_oauth_init(pgthreadlock_t threadlock,
     +						libpq_gettext_func gettext_impl,
    -+						conn_errorMessage_func errmsg_impl);
    ++						conn_errorMessage_func errmsg_impl,
    ++						conn_oauth_client_id_func clientid_impl,
    ++						conn_oauth_client_secret_func clientsecret_impl,
    ++						conn_oauth_discovery_uri_func discoveryuri_impl,
    ++						conn_oauth_issuer_id_func issuerid_impl,
    ++						conn_oauth_scope_func scope_impl,
    ++						conn_sasl_state_func saslstate_impl,
    ++						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 libpq to expose conn->errorMessage, via the errmsg_impl. This
    -+is done to decouple the module ABI from the offset of errorMessage, which can
    -+change positions depending on configure-time options. This way we can safely
    -+search the standard dlopen() paths (e.g. RPATH, LD_LIBRARY_PATH, the SO cache)
    -+for an implementation module to use, even if that module wasn't compiled at the
    -+same time as libpq.
    ++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
    ++change offsets depending on configure-time options). Therefore the necessary
    ++accessors (named conn_*) and mutators (set_conn_*) are injected here. With this
    ++approach, we can safely search the standard dlopen() paths (e.g. RPATH,
    ++LD_LIBRARY_PATH, the SO cache) for an implementation module to use, even if that
    ++module wasn't compiled at the same time as libpq -- which becomes especially
    ++important during "live upgrade" situations where a running libpq application has
    ++the libpq-oauth module updated out from under it before it's first loaded from
    ++disk.
     +
     += Static Build =
     +
     +The static library libpq.a does not perform any dynamic loading. If the builtin
     +flow is enabled, the application is expected to link against libpq-oauth.a
    -+directly to provide the necessary symbols.
    ++directly to provide the necessary symbols. (libpq.a and libpq-oauth.a must be
    ++part of the same build. Unlike the dynamic module, there are no translation
    ++shims provided.)
     
      ## src/interfaces/libpq-oauth/exports.txt (new) ##
     @@
    @@ src/interfaces/libpq-oauth/meson.build (new)
     +
     +# This is an internal module; we don't want an SONAME and therefore do not set
     +# SO_MAJOR_VERSION.
    -+libpq_oauth_name = 'libpq-oauth-@0@-@1@'.format(pg_version_major, pg_version_minor)
    ++libpq_oauth_name = 'libpq-oauth-@0@'.format(pg_version_major)
     +
     +libpq_oauth_so = shared_module(libpq_oauth_name,
     +  libpq_oauth_sources + libpq_oauth_so_sources,
    @@ src/interfaces/libpq/fe-auth-oauth-curl.c => src/interfaces/libpq-oauth/oauth-cu
     -#include <unistd.h>
      
      #include "common/jsonapi.h"
    - #include "fe-auth.h"
    +-#include "fe-auth.h"
      #include "fe-auth-oauth.h"
     -#include "libpq-int.h"
      #include "mb/pg_wchar.h"
     +#include "oauth-curl.h"
    ++
     +#ifdef USE_DYNAMIC_OAUTH
    ++
    ++/*
    ++ * The module build is decoupled from libpq-int.h, to try to avoid inadvertent
    ++ * ABI breaks during minor version bumps. Replacements for the missing internals
    ++ * are provided by oauth-utils.
    ++ */
     +#include "oauth-utils.h"
    ++
    ++#else							/* !USE_DYNAMIC_OAUTH */
    ++
    ++/*
    ++ * Static builds may rely on PGconn offsets directly. Keep these aligned with
    ++ * the bank of callbacks in oauth-utils.h.
    ++ */
    ++#include "libpq-int.h"
    ++
    ++#define conn_errorMessage(CONN) (&CONN->errorMessage)
    ++#define conn_oauth_client_id(CONN) (CONN->oauth_client_id)
    ++#define conn_oauth_client_secret(CONN) (CONN->oauth_client_secret)
    ++#define conn_oauth_discovery_uri(CONN) (CONN->oauth_discovery_uri)
    ++#define conn_oauth_issuer_id(CONN) (CONN->oauth_issuer_id)
    ++#define conn_oauth_scope(CONN) (CONN->oauth_scope)
    ++#define conn_sasl_state(CONN) (CONN->sasl_state)
    ++
    ++#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 */
    ++
    ++/* 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
     +#endif
      
      /*
       * It's generally prudent to set a maximum response size to buffer in memory,
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: free_async_ctx(PGconn *conn, struct async_ctx *actx)
    + void
    + pg_fe_cleanup_oauth_flow(PGconn *conn)
    + {
    +-	fe_oauth_state *state = conn->sasl_state;
    ++	fe_oauth_state *state = conn_sasl_state(conn);
    + 
    + 	if (state->async_ctx)
    + 	{
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_cleanup_oauth_flow(PGconn *conn)
    + 		state->async_ctx = NULL;
    + 	}
    + 
    +-	conn->altsock = PGINVALID_SOCKET;
    ++	set_conn_altsock(conn, PGINVALID_SOCKET);
    + }
    + 
    + /*
     @@ src/interfaces/libpq-oauth/oauth-curl.c: parse_access_token(struct async_ctx *actx, struct token *tok)
      static bool
      setup_multiplexer(struct async_ctx *actx)
    @@ src/interfaces/libpq-oauth/oauth-curl.c: timer_expired(struct async_ctx *actx)
      }
      
      /*
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: static bool
    + check_issuer(struct async_ctx *actx, PGconn *conn)
    + {
    + 	const struct provider *provider = &actx->provider;
    ++	const char *oauth_issuer_id = conn_oauth_issuer_id(conn);
    + 
    +-	Assert(conn->oauth_issuer_id);	/* ensured by setup_oauth_parameters() */
    ++	Assert(oauth_issuer_id);	/* ensured by setup_oauth_parameters() */
    + 	Assert(provider->issuer);	/* ensured by parse_provider() */
    + 
    + 	/*---
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: check_issuer(struct async_ctx *actx, PGconn *conn)
    + 	 *    sent to. This comparison MUST use simple string comparison as defined
    + 	 *    in Section 6.2.1 of [RFC3986].
    + 	 */
    +-	if (strcmp(conn->oauth_issuer_id, provider->issuer) != 0)
    ++	if (strcmp(oauth_issuer_id, provider->issuer) != 0)
    + 	{
    + 		actx_error(actx,
    + 				   "the issuer identifier (%s) does not match oauth_issuer (%s)",
    +-				   provider->issuer, conn->oauth_issuer_id);
    ++				   provider->issuer, oauth_issuer_id);
    + 		return false;
    + 	}
    + 
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: check_for_device_flow(struct async_ctx *actx)
    + static bool
    + add_client_identification(struct async_ctx *actx, PQExpBuffer reqbody, PGconn *conn)
    + {
    ++	const char *oauth_client_id = conn_oauth_client_id(conn);
    ++	const char *oauth_client_secret = conn_oauth_client_secret(conn);
    ++
    + 	bool		success = false;
    + 	char	   *username = NULL;
    + 	char	   *password = NULL;
    + 
    +-	if (conn->oauth_client_secret)	/* Zero-length secrets are permitted! */
    ++	if (oauth_client_secret)	/* Zero-length secrets are permitted! */
    + 	{
    + 		/*----
    + 		 * Use HTTP Basic auth to send the client_id and secret. Per RFC 6749,
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: add_client_identification(struct async_ctx *actx, PQExpBuffer reqbody, PGconn *c
    + 		 * would it be redundant, but some providers in the wild (e.g. Okta)
    + 		 * refuse to accept it.
    + 		 */
    +-		username = urlencode(conn->oauth_client_id);
    +-		password = urlencode(conn->oauth_client_secret);
    ++		username = urlencode(oauth_client_id);
    ++		password = urlencode(oauth_client_secret);
    + 
    + 		if (!username || !password)
    + 		{
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: add_client_identification(struct async_ctx *actx, PQExpBuffer reqbody, PGconn *c
    + 		 * If we're not otherwise authenticating, client_id is REQUIRED in the
    + 		 * request body.
    + 		 */
    +-		build_urlencoded(reqbody, "client_id", conn->oauth_client_id);
    ++		build_urlencoded(reqbody, "client_id", oauth_client_id);
    + 
    + 		CHECK_SETOPT(actx, CURLOPT_HTTPAUTH, CURLAUTH_NONE, goto cleanup);
    + 		actx->used_basic_auth = false;
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: cleanup:
    + static bool
    + start_device_authz(struct async_ctx *actx, PGconn *conn)
    + {
    ++	const char *oauth_scope = conn_oauth_scope(conn);
    + 	const char *device_authz_uri = actx->provider.device_authorization_endpoint;
    + 	PQExpBuffer work_buffer = &actx->work_data;
    + 
    +-	Assert(conn->oauth_client_id);	/* ensured by setup_oauth_parameters() */
    ++	Assert(conn_oauth_client_id(conn)); /* ensured by setup_oauth_parameters() */
    + 	Assert(device_authz_uri);	/* ensured by check_for_device_flow() */
    + 
    + 	/* Construct our request body. */
    + 	resetPQExpBuffer(work_buffer);
    +-	if (conn->oauth_scope && conn->oauth_scope[0])
    +-		build_urlencoded(work_buffer, "scope", conn->oauth_scope);
    ++	if (oauth_scope && oauth_scope[0])
    ++		build_urlencoded(work_buffer, "scope", oauth_scope);
    + 
    + 	if (!add_client_identification(actx, work_buffer, conn))
    + 		return false;
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: start_token_request(struct async_ctx *actx, PGconn *conn)
    + 	const char *device_code = actx->authz.device_code;
    + 	PQExpBuffer work_buffer = &actx->work_data;
    + 
    +-	Assert(conn->oauth_client_id);	/* ensured by setup_oauth_parameters() */
    ++	Assert(conn_oauth_client_id(conn)); /* ensured by setup_oauth_parameters() */
    + 	Assert(token_uri);			/* ensured by parse_provider() */
    + 	Assert(device_code);		/* ensured by parse_device_authz() */
    + 
     @@ src/interfaces/libpq-oauth/oauth-curl.c: prompt_user(struct async_ctx *actx, PGconn *conn)
      		.verification_uri_complete = actx->authz.verification_uri_complete,
      		.expires_in = actx->authz.expires_in,
    @@ src/interfaces/libpq-oauth/oauth-curl.c: prompt_user(struct async_ctx *actx, PGc
      
      	if (!res)
      	{
    -@@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: done:
    + static PostgresPollingStatusType
    + pg_fe_run_oauth_flow_impl(PGconn *conn)
      {
    - 	fe_oauth_state *state = conn->sasl_state;
    +-	fe_oauth_state *state = conn->sasl_state;
    ++	fe_oauth_state *state = conn_sasl_state(conn);
      	struct async_ctx *actx;
    ++	char	   *oauth_token = NULL;
     +	PQExpBuffer errbuf;
      
      	if (!initialize_curl(conn))
      		return PGRES_POLLING_FAILED;
     @@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
    + 	do
    + 	{
    + 		/* By default, the multiplexer is the altsock. Reassign as desired. */
    +-		conn->altsock = actx->mux;
    ++		set_conn_altsock(conn, actx->mux);
    + 
    + 		switch (actx->step)
    + 		{
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
    + 				 */
    + 				if (!timer_expired(actx))
    + 				{
    +-					conn->altsock = actx->timerfd;
    ++					set_conn_altsock(conn, actx->timerfd);
    + 					return PGRES_POLLING_READING;
    + 				}
      
    - error_return:
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
    + 		{
    + 			case OAUTH_STEP_INIT:
    + 				actx->errctx = "failed to fetch OpenID discovery document";
    +-				if (!start_discovery(actx, conn->oauth_discovery_uri))
    ++				if (!start_discovery(actx, conn_oauth_discovery_uri(conn)))
    + 					goto error_return;
      
    -+	/*
    -+	 * For the dynamic module build, we can't safely rely on the offset of
    -+	 * conn->errorMessage, since it depends on build options like USE_SSL et
    -+	 * al. libpq gives us a translator function instead.
    -+	 */
    -+#ifdef USE_DYNAMIC_OAUTH
    + 				actx->step = OAUTH_STEP_DISCOVERY;
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
    + 				break;
    + 
    + 			case OAUTH_STEP_TOKEN_REQUEST:
    +-				if (!handle_token_response(actx, &conn->oauth_token))
    ++				if (!handle_token_response(actx, &oauth_token))
    + 					goto error_return;
    + 
    ++				/*
    ++				 * Hook any oauth_token into the PGconn immediately so that
    ++				 * the allocation isn't lost in case of an error.
    ++				 */
    ++				set_conn_oauth_token(conn, oauth_token);
    ++
    + 				if (!actx->user_prompted)
    + 				{
    + 					/*
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
    + 					actx->user_prompted = true;
    + 				}
    + 
    +-				if (conn->oauth_token)
    ++				if (oauth_token)
    + 					break;		/* done! */
    + 
    + 				/*
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
    + 				 * the client wait directly on the timerfd rather than the
    + 				 * multiplexer.
    + 				 */
    +-				conn->altsock = actx->timerfd;
    ++				set_conn_altsock(conn, actx->timerfd);
    + 
    + 				actx->step = OAUTH_STEP_WAIT_INTERVAL;
    + 				actx->running = 1;
    +@@ src/interfaces/libpq-oauth/oauth-curl.c: pg_fe_run_oauth_flow_impl(PGconn *conn)
    + 		 * point, actx->running will be set. But there are some corner cases
    + 		 * where we can immediately loop back around; see start_request().
    + 		 */
    +-	} while (!conn->oauth_token && !actx->running);
    ++	} while (!oauth_token && !actx->running);
    + 
    + 	/* If we've stored a token, we're done. Otherwise come back later. */
    +-	return conn->oauth_token ? PGRES_POLLING_OK : PGRES_POLLING_READING;
    ++	return oauth_token ? PGRES_POLLING_OK : PGRES_POLLING_READING;
    + 
    + error_return:
     +	errbuf = conn_errorMessage(conn);
    -+#else
    -+	errbuf = &conn->errorMessage;
    -+#endif
    -+
    + 
      	/*
      	 * Assemble the three parts of our error: context, body, and detail. See
      	 * also the documentation for struct async_ctx.
    @@ src/interfaces/libpq-oauth/oauth-utils.c (new)
     +
     +#include <signal.h>
     +
    -+#include "libpq-int.h"
     +#include "oauth-utils.h"
     +
     +#ifndef USE_DYNAMIC_OAUTH
     +#error oauth-utils.c is not supported in static builds
     +#endif
     +
    -+static libpq_gettext_func libpq_gettext_impl;
    ++#ifdef LIBPQ_INT_H
    ++#error do not rely on libpq-int.h in libpq-oauth
    ++#endif
    ++
    ++/*
    ++ * Function pointers set by libpq_oauth_init().
    ++ */
     +
     +pgthreadlock_t pg_g_threadlock;
    ++static libpq_gettext_func libpq_gettext_impl;
    ++
     +conn_errorMessage_func conn_errorMessage;
    ++conn_oauth_client_id_func conn_oauth_client_id;
    ++conn_oauth_client_secret_func conn_oauth_client_secret;
    ++conn_oauth_discovery_uri_func conn_oauth_discovery_uri;
    ++conn_oauth_issuer_id_func conn_oauth_issuer_id;
    ++conn_oauth_scope_func conn_oauth_scope;
    ++conn_sasl_state_func conn_sasl_state;
    ++
    ++set_conn_altsock_func set_conn_altsock;
    ++set_conn_oauth_token_func set_conn_oauth_token;
     +
     +/*-
     + * Initializes libpq-oauth by setting necessary callbacks.
    @@ src/interfaces/libpq-oauth/oauth-utils.c (new)
     + *
     + * - libpq_gettext: translates error messages using libpq's message domain
     + *
    -+ * - conn->errorMessage: holds translated errors for the connection. This is
    -+ *   handled through a translation shim, which avoids either depending on the
    -+ *   offset of the errorMessage in PGconn, or needing to export the variadic
    -+ *   libpq_append_conn_error().
    ++ * The implementation also needs access to several members of the PGconn struct,
    ++ * which are not guaranteed to stay in place across minor versions. Accessors
    ++ * (named conn_*) and mutators (named set_conn_*) are injected here.
     + */
     +void
     +libpq_oauth_init(pgthreadlock_t threadlock_impl,
     +				 libpq_gettext_func gettext_impl,
    -+				 conn_errorMessage_func errmsg_impl)
    ++				 conn_errorMessage_func errmsg_impl,
    ++				 conn_oauth_client_id_func clientid_impl,
    ++				 conn_oauth_client_secret_func clientsecret_impl,
    ++				 conn_oauth_discovery_uri_func discoveryuri_impl,
    ++				 conn_oauth_issuer_id_func issuerid_impl,
    ++				 conn_oauth_scope_func scope_impl,
    ++				 conn_sasl_state_func saslstate_impl,
    ++				 set_conn_altsock_func setaltsock_impl,
    ++				 set_conn_oauth_token_func settoken_impl)
     +{
     +	pg_g_threadlock = threadlock_impl;
     +	libpq_gettext_impl = gettext_impl;
     +	conn_errorMessage = errmsg_impl;
    ++	conn_oauth_client_id = clientid_impl;
    ++	conn_oauth_client_secret = clientsecret_impl;
    ++	conn_oauth_discovery_uri = discoveryuri_impl;
    ++	conn_oauth_issuer_id = issuerid_impl;
    ++	conn_oauth_scope = scope_impl;
    ++	conn_sasl_state = saslstate_impl;
    ++	set_conn_altsock = setaltsock_impl;
    ++	set_conn_oauth_token = settoken_impl;
     +}
     +
     +/*
    @@ src/interfaces/libpq-oauth/oauth-utils.h (new)
     +#ifndef OAUTH_UTILS_H
     +#define OAUTH_UTILS_H
     +
    ++#include "fe-auth-oauth.h"
     +#include "libpq-fe.h"
     +#include "pqexpbuffer.h"
     +
    ++/*
    ++ * A bank of callbacks to safely access members of PGconn, which are all passed
    ++ * to libpq_oauth_init() by libpq.
    ++ *
    ++ * Keep these aligned with the definitions in fe-auth-oauth.c as well as the
    ++ * static declarations in oauth-curl.c.
    ++ */
    ++#define DECLARE_GETTER(TYPE, MEMBER) \
    ++	typedef TYPE (*conn_ ## MEMBER ## _func) (PGconn *conn); \
    ++	extern conn_ ## MEMBER ## _func conn_ ## MEMBER;
    ++
    ++#define DECLARE_SETTER(TYPE, MEMBER) \
    ++	typedef void (*set_conn_ ## MEMBER ## _func) (PGconn *conn, TYPE val); \
    ++	extern set_conn_ ## MEMBER ## _func set_conn_ ## MEMBER;
    ++
    ++DECLARE_GETTER(PQExpBuffer, errorMessage);
    ++DECLARE_GETTER(char *, oauth_client_id);
    ++DECLARE_GETTER(char *, oauth_client_secret);
    ++DECLARE_GETTER(char *, oauth_discovery_uri);
    ++DECLARE_GETTER(char *, oauth_issuer_id);
    ++DECLARE_GETTER(char *, oauth_scope);
    ++DECLARE_GETTER(fe_oauth_state *, sasl_state);
    ++
    ++DECLARE_SETTER(pgsocket, altsock);
    ++DECLARE_SETTER(char *, oauth_token);
    ++
    ++#undef DECLARE_GETTER
    ++#undef DECLARE_SETTER
    ++
     +typedef char *(*libpq_gettext_func) (const char *msgid);
    -+typedef PQExpBuffer (*conn_errorMessage_func) (PGconn *conn);
     +
     +/* Initializes libpq-oauth. */
     +extern PGDLLEXPORT void libpq_oauth_init(pgthreadlock_t threadlock,
     +										 libpq_gettext_func gettext_impl,
    -+										 conn_errorMessage_func errmsg_impl);
    ++										 conn_errorMessage_func errmsg_impl,
    ++										 conn_oauth_client_id_func clientid_impl,
    ++										 conn_oauth_client_secret_func clientsecret_impl,
    ++										 conn_oauth_discovery_uri_func discoveryuri_impl,
    ++										 conn_oauth_issuer_id_func issuerid_impl,
    ++										 conn_oauth_scope_func scope_impl,
    ++										 conn_sasl_state_func saslstate_impl,
    ++										 set_conn_altsock_func setaltsock_impl,
    ++										 set_conn_oauth_token_func settoken_impl);
     +
    -+/* Callback to safely obtain conn->errorMessage from a PGconn. */
    -+extern conn_errorMessage_func conn_errorMessage;
    ++/*
    ++ * Duplicated APIs, copied from libpq (primarily libpq-int.h, which we cannot
    ++ * depend on here).
    ++ */
    ++
    ++typedef enum
    ++{
    ++	PG_BOOL_UNKNOWN = 0,		/* Currently unknown */
    ++	PG_BOOL_YES,				/* Yes (true) */
    ++	PG_BOOL_NO					/* No (false) */
    ++} PGTernaryBool;
     +
    -+/* Duplicated APIs, copied from libpq. */
     +extern void libpq_append_conn_error(PGconn *conn, const char *fmt,...) pg_attribute_printf(2, 3);
     +extern bool oauth_unsafe_debugging_enabled(void);
     +extern int	pq_block_sigpipe(sigset_t *osigset, bool *sigpipe_pending);
     +extern void pq_reset_sigpipe(sigset_t *osigset, bool sigpipe_pending, bool got_epipe);
     +
    ++#ifdef ENABLE_NLS
    ++extern char *libpq_gettext(const char *msgid) pg_attribute_format_arg(1);
    ++#else
    ++#define libpq_gettext(x) (x)
    ++#endif
    ++
    ++extern pgthreadlock_t pg_g_threadlock;
    ++
    ++#define pglock_thread()		pg_g_threadlock(true)
    ++#define pgunlock_thread()	pg_g_threadlock(false)
    ++
     +#endif							/* OAUTH_UTILS_H */
     
      ## src/interfaces/libpq/Makefile ##
    @@ src/interfaces/libpq/fe-auth-oauth.c: cleanup_user_oauth_flow(PGconn *conn)
     + */
     +
     +typedef char *(*libpq_gettext_func) (const char *msgid);
    -+typedef PQExpBuffer (*conn_errorMessage_func) (PGconn *conn);
     +
     +/*
    -+ * This shim is injected into libpq-oauth so that it doesn't depend on the
    -+ * offset of conn->errorMessage.
    -+ *
    -+ * TODO: look into exporting libpq_append_conn_error or a comparable API from
    -+ * libpq, instead.
    ++ * Define accessor/mutator shims to inject into libpq-oauth, so that it doesn't
    ++ * depend on the offsets within PGconn. (These have changed during minor version
    ++ * updates in the past.)
     + */
    -+static PQExpBuffer
    -+conn_errorMessage(PGconn *conn)
    -+{
    -+	return &conn->errorMessage;
    -+}
    ++
    ++#define DEFINE_GETTER(TYPE, MEMBER) \
    ++	typedef TYPE (*conn_ ## MEMBER ## _func) (PGconn *conn); \
    ++	static TYPE conn_ ## MEMBER(PGconn *conn) { return conn->MEMBER; }
    ++
    ++/* Like DEFINE_GETTER, but returns a pointer to the member. */
    ++#define DEFINE_GETTER_P(TYPE, MEMBER) \
    ++	typedef TYPE (*conn_ ## MEMBER ## _func) (PGconn *conn); \
    ++	static TYPE conn_ ## MEMBER(PGconn *conn) { return &conn->MEMBER; }
    ++
    ++#define DEFINE_SETTER(TYPE, MEMBER) \
    ++	typedef void (*set_conn_ ## MEMBER ## _func) (PGconn *conn, TYPE val); \
    ++	static void set_conn_ ## MEMBER(PGconn *conn, TYPE val) { conn->MEMBER = val; }
    ++
    ++DEFINE_GETTER_P(PQExpBuffer, errorMessage);
    ++DEFINE_GETTER(char *, oauth_client_id);
    ++DEFINE_GETTER(char *, oauth_client_secret);
    ++DEFINE_GETTER(char *, oauth_discovery_uri);
    ++DEFINE_GETTER(char *, oauth_issuer_id);
    ++DEFINE_GETTER(char *, oauth_scope);
    ++DEFINE_GETTER(fe_oauth_state *, sasl_state);
    ++
    ++DEFINE_SETTER(pgsocket, altsock);
    ++DEFINE_SETTER(char *, oauth_token);
     +
     +/*
     + * Loads the libpq-oauth plugin via dlopen(), initializes it, and plugs its
    @@ src/interfaces/libpq/fe-auth-oauth.c: cleanup_user_oauth_flow(PGconn *conn)
     +
     +	void		(*init) (pgthreadlock_t threadlock,
     +						 libpq_gettext_func gettext_impl,
    -+						 conn_errorMessage_func errmsg_impl);
    ++						 conn_errorMessage_func errmsg_impl,
    ++						 conn_oauth_client_id_func clientid_impl,
    ++						 conn_oauth_client_secret_func clientsecret_impl,
    ++						 conn_oauth_discovery_uri_func discoveryuri_impl,
    ++						 conn_oauth_issuer_id_func issuerid_impl,
    ++						 conn_oauth_scope_func scope_impl,
    ++						 conn_sasl_state_func saslstate_impl,
    ++						 set_conn_altsock_func setaltsock_impl,
    ++						 set_conn_oauth_token_func settoken_impl);
     +	PostgresPollingStatusType (*flow) (PGconn *conn);
     +	void		(*cleanup) (PGconn *conn);
     +
    @@ src/interfaces/libpq/fe-auth-oauth.c: cleanup_user_oauth_flow(PGconn *conn)
     +	 */
     +	const char *const module_name =
     +#if defined(__darwin__)
    -+		LIBDIR "/libpq-oauth-" PG_MAJORVERSION "-" PG_MINORVERSION DLSUFFIX;
    ++		LIBDIR "/libpq-oauth-" PG_MAJORVERSION DLSUFFIX;
     +#else
    -+		"libpq-oauth-" PG_MAJORVERSION "-" PG_MINORVERSION DLSUFFIX;
    ++		"libpq-oauth-" PG_MAJORVERSION DLSUFFIX;
     +#endif
     +
     +	state->builtin_flow = dlopen(module_name, RTLD_NOW | RTLD_LOCAL);
    @@ src/interfaces/libpq/fe-auth-oauth.c: cleanup_user_oauth_flow(PGconn *conn)
     +#else
     +			 NULL,
     +#endif
    -+			 conn_errorMessage);
    ++			 conn_errorMessage,
    ++			 conn_oauth_client_id,
    ++			 conn_oauth_client_secret,
    ++			 conn_oauth_discovery_uri,
    ++			 conn_oauth_issuer_id,
    ++			 conn_oauth_scope,
    ++			 conn_sasl_state,
    ++			 set_conn_altsock,
    ++			 set_conn_oauth_token);
     +
     +		initialized = true;
     +	}
    @@ src/interfaces/libpq/fe-auth-oauth.c: setup_token_request(PGconn *conn, fe_oauth
      	return true;
     
      ## src/interfaces/libpq/fe-auth-oauth.h ##
    -@@ src/interfaces/libpq/fe-auth-oauth.h: typedef struct
    +@@
    + #ifndef FE_AUTH_OAUTH_H
    + #define FE_AUTH_OAUTH_H
    + 
    ++#include "fe-auth-sasl.h"
    + #include "libpq-fe.h"
    +-#include "libpq-int.h"
    + 
    + 
    + enum fe_oauth_step
    +@@ src/interfaces/libpq/fe-auth-oauth.h: enum fe_oauth_step
    + 	FE_OAUTH_SERVER_ERROR,
    + };
    + 
    ++/*
    ++ * This struct is exported to the libpq-oauth module. If changes are needed
    ++ * during backports to stable branches, please keep ABI compatibility (no
    ++ * changes to existing members, add new members at the end, etc.).
    ++ */
    + typedef struct
    + {
    + 	enum fe_oauth_step step;
      
      	PGconn	   *conn;
      	void	   *async_ctx;

---

From: Jacob Champion <[email-masked]>
Date: Wed, 30 Apr 2025 10:59:43 -0700

On Wed, Apr 30, 2025 at 5:55 AM Daniel Gustafsson <[email-masked]> wrote:
> 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 <[email-masked]>
         Reviewed-by: Andres Freund <[email-masked]>
         Reviewed-by: Christoph Berg <[email-masked]>
    +    Reviewed-by: Daniel Gustafsson <[email-masked]>
         Reviewed-by: Jelte Fennema-Nio <[email-masked]>
         Reviewed-by: Peter Eisentraut <[email-masked]>
         Reviewed-by: Wolfgang Walther <[email-masked]>
    @@ 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
     +
     +/*

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 30 Apr 2025 20:09:28 +0200

> On 30 Apr 2025, at 19:59, Jacob Champion <[email-masked]> wrote:

> see it. I've also fixed up an errant #endif label right above it.

That's indeed better than my suggestion.

> I'd ideally like to get a working split in for beta.

+Many

> Barring
> freeze.

I'll try to kick the tyres a bit more as well.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Thu, 1 May 2025 10:38:15 -0700

> I'll try to kick the tyres a bit more as well.

Thanks! Alpine seems to be happy with the dlopen() arrangement. And
I've thrown some more Autoconf testing at Rocky, Mac, and Ubuntu.

So, committed. Thanks everyone for all the excellent feedback!
(Further feedback is still very welcome.)

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 1 May 2025 12:24:19 -0700

On Thu, May 1, 2025 at 10:38 AM Jacob Champion
<[email-masked]> wrote:
> So, committed.

I forgot --enable-nls in my Mac testing, so indri complains about my
omission of -lintl... I'd incorrectly thought it was no longer needed
after all the gettext motion.

I'm running the attached fixup through CI now.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 1 May 2025 13:31:05 -0700

> I'm running the attached fixup through CI now.

(Pushed, and indri is happy again.)

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 1 May 2025 13:41:30 -0700

On Mon, Apr 21, 2025 at 9:57 AM Jacob Champion
<[email-masked]> wrote:
> So to recap: I'm happy to add a Google compatibility mode, but I'd
> like to gather some evidence that their device flow can actually
> authorize tokens for third parties safely, before we commit to that.
> Thoughts?

Hi Ivan, I know the thread has been deep in discussion around the
module split, but I was wondering if you'd had any thoughts on the
Google safety problem?

--Jacob

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 2 May 2025 10:11:08 -0500

After commit b0635bf, I'm seeing the following meson build failures on
macOS:

	In file included from ../postgresql/src/interfaces/libpq-oauth/oauth-curl.c:51:
	../postgresql/src/interfaces/libpq/libpq-int.h:70:10: fatal error: 'openssl/ssl.h' file not found
	   70 | #include <openssl/ssl.h>
	      |          ^~~~~~~~~~~~~~~
	1 error generated.

The following patch seems to resolve it.  I'm curious if commit 4ea1254
might apply to meson, too, but FWIW I haven't noticed any related failures
on my machine.

diff --git a/meson.build b/meson.build
index 29d46c8ad01..19ad03042d3 100644
--- a/meson.build
+++ b/meson.build
@@ -3295,6 +3295,7 @@ libpq_deps += [

 libpq_oauth_deps += [
   libcurl,
+  ssl,
 ]

 subdir('src/interfaces/libpq')

-- 
nathan

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 08:46:21 -0700

> After commit b0635bf, I'm seeing the following meson build failures on

Thanks for the report, and sorry for the breakage.

>            70 | #include <openssl/ssl.h>

Hm. My test setup here is Homebrew with -Dextra_include_dirs, which
may explain why it's not failing for me. Looks like Cirrus also has
-Dextra_include_dirs...

> might apply to meson, too, but FWIW I haven't noticed any related failures

Yeah, I wonder if libintl is being similarly "cheated" on the Meson side.

> @@ -3295,6 +3295,7 @@ libpq_deps += [

Thanks! I think the include directory is the only thing needed for the
static library, not the full link dependency. But I'll try to build up
a new Meson setup, with minimal added settings, to see if I can
reproduce here. Can you share your Meson configuration?

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 08:59:18 -0700

> Yeah, I wonder if libintl is being similarly "cheated" on the Meson side.

libintl is already coming in via frontend_stlib_code, so that's fine.
So now I'm wondering if any other static clients of libpq-int.h (if
there are any) need the ssl dependency too, for correctness, or if
it's just me.

> But I'll try to build up

(Never mind -- this was pretty easy to hit in a from-scratch configuration.)

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 10:05:26 -0700

> there are any) need the ssl dependency too, for correctness, or if

Looks like it's just me. And using partial_dependency for the includes
seems like overkill, so I've kept the full ssl dependency object, but
moved it to the staticlib only, which is enough to solve the breakage
on my machine.

Nathan, if you get a chance, does the attached patch work for you?

Thanks!
--Jacob

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 2 May 2025 12:31:27 -0500

> Nathan, if you get a chance, does the attached patch work for you?

Yup, thanks!

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Fri, 02 May 2025 13:35:30 -0400

> Nathan, if you get a chance, does the attached patch work for you?

FWIW, on my Mac a meson build from HEAD goes through fine, with or
without this patch.  I'm getting openssl and libcurl from MacPorts
not Homebrew, but I don't know why that would make any difference.

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 10:42:25 -0700

> not Homebrew, but I don't know why that would make any difference.

Do your <libintl.h> and <openssl/*.h> live in the same place? Mine do,
so I had to disable NLS to get this to break.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 10:42:34 -0700

> Yup, thanks!

Great, thanks. I'll push it soon.

--Jacob

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 2 May 2025 12:59:12 -0500

> so I had to disable NLS to get this to break.

I enabled NLS and the problem disappeared for me, but that seems to be a
side effect of setting -Dextra_{include,lib}_dirs to point to my Homebrew
directories, which I needed to do to get NLS to work.

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Fri, 02 May 2025 14:25:49 -0400

> so I had to disable NLS to get this to break.

Yeah, they are both under /opt/local/include in a MacPorts setup.
But disabling NLS doesn't break it for me.  I tried

meson setup build --auto-features=disabled -Dlibcurl=enabled

to make sure that /opt/local/include wasn't getting pulled in
some other way, and it still builds.

Apropos of that: our fine manual claims that option is spelled
--auto_features, but that fails for me.  Is that a typo in our
manual, or do some meson versions accept the underscore?

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 11:46:15 -0700

> some other way, and it still builds.

Hm. If you clear out the build artifacts under
src/interfaces/libpq-oauth, and then build with

    $ ninja -v src/interfaces/libpq-oauth/libpq-oauth.a

does that help surface anything interesting?

> manual, or do some meson versions accept the underscore?

--auto_features doesn't work for me either. That looks like an
accidental mashup of --auto-features and -Dauto_features.

--Jacob

---

From: Tom Lane <[email-masked]>
Date: Fri, 02 May 2025 14:52:36 -0400

> does that help surface anything interesting?

$ rm -rf src/interfaces/libpq-oauth
$ ninja -v src/interfaces/libpq-oauth/libpq-oauth.a
[2/2] rm -f src/interfaces/libpq-oauth/libpq-oauth.a && ar csr src/interfaces/libpq-oauth/libpq-oauth.a src/interfaces/libpq-oauth/libpq-oauth.a.p/oauth-curl.c.o && ranlib -c src/interfaces/libpq-oauth/libpq-oauth.a

So it's getting -I/opt/local/include and also
-I/opt/local/libexec/openssl3/include from somewhere,
which I guess must be libcurl's pkg-config data ... yup:

$ pkg-config --cflags libcurl
-I/opt/local/include -I/opt/local/libexec/openssl3/include -I/opt/local/include

I bet Homebrew's libcurl packaging doesn't do that.

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 11:56:46 -0700

> I bet Homebrew's libcurl packaging doesn't do that.

Nope, Homebrew breaks them out into smaller pieces:

    % PKG_CONFIG_PATH=/opt/homebrew/opt/curl/lib/pkgconfig pkg-config
--cflags libcurl
    -I/opt/homebrew/Cellar/curl/8.13.0/include
-I/opt/homebrew/Cellar/brotli/1.1.0/include
-I/opt/homebrew/opt/zstd/include
-I/opt/homebrew/Cellar/libssh2/1.11.1/include
-I/opt/homebrew/Cellar/rtmpdump/2.4-20151223_3/include
-I/opt/homebrew/Cellar/openssl@3/3.5.0/include
-I/opt/homebrew/Cellar/libnghttp2/1.65.0/include

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 May 2025 12:05:10 -0700

> -I/opt/homebrew/Cellar/openssl@3/3.5.0/include

Except it _is_ right there.

Oh, ha -- I'm not using Homebrew's Curl in this minimal build. Looks
like it's coming from the sysroot.

    % ls -l /Library/Developer/CommandLineTools/SDKs/MacOSX15.2.sdk/usr/include/curl
    total 208
    -rw-r--r--  1 root  wheel  129052 Nov  9 22:54 curl.h
    -rw-r--r--  1 root  wheel    3044 Nov  9 22:54 curlver.h
    ...

Well, that was fun.

--Jacob

---

From: Tom Lane <[email-masked]>
Date: Fri, 02 May 2025 15:05:26 -0400

> accidental mashup of --auto-features and -Dauto_features.

Ah, I see somebody already complained of this [1], but apparently
we did nothing about it.  I shall go fix it now.

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/172465652540.862882.17808523044292761256%40wrigleys.postgresql.org

---

From: Christoph Berg <[email-masked]>
Date: Sat, 3 May 2025 16:54:59 +0200

Re: Jacob Champion
> So, committed. Thanks everyone for all the excellent feedback!

The package split between libpq5 and libpq-oauth in Debian has already
been accepted into the experimental branch.

Thanks,
Christoph

---

From: Wolfgang Walther <[email-masked]>
Date: Sun, 4 May 2025 14:58:48 +0200

> Nathan, if you get a chance, does the attached patch work for you?

I couldn't reproduce the problem, so did not test the latest patch. But 
I tested a lot of scenarios on nixpkgs with latest master (250a718a):

- aarch64 + x86_64 architectures, both Linux and MacOS

- Autoconf and Meson

- Various features enabled / disabled in different configurations (NLS, 
OpenSSL, GSSAPI)

- And additionally some cross-compiling from x86_64 Linux to aarch64 
Linux and x86_64 FreeBSD

Worked very well.

The only inconsistency I was able to find is the autoconf-generated 
libpq.pc file, which has this:

   Requires.private: libssl, libcrypto libcurl

Note the missing "," before libcurl.

It does *not* affect functionality, though:

   pkg-config --print-requires-private libpq
   libssl
   libcrypto
   libcurl

The meson-generated libpq.pc looks like this:

   Requires.private: openssl, krb5-gssapi, libcurl >=  7.61.0

I was only able to test the latter in an end-to-end fully static build 
of a downstream dependency - works great. The final executable has all 
the expected oauth strings in it.

Best,

Wolfgang

---

From: Devrim Gündüz <[email-masked]>
Date: Mon, 05 May 2025 15:37:45 +0300

Hi,

> been accepted into the experimental branch.

RPMs will ship postgresql18-libs and postgresql18-libs-oauth. The latter
depends on the former for sure.

Regards,

-- 
Devrim Gündüz
Open Source Solution Architect, PostgreSQL Major Contributor
BlueSky: @devrim.gunduz.org , @gunduz.org

---

From: Jacob Champion <[email-masked]>
Date: Tue, 6 May 2025 10:49:31 -0700

>    Requires.private: libssl, libcrypto libcurl

Oh, I see what I did. Will fix, thanks.

> the expected oauth strings in it.

Thank you so much for all the detailed testing!

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 12 Jun 2025 12:58:44 -0700

On Thu, Mar 6, 2025 at 12:57 PM Jacob Champion
<[email-masked]> wrote:
> 3) There is a related performance bug on other platforms. If a Curl
> timeout happens partway through a request (so libcurl won't clear it),
> the timer-expired event will stay set and CPU will be burned to spin
> pointlessly on drive_request(). This is much easier to notice after
> taking Happy Eyeballs out of the picture. It doesn't cause logical
> failures -- Curl basically discards the unnecessary calls -- but it's
> definitely unintended.
>
> ...
>
> I plan to defer working on Problem 3, which should just be a
> performance bug, until the tests are green again. And I would like to
> eventually add some stronger unit tests for the timer behavior, to
> catch other potential OS-specific problems in the future.

To follow up on this: I had intended to send a patch fixing the timer
bug this week, but after fixing it, the performance problem did not
disappear. Turns out: other file descriptors can get stuck open on
BSD, depending on how complicated Curl wants to make the order of
operations, and the existing tests aren't always enough to expose it.
(It also depends on the Curl version installed.)

I will split this off into its own thread soon, because this
megathread is just too big, but I wanted to make a note here and file
an open item. As part of that, I have a set of more rigorous unit
tests for the libcurl-libpq interaction that I'm working on, since the
external view of "the flow worked/didn't work" is not enough to
indicate internal health.

--Jacob

---

From: Ivan Kush <[email-masked]>
Date: Fri, 20 Jun 2025 13:08:35 +0300

Hello!

This patch fixes CPPFLAGS, LDFLAGS, LIBS when checking AsyncDNS libcurl 
support in configure

Custom parameters and paths to libcurl were mistakenly excluded from 
CPPFLAGS, LDFLAGS, and LIBS, although AsyncDNS check was OK.

For example, the command `pkg-config --libs libcurl` gives 
`-L/usr/local/lib -lcurl`. LDFLAGS will not contain `-L/usr/local/lib`.

This patch fixes such behaviour.

Test case:

I've tested custom Postgres in an old Debian based Linux distro. This 
distro contains old libcurl (< 7.61, package libcurl3) that was compiled 
with symbols CURL_OPENSSL_3. So I've installed newer version of 
libcurlssl, package libcurl4-openssl-dev, that contains symbols 
CURL_OPENSSL_4 and compiled my libcurl version > 7.61.

After compilation during testing some Postgres shared libraries or 
binaries that was linked with libcurl showed an error "version 
CURL_OPENSSL_3 not found (required by …/libcurl.so.4)"

-- 
Best wishes,
Ivan Kush
Tantor Labs LLC
From 8a24c24f85c40e2aa0c40afc8f9cd7a19afa66c3 Mon Sep 17 00:00:00 2001
From: Ivan Kush <[email-masked]>
Date: Fri, 20 Jun 2025 12:16:47 +0300
Subject: [PATCH] oauth: Fix CPPFLAGS, LDFLAGS, LIBS when checking AsyncDNS libcurl support

Custom parameters and paths to libcurl were mistakenly excluded from CPPFLAGS,
LDFLAGS, and LIBS.
For example, the command `pkg-config --libs libcurl` gives
`-L/usr/local/lib -lcurl`. LDFLAGS will not contain `-L/usr/local/lib`

This patch fixes this.

Author: Ivan Kush <[email-masked]>
Author: Lev Nikolaev <[email-masked]>
Reviewed-by:
Discussion:
---
 config/programs.m4 | 7 +++----
 configure          | 8 +++-----
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/config/programs.m4 b/config/programs.m4
index 0ad1e58b48d..2556e469323 100644
--- a/config/programs.m4
+++ b/config/programs.m4
@@ -348,9 +348,8 @@ AC_DEFUN([PGAC_CHECK_LIBCURL],
 *** The installed version of libcurl does not support asynchronous DNS
 *** lookups. Rebuild libcurl with the AsynchDNS feature enabled in order
 *** to use it with libpq.])
+    CPPFLAGS=$pgac_save_CPPFLAGS
+    LDFLAGS=$pgac_save_LDFLAGS
+    LIBS=$pgac_save_LIBS
   fi
-
-  CPPFLAGS=$pgac_save_CPPFLAGS
-  LDFLAGS=$pgac_save_LDFLAGS
-  LIBS=$pgac_save_LIBS
 ])# PGAC_CHECK_LIBCURL
diff --git a/configure b/configure
index 4f15347cc95..46a011d1d1b 100755
--- a/configure
+++ b/configure
@@ -12883,12 +12883,10 @@ $as_echo "$pgac_cv__libcurl_async_dns" >&6; }
 *** The installed version of libcurl does not support asynchronous DNS
 *** lookups. Rebuild libcurl with the AsynchDNS feature enabled in order
 *** to use it with libpq." "$LINENO" 5
+    CPPFLAGS=$pgac_save_CPPFLAGS
+    LDFLAGS=$pgac_save_LDFLAGS
+    LIBS=$pgac_save_LIBS
   fi
-
-  CPPFLAGS=$pgac_save_CPPFLAGS
-  LDFLAGS=$pgac_save_LDFLAGS
-  LIBS=$pgac_save_LIBS
-
 fi

 if test "$with_gssapi" = yes ; then
--
2.34.1

---

From: Jacob Champion <[email-masked]>
Date: Mon, 23 Jun 2025 08:32:26 -0700

> This patch fixes CPPFLAGS, LDFLAGS, LIBS when checking AsyncDNS libcurl

Hi Ivan, thanks for the report! Your patch puts new logic directly
after an AC_MSG_ERROR() call, so any effect has to come from the fact
that we're no longer restoring the old compiler and linker flags.
That's not what we want -- Curl needs to be isolated from the rest of
the build.

> CURL_OPENSSL_3 not found (required by …/libcurl.so.4)"

What's your configure line? You need to make sure that your custom
libcurl is used at configure-time, compile-time, and run-time.

And which binaries are complaining? The only thing that should ever be
linked against libcurl is libpq-oauth-18.so.

Thanks,
--Jacob

---

From: Andres Freund <[email-masked]>
Date: Mon, 30 Jun 2025 12:58:18 -0400

Hi,

> Great, thanks. I'll push it soon.

I just noticed that I think the dependencies on the meson build aren't quite
sufficient:

andres@awork3:/srv/dev/build/postgres/m-dev-assert$ ninja install-quiet
[2205/2205 1 100%] Generating install-quiet with a custom command
FAILED: install-quiet
/usr/bin/python3 /home/andres/src/meson/meson.py install --quiet --no-rebuild

ERROR: File 'src/interfaces/libpq-oauth/libpq-oauth.a' could not be found
ninja: build stopped: subcommand failed.

Probably just needs to be added to the installed_targets list.

Greetings,

Andres Freund

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 30 Jun 2025 19:01:50 +0200

> On 30 Jun 2025, at 18:58, Andres Freund <[email-masked]> wrote:
> 
> Hi,
> 

Thanks for the report, I'll take a look today to get it fixed.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Mon, 30 Jun 2025 11:33:27 -0700

> Thanks for the report, I'll take a look today to get it fixed.

Thanks both!

Looking at the installed_targets stuff, though... why do we use `meson
install --no-rebuild` in combination with `depends:
installed_targets`? Can't we just use Meson's dependency tracking
during installation, and avoid this hazard?

--Jacob

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 1 Jul 2025 00:52:49 +0200

> On 30 Jun 2025, at 20:33, Jacob Champion <[email-masked]> wrote:
> 

I suspect it is because without --no-rebuild the quiet target isn't entirely
quiet.  Still, I was unable to make something that work in all build
combinations while keeping --no-rebuild (which isn't indicative of it being
possible to do).  Is --no-rebuild just there to reduce output noise, or is
there another reason that I don't see?

--
Daniel Gustafsson

---

From: Andres Freund <[email-masked]>
Date: Mon, 30 Jun 2025 19:42:51 -0400

Hi,

> > during installation, and avoid this hazard?

I don't think that's really possible - the dependency tracking is useful to
generate granular *rebuild* information, but doesn't help with the first
build.

If we had dependency generation for the install target it could be helpful to
discover missing dependencies though.

> quiet.

No - the issue is that you're not allowed to run ninja while ninja is running,
as that would corrupt it's tracking (and build things multiple times). meson
install --no-rebuild would run ninja to build things...

> Still, I was unable to make something that work in all build combinations
> while keeping --no-rebuild (which isn't indicative of it being possible to
> do).

Hm, what problem did you encounter? I don't think there should be any
difficulty?

Greetings,

Andres Freund

---

From: Ivan Kush <[email-masked]>
Date: Wed, 2 Jul 2025 15:45:30 +0300

Thanks for the clarification! I thought linker flags should be installed 
globally for all compilation targets.

Another question:

Why don't we set LIBS in the configure in "checking for curl_multi_init" 
using LIBCURL_LIBS or LIBCURL_LDFLAGS?
https://github.com/postgres/postgres/blob/master/configure#L12734

Like this:
     LIBS="$(LIBCURL_LDFLAGS) $(LIBCURL_LDLIBS)"

And set LIBS with -lcurl.

As I understand we need to check the properties of libcurl we are 
compiling with?
It may be some local libcurl from /opt/my_libcurl. So LIBCURL_... may 
contain a flag like -L/opt/my_libcurl
Without these LIBCURL... variables we will check a system libcurl, not 
our local.

I mean why don't we set LIBS

current *configure*

$as_echo_n "checking for curl_multi_init in -lcurl... " >&6; }
....
else
   ac_check_lib_save_LIBS=$LIBS
LIBS="-lcurl  $LIBS"
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h.  */

https://github.com/postgres/postgres/blob/master/configure#L12734

For example, I've logged flags after this code sample and they don't 
contain -L/opt/my_libcurl

     IVK configure:13648: CFLAGS=-Wall -Wmissing-prototypes 
-Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels 
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing 
-fwrapv -fexcess-precision=standard -pipe -O2
     IVK configure:13649: LDFLAGS=-Wl,-z,relro -Wl,-z,now -flto=auto 
-ffat-lto-objects -L/usr/lib/llvm-10/lib -L/usr/local/lib/zstd
     IVK configure:13650: LIBS=-lcurl  -lz -lreadline -lpthread -lrt 
-ldl -lm
     IVK configure:13651: LDLIBS=

-- 
Best wishes,
Ivan Kush
Tantor Labs LLC

---

From: Jacob Champion <[email-masked]>
Date: Wed, 2 Jul 2025 07:46:40 -0700

> globally for all compilation targets.

Not for libcurl, since the libpq-oauth module split.

> Without these LIBCURL... variables we will check a system libcurl, not

Ah, that's definitely a bug. I've tested alternate PKG_CONFIG_PATHs,
but I haven't regularly tested on systems that have no system libcurl
at all. So those header and lib checks need to be moved after the use
of LIBCURL_CPPFLAGS and LIBCURL_LDFLAGS to prevent a false failure.
Otherwise they're only useful for the LIBCURL_LDLIBS assignment.

I wonder if I should just get rid of those to better match the Meson
implementation... but the error messages from the checks will likely
be nicer than compilation failures during the later test programs. Hm.

(Thanks for the report!)

--Jacob

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Jul 2025 13:36:26 -0400

> Ah, that's definitely a bug.

I just ran into a vaguely-related failure: on RHEL8, building
with --with-libcurl leads to failures during check-world:

../../../../src/interfaces/libpq/libpq.so: undefined reference to `dlopen'
../../../../src/interfaces/libpq/libpq.so: undefined reference to `dlclose'
../../../../src/interfaces/libpq/libpq.so: undefined reference to `dlerror'
../../../../src/interfaces/libpq/libpq.so: undefined reference to `dlsym'
collect2: error: ld returned 1 exit status

Per "man dlopen", you have to link with libdl to use these functions
on this platform.  (Curiously, although RHEL9 still says that in the
documentation, it doesn't seem to actually need -ldl.)  I was able
to resolve this by adding -ldl in libpq's Makefile:

-SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket -lnsl -lresolv -lintl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS)
+SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket -lnsl -lresolv -lintl -ldl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS)

It doesn't look like the Meson support needs such explicit tracking of
required libraries, but perhaps I'm missing something?  I'm not able
to test that directly for lack of a usable ninja version on this
platform.

Apologies for not noticing this sooner.  I don't think I'd tried
--with-libcurl since the changes to split out libpq-oauth.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Wed, 9 Jul 2025 13:46:29 -0400

Hi,

On 2025-07-09 13:36:26 -0400, Tom Lane wrote:
> required libraries, but perhaps I'm missing something?

It should be fine, -ldl is added to "os_deps" if needed, and os_deps is used
for all code in pg.

Greetings,

Andres Freund

---

From: Jacob Champion <[email-masked]>
Date: Wed, 9 Jul 2025 10:59:47 -0700

> +SHLIB_LINK += $(filter -lcrypt -ldes -lcom_err -lcrypto -lk5crypto -lkrb5 -lgssapi_krb5 -lgss -lgssapi -lssl -lsocket -lnsl -lresolv -lintl -ldl -lm, $(LIBS)) $(LDAP_LIBS_FE) $(PTHREAD_LIBS)

Hmm, okay. That analysis and fix look good to me. (It looks like none
of the RHEL animals are testing with Curl yet, and locally I was using
Rocky 9...)

I'll work up a patch to send through the CI. I can't currently test
RHEL8 easily -- Rocky 8 is incompatible with my Macbook? -- which I
will need to rectify eventually, but I can't this week.

Thanks!
--Jacob

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Jul 2025 14:13:08 -0400

> will need to rectify eventually, but I can't this week.

No need, I already tested locally.  Will push shortly.

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Wed, 9 Jul 2025 11:39:41 -0700

> No need, I already tested locally.  Will push shortly.

Thank you very much!

Here is a draft patch for Ivan's reported issue; I still need to put
it through its paces with some more unusual setups, but I want to get
cfbot on it.

--Jacob

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Jul 2025 15:07:07 -0400

> it through its paces with some more unusual setups, but I want to get

I'm confused about why this moves up the temporary changes of
CPPFLAGS and LDFLAGS, but not LIBS?  Maybe that's actually correct,
but it looks strange (and perhaps deserves a comment about why).

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Wed, 9 Jul 2025 12:28:52 -0700

> but it looks strange (and perhaps deserves a comment about why).

Yeah, that's fair. It's because LIBCURL_LDLIBS isn't set until that
AC_CHECK_LIB test is run, and the test needs LIBCURL_LDFLAGS to be in
force.

(Upthread, I was idly wondering if those AC_CHECKs should just be
removed -- after all, PKG_CHECK_MODULES just told us where Curl was --
but I'm nervous that this might make more niche use cases like
cross-compilation harder to use in practice?)

Does the attached help clarify?

Thanks,
--Jacob

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Jul 2025 15:42:30 -0400

> Does the attached help clarify?

Yes, thanks.

> cross-compilation harder to use in practice?)

Nah, let's keep them.  We do document for at least some libraries
how to manually specify the include and link options without
depending on pkg-config.  If someone tries that with libcurl,
it'd be good to have sanity checks on the results.

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Wed, 9 Jul 2025 16:54:02 -0700

> it'd be good to have sanity checks on the results.

Sounds good, thanks for the review!

> it through its paces with some more unusual setups, but I want to get

On HEAD, Rocky 9 fails to build with a custom Curl PKG_CONFIG_PATH and
no libcurl-devel installed. With this patch, that build now succeeds,
and it still succeeds after libcurl-devel is reinstalled, with the
compiler tests continuing to use the custom libcurl and not the
system's.

So I'll give Ivan a little time in case he'd like to test/review
again, but otherwise I plan to push it this week.

Thanks,
--Jacob

---

From: [email-masked]
Date: Thu, 10 Jul 2025 17:41:07 +0300

I agree with the patch. Works in my OSes

---

From: Jacob Champion <[email-masked]>
Date: Fri, 11 Jul 2025 10:33:20 -0700

> I agree with the patch. Works in my OSes

Thanks Ivan! Committed.

--Jacob

---

From: Andres Freund <[email-masked]>
Date: Fri, 18 Jul 2025 13:26:17 -0400

Hi,

> Hm, what problem did you encounter? I don't think there should be any

Ping?

Greetings,

Andres Freund

---

From: Daniel Gustafsson <[email-masked]>
Date: Sat, 19 Jul 2025 00:29:08 +0200

> On 18 Jul 2025, at 19:26, Andres Freund <[email-masked]> wrote:

>> Hm, what problem did you encounter? I don't think there should be any

Ugh, In preparing for going on vacation this fell off the radar.  I'll try to
get to looking at it tomorrow during downtime unless beaten to it.

--
Daniel Gustafsson

---

From: Jacob Champion <[email-masked]>
Date: Fri, 18 Jul 2025 16:31:21 -0700

> get to looking at it tomorrow during downtime unless beaten to it.

Your earlier mail made me worried I'd missed something, but is the
attached diff what Andres was asking for? A `ninja clean; ninja
install-quiet` now works for me with this applied.

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Tue, 5 Aug 2025 11:54:35 -0700

> install-quiet` now works for me with this applied.

Ping. I'll plan to commit this by the beta3 cutoff but it'd be nice to
verify that I'm not missing something obvious. :D

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Fri, 8 Aug 2025 09:23:30 -0700

> verify that I'm not missing something obvious. :D

(Committed yesterday.)

I wonder if there's a Meson feature request in here somewhere, to be
able to compose targets from other builtin targets without having to
duplicate the internal bookkeeping.

--Jacob