Re: [v9.3] Row-Level Security - Mailing list pgsql-hackers

> 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@kaigai.gr.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@kaigai.gr.jp>:
>> 2012/10/22 Robert Haas <robertmhaas@gmail.com>:
>>> On Thu, Oct 18, 2012 at 2:19 PM, Alvaro Herrera
>>> <alvherre@2ndquadrant.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@kaigai.gr.jp>
>
>
>
> --
> KaiGai Kohei <kaigai@kaigai.gr.jp>



--
KaiGai Kohei <kaigai@kaigai.gr.jp>