Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

* Stephen Frost (sfr...@snowman.net) wrote:
> Updated patch attached.

Erp, actually attached this time.

Thanks again!

Stephen
From 27e5fdac801cecc5ac33daccf979bbc59458dbbc Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 1 Sep 2016 02:11:30 -0400
Subject: [PATCH] Add support for restrictive RLS policies

We have had support for restrictive RLS policies since 9.5, but they
were only available through extensions which use the appropriate hooks.
This adds support into the grammer, catalog, psql and pg_dump for
restrictive RLS policies, thus reducing the cases where an extension is
necessary.

In passing, also move away from using "AND"d and "OR"d in comments.
As pointed out by Alvaro, it's not really appropriate to attempt
to make verbs out of "AND" and "OR", so reword those comments which
attempted to.

Reviewed By: Jeevan Chalke, Dean Rasheed
Discussion: https://postgr.es/m/20160901063404.gy4...@tamriel.snowman.net
---
 doc/src/sgml/catalogs.sgml|  13 ++
 doc/src/sgml/ddl.sgml |  58 +-
 doc/src/sgml/ref/alter_policy.sgml|   7 +-
 doc/src/sgml/ref/create_policy.sgml   |  38 
 src/backend/catalog/system_views.sql  |   6 +
 src/backend/commands/policy.c |   9 +
 src/backend/nodes/copyfuncs.c |   1 +
 src/backend/nodes/equalfuncs.c|   1 +
 src/backend/parser/gram.y |  43 +++--
 src/backend/rewrite/rowsecurity.c |  54 +++---
 src/bin/pg_dump/pg_dump.c |  69 +---
 src/bin/pg_dump/pg_dump.h |   3 +-
 src/bin/pg_dump/t/002_pg_dump.pl  |  33 +++-
 src/bin/psql/describe.c   | 100 ---
 src/bin/psql/tab-complete.c   |  29 ++-
 src/include/catalog/pg_policy.h   |  16 +-
 src/include/nodes/parsenodes.h|   1 +
 src/include/rewrite/rowsecurity.h |   1 +
 src/test/regress/expected/rowsecurity.out | 284 --
 src/test/regress/expected/rules.out   |   4 +
 src/test/regress/sql/rowsecurity.sql  |  45 -
 21 files changed, 665 insertions(+), 150 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 561e228..c4246dc 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -4748,6 +4748,13 @@
  
 
  
+  polpermissive
+  boolean
+  
+  Is the policy permissive or restrictive?
+ 
+
+ 
   polroles
   oid[]
   pg_authid.oid
@@ -8438,6 +8445,12 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
   Name of policy
  
  
+  polpermissive
+  text
+  
+  Is the policy permissive or restrictive?
+ 
+ 
   roles
   name[]
   
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 157512c..7e1bc0e 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -1599,9 +1599,11 @@ REVOKE ALL ON accounts FROM PUBLIC;
 
   
When multiple policies apply to a given query, they are combined using
-   OR, so that a row is accessible if any policy allows
-   it.  This is similar to the rule that a given role has the privileges
-   of all roles that they are a member of.
+   either OR (for permissive policies, which are the
+   default) or using AND (for restrictive policies).
+   This is similar to the rule that a given role has the privileges
+   of all roles that they are a member of.  Permissive vs. restrictive
+   policies are discussed further below.
   
 
   
@@ -1764,6 +1766,56 @@ UPDATE 1
 
 
   
+   All of the policies constructed thus far have been permissive policies,
+   meaning that when multiple policies are applied they are combined using
+   the "OR" boolean operator.  While permissive policies can be constructed
+   to only allow access to rows in the intended cases, it can be simpler to
+   combine permissive policies with restrictive policies (which the records
+   must pass and which are combined using the "AND" boolean operator).
+   Building on the example above, we add a restrictive policy to require
+   the administrator to be connected over a local unix socket to access the
+   records of the passwd table:
+  
+
+
+CREATE POLICY admin_local_only ON passwd AS RESTRICTIVE TO admin
+USING (pg_catalog.inet_client_addr() IS NULL);
+
+
+  
+   We can then see that an administrator connecting over a network will not
+   see any records, due to the restrictive policy:
+  
+
+
+= SELECT current_user;
+ current_user 
+--
+ admin
+(1 row)
+
+= select inet_client_addr();
+ inet_client_addr 
+--
+ 127.0.0.1
+(1 row)
+
+= SELECT current_user;
+ current_user 
+--
+ admin
+(1 row)
+
+= TABLE passwd;
+ user_name | pwhash | uid | gid | real_name | home_phone | extra_info | home_dir | shell
+---++-+-+---+++--+---
+(0 rows)
+
+= UPDATE passwd set pwhash = NULL;
+UPDATE 0
+
+
+ 

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

Dean,

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> This note reads a little awkwardly to me. I think I would write it as:
> 
> Note that ALTER POLICY only allows the set of roles
> to which the policy applies and the USING and
> WITH CHECK expressions to be modified.  To change
> other properties of a policy, such as the command to which it applies
> or whether it is permissive or restrictive, the policy must be dropped
> and recreated.

Done.

[...]
> really makes sense in this context). So perhaps an additional note
> along the lines of:
> 
> Note that there needs to be at least one permissive policy to grant
> access to records before restrictive policies can be usefully used to
> reduce that access. If only restrictive policies exist, then no records
> will be accessible. When a mix of permissive and restrictive policies
> are present, a record is only accessible if at least one of the
> permissive policies passes, in addition to all the restrictive
> policies.

Done.

> Also, I don't think it's necessary to keep quoting "restrictive" and
> "permissive" here.

Quotes removed.

> In get_policies_for_relation(), after the loop that does this:
[...]
> I think it should sort the restrictive policies by name, for the same
> reason that hook-restrictive policies are sorted -- so that the WITH
> CHECK expressions are checked in a well-defined order (which was
> chosen to be consistent with the order of checking of other similar
> things, like CHECK constraints and triggers). I also think that this
> should be a separate sort step from the sort for hook policies,
> inserted just after the loop above, so that the order is all internal
> policies sorted by name, followed by all hook policies sorted by name,
> to be consistent with the existing principle that hook policies are
> applied after internal policies.

Done, adjusted the comments a bit also and added to the regression tests
to test that the sorting is happening as expected.

> I looked through this in a little more detail and I found one other
> issue: the documentation for the system catalog table pg_policy and
> the view pg_policies needs to be updated to include the new columns
> that have been added.

I had noticed this also while going through it again, but thanks again
for the thorough review!

> Other than that, it all looks good to me, subject to the previous comments.

Updated patch attached.

I'll push this in a bit.

Thanks to all who helped!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Add support for restrictive RLS policies

[Dean Rasheed]

Stephen,

I looked through this in a little more detail and I found one other
issue: the documentation for the system catalog table pg_policy and
the view pg_policies needs to be updated to include the new columns
that have been added.

Other than that, it all looks good to me, subject to the previous comments.

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

Re: [HACKERS] Add support for restrictive RLS policies

[Haribabu Kommi]

On Fri, Dec 2, 2016 at 2:09 AM, Stephen Frost  wrote:

> Dean,
>
> * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> > On 1 December 2016 at 14:38, Stephen Frost  wrote:
> > > * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> > >> In get_policies_for_relation() ...
> > >> ... I think it should sort the restrictive policies by name
> > >
> > > Hmmm, is it really the case that the quals will always end up being
> > > evaluated in that order though?  Isn't order_qual_clauses() going to
> end
> > > up changing the order based on the relative cost?  If the cost is the
> > > same it should maintain the order, but even that could change in the
> > > future based on the comments, no?  In short, I'm not entirely sure that
> > > we actually want to be required to always evaluate the quals in order
> of
> > > policy name and we might get complaints if we happen to make that work
> > > today and it ends up being changed later.
> >
> > No, this isn't about the quals that get put into the WHERE clause of
> > the resulting queries. As you say, order_quals_clauses() is going to
> > re-order those anyway. This is about the WithCheckOption's that get
> > generated for UPDATEs and INSERTs, and having those checked in a
> > predictable order. The main advantage to that is to guarantee a
> > predictable error message from self tests that attempt to insert
> > invalid data. This is basically the same as what was done for CHECK
> > constraints in e5f455f59fed0632371cddacddd79895b148dc07.
>
> You know, you said that in your email, and I read it and it made sense
> to me, and then I went off to do something else and came back and
> completely forgot that you were referring to the WITH CHECK case.
>
> You're right, we should order the WithCheckOptions.  I'll update the
> patch accordingly.
>


Moved to next CF with "waiting on author" status.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

Dean,

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> On 1 December 2016 at 14:38, Stephen Frost  wrote:
> > * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> >> In get_policies_for_relation() ...
> >> ... I think it should sort the restrictive policies by name
> >
> > Hmmm, is it really the case that the quals will always end up being
> > evaluated in that order though?  Isn't order_qual_clauses() going to end
> > up changing the order based on the relative cost?  If the cost is the
> > same it should maintain the order, but even that could change in the
> > future based on the comments, no?  In short, I'm not entirely sure that
> > we actually want to be required to always evaluate the quals in order of
> > policy name and we might get complaints if we happen to make that work
> > today and it ends up being changed later.
> 
> No, this isn't about the quals that get put into the WHERE clause of
> the resulting queries. As you say, order_quals_clauses() is going to
> re-order those anyway. This is about the WithCheckOption's that get
> generated for UPDATEs and INSERTs, and having those checked in a
> predictable order. The main advantage to that is to guarantee a
> predictable error message from self tests that attempt to insert
> invalid data. This is basically the same as what was done for CHECK
> constraints in e5f455f59fed0632371cddacddd79895b148dc07.

You know, you said that in your email, and I read it and it made sense
to me, and then I went off to do something else and came back and
completely forgot that you were referring to the WITH CHECK case.

You're right, we should order the WithCheckOptions.  I'll update the
patch accordingly.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Add support for restrictive RLS policies

[Dean Rasheed]

On 1 December 2016 at 14:38, Stephen Frost  wrote:
> * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
>> In get_policies_for_relation() ...
>> ... I think it should sort the restrictive policies by name
>
> Hmmm, is it really the case that the quals will always end up being
> evaluated in that order though?  Isn't order_qual_clauses() going to end
> up changing the order based on the relative cost?  If the cost is the
> same it should maintain the order, but even that could change in the
> future based on the comments, no?  In short, I'm not entirely sure that
> we actually want to be required to always evaluate the quals in order of
> policy name and we might get complaints if we happen to make that work
> today and it ends up being changed later.
>

