Re: [v9.2] Fix leaky-view problem, part 1

On Wed, Jun 29, 2011 at 05:05:22PM +0100, Kohei KaiGai wrote:
> 2011/6/28 Noah Misch <noah(at)leadboat(dot)com>:
> > On Tue, Jun 28, 2011 at 10:11:59PM +0200, Kohei KaiGai wrote:

> > CREATE VIEW a AS SELECT * FROM ta WHERE ac = 5;
> > ALTER VIEW a OWNER TO alice;
> > CREATE VIEW b AS SELECT * FROM tb WHERE bc = 6;
> > ALTER VIEW b OWNER TO bob;
> > SELECT * FROM a, b;
> >
> > Both the ac=5 and the bc=6 quals do run at the same depth despite enforcing
> > security for different principals. ?I can't think of a way that one view owner
> > could use this situation to subvert the security of the other view owner, but I
> > wanted to throw it out.
> >
> Even if view owner set a trap in his view, we have no way to reference variables
> come from outside of the view. In above example, even if I added f_leak() into
> the definition of VIEW A, we cannot give argument to reference VIEW B.

Good point. Yes, it should be rigorously safe on that account.

> > I was referring to this paragraph:
> >
> > ?On the technical side, I am pretty doubtful that the approach of adding a
> > ?nestlevel to FuncExpr and RelOptInfo is the right way to go. ?I believe we
> > ?have existing code (to handle left joins) that prevents quals from being
> > ?pushed down too far by fudging the set of relations that are supposedly needed
> > ?to evaluate the qual. ?I suspect a similar approach would work here.
> >
> It seems to me the later half of this paragraph is talking about the problem of
> unexpected qualifier pushing-down over the security barrier; I'm trying to solve
> the problem with the part.2 patch.
> The scenario the part.1 patch tries to solve is order to launch qualifiers, not
> unexpected pushing-down.

Okay, you're probably correct that it wasn't referring to the topic at hand.
I'm still suspicious of the silent assumption about how quals can be assigned
to plan nodes, but I don't have any concrete ideas for avoiding that.

> >> In addition, implementation will become complex, if both of qualifiers pulled-up
> >> from security barrier view and qualifiers pulled-up from regular views are mixed
> >> within a single qualifier list.
> >
> > I only scanned the part 2 patch, but isn't the bookkeeping already happening for
> > its purposes? ?How much more complexity would we get to apply the same strategy
> > to the behavior of this patch?
> >
> If conditional, what criteria we should have to reorder the quelifier?
> The current patch checks the depth at first, then it checks cost if same deptn.
> It is quite simple rule. I have no idea of the criteria to order the
> mixed qualifier
> come from security-barrier views and regular views.

Let's see. Every qual list will have some depth d such that all quals having
depth >= d are security-relevant, and all others are not security-relevant.
(This does not hold for all means of identifying security-relevant quals, but
it does hold for the CREATE SECURITY VIEW/reloptions strategy proposed in your
part 2 patch.) Suppose you track whether each Query node represents a
security view, then only increment the qualifier depth for such Query nodes,
rather than all Query nodes. The tracked depth then becomes a security
partition depth. Keep the actual sorting algorithm the same. (Disclaimer: I
haven't been thinking about this nearly as long as you have, so I may be
missing something relatively obvious.)

As it stands, the patch badly damages the performance of this example:

CREATE FUNCTION expensive(int) RETURNS boolean LANGUAGE sql
AS 'SELECT pg_sleep(1); SELECT true' COST 1000000;
CREATE TABLE t(c) AS SELECT * FROM generate_series(1,3);
EXPLAIN ANALYZE
SELECT * FROM (SELECT * FROM t WHERE expensive(c)) t0 WHERE c = 2;

That doesn't even use a view, let alone a security view. While I like the
patch's current simplicity, we need to narrow its impact.

Thanks,
nm