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

On 8 January 2014 09:02, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> Dean,
>
> Short version
> -------------
>
> Looks amazing overall. Very clever to zip up the s.b. quals, let the
> rest of the rewriter and planer do their work normally, then unpack them
> into subqueries inserted in the planner once inheritance appendrels are
> expanded, etc.
>
> My main concern is that the securityQuals appear to bypass all later
> rewrite stages, inheritance expansion during planning, etc. I suspect
> this might be hard to get around (because these are disembodied quals
> which may have nonsense varnos), but I'm looking into it now.
>
> There's also an assertion failure whenever a correlated subquery appears
> as a security barrier view qual. Again, looking at it.
>
> Ideas on that issue?
>
>
>
> Much longer version: My understanding of how it works
> -----------------------------------------------------
>
> My understanding from reading the patch is that this:
>
> - Flattens target views in rewriteTargetView, as in current master. If
> the target view is a security barrier view, the view quals are appended
> to a list of security barrier quals on the new RTE, instead of appended
> to the RTE's normal quals like for normal views.
>
> After rewrite the views are fully flattened down to a RTE_RELATION,
> which becomes the resultRelation. An unreferenced RTE for each view
> that's been rewritten is preserved in the range-table for permissions
> checking purposes only (same as current master).
>
> - Inheritance expansion, tlist expansion, etc then occurrs as normal.
>
> - In planning, in inheritance_planner, if any RTE has any stashed
> security quals in its RangeTableEntry, expand_security_qual is invoked.
> This iteratively wraps the base relation in a subquery with the saved
> security barrier quals, creating nested subqueries around the original
> RTE. At each pass resultRelation is changed to point to the new
> outer-most subquery.
>
>
> As a result of this approach everything looks normal to
> preprocess_targetlist, row-marking, etc, because they're seeing a normal
> RTE_RELATION as resultRelation. The security barrier quals are, at this
> stage, stashed aside. If there's inheritance involved, RTEs copied
> during appendrel expansion get copies of the security quals on in the
> parent RTE.
>
> Problem with inheritance, views, etc in s.b. quals
> --------------------------------------------------
>
> After inheritance expansion, tlist expansion, etc, the s.b. quals are
> unpacked to create subqueries wrapping the original RTEs.
>
>
> So, with:
>
> CREATE TABLE t1 (x float, b integer, secret1 text, secret2 text);
> CREATE TABLE t1child (z integer) INHERITS (t1);
>
> INSERT INTO t1 (x, b, secret1, secret2)
> VALUES
> (0,0,'secret0', 'supersecret'),
> (1,1,'secret1', 'supersecret'),
> (2,2,'secret2', 'supersecret'),
> (3,3,'secret3', 'supersecret'),
> (4,4,'secret4', 'supersecret'),
> (5,6,'secret5', 'supersecret');
>
> INSERT INTO t1child (x, b, secret1, secret2, z)
> VALUES
> (8,8,'secret8', 'ss', 8),
> (9,9,'secret8', 'ss', 9),
> (10,10,'secret8', 'ss', 10);
>
> CREATE VIEW v1
> WITH (security_barrier)
> AS
> SELECT b AS b1, x AS x1, secret1
> FROM t1 WHERE b % 2 = 0;
>
> CREATE VIEW v2
> WITH (security_barrier)
> AS
> SELECT b1 AS b2, x1 AS x2
> FROM v1 WHERE b1 % 4 = 0;
>
>
>
> then a statement like:
>
> UPDATE v2
> SET x2 = x2 + 32;
>
> will be rewritten into something like (imaginary sql)
>
> UPDATE t1 WITH SECURITY QUALS ((b % 2 == 0), (b % 4 == 0))
> SET x = x + 32
>
> inheritance-expanded and tlist-expanded into something like (imaginary SQL)
>
>
> UPDATE
> (t1 WITH SECURITY QUALS ((b % 2 == 0), (b % 4 == 0)))
> UNION ALL
> (t1child WITH SECURITY QUALS ((b % 2 == 0), (b % 4 == 0)))
> SET x = x + 32;
>
>
> after which security qual expansion occurs, giving us something like:
>
>
> UPDATE
> t1, t1child <--- resultRelations
> (
> SELECT v2.ctid, v2.*
> FROM (
> SELECT v1.ctid, v1.*
> FROM (
> SELECT t1.ctid, t1.*
> FROM t1
> WHERE b % 2 == 0
> ) v1
> WHERE b % 4 == 0
> ) v2
>
> UNION ALL
>
> SELECT v2.ctid, v2.*
> FROM (
> SELECT v1.ctid, v1.*
> FROM (
> SELECT t1child.ctid, t1child.*
> FROM t1child
> WHERE b % 2 == 0
> ) v1
> WHERE b % 4 == 0
> ) v2
>
> )
> SET x = x + 32;
>
>
> Giving a plan looking like:
>
> EXPLAIN UPDATE v2 SET x2 = 32
>
>
> QUERY PLAN
> ---------------------------------------------------------------------------
> Update on t1 t1_2 (cost=0.00..23.35 rows=2 width=76)
> -> Subquery Scan on t1 (cost=0.00..2.18 rows=1 width=74)
> -> Subquery Scan on t1_3 (cost=0.00..2.17 rows=1 width=74)
> Filter: ((t1_3.b % 4) = 0)
> -> Seq Scan on t1 t1_4 (cost=0.00..2.16 rows=1 width=74)
> Filter: ((b % 2) = 0)
> -> Subquery Scan on t1_1 (cost=0.00..21.17 rows=1 width=78)
> -> Subquery Scan on t1_5 (cost=0.00..21.16 rows=1 width=78)
> Filter: ((t1_5.b % 4) = 0)
> -> Seq Scan on t1child (cost=0.00..21.10 rows=4 width=78)
> Filter: ((b % 2) = 0)
> (11 rows)
>
>
>
>
> So far this looks like a really clever approach. My only real concern is
> that the security quals are currently hidden from rewrite and parsing
> before during the period they're hidden away in the security quals RTI
> field.
>
> This means they aren't processed for things like inheritance expansion. e.g.
>
> CREATE TABLE rowfilter (remainder integer, userid text);
> CREATE TABLE rowfilterchild () INHERITS (rowfilter);
> INSERT INTO rowfilterchild(remainder, userid) values (0, current_user);
>
> a view with a security qual that refers to an inherited relation won't work:
>
> CREATE VIEW sqv
> WITH (security_barrier)
> AS
> SELECT x, b FROM t1 WHERE (
> SELECT b % 4 = remainder
> FROM rowfilter
> WHERE userid = current_user
> OFFSET 0
> );
>
> This is a bit contrived to force the optimiser to treat the subquery as
> correlated and thus make sure the ref to rowfilter gets into the
> securityQuals list.
>
> I expected zero results (a scan of rowfilter, but not rowfilterchild,
> resulting in a null subquery return) but land up with an assertion
> failure instead. The assertion triggers for any security qual containing
> a correlated subquery, so it's crashing out before we can break due to
> failure to expand inheritance.
>
>
> This isn't just about inheritance. In general, we'd need a way to
> process those securityQuals through any rewrite phases and through the
> parts of planning before they get merged back in, so they're subject to
> things like inheritance appendrel expansion.
>
> Same if the security qual contains a view ref:
>
> CREATE VIEW dumbview(zero)
> AS SELECT 0;
>
> CREATE VIEW sqv2
> WITH (security_barrier)
> AS
> SELECT x, b FROM t1
> WHERE (SELECT b % 2 = zero FROM dumbview OFFSET 0);
>

Thanks for testing, and good catch on the sublinks. There was a
trivial bug in my new code in rewriteTargetView() --- it needs to
check the added security barrier qual for sublinks and mark the parent
query accordingly. After that the rewriter will descend into the
sublinks on the security barrier quals expanding any views they
contain, so the fix for that part is trivial (see the attached
update).

The assertion failure with inheritance and sublinks is a separate
issue --- adjust_appendrel_attrs() is not expecting to find any
unplanned sublinks in the query tree when it is invoked, since they
would normally have all been planned by that point. However, the
addition of the new security barrier subqueries after inheritance
expansion can now insert new sublinks which need to be planned. I'll
look into how best to make that happen.

Regards,
Dean