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: <CAOYmi+mJkc4mLD0+LtbZ1VxeFUSOQvXQ_HUx6eBBmHTXLuoK3g@mail.gmail.com>
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> <2453.1729230681@antos> <CAOYmi+mJkc4mLD0+LtbZ1VxeFUSOQvXQ_HUx6eBBmHTXLuoK3g@mail.gmail.com>
Comments: In-reply-to Jacob Champion <jacob.champion@enterprisedb.com>
   message dated "Wed, 23 Oct 2024 15:40:57 -0700."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Thu, 31 Oct 2024 12:05:46 +0100
Message-ID: <4136.1730372746@antos>
Archived-At: <https://www.postgresql.org/message-id/4136.1730372746%40antos>
Precedence: bulk

Jacob Champion <jacob.champion@enterprisedb.com> wrote:

> On Thu, Oct 17, 2024 at 10:51=E2=80=AFPM Antonin Houska <ah@cybertec.at> =
wrote:
> > * oauth_validator_library is defined as PGC_SIGHUP - is that intentiona=
l?
>=20
> 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?

No concern. I was just trying to imagine when the module needs to be change=
d.

> >   And regardless, the library appears to be loaded by every backend dur=
ing
> >   authentication. Why isn't it loaded by postmaster like libraries list=
ed in
> >   shared_preload_libraries? fork() would then ensure that the backends =
do have
> >   the library in their address space.
>=20
> 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.

Loading the library by postmaster does not prevent the backends from reload=
ing
it on SIGHUP later. I was simply concerned about performance. (I proposed
loading the library at another stage of backend initialization rather than
adding _PG_init() to it.)

> > * 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 reques=
t must
> >   always fail. It seems more logical if the prompt is set to the user b=
efore
> >   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.)
>=20
> 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.
>=20
> There's a comment immediately below the part you quoted that mentions
> this briefly; maybe I should move it up a bit?

That's fine, I understand now.

> > * As long as I understand, the following comment would make sense:
> >
> > diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libp=
q/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 =3D=3D -1);
> >
> >                         if (!derive_discovery_uri(conn))
>=20
> 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.

Easiness of reading is the only "question" here :-) It's might not always be
obvious why a variable should have some particular value. In general, the
Assert() statements are almost always preceded with a comment in the PG
source.

> >   Or, doesn't the FE_OAUTH_INIT branch of the switch statement actually=
 fit
> >   better into oauth_init()?
>=20
> 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.

Sure. The FE_OAUTH_INIT branch in oauth_exchange() (FE) also does not gener=
ate
the initial client response.

Based on reading the SCRAM implementation, I concluded that the init()
callback can do authentication method specific things, but unlike exchange()
it does not generate any output.

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

I didn't want to skip the flow. I thought that the init() callback could be
made responsible for getting the token, but forgot that it still needs some
way to signal to the caller that the async flow is needed.

Anyway, are you sure that pg_SASL_continue() can also receive the SASL_ASYNC
value from oauth_exchange()? My understanding is that pg_SASL_init() receiv=
es
it if there is no token, but after that, oauth_exchange() is not called util
the token is available, and thus it should not return SASL_ASYNC anymore.

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