pgsql: Row-Level Security Policies (RLS)

Started by Stephen Frostover 11 years ago19 messageshackers
Jump to latest
#1Stephen Frost
sfrost@snowman.net

Row-Level Security Policies (RLS)

Building on the updatable security-barrier views work, add the
ability to define policies on tables to limit the set of rows
which are returned from a query and which are allowed to be added
to a table. Expressions defined by the policy for filtering are
added to the security barrier quals of the query, while expressions
defined to check records being added to a table are added to the
with-check options of the query.

New top-level commands are CREATE/ALTER/DROP POLICY and are
controlled by the table owner. Row Security is able to be enabled
and disabled by the owner on a per-table basis using
ALTER TABLE .. ENABLE/DISABLE ROW SECURITY.

Per discussion, ROW SECURITY is disabled on tables by default and
must be enabled for policies on the table to be used. If no
policies exist on a table with ROW SECURITY enabled, a default-deny
policy is used and no records will be visible.

By default, row security is applied at all times except for the
table owner and the superuser. A new GUC, row_security, is added
which can be set to ON, OFF, or FORCE. When set to FORCE, row
security will be applied even for the table owner and superusers.
When set to OFF, row security will be disabled when allowed and an
error will be thrown if the user does not have rights to bypass row
security.

Per discussion, pg_dump sets row_security = OFF by default to ensure
that exports and backups will have all data in the table or will
error if there are insufficient privileges to bypass row security.
A new option has been added to pg_dump, --enable-row-security, to
ask pg_dump to export with row security enabled.

A new role capability, BYPASSRLS, which can only be set by the
superuser, is added to allow other users to be able to bypass row
security using row_security = OFF.

Many thanks to the various individuals who have helped with the
design, particularly Robert Haas for his feedback.

Authors include Craig Ringer, KaiGai Kohei, Adam Brightwell, Dean
Rasheed, with additional changes and rework by me.

Reviewers have included all of the above, Greg Smith,
Jeff McCormick, and Robert Haas.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/491c029dbc4206779cf659aa0ff986af7831d2ff

