Dear colleagues,
Please see my suggestions below and the updated patch attached.
>Среда, 29 сентября 2021, 15:14 +03:00 от Teodor Sigaev < teodor(at)sigaev(dot)ru >:
>
>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.
This is a common problem of logging errors and unsuccessful transactions of any kind. It can be solved by:
- logging to external log storage (stupid logging to files or syslog or whatever you can imagine with PL/Perl (sorry))
- logging inside the database by db_link or through background worker (like described in https://dpavlin.wordpress.com/2017/05/09/david-rader-autonomous-transactions-using-pg_background/ )
- or by implementing autonomous transactions in PostgreSQL, which is already under development by some of my and Teodor’s colleagues.
So I propose not to invent another solution to this common problem here.
>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 the trigger contains an error, nobody can login. The database is bricked.
Previous variant of the patch proposed to fix this with going to single-user mode.
For faster recovery I propose to have also a GUC variable to turn on/off the login triggers. It should be 'on' by default.
>
>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.
Such cases can be avoided by careful design of the login triggers and related permissions. Added such note to the documentation.
>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.
Yes. Btw, note that pg_hba functionality can be implemented completely inside the login trigger :) !
>
>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.
Any solutions which make syntax more complicated can lead Postgres to become Oracle (in the worst sense). I do not like this.
A new version of the patch is attached. It applies to the current master and contains the above mentioned GUC code, relevant tests and docs.
Regards,
Ivan Panchenko
>
>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/