No, this isn't about the quals that get put into the WHERE clause of
the resulting queries. As you say, order_quals_clauses() is going to
re-order those anyway. This is about the WithCheckOption's that get
generated for UPDATEs and INSERTs, and having those checked in a
predictable order. The main advantage to that is to guarantee a
predictable error message from self tests that attempt to insert
invalid data. This is basically the same as what was done for CHECK
constraints in e5f455f59fed0632371cddacddd79895b148dc07.

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

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

Dean,

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> On 30 November 2016 at 21:54, Stephen Frost  wrote:
> > Unless there's further comments, I'll plan to push this sometime
> > tomorrow.
> 
> Sorry I didn't have time to look at this properly. I was intending to,
> but my day job just keeps getting in the way.

No worries, took me a while to get back to adding the requested
documentation too.

> I do have a couple of
> comments relating to the documentation and one relating to the code:

Thanks!

> -   row-level security policy.
> +   row-level security policy.  Note that only the set of roles which the
> +   policy applies to and the USING and
> +   WITH CHECK expressions are able to be changed using
> +   ALTER POLICY.  To change other properties of a policy,
> +   such as the command it is applied for or if it is a permissive or
> +   restrictive policy, the policy must be dropped and recreated.
> 
> This note reads a little awkwardly to me. I think I would write it as:
> 
> Note that ALTER POLICY only allows the set of roles
> to which the policy applies and the USING and
> WITH CHECK expressions to be modified.  To change
> other properties of a policy, such as the command to which it applies
> or whether it is permissive or restrictive, the policy must be dropped
> and recreated.

I agree, that's better, will update.

> +PERMISSIVE
> +
> + 
> +  Specify that the policy is to be created as a "permissive" policy.
> +  All "permissive" policies which are applicable to a given query will
> +  be combined together using the boolean "OR" operator.  By creating
> +  "permissive" policies, administrators can add to the set of records
> +  which can be accessed.  Policies are PERMISSIVE by default.
> + 
> +
> +   
> +
> +   
> +RESTRICTIVE
> +
> + 
> +  Specify that the policy is to be created as a "restrictive" policy.
> +  All "restrictive" policies which are applicable to a given query will
> +  be combined together using the boolean "AND" operator.  By creating
> +  "restrictive" policies, administrators can reduce the set of records
> +  which can be accessed as all "restrictive" policies must be passed for
> +  each record.
> + 
> +
> +   
> 
> I don't think this or anywhere else makes it entirely clear that the
> user needs to have at least one permissive policy in addition to any
> restrictive policies for this to be useful. I think this section is
> probably a good place to mention that, since it's probably the first
> place people will read about restrictive policies. I think it also
> needs to be spelled out exactly how a mix of permissive and
> restrictive policies are combined, because there is more than one way
> to combine a set of quals with ANDs and ORs (although only one way
> really makes sense in this context). So perhaps an additional note
> along the lines of:
> 
> Note that there needs to be at least one permissive policy to grant
> access to records before restrictive policies can be usefully used to
> reduce that access. If only restrictive policies exist, then no records
> will be accessible. When a mix of permissive and restrictive policies
> are present, a record is only accessible if at least one of the
> permissive policies passes, in addition to all the restrictive
> policies.
> 
> Also, I don't think it's necessary to keep quoting "restrictive" and
> "permissive" here.

Works for me, I'll add that, and remove the quotes around restrictive
and permissive.

> In get_policies_for_relation(), after the loop that does this:
> 
> -*permissive_policies = lappend(*permissive_policies, policy);
> +{
> +if (policy->permissive)
> +*permissive_policies = lappend(*permissive_policies, policy);
> +else
> +*restrictive_policies = lappend(*restrictive_policies, 
> policy);
> +}
> 
> I think it should sort the restrictive policies by name, for the same
> reason that hook-restrictive policies are sorted -- so that the WITH
> CHECK expressions are checked in a well-defined order (which was
> chosen to be consistent with the order of checking of other similar
> things, like CHECK constraints and triggers). I also think that this
> should be a separate sort step from the sort for hook policies,
> inserted just after the loop above, so that the order is all internal
> policies sorted by name, followed by all hook policies sorted by name,
> to be consistent with the existing principle that hook policies are
> applied after internal policies.

Hmmm, is it really the case that the quals will always end up being
evaluated in that order though?  Isn't order_qual_clauses() going to end
up changing the order based on the relative cost?  If the cost is the
same it should maintain the order, but even that could change in the
future based on the comments, no?  In 

Re: [HACKERS] Add support for restrictive RLS policies

[Dean Rasheed]

On 30 November 2016 at 21:54, Stephen Frost  wrote:
> Unless there's further comments, I'll plan to push this sometime
> tomorrow.
>

Sorry I didn't have time to look at this properly. I was intending to,
but my day job just keeps getting in the way. I do have a couple of
comments relating to the documentation and one relating to the code:


-   row-level security policy.
+   row-level security policy.  Note that only the set of roles which the
+   policy applies to and the USING and
+   WITH CHECK expressions are able to be changed using
+   ALTER POLICY.  To change other properties of a policy,
+   such as the command it is applied for or if it is a permissive or
+   restrictive policy, the policy must be dropped and recreated.

This note reads a little awkwardly to me. I think I would write it as:

Note that ALTER POLICY only allows the set of roles
to which the policy applies and the USING and
WITH CHECK expressions to be modified.  To change
other properties of a policy, such as the command to which it applies
or whether it is permissive or restrictive, the policy must be dropped
and recreated.


+PERMISSIVE
+
+ 
+  Specify that the policy is to be created as a "permissive" policy.
+  All "permissive" policies which are applicable to a given query will
+  be combined together using the boolean "OR" operator.  By creating
+  "permissive" policies, administrators can add to the set of records
+  which can be accessed.  Policies are PERMISSIVE by default.
+ 
+
+   
+
+   
+RESTRICTIVE
+
+ 
+  Specify that the policy is to be created as a "restrictive" policy.
+  All "restrictive" policies which are applicable to a given query will
+  be combined together using the boolean "AND" operator.  By creating
+  "restrictive" policies, administrators can reduce the set of records
+  which can be accessed as all "restrictive" policies must be passed for
+  each record.
+ 
+
+   

I don't think this or anywhere else makes it entirely clear that the
user needs to have at least one permissive policy in addition to any
restrictive policies for this to be useful. I think this section is
probably a good place to mention that, since it's probably the first
place people will read about restrictive policies. I think it also
needs to be spelled out exactly how a mix of permissive and
restrictive policies are combined, because there is more than one way
to combine a set of quals with ANDs and ORs (although only one way
really makes sense in this context). So perhaps an additional note
along the lines of:

Note that there needs to be at least one permissive policy to grant
access to records before restrictive policies can be usefully used to
reduce that access. If only restrictive policies exist, then no records
will be accessible. When a mix of permissive and restrictive policies
are present, a record is only accessible if at least one of the
permissive policies passes, in addition to all the restrictive
policies.

Also, I don't think it's necessary to keep quoting "restrictive" and
"permissive" here.


In get_policies_for_relation(), after the loop that does this:

-*permissive_policies = lappend(*permissive_policies, policy);
+{
+if (policy->permissive)
+*permissive_policies = lappend(*permissive_policies, policy);
+else
+*restrictive_policies = lappend(*restrictive_policies, policy);
+}

I think it should sort the restrictive policies by name, for the same
reason that hook-restrictive policies are sorted -- so that the WITH
CHECK expressions are checked in a well-defined order (which was
chosen to be consistent with the order of checking of other similar
things, like CHECK constraints and triggers). I also think that this
should be a separate sort step from the sort for hook policies,
inserted just after the loop above, so that the order is all internal
policies sorted by name, followed by all hook policies sorted by name,
to be consistent with the existing principle that hook policies are
applied after internal policies.

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

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

* Stephen Frost (sfr...@snowman.net) wrote:
> * Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote:
> > 4. It will be good if we have an example for this in section
> > "5.7. Row Security Policies"
> 
> I haven't added one yet, but will plan to do so.

I've now added and cleaned up the Row Security Policies section to
discuss restrictive policies and to include an example.  I also added
some documentation to ALTER POLICY.

I've also re-based the patch to current master, but the only conflict
was in the pg_dump regression test definition, which was easily
corrected.

Unless there's further comments, I'll plan to push this sometime
tomorrow.

Thanks!

Stephen
From 066575b8f5112c9750a9005e2f50219d044a980c Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 1 Sep 2016 02:11:30 -0400
Subject: [PATCH] Add support for restrictive RLS policies

We have had support for restrictive RLS policies since 9.5, but they
were only available through extensions which use the appropriate hooks.
This adds support into the grammer, catalog, psql and pg_dump for
restrictive RLS policies, thus reducing the cases where an extension is
necessary.

In passing, also move away from using "AND"d and "OR"d in comments.
As pointed out by Alvaro, it's not really appropriate to attempt
to make verbs out of "AND" and "OR", so reword those comments which
attempted to.

Reviewed By: Jeevan Chalke
Discussion: https://postgr.es/m/20160901063404.gy4...@tamriel.snowman.net
---
 doc/src/sgml/ddl.sgml |  58 ++-
 doc/src/sgml/ref/alter_policy.sgml|   7 +-
 doc/src/sgml/ref/create_policy.sgml   |  28 
 src/backend/catalog/system_views.sql  |   6 +
 src/backend/commands/policy.c |   9 +
 src/backend/nodes/copyfuncs.c |   1 +
 src/backend/nodes/equalfuncs.c|   1 +
 src/backend/parser/gram.y |  43 +++--
 src/backend/rewrite/rowsecurity.c |  42 +++--
 src/bin/pg_dump/pg_dump.c |  69 +---
 src/bin/pg_dump/pg_dump.h |   3 +-
 src/bin/pg_dump/t/002_pg_dump.pl  |  33 +++-
 src/bin/psql/describe.c   | 100 ---
 src/bin/psql/tab-complete.c   |  29 +++-
 src/include/catalog/pg_policy.h   |  16 +-
 src/include/nodes/parsenodes.h|   1 +
 src/include/rewrite/rowsecurity.h |   1 +
 src/test/regress/expected/rowsecurity.out | 264 --
 src/test/regress/expected/rules.out   |   4 +
 src/test/regress/sql/rowsecurity.sql  |  32 +++-
 20 files changed, 599 insertions(+), 148 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 157512c..7e1bc0e 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -1599,9 +1599,11 @@ REVOKE ALL ON accounts FROM PUBLIC;
 
   
