pgsql: RLS fixes, new hooks, and new test module

Started by Stephen Frostover 10 years ago6 messages
#1Stephen Frost
sfrost@snowman.net

RLS fixes, new hooks, and new test module

In prepend_row_security_policies(), defaultDeny was always true, so if
there were any hook policies, the RLS policies on the table would just
get discarded. Fixed to start off with defaultDeny as false and then
properly set later if we detect that only the default deny policy exists
for the internal policies.

The infinite recursion detection in fireRIRrules() didn't properly
manage the activeRIRs list in the case of WCOs, so it would incorrectly
report infinite recusion if the same relation with RLS appeared more
than once in the rtable, for example "UPDATE t ... FROM t ...".

Further, the RLS expansion code in fireRIRrules() was handling RLS in
the main loop through the rtable, which lead to RTEs being visited twice
if they contained sublink subqueries, which
prepend_row_security_policies() attempted to handle by exiting early if
the RTE already had securityQuals. That doesn't work, however, since
if the query involved a security barrier view on top of a table with
RLS, the RTE would already have securityQuals (from the view) by the
time fireRIRrules() was invoked, and so the table's RLS policies would
be ignored. This is fixed in fireRIRrules() by handling RLS in a
separate loop at the end, after dealing with any other sublink
subqueries, thus ensuring that each RTE is only visited once for RLS
expansion.

The inheritance planner code didn't correctly handle non-target
relations with RLS, which would get turned into subqueries during
planning. Thus an update of the form "UPDATE t1 ... FROM t2 ..." where
t1 has inheritance and t2 has RLS quals would fail. Fix by making sure
to copy in and update the securityQuals when they exist for non-target
relations.

process_policies() was adding WCOs to non-target relations, which is
unnecessary, and could lead to a lot of wasted time in the rewriter and
the planner. Fix by only adding WCO policies when working on the result
relation. Also in process_policies, we should be copying the USING
policies to the WITH CHECK policies on a per-policy basis, fix by moving
the copying up into the per-policy loop.

Lastly, as noted by Dean, we were simply adding policies returned by the
hook provided to the list of quals being AND'd, meaning that they would
actually restrict records returned and there was no option to have
internal policies and hook-based policies work together permissively (as
all internal policies currently work). Instead, explicitly add support
for both permissive and restrictive policies by having a hook for each
and combining the results appropriately. To ensure this is all done
correctly, add a new test module (test_rls_hooks) to test the various
combinations of internal, permissive, and restrictive hook policies.

Largely from Dean Rasheed (thanks!):

CAEZATCVmFUfUOwwhnBTcgi6AquyjQ0-1fyKd0T3xBWJvn+xsFA@mail.gmail.com

Author: Dean Rasheed, though I added the new hooks and test module.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/0bf22e0c8b1114ae37939c500535307abefd38e1

Modified Files
--------------
src/backend/optimizer/plan/planner.c | 45 ++-
src/backend/rewrite/rewriteHandler.c | 127 +++++---
src/backend/rewrite/rowsecurity.c | 260 ++++++++++------
src/include/rewrite/rowsecurity.h | 9 +-
src/test/modules/Makefile | 1 +
src/test/modules/test_rls_hooks/.gitignore | 4 +
src/test/modules/test_rls_hooks/Makefile | 22 ++
src/test/modules/test_rls_hooks/README | 16 +
.../test_rls_hooks/expected/test_rls_hooks.out | 193 ++++++++++++
src/test/modules/test_rls_hooks/rls_hooks.conf | 1 +
.../modules/test_rls_hooks/sql/test_rls_hooks.sql | 157 ++++++++++
src/test/modules/test_rls_hooks/test_rls_hooks.c | 170 +++++++++++
.../modules/test_rls_hooks/test_rls_hooks.control | 4 +
src/test/modules/test_rls_hooks/test_rls_hooks.h | 25 ++
src/test/regress/expected/rowsecurity.out | 309 +++++++++++++++++++-
src/test/regress/sql/rowsecurity.sql | 75 ++++-
16 files changed, 1271 insertions(+), 147 deletions(-)

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

