Re: [HACKERS] Row-security on updatable s.b. views

[Craig Ringer]

On 03/08/2014 01:56 AM, Tom Lane wrote:
 Craig Ringer cr...@2ndquadrant.com writes:
 What I'm concerned about is the locking. It looks to me like we're
 causing the user to lock rows that they may not intend to lock, by
 applying a LockRows step *before* the user supplied qual. (I'm going to
 test that tomorrow, it's sleep time in Australia).
 
 The fact that there are two LockRows nodes seems outright broken.
 The one at the top of the plan is correctly placed, but how did the
 other one get in there?

I initially thought it was the updatable security barrier views code
pushing the RowMark down into the generated subquery. But if I remove
the pushdown code the inner LockRows node still seems to get emitted.

In fact, it's not a new issue. In vanilla 9.3.1:


regress= select version();
   version

--
 PostgreSQL 9.3.1 on x86_64-unknown-linux-gnu, compiled by gcc (GCC)
4.8.1 20130603 (Red Hat 4.8.1-1), 64-bit
(1 row)

regress= CREATE TABLE t1(x integer, y integer);
CREATE TABLE
regress= INSERT INTO t1(x,y) VALUES (1,1), (2,2), (3,3), (4,4);
INSERT 0 4
regress= CREATE VIEW v1 WITH (security_barrier) AS SELECT x, y FROM t1
  WHERE x % 2 = 0;
CREATE VIEW
regress= CREATE OR REPLACE FUNCTION user_qual() RETURNS boolean AS $$
  BEGIN RETURN TRUE; END;
  $$ LANGUAGE plpgsql;
CREATE FUNCTION

regress=  EXPLAIN SELECT * FROM v1 WHERE user_qual() FOR UPDATE;
  QUERY PLAN
---
 LockRows  (cost=0.00..45.11 rows=4 width=40)
   -  Subquery Scan on v1  (cost=0.00..45.07 rows=4 width=40)
 Filter: user_qual()
 -  LockRows  (cost=0.00..42.21 rows=11 width=14)
   -  Seq Scan on t1  (cost=0.00..42.10 rows=11 width=14)
 Filter: ((x % 2) = 0)
(6 rows)



so it looks like security barrier views are locking rows they should not be.

I can confirm that on 9.3.1 with:

CREATE OR REPLACE FUNCTION row_is(integer, integer) RETURNS boolean as
$$ begin return (select $1 = $2); end; $$ language plpgsql;

then in two sessions:

SELECT * FROM v1 WHERE row_is(x, 2) FOR UPDATE;

and

SELECT * FROM v1 WHERE row_is(x, 4) FOR UPDATE;


These should not block each other, but do.

So there's a pre-existing bug here.

-- 
 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] Row-security on updatable s.b. views

[Craig Ringer]

On 03/05/2014 11:02 AM, Craig Ringer wrote:
 The main known issue remaining is plan invalidation.

I've pushed a version with a plan invalidation implementation. It's tagged:

  rls-9.4-upd-sb-views-v9

in

  g...@github.com:ringerc/postgres.git

The invalidation implementation does not yet handle foreign key checks;
that will require additional changes. I'll push an update to the
rls-9.4-upd-sb-views and post an update later, at which time I'll rebase
the changes back into the history.

-- 
 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] Row-security on updatable s.b. views

[Craig Ringer]

On 03/07/2014 09:33 PM, Craig Ringer wrote:
 On 03/05/2014 11:02 AM, Craig Ringer wrote:
 The main known issue remaining is plan invalidation.
 
 I've pushed a version with a plan invalidation implementation. It's tagged:
 
   rls-9.4-upd-sb-views-v9
 
 in
 
   g...@github.com:ringerc/postgres.git
 
 The invalidation implementation does not yet handle foreign key checks;
 that will require additional changes. I'll push an update to the
 rls-9.4-upd-sb-views and post an update later, at which time I'll rebase
 the changes back into the history.

Well, that was easy. Done.

  rls-9.4-upd-sb-views-v11

and rebased the rls-9.4-upd-sb-views branch to incorporate the changes.

The regression tests have further failures, but some are due to changes
in the inheritance semantics. I'm going through them now.

-- 
 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] Row-security on updatable s.b. views

[Craig Ringer]

On 03/07/2014 10:07 PM, Craig Ringer wrote:
 On 03/07/2014 09:33 PM, Craig Ringer wrote:
 On 03/05/2014 11:02 AM, Craig Ringer wrote:
 The main known issue remaining is plan invalidation.

 I've pushed a version with a plan invalidation implementation. It's tagged:

   rls-9.4-upd-sb-views-v9

 in

   g...@github.com:ringerc/postgres.git

 The invalidation implementation does not yet handle foreign key checks;
 that will require additional changes. I'll push an update to the
 rls-9.4-upd-sb-views and post an update later, at which time I'll rebase
 the changes back into the history.
 
 Well, that was easy. Done.
 
   rls-9.4-upd-sb-views-v11
 
 and rebased the rls-9.4-upd-sb-views branch to incorporate the changes.
 
 The regression tests have further failures, but some are due to changes
 in the inheritance semantics. I'm going through them now.
 


Need a quick opinion.

KaiGai's original code produced a plan like this for an inheritance set:

  EXPLAIN (costs off) SELECT * FROM t1 WHERE f_leak(b) FOR SHARE;
! QUERY PLAN
! ---
   LockRows
!-  Append
!  -  Subquery Scan on t1
!Filter: f_leak(t1.b)
!-  Seq Scan on t1 t1_1
!  Filter: ((a % 2) = 0)
!  -  Subquery Scan on t2
!Filter: f_leak(t2.b)
!-  Seq Scan on t2 t2_1
!  Filter: ((a % 2) = 1)
!  -  Seq Scan on t3
!Filter: f_leak(b)
  (12 rows)



The new code, using updatable s.b. views, instead produces:


  EXPLAIN (costs off) SELECT * FROM t1 WHERE f_leak(b) FOR SHARE;
!   QUERY PLAN
! ---
   LockRows
!-  Subquery Scan on t1
!  Filter: f_leak(t1.b)
!  -  LockRows
!-  Result
!  -  Append
!-  Seq Scan on t1 t1_1
!  Filter: ((a % 2) = 0)
!-  Seq Scan on t2
!  Filter: ((a % 2) = 0)
!-  Seq Scan on t3
!  Filter: ((a % 2) = 0)
  (12 rows)



The different quals are expected, because of the change to the
definition of inheritance handling in row security.

What I'm concerned about is the locking. It looks to me like we're
causing the user to lock rows that they may not intend to lock, by
applying a LockRows step *before* the user supplied qual. (I'm going to
test that tomorrow, it's sleep time in Australia).

This seems to be related to RowMark handling in updatable security
barrier views. I need to check whether it happens with updates to
security barrier views, as well as with row security.

Dean, any thoughts?



-- 
 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] Row-security on updatable s.b. views

[Tom Lane]

Craig Ringer cr...@2ndquadrant.com writes:
 What I'm concerned about is the locking. It looks to me like we're
 causing the user to lock rows that they may not intend to lock, by
 applying a LockRows step *before* the user supplied qual. (I'm going to
 test that tomorrow, it's sleep time in Australia).

The fact that there are two LockRows nodes seems outright broken.
The one at the top of the plan is correctly placed, but how did the
other one get in there?

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] Row-security on updatable s.b. views

[Yeb Havinga]

On 05/03/14 15:44, Craig Ringer wrote:

On 03/05/2014 05:25 PM, Yeb Havinga wrote:

Maybe a naive thought, but shouldn't all plans that include a table with
an RLS clause be invalidated when the session role switches, regardless
of which users from and to?

Only if the plan is actually accessed when under a different user ID.
Consider SECURITY DEFINER functions; you don't want to flush all cached
plans just because you ran a SECURITY DEFINER function that doesn't even
share any statements with the outer transaction.

Hmm good point.


I've also got some concerns about the user visible API; I'm not sure it
makes sense to set row security policy for row reads per-command in
PostgreSQL, since we have the RETURNING clause. Read-side policy should
just be FOR READ.

Hmm but FOR READ would mean new keywords, and SELECT is also a concept
known to users. I didn't find the api problematic to understand, on the
contrary.

Would you expect that FOR SELECT also affects rows you can see to
UPDATE, INSERT, or DELETE?

Yes.