When multiple policies apply to a given query, they are combined using
-   OR, so that a row is accessible if any policy allows
-   it.  This is similar to the rule that a given role has the privileges
-   of all roles that they are a member of.
+   either OR (for permissive policies, which are the
+   default) or using AND (for restrictive policies).
+   This is similar to the rule that a given role has the privileges
+   of all roles that they are a member of.  Permissive vs. restrictive
+   policies are discussed further below.
   
 
   
@@ -1764,6 +1766,56 @@ UPDATE 1
 
 
   
+   All of the policies constructed thus far have been permissive policies,
+   meaning that when multiple policies are applied they are combined using
+   the "OR" boolean operator.  While permissive policies can be constructed
+   to only allow access to rows in the intended cases, it can be simpler to
+   combine permissive policies with restrictive policies (which the records
+   must pass and which are combined using the "AND" boolean operator).
+   Building on the example above, we add a restrictive policy to require
+   the administrator to be connected over a local unix socket to access the
+   records of the passwd table:
+  
+
+
+CREATE POLICY admin_local_only ON passwd AS RESTRICTIVE TO admin
+USING (pg_catalog.inet_client_addr() IS NULL);
+
+
+  
+   We can then see that an administrator connecting over a network will not
+   see any records, due to the restrictive policy:
+  
+
+
+= SELECT current_user;
+ current_user 
+--
+ admin
+(1 row)
+
+= select inet_client_addr();
+ inet_client_addr 
+--
+ 127.0.0.1
+(1 row)
+
+= SELECT current_user;
+ current_user 
+--
+ admin
+(1 row)
+
+= TABLE passwd;
+ user_name | pwhash | uid | gid | real_name | home_phone | extra_info | home_dir | shell
+---++-+-+---+++--+---
+(0 rows)
+
+= UPDATE passwd set pwhash = NULL;
+UPDATE 0
+
+
+  
Referential integrity checks, such as unique or primary key constraints
and foreign key references, always bypass row security to 

Re: [HACKERS] Add support for restrictive RLS policies

[Michael Paquier]

On Thu, Sep 29, 2016 at 7:18 PM, Jeevan Chalke
 wrote:
> Hi Stephen,
>
>
>> > 4. It will be good if we have an example for this in section
>> > "5.7. Row Security Policies"
>>
>> I haven't added one yet, but will plan to do so.
>>
> I think you are going to add this in this patch itself, right?
>
> I have reviewed your latest patch and it fixes almost all my review
> comments.
> Also I am agree with your responses for couple of comments like response on
> ALTER POLICY and tab completion. No issues with that.
>
> However in documentation, PERMISSIVE and RESTRICTIVE are actually literals
> and not parameters as such.  Also can we combine these two options into one
> like below (similar to how we document CASCADE and RESTRICT for DROP
> POLICY):
>
>
> PERMISSIVE
> RESTRICTIVE
>
> 
>  
> ... explain PERMISSIVE ...
>  
>  
> ... explain RESTRICTIVE ...
>  
> 
>
>
> Apart from this changes look excellent to me.

I have moved that to next CF, my guess is that Stephen is going to
update soon and the activity is fresh.
-- 
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] Add support for restrictive RLS policies

[Jeevan Chalke]

Hi Stephen,


> 4. It will be good if we have an example for this in section
> > "5.7. Row Security Policies"
>
> I haven't added one yet, but will plan to do so.
>
> I think you are going to add this in this patch itself, right?

I have reviewed your latest patch and it fixes almost all my review
comments.
Also I am agree with your responses for couple of comments like response on
ALTER POLICY and tab completion. No issues with that.

However in documentation, PERMISSIVE and RESTRICTIVE are actually literals
and not parameters as such.  Also can we combine these two options into one
like below (similar to how we document CASCADE and RESTRICT for DROP
POLICY):

   
PERMISSIVE
RESTRICTIVE


 
... explain PERMISSIVE ...
 
 
... explain RESTRICTIVE ...
 

   

Apart from this changes look excellent to me.

Thanks

-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Re: [HACKERS] Add support for restrictive RLS policies

[Craig Ringer]

On 27 September 2016 at 15:15, Jeevan Chalke
 wrote:
> Hello Stephen,
>
> On Tue, Sep 27, 2016 at 12:57 AM, Stephen Frost  wrote:
>>
>> Jeevan,
>>
>> * Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote:
>> > I have started reviewing this patch and here are couple of points I have
>> > observed so far:
>> >
>> > 1. Patch applies cleanly
>> > 2. make / make install / initdb all good.
>> > 3. make check (regression) FAILED. (Attached diff file for reference).
>>
>> I've re-based my patch on top of current head and still don't see the
>> failures which you are getting during the regression tests.  Is it
>> possible you were doing the tests without a full rebuild of the source
>> tree..?
>>
>> Can you provide details of your build/test environment and the full
>> regression before and after output?
>
>
> I still get same failures with latest sources and with new patch. Here are
> few details of my setup. Let me know if I missed any.
>
> $ uname -a
> Linux centos7 3.10.0-327.28.3.el7.x86_64 #1 SMP Thu Aug 18 19:05:49 UTC 2016
> x86_64 x86_64 x86_64 GNU/Linux
>
> HEAD at
> commit 51c3e9fade76c12e4aa37bffdf800bbf74fb3fb1
>
> configure switches:
> ./configure --with-openssl --with-tcl --with-perl --with-python
> --with-ossp-uuid --with-ldap --with-pam --with-zlib --with-pgport=5432
> --enable-depend --enable-debug --enable-cassert --prefix=`pwd`/install
> CFLAGS="-g -O0"

I suggest:

git reset --hard
git clean -fdx
ccache -C

then re-apply patch and re-check.

