Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

Rod,

* Rod Taylor (rod.tay...@gmail.com) wrote:
> UPDATE seems to work as described (unable to create records you cannot
> select); both the single rule version and multi-rule version seem to work
> the same.

Thanks for reporting this and helping to test the fix.  I've now pushed
the fix and it'll be included in the next round of patch releases.

I'd definitely like to chat further at some point regarding what really
makes sense here and if we should be considering a change, and, further,
how we can improve the documentation to be more clear.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

Rod,

* Rod Taylor (rod.tay...@gmail.com) wrote:
> UPDATE seems to work as described (unable to create records you cannot
> select); both the single rule version and multi-rule version seem to work
> the same.

I'm glad they work the same now, as they were always intended to.

Regarding the "rules", they're actually based on what the ACL rules are.
In particular, an UPDATE with a WHERE clause requires SELECT rights- and
so the code mandates that the UPDATE can see both the row-to-be-updated
and the row-post-updating, but you're able to INSERT records which you
can't then see, as an INSERT doesn't generally require SELECT
privileges.

> This combination works too though it seems funny that UPDATE can create an
> entry it cannot reverse. I can set the value to 100 (going to -1 fails) but
> the UPDATE cannot see the record to set it back. I can see use cases for
> it, for example you might be able to promote someone to manager but not
> demote a manager to front-desk. We also allow INSERT on tables you cannot
> delete from, so it's not inconsistent.
> 
> CREATE POLICY split_select ON t FOR SELECT TO split USING (value > 0);
> CREATE POLICY split_update ON t FOR UPDATE TO split USING (value < 10) WITH
> CHECK (value > 2);
> SET session authorization split;
> update t set value = 100 where value = 4; -- 1 record changed
> update t set value = 5 where value = 100; -- 0 records changed

Being able to UPDATE records to change them in a way that you're not
able to subsequently UPDATE them seems reasonable to me, at least.

> However, despite INSERT also functioning the same for both styles of
> commands it's definitely not obeying the `cannot give away records` rule:

Right, this is because INSERT doesn't generally require SELECT
privileges, and therefore doesn't require the USING clause to be
checked also.

We could certainly debate if the approach of applying policies based on
the privileges required for the command is the "correct" approach, but
it's definitely what was intended and generally works well, based on
what I've seen thus far in terms of actual usage.  Admittedly, it's a
bit of an edge case which doesn't come up very often either, so perhaps
we should consider changing this down the road, but for now we should go
ahead and fix the obvious unintentional bug in the code around ALL
policies and back-patch that as a bug fix, we can then consider if
changes should be made here in future releases independently.

Assuming there aren't objections to that, I'll plan to push this fix
later tonight or tomorrow.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Rod Taylor]

Hmm.

UPDATE seems to work as described (unable to create records you cannot
select); both the single rule version and multi-rule version seem to work
the same.

This combination works too though it seems funny that UPDATE can create an
entry it cannot reverse. I can set the value to 100 (going to -1 fails) but
the UPDATE cannot see the record to set it back. I can see use cases for
it, for example you might be able to promote someone to manager but not
demote a manager to front-desk. We also allow INSERT on tables you cannot
delete from, so it's not inconsistent.

CREATE POLICY split_select ON t FOR SELECT TO split USING (value > 0);
CREATE POLICY split_update ON t FOR UPDATE TO split USING (value < 10) WITH
CHECK (value > 2);
SET session authorization split;
update t set value = 100 where value = 4; -- 1 record changed
update t set value = 5 where value = 100; -- 0 records changed


However, despite INSERT also functioning the same for both styles of
commands it's definitely not obeying the `cannot give away records` rule:

CREATE USER simple;
CREATE USER split;
CREATE TABLE t(value int);
grant select, update, insert, delete on table t to simple, split;

INSERT INTO t values (1), (2);

ALTER TABLE t ENABLE ROW LEVEL SECURITY;
CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK (true);


CREATE POLICY split_select ON t FOR SELECT TO split USING (value > 0);
CREATE POLICY split_insert ON t FOR INSERT TO split WITH CHECK (true);


SET session authorization simple;
INSERT INTO t VALUES (3), (-3);
SELECT * FROM t;
 value