Because that's what it would have to mean, really. Otherwise, you could
just use `UPDATE thetable SET id = id RETURNING *` (or whatever) to read
the rows out if you had UPDATE rights. Or do the same with DELETE.

With RETURNING, it doesn't make much sense for different statements to
have different read access. Can you think of a case where it'd be
reasonable to deny SELECT, but allow someone to see the same rows with
`UPDATE ... RETURNING` ?


It might be an idea to add the SELECT RLS clause for DML
queries that contain a RETURNING clause.

That way lies madness: A DML statement that affects *a different set of
rows* depending on whether or not it has a RETURNING clause.
If you state it like that, it sounds like a POLA violation. But the 
complete story is: A user is allowed to UPDATE a set of rows from a 
table that is not a subsect of the set of rows he can SELECT from the 
table, iow he can UPDATE rows he is not allowed to SELECT. This can lead 
to unexpected results: When the user issues an UPDATE of the table 
without a returning clause, all rows the user may UPDATE are affected. 
When the user issues an UPDATE of the table with a returning clause, all 
rows the user may UPDATE and SELECT are affected.


So the madness comes from the fact that it is allowed to define RLS that 
allow to modify rows you cannot select. Either prevent these conditions 
(i.e. proof that all DML RLS qual implies the SELECT qual, otherwise 
give an error on DML with a RETURNING clause), or allow it without 
violating the RLS rules but accept that a DML with RETURNING is 
different from a DML only.


regards,
Yeb



--
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] Row-security on updatable s.b. views

[Yeb Havinga]

On 06/03/14 02:56, Craig Ringer wrote:

On 03/06/2014 04:56 AM, Yeb Havinga wrote:


If you state it like that, it sounds like a POLA violation. But the
complete story is: A user is allowed to UPDATE a set of rows from a
table that is not a subsect of the set of rows he can SELECT from the
table, iow he can UPDATE rows he is not allowed to SELECT.




Is there a compelling use case for this? Where it really makes sense to
let users update/delete rows they cannot see via row security? We
support it in the table based permissions model, but it's possible to do
it with much saner semantics there. And with row security, it'll be
possible with security definer functions. I intend to add a row
security exempt flag for functions down the track, too.

Use case: https://en.wikipedia.org/wiki/Bell-La_Padula_model - being 
able to write up and read down access levels.


So for instance in healthcare, a data enterer may enter from hand 
written notes sensitive data (like subject has aids) in the electronic 
health record, without having general read access of the level of 
sensitivity of aids diagnosis. I think what is important in use cases 
like this, is that at data entry time, the actual data is still at the 
desk, so having data returned for inserts in the running transaction 
might not be problematic. As most EHR's today are additive in nature, 
future additions about the aids conditions would be inserts as well, no 
updates required. For updates my best guess would be that allowing the 
command to run with rls permissions different from the select is not 
required.


regards,
Yeb



--
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] Row-security on updatable s.b. views

[Yeb Havinga]

On 2014-03-05 04:02, Craig Ringer wrote:

On 03/04/2014 09:41 PM, Yeb Havinga wrote:

On 04/03/14 02:36, Craig Ringer wrote:

I've pushed an update to the branch with the fix for varno handling.
Thanks. It's tagged rls-9.4-upd-sb-views-v8 .

I've almost run out of time to spend on row security for this
commitfest, unfortunately. I'm putting a blog together with a current
status update. Frustrating, as it's coming together now.

Open issues include:

- Passing plan inval items from rewriter into planner
- COPY support pending
- Clear syntax in DDL

Most of the rest are solved; it's actually looking pretty good.

Hi Craig,

I've tested the results from the minirim.sql that was posted earlier,
and the v8 gives the same results as v4 :-)

Good to hear.

The main known issue remaining is plan invalidation. Row security needs
to create PlanInvalItems during _rewrite_ and also needs to set a
PlannerGlobal field for the user ID the plan depends on. If it fails to
do this then the superuser will sometimes run queries and have
row-security applied, non-superusers might skip row security when it
should be applied. This seems to be the cause of foreign key check
issues I've observed, too.

That's not trivial, because right now the rewriter only returns a Query
node. It doesn't have anywhere to stash information that's global across
the whole query, and adding fields to Query for the purpose doesn't seem
ideal, since it's also used for subqueries, and in the planner.


I looked up the Query structure and steps of e.g. exec_simple_query(), 
but ISTM that Query would be the place to store a used id. After all it 
is meta data about the query. Duplication of this information in the 
presence of subqueries seems less ugly to me than trying to evade 
duplication by wrapping a structure around a query list.


Maybe a naive thought, but shouldn't all plans that include a table with 
an RLS clause be invalidated when the session role switches, regardless 
of which users from and to?




I've also got some concerns about the user visible API; I'm not sure it
makes sense to set row security policy for row reads per-command in
PostgreSQL, since we have the RETURNING clause. Read-side policy should
just be FOR READ.


Hmm but FOR READ would mean new keywords, and SELECT is also a concept 
known to users. I didn't find the api problematic to understand, on the 
contrary. It might be an idea to add the SELECT RLS clause for DML 
queries that contain a RETURNING clause.


regards,
Yeb



--
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] Row-security on updatable s.b. views

[Craig Ringer]

On 03/05/2014 05:25 PM, Yeb Havinga wrote:
 Maybe a naive thought, but shouldn't all plans that include a table with
 an RLS clause be invalidated when the session role switches, regardless
 of which users from and to?

Only if the plan is actually accessed when under a different user ID.
Consider SECURITY DEFINER functions; you don't want to flush all cached
plans just because you ran a SECURITY DEFINER function that doesn't even
share any statements with the outer transaction.

Anyway, the same issue remains: how to pass the information this plan
is user-id specific from rewriter to planner.

 I've also got some concerns about the user visible API; I'm not sure it
 makes sense to set row security policy for row reads per-command in
 PostgreSQL, since we have the RETURNING clause. Read-side policy should
 just be FOR READ.
 
 Hmm but FOR READ would mean new keywords, and SELECT is also a concept
 known to users. I didn't find the api problematic to understand, on the
 contrary.

Would you expect that FOR SELECT also affects rows you can see to
UPDATE, INSERT, or DELETE?

Because that's what it would have to mean, really. Otherwise, you could
just use `UPDATE thetable SET id = id RETURNING *` (or whatever) to read
the rows out if you had UPDATE rights. Or do the same with DELETE.

With RETURNING, it doesn't make much sense for different statements to
have different read access. Can you think of a case where it'd be
reasonable to deny SELECT, but allow someone to see the same rows with
`UPDATE ... RETURNING` ?

 It might be an idea to add the SELECT RLS clause for DML
 queries that contain a RETURNING clause.

That way lies madness: A DML statement that affects *a different set of
rows* depending on whether or not it has a RETURNING clause.

-- 
 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] Row-security on updatable s.b. views

[Craig Ringer]

On 03/06/2014 04:56 AM, Yeb Havinga wrote:
 It might be an idea to add the SELECT RLS clause for DML
 queries that contain a RETURNING clause.
 That way lies madness: A DML statement that affects *a different set of
 rows* depending on whether or not it has a RETURNING clause.
 If you state it like that, it sounds like a POLA violation. But the
 complete story is: A user is allowed to UPDATE a set of rows from a
 table that is not a subsect of the set of rows he can SELECT from the
 table, iow he can UPDATE rows he is not allowed to SELECT. This can lead
 to unexpected results: When the user issues an UPDATE of the table
 without a returning clause, all rows the user may UPDATE are affected.
 When the user issues an UPDATE of the table with a returning clause, all
 rows the user may UPDATE and SELECT are affected.

Well, I think that's bizarre behaviour.

It doesn't make sense for RETURNING to affect the behaviour of the
command it's applied to. It never has before, and it's defined to return
the set of rows affected by the command. It shouldn't be changing that
set, it isn't a predicate.

 So the madness comes from the fact that it is allowed to define RLS that
 allow to modify rows you cannot select. Either prevent these conditions
 (i.e. proof that all DML RLS qual implies the SELECT qual, otherwise
 give an error on DML with a RETURNING clause)

Equivalence proofs in predicates are WAY outside what's going to be
reasonable to tackle in a feature like this. Especially since the row
security expression may be my_c_function(the_row), and all the detail
of behaviour is hidden behind some C function.

