Re: [RFC] Interface of Row Level Security

2012/5/24 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, May 24, 2012 at 6:11 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> Perhaps when we see that RLS
>>> applies, we should replace the reference to the original table with a
>>> subquery RTE that has the security_barrier flag set - essentially
>>> treating a table with RLS as if it were a security view.
>>>
>> I become to think it is a better approach than tracking origin of each
>> qualifiers. One problem is case handling on update or delete statement.
>>
>> It may be possible to rewrite the update / delete query as follows:
>>
>> From:
>>  UPDATE tbl SET X = X + 1 WHERE f_leak(Y)
>> To:
>>  UPDATE tbl SET X = X + 1 WHERE ctid = (
>>      SELECT * FROM (
>>          SELECT ctid FROM tbl WHERE uname = getpgusername()  <== (*)
>> should have security-barrier
>>      ) AS tbl_subqry WHERE f_leak(Y)
>>  );
>>
>> Expanded sub-queries will have security-barrier flag, so it enforces
>> the "uname = getpgusername()" being checked earlier than f_leak(Y).
>> We may need to measure the performance impact due to the reform.
>
> The problem with this is that it introduces an extra instance of tbl
> into the query - there are now two rather than one.  UPDATE .. FROM is
> supposed to be a way to avoid this, but it's insufficiently general to
> handle all the cases (e.g. UPDATE a LEFT JOIN b can't be written using
> the existing syntax).  Anyway we want to avoid inserting self-joins
> for performance reasons if at all possible.  It should be easy to do
> that in the case of SELECT; UPDATE and DELETE may need a bit more
> work.
>
I'll try to investigate a way to solve the matter without twice scan.
Right now, I have no reasonable ideas. Please give us suggestion
if you have something...

>> I think, this situation is similar to a case when we reference a view
>> without privileges to underlying tables. If Bob set up a view with
>> something "tricky" function, it allows Bob to reference credentials
>> of users who reference the view.
>> More or less, it might be a problem when a user try to invoke
>> a user defined function declared by others.
>> (Thus, sepgsql policy does not allow users to invoke a function
>> declared by another one in different domain; without DBA's checks.)
>
> This is true, but there are still some new threat models.  For
> example, currently, pg_dump isn't going to run any user-defined code
> just because you do SELECT * FROM table, but that will change with
> this patch.  Note that pg_dump need not actually select from views,
> only tables.
>
>> I think it is a good idea not to apply RLS when current user has
>> superuser privilege from perspective of security model consistency,
>> but it is inconsistent to check privileges underlying tables.
>
> Seems like a somewhat random wart, if it's just an exception for
> superusers.  I think we need to do better than that.  For example, at
> my last company, sales reps A and B were permitted to see all
> customers of the company, but sales reps C, D, E, F, G, H, I, and J
> were permitted to see only their own accounts.  Those sorts of
> policies need to be easy to implement.
>
Probably, if "sales_rep" column records its responsible repo, its
security policy is able to be described as:
(my_sales_rep() in ('A', 'B') OR sales_rep = my_sales_rep())

Indeed, the design to check underlying table seems to me like
column-level privileges towards table-level privileges, since it
is checked only when user does not have requires privileges
on whole of the table.
However, I have no idea to modify ExecCheckRTEPerms()
regarding to RLS. If we assume RLS is applied when user has
no privileges on tables, the current ExecCheckRTEPerms()
always raises an error towards unprivileged users, prior to
execution of queries.
Isn't it preferable behavior to allow unprivileged users to
reference a table (or columns) when it has RLS policy?

I think, table and column level privilege should be checked
individually, in addition to row-level security policy.

>>> Another idea is to set things up so that the RLS policy function isn't
>>> applied to each row directly; instead, it's invoked once per query and
>>> *returns* a WHERE clause.  This would be a lot more powerful than the
>>> proposed design, because now the table owner can write a function that
>>> imposes quals on some people but not others, which seems very useful.
>>>
>> Sorry, I don't favor this idea. Even if table owner set up a function to
>> generate additional qualifiers, it also has no guarantee the qualifiers
>> are invoked prior to user-given one.
>> It seems to me this approach will have same problem...
>
> It's not intended to solve the qual-ordering problem, just to allow
> additional policy flexibility.
>
At the beginning, I thought it takes complex code to parse
where-clause being provided as security policy, so it is the
reason why I was inclined to give a function, instead of a clause.
But I noticed we already have similar code at CreateTrigger()
to handle it.
Does it give policy flexibility?

>>> It's not clear to me that there is any need for built-in server
>>> functionality here.  If the table owner wants to enforce some sort of
>>> policy regarding INSERT or UPDATE or DELETE, they can already do that
>>> today just by attaching a trigger to the table.  And they can enforce
>>> whatever policy they like that way.  Before designing any new
>>> mechanism, what's wrong with the existing one?
>>>
>> Yes, we don't need any new invent to check the value of new tuples.
>> But it should be done after all the user-defined triggers. Existing
>> trigger does not have a mechanism to enforce order to be invoked.
>> So, what I really implement is a mechanism to inject some pseudo
>> triggers "at tail of the Trigger array".
>
> Start the trigger names with the letter "z".
>
OK, I'd like to postpone this future from the 1st stage.

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