I've had a couple of issues recently caused by ccache doing something
funky :( but haven't been able to track it down yet.


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Stephen, the typo "awseome" in the tests is a bit distracting.  Can you
> please fix it?

Done.

> I think you should use braces here, not parens:

Fixed.

> I don't think this paragraph is right -- you should call out each of the
> values PERMISSIVE and RESTRICTIVE (in upper case) instead.  Also note
> typos "Alternativly" and "visibillity".

Done.

> I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
> only appear in comments within rowsecurity.c (of your authorship too, I
> imagine).  I think it'd be better to find actual words for those
> actions.

Reworded to not attempt to use AND and OR as verbs.  Additionally, a
patch is also included to remove those from the comments in
rowsecurity.c.  There are a few other places where we have "OR'd" in the
code base, but I didn't think it made sense to change those as part of
this effort.

* Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote:
> With this patch, pg_policy catalog now has seven columns, however
> Natts_pg_policy is still set to 6. It should be updated to 7 now.
> Doing this regression seems OK.

Ah, certainly interesting that it only caused incorrect behavior and not
a crash (and no incorrect behavior even on my system, at least with the
regression tests and other testing I've done).

Fixed.

> 1. In documentation, we should put both permissive as well as restrictive in
> the header like permissive|restrictive. 

I'm not sure which place in the documentation you are referring to
here..?  [ AS { PERMISSIVE | RESTRICTIVE } ] was added to the CREATE
POLICY synopsis documentation.

> 2. "If the policy is a "permissive" or "restrictive" policy." seems broken
> as
> sentence starts with "If" and there is no other part to it. Will it be
> better
> to say "Specifies whether the policy is a "permissive" or "restrictive"
> policy."?

Rewrote this to be clearer, I hope.

> 3. " .. a policy can instead by "restrictive""
> Do you mean "instead be" here?

This was also rewritten.

> 4. It will be good if we have an example for this in section
> "5.7. Row Security Policies"

I haven't added one yet, but will plan to do so.

> 5. AS ( PERMISSIVE | RESTRICTIVE )
> should be '{' and '}' instead of parenthesis.

Fixed.

> 6. I think following changes are irrelevant for this patch.
> Should be submitted as separate patch if required.

As mentioned, this is tab-completion for the new options which this
patch introduces.

> 7. Natts_pg_policy should be updated to 7 now.

Fixed.

> 8. In following error, $2 and @2 should be used to correctly display the
> option and location.

Fixed.

> I think adding negative test to test this error should be added in
> regression.

Done.

> 9. Need to update following comments in gram.y to reflect new changes.

Done.

> 10. ALTER POLICY has no changes for this. Any reason why that is not
> considered here.

As mentioned, I don't see a use-case for it currently.

> 11. Will it be better to use boolean for polpermissive in _policyInfo?
> And then set that appropriately while getting the policies. So that later we
> only need to test the boolean avoiding string comparison.

Done.

> 12. Since PERMISSIVE is default, we should dump only "RESTRICTIVE" when
> appropriate, like other default cases.

Done, for this and the other defaults.

> 13. Since PERMISSIVE is default, do we need changes like below?
> -\QCREATE POLICY p1 ON test_table FOR ALL TO PUBLIC \E
> +\QCREATE POLICY p1 ON test_table AS PERMISSIVE FOR ALL TO
> PUBLIC \E

Updated to reflect what pg_dump now produces.

> 14. While displaying policy details in permissionsList, per syntax, we
> should
> display (RESTRICT) before the command option. Also will it be better to use
> (RESTRICTIVE) instead of (RESTRICT)?

Fixed.

> 15. Similarly in describeOneTableDetails() too, can we have RESTRICTIVE
> after
> policy name and before command option ?
> If we do that then changes related to adding "POLICY" followed by
> "RESTRICTIVE"
> will be straight forward.

Fixed.

> 16. It be good to have test-coverage for permissionsList,
> describeOneTableDetails and dump-restore changes. Please add those.

Done.

> 17. In pg_policies view, we need to add details related to PERMISSIVE and
> RESTRICTIVE. Please do so. Also add test for it.

Done.

> 18. Fix typos pointed earlier by Alvera.

Done.

Updated patch attached.

Thanks!

Stephen
From 020871cddd3c7187bd55a52673cae0af17a95246 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 1 Sep 2016 02:11:30 -0400
Subject: [PATCH 1/2] Add support for restrictive RLS policies

We have had support for restrictive RLS policies since 9.5, but they
were only available through extensions which use the appropriate hooks.
This adds support into the grammer, catalog, psql and pg_dump for
restrictive RLS policies, thus reducing the cases where an extension is
necessary.
---
 doc/src/sgml/ref/create_policy.sgml   |  28 
 

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

Jeevan,

* Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote:
> Here are the review comments:

[... many good comments ...]

Many thanks for the review, I believe I agree with pretty much all your
comments and will update the patch accordingly.

Responses for a couple of them are below.

> 6. I think following changes are irrelevant for this patch.
> Should be submitted as separate patch if required.

Those changes were adding support for tab completion of the new
restrictive / permissive options, which certainly seems appropriate for
the patch which is adding those options.  I realize it looks like a lot
for just two new options, but that's because there's a number of ways to
get to them.

> 10. ALTER POLICY has no changes for this. Any reason why that is not
> considered here.

Generally speaking, I don't believe it makes sense to flip a policy from
permissive to restrictive or vice-versa, they're really quite different
things.  We also don't support altering the "command" type for a policy.

> 12. Since PERMISSIVE is default, we should dump only "RESTRICTIVE" when
> appropriate, like other default cases.
> strcmp(polinfo->polpermissive,"t") == 0 ? "PERMISSIVE" : "RESTRICTIVE"

Sure, we could do that, though I suppose we'd want to do that for all of
the defaults for policies.

> 13. Since PERMISSIVE is default, do we need changes like below?
> -\QCREATE POLICY p1 ON test_table FOR ALL TO PUBLIC \E
> +\QCREATE POLICY p1 ON test_table AS PERMISSIVE FOR ALL TO
> PUBLIC \E
> 
> I am not familiar or used TAP framework. So no idea about these changes.

If we change pg_dump to not output AS PERMISSIVE for permissive
policies, then the TAP test will need to be updated to not include AS
PERMISSIVE (or FOR ALL TO PUBLIC, if we're going down that route).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Add support for restrictive RLS policies

[Jeevan Chalke]

On Tue, Sep 27, 2016 at 12:45 PM, Jeevan Chalke <
jeevan.cha...@enterprisedb.com> wrote:

> Hello Stephen,
>


> I am reviewing the latest patch in detail now and will post my review
> comments later.
>


Here are the review comments:

1. In documentation, we should put both permissive as well as restrictive in
the header like permissive|restrictive. Or may be a generic term, say,
policy
type (like we have command and then options mentioned like all, select
etc.),
followed by options in the description. Or like we have CASCADE and RESTRICT
in drop case.

2. "If the policy is a "permissive" or "restrictive" policy." seems broken
as
sentence starts with "If" and there is no other part to it. Will it be
better
to say "Specifies whether the policy is a "permissive" or "restrictive"
policy."?

3. " .. a policy can instead by "restrictive""
Do you mean "instead be" here?

4. It will be good if we have an example for this in section
"5.7. Row Security Policies"

5. AS ( PERMISSIVE | RESTRICTIVE )
should be '{' and '}' instead of parenthesis.

6. I think following changes are irrelevant for this patch.
Should be submitted as separate patch if required.

@@ -2133,6 +2139,25 @@ psql_completion(const char *text, int start, int end)
 /* Complete "CREATE POLICY  ON  USING (" */
 else if (Matches6("CREATE", "POLICY", MatchAny, "ON", MatchAny,
"USING"))
 COMPLETE_WITH_CONST("(");
+/* CREATE POLICY  ON  AS PERMISSIVE|RESTRICTIVE FOR
ALL|SELECT|INSERT|UPDATE|DELETE */
+else if (Matches8("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS",
MatchAny, "FOR"))
+COMPLETE_WITH_LIST5("ALL", "SELECT", "INSERT", "UPDATE", "DELETE");
+/* Complete "CREATE POLICY  ON  AS PERMISSIVE|RESTRICTIVE
FOR INSERT TO|WITH CHECK" */
+else if (Matches9("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS",
MatchAny, "FOR", "INSERT"))
+COMPLETE_WITH_LIST2("TO", "WITH CHECK (");
+/* Complete "CREATE POLICY  ON  AS PERMISSIVE|RESTRICTIVE
FOR SELECT|DELETE TO|USING" */
+else if (Matches9("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS",
MatchAny, "FOR", "SELECT|DELETE"))
+COMPLETE_WITH_LIST2("TO", "USING (");
+/* CREATE POLICY  ON  AS PERMISSIVE|RESTRICTIVE FOR
ALL|UPDATE TO|USING|WITH CHECK */
+else if (Matches9("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS",
MatchAny, "FOR", "ALL|UPDATE"))
+COMPLETE_WITH_LIST3("TO", "USING (", "WITH CHECK (");
+/* Complete "CREATE POLICY  ON  AS PERMISSIVE|RESTRICTIVE
TO " */
+else if (Matches8("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS",
MatchAny, "TO"))
+COMPLETE_WITH_QUERY(Query_for_list_of_grant_roles);
+/* Complete "CREATE POLICY  ON  AS PERMISSIVE|RESTRICTIVE
USING (" */
+else if (Matches8("CREATE", "POLICY", MatchAny, "ON", MatchAny, "AS",
MatchAny, "USING"))
+COMPLETE_WITH_CONST("(");

7. Natts_pg_policy should be updated to 7 now.

8. In following error, $2 and @2 should be used to correctly display the
option and location.

ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
 errmsg("unrecognized row security option
\"%s\"", $1),
 errhint("Only PERMISSIVE or RESTRICTIVE
policies are supported currently."),
 parser_errposition(@1)));

You will see following result otherwise:

postgres=# CREATE POLICY my_policy ON foo AS a123;
ERROR:  unrecognized row security option "as"
LINE 1: CREATE POLICY my_policy ON foo AS a123;
   ^
HINT:  Only PERMISSIVE or RESTRICTIVE policies are supported currently.

I think adding negative test to test this error should be added in
regression.

9. Need to update following comments in gram.y to reflect new changes.

 *QUERIES:
 *CREATE POLICY name ON table [FOR cmd] [TO role, ...]
 *[USING (qual)] [WITH CHECK (with_check)]

10. ALTER POLICY has no changes for this. Any reason why that is not
considered here.

11. Will it be better to use boolean for polpermissive in _policyInfo?
And then set that appropriately while getting the policies. So that later we
only need to test the boolean avoiding string comparison.

12. Since PERMISSIVE is default, we should dump only "RESTRICTIVE" when
appropriate, like other default cases.
strcmp(polinfo->polpermissive,"t") == 0 ? "PERMISSIVE" : "RESTRICTIVE"

13. Since PERMISSIVE is default, do we need changes like below?
-\QCREATE POLICY p1 ON test_table FOR ALL TO PUBLIC \E
+\QCREATE POLICY p1 ON test_table AS PERMISSIVE FOR ALL TO
PUBLIC \E

I am not familiar or used TAP framework. So no idea about these changes.

14. While displaying policy details in permissionsList, per syntax, we
should
display (RESTRICT) before the command option. Also will it be better to use
(RESTRICTIVE) instead of (RESTRICT)?

15. Similarly in describeOneTableDetails() too, can we have RESTRICTIVE

Re: [HACKERS] Add support for restrictive RLS policies

[Jeevan Chalke]

Hello Stephen,

On Tue, Sep 27, 2016 at 12:57 AM, Stephen Frost  wrote:

> Jeevan,
>
> * Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote:
> > I have started reviewing this patch and here are couple of points I have
> > observed so far:
> >
> > 1. Patch applies cleanly
> > 2. make / make install / initdb all good.
> > 3. make check (regression) FAILED. (Attached diff file for reference).
>
> I've re-based my patch on top of current head and still don't see the
> failures which you are getting during the regression tests.  Is it
> possible you were doing the tests without a full rebuild of the source
> tree..?
>
> Can you provide details of your build/test environment and the full
> regression before and after output?
>

I still get same failures with latest sources and with new patch. Here are
few details of my setup. Let me know if I missed any.

$ uname -a
Linux centos7 3.10.0-327.28.3.el7.x86_64 #1 SMP Thu Aug 18 19:05:49 UTC
2016 x86_64 x86_64 x86_64 GNU/Linux

HEAD at
commit 51c3e9fade76c12e4aa37bffdf800bbf74fb3fb1

configure switches:
./configure --with-openssl --with-tcl --with-perl --with-python
--with-ossp-uuid --with-ldap --with-pam --with-zlib --with-pgport=5432
--enable-depend --enable-debug --enable-cassert --prefix=`pwd`/install
CFLAGS="-g -O0"

Regression FAILED. Regression diff is same as previous one.

Without patch I don't get any regression failure.

Well, I could not restrict myself debugging this mystery and finally able
to find the reason why this is failing. It was strange that it did not
crash and simply gave different results.

With this patch, pg_policy catalog now has seven columns, however
Natts_pg_policy is still set to 6. It should be updated to 7 now.
Doing this regression seems OK.

I am reviewing the latest patch in detail now and will post my review
comments later.

Thanks


>
> Thanks!
>
> Stephen
>



-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Re: [HACKERS] Add support for restrictive RLS policies

[Alvaro Herrera]

Stephen Frost wrote:

> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> > Stephen Frost wrote:

> > > + 
> > > +  If the policy is a "permissive" or "restrictive" policy.  
> > > Permissive
> > > +  policies are the default and always add visibillity- they ar "OR"d
> > > +  together to allow the user access to all rows through any of the
> > > +  permissive policies they have access to.  Alternativly, a policy 
> > > can
> > > +  instead by "restrictive", meaning that the policy will be "AND"d
> > > +  with other restrictive policies and with the result of all of the
> > > +  permissive policies on the table.
> > > + 
> > > +
> > > +   

Stephen,

> > I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
> > only appear in comments within rowsecurity.c (of your authorship too, I
> > imagine).  I think it'd be better to find actual words for those
> > actions.
> 
> I'm certainly open to suggestions, should you, or anyone else, have
> them.  I'll try and come up with something else, but that really is what
> we're doing- literally using either AND or OR to join the expressions
> together.

I understand, but the words "and" and "or" are not verbs.  I don't know
what are good verbs to use for this but I suppose there must be verbs
related to "conjunction" and "disjunction" ("to conjoin" and "to
disjoin" appear in the Merriam-Webster dictionary but I don't think they
represent the operation very well).  Maybe some native speaker would
have a better suggestion.

> > I don't understand this part.  Didn't you say previously that we already
> > had this capability in 9.5 and you were only exposing it over SQL?  If
> > that is true, how come you need to add a new column to this catalog?
> 
> The capability exists in 9.5 through hooks which are available to
> extensions, see the test_rls_hooks extension.  Those hooks are called
> every time and therefore don't require the information about restrictive
> policies to be tracked in pg_policy, and so they weren't.  Since this is
> adding support for users to define restrictive policies, we need to save
> those policies and therefore we need to distinguish which policies are
> restrictive and which are permissive, hence the need to modify pg_policy
> to track that information.

Ah, okay.  I thought you meant that it was already possible to create a
policy to behave this way, just not using SQL-based DDL.

-- 
Álvaro Herrerahttps://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

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

Alvaro,

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> 
> Stephen, the typo "awseome" in the tests is a bit distracting.  Can you
> please fix it?

Will fix.

> > Updated patch changes to using IDENT and an strcmp() (similar to
> > AlterOptRoleElem and vacuum_option_elem) to check the results at parse-time,
> > and then throwing a more specific error if an unexpected value is found
> > (instead of just 'syntax error').  This avoids adding any keywords.
> 
> Thanks.

No problem.

> > diff --git a/doc/src/sgml/ref/create_policy.sgml 
> > b/doc/src/sgml/ref/create_policy.sgml
> > index 89d2787..d930052 100644
> > --- a/doc/src/sgml/ref/create_policy.sgml
> > +++ b/doc/src/sgml/ref/create_policy.sgml
> > @@ -22,6 +22,7 @@ PostgreSQL documentation
> >   
> >  
> >  CREATE POLICY name ON 
> > table_name
> > +[ AS ( PERMISSIVE | RESTRICTIVE ) ]
> 
> I think you should use braces here, not parens:
> 
> [ AS { PERMISSIVE | RESTRICTIVE } ]

Will fix.

> > 
> > +permissive
> > +
> > + 
> > +  If the policy is a "permissive" or "restrictive" policy.  Permissive
> > +  policies are the default and always add visibillity- they ar "OR"d
> > +  together to allow the user access to all rows through any of the
> > +  permissive policies they have access to.  Alternativly, a policy can
> > +  instead by "restrictive", meaning that the policy will be "AND"d
> > +  with other restrictive policies and with the result of all of the
> > +  permissive policies on the table.
> > + 
> > +
> > +   
> 
> I don't think this paragraph is right -- you should call out each of the
> values PERMISSIVE and RESTRICTIVE (in upper case) instead.  Also note
> typos "Alternativly" and "visibillity".

Will fix, along with the 'ar' typo.

> I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
> only appear in comments within rowsecurity.c (of your authorship too, I
> imagine).  I think it'd be better to find actual words for those
> actions.

I'm certainly open to suggestions, should you, or anyone else, have
them.  I'll try and come up with something else, but that really is what
we're doing- literally using either AND or OR to join the expressions
together.

> > diff --git a/src/include/catalog/pg_policy.h 
> > b/src/include/catalog/pg_policy.h
> > index d73e9c2..30dc367 100644
> > --- a/src/include/catalog/pg_policy.h
> > +++ b/src/include/catalog/pg_policy.h
> > @@ -23,6 +23,7 @@ CATALOG(pg_policy,3256)
> > NameDatapolname;/* Policy name. */
> > Oid polrelid;   /* Oid of the relation 
> > with policy. */
> > charpolcmd; /* One of ACL_*_CHR, or '*' for 
> > all */
> > +   boolpolpermissive;  /* restrictive or permissive policy */
> >  
> >  #ifdef CATALOG_VARLEN
> > Oid polroles[1];/* Roles associated with 
> > policy, not-NULL */
> > @@ -42,12 +43,13 @@ typedef FormData_pg_policy *Form_pg_policy;
> >   * compiler constants for pg_policy
> >   * 
> >   */
> > -#define Natts_pg_policy6
> > -#define Anum_pg_policy_polname 1
> > -#define Anum_pg_policy_polrelid2
> > -#define Anum_pg_policy_polcmd  3
> > -#define Anum_pg_policy_polroles4
> > -#define Anum_pg_policy_polqual 5
> > -#define Anum_pg_policy_polwithcheck 6
> > +#define Natts_pg_policy6
> > +#define Anum_pg_policy_polname 1
> > +#define Anum_pg_policy_polrelid2
> > +#define Anum_pg_policy_polcmd  3
> > +#define Anum_pg_policy_polpermissive   4
> > +#define Anum_pg_policy_polroles5
> > +#define Anum_pg_policy_polqual 6
> > +#define Anum_pg_policy_polwithcheck7
> 
> I don't understand this part.  Didn't you say previously that we already
> had this capability in 9.5 and you were only exposing it over SQL?  If
> that is true, how come you need to add a new column to this catalog?

The capability exists in 9.5 through hooks which are available to
extensions, see the test_rls_hooks extension.  Those hooks are called
every time and therefore don't require the information about restrictive
policies to be tracked in pg_policy, and so they weren't.  Since this is
adding support for users to define restrictive policies, we need to save
those policies and therefore we need to distinguish which policies are
restrictive and which are permissive, hence the need to modify pg_policy
to track that information.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Add support for restrictive RLS policies

[Alvaro Herrera]

Stephen Frost wrote:

Stephen, the typo "awseome" in the tests is a bit distracting.  Can you
please fix it?

> Updated patch changes to using IDENT and an strcmp() (similar to
> AlterOptRoleElem and vacuum_option_elem) to check the results at parse-time,
> and then throwing a more specific error if an unexpected value is found
> (instead of just 'syntax error').  This avoids adding any keywords.

Thanks.

> diff --git a/doc/src/sgml/ref/create_policy.sgml 
> b/doc/src/sgml/ref/create_policy.sgml
> index 89d2787..d930052 100644
> --- a/doc/src/sgml/ref/create_policy.sgml
> +++ b/doc/src/sgml/ref/create_policy.sgml
> @@ -22,6 +22,7 @@ PostgreSQL documentation
>   
>  
>  CREATE POLICY name ON 
> table_name
> +[ AS ( PERMISSIVE | RESTRICTIVE ) ]

I think you should use braces here, not parens:

[ AS { PERMISSIVE | RESTRICTIVE } ]

> 
> +permissive
> +
> + 
> +  If the policy is a "permissive" or "restrictive" policy.  Permissive
> +  policies are the default and always add visibillity- they ar "OR"d
> +  together to allow the user access to all rows through any of the
> +  permissive policies they have access to.  Alternativly, a policy can
> +  instead by "restrictive", meaning that the policy will be "AND"d
> +  with other restrictive policies and with the result of all of the
> +  permissive policies on the table.
> + 
> +
> +   

I don't think this paragraph is right -- you should call out each of the
values PERMISSIVE and RESTRICTIVE (in upper case) instead.  Also note
typos "Alternativly" and "visibillity".

I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
only appear in comments within rowsecurity.c (of your authorship too, I
imagine).  I think it'd be better to find actual words for those
actions.

> diff --git a/src/include/catalog/pg_policy.h b/src/include/catalog/pg_policy.h
> index d73e9c2..30dc367 100644
> --- a/src/include/catalog/pg_policy.h
> +++ b/src/include/catalog/pg_policy.h
> @@ -23,6 +23,7 @@ CATALOG(pg_policy,3256)
>   NameDatapolname;/* Policy name. */
>   Oid polrelid;   /* Oid of the relation 
> with policy. */
>   charpolcmd; /* One of ACL_*_CHR, or '*' for 
> all */
> + boolpolpermissive;  /* restrictive or permissive policy */
>  
>  #ifdef CATALOG_VARLEN
>   Oid polroles[1];/* Roles associated with 
> policy, not-NULL */
> @@ -42,12 +43,13 @@ typedef FormData_pg_policy *Form_pg_policy;
>   *   compiler constants for pg_policy
>   * 
>   */
> -#define Natts_pg_policy  6
> -#define Anum_pg_policy_polname   1
> -#define Anum_pg_policy_polrelid  2
> -#define Anum_pg_policy_polcmd3
> -#define Anum_pg_policy_polroles  4
> -#define Anum_pg_policy_polqual   5
> -#define Anum_pg_policy_polwithcheck 6
> +#define Natts_pg_policy  6
> +#define Anum_pg_policy_polname   1
> +#define Anum_pg_policy_polrelid  2
> +#define Anum_pg_policy_polcmd3
> +#define Anum_pg_policy_polpermissive 4
> +#define Anum_pg_policy_polroles  5
> +#define Anum_pg_policy_polqual   6
> +#define Anum_pg_policy_polwithcheck  7

I don't understand this part.  Didn't you say previously that we already
had this capability in 9.5 and you were only exposing it over SQL?  If
that is true, how come you need to add a new column to this catalog?

-- 
Álvaro Herrerahttps://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

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

Jeevan, all,

* Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote:
> On Mon, Sep 12, 2016 at 7:27 AM, Stephen Frost  wrote:
> > * Robert Haas (robertmh...@gmail.com) wrote:
> > > On Thu, Sep 8, 2016 at 5:21 PM, Tom Lane  wrote:
> > > > Stephen Frost  writes:
> > > >> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> > > >>> Can't you keep those words as Sconst or something (DefElems?) until
> > the
> > > >>> execution phase, so that they don't need to be keywords at all?
> > > >
> > > >> Seems like we could do that, though I'm not convinced that it really
> > > >> gains us all that much.  These are only unreserved keywords, of
> > course,
> > > >> so they don't impact users the way reserved keywords (of any kind)
> > can.
> > > >> While there may be some places where we use a string to represent a
> > set
> > > >> of defined options, I don't believe that's typical
> > > >
> > > > -1 for having to write them as string literals; but I think what Alvaro
> > > > really means is to arrange for the words to just be identifiers in the
> > > > grammar, which you strcmp against at execution.  See for example
> > > > reloption_list.  (Whether you use DefElem as the internal
> > representation
> > > > is a minor detail, though it might help for making the parsetree
> > > > copyObject-friendly.)
> > > >
> > > > vacuum_option_elem shows another way to avoid making a word into a
> > > > keyword, although to me that one is more of an antipattern; it'd be
> > better
> > > > to leave the strcmp to execution, since there's so much other code that
> > > > does things that way.
> > >
> > > There are other cases like that, too, e.g. AlterOptRoleElem; I don't
> > > think it's a bad pattern.  Regardless of the specifics, I do think
> > > that it would be better not to bloat the keyword table with things
> > > that don't really need to be keywords.
> >
> > The AlterOptRoleElem case is, I believe, what Tom was just suggesting as
> > an antipattern, since the strcmp() is being done at parse time instead
> > of at execution time.
> >
> > If we are concerned about having too many unreserved keywords, then I
> > agree that AlterOptRoleElem is a good candidate to look at for reducing
> > the number we have, as it appears to contain 3 keywords which are not
> > used anywhere else (and 1 other which is only used in one other place).
> >
> > I do think that using IDENT for the various role attributes does make
> > sense, to be clear, as there are quite a few of them, they change
> > depending on major version, and those keywords are very unlikely to ever
> > have utilization elsewhere.
> >
> > For this case, there's just 2 keywords which seem like they may be used
> > again (perhaps for ALTER or DROP POLICY, or default policies if we grow
> > those in the future), and we're unlikly to grow more in the future for
> > that particular case (as we only have two binary boolean operators and
> > that seems unlikely to change), though, should that happens, we could
> > certainly revisit this.
> >
> 
> To me, adding two new keywords for two new options does not look good as it
> will bloat keywords list. Per my understanding we should add keyword if and
> only if we have no option than adding at, may be to avoid grammar conflicts.
> 
> I am also inclined to think that using identifier will be a good choice
> here.
> However I would prefer to do the string comparison right into the grammar
> itself, so that if we have wrong option as input there, then we will not
> proceed further with it. We are anyway going to throw an error later then
> why not at early stage.

Updated patch changes to using IDENT and an strcmp() (similar to
AlterOptRoleElem and vacuum_option_elem) to check the results at parse-time,
and then throwing a more specific error if an unexpected value is found
(instead of just 'syntax error').  This avoids adding any keywords.

Thanks!

Stephen
From 11471c7921271e3c03078f3d31148dd4afd9d6e0 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 1 Sep 2016 02:11:30 -0400
Subject: [PATCH] Add support for restrictive RLS policies

We have had support for restrictive RLS policies since 9.5, but they
were only available through extensions which use the appropriate hooks.
This adds support into the grammer, catalog, psql and pg_dump for
restrictive RLS policies, thus reducing the cases where an extension is
necessary.
---
 doc/src/sgml/ref/create_policy.sgml   |  16 +++
 src/backend/commands/policy.c |   9 ++
 src/backend/nodes/copyfuncs.c |   1 +
 src/backend/nodes/equalfuncs.c|   1 +
 src/backend/parser/gram.y |  34 -
 src/backend/rewrite/rowsecurity.c |   7 +-
 src/bin/pg_dump/pg_dump.c |  38 --
 src/bin/pg_dump/pg_dump.h |   1 +
 src/bin/pg_dump/t/002_pg_dump.pl  |  39 +-
 src/bin/psql/describe.c   | 109 

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

Jeevan,

* Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote:
> I have started reviewing this patch and here are couple of points I have
> observed so far:
> 
> 1. Patch applies cleanly
> 2. make / make install / initdb all good.
> 3. make check (regression) FAILED. (Attached diff file for reference).

I've re-based my patch on top of current head and still don't see the
failures which you are getting during the regression tests.  Is it
possible you were doing the tests without a full rebuild of the source
tree..?

Can you provide details of your build/test environment and the full
regression before and after output?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Add support for restrictive RLS policies

[Jeevan Chalke]

Hi,

I have started reviewing this patch and here are couple of points I have
observed so far:

1. Patch applies cleanly
2. make / make install / initdb all good.
3. make check (regression) FAILED. (Attached diff file for reference).

Please have a look over failures.

Meanwhile I will go ahead and review the code changes.

Thanks

-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company


regression.diffs
Description: Binary data

-- 
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] Add support for restrictive RLS policies

[Jeevan Chalke]

On Mon, Sep 12, 2016 at 7:27 AM, Stephen Frost  wrote:

> Robert,
>
> * Robert Haas (robertmh...@gmail.com) wrote:
> > On Thu, Sep 8, 2016 at 5:21 PM, Tom Lane  wrote:
> > > Stephen Frost  writes:
> > >> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> > >>> Can't you keep those words as Sconst or something (DefElems?) until
> the
> > >>> execution phase, so that they don't need to be keywords at all?
> > >
> > >> Seems like we could do that, though I'm not convinced that it really
> > >> gains us all that much.  These are only unreserved keywords, of
> course,
> > >> so they don't impact users the way reserved keywords (of any kind)
> can.
> > >> While there may be some places where we use a string to represent a
> set
> > >> of defined options, I don't believe that's typical
> > >
> > > -1 for having to write them as string literals; but I think what Alvaro
> > > really means is to arrange for the words to just be identifiers in the
> > > grammar, which you strcmp against at execution.  See for example
> > > reloption_list.  (Whether you use DefElem as the internal
> representation
> > > is a minor detail, though it might help for making the parsetree
> > > copyObject-friendly.)
> > >
> > > vacuum_option_elem shows another way to avoid making a word into a
> > > keyword, although to me that one is more of an antipattern; it'd be
> better
> > > to leave the strcmp to execution, since there's so much other code that
> > > does things that way.
> >
> > There are other cases like that, too, e.g. AlterOptRoleElem; I don't
> > think it's a bad pattern.  Regardless of the specifics, I do think
> > that it would be better not to bloat the keyword table with things
> > that don't really need to be keywords.
>
> The AlterOptRoleElem case is, I believe, what Tom was just suggesting as
> an antipattern, since the strcmp() is being done at parse time instead
> of at execution time.
>
> If we are concerned about having too many unreserved keywords, then I
> agree that AlterOptRoleElem is a good candidate to look at for reducing
> the number we have, as it appears to contain 3 keywords which are not
> used anywhere else (and 1 other which is only used in one other place).
>
> I do think that using IDENT for the various role attributes does make
> sense, to be clear, as there are quite a few of them, they change
> depending on major version, and those keywords are very unlikely to ever
> have utilization elsewhere.
>
> For this case, there's just 2 keywords which seem like they may be used
> again (perhaps for ALTER or DROP POLICY, or default policies if we grow
> those in the future), and we're unlikly to grow more in the future for
> that particular case (as we only have two binary boolean operators and
> that seems unlikely to change), though, should that happens, we could
> certainly revisit this.
>

To me, adding two new keywords for two new options does not look good as it
will bloat keywords list. Per my understanding we should add keyword if and
only if we have no option than adding at, may be to avoid grammar conflicts.

I am also inclined to think that using identifier will be a good choice
here.
However I would prefer to do the string comparison right into the grammar
itself, so that if we have wrong option as input there, then we will not
proceed further with it. We are anyway going to throw an error later then
why not at early stage.

Thanks


>
> Thanks!
>
> Stephen
>



-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Sep 8, 2016 at 5:21 PM, Tom Lane  wrote:
> > Stephen Frost  writes:
> >> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> >>> Can't you keep those words as Sconst or something (DefElems?) until the
> >>> execution phase, so that they don't need to be keywords at all?
> >
> >> Seems like we could do that, though I'm not convinced that it really
> >> gains us all that much.  These are only unreserved keywords, of course,
> >> so they don't impact users the way reserved keywords (of any kind) can.
> >> While there may be some places where we use a string to represent a set
> >> of defined options, I don't believe that's typical
> >
> > -1 for having to write them as string literals; but I think what Alvaro
> > really means is to arrange for the words to just be identifiers in the
> > grammar, which you strcmp against at execution.  See for example
> > reloption_list.  (Whether you use DefElem as the internal representation
> > is a minor detail, though it might help for making the parsetree
> > copyObject-friendly.)
> >
> > vacuum_option_elem shows another way to avoid making a word into a
> > keyword, although to me that one is more of an antipattern; it'd be better
> > to leave the strcmp to execution, since there's so much other code that
> > does things that way.
> 
> There are other cases like that, too, e.g. AlterOptRoleElem; I don't
> think it's a bad pattern.  Regardless of the specifics, I do think
> that it would be better not to bloat the keyword table with things
> that don't really need to be keywords.

The AlterOptRoleElem case is, I believe, what Tom was just suggesting as
an antipattern, since the strcmp() is being done at parse time instead
of at execution time.

If we are concerned about having too many unreserved keywords, then I
agree that AlterOptRoleElem is a good candidate to look at for reducing
the number we have, as it appears to contain 3 keywords which are not
used anywhere else (and 1 other which is only used in one other place).

I do think that using IDENT for the various role attributes does make
sense, to be clear, as there are quite a few of them, they change
depending on major version, and those keywords are very unlikely to ever
have utilization elsewhere.

For this case, there's just 2 keywords which seem like they may be used
again (perhaps for ALTER or DROP POLICY, or default policies if we grow
those in the future), and we're unlikly to grow more in the future for
that particular case (as we only have two binary boolean operators and
that seems unlikely to change), though, should that happens, we could
certainly revisit this.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Add support for restrictive RLS policies

[Robert Haas]

On Thu, Sep 8, 2016 at 5:21 PM, Tom Lane  wrote:
> Stephen Frost  writes:
>> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
>>> Can't you keep those words as Sconst or something (DefElems?) until the
>>> execution phase, so that they don't need to be keywords at all?
>
>> Seems like we could do that, though I'm not convinced that it really
>> gains us all that much.  These are only unreserved keywords, of course,
>> so they don't impact users the way reserved keywords (of any kind) can.
>> While there may be some places where we use a string to represent a set
>> of defined options, I don't believe that's typical
>
> -1 for having to write them as string literals; but I think what Alvaro
> really means is to arrange for the words to just be identifiers in the
> grammar, which you strcmp against at execution.  See for example
> reloption_list.  (Whether you use DefElem as the internal representation
> is a minor detail, though it might help for making the parsetree
> copyObject-friendly.)
>
> vacuum_option_elem shows another way to avoid making a word into a
> keyword, although to me that one is more of an antipattern; it'd be better
> to leave the strcmp to execution, since there's so much other code that
> does things that way.

There are other cases like that, too, e.g. AlterOptRoleElem; I don't
think it's a bad pattern.  Regardless of the specifics, I do think
that it would be better not to bloat the keyword table with things
that don't really need to be keywords.

-- 
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] Add support for restrictive RLS policies

