Re: [RFC] Interface of Row Level Security - Mailing list pgsql-hackers

2012/6/5 Florian Pflug <fgp@phlo.org>:
> On Jun5, 2012, at 15:07 , Kohei KaiGai wrote:
>> 2012/6/5 Florian Pflug <fgp@phlo.org>:
>>> On Jun5, 2012, at 11:43 , Kohei KaiGai wrote:
>>>> I think it does not require to add a mechanism to invalidate
>>>> prepared-statement, because all the checks are applied on
>>>> executor stage. And these functions can be stable functions,
>>>> so I believe some enhancements at planner will correctly wipe
>>>> out prior to query execution at the next step.
>
>>> That won't work, since the current role can change any time mid-query,
>>> at least if you're using cursors. Here's an example
>
> <snipped example>
>
>>> Imagine that the current_user function call was actually part of the
>>> RLS policy. Then the example above shows that *which* role we check
>>> access against (the one active at the first fetch, or currently active
>>> one) depends on which plan the optimizer happens to pick. This, IMHO,
>>> is not acceptable for a security feature like RLS.
>
>> Which is your expected behavior in this example?
>
> I can live with any behaviour, as long as it doesn't depends on details
> of the query plan. My vote would be for always using the role which was
> active at statement creation time (i.e. at PREPARE/DECLARE time) for
> RLS purposes though. That way, we could treat the role as being IMMUTABLE
> I think.
>
I have same opinion at the point where the "active role" should be consistent,
but a bit different in details. I think, it should be a timing at
beginning of execution,
not plan creation time, like as existing permission checks are applied
on InitPlan()
or its callee.
So, I believe EXECUTE is more right place to check it rather than PREPARE.
Although FETCH command actually handles query execution, do we ought to
consider DECLARE do both of query plan and execution from the perspective
of model?

>> It seems to me, caution of the problem is current_user is performing out
>> of the snapshot controls. According to the definition of stable function,
>> its result should not be changed during a particular scan.
>
> Yeah, well, ISTM that our definition of STABLE doesn't really take functions
> whose return value depends on GUCs into account.
>
Probably, has_XXX_privilege() should work with reliable data source being
safe from user-id switching. But it is a tough work to enhance whole of the
GUC stuff for snapshot aware...

>> At least, it is not a fundamental problem of RLS implementation, although
>> it needs to take an enhancement something like effective user-id per
>> snapshot basis.
>
> Right. We need something like "statement_role" which returns the role the
> statement is to be executed as (However we define that, though as I said
> above my vote is for it being the role which created the statement). To make
> that work, however, the plan cache needs to store that role, since the
> statement might be re-planned after its initial creation, but before it is
> executed. At which point the amount of complexity saved by not implementing
> RLSBYPASS is very nearly zero.
>
I'm not sure why the plan cache needs to be stored with a particular user-id.
As long as we check permissions at executor stage, all we need to do is
implementation of a function to check permission based on a particular
user-id at the timing of executor initialization.

And, I think it can be a separated efforts from the first version of RLS.

> To reiterate, my point is that we won't get away with zero planner changes
> even without RLSBYPASS. In fact, we'll be paying *most* of the complexity
> costs of RLSBYPASS whether we actually add that feature or not. Which makes
> not adding it look like a pretty bad deal to me.
>
It seems to me, 95% of our opinion is common, except for a few detailed stuff.
I never dislike the idea of RLSBYPASS permission, but I'd like to investigate
this idea with more time, until v9.3 being closed.
At least, nobody opposed to allow superusers to bypass RLS policy.
So, it can be a minimum startup point, isn't it? RLS patch will takes hundreds
of lines due to syntax support or query rewriting stuff. It is always good idea
to focus on the core functionality at the starting point.

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