Re: [v9.3] Row-Level Security

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