[Tom Lane]

Stephen Frost  writes:
> I saw the various list-based cases and commented in my reply (the one you
> quote part of above) why those didn't seem to make sense for this case
> (it's not a list and I don't see it ever growing into one).

I think Alvaro was simply questioning whether there would ever be a need
for more than two alternatives.

regards, tom lane


-- 
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] Add support for restrictive RLS policies

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> >> Can't you keep those words as Sconst or something (DefElems?) until the
> >> execution phase, so that they don't need to be keywords at all?
> 
> > Seems like we could do that, though I'm not convinced that it really
> > gains us all that much.  These are only unreserved keywords, of course,
> > so they don't impact users the way reserved keywords (of any kind) can.
> > While there may be some places where we use a string to represent a set
> > of defined options, I don't believe that's typical
> 
> -1 for having to write them as string literals; but I think what Alvaro
> really means is to arrange for the words to just be identifiers in the
> grammar, which you strcmp against at execution.  See for example
> reloption_list.  (Whether you use DefElem as the internal representation
> is a minor detail, though it might help for making the parsetree
> copyObject-friendly.)

I saw the various list-based cases and commented in my reply (the one you
quote part of above) why those didn't seem to make sense for this case
(it's not a list and I don't see it ever growing into one).

> vacuum_option_elem shows another way to avoid making a word into a
> keyword, although to me that one is more of an antipattern; it'd be better
> to leave the strcmp to execution, since there's so much other code that
> does things that way.

