Re: [PoC] Federated Authn/z with OAUTHBEARER

Hi all,

Here's v30, which aims to fix the infinite-retry problem: you get a
maximum of two OAuth connections to the server, and we won't prompt
the user more than once per transport. (I still need to wrap my head
around the retry behavior during transport negotiation.)

On Wed, Sep 11, 2024 at 3:54 PM Jacob Champion
<jacob(dot)champion(at)enterprisedb(dot)com> wrote:
> On Wed, Sep 11, 2024 at 6:44 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> >
> > 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 can definitely do that (and yeah, it was to make the review slightly
> less daunting). The server side could potentially be committed
> independently, if you want to parallelize a bit, but it'd have to be
> torn back out if the libpq stuff didn't land in time.

I plan to do the combination in the near future, when I'm not making a
bunch of other changes at the same time.

> > > (I suppose the 0004 "review comments" patch should be folded into the respective other patches?)
>
> Yes. I'm using that patch as a holding area while I write tests for
> the hunks, and then moving them backwards.

This is almost gone; just one piece remaining as of v30. Everything
else has been folded in with new tests.

> > 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).
>
> Ugh... maybe a case for a pre_indent rule in pgindent?

I've taken a stab at a pre_indent rule that seems to work well enough
(though the regex itself is write-once-read-never).

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

The rest of your second comments patch has been incorporated now, with
the exception of the following hunk:

- read($read_fh, $port, 7) // die "failed to read port number: $!";
+ read($read_fh, $port, 7) or die "failed to read port number: $!";

read() doesn't set $! unless it returns undef, according to the docs
[1]. A zero read (i.e. immediate EOF) will be handled further down.

> > To further pick away at this huge patch I propose to merge the SASL message
> > length hunk which can be extracted separately.

I've pulled this out into 0001.

Thanks,
--Jacob

[1] https://perldoc.perl.org/functions/read