diff --git a/doc/src/sgml/oauth-validators.sgml b/doc/src/sgml/oauth-validators.sgml index 704089dd7b3..5f29f2be186 100644 --- a/doc/src/sgml/oauth-validators.sgml +++ b/doc/src/sgml/oauth-validators.sgml @@ -192,11 +192,20 @@ Logging - Modules may use the same logging + To simply log the reason for a validation failure, modules may set the + freeform error_detail field during the + validate callback. + ( has guidelines for writing good + DETAIL messages.) error_detail + 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 +379,7 @@ typedef struct ValidatorModuleResult { bool authorized; char *authn_id; + char *error_detail; } ValidatorModuleResult; @@ -387,6 +397,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/backend/libpq/auth-oauth.c b/src/backend/libpq/auth-oauth.c index 894efe3c904..6a75b79efbf 100644 --- a/src/backend/libpq/auth-oauth.c +++ b/src/backend/libpq/auth-oauth.c @@ -74,7 +74,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 */ @@ -305,7 +305,7 @@ oauth_exchange(void *opaq, const char *input, int inputlen, ctx->state = OAUTH_STATE_ERROR_DISCOVERY; status = PG_SASL_EXCHANGE_CONTINUE; } - else if (!validate(ctx->port, auth)) + else if (!validate(ctx->port, auth, logdetail)) { generate_error_response(ctx, output, outputlen); @@ -650,7 +650,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; @@ -677,7 +677,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; } @@ -690,10 +693,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; @@ -714,10 +717,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 fdacc060381..47b5eeb8f22 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -639,7 +639,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, &abandoned); break; } 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/test/modules/oauth_validator/t/001_server.pl b/src/test/modules/oauth_validator/t/001_server.pl index c9c46e63539..ac62555675a 100644 --- a/src/test/modules/oauth_validator/t/001_server.pl +++ b/src/test/modules/oauth_validator/t/001_server.pl @@ -544,8 +544,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 @@ -564,10 +564,48 @@ $node->connect_fails( expected_stderr => qr/OAuth bearer authentication failed/, log_like => [ qr/connection authenticated: identity="test\@example\.org"/, - qr/DETAIL:\s+Validator failed to authorize the provided token/, 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);