Feedback-ID: ie0a040ee:Fastmail
Message-ID: <5bec3d4f-613f-425b-88c4-59e71c70f7d6@eisentraut.org>
Date: Mon, 7 Apr 2025 15:59:19 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
To: Jacob Champion <jacob.champion@enterprisedb.com>,
 Christoph Berg <myon@debian.org>, Andres Freund <andres@anarazel.de>,
 Tom Lane <tgl@sss.pgh.pa.us>, Bruce Momjian <bruce@momjian.us>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Daniel Gustafsson <daniel@yesql.se>, Thomas Munro <thomas.munro@gmail.com>,
 Nazir Bilal Yavuz <byavuz81@gmail.com>, Antonin Houska <ah@cybertec.at>
References: <CA+hUKG+dMm6FyQvXM2oD269u6BBnaDoPM4DEyYzZJK6SAFCEag@mail.gmail.com>
 <636879.1742357835@sss.pgh.pa.us>
 <CA+hUKGKCYRS34L+BUy!5qhMF55Zg-oCMxDhnSgqm0jWkP+6jYBA@mail.gmail.com>
 <641687.1742360249@sss.pgh.pa.us>
 <A50E37E8-05A4-4406-945D-14C757AE4AD6@yesql.se> <Z9taYF0kIuv42N8e@momjian.us>
 <CAOYmi+kn+NYA=DNMZds8sPXonAFg3dJnTgQ+vsOMXsk1Yz+3Gg@mail.gmail.com>
 <Z9x_sUXIFz8rjvBN@momjian.us> <1120967.1742504934@sss.pgh.pa.us>
 <wbqaa72xxfnqtsspanbteoycmtpb6oshtwbrm7uwiw3pur4ll4@tybxmaasfjkv>
 <Z-qheTno9lYmjrJu@msg.df7cb.de>
 <CAOYmi+n+WB9fuQeoPL-0FWvC+fzJXUEfH=Ne1q7q5xT936TE=A@mail.gmail.com>
 <CAOYmi+moTsgohh5Tf1gn7dBynARV9EFfWaBVPcJD9O=h6RkSCw@mail.gmail.com>
Content-Language: de-DE, en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <CAOYmi+moTsgohh5Tf1gn7dBynARV9EFfWaBVPcJD9O=h6RkSCw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/5bec3d4f-613f-425b-88c4-59e71c70f7d6%40eisentraut.org>
Precedence: bulk

On 05.04.25 02:27, Jacob Champion wrote:
> On Tue, Apr 1, 2025 at 3:40 PM Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
>> 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.
> 
> 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.

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


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

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.


> 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?

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.