RLS with check option - surprised design

Hello

I am playing with RLS. I created simple table

table_data (inserted_by text, v integer);

I created two policies

create policy p1 on data with check (inserted_by = session_user);
create policy p2 on data with check (v between 10 and 1000);

I was surprised so p2 effectively disables p1;

next a message:

ERROR: new row violates WITH CHECK OPTION for "data"
DETAIL: Failing row contains (2014-10-05 12:28:30.79652, petr, 1000).

Doesn't inform about broken policy.

Regards

Pavel

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> I am playing with RLS. I created simple table
>
> table_data (inserted_by text, v integer);
>
> I created two policies
>
> create policy p1 on data with check (inserted_by = session_user);
> create policy p2 on data with check (v between 10 and 1000);
>
> I was surprised so p2 effectively disables p1;

It doesn't disable it at all- both are applied using OR, as documented
and discussed extensively earlier this year..

I'm not against revisiting that and there has been suggestions about
providing a 'RESTRICTED' policy type which AND's them together, but I
hope it isn't surprising to anyone who has looked at the documentation..
You might also have a policy which applies to all roles and then a more
permissive policy for an 'admin' type of user- look at the "Unix passwd"
example outlined in the documentation.

> next a message:
>
> ERROR: new row violates WITH CHECK OPTION for "data"
> DETAIL: Failing row contains (2014-10-05 12:28:30.79652, petr, 1000).
>
> Doesn't inform about broken policy.

I'm guessing this is referring to the above policies and so my comments
there apply.. One thing to note about this is that there is an active
discussion about removing the 'DETAIL' part of that error message as it
may be an information leak.

Thanks,

Stephen

2014-10-05 14:16 GMT+02:00 Stephen Frost <sfrost(at)snowman(dot)net>:

> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> > I am playing with RLS. I created simple table
> >
> > table_data (inserted_by text, v integer);
> >
> > I created two policies
> >
> > create policy p1 on data with check (inserted_by = session_user);
> > create policy p2 on data with check (v between 10 and 1000);
> >
> > I was surprised so p2 effectively disables p1;
>
> It doesn't disable it at all- both are applied using OR, as documented
> and discussed extensively earlier this year..
>

I didn't watch a discussion about RLS this year.

Please, can you show me some use case, where OR has bigger sense than AND?

Thank you

Pavel

>
> I'm not against revisiting that and there has been suggestions about
> providing a 'RESTRICTED' policy type which AND's them together, but I
> hope it isn't surprising to anyone who has looked at the documentation..
> You might also have a policy which applies to all roles and then a more
> permissive policy for an 'admin' type of user- look at the "Unix passwd"
> example outlined in the documentation.
>
> > next a message:
> >
> > ERROR: new row violates WITH CHECK OPTION for "data"
> > DETAIL: Failing row contains (2014-10-05 12:28:30.79652, petr, 1000).
> >
> > Doesn't inform about broken policy.
>
> I'm guessing this is referring to the above policies and so my comments
> there apply.. One thing to note about this is that there is an active
> discussion about removing the 'DETAIL' part of that error message as it
> may be an information leak.
>
> Thanks,
>
> Stephen
>

Pavel,

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> Please, can you show me some use case, where OR has bigger sense than AND?
[...]
> > You might also have a policy which applies to all roles and then a more
> > permissive policy for an 'admin' type of user- look at the "Unix passwd"
> > example outlined in the documentation.

Thanks,

Stephen

On Sun, Oct 5, 2014 at 5:16 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> next a message:
>>
>> ERROR: new row violates WITH CHECK OPTION for "data"
>> DETAIL: Failing row contains (2014-10-05 12:28:30.79652, petr, 1000).
>>
>> Doesn't inform about broken policy.
>
> I'm guessing this is referring to the above policies and so my comments
> there apply.. One thing to note about this is that there is an active
> discussion about removing the 'DETAIL' part of that error message as it
> may be an information leak.

I should point out that there is an issue with the ON CONFLICT UPDATE
patch and RLS, as described here:

https://wiki.postgresql.org/wiki/UPSERT#RLS

I think it'll be possible to prevent the current information leak that
my example illustrates (by making sure that there is an appropriate
predicate associated with the auxiliary UPDATE plan, like any other
UPDATE). After all, the auxiliary UPDATE accepts a WHERE clause,
subject only to a few restrictions that are not relevant for the
purposes of appending security quals.

I actually spent over a day trying to figure out how to make this
work, but gave up before the most recent revision, V1.4 was submitted.
I guess I'll have to look at the problem again soon. I don't grok RLS,
but offhand I think simply not including the DETAIL message might be
good enough to fix my case. Maybe you have an opinion on that.

--
Peter Geoghegan

* Peter Geoghegan (pg(at)heroku(dot)com) wrote:
> On Sun, Oct 5, 2014 at 5:16 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> next a message:
> >>
> >> ERROR: new row violates WITH CHECK OPTION for "data"
> >> DETAIL: Failing row contains (2014-10-05 12:28:30.79652, petr, 1000).
> >>
> >> Doesn't inform about broken policy.
> >
> > I'm guessing this is referring to the above policies and so my comments
> > there apply.. One thing to note about this is that there is an active
> > discussion about removing the 'DETAIL' part of that error message as it
> > may be an information leak.
>
> I should point out that there is an issue with the ON CONFLICT UPDATE
> patch and RLS, as described here:
>
> https://wiki.postgresql.org/wiki/UPSERT#RLS
>
> I think it'll be possible to prevent the current information leak that
> my example illustrates (by making sure that there is an appropriate
> predicate associated with the auxiliary UPDATE plan, like any other
> UPDATE). After all, the auxiliary UPDATE accepts a WHERE clause,
> subject only to a few restrictions that are not relevant for the
> purposes of appending security quals.
>
> I actually spent over a day trying to figure out how to make this
> work, but gave up before the most recent revision, V1.4 was submitted.
> I guess I'll have to look at the problem again soon. I don't grok RLS,
> but offhand I think simply not including the DETAIL message might be
> good enough to fix my case. Maybe you have an opinion on that.

Are you sure this isn't just another example of an existing issue we
have wrt column privileges..? I'm working on a patch already to address
those issues in back-branches and will be considering what needs to be
done for RLS also. One option for RLS is to not produce the 'detail'
info when RLS exists on the relation, but Robert makes a good point that
the detail information is valuable results for normal users, so I'm
hopeful that we'll be able to do better than that.

Another way to address this general concern (I've not looked to see if
it would help with your UPSERT work specifically) might be to change
where the RLS checks are done to be earlier- in particular, *before*
checking for constraint and key violations. The concern there is that
RLS is currently using the security barrier views WITH CHECK
infrastructure and Dean was concerned that the SQL spec requires that
such key violations be reported before WITH CHECK violations. RLS isn't
part of spec, of course, and so we can be open to change that if we feel
it's appropriate but I'm really curious why the spec would require that
key violations be checked first; that doesn't make a huge amount of
sense to me.. Another question is how other databases with similar
capabilities address this.

Thanks!

Stephen

On Fri, Nov 21, 2014 at 6:57 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Are you sure this isn't just another example of an existing issue we
> have wrt column privileges..? I'm working on a patch already to address
> those issues in back-branches and will be considering what needs to be
> done for RLS also.

I thought it was pretty conclusively the case that it wasn't (i.e.
that I definitely had an obligation to figure out a way to get the
security quals appended to the UPDATE predicate). I'm slightly
surprised that you don't immediately agree - after all, I could
manually append a qual that will "stop the leak", since the UPDATE
then won't affect what it shouldn't (though you're still locking the
row, as always happens with ON CONFLICT UPDATE when the WHERE clause
doesn't pass [1], which might need to be considered. But the same is
true with postgres_fdw,)

> One option for RLS is to not produce the 'detail'
> info when RLS exists on the relation, but Robert makes a good point that
> the detail information is valuable results for normal users, so I'm
> hopeful that we'll be able to do better than that.

I agree that losing that would be unfortunate and best avoided.

[1] https://wiki.postgresql.org/wiki/UPSERT#Why_still_lock_row_when_UPDATE_predicate_isn.27t_satisfied.3F
--
Peter Geoghegan

* Peter Geoghegan (pg(at)heroku(dot)com) wrote:
> On Fri, Nov 21, 2014 at 6:57 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > Are you sure this isn't just another example of an existing issue we
> > have wrt column privileges..? I'm working on a patch already to address
> > those issues in back-branches and will be considering what needs to be
> > done for RLS also.
>
> I thought it was pretty conclusively the case that it wasn't (i.e.
> that I definitely had an obligation to figure out a way to get the
> security quals appended to the UPDATE predicate). I'm slightly
> surprised that you don't immediately agree - after all, I could
> manually append a qual that will "stop the leak", since the UPDATE
> then won't affect what it shouldn't (though you're still locking the
> row, as always happens with ON CONFLICT UPDATE when the WHERE clause
> doesn't pass [1], which might need to be considered. But the same is
> true with postgres_fdw,)

The issue is that the entire row is being reported though, no? That's
the same issue we have today. If only the values provided by the user
were reported then there wouldn't be an issue with the detail in the
error message. This is what Dean was getting at when he suggested
looking at modifiedCols to see what should be reported back to the user
in the details.

As for adding quals to the UPDATE- what would end up happening instead?

There's two ways to consider how this can be handled and it depends on
if we think the command should end up failing or doing nothing. I can
see arguments for both, but my feeling is the the command needs to fail
in some way since we can't allow the INSERT to complete (due to the PK
violation) nor the UPDATE (as the user should not be able to see that
row per the USING policy, and likely the new row wouldn't be allowed by
the WITH CHECK policy either).

I don't think we'd want the command to appear to succeed for the same
reason that a straight INSERT doesn't succeed- we'd be accepting data
and then throwing it away. I don't think anyone would intentionally
issue a UPSERT and expect it to be a no-op, while that can certainly
happen with an UPDATE that has quals which end up not matching any
records in the table.

Thanks,

Stephen

On Sat, Nov 22, 2014 at 5:59 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Peter Geoghegan (pg(at)heroku(dot)com) wrote:
>> On Fri, Nov 21, 2014 at 6:57 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > [blah]
>> [re-blah]
> [re-re-blah]
This patch has already been committed, but there are still many
concerns shared, so moving it to CF 2014-12 as it needs more review.
--
Michael