From aeba608752024ff924aa9e5af7e2c3d8a135ca78 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi 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