Re: [PoC] Federated Authn/z with OAUTHBEARER

Antonin Houska <ah(at)cybertec(dot)at> wrote:

> I'd like to play with the code a bit and provide some review before or during
> the next CF. That will probably generate some more questions.

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.

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

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

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

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

* oauth_validator_library is defined as PGC_SIGHUP - is that intentional?

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.

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

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

Or, doesn't the FE_OAUTH_INIT branch of the switch statement actually fit
better into oauth_init()? A side-effect of that might be (I only judge from
reading the code, haven't tried to implement this suggestion) that
oauth_exchange() would no longer return the SASL_ASYNC status. Furthermore,
I'm not sure if pg_SASL_continue() can receive the SASL_ASYNC at all. So I
wonder if moving that part from oauth_exchange() to oauth_init() would make
the SASL_ASYNC state unnecessary.

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

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