Re: [PoC] Federated Authn/z with OAUTHBEARER - Mailing list pgsql-hackers

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