Modified Files
--------------
doc/src/sgml/catalogs.sgml | 100 ++
doc/src/sgml/config.sgml | 40 +
doc/src/sgml/event-trigger.sgml | 18 +
doc/src/sgml/keywords.sgml | 7 +
doc/src/sgml/ref/allfiles.sgml | 3 +
doc/src/sgml/ref/alter_policy.sgml | 135 ++
doc/src/sgml/ref/alter_role.sgml | 3 +
doc/src/sgml/ref/alter_table.sgml | 17 +
doc/src/sgml/ref/create_policy.sgml | 318 ++++
doc/src/sgml/ref/create_role.sgml | 20 +
doc/src/sgml/ref/drop_policy.sgml | 109 ++
doc/src/sgml/reference.sgml | 3 +
src/backend/catalog/Makefile | 2 +-
src/backend/catalog/aclchk.c | 19 +
src/backend/catalog/dependency.c | 9 +
src/backend/catalog/heap.c | 1 +
src/backend/catalog/objectaddress.c | 58 +
src/backend/catalog/system_views.sql | 32 +-
src/backend/commands/Makefile | 2 +-
src/backend/commands/alter.c | 4 +
src/backend/commands/copy.c | 66 +-
src/backend/commands/createas.c | 14 +
src/backend/commands/dropcmds.c | 9 +
src/backend/commands/event_trigger.c | 3 +
src/backend/commands/policy.c | 988 +++++++++++
src/backend/commands/tablecmds.c | 70 +
src/backend/commands/user.c | 46 +
src/backend/executor/execMain.c | 18 +-
src/backend/nodes/copyfuncs.c | 37 +-
src/backend/nodes/equalfuncs.c | 33 +-
src/backend/nodes/outfuncs.c | 1 +
src/backend/nodes/readfuncs.c | 1 +
src/backend/optimizer/plan/planner.c | 3 +
src/backend/optimizer/plan/setrefs.c | 8 +-
src/backend/parser/gram.y | 153 +-
src/backend/rewrite/Makefile | 3 +-
src/backend/rewrite/rewriteHandler.c | 106 +-
src/backend/rewrite/rowsecurity.c | 557 ++++++
src/backend/tcop/utility.c | 31 +
src/backend/utils/adt/acl.c | 3 +-
src/backend/utils/adt/ri_triggers.c | 29 +-
src/backend/utils/cache/plancache.c | 45 +-
src/backend/utils/cache/relcache.c | 28 +-
src/backend/utils/misc/guc.c | 30 +
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/bin/pg_dump/common.c | 4 +
src/bin/pg_dump/pg_backup.h | 1 +
src/bin/pg_dump/pg_backup_archiver.c | 9 +
src/bin/pg_dump/pg_dump.c | 308 +++-
src/bin/pg_dump/pg_dump.h | 22 +-
src/bin/pg_dump/pg_dump_sort.c | 11 +-
src/bin/pg_dump/pg_dumpall.c | 23 +-
src/bin/pg_dump/pg_restore.c | 4 +
src/bin/psql/describe.c | 150 +-
src/bin/psql/tab-complete.c | 158 +-
src/include/catalog/catversion.h | 2 +-
src/include/catalog/dependency.h | 1 +
src/include/catalog/indexing.h | 6 +
src/include/catalog/pg_authid.h | 12 +-
src/include/catalog/pg_class.h | 24 +-
src/include/catalog/pg_rowsecurity.h | 53 +
src/include/commands/policy.h | 33 +
src/include/miscadmin.h | 1 +
src/include/nodes/nodes.h | 2 +
src/include/nodes/parsenodes.h | 33 +
src/include/nodes/plannodes.h | 3 +
src/include/nodes/relation.h | 3 +
src/include/optimizer/planmain.h | 3 +-
src/include/parser/kwlist.h | 1 +
src/include/rewrite/rowsecurity.h | 80 +
src/include/utils/acl.h | 2 +
src/include/utils/plancache.h | 4 +
src/include/utils/rel.h | 2 +
src/test/regress/expected/dependency.out | 20 +-
src/test/regress/expected/privileges.out | 46 +-
src/test/regress/expected/rowsecurity.out | 2236 +++++++++++++++++++++++++
src/test/regress/expected/rules.out | 30 +-
src/test/regress/expected/sanity_check.out | 1 +
src/test/regress/expected/updatable_views.out | 36 +-
src/test/regress/parallel_schedule | 2 +-
src/test/regress/serial_schedule | 1 +
src/test/regress/sql/rowsecurity.sql | 921 ++++++++++
82 files changed, 7279 insertions(+), 152 deletions(-)

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

