From: Antonin Houska <ah@cybertec.at>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: Daniel Gustafsson <daniel@yesql.se>,
    Peter Eisentraut <peter@eisentraut.org>,
    PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
In-reply-to: <63648.1728384409@antos>
References: <d1b467a78e0e36ed85a09adf979d04cf124a9d4b.camel@vmware.com>
 <e44faf68-6538-41b9-84d2-b259294e5c28@eisentraut.org> <CAOYmi+myshCL_yaWQiu54Kj5in93D5nPyw7yXj2jZnDKi73SHQ@mail.gmail.com>
 <f9e49a45-ed19-412f-8bd4-f956e7a35e66@eisentraut.org> <CAOYmi+nHJgPAYHRUhvh=9rfqnkphRHObKjbpjYyZWi9jkNic3w@mail.gmail.com>
 <fa1c769b-42aa-46c2-ba0c-aa21f3eeb0a1@eisentraut.org> <CAOYmi+=pg=W5L1h=3MEP_EB24jaBu2FyATrLXqQHGe7cpuvwyg@mail.gmail.com>
 <3603e9bb-9a69-4761-869b-f5b1f204b840@eisentraut.org> <e4b1456c-318d-472c-8713-dcfac1ad6a29@eisentraut.org>
 <CAOYmi+n_Pu5X=zC-MhAmW_zV=rXH8Zzz1gXHdjbLf7YeGNDWBw@mail.gmail.com>
 <c7046d57-978d-4ad8-b130-070f38909ea1@eisentraut.org> <CAOYmi+n9qa3+fM0u5+=9vvr+TirM54Y8NMDUpPBn-8xLGHcFgw@mail.gmail.com>
 <CAOYmi+nwexxyLWxFFV1_owEY4DOOQL2dM31L98pNvg_8M7zAig@mail.gmail.com>
 <a12b74b7-9ef9-4720-8f73-5922b9122316@eisentraut.org> <CAOYmi+k6oSACjoFvCUPipeLJBOrH=OgaeY9BeN1Mpsjna8taZg@mail.gmail.com>
 <390f9699-6275-4e1e-a600-11452bee0576@eisentraut.org> <67b7e52e-0d6e-4eb5-9318-fdcfa4e7ea51@eisentraut.org>
 <20A6A78C-D5B1-4BA3-8CD0-99AF18A04B48@yesql.se> <CAOYmi+m9uPWsqz_ZrbMPos7YDrwZqu7WgjiGG3q54oUa8XuX_g@mail.gmail.com>
 <4812.1727459899@antos> <CAOYmi+=MFyrjDps-YNtem3=Gr3mUsgZ49m7bfMCgr1TDjHL58g@mail.gmail.com>
 <4503.1727703521@antos> <CAOYmi+=8rkKh_8o9iyGQk_J4MQRCfpq3Qj3-dDyrnJPQ96bHYQ@mail.gmail.com>
 <63648.1728384409@antos>
Comments: In-reply-to Antonin Houska <ah@cybertec.at>
   message dated "Tue, 08 Oct 2024 12:46:49 +0200."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2452.1729230680.1@antos>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 18 Oct 2024 07:51:21 +0200
Message-ID: <2453.1729230681@antos>
Archived-At: <https://www.postgresql.org/message-id/2453.1729230681%40antos>
Precedence: bulk

Antonin Houska <ah@cybertec.at> wrote:

> I'd like to play with the code a bit and provide some review before or d=
uring
> 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 futur=
e.


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


* 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 th=
ese
  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 memb=
er
  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 m=
ust
  always fail. It seems more logical if the prompt is set to the user befo=
re
  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/f=
e-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 =3D=3D -1);
 =

                        if (!derive_discovery_uri(conn))

  Or, doesn't the FE_OAUTH_INIT branch of the switch statement actually fi=
t
  better into oauth_init()? A side-effect of that might be (I only judge f=
rom
  reading the code, haven't tried to implement this suggestion) that
  oauth_exchange() would no longer return the SASL_ASYNC status. Furthermo=
re,
  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 m=
ake
  the SASL_ASYNC state unnecessary.


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

-- =

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