Both of those cases are for lists, which this is not.  I certainly
understand that it makes sense to allow a list of options to be passed
in any order and that means we need to build just the list with the
grammar and then check what's in the list at execution time, and further
check that the user didn't provide a set of invalid or duplicative
options, but this isn't a list.  If the thinking is that it *should* be
a list, then it'd be really helpful to see an example or two of what the
list-based syntax would be.  I provided one in my reply and commented on
why it didn't seem like a good approach.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Add support for restrictive RLS policies

[Tom Lane]

Stephen Frost  writes:
> * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
>> Can't you keep those words as Sconst or something (DefElems?) until the
>> execution phase, so that they don't need to be keywords at all?

> Seems like we could do that, though I'm not convinced that it really
> gains us all that much.  These are only unreserved keywords, of course,
> so they don't impact users the way reserved keywords (of any kind) can.
> While there may be some places where we use a string to represent a set
> of defined options, I don't believe that's typical

-1 for having to write them as string literals; but I think what Alvaro
really means is to arrange for the words to just be identifiers in the
grammar, which you strcmp against at execution.  See for example
reloption_list.  (Whether you use DefElem as the internal representation
is a minor detail, though it might help for making the parsetree
copyObject-friendly.)

vacuum_option_elem shows another way to avoid making a word into a
keyword, although to me that one is more of an antipattern; it'd be better
to leave the strcmp to execution, since there's so much other code that
does things that way.