---
 1
 2
 3
(3 rows)


SET session authorization split;
INSERT INTO t VALUES (4), (-4);
SELECT * FROM t;
 value
---
 1
 2
 3
 4
(4 rows)


SET session authorization default;
SELECT * FROM t;
 value
---
 1
 2
 3
-3
 4
-4
(6 rows)


regards,

Rod



On Fri, May 5, 2017 at 12:10 PM, Stephen Frost  wrote:

> Rod, Robert,
>
> * Robert Haas (robertmh...@gmail.com) wrote:
> > On Fri, Apr 14, 2017 at 9:16 AM, Stephen Frost 
> wrote:
> > > I agreed already up-thread that there's an issue there and will be
> > > looking to fix it.  That comment was simply replying to Rod's point
> that
> > > the documentation could also be improved.
> >
> > OK, thanks.  The wrap for the next set of minor releases is, according
> > to my understanding, scheduled for Monday, so you'd better jump on
> > this soon if you're hoping to get a fix out this time around.
>
> The attached patch against master fixes this issue.  Rod, if you get a
> chance, would be great for you to check that you no longer see a
> difference between the single ALL policy and the split SELECT/UPDATE
> policies.
>
> Thanks!
>
> Stephen
>



-- 
Rod Taylor

Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

Rod, Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Fri, Apr 14, 2017 at 9:16 AM, Stephen Frost  wrote:
> > I agreed already up-thread that there's an issue there and will be
> > looking to fix it.  That comment was simply replying to Rod's point that
> > the documentation could also be improved.
> 
> OK, thanks.  The wrap for the next set of minor releases is, according
> to my understanding, scheduled for Monday, so you'd better jump on
> this soon if you're hoping to get a fix out this time around.

The attached patch against master fixes this issue.  Rod, if you get a
chance, would be great for you to check that you no longer see a
difference between the single ALL policy and the split SELECT/UPDATE
policies.

Thanks!

Stephen
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
new file mode 100644
index 5c8c0cf..5a2c78b
*** a/src/backend/rewrite/rowsecurity.c
--- b/src/backend/rewrite/rowsecurity.c
*** static void add_with_check_options(Relat
*** 78,84 
  	   List *permissive_policies,
  	   List *restrictive_policies,
  	   List **withCheckOptions,
! 	   bool *hasSubLinks);
  
  static bool check_role_for_policy(ArrayType *policy_roles, Oid user_id);
  
--- 78,85 
  	   List *permissive_policies,
  	   List *restrictive_policies,
  	   List **withCheckOptions,
! 	   bool *hasSubLinks,
! 	   bool force_using);
  
  static bool check_role_for_policy(ArrayType *policy_roles, Oid user_id);
  
*** get_row_security_policies(Query *root, R
*** 272,278 
  			   permissive_policies,
  			   restrictive_policies,
  			   withCheckOptions,
! 			   hasSubLinks);
  
  		/*
  		 * Get and add ALL/SELECT policies, if SELECT rights are required for
--- 273,280 
  			   permissive_policies,
  			   restrictive_policies,
  			   withCheckOptions,
! 			   hasSubLinks,
! 			   false);
  
  		/*
  		 * Get and add ALL/SELECT policies, if SELECT rights are required for
*** get_row_security_policies(Query *root, R
*** 295,301 
     select_permissive_policies,
     select_restrictive_policies,
     withCheckOptions,
!    hasSubLinks);
  		}
  
  		/*
--- 297,304 
     select_permissive_policies,
     select_restrictive_policies,
     withCheckOptions,
!    hasSubLinks,
!    true);
  		}
  
  		/*
*** get_row_security_policies(Query *root, R
*** 324,330 
     conflict_permissive_policies,
     conflict_restrictive_policies,
     withCheckOptions,
!    hasSubLinks);
  
  			/*
  			 * Get and add ALL/SELECT policies, as WCO_RLS_CONFLICT_CHECK WCOs
--- 327,334 
     conflict_permissive_policies,
     conflict_restrictive_policies,
     withCheckOptions,
!    hasSubLinks,
!    true);
  
  			/*
  			 * Get and add ALL/SELECT policies, as WCO_RLS_CONFLICT_CHECK WCOs
*** get_row_security_policies(Query *root, R
*** 346,352 
  	   conflict_select_permissive_policies,
  	   conflict_select_restrictive_policies,
  	   withCheckOptions,
! 	   hasSubLinks);
  			}
  
  			/* Enforce the WITH CHECK clauses of the UPDATE policies */
