Обсуждение: [oauth] Split and extend PGOAUTHDEBUG

Hello!

I'm proposing 2 patches:

1 is the same patch I already sent as part of the PGOAUTHCAFILE
discussion[1], rebased on the current master: it splits
PGOAUTHDEBUG=UNSAFE into separate unsafe/safe settings which users can
toggle one by one.

2 is a new unsafe setting issuer-mismatch, which allows a connection
to continue if the client and server issuers don't match. While this
isn't useful for end users, it makes testing validators easier, as
validators authors should be able to verify that mismatched
configurations are rejected properly by the validator.

I based 2 on 1 because unconditionally adding this new unsafe option
would conflict with some tests. This way that test can use a limited
subset of PGOAUTHDEBUG and still work as intended.

Even in this form it is a best effort, as this is a debugging/testing flag:

a. If a custom client uses a custom PG_AUTHDATA_HOOK and provides a
custom token, libpq accepts any issues URL
b. If the issuer is a well known URI, used directly by libpq, it
accept the URL as is
c. if the url is not a well known URI, but doesn't match the server
URI - it doesn't work that nicely, it accepts the difference but
continues but retrieves the well known URI from the server, so ignores
the client setting

Technically this was already possible by a variation of (a) without
this patch, by implementing a custom client with a PG_AUTHDATA_HOOK,
providing a token from a different issuer to it, and lying about the
issuer to libpq (providing what the server expects). But that's not an
easy way to do it and requires all validators to implement custom
clients for testing.