regards, tom lane


-- 
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] Add support for restrictive RLS policies

[Stephen Frost]

Alvaro,

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > * Stephen Frost (sfr...@snowman.net) wrote:
> > > Based on Robert's suggestion and using Thom's verbiage, I've tested this
> > > out:
> > > 
> > > CREATE POLICY pol ON tab AS [PERMISSIVE|RESTRICTIVE] ...
> 
> Can't you keep those words as Sconst or something (DefElems?) until the
> execution phase, so that they don't need to be keywords at all?  I'm
> fairly sure we do that kind of thing elsewhere.  Besides, that let you
> throw errors such as "keyword 'foobarive' not recognized" instead of a
> generic "syntax error" if the user enters a bogus permissivity (?)
> keyword.

Seems like we could do that, though I'm not convinced that it really
gains us all that much.  These are only unreserved keywords, of course,
so they don't impact users the way reserved keywords (of any kind) can.
While there may be some places where we use a string to represent a set
of defined options, I don't believe that's typical- certainly something
like DISCARD has a set of explicit values, same for CASCADE vs.
RESTRICT, replica_identity, TableLikeOption, etc..

We do have a few 'not recognized' messages in the backend, though
they're usually 'option %s not recognized' (there aren't any which use
'keyword') and they're in places where we support a list of options to
be specified (which also means they require additional code to check for
conflicting/redundant options).  We could possibly rearrange the entire
CREATE POLICY comamnd to use a list of options instead, along the lines
of what we do for views:

CREATE POLICY p1 ON t1
WITH (command=select,combine_using=AND)
USING ...;

but that hardly seems better.

Perhaps I'm not understanding what you're getting at though- is there
something in the existing grammar, in particular, that you're comparing
this to?

> Is the permissive/restrictive dichotomy enough to support all
> interesting use cases?  What I think is the equivalent concept in PAM
> uses required/requisite/sufficient/optional as possibilities, which
> allows for finer grained control.  Even there that's just the historical
> interface, and the replacement syntax has more gadgets.

Restrictive vs. Permissive very simply maps to the logical AND and OR
operators.  Those are the only binary logical operators we have and it
seems unlikely that we're going to get any more anytime soon.

I don't believe the comparison to PAM is really fair, as PAM is trying
to support the flexibility we already have by allowing users to specify
an expression in the policy itself.

Perhaps we may wish to come up with a more complex approach for how to
combine policies, but in that case, I'd think we'd do something like:

CREATE POLICY p1 ON t1 COMBINING ((policy1 AND policy2) OR policy3);

though I've not yet come across a case that requires something more
complicated than what we can do already with policies and the
restrictive / permissive options (note that the case above can be
handled that way, in fact, by making policy1 and policy2 restrictive and
policy3 permissive).  Perhaps that's because that more complicated logic
is generally understood and expected to be part of the policy expression
itself instead.

Also, as mentioned at the start of this thread, the capability for
restrictive policies has existed since 9.5, this change is simply
exposing that to users without having to require using an extension.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Add support for restrictive RLS policies

[Alvaro Herrera]

Stephen Frost wrote:
> Greetings!
> 
> * Stephen Frost (sfr...@snowman.net) wrote:
> > Based on Robert's suggestion and using Thom's verbiage, I've tested this
> > out:
> > 
> > CREATE POLICY pol ON tab AS [PERMISSIVE|RESTRICTIVE] ...

Can't you keep those words as Sconst or something (DefElems?) until the
execution phase, so that they don't need to be keywords at all?  I'm
fairly sure we do that kind of thing elsewhere.  Besides, that let you
throw errors such as "keyword 'foobarive' not recognized" instead of a
generic "syntax error" if the user enters a bogus permissivity (?)
keyword.

Is the permissive/restrictive dichotomy enough to support all
interesting use cases?  What I think is the equivalent concept in PAM
uses required/requisite/sufficient/optional as possibilities, which
allows for finer grained control.  Even there that's just the historical
interface, and the replacement syntax has more gadgets.

-- 
Álvaro Herrerahttp://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

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

Greetings!

* Stephen Frost (sfr...@snowman.net) wrote:
> Based on Robert's suggestion and using Thom's verbiage, I've tested this
> out:
> 
> CREATE POLICY pol ON tab AS [PERMISSIVE|RESTRICTIVE] ...
> 
> and it appears to work fine with the grammar, etc.
> 
> Unless there's other thoughts on this, I'll update the patch to reflect
> this grammar in a couple days.

Updated patch attached which uses the above approach, includes
some initial documentation, and has fixes for the tab completion, 

I'm planning to add more documentation.  Otherwise, testing and code
review would certainly be appreciated.

Thanks!

Stpehen
From 6fad316de3cc50f4cc2c3bbe3c6fac91afc881e6 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 1 Sep 2016 02:11:30 -0400
Subject: [PATCH] Add support for restrictive RLS policies

We have had support for restrictive RLS policies since 9.5, but they
were only available through extensions which use the appropriate hooks.
This adds support into the grammer, catalog, psql and pg_dump for
restrictive RLS policies, thus reducing the cases where an extension is
necessary.
---
 doc/src/sgml/ref/create_policy.sgml   |  16 +++
 src/backend/commands/policy.c |   9 ++
 src/backend/nodes/copyfuncs.c |   1 +
 src/backend/nodes/equalfuncs.c|   1 +
 src/backend/parser/gram.y |  32 +++--
 src/backend/rewrite/rowsecurity.c |   7 +-
 src/bin/pg_dump/pg_dump.c |  38 --
 src/bin/pg_dump/pg_dump.h |   1 +
 src/bin/pg_dump/t/002_pg_dump.pl  |  39 +-
 src/bin/psql/describe.c   | 109 
 src/bin/psql/tab-complete.c   |  29 -
 src/include/catalog/pg_policy.h   |  16 ++-
 src/include/nodes/parsenodes.h|   1 +
 src/include/parser/kwlist.h   |   2 +
 src/include/rewrite/rowsecurity.h |   1 +
 src/test/regress/expected/rowsecurity.out | 209 ++
 src/test/regress/sql/rowsecurity.sql  |  24 +++-
 17 files changed, 417 insertions(+), 118 deletions(-)

diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml
index 89d2787..d930052 100644
--- a/doc/src/sgml/ref/create_policy.sgml
+++ b/doc/src/sgml/ref/create_policy.sgml
@@ -22,6 +22,7 @@ PostgreSQL documentation
  
 
 CREATE POLICY name ON table_name
+[ AS ( PERMISSIVE | RESTRICTIVE ) ]
 [ FOR { ALL | SELECT | INSERT | UPDATE | DELETE } ]
 [ TO { role_name | PUBLIC | CURRENT_USER | SESSION_USER } [, ...] ]
 [ USING ( using_expression ) ]
@@ -120,6 +121,21 @@ CREATE POLICY name ON 
 