--- 350,357 
  	   conflict_select_permissive_policies,
  	   conflict_select_restrictive_policies,
  	   withCheckOptions,
! 	   hasSubLinks,
! 	   true);
  			}
  
  			/* Enforce the WITH CHECK clauses of the UPDATE policies */
*** get_row_security_policies(Query *root, R
*** 355,361 
     conflict_permissive_policies,
     conflict_restrictive_policies,
     withCheckOptions,
!    hasSubLinks);
  		}
  	}
  
--- 360,367 
     conflict_permissive_policies,
     conflict_restrictive_policies,
     withCheckOptions,
!    hasSubLinks,
!    false);
  		}
  	}
  
*** add_with_check_options(Relation rel,
*** 659,671 
  	   List *permissive_policies,
  	   List *restrictive_policies,
  	   List **withCheckOptions,
! 	   bool *hasSubLinks)
  {
  	ListCell   *item;
  	List	   *permissive_quals = NIL;
  
  #define QUAL_FOR_WCO(policy) \
! 	( kind != WCO_RLS_CONFLICT_CHECK && \
  	  (policy)->with_check_qual != NULL ? \
  	  (policy)->with_check_qual : (policy)->qual )
  
--- 665,678 
  	   List *permissive_policies,
  	   List *restrictive_policies,
  	   List **withCheckOptions,
! 	   bool *hasSubLinks,
! 	   bool force_using)
  {
  	ListCell   *item;
  	List	   *permissive_quals = NIL;
  
  #define QUAL_FOR_WCO(policy) \
! 	( !force_using && \
  	  (policy)->with_check_qual != NULL ? \
  	  (policy)->with_check_qual : (policy)->qual )
  


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

Robert, all,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Fri, Apr 14, 2017 at 9:16 AM, Stephen Frost  wrote:
> > I agreed already up-thread that there's an issue there and will be
> > looking to fix it.  That comment was simply replying to Rod's point that
> > the documentation could also be improved.
> 
> OK, thanks.  The wrap for the next set of minor releases is, according
> to my understanding, scheduled for Monday, so you'd better jump on
> this soon if you're hoping to get a fix out this time around.

I've worked out what's happening here and it's because the ALL policy
has both USING and WITH CHECK that it's not acting the same as the
SELECT policy (which can only have USING).  add_with_check_quals() is
what determines if the WITH CHECK policy or the USING policy should be
used (through a bit of a grotty #define, if you ask me..).

I've been considering how best to fix it.  The two main options are to
use a different WCOKind and then track that through, which might be nice
as we might be able to provide a more useful error message in that case,
or to just add an additional flag to add_with_check_quals() to say
"always add the USING clause when this flag is true."

Either way, I expect to wrap this up either later tonight or tomorrow.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Fri, Apr 14, 2017 at 9:16 AM, Stephen Frost  wrote:
> > I agreed already up-thread that there's an issue there and will be
> > looking to fix it.  That comment was simply replying to Rod's point that
> > the documentation could also be improved.
> 
> OK, thanks.  The wrap for the next set of minor releases is, according
> to my understanding, scheduled for Monday, so you'd better jump on
> this soon if you're hoping to get a fix out this time around.

Thanks for the reminder.  I do know the release is looming and I am
hoping to have a patch tomorrow or Thursday.

Thanks again!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Robert Haas]

On Fri, Apr 14, 2017 at 9:16 AM, Stephen Frost  wrote:
> I agreed already up-thread that there's an issue there and will be
> looking to fix it.  That comment was simply replying to Rod's point that
> the documentation could also be improved.

OK, thanks.  The wrap for the next set of minor releases is, according
to my understanding, scheduled for Monday, so you'd better jump on
this soon if you're hoping to get a fix out this time around.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

Rod,

* Rod Taylor (rod.tay...@gmail.com) wrote:
> Yep. It's equivalent to a DELETE or DEACTIVATE. RLS may not be the right
> facility but it was very close to working exactly the way I wanted in FOR
> ALL mode.

Turning an UPDATE into, effectively, a DELETE, does seem like it's
beyond the mandate of RLS.  Using an on-delete trigger strikes me as a
good approach.

The base premise of not allowing rows to be 'given away', similar to how
we don't allow full objects to be given away, should be enforced for the
'ALL' policy case, just as it is for the individual-command case.  I'll
get that addressed before the next set of minor releases and will also
see about improving the documentation and code comments to make it more
clear.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Rod Taylor]