#2Dimitri Fontaine
dimitri@2ndQuadrant.fr
In reply to: Stephen Frost (#1)
Re: pgsql: Row-Level Security Policies (RLS)

Stephen Frost <sfrost@snowman.net> writes:

Row-Level Security Policies (RLS)

In http://www.postgresql.org/docs/devel/static/sql-createpolicy.html in
Per-Command policies DELETE is mentionned twice, once for UPDATE and
once for DELETE.

Also, I guess it would be useful to provide for some examples on how to
use the feature? I'm not seeing any high level description of it with
use case examples… and I think this feature does warrant a high level
introductory chapter in the manual…

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

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

#3Peter Eisentraut
peter_e@gmx.net
In reply to: Stephen Frost (#1)
Re: pgsql: Row-Level Security Policies (RLS)

On 9/19/14 11:41 AM, Stephen Frost wrote:

Row-Level Security Policies (RLS)

src/include/commands/policy.h needs to include a file that defines
Relation, so that it can stand on its own.

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

#4Stephen Frost
sfrost@snowman.net
In reply to: Peter Eisentraut (#3)
Re: pgsql: Row-Level Security Policies (RLS)

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 9/19/14 11:41 AM, Stephen Frost wrote:

Row-Level Security Policies (RLS)

src/include/commands/policy.h needs to include a file that defines
Relation, so that it can stand on its own.

Hum. I wonder if that's because I got a bit over-aggressive at removing
#includes while reviewing the patch. In any case, will fix.

Thanks!

Stephen

#5Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Stephen Frost (#4)
Re: pgsql: Row-Level Security Policies (RLS)

Stephen Frost wrote:

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 9/19/14 11:41 AM, Stephen Frost wrote:

Row-Level Security Policies (RLS)

src/include/commands/policy.h needs to include a file that defines
Relation, so that it can stand on its own.

Hum. I wonder if that's because I got a bit over-aggressive at removing
#includes while reviewing the patch. In any case, will fix.

This kind of problem is easy to miss. We need some way to have includes
checked and failures reported by buildfarm, or perhaps directly during
compilation. I have a make rule for that somewhere, gcc-dependent
AFAIK, but it's ugly and leaves .gch files behind.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

#6Andres Freund
andres@anarazel.de
In reply to: Alvaro Herrera (#5)
Re: pgsql: Row-Level Security Policies (RLS)

On 2014-09-21 13:41:55 -0300, Alvaro Herrera wrote:

Stephen Frost wrote:

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 9/19/14 11:41 AM, Stephen Frost wrote:

Row-Level Security Policies (RLS)

src/include/commands/policy.h needs to include a file that defines
Relation, so that it can stand on its own.

Hum. I wonder if that's because I got a bit over-aggressive at removing
#includes while reviewing the patch. In any case, will fix.

This kind of problem is easy to miss. We need some way to have includes
checked and failures reported by buildfarm, or perhaps directly during
compilation. I have a make rule for that somewhere, gcc-dependent
AFAIK, but it's ugly and leaves .gch files behind.

IIRC cplupluscheck catches such problem. Annoyingly it doesn't work
properly in vpath builds...

Master causes these warnings for me btw:
/home/andres/src/postgresql/src/backend/commands/policy.c:48:19:
warning: type qualifiers ignored on function return type
[-Wignored-qualifiers]
static const char parse_row_security_command(const char *cmd_name);
^
/home/andres/src/postgresql/src/backend/commands/policy.c:106:1:
warning: type qualifiers ignored on function return type
[-Wignored-qualifiers]
parse_row_security_command(const char *cmd_name)
^

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

#7Stephen Frost
sfrost@snowman.net
In reply to: Andres Freund (#6)
Re: pgsql: Row-Level Security Policies (RLS)

Andres,

On Sunday, September 21, 2014, Andres Freund <andres@2ndquadrant.com> wrote:

IIRC cplupluscheck catches such problem. Annoyingly it doesn't work
properly in vpath builds...

Doh- that is annoying as that's almost exclusively what I use..

Master causes these warnings for me btw:
/home/andres/src/postgresql/src/backend/commands/policy.c:48:19:
warning: type qualifiers ignored on function return type
[-Wignored-qualifiers]
static const char parse_row_security_command(const char *cmd_name);
^
/home/andres/src/postgresql/src/backend/commands/policy.c:106:1:
warning: type qualifiers ignored on function return type
[-Wignored-qualifiers]
parse_row_security_command(const char *cmd_name)
^

Right- those were noted up-thread by Andrew and are also on my list.

Thanks!

Stephen

#8Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Stephen Frost (#1)
Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

Stephen Frost wrote:

Row-Level Security Policies (RLS)

What do we need RowSecurityPolicy->policy_id for? It seems to me that
it is only used to determine whether the policy is the "default deny"
one, so that it can later be removed if a hook adds a different one.
This seems contrived as well as under-documented. Why isn't a boolean
flag sufficient?

(Not an actual review of this patch. I was merely skimming the code.)

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

#9Stephen Frost
sfrost@snowman.net
In reply to: Alvaro Herrera (#8)
Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

What do we need RowSecurityPolicy->policy_id for? It seems to me that
it is only used to determine whether the policy is the "default deny"
one, so that it can later be removed if a hook adds a different one.
This seems contrived as well as under-documented. Why isn't a boolean
flag sufficient?

Thanks for taking a look!

It's also used during relcache updates (see equalPolicy()). That wasn't
originally the case (I had missed adding the necessary bits to relcache
in the original patch), but I wouldn't want to remove that piece now
and, given that it's there, using InvalidOid to indicate when it's the
default-deny policy (and therefore this is no actual Oid) seems
sensible.

Thanks again!

Stephen

#10Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Stephen Frost (#9)
Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

Stephen Frost wrote:

Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

What do we need RowSecurityPolicy->policy_id for? It seems to me that
it is only used to determine whether the policy is the "default deny"
one, so that it can later be removed if a hook adds a different one.
This seems contrived as well as under-documented. Why isn't a boolean
flag sufficient?

Thanks for taking a look!

It's also used during relcache updates (see equalPolicy()).

Hmm, but the policy name is unique also, right? So the policy_id check
is redundant ...

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

#11Stephen Frost
sfrost@snowman.net
In reply to: Alvaro Herrera (#10)
Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Stephen Frost wrote:

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

What do we need RowSecurityPolicy->policy_id for? It seems to me that
it is only used to determine whether the policy is the "default deny"
one, so that it can later be removed if a hook adds a different one.
This seems contrived as well as under-documented. Why isn't a boolean
flag sufficient?

Thanks for taking a look!

It's also used during relcache updates (see equalPolicy()).

Hmm, but the policy name is unique also, right? So the policy_id check
is redundant ...

I don't disagree with that, but surely checking if it's the same OID and
exiting immediately is going to be faster than comparing the policy
names.

Now, looking at the code, I'm actually failing to see a case where we
use the RowSecurityPolicy->policy_name.. Perhaps *that's* what we
should be looking to remove?

Thanks!

Stephen

#12Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Stephen Frost (#11)
Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

On 27 May 2015 at 02:42, Stephen Frost <sfrost@snowman.net> wrote:

Now, looking at the code, I'm actually failing to see a case where we
use the RowSecurityPolicy->policy_name.. Perhaps *that's* what we
should be looking to remove?

If we add support for restrictive policies, it would be possible, and
I think desirable, to report on which policy was violated. For that,
having the policy name would be handy. We might also arguably decide
to enforce restrictive RLS policies in name order, like check
constraints. Of course none of that means it must be kept now, but it
feels like a useful field to keep nonetheless.

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

#13Stephen Frost
sfrost@snowman.net
In reply to: Dean Rasheed (#12)
Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

Dean,

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

On 27 May 2015 at 02:42, Stephen Frost <sfrost@snowman.net> wrote:

Now, looking at the code, I'm actually failing to see a case where we
use the RowSecurityPolicy->policy_name.. Perhaps *that's* what we
should be looking to remove?

If we add support for restrictive policies, it would be possible, and
I think desirable, to report on which policy was violated. For that,
having the policy name would be handy. We might also arguably decide
to enforce restrictive RLS policies in name order, like check
constraints. Of course none of that means it must be kept now, but it
feels like a useful field to keep nonetheless.

I agree that it could be useful, but we really shouldn't have fields in
the current code base which are "just in case".. The one exception
which comes to mind is if we should keep it for use by extensions.
Those operate on an independent cycle from our major releases and would
likely find having the name field useful.

One thing which now occurs to me, however, is that, while the current
coding is fine, using InvalidOid as an indicator for the default-deny
policy, in general, does fall over when we consider policies added by
extensions. Those policies are necessairly going to need to put
InvalidOid into that field, unless they acquire an OID somehow
themselves (it doesn't seem reasonable to make that a requirement,
though I suppose we could..), and, therefore, perhaps we should add a
boolean field which indicates which is the defaultDeny policy anyway and
not use that field for that purpose.

Thoughts?

Thanks!

Stephen

#14Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Stephen Frost (#13)
Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

On 27 May 2015 at 14:51, Stephen Frost <sfrost@snowman.net> wrote:

On 27 May 2015 at 02:42, Stephen Frost <sfrost@snowman.net> wrote:

Now, looking at the code, I'm actually failing to see a case where we
use the RowSecurityPolicy->policy_name.. Perhaps *that's* what we
should be looking to remove?

If we add support for restrictive policies, it would be possible, and
I think desirable, to report on which policy was violated. For that,
having the policy name would be handy. We might also arguably decide
to enforce restrictive RLS policies in name order, like check
constraints. Of course none of that means it must be kept now, but it
feels like a useful field to keep nonetheless.

I agree that it could be useful, but we really shouldn't have fields in
the current code base which are "just in case".. The one exception
which comes to mind is if we should keep it for use by extensions.
Those operate on an independent cycle from our major releases and would
likely find having the name field useful.

Hmm, when I wrote that I had forgotten that we already allow
extensions to add restrictive policies. I think it would be good to
allow those policies to have names, and if they do, to copy those
names to any WCOs created. Then, if a WCO is violated, and it has a
non-NULL policy name associated with it, we should include that in the
error report.

One thing which now occurs to me, however, is that, while the current
coding is fine, using InvalidOid as an indicator for the default-deny
policy, in general, does fall over when we consider policies added by
extensions. Those policies are necessairly going to need to put
InvalidOid into that field, unless they acquire an OID somehow
themselves (it doesn't seem reasonable to make that a requirement,
though I suppose we could..), and, therefore, perhaps we should add a
boolean field which indicates which is the defaultDeny policy anyway and
not use that field for that purpose.

Thoughts?

Actually I think a new boolean field is unnecessary, and probably the
policy_id field is too. Re-reading the code in rowsecurity.c, I think
it could use a bit of refactoring. Essentially what it needs to do
(for permissive policies at least) is just

* fetch a list of internal policies
* fetch a list of external policies
* concatenate them
* if the resulting list is empty, add a default-deny qual and/or WCO

By only building the default-deny qual/WCO at the end, rather than
half-way through and pretending that it's a fully-fledged policy, the
code ought to be simpler.

I might get some time at the weekend, so I can take a look and see if
refactoring it this way works out in practice.

BTW, I just spotted another problem with the current code, which is
that policies from extensions aren't being checked against the current
role (policy->roles is just being ignored). I think it would be neater
and safer to move the check_role_for_policy() check up and apply it to
the entire concatenated list of policies, rather than having the lower
level code have to worry about that.

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

#15Stephen Frost
sfrost@snowman.net
In reply to: Dean Rasheed (#14)
Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

Dean,

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

On 27 May 2015 at 14:51, Stephen Frost <sfrost@snowman.net> wrote:

On 27 May 2015 at 02:42, Stephen Frost <sfrost@snowman.net> wrote:

Now, looking at the code, I'm actually failing to see a case where we
use the RowSecurityPolicy->policy_name.. Perhaps *that's* what we
should be looking to remove?

If we add support for restrictive policies, it would be possible, and
I think desirable, to report on which policy was violated. For that,
having the policy name would be handy. We might also arguably decide
to enforce restrictive RLS policies in name order, like check
constraints. Of course none of that means it must be kept now, but it
feels like a useful field to keep nonetheless.

I agree that it could be useful, but we really shouldn't have fields in
the current code base which are "just in case".. The one exception
which comes to mind is if we should keep it for use by extensions.
Those operate on an independent cycle from our major releases and would
likely find having the name field useful.

Hmm, when I wrote that I had forgotten that we already allow
extensions to add restrictive policies. I think it would be good to
allow those policies to have names, and if they do, to copy those
names to any WCOs created. Then, if a WCO is violated, and it has a
non-NULL policy name associated with it, we should include that in the
error report.

I'm certainly not against that and I agree that it'd be nicer than
simply reporting that there was a violation, but we combine all of the
various pieces together, no? Are we really going to be able to
confidently say which policy was violated? Even if we are able to say
the first which was violated, more than one might be. Is that an issue
we need to address? Perhaps not, but it's something to consider.

One thing which now occurs to me, however, is that, while the current
coding is fine, using InvalidOid as an indicator for the default-deny
policy, in general, does fall over when we consider policies added by
extensions. Those policies are necessairly going to need to put
InvalidOid into that field, unless they acquire an OID somehow
themselves (it doesn't seem reasonable to make that a requirement,
though I suppose we could..), and, therefore, perhaps we should add a
boolean field which indicates which is the defaultDeny policy anyway and
not use that field for that purpose.

Actually I think a new boolean field is unnecessary, and probably the
policy_id field is too. Re-reading the code in rowsecurity.c, I think
it could use a bit of refactoring. Essentially what it needs to do
(for permissive policies at least) is just

* fetch a list of internal policies
* fetch a list of external policies
* concatenate them
* if the resulting list is empty, add a default-deny qual and/or WCO

By only building the default-deny qual/WCO at the end, rather than
half-way through and pretending that it's a fully-fledged policy, the
code ought to be simpler.

I tend to agree.

I might get some time at the weekend, so I can take a look and see if
refactoring it this way works out in practice.

That would certainly be fantastic and most appreciated. Beyond that, we
have the add-a-default-deny-policy logic in multiple places, as I
recall, and I wonder if that's overkill and done out of paranoia rather
than sound judgement. I'm certainly not suggesting that we take
unnecessary risks, and so perhaps we should keep it, but I do think it's
something which should be reviewed and considered (I've been planning to
do so for a while, in fact).

BTW, I just spotted another problem with the current code, which is
that policies from extensions aren't being checked against the current
role (policy->roles is just being ignored). I think it would be neater
and safer to move the check_role_for_policy() check up and apply it to
the entire concatenated list of policies, rather than having the lower
level code have to worry about that.

Excellent point... We should address that and your proposed approach
sounds reasonable to me. If you're able to work on that this weekend,
I'd be happy to review next week.

Thanks!

Stephen

#16Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Stephen Frost (#15)
Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

On 28 May 2015 at 08:51, Stephen Frost <sfrost@snowman.net> wrote:

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

Actually I think a new boolean field is unnecessary, and probably the
policy_id field is too. Re-reading the code in rowsecurity.c, I think
it could use a bit of refactoring. Essentially what it needs to do
(for permissive policies at least) is just

* fetch a list of internal policies
* fetch a list of external policies
* concatenate them
* if the resulting list is empty, add a default-deny qual and/or WCO

By only building the default-deny qual/WCO at the end, rather than
half-way through and pretending that it's a fully-fledged policy, the
code ought to be simpler.

I tend to agree.

I might get some time at the weekend, so I can take a look and see if
refactoring it this way works out in practice.

That would certainly be fantastic and most appreciated. Beyond that, we
have the add-a-default-deny-policy logic in multiple places, as I
recall, and I wonder if that's overkill and done out of paranoia rather
than sound judgement. I'm certainly not suggesting that we take
unnecessary risks, and so perhaps we should keep it, but I do think it's
something which should be reviewed and considered (I've been planning to
do so for a while, in fact).

So I had a go at refactoring the code in rowsecurity.c to simplify the
default-deny policy handling, as described. In the end my changes were
quite extensive, so I'll understand if you don't have time to review
it, but I think that it's worth it for the improved code clarity,
simplicity and more detailed error messages for restrictive policies.
In any case, there are a couple of bug fixes in there that ought to be
considered.

The main changes are:

* pull_row_security_policies() is replaced by
get_policies_for_relation(), whose remit is to fetch both the internal
and external policies that apply to the relation. It returns the
permissive and restrictive policies as separate lists, each of which
contains any internal policies followed by any external policies
(except of course that internal restrictive policies aren't possible
yet). Unlike pull_row_security_policies() this does not try to build a
default-deny policy, and instead may return empty lists. All the
returned policies are filtered according to the current role, thus
fixing the bug that external policies weren't being filtered.

* process_policies() is replaced by build_security_quals() and
build_with_check_options(), whose remits are to build the lists of
security quals and WithCheckOption checks from the lists of permissive
and restrictive policies. These new functions now have sole
responsibility for handling the default-deny case if there are no
explicit policies to apply, which means that it is no longer necessary
to build RowSecurityPolicy objects representing the default-deny case
(hence no more InvalidOid policy_id).

* get_row_security_policies() is now greatly simplified, since it no
longer has to merge internal and external policies, or worry about
default-deny policies. The guts of the work is now done by the new
functions described above.

* The way that ON CONFLICT DO UPDATE is handled is also simplified ---
rather than recursively calling get_row_security_policies() and
turning the returned list of security quals into WCOs, it now simply
calls build_with_check_options() a couple more times to build the
additional kinds of WCOs needed for the auxiliary UPDATE.

* RelationBuildRowSecurity() no longer builds a default-deny policy,
and the resulting policy list is now allowed to be empty. This removes
the need for RowSecurityPolicy's policy_id field. Actually the old
code had 3 separate places with default-deny policy handling. That is
now all localised in the functions that build security quals and WCOs
from the policy lists.

* Finally, WCOs for restrictive policies now report the name of the
policy violated. Of course this means that the actual error might
depend on the order in which the policies are checked. I've tackled
that in the same way as was recently used for CHECK constraints, which
is to always check restrictive policies in a well-defined order (name
order) so that self-test output is predictable.

Overall, I think this reduces the code complexity (although I think
the total line count is about the same), and there is now a clearer
separation of concerns across the various functions. Also I think it
will make it easier to add support for internal restrictive policies
in the future.

While going through this, I spotted another issue --- in a DML query
with additional non-target relations, such as UPDATE t1 .. FROM t2 ..,
the old code was checking the UPDATE policies of both t1 and t2, but
really I think it ought to be checking the SELECT policies of t2 (in
the same way as this query requires SELECT table permissions on t2,
not UPDATE permissions). I've changed that and added new regression
tests to test that change.

Regards,
Dean

Attachments:

rls-refactoring.patchtext/x-patch; charset=US-ASCII; name=rls-refactoring.patchDownload+927-856
#17Joe Conway
mail@joeconway.com
In reply to: Dean Rasheed (#16)
Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/01/2015 02:21 AM, Dean Rasheed wrote:

While going through this, I spotted another issue --- in a DML
query with additional non-target relations, such as UPDATE t1 ..
FROM t2 .., the old code was checking the UPDATE policies of both
t1 and t2, but really I think it ought to be checking the SELECT
policies of t2 (in the same way as this query requires SELECT table
permissions on t2, not UPDATE permissions). I've changed that and
added new regression tests to test that change.

I assume the entire refactoring patch needs a fair bit of work to
rebase against current HEAD, but I picked out the attached to address
just the above issue. Does this look correct, and if so does it make
sense to apply at least this part right now?

Thanks,

Joe

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVuXFuAAoJEDfy90M199hlOMkP/RZoBU0MJF64GjHfuLVa3T5w
PnDrnIoLBMOgOzkXrI+Ov0w0ESXltNvYjyIxkuyaB5PuoeDOdyYnnzbpPe7hH4pv
zDjSinJnfNmFEcm0UjBzuBiSH0dv52PEIToexTKu6SnjvH3Co/Hk4Ar2uZ0r7bRF
krl+Dd4kZX1uuRIigsSFqy873C79m3o11Szs5aW8c5od9adGxSvRHqZNf/UIDIbZ
CxAo0E3Tlw0DmGl1cw1tdN8gWMzmvx5dQ0ih3+0/hkVUqN9p3Pg8BGajSvxxFlb2
l4+6RZGUw5ZTpJxRZf/zPc4updhq0zf/Z5g7GUYddrhO29eLS6al2ySlb7HL5G9M
VPMjEzXuhFwhxSMdgHfz8UQh82KuNENMTKp81BvtIgZ7w86A9lWrKxCLaVMGhi8m
MnfCQ4cdOmnE2vEHi0Ue3Dg/rvYO8QpVW0JKDOdzoqPErC7to9vorXI5X3vcfLbF
fYfiJe6wUwbDEdxh/z0oKVFxWlf2NRk4pd+jZ7C+ibraLIwgEgZe7G4GEje5LxSP
h4zTfx0sj3IyrvqziurO/aZqIBXBZEsm3Gv6OQs26C5Xvkx/QmgROB42lHwcDYri
BTk6+uzNYKbX+kW56vEY0f0WMTLYZzc6jZRIVr3s+aLeyG0P9acY/n3uY1xBFCZZ
Xb7gmepAN3rY1CF7By9o
=e5hz
-----END PGP SIGNATURE-----

Attachments:

20150729.00-rls-non-tgt-rel-v0.patchtext/x-diff; name=20150729.00-rls-non-tgt-rel-v0.patchDownload+135-10
#18Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Joe Conway (#17)
Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

On 30 July 2015 at 01:35, Joe Conway <mail@joeconway.com> wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/01/2015 02:21 AM, Dean Rasheed wrote:

While going through this, I spotted another issue --- in a DML
query with additional non-target relations, such as UPDATE t1 ..
FROM t2 .., the old code was checking the UPDATE policies of both
t1 and t2, but really I think it ought to be checking the SELECT
policies of t2 (in the same way as this query requires SELECT table
permissions on t2, not UPDATE permissions). I've changed that and
added new regression tests to test that change.

I assume the entire refactoring patch needs a fair bit of work to
rebase against current HEAD,

Actually, there haven't been any conflicting changes so far, so a git
rebase was able to automatically merge correctly -- new patch
attached, with some minor comment rewording (not affecting the bug-fix
part).

Even so, I agree that it makes sense to apply the bug-fix separately,
since it's not really anything to do with the refactoring.

but I picked out the attached to address
just the above issue. Does this look correct, and if so does it make
sense to apply at least this part right now?

Looks correct to me.

Thanks.

Regards,
Dean

Attachments:

rls-refactoring.patchtext/x-patch; charset=US-ASCII; name=rls-refactoring.patchDownload+559-444
#19Joe Conway
mail@joeconway.com
In reply to: Dean Rasheed (#18)
Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/30/2015 06:52 AM, Dean Rasheed wrote:

On 30 July 2015 at 01:35, Joe Conway <mail@joeconway.com> wrote:

On 06/01/2015 02:21 AM, Dean Rasheed wrote:

While going through this, I spotted another issue --- in a DML
query with additional non-target relations, such as UPDATE t1
.. FROM t2 .., the old code was checking the UPDATE policies of
both t1 and t2, but really I think it ought to be checking the
SELECT policies of t2 (in the same way as this query requires
SELECT table permissions on t2, not UPDATE permissions). I've
changed that and added new regression tests to test that
change.

I assume the entire refactoring patch needs a fair bit of work
to rebase against current HEAD,

Actually, there haven't been any conflicting changes so far, so a
git rebase was able to automatically merge correctly -- new patch
attached, with some minor comment rewording (not affecting the
bug-fix part).

Good to hear.

Even so, I agree that it makes sense to apply the bug-fix
separately, since it's not really anything to do with the
refactoring.

but I picked out the attached to address just the above issue.
Does this look correct, and if so does it make sense to apply at
least this part right now?

Looks correct to me.

Thanks -- committed and pushed to HEAD and 9.5

- --
Joe Conway
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJVulOVAAoJEDfy90M199hlURUP/2TF7r77taLPhQtEk3CFFQEn
mrt90N4DJ43VwGwC7mfdBsKXoJ+3jF3Hpghw/7ulI731/Io7C514gYDDvwGkrJWu
vK3vzXEQu9CIIfh97CsJ5mmenaaxrF9ZSaWDbYvQQMwMnxUS5CGWwR6VSp+NhCZ9
cnfA7idbxNjfBsjG0nQvtSgV/HOp0tP3A6dlYPTXPiIzbT9IiOpxWPwoQYgMSTcP
TBgK5MHG5JWrK1Bcg3BlQzYZefKEwN+LGU6zal4P4AjM14FfyMKfMc9A6EP9vtRl
jFekmRUddbXxWddl0ZSSV5BY9BLTRL2CZFhMQNQ9xKDlsK1cuYORN4v+TgRCjPKy
PdMH2tgndWsNNRTmj/vWUJxMXnHARl8MmtY8pau5Z6PKNUcASqd5Xq+zfKxOw8vf
lS8c4eRsLcCD+TZ1rv5K6VULmwBViU0gKP6Xs62yDHsz/Zwvt2ar1fW/25peohhb
m4j8vOCsdik9DDcJf6dF8sbb/z0FVh+JQqWhjbSJYioX9BOw/1AoNbi44wS7HzV1
vdhWx6qGWZnxi5qtb9j8BU0eFr/Q65kU3hsp2smRA/IK0bCQaO08rDQlPYsIHq10
SFodULNKFzpGvkzQ2Yqv1oyJ6jMvLtWgr9vBUZvPo8OHUyAkR8kfrZyzWyr/L/Mm
6jcuFNdYSS5F7W/S7ost
=GJw9
-----END PGP SIGNATURE-----

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