Horton

Re: [PoC] Federated Authn/z with OAUTHBEARER

Jacob Champion <jacob.champion@enterprisedb.com>

View thread
From: Jacob Champion <jacob.champion@enterprisedb.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Shlok Kyal <shlok.kyal.oss@gmail.com>, mahendrakar s <mahendrakarforpg@gmail.com>, Andrey Chudnovsky <achudnovskij@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, "hlinnaka@iki.fi" <hlinnaka@iki.fi>, "michael@paquier.xyz" <michael@paquier.xyz>, "smilingsamay@gmail.com" <smilingsamay@gmail.com>, Stephen Frost <sfrost@snowman.net>, Andrew Dunstan <andrew@dunslane.net>, Peter Eisentraut <peter@eisentraut.org>
Date: 2024-07-10T00:05:18Z
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

Attachments

Hi Daniel,

On Mon, Apr 1, 2024 at 3:07 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Of the Cirrus machines, it looks like only FreeBSD has a new enough
> libcurl for that. Ubuntu won't until 24.04, Debian Bookworm doesn't
> have it unless you're running backports, RHEL 9 is still on 7.x... I
> think requiring libcurl 8 is effectively saying no one will be able to
> use this for a long time. Is there an alternative?

Since the exit() checks appear to be happy now that fe_memutils is
out, I've lowered the requirement to the version of libcurl that seems
to be shipped in RHEL 8 (7.61.0). This also happens to be when TLS 1.3
ciphersuite control was added to Curl, which seems like something we
may want in the very near future, so I'm taking that as a good sign
for what is otherwise a very arbitrary cutoff point. Counterproposals
welcome :D

> Good catch. application/json no longer defines charsets officially
> [1], so I think we should be able to just ignore them. The new
> strncasecmp needs to handle a spurious prefix, too; I have that on my
> TODO list.

I've expanded this handling in v24, attached.

> This new way doesn't do the same thing. Here's a sample error:
>
>     connection to server at "127.0.0.1", port 56619 failed: failed to
> fetch OpenID discovery document: Weird server reply (  Trying
> 127.0.0.1:36647...
>     Connected to localhost (127.0.0.1) port 36647 (#0)
>     Mark bundle as not supporting multiuse
>     HTTP 1.0, assume close after body
>     Invalid Content-Length: value
>     Closing connection 0
>     )
>
> IMO that's too much noise. Prior to the change, the same error would have been
>
>     connection to server at "127.0.0.1", port 56619 failed: failed to
> fetch OpenID discovery document: Weird server reply (Invalid
> Content-Length: value)

I have reverted this change for now, but I'm still hoping there's an
alternative that can help us clean up?

> `running` can be set to zero on success, too. I'm having trouble
> forcing that code path in a test so far, but we're going to have to do
> something special in that case.

For whatever reason, the magic timing for this is popping up more and
more often on Cirrus, leading to really annoying test failures. So I
may have to abandon the search for a perfect test case and just fix
it.

> > I did drop
> > the Python pytest patch since I feel that it's unlikely to go in from this
> > thread (adding pytest seems worthy of its own thread and discussion), and the
> > weight of it makes this seem scarier than it is.
>
> Until its coverage gets ported over, can we keep it as a `DO NOT
> MERGE` patch? Otherwise there's not much to run in Cirrus.

I have added this back (marked loudly as don't-merge) so that we keep
the test coverage for now. The Perl suite (plus Python server) has
been tricked out a lot more in v24, so it shouldn't be too bad to get
things ported.

> > Next I intend to work on writing documentation for this.
>
> Awesome, thank you! I will start adding coverage to the new code paths.

Peter E asked for some documentation stubs to ease review, which I've
added. Hopefully that doesn't step on your toes any.

A large portion of your "Review comments" patch has been pulled
backwards into the previous commits; the remaining pieces are things
I'm still peering at and/or writing tests for. I also owe this thread
an updated roadmap and summary, to make it a little less daunting for
new reviewers. Soon (tm).

Thanks!
--Jacob