Re: [v9.3] Row-Level Security

The attached patch provides bare row-level security feature.

Table's owner can set its own security policy using the following syntax:
ALTER TABLE <table> SET ROW LEVEL SECURITY (<condition>);
ALTER TABLE <table> RESET ROW LEVEL SECURITY;

The condition must be an expression that returns a boolean value
and can reference contents of each rows. (Right now, it does not
support to contain SubLink in the expression; to be improved)

In the previous discussion, we planned to add a syntax option to
clarify the command type to fire the RLS policy, such as FOR UPDATE.
But current my opinion is, it is not necessary. For example, we can
reference the contents of rows being updated / deleted using
RETURNING clause. So, it does not make sense to have different
RLS policy at UPDATE / DELETE from SELECT.

If and when user's query (SELECT, UPDATE or DELETE, not INSERT)
references the relation with RLS policy, only rows that satisfies the
supplied condition are available to access.
It performs as if the configured condition was implicitly added to
the WHERE clause, however, this mechanism tries to replace
references to the table with RLS policy by a simple sub-query
that scans the target table with RLS policy, to ensure the policy
condition is evaluated earlier than any other user given qualifier.

EXPLAIN shows how RLS works.

postgres=# ALTER TABLE sample SET ROW LEVEL SECURITY (z > current_date - 10);
ALTER TABLE

postgres=# EXPLAIN SELECT * FROM sample WHERE f_leak(y);
QUERY PLAN
------------------------------------------------------------------------------------
Subquery Scan on sample (cost=0.00..42.54 rows=215 width=40)
Filter: f_leak(sample.y)
-> Seq Scan on sample (cost=0.00..36.10 rows=644 width=66)
Filter: ((z > (('now'::cstring)::date - 10)) OR
has_superuser_privilege())
(4 rows)

In above example, the security policy does not allow to reference
rows earlier than 10 days. Then, SELECT on the table was
expanded to a sub-query and configured expression was added
inside of the sub-query. Database superuser can bypass any
security checks, so "OR has_superuser_privilege()" was automatically
attached in addition to user configured expression.

On the other hand, I'm not 100% sure about my design to restrict
rows to be updated and deleted. Similarly, it expands the target
relation of UPDATE or DELETE statement into a sub-query with
condition. ExecModifyTable() pulls a tuple from the sub-query,
instead of regular table, so it seems to me working at least, but
I didn't try all the possible cases of course.

postgres=# EXPLAIN UPDATE sample SET y = y || '_updt' WHERE f_leak(y);
QUERY PLAN
------------------------------------------------------------------------------------------
Update on sample (cost=0.00..43.08 rows=215 width=46)
-> Subquery Scan on sample (cost=0.00..43.08 rows=215 width=46)
Filter: f_leak(sample.y)
-> Seq Scan on sample (cost=0.00..36.10 rows=644 width=66)
Filter: ((z > (('now'::cstring)::date - 10)) OR
has_superuser_privilege())
(5 rows)

I have two other ideas to implement writer side RLS.

The first idea modifies WHERE clause to satisfies RLS policy, but Robert
concerned about this idea in the previous discussion, because it takes
twice scans.
UPDATE sample SET y = y || '_updt' WHERE f_leak(y);
shall be modified to:
UPDATE sample SET y = y || '_updt' WHERE ctid = (
SELECT ctid FROM (
SELECT ctid, * FROM sample WHERE <RLS policy>
) AS pseudo_sample WHERE f_leak(y)
);
Although the outer scan is ctid scan, it takes seq-scan at first level.

The second idea tries to duplicate RangeTblEntry of the target relation
to be updated or deleted, then one perform as target relation as is, and
the other performs as data source to produce older version of tuples;
being replaced by a sub-query with RLS condition.
I didn't try the second idea yet. As long as we can patch the code that
assumes the target relation has same rtindex with source relation, it
might be safe approach. However, I'm not sure which is less invasive
approach compared to the current patch.

Of course, here is some limitations, to keep the patch size reasonable
level to review.
- The permission to bypass RLS policy was under discussion.
If and when we should provide a special permission to bypass RLS
policy, the "OR has_superuser_privilege()" shall be replaced by
"OR has_table_privilege(tableoid, 'RLSBYPASS')".
Right now, I allows only superuser to bypass RLS policy.
- This patch focuses on the bare feature only, not any enhancement
at query optimization feature, so RLS policy might prevent index-scan,
right now.
- RLS policy is not applied to the row to be inserted, or newer version
of row to be updated. It can be implemented using before-row trigger.
It might be an idea to inject RLS trigger function automatically, like
FK constraints, but not yet.
- As Florian pointed out, current_user may change during query
execution if DECLARE and FETCH are used.
Although it is not a matter in RLS itself, should be improved later.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Thu, Jun 14, 2012 at 11:43 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> In the previous discussion, we planned to add a syntax option to
> clarify the command type to fire the RLS policy, such as FOR UPDATE.
> But current my opinion is, it is not necessary. For example, we can
> reference the contents of rows being updated / deleted using
> RETURNING clause. So, it does not make sense to have different
> RLS policy at UPDATE / DELETE from SELECT.

I agree. That doesn't make sense, and we don't need to support it.

> On the other hand, I'm not 100% sure about my design to restrict
> rows to be updated and deleted. Similarly, it expands the target
> relation of UPDATE or DELETE statement into a sub-query with
> condition. ExecModifyTable() pulls a tuple from the sub-query,
> instead of regular table, so it seems to me working at least, but
> I didn't try all the possible cases of course.

I don't think there's any reason why that shouldn't work. DELETE ..
USING already allows ModifyTable to be scanning a join product, so
this is not much different.

> Of course, here is some limitations, to keep the patch size reasonable
> level to review.
> - The permission to bypass RLS policy was under discussion.
>  If and when we should provide a special permission to bypass RLS
>  policy, the "OR has_superuser_privilege()" shall be replaced by
>  "OR has_table_privilege(tableoid, 'RLSBYPASS')".

I think you're missing the point. Everyone who has commented on this
issue is in favor of having some check that causes the RLS predicate
*not to get added in the first place*. Adding a modified predicate is
not the same thing. First, the query planner might not be smart
enough to optimize away the clause even when the predicate holds,
causing an unnecessary performance drain. Second, there's too much
danger of being able to set a booby-trap for the superuser this way.
Suppose that the RLS subsystem replaces f_malicious() by f_malicious
OR has_superuser_privilege(). Now the superuser comes along with the
nightly pg_dump run and the query optimizer executes SELECT * FROM
nuts WHERE f_malicious() OR has_superuser_privilege(). The query
optimizer notes that the cost of f_malicious() is very low and decides
to execute that before has_superuser_privilege(). Oops. I think it's
just not acceptable to handle this by clause-munging: we need to not
add the clause in the first place.

Comments on the patch itself:

1. Please do not abbreviate rowlevel to rowlv or RowLevel to RowLv or
ROWLEVEL to ROWLV. That makes it harder to read and harder to grep.
Spell it out.

2. Since the entirety of ATExecSetRowLvSecurity is conditional on
whether clause != NULL, you might as well split it into two functions,
one for each case.

3. The fact that you've had to hack preprocess_targetlist and
adjust_appendrel_attrs_mutator suggests to me that the insertion of
the generate subquery is happening at the wrong phase of the process.
We don't need those special cases for views, so it seems like we
shouldn't need them here, either.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2012/6/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> Of course, here is some limitations, to keep the patch size reasonable
>> level to review.
>> - The permission to bypass RLS policy was under discussion.
>>  If and when we should provide a special permission to bypass RLS
>>  policy, the "OR has_superuser_privilege()" shall be replaced by
>>  "OR has_table_privilege(tableoid, 'RLSBYPASS')".
>
> I think you're missing the point.  Everyone who has commented on this
> issue is in favor of having some check that causes the RLS predicate
> *not to get added in the first place*.  Adding a modified predicate is
> not the same thing.  First, the query planner might not be smart
> enough to optimize away the clause even when the predicate holds,
> causing an unnecessary performance drain.  Second, there's too much
> danger of being able to set a booby-trap for the superuser this way.
> Suppose that the RLS subsystem replaces f_malicious() by f_malicious
> OR has_superuser_privilege().  Now the superuser comes along with the
> nightly pg_dump run and the query optimizer executes SELECT * FROM
> nuts WHERE f_malicious() OR has_superuser_privilege().  The query
> optimizer notes that the cost of f_malicious() is very low and decides
> to execute that before has_superuser_privilege().  Oops.  I think it's
> just not acceptable to handle this by clause-munging: we need to not
> add the clause in the first place.
>
Here is a simple idea to avoid the second problematic scenario; that
assign 0 as cost of has_superuser_privilege(). I allows to keep this
function more lightweight than any possible malicious function, since
CreateFunction enforces positive value.

But the first point is still remaining.

As you pointed out before, it might be a solution to have case-handling
for superusers and others in case of simple query protocol; that uses
same snapshot for planner and executor stage.

How should we handle the issue?

During the previous discussion, Tom mentioned about an idea that
saves prepared statement hashed with user-id to switch the query
plan depending on user's privilege.
Even though I hesitated to buy this idea at that time, it might be
worth to investigate this idea to satisfy both security and performance;
that will generate multiple query plans to be chosen at executor
stage later.

How about your opinion?

> Comments on the patch itself:
>
> 1. Please do not abbreviate rowlevel to rowlv or RowLevel to RowLv or
> ROWLEVEL to ROWLV.  That makes it harder to read and harder to grep.
> Spell it out.
>
OK,

> 2. Since the entirety of ATExecSetRowLvSecurity is conditional on
> whether clause != NULL, you might as well split it into two functions,
> one for each case.
>
OK,