Additionally this feature also could be useful for demoing that
validators are secure to users ("see, the validator rejects the
request even if we trick the client into continuing with
authentication")

[1] :
https://www.postgresql.org/message-id/CAN4CZFNvZ9%2BpQ%3DOA4m%3DHcDgip84GHnekh4gUhYWfK3Q4%2BrBMxA%40mail.gmail.com

On Wed, Feb 18, 2026 at 7:08 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
> 1 is the same patch I already sent as part of the PGOAUTHCAFILE
> discussion[1], rebased on the current master: it splits
> PGOAUTHDEBUG=UNSAFE into separate unsafe/safe settings which users can
> toggle one by one.
>
> 2 is a new unsafe setting issuer-mismatch, which allows a connection
> to continue if the client and server issuers don't match. While this
> isn't useful for end users, it makes testing validators easier, as
> validators authors should be able to verify that mismatched
> configurations are rejected properly by the validator.

v2, attached, rebases this over 993368113. The big change is the
removal of `custom-ca`; there were a couple of other tweaks to get
both commits compiling independently.

--Jacob

On Mon, Mar 30, 2026 at 2:41 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> v2, attached, rebases this over 993368113. The big change is the
> removal of `custom-ca`; there were a couple of other tweaks to get
> both commits compiling independently.

Now for review. 0001:

I like how the UNSAFE: split works in practice. Implementation-wise, I
think I'd prefer that the debug flags be implemented as bits in a
uint32, and then we can cut down on the boilerplate.
fe-auth-oauth-debug.c is IMO too much code for what the feature is
providing. We should ideally be able to include this in a header from
both locations it's used.

I think `fast-retry` needs to be moved under UNSAFE and renamed to
something that doesn't sound "good". `dos-interval` maybe?

nitpick: `poll-counts` and `print-plugin-errors` choose different
naming conventions, and we're not referring to the poll() API for the
former. `call-count`? `dlopen`?

We need to test that ignored unsafe options are in fact ignored (the
parsed flag is currently being accumulated anyway, in contradiction to
the warning message).

I have a sample patch locally for these suggestions, if you'd like.

--

I'm not a fan of 0002; I don't think it provides enough useful
functionality to offset the cost. You said

> validators authors should be able to verify that mismatched
> configurations are rejected properly by the validator.

but "mismatched configuration" is kind of meaningless here: the
validator interacts with a token, not a client. Real-world validator
implementations already need to test against all manner of broken
tokens -- creating one that's signed by the wrong party shouldn't
really be harder to create than one that's signed by the right party
-- and the code to send a custom token from libpq is minimal (<40
lines).

--Jacob

Thanks for the review, these changes generally sound good.

> I think `fast-retry` needs to be moved under UNSAFE and renamed to
> something that doesn't sound "good". `dos-interval` maybe?

I would use a different name, for something like `dos-interval` I
would expect to provide a time since it's an interval?
`immediate-retry` maybe? Or `dos-retry`?

> nitpick: `poll-counts` and `print-plugin-errors` choose different
> naming conventions, and we're not referring to the poll() API for the
> former. `call-count`? `dlopen`?

I didn't want to write "print-poll-counts" and "print-trace" as those
are just longer, while simply writing "plugin-errors" without print
also seemed wrong. Maybe it could be "plugin-debug" instead, that
sounds good even withour print?

> I have a sample patch locally for these suggestions, if you'd like.

I can create a patch with these updates tomorrow, but if you already
have it, that might be easier/quicker.

> I'm not a fan of 0002

That's okay, I am fine with dropping that. We are already using that
small custom libpq client for testing, so we can keep using it. I just
thought this could make things easier/clearer to others.

On Tue, Mar 31, 2026 at 10:45 AM Zsolt Parragi
<zsolt.parragi@percona.com> wrote:
> I didn't want to write "print-poll-counts" and "print-trace" as those
> are just longer, while simply writing "plugin-errors" without print
> also seemed wrong. Maybe it could be "plugin-debug" instead, that
> sounds good even withour print?

I like `plugin-errors`, actually -- "I want to debug these plugin
errors." Adding -debug to the name doesn't seem great to me, since
it's clear from the context that we're debugging.

`dos-retry` still doesn't quite convey what we're doing...
`dos-endpoint` maybe? `busy-loop`? The unsafe aspect is the resource
consumption...

To keep the brainstorming going, I chose the following for v3, but
they're by no means settled:

- http
- trace
- dos-endpoint
- call-count
- plugin-errors

> > I have a sample patch locally for these suggestions, if you'd like.
>
> I can create a patch with these updates tomorrow, but if you already
> have it, that might be easier/quicker.

In the interest of time I've attached it as a single patch, but the
range-diff is rough to read. If you'd like, I can split the code
motion apart from the logical changes tomorrow, to see if it helps
with review.

--Jacob

> On Apr 1, 2026, at 07:50, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
>
> On Tue, Mar 31, 2026 at 10:45 AM Zsolt Parragi
> <zsolt.parragi@percona.com> wrote:
>> I didn't want to write "print-poll-counts" and "print-trace" as those
>> are just longer, while simply writing "plugin-errors" without print
>> also seemed wrong. Maybe it could be "plugin-debug" instead, that
>> sounds good even withour print?
>
> I like `plugin-errors`, actually -- "I want to debug these plugin
> errors." Adding -debug to the name doesn't seem great to me, since
> it's clear from the context that we're debugging.
>
> `dos-retry` still doesn't quite convey what we're doing...
> `dos-endpoint` maybe? `busy-loop`? The unsafe aspect is the resource
> consumption...
>
> To keep the brainstorming going, I chose the following for v3, but
> they're by no means settled:
>
> - http
> - trace
> - dos-endpoint
> - call-count
> - plugin-errors
>
>>> I have a sample patch locally for these suggestions, if you'd like.
>>
>> I can create a patch with these updates tomorrow, but if you already
>> have it, that might be easier/quicker.
>
> In the interest of time I've attached it as a single patch, but the
> range-diff is rough to read. If you'd like, I can split the code
> motion apart from the logical changes tomorrow, to see if it helps
> with review.
>
> --Jacob
> <since-v2.nocfbot.diff><v3-0001-Split-PGOAUTHDEBUG-UNSAFE-into-multiple-options.patch>


Looks like this patch helps reduce uninteresting debug logs. Overall, it looks good to me. I just have a couple of
smallcomments. 

1
```
+#define OAUTHDEBUG_UNSAFE_HTTP            (1<<0)
```

Since the flags are defined as uint32, does it make sense to define these flag constants as ((uint32) 1 << 0)?

2 oauth_get_debug_flags() reparses PGOAUTHDEBUG every time it is called, which feels a bit unnecessary. But I don't
thinkthis is a big deal, since these debug options should never be enabled in production. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

> To keep the brainstorming going, I chose the following for v3, but
> they're by no means settled: ...

The names look good. I'm still not 100% sure about plugin-errors, but
since we already have debug in the variable name, it might be okay.

+/* all safe and unsafe flags, for the legacy UNSAFE behavior */
+#define OAUTHDEBUG_UNSAFE_ALL ((uint32) ~0)

The name of this variable is a bit confusing, it's not only about
unsafe settings. I understand why you added the unsafe to it, but
before checking the value/comment I thought this will be the bitmask
of all unsafe options. On the other hand I don't have a better idea
other than simply using ALL.

+oauth_get_debug_flags(void)
+{
+ uint32 flags = 0;
+ const char *env = getenv("PGOAUTHDEBUG");
...

One of the reasons why I implemented this in a C file is because I
wanted to avoid reparsing and warning spam/duplication. Reparsing
shouldn't be a major issue since this is a debug feature, but this
approach causes a warning to print twice in a few corner cases.

That's probably acceptable, the issue is limited in the current code,
but I wanted to mention it as a limitation, as I don't think there's a
good way to fix this without a .c file.

On Tue, Mar 31, 2026 at 8:45 PM Chao Li <li.evan.chao@gmail.com> wrote:
> +#define OAUTHDEBUG_UNSAFE_HTTP                 (1<<0)
>
> Since the flags are defined as uint32, does it make sense to define these flag constants as ((uint32) 1 << 0)?

No, I don't think so. (If we ever got to <<31 we'd need to switch to
1U instead of 1, I think, but I still wouldn't want to write it as a
cast. Bitflags are in pretty wide use across our codebase and I don't
want to introduce a new spelling.)

> 2 oauth_get_debug_flags() reparses PGOAUTHDEBUG every time it is called, which feels a bit unnecessary.

We could maybe rename it oauth_parse_debug_flags(), so it's at least
not hidden/surprising?

> But I don't think this is a big deal, since these debug options should never be enabled in production.

Right.

--Jacob

On Wed, Apr 1, 2026 at 2:35 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
> +/* all safe and unsafe flags, for the legacy UNSAFE behavior */
> +#define OAUTHDEBUG_UNSAFE_ALL ((uint32) ~0)
>
> The name of this variable is a bit confusing, it's not only about
> unsafe settings. I understand why you added the unsafe to it, but
> before checking the value/comment I thought this will be the bitmask
> of all unsafe options.

I agree.

> On the other hand I don't have a better idea
> other than simply using ALL.

OAUTHDEBUG_LEGACY_UNSAFE?

> +oauth_get_debug_flags(void)
> +{
> + uint32 flags = 0;
> + const char *env = getenv("PGOAUTHDEBUG");
> ...
>
> One of the reasons why I implemented this in a C file is because I
> wanted to avoid reparsing and warning spam/duplication.

I think I'm missing something; how does the choice of .c/.h change
things? There's no static tracking in v1 of the patchset (nor should
there be without locking or atomics, which is not maintenance I really
want to introduce for a debug feature).

> Reparsing
> shouldn't be a major issue since this is a debug feature, but this
> approach causes a warning to print twice in a few corner cases.

Which new corner cases? v1 also prints duplicates (e.g. with
`UNSAFE:blah,http`). I didn't intend to introduce any new calls to
oauth_get_debug_flags() over those already done in v1/v2; if I did
that's a bug.

--Jacob

On Wed, Apr 1, 2026 at 10:09 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> I didn't intend to introduce any new calls to
> oauth_get_debug_flags() over those already done in v1/v2; if I did
> that's a bug.

To make seeing that a little easier, here's the promised version of v3
as an exploded patch series with more detailed justification, based on
v2-0001.

I'm glad I did that, because I forgot to call attention to a
particular change I made that I think is important:

>             fprintf(stderr,
> -                   "WARNING: PGOAUTHDEBUG: unsafe option \"%s\" requires UNSAFE: prefix (ignored)\n"
> -                   "Use: PGOAUTHDEBUG=UNSAFE:%s\n",
> -                   option, option);
> +                   libpq_gettext("WARNING: PGOAUTHDEBUG option \"%s\" is unsafe (ignored)\n"),
> +                   option);

