On Thu, Feb 12, 2026 at 10:51 AM Zsolt Parragi
<[email protected]> wrote:
>
> Thank you for the quick review!
Thanks for tackling this TODO! :D
I have a lot of little nitpicky feedback, but please push back on
anything you disagree with (trying to avoid Primary Author Syndrome).
I've also copied Michael for opinions on the new API.
= Mechanism-Independent Changes =
> +#define PG_SASL_EXCHANGE_RESTART 3
I think the "restart" nomenclature is maybe a subtle layering
violation. From the point of view of the server (and especially the
mechanism-independent code in auth-sasl.c), we have no idea if the
client's going to retry or not. All we know is that the client doesn't
intend to authenticate on this connection. (There's also maybe some
relationship with SASL's concept of client-aborted exchanges, which we
don't support AFAIK, and would be handled at the SASL layer rather
than the mech layer.)
Maybe something like PG_SASL_EXCHANGE_ABANDONED?
> + if (result == PG_SASL_EXCHANGE_RESTART)
> + return STATUS_EOF;
> +
> /* Oops, Something bad happened */
> if (result != PG_SASL_EXCHANGE_SUCCESS)
Let's keep these two cases together, either as an if/else-if, or by
switching STATUS_ERROR to STATUS_EOF as needed.
= Mechanism-Specific Changes =
> + if (auth[0] == '\0')
> + ctx->discovery = true;
> +
> if (!validate(ctx->port, auth))
> {
I think this might be more straightforward as a variant of the
OAUTH_STATE_ERROR state (OAUTH_STATE_ERROR_DISCOVERY?) rather than a
separate `discovery` flag.
> - * TODO: should we find a way to return STATUS_EOF at the top level,
> - * to suppress the authentication error entirely?
> + * The caller detects this case and returns
> + * PG_SASL_EXCHANGE_RESTART to suppress the authentication FATAL.
> */
IMO this shows that there's no reason for me to have called validate()
from oauth_exchange() for this case -- there's nothing to validate.
validate() should just Assert that it's been passed a non-empty token.
> + errmsg("OAuth issuer discovery requested by user \"%s\"",
> + ctx->port->user_name));
Since authentication isn't complete, we don't really know "who"
requested discovery. I think we should rely on log_[dis]connections to
provide debugging info to the DBA in these situations; this can just
log the fact that discovery was requested.
Thanks!
--Jacob
