Re: [PoC] Federated Authn/z with OAUTHBEARER

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Peter Eisentraut <peter(at)eisentraut(dot)org>, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
Date: 2024-09-11 13:44:37
Message-ID: 20A6A78C-D5B1-4BA3-8CD0-99AF18A04B48@yesql.se
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 11 Sep 2024, at 09:37, Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:

> Is there any sense in dealing with the libpq and backend patches separately in sequence, or is this split just for ease of handling?

I think it's just make reviewing a bit easier. At this point I think they can
be merged together, it's mostly out of historic reasons IIUC since the patchset
earlier on supported more than one library.

> (I suppose the 0004 "review comments" patch should be folded into the respective other patches?)

Yes (0003 now), along with the 0004 in the attached version (I bumped to v29 as
one commit is now committed, but the attached doesn't change Jacobs commits but
rather add to them) which contains more review comments. More on that below:

I added a warning to autconf in case --with-oauth is used without --with-python
since this combination will error out in running the tests. Might be
superfluous but I had an embarrassingly long headscratcher myself as to why the
tests kept failing =)

CURL_IGNORE_DEPRECATION(x;) broke pgindent, it needs to keep the semicolon on
the outside like CURL_IGNORE_DEPRECATION(x);. This doesn't really work well
with how the macro is defined, not sure how we should handle that best (the
attached makes the style as per how pgindent want's it with the semicolon
returned).

The oauth_validator test module need to load Makefile.global before exporting
the symbols from there. I also removed the placeholder regress test which did
nothing and turned diag() calls into note() calls to keep the output from
cluttering.

There is a first stab at documenting the validator module API, more to come (it
doesn't compile right now).

It contains a pgindent and pgperltidy run to keep things as close to in final
sync as we can to catch things like the curl deprecation macro mentioned above
early.

> What could be the next steps to keep this moving along, other than stare at the remaining patches until we're content with them? ;-)

I'm in the "stare at things" stage now to try and get this into the tree =)

To further pick away at this huge patch I propose to merge the SASL message
length hunk which can be extracted separately. The attached .txt (to keep the
CFBot from poking at it) contains a diff which can be committed ahead of the
rest of this patch to make it a tad smaller and to keep the history of that
change a bit clearer.

--
Daniel Gustafsson

Attachment Content-Type Size
v29-0004-Review-comments-2024-09-11.patch application/octet-stream 17.9 KB
v29-0003-Review-comments.patch application/octet-stream 6.3 KB
v29-0002-backend-add-OAUTHBEARER-SASL-mechanism.patch application/octet-stream 67.8 KB
v29-0001-libpq-add-OAUTHBEARER-SASL-mechanism.patch application/octet-stream 118.7 KB
saslmsglength.txt text/plain 2.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2024-09-11 14:01:09 Re: Test improvements and minor code fixes for formatting.c.
Previous Message Nazir Bilal Yavuz 2024-09-11 13:39:57 Re: Allow CI to only run the compiler warnings task