We need to treat row security expressions as pretty much opaque.

 or allow it without
 violating the RLS rules but accept that a DML with RETURNING is
 different from a DML only.

I don't think that's acceptable. Too many tools automatically add a
RETURNING clause, or use one in generated SQL. Notably, PgJDBC will
append a RETURNING clause to your query so it can return generated keys.

With regular permissions, we require that the user has SELECT rights if
they use a RETURNING clause. That works because it's a simple
permissions check. With row security, we'd be affecting a different set
of rows instead, and doing so silently. That's just ugly. (It also
creates execution inefficiencies where the SELECT and UPDATE/DELETE
predicates are the same).

Additionally, in PostgreSQL if you can supply a predicate for a row, you
can leak the row via a RAISE NOTICE or other tricks. So even without
RETURNING, allowing a user to update/delete a row permits them to
potentially see the row. (This is an issue with our current permissions
too; if you DELETE with a leaky predicate, you can see the rows you
deleted even without SELECT rights on a table).

That, IMO, is two good reasons not to differentiate between command
types for the purpose of which rows they can see in a scan.

We already have a mechanism for allowing users to do things that they
can't normally do under controlled and restricted circumstances:
SECURITY DEFINER functions.

I don't think we need to introduce bizarre and surprising behaviour in
DML when we have a viable mechanism for people who need to do this.

Is there a compelling use case for this? Where it really makes sense to
let users update/delete rows they cannot see via row security? We
support it in the table based permissions model, but it's possible to do
it with much saner semantics there. And with row security, it'll be
possible with security definer functions. I intend to add a row
security exempt flag for functions down the track, too.

-- 
 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] Row-security on updatable s.b. views

[Yeb Havinga]

On 04/03/14 02:36, Craig Ringer wrote:

On 02/25/2014 01:28 AM, Dean Rasheed wrote:

On 13 February 2014 04:12, Craig Ringer cr...@2ndquadrant.com wrote:

It's crashing while pulling up the query over emp (hl7.employee) and
part (hl7.participation).

Given the simplicity of what the row-security code its self is doing,
I'm wondering if this is a case that isn't handled in updatable s.b.
views. I'll look into it.

I'm not sure how much further you've got with this, but I think the
issue is that the securityQuals that you're adding don't refer to the
correct RTE. When adding securityQuals to an RTE, they are expected to
have Vars whose varno matches the rt_index of the RTE (see for example
the code in rewriteTargetView() which calls ChangeVarNodes() on
viewqual before adding the qual to securityQuals or the main query
jointree). prepend_row_security_quals() doesn't appear to have any
similar code, and it would need to be passed the rt_index to do that.

Thanks for the pointer. That was indeed the issue.

I've pushed an update to the branch with the fix for varno handling.
Thanks. It's tagged rls-9.4-upd-sb-views-v8 .

I've almost run out of time to spend on row security for this
commitfest, unfortunately. I'm putting a blog together with a current
status update. Frustrating, as it's coming together now.

Open issues include:

- Passing plan inval items from rewriter into planner
- COPY support pending
- Clear syntax in DDL

Most of the rest are solved; it's actually looking pretty good.


Hi Craig,

I've tested the results from the minirim.sql that was posted earlier, 
and the v8 gives the same results as v4 :-)


Thanks for all the work!
Yeb



--
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] Row-security on updatable s.b. views

[Yeb Havinga]

On 04/03/14 02:36, Craig Ringer wrote:

On 02/25/2014 01:28 AM, Dean Rasheed wrote:

On 13 February 2014 04:12, Craig Ringer cr...@2ndquadrant.com wrote:

It's crashing while pulling up the query over emp (hl7.employee) and
part (hl7.participation).

Given the simplicity of what the row-security code its self is doing,
I'm wondering if this is a case that isn't handled in updatable s.b.
views. I'll look into it.

I'm not sure how much further you've got with this, but I think the
issue is that the securityQuals that you're adding don't refer to the
correct RTE. When adding securityQuals to an RTE, they are expected to
have Vars whose varno matches the rt_index of the RTE (see for example
the code in rewriteTargetView() which calls ChangeVarNodes() on
viewqual before adding the qual to securityQuals or the main query
jointree). prepend_row_security_quals() doesn't appear to have any
similar code, and it would need to be passed the rt_index to do that.

Thanks for the pointer. That was indeed the issue.

I've pushed an update to the branch with the fix for varno handling.
Thanks. It's tagged rls-9.4-upd-sb-views-v8 .

I've almost run out of time to spend on row security for this
commitfest, unfortunately. I'm putting a blog together with a current
status update. Frustrating, as it's coming together now.

Open issues include:

- Passing plan inval items from rewriter into planner
- COPY support pending
- Clear syntax in DDL

Most of the rest are solved; it's actually looking pretty good.


Hi Craig,

I've tested the results from the minirim.sql that was posted earlier, 
and the v8 gives the same results as v4 :-)


Thanks for all the work!
Yeb



--
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] Row-security on updatable s.b. views

[Craig Ringer]

On 03/04/2014 09:41 PM, Yeb Havinga wrote:
 On 04/03/14 02:36, Craig Ringer wrote:

 I've pushed an update to the branch with the fix for varno handling.
 Thanks. It's tagged rls-9.4-upd-sb-views-v8 .

 I've almost run out of time to spend on row security for this
 commitfest, unfortunately. I'm putting a blog together with a current
 status update. Frustrating, as it's coming together now.

 Open issues include:

 - Passing plan inval items from rewriter into planner
 - COPY support pending
 - Clear syntax in DDL

 Most of the rest are solved; it's actually looking pretty good.
 
 Hi Craig,
 
 I've tested the results from the minirim.sql that was posted earlier,
 and the v8 gives the same results as v4 :-)

Good to hear.

The main known issue remaining is plan invalidation. Row security needs
to create PlanInvalItems during _rewrite_ and also needs to set a
PlannerGlobal field for the user ID the plan depends on. If it fails to
do this then the superuser will sometimes run queries and have
row-security applied, non-superusers might skip row security when it
should be applied. This seems to be the cause of foreign key check
issues I've observed, too.

That's not trivial, because right now the rewriter only returns a Query
node. It doesn't have anywhere to stash information that's global across
the whole query, and adding fields to Query for the purpose doesn't seem
ideal, since it's also used for subqueries, and in the planner. Changing
the rewriter to return a RewriteResult struct that contains the Query
and some other global info would be very intrusive, though, as it'd
affect all the plan cache and invalidation machinery and the various
rewriter/planner call sites.

I've also got some concerns about the user visible API; I'm not sure it
makes sense to set row security policy for row reads per-command in
PostgreSQL, since we have the RETURNING clause. Read-side policy should
just be FOR READ. Initially I think we should be using triggers to
enforce write side policy, which should raise errors if you try to
insert/update/delete the wrong thing. Could move to something working a
bit more like WITH CHECK OPTION later.


-- 
 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] Row-security on updatable s.b. views

[Craig Ringer]

On 02/25/2014 01:28 AM, Dean Rasheed wrote:
 On 13 February 2014 04:12, Craig Ringer cr...@2ndquadrant.com wrote:

 It's crashing while pulling up the query over emp (hl7.employee) and
 part (hl7.participation).

 Given the simplicity of what the row-security code its self is doing,
 I'm wondering if this is a case that isn't handled in updatable s.b.
 views. I'll look into it.
 
 I'm not sure how much further you've got with this, but I think the
 issue is that the securityQuals that you're adding don't refer to the
 correct RTE. When adding securityQuals to an RTE, they are expected to
 have Vars whose varno matches the rt_index of the RTE (see for example
 the code in rewriteTargetView() which calls ChangeVarNodes() on
 viewqual before adding the qual to securityQuals or the main query
 jointree). prepend_row_security_quals() doesn't appear to have any
 similar code, and it would need to be passed the rt_index to do that.

Thanks for the pointer. That was indeed the issue.

I've pushed an update to the branch with the fix for varno handling.
Thanks. It's tagged rls-9.4-upd-sb-views-v8 .

I've almost run out of time to spend on row security for this
commitfest, unfortunately. I'm putting a blog together with a current
status update. Frustrating, as it's coming together now.

Open issues include:

- Passing plan inval items from rewriter into planner
- COPY support pending
- Clear syntax in DDL

Most of the rest are solved; it's actually looking pretty good.

-- 
 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] Row-security on updatable s.b. views

