Re: [HACKERS] RLS with check option - surprised design
On Sat, Nov 22, 2014 at 5:59 AM, Stephen Frost sfr...@snowman.net wrote: * Peter Geoghegan (p...@heroku.com) wrote: On Fri, Nov 21, 2014 at 6:57 AM, Stephen Frost sfr...@snowman.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 -- 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] RLS with check option - surprised design
* Peter Geoghegan (p...@heroku.com) wrote: On Sun, Oct 5, 2014 at 5:16 AM, Stephen Frost sfr...@snowman.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 signature.asc Description: Digital signature
Re: [HACKERS] RLS with check option - surprised design
On Fri, Nov 21, 2014 at 6:57 AM, Stephen Frost sfr...@snowman.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 -- 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] RLS with check option - surprised design
* Peter Geoghegan (p...@heroku.com) wrote: On Fri, Nov 21, 2014 at 6:57 AM, Stephen Frost sfr...@snowman.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 signature.asc Description: Digital signature
Re: [HACKERS] RLS with check option - surprised design
On Sun, Oct 5, 2014 at 5:16 AM, Stephen Frost sfr...@snowman.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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
Re: [HACKERS] RLS with check option - surprised design
* Pavel Stehule (pavel.steh...@gmail.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 signature.asc Description: Digital signature
Re: [HACKERS] RLS with check option - surprised design
2014-10-05 14:16 GMT+02:00 Stephen Frost sfr...@snowman.net: * Pavel Stehule (pavel.steh...@gmail.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
Re: [HACKERS] RLS with check option - surprised design
Pavel, * Pavel Stehule (pavel.steh...@gmail.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 signature.asc Description: Digital signature
