public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jacob Champion <[email protected]>
To: Zsolt Parragi <[email protected]>
Cc: Andrey Borodin <[email protected]>
Cc: Chao Li <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Tom Lane <[email protected]>
Subject: Re: Improve OAuth discovery logging
Date: Mon, 16 Mar 2026 17:24:58 -0700
Message-ID: <CAOYmi+nsK1dSXaB+oicoyA6kM9ymygCLhSiKtkg1ph_P1uhYOQ@mail.gmail.com> (raw)
In-Reply-To: <CAN4CZFP--Ec8hMgpu7JojgK9qS08bNnev0c6goA++T4Ozy8bOQ@mail.gmail.com>
References: <CAN4CZFPim7hUiyb7daNKQPSZ8CvQRBGkVhbvED7yZi8VktSn4Q@mail.gmail.com>
<[email protected]>
<CAN4CZFNNfhFCQdFWui5HWbQR60eM-cyndZ7YgSv7b5SKxB9C2A@mail.gmail.com>
<CAOYmi+mDSmh6RNizHRmMAwg4ZP2W=uai3Fr3-wm186NMypf_Pg@mail.gmail.com>
<CAN4CZFNJftK8NaREYaLi-wqpEz3=crQ=1+3f_XUVji=aOrDSWA@mail.gmail.com>
<[email protected]>
<CAOYmi+kjtmRMBdBU3_bGKGDoRSK2AErXbGtHkAjFRapcQNmjhA@mail.gmail.com>
<CAN4CZFNWBXtF-ML3yzdOvX3QEuUwVo5VrBzyWU3O=y-7SeDstA@mail.gmail.com>
<[email protected]>
<CAN4CZFNscs=hiOkRJYF39r7AD7ef9+MR+O2BQdEtE_2Ajdo5qw@mail.gmail.com>
<CAOYmi+nVzkoLjzNk_58e0NnUPi9uVXwmurK2QP6CzC2WOpqwbg@mail.gmail.com>
<CAN4CZFPjiUQbKo2q+ovs--AHkjvaE8OJyncB9xu5b+1gp=HHPQ@mail.gmail.com>
<CAOYmi+=SR_nJJBh7UXZzK8Zbs21L2RUNkW3d9aPRkQOHj1bBPA@mail.gmail.com>
<CAN4CZFO7ju7fjjv+qwObP8_V-Tdx463zV8F7u_s6wtg9ANVWVg@mail.gmail.com>
<CAOYmi+kEYA0Tp2son-+Ti1wvSAPov87AVFf4qXATTOHRX1F2gg@mail.gmail.com>
<CAN4CZFOmym1BaV_U2V56aOyRp2JMrw5nfn6kwcAEcu_RWK-F3Q@mail.gmail.com>
<[email protected]>
<CAN4CZFN7u1kX3_0cfyVvtfiWpORxnvZo=xCr9Ag-F5Onp-hpbA@mail.gmail.com>
<[email protected]>
<CAOYmi+kxfGEKw7frQPxWYEA6Qe4BLc683UCNPTYCLdCCV0b4Jw@mail.gmail.com>
<CAN4CZFP--Ec8hMgpu7JojgK9qS08bNnev0c6goA++T4Ozy8bOQ@mail.gmail.com>
On Mon, Mar 16, 2026 at 12:45 PM Zsolt Parragi
<[email protected]> wrote:
> I tried to figure out if this is fine or not, but isn't it the same as
> the existing ereport(ERROR, ...) calls everywhere in the sasl/scram
> code?
Those are *also* not good, IMHO; they're what I had in mind when I
said "it's unusual/invisible". (ERROR is upgraded to FATAL here, so
they're also misleading.) OAuth inherited a few of those from SCRAM to
avoid divergent behavior for protocol violations, but I don't really
want to lock that usage into the SASL architecture by myself,
especially not for normal operation. CheckSASLAuth should ideally have
control over the logic flow.
(It might be nice to make it possible to throw ERRORs from inside
authentication code without bypassing the top level. Then maybe we
could square that with our treatment of logdetail et al. But we'd have
to decide how we want protocol errors to interact with the hook.)
On Mon, Mar 16, 2026 at 11:14 AM Jacob Champion
<[email protected]> wrote:
> I'm working on a three-patch set to add FATAL_CLIENT_ONLY, the new
> abandoned state, and the log fix making use of both.
Attached as v8.
--Jacob
Attachments:
[application/octet-stream] v8-0001-Add-FATAL_CLIENT_ONLY-to-ereport-elog.patch (3.0K, 2-v8-0001-Add-FATAL_CLIENT_ONLY-to-ereport-elog.patch)
download | inline diff:
From dbe0717df83787d9523055073d30839cfe7eee1d Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Wed, 11 Mar 2026 15:49:02 -0700
Subject: [PATCH v8 1/3] Add FATAL_CLIENT_ONLY to ereport/elog
SASL exchanges must end with either an AuthenticationOk or an
ErrorResponse from the server, and the standard way to produce an
ErrorResponse packet is for auth_failed() to call ereport(FATAL). This
means that there's no way for a SASL mechanism to suppress the server
log entry if the "authentication attempt" was really just a query for
authentication metadata, as is done with OAUTHBEARER.
Following the example of 1f9158ba4, add a FATAL_CLIENT_ONLY elevel. This
will allow ClientAuthentication() to choose not to log a particular
failure, while still correctly ending the authentication exchange before
process exit.
---
src/include/utils/elog.h | 3 ++-
src/backend/utils/error/elog.c | 7 +++++--
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index a12b379e09a..440a02dd147 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -53,7 +53,8 @@ struct Node;
* known state */
#define PGERROR 21 /* Must equal ERROR; see NOTE below. */
#define FATAL 22 /* fatal error - abort process */
-#define PANIC 23 /* take down the other backends with me */
+#define FATAL_CLIENT_ONLY 23 /* fatal version of WARNING_CLIENT_ONLY */
+#define PANIC 24 /* take down the other backends with me */
/*
* NOTE: the alternate names PGWARNING and PGERROR are useful for dealing
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 80b78f25267..2719049040a 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -217,7 +217,7 @@ is_log_level_output(int elevel, int log_min_level)
if (log_min_level == LOG || log_min_level <= ERROR)
return true;
}
- else if (elevel == WARNING_CLIENT_ONLY)
+ else if (elevel == WARNING_CLIENT_ONLY || elevel == FATAL_CLIENT_ONLY)
{
/* never sent to log, regardless of log_min_level */
return false;
@@ -573,7 +573,7 @@ errfinish(const char *filename, int lineno, const char *funcname)
/*
* Perform error recovery action as specified by elevel.
*/
- if (elevel == FATAL)
+ if (elevel == FATAL || elevel == FATAL_CLIENT_ONLY)
{
/*
* For a FATAL error, we let proc_exit clean up and exit.
@@ -2965,6 +2965,7 @@ write_eventlog(int level, const char *line, int len)
break;
case ERROR:
case FATAL:
+ case FATAL_CLIENT_ONLY:
case PANIC:
default:
eventlevel = EVENTLOG_ERROR_TYPE;
@@ -3800,6 +3801,7 @@ send_message_to_server_log(ErrorData *edata)
syslog_level = LOG_WARNING;
break;
case FATAL:
+ case FATAL_CLIENT_ONLY:
syslog_level = LOG_ERR;
break;
case PANIC:
@@ -4182,6 +4184,7 @@ error_severity(int elevel)
prefix = gettext_noop("ERROR");
break;
case FATAL:
+ case FATAL_CLIENT_ONLY:
prefix = gettext_noop("FATAL");
break;
case PANIC:
--
2.34.1
[application/octet-stream] v8-0003-oauth-Don-t-log-discovery-connections-by-default.patch (4.6K, 3-v8-0003-oauth-Don-t-log-discovery-connections-by-default.patch)
download | inline diff:
From 85d703f5f8ac331c384ad0b54fecd0475e14de47 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Mon, 16 Mar 2026 17:01:05 -0700
Subject: [PATCH v8 3/3] oauth: Don't log discovery connections by default
Currently, when the client sends a parameter discovery request within
OAUTHBEARER, the server logs the attempt with
FATAL: OAuth bearer authentication failed for user
These log entries are difficult to distinguish from true authentication
failures, and by default, libpq sends a discovery request as part of
every OAuth connection, making them annoyingly noisy. Use the new
PG_SASL_EXCHANGE_ABANDONED status to suppress them.
Author: Zsolt Parragi <[email protected]>
---
src/backend/libpq/auth-oauth.c | 45 ++++++++++++-------
.../modules/oauth_validator/t/001_server.pl | 6 ++-
2 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c
index 11365048951..894efe3c904 100644
--- a/src/backend/libpq/auth-oauth.c
+++ b/src/backend/libpq/auth-oauth.c
@@ -58,6 +58,7 @@ enum oauth_state
{
OAUTH_STATE_INIT = 0,
OAUTH_STATE_ERROR,
+ OAUTH_STATE_ERROR_DISCOVERY,
OAUTH_STATE_FINISHED,
};
@@ -181,6 +182,7 @@ oauth_exchange(void *opaq, const char *input, int inputlen,
break;
case OAUTH_STATE_ERROR:
+ case OAUTH_STATE_ERROR_DISCOVERY:
/*
* Only one response is valid for the client during authentication
@@ -192,7 +194,19 @@ oauth_exchange(void *opaq, const char *input, int inputlen,
errmsg("malformed OAUTHBEARER message"),
errdetail("Client did not send a kvsep response."));
- /* The (failed) handshake is now complete. */
+ /*
+ * The (failed) handshake is now complete. Don't report discovery
+ * requests in the server log unless the log level is high enough.
+ */
+ if (ctx->state == OAUTH_STATE_ERROR_DISCOVERY)
+ {
+ ereport(DEBUG1, errmsg("OAuth issuer discovery requested"));
+
+ ctx->state = OAUTH_STATE_FINISHED;
+ return PG_SASL_EXCHANGE_ABANDONED;
+ }
+
+ /* We're not in discovery, so this is just a normal auth failure. */
ctx->state = OAUTH_STATE_FINISHED;
return PG_SASL_EXCHANGE_FAILURE;
@@ -279,7 +293,19 @@ oauth_exchange(void *opaq, const char *input, int inputlen,
errmsg("malformed OAUTHBEARER message"),
errdetail("Message contains additional data after the final terminator."));
- if (!validate(ctx->port, auth))
+ if (auth[0] == '\0')
+ {
+ /*
+ * An empty auth value represents a discovery request; the client
+ * expects it to fail. Skip validation entirely and move directly to
+ * the error response.
+ */
+ generate_error_response(ctx, output, outputlen);
+
+ ctx->state = OAUTH_STATE_ERROR_DISCOVERY;
+ status = PG_SASL_EXCHANGE_CONTINUE;
+ }
+ else if (!validate(ctx->port, auth))
{
generate_error_response(ctx, output, outputlen);
@@ -564,19 +590,8 @@ validate_token_format(const char *header)
/* Missing auth headers should be handled by the caller. */
Assert(header);
-
- if (header[0] == '\0')
- {
- /*
- * A completely empty auth header represents a query for
- * authentication parameters. The client expects it to fail; there's
- * no need to make any extra noise in the logs.
- *
- * TODO: should we find a way to return STATUS_EOF at the top level,
- * to suppress the authentication error entirely?
- */
- return NULL;
- }
+ /* Empty auth (discovery) should be handled before calling validate(). */
+ Assert(header[0] != '\0');
if (pg_strncasecmp(header, BEARER_SCHEME, strlen(BEARER_SCHEME)))
{
diff --git a/src/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl
index cdad2ae8011..f5dc427cdd1 100644
--- a/src/test/modules/oauth_validator/t/001_server.pl
+++ b/src/test/modules/oauth_validator/t/001_server.pl
@@ -151,7 +151,8 @@ $node->connect_ok(
qr/oauth_validator: issuer="\Q$issuer\E", scope="openid postgres"/,
qr/connection authenticated: identity="test" method=oauth/,
qr/connection authorized/,
- ]);
+ ],
+ log_unlike => [qr/FATAL.*OAuth bearer authentication failed/]);
# The /alternate issuer uses slightly different parameters, along with an
# OAuth-style discovery document.
@@ -166,7 +167,8 @@ $node->connect_ok(
qr|oauth_validator: issuer="\Q$issuer/.well-known/oauth-authorization-server/alternate\E", scope="openid postgres alt"|,
qr/connection authenticated: identity="testalt" method=oauth/,
qr/connection authorized/,
- ]);
+ ],
+ log_unlike => [qr/FATAL.*OAuth bearer authentication failed/]);
# The issuer linked by the server must match the client's oauth_issuer setting.
$node->connect_fails(
--
2.34.1
[application/octet-stream] v8-0002-sasl-Allow-backend-mechanisms-to-abandon-exchange.patch (8.4K, 4-v8-0002-sasl-Allow-backend-mechanisms-to-abandon-exchange.patch)
download | inline diff:
From f124deb76d95d6ac41cfc8f20f41b10249f9f2d1 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Mon, 16 Mar 2026 17:09:08 -0700
Subject: [PATCH v8 2/3] sasl: Allow backend mechanisms to "abandon" exchanges
Introduce PG_SASL_EXCHANGE_ABANDONED, which allows CheckSASLAuth to
suppress the failing log entry for any SASL exchange that isn't actually
an authentication attempt. This is desirable for OAUTHBEARER's discovery
exchanges (and a subsequent commit will make use of it there).
This might have some overlap in the future with in-band aborts for SASL
exchanges, but it's intentionally not named _ABORTED to avoid confusion.
(We don't currently support clientside aborts in our SASL profile.)
Adapted from a patch by Zsolt Parragi.
Author: Zsolt Parragi <[email protected]>
Co-authored-by: Jacob Champion <[email protected]>
---
src/include/libpq/sasl.h | 15 +++++++++------
src/backend/libpq/auth-sasl.c | 24 ++++++++++++++++++++++--
src/backend/libpq/auth.c | 32 +++++++++++++++++++++++++-------
3 files changed, 56 insertions(+), 15 deletions(-)
diff --git a/src/include/libpq/sasl.h b/src/include/libpq/sasl.h
index 1e8ec7d6293..bb2af7a7aff 100644
--- a/src/include/libpq/sasl.h
+++ b/src/include/libpq/sasl.h
@@ -25,6 +25,7 @@
#define PG_SASL_EXCHANGE_CONTINUE 0
#define PG_SASL_EXCHANGE_SUCCESS 1
#define PG_SASL_EXCHANGE_FAILURE 2
+#define PG_SASL_EXCHANGE_ABANDONED 3
/*
* Maximum accepted size of SASL messages.
@@ -92,8 +93,8 @@ typedef struct pg_be_sasl_mech
*
* Produces a server challenge to be sent to the client. The callback
* must return one of the PG_SASL_EXCHANGE_* values, depending on
- * whether the exchange continues, has finished successfully, or has
- * failed.
+ * whether the exchange continues, has finished successfully, has
+ * failed, or was abandoned by the client.
*
* Input parameters:
*
@@ -118,8 +119,9 @@ typedef struct pg_be_sasl_mech
* returned and the mechanism requires data to be sent during
* a successful outcome). The callback should set this to
* NULL if the exchange is over and no output should be sent,
- * which should correspond to either PG_SASL_EXCHANGE_FAILURE
- * or a PG_SASL_EXCHANGE_SUCCESS with no outcome data.
+ * which should correspond to either PG_SASL_EXCHANGE_FAILURE,
+ * PG_SASL_EXCHANGE_ABANDONED, or a PG_SASL_EXCHANGE_SUCCESS
+ * with no outcome data.
*
* outputlen: The length of the challenge data. Ignored if *output is
* NULL.
@@ -128,7 +130,7 @@ typedef struct pg_be_sasl_mech
* server log, to disambiguate failure modes. (The client
* will only ever see the same generic authentication
* failure message.) Ignored if the exchange is completed
- * with PG_SASL_EXCHANGE_SUCCESS.
+ * with PG_SASL_EXCHANGE_SUCCESS or PG_SASL_EXCHANGE_ABANDONED.
*---------
*/
int (*exchange) (void *state,
@@ -142,6 +144,7 @@ typedef struct pg_be_sasl_mech
/* Common implementation for auth.c */
extern int CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port,
- char *shadow_pass, const char **logdetail);
+ char *shadow_pass, const char **logdetail,
+ bool *abandoned);
#endif /* PG_SASL_H */
diff --git a/src/backend/libpq/auth-sasl.c b/src/backend/libpq/auth-sasl.c
index 36cb748d927..59ac38fca50 100644
--- a/src/backend/libpq/auth-sasl.c
+++ b/src/backend/libpq/auth-sasl.c
@@ -30,6 +30,12 @@
* be found for the role (or the user does not exist), and the mechanism
* should fail the authentication exchange.
*
+ * Some SASL mechanisms (e.g. OAUTHBEARER) define special exchanges for
+ * parameter discovery. These exchanges will always result in STATUS_ERROR,
+ * since we can't let the connection continue, but we shouldn't consider them to
+ * be failed authentication attempts. *abandoned will be set to true in this
+ * case.
+ *
* Mechanisms must take care not to reveal to the client that a user entry
* does not exist; ideally, the external failure mode is identical to that
* of an incorrect password. Mechanisms may instead use the logdetail
@@ -42,7 +48,7 @@
*/
int
CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass,
- const char **logdetail)
+ const char **logdetail, bool *abandoned)
{
StringInfoData sasl_mechs;
int mtype;
@@ -167,7 +173,7 @@ CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass,
* PG_SASL_EXCHANGE_FAILURE with some output is forbidden by SASL.
* Make sure here that the mechanism used got that right.
*/
- if (result == PG_SASL_EXCHANGE_FAILURE)
+ if (result == PG_SASL_EXCHANGE_FAILURE || result == PG_SASL_EXCHANGE_ABANDONED)
elog(ERROR, "output message found after SASL exchange failure");
/*
@@ -184,6 +190,20 @@ CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass,
}
} while (result == PG_SASL_EXCHANGE_CONTINUE);
+ if (result == PG_SASL_EXCHANGE_ABANDONED)
+ {
+ if (!abandoned)
+ {
+ /*
+ * Programmer error: caller needs to track the abandoned state for
+ * this mechanism.
+ */
+ elog(ERROR, "SASL exchange was abandoned, but CheckSASLAuth isn't tracking it");
+ }
+
+ *abandoned = true;
+ }
+
/* Oops, Something bad happened */
if (result != PG_SASL_EXCHANGE_SUCCESS)
{
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index e04aa2e68ed..fdacc060381 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -45,7 +45,8 @@
* Global authentication functions
*----------------------------------------------------------------
*/
-static void auth_failed(Port *port, int status, const char *logdetail);
+static void auth_failed(Port *port, int elevel, int status,
+ const char *logdetail);
static char *recv_password_packet(Port *port);
@@ -233,15 +234,18 @@ ClientAuthentication_hook_type ClientAuthentication_hook = NULL;
* anyway.
* Note that many sorts of failure report additional information in the
* postmaster log, which we hope is only readable by good guys. In
- * particular, if logdetail isn't NULL, we send that string to the log.
+ * particular, if logdetail isn't NULL, we send that string to the log
+ * when the elevel allows.
*/
static void
-auth_failed(Port *port, int status, const char *logdetail)
+auth_failed(Port *port, int elevel, int status, const char *logdetail)
{
const char *errstr;
char *cdetail;
int errcode_return = ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION;
+ Assert(elevel >= FATAL); /* we must exit here */
+
/*
* If we failed due to EOF from client, just quit; there's no point in
* trying to send a message to the client, and not much point in logging
@@ -314,12 +318,13 @@ auth_failed(Port *port, int status, const char *logdetail)
else
logdetail = cdetail;
- ereport(FATAL,
+ ereport(elevel,
(errcode(errcode_return),
errmsg(errstr, port->user_name),
logdetail ? errdetail_log("%s", logdetail) : 0));
/* doesn't return */
+ pg_unreachable();
}
@@ -381,6 +386,15 @@ ClientAuthentication(Port *port)
int status = STATUS_ERROR;
const char *logdetail = NULL;
+ /*
+ * "Abandoned" is a SASL-specific state similar to STATUS_EOF, in that we
+ * don't want to generate any server logs. But it's caused by an in-band
+ * client action that requires a server response, not an out-of-band
+ * connection closure, so we can't just proc_exit() like we do with
+ * STATUS_EOF.
+ */
+ bool abandoned = false;
+
/*
* Get the authentication method to use for this frontend/database
* combination. Note: we do not parse the file at this point; this has
@@ -625,7 +639,8 @@ ClientAuthentication(Port *port)
status = STATUS_OK;
break;
case uaOAuth:
- status = CheckSASLAuth(&pg_be_oauth_mech, port, NULL, NULL);
+ status = CheckSASLAuth(&pg_be_oauth_mech, port, NULL, NULL,
+ &abandoned);
break;
}
@@ -666,7 +681,10 @@ ClientAuthentication(Port *port)
if (status == STATUS_OK)
sendAuthRequest(port, AUTH_REQ_OK, NULL, 0);
else
- auth_failed(port, status, logdetail);
+ auth_failed(port,
+ abandoned ? FATAL_CLIENT_ONLY : FATAL,
+ status,
+ logdetail);
}
@@ -860,7 +878,7 @@ CheckPWChallengeAuth(Port *port, const char **logdetail)
auth_result = CheckMD5Auth(port, shadow_pass, logdetail);
else
auth_result = CheckSASLAuth(&pg_be_scram_mech, port, shadow_pass,
- logdetail);
+ logdetail, NULL /* can't abandon SCRAM */ );
if (shadow_pass)
pfree(shadow_pass);
--
2.34.1
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], [email protected], [email protected], [email protected]
Subject: Re: Improve OAuth discovery logging
In-Reply-To: <CAOYmi+nsK1dSXaB+oicoyA6kM9ymygCLhSiKtkg1ph_P1uhYOQ@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