[Dean Rasheed]

On 13 February 2014 04:12, Craig Ringer cr...@2ndquadrant.com wrote:
 On 02/11/2014 08:19 PM, Yeb Havinga wrote:

 I compared output of psql -ef of the minirim.sql script posted earlier
 in http://www.postgresql.org/message-id/52f54927.1040...@gmail.com
 between v4 and v7.

 Not everything is ok.

 +psql:/home/m/minirim2.sql:409: ERROR:  attribute 6 has wrong type
 +DETAIL:  Table has type tsrange, but query expects _confidentialitycode.

 This looks like an issue with attribute transformations made when
 applying security barrier quals.

 +psql:/home/m/minirim2.sql:439: connection to server was lost

 That's dying with:

 Program received signal SIGABRT, Aborted.
 0x003f02c359e9 in __GI_raise (sig=sig@entry=6) at 
 ../nptl/sysdeps/unix/sysv/linux/raise.c:56
 56return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
 (gdb) bt
 #0  0x003f02c359e9 in __GI_raise (sig=sig@entry=6) at 
 ../nptl/sysdeps/unix/sysv/linux/raise.c:56
 #1  0x003f02c370f8 in __GI_abort () at abort.c:90
 #2  0x00758d9d in ExceptionalCondition 
 (conditionName=conditionName@entry=0x8b7bf0 !(!bms_is_empty(upper_varnos)),
 errorType=errorType@entry=0x78e89c FailedAssertion, 
 fileName=fileName@entry=0x8b78d7 subselect.c, 
 lineNumber=lineNumber@entry=1394) at assert.c:54
 #3  0x0061de2b in convert_EXISTS_sublink_to_join (root=optimized 
 out, sublink=optimized out, under_not=under_not@entry=0 '\000', 
 available_rels=0x117d038)
 at subselect.c:1394
 #4  0x0061efa9 in pull_up_sublinks_qual_recurse 
 (root=root@entry=0x117d058, node=0x117a0f8, 
 jtlink1=jtlink1@entry=0x7fff6d97f708,
 available_rels1=available_rels1@entry=0x117d038, 
 jtlink2=jtlink2@entry=0x0, available_rels2=available_rels2@entry=0x0) at 
 prepjointree.c:391
 #5  0x0061f339 in pull_up_sublinks_jointree_recurse 
 (root=root@entry=0x117d058, jtnode=0x117a040, 
 relids=relids@entry=0x7fff6d97f758) at prepjointree.c:208
 #6  0x0062066f in pull_up_sublinks (root=root@entry=0x117d058) at 
 prepjointree.c:147
 #7  0x0061967e in subquery_planner (glob=0x10c3fb8, 
 parse=parse@entry=0x1179af8, parent_root=parent_root@entry=0x1182fd0, 
 hasRecursion=hasRecursion@entry=0 '\000',
 `
 #8  0x005fc093 in set_subquery_pathlist (root=root@entry=0x1182fd0, 
 rel=rel@entry=0x1179370, rti=rti@entry=4, rte=rte@entry=0x1183980) at 
 allpaths.c:1209
 #9  0x005fc54e in set_rel_size (root=root@entry=0x1182fd0, 
 rel=0x1179370, rti=rti@entry=4, rte=0x1183980) at allpaths.c:265
 #10 0x005fc62e in set_base_rel_sizes (root=root@entry=0x1182fd0) at 
 allpaths.c:180
 #11 0x005fd584 in make_one_rel (root=root@entry=0x1182fd0, 
 joinlist=joinlist@entry=0x1179560) at allpaths.c:138
 #12 0x0061617a in query_planner (root=root@entry=0x1182fd0, 
 tlist=tlist@entry=0x11771c8, qp_callback=qp_callback@entry=0x616fd6 
 standard_qp_callback,
 qp_extra=qp_extra@entry=0x7fff6d97fa00) at planmain.c:236
 #13 0x006182f2 in grouping_planner (root=root@entry=0x1182fd0, 
 tuple_fraction=tuple_fraction@entry=0) at planner.c:1280
 #14 0x00619a11 in subquery_planner (glob=glob@entry=0x10c3fb8, 
 parse=parse@entry=0x10c3d30, parent_root=parent_root@entry=0x0,
 hasRecursion=hasRecursion@entry=0 '\000', tuple_fraction=0, 
 subroot=subroot@entry=0x7fff6d97fb58) at planner.c:574
 #15 0x00619c1f in standard_planner (parse=0x10c3d30, 
 cursorOptions=0, boundParams=0x0) at planner.c:211
 #16 0x00619e35 in planner (parse=parse@entry=0x10c3d30, 
 cursorOptions=cursorOptions@entry=0, boundParams=boundParams@entry=0x0) at 
 planner.c:139
 #17 0x0068c64b in pg_plan_query (querytree=0x10c3d30, 
 cursorOptions=cursorOptions@entry=0, boundParams=boundParams@entry=0x0) at 
 postgres.c:759
 #18 0x0068c6d3 in pg_plan_queries (querytrees=optimized out, 
 cursorOptions=cursorOptions@entry=0, boundParams=boundParams@entry=0x0) at 
 postgres.c:818
 #19 0x0068c932 in exec_simple_query 
 (query_string=query_string@entry=0x10c2d78 SELECT * FROM hl7.test;) at 
 postgres.c:983
 #20 0x0068e46e in PostgresMain (argc=optimized out, 
 argv=argv@entry=0x10cd1f0, dbname=0x10cd058 regress, username=optimized 
 out) at postgres.c:4003
 #21 0x006419a7 in BackendRun (port=port@entry=0x10c7e50) at 
 postmaster.c:4080
 #22 0x006432c2 in BackendStartup (port=port@entry=0x10c7e50) at 
 postmaster.c:3769
 #23 0x00643526 in ServerLoop () at postmaster.c:1580
 #24 0x0064467c in PostmasterMain (argc=argc@entry=4, 
 argv=argv@entry=0x10a8750) at postmaster.c:1235
 #25 0x005d692c in main (argc=4, argv=0x10a8750) at main.c:205
 (gdb)


 It's crashing while pulling up the query over emp (hl7.employee) and
 part (hl7.participation).

 Given the simplicity of what the row-security code its self is doing,
 I'm wondering if this is a case that isn't handled in updatable s.b.
 views. I'll look into it.


I'm not sure how much further you've got 

Re: [HACKERS] Row-security on updatable s.b. views

[Craig Ringer]

On 02/11/2014 08:19 PM, Yeb Havinga wrote:

 I compared output of psql -ef of the minirim.sql script posted earlier
 in http://www.postgresql.org/message-id/52f54927.1040...@gmail.com
 between v4 and v7.
 
 Not everything is ok.

 +psql:/home/m/minirim2.sql:409: ERROR:  attribute 6 has wrong type
 +DETAIL:  Table has type tsrange, but query expects _confidentialitycode.

This looks like an issue with attribute transformations made when
applying security barrier quals.

 +psql:/home/m/minirim2.sql:439: connection to server was lost

That's dying with:

 Program received signal SIGABRT, Aborted.
 0x003f02c359e9 in __GI_raise (sig=sig@entry=6) at 
 ../nptl/sysdeps/unix/sysv/linux/raise.c:56
 56return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
 (gdb) bt
 #0  0x003f02c359e9 in __GI_raise (sig=sig@entry=6) at 
 ../nptl/sysdeps/unix/sysv/linux/raise.c:56
 #1  0x003f02c370f8 in __GI_abort () at abort.c:90
 #2  0x00758d9d in ExceptionalCondition 
 (conditionName=conditionName@entry=0x8b7bf0 !(!bms_is_empty(upper_varnos)), 
 errorType=errorType@entry=0x78e89c FailedAssertion, 
 fileName=fileName@entry=0x8b78d7 subselect.c, 
 lineNumber=lineNumber@entry=1394) at assert.c:54
 #3  0x0061de2b in convert_EXISTS_sublink_to_join (root=optimized 
 out, sublink=optimized out, under_not=under_not@entry=0 '\000', 
 available_rels=0x117d038)
 at subselect.c:1394
 #4  0x0061efa9 in pull_up_sublinks_qual_recurse 
 (root=root@entry=0x117d058, node=0x117a0f8, 
 jtlink1=jtlink1@entry=0x7fff6d97f708, 
 available_rels1=available_rels1@entry=0x117d038, 
 jtlink2=jtlink2@entry=0x0, available_rels2=available_rels2@entry=0x0) at 
 prepjointree.c:391
 #5  0x0061f339 in pull_up_sublinks_jointree_recurse 
 (root=root@entry=0x117d058, jtnode=0x117a040, 
 relids=relids@entry=0x7fff6d97f758) at prepjointree.c:208
 #6  0x0062066f in pull_up_sublinks (root=root@entry=0x117d058) at 
 prepjointree.c:147
 #7  0x0061967e in subquery_planner (glob=0x10c3fb8, 
 parse=parse@entry=0x1179af8, parent_root=parent_root@entry=0x1182fd0, 
 hasRecursion=hasRecursion@entry=0 '\000', 
`
 #8  0x005fc093 in set_subquery_pathlist (root=root@entry=0x1182fd0, 
 rel=rel@entry=0x1179370, rti=rti@entry=4, rte=rte@entry=0x1183980) at 
 allpaths.c:1209
 #9  0x005fc54e in set_rel_size (root=root@entry=0x1182fd0, 
 rel=0x1179370, rti=rti@entry=4, rte=0x1183980) at allpaths.c:265
 #10 0x005fc62e in set_base_rel_sizes (root=root@entry=0x1182fd0) at 
 allpaths.c:180
 #11 0x005fd584 in make_one_rel (root=root@entry=0x1182fd0, 
 joinlist=joinlist@entry=0x1179560) at allpaths.c:138
 #12 0x0061617a in query_planner (root=root@entry=0x1182fd0, 
 tlist=tlist@entry=0x11771c8, qp_callback=qp_callback@entry=0x616fd6 
 standard_qp_callback, 
 qp_extra=qp_extra@entry=0x7fff6d97fa00) at planmain.c:236
 #13 0x006182f2 in grouping_planner (root=root@entry=0x1182fd0, 
 tuple_fraction=tuple_fraction@entry=0) at planner.c:1280
 #14 0x00619a11 in subquery_planner (glob=glob@entry=0x10c3fb8, 
 parse=parse@entry=0x10c3d30, parent_root=parent_root@entry=0x0, 
 hasRecursion=hasRecursion@entry=0 '\000', tuple_fraction=0, 
 subroot=subroot@entry=0x7fff6d97fb58) at planner.c:574
 #15 0x00619c1f in standard_planner (parse=0x10c3d30, cursorOptions=0, 
 boundParams=0x0) at planner.c:211
 #16 0x00619e35 in planner (parse=parse@entry=0x10c3d30, 
 cursorOptions=cursorOptions@entry=0, boundParams=boundParams@entry=0x0) at 
 planner.c:139
 #17 0x0068c64b in pg_plan_query (querytree=0x10c3d30, 
 cursorOptions=cursorOptions@entry=0, boundParams=boundParams@entry=0x0) at 
 postgres.c:759
 #18 0x0068c6d3 in pg_plan_queries (querytrees=optimized out, 
 cursorOptions=cursorOptions@entry=0, boundParams=boundParams@entry=0x0) at 
 postgres.c:818
 #19 0x0068c932 in exec_simple_query 
 (query_string=query_string@entry=0x10c2d78 SELECT * FROM hl7.test;) at 
 postgres.c:983
 #20 0x0068e46e in PostgresMain (argc=optimized out, 
 argv=argv@entry=0x10cd1f0, dbname=0x10cd058 regress, username=optimized 
 out) at postgres.c:4003
 #21 0x006419a7 in BackendRun (port=port@entry=0x10c7e50) at 
 postmaster.c:4080
 #22 0x006432c2 in BackendStartup (port=port@entry=0x10c7e50) at 
 postmaster.c:3769
 #23 0x00643526 in ServerLoop () at postmaster.c:1580
 #24 0x0064467c in PostmasterMain (argc=argc@entry=4, 
 argv=argv@entry=0x10a8750) at postmaster.c:1235
 #25 0x005d692c in main (argc=4, argv=0x10a8750) at main.c:205
 (gdb) 


