Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3776.700.51.11.1\))
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <CAOYmi+nHG7oy+ybHH72WjiXAQG3tE6v_at-K9ebRy2oqo92V+A@mail.gmail.com>
Date: Fri, 7 Feb 2025 21:12:34 +0100
Cc: Peter Eisentraut <peter@eisentraut.org>,
 Antonin Houska <ah@cybertec.at>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <C35FE624-DEF8-4359-9C7E-ED974A10CC29@yesql.se>
References: <d1b467a78e0e36ed85a09adf979d04cf124a9d4b.camel@vmware.com>
 <CAOYmi+kHXMG2Z2KR7y36Cbsop_7t-YugrpfCnOXuXz8BpfaXaw@mail.gmail.com>
 <6bde5f56-9e7a-4148-b81c-eb6532cb3651@eisentraut.org>
 <CAOYmi+=ee8H=GaGNH_gZvBE+YPtmK9eqUdXqemk=zKDnO8YXRw@mail.gmail.com>
 <CAOYmi+kLTJ1wZ6gxRbgtR52E=EyiCpmp6J3mmSvtc1a6i7sZ3Q@mail.gmail.com>
 <CAOYmi+=ceXepXOrQXsxca-u57GxM+x_q34fpyNJ8PLmbUWPwEQ@mail.gmail.com>
 <9B417838-B872-453D-BBFB-99FA957EB723@yesql.se>
 <CAOYmi+=vJQ8EUiVf-iHYBwz4-tLHLrvp8rYDoNicB2yJrBKraw@mail.gmail.com>
 <CAOYmi+=FzVg+C-pQHCwjW0qU-POHmzZaD2z3CdsACj==14H8kQ@mail.gmail.com>
 <b09bc106-32f2-4e71-8c35-4db8608776a1@eisentraut.org>
 <CAOYmi+kjPLfT7iFqNMVV44sAUJ9m10TfrSMRPUYU2OgH5j0AYg@mail.gmail.com>
 <effc7572-c7ce-4ff2-9066-91d084401e1c@eisentraut.org>
 <CAOYmi+kn0_FioLpDqctn97a_W8n90XLt_OmU1xL6ZdK8DP8ufA@mail.gmail.com>
 <54c008ad-1c23-47d8-ba6c-bed98a39c31c@eisentraut.org>
 <CAOYmi+mEkXu9Jfg9v-5z9DmBCRCMS2dt43Z=TCbgVkbrqJXxRg@mail.gmail.com>
 <06102deb-00ef-431d-a3ea-65afb246700a@eisentraut.org>
 <CAOYmi+kZAka0sdxCOBxsQc2ozEZGZKHWU_9nrPXg3sG1NJ-zJw@mail.gmail.com>
 <CE4D545A-7583-4FA9-98DD-57388CCC006F@yesql.se>
 <CAOYmi+kQFrKGmC0=Um-QryU1fvCAOC2bpjcz-4rxgLrp69mSOg@mail.gmail.com>
 <39A09EC8-2D72-4130-8C3E-8F3D1CC69A12@yesql.se>
 <CAOYmi+=K3BBRXwvJpt5D_4nK_EimCJBj7a42maACszsQC3=t9w@mail.gmail.com>
 <55EA9E76-9866-486C-9C7F-C5E8B071C50E@yesql.se>
 <CAOYmi+kJqzo6XsR9TEhvVfeVNQ-TyFM5LATypm9yoQVYk=4Wrw@mail.gmail.com>
 <35CBB6AD-B847-4C26-9A7B-019279F280C8@yesql.se>
 <9F184CFE-ED8D-4B86-9529-F07821108C93@yesql.se>
 <CAOYmi+nHG7oy+ybHH72WjiXAQG3tE6v_at-K9ebRy2oqo92V+A@mail.gmail.com>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Archived-At: <https://www.postgresql.org/message-id/C35FE624-DEF8-4359-9C7E-ED974A10CC29%40yesql.se>
Precedence: bulk

> On 7 Feb 2025, at 06:48, Jacob Champion =
<jacob.champion@enterprisedb.com> wrote:
>=20
> On Thu, Feb 6, 2025 at 2:02=E2=80=AFPM Daniel Gustafsson =
<daniel@yesql.se> wrote:
>> Attached is a v46 which is v45 minus the now committed patch.
>=20
> Thank you! Attached is v47, which creeps ever closer to the finish =
line.

Great, thanks!  Below are a few quick comments after a first =
read-through and
compile/test cycle:

> - 0005 warns at configure time if libcurl doesn't have a nonblocking
> DNS implementation.

Is it really enough to do this at build time?  A very small percentage =
of users
running this will also be building their own libpq so the warning is =
lost on
them.  That being said, I'm not entirely sure what else we could do =
(bleeping a
warning every time is clearly not userfriendly) so maybe this is a TODO =
in the
code?

> - 0006 augments bare Asserts during client-side JSON parsing with code
> that will fail gracefully in production builds as well.

+  oauth_json_set_error(ctx,       /* don't bother translating */
With the project style format for translator comments this should be:

	+  /* translator: xxx */
	+  oauth_json_set_error(ctx,

> - 0007 escapes binary data during the printing of libcurl debug
> output. (If you're having a bad enough day to need the debug spray,
> you're probably not in the mood for the sound of a hundred BELs.)

Does it make sense to prefix the printing in debug_callback() with some =
header
stating that the following data is debug output from curl and not =
postgres?  I
have a feeling I'm stockholm syndromed by knowing the internals so I'm =
not sure
if that would be helpful to someone not knowing the implementation =
details.

> - 0008 parses and passes through the expires_in and optional
> verification_uri_complete fields from the device endpoint to any
> custom user prompt. (We do not use them ourselves, at the moment. But
> after seeing some nice demos of RHEL/PAM/sssd support for device flow
> QR codes at FOSDEM, I think we're definitely going to want to make
> those available to devs.)

Aha, cool!  I was a bit surprised to not find a definition of expires_in =
in RFC
8628, as in what happens if -1 is passed?  8628 seems to broadly =
speaking fall
into the category of "just dont do the wrong thing and all will be fine" =
=3D/.
Another question that comes to mind is how the reciever should interpret =
the
information since it doesn't know when the device_code/user_code was =
generated
so it doesn't know how much of expires_in which has already passed.  =
(Which is
not something for us to solve, just a general observation.)

+   even if they can't use the non-textual method. Review the RFC's
+   <ulink =
url=3D"https://datatracker.ietf.org/doc/html/rfc8628#section-3.3.1">notes
+   on non-textual verification</ulink>.
To align more with the rest of the documentation I think something along =
these
lines is better: "For more information, see section 3.3.1 in <ulink =
..>RFC
8628</ulink>.

      Assert(cnt =3D=3D 1);
-     return 1;                       /* don't fall through in release =
builds */
+     return 0;
While not introduced in this fixup patch, reading it again now I sort of =
think
we should make that Assert(false) to clearly indicate that we don't =
think the
assertion will ever pass, we're just asking to error out since we =
already know
the failure condition holds.

> - 0009 is gold-plating for the OAUTH_STEP_WAIT_INTERVAL state:

+  actx_error(actx, "failed to create timer kqueue: %m");
Maybe we should add a translator note explaining that kqueue should not =
be
translated since it's very easy to mistake it for "queue".  Doing it on =
the
first string including kqueue should be enough I suppose.

--
Daniel Gustafsson