Re: [PoC] Federated Authn/z with OAUTHBEARER - Mailing list pgsql-hackers
From | Jacob Champion |
---|---|
Subject | Re: [PoC] Federated Authn/z with OAUTHBEARER |
Date | |
Msg-id | CAOYmi+mJkc4mLD0+LtbZ1VxeFUSOQvXQ_HUx6eBBmHTXLuoK3g@mail.gmail.com Whole thread Raw |
In response to | Re: [PoC] Federated Authn/z with OAUTHBEARER (Antonin Houska <ah@cybertec.at>) |
List | pgsql-hackers |
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
pgsql-hackers by date: