Обсуждение: unclear OAuth error message
Hello,
While updating the translation, I noticed this code
/*
* Log any authentication results even if the token isn't authorized; it
* might be useful for auditing or troubleshooting.
*/
if (ret->authn_id)
set_authn_id(port, ret->authn_id);
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."));
status = false;
goto cleanup;
}
I'm not sure I understand the errdetail() part of it. At first it made
me wonder if it was about a user-supplied module that had an internal
failure preventing it from deciding whether the user was authorized or
not (which would have been something like "Validator failed while ...").
But the code suggests that the module worked fine and made the
determination not to authorize the user. If that's so, then why do we
have the errdetail at all? Can't we just get rid of it and let the
errmsg stand on its own merit?
There is one more case for this exact errmsg to be given:
/* 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."));
Here it seems the validator did indeed have an internal problem of some
sort, because while it did declare that the user was authorized, it did
not provide what we were expecting from it. Should in this case the
errmsg() be different?
(Actually, there's also auth_failed() giving the same message.)
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"The saddest aspect of life right now is that science gathers knowledge faster
than society gathers wisdom." (Isaac Asimov)
> But the code suggests that the module worked fine and made the > determination not to authorize the user. If that's so, then why do we > have the errdetail at all? Can't we just get rid of it and let the > errmsg stand on its own merit? This also confused me when I worked on our validator plugin. But the validator has two places to return failure: * res->authorized, which should be true if we are allowed to continue with the login, false if not * and the return value, which should be true if the validation process completed successfully, false if not At least that's how I understand it. This check is about the first one, which means there wasn't any problem during the validation process, but the validator decided not to allow the login to proceed. I also want to add that these error messages are not that useful for figuring out what went wrong. In practice, the validator has to report more specific error messages before this happens, otherwise the user won't be able to figure out why we are rejecting some login. (both for internal errors and rejecting authentication) And if it reports an error, and stops the code flow, these won't be displayed at all - but then why do we have all these bool outputs in the validator? And even the messages in the caller code are WARNING/LOG. The intention of the API seems to be to report WARNINGs for validation failures, even internal errors, so for now we added most error checks in the validator as that, allowing the code to continue.
On Sat, Jan 24, 2026 at 6:50 AM Álvaro Herrera <alvherre@kurilemu.de> wrote: > But the code suggests that the module worked fine and made the > determination not to authorize the user. If that's so, then why do we > have the errdetail at all? Can't we just get rid of it and let the > errmsg stand on its own merit? For that code path I suspect we could get rid of the entire message, because of what you mentioned later: auth_failed() is already going to give us that. The validator can log what's important if needed, or not. We could add some DEBUGs, maybe, so that you can still figure out what's going on if a validator fails silently? > Here it seems the validator did indeed have an internal problem of some > sort, because while it did declare that the user was authorized, it did > not provide what we were expecting from it. Should in this case the > errmsg() be different? Yeah, I think so. The errdetail should probably become the errmsg, essentially (but with more context). Thanks, --Jacob
On Mon, Jan 26, 2026 at 12:53 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote: > I also want to add that these error messages are not that useful for > figuring out what went wrong. In practice, the validator has to report > more specific error messages before this happens, otherwise the user > won't be able to figure out why we are rejecting some login. Right. > And if it reports an error, and stops the code flow, these won't be > displayed at all - but then why do we have all these bool outputs in > the validator? In general, don't stop the code flow. We want failures to be handled in-band [1] so that the server's communication with an unauthenticated client is standard across validators and auth methods. (Our SASL abstraction layer is leaky here, in the sense that mechanisms have too much power over the network layer.) And in general, we want to avoid leaking token information to unauthenticated clients. The server should not act as an oracle for people with ill-gotten tokens. > And even the messages in the caller code are > WARNING/LOG. The intention of the API seems to be to report WARNINGs > for validation failures, even internal errors, so for now we added > most error checks in the validator as that, allowing the code to > continue. COMMERROR (LOG_SERVER_ONLY), which is also documented in [1], is a good bet. WARNING is fine only in the sense that it's not emitted during authentication, but it'd be odd to see a warning for a run-of-the-mill validation failure. (For internal errors, like "I couldn't get the public keys", I think WARNING probably makes sense. Or if something causes you to think that somebody is under active attack.) --Jacob [1] https://www.postgresql.org/docs/18/oauth-validator-design.html#OAUTH-VALIDATOR-DESIGN-GUIDELINES