public inbox for [email protected]help / color / mirror / Atom feed
Re: Improve OAuth discovery logging 26+ messages / 4 participants [nested] [flat]
* Re: Improve OAuth discovery logging @ 2026-02-12 18:51 Zsolt Parragi <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Zsolt Parragi @ 2026-02-12 18:51 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: pgsql-hackers; Jacob Champion <[email protected]> Thank you for the quick review! > Is this valuable to administrators in production, or should this perhaps be a > DEBUGx level logging? No, I was even thinking about removing that completely, and then forgot about it before sending my email. I changed it to DEBUG1, it's definitely not needed outside of debugging oauth issues. Attachments: [application/octet-stream] v2-0001-Improve-OAuth-discovery-logging.patch (5.7K, 2-v2-0001-Improve-OAuth-discovery-logging.patch) download | inline diff: From 96e3326bb3b6e3e0c7c47ef4683500171580bd66 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi <[email protected]> Date: Wed, 11 Feb 2026 19:28:05 +0100 Subject: [PATCH] Improve OAuth discovery logging Currently when the client sends an empty OAuth token to request the issuer URL, the server logs the attempt with FATAL: OAuth bearer authentication failed for user Which is quite confusing, as this is an expected part of the OAuth authentication flow and not an error at all. This in practice results in the server spamming the log with these messages, which are difficult to separate from real (OAuth) authentication failures. This patch improves this by handling the situation properly in the SASL/Oauth code, by introducing a new SASL authentication status, PG_SASL_EXCHANGE_RESTART. The expectation is that authentication mechanisms can set this if they request a restart of the authentication flow. Restart currently requires starting with a new connection, so this simply sets STATUS_EOF. The above prevents logging a fatal error at the end, so instead the OAuth exchange code outputs a simple log message instead. --- src/backend/libpq/auth-oauth.c | 17 +++++++++++++++-- src/backend/libpq/auth-sasl.c | 5 ++++- src/include/libpq/sasl.h | 1 + .../modules/oauth_validator/t/001_server.pl | 8 ++++++-- 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c index 11365048951..10782b278f0 100644 --- a/src/backend/libpq/auth-oauth.c +++ b/src/backend/libpq/auth-oauth.c @@ -68,6 +68,7 @@ struct oauth_ctx Port *port; const char *issuer; const char *scope; + bool discovery; }; static char *sanitize_char(char c); @@ -194,6 +195,15 @@ oauth_exchange(void *opaq, const char *input, int inputlen, /* The (failed) handshake is now complete. */ ctx->state = OAUTH_STATE_FINISHED; + + if (ctx->discovery) + { + ereport(DEBUG1, + errmsg("OAuth issuer discovery requested by user \"%s\"", + ctx->port->user_name)); + return PG_SASL_EXCHANGE_RESTART; + } + return PG_SASL_EXCHANGE_FAILURE; default: @@ -279,6 +289,9 @@ oauth_exchange(void *opaq, const char *input, int inputlen, errmsg("malformed OAUTHBEARER message"), errdetail("Message contains additional data after the final terminator.")); + if (auth[0] == '\0') + ctx->discovery = true; + if (!validate(ctx->port, auth)) { generate_error_response(ctx, output, outputlen); @@ -572,8 +585,8 @@ validate_token_format(const char *header) * 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? + * The caller detects this case and returns + * PG_SASL_EXCHANGE_RESTART to suppress the authentication FATAL. */ return NULL; } diff --git a/src/backend/libpq/auth-sasl.c b/src/backend/libpq/auth-sasl.c index 36cb748d927..29f3839453b 100644 --- a/src/backend/libpq/auth-sasl.c +++ b/src/backend/libpq/auth-sasl.c @@ -167,7 +167,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_RESTART) elog(ERROR, "output message found after SASL exchange failure"); /* @@ -184,6 +184,9 @@ CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass, } } while (result == PG_SASL_EXCHANGE_CONTINUE); + if (result == PG_SASL_EXCHANGE_RESTART) + return STATUS_EOF; + /* Oops, Something bad happened */ if (result != PG_SASL_EXCHANGE_SUCCESS) { diff --git a/src/include/libpq/sasl.h b/src/include/libpq/sasl.h index 1e8ec7d6293..4d96afde198 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_RESTART 3 /* * Maximum accepted size of SASL messages. diff --git a/src/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl index 6b649c0b06f..9d96692312f 100644 --- a/src/test/modules/oauth_validator/t/001_server.pl +++ b/src/test/modules/oauth_validator/t/001_server.pl @@ -114,11 +114,13 @@ $node->connect_ok( expected_stderr => qr@Visit https://example\.com/ and enter the code: postgresuser@, log_like => [ + qr/OAuth issuer discovery requested by user "$user"/, qr/oauth_validator: token="9243959234", role="$user"/, 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. @@ -129,11 +131,13 @@ $node->connect_ok( expected_stderr => qr@Visit https://example\.org/ and enter the code: postgresuser@, log_like => [ + qr/OAuth issuer discovery requested by user "$user"/, qr/oauth_validator: token="9243959234-alt", role="$user"/, 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.43.0 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-02-12 20:30 Jacob Champion <[email protected]> parent: Zsolt Parragi <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Jacob Champion @ 2026-02-12 20:30 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]> 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 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-02-13 13:13 Zsolt Parragi <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Zsolt Parragi @ 2026-02-13 13:13 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]> These all are good suggestions, attached updated patch. > Maybe something like PG_SASL_EXCHANGE_ABANDONED? This is the only one I wasn't sure of, I used RESTART because I was focusing more on the intention of the server ("please restart authentication with this additional information"), and a bit also on the idea that later restart could stay even within the same connection, both in this case and if we add support for reauthentication on token expiration. On the other hand I'm not 100% sure how the other two would work, and ABANDONED is a better description for the current situation, so I adjusted the patch to use that. Attachments: [application/octet-stream] v3-0001-Improve-OAuth-discovery-logging.patch (6.7K, 2-v3-0001-Improve-OAuth-discovery-logging.patch) download | inline diff: From e9b16515ea542cb5ee0e42cf5c5a1c7c246df081 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi <[email protected]> Date: Wed, 11 Feb 2026 19:28:05 +0100 Subject: [PATCH v3] Improve OAuth discovery logging Currently when the client sends an empty OAuth token to request the issuer URL, the server logs the attempt with FATAL: OAuth bearer authentication failed for user Which is quite confusing, as this is an expected part of the OAuth authentication flow and not an error at all. This in practice results in the server spamming the log with these messages, which are difficult to separate from real (OAuth) authentication failures. This patch improves this by handling the situation properly in the SASL/Oauth code, by introducing a new SASL authentication status, PG_SASL_EXCHANGE_RESTART. The expectation is that authentication mechanisms can set this if they request a restart of the authentication flow. Restart currently requires starting with a new connection, so this simply sets STATUS_EOF. The above prevents logging a fatal error at the end, so instead the OAuth exchange code outputs a simple log message instead. --- src/backend/libpq/auth-oauth.c | 39 ++++++++++++------- src/backend/libpq/auth-sasl.c | 14 ++++--- src/include/libpq/sasl.h | 1 + .../modules/oauth_validator/t/001_server.pl | 8 +++- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c index 11365048951..3790d1686ee 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 @@ -193,6 +195,14 @@ oauth_exchange(void *opaq, const char *input, int inputlen, errdetail("Client did not send a kvsep response.")); /* The (failed) handshake is now complete. */ + if (ctx->state == OAUTH_STATE_ERROR_DISCOVERY) + { + ctx->state = OAUTH_STATE_FINISHED; + ereport(DEBUG1, + errmsg("OAuth issuer discovery requested")); + return PG_SASL_EXCHANGE_ABANDONED; + } + ctx->state = OAUTH_STATE_FINISHED; return PG_SASL_EXCHANGE_FAILURE; @@ -279,7 +289,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 +586,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/backend/libpq/auth-sasl.c b/src/backend/libpq/auth-sasl.c index 36cb748d927..62bb06b579a 100644 --- a/src/backend/libpq/auth-sasl.c +++ b/src/backend/libpq/auth-sasl.c @@ -167,7 +167,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,11 +184,13 @@ CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass, } } while (result == PG_SASL_EXCHANGE_CONTINUE); - /* Oops, Something bad happened */ - if (result != PG_SASL_EXCHANGE_SUCCESS) + switch (result) { - return STATUS_ERROR; + case PG_SASL_EXCHANGE_SUCCESS: + return STATUS_OK; + case PG_SASL_EXCHANGE_ABANDONED: + return STATUS_EOF; + default: + return STATUS_ERROR; } - - return STATUS_OK; } diff --git a/src/include/libpq/sasl.h b/src/include/libpq/sasl.h index 1e8ec7d6293..31a97747fed 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. diff --git a/src/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl index 6b649c0b06f..9f2f9f49077 100644 --- a/src/test/modules/oauth_validator/t/001_server.pl +++ b/src/test/modules/oauth_validator/t/001_server.pl @@ -114,11 +114,13 @@ $node->connect_ok( expected_stderr => qr@Visit https://example\.com/ and enter the code: postgresuser@, log_like => [ + qr/OAuth issuer discovery requested/, qr/oauth_validator: token="9243959234", role="$user"/, 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. @@ -129,11 +131,13 @@ $node->connect_ok( expected_stderr => qr@Visit https://example\.org/ and enter the code: postgresuser@, log_like => [ + qr/OAuth issuer discovery requested/, qr/oauth_validator: token="9243959234-alt", role="$user"/, 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.43.0 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-02-24 03:00 Chao Li <[email protected]> parent: Zsolt Parragi <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Chao Li @ 2026-02-24 03:00 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Jacob Champion <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]> > On Feb 13, 2026, at 21:13, Zsolt Parragi <[email protected]> wrote: > > These all are good suggestions, attached updated patch. > >> Maybe something like PG_SASL_EXCHANGE_ABANDONED? > > This is the only one I wasn't sure of, I used RESTART because I was > focusing more on the intention of the server ("please restart > authentication with this additional information"), and a bit also on > the idea that later restart could stay even within the same > connection, both in this case and if we add support for > reauthentication on token expiration. > > On the other hand I'm not 100% sure how the other two would work, and > ABANDONED is a better description for the current situation, so I > adjusted the patch to use that. > <v3-0001-Improve-OAuth-discovery-logging.patch> Hi Zsolt, Thanks for the patch. A few small comments: 1 - commit message ``` SASL/Oauth code, by introducing a new SASL authentication status, PG_SASL_EXCHANGE_RESTART. The expectation is that authentication ``` Looks like you forgot to update the commit message to change PG_SASL_EXCHANGE_RESTART to PG_SASL_EXCHANGE_ABANDONED. 2 - auth-oauth.c ``` /* The (failed) handshake is now complete. */ + if (ctx->state == OAUTH_STATE_ERROR_DISCOVERY) + { + ctx->state = OAUTH_STATE_FINISHED; + ereport(DEBUG1, + errmsg("OAuth issuer discovery requested")); + return PG_SASL_EXCHANGE_ABANDONED; + } + ctx->state = OAUTH_STATE_FINISHED; return PG_SASL_EXCHANGE_FAILURE; ``` "ctx->state = OAUTH_STATE_FINISHED;" is duplicated in the “if” and after the “if”, so it can be pull up to before the “if”. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-02-24 23:24 Jacob Champion <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Jacob Champion @ 2026-02-24 23:24 UTC (permalink / raw) To: Chao Li <[email protected]>; Zsolt Parragi <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]> > On Feb 13, 2026, at 21:13, Zsolt Parragi <[email protected]> wrote: > > Maybe something like PG_SASL_EXCHANGE_ABANDONED? > > This is the only one I wasn't sure of, I used RESTART because I was > focusing more on the intention of the server ("please restart > authentication with this additional information"), and a bit also on > the idea that later restart could stay even within the same > connection, both in this case and if we add support for > reauthentication on token expiration. I think "abandoned" would still work as a descriptor if we eventually supported multiple SASL exchanges per connection. On Mon, Feb 23, 2026 at 7:01 PM Chao Li <[email protected]> wrote: > Looks like you forgot to update the commit message to change PG_SASL_EXCHANGE_RESTART to PG_SASL_EXCHANGE_ABANDONED. Yes -- though keep in mind that committers will often rewrite commit messages from scratch. So while keeping it accurate and well-written should be the goal, perfection isn't required to move something into RfC. Speaking of which: Zsolt, would you mind adding this to the Commitfest? > "ctx->state = OAUTH_STATE_FINISHED;" is duplicated in the “if” and after the “if”, so it can be pull up to before the “if”. +1 --Jacob ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-02-25 13:14 Zsolt Parragi <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 2 replies; 26+ messages in thread From: Zsolt Parragi @ 2026-02-25 13:14 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]> > "ctx->state = OAUTH_STATE_FINISHED;" is duplicated in the “if” and after the “if”, so it can be pull up to before the “if”. It can't, because the if is based on ctx->state. If I move it to before the if, I have to save the previous value, which just makes the code longer. I attached v4 with an edited commit message, nothing else changed. Commitfest: https://commitfest.postgresql.org/patch/6529/ Attachments: [application/octet-stream] v4-0001-Improve-OAuth-discovery-logging.patch (6.7K, 2-v4-0001-Improve-OAuth-discovery-logging.patch) download | inline diff: From 46a26f5c15a869681601547bb7cb4f6e61829cd7 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi <[email protected]> Date: Wed, 11 Feb 2026 19:28:05 +0100 Subject: [PATCH v4] Improve OAuth discovery logging Currently when the client sends an empty OAuth token to request the issuer URL, the server logs the attempt with FATAL: OAuth bearer authentication failed for user Which is quite confusing, as this is an expected part of the OAuth authentication flow and not an error at all. This in practice results in the server spamming the log with these messages, which are difficult to separate from real (OAuth) authentication failures. This patch improves this by handling the situation properly in the SASL/Oauth code, by introducing a new SASL authentication status, PG_SASL_EXCHANGE_ABANDONED. The expectation is that authentication mechanisms can set this if the current authentication attempt should be gracefully aandoned, without printing a fatal error about authentication failure. This is currently only used by OAuth, where libpq typically restarts with another, more detailed authentication attempt soon afterwards. --- src/backend/libpq/auth-oauth.c | 40 ++++++++++++------- src/backend/libpq/auth-sasl.c | 14 ++++--- src/include/libpq/sasl.h | 1 + .../modules/oauth_validator/t/001_server.pl | 8 +++- 4 files changed, 41 insertions(+), 22 deletions(-) diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c index 11365048951..39f66aef4d7 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 @@ -193,7 +195,16 @@ oauth_exchange(void *opaq, const char *input, int inputlen, errdetail("Client did not send a kvsep response.")); /* The (failed) handshake is now complete. */ + if (ctx->state == OAUTH_STATE_ERROR_DISCOVERY) + { + ctx->state = OAUTH_STATE_FINISHED; + ereport(DEBUG1, + errmsg("OAuth issuer discovery requested")); + return PG_SASL_EXCHANGE_ABANDONED; + } + ctx->state = OAUTH_STATE_FINISHED; + return PG_SASL_EXCHANGE_FAILURE; default: @@ -279,7 +290,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 +587,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/backend/libpq/auth-sasl.c b/src/backend/libpq/auth-sasl.c index 36cb748d927..62bb06b579a 100644 --- a/src/backend/libpq/auth-sasl.c +++ b/src/backend/libpq/auth-sasl.c @@ -167,7 +167,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,11 +184,13 @@ CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass, } } while (result == PG_SASL_EXCHANGE_CONTINUE); - /* Oops, Something bad happened */ - if (result != PG_SASL_EXCHANGE_SUCCESS) + switch (result) { - return STATUS_ERROR; + case PG_SASL_EXCHANGE_SUCCESS: + return STATUS_OK; + case PG_SASL_EXCHANGE_ABANDONED: + return STATUS_EOF; + default: + return STATUS_ERROR; } - - return STATUS_OK; } diff --git a/src/include/libpq/sasl.h b/src/include/libpq/sasl.h index 1e8ec7d6293..31a97747fed 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. diff --git a/src/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl index 6b649c0b06f..9f2f9f49077 100644 --- a/src/test/modules/oauth_validator/t/001_server.pl +++ b/src/test/modules/oauth_validator/t/001_server.pl @@ -114,11 +114,13 @@ $node->connect_ok( expected_stderr => qr@Visit https://example\.com/ and enter the code: postgresuser@, log_like => [ + qr/OAuth issuer discovery requested/, qr/oauth_validator: token="9243959234", role="$user"/, 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. @@ -129,11 +131,13 @@ $node->connect_ok( expected_stderr => qr@Visit https://example\.org/ and enter the code: postgresuser@, log_like => [ + qr/OAuth issuer discovery requested/, qr/oauth_validator: token="9243959234-alt", role="$user"/, 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.43.0 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-02-26 05:51 Andrey Borodin <[email protected]> parent: Zsolt Parragi <[email protected]> 1 sibling, 1 reply; 26+ messages in thread From: Andrey Borodin @ 2026-02-26 05:51 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Jacob Champion <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]> This looks like nice patch addressing real issue in log analyzing. Basic idea seems correct to me WRT OAuth, but I'm not a real expert in auth. > On 25 Feb 2026, at 18:14, Zsolt Parragi <[email protected]> wrote: > > It can't, because the if is based on ctx->state. If I move it to > before the if, I have to save the previous value, which just makes the > code longer. Well, you can do something in a line with bool was_discovery = (ctx->state == OAUTH_STATE_ERROR_DISCOVERY); ctx->state = OAUTH_STATE_FINISHED; if (was_discovery) { } But it's a matter of taste. Your code is correct anyway. We can tweak comments a bit in sasl.h: /*--------- * exchange() * * 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. <---- , or was abandoned by the client. * 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. <----- or ABANDONED * failure message.) Ignored if the exchange is completed * with PG_SASL_EXCHANGE_SUCCESS. <------ or ABANDONED That's all what I could grep. And thanks for your review in my thread! Best regards, Andrey Borodin. ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-02-26 16:17 Jacob Champion <[email protected]> parent: Zsolt Parragi <[email protected]> 1 sibling, 0 replies; 26+ messages in thread From: Jacob Champion @ 2026-02-26 16:17 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]> On Wed, Feb 25, 2026 at 5:15 AM Zsolt Parragi <[email protected]> wrote: > Commitfest: https://commitfest.postgresql.org/patch/6529/ This is in my commit queue. (I didn't read adequately and ended up duplicating the patch in CF; I've withdrawn that. Sorry for the confusion, if you received any notification emails.) Thanks! --Jacob ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-02-27 19:51 Zsolt Parragi <[email protected]> parent: Andrey Borodin <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Zsolt Parragi @ 2026-02-27 19:51 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; +Cc: Jacob Champion <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]> > Well, you can do something in a line with Yes, but I either have to declare was_discovery at the beginning of the function, or add { ... } for the case and declare it at the beginning of the case branch. Neither of those seems to be more readable to me. With different coding conventions I would definitely do a `const bool was_discovery` directly before the if, but I can't do that here. Thanks for the comment suggestions, I attached an updated version with an edited comment for exchange(). Attachments: [application/octet-stream] v5-0001-Improve-OAuth-discovery-logging.patch (8.2K, 2-v5-0001-Improve-OAuth-discovery-logging.patch) download | inline diff: From ef1b27b98fe23df290cf282870d73be36046cf5b Mon Sep 17 00:00:00 2001 From: Zsolt Parragi <[email protected]> Date: Wed, 11 Feb 2026 19:28:05 +0100 Subject: [PATCH v5] Improve OAuth discovery logging Currently when the client sends an empty OAuth token to request the issuer URL, the server logs the attempt with FATAL: OAuth bearer authentication failed for user Which is quite confusing, as this is an expected part of the OAuth authentication flow and not an error at all. This in practice results in the server spamming the log with these messages, which are difficult to separate from real (OAuth) authentication failures. This patch improves this by handling the situation properly in the SASL/Oauth code, by introducing a new SASL authentication status, PG_SASL_EXCHANGE_ABANDONED. The expectation is that authentication mechanisms can set this if the current authentication attempt should be gracefully aandoned, without printing a fatal error about authentication failure. This is currently only used by OAuth, where libpq typically restarts with another, more detailed authentication attempt soon afterwards. --- src/backend/libpq/auth-oauth.c | 40 ++++++++++++------- src/backend/libpq/auth-sasl.c | 14 ++++--- src/include/libpq/sasl.h | 12 +++--- .../modules/oauth_validator/t/001_server.pl | 8 +++- 4 files changed, 47 insertions(+), 27 deletions(-) diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c index 11365048951..39f66aef4d7 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 @@ -193,7 +195,16 @@ oauth_exchange(void *opaq, const char *input, int inputlen, errdetail("Client did not send a kvsep response.")); /* The (failed) handshake is now complete. */ + if (ctx->state == OAUTH_STATE_ERROR_DISCOVERY) + { + ctx->state = OAUTH_STATE_FINISHED; + ereport(DEBUG1, + errmsg("OAuth issuer discovery requested")); + return PG_SASL_EXCHANGE_ABANDONED; + } + ctx->state = OAUTH_STATE_FINISHED; + return PG_SASL_EXCHANGE_FAILURE; default: @@ -279,7 +290,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 +587,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/backend/libpq/auth-sasl.c b/src/backend/libpq/auth-sasl.c index 36cb748d927..62bb06b579a 100644 --- a/src/backend/libpq/auth-sasl.c +++ b/src/backend/libpq/auth-sasl.c @@ -167,7 +167,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,11 +184,13 @@ CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass, } } while (result == PG_SASL_EXCHANGE_CONTINUE); - /* Oops, Something bad happened */ - if (result != PG_SASL_EXCHANGE_SUCCESS) + switch (result) { - return STATUS_ERROR; + case PG_SASL_EXCHANGE_SUCCESS: + return STATUS_OK; + case PG_SASL_EXCHANGE_ABANDONED: + return STATUS_EOF; + default: + return STATUS_ERROR; } - - return STATUS_OK; } diff --git a/src/include/libpq/sasl.h b/src/include/libpq/sasl.h index 1e8ec7d6293..130e02337f7 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, diff --git a/src/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl index 6b649c0b06f..9f2f9f49077 100644 --- a/src/test/modules/oauth_validator/t/001_server.pl +++ b/src/test/modules/oauth_validator/t/001_server.pl @@ -114,11 +114,13 @@ $node->connect_ok( expected_stderr => qr@Visit https://example\.com/ and enter the code: postgresuser@, log_like => [ + qr/OAuth issuer discovery requested/, qr/oauth_validator: token="9243959234", role="$user"/, 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. @@ -129,11 +131,13 @@ $node->connect_ok( expected_stderr => qr@Visit https://example\.org/ and enter the code: postgresuser@, log_like => [ + qr/OAuth issuer discovery requested/, qr/oauth_validator: token="9243959234-alt", role="$user"/, 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.43.0 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-04 19:37 Jacob Champion <[email protected]> parent: Zsolt Parragi <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Jacob Champion @ 2026-03-04 19:37 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]> On Fri, Feb 27, 2026 at 11:51 AM Zsolt Parragi <[email protected]> wrote: > Thanks for the comment suggestions, I attached an updated version with > an edited comment for exchange(). Cirrus seems to have noticed an intermittent failure [1]; what's that about? --Jacob [1] https://cirrus-ci.com/task/5855947479842816 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-04 20:40 Zsolt Parragi <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Zsolt Parragi @ 2026-03-04 20:40 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]> > Cirrus seems to have noticed an intermittent failure [1]; what's that about? Looks like we have some out of order logging, because of the multiple backends involved. DISCOVERY 2026-02-27 20:11:12.104 UTC postmaster[43175] DEBUG: forked new client backend, pid=43267 socket=7 DISCOVERY 2026-02-27 20:11:12.118 UTC client backend[43267] [unknown] LOG: connection received: host=[local] 2026-02-27 20:11:12.179 UTC postmaster[43175] DEBUG: assigned pm child slot 3 for client backend 2026-02-27 20:11:12.179 UTC postmaster[43175] DEBUG: forked new client backend, pid=43281 socket=7 LOGIN 2026-02-27 20:11:12.179 UTC client backend[43281] [unknown] LOG: connection received: host=[local] LOGIN 2026-02-27 20:11:12.180 UTC client backend[43281] [unknown] LOG: oauth_validator: token="9243959234", role="test" LOGIN 2026-02-27 20:11:12.180 UTC client backend[43281] [unknown] LOG: oauth_validator: issuer="http://127.0.0.1:44122";, scope="openid postgres" LOGIN 2026-02-27 20:11:12.180 UTC client backend[43281] [unknown] LOG: connection authenticated: identity="test" method=oauth (/tmp/cirrus-ci-build/build/testrun/oauth_validator/001_server/data/t_001_server_primary_data/pgdata/pg_hba.conf:2) LOGIN 2026-02-27 20:11:12.180 UTC client backend[43281] [unknown] LOG: connection authorized: user=test database=postgres application_name=001_server.pl LOGIN 2026-02-27 20:11:12.180 UTC client backend[43281] 001_server.pl LOG: connection ready: setup total=1.804 ms, fork=0.424 ms, authentication=0.287 ms LOGIN 2026-02-27 20:11:12.181 UTC client backend[43281] 001_server.pl LOG: statement: SELECT $$connected with user=test dbname=postgres oauth_issuer=http://127.0.0.1:44122 oauth_client_id=f02c6361-0635$$ ... we read the log around this part ... DISCOVERY 2026-02-27 20:11:12.189 UTC client backend[43267] [unknown] DEBUG: OAuth issuer discovery requested 2026-02-27 20:11:12.190 UTC postmaster[43175] DEBUG: releasing pm child slot 1 DISCOVERY 2026-02-27 20:11:12.190 UTC postmaster[43175] DEBUG: client backend (PID 43267) exited with exit code 0 So it's a scheduling issue, since we log the "oauth discovery requested" after we already sent the issuer info to the client, so the other connection attempt can already be in progress (at least with simple setups like this test) Also related that connect_fails uses wait_for_log, noticing log messages that appear later, while connect_ok simply checks the logs at that moment. I'm not sure which solution is better for this: removing the check for this message from the test or modifying connect_ok to wait for all backends that started since the connection attempt to finish? Modifying it seems the better choice to me, but that is also a separate change. ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-04 22:18 Jacob Champion <[email protected]> parent: Zsolt Parragi <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Jacob Champion @ 2026-03-04 22:18 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> [cc'ing Tom for awareness] On Wed, Mar 4, 2026 at 12:40 PM Zsolt Parragi <[email protected]> wrote: > Looks like we have some out of order logging, because of the multiple > backends involved. Ugh. In retrospect, then, my commit ab8af1db4 was probably useless. It doesn't improve any existing usage and it can't help future usage. > I'm not sure which solution is better for this: removing the check for > this message from the test The log_unlike() part is the important bit, so that would be my preference. Unfortunately that means the intermittent false failure becomes an intermittent false success, but unless I'm missing something, we might not be able to do better with the current tools. > or modifying connect_ok Well, connect_fails() would seem to suffer the same problem, right? Tom's solution in e0f373ee4 was never meant to handle concurrent backends. I hadn't really considered that all "normal" OAuth connections can have two concurrent backends in practice. (I think of them as serial, but they're not.) We're just getting lucky that we haven't made use of log_[un]like in many problematic cases yet. > to wait for all > backends that started since the connection attempt to finish? Easier said than done, I think. I've wanted to teach the server how to bracket logs of interest for testing purposes for a while now; I don't mind using this as a catalyst. But I don't think it should be done as part of this thread. Thanks, --Jacob ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-05 20:11 Zsolt Parragi <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Zsolt Parragi @ 2026-03-05 20:11 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> Attached v6 with the problematic log expectation removed. > Easier said than done, I think. I've wanted to teach the server how to > bracket logs of interest for testing purposes for a while now; I don't > mind using this as a catalyst. But I don't think it should be done as > part of this thread. Yes, I want to look at how to make connect_ok/connect_fails more reliable for oidc scenarios, but let's not make this dependent on that, my first idea to do it wasn't 100% reliable based on some targeted testing. Attachments: [application/octet-stream] v6-0001-Improve-OAuth-discovery-logging.patch (7.8K, 2-v6-0001-Improve-OAuth-discovery-logging.patch) download | inline diff: From aeba608752024ff924aa9e5af7e2c3d8a135ca78 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi <[email protected]> Date: Wed, 11 Feb 2026 19:28:05 +0100 Subject: [PATCH v6] Improve OAuth discovery logging Currently when the client sends an empty OAuth token to request the issuer URL, the server logs the attempt with FATAL: OAuth bearer authentication failed for user Which is quite confusing, as this is an expected part of the OAuth authentication flow and not an error at all. This in practice results in the server spamming the log with these messages, which are difficult to separate from real (OAuth) authentication failures. This patch improves this by handling the situation properly in the SASL/Oauth code, by introducing a new SASL authentication status, PG_SASL_EXCHANGE_ABANDONED. The expectation is that authentication mechanisms can set this if the current authentication attempt should be gracefully aandoned, without printing a fatal error about authentication failure. This is currently only used by OAuth, where libpq typically restarts with another, more detailed authentication attempt soon afterwards. --- src/backend/libpq/auth-oauth.c | 40 ++++++++++++------- src/backend/libpq/auth-sasl.c | 14 ++++--- src/include/libpq/sasl.h | 12 +++--- .../modules/oauth_validator/t/001_server.pl | 6 ++- 4 files changed, 45 insertions(+), 27 deletions(-) diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c index 11365048951..39f66aef4d7 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 @@ -193,7 +195,16 @@ oauth_exchange(void *opaq, const char *input, int inputlen, errdetail("Client did not send a kvsep response.")); /* The (failed) handshake is now complete. */ + if (ctx->state == OAUTH_STATE_ERROR_DISCOVERY) + { + ctx->state = OAUTH_STATE_FINISHED; + ereport(DEBUG1, + errmsg("OAuth issuer discovery requested")); + return PG_SASL_EXCHANGE_ABANDONED; + } + ctx->state = OAUTH_STATE_FINISHED; + return PG_SASL_EXCHANGE_FAILURE; default: @@ -279,7 +290,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 +587,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/backend/libpq/auth-sasl.c b/src/backend/libpq/auth-sasl.c index 36cb748d927..62bb06b579a 100644 --- a/src/backend/libpq/auth-sasl.c +++ b/src/backend/libpq/auth-sasl.c @@ -167,7 +167,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,11 +184,13 @@ CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass, } } while (result == PG_SASL_EXCHANGE_CONTINUE); - /* Oops, Something bad happened */ - if (result != PG_SASL_EXCHANGE_SUCCESS) + switch (result) { - return STATUS_ERROR; + case PG_SASL_EXCHANGE_SUCCESS: + return STATUS_OK; + case PG_SASL_EXCHANGE_ABANDONED: + return STATUS_EOF; + default: + return STATUS_ERROR; } - - return STATUS_OK; } diff --git a/src/include/libpq/sasl.h b/src/include/libpq/sasl.h index 1e8ec7d6293..130e02337f7 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, diff --git a/src/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl index 6b649c0b06f..0e3ae599352 100644 --- a/src/test/modules/oauth_validator/t/001_server.pl +++ b/src/test/modules/oauth_validator/t/001_server.pl @@ -118,7 +118,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. @@ -133,7 +134,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.43.0 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-10 21:52 Jacob Champion <[email protected]> parent: Zsolt Parragi <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Jacob Champion @ 2026-03-10 21:52 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> On Thu, Mar 5, 2026 at 12:11 PM Zsolt Parragi <[email protected]> wrote: > Attached v6 with the problematic log expectation removed. Okay, I was doing some final pre-commit review today and... unfortunately, using STATUS_EOF like my "TODO" suggested breaks our de facto SASL profile. The server hasn't completed its side of the exchange until it sends either [AuthenticationSASLFinal+]AuthenticationOk or ErrorResponse. Since STATUS_EOF suppresses not only the log message but the entire ereport(FATAL), we'll never send that last packet, so a more polite client can't tell whether the server finished the exchange or just crashed. v6 doesn't fail any tests because of a shortcut I took in PQconnectPoll() in libpq, which skips reading the final message from a known-doomed OAuth discovery connection. But you can see it if you apply the attached patch. (It's not a correct patch; it just shows the problem.) I'm experimenting with an ereport(FATAL_CLIENT_ONLY) option, in the same vein as WARNING_CLIENT_ONLY, to try to cover this. --Jacob P.S. I would eventually like to record our undocumented SASL profile in a test suite (he said, staring at pg-pytest)... Attachments: [application/octet-stream] nocfbot.polite.diff (1.9K, 2-nocfbot.polite.diff) download | inline diff: commit f21c1b8aecc2a7e8baa1b1b004055eabb057403b Author: Jacob Champion <[email protected]> Date: Tue Mar 10 12:58:05 2026 XXX polite libpq diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c index 2aef327c68b..562c8595c32 100644 --- a/src/interfaces/libpq/fe-auth-oauth.c +++ b/src/interfaces/libpq/fe-auth-oauth.c @@ -1400,7 +1400,7 @@ reconnect: */ libpq_append_conn_error(conn, "retrying connection with new bearer token"); conn->oauth_want_retry = true; - return SASL_FAILED; + return SASL_COMPLETE; } static bool diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index db9b4c8edbf..151d4096f32 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -4130,6 +4130,17 @@ keep_going: /* We will come back to here until there is /* Handle errors. */ if (beresp == PqMsg_ErrorResponse) { + /* + * OAuth connections may perform two-step discovery, where + * the first connection is a dummy. We expect a failure + * message to end the SASL exchange from the server side. + */ + if (conn->sasl == &pg_oauth_mech && conn->oauth_want_retry) + { + need_new_connection = true; + goto keep_going; + } + if (pqGetErrorNotice3(conn, true)) { libpq_append_conn_error(conn, "received invalid error message"); @@ -4243,19 +4254,7 @@ keep_going: /* We will come back to here until there is conn->inStart = conn->inCursor; if (res != STATUS_OK) - { - /* - * OAuth connections may perform two-step discovery, where - * the first connection is a dummy. - */ - if (conn->sasl == &pg_oauth_mech && conn->oauth_want_retry) - { - need_new_connection = true; - goto keep_going; - } - goto error_return; - } /* * Just make sure that any data sent by pg_fe_sendauth is ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-16 06:24 Zsolt Parragi <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Zsolt Parragi @ 2026-03-16 06:24 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> > I'm experimenting with an ereport(FATAL_CLIENT_ONLY) option, in the > same vein as WARNING_CLIENT_ONLY, to try to cover this. I attached v7 that uses that and removes the abandoned handling as it is no longer needed with it. > P.S. I would eventually like to record our undocumented SASL profile > in a test suite (he said, staring at pg-pytest)... That would be definitely useful, with the todo comment and this not being documented I thought that this is a proper way to handle the issue. Even a proper documentation about it would be a good starting point. Attachments: [application/octet-stream] v7-0001-Improve-OAuth-discovery-logging.patch (7.0K, 2-v7-0001-Improve-OAuth-discovery-logging.patch) download | inline diff: From 5e7887e7063b8d44303a118b90525c689d86082c Mon Sep 17 00:00:00 2001 From: Zsolt Parragi <[email protected]> Date: Wed, 11 Feb 2026 19:28:05 +0100 Subject: [PATCH v7] Improve OAuth discovery logging Currently when the client sends an empty OAuth token to request the issuer URL, the server logs the attempt with FATAL: OAuth bearer authentication failed for user Which is quite confusing, as this is an expected part of the OAuth authentication flow and not an error at all. This in practice results in the server spamming the log with these messages, which are difficult to separate from real (OAuth) authentication failures. This patch introduces FATAL_CLIENT_ONLY, a new ereport level analogous to WARNING_CLIENT_ONLY. It sends the ErrorResponse to the client (properly completing the SASL exchange per protocol requirements) but suppresses the message from the server log. The OAuth discovery handler uses this to terminate the discovery exchange cleanly: the client receives the expected ErrorResponse, but no misleading FATAL entry appears in the server log. --- src/backend/libpq/auth-oauth.c | 42 ++++++++++++------- src/backend/utils/error/elog.c | 7 +++- src/include/utils/elog.h | 4 +- .../modules/oauth_validator/t/001_server.pl | 6 ++- 4 files changed, 40 insertions(+), 19 deletions(-) diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c index 11365048951..319dbc5bbbc 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 @@ -193,7 +195,18 @@ oauth_exchange(void *opaq, const char *input, int inputlen, errdetail("Client did not send a kvsep response.")); /* The (failed) handshake is now complete. */ + if (ctx->state == OAUTH_STATE_ERROR_DISCOVERY) + { + ctx->state = OAUTH_STATE_FINISHED; + ereport(FATAL_CLIENT_ONLY, + errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), + errmsg("OAuth bearer authentication failed for user \"%s\"", + ctx->port->user_name), + errdetail("Empty request, discovery requested?")); + } + ctx->state = OAUTH_STATE_FINISHED; + return PG_SASL_EXCHANGE_FAILURE; default: @@ -279,7 +292,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 +589,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/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index 0d0bf0f6aa5..2727d790e32 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. @@ -2966,6 +2966,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; @@ -3797,6 +3798,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: @@ -4179,6 +4181,7 @@ error_severity(int elevel) prefix = gettext_noop("ERROR"); break; case FATAL: + case FATAL_CLIENT_ONLY: prefix = gettext_noop("FATAL"); break; case PANIC: diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index a12b379e09a..13e664e8210 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -53,7 +53,9 @@ 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 error sent to the client, but never + * to the server log. */ +#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/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl index 6b649c0b06f..0e3ae599352 100644 --- a/src/test/modules/oauth_validator/t/001_server.pl +++ b/src/test/modules/oauth_validator/t/001_server.pl @@ -118,7 +118,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. @@ -133,7 +134,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.43.0 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-16 07:10 Andrey Borodin <[email protected]> parent: Zsolt Parragi <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Andrey Borodin @ 2026-03-16 07:10 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Jacob Champion <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> > On 16 Mar 2026, at 11:24, Zsolt Parragi <[email protected]> wrote: > >> I'm experimenting with an ereport(FATAL_CLIENT_ONLY) option, in the >> same vein as WARNING_CLIENT_ONLY, to try to cover this. > > I attached v7 that uses that and removes the abandoned handling as it > is no longer needed with it. > >> P.S. I would eventually like to record our undocumented SASL profile >> in a test suite (he said, staring at pg-pytest)... > > That would be definitely useful, with the todo comment and this not > being documented I thought that this is a proper way to handle the > issue. Even a proper documentation about it would be a good starting > point. > <v7-0001-Improve-OAuth-discovery-logging.patch> I've took a look into v7. FATAL_CLIENT_ONLY approach LGTM. pg_stat_database.sessions_fatal seems to be still incremented, but, probably, we can live with it. But also we can fix it. Changes to send_message_to_server_log() seems unreachable to me. I think is_log_level_output() returns false for FATAL_CLIENT_ONLY, so edata->output_to_server is never set to true for this level, and these functions are never called. FATAL_CLIENT_ONLY = 23 sits between FATAL (22) and PANIC (24). Consider swapping FATAL and FATAL_CLIENT_ONLY, so that code like this will have more sense: elevel = Max(elevel, errordata[i].elevel); Does this assignment have an effect? + ctx->state = OAUTH_STATE_FINISHED; + ereport(FATAL_CLIENT_ONLY, Thanks! Best regards, Andrey Borodin. ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-16 07:32 Zsolt Parragi <[email protected]> parent: Andrey Borodin <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Zsolt Parragi @ 2026-03-16 07:32 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; +Cc: Jacob Champion <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> Thanks for the quick review! > pg_stat_database.sessions_fatal seems to be still incremented, but, probably, > we can live with it. But also we can fix it. We are still doing a fatal disconnect, so it seems appropriate to me? > Changes to send_message_to_server_log() > ... > FATAL_CLIENT_ONLY = 23 sits between FATAL (22) and PANIC (24). I handled these the same way as the existing WARNING_CLIENT_ONLY. We can change it, but then we probably should also update the warning case. > Does this assignment have an effect? No, but that's also true for the other already existing assignment in this branch, I think these are mostly there for internal bookkeeping/consistency? ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-16 18:14 Jacob Champion <[email protected]> parent: Zsolt Parragi <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Jacob Champion @ 2026-03-16 18:14 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> On Mon, Mar 16, 2026 at 1:30 AM Andrey Borodin <[email protected]> wrote: > I have no more comments about the patch, feel free to flip it to RfC. Thanks all, v7 looks very similar to one of my local patches. I don't want to escape the authentication flow from inside a SASL mech, though (it's unusual/invisible to other maintainers, plus it bypasses the ClientAuthentication_hook). 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. Should have something posted today if things go my way. --Jacob ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-16 19:45 Zsolt Parragi <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Zsolt Parragi @ 2026-03-16 19:45 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> > I don't > want to escape the authentication flow from inside a SASL mech, though > (it's unusual/invisible to other maintainers, plus it bypasses the > ClientAuthentication_hook). 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? I didn't see any clear pattern, for example the LDAP code clearly uses ereport(LOG, ...); return STATUS_ERROR; even for internal/configuration errors, while the scram/sasl code uses ereport(ERROR, ...) for those errors. ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-17 00:24 Jacob Champion <[email protected]> parent: Zsolt Parragi <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Jacob Champion @ 2026-03-17 00:24 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Chao Li <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> 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 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-17 05:29 Zsolt Parragi <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 2 replies; 26+ messages in thread From: Zsolt Parragi @ 2026-03-17 05:29 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Jacob Champion <[email protected]>; Andrey Borodin <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> > As is_log_level_output() returns false against FATAL_CLIENT_ONLY, so that FATAL_CLIENT_ONLY should not reach send_message_to_server_log(). Should we assert edata->elevel != FATAL_CLIENT_ONLY? Andrey asked the same question upthread, this mirrors how WARNING_CLIENT_ONLY is implemented. > As far as I understand, for a programmer error, Assert should be used. Why do we use elog(ERROR) here? +1, 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. So the assert/if should be at the beginning of the function, not in the error path. Or instead: + /* + * "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; + 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. ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-17 15:38 Jacob Champion <[email protected]> parent: Zsolt Parragi <[email protected]> 1 sibling, 0 replies; 26+ messages in thread From: Jacob Champion @ 2026-03-17 15:38 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; Andrey Borodin <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> On Mon, Mar 16, 2026 at 11:19 PM Chao Li <[email protected]> wrote: > Do you mean that we do the same as WARNING_CLIENT_ONLY in this patch, and use a separate patch to fix them together? I'm not sure I want to fix it at all; it keeps the code coherent even if someone later decides they really want to override the CLIENT_ONLY directive for some reason. On the WARNING_CLIENT_ONLY thread [1], Andres said > I don't think it needs to be done right now, but I again want to suggest > it'd be nice if we split log levels into a bitmask. If we bits, separate > from the log level, for do-not-log-to-client and do-not-log-to-server > some of this code would imo look nicer. and I think I agree that would be a good way for future improvement. --Jacob [1] https://postgr.es/m/20201228191428.p5bhcvd4ixsuyifd%40alap3.anarazel.de ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-17 16:25 Jacob Champion <[email protected]> parent: Zsolt Parragi <[email protected]> 1 sibling, 1 reply; 26+ messages in thread From: Jacob Champion @ 2026-03-17 16:25 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Chao Li <[email protected]>; Andrey Borodin <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> On Mon, Mar 16, 2026 at 10:29 PM Zsolt Parragi <[email protected]> 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 ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-17 21:19 Zsolt Parragi <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Zsolt Parragi @ 2026-03-17 21:19 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Chao Li <[email protected]>; Andrey Borodin <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> > That's not really true, because the caller hardcodes the mechanism > descriptor. I meant that the caller shouldn't depend on the implementation details of the mechanism. The abandoned comment says that '"Abandoned" is a SASL-specific state similar to STATUS_EOF ...', yet later it also depends on an implementation detail of which sasl mechanism actually use it. > (If more things than OAuth need this eventually, maybe it becomes > STATUS_SILENT_ERROR or something, to make it even more generic?) That's a good idea, better than my error level suggestion. The code would actually shorter, because you could remove the programmer error check from CheckSASLAuth. The diff also, because it would work without modifying the calls to it. The patch is also good as-is, all these comments in the last few messages are just very minor details, I probably spent way too much time thinging about how to make this not oauth specific in the generic part of the code. ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-20 18:14 Jacob Champion <[email protected]> parent: Zsolt Parragi <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Jacob Champion @ 2026-03-20 18:14 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: Chao Li <[email protected]>; Andrey Borodin <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Michael Paquier <[email protected]>; Tom Lane <[email protected]> On Tue, Mar 17, 2026 at 2:19 PM Zsolt Parragi <[email protected]> wrote: > > That's not really true, because the caller hardcodes the mechanism > > descriptor. > > I meant that the caller shouldn't depend on the implementation details > of the mechanism. The abandoned comment says that '"Abandoned" is a > SASL-specific state similar to STATUS_EOF ...', yet later it also > depends on an implementation detail of which sasl mechanism actually > use it. I don't disagree, I'm just trying to point out that this coupling is already part of CheckSASLAuth. See e.g. the handling of shadow_pass. (I'm not very worried about this, because we're free to improve this API at any time, and there are only two callers. Michael was very receptive to prefactoring patches here prior to the addition of OAUTHBEARER, and I expect we'll continue to refactor it if/when more mechanisms show up. It's just hard to pull a general interface out of two mechanisms as dissimilar as SCRAM and OAuth.) > The patch is also good as-is, all these comments in the last few > messages are just very minor details, I probably spent way too much > time thinging about how to make this not oauth specific in the generic > part of the code. I appreciate the review! I'm not in a rush to get this patch pushed, and I want to give Michael ample time to weigh in. (Personally, I don't think anyone is likely to argue against the behavior change here, only against how it's being done. We have alternative implementations available if there are strong opinions late in the cycle. So I feel pretty confident we can land a fix for 19.) Thanks, --Jacob ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Improve OAuth discovery logging @ 2026-03-31 18:51 Jacob Champion <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Jacob Champion @ 2026-03-31 18:51 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Chao Li <[email protected]>; Andrey Borodin <[email protected]>; Zsolt Parragi <[email protected]>; Daniel Gustafsson <[email protected]>; pgsql-hackers; Tom Lane <[email protected]> On Fri, Mar 20, 2026 at 11:14 AM Jacob Champion <[email protected]> wrote: > I'm not in a rush to get this patch pushed, and I want to give Michael > ample time to weigh in. (Personally, I don't think anyone is likely to > argue against the behavior change here, only against how it's being > done. We have alternative implementations available if there are > strong opinions late in the cycle. So I feel pretty confident we can > land a fix for 19.) Pushed (but post-commit review is very welcome :D). --Jacob ^ permalink raw reply [nested|flat] 26+ messages in thread
end of thread, other threads:[~2026-03-31 18:51 UTC | newest] Thread overview: 26+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-02-12 18:51 Re: Improve OAuth discovery logging Zsolt Parragi <[email protected]> 2026-02-12 20:30 ` Jacob Champion <[email protected]> 2026-02-13 13:13 ` Zsolt Parragi <[email protected]> 2026-02-24 03:00 ` Chao Li <[email protected]> 2026-02-24 23:24 ` Jacob Champion <[email protected]> 2026-02-25 13:14 ` Zsolt Parragi <[email protected]> 2026-02-26 05:51 ` Andrey Borodin <[email protected]> 2026-02-27 19:51 ` Zsolt Parragi <[email protected]> 2026-03-04 19:37 ` Jacob Champion <[email protected]> 2026-03-04 20:40 ` Zsolt Parragi <[email protected]> 2026-03-04 22:18 ` Jacob Champion <[email protected]> 2026-03-05 20:11 ` Zsolt Parragi <[email protected]> 2026-03-10 21:52 ` Jacob Champion <[email protected]> 2026-03-16 06:24 ` Zsolt Parragi <[email protected]> 2026-03-16 07:10 ` Andrey Borodin <[email protected]> 2026-03-16 07:32 ` Zsolt Parragi <[email protected]> 2026-03-16 18:14 ` Jacob Champion <[email protected]> 2026-03-16 19:45 ` Zsolt Parragi <[email protected]> 2026-03-17 00:24 ` Jacob Champion <[email protected]> 2026-03-17 05:29 ` Zsolt Parragi <[email protected]> 2026-03-17 15:38 ` Jacob Champion <[email protected]> 2026-03-17 16:25 ` Jacob Champion <[email protected]> 2026-03-17 21:19 ` Zsolt Parragi <[email protected]> 2026-03-20 18:14 ` Jacob Champion <[email protected]> 2026-03-31 18:51 ` Jacob Champion <[email protected]> 2026-02-26 16:17 ` Jacob Champion <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox