Horton

Re: [PoC] Federated Authn/z with OAUTHBEARER

Jacob Champion <jchampion@timescale.com>

View thread
From: Jacob Champion <jchampion@timescale.com>
To: Andrey Chudnovsky <achudnovskij@gmail.com>
Cc: Thomas Munro <thomas.munro@gmail.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, mahendrakar s <mahendrakarforpg@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>
Date: 2023-07-10T23:21:58Z
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, Jul 7, 2023 at 2:16 PM Andrey Chudnovsky <achudnovskij@gmail.com> wrote:
> Please confirm my understanding of the flow is correct:
> 1. Client calls PQconnectStart.
>   - The client doesn't know yet what is the issuer and the scope.

Right. (Strictly speaking it doesn't even know that OAuth will be used
for the connection, yet, though at some point we'll be able to force
the issue with e.g. `require_auth=oauth`. That's not currently
implemented.)

>   - Parameters are strings, so callback is not provided yet.
> 2. Client gets PgConn from PQconnectStart return value and updates
> conn->async_auth to its own callback.

This is where some sort of official authn callback registration (see
above reply to Daniele) would probably come in handy.

> 3. Client polls PQconnectPoll and checks conn->sasl_state until the
> value is SASL_ASYNC

In my head, the client's custom callback would always be invoked
during the call to PQconnectPoll, rather than making the client do
work in between calls. That way, a client can use custom flows even
with a synchronous PQconnectdb().

> 4. Client accesses conn->oauth_issuer and conn->oauth_scope and uses
> those info to trigger the token flow.

Right.

> 5. Expectations on async_auth:
>     a. It returns PGRES_POLLING_READING while token acquisition is going on
>     b. It returns PGRES_POLLING_OK and sets conn->sasl_state->token
> when token acquisition succeeds.

Yes. Though the token should probably be returned through some
explicit part of the callback, now that you mention it...

> 6. Is the client supposed to do anything with the altsock parameter?

The callback needs to set the altsock up with a select()able
descriptor, which wakes up the client when more work is ready to be
done. Without that, you can't handle multiple connections on a single
thread.

> If yes, it looks workable with a couple of improvements I think would be nice:
> 1. Currently, oauth_exchange function sets conn->async_auth =
> pg_fe_run_oauth_flow and starts Device Code flow automatically when
> receiving challenge and metadata from the server.
>     There probably should be a way for the client to prevent default
> Device Code flow from triggering.

Agreed. I'd like the client to be able to override this directly.

> 2. The current signature and expectations from async_auth function
> seems to be tightly coupled with the internal implementation:
>     - Pieces of information need to be picked and updated in different
> places in the PgConn structure.
>     - Function is expected to return PostgresPollingStatusType which
> is used to communicate internal state to the client.
>    Would it make sense to separate the internal callback used to
> communicate with Device Code flow from client facing API?
>    I.e. introduce a new client facing structure and enum to facilitate
> callback and its return value.

Yep, exactly right! I just wanted to check that the architecture
*looked* sufficient before pulling it up into an API.

> On a separate note:
> The backend code currently spawns an external command for token validation.
> As we discussed before, an extension hook would be a more efficient
> extensibility option.
> We see clients make 10k+ connections using OAuth tokens per minute to
> our service, and stating external processes would be too much overhead
> here.

+1. I'm curious, though -- what language do you expect to use to write
a production validator hook? Surely not low-level C...?

> > 5) Does this maintenance tradeoff (full control over the client vs. a
> > large amount of RFC-governed code) seem like it could be okay?
>
> It's nice for psql to have Device Code flow. Can be made even more
> convenient with refresh tokens support.
> And for clients on resource constrained devices to be able to
> authenticate with Client Credentials (app secret) without bringing
> more dependencies.
>
> In most other cases, upstream PostgreSQL drivers written in higher
> level languages have libraries / abstractions to implement OAUTH flows
> for the platforms they support.

Yeah, I'm really interested in seeing which existing high-level flows
can be mixed in through a driver. Trying not to get too far ahead of
myself :D

Thanks for the review!

--Jacob