Re: Improve OAuth discovery logging

On Mon, Mar 16, 2026 at 10:29 PM Zsolt Parragi
<zsolt.parragi@percona.com> wrote:
> > As far as I understand, for a programmer error, Assert should be used. Why do we use elog(ERROR) here?
>
> +1

A couple reasons:
- to keep it parallel with the similar elog directly above it
- to strengthen the API boundary for an esoteric edge case without
requiring assertions to be enabled

I don't mind relying on assertions for (not an exhaustive list!)
well-exercised code, or when performance is critical, or when the
callers and callees are in the same source file, or when it's not a
security-critical context... This would seem to fail all of those
tests.

I imagine some of the existing assertions in OAuth should ideally be
strengthened into production checks, too. I've considered adding a
"production assert" variant for our security code in general, client-
and server-side.

> I would also say that for CheckSASLAuth, specifying abandoned is
> always required, since the caller can't know when it will result in an
> error.

That's not really true, because the caller hardcodes the mechanism
descriptor. The layers above and below CheckSASLAuth are coupled via
injection -- ClientAuthentication knows full well that a uaOAuth HBA
entry can't be satisfied by the SCRAM mechanisms, and vice-versa.

If that were to change in a meaningful way, then sure, the caller
would need to always provide the currently-OAuth-specific-edge-case
flag. (But in that case, if the caller somehow doesn't have to know
the mechanism in use, we could presumably also centralize a single
call to CheckSASLAuth().)

> Have you considered adding the error level here instead, the same way
> as in auth_failed, explicitly defaulted to normal fatal by the caller,
> so existing code don't have to change it? That wouldn't need an
> SASL-specific explanation or flag in the generic code.

I don't think I can visualize what you're proposing. If you mean that
CheckSASLAuth should set the elevel with an output parameter, I'd
rather not; that moves the responsibility for a very critical
assumption (we're ending the process now) across a bunch of different
files.

(If more things than OAuth need this eventually, maybe it becomes
STATUS_SILENT_ERROR or something, to make it even more generic?)

Thanks,
--Jacob