Re: [HACKERS] Add support for restrictive RLS policies
* 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 + + + Referenti
Re: [HACKERS] Add support for restrictive RLS policies
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
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
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
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
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
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 short, I'm not entirely s
Re: [HACKERS] Add support for restrictive RLS policies
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 (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 ensure that dat
Re: [HACKERS] Add support for restrictive RLS policies
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
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
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
* 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 src/backend/catalog/system_views.
Re: [HACKERS] Add support for restrictive RLS policies
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
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 after
Re: [HACKERS] Add support for restrictive RLS policies
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
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
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
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
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 src/bin/psql/tab-complete.c | 29 - src/include/catalog/pg_
Re: [HACKERS] Add support for restrictive RLS policies
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
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
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
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
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
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
* 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
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
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
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
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), &isnull); + Assert(!isnull); + permissive_value = DatumGetBool(value_datum); + /* Get policy name */ value_datum = heap_getattr(tuple, Anum_pg_policy_polname, RelationGetDescr(catalog), &isnull); @@ -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 4f39da
Re: [HACKERS] Add support for restrictive RLS policies
* 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
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
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
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), &isnull); + Assert(!isnull); + permissive_value = DatumGetBool(value_datum); + /* Get policy name */ value_datum = heap_getattr(tuple, Anum_pg_policy_polname, RelationGetDescr(catalog), &isnull); @@ -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 RowSecurit