Re: [PoC] Federated Authn/z with OAUTHBEARER

On Thu, Oct 17, 2024 at 10:51 PM Antonin Houska <ah@cybertec.at> wrote:
> This is the 1st round, based on reading the code. I'll continue paying
> attention to the project and possibly post some more comments in the future.

Thanks again for the reviews!

> * Information on the new method should be added to pg_hba.conf.sample.method.

Whoops, this will be fixed in v34.

> * Is it important that fe_oauth_state.token also contains the "Bearer"
>   keyword? I'd expect only the actual token value here. The keyword can be
>   added to the authentication message w/o storing it.
>
>   The same applies to the 'token' structure in fe-auth-oauth-curl.c.

Excellent question; I've waffled a bit on that myself. I think you're
probably right, but here's some background on why I originally made
that decision.

RFC 7628 defines not only OAUTHBEARER but also a generic template for
future OAuth-based SASL methods, and as part of that, the definition
of the "auth" key is incredibly vague:

      auth (REQUIRED):  The payload that would be in the HTTP
         Authorization header if this OAuth exchange was being carried
         out over HTTP.

I was worried that forcing a specific format would prevent future
extensibility, if say the Bearer scheme were updated to add additional
auth-params. I was also wondering if maybe a future specification
would allow OAUTHBEARER to carry a different scheme altogether, such
as DPoP [1].

However:
- auth-param support for Bearer was considered at the draft stage and
explicitly removed, with the old drafts stating "If additional
parameters are needed in the future, a different scheme would need to
be defined."
- I think the intent of RFC 7628 is that a new SASL mechanism will be
named for each new scheme (even if the new scheme shares all of the
bones of the old one). So DPoP tokens wouldn't piggyback on
OAUTHBEARER, and instead something like an OAUTHDPOP mech would need
to be defined.

So: the additional complexity in the current API is probably a YAGNI
violation, and I should just hardcode the Bearer format as you
suggest. Any future OAuth SASL mechanisms we support will have to go
through a different PQAUTHDATA type, e.g. PQAUTHDATA_OAUTH_DPOP_TOKEN.
And I'll need to make sure that I'm not improperly coupling the
concepts elsewhere in the API.

> * Does PQdefaultAuthDataHook() have to be declared extern and exported via
>   libpq/exports.txt ? Even if the user was interested in it, he can use
>   PQgetAuthDataHook() to get the pointer (unless he already installed his
>   custom hook).

I guess I don't have a strongly held opinion, but is there a good
reason not to? Exposing it means that a client application may answer
questions like "is the current hook set to the default?" and so on.
IME, hook-chain maintenance is not a lot of fun in general, and having
more visibility can be nice for third-party developers.

> * I wonder if the hooks (PQauthDataHook) can be implemented in a separate
>   diff. Couldn't the first version of the feature be commitable without these
>   hooks?

I am more than happy to split things up as needed! But in the end, I
think this is a question that can only be answered by the first brave
committer to take a bite. :)

(The original patchset didn't have these hooks; they were added as a
compromise, to prevent the builtin implementation from having to be
all things for all people.)

> * Instead of allocating an instance of PQoauthBearerRequest, assigning it to
>   fe_oauth_state.async_ctx, and eventually having to all its cleanup()
>   function, wouldn't it be simpler to embed PQoauthBearerRequest as a member
>   in fe_oauth_state ?

Hmm, that would maybe be simpler. But you'd still have to call
cleanup() and set the async_ctx, right? The primary gain would be in
reducing the number of malloc calls.

> * 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?

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

> * 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?

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

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

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.

> * Finally, the user documentation is almost missing. I say that just for the
>   sake of completeness, you obviously know it. (On the other hand, I think
>   that the lack of user information might discourage some people from running
>   the code and testing it.)

Yeah, the catch-22 of writing huge features... By the way, if anyone's
reading along and dissuaded by the lack of docs, please say so!
(Daniel has been helping me out so much with the docs; thanks again,
Daniel.)

--Jacob

[1] https://datatracker.ietf.org/doc/html/rfc9449