`UNSAFE` is intended to be a weak defense against social engineering
attacks. So these warnings need to be translated, if possible, and we
should not provide instructions on how to defeat that defense. The
only people who _should_ be using an unsafe feature should also know
how to fix this problem.

--Jacob

> OAUTHDEBUG_LEGACY_UNSAFE?

That sounds better

> I think I'm missing something; how does the choice of .c/.h change
> things? There's no static tracking in v1 of the patchset

Eh, sorry about that, I was sure that I sent a version which handled
that to the list, but apparently I didn't. It didn't use
atomics/mutexes, so maybe it's better.

> `UNSAFE` is intended to be a weak defense against social engineering
> attacks. So these warnings need to be translated, if possible, and we
> should not provide instructions on how to defeat that defense.

With the same logic, shouldn't we print a very visible warning when
somebody enables trace? Since it's a long output, maybe to both the
beginning and end of the flow?

On Wed, Apr 1, 2026 at 2:13 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
> With the same logic, shouldn't we print a very visible warning when
> somebody enables trace? Since it's a long output, maybe to both the
> beginning and end of the flow?

I'm more than happy to strengthen this as well, but let's kick that
out to its own thread, especially if pieces are backpatchable.

--Jacob

On Wed, Apr 1, 2026 at 2:13 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
>
> > OAUTHDEBUG_LEGACY_UNSAFE?
>
> That sounds better

Done. v4 also renames oauth_get_debug_flags() to
oauth_parse_debug_flags() (and adds a related doc comment) after Chao
Li's feedback, and tightens up some of the new documentation.

Thanks,
--Jacob