Обсуждение: 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

On Mon, Jan 26, 2026 at 2:17 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> 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?

I only just remembered that this is exactly what logdetail is designed
to do. It's passed in from CheckSASLAuth, but OAuth doesn't make use
of it. (My original patchset carried a TODO for this for a long time,
but I lost it at some point...)

I have a parallel patchset that also needs logdetail, so this fix can
piggyback on some of that work.

--Jacob

On Wed, Mar 18, 2026 at 9:47 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> I have a parallel patchset that also needs logdetail, so this fix can
> piggyback on some of that work.

Here is (the confusingly named) v3-0001, which I hope fixes both
upthread complaints. It comes from [1].

--Jacob

[1] https://postgr.es/m/CAOYmi%2BnTXGcroZD_Mnkc8LYWYFbfDYNR4ML_yQ5sF9%2BDY2amcg%40mail.gmail.com

This is definitely a nice improvement, I only have two minor questions:

- errmsg("internal error in OAuth validator module"));
+ errmsg("internal error in OAuth validator module"),
+ ret->error_detail ? errdetail_log("%s", ret->error_detail) : 0);
+

Isn't including the detail for both the warning and the fatal error
still overly verbose?

+ res->error_detail = error_detail; /* only relevant for failures */
+ if (internal_error)
+ return false;
+

Shouldn't the oauth code include a sanity check to ensure validators
return no error_detail on success instead of silently ignoring it?

> On Mar 24, 2026, at 05:21, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
>
> This is definitely a nice improvement, I only have two minor questions:
>
> - errmsg("internal error in OAuth validator module"));
> + errmsg("internal error in OAuth validator module"),
> + ret->error_detail ? errdetail_log("%s", ret->error_detail) : 0);
> +
>
> Isn't including the detail for both the warning and the fatal error
> still overly verbose?
>

I have the same feeling. If the detail has already been printed together with the WARNING, then maybe there is no
longermuch need to pass it out through logdetail. 

I also have a small comment on the doc change. The documentation already mentions the memory requirements for
error_detail,so I wonder if it also makes sense to mention its style. Since it is emitted as a DETAIL message, I think
itshould probably follow the usual style rules for detail messages, i.e., start with a capital letter and end with a
period.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Mon, Mar 23, 2026 at 2:21 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
> Isn't including the detail for both the warning and the fatal error
> still overly verbose?

I'm not too worried about verbosity for an internal error situation;
users shouldn't see it. If they do, I don't mind being very loud about
whose fault it is.

(I'm also influenced by some recent support work on clusters that have
huge log volumes. If someone is focused on the internal error, they
should be able to see at a glance what caused that error, and if
someone is focused on the authentication failure, they should be able
to see at a glance what caused that. The more logs you have to
correlate in a "help! no one can log in" panic situation, the less
likely you are to succeed.)

> Shouldn't the oauth code include a sanity check to ensure validators
> return no error_detail on success instead of silently ignoring it?

IMO, no. I don't want error_detail to add semantics to the API, just
descriptive power. Plus, I think a design that sets a possible error
message before entering a complex operation, knowing that it will be
ignored on success, is perfectly valid. libpq-oauth, and to a lesser
extent libpq, make use of that pattern.

--Jacob

On Mon, Mar 23, 2026 at 10:18 PM Chao Li <li.evan.chao@gmail.com> wrote:
> I also have a small comment on the doc change. The documentation already mentions the memory requirements for
error_detail,so I wonder if it also makes sense to mention its style. Since it is emitted as a DETAIL message, I think
itshould probably follow the usual style rules for detail messages, i.e., start with a capital letter and end with a
period.

Sure, I think it makes sense to add a quick link to our style guide as
part of the "best practices" section.

Thanks,
--Jacob

> On 27 Mar 2026, at 18:01, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
>
> On Mon, Mar 23, 2026 at 2:21 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
>> Isn't including the detail for both the warning and the fatal error
>> still overly verbose?
>
> I'm not too worried about verbosity for an internal error situation;
> users shouldn't see it. If they do, I don't mind being very loud about
> whose fault it is.
>
> (I'm also influenced by some recent support work on clusters that have
> huge log volumes. If someone is focused on the internal error, they
> should be able to see at a glance what caused that error, and if
> someone is focused on the authentication failure, they should be able
> to see at a glance what caused that. The more logs you have to
> correlate in a "help! no one can log in" panic situation, the less
> likely you are to succeed.)

Agreed.

>> Shouldn't the oauth code include a sanity check to ensure validators
>> return no error_detail on success instead of silently ignoring it?
>
> IMO, no. I don't want error_detail to add semantics to the API, just
> descriptive power. Plus, I think a design that sets a possible error
> message before entering a complex operation, knowing that it will be
> ignored on success, is perfectly valid. libpq-oauth, and to a lesser
> extent libpq, make use of that pattern.

Callsites can also clear the error message on success and not even rely on it
being ignored.

+     * This string may be either of static duration or palloc'd.
+     */
+    char       *error_detail;

I'm not a big fan of "either static or allocated" and prefer if we just require
one or the other.  We have this pattern in other places so it's not a blocker
for going it, but.

--
Daniel Gustafsson

On Fri, Mar 27, 2026 at 3:24 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> > IMO, no. I don't want error_detail to add semantics to the API, just
> > descriptive power. Plus, I think a design that sets a possible error
> > message before entering a complex operation, knowing that it will be
> > ignored on success, is perfectly valid. libpq-oauth, and to a lesser
> > extent libpq, make use of that pattern.
>
> Callsites can also clear the error message on success and not even rely on it
> being ignored.

Agreed, but are you saying that as an argument for my approach, or for Zsolt's?

> +        * This string may be either of static duration or palloc'd.
> +        */
> +       char       *error_detail;
>
> I'm not a big fan of "either static or allocated" and prefer if we just require
> one or the other.  We have this pattern in other places so it's not a blocker
> for going it, but.

I don't think I can enforce either choice, though: I pass the
error_detail into the ereport(FATAL), so the process is about to go
down, and I'm never going to pfree() it.

--Jacob

On Fri, Mar 27, 2026 at 4:20 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> I don't think I can enforce either choice, though: I pass the
> error_detail into the ereport(FATAL), so the process is about to go
> down, and I'm never going to pfree() it.

(I also think it's reasonable to do something like

    res->error_detail = "Help! it's again.";

if there's nothing to format, without mandating a superfluous pstrdup().)

v4 rebases, incorporates Chao Li's feedback, rewrites the surrounding
paragraph a bit, and fixes a silly misuse of <xref> when I meant
<link>.

--Jacob

> On 2 Apr 2026, at 00:57, Jacob Champion <jacob.champion@enterprisedb.com> wrote:

> v4 rebases, incorporates Chao Li's feedback, rewrites the surrounding
> paragraph a bit, and fixes a silly misuse of <xref> when I meant
> <link>.

This version LGTM.

--
Daniel Gustafsson