On Fri, Apr 14, 2017 at 9:09 AM, Stephen Frost  wrote:

> Rod,
>
> * Rod Taylor (rod.tay...@gmail.com) wrote:
> > My actual use-case involves a range. Most users can see and manipulate
> the
> > record when CURRENT_TIMESTAMP is within active_period. Some users
> > (staff/account admins) can see recently dead records too. And a 3rd group
> > (senior staff) have no time restriction, though there are a few customers
> > they cannot see due to their information being a touch more sensitive.
> > I've simplified the below rules to just deal with active_period and the
> > majority of user view (@> CURRENT_TIMESTAMP).
>
> Interesting.
>
> > NOTE: the active_period range is '[)' by default, so records with
> upper() =
> > CURRENT_TIMESTAMP are not visible with @> CURRENT_TIMESTAMP restriction.
>
> Is that really what you intend/want though?  For records with
> upper() = CURRENT_TIMESTAMP to not be visible?  You are able to change
> the range returned from tstzrange by specifying what you want, eg:
>

Yeah, think of it like a delete. Once a record is deleted you want it to
disappear. From the users point of view, who doesn't have time-travel
privileges, an UPDATE to upper() = CURRENT_TIMESTAMP should disappear from
any actions that take place later in the transaction.

A more common way of implementing this is an archive table. Have a DELETE
trigger and shuffle the record to another storage area but with many
dependent tuples via foreign key this can be very time consuming. Flipping
a time period is consistently fast with the caveat that SELECT pays a price.

If you decide Pg shouldn't allow a user to make a tuple disappear, I would
probably make a DO INSTEAD SECURITY DEFINER function that triggers on
DELETE for that role only and changes the time range. Reality is after
about 1 week for customers to contact their account administrator and say
"I accidentally deleted X" it would likely be moved to an archive structure.


select tstzrange(current_timestamp, current_timestamp, '[]');
>
> > CREATE A TABLE t (id integer, active_period tstzrange NOT NULL DEFAULT
> > tstzrange(current_timestamp, NULL));
>
> Why NULL instead of 'infinity'...?
>

Diskspace. NULL works (almost) the same as infinity but the storage is
quite a bit smaller.



>
> > -- Disallowed due to hide_old_select policy.
> > UPDATE t SET active_period = tstzrange(lower(active_period),
> > CURRENT_TIMESTAMP);
>
> Guess I'm still trying to figure out if you really intend for this to
> make the records invisible to the 'most users' case.
>

Yep. It's equivalent to a DELETE or DEACTIVATE. RLS may not be the right
facility but it was very close to working exactly the way I wanted in FOR
ALL mode.

-- 
Rod Taylor

Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Fri, Apr 14, 2017 at 9:09 AM, Stephen Frost  wrote:
> > I agree that the documentation could be improved here.
> 
> This does not seem like it's only a documentation problem.  There
> seems to be no principled reason why a grant for ALL shouldn't have
> exactly the same effect as one grant per relevant operation type.

I agreed already up-thread that there's an issue there and will be
looking to fix it.  That comment was simply replying to Rod's point that
the documentation could also be improved.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Robert Haas]

On Fri, Apr 14, 2017 at 9:09 AM, Stephen Frost  wrote:
> I agree that the documentation could be improved here.

This does not seem like it's only a documentation problem.  There
seems to be no principled reason why a grant for ALL shouldn't have
exactly the same effect as one grant per relevant operation type.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

Rod,

* Rod Taylor (rod.tay...@gmail.com) wrote:
> My actual use-case involves a range. Most users can see and manipulate the
> record when CURRENT_TIMESTAMP is within active_period. Some users
> (staff/account admins) can see recently dead records too. And a 3rd group
> (senior staff) have no time restriction, though there are a few customers
> they cannot see due to their information being a touch more sensitive.
> I've simplified the below rules to just deal with active_period and the
> majority of user view (@> CURRENT_TIMESTAMP).

Interesting.

> NOTE: the active_period range is '[)' by default, so records with upper() =
> CURRENT_TIMESTAMP are not visible with @> CURRENT_TIMESTAMP restriction.

Is that really what you intend/want though?  For records with
upper() = CURRENT_TIMESTAMP to not be visible?  You are able to change
the range returned from tstzrange by specifying what you want, eg:

select tstzrange(current_timestamp, current_timestamp, '[]');

> CREATE A TABLE t (id integer, active_period tstzrange NOT NULL DEFAULT
> tstzrange(current_timestamp, NULL));

Why NULL instead of 'infinity'...?

> -- Disallowed due to hide_old_select policy.
> UPDATE t SET active_period = tstzrange(lower(active_period),
> CURRENT_TIMESTAMP);

Guess I'm still trying to figure out if you really intend for this to
make the records invisible to the 'most users' case.

> I'm happy to help with testing and documentation but first I need to
> understand what the intended functionality was. Right now it seems
> inconsistent between the simple single policy version and the multi policy
> version; the docs imply the single policy version is correct (it only seems
> to mention SELECT checks on RETURNING clauses).

I agree that the documentation could be improved here.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Rod Taylor]

On Fri, Apr 14, 2017 at 7:41 AM, Rod Taylor  wrote:

>
>
>
> On Thu, Apr 13, 2017 at 5:31 PM, Stephen Frost  wrote:
>
>> Rod, all,
>>
>> * Joe Conway (m...@joeconway.com) wrote:
>> > On 04/13/2017 01:31 PM, Stephen Frost wrote:
>> > > * Robert Haas (robertmh...@gmail.com) wrote:
>> > >> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor 
>> wrote:
>> > >> > I'm a little confused on why a SELECT policy fires against the NEW
>> record
>> > >> > for an UPDATE when using multiple FOR policies. The ALL policy
>> doesn't seem
>> > >> > to have that restriction.
>> > >>
>> > >> My guess is that you have found a bug.
>> > >
>> > > Indeed.  Joe's been looking into it and I'm hoping to find some time
>> to
>> > > dig into it shortly.
>> >
>> > >> CREATE POLICY split_select ON t FOR SELECT TO split
>> > >> USING (value > 0);
>> > >> CREATE POLICY split_update ON t FOR UPDATE TO split
>> > >> USING (true) WITH CHECK (true);
>> >
>> > Yes -- from what I can see in gdb:
>>
>> Actually, looking at this again, the complaint appears to be that you
>> can't "give away" records.  That was a topic of much discussion and I'm
>> reasonably sure that was what we ended up deciding made the most sense.
>> You have to be able to see records to be able to update them (you can't
>> update records you can't see), and you have to be able to see the result
>> of your update.  I don't doubt that we could improve the documentation
>> around this (and apparently the code comments, according to Joe..).
>>
>>
> Then there is a bug in the simpler statement which happily lets you give
> away records.
>
> CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK
> (true);
>
> SET session authorization simple;
> SELECT * FROM t;
> UPDATE t SET value = value * -1 WHERE value = 1;
> -- No error and value is -1 at the end.
>


My actual use-case involves a range. Most users can see and manipulate the
record when CURRENT_TIMESTAMP is within active_period. Some users
(staff/account admins) can see recently dead records too. And a 3rd group
(senior staff) have no time restriction, though there are a few customers
they cannot see due to their information being a touch more sensitive.
I've simplified the below rules to just deal with active_period and the
majority of user view (@> CURRENT_TIMESTAMP).

NOTE: the active_period range is '[)' by default, so records with upper() =
CURRENT_TIMESTAMP are not visible with @> CURRENT_TIMESTAMP restriction.


CREATE A TABLE t (id integer, active_period tstzrange NOT NULL DEFAULT
tstzrange(current_timestamp, NULL));


The below policy is allowed but requires that 1ms slop to accommodate the wi

Updated record invisible to USING but requires a trigger to enforce
specific upper
and starting values. I have a trigger enforcing specific upper/lower values
for the range
for specific ROLEs. So I had the thought that I might move ROLE specific
trigger logic into
the RLS mechanism.

CREATE POLICY hide_old ON t TO s;
  USING ( active_period @> CURRENT_TIMESTAMP)
 WITH CHECK ( active_period && tstzrange(current_timestamp - interval
'0.001 seconds', current_timestamp, '[]'));

-- This is effectively a delete for the above policy. It becomes
immediately invisible due to USING restriction.
UPDATE t SET active_period = tstzrange(lower(active_period),
CURRENT_TIMESTAMP);
SELECT count(*) FROM t; -- 0 records



I tried to tighten the above rules, so INSERT must have upper of NULL and
UPDATE must
set upper to exactly CURRENT_TIMESTAMP. Clearly I can achieve this using
triggers for
enforcement but I tried to abuse RLS instead because it is a role specific
restriction.

I was surprised when hide_old_select->USING was preventing the UPDATE when
the simple single policy version let it through.

CREATE POLICY hide_old_select ON t FOR SELECT TO s
  USING ( active_period @> CURRENT_TIMESTAMP);
CREATE POLICY hide_old_insert ON t FOR INSERT to s
 WITH CHECK ( lower(active_period) = CURRENT_TIMESTAMP AND
upper(active_period) IS NULL);

CREATE POLICY hide_old_update ON t FOR UPDATE TO s
  USING ( active_period @> CURRENT_TIMESTAMP)
 WITH CHECK ( upper(active_period) = CURRENT_TIMESTAMP);

-- Disallowed due to hide_old_select policy.
UPDATE t SET active_period = tstzrange(lower(active_period),
CURRENT_TIMESTAMP);



I'm happy to help with testing and documentation but first I need to
understand what the intended functionality was. Right now it seems
inconsistent between the simple single policy version and the multi policy
version; the docs imply the single policy version is correct (it only seems
to mention SELECT checks on RETURNING clauses).


-- 
Rod Taylor

Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

Rod,

* Rod Taylor (rod.tay...@gmail.com) wrote:
> Then there is a bug in the simpler statement which happily lets you give
> away records.
> 
> CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK (true);
> 
> SET session authorization simple;
> SELECT * FROM t;
> UPDATE t SET value = value * -1 WHERE value = 1;
> -- No error and value is -1 at the end.

Hm, that does seem like it's not matching up with the intent, likely
because it's an 'ALL' policy instead of individual policies.

Out of curiosity, is there a particular use-case here that you're
working towards, or just testing it out to see how it works?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Rod Taylor]

On Thu, Apr 13, 2017 at 5:31 PM, Stephen Frost  wrote:

