Re: On login trigger: take three

út 15. 12. 2020 v 15:06 odesílatel Konstantin Knizhnik <
k(dot)knizhnik(at)postgrespro(dot)ru> napsal:

>
>
> On 15.12.2020 16:18, Pavel Stehule wrote:
>
>
>
> út 15. 12. 2020 v 14:12 odesílatel Konstantin Knizhnik <
> k(dot)knizhnik(at)postgrespro(dot)ru> napsal:
>
>>
>>
>> On 11.12.2020 19:27, Pavel Stehule wrote:
>>
>>
>>
>> pá 11. 12. 2020 v 17:05 odesílatel Konstantin Knizhnik <
>> k(dot)knizhnik(at)postgrespro(dot)ru> napsal:
>>
>>>
>>>
>>> On 11.12.2020 18:40, Pavel Stehule wrote:
>>>
>>>
>>> is not correct. It makes it not possible to superuser to disable
>>>> triggers for all users.
>>>>
>>>
>>> pg_database_ownercheck returns true for superuser always.
>>>
>>>
>>> Sorry, but I consider different case: when normal user is connected to
>>> the database.
>>> In this case pg_database_ownercheck returns false and trigger is not
>>> disabled, isn't it?
>>>
>>
>> My idea was to reduce necessary rights to database owners. But you have
>> a true, so only superuser can create event trigger, so this feature cannot
>> be used in DBaaS environments, and then my original idea was wrong.
>>
>>
>>>
>>> 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.
>>>
>>>
>>>
>>> But only superusers can set login triggers, right?
>>> So only superuser can make a mistake in this trigger. But he have enough
>>> rights to recover this error. Normal users are not able to define on
>>> connection triggers and
>>> should not have rights to disable them.
>>>
>>
>> yes, it is true
>>
>> Pavel
>>
>>
>>> --
>>> Konstantin Knizhnik
>>> Postgres Professional: http://www.postgrespro.com
>>> The Russian Postgres Company
>>>
>>>
>> So what's next?
>> I see three options:
>>
>> 1. Do not introduce GUC for disabling all event triggers (especially
>> taken in account that them are disabled by default).
>> Return back to the patch
>> on_connect_event_trigger_WITH_SUGGESTED_UPDATES.patch with
>> "disable_client_connection_trigger"
>> and make it true by default (to eliminate any overhead for users which
>> are not using on logintriggers).
>>
>> 2. Have two GUCS: "disable_client_connection_trigger" and
>> "disable_event_triggers".
>>
>> 3. Implement some mechanism for caching presence of event triggers in
>> shared memory.
>>
>
> @3 is the best design (the things should work well by default), @2 is a
> little bit chaotic and @1 looks like a workaround.
>
>
> Please notice that we still need GUC to disable on-login triggers: to make
> it possible for superuser who did mistake and defined incorrect on-login
> trigger to
> login to the system.
> Do we need GUC to disable all other event triggers? May be I am wrong, but
> I do not see much need in such GUC: error in any of such event triggers is
> non fatal
> and can be easily reverted.
> So the only question is whether "disable_client_connection_trigger" should
> be true by default or not...
>
> I agree with you that @2 is a little bit chaotic and @1 looks like a
> workaround.
> But from my point of view @3 is not the best solution but overkill:
> maintaining yet another shared hash just to save few milliseconds on login
> seems to be too high price.
> Actually there are many things which are loaded by new backend from the
> database on start: for example - catalog.
> This is why launch of new backend is an expensive operation.
> Certainly if we execute "select 1", then system catalog is not needed...
> But does anybody start new backend just to execute "select 1" and exit?
>
>
>
I understand so the implementation of a new shared cache can be a lot of
work. The best way is enhancing pg_database about one column with
information about the login triggers (dathaslogontriggers). In init time
these data are in syscache, and can be easily checked. Some like
pg_attribute have an atthasdef column. Same fields has pg_class -
relhasrules, relhastriggers, ... Then the overhead of this design should be
really zero.

What do you think about it?

Pavel

>
>
>
>
> Regards
>
> Pavel
>
>
>>
>> --
>> Konstantin Knizhnik
>> Postgres Professional: http://www.postgrespro.com
>> The Russian Postgres Company
>>
>>
> --
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>