Re: On login trigger: take three

pá 11. 12. 2020 v 16:07 odesílatel Konstantin Knizhnik <
k(dot)knizhnik(at)postgrespro(dot)ru> napsal:

>
>
> On 09.12.2020 15:24, Pavel Stehule wrote:
>
>
>
> st 9. 12. 2020 v 13:17 odesílatel Greg Nancarrow <gregn4422(at)gmail(dot)com>
> napsal:
>
>> On Tue, Dec 8, 2020 at 3:26 PM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>> wrote:
>> >
>> >
>> > There are two maybe generic questions?
>> >
>> > 1. Maybe we can introduce more generic GUC for all event triggers like
>> disable_event_triggers? This GUC can be checked only by the database owner
>> or super user. It can be an alternative ALTER TABLE DISABLE TRIGGER ALL. It
>> can be protection against necessity to restart to single mode to repair the
>> event trigger. I think so more generic solution is better than special
>> disable_client_connection_trigger GUC.
>> >
>> > 2. I have no objection against client_connection. It is probably better
>> for the mentioned purpose - possibility to block connection to database.
>> Can be interesting, and I am not sure how much work it is to introduce the
>> second event - session_start. This event should be started after connecting
>> - so the exception there doesn't block connect, and should be started also
>> after the new statement "DISCARD SESSION", that will be started
>> automatically after DISCARD ALL. This feature should not be implemented in
>> first step, but it can be a plan for support pooled connections
>> >
>>
>
> PGC_SU_BACKEND is too strong, there should be PGC_BACKEND if this option
> can be used by database owner
>
> Pavel
>
>
>> I've created a separate patch to address question (1), rather than
>> include it in the main patch, which I've adjusted accordingly. I'll
>> leave question (2) until another time, as you suggest.
>> See the attached patches.
>>
>> Regards,
>> Greg Nancarrow
>> Fujitsu Australia
>>
>
> It seems to me that current implementation of EventTriggersDisabled:
>
>
> /*
> * Indicates whether all event triggers are currently disabled
> * for the current user.
> * Event triggers are disabled when configuration parameter
> * "disable_event_triggers" is true, and the current user
> * is the database owner or has superuser privileges.
> */
> static inline bool
> EventTriggersDisabled(void)
> {
> return (disable_event_triggers && pg_database_ownercheck(MyDatabaseId,
> GetUserId()));
> }
>
>
> is not correct. It makes it not possible to superuser to disable triggers
> for all users.
>

pg_database_ownercheck returns true for superuser always.

Also GUCs are not associated with any database. So I do not understand why
> this check of database ownership is relevant at all?
>
> What kind of protection violation we want to prevent?
>
> It seems to be obvious that normal user should not be able to prevent
> trigger execution because this triggers may be used to enforce some
> security policies.
> If trigger was created by user itself, then it can drop or disable it
> using ALTER statement. GUC is not needed for it.
>

when you cannot connect to the database, then you cannot do ALTER. In DBaaS
environments lot of users has not superuser rights.

> So I think that EventTriggersDisabled function is not needed and can be
> replaced just with disable_event_triggers GUC check.
> And this option should be defined with PGC_SU_BACKEND
>
>
>
>
> --
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>