It's crashing while pulling up the query over emp (hl7.employee) and
part (hl7.participation).

Given the simplicity of what the row-security code its self is doing,
I'm wondering if this is a case that isn't handled in updatable s.b.
views. I'll look into it.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  

Re: [HACKERS] Row-security on updatable s.b. views

[Craig Ringer]

On 02/06/2014 10:19 PM, Craig Ringer wrote:
 On 02/06/2014 12:43 PM, Craig Ringer wrote:
 1. Try (again) to do row-security in the rewriter. This was previously
 impossible because of the definition of row-security behaviour around
 inheritance, but with the simplified inheritance model now proposed I
 think it's possible.
 
 Thanks to the simplified requirements for inheritance, this turns out to
 be fairly easy. There's a version rewritten to use the rewriter in the tag:
 
rls-9.4-upd-sb-views-v6
 
 on https://github.com/ringerc/postgres.git

... which was totally wrong, and I blame lack of sleep for it ever
getting pushed. I didn't understand the rewriter as well as I thought.

v7 applies row-security quals in fireRIRrules .

It handles recursion correctly now, and works fine for both target and
non-target relations. I've cleaned out most of the cruft from the
previous optimizer based approach too.

I haven't figured out how to pass the plan invalidation information (so
plans are invalidated properly when they depend on row security quals)
down into the planner yet, that's next.

COPY still just ERROR's if you try to copy to/from a rel with
row-security quals, but again, just a matter of getting it done, I have
KaiGai's patch to work from.

Regression tests fail, including a segfault in the executor. Cause as
yet unknown, but it takes a hairy view+rowsecrule combo to trigger it.


New tag:

rls-9.4-upd-sb-views-v6

 The regression test expected file needs adjustment to match the new
 inheritance rules, and to cover a couple of new tests I've added.

Still true.

 Need to cherry-pick the docs patch back on top of the patch tree now
 things are settling.

Still true.


-- 
 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] Row-security on updatable s.b. views

[Yeb Havinga]

On 2014-02-11 09:36, Craig Ringer wrote:

On 02/06/2014 10:19 PM, Craig Ringer wrote:

On 02/06/2014 12:43 PM, Craig Ringer wrote:

1. Try (again) to do row-security in the rewriter. This was previously
impossible because of the definition of row-security behaviour around
inheritance, but with the simplified inheritance model now proposed I
think it's possible.

Thanks to the simplified requirements for inheritance, this turns out to
be fairly easy. There's a version rewritten to use the rewriter in the tag:

rls-9.4-upd-sb-views-v6

on https://github.com/ringerc/postgres.git

... which was totally wrong, and I blame lack of sleep for it ever
getting pushed. I didn't understand the rewriter as well as I thought.

v7 applies row-security quals in fireRIRrules .



New tag:

rls-9.4-upd-sb-views-v6


Hi Craig,

This looks to be the same v6 version as the initial rewriter version.
https://github.com/ringerc/postgres/commits/rls-9.4-upd-sb-views-v6

regards,
Yeb



--
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] Row-security on updatable s.b. views

[Craig Ringer]

On 02/11/2014 06:05 PM, Yeb Havinga wrote:
 On 2014-02-11 09:36, Craig Ringer wrote:
 On 02/06/2014 10:19 PM, Craig Ringer wrote:
 On 02/06/2014 12:43 PM, Craig Ringer wrote:
 1. Try (again) to do row-security in the rewriter. This was previously
 impossible because of the definition of row-security behaviour around
 inheritance, but with the simplified inheritance model now proposed I
 think it's possible.
 Thanks to the simplified requirements for inheritance, this turns out to
 be fairly easy. There's a version rewritten to use the rewriter in
 the tag:

 rls-9.4-upd-sb-views-v6

 on https://github.com/ringerc/postgres.git
 ... which was totally wrong, and I blame lack of sleep for it ever
 getting pushed. I didn't understand the rewriter as well as I thought.

 v7 applies row-security quals in fireRIRrules .
 
 New tag:

 rls-9.4-upd-sb-views-v6
 
 Hi Craig,
 
 This looks to be the same v6 version as the initial rewriter version.
 https://github.com/ringerc/postgres/commits/rls-9.4-upd-sb-views-v6

Whoops, wrong paste.

rls-9.4-upd-sb-views-v7

-- 
 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] Row-security on updatable s.b. views

[Yeb Havinga]

