Re: [HACKERS] [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

2015-07-30 Thread Dean Rasheed 
On 30 July 2015 at 01:35, Joe Conway m...@joeconway.com wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1

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

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

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

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


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


Looks correct to me.

Thanks.

Regards,
Dean
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
new file mode 100644
index bcf4a8f..cb689ec
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -186,9 +186,6 @@ policy_role_list_to_array(List *roles, i
 /*
  * Load row security policy from the catalog, and store it in
  * the relation's relcache entry.
- *
- * We will always set up some kind of policy here.  If no explicit policies
- * are found then an implicit default-deny policy is created.
  */
 void
 RelationBuildRowSecurity(Relation relation)
@@ -246,7 +243,6 @@ RelationBuildRowSecurity(Relation relati
 			char	   *with_check_value;
 			Expr	   *with_check_qual;
 			char	   *policy_name_value;
-			Oid			policy_id;
 			bool		isnull;
 			RowSecurityPolicy *policy;
 
@@ -298,14 +294,11 @@ RelationBuildRowSecurity(Relation relati
 			else
 with_check_qual = NULL;
 
-			policy_id = HeapTupleGetOid(tuple);
-
 			/* Now copy everything into the cache context */
 			MemoryContextSwitchTo(rscxt);
 
 			policy = palloc0(sizeof(RowSecurityPolicy));
 			policy-policy_name = pstrdup(policy_name_value);
-			policy-policy_id = policy_id;
 			policy-polcmd = cmd_value;
 			policy-roles = DatumGetArrayTypePCopy(roles_datum);
 			policy-qual = copyObject(qual_expr);
@@ -326,40 +319,6 @@ RelationBuildRowSecurity(Relation relati
 
 		systable_endscan(sscan);
 		heap_close(catalog, AccessShareLock);
-
-		/*
-		 * Check if no policies were added
-		 *
-		 * If no policies exist in pg_policy for this relation, then we need
-		 * to create a single default-deny policy.  We use InvalidOid for the
-		 * Oid to indicate that this is the default-deny policy (we may decide
-		 * to ignore the default policy if an extension adds policies).
-		 */
-		if (rsdesc-policies == NIL)
-		{
-			RowSecurityPolicy *policy;
-			Datum		role;
-
-			MemoryContextSwitchTo(rscxt);
-
-			role = ObjectIdGetDatum(ACL_ID_PUBLIC);
-
-			policy = palloc0(sizeof(RowSecurityPolicy));
-			policy-policy_name = pstrdup(default-deny policy);
-			policy-policy_id = InvalidOid;
-			policy-polcmd = '*';
-			policy-roles = construct_array(role, 1, OIDOID, sizeof(Oid), true,
-			'i');
-			policy-qual = (Expr *) makeConst(BOOLOID, -1, InvalidOid,
-		   sizeof(bool), BoolGetDatum(false),
-			  false, true);
-			policy-with_check_qual = copyObject(policy-qual);
-			policy-hassublinks = false;
-
-			rsdesc-policies = lcons(policy, rsdesc-policies);
-
-			MemoryContextSwitchTo(oldcxt);
-		}
 	}
 	PG_CATCH();
 	{
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
new file mode 100644
index 2c65a90..c28eb2b
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1815,14 +1815,26 @@ ExecWithCheckOptions(WCOKind kind, Resul
 	break;
 case WCO_RLS_INSERT_CHECK:
 case WCO_RLS_UPDATE_CHECK:
-	ereport(ERROR,
-			(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+	if (wco-polname != NULL)
+		ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+			 errmsg(new row violates row level security policy \%s\ for \%s\,
+	wco-polname, wco-relname)));
+	else
+		ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 			 errmsg(new row violates row level security policy for \%s\,
 	wco-relname)));
 	break;
 case WCO_RLS_CONFLICT_CHECK:
-	ereport(ERROR,
-			(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+	if (wco-polname != NULL)
+		ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+			 errmsg(new row violates row level security policy \%s\ (USING expression) for \%s\,
+	wco-polname, wco-relname)));
+	else
+		ereport(ERROR,
+

Re: [HACKERS] [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

2015-07-30 Thread Joe Conway 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/30/2015 06:52 AM, Dean Rasheed wrote:
 On 30 July 2015 at 01:35, Joe Conway m...@joeconway.com wrote:
 On 06/01/2015 02:21 AM, Dean Rasheed wrote:
 While going through this, I spotted another issue --- in a DML 
 query with additional non-target relations, such as UPDATE t1
 .. FROM t2 .., the old code was checking the UPDATE policies of
 both t1 and t2, but really I think it ought to be checking the
 SELECT policies of t2 (in the same way as this query requires
 SELECT table permissions on t2, not UPDATE permissions). I've
 changed that and added new regression tests to test that
 change.
 
 I assume the entire refactoring patch needs a fair bit of work
 to rebase against current HEAD,
 
 Actually, there haven't been any conflicting changes so far, so a
 git rebase was able to automatically merge correctly -- new patch 
 attached, with some minor comment rewording (not affecting the
 bug-fix part).

Good to hear.

 Even so, I agree that it makes sense to apply the bug-fix
 separately, since it's not really anything to do with the
 refactoring.
 
 but I picked out the attached to address just the above issue.
 Does this look correct, and if so does it make sense to apply at
 least this part right now?
 
 Looks correct to me.

Thanks -- committed and pushed to HEAD and 9.5

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

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

2015-07-29 Thread Joe Conway 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

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

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

Thanks,

Joe

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

iQIcBAEBAgAGBQJVuXFuAAoJEDfy90M199hlOMkP/RZoBU0MJF64GjHfuLVa3T5w
PnDrnIoLBMOgOzkXrI+Ov0w0ESXltNvYjyIxkuyaB5PuoeDOdyYnnzbpPe7hH4pv
zDjSinJnfNmFEcm0UjBzuBiSH0dv52PEIToexTKu6SnjvH3Co/Hk4Ar2uZ0r7bRF
krl+Dd4kZX1uuRIigsSFqy873C79m3o11Szs5aW8c5od9adGxSvRHqZNf/UIDIbZ
CxAo0E3Tlw0DmGl1cw1tdN8gWMzmvx5dQ0ih3+0/hkVUqN9p3Pg8BGajSvxxFlb2
l4+6RZGUw5ZTpJxRZf/zPc4updhq0zf/Z5g7GUYddrhO29eLS6al2ySlb7HL5G9M
VPMjEzXuhFwhxSMdgHfz8UQh82KuNENMTKp81BvtIgZ7w86A9lWrKxCLaVMGhi8m
MnfCQ4cdOmnE2vEHi0Ue3Dg/rvYO8QpVW0JKDOdzoqPErC7to9vorXI5X3vcfLbF
fYfiJe6wUwbDEdxh/z0oKVFxWlf2NRk4pd+jZ7C+ibraLIwgEgZe7G4GEje5LxSP
h4zTfx0sj3IyrvqziurO/aZqIBXBZEsm3Gv6OQs26C5Xvkx/QmgROB42lHwcDYri
BTk6+uzNYKbX+kW56vEY0f0WMTLYZzc6jZRIVr3s+aLeyG0P9acY/n3uY1xBFCZZ
Xb7gmepAN3rY1CF7By9o
=e5hz
-END PGP SIGNATURE-
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index 2386cf0..562dbc9 100644
*** a/src/backend/rewrite/rowsecurity.c
--- b/src/backend/rewrite/rowsecurity.c
*** get_row_security_policies(Query *root, C
*** 147,154 
  		return;
  	}
  
! 	/* Grab the built-in policies which should be applied to this relation. */
  	rel = heap_open(rte-relid, NoLock);
  
  	rowsec_policies = pull_row_security_policies(commandType, rel,
   user_id);
--- 147,164 
  		return;
  	}
  
! 	/*
! 	 * RLS is enabled for this relation.
! 	 *
! 	 * Get the security policies that should be applied, based on the command
! 	 * type.  Note that if this isn't the target relation, we actually want
! 	 * the relation's SELECT policies, regardless of the query command type,
! 	 * for example in UPDATE t1 ... FROM t2 we need to apply t1's UPDATE
! 	 * policies and t2's SELECT policies.
! 	 */
  	rel = heap_open(rte-relid, NoLock);
+ 	if (rt_index != root-resultRelation)
+ 		commandType = CMD_SELECT;
  
  	rowsec_policies = pull_row_security_policies(commandType, rel,
   user_id);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index b0556c2..6fc80af 100644
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*** CREATE POLICY p ON t USING (max(c)); --
*** 3033,3038 
--- 3033,3121 
  ERROR:  aggregate functions are not allowed in policy expressions
  ROLLBACK;
  --
+ -- Non-target relations are only subject to SELECT policies
+ --
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ CREATE TABLE r1 (a int);
+ CREATE TABLE r2 (a int);
+ INSERT INTO r1 VALUES (10), (20);
+ INSERT INTO r2 VALUES (10), (20);
+ GRANT ALL ON r1, r2 TO rls_regress_user1;
+ CREATE POLICY p1 ON r1 USING (true);
+ ALTER TABLE r1 ENABLE ROW LEVEL SECURITY;
+ CREATE POLICY p1 ON r2 FOR SELECT USING (true);
+ CREATE POLICY p2 ON r2 FOR INSERT WITH CHECK (false);
+ CREATE POLICY p3 ON r2 FOR UPDATE USING (false);
+ CREATE POLICY p4 ON r2 FOR DELETE USING (false);
+ ALTER TABLE r2 ENABLE ROW LEVEL SECURITY;
+ SET SESSION AUTHORIZATION rls_regress_user1;
+ SELECT * FROM r1;
+  a  
+ 
+  10
+  20
+ (2 rows)
+ 
+ SELECT * FROM r2;
+  a  
+ 
+  10
+  20
+ (2 rows)
+ 
+ -- r2 is read-only
+ INSERT INTO r2 VALUES (2); -- Not allowed
+ ERROR:  new row violates row level security policy for r2
+ UPDATE r2 SET a = 2 RETURNING *; -- Updates nothing
+  a 
+ ---
+ (0 rows)
+ 
+ DELETE FROM r2 RETURNING *; -- Deletes nothing
+  a 
+ ---
+ (0 rows)
+ 
+ -- r2 can be used as a non-target relation in DML
+ INSERT INTO r1 SELECT a + 1 FROM r2 RETURNING *; -- OK
+  a  
+ 
+  11
+  21
+ (2 rows)
+ 
+ UPDATE r1 SET a = r2.a + 2 FROM r2 WHERE r1.a = r2.a RETURNING *; -- OK
+  a  | a  
+ +
+  12 | 10
+  22 | 20
+ (2 rows)
+ 
+ DELETE FROM r1 USING r2 WHERE r1.a = r2.a + 2 RETURNING *; -- OK
+  a  | a  
+ +
+  12 | 10
+  22 | 20
+ (2 rows)
+ 
+ SELECT * FROM r1;
+  a  
+ 
+  11
+  21
+ (2 rows)
+ 
+ SELECT * FROM r2;
+  a  
+ 
+  10
+  20
+ (2 rows)
+ 
+ SET SESSION AUTHORIZATION rls_regress_user0;
+ DROP TABLE r1;
+ DROP TABLE r2;
+ --
  -- Clean up objects
  --
  RESET SESSION AUTHORIZATION;
diff --git a/src/test/regress/sql/rowsecurity.sql

Re: [HACKERS] [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

2015-06-01 Thread Dean Rasheed 
On 28 May 2015 at 08:51, Stephen Frost sfr...@snowman.net wrote:
 * Dean Rasheed (dean.a.rash...@gmail.com) wrote:
 Actually I think a new boolean field is unnecessary, and probably the
 policy_id field is too. Re-reading the code in rowsecurity.c, I think
 it could use a bit of refactoring. Essentially what it needs to do
 (for permissive policies at least) is just

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

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

 I tend to agree.

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

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


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

The main changes are:

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

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

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

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

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

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

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

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

Regards,
Dean
diff --git

Re: [HACKERS] [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

2015-05-28 Thread Dean Rasheed 
On 27 May 2015 at 14:51, Stephen Frost sfr...@snowman.net wrote:
 On 27 May 2015 at 02:42, Stephen Frost sfr...@snowman.net wrote:
  Now, looking at the code, I'm actually failing to see a case where we
  use the RowSecurityPolicy-policy_name..  Perhaps *that's* what we
  should be looking to remove?

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

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


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


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

 Thoughts?


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

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

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

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

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

Regards,
Dean


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

Re: [HACKERS] [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

2015-05-28 Thread Stephen Frost 
Dean,

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
 On 27 May 2015 at 14:51, Stephen Frost sfr...@snowman.net wrote:
  On 27 May 2015 at 02:42, Stephen Frost sfr...@snowman.net wrote:
   Now, looking at the code, I'm actually failing to see a case where we
   use the RowSecurityPolicy-policy_name..  Perhaps *that's* what we
   should be looking to remove?
 
  If we add support for restrictive policies, it would be possible, and
  I think desirable, to report on which policy was violated. For that,
  having the policy name would be handy. We might also arguably decide
  to enforce restrictive RLS policies in name order, like check
  constraints. Of course none of that means it must be kept now, but it
  feels like a useful field to keep nonetheless.
 
  I agree that it could be useful, but we really shouldn't have fields in
  the current code base which are just in case..  The one exception
  which comes to mind is if we should keep it for use by extensions.
  Those operate on an independent cycle from our major releases and would
  likely find having the name field useful.
 
 Hmm, when I wrote that I had forgotten that we already allow
 extensions to add restrictive policies. I think it would be good to
 allow those policies to have names, and if they do, to copy those
 names to any WCOs created. Then, if a WCO is violated, and it has a
 non-NULL policy name associated with it, we should include that in the
 error report.

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

  One thing which now occurs to me, however, is that, while the current
  coding is fine, using InvalidOid as an indicator for the default-deny
  policy, in general, does fall over when we consider policies added by
  extensions.  Those policies are necessairly going to need to put
  InvalidOid into that field, unless they acquire an OID somehow
  themselves (it doesn't seem reasonable to make that a requirement,
  though I suppose we could..), and, therefore, perhaps we should add a
  boolean field which indicates which is the defaultDeny policy anyway and
  not use that field for that purpose.
 
 Actually I think a new boolean field is unnecessary, and probably the
 policy_id field is too. Re-reading the code in rowsecurity.c, I think
 it could use a bit of refactoring. Essentially what it needs to do
 (for permissive policies at least) is just
 
 * fetch a list of internal policies
 * fetch a list of external policies
 * concatenate them
 * if the resulting list is empty, add a default-deny qual and/or WCO
 
 By only building the default-deny qual/WCO at the end, rather than
 half-way through and pretending that it's a fully-fledged policy, the
 code ought to be simpler.

I tend to agree.

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

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

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

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

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

2015-05-27 Thread Stephen Frost 
Dean,

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
 On 27 May 2015 at 02:42, Stephen Frost sfr...@snowman.net wrote:
  Now, looking at the code, I'm actually failing to see a case where we
  use the RowSecurityPolicy-policy_name..  Perhaps *that's* what we
  should be looking to remove?
 
 If we add support for restrictive policies, it would be possible, and
 I think desirable, to report on which policy was violated. For that,
 having the policy name would be handy. We might also arguably decide
 to enforce restrictive RLS policies in name order, like check
 constraints. Of course none of that means it must be kept now, but it
 feels like a useful field to keep nonetheless.

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

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

Thoughts?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

2015-05-26 Thread Stephen Frost 
Alvaro,

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 Stephen Frost wrote:
  * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
   What do we need RowSecurityPolicy-policy_id for?  It seems to me that
   it is only used to determine whether the policy is the default deny
   one, so that it can later be removed if a hook adds a different one.
   This seems contrived as well as under-documented.  Why isn't a boolean
   flag sufficient?
  
  Thanks for taking a look!
  
  It's also used during relcache updates (see equalPolicy()).
 
 Hmm, but the policy name is unique also, right?  So the policy_id check
 is redundant ...

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

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

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

2015-05-26 Thread Stephen Frost 
Alvaro,

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
 What do we need RowSecurityPolicy-policy_id for?  It seems to me that
 it is only used to determine whether the policy is the default deny
 one, so that it can later be removed if a hook adds a different one.
 This seems contrived as well as under-documented.  Why isn't a boolean
 flag sufficient?

Thanks for taking a look!

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

Thanks again!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

2015-05-26 Thread Alvaro Herrera 
Stephen Frost wrote:
 Alvaro,
 
 * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
  What do we need RowSecurityPolicy-policy_id for?  It seems to me that
  it is only used to determine whether the policy is the default deny
  one, so that it can later be removed if a hook adds a different one.
  This seems contrived as well as under-documented.  Why isn't a boolean
  flag sufficient?
 
 Thanks for taking a look!
 
 It's also used during relcache updates (see equalPolicy()).

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

-- 
Álvaro 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] [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

2015-05-22 Thread Alvaro Herrera 
Stephen Frost wrote:
 Row-Level Security Policies (RLS)

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

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

-- 
Álvaro 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