Re: On login trigger: take three

Sergey Shinderuk <s.shinderuk@postgrespro.ru>

From: Sergey Shinderuk <s.shinderuk@postgrespro.ru>
To: Daniel Gustafsson <daniel@yesql.se>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Cc: Ivan Panchenko <wao@mail.ru>, Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>, Greg Nancarrow <gregn4422@gmail.com>, Teodor Sigaev <teodor@sigaev.ru>, Ibrar Ahmed <ibrar.ahmad@gmail.com>, vignesh C <vignesh21@gmail.com>, Pavel Stehule <pavel.stehule@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, a.sokolov@postgrespro.ru
Date: 2022-09-20T13:43:59Z
Lists: pgsql-hackers
On 02.09.2022 18:36, Daniel Gustafsson wrote:
> This had bitrotted a fair bit, attached is a rebase along with (mostly)
> documentation fixes.  0001 adds a generic GUC for ignoring event triggers and
> 0002 adds the login event with event trigger support, and hooks it up to the
> GUC such that broken triggers wont require single-user mode.  Moving the CF
> entry back to Needs Review.


Hello!

There is a race around setting and clearing of dathasloginevt.

Steps to reproduce:

1. Create a trigger:

   CREATE FUNCTION on_login_proc() RETURNS event_trigger AS $$
   BEGIN
     RAISE NOTICE 'You are welcome!';
   END;
   $$ LANGUAGE plpgsql;

   CREATE EVENT TRIGGER on_login_trigger ON login EXECUTE PROCEDURE 
on_login_proc();

2. Then drop it, but don't start new sessions:

   DROP EVENT TRIGGER on_login_trigger;

3. Create another trigger, but don't commit yet:

   BEGIN;
   CREATE EVENT TRIGGER on_login_trigger ON login EXECUTE PROCEDURE 
on_login_proc();

4. Start a new session. This clears dathasloginevt.

5. Commit the transaction:

   COMMIT;

Now we have a trigger, but it doesn't fire, because dathasloginevt=false.


If two sessions create triggers concurrently, one of them will fail.

Steps:

1. In the first session, start a transaction and create a trigger:

   BEGIN;
   CREATE EVENT TRIGGER on_login_trigger1 ON login EXECUTE PROCEDURE 
on_login_proc();

2. In the second session, create another trigger (this query blocks):

   CREATE EVENT TRIGGER on_login_trigger2 ON login EXECUTE PROCEDURE 
on_login_proc();

3. Commit in the first session:

   COMMIT;

The second session fails:

   postgres=# CREATE EVENT TRIGGER on_login_trigger2 ON login EXECUTE 
PROCEDURE on_login_proc();
   ERROR:  tuple concurrently updated



What else bothers me is that login triggers execute in an environment 
under user control and one has to be very careful. The example trigger 
from the documentation

+DECLARE

+  hour integer = EXTRACT('hour' FROM current_time);

+  rec boolean;

+BEGIN

+-- 1. Forbid logging in between 2AM and 4AM.

+IF hour BETWEEN 2 AND 4 THEN

+  RAISE EXCEPTION 'Login forbidden';

+END IF;


can be bypassed with PGOPTIONS='-c timezone=...'. Probably this is 
nothing new and concerns any SECURITY DEFINER function, but still...


Best regards,

-- 
Sergey Shinderuk		https://postgrespro.com/



Commits

  1. Fix some typos in event trigger docs

  2. Use heap_inplace_update() to unset pg_database.dathasloginevt

  3. Remove the flaky check in event_trigger_login regression test

  4. Fix instable 006_login_trigger.pl test

  5. Add support event triggers on authenticated login

  6. Add GUC for temporarily disabling event triggers

  7. Fix typo in reference to __FreeBSD__.

  8. Restore robustness of TAP tests that wait for postmaster restart.

  9. Restore the portal-level snapshot after procedure COMMIT/ROLLBACK.