> Rod, all,
>
> * Joe Conway (m...@joeconway.com) wrote:
> > On 04/13/2017 01:31 PM, Stephen Frost wrote:
> > > * Robert Haas (robertmh...@gmail.com) wrote:
> > >> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor 
> wrote:
> > >> > I'm a little confused on why a SELECT policy fires against the NEW
> record
> > >> > for an UPDATE when using multiple FOR policies. The ALL policy
> doesn't seem
> > >> > to have that restriction.
> > >>
> > >> My guess is that you have found a bug.
> > >
> > > Indeed.  Joe's been looking into it and I'm hoping to find some time to
> > > dig into it shortly.
> >
> > >> CREATE POLICY split_select ON t FOR SELECT TO split
> > >> USING (value > 0);
> > >> CREATE POLICY split_update ON t FOR UPDATE TO split
> > >> USING (true) WITH CHECK (true);
> >
> > Yes -- from what I can see in gdb:
>
> Actually, looking at this again, the complaint appears to be that you
> can't "give away" records.  That was a topic of much discussion and I'm
> reasonably sure that was what we ended up deciding made the most sense.
> You have to be able to see records to be able to update them (you can't
> update records you can't see), and you have to be able to see the result
> of your update.  I don't doubt that we could improve the documentation
> around this (and apparently the code comments, according to Joe..).
>
>
Then there is a bug in the simpler statement which happily lets you give
away records.

CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK (true);

SET session authorization simple;
SELECT * FROM t;
UPDATE t SET value = value * -1 WHERE value = 1;
-- No error and value is -1 at the end.



-- 
Rod Taylor

Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

Rod, all,

* Joe Conway (m...@joeconway.com) wrote:
> On 04/13/2017 01:31 PM, Stephen Frost wrote:
> > * Robert Haas (robertmh...@gmail.com) wrote:
> >> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor  wrote:
> >> > I'm a little confused on why a SELECT policy fires against the NEW record
> >> > for an UPDATE when using multiple FOR policies. The ALL policy doesn't 
> >> > seem
> >> > to have that restriction.
> >> 
> >> My guess is that you have found a bug.
> > 
> > Indeed.  Joe's been looking into it and I'm hoping to find some time to
> > dig into it shortly.
> 
> >> CREATE POLICY split_select ON t FOR SELECT TO split
> >> USING (value > 0);
> >> CREATE POLICY split_update ON t FOR UPDATE TO split
> >> USING (true) WITH CHECK (true);
> 
> Yes -- from what I can see in gdb:

Actually, looking at this again, the complaint appears to be that you
can't "give away" records.  That was a topic of much discussion and I'm
reasonably sure that was what we ended up deciding made the most sense.
You have to be able to see records to be able to update them (you can't
update records you can't see), and you have to be able to see the result
of your update.  I don't doubt that we could improve the documentation
around this (and apparently the code comments, according to Joe..).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Joe Conway]

On 04/13/2017 01:31 PM, Stephen Frost wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor  wrote:
>> > I'm a little confused on why a SELECT policy fires against the NEW record
>> > for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem
>> > to have that restriction.
>> 
>> My guess is that you have found a bug.
> 
> Indeed.  Joe's been looking into it and I'm hoping to find some time to
> dig into it shortly.


>> CREATE POLICY split_select ON t FOR SELECT TO split
>> USING (value > 0);
>> CREATE POLICY split_update ON t FOR UPDATE TO split
>> USING (true) WITH CHECK (true);

Yes -- from what I can see in gdb:

1) add_with_check_options() adds both (value > 0) and (true) to
   withCheckOptions -- this seems correct as the USING expression
   is used for WITH CHECK when the latter is not specified

2) ExecWithCheckOptions() checks (value > 0) which fails, and it
   immediately throws an ERROR, i.e. it never checks the (true)
   expression and therefore never ORs the results -- this seems
   incorrect, it uses restrictive not permissive

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor  wrote:
> > I'm a little confused on why a SELECT policy fires against the NEW record
> > for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem
> > to have that restriction.
> 
> My guess is that you have found a bug.

Indeed.  Joe's been looking into it and I'm hoping to find some time to
dig into it shortly.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Robert Haas]

On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor  wrote:
> I'm a little confused on why a SELECT policy fires against the NEW record
> for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem
> to have that restriction.

My guess is that you have found a bug.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Row Level Security UPDATE Confusion

[Rod Taylor]

I'm a little confused on why a SELECT policy fires against the NEW record
for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem
to have that restriction.


DROP TABLE IF EXISTS t;

CREATE USER simple;
CREATE USER split;
CREATE TABLE t(value int);
grant select, update on table t to simple, split;

INSERT INTO t values (1), (2);


ALTER TABLE t ENABLE ROW LEVEL SECURITY;
CREATE POLICY simple_all ON t TO simple USING (value > 0) WITH CHECK (true);

CREATE POLICY split_select ON t FOR SELECT TO split USING (value > 0);
CREATE POLICY split_update ON t FOR UPDATE TO split USING (true) WITH CHECK
(true);


SET session authorization simple;
SELECT * FROM t;
UPDATE t SET value = value * -1 WHERE value = 1;

SET session authorization split;
SELECT * FROM t;
UPDATE t SET value = value * -1 WHERE value = 2;
/* UPDATE fails with below error:
psql:/tmp/t.sql:24: ERROR:  42501: new row violates row-level security
policy for table "t"
LOCATION:  ExecWithCheckOptions, execMain.c:2045
*/

SET session authorization default;
SELECT * FROM t;

This seems consistent in both Pg 9.5 and upcoming Pg 10.


-- 
Rod Taylor