Re: On login trigger: take three

On Wed, Sep 29, 2021 at 10:14 PM Teodor Sigaev <teodor(at)sigaev(dot)ru> wrote:
>
> Nice feature, but, sorry, I see some design problem in suggested feature. AFAIK,
> there is two use cases for this feature:
> 1 A permission / prohibition to login some users
> 2 Just a logging of facts of user's login
>
> Suggested patch proposes prohibition of login only by failing of login trigger
> and it has at least two issues:
> 1 In case of prohibition to login, there is no clean way to store information
> about unsuccessful login. Ok, it could be solved by dblink module but it seems
> to scary.

It's an area that could be improved, but the patch is more intended to
allow additional actions on successful login, as described by the
following (taken from the doc updates in the patch):

+ <para>
+ The event trigger on the <literal>login</literal> event can be
+ useful for logging user logins, for verifying the connection and
+ assigning roles according to current circumstances, or for some
session data
+ initialization.
+ </para>

> 2 For logging purpose failing of trigger will cause impossibility to login, it
> could be workarounded by catching error in trigger function, but it's not so
> obvious for DBA.
>

If you look back on the past discussions on this thread, you'll see
that originally the patch added a GUC for the purpose of allowing
superusers to disable the event trigger, to allow login in this case.
However it was argued that there was already a documented way of
bypassing buggy event triggers (i.e. restart in single-user mode), so
that GUC was removed.
Also previously in the patch, login trigger errors for superusers were
ignored in order to allow them to login in this case, but it was
argued that it could well be unsafe to continue on after an error, so
that too was removed.
See below:

>
> + {"enable_client_connection_trigger", PGC_SU_BACKEND, DEVELOPER_OPTIONS,
> + gettext_noop("Enables the client_connection event trigger."),
> + gettext_noop("In case of errors in the ON client_connection EVENT TRIGGER procedure, "
> ..and..
> + /*
> + * Try to ignore error for superuser to make it possible to login even in case of errors
> + * during trigger execution
> + */
> + if (!is_superuser)
> + PG_RE_THROW();
> This patch adds two ways for superusers to bypass this event trigger in case of
> it being faulty, but for every other event trigger we've documented to restart
> in single-user mode and fixing it there. Why does this need to be different?
> This clearly has a bigger chance of being a footgun but I don't see that as a
> reason to add a GUC and a bypass that other footguns lack.
>

So I really don't think that a failing event trigger will cause an
"impossibility to login".
The patch updates the documentation to explain the use of single-user
mode to fix buggy login event triggers. See below:

+ <para>
+ The <literal>login</literal> event occurs when a user logs in to the
+ system.
+ Any bugs in a trigger procedure for this event may prevent successful
+ login to the system. Such bugs may be fixed after first restarting the
+ system in single-user mode (as event triggers are disabled in this mode).
+ See the <xref linkend="app-postgres"/> reference page for details about
+ using single-user mode.
+ </para>
+

Regards,
Greg Nancarrow
Fujitsu Australia