| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> |
| Cc: | "Jonathan Gonzalez V(dot)" <jonathan(dot)abdiel(at)gmail(dot)com>, Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: [oauth] Stabilize the libpq-oauth ABI (and allow alternative implementations?) |
| Date: | 2026-03-16 07:33:10 |
| Message-ID: | 821576A8-5958-401D-B8D1-7E4E30F5A40F@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Mar 14, 2026, at 01:59, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> wrote:
>
> On Fri, Mar 13, 2026 at 12:24 AM Jonathan Gonzalez V.
> <jonathan(dot)abdiel(at)gmail(dot)com> wrote:
>> While rebasing this patch[1] I notice that the test where wailing, that
>> was due to the following missing dependency in the test, small patch
>> here:
>
> Fixed in v8, thanks.
>
> v7-0001 and -0002 are committed, plus the removal of some additional
> typedefs, plus a fix for a silly bug in the Makefile that indri
> caught.
>
> --Jacob
> <since-v7.diff.txt><v8-0001-libpq-Poison-the-v2-part-of-a-v1-Bearer-request.patch><v8-0002-libpq-Allow-developers-to-reimplement-libpq-oauth.patch><v8-0003-WIP-Introduce-third-party-OAuth-flow-plugins.patch>
Hi Jacob, a few comments on v8.
1 - 0001
```
+ /*
+ * For uninstrumented builds, make sure request->error wasn't touched.
+ */
+ if (request->error)
+ {
+ fprintf(stderr,
+ "abort! out-of-bounds write to PGoauthBearerRequest by PQAUTHDATA_OAUTH_BEARER_TOKEN hook\n");
+ abort();
+ }
```
I think this check relies on that when poisoning, request->error should be NULL. So, does it make sense to Assert(request->error==NULL) in the poison branch?
2 - 0002 Overall LGTM. A small comment is that, now use_builtin_flow() becomes a bit misleading because it may turn to non-builtin when libpq_oauth_init is not provided by the lib. So, maybe rename the function to something like try_builtin_flow()?
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Hüseyin Demir | 2026-03-16 07:44:44 | Re: Improve checks for GUC recovery_target_xid |
| Previous Message | Zsolt Parragi | 2026-03-16 07:32:53 | Re: Improve OAuth discovery logging |