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: 2024-11-12T21:47:07Z
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 Fri, Nov 8, 2024 at 1:21 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> Assorted review comments from me:

Thank you! I will cherry-pick some responses here and plan to address
the rest in a future patchset.

> trust_validator_authz: Personally, I'm not a fan of the "authz" and
> "authn" abbreviations.  I know this is security jargon.  But are
> regular users going to understand this?  Can we just spell it out?

Yes. That name's a holdover from the very first draft, actually.

Is "trust_validator_authorization" a great name in the first place?
The key concept is that user mapping is being delegated to the OAuth
system itself, so you'd better make sure that the validator has been
built to do that. (Anyone have any suggestions?)

> I find the way the installation options are structured a bit odd.  I
> would have expected --with-libcurl and -Dlibcurl (or --with-curl and
> -Dcurl).  These build options usually just say, use this library.

It's patterned directly off of -Dssl/--with-ssl (which I liberally
borrowed from) because the builtin client implementation used to have
multiple options for the library in use. I can change it if needed,
but I thought it'd be helpful for future devs if I didn't undo the
generalization.

> Maybe oauth_issuer should be oauth_issuer_url?  Otherwise one might
> expect to just write "google" here or something.  Or there might be
> other ways to contact an issuer in the future?  Just a thought.

More specifically this is an "issuer identifier", as defined by the
OAuth/OpenID discovery specs. It's a subset of a URL, and I want to
make sure users know how to differentiate between an "issuer" they
trust and the "discovery URI" that's in use for that issuer. They may
want to set one or the other -- a discovery URI is associated with
exactly one issuer, but unfortunately an issuer may have multiple
discovery URIs, which I'm actively working on. (There is also some
relation to the multiple-issuers problem mentioned below.)

> I'm confused by the use of PG_MAX_AUTH_TOKEN_LENGTH in the
> pg_be_oauth_mech definition.  What does that mean?

Just that Bearer tokens can be pretty long, so we don't want to limit
them to 1k like SCRAM does. 64k is probably overkill, but I've seen
anecdotal reports of tens of KBs and it seemed reasonable to match
what we're doing for GSS tokens.

> Also, shouldn't [oauth_validator_library] be an hba option instead?  What if you want to
> use different validators for different connections?

Yes. This is again the multiple-issuers problem; I will split that off
into its own email since this one's getting long. It has security
implications.

> The CURL_IGNORE_DEPRECATION thing needs clarification.  Is that in
> progress?

Thanks for the nudge, I've started a thread:

    https://curl.se/mail/lib-2024-11/0028.html

> This is an anonymous union, which requires C11.  Strangely, I cannot
> get clang to warn about this with -Wc11-extensions.  Probably better
> to fix anyway.  (The trailing supported MSVC versions don't support
> C11 yet.)

Oh, that's not going to be fun.

> This is only used once, in append_urlencoded(), and there are other
> ways to communicate errors, for example returning a bool.

I'd rather not introduce two parallel error indicators for the caller
to have to check for that particular part. But I can change over to
using the (identical!) termPQExpBuffer. I felt like the other API
signaled the intent a little better, though.

> On Cirrus CI Windows task, this test reports SKIP.  Can't tell why,
> because the log is not kept.  I suppose you expect this to work on
> Windows (but see my comment below)

No, builtin client support does not exist on Windows. If/when it's
added, the 001_server tests will need to be ported.

> +my $issuer = "https://127.0.0.1:54321";
>
> Use PostgreSQL::Test::Cluster::get_free_port() instead of hardcoding
> port numbers.
>
> Or is this a real port?  I don't see it used anywhere else.

It's not real; 002_client.pl doesn't start an authorization server at
all. I can make that more explicit.

> src/test/perl/PostgreSQL/Test/OAuthServer.pm: Return value of flagged
> function ignored - read at line 39, column 2.

So perlcritic recognizes "or" but not the "//" operator... Lovely.

Thanks!
--Jacob