Re: [PoC] Federated Authn/z with OAUTHBEARER - Mailing list pgsql-hackers

On Thu, Jan 2, 2025 at 2:11 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> There is a mix of using "URL" and "URI" throughout the patch.  I tried
> to look up in the source material (RFCs) what the correct use would
> be, but even they are mixing it in nonobvious ways.  Maybe this is
> just hopelessly confused, or maybe there is a system that I don't
> recognize.

Ugh, yeah. I think my "system" was whether the RFC I read most
recently had used "URL" or "URI" more in the text.

In an ideal world, I'd just switch to "URL" to avoid confusion. There
are some URNs in use as part of OAuth (e.g.
`urn:ietf:params:oauth:grant-type:device_code`) but I don't think I
refer to those as URIs anyway. And more esoteric forms of URI (data:)
are not allowed.

However... there are some phrases, like "Well-Known URI", where that's
just the Name of the Thing. Similarly, when the wire protocol itself
uses "URI" (say, in JSON field names), I'd rather be consistent to
make searching easier.

Is it enough to prefer "URL" in the user-facing documentation (at
least, when it doesn't conflict with other established naming
conventions), and accept both in the code?

> * src/interfaces/libpq/libpq-fe.h
>
> +#ifdef WIN32
> +#include <winsock2.h>          /* for SOCKET */
> +#endif
>
> Including a system header like that seems a bit unpleasant.  AFAICT,
> you only need it for this:
>
> +   PostgresPollingStatusType (*async) (PGconn *conn,
> +                                       struct _PGoauthBearerRequest
> *request,
> +                                       SOCKTYPE * altsock);
>
> But that function could already get the altsock handle via
> conn->altsock.  So maybe that is a way to avoid the whole socket type
> dance in this header.

It'd also couple clients against libpq-int.h, so they'd have to
remember to recompile every release. I'm worried that'd cause a bunch
of ABI problems...

I could cheat and use uintptr_t instead of SOCKET on Windows, but that
seems like it might bite us in Win32-adjacent environments? It seems
to pass Cirrus okay. Other ideas?

> * src/test/authentication/t/001_password.pl
>
> I suppose this file could be a separate commit?  It just separates the
> SASL/SCRAM terminology for existing functionality.

The scram-sha-256 duplication tests could, I suppose. But the only
reason that's interesting to test now is because of the change to the
internals. The "server requested SCRAM-SHA-256 authentication" error
message change comes in with the new require_auth handling, so that
should all land in the same patch.

Along those lines, though, Michael Paquier suggested that maybe I
could pull the require_auth prefactoring up to the front of the
patchset. That might look a bit odd until OAuth support lands, since
it won't be adding any new useful value, but I will give it a shot.

> * src/test/modules/oauth_validator/fail_validator.c
>
> +{
> +   elog(FATAL, "fail_validator: sentinel error");
> +   pg_unreachable();
> +}
>
> This pg_unreachable() is probably not necessary after elog(FATAL).

Cirrus completes successfully with that, but MSVC starts complaining:

    warning C4715: 'fail_token': not all control paths return a value

Is that expected?

--

All other suggestions will be addressed in the next patchset. Thanks!

--Jacob