Horton

Re: [PoC] Federated Authn/z with OAUTHBEARER

Jacob Champion <jacob.champion@enterprisedb.com>

View thread
From: Jacob Champion <jacob.champion@enterprisedb.com>
To: Peter Eisentraut <peter@eisentraut.org>
Cc: Daniel Gustafsson <daniel@yesql.se>, Antonin Houska <ah@cybertec.at>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2025-01-07T22:24:39Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. meson: Fix install-quiet after clean

  2. oauth: Run Autoconf tests with correct compiler flags

  3. Link libpq with libdl if the platform needs that.

  4. Doc: correct spelling of meson switch.

  5. oauth: Correct SSL dependency for libpq-oauth.a

  6. oauth: Fix Autoconf build on macOS

  7. oauth: Move the builtin flow into a separate module

  8. Remove a stray "pgrminclude" annotation

  9. oauth: Simplify copy of PGoauthBearerRequest

  10. oauth: Improve validator docs on interruptibility

  11. oauth: Disallow synchronous DNS in libcurl

  12. oauth: Fix postcondition for set_timer on macOS

  13. oauth: Use IPv4-only issuer in oauth_validator tests

  14. Work around OAuth/EVFILT_TIMER quirk on NetBSD.

  15. oauth: Fix incorrect const markers in struct

  16. Add missing entry to oauth_validator test .gitignore

  17. cirrus: Temporarily fix libcurl link error

  18. Add support for OAUTHBEARER SASL mechanism

  19. libpq: Handle asynchronous actions during SASL

  20. require_auth: prepare for multiple SASL mechanisms

  21. Move PG_MAX_AUTH_TOKEN_LENGTH to libpq/auth.h

  22. Make SASL max message length configurable

  23. jsonapi: fully initialize dummy lexer

  24. common/jsonapi: support libpq as a client

  25. Remove fe_memutils from libpgcommon_shlib

  26. Revert ECPG's use of pnstrdup()

  27. Explicitly require password for SCRAM exchange

  28. Refactor SASL exchange to return tri-state status

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