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

Antonin Houska <ah@cybertec.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