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

From: Craig Ringer <craig(at)2ndquadrant(dot)com>
To: Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
Cc: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>, Robert Haas <robertmhaas(at)gmail(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>
Subject: Re: WIP patch (v2) for updatable security barrier views
Date: 2014-01-30 05:36:22
Message-ID: 52E9E4D6.7070005@2ndquadrant.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 01/29/2014 08:29 PM, Dean Rasheed wrote:
> On 29 January 2014 11:34, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> On 01/23/2014 06:06 PM, Dean Rasheed wrote:
>>> On 21 January 2014 09:18, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)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.

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.

This isn't triggered by FOR SHARE / FOR UPDATE rowmark on a security
barrier view because the flattening to securityQuals and re-expansion in
the optimizer is only done for _target_ security barrier views. For
target views, different rowmark handling applies, so they don't trigger
it either.

This is triggered by adding securityQuals to a non-target relation
during row-security processing, though.

> Sounds sensible. Feel free to add any test cases you think up to the
> wiki page. Even if we don't like this design, any alternative must at
> least pass all the tests listed there.

Eh, much better to add directly to src/regress/ IMO. If they're on the
wiki they can get overlooked/forgotten.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachment Content-Type Size
relative-upd-sb-views-fixup-rowmarks.diff text/x-patch 5.1 KB
0001-Updatable-S-B-Views-Dean-With-RowMark-Fixes.diff text/x-patch 56.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Fetter 2014-01-30 06:14:30 Re: GSoC 2014 - mentors, students and admins
Previous Message Craig Ringer 2014-01-30 05:25:03 Re: Row-security on updatable s.b. views