+permissive
+
+ 
+  If the policy is a "permissive" or "restrictive" policy.  Permissive
+  policies are the default and always add visibillity- they ar "OR"d
+  together to allow the user access to all rows through any of the
+  permissive policies they have access to.  Alternativly, a policy can
+  instead by "restrictive", meaning that the policy will be "AND"d
+  with other restrictive policies and with the result of all of the
+  permissive policies on the table.
+ 
+
+   
+
+   
 command
 
  
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index d694cf8..70e22c1 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -235,6 +235,7 @@ RelationBuildRowSecurity(Relation relation)
 		{
 			Datum		value_datum;
 			char		cmd_value;
+			bool		permissive_value;
 			Datum		roles_datum;
 			char	   *qual_value;
 			Expr	   *qual_expr;
@@ -257,6 +258,12 @@ RelationBuildRowSecurity(Relation relation)
 			Assert(!isnull);
 			cmd_value = DatumGetChar(value_datum);
 
+			/* Get policy permissive or restrictive */
+			value_datum = heap_getattr(tuple, Anum_pg_policy_polpermissive,
+	   RelationGetDescr(catalog), );
+			Assert(!isnull);
+			permissive_value = DatumGetBool(value_datum);
+
 			/* Get policy name */
 			value_datum = heap_getattr(tuple, Anum_pg_policy_polname,
 	   RelationGetDescr(catalog), );
@@ -298,6 +305,7 @@ RelationBuildRowSecurity(Relation relation)
 			policy = palloc0(sizeof(RowSecurityPolicy));
 			policy->policy_name = pstrdup(policy_name_value);
 			policy->polcmd = cmd_value;
+			policy->permissive = permissive_value;
 			policy->roles = DatumGetArrayTypePCopy(roles_datum);
 			policy->qual = copyObject(qual_expr);
 			policy->with_check_qual = copyObject(with_check_qual);
@@ -796,6 +804,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
 	values[Anum_pg_policy_polname - 1] = DirectFunctionCall1(namein,
 		 CStringGetDatum(stmt->policy_name));
 	values[Anum_pg_policy_polcmd - 1] = CharGetDatum(polcmd);
+	values[Anum_pg_policy_polpermissive - 1] = BoolGetDatum(stmt->permissive);
 	values[Anum_pg_policy_polroles - 1] = PointerGetDatum(role_ids);
 
 	/* Add qual if present. */
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, Sep 1, 2016 at 12:04 PM, Stephen Frost  wrote:
> > As outlined in the commit message, this adds support for restrictive RLS
> > policies.  We've had this in the backend since 9.5, but they were only
> > available via hooks and therefore extensions.  This adds support for
> > them to be configured through regular DDL commands.  These policies are,
> > essentially "AND"d instead of "OR"d.
> >
> > Includes updates to the catalog, grammer, psql, pg_dump, and regression
> > tests.  Documentation will be added soon, but until then, would be great
> > to get feedback on the grammer, catalog and code changes.
> 
> I don't like CREATE RESTRICT POLICY much.  It's not very good grammar,
> for one thing.  I think putting the word RESTRICT, or maybe AS
> RESTRICT, somewhere later in the command would be better.

I had been notionally thinking RESTRICTIVE, but ended up just using
RESTRICT since it was already an unreserved keyword.  Of course, that's
not a good reason.

> I also think that it is very strange to have the grammar keyword be
> "restrict" but the internal flag be called "permissive".  It would be
> better to have the sense of those flags match.

Permissive is the default and should just be added to the grammar, so
users can be explicit, if they wish to.

* Thom Brown (t...@linux.com) wrote:
> On 1 September 2016 at 10:02, Robert Haas  wrote:
> > I don't like CREATE RESTRICT POLICY much.  It's not very good grammar,
> > for one thing.  I think putting the word RESTRICT, or maybe AS
> > RESTRICT, somewhere later in the command would be better.
> >
> > I also think that it is very strange to have the grammar keyword be
> > "restrict" but the internal flag be called "permissive".  It would be
> > better to have the sense of those flags match.
> >
> > (This is not intended as a full review, just a quick comment.)
> 
> I had proposed this sort of functionality a couple years back:
> https://www.depesz.com/2014/10/02/waiting-for-9-5-row-level-security-policies-rls/#comment-187800
> 
> And I suggested CREATE RESTRICTIVE POLICY, but looking back at that,
> perhaps you're right, and it would be better to add it later in the
> command.

Ah, I had recalled seeing something along those lines somewhere, but
didn't know where, thanks.

Based on Robert's suggestion and using Thom's verbiage, I've tested this
out:

CREATE POLICY pol ON tab AS [PERMISSIVE|RESTRICTIVE] ...

and it appears to work fine with the grammar, etc.

Unless there's other thoughts on this, I'll update the patch to reflect
this grammar in a couple days.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Add support for restrictive RLS policies

[Thom Brown]

On 1 September 2016 at 10:02, Robert Haas  wrote:
> On Thu, Sep 1, 2016 at 12:04 PM, Stephen Frost  wrote:
>> As outlined in the commit message, this adds support for restrictive RLS
>> policies.  We've had this in the backend since 9.5, but they were only
>> available via hooks and therefore extensions.  This adds support for
>> them to be configured through regular DDL commands.  These policies are,
>> essentially "AND"d instead of "OR"d.
>>
>> Includes updates to the catalog, grammer, psql, pg_dump, and regression
>> tests.  Documentation will be added soon, but until then, would be great
>> to get feedback on the grammer, catalog and code changes.
>
> I don't like CREATE RESTRICT POLICY much.  It's not very good grammar,
> for one thing.  I think putting the word RESTRICT, or maybe AS
> RESTRICT, somewhere later in the command would be better.
>
> I also think that it is very strange to have the grammar keyword be
> "restrict" but the internal flag be called "permissive".  It would be
> better to have the sense of those flags match.
>
> (This is not intended as a full review, just a quick comment.)

I had proposed this sort of functionality a couple years back:
https://www.depesz.com/2014/10/02/waiting-for-9-5-row-level-security-policies-rls/#comment-187800

And I suggested CREATE RESTRICTIVE POLICY, but looking back at that,
perhaps you're right, and it would be better to add it later in the
command.

Thom


-- 
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] Add support for restrictive RLS policies

[Robert Haas]

On Thu, Sep 1, 2016 at 12:04 PM, Stephen Frost  wrote:
> As outlined in the commit message, this adds support for restrictive RLS
> policies.  We've had this in the backend since 9.5, but they were only
> available via hooks and therefore extensions.  This adds support for
> them to be configured through regular DDL commands.  These policies are,
> essentially "AND"d instead of "OR"d.
>
> Includes updates to the catalog, grammer, psql, pg_dump, and regression
> tests.  Documentation will be added soon, but until then, would be great
> to get feedback on the grammer, catalog and code changes.

I don't like CREATE RESTRICT POLICY much.  It's not very good grammar,
for one thing.  I think putting the word RESTRICT, or maybe AS
RESTRICT, somewhere later in the command would be better.

I also think that it is very strange to have the grammar keyword be
"restrict" but the internal flag be called "permissive".  It would be
better to have the sense of those flags match.

(This is not intended as a full review, just a quick comment.)

-- 
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] Add support for restrictive RLS policies

[Stephen Frost]

Greetings,

As outlined in the commit message, this adds support for restrictive RLS
policies.  We've had this in the backend since 9.5, but they were only
available via hooks and therefore extensions.  This adds support for
them to be configured through regular DDL commands.  These policies are,
essentially "AND"d instead of "OR"d.

Includes updates to the catalog, grammer, psql, pg_dump, and regression
tests.  Documentation will be added soon, but until then, would be great
to get feedback on the grammer, catalog and code changes.

Thanks!

Stephen
From f4195e9c109d8323266419e487eed2b4cbaafdef Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 1 Sep 2016 02:11:30 -0400
Subject: [PATCH] Add support for restrictive RLS policies

We have had support for restrictive RLS policies since 9.5, but they
were only available through extensions which use the appropriate hooks.
This adds support into the grammer, catalog, psql and pg_dump for
restrictive RLS policies, thus reducing the cases where an extension is
necessary.
---
 src/backend/commands/policy.c |   9 ++
 src/backend/nodes/copyfuncs.c |   1 +
 src/backend/nodes/equalfuncs.c|   1 +
 src/backend/parser/gram.y |  15 +++
 src/backend/rewrite/rowsecurity.c |   7 +-
 src/bin/pg_dump/pg_dump.c |  39 --
 src/bin/pg_dump/pg_dump.h |   1 +
 src/bin/psql/describe.c   | 109 
 src/bin/psql/tab-complete.c   |   1 +
 src/include/catalog/pg_policy.h   |  16 ++-
 src/include/nodes/parsenodes.h|   1 +
 src/include/rewrite/rowsecurity.h |   1 +
 src/test/regress/expected/rowsecurity.out | 207 ++
 src/test/regress/sql/rowsecurity.sql  |  22 +++-
 14 files changed, 332 insertions(+), 98 deletions(-)

diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index d694cf8..70e22c1 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -235,6 +235,7 @@ RelationBuildRowSecurity(Relation relation)
 		{
 			Datum		value_datum;
 			char		cmd_value;
+			bool		permissive_value;
 			Datum		roles_datum;
 			char	   *qual_value;
 			Expr	   *qual_expr;
@@ -257,6 +258,12 @@ RelationBuildRowSecurity(Relation relation)
 			Assert(!isnull);
 			cmd_value = DatumGetChar(value_datum);
 
+			/* Get policy permissive or restrictive */
+			value_datum = heap_getattr(tuple, Anum_pg_policy_polpermissive,
+	   RelationGetDescr(catalog), );
+			Assert(!isnull);
+			permissive_value = DatumGetBool(value_datum);
+
 			/* Get policy name */
 			value_datum = heap_getattr(tuple, Anum_pg_policy_polname,
 	   RelationGetDescr(catalog), );
@@ -298,6 +305,7 @@ RelationBuildRowSecurity(Relation relation)
 			policy = palloc0(sizeof(RowSecurityPolicy));
 			policy->policy_name = pstrdup(policy_name_value);
 			policy->polcmd = cmd_value;
+			policy->permissive = permissive_value;
 			policy->roles = DatumGetArrayTypePCopy(roles_datum);
 			policy->qual = copyObject(qual_expr);
 			policy->with_check_qual = copyObject(with_check_qual);
@@ -796,6 +804,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
 	values[Anum_pg_policy_polname - 1] = DirectFunctionCall1(namein,
 		 CStringGetDatum(stmt->policy_name));
 	values[Anum_pg_policy_polcmd - 1] = CharGetDatum(polcmd);
+	values[Anum_pg_policy_polpermissive - 1] = BoolGetDatum(stmt->permissive);
 	values[Anum_pg_policy_polroles - 1] = PointerGetDatum(role_ids);
 
 	/* Add qual if present. */
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 1877fb4..4fc9525 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4150,6 +4150,7 @@ _copyCreatePolicyStmt(const CreatePolicyStmt *from)
 	COPY_STRING_FIELD(policy_name);
 	COPY_NODE_FIELD(table);
 	COPY_STRING_FIELD(cmd_name);
+	COPY_SCALAR_FIELD(permissive);
 	COPY_NODE_FIELD(roles);
 	COPY_NODE_FIELD(qual);
 	COPY_NODE_FIELD(with_check);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 448e1a9..3e4e15b 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2122,6 +2122,7 @@ _equalCreatePolicyStmt(const CreatePolicyStmt *a, const CreatePolicyStmt *b)
 	COMPARE_STRING_FIELD(policy_name);
 	COMPARE_NODE_FIELD(table);
 	COMPARE_STRING_FIELD(cmd_name);
+	COMPARE_SCALAR_FIELD(permissive);
 	COMPARE_NODE_FIELD(roles);
 	COMPARE_NODE_FIELD(qual);
 	COMPARE_NODE_FIELD(with_check);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index cb5cfc4..a79a1e6 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4633,11 +4633,26 @@ CreatePolicyStmt:
 	n->policy_name = $3;
 	n->table = $5;
 	n->cmd_name = $6;
+	n->permissive = true;
 	n->roles = $7;
 	n->qual = $8;
 	n->with_check = $9;
 	$$ = (Node *) n;
 }
+			| CREATE RESTRICT POLICY name ON qualified_name