On 2014-02-11 12:09, Craig Ringer wrote:

On 02/11/2014 06:05 PM, Yeb Havinga wrote:

On 2014-02-11 09:36, Craig Ringer wrote:

On 02/06/2014 10:19 PM, Craig Ringer wrote:

On 02/06/2014 12:43 PM, Craig Ringer wrote:

1. Try (again) to do row-security in the rewriter. This was previously
impossible because of the definition of row-security behaviour around
inheritance, but with the simplified inheritance model now proposed I
think it's possible.

Thanks to the simplified requirements for inheritance, this turns out to
be fairly easy. There's a version rewritten to use the rewriter in
the tag:

 rls-9.4-upd-sb-views-v6

on https://github.com/ringerc/postgres.git

... which was totally wrong, and I blame lack of sleep for it ever
getting pushed. I didn't understand the rewriter as well as I thought.

v7 applies row-security quals in fireRIRrules .
New tag:

rls-9.4-upd-sb-views-v6

Hi Craig,

This looks to be the same v6 version as the initial rewriter version.
https://github.com/ringerc/postgres/commits/rls-9.4-upd-sb-views-v6

Whoops, wrong paste.

rls-9.4-upd-sb-views-v7


Hi Craig,

I compared output of psql -ef of the minirim.sql script posted earlier 
in http://www.postgresql.org/message-id/52f54927.1040...@gmail.com 
between v4 and v7.


Not everything is ok.

  Seq Scan on patient  (cost=0.00..29589.31 rows=495 width=52)
Filter: (SubPlan 1)
SubPlan 1
@@ -555,7 +592,7 @@
  -  Materialize  (cost=26.39..570.62 rows=1014 width=4)
-  Subquery Scan on act (cost=26.39..565.55 
rows=1014 width=4)
  -  Nested Loop Semi Join 
(cost=26.39..555.41 rows=1014 width=108)
-   Join Filter: (((part.act = act_1.id) 
AND (emp_2.pgname = (current_user())::text)) OR (NOT 
((act_1.confidentialitycode)::text[] @ '{s}'::text[])))
+   Join Filter: (((part.act = act_1.id) 
AND (emp_2.pgname = (current_user())::text)) OR (NOT 
((act_1.effectivetime)::text[] @ '{s}'::text[])))
-  Append  (cost=0.00..31.19 
rows=1019 width=108)
  -  Seq Scan on act act_1  
(cost=0.00..1.59 rows=59 width=108)


@@ -587,12 +624,8 @@
 FROM patient, person, organization
 WHERE patient.player = person.id
 AND patient.scoper = organization.id;
- id | vipcode |   name   |  birthtime  | name
-+-+--+-+
- 10 | | John Doe | 1963-04-01 00:00:00 | Community Health and 
Hospitals
- 16 | | John Doe | 1963-04-01 00:00:00 | Community Mental 
Health Clinic

-(2 rows)
-
+psql:/home/m/minirim2.sql:409: ERROR:  attribute 6 has wrong type
+DETAIL:  Table has type tsrange, but query expects _confidentialitycode.


@@ -629,7 +662,4 @@
 SET SESSION AUTHORIZATION sigmund;
 SET
 SELECT * FROM test;
- id | classcode | moodcode | code | confidentialitycode | effectivetime
-+---+--+--+-+---
-(0 rows)
-
+psql:/home/m/minirim2.sql:439: connection to server was lost


regards,
Yeb Havinga



--
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] Row-security on updatable s.b. views

[Craig Ringer]

On 02/11/2014 08:19 PM, Yeb Havinga wrote:
 On 2014-02-11 12:09, Craig Ringer wrote:

 rls-9.4-upd-sb-views-v7

 Hi Craig,
 
 I compared output of psql -ef of the minirim.sql script posted earlier
 in http://www.postgresql.org/message-id/52f54927.1040...@gmail.com
 between v4 and v7.
 
 Not everything is ok.

 +psql:/home/m/minirim2.sql:409: ERROR:  attribute 6 has wrong type
 +DETAIL:  Table has type tsrange, but query expects _confidentialitycode.

That's downright strange. I'll need to look into that one.

 +psql:/home/m/minirim2.sql:439: connection to server was lost

I've seen a server crash for causes as yet unknown in the main
regression tests too.

I'd love to stick with the in-optimizer approach used in v4, which -
after all - works. The trouble is that it cannot support row-security
quals that incorporate views correctly. I would have to invoke the
rewriter from the optimizer and deal with recursion detection to make
that work, and it looks pretty ugly.

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

Review of RLS on inheritance schema HL7 RIM (was Re: [HACKERS] Row-security on updatable s.b. views)

[Yeb Havinga]

On 06/02/14 15:19, Craig Ringer wrote:


Thanks to the simplified requirements for inheritance, this turns out to
be fairly easy. There's a version rewritten to use the rewriter in the tag:

rls-9.4-upd-sb-views-v6

on https://github.com/ringerc/postgres.git


Hi Craig, list,

This is review of the current RLS patch on a database schema that uses 
inheritance in the 'classical' sense (not partitioning). The review was 
done on rls-9.4-upd-sb-views-v4 and hence all comments are about that 
version. Comparing output of the minisql script between v4 and v6 gives 
differences, as v6 seems to be WIP.


Our goal is to implement the HL7 Reference Information Model (RIM) in 
PostgreSQL. A fine-grained access control on the tables would have a 
practical use in the context of RIM. So, we have made some preliminary 
tests of the Row Security patch for such a specific data model. For the 
purpose of reviewing RLS, we have restricted the full RIM to just a few 
tables which we call the mini-RIM. It is important to observe that the 
RIM uses inheritance, and we use PostgreSQL inheritance to implement the 
RIM's inheritance. More details about the RIM are presented below.


In the attached SQL script we list a mini-RIM, along with examples of 
RLS enforcement.


General comments about RLS applied on (a minimalistic version of) the 
RIM can be summarized as follows:


1. The current RLS implementation works for use cases where 
confidentiality attributes are specified in the inheritance root 
relation. Since security labelling in the RIM is done on 
confidentialitycodes that are present in the inheritance roots (e.g., 
Role and Act), the current RLS works for the RIM.


2. Infinite recursion is well captured in case of recursive restrictions 
to tables.


3. RLS syntax is readable and easy to use.

4. Documentation needs work.

5. Subqueries in RLS quals can be pulled up, so opens the ability for 
fast processing.



Overall from a users perspective the patch gave a solid impression.

regards,
Yeb Havinga
Albana Gaba
Henk-Jan Meijer

Portavita B.V. The Netherlands

BACKGROUND ON THE REFERENCE INFORMATION MODEL:

To understand how The HL7 Reference Information Model (RIM) uses 
PostgreSQL inheritance, it is helpful to understand the meaning of the 
content of the parent and child tables. This section describes the 
background of the RIM, and describes a few classes of the “Act” hierarchy.


The HL7 RIM[1] is not just yet another information model. It is a 
mature, standard information model that has been used and refined over 
the course of many years [2][3]. Its purpose is to capture detailed 
information for medical records. Pivotal in the RIM is its action-based 
modelling, based on ideas that can be traced back to the American 
philosopher C.S. Peirce. A direct line from Peirce’s Logic of Relatives 
to the foundations of relational databases has been introduced in [4].

Versions of the RIM are now being released as an ANSI standard.

An illustration of the RIM is available at 
http://www.healthintersections.com.au/wp-content/uploads/2011/05/RIM.png
The RIM is a set of UML classes, each containing one or more attributes. 
The classes are an abstraction of subjects or other concepts that are 
relevant within the healthcare domain. To avoid a model with a huge 
number of classes, the RIM defines six core classes whereas the vast 
majority of the classes are defined as specializations based on the core 
ones. The specialization classes inherit all the properties of the 
generalization classes while adding specific attributes of its own. To 
make matters concrete, let us look at Act class.


“Act”: Looking at the right hand side of the RIM illustration referenced 
above, we can see the class “Act” and its specializations, and this is 
the focal point for the RIM’s action based modeling. Description from 
the standard: “Acts are the pivot of the RIM: domain information and 
process records are represented primarily in Acts. Any profession or 
business, including healthcare, is primarily constituted of intentional 
and occasionally non-intentional actions, performed and recorded by 
responsible actors. An Act-instance is a record of such an action.” 
Notable attributes of “Act” are:


“id” - A unique identifier for the Act. Each Act is associated with a 
unique id. All specialization of Act inherit this id. This means that if 
there is, for example, an instance of Observation with id 5, there exist 
no other acts with id 5. In fact, since technically in the RIM all 
identifiers stem from a single infrastructure root, the identifiers are 
globally unique: there exists a single object with id 5. This single 
object is an instance of Observation, and since Observation is a 
specialization of Act, it is also an instance of Act.


“classCode” – The major class of Acts to which an Act-instance belongs. 
The allowed codes in classCode form a hierarchical code system. In the 
2011 RIM, there are 124 different class codes. This 

Re: [HACKERS] Row-security on updatable s.b. views

[Yeb Havinga]

On 2014-02-06 05:43, Craig Ringer wrote:

Based on Tom's objections, another approach is presented in
rls-9.4-upd-sb-views-v5 on  g...@github.com:ringerc/postgres.git . The
Query node is used to record the recursive expansion parent list
instead, and copying is avoided.


Cannot fetch or clone.

github on web says This repository is temporarily unavailable. The 
backend storage is temporarily offline. Usually this means the storage 
server is undergoing maintenance. Please contact support if the problem 
persists.


regards,
Yeb



--
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] Row-security on updatable s.b. views

[Craig Ringer]

On 02/06/2014 04:54 PM, Yeb Havinga wrote:
 On 2014-02-06 05:43, Craig Ringer wrote:
 Based on Tom's objections, another approach is presented in
 rls-9.4-upd-sb-views-v5 on  g...@github.com:ringerc/postgres.git . The
 Query node is used to record the recursive expansion parent list
 instead, and copying is avoided.
 
 Cannot fetch or clone.
 
 github on web says This repository is temporarily unavailable. The
 backend storage is temporarily offline. Usually this means the storage
 server is undergoing maintenance. Please contact support if the problem
 persists.

If this persists, or someone else has the same issue, try the HTTPS url:

https://github.com/ringerc/postgres.git

per: http://github.com/ringerc/postgres/

(I need to chase up my git.postgresql.org account request, it seems to
have got lost).


-- 
 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] Row-security on updatable s.b. views

[Craig Ringer]

On 02/06/2014 12:43 PM, Craig Ringer wrote:
 1. Try (again) to do row-security in the rewriter. This was previously
 impossible because of the definition of row-security behaviour around
 inheritance, but with the simplified inheritance model now proposed I
 think it's possible.

Thanks to the simplified requirements for inheritance, this turns out to
be fairly easy. There's a version rewritten to use the rewriter in the tag:

   rls-9.4-upd-sb-views-v6

on https://github.com/ringerc/postgres.git

The trickiest bit remaining is how to register the PlanInvalItem to
force plan invalidation when the user-id changes. This was easy in the
optimizer, but it's not obvious how to do it cleanly in the rewriter.
I've got a couple of ideas but don't much like either of them.
Recommendations from the experienced welcomed.

Other more minor open items, each of which look quite quick:

I haven't plugged in rewriter-based recursion detection yet, but that
looks fairly easy (famous last words?). It's 10pm and I have an early
start, so that won't happen tonight.

I haven't removed some cruft from the previous approach from the Query
node either; I need to trim the diff.

The regression test expected file needs adjustment to match the new
inheritance rules, and to cover a couple of new tests I've added.

Needs a better error when it gets an unacceptable rewrite product during
security qual rewriting (modeled on the errors for CTEs). Easy, just
some cookie cutter code.

Need to cherry-pick the docs patch back on top of the patch tree now
things are settling.



Amazingly, I think this has a hope.

-- 
 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] Row-security on updatable s.b. views

[Craig Ringer]

On 02/06/2014 10:19 PM, Craig Ringer wrote:
 On 02/06/2014 12:43 PM, Craig Ringer wrote:
 1. Try (again) to do row-security in the rewriter. This was previously
 impossible because of the definition of row-security behaviour around
 inheritance, but with the simplified inheritance model now proposed I
 think it's possible.
 
 Thanks to the simplified requirements for inheritance, this turns out to
 be fairly easy. There's a version rewritten to use the rewriter in the tag:
 
rls-9.4-upd-sb-views-v6
 
 on https://github.com/ringerc/postgres.git
 
 The trickiest bit remaining is how to register the PlanInvalItem to
 force plan invalidation when the user-id changes. This was easy in the
 optimizer, but it's not obvious how to do it cleanly in the rewriter.
 I've got a couple of ideas but don't much like either of them.
 Recommendations from the experienced welcomed.

Or, after thinking about it for a second with my tired brain, not so much.

We don't rerun rewrite on plan invalidation.

So that means the superuser exemption won't work properly with this patch.

So much for having a hope, that's not a small thing to fix.

So: either I invoke the rewriter from within the optimizer on the
security quals, or I make the rewriter re-run on plan invalidation.
Neither is small or simple.

Blast.

-- 
 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] Row-security on updatable s.b. views

[Tom Lane]

Craig Ringer cr...@2ndquadrant.com writes:
 We don't rerun rewrite on plan invalidation.

Don't we?  plancache.c certainly does, in fact it starts from the raw
grammar output.  Skipping the rewriter would mean failing to respond
to CREATE OR REPLACE VIEW, for example.

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] Row-security on updatable s.b. views

[Craig Ringer]

On 02/06/2014 11:11 PM, Tom Lane wrote:
 Craig Ringer cr...@2ndquadrant.com writes:
 We don't rerun rewrite on plan invalidation.
 
 Don't we?  plancache.c certainly does, in fact it starts from the raw
 grammar output.  Skipping the rewriter would mean failing to respond
 to CREATE OR REPLACE VIEW, for example.

I was thinking about exactly that case as I went to sleep - especially
as it's things like CREATE OR REPLACE VIEW that prevent me from just
rewriting the security qual when it's initially added, before storage in
the catalogs.

I could've sworn discussion around row security in the past concluded
that it couldn't be properly done in the rewriter because of an
inability to correctly invalidate plans. Searching the archives, though,
I don't find anything to support that. I'll just say I'm glad to be
wrong, and proceed from there.

-- 
 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] Row-security on updatable s.b. views

[Craig Ringer]

On 02/04/2014 02:43 PM, Craig Ringer wrote:
 On 01/30/2014 04:05 PM, Craig Ringer wrote:
 On 01/30/2014 01:25 PM, Craig Ringer wrote:
 On 01/29/2014 09:47 PM, Craig Ringer wrote:
 https://github.com/ringerc/postgres/compare/rls-9.4-upd-sb-views

 i.e. https://github.com/ringerc/postgres.git ,
  branch rls-9.4-upd-sb-views

 (subject to rebasing) or the non-rebased tag rls-9.4-upd-sb-views-v2

 Pushed an update to the branch. New update tagged
 rls-9.4-upd-sb-views-v3 . Fixes an issue with rowmarking that stems from
 the underlying updatable s.b. views patch.

 Other tests continue to fail, this isn't ready yet.

 Specifically:
 
 - row-security rule recursion detection isn't solved yet, it just
 overflows the stack.
 
 This is now fixed in the newly tagged rls-9.4-upd-sb-views-v4 in
 g...@github.com:ringerc/postgres.git .

Based on Tom's objections, another approach is presented in
rls-9.4-upd-sb-views-v5 on  g...@github.com:ringerc/postgres.git . The
Query node is used to record the recursive expansion parent list
instead, and copying is avoided.

However, I've separately tracked down the cause of the test failures like:

ERROR:  could not open file base/16384/30135: No such file or directory

This occurs where a row-security qual is declared to use a view.
Row-security quals get stored without rewriting (which is necessary, see
below). The qual is injected into securityQuals and expanded, but *not
rewritten*. So the executor sees an unexpected view in the tree.

Because a view RTE has its relid field set to the view's oid, this
doesn't get picked up until we try to actually scan the view relation in
the executor.

(I'd like to add Asserts to make the executor fail a bit more
informatively when you try to scan a view, but that's separate.)


So, that's clearly broken. There are really two possible solutions:

1. Try (again) to do row-security in the rewriter. This was previously
impossible because of the definition of row-security behaviour around
inheritance, but with the simplified inheritance model now proposed I
think it's possible.

2. Invoke RewriteQuery from within expand_security_quals, rewriting the
query after security qual expansion. This is only needed for
row-security; for updatable s.b. views rewriting has happened with
recursion into securityQuals during the original rewrite pass.


I suspect that (2) will result in a resounding yuck.

So I have to see if I can now turn around *again* and plug row-security
into the rewriter after all.  That's a pretty frustrating thing to
discover in mid-late CF4.

-- 
 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] Row-security on updatable s.b. views

[Craig Ringer]

On 01/30/2014 04:05 PM, Craig Ringer wrote:
 On 01/30/2014 01:25 PM, Craig Ringer wrote:
 On 01/29/2014 09:47 PM, Craig Ringer wrote:
 https://github.com/ringerc/postgres/compare/rls-9.4-upd-sb-views

 i.e. https://github.com/ringerc/postgres.git ,
  branch rls-9.4-upd-sb-views

 (subject to rebasing) or the non-rebased tag rls-9.4-upd-sb-views-v2

 Pushed an update to the branch. New update tagged
 rls-9.4-upd-sb-views-v3 . Fixes an issue with rowmarking that stems from
 the underlying updatable s.b. views patch.

 Other tests continue to fail, this isn't ready yet.
 
 Specifically:

 - row-security rule recursion detection isn't solved yet, it just
 overflows the stack.

This is now fixed in the newly tagged rls-9.4-upd-sb-views-v4 in
g...@github.com:ringerc/postgres.git .

I landed up adding a field to RangeTblEntry that keeps track of all the
oids of relations row-security expanded to produce this RTE. When
testing an RTE for row-security policy, this list is checked to see if
the oid of the relation being expanded is already on the list. The list
is copied to all RTEs inside sublinks after a relation is expanded using
a query_tree_walker.

 - COPY doesn't know anything about row-security

COPY will ERROR on row-security rels in rls-9.4-upd-sb-views-v4.

I'm looking at integrating the approach Kohei KaiGai took in the
original patch now, then I'll be moving on to:

 - I'm just starting to chase some odd errors in the tests, ERROR:
 failed to find unique expression in subplan tlist and ERROR:  could
 not open file base/16384/30070: No such file or directory. Their
 cause/origin is not yet known, but they're specific to when row-security
 policy is being applied.

I was hoping these would be fixed by solving the recursion issues, but
that wasn not the case.

Input/comments would be appreciated. I haven't looked into this yet.

 - policies based on current_user don't remember current_user when rows
 are pulled from refcursor returned by a security definer function.

This is actually a separate, existing bug, or surprising behaviour. I'd
like to address it, but it's really a separate patch.


-- 
 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] Row-security on updatable s.b. views

[Tom Lane]

Craig Ringer cr...@2ndquadrant.com writes:
 I landed up adding a field to RangeTblEntry that keeps track of all the
 oids of relations row-security expanded to produce this RTE. When
 testing an RTE for row-security policy, this list is checked to see if
 the oid of the relation being expanded is already on the list. The list
 is copied to all RTEs inside sublinks after a relation is expanded using
 a query_tree_walker.

That's impossibly ugly, both as to the idea that an RTE is a mutable
data structure for this processing, and as to having to copy the list
around (meaning that you can't even claim to have one source of truth
for the state...)

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] Row-security on updatable s.b. views

[Craig Ringer]

On 02/04/2014 03:14 PM, Tom Lane wrote:
 Craig Ringer cr...@2ndquadrant.com writes:
 I landed up adding a field to RangeTblEntry that keeps track of all the
 oids of relations row-security expanded to produce this RTE. When
 testing an RTE for row-security policy, this list is checked to see if
 the oid of the relation being expanded is already on the list. The list
 is copied to all RTEs inside sublinks after a relation is expanded using
 a query_tree_walker.
 
 That's impossibly ugly, both as to the idea that an RTE is a mutable
 data structure for this processing, and as to having to copy the list
 around (meaning that you can't even claim to have one source of truth
 for the state...)

I see. Do you have any suggestion about how this might be approached in
a manner that would be more satisfactory?

It's not strictly necessary to duplicate the parent list when annotating
the RTEs in an expanded row-security qual; I did so only out of concern
that an object of indeterminate/shared ownership may not be considered
desirable.

The only time it's strictly neccessary to copy the parent list is when a
new row-security qual is being expanded. In this case it cannot be
shared, because there may be multiple relations with row-security
applied, and a separate path is required for each. If the list is
shared, infinite recursion is falsely reported in cases like:

rel a has a qual referring to b
rel b has a qual referring to c
rel c has a qual referring to d

select ...
from a inner join b on ...

There's no infinite recursion there, but a simple approach that keeps a
global list of expanded quals will think there is as it'll see b and
c expanded twice.

So whenever you expand a qual, you have to fork the parent list,
copying it.

The approach used in the rewriter won't work here, because the rewriter
eagerly depth-first recurses when expanding things. In the optimizer,
all securityQuals get expanded in one pass, _then_ sublink processing
expands any added sublinks in a separate pass.

It's possible the parent list could be associated with the Query that's
produced by securityQual expansion instead, but at the time you're
expanding row-security on any RTEs within that you don't necessarily
have access to the Query node that might be a couple of subquery levels
up, since the security qual can be an arbitrary expression.

I looked at keeping a structure in PlannerGlobal that tracked the chain
of row-security qual expansion, but I couldn't figure out a way to
cleanly tell what branch of the query a given RTE that's being
expanded was in, and thus how to walk the global tree to check for
infinite recursion.



The only alternative I see is to immediately and recursively expand
row-security entries within subqueries using a walker, as soon as the
initial row-security qual is expanded on the top level rel.

I'm concerned about code duplication when following that path, since
that's pretty much what's already happening with sublink expansion, just
using the existing execution paths.

If I'm right, and immediate recursive expansion isn't an acceptable
alternative, any ideas on how the row-security expansion breadcrumb
trail might be stored in a more appropriate manner and accessed from
where they're needed?

Note that it's trivial to prevent simple, direct recursion. Preventing
mutual recursion without also breaking non-recursive cases where rels
are used in totally different parts of the query is harder.

-- 
 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] Row-security on updatable s.b. views

[Craig Ringer]

On 01/30/2014 01:25 PM, Craig Ringer wrote:
 On 01/29/2014 09:47 PM, Craig Ringer wrote:
 https://github.com/ringerc/postgres/compare/rls-9.4-upd-sb-views

 i.e. https://github.com/ringerc/postgres.git ,
  branch rls-9.4-upd-sb-views

 (subject to rebasing) or the non-rebased tag rls-9.4-upd-sb-views-v2
 
 Pushed an update to the branch. New update tagged
 rls-9.4-upd-sb-views-v3 . Fixes an issue with rowmarking that stems from
 the underlying updatable s.b. views patch.
 
 Other tests continue to fail, this isn't ready yet.

Specifically:

- Needs checks in AT INHERITS, AT SET ROW SECURITY, and CT INHERITS to
prohibit any combination of inheritance and row-security, per:

  http://www.postgresql.org/message-id/52ea01c3.70...@2ndquadrant.com

- row-security rule recursion detection isn't solved yet, it just
overflows the stack.

- COPY doesn't know anything about row-security

- I'm just starting to chase some odd errors in the tests, ERROR:
failed to find unique expression in subplan tlist and ERROR:  could
not open file base/16384/30070: No such file or directory. Their
cause/origin is not yet known, but they're specific to when row-security
policy is being applied.

- policies based on current_user don't remember current_user when rows
are pulled from refcursor returned by a security definer function.


There is a chunk of work here. Anybody who wants row-security to happen
for 9.4, please pick something and pitch in.


(Or we could just decide that my rebased and tweaked version of KaiGai's
original patch internal query structure twiddling aside, is the best way
forward after all. That leaves only the last item to deal with.)

-- 
 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] Row-security on updatable s.b. views

[Craig Ringer]

On 01/29/2014 09:47 PM, Craig Ringer wrote:
 https://github.com/ringerc/postgres/compare/rls-9.4-upd-sb-views
 
 i.e. https://github.com/ringerc/postgres.git ,
  branch rls-9.4-upd-sb-views
 
 (subject to rebasing) or the non-rebased tag rls-9.4-upd-sb-views-v2

Pushed an update to the branch. New update tagged
rls-9.4-upd-sb-views-v3 . Fixes an issue with rowmarking that stems from
the underlying updatable s.b. views patch.

Other tests continue to fail, this isn't ready 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