#2Stephen Frost
sfrost@snowman.net
In reply to: Stephen Frost (#1)
Re: pgsql: RLS fixes, new hooks, and new test module

* Stephen Frost (sfrost@snowman.net) wrote:

RLS fixes, new hooks, and new test module

Looks like the buildfarm is unhappy with this.. On first blush, I
believe the installcheck path isn't loading the module from the config
file.. Looking into it.

Thanks,

Stephen

#3Christian Ullrich
chris@chrullrich.net
In reply to: Stephen Frost (#1)
Re: [committers] pgsql: RLS fixes, new hooks, and new test module

* Stephen Frost wrote:

RLS fixes, new hooks, and new test module

The buildfarm says that with -DCLOBBER_CACHE_ALWAYS, the RLS violations
get blamed on the wrong tables. Mostly, they are catalogs (I have seen
pg_opclass, pg_am, and pg_amproc), but some also come up with binary
garbage instead, e.g.

-
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=markhor&dt=2015-04-23%2000%3A00%3A12
-
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=friarbird&dt=2015-04-23%2004%3A20%3A00
-
http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=tick&dt=2015-04-22%2019%3A56%3A53

--
Christian

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

#4Stephen Frost
sfrost@snowman.net
In reply to: Christian Ullrich (#3)
Re: [committers] pgsql: RLS fixes, new hooks, and new test module

Christian,

* Christian Ullrich (chris@chrullrich.net) wrote:

* Stephen Frost wrote:

RLS fixes, new hooks, and new test module

The buildfarm says that with -DCLOBBER_CACHE_ALWAYS, the RLS
violations get blamed on the wrong tables. Mostly, they are catalogs
(I have seen pg_opclass, pg_am, and pg_amproc), but some also come
up with binary garbage instead, e.g.

- http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=markhor&dt=2015-04-23%2000%3A00%3A12
- http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=friarbird&dt=2015-04-23%2004%3A20%3A00
- http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=tick&dt=2015-04-22%2019%3A56%3A53

Yup, thanks, Robert pointed this out on another thread and I'm looking
into it.

Thanks again!

Stephen

#5Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Stephen Frost (#4)
Re: [committers] pgsql: RLS fixes, new hooks, and new test module

On 24 April 2015 at 04:31, Stephen Frost <sfrost@snowman.net> wrote:

Christian,

* Christian Ullrich (chris@chrullrich.net) wrote:

* Stephen Frost wrote:

RLS fixes, new hooks, and new test module

The buildfarm says that with -DCLOBBER_CACHE_ALWAYS, the RLS
violations get blamed on the wrong tables. Mostly, they are catalogs
(I have seen pg_opclass, pg_am, and pg_amproc), but some also come
up with binary garbage instead, e.g.

- http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=markhor&amp;dt=2015-04-23%2000%3A00%3A12
- http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=friarbird&amp;dt=2015-04-23%2004%3A20%3A00
- http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=tick&amp;dt=2015-04-22%2019%3A56%3A53

Yup, thanks, Robert pointed this out on another thread and I'm looking
into it.

Ah, yes it looks like a couple of places in
get_row_security_policies() ought to be making a copy of the relation
name when setting it on the WCO.

Regards,
Dean

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

#6Stephen Frost
sfrost@snowman.net
In reply to: Dean Rasheed (#5)
Re: [committers] pgsql: RLS fixes, new hooks, and new test module

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

On 24 April 2015 at 04:31, Stephen Frost <sfrost@snowman.net> wrote:

* Christian Ullrich (chris@chrullrich.net) wrote:

* Stephen Frost wrote:

RLS fixes, new hooks, and new test module

The buildfarm says that with -DCLOBBER_CACHE_ALWAYS, the RLS
violations get blamed on the wrong tables. Mostly, they are catalogs
(I have seen pg_opclass, pg_am, and pg_amproc), but some also come
up with binary garbage instead, e.g.

- http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=markhor&amp;dt=2015-04-23%2000%3A00%3A12
- http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=friarbird&amp;dt=2015-04-23%2004%3A20%3A00
- http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=tick&amp;dt=2015-04-22%2019%3A56%3A53

Yup, thanks, Robert pointed this out on another thread and I'm looking
into it.

Ah, yes it looks like a couple of places in
get_row_security_policies() ought to be making a copy of the relation
name when setting it on the WCO.

Yup, that was the same conclusion that I came to last night, just hadn't
finished testing before needing sleep. :) Will push a fix shortly for
it.

Thanks!

Stephen