Re: [PoC] Federated Authn/z with OAUTHBEARER

On Thu, Oct 31, 2024 at 4:05 AM Antonin Houska <ah(at)cybertec(dot)at> wrote:
> > > 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.)

Okay. I think this is going to be one of the slower authentication
methods by necessity: the builtin flow in libpq requires a human in
the loop, and an online validator is going to be making several HTTP
calls from the backend. So if it turns out later that we need to
optimize the backend logic, I'd prefer to have a case study in hand;
otherwise I think we're likely to optimize the wrong things.

> 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.

Oh, an assertion label! I can absolutely add one; I originally thought
you were proposing a label for the case itself.

> > > 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.

It might, if it ends up falling through to FE_OAUTH_REQUESTING_TOKEN.
There are two paths that can do that: the case where we have no
discovery URI, and the case where a custom user flow returns a token
synchronously (it was probably cached).

> 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.

Correct -- the only way for the current implementation of the
OAUTHBEARER mechanism to return SASL_ASYNC is during the very first
call. That's not an assumption I want to put into the higher levels,
though; I think Michael will be unhappy with me if I introduce
additional SASL coupling after the decoupling work that's been done
over the last few releases. :D

Thanks again,
--Jacob