Re: On login trigger: take three

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
> <mailto:gregn4422(at)gmail(dot)com>> napsal:
>
> On Tue, Dec 8, 2020 at 3:26 PM Pavel Stehule
> <pavel(dot)stehule(at)gmail(dot)com <mailto: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.
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.

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