Re: WIP patch (v2) for updatable security barrier views

On 30 January 2014 05:36, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 01/29/2014 08:29 PM, Dean Rasheed wrote:
>> On 29 January 2014 11:34, Craig Ringer <craig@2ndquadrant.com> wrote:
>>> On 01/23/2014 06:06 PM, Dean Rasheed wrote:
>>>> On 21 January 2014 09:18, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>>>>> Yes, please review the patch from 09-Jan
>>>>> (http://www.postgresql.org/message-id/CAEZATCUiKxOg=vOOvjA2S6G-sixzzxg18ToTggP8zOBq6QnQHQ@mail.gmail.com).
>>>>>
>>>>
>>>> After further testing I found a bug --- it involves having a security
>>>> barrier view on top of a base relation that has a rule that rewrites
>>>> the query to have a different result relation, and possibly also a
>>>> different command type, so that the securityQuals are no longer on the
>>>> result relation, which is a code path not previously tested and the
>>>> rowmark handling was wrong. That's probably a pretty obscure case in
>>>> the context of security barrier views, but that code path would be
>>>> used much more commonly if RLS were built on top of this. Fortunately
>>>> the fix is trivial --- updated patch attached.
>>>
>>> This is the most recent patch I see, and the one I've been working on
>>> top of.
>>>
>>> Are there any known tests that this patch fails?
>>>
>>
>> None that I've been able to come up with.
>
> I've found an issue. I'm not sure if it can be triggered from SQL, but
> it affects in-code users who add their own securityQuals.
>

It's difficult to fix this kind of issue without a reproducible test
case, but...


> expand_security_quals fails to update any rowMarks that may point to a
> relation being expanded. If the relation isn't the target relation, this
> causes a rowmark to refer to a RTE with no relid, and thus failure in
> the executor.
>
> Relative patch against updatable s.b. views attached (for easier
> reading), along with a new revision of updatable s.b. views that
> incorporates the patch.
>

I don't like this fix --- you appear to be adding another RTE to the
rangetable (one not in the FROM list) and applying the rowmarks to it,
which seems wrong because you're not locking the right set of rows.
This is reflected in the change to the regression test output where,
in one of the tests, the ctids for the table to update are no longer
coming from the same table. I think a better approach is to push down
the rowmark into the subquery so that any locking required applies to
the pushed down RTE --- see the attached patch.

This is an alternative (and more general) fix to the problem I found
upthread (http://www.postgresql.org/message-id/CAEZATCVAqJV5WTjLmyObP21n+CzhbEx2AOzH4e6qmTcueVDjdQ@mail.gmail.com)
with rules on the base table, but hopefully it will solve the issue
you're seeing too.

It doesn't change any of the existing regression test output, but
neither does it add any new tests, since I can't see a way to provoke
your issue in isolation using SQL without first running into the
pre-existing bug with rules that turn DML into SELECT ... FOR UPDATE.

Anyway, please test if this works with your RLS code.

Regards,
Dean