> 3. The fact that you've had to hack preprocess_targetlist and
> adjust_appendrel_attrs_mutator suggests to me that the insertion of
> the generate subquery is happening at the wrong phase of the process.
> We don't need those special cases for views, so it seems like we
> shouldn't need them here, either.
>
Main reason why I had to patch them is special case handling for
references to system columns; that is unavailable to have for sub-
queries.
But, I'm not 100% sure around these implementation. So, it needs
more investigations.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> 2012/6/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> I think you're missing the point. Everyone who has commented on this
>> issue is in favor of having some check that causes the RLS predicate
>> *not to get added in the first place*.

> Here is a simple idea to avoid the second problematic scenario; that
> assign 0 as cost of has_superuser_privilege().

I am not sure which part of "this isn't safe" isn't getting through to
you. Aside from the scenarios Robert mentioned, consider the
possibility that f_malicious() is marked immutable, so that the planner
is likely to call it (to replace the call with its value) before it will
ever think about whether has_superuser_privilege should be called first.

Please just do what everybody is asking for, and create a bypass that
does not require fragile, easily-broken-by-future-changes assumptions
about what the planner will do with a WHERE clause.

regards, tom lane

2012/6/26 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> 2012/6/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> I think you're missing the point.  Everyone who has commented on this
>>> issue is in favor of having some check that causes the RLS predicate
>>> *not to get added in the first place*.
>
>> Here is a simple idea to avoid the second problematic scenario; that
>> assign 0 as cost of has_superuser_privilege().
>
> I am not sure which part of "this isn't safe" isn't getting through to
> you.  Aside from the scenarios Robert mentioned, consider the
> possibility that f_malicious() is marked immutable, so that the planner
> is likely to call it (to replace the call with its value) before it will
> ever think about whether has_superuser_privilege should be called first.
>
> Please just do what everybody is asking for, and create a bypass that
> does not require fragile, easily-broken-by-future-changes assumptions
> about what the planner will do with a WHERE clause.
>
The problem is the way to implement it.
If we would have permission checks on planner stage, it cannot handle
a case when user-id would be switched prior to executor stage, thus
it needs something remedy to handle the scenario correctly.
Instead of a unique plan per query, it might be a solution to generate
multiple plans depending on user-id, and choose a proper one in
executor stage.

Which type of implementation is what everybody is asking for?

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Jun27, 2012, at 07:18 , Kohei KaiGai wrote:
> The problem is the way to implement it.
> If we would have permission checks on planner stage, it cannot handle
> a case when user-id would be switched prior to executor stage, thus
> it needs something remedy to handle the scenario correctly.
> Instead of a unique plan per query, it might be a solution to generate
> multiple plans depending on user-id, and choose a proper one in
> executor stage.
>
> Which type of implementation is what everybody is asking for?

I think you need to

a) Determine the user-id at planning time, and insert the matching
RLS clause

b1) Either re-plan the query if the user-id changes between planning
and execution time, which means making the user-id a part of the
plan-cache key.

b2) Or decree that for RLS purposes, it's the user-id at planning time,
not execution time, that counts.

best regards,
Florian Pflug

2012/6/27 Florian Pflug <fgp(at)phlo(dot)org>:
> On Jun27, 2012, at 07:18 , Kohei KaiGai wrote:
>> The problem is the way to implement it.
>> If we would have permission checks on planner stage, it cannot handle
>> a case when user-id would be switched prior to executor stage, thus
>> it needs something remedy to handle the scenario correctly.
>> Instead of a unique plan per query, it might be a solution to generate
>> multiple plans depending on user-id, and choose a proper one in
>> executor stage.
>>
>> Which type of implementation is what everybody is asking for?
>
> I think you need to
>
>  a) Determine the user-id at planning time, and insert the matching
>    RLS clause
>
> b1) Either re-plan the query if the user-id changes between planning
>    and execution time, which means making the user-id a part of the
>    plan-cache key.
>
> b2) Or decree that for RLS purposes, it's the user-id at planning time,
>    not execution time, that counts.
>
My preference is b1, because b2 approach takes user visible changes
in concepts of permission checks.

Probably, plan-cache should be also invalidated when user's property
was modified or grant/revoke is issued, in addition to the table itself.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Wed, Jun 27, 2012 at 7:21 AM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
> On Jun27, 2012, at 07:18 , Kohei KaiGai wrote:
>> The problem is the way to implement it.
>> If we would have permission checks on planner stage, it cannot handle
>> a case when user-id would be switched prior to executor stage, thus
>> it needs something remedy to handle the scenario correctly.
>> Instead of a unique plan per query, it might be a solution to generate
>> multiple plans depending on user-id, and choose a proper one in
>> executor stage.
>>
>> Which type of implementation is what everybody is asking for?
>
> I think you need to
>
>  a) Determine the user-id at planning time, and insert the matching
>    RLS clause
>
> b1) Either re-plan the query if the user-id changes between planning
>    and execution time, which means making the user-id a part of the
>    plan-cache key.
>
> b2) Or decree that for RLS purposes, it's the user-id at planning time,
>    not execution time, that counts.

Or b3, flag plans that depend on the user ID inside the plan-cache,
and just flush all of those (but only those) when the user ID changes.
In the common case where RLS is not used, that might ease the sting.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2012/6/27 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Wed, Jun 27, 2012 at 7:21 AM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
>> On Jun27, 2012, at 07:18 , Kohei KaiGai wrote:
>>> The problem is the way to implement it.
>>> If we would have permission checks on planner stage, it cannot handle
>>> a case when user-id would be switched prior to executor stage, thus
>>> it needs something remedy to handle the scenario correctly.
>>> Instead of a unique plan per query, it might be a solution to generate
>>> multiple plans depending on user-id, and choose a proper one in
>>> executor stage.
>>>
>>> Which type of implementation is what everybody is asking for?
>>
>> I think you need to
>>
>>  a) Determine the user-id at planning time, and insert the matching
>>    RLS clause
>>
>> b1) Either re-plan the query if the user-id changes between planning
>>    and execution time, which means making the user-id a part of the
>>    plan-cache key.
>>
>> b2) Or decree that for RLS purposes, it's the user-id at planning time,
>>    not execution time, that counts.
>
> Or b3, flag plans that depend on the user ID inside the plan-cache,
> and just flush all of those (but only those) when the user ID changes.
>  In the common case where RLS is not used, that might ease the sting.
>
Probably, PlannedStmt->invalItems allows to handle invalidation of
plan-cache without big code changes. I'll try to put a flag of user-id
to track the query plan with RLS assumed, or InvalidOid if no RLS
was applied in this plan.
I'll investigate the implementation for more details.

Do we have any other scenario that run a query plan under different
user privilege rather than planner stage?

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Jun27, 2012, at 15:07 , Kohei KaiGai wrote:
> Probably, PlannedStmt->invalItems allows to handle invalidation of
> plan-cache without big code changes. I'll try to put a flag of user-id
> to track the query plan with RLS assumed, or InvalidOid if no RLS
> was applied in this plan.
> I'll investigate the implementation for more details.
>
> Do we have any other scenario that run a query plan under different
> user privilege rather than planner stage?

Hm, what happens if a SECURITY DEFINER functions returns a refcursor?

Actually, I wonder how we handle that today. If the executor is
responsible for permission checks, that wouldn't we apply the calling
function's privilege level in that case, at least of the cursor isn't
fetched from in the SECURITY DEFINER function? If I find some time,
I'll check...

best regards,
Florian Pflug

2012/6/27 Florian Pflug <fgp(at)phlo(dot)org>:
> On Jun27, 2012, at 15:07 , Kohei KaiGai wrote:
>> Probably, PlannedStmt->invalItems allows to handle invalidation of
>> plan-cache without big code changes. I'll try to put a flag of user-id
>> to track the query plan with RLS assumed, or InvalidOid if no RLS
>> was applied in this plan.
>> I'll investigate the implementation for more details.
>>
>> Do we have any other scenario that run a query plan under different
>> user privilege rather than planner stage?
>
> Hm, what happens if a SECURITY DEFINER functions returns a refcursor?
>
> Actually, I wonder how we handle that today. If the executor is
> responsible for permission checks, that wouldn't we apply the calling
> function's privilege level in that case, at least of the cursor isn't
> fetched from in the SECURITY DEFINER function? If I find some time,
> I'll check...
>
My impression is, here is no matter even if SECURITY DEFINER function
returns refcursor.

A SECURITY DEFINER function (or Trusted Procedure on sepgsql, or
Set-UID program on Linux) provides unprivileged users a particular
"limited way" to access protected data. It means owner of the security
definer function admits it is reasonable to show the protected data
as long as unprivileged users access them via the function.

It is same reason why we admit view's access for users who have
privileges on views but unprivileged to underlying tables.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> 2012/6/27 Florian Pflug <fgp(at)phlo(dot)org>:
>> Hm, what happens if a SECURITY DEFINER functions returns a refcursor?

> My impression is, here is no matter even if SECURITY DEFINER function
> returns refcursor.

I think Florian has a point: it *should* work, but *will* it?

I believe it works today, because the executor only applies permissions
checks during query startup. So those checks are executed while still
within the SECURITY DEFINER context, and should behave as expected.
Subsequently, the cursor portal is returned to caller and caller can
execute it to completion, no problem.

However, with RLS security-related checks will happen throughout the
execution of the portal. They might do the wrong thing once the
SECURITY DEFINER function has been exited.

We might need to consider that a portal has a local value of
"current_user", which is kind of a pain, but probably doable.

regards, tom lane

On Jun28, 2012, at 17:29 , Tom Lane wrote:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> 2012/6/27 Florian Pflug <fgp(at)phlo(dot)org>:
>>> Hm, what happens if a SECURITY DEFINER functions returns a refcursor?
>
>> My impression is, here is no matter even if SECURITY DEFINER function
>> returns refcursor.
>
> I think Florian has a point: it *should* work, but *will* it?
>
> I believe it works today, because the executor only applies permissions
> checks during query startup. So those checks are executed while still
> within the SECURITY DEFINER context, and should behave as expected.
> Subsequently, the cursor portal is returned to caller and caller can
> execute it to completion, no problem.

Don't we (sometimes?) defer query startup to the first time FETCH is
called?

best regards,
Florian Pflug

Florian Pflug <fgp(at)phlo(dot)org> writes:
> On Jun28, 2012, at 17:29 , Tom Lane wrote:
>> I believe it works today, because the executor only applies permissions
>> checks during query startup. So those checks are executed while still
>> within the SECURITY DEFINER context, and should behave as expected.
>> Subsequently, the cursor portal is returned to caller and caller can
>> execute it to completion, no problem.

> Don't we (sometimes?) defer query startup to the first time FETCH is
> called?

There are things inside individual plan node functions that may only
happen when the first row is demanded, but permissions checks are done
in ExecutorStart().

regards, tom lane

2012/6/28 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> 2012/6/27 Florian Pflug <fgp(at)phlo(dot)org>:
>>> Hm, what happens if a SECURITY DEFINER functions returns a refcursor?
>
>> My impression is, here is no matter even if SECURITY DEFINER function
>> returns refcursor.
>
> I think Florian has a point: it *should* work, but *will* it?
>
> I believe it works today, because the executor only applies permissions
> checks during query startup. So those checks are executed while still
> within the SECURITY DEFINER context, and should behave as expected.
> Subsequently, the cursor portal is returned to caller and caller can
> execute it to completion, no problem.
>
> However, with RLS security-related checks will happen throughout the
> execution of the portal. They might do the wrong thing once the
> SECURITY DEFINER function has been exited.
>
I tried the scenario that Florian pointed out.

postgres=# CREATE OR REPLACE FUNCTION f_test(refcursor) RETURNS refcursor
postgres-# SECURITY DEFINER LANGUAGE plpgsql
postgres-# AS 'BEGIN OPEN $1 FOR SELECT current_user, * FROM t1;
RETURN $1; END';
CREATE FUNCTION
postgres=# BEGIN;
BEGIN
postgres=# SELECT f_test('abc');
f_test
--------
abc
(1 row)

postgres=# FETCH abc;
current_user | a | b
--------------+---+-----
kaigai | 1 | aaa
(1 row)

postgres=# SET SESSION AUTHORIZATION alice;
SET
postgres=> FETCH abc;
current_user | a | b
--------------+---+-----
alice | 2 | bbb
(1 row)

postgres=> SET SESSION AUTHORIZATION bob;
SET
postgres=> FETCH abc;
current_user | a | b
--------------+---+-----
bob | 3 | ccc
(1 row)

Indeed, the output of "current_user" depends on the setting of session
authorization, even though it was declared within security definer
functions (thus, its security checks are applied on the privileges of
function owner).

> We might need to consider that a portal has a local value of
> "current_user", which is kind of a pain, but probably doable.
>
It seems to me, it is an individual matter to be fixed independent
from RLS feature. How about your opinion?

If we go ahead, an idea to tackle this matter is switch user-id
into saved one in the Portal at the beginning of PortanRun or
PortalRunFetch.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

The attached patch is a revised version of row-level security feature.

I added a feature to invalidate plan cache if user-id was switched
between planner and optimizer. It enabled to generate more optimized
plan than the previous approach; that adds hardwired "OR superuser()".

Example)
postgres=# PREPARE p1(int) AS SELECT * FROM t1 WHERE x > $1 AND f_leak(y);
PREPARE
postgres=# EXPLAIN (costs off) EXECUTE p1(2);
QUERY PLAN
-----------------------------------
Seq Scan on t1
Filter: (f_leak(y) AND (x > 2))
(2 rows)

postgres=# SET SESSION AUTHORIZATION alice;
SET
postgres=> EXPLAIN (costs off) EXECUTE p1(2);
QUERY PLAN
---------------------------------------------
Subquery Scan on t1
Filter: f_leak(t1.y)
-> Seq Scan on t1
Filter: ((x > 2) AND ((x % 2) = 0))
(4 rows)

On the other hand, I removed support for UPDATE / DELETE commands
in this revision, because I'm still uncertain on the implementation that I
adopted in the previous patch. I believe it helps to keep patch size being
minimum reasonable.
Due to same reason, RLS is not supported on COPY TO command.

According to the Robert's comment, I revised the place to inject
applyRowLevelSecurity(). The reason why it needed to patch on
adjust_appendrel_attrs_mutator() was, we handled expansion from
regular relation to sub-query after expand_inherited_tables().
In this revision, it was moved to the head of sub-query planner.

Even though I added a syscache entry for pg_rowlevelsec catalog,
it was revised to read the catalog on construction of relcache, like
trigger descriptor, because it enables to reduce cost to parse an
expression tree in text format and memory consumption of hash
slot.

This revision adds support on pg_dump, and also adds support
to include SubLinks in the row-level security policy.

Thanks,

2012/7/1 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/6/28 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>>> 2012/6/27 Florian Pflug <fgp(at)phlo(dot)org>:
>>>> Hm, what happens if a SECURITY DEFINER functions returns a refcursor?
>>
>>> My impression is, here is no matter even if SECURITY DEFINER function
>>> returns refcursor.
>>
>> I think Florian has a point: it *should* work, but *will* it?
>>
>> I believe it works today, because the executor only applies permissions
>> checks during query startup. So those checks are executed while still
>> within the SECURITY DEFINER context, and should behave as expected.
>> Subsequently, the cursor portal is returned to caller and caller can
>> execute it to completion, no problem.
>>
>> However, with RLS security-related checks will happen throughout the
>> execution of the portal. They might do the wrong thing once the
>> SECURITY DEFINER function has been exited.
>>
> I tried the scenario that Florian pointed out.
>
> postgres=# CREATE OR REPLACE FUNCTION f_test(refcursor) RETURNS refcursor
> postgres-# SECURITY DEFINER LANGUAGE plpgsql
> postgres-# AS 'BEGIN OPEN $1 FOR SELECT current_user, * FROM t1;
> RETURN $1; END';
> CREATE FUNCTION
> postgres=# BEGIN;
> BEGIN
> postgres=# SELECT f_test('abc');
> f_test
> --------
> abc
> (1 row)
>
> postgres=# FETCH abc;
> current_user | a | b
> --------------+---+-----
> kaigai | 1 | aaa
> (1 row)
>
> postgres=# SET SESSION AUTHORIZATION alice;
> SET
> postgres=> FETCH abc;
> current_user | a | b
> --------------+---+-----
> alice | 2 | bbb
> (1 row)
>
> postgres=> SET SESSION AUTHORIZATION bob;
> SET
> postgres=> FETCH abc;
> current_user | a | b
> --------------+---+-----
> bob | 3 | ccc
> (1 row)
>
> Indeed, the output of "current_user" depends on the setting of session
> authorization, even though it was declared within security definer
> functions (thus, its security checks are applied on the privileges of
> function owner).
>
>> We might need to consider that a portal has a local value of
>> "current_user", which is kind of a pain, but probably doable.
>>
> It seems to me, it is an individual matter to be fixed independent
> from RLS feature. How about your opinion?
>
> If we go ahead, an idea to tackle this matter is switch user-id
> into saved one in the Portal at the beginning of PortanRun or
> PortalRunFetch.
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sun, Jul 15, 2012 at 5:52 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> The attached patch is a revised version of row-level security feature.
>
> I added a feature to invalidate plan cache if user-id was switched
> between planner and optimizer. It enabled to generate more optimized
> plan than the previous approach; that adds hardwired "OR superuser()".
>
> Example)
> postgres=# PREPARE p1(int) AS SELECT * FROM t1 WHERE x > $1 AND f_leak(y);
> PREPARE
> postgres=# EXPLAIN (costs off) EXECUTE p1(2);
> QUERY PLAN
> -----------------------------------
> Seq Scan on t1
> Filter: (f_leak(y) AND (x > 2))
> (2 rows)
>
> postgres=# SET SESSION AUTHORIZATION alice;
> SET
> postgres=> EXPLAIN (costs off) EXECUTE p1(2);
> QUERY PLAN
> ---------------------------------------------
> Subquery Scan on t1
> Filter: f_leak(t1.y)
> -> Seq Scan on t1
> Filter: ((x > 2) AND ((x % 2) = 0))
> (4 rows)
>
> On the other hand, I removed support for UPDATE / DELETE commands
> in this revision, because I'm still uncertain on the implementation that I
> adopted in the previous patch. I believe it helps to keep patch size being
> minimum reasonable.
> Due to same reason, RLS is not supported on COPY TO command.
>
> According to the Robert's comment, I revised the place to inject
> applyRowLevelSecurity(). The reason why it needed to patch on
> adjust_appendrel_attrs_mutator() was, we handled expansion from
> regular relation to sub-query after expand_inherited_tables().
> In this revision, it was moved to the head of sub-query planner.
>
> Even though I added a syscache entry for pg_rowlevelsec catalog,
> it was revised to read the catalog on construction of relcache, like
> trigger descriptor, because it enables to reduce cost to parse an
> expression tree in text format and memory consumption of hash
> slot.
>
> This revision adds support on pg_dump, and also adds support
> to include SubLinks in the row-level security policy.

This revision is too late for this CommitFest; I've moved it to the
next CommitFest and will look at it then, or hopefully sooner.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2012/7/17 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Jul 15, 2012 at 5:52 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> The attached patch is a revised version of row-level security feature.
>>
>> I added a feature to invalidate plan cache if user-id was switched
>> between planner and optimizer. It enabled to generate more optimized
>> plan than the previous approach; that adds hardwired "OR superuser()".
>>
>> Example)
>> postgres=# PREPARE p1(int) AS SELECT * FROM t1 WHERE x > $1 AND f_leak(y);
>> PREPARE
>> postgres=# EXPLAIN (costs off) EXECUTE p1(2);
>> QUERY PLAN
>> -----------------------------------
>> Seq Scan on t1
>> Filter: (f_leak(y) AND (x > 2))
>> (2 rows)
>>
>> postgres=# SET SESSION AUTHORIZATION alice;
>> SET
>> postgres=> EXPLAIN (costs off) EXECUTE p1(2);
>> QUERY PLAN
>> ---------------------------------------------
>> Subquery Scan on t1
>> Filter: f_leak(t1.y)
>> -> Seq Scan on t1
>> Filter: ((x > 2) AND ((x % 2) = 0))
>> (4 rows)
>>
>> On the other hand, I removed support for UPDATE / DELETE commands
>> in this revision, because I'm still uncertain on the implementation that I
>> adopted in the previous patch. I believe it helps to keep patch size being
>> minimum reasonable.
>> Due to same reason, RLS is not supported on COPY TO command.
>>
>> According to the Robert's comment, I revised the place to inject
>> applyRowLevelSecurity(). The reason why it needed to patch on
>> adjust_appendrel_attrs_mutator() was, we handled expansion from
>> regular relation to sub-query after expand_inherited_tables().
>> In this revision, it was moved to the head of sub-query planner.
>>
>> Even though I added a syscache entry for pg_rowlevelsec catalog,
>> it was revised to read the catalog on construction of relcache, like
>> trigger descriptor, because it enables to reduce cost to parse an
>> expression tree in text format and memory consumption of hash
>> slot.
>>
>> This revision adds support on pg_dump, and also adds support
>> to include SubLinks in the row-level security policy.
>
> This revision is too late for this CommitFest; I've moved it to the
> next CommitFest and will look at it then, or hopefully sooner.
>
It seems to me fair enough. I may be able to add UPDATE /
DELETE support until next commit-fest.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On 17 July 2012 05:02, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2012/7/17 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Sun, Jul 15, 2012 at 5:52 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> The attached patch is a revised version of row-level security feature.
>>> ...
>>> According to the Robert's comment, I revised the place to inject
>>> applyRowLevelSecurity(). The reason why it needed to patch on
>>> adjust_appendrel_attrs_mutator() was, we handled expansion from
>>> regular relation to sub-query after expand_inherited_tables().
>>> In this revision, it was moved to the head of sub-query planner.
>>>

Hi,

I had a quick look at this and spotted a problem - certain types of
query are able to bypass the RLS quals. For example:

SELECT * FROM (SELECT * FROM foo) foo;

since the RLS policy doesn't descend into subqueries, and is applied
before they are pulled up into the main query. Similarly for views on
top of tables with RLS, and SRF functions that query a table with RLS
that get inlined.

Also queries using UNION ALL are vulnerable if they end up being
flattened, for example:

SELECT * FROM foo UNION ALL SELECT * FROM foo;

FWIW I recently developed some similar code as part of a patch to
implement automatically updatable views
(http://archives.postgresql.org/pgsql-hackers/2012-08/msg00303.php).
Some parts of that code may be useful, possibly for adding
UPDATE/DELETE support.

Regards,
Dean

2012/9/2 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
> On 17 July 2012 05:02, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 2012/7/17 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> On Sun, Jul 15, 2012 at 5:52 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> The attached patch is a revised version of row-level security feature.
>>>> ...
>>>> According to the Robert's comment, I revised the place to inject
>>>> applyRowLevelSecurity(). The reason why it needed to patch on
>>>> adjust_appendrel_attrs_mutator() was, we handled expansion from
>>>> regular relation to sub-query after expand_inherited_tables().
>>>> In this revision, it was moved to the head of sub-query planner.
>>>>
>
> Hi,
>
> I had a quick look at this and spotted a problem - certain types of
> query are able to bypass the RLS quals. For example:
>
> SELECT * FROM (SELECT * FROM foo) foo;
>
> since the RLS policy doesn't descend into subqueries, and is applied
> before they are pulled up into the main query. Similarly for views on
> top of tables with RLS, and SRF functions that query a table with RLS
> that get inlined.
>
> Also queries using UNION ALL are vulnerable if they end up being
> flattened, for example:
>
> SELECT * FROM foo UNION ALL SELECT * FROM foo;
>
Thanks for your comment.

Indeed, I missed the case of simple sub-queries and union-all being
pulled up into the main query. So, I adjusted the location to invoke
applyRowLevelSecurity() between all the pull-up stuff and expanding
inherited tables.

The attached patch is a fixed and rebased revision for CF:Sep.

> FWIW I recently developed some similar code as part of a patch to
> implement automatically updatable views
> (http://archives.postgresql.org/pgsql-hackers/2012-08/msg00303.php).
> Some parts of that code may be useful, possibly for adding
> UPDATE/DELETE support.
>
Let me check it.

Best regards,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

2012/9/3 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/9/2 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
>> On 17 July 2012 05:02, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> 2012/7/17 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>>> On Sun, Jul 15, 2012 at 5:52 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>>> The attached patch is a revised version of row-level security feature.
>>>>> ...
>>>>> According to the Robert's comment, I revised the place to inject
>>>>> applyRowLevelSecurity(). The reason why it needed to patch on
>>>>> adjust_appendrel_attrs_mutator() was, we handled expansion from
>>>>> regular relation to sub-query after expand_inherited_tables().
>>>>> In this revision, it was moved to the head of sub-query planner.
>>>>>
>>
>> Hi,
>>
>> I had a quick look at this and spotted a problem - certain types of
>> query are able to bypass the RLS quals. For example:
>>
>> SELECT * FROM (SELECT * FROM foo) foo;
>>
>> since the RLS policy doesn't descend into subqueries, and is applied
>> before they are pulled up into the main query. Similarly for views on
>> top of tables with RLS, and SRF functions that query a table with RLS
>> that get inlined.
>>
>> Also queries using UNION ALL are vulnerable if they end up being
>> flattened, for example:
>>
>> SELECT * FROM foo UNION ALL SELECT * FROM foo;
>>
> Thanks for your comment.
>
> Indeed, I missed the case of simple sub-queries and union-all being
> pulled up into the main query. So, I adjusted the location to invoke
> applyRowLevelSecurity() between all the pull-up stuff and expanding
> inherited tables.
>
> The attached patch is a fixed and rebased revision for CF:Sep.
>
Sorry! I attached incorrect revision. The attached patch is right one.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

The attached patch is a refreshed version towards the latest master branch,
to fix up patch conflicts.
Here is no other difference from the previous revision.

Thanks,

2012/9/5 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/9/3 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>> 2012/9/2 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
>>> On 17 July 2012 05:02, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> 2012/7/17 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>>>> On Sun, Jul 15, 2012 at 5:52 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>>>> The attached patch is a revised version of row-level security feature.
>>>>>> ...
>>>>>> According to the Robert's comment, I revised the place to inject
>>>>>> applyRowLevelSecurity(). The reason why it needed to patch on
>>>>>> adjust_appendrel_attrs_mutator() was, we handled expansion from
>>>>>> regular relation to sub-query after expand_inherited_tables().
>>>>>> In this revision, it was moved to the head of sub-query planner.
>>>>>>
>>>
>>> Hi,
>>>
>>> I had a quick look at this and spotted a problem - certain types of
>>> query are able to bypass the RLS quals. For example:
>>>
>>> SELECT * FROM (SELECT * FROM foo) foo;
>>>
>>> since the RLS policy doesn't descend into subqueries, and is applied
>>> before they are pulled up into the main query. Similarly for views on
>>> top of tables with RLS, and SRF functions that query a table with RLS
>>> that get inlined.
>>>
>>> Also queries using UNION ALL are vulnerable if they end up being
>>> flattened, for example:
>>>
>>> SELECT * FROM foo UNION ALL SELECT * FROM foo;
>>>
>> Thanks for your comment.
>>
>> Indeed, I missed the case of simple sub-queries and union-all being
>> pulled up into the main query. So, I adjusted the location to invoke
>> applyRowLevelSecurity() between all the pull-up stuff and expanding
>> inherited tables.
>>
>> The attached patch is a fixed and rebased revision for CF:Sep.
>>
> Sorry! I attached incorrect revision. The attached patch is right one.
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On 8 October 2012 15:57, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> The attached patch is a refreshed version towards the latest master branch,
> to fix up patch conflicts.
> Here is no other difference from the previous revision.
>
> Thanks,
>

I had another look at this over the weekend and I found couple of
additional problems (test cases attached):

1). It is possible to define a RLS qual that refers back to the table
that it's defined on, in such a way that causes infinite recursion in
the planner, giving "ERROR: stack depth limit exceeded". I think it
would be preferable to trap this and report a more meaningful error
back to the user, along similar lines to a self-referencing view.

2). In other cases it is possible to define a RLS qual that refers to
another table with a RLS qual in such a way that the second table's
RLS qual is not checked, thus allowing a user to bypass the security
check.

3). If a RLS qual refers to a view it errors, since the RLS quals are
added after rule expansion, and so the view is not rewritten.

To me this suggests that perhaps the expansion of RLS quals should be
done in the rewriter. I've not thought that through in any detail, but
ISTM that a RIR rule could add a table with a RLS qual, and a RLS qual
could add a relation with a RIR rule that needs expanding, and so the
2 need to be processed together. This could also make use of the
existing recursion-checking code in the rewriter.

Regards,
Dean

2012/10/8 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
> On 8 October 2012 15:57, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> The attached patch is a refreshed version towards the latest master branch,
>> to fix up patch conflicts.
>> Here is no other difference from the previous revision.
>>
>> Thanks,
>>
>
> I had another look at this over the weekend and I found couple of
> additional problems (test cases attached):
>
> 1). It is possible to define a RLS qual that refers back to the table
> that it's defined on, in such a way that causes infinite recursion in
> the planner, giving "ERROR: stack depth limit exceeded". I think it
> would be preferable to trap this and report a more meaningful error
> back to the user, along similar lines to a self-referencing view.
>
> 2). In other cases it is possible to define a RLS qual that refers to
> another table with a RLS qual in such a way that the second table's
> RLS qual is not checked, thus allowing a user to bypass the security
> check.
>
> 3). If a RLS qual refers to a view it errors, since the RLS quals are
> added after rule expansion, and so the view is not rewritten.
>
> To me this suggests that perhaps the expansion of RLS quals should be
> done in the rewriter. I've not thought that through in any detail, but
> ISTM that a RIR rule could add a table with a RLS qual, and a RLS qual
> could add a relation with a RIR rule that needs expanding, and so the
> 2 need to be processed together. This could also make use of the
> existing recursion-checking code in the rewriter.
>
Thanks for your checks. I missed some cases that you suggested.

The reason why we need to put RLS expansion at planner stage is
requirement towards plan cache invalidation. Due to special case
handling for superuser, plan cache has to be invalidated if user-id
to run executor was switched since planner stage. The planner shall
be invoked again, but not rewritter, on its invalidation.

Probably, it make sense to invoke rewriter's logic to solve RLS
policy from planner stage (that allows plan-cache invalidation).
Let me investigate the code of rewriter.

Best regards,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

The revised patch fixes the problem that Daen pointed out.

In case when row-level security policy contains SubLink node,
it become to call the rewriter to expand views being contained
within SubLink, and also append qualifier of security policy
onto underlying tables. It enables to apply configured policy
on nested relations also, as if top-level ones.
In addition, I added a check for infinite recursion when two
different tables have row-level policy that references each
other.

So, for example, self-recursion RLS shall be prevented.
postgres=> ALTER TABLE foo SET ROW LEVEL SECURITY (a < (SELECT
max(a) FROM foo));
ALTER TABLE
postgres=> SELECT * FROM foo;
ERROR: infinite recursion detected for relation "foo"

It shows RLS policy is recursively applied.
postgres=> ALTER TABLE foo SET ROW LEVEL SECURITY (a > 0);
ALTER TABLE
postgres=> ALTER TABLE bar SET ROW LEVEL SECURITY
(EXISTS(SELECT 1 FROM foo WHERE foo.a = bar.a));
ALTER TABLE
postgres=> EXPLAIN SELECT * FROM bar;
QUERY PLAN
--------------------------------------------------------------------------------------------
Hash Join (cost=44.95..111.95 rows=1200 width=4)
Hash Cond: (bar.a = foo.a)
-> Seq Scan on bar (cost=0.00..34.00 rows=2400 width=4)
-> Hash (cost=42.45..42.45 rows=200 width=4)
-> HashAggregate (cost=40.45..42.45 rows=200 width=4)
-> Bitmap Heap Scan on foo (cost=10.45..30.45
rows=800 width=4)
Recheck Cond: (a > 0)
-> Bitmap Index Scan on foo_pkey
(cost=0.00..10.25 rows=800 width=0)
Index Cond: (a > 0)
(9 rows)

Even if RLS policy contains a view, it works fine.
postgres=> CREATE VIEW foo_v AS SELECT * FROM foo;
CREATE VIEW
postgres=> ALTER TABLE bar SET ROW LEVEL SECURITY
(a IN (SELECT * FROM foo_v));
ALTER TABLE
postgres=> EXPLAIN SELECT * FROM bar;
QUERY PLAN
--------------------------------------------------------------------------------------------
Hash Join (cost=44.95..111.95 rows=1200 width=4)
Hash Cond: (bar.a = foo.a)
-> Seq Scan on bar (cost=0.00..34.00 rows=2400 width=4)
-> Hash (cost=42.45..42.45 rows=200 width=4)
-> HashAggregate (cost=40.45..42.45 rows=200 width=4)
-> Bitmap Heap Scan on foo (cost=10.45..30.45
rows=800 width=4)
Recheck Cond: (a > 0)
-> Bitmap Index Scan on foo_pkey
(cost=0.00..10.25 rows=800 width=0)
Index Cond: (a > 0)
(9 rows)

Thanks,

2012/10/8 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/10/8 Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>:
>> On 8 October 2012 15:57, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> The attached patch is a refreshed version towards the latest master branch,
>>> to fix up patch conflicts.
>>> Here is no other difference from the previous revision.
>>>
>>> Thanks,
>>>
>>
>> I had another look at this over the weekend and I found couple of
>> additional problems (test cases attached):
>>
>> 1). It is possible to define a RLS qual that refers back to the table
>> that it's defined on, in such a way that causes infinite recursion in
>> the planner, giving "ERROR: stack depth limit exceeded". I think it
>> would be preferable to trap this and report a more meaningful error
>> back to the user, along similar lines to a self-referencing view.
>>
>> 2). In other cases it is possible to define a RLS qual that refers to
>> another table with a RLS qual in such a way that the second table's
>> RLS qual is not checked, thus allowing a user to bypass the security
>> check.
>>
>> 3). If a RLS qual refers to a view it errors, since the RLS quals are
>> added after rule expansion, and so the view is not rewritten.
>>
>> To me this suggests that perhaps the expansion of RLS quals should be
>> done in the rewriter. I've not thought that through in any detail, but
>> ISTM that a RIR rule could add a table with a RLS qual, and a RLS qual
>> could add a relation with a RIR rule that needs expanding, and so the
>> 2 need to be processed together. This could also make use of the
>> existing recursion-checking code in the rewriter.
>>
> Thanks for your checks. I missed some cases that you suggested.
>
> The reason why we need to put RLS expansion at planner stage is
> requirement towards plan cache invalidation. Due to special case
> handling for superuser, plan cache has to be invalidated if user-id
> to run executor was switched since planner stage. The planner shall
> be invoked again, but not rewritter, on its invalidation.
>
> Probably, it make sense to invoke rewriter's logic to solve RLS
> policy from planner stage (that allows plan-cache invalidation).
> Let me investigate the code of rewriter.
>
> Best regards,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai escribió:
> The revised patch fixes the problem that Daen pointed out.

Robert, would you be able to give this latest version of the patch a
look?

(KaiGai, does it still apply cleanly? If not, please submit a rebased
version.)

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

2012/10/18 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
> Kohei KaiGai escribió:
>> The revised patch fixes the problem that Daen pointed out.
>
> Robert, would you be able to give this latest version of the patch a
> look?
>
> (KaiGai, does it still apply cleanly? If not, please submit a rebased
> version.)
>
I confirmed I could apply the latest patch cleanly.

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Fri, October 19, 2012 11:00, Kohei KaiGai wrote:
> I confirmed I could apply the latest patch cleanly.
>

FWIW, I spent a few sessions (amounting to a few hours) trying to break, or get past SET ROW LEVEL
SECURITY and have not yet succeeded. So far so good.

(I haven't looked at code)

Erik Rijkers

On Thu, Oct 18, 2012 at 2:19 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Kohei KaiGai escribió:
>> The revised patch fixes the problem that Daen pointed out.
>
> Robert, would you be able to give this latest version of the patch a
> look?

Yeah, sorry I've been completely sidelined this CommitFest. It's been
a crazy couple of months. Prognosis for future craziness reduction
uncertain. Comments:

The documentation lists several documented limitations that I would
like to analyze a little bit. First, it says that row-level security
policies are not applied on UPDATE or DELETE. That sounds downright
dangerous to me. Is there some really compelling reason we're not
doing it? Second, it says that row-level security policies are not
currently applied on INSERT, so you should use a trigger, but implies
that this will change in the future. I don't think we should change
that in the future; I think relying on triggers for that case is just
fine. Note that it could be an issue with the post-image for UPDATES,
as well, and I think the trigger solution is similarly adequate to
cover that case. With respect to the documented limitation regarding
DECLARE/FETCH, what exactly will happen? Can we describe this a bit
more clearly rather than just saying the behavior will be
unpredictable?

It looks suspiciously as if the row-level security mode needs to be
saved and restored in all the same places we call save and restore the
user ID and security context. Is there some reason the
row-level-security-enabled flag shouldn't just become another bit in
the security context? Then we'd get all of this save/restore logic
mostly for free.

ATExecSetRowLevelSecurity() calls SetRowLevelSecurity() or
ResetRowLevelSecurity() to update pg_rowlevelsec, but does the
pg_class update itself. I think that all of this logic should be
moved into a single function, or at least functions in the same file,
with the one that only updates pg_rowlevelsec being static and
therefore not able to be called from outside the file. We always need
the pg_class update and the pg_rowlevelsec update to happen together,
so it's not good to have an exposed function that does one of those
updates but not the other. I think the simplest thing is just to move
ATExecSetRowLevelSecurity to pg_rowlevelsec.c and rename it to
SetRowLevelSecurity() and then give it two static helper functions,
say InsertPolicyRow() and DeletePolicyRow().

I think it would be good if Tom could review the query-rewriting parts
of this (viz rowlevelsec.c) as I am not terribly familiar with this
machinery, and of course anything we get wrong here will have security
consequences. At first blush, I'm somewhat concerned about the fact
that we're trying to do this after query rewriting; that seems like it
could break things. I know KaiGai mentioned upthread that the
rewriter won't be rerun if the plan is invalidated, but (1) why is
that OK now? and (2) if it is OK now, then why is it OK to do
rewriting of the RLS qual - only - after rewriting if all of the rest
of the rewriting needs to happen earlier?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> The documentation lists several documented limitations that I would
> like to analyze a little bit. First, it says that row-level security
> policies are not applied on UPDATE or DELETE. That sounds downright
> dangerous to me. Is there some really compelling reason we're not
> doing it?

[ blink... ] Isn't that a security hole big enough for a Mack truck?

UPDATE tab SET foo = foo RETURNING *;

sucks out all the data just fine, if RLS doesn't apply to it.

Having said that, I fear that sensible row-level security for updates is
at least one order of magnitude harder than sensible row-level security
for selects. We've speculated about how to define that in the past,
IIRC, but without any very satisfactory outcome.

regards, tom lane

On Mon, Oct 22, 2012 at 12:17 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> The documentation lists several documented limitations that I would
>> like to analyze a little bit. First, it says that row-level security
>> policies are not applied on UPDATE or DELETE. That sounds downright
>> dangerous to me. Is there some really compelling reason we're not
>> doing it?
>
> [ blink... ] Isn't that a security hole big enough for a Mack truck?
>
> UPDATE tab SET foo = foo RETURNING *;
>
> sucks out all the data just fine, if RLS doesn't apply to it.

Yep.

> Having said that, I fear that sensible row-level security for updates is
> at least one order of magnitude harder than sensible row-level security
> for selects. We've speculated about how to define that in the past,
> IIRC, but without any very satisfactory outcome.

Uh, I don't agree. SELECT and DELETE are pretty much identical cases.
UPDATE needs all the same stuff that those two cases need, plus it
has an additional problem that it shares with INSERT - namely, someone
might insert a tuple that they cannot see or update a tuple such that
they can no longer see it. However, both of those problems can be
handled via triggers, for now and maybe forever. In contrast, the
problem that SELECT has - which UPDATE and DELETE also share - namely,
of rows being visible that should not be - is not nearly so
susceptible to that approach, both for performance reasons and because
there's no such thing as a trigger on SELECT.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2012/10/22 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Oct 18, 2012 at 2:19 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
>> Kohei KaiGai escribió:
>>> The revised patch fixes the problem that Daen pointed out.
>>
>> Robert, would you be able to give this latest version of the patch a
>> look?
>
> Yeah, sorry I've been completely sidelined this CommitFest. It's been
> a crazy couple of months. Prognosis for future craziness reduction
> uncertain. Comments:
>
> The documentation lists several documented limitations that I would
> like to analyze a little bit. First, it says that row-level security
> policies are not applied on UPDATE or DELETE. That sounds downright
> dangerous to me. Is there some really compelling reason we're not
> doing it?

It intends to simplify the patch to avoid doing everything within a single
patch. I'll submit the patch supporting UPDATE and DELETE for CF-Nov
in addition to the base one.

> Second, it says that row-level security policies are not
> currently applied on INSERT, so you should use a trigger, but implies
> that this will change in the future. I don't think we should change
> that in the future; I think relying on triggers for that case is just
> fine. Note that it could be an issue with the post-image for UPDATES,
> as well, and I think the trigger solution is similarly adequate to
> cover that case.

Hmm. I should not have written this in section of the current limitation.
It may give impression the behavior will be changed future.
OK, I'll try to revise the documentation stuff.

> With respect to the documented limitation regarding
> DECLARE/FETCH, what exactly will happen? Can we describe this a bit
> more clearly rather than just saying the behavior will be
> unpredictable?
>
In case when user-id was switched after declaration of a cursor that
contains qualifier depending on current_user, its results set contains
rows with old user-id and rows with new user-id.

Here is one other option rather than documentation fix.
As we had a discussion on the upthread, it can be solved if we restore
the user-id associated with the portal to be run, however, a problem is
some commands switches user-id inside of the portal.
http://archives.postgresql.org/pgsql-hackers/2012-07/msg00055.php

Is there some good idea to avoid the problem?

> It looks suspiciously as if the row-level security mode needs to be
> saved and restored in all the same places we call save and restore the
> user ID and security context. Is there some reason the
> row-level-security-enabled flag shouldn't just become another bit in
> the security context? Then we'd get all of this save/restore logic
> mostly for free.
>
It seems to me a good idea, but I didn't find out this.

> ATExecSetRowLevelSecurity() calls SetRowLevelSecurity() or
> ResetRowLevelSecurity() to update pg_rowlevelsec, but does the
> pg_class update itself. I think that all of this logic should be
> moved into a single function, or at least functions in the same file,
> with the one that only updates pg_rowlevelsec being static and
> therefore not able to be called from outside the file. We always need
> the pg_class update and the pg_rowlevelsec update to happen together,
> so it's not good to have an exposed function that does one of those
> updates but not the other. I think the simplest thing is just to move
> ATExecSetRowLevelSecurity to pg_rowlevelsec.c and rename it to
> SetRowLevelSecurity() and then give it two static helper functions,
> say InsertPolicyRow() and DeletePolicyRow().
>
OK, I'll rework the code.

> I think it would be good if Tom could review the query-rewriting parts
> of this (viz rowlevelsec.c) as I am not terribly familiar with this
> machinery, and of course anything we get wrong here will have security
> consequences. At first blush, I'm somewhat concerned about the fact
> that we're trying to do this after query rewriting; that seems like it
> could break things. I know KaiGai mentioned upthread that the
> rewriter won't be rerun if the plan is invalidated, but (1) why is
> that OK now? and (2) if it is OK now, then why is it OK to do
> rewriting of the RLS qual - only - after rewriting if all of the rest
> of the rewriting needs to happen earlier?
>
I just follow the existing behavior of plan invalidation; that does not
re-run the query rewriter. So, if we have no particular reason why
we should not run the rewriter again to handle RLS quals, it might
be an option to handle RLS as a part of rewriter.

At least, here is two problems. 1) System column is problematic
when SELECT statement is replaced by sub-query. 2) It makes
infinite recursion when a certain table has SELECT INSTEAD
rule with a sub-query on the same table.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

The attached patch is a revised version of row-level security
feature.
According to Robert's suggestion, I reworked implementation
around ALTER command, and logic to disable RLS during
FK/PK constraint checks.

In addition, I moved the entrypoint to apply row-level security
policy on the query tree next to the expand_inherited_tables,
because it became clear my previous approach is not
a straight-forward way to support update / delete cases.

This patch performs to replace RangeTblEntry of tables with
RLS policy by sub-queries that simply references the original
table with configured RLS policy. Also, the sub-queries have
security_barrier flag to prevent non-leakproof functions being
pushed down from outside of the sub-query.

This sub-query has target-list that just references columns of
underlying table, and ordered according to column definition
of the original table. So, we don't need to adjust varattno of
Var-node that reference regular columns, even though the
RangeTblEntry was replaced.
On the other hand, system-column is problematic because
sub-query does not have these columns due to nature of them.
So, I inject a logic to adjust varattno of Var-node that references
system-column of the target tables being replaced.
It works fine as follows:

postgres=> ALTER TABLE t1 SET ROW LEVEL SECURITY (a % 2 = 0);
ALTER TABLE
postgres=> ALTER TABLE t2 SET ROW LEVEL SECURITY (a % 2 = 1);
ALTER TABLE
postgres=> EXPLAIN (costs off) SELECT tableoid, * FROM t1 WHERE b like '%';
QUERY PLAN
-------------------------------------------
Result
-> Append
-> Subquery Scan on t1
Filter: (t1.b ~~ '%'::text)
-> Seq Scan on t1 t1_1
Filter: ((a % 2) = 0)
-> Subquery Scan on t2
Filter: (t2.b ~~ '%'::text)
-> Seq Scan on t2 t2_1
Filter: ((a % 2) = 1)
-> Seq Scan on t3
Filter: (b ~~ '%'::text)
(12 rows)

postgres=> SELECT tableoid, * FROM t1 WHERE b like '%';
tableoid | a | b
----------+----+-----
16385 | 2 | bbb
16385 | 4 | ddd
16385 | 6 | fff
16391 | 11 | sss
16391 | 13 | uuu
16391 | 15 | yyy
16397 | 21 | xyz
16397 | 22 | yzx
16397 | 23 | zxy
(9 rows)

Also, UPDATE / DELETE statement

postgres=> EXPLAIN (costs off) UPDATE t1 SET b = b || '_updt' WHERE b like '%';
QUERY PLAN
-------------------------------------
Update on t1
-> Subquery Scan on t1
Filter: (t1.b ~~ '%'::text)
-> Seq Scan on t1 t1_1
Filter: ((a % 2) = 0)
-> Subquery Scan on t2
Filter: (t2.b ~~ '%'::text)
-> Seq Scan on t2 t2_1
Filter: ((a % 2) = 1)
-> Seq Scan on t3
Filter: (b ~~ '%'::text)
(11 rows)

postgres=> UPDATE t1 SET b = b || '_updt' WHERE b like '%';
UPDATE 9

However, UPDATE / DELETE support is not perfect right now.
In case when we try to update / delete a table with inherited
children and RETURNING clause was added, is loses right
references to the pseudo columns, even though it works fine
without inherited children.

postgres=> UPDATE only t1 SET b = b || '_updt' WHERE b like '%' RETURNING *;
a | b
---+----------
2 | bbb_updt
4 | ddd_updt
6 | fff_updt
(3 rows)

UPDATE 3
postgres=> UPDATE t1 SET b = b || '_updt' WHERE b like '%' RETURNING *;
ERROR: variable not found in subplan target lists

I'm still under investigation of this behavior. Any comments
will be helpful to solve this problem.

Thanks,

2012/10/22 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/10/22 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Thu, Oct 18, 2012 at 2:19 PM, Alvaro Herrera
>> <alvherre(at)2ndquadrant(dot)com> wrote:
>>> Kohei KaiGai escribió:
>>>> The revised patch fixes the problem that Daen pointed out.
>>>
>>> Robert, would you be able to give this latest version of the patch a
>>> look?
>>
>> Yeah, sorry I've been completely sidelined this CommitFest. It's been
>> a crazy couple of months. Prognosis for future craziness reduction
>> uncertain. Comments:
>>
>> The documentation lists several documented limitations that I would
>> like to analyze a little bit. First, it says that row-level security
>> policies are not applied on UPDATE or DELETE. That sounds downright
>> dangerous to me. Is there some really compelling reason we're not
>> doing it?
>
> It intends to simplify the patch to avoid doing everything within a single
> patch. I'll submit the patch supporting UPDATE and DELETE for CF-Nov
> in addition to the base one.
>
>> Second, it says that row-level security policies are not
>> currently applied on INSERT, so you should use a trigger, but implies
>> that this will change in the future. I don't think we should change
>> that in the future; I think relying on triggers for that case is just
>> fine. Note that it could be an issue with the post-image for UPDATES,
>> as well, and I think the trigger solution is similarly adequate to
>> cover that case.
>
> Hmm. I should not have written this in section of the current limitation.
> It may give impression the behavior will be changed future.
> OK, I'll try to revise the documentation stuff.
>
>> With respect to the documented limitation regarding
>> DECLARE/FETCH, what exactly will happen? Can we describe this a bit
>> more clearly rather than just saying the behavior will be
>> unpredictable?
>>
> In case when user-id was switched after declaration of a cursor that
> contains qualifier depending on current_user, its results set contains
> rows with old user-id and rows with new user-id.
>
> Here is one other option rather than documentation fix.
> As we had a discussion on the upthread, it can be solved if we restore
> the user-id associated with the portal to be run, however, a problem is
> some commands switches user-id inside of the portal.
> http://archives.postgresql.org/pgsql-hackers/2012-07/msg00055.php
>
> Is there some good idea to avoid the problem?
>
>> It looks suspiciously as if the row-level security mode needs to be
>> saved and restored in all the same places we call save and restore the
>> user ID and security context. Is there some reason the
>> row-level-security-enabled flag shouldn't just become another bit in
>> the security context? Then we'd get all of this save/restore logic
>> mostly for free.
>>
> It seems to me a good idea, but I didn't find out this.
>
>> ATExecSetRowLevelSecurity() calls SetRowLevelSecurity() or
>> ResetRowLevelSecurity() to update pg_rowlevelsec, but does the
>> pg_class update itself. I think that all of this logic should be
>> moved into a single function, or at least functions in the same file,
>> with the one that only updates pg_rowlevelsec being static and
>> therefore not able to be called from outside the file. We always need
>> the pg_class update and the pg_rowlevelsec update to happen together,
>> so it's not good to have an exposed function that does one of those
>> updates but not the other. I think the simplest thing is just to move
>> ATExecSetRowLevelSecurity to pg_rowlevelsec.c and rename it to
>> SetRowLevelSecurity() and then give it two static helper functions,
>> say InsertPolicyRow() and DeletePolicyRow().
>>
> OK, I'll rework the code.
>
>> I think it would be good if Tom could review the query-rewriting parts
>> of this (viz rowlevelsec.c) as I am not terribly familiar with this
>> machinery, and of course anything we get wrong here will have security
>> consequences. At first blush, I'm somewhat concerned about the fact
>> that we're trying to do this after query rewriting; that seems like it
>> could break things. I know KaiGai mentioned upthread that the
>> rewriter won't be rerun if the plan is invalidated, but (1) why is
>> that OK now? and (2) if it is OK now, then why is it OK to do
>> rewriting of the RLS qual - only - after rewriting if all of the rest
>> of the rewriting needs to happen earlier?
>>
> I just follow the existing behavior of plan invalidation; that does not
> re-run the query rewriter. So, if we have no particular reason why
> we should not run the rewriter again to handle RLS quals, it might
> be an option to handle RLS as a part of rewriter.
>
> At least, here is two problems. 1) System column is problematic
> when SELECT statement is replaced by sub-query. 2) It makes
> infinite recursion when a certain table has SELECT INSTEAD
> rule with a sub-query on the same table.
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

> However, UPDATE / DELETE support is not perfect right now.
> In case when we try to update / delete a table with inherited
> children and RETURNING clause was added, is loses right
> references to the pseudo columns, even though it works fine
> without inherited children.
>
The attached patch fixed this known problem.

I oversight that inheritance_planner() fixup Var->varno when
it references a sub-query; even if it originated from regular
table with row-level security policy.

In case when system-column or whole-row of the table with
row-level security policy referenced, rowlevelsec.c adds
relevant target-entries on the sub-query that wraps this
table-reference, and "varattno" of Var node towards system-
columns is adjusted later.
However, we need to treat RETURNING clause in a special way
because its Var node is evaluated at ExecUpdate or ExecDelete,
therefore, its attribute number should indicate raw-table, not
scanned virtual tuple on sub-query.

So, I added a logic to keep Var->varattno when it tries to reference
either system-column or whole-row of the replaced tables due to
row-level security.

Thanks,

2012/11/15 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> The attached patch is a revised version of row-level security
> feature.
> According to Robert's suggestion, I reworked implementation
> around ALTER command, and logic to disable RLS during
> FK/PK constraint checks.
>
> In addition, I moved the entrypoint to apply row-level security
> policy on the query tree next to the expand_inherited_tables,
> because it became clear my previous approach is not
> a straight-forward way to support update / delete cases.
>
> This patch performs to replace RangeTblEntry of tables with
> RLS policy by sub-queries that simply references the original
> table with configured RLS policy. Also, the sub-queries have
> security_barrier flag to prevent non-leakproof functions being
> pushed down from outside of the sub-query.
>
> This sub-query has target-list that just references columns of
> underlying table, and ordered according to column definition
> of the original table. So, we don't need to adjust varattno of
> Var-node that reference regular columns, even though the
> RangeTblEntry was replaced.
> On the other hand, system-column is problematic because
> sub-query does not have these columns due to nature of them.
> So, I inject a logic to adjust varattno of Var-node that references
> system-column of the target tables being replaced.
> It works fine as follows:
>
> postgres=> ALTER TABLE t1 SET ROW LEVEL SECURITY (a % 2 = 0);
> ALTER TABLE
> postgres=> ALTER TABLE t2 SET ROW LEVEL SECURITY (a % 2 = 1);
> ALTER TABLE
> postgres=> EXPLAIN (costs off) SELECT tableoid, * FROM t1 WHERE b like '%';
> QUERY PLAN
> -------------------------------------------
> Result
> -> Append
> -> Subquery Scan on t1
> Filter: (t1.b ~~ '%'::text)
> -> Seq Scan on t1 t1_1
> Filter: ((a % 2) = 0)
> -> Subquery Scan on t2
> Filter: (t2.b ~~ '%'::text)
> -> Seq Scan on t2 t2_1
> Filter: ((a % 2) = 1)
> -> Seq Scan on t3
> Filter: (b ~~ '%'::text)
> (12 rows)
>
> postgres=> SELECT tableoid, * FROM t1 WHERE b like '%';
> tableoid | a | b
> ----------+----+-----
> 16385 | 2 | bbb
> 16385 | 4 | ddd
> 16385 | 6 | fff
> 16391 | 11 | sss
> 16391 | 13 | uuu
> 16391 | 15 | yyy
> 16397 | 21 | xyz
> 16397 | 22 | yzx
> 16397 | 23 | zxy
> (9 rows)
>
> Also, UPDATE / DELETE statement
>
> postgres=> EXPLAIN (costs off) UPDATE t1 SET b = b || '_updt' WHERE b like '%';
> QUERY PLAN
> -------------------------------------
> Update on t1
> -> Subquery Scan on t1
> Filter: (t1.b ~~ '%'::text)
> -> Seq Scan on t1 t1_1
> Filter: ((a % 2) = 0)
> -> Subquery Scan on t2
> Filter: (t2.b ~~ '%'::text)
> -> Seq Scan on t2 t2_1
> Filter: ((a % 2) = 1)
> -> Seq Scan on t3
> Filter: (b ~~ '%'::text)
> (11 rows)
>
> postgres=> UPDATE t1 SET b = b || '_updt' WHERE b like '%';
> UPDATE 9
>
> However, UPDATE / DELETE support is not perfect right now.
> In case when we try to update / delete a table with inherited
> children and RETURNING clause was added, is loses right
> references to the pseudo columns, even though it works fine
> without inherited children.
>
> postgres=> UPDATE only t1 SET b = b || '_updt' WHERE b like '%' RETURNING *;
> a | b
> ---+----------
> 2 | bbb_updt
> 4 | ddd_updt
> 6 | fff_updt
> (3 rows)
>
> UPDATE 3
> postgres=> UPDATE t1 SET b = b || '_updt' WHERE b like '%' RETURNING *;
> ERROR: variable not found in subplan target lists
>
> I'm still under investigation of this behavior. Any comments
> will be helpful to solve this problem.
>
> Thanks,
>
> 2012/10/22 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>> 2012/10/22 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> On Thu, Oct 18, 2012 at 2:19 PM, Alvaro Herrera
>>> <alvherre(at)2ndquadrant(dot)com> wrote:
>>>> Kohei KaiGai escribió:
>>>>> The revised patch fixes the problem that Daen pointed out.
>>>>
>>>> Robert, would you be able to give this latest version of the patch a
>>>> look?
>>>
>>> Yeah, sorry I've been completely sidelined this CommitFest. It's been
>>> a crazy couple of months. Prognosis for future craziness reduction
>>> uncertain. Comments:
>>>
>>> The documentation lists several documented limitations that I would
>>> like to analyze a little bit. First, it says that row-level security
>>> policies are not applied on UPDATE or DELETE. That sounds downright
>>> dangerous to me. Is there some really compelling reason we're not
>>> doing it?
>>
>> It intends to simplify the patch to avoid doing everything within a single
>> patch. I'll submit the patch supporting UPDATE and DELETE for CF-Nov
>> in addition to the base one.
>>
>>> Second, it says that row-level security policies are not
>>> currently applied on INSERT, so you should use a trigger, but implies
>>> that this will change in the future. I don't think we should change
>>> that in the future; I think relying on triggers for that case is just
>>> fine. Note that it could be an issue with the post-image for UPDATES,
>>> as well, and I think the trigger solution is similarly adequate to
>>> cover that case.
>>
>> Hmm. I should not have written this in section of the current limitation.
>> It may give impression the behavior will be changed future.
>> OK, I'll try to revise the documentation stuff.
>>
>>> With respect to the documented limitation regarding
>>> DECLARE/FETCH, what exactly will happen? Can we describe this a bit
>>> more clearly rather than just saying the behavior will be
>>> unpredictable?
>>>
>> In case when user-id was switched after declaration of a cursor that
>> contains qualifier depending on current_user, its results set contains
>> rows with old user-id and rows with new user-id.
>>
>> Here is one other option rather than documentation fix.
>> As we had a discussion on the upthread, it can be solved if we restore
>> the user-id associated with the portal to be run, however, a problem is
>> some commands switches user-id inside of the portal.
>> http://archives.postgresql.org/pgsql-hackers/2012-07/msg00055.php
>>
>> Is there some good idea to avoid the problem?
>>
>>> It looks suspiciously as if the row-level security mode needs to be
>>> saved and restored in all the same places we call save and restore the
>>> user ID and security context. Is there some reason the
>>> row-level-security-enabled flag shouldn't just become another bit in
>>> the security context? Then we'd get all of this save/restore logic
>>> mostly for free.
>>>
>> It seems to me a good idea, but I didn't find out this.
>>
>>> ATExecSetRowLevelSecurity() calls SetRowLevelSecurity() or
>>> ResetRowLevelSecurity() to update pg_rowlevelsec, but does the
>>> pg_class update itself. I think that all of this logic should be
>>> moved into a single function, or at least functions in the same file,
>>> with the one that only updates pg_rowlevelsec being static and
>>> therefore not able to be called from outside the file. We always need
>>> the pg_class update and the pg_rowlevelsec update to happen together,
>>> so it's not good to have an exposed function that does one of those
>>> updates but not the other. I think the simplest thing is just to move
>>> ATExecSetRowLevelSecurity to pg_rowlevelsec.c and rename it to
>>> SetRowLevelSecurity() and then give it two static helper functions,
>>> say InsertPolicyRow() and DeletePolicyRow().
>>>
>> OK, I'll rework the code.
>>
>>> I think it would be good if Tom could review the query-rewriting parts
>>> of this (viz rowlevelsec.c) as I am not terribly familiar with this
>>> machinery, and of course anything we get wrong here will have security
>>> consequences. At first blush, I'm somewhat concerned about the fact
>>> that we're trying to do this after query rewriting; that seems like it
>>> could break things. I know KaiGai mentioned upthread that the
>>> rewriter won't be rerun if the plan is invalidated, but (1) why is
>>> that OK now? and (2) if it is OK now, then why is it OK to do
>>> rewriting of the RLS qual - only - after rewriting if all of the rest
>>> of the rewriting needs to happen earlier?
>>>
>> I just follow the existing behavior of plan invalidation; that does not
>> re-run the query rewriter. So, if we have no particular reason why
>> we should not run the rewriter again to handle RLS quals, it might
>> be an option to handle RLS as a part of rewriter.
>>
>> At least, here is two problems. 1) System column is problematic
>> when SELECT statement is replaced by sub-query. 2) It makes
>> infinite recursion when a certain table has SELECT INSTEAD
>> rule with a sub-query on the same table.
>>
>> Thanks,
>> --
>> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>
>
>
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sun, Nov 25, 2012 at 03:20:28PM +0100, Kohei KaiGai wrote:
> > However, UPDATE / DELETE support is not perfect right now.
> > In case when we try to update / delete a table with inherited
> > children and RETURNING clause was added, is loses right
> > references to the pseudo columns, even though it works fine
> > without inherited children.
> >
> The attached patch fixed this known problem.

This patch no longer applies to git master. Any chance of a rebase?

Also, might this approach work for the catalog? The use case I have
in mind is multi-tenancy, although one can imagine organizations where
internal access controls might be required on it, too.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

2012/12/3 David Fetter <david(at)fetter(dot)org>:
> On Sun, Nov 25, 2012 at 03:20:28PM +0100, Kohei KaiGai wrote:
>> > However, UPDATE / DELETE support is not perfect right now.
>> > In case when we try to update / delete a table with inherited
>> > children and RETURNING clause was added, is loses right
>> > references to the pseudo columns, even though it works fine
>> > without inherited children.
>> >
>> The attached patch fixed this known problem.
>
> This patch no longer applies to git master. Any chance of a rebase?
>
OK, I'll rebese it.

> Also, might this approach work for the catalog? The use case I have
> in mind is multi-tenancy, although one can imagine organizations where
> internal access controls might be required on it, too.
>
If you intend to control behavior of DDL commands that internally takes
access towards system catalog, RLS feature is not helpful.
Please use sepgsql instead. :-)
If you intend to control DML commands towards system catalogs, here
is nothing special, so I expect it works as we are doing at user tables.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On 3 December 2012 15:36, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2012/12/3 David Fetter <david(at)fetter(dot)org>:
>> On Sun, Nov 25, 2012 at 03:20:28PM +0100, Kohei KaiGai wrote:
>>> > However, UPDATE / DELETE support is not perfect right now.
>>> > In case when we try to update / delete a table with inherited
>>> > children and RETURNING clause was added, is loses right
>>> > references to the pseudo columns, even though it works fine
>>> > without inherited children.
>>> >
>>> The attached patch fixed this known problem.
>>
>> This patch no longer applies to git master. Any chance of a rebase?
>>
> OK, I'll rebese it.

No chunk failures, its just fuzz.

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

On Mon, Dec 03, 2012 at 03:41:47PM +0000, Simon Riggs wrote:
> On 3 December 2012 15:36, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> > 2012/12/3 David Fetter <david(at)fetter(dot)org>:
> >> On Sun, Nov 25, 2012 at 03:20:28PM +0100, Kohei KaiGai wrote:
> >>> > However, UPDATE / DELETE support is not perfect right now.
> >>> > In case when we try to update / delete a table with inherited
> >>> > children and RETURNING clause was added, is loses right
> >>> > references to the pseudo columns, even though it works fine
> >>> > without inherited children.
> >>> >
> >>> The attached patch fixed this known problem.
> >>
> >> This patch no longer applies to git master. Any chance of a rebase?
> >>
> > OK, I'll rebese it.
>
> No chunk failures, its just fuzz.

I must have done something wrong.

I downloaded the patch from the web email archives, then ran "git
apply" on it, and got this:

$ git apply ../pgsql-v9.3-row-level-security.rw.v7.patch
../pgsql-v9.3-row-level-security.rw.v7.patch:806: space before tab in indent.
* row-level security policy
../pgsql-v9.3-row-level-security.rw.v7.patch:1909: trailing whitespace.
* would be given. Because the sub-query has security barrier flag,
../pgsql-v9.3-row-level-security.rw.v7.patch:2886: trailing whitespace.
did | cid | dlevel | dauthor | dtitle
../pgsql-v9.3-row-level-security.rw.v7.patch:2899: trailing whitespace.
cid | did | dlevel | dauthor | dtitle | cname
../pgsql-v9.3-row-level-security.rw.v7.patch:2918: trailing whitespace.
did | cid | dlevel | dauthor | dtitle
error: patch failed: src/backend/commands/copy.c:34
error: src/backend/commands/copy.c: patch does not apply

What did I do wrong here?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter escribió:
> On Mon, Dec 03, 2012 at 03:41:47PM +0000, Simon Riggs wrote:
> > On 3 December 2012 15:36, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> > > 2012/12/3 David Fetter <david(at)fetter(dot)org>:
> > >> On Sun, Nov 25, 2012 at 03:20:28PM +0100, Kohei KaiGai wrote:
> > >>> > However, UPDATE / DELETE support is not perfect right now.
> > >>> > In case when we try to update / delete a table with inherited
> > >>> > children and RETURNING clause was added, is loses right
> > >>> > references to the pseudo columns, even though it works fine
> > >>> > without inherited children.
> > >>> >
> > >>> The attached patch fixed this known problem.
> > >>
> > >> This patch no longer applies to git master. Any chance of a rebase?
> > >>
> > > OK, I'll rebese it.
> >
> > No chunk failures, its just fuzz.
>
> I must have done something wrong.
>
> I downloaded the patch from the web email archives, then ran "git
> apply" on it, and got this:

git apply is much stricter than other tools; if the patch requires a
merge, it doesn't try. Try applying it with "patch -p1 < /path/to/patch"
(this is what I always use)

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

On 15 November 2012 21:07, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:

> The attached patch is a revised version of row-level security
> feature.

I've got time to review this patch, so I've added myself as a CF reviewer.

Definitely looks very interesting, well done for getting it this far along.

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