From f36e6becc34d05834c3d808575b04d39badf7569 Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Thu, 19 Mar 2026 09:37:20 -0700 Subject: [PATCH v3 1/2] oauth: Let validators provide failure DETAILs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment, the only way for a validator module to report error details on failure is to log them separately before returning from validate_cb. Independently of that problem, the ereport() calls that we make during validation failure partially duplicate some of the work of auth_failed(). The end result is overly verbose and confusing for readers of the logs: [768233] LOG: [my_validator] bad signature in bearer token [768233] LOG: OAuth bearer authentication failed for user "jacob" [768233] DETAIL: Validator failed to authorize the provided token. [768233] FATAL: OAuth bearer authentication failed for user "jacob" [768233] DETAIL: Connection matched file ".../pg_hba.conf" line ... Solve both problems by making use of the existing logdetail pointer that's provided by ClientAuthentication. Validator modules may set ValidatorModuleResult->error_detail to override our default generic message. The end result looks something like [242284] FATAL: OAuth bearer authentication failed for user "jacob" [242284] DETAIL: [my_validator] bad signature in bearer token Connection matched file ".../pg_hba.conf" line ... Reported-by: Álvaro Herrera Reported-by: Zsolt Parragi Discussion: https://postgr.es/m/202601241015.y5uvxd7oxnfs%40alvherre.pgsql Discussion: TODO --- doc/src/sgml/oauth-validators.sgml | 21 +++++++++- src/include/libpq/oauth.h | 14 +++++++ src/backend/libpq/auth-oauth.c | 24 +++++------ src/backend/libpq/auth.c | 2 +- .../modules/oauth_validator/t/001_server.pl | 40 ++++++++++++++++++- src/test/modules/oauth_validator/validator.c | 29 ++++++++++++++ 6 files changed, 114 insertions(+), 16 deletions(-) diff --git a/doc/src/sgml/oauth-validators.sgml b/doc/src/sgml/oauth-validators.sgml index 704089dd7b3..a7140eae84e 100644 --- a/doc/src/sgml/oauth-validators.sgml +++ b/doc/src/sgml/oauth-validators.sgml @@ -192,11 +192,18 @@ Logging - Modules may use the same logging + To simply log the reason for a validation failure, validators may set + the freeform error_detail field during the + . This is printed only + to the server log, as part of the final authentication failure message, + and it is not shared with the client. + + + Modules may also use the same logging facilities as standard extensions; however, the rules for emitting log entries to the client are subtly different during the authentication phase of the connection. Generally speaking, modules should log - verification problems at the COMMERROR level and return + problems at the COMMERROR level and return normally, instead of using ERROR/FATAL to unwind the stack, to avoid leaking information to unauthenticated clients. @@ -370,6 +377,7 @@ typedef struct ValidatorModuleResult { bool authorized; char *authn_id; + char *error_detail; } ValidatorModuleResult; @@ -387,6 +395,15 @@ typedef struct ValidatorModuleResult Otherwise the validator should return true to indicate that it has processed the token and made an authorization decision. + + In either failure case (validation error or internal error) the module may + store a user-readable reason for the failure in result->error_detail. + This will be printed to the server logs (not sent to the client) as a + DETAIL entry for the authentication failure. The memory + pointed to by error_detail may be either palloc'd + or of static duration. error_detail is ignored + on success. + The behavior after validate_cb returns depends on the specific HBA setup. Normally, the result->authn_id user diff --git a/src/include/libpq/oauth.h b/src/include/libpq/oauth.h index 4a822e9a1f2..60f493acddd 100644 --- a/src/include/libpq/oauth.h +++ b/src/include/libpq/oauth.h @@ -49,6 +49,20 @@ typedef struct ValidatorModuleResult * delegation. See the validator module documentation for details. */ char *authn_id; + + /* + * When validation fails, this may optionally be set to a string + * containing an explanation for the failure. It will be sent to the + * server log only; it is not provided to the client, and it's ignored if + * validation succeeds. + * + * This description will be attached to the final authentication failure + * message in the logs, as a DETAIL, which may be preferable to separate + * ereport() calls that have to be correlated by the reader. + * + * This string may be either of static duration or palloc'd. + */ + char *error_detail; } ValidatorModuleResult; /* diff --git a/src/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c index 11365048951..6d4be70aada 100644 --- a/src/backend/libpq/auth-oauth.c +++ b/src/backend/libpq/auth-oauth.c @@ -73,7 +73,7 @@ struct oauth_ctx static char *sanitize_char(char c); static char *parse_kvpairs_for_auth(char **input); static void generate_error_response(struct oauth_ctx *ctx, char **output, int *outputlen); -static bool validate(Port *port, const char *auth); +static bool validate(Port *port, const char *auth, const char **logdetail); /* Constants seen in an OAUTHBEARER client initial response. */ #define KVSEP 0x01 /* separator byte for key/value pairs */ @@ -279,7 +279,7 @@ 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 (!validate(ctx->port, auth, logdetail)) { generate_error_response(ctx, output, outputlen); @@ -635,7 +635,7 @@ validate_token_format(const char *header) * authorization. Returns true if validation succeeds. */ static bool -validate(Port *port, const char *auth) +validate(Port *port, const char *auth, const char **logdetail) { int map_status; ValidatorModuleResult *ret; @@ -662,7 +662,10 @@ validate(Port *port, const char *auth) { ereport(WARNING, errcode(ERRCODE_INTERNAL_ERROR), - errmsg("internal error in OAuth validator module")); + errmsg("internal error in OAuth validator module"), + ret->error_detail ? errdetail_log("%s", ret->error_detail) : 0); + + *logdetail = ret->error_detail; return false; } @@ -675,10 +678,10 @@ validate(Port *port, const char *auth) if (!ret->authorized) { - ereport(LOG, - errmsg("OAuth bearer authentication failed for user \"%s\"", - port->user_name), - errdetail_log("Validator failed to authorize the provided token.")); + if (ret->error_detail) + *logdetail = ret->error_detail; + else + *logdetail = _("Validator failed to authorize the provided token."); status = false; goto cleanup; @@ -699,10 +702,7 @@ validate(Port *port, const char *auth) /* Make sure the validator authenticated the user. */ if (ret->authn_id == NULL || ret->authn_id[0] == '\0') { - ereport(LOG, - errmsg("OAuth bearer authentication failed for user \"%s\"", - port->user_name), - errdetail_log("Validator provided no identity.")); + *logdetail = _("Validator provided no identity."); status = false; goto cleanup; diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index e04aa2e68ed..6801ea9c566 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -625,7 +625,7 @@ 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, &logdetail); break; } diff --git a/src/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl index cdad2ae8011..28d123b833e 100644 --- a/src/test/modules/oauth_validator/t/001_server.pl +++ b/src/test/modules/oauth_validator/t/001_server.pl @@ -521,8 +521,8 @@ $node->connect_fails( expected_stderr => qr/OAuth bearer authentication failed/, log_like => [ qr/connection authenticated: identity=""/, - qr/DETAIL:\s+Validator provided no identity/, qr/FATAL:\s+OAuth bearer authentication failed/, + qr/DETAIL:\s+Validator provided no identity/, ]); # Even if a validator authenticates the user, if the token isn't considered @@ -541,10 +541,48 @@ $node->connect_fails( expected_stderr => qr/OAuth bearer authentication failed/, log_like => [ qr/connection authenticated: identity="test\@example\.org"/, + qr/FATAL:\s+OAuth bearer authentication failed/, qr/DETAIL:\s+Validator failed to authorize the provided token/, + ]); + +# Validators can provide their own explanations. +$bgconn->query_safe( + "ALTER SYSTEM SET oauth_validator.error_detail TO 'something failed'"); +$node->reload; +$log_start = + $node->wait_for_log(qr/reloading configuration files/, $log_start); + +$node->connect_fails( + "$common_connstr user=test", + "validator must authorize token explicitly (custom logdetail)", + expected_stderr => qr/OAuth bearer authentication failed/, + log_like => [ + qr/connection authenticated: identity="test\@example\.org"/, qr/FATAL:\s+OAuth bearer authentication failed/, + qr/DETAIL:\s+something failed/, ]); +$bgconn->query_safe( + "ALTER SYSTEM SET oauth_validator.internal_error TO true"); +$node->reload; +$log_start = + $node->wait_for_log(qr/reloading configuration files/, $log_start); + +$node->connect_fails( + "$common_connstr user=test", + "validator internal error (custom logdetail)", + expected_stderr => qr/OAuth bearer authentication failed/, + log_like => [ + qr/WARNING:\s+internal error in OAuth validator module/, + qr/DETAIL:\s+something failed/, + ]); + +$bgconn->query_safe("ALTER SYSTEM RESET oauth_validator.error_detail"); +$bgconn->query_safe("ALTER SYSTEM RESET oauth_validator.internal_error"); +$node->reload; +$log_start = + $node->wait_for_log(qr/reloading configuration files/, $log_start); + # # Test user mapping. # diff --git a/src/test/modules/oauth_validator/validator.c b/src/test/modules/oauth_validator/validator.c index 0b983a9dc8f..353e0e0d32a 100644 --- a/src/test/modules/oauth_validator/validator.c +++ b/src/test/modules/oauth_validator/validator.c @@ -40,6 +40,8 @@ static const OAuthValidatorCallbacks validator_callbacks = { /* GUCs */ static char *authn_id = NULL; static bool authorize_tokens = true; +static char *error_detail = NULL; +static bool internal_error = false; /*--- * Extension entry point. Sets up GUCs for use by tests: @@ -51,6 +53,13 @@ static bool authorize_tokens = true; * - oauth_validator.authorize_tokens * Sets whether to successfully validate incoming * tokens. Defaults to true. + * + * - oauth_validator.error_detail + * Sets an error message to be included as a + * DETAIL on failure. + * + * - oauth_validator.internal_error + * Reports an internal error to the server. */ void _PG_init(void) @@ -71,6 +80,22 @@ _PG_init(void) PGC_SIGHUP, 0, NULL, NULL, NULL); + DefineCustomStringVariable("oauth_validator.error_detail", + "Error message to print during failures", + NULL, + &error_detail, + NULL, + PGC_SIGHUP, + 0, + NULL, NULL, NULL); + DefineCustomBoolVariable("oauth_validator.internal_error", + "Should the validator report an internal error?", + NULL, + &internal_error, + false, + PGC_SIGHUP, + 0, + NULL, NULL, NULL); MarkGUCPrefixReserved("oauth_validator"); } @@ -133,6 +158,10 @@ validate_token(const ValidatorModuleState *state, MyProcPort->hba->oauth_issuer, MyProcPort->hba->oauth_scope); + res->error_detail = error_detail; /* only relevant for failures */ + if (internal_error) + return false; + res->authorized = authorize_tokens; if (authn_id) res->authn_id = pstrdup(authn_id); -- 2.34.1