From: Tom Lane <tgl@sss.pgh.pa.us>
To: Daniel Gustafsson <daniel@yesql.se>
cc: Jacob Champion <jacob.champion@enterprisedb.com>,
        Peter Eisentraut <peter@eisentraut.org>,
        Antonin Houska <ah@cybertec.at>,
        PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
In-reply-to: <13F329B6-86BC-40A5-96F4-102784A0357A@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-65afb24!
 6700a@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> <C35FE624-DEF8-4359-9C7E-ED974A10CC29@yesql.se> <CAOYmi+mFZsExAYWy-bS9=yki_fWC+MC0MufpEZ_SycrbCAJrEg@mail.gmail.com> <83C44AB4-24B0-437F-B139-B5CBC5821BB1@yesql.se> <CAOYmi+=zgJinCoqdQ70fvKmvyrLekd9-supkuj57Poa1FmX8bw@mail.gmail.com> <2A1511A0-C04B-47E4-B1C3-54C2A1C765B8@yesql.se> <CAOYmi+=1_AqPL0Y7qdqg1ER0-bBjCUKUZfrLcgob5bHjzSsvbA@mail.gmail.com> <CAOYmi+nP8AM9xm+xUW5kDz7XDF7M!
 KBjuDnQ4LjMEm07tpwDgrg@mail.gmail.com> <AACF415F-894A-434F-9FE2-272ED8FA4981@yesql.se> <13F329B6-86BC-40A5-96F4-102784A0357A@yesql.se>
Comments: In-reply-to Daniel Gustafsson <daniel@yesql.se>
	message dated "Thu, 20 Feb 2025 17:29:58 +0100"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <912515.1740329361.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sun, 23 Feb 2025 11:49:21 -0500
Message-ID: <912516.1740329361@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/912516.1740329361%40sss.pgh.pa.us>
Precedence: bulk

Daniel Gustafsson <daniel@yesql.se> writes:
> I spent a few more hours staring at this, and ran it through a number of=
 CI and
> local builds, without anything showing up.  Pushed to master with the fi=
rst set
> of buildfarm animals showing green builds.

Coverity has a nit-pick about this:

/srv/coverity/git/pgsql-git/postgresql/src/interfaces/libpq/fe-auth-oauth.=
c: 784 in setup_token_request()
778     		if (!request_copy)
779     		{
780     			libpq_append_conn_error(conn, "out of memory");
781     			goto fail;
782     		}
783     =

>>>     CID 1643156:  High impact quality  (WRITE_CONST_FIELD)
>>>     A write to an aggregate overwrites a const-qualified field within =
the aggregate.
784     		memcpy(request_copy, &request, sizeof(request));
785     =

786     		conn->async_auth =3D run_user_oauth_flow;
787     		conn->cleanup_async_auth =3D cleanup_user_oauth_flow;
788     		state->async_ctx =3D request_copy;
789     	}

This is evidently because of the fields declared const:

	/* Hook inputs (constant across all calls) */
	const char *const openid_configuration; /* OIDC discovery URI */
	const char *const scope;	/* required scope(s), or NULL */

IMO, the set of cases where it's legitimate to mark individual struct
fields as const is negligibly small, and this doesn't seem to be one
of them.  It's not obvious to me where/how PGoauthBearerRequest
structs are supposed to be constructed, but I find it hard to believe
that they will all spring full-grown from the forehead of Zeus.
Nonetheless, this declaration requires exactly that.

(I'm kind of surprised that we're not getting similar bleats from
any buildfarm animals, but so far I don't see any.)

BTW, as another nitpicky style matter: why do PGoauthBearerRequest
etc. spell their struct tag names differently from their typedef names
(that is, with/without an underscore)?  That is not our project style
anywhere else, and I'm failing to detect a good reason to do it here.

			regards, tom lane