Re: On login trigger: take three

Hi!

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.
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.

some other issues:
3 It's easy to create security hole, see attachment where non-privileged user
can close access to database even for superuser.
4 Feature is not applicable for logging unsuccessful login with wrong
username/password or not matched by pg_hba.conf. Oracle could operate with such
cases. But I don't think that this issue is a stopper for the patch.

May be, to solve that issues we could introduce return value of trigger and/or
add something like IGNORE ERROR to CREATE EVENT TRIGGER command.

On 15.09.2021 16:32, Greg Nancarrow wrote:
> On Wed, Sep 8, 2021 at 10:56 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>>
>> I took a look at this, and while I like the proposed feature I think the patch
>> has a bit more work required.
>>
>
> Thanks for reviewing the patch.
> I am not the original patch author (who no longer seems active) but
> I've been contributing a bit and keeping the patch alive because I
> think it's a worthwhile feature.
>
>>
>> +
>> +-- 2) Initialize some user session data
>> + CREATE TEMP TABLE session_storage (x float, y integer);
>> The example in the documentation use a mix of indentations, neither of which is
>> the 2-space indentation used elsewhere in the docs.
>>
>
> Fixed, using 2-space indentation.
> (to be honest, the indentation seems inconsistent elsewhere in the
> docs e.g. I'm seeing a nearby case of 2-space on 1st indent, 6-space
> on 2nd indent)
>
>>
>> + /* Get the command tag. */
>> + tag = parsetree ? CreateCommandTag(parsetree) : CMDTAG_CONNECT;
>> This is hardcoding knowledge that currently only this trigger wont have a
>> parsetree, and setting the tag accordingly. This should instead check the
>> event and set CMDTAG_UNKNOWN if it isn't the expected one.
>>
>
> I updated that, but maybe not exactly how you expected?
>
>>
>> + /* database has on-login triggers */
>> + bool dathaslogontriggers;
>> This patch uses three different names for the same thing: client connection,
>> logontrigger and login trigger. Since this trigger only fires after successful
>> authentication it’s not accurate to name it client connection, as that implies
>> it running on connections rather than logins. The nomenclature used in the
>> tree is "login" so the patch should be adjusted everywhere to use that.
>>
>
> Fixed.
>
>>
>> +/* Hook for plugins to get control at start of session */
>> +client_connection_hook_type client_connection_hook = EventTriggerOnConnect;
>> I don't see the reason for adding core functionality by hooks. Having a hook
>> might be a useful thing on its own (to be discussed in a new thread, not hidden
>> here), but core functionality should not depend on it IMO.
>>
>
> Fair enough, I removed the hook.
>
>>
>> + {"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.
>>
>
> OK, I removed those bypasses. We'll see what others think.
>
>>
>> + elog(NOTICE, "client_connection trigger failed with message: %s", error->message);
>> Calling elog() in a PG_CATCH() block isn't allowed is it?
>>
>
> I believe it is allowed (e.g. there's a case in libpq), but I removed
> this anyway as part of the superuser bypass.
>
>>
>> + /* Runtlist is empty: clear dathaslogontriggers flag
>> + */
>> s/Runtlist/Runlist/ and also commenting style.
>>
>
> Fixed.
>
>>
>> The logic for resetting the pg_database flag when firing event trigger in case
>> the trigger has gone away is messy and a problem waiting to happen IMO. For
>> normal triggers we don't bother with that on the grounds of it being racy, and
>> instead perform relhastrigger removal during VACUUM. Another approach is using
>> a counter as propose upthread, since updating that can be made safer. The
>> current coding also isn't instructing concurrent backends to perform relcache
>> rebuild.
>>
>
> I think there are pros and cons of each possible approach, but I think
> I prefer the current way (which I have tweaked a bit) for similar
> reasons to those explained by the original patch author when debating
> whether to use reference-counting instead, in the discussion upthread
> (e.g. it keeps it all in one place). Also, it seems to be more inline
> with the documented reason why that pg_database flag was added in the
> first place. I have debugged a few concurrent scenarios with the
> current mechanism in place. If you still dislike the logic for
> resetting the flag, please elaborate on the issues you foresee and one
> of the alternative approaches can be tried.
>
> I've attached the updated patch.
>
> Regards,
> Greg Nancarrow
> Fujitsu Australia
>

--
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/