public inbox for [email protected]  

help / color / mirror / Atom feed

From: Jacob Champion <[email protected]>
To: Zsolt Parragi <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Michael Paquier <[email protected]>
Subject: Re: Improve OAuth discovery logging
Date: Thu, 12 Feb 2026 12:30:12 -0800
Message-ID: <CAOYmi+mDSmh6RNizHRmMAwg4ZP2W=uai3Fr3-wm186NMypf_Pg@mail.gmail.com> (raw)
In-Reply-To: <CAN4CZFNNfhFCQdFWui5HWbQR60eM-cyndZ7YgSv7b5SKxB9C2A@mail.gmail.com>
References: <CAN4CZFPim7hUiyb7daNKQPSZ8CvQRBGkVhbvED7yZi8VktSn4Q@mail.gmail.com>
	<[email protected]>
	<CAN4CZFNNfhFCQdFWui5HWbQR60eM-cyndZ7YgSv7b5SKxB9C2A@mail.gmail.com>

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

view thread (26+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Improve OAuth discovery logging
  In-Reply-To: <CAOYmi+mDSmh6RNizHRmMAwg4ZP2W=uai3Fr3-wm186NMypf_Pg@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox