Re: [v9.3] Row-Level Security

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>