Horton

Re: [PoC] Federated Authn/z with OAUTHBEARER

Antonin Houska <ah@cybertec.at>

View thread
From: Antonin Houska <ah@cybertec.at>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: Daniel Gustafsson <daniel@yesql.se>, Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2024-10-31T11:05:46Z
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

Jacob Champion <jacob.champion@enterprisedb.com> wrote:

> On Thu, Oct 17, 2024 at 10:51 PM Antonin Houska <ah@cybertec.at> wrote:
> > * oauth_validator_library is defined as PGC_SIGHUP - is that intentional?
> 
> Yes, I think it's going to be important to let DBAs migrate their
> authentication modules without a full restart. That probably deserves
> more explicit testing, now that you mention it. Is there a specific
> concern that you have with that?

No concern. I was just trying to imagine when the module needs to be changed.

> >   And regardless, the library appears to be loaded by every backend during
> >   authentication. Why isn't it loaded by postmaster like libraries listed in
> >   shared_preload_libraries? fork() would then ensure that the backends do have
> >   the library in their address space.
> 
> It _can_ be, if you want -- there's nothing that I know of preventing
> the validator from also being preloaded with its own _PG_init(), is
> there? But I don't think it's a good idea to force that, for the same
> reason we want to allow SIGHUP.

Loading the library by postmaster does not prevent the backends from reloading
it on SIGHUP later. I was simply concerned about performance. (I proposed
loading the library at another stage of backend initialization rather than
adding _PG_init() to it.)

> > * pg_fe_run_oauth_flow()
> >
> >   When first time here
> >                         case OAUTH_STEP_TOKEN_REQUEST:
> >                                 if (!handle_token_response(actx, &state->token))
> >                                         goto error_return;
> >
> >   the user hasn't been prompted yet so ISTM that the first token request must
> >   always fail. It seems more logical if the prompt is set to the user before
> >   sending the token request to the server. (Although the user probably won't
> >   be that fast to make the first request succeed, so consider this just a
> >   hint.)
> 
> That's also intentional -- if the first token response fails for a
> reason _other_ than "we're waiting for the user", then we want to
> immediately fail hard instead of making them dig out their phone and
> go on a two-minute trip, because they're going to come back and find
> that it was all for nothing.
> 
> There's a comment immediately below the part you quoted that mentions
> this briefly; maybe I should move it up a bit?

That's fine, I understand now.

> > * As long as I understand, the following comment would make sense:
> >
> > diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c
> > index f943a31cc08..97259fb5654 100644
> > --- a/src/interfaces/libpq/fe-auth-oauth.c
> > +++ b/src/interfaces/libpq/fe-auth-oauth.c
> > @@ -518,6 +518,7 @@ oauth_exchange(void *opaq, bool final,
> >         switch (state->state)
> >         {
> >                 case FE_OAUTH_INIT:
> > +                       /* Initial Client Response */
> >                         Assert(inputlen == -1);
> >
> >                         if (!derive_discovery_uri(conn))
> 
> There are multiple "initial client response" cases, though. What
> questions are you hoping to clarify with the comment? Maybe we can
> find a more direct answer.

Easiness of reading is the only "question" here :-) It's might not always be
obvious why a variable should have some particular value. In general, the
Assert() statements are almost always preceded with a comment in the PG
source.

> >   Or, doesn't the FE_OAUTH_INIT branch of the switch statement actually fit
> >   better into oauth_init()?
> 
> oauth_init() is the mechanism initialization for the SASL framework
> itself, which is shared with SCRAM. In the current architecture, the
> init callback doesn't take the initial client response into
> consideration at all.

Sure. The FE_OAUTH_INIT branch in oauth_exchange() (FE) also does not generate
the initial client response.

Based on reading the SCRAM implementation, I concluded that the init()
callback can do authentication method specific things, but unlike exchange()
it does not generate any output.

> Generating the client response is up to the exchange callback -- and
> even if we moved the SASL_ASYNC processing elsewhere, I don't think we
> can get rid of its added complexity. Something has to signal upwards
> that it's time to transfer control to an async engine. And we can't
> make the asynchronicity a static attribute of the mechanism itself,
> because we can skip the flow if something gives us a cached token.

I didn't want to skip the flow. I thought that the init() callback could be
made responsible for getting the token, but forgot that it still needs some
way to signal to the caller that the async flow is needed.

Anyway, are you sure that pg_SASL_continue() can also receive the SASL_ASYNC
value from oauth_exchange()? My understanding is that pg_SASL_init() receives
it if there is no token, but after that, oauth_exchange() is not called util
the token is available, and thus it should not return SASL_ASYNC anymore.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com