Message-ID: <e89e8dd9-7143-4db8-ac19-b2951cb0c0da@gmail.com>
Date: Mon, 1 Jul 2024 15:39:39 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Wrong security context for deferred triggers?
To: pgsql-hackers@lists.postgresql.org
References: <77ee784cf248e842f74588418f55c2931e47bd78.camel@cybertec.at>
 <dede297a-3d24-7029-dfd9-06aeef8b9766@enterprisedb.com>
 <6f27f51bc98354d7254bbde1c171fa13aca03f28.camel@cybertec.at>
 <CAAvxfHeZzH=UeK4euOPC9vHAHh4k0eK09-89vvpY45cGnox4RA@mail.gmail.com>
 <CAKFQuwbJnKm9qSmOpWEebbww4mHw4LLFLMbgZO3sept9veUapg@mail.gmail.com>
 <CAAvxfHcb_Wijv7s6FvqVCeNSDjg+UDNx9MuT=-3ZkMbbwvH=cQ@mail.gmail.com>
 <CAKFQuwaojWrn5TOF6SByzpGmUOC=kY_NHOPp_sp_fBRaosb2-g@mail.gmail.com>
 <8e8ef9739bdcc99d4e58916578a2d0a7b5c4268b.camel@cybertec.at>
 <CAKFQuwYsXRUbTphQeWf6u-ZdwV563nnjTVY1j9FoAS+x1BSQLA@mail.gmail.com>
 <fa0124428f6715143ba01ee669b0591dfe958817.camel@cybertec.at>
Content-Language: en-US
From: Bennie Swart <bennieswart@gmail.com>
In-Reply-To: <fa0124428f6715143ba01ee669b0591dfe958817.camel@cybertec.at>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/e89e8dd9-7143-4db8-ac19-b2951cb0c0da%40gmail.com>
Precedence: bulk

Hi,

Allow me to provide some background on how we came across this.

(This is my first time posting to a pgsql list so hopefully I've got 
everything set up correctly.)

We have a db with a big legacy section that we're in the process of 
modernizing. To compensate for some of the shortcomings we have designed 
a layer of writable views to better represent the underlying data and 
make working with it more convenient. The following should illustrate 
what we're doing:

     -- this is the schema containing the view layer.
     create schema api;
     -- and this user is granted access to the api, but not the rest of 
the legacy db.
     create role apiuser;
     grant usage on schema api to apiuser;

     -- some dummy objects in the legacy db - poorly laid out and poorly 
named.
     create schema legacy;
     create table legacy.stock_base (
         id serial primary key
       , lbl text not null unique
       , num int not null
       -- etc
     );
     create table legacy.stock_extra (
         id int not null unique references legacy.stock_base (id)
       , man text
       -- etc
     );

     -- create the stock view which better names and logically groups 
the data.
     create view api.stock as
       select sb.id
            , sb.lbl as description
            , sb.num as quantity
            , se.man as manufacturer
         from legacy.stock_base sb
           left join legacy.stock_extra se using (id);
     -- make it writable so it is easier to work with. use security 
definer to allow access to legacy sections.
     create function api.stock_cud() returns trigger language plpgsql 
security definer as $$
     begin
         -- insert/update legacy.stock_base and legacy.stock_extra 
depending on trigger action, modified fields, etc.
         assert tg_op = 'INSERT'; -- assume insert for example's sake.
         insert into legacy.stock_base (lbl, num) values 
(new.description, new.quantity) returning id into new.id;
         insert into legacy.stock_extra (id, man) values (new.id, 
new.manufacturer);
         return new;
     end;
     $$;
     create trigger stock_cud
         instead of insert or update or delete on api.stock
         for each row execute function api.stock_cud();

     -- grant the apiuser permission to work with the view.
     grant insert, update, delete on api.stock to apiuser;

     -- insert as superuser - works as expected.
     insert into api.stock (description, quantity, manufacturer) values 
('item1', 10, 'manufacturer1');
     -- insert as apiuser - works as expected.
     set role apiuser;
     insert into api.stock (description, quantity, manufacturer) values 
('item2', 10, 'manufacturer2');

In some cases there are constraint triggers on the underlying tables to 
validate certain states. It is, however, possible for a state to be 
temporarily invalid between statements, so long as it is valid at tx 
commit. For this reason the triggers are deferred by default. Consider 
the following example:

     reset role;
     create function legacy.stock_check_state() returns trigger language 
plpgsql as $$
     begin
         -- do some queries to check the state of stock based on 
modified rows and error if invalid.
         raise notice 'current_user %', current_user;
         -- dummy validation code.
         perform * from legacy.stock_base sb left join 
legacy.stock_extra se using (id) where sb.id = new.id;
         return new;
     end;
     $$;
     create constraint trigger stock_check_state
         after insert or update or delete on legacy.stock_base
         deferrable initially deferred
         for each row execute function legacy.stock_check_state();

Then repeat the inserts:

     -- insert as superuser - works as expected.
     reset role;
     insert into api.stock (description, quantity, manufacturer) values 
('item3', 10, 'manufacturer3'); -- NOTICE:  current_user postgres
     -- insert as apiuser - fails.
     set role apiuser;
     insert into api.stock (description, quantity, manufacturer) values 
('item4', 10, 'manufacturer4'); -- NOTICE:  current_user apiuser

     -- insert as apiuser, not deferred - works as expected.
     begin;
     set constraints all immediate;
     insert into api.stock (description, quantity, manufacturer) values 
('item4', 10, 'manufacturer4'); -- NOTICE:  current_user postgres
     commit;

The insert as apiuser fails when the constraint trigger is deferred, but 
works as expected when it is immediate.

Hopefully this helps to better paint the picture. Our workaround 
currently is to just make `legacy.stock_check_state()` security definer 
as well. As I told Laurenz, we're not looking to advocate for any 
specific outcome here. We noticed this strange behaviour and thought it 
to be a bug that should be fixed - whatever "fixed" ends up meaning.

Regards,

Bennie Swart


On 2024/06/26 17:53, Laurenz Albe wrote:
> On Wed, 2024-06-26 at 07:38 -0700, David G. Johnston wrote:
>> We have a few choices then:
>> 1. Status quo + documentation backpatch
>> 2. Change v18 narrowly + documentation backpatch
>> 3. Backpatch narrowly (one infers the new behavior after reading the existing documentation)
>> 4. Option 1, plus a new v18 owner-execution mode in lieu of the narrow change to fix the POLA violation
>>
>> I've been presenting option 4.
>>
>> Pondering further, I see now that having the owner-execution mode be the only way to avoid
>> the POLA violation in deferred triggers isn't great since many triggers benefit from the
>> implied security of being able to run in the invoker's execution context - especially if
>> the trigger doesn't do anything that PUBLIC cannot already do.
>>
>> So, I'm on board with option 2 at this point.
> Nice.
>
> I think we can safely rule out option 3.
> Even if it is a bug, it is not one that has bothered anybody so far that a backpatch
> is indicated.
>
> Yours,
> Laurenz Albe
>
>
>
>