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

I'd like to summarize expected issues corresponding to leaky-view and RLS
towards v9.2, and PGcon2011/Developer Meeting.

We already made consensus the leaky-view is a problem to be fixed previous
to the row-level security feature.

We know several ways to leak/infer contents of tuples to be invisible using
a view that is defined to row-level security purpose.

[1] User defined functions with small-cost and side-effects (E.g error message)

If these functions are appended to WHERE clause, it may be executed earlier
than functions to be performed as row-level security policy of security views.
These function can take arguments that references either visible or invisible
tuples, so functions with side-effects enables to leak the contents of invisible
tuples, when it was invoked earlier than conditions to filter out.

[2] Iteration in proving PK/FK or UNIQUE constratins

This type of iteration enables to estimate invisible values. E.g, fails to insert
a tuple with a particular primary-key gives us a hint of existence of invisible
tuple. We made consensus that RLS does not handle this type of scenario called as
covert-channels. The point is user cannot see the hidden value directly.

[3] Reference to statistics delivered from table contents

One example was selectivity-estimator function; Tom mentioned about before.
The pg_statistic holds statistical information delivered from table contents,
so it may help users to infer the contents using its histograms.
The discussion didn't get hot at that time. However, the point of mine is same
as the reason why we don't handle covert-channels.
The statistical is irreversible translation from the original data, so we need
an intelligence process to infer them, not a direct data reference.

We also had a discussion about a principle what type of scenarios should be
considered as problem.
One was that we didn't consider it is not a problem if a function internally
references invisible rows, as long as it consumes them internally. Thus, we
considered index-access-methods can be launched earlier than functions to
filter out violated rows.
Second was that we didn't consider it is not a problem if RLS allows users
to infer invisible rows from the circumstances, as long as it is not possible
to dump invisible data directly, because its bandwidth to leak information
was quite slow. So, we decided not to handle covert-channel such as iteration
of PK/FK proving in RLS.

I still think the scenario [1] is the only problem to be solved prior to RLS
features. The scenario [2] and [3] don't allow to leak the original data
directly. In addition, the estimator function mentioned in [3] just references
statistical data internally. It is same situation with index-access-method.

Please give us the points at issue, if I now overlooked.

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)eu(dot)nec(dot)com>

Oops, I overlooked a scenario that we discussed before.

[4] Unexpected qualifier distributions into inside of join-loop
If a qualifier being appended on outside of join-loop references only one-side of the join,
the optimizer will unexpectedly distribute it into inside of the join-loop.
In the result, these exogenetic qualifiers may be evaluated earlier than security policy
functions inside of a view that contains join.

It also allows unprivileged users to reference contents of invisible tuples, so it shall be
a problem to be fixed up according to the principle I described below.

As a reference information, we had discussed the following approach:
We tried to extend CREATE VIEW statement to accept SECURITY attribute to prevent unexpected
qualifier distribution into inside of join-loop originated from security views.
In addition, we also tried to add a variable to track original nest-level of qualifiers to
order qualifiers in WHERE clause. It enabled to fix up [1] and [4].

However, the [3] was pointed out during the discussion and I was not sure where was the
essence of this issue. So, the patch was returned with feedbacks, then it got postponed
to v9.2 to solve this problem.

Thanks,

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of
> Kohei Kaigai
> Sent: 10. Mai 2011 12:56
> To: Robert Haas; Stephen Frost; Heikki Linnakangas; Tom Lane
> Cc: pgsql-hackers
> Subject: [HACKERS] [v9.2] Leaky view and RLS
>
> I'd like to summarize expected issues corresponding to leaky-view and RLS
> towards v9.2, and PGcon2011/Developer Meeting.
>
> We already made consensus the leaky-view is a problem to be fixed previous
> to the row-level security feature.
>
> We know several ways to leak/infer contents of tuples to be invisible using
> a view that is defined to row-level security purpose.
>
> [1] User defined functions with small-cost and side-effects (E.g error message)
>
> If these functions are appended to WHERE clause, it may be executed earlier
> than functions to be performed as row-level security policy of security views.
> These function can take arguments that references either visible or invisible
> tuples, so functions with side-effects enables to leak the contents of invisible
> tuples, when it was invoked earlier than conditions to filter out.
>
> [2] Iteration in proving PK/FK or UNIQUE constratins
>
> This type of iteration enables to estimate invisible values. E.g, fails to insert
> a tuple with a particular primary-key gives us a hint of existence of invisible
> tuple. We made consensus that RLS does not handle this type of scenario called as
> covert-channels. The point is user cannot see the hidden value directly.
>
> [3] Reference to statistics delivered from table contents
>
> One example was selectivity-estimator function; Tom mentioned about before.
> The pg_statistic holds statistical information delivered from table contents,
> so it may help users to infer the contents using its histograms.
> The discussion didn't get hot at that time. However, the point of mine is same
> as the reason why we don't handle covert-channels.
> The statistical is irreversible translation from the original data, so we need
> an intelligence process to infer them, not a direct data reference.
>
>
> We also had a discussion about a principle what type of scenarios should be
> considered as problem.
> One was that we didn't consider it is not a problem if a function internally
> references invisible rows, as long as it consumes them internally. Thus, we
> considered index-access-methods can be launched earlier than functions to
> filter out violated rows.
> Second was that we didn't consider it is not a problem if RLS allows users
> to infer invisible rows from the circumstances, as long as it is not possible
> to dump invisible data directly, because its bandwidth to leak information
> was quite slow. So, we decided not to handle covert-channel such as iteration
> of PK/FK proving in RLS.
>
> I still think the scenario [1] is the only problem to be solved prior to RLS
> features. The scenario [2] and [3] don't allow to leak the original data
> directly. In addition, the estimator function mentioned in [3] just references
> statistical data internally. It is same situation with index-access-method.
>
> Please give us the points at issue, if I now overlooked.
>
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

This patch enables to fix up leaky-view problem using functions with tiny cost estimation scenario.

The point of this scenario is criteria to reorder qualifiers of scanning plan in order_qual_clauses(). The optimizer may pull up simple subqueries into upper level, then its qualifier will get merged with ones in the upper level. When executor scans a relation, qualifiers with smaller cost shall be executed earlier to minimize cost to filter out invisible tuples. However, we know unpreferable side-effects when we use a view for row-level security.
Even if a certain subquery rewritten from a view is defined for row-level security, a function with tiny cost appended from outside of the view may executed earlier than qualifiers to perform as security policy of the view, as long as the view is enough simple and the supplied function has tiny cost. In the result, this function can see the arguments come from invisible tuples, and leak them into somewhere.

The solution is quite simple. This patch enables to track original depth of qualifiers and modify criteria to sort qualifiers in order_qual_clauses().
Even if a function with tiny cost is supplied from outside of views, the patched optimizer does not prioritize cost estimation more than the depth.

It fixes up the scenario [1] in the bellow descriprions.

--------
The background of the leaky-view problem is well summarized at:
http://wiki.postgresql.org/wiki/RLS

We had discussed several scenarios in v9.1 development cycle, and the last developer meeting. We almost concluded the following criteria to characterize whether a leak-view scenario is problematic to be fixed, or not.
* If unprived user can directly reference contents of invisible tuples, it is a problem to be fixed.
* As long as contents of invisible tuples are consumed by internal stuff (eg, index-access method), it is not a problem to be fixed.

Thus, the scenario [1] and [2] are problematic to be fixed, but [3] and [4] are not. So, I'll try to fix up these two scenario with the patch part-1 amd part-2.

[1] unexpected reorder of functions with tiny-cost and side-effects

Qualifiers of WHERE or JOIN ... IN clause shall be sorted by estimated cost, not depth of nest level. Thus, this logic can make order reversal when user-given qualifier has smaller cost than qualifiers to perform as security policy inside of view.
In the result, these qualifiers can reference both of visible and invisible tuples prior to the filtering by row-level security policy of the view. Thus, this behavior can be used to leak contents of invisible tuples.

[2] unexpected push-down of functions with side-effect into join-loop

If arguments of qualifier being appended on outside of join-loop references only one-side of the join-loop, it is a good strategy to distribute this qualifier into inside of the join-loop to minimize number of tuples to be joined, from the viewpoint of performance.
However, it also makes order reversal when the join-loop is a part of view definition that should perform row-level security policy. Then, these exogenetic qualifiers may be executed prior to the filtering by row-level security policy of the view. Thus, this behavior can be used to leak contents of invisible tuple.

[3] estimation of hidden value using iteration of PK/FK proves

Due to the nature of PK/FK constraints, we can infer existence of key values being stored within invisible tuple, even if we never allows users to reference contents of invisible tuples.
We commonly call this type of information leaks "covert-channel", and it is basically impossible to prevent according to the previous security research, however, its risk is also relatively small because of slow bandwidth to leak.
We already made consensus this scenario is not a problem to be fixed.

[4] estimation of hidden value using statistics

One example was selectivity-estimator function; that may reference statistical information delivered from the tables have invisible tuples for optimization. Here are two points to be considered. The one is purely internal stuff may be able to reference invisible tuples, however, it is not a problem as long as it does not leak them into end-users; such as index access methods. The second is statistical or other form of date delivered from invisible tuples. We can set up a table that contains data delivered from invisible tuples using row-level triggers, however, it is quite a matter of database administration. Unless owner of tables set up such a leakable configuration, other users cannot reference them.

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

This patch enables to fix up leaky-view problem using qualifiers that reference only one-side of join-loop inside of view definition.

The point of this scenario is criteria to distribute qualifiers of scanning-plan distributed in distribute_qual_to_rels(). If and when a qualifiers that reference only one-side of join-loop, the optimizer may distribute this qualifier into inside of the join-loop, even if it goes over the boundary of a subquery expanded from a view for row-level security.
This behavior allows us to reference whole of one-side of join-loop using functions with side-effects.
The solution is quite simple; it prohibits to distribute qualifiers over the boundary of subquery, however, performance cost is unignorable, because it also disables to utilize obviously indexable qualifiers such as (id=123), so this patch requires users a hint whether a particular view is for row-level security, or not.

This patch newly adds "CREATE SECURITY VIEW" statement that marks a flag to show this view was defined for row-level security purpose. This flag shall be stored as reloptions.
If this flag was set, the optimizer does not distribute qualifiers over the boundary of subqueries expanded from security views, except for obviously safe qualifiers.
(Right now, we consider built-in indexable operators are safe, but it might be arguable.)

It fixes up the scenario [2] in the bellow descriprions.

--------
The background of the leaky-view problem is well summarized at:
http://wiki.postgresql.org/wiki/RLS

We had discussed several scenarios in v9.1 development cycle, and the last developer meeting. We almost concluded the following criteria to characterize whether a leak-view scenario is problematic to be fixed, or not.
* If unprived user can directly reference contents of invisible tuples, it is a problem to be fixed.
* As long as contents of invisible tuples are consumed by internal stuff (eg, index-access method), it is not a problem to be fixed.

Thus, the scenario [1] and [2] are problematic to be fixed, but [3] and [4] are not. So, I'll try to fix up these two scenario with the patch part-1 amd part-2.

[1] unexpected reorder of functions with tiny-cost and side-effects

Qualifiers of WHERE or JOIN ... IN clause shall be sorted by estimated cost, not depth of nest level. Thus, this logic can make order reversal when user-given qualifier has smaller cost than qualifiers to perform as security policy inside of view.
In the result, these qualifiers can reference both of visible and invisible tuples prior to the filtering by row-level security policy of the view. Thus, this behavior can be used to leak contents of invisible tuples.

[2] unexpected push-down of functions with side-effect into join-loop

If arguments of qualifier being appended on outside of join-loop references only one-side of the join-loop, it is a good strategy to distribute this qualifier into inside of the join-loop to minimize number of tuples to be joined, from the viewpoint of performance.
However, it also makes order reversal when the join-loop is a part of view definition that should perform row-level security policy. Then, these exogenetic qualifiers may be executed prior to the filtering by row-level security policy of the view. Thus, this behavior can be used to leak contents of invisible tuple.

[3] estimation of hidden value using iteration of PK/FK proves

Due to the nature of PK/FK constraints, we can infer existence of key values being stored within invisible tuple, even if we never allows users to reference contents of invisible tuples.
We commonly call this type of information leaks "covert-channel", and it is basically impossible to prevent according to the previous security research, however, its risk is also relatively small because of slow bandwidth to leak.
We already made consensus this scenario is not a problem to be fixed.

[4] estimation of hidden value using statistics

One example was selectivity-estimator function; that may reference statistical information delivered from the tables have invisible tuples for optimization. Here are two points to be considered. The one is purely internal stuff may be able to reference invisible tuples, however, it is not a problem as long as it does not leak them into end-users; such as index access methods. The second is statistical or other form of date delivered from invisible tuples. We can set up a table that contains data delivered from invisible tuples using row-level triggers, however, it is quite a matter of database administration. Unless owner of tables set up such a leakable configuration, other users cannot reference them.

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>

I took a look at this patch. It's incredibly simple, which is great, and it
seems to achieve its goal.

Suppose your query references two views owned by different roles. The quals
of those views will have the same depth. Is there a way for information to
leak from one view owner to another due to that?

I like how you've assumed that the table owner trusts the operator class
functions of indexes on his table to not leak information. That handily
catches some basic and important qual pushdowns that would otherwise be lost.

This makes assumptions, at a distance, about the possible scan types and how
quals can be fitted to them. Specifically, this patch achieves its goals
because any indexable qual is trustworthy, and any non-indexable qual cannot
be pushed down further than the view's own quals. That seems to be true with
current plan types, but it feels fragile. I don't have a concrete idea for
improvement, though. Robert suggested another approach in
http://archives.postgresql.org/message-id/AANLkTimbN_6tYxReh5Rc7pmizT-VJB3xgp8CuHO0OAHC@mail.gmail.com
; might that approach avoid this hazard?

The part 2 patch in this series implements the planner restriction more likely
to yield performance regressions, so it introduces a mechanism for identifying
when to apply the restriction. This patch, however, applies its restriction
unconditionally. Since we will inevitably have a such mechanism before you
are done sealing the leaks in our view implementation, the restriction in this
patch should also use that mechanism. Therefore, I think we should review and
apply part 2 first, then update this patch to use its conditional behavior.

A few minor questions/comments on the implementation:

On Mon, Jun 06, 2011 at 01:37:11PM +0100, Kohei Kaigai wrote:
> --- a/src/backend/optimizer/plan/planner.c
> +++ b/src/backend/optimizer/plan/planner.c

> + else if (IsA(node, Query))
> + {
> + depth += 2;

Why two?

> --- a/src/test/regress/expected/select_views.out
> +++ b/src/test/regress/expected/select_views.out

> +EXPLAIN SELECT * FROM your_credit WHERE f_leak(number,expired);
> + QUERY PLAN
> +--------------------------------------------------------------------------
> + Seq Scan on credit_cards (cost=0.00..181.20 rows=1 width=96)

Use "EXPLAIN (COSTS OFF)" in regression tests. We do not put much effort into
the stability of exact cost values, and they do not matter for the purpose of
this test.

> --- a/src/test/regress/sql/select_views.sql
> +++ b/src/test/regress/sql/select_views.sql
> @@ -8,3 +8,46 @@ SELECT * FROM street;
> SELECT name, #thepath FROM iexit ORDER BY 1, 2;
>
> SELECT * FROM toyemp WHERE name = 'sharon';
> +
> +--
> +-- Test for leaky-view problem
> +--
> +
> +-- setups
> +SET client_min_messages TO 'warning';
> +
> +DROP ROLE IF EXISTS alice;
> +DROP FUNCTION IF EXISTS f_leak(text);
> +DROP TABLE IF EXISTS credit_cards;
> +
> +RESET client_min_messages;

No need for this. The regression tests always start on a clean database.

> +
> +CREATE USER alice;
> +CREATE FUNCTION f_leak(text, text)
> + RETURNS bool LANGUAGE 'plpgsql'
> + AS 'begin raise notice ''% => %'', $1, $2; return true; end';

I ran this test case on master, and it did not reproduce the problem.
However, adding "COST 0.1" to this CREATE FUNCTION did yield the expected
problem behavior. I suggest adding that.

Thanks,
nm

Thanks for your reviewing,

2011/6/28 Noah Misch <noah(at)leadboat(dot)com>:
> I took a look at this patch.  It's incredibly simple, which is great, and it
> seems to achieve its goal.
>
> Suppose your query references two views owned by different roles.  The quals
> of those views will have the same depth.  Is there a way for information to
> leak from one view owner to another due to that?
>
Even if multiple subqueries were pulled-up and qualifiers got merged, user given
qualifiers shall have smaller depth value, so it will be always
launched after the
qualifiers originally used in the subqueries.

Of course, it is another topic in the case when the view is originally
defined with
leaky functions.

> I like how you've assumed that the table owner trusts the operator class
> functions of indexes on his table to not leak information.  That handily
> catches some basic and important qual pushdowns that would otherwise be lost.
>
> This makes assumptions, at a distance, about the possible scan types and how
> quals can be fitted to them.  Specifically, this patch achieves its goals
> because any indexable qual is trustworthy, and any non-indexable qual cannot
> be pushed down further than the view's own quals.  That seems to be true with
> current plan types, but it feels fragile.  I don't have a concrete idea for
> improvement, though.  Robert suggested another approach in
> http://archives.postgresql.org/message-id/AANLkTimbN_6tYxReh5Rc7pmizT-VJB3xgp8CuHO0OAHC@mail.gmail.com
> ; might that approach avoid this hazard?
>
The reason why we didn't adopt the idea to check privileges of underlying tables
is that PostgreSQL checks privileges on executor phase, not planner phase.

If we try to have a flag on pg_proc, it is a tough work to categolize trustworth
functions and non-trustworh ones from beginning, because we have more than
2000 of built-in functions.
So, it is reasonable assumption that index access methods does not leak
contents of tuples scanned, because only superuser can define them.

> The part 2 patch in this series implements the planner restriction more likely
> to yield performance regressions, so it introduces a mechanism for identifying
> when to apply the restriction.  This patch, however, applies its restriction
> unconditionally.  Since we will inevitably have a such mechanism before you
> are done sealing the leaks in our view implementation, the restriction in this
> patch should also use that mechanism.  Therefore, I think we should review and
> apply part 2 first, then update this patch to use its conditional behavior.
>
The reason why this patch always gives the depth higher priority is the matter
is relatively minor compared to the issue the part.2 patch tries to tackle.
That affects the selection of scan plan (IndexScan or SeqScan), so it
is significant
decision to be controllable. However, this issue is just on a particular scan.

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.

> A few minor questions/comments on the implementation:
>
> On Mon, Jun 06, 2011 at 01:37:11PM +0100, Kohei Kaigai wrote:
>> --- a/src/backend/optimizer/plan/planner.c
>> +++ b/src/backend/optimizer/plan/planner.c
>
>> +     else if (IsA(node, Query))
>> +     {
>> +             depth += 2;
>
> Why two?
>
This patch is a groundwork for the upcoming row-level security feature.
In the case when the backend appends security policy functions, it should
be launched prior to user given qualifiers. So, I intends to give odd depth
numbers for these functions to have higher priorities to other one.
Of course, 1 might work well right now.

>> --- a/src/test/regress/expected/select_views.out
>> +++ b/src/test/regress/expected/select_views.out
>
>> +EXPLAIN SELECT * FROM your_credit WHERE f_leak(number,expired);
>> +                                QUERY PLAN
>> +--------------------------------------------------------------------------
>> + Seq Scan on credit_cards  (cost=0.00..181.20 rows=1 width=96)
>
> Use "EXPLAIN (COSTS OFF)" in regression tests.  We do not put much effort into
> the stability of exact cost values, and they do not matter for the purpose of
> this test.
>
OK, fixed.

>> --- a/src/test/regress/sql/select_views.sql
>> +++ b/src/test/regress/sql/select_views.sql
>> @@ -8,3 +8,46 @@ SELECT * FROM street;
>>  SELECT name, #thepath FROM iexit ORDER BY 1, 2;
>>
>>  SELECT * FROM toyemp WHERE name = 'sharon';
>> +
>> +--
>> +-- Test for leaky-view problem
>> +--
>> +
>> +-- setups
>> +SET client_min_messages TO 'warning';
>> +
>> +DROP ROLE IF EXISTS alice;
>> +DROP FUNCTION IF EXISTS f_leak(text);
>> +DROP TABLE IF EXISTS credit_cards;
>> +
>> +RESET client_min_messages;
>
> No need for this.  The regression tests always start on a clean database.
>
Fixed.

>> +
>> +CREATE USER alice;
>> +CREATE FUNCTION f_leak(text, text)
>> +    RETURNS bool LANGUAGE 'plpgsql'
>> +    AS 'begin raise notice ''% => %'', $1, $2; return true; end';
>
> I ran this test case on master, and it did not reproduce the problem.
> However, adding "COST 0.1" to this CREATE FUNCTION did yield the expected
> problem behavior.  I suggest adding that.
>
Thanks, I might miss the point of regression test.

The attached patch is revised one on regression test.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Tue, Jun 28, 2011 at 10:11:59PM +0200, Kohei KaiGai wrote:
> 2011/6/28 Noah Misch <noah(at)leadboat(dot)com>:
> > Suppose your query references two views owned by different roles. ?The quals
> > of those views will have the same depth. ?Is there a way for information to
> > leak from one view owner to another due to that?
> >
> Even if multiple subqueries were pulled-up and qualifiers got merged, user given
> qualifiers shall have smaller depth value, so it will be always
> launched after the
> qualifiers originally used in the subqueries.
>
> Of course, it is another topic in the case when the view is originally
> defined with
> leaky functions.

Right. I was thinking of a pair of quals, one in each of two view definitions.
The views are mutually-untrusting. Something like this:

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.

> > This makes assumptions, at a distance, about the possible scan types and how
> > quals can be fitted to them. ?Specifically, this patch achieves its goals
> > because any indexable qual is trustworthy, and any non-indexable qual cannot
> > be pushed down further than the view's own quals. ?That seems to be true with
> > current plan types, but it feels fragile. ?I don't have a concrete idea for
> > improvement, though. ?Robert suggested another approach in
> > http://archives.postgresql.org/message-id/AANLkTimbN_6tYxReh5Rc7pmizT-VJB3xgp8CuHO0OAHC@mail.gmail.com
> > ; might that approach avoid this hazard?
> >
> The reason why we didn't adopt the idea to check privileges of underlying tables
> is that PostgreSQL checks privileges on executor phase, not planner phase.
>
> If we try to have a flag on pg_proc, it is a tough work to categolize trustworth
> functions and non-trustworh ones from beginning, because we have more than
> 2000 of built-in functions.
> So, it is reasonable assumption that index access methods does not leak
> contents of tuples scanned, because only superuser can define them.

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.

> > The part 2 patch in this series implements the planner restriction more likely
> > to yield performance regressions, so it introduces a mechanism for identifying
> > when to apply the restriction. ?This patch, however, applies its restriction
> > unconditionally. ?Since we will inevitably have a such mechanism before you
> > are done sealing the leaks in our view implementation, the restriction in this
> > patch should also use that mechanism. ?Therefore, I think we should review and
> > apply part 2 first, then update this patch to use its conditional behavior.
> >
> The reason why this patch always gives the depth higher priority is the matter
> is relatively minor compared to the issue the part.2 patch tries to tackle.
> That affects the selection of scan plan (IndexScan or SeqScan), so it
> is significant
> decision to be controllable. However, this issue is just on a particular scan.

True. The lost optimization opportunity is relatively minor, but perhaps not
absolutely minor. It would be one thing if we could otherwise get away without
designing any mechanism for applying these restrictions conditionally. However,
since you have implemented the conditional behavior elsewhere, it would be nice
to apply it here.

> 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?

> > On Mon, Jun 06, 2011 at 01:37:11PM +0100, Kohei Kaigai wrote:
> >> --- a/src/backend/optimizer/plan/planner.c
> >> +++ b/src/backend/optimizer/plan/planner.c
> >
> >> + ? ? else if (IsA(node, Query))
> >> + ? ? {
> >> + ? ? ? ? ? ? depth += 2;
> >
> > Why two?
> >
> This patch is a groundwork for the upcoming row-level security feature.
> In the case when the backend appends security policy functions, it should
> be launched prior to user given qualifiers. So, I intends to give odd depth
> numbers for these functions to have higher priorities to other one.
> Of course, 1 might work well right now.

I'd say it should either be 1 until such time as that's needed, or it needs a
comment noting why it's 2.

Thanks,
nm

2011/6/28 Noah Misch <noah(at)leadboat(dot)com>:
> On Tue, Jun 28, 2011 at 10:11:59PM +0200, Kohei KaiGai wrote:
>> 2011/6/28 Noah Misch <noah(at)leadboat(dot)com>:
>> > Suppose your query references two views owned by different roles. ?The quals
>> > of those views will have the same depth. ?Is there a way for information to
>> > leak from one view owner to another due to that?
>> >
>> Even if multiple subqueries were pulled-up and qualifiers got merged, user given
>> qualifiers shall have smaller depth value, so it will be always
>> launched after the
>> qualifiers originally used in the subqueries.
>>
>> Of course, it is another topic in the case when the view is originally
>> defined with
>> leaky functions.
>
> Right.  I was thinking of a pair of quals, one in each of two view definitions.
> The views are mutually-untrusting.  Something like this:
>
> 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.

> 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.

As long as we focus on the ordering problem, it is reasonable approach to
track original nestlevel to enforce to launch qualifier come from deeper level
earlier. Because it makes performance damages, if we prevent pull-up
subqueries come from security view.

For example, if we cannot pull-up this subquery, we will need to scan the
relation twice.
SELECT * FROM (SELECT * FROM tbl WHERE f_policy(x)) WHERE f_leak(y);

Normally, it should be pulled-up into the following form:
SELECT * FROM tbl WHERE f_policy(x) AND f_leak(y);

>> 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.

>> > On Mon, Jun 06, 2011 at 01:37:11PM +0100, Kohei Kaigai wrote:
>> >> --- a/src/backend/optimizer/plan/planner.c
>> >> +++ b/src/backend/optimizer/plan/planner.c
>> >
>> >> + ? ? else if (IsA(node, Query))
>> >> + ? ? {
>> >> + ? ? ? ? ? ? depth += 2;
>> >
>> > Why two?
>> >
>> This patch is a groundwork for the upcoming row-level security feature.
>> In the case when the backend appends security policy functions, it should
>> be launched prior to user given qualifiers. So, I intends to give odd depth
>> numbers for these functions to have higher priorities to other one.
>> Of course, 1 might work well right now.
>
> I'd say it should either be 1 until such time as that's needed, or it needs a
> comment noting why it's 2.
>
OK, I'll add comment to introduce why the depth is incremented by 2.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

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

>> > 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.
>
If a subquery is enough simple to pull up, pull_up_simple_subquery() pulls up
the subquery. Its simpleness is checked by is_simple_subquery().
At that timing, qualifiers of the subquery are also pulled-up, so upper level
query shall have qualifiers with mixed nest-level.

Then, every qualifiers shall be distributed to a particular scan plan on
the distribute_qual_to_rels(); Its current criteria to distribute them is
"as deep as possible" according to the references of qualifiers.
This criteria cause a problem as I tried to tackle on the part.2 patch,
so it tries to put security-barrier to prevent unexpected pushing-down.

After the distribution, a scan plan will have qualifiers come from different
nest-levels. The order_qual_clauses() reorders the qualifiers according
to its cost estimation, so it possibly launches qualifiers come from upper
level prior to deeper one.

>> >> 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.)
>
It might be an idea to increment the depth only when we go across security
barrier view. In other words, all the qualifiers will have same depth unless
it does not come from inside of the security view.

> 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.
>
If we apply above idea I explained, c=2 and expensive(c) will belong
to same depth,
then it shall be reordered according to cost estimation.
In the case when "(SELECT * FROM t WHERE expensive(c))" come from security
view, the performance damage is unavoidable, because DBA explicitly specified
its main purpose is security.

So, it might be a good idea to split out my two patches into three.
1. Add "SECURITY VIEW" support.
2. Fix leaky view part.1 - order of qualifiers
3. Fix leaky view part.2 - unexpected pushing down

How about your opinion?
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sat, Jul 02, 2011 at 12:48:32PM +0200, Kohei KaiGai wrote:
> > 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.)
> >
> It might be an idea to increment the depth only when we go across security
> barrier view. In other words, all the qualifiers will have same depth unless
> it does not come from inside of the security view.

Yes; that sounds suitable.

> > 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.
> >
> If we apply above idea I explained, c=2 and expensive(c) will belong
> to same depth,
> then it shall be reordered according to cost estimation.
> In the case when "(SELECT * FROM t WHERE expensive(c))" come from security
> view, the performance damage is unavoidable, because DBA explicitly specified
> its main purpose is security.
>
> So, it might be a good idea to split out my two patches into three.
> 1. Add "SECURITY VIEW" support.
> 2. Fix leaky view part.1 - order of qualifiers
> 3. Fix leaky view part.2 - unexpected pushing down
>
> How about your opinion?

I'd say, for CommitFest purposes, keep SECURITY VIEW attached to one of the
other patches. It's not likely it would be committed without anything hooked up
to actually use it. Splitting it out into its own patch *file* and attaching
that and the part 1 patch to the same email would be fine, though.

BTW, regarding to the statement support for security barrier views,
the following syntax might be more consistent with existing ones:
CREATE VIEW view_name WITH ( param [=value]) AS query ... ;
rather than
CREATE SECURITY VIEW view_name AS query ...;

Any comments?

2011/7/2 Noah Misch <noah(at)2ndquadrant(dot)com>:
> On Sat, Jul 02, 2011 at 12:48:32PM +0200, Kohei KaiGai wrote:
>> > 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.)
>> >
>> It might be an idea to increment the depth only when we go across security
>> barrier view. In other words, all the qualifiers will have same depth unless
>> it does not come from inside of the security view.
>
> Yes; that sounds suitable.
>
>> > 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.
>> >
>> If we apply above idea I explained, c=2 and expensive(c) will belong
>> to same depth,
>> then it shall be reordered according to cost estimation.
>> In the case when "(SELECT * FROM t WHERE expensive(c))" come from security
>> view, the performance damage is unavoidable, because DBA explicitly specified
>> its main purpose is security.
>>
>> So, it might be a good idea to split out my two patches into three.
>> 1. Add "SECURITY VIEW" support.
>> 2. Fix leaky view part.1 - order of qualifiers
>> 3. Fix leaky view part.2 - unexpected pushing down
>>
>> How about your opinion?
>
> I'd say, for CommitFest purposes, keep SECURITY VIEW attached to one of the
> other patches.  It's not likely it would be committed without anything hooked up
> to actually use it.  Splitting it out into its own patch *file* and attaching
> that and the part 1 patch to the same email would be fine, though.
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sat, Jul 2, 2011 at 1:54 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> BTW, regarding to the statement support for security barrier views,
> the following syntax might be more consistent with existing ones:
>  CREATE VIEW view_name WITH ( param [=value]) AS query ... ;
> rather than
>  CREATE SECURITY VIEW view_name AS query ...;
>
> Any comments?

I think I mildly prefer CREATE SECURITY VIEW to the parameter syntax
in this case, but I don't hate the other one.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Jul 2, 2011 at 1:54 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> BTW, regarding to the statement support for security barrier views,
>> the following syntax might be more consistent with existing ones:
>> CREATE VIEW view_name WITH ( param [=value]) AS query ... ;
>> rather than
>> CREATE SECURITY VIEW view_name AS query ...;
>>
>> Any comments?

> I think I mildly prefer CREATE SECURITY VIEW to the parameter syntax
> in this case, but I don't hate the other one.

The WITH idea seems a bit more future-proof; in particular it would
easily accommodate specifying a security type, if we decide we need
various levels of leak-proof-ness.

regards, tom lane

On Sat, Jul 2, 2011 at 3:46 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sat, Jul 2, 2011 at 1:54 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> BTW, regarding to the statement support for security barrier views,
>>> the following syntax might be more consistent with existing ones:
>>>  CREATE VIEW view_name WITH ( param [=value]) AS query ... ;
>>> rather than
>>>  CREATE SECURITY VIEW view_name AS query ...;
>>>
>>> Any comments?
>
>> I think I mildly prefer CREATE SECURITY VIEW to the parameter syntax
>> in this case, but I don't hate the other one.
>
> The WITH idea seems a bit more future-proof; in particular it would
> easily accommodate specifying a security type, if we decide we need
> various levels of leak-proof-ness.

Or other kinds of view options. I'm not going to argue against that
too forcefully, since I've advocated introducing that sort of syntax
elsewhere. I think it's mostly that I thought this feature might be
significant enough to merit a syntax that would make it a little more
prominent, but perhaps not.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

The attached patches are revised version.

The part-0 provides 'security_barrier' option for view definition, and performs
as a common basis of part-1 and part-2 patches.
Syntax is extended as follows:

CREATE VIEW view_name [WITH (param [=value])] AS query;

We can also turn on/off this security_barrier setting by ALTER TABLE with
SET/RESET options.

The part-1 patch enforces the qualifiers originally located under the security
barrier view to be launched prior to ones supplied on upper level.
The differences from the previous version is this barrier become conditional,
not always. So, existing optimization will be applied without any changes
onto non-security-barrier views.

Example)
postgres=# CREATE FUNCTION f_leak(text,text) RETURNS bool
COST 0.0001 LANGUAGE 'plpgsql'
AS 'begin raise notice ''% => %'', $1, $2; return true; end';
CREATE FUNCTION
postgres=# CREATE TABLE credit_card (cname text, cnumber text, cexpired text);
INSERT INTO credit_card (cname, cnumber, cexpired)
CREATE TABLE
postgres=# INSERT INTO credit_card (cname, cnumber, cexpired)
VALUES ('alice', '1111-2222-3333-4444', '07/2014'),
('bob', '5555-6666-7777-8888', '11/2013'),
('eve', '1234-5678-9012-3456', '05/2015');
INSERT 0 3
postgres=# CREATE VIEW my_credit_card AS SELECT * FROM credit_card
WHERE cname = getpgusername();
CREATE VIEW
postgres=# CREATE VIEW my_credit_card_sec WITH (security_barrier) AS
SELECT * FROM credit_card WHERE cname = getpgusername();
CREATE VIEW
postgres=# GRANT SELECT ON my_credit_card TO public;
GRANT
postgres=# GRANT SELECT ON my_credit_card_sec TO public;
GRANT
postgres=# SET SESSION AUTHORIZATION alice;
SET
postgres=> SELECT * FROM my_credit_card WHERE f_leak(cnumber,cexpired);
NOTICE: 1111-2222-3333-4444 => 07/2014
NOTICE: 5555-6666-7777-8888 => 11/2013
NOTICE: 1234-5678-9012-3456 => 05/2015
cname | cnumber | cexpired
-------+---------------------+----------
alice | 1111-2222-3333-4444 | 07/2014
(1 row)

postgres=> SELECT * FROM my_credit_card_sec WHERE f_leak(cnumber,cexpired);
NOTICE: 1111-2222-3333-4444 => 07/2014
cname | cnumber | cexpired
-------+---------------------+----------
alice | 1111-2222-3333-4444 | 07/2014
(1 row)

postgres=> EXPLAIN SELECT * FROM my_credit_card WHERE f_leak(cnumber,cexpired);
QUERY PLAN
-----------------------------------------------------------------------------
Seq Scan on credit_card (cost=0.00..21.20 rows=1 width=96)
Filter: (f_leak(cnumber, cexpired) AND (cname = (getpgusername())::text))
(2 rows)

postgres=> EXPLAIN SELECT * FROM my_credit_card_sec WHERE
f_leak(cnumber,cexpired);
QUERY PLAN
-----------------------------------------------------------------------------
Seq Scan on credit_card (cost=0.00..21.20 rows=1 width=96)
Filter: ((cname = (getpgusername())::text) AND f_leak(cnumber, cexpired))
(2 rows)

Thanks,

2011/7/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sat, Jul 2, 2011 at 3:46 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Sat, Jul 2, 2011 at 1:54 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> BTW, regarding to the statement support for security barrier views,
>>>> the following syntax might be more consistent with existing ones:
>>>>  CREATE VIEW view_name WITH ( param [=value]) AS query ... ;
>>>> rather than
>>>>  CREATE SECURITY VIEW view_name AS query ...;
>>>>
>>>> Any comments?
>>
>>> I think I mildly prefer CREATE SECURITY VIEW to the parameter syntax
>>> in this case, but I don't hate the other one.
>>
>> The WITH idea seems a bit more future-proof; in particular it would
>> easily accommodate specifying a security type, if we decide we need
>> various levels of leak-proof-ness.
>
> Or other kinds of view options.  I'm not going to argue against that
> too forcefully, since I've advocated introducing that sort of syntax
> elsewhere.  I think it's mostly that I thought this feature might be
> significant enough to merit a syntax that would make it a little more
> prominent, but perhaps not.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

The simplified version of fix-leaky-view patch. The part of reloptions
for views got splitted out
into the part-0 patch, so it needs to be applied prior to this patch.
Rest of logic to prevent unexpected pushing down across security
barrier is not changed.

Thanks,

2011/6/6 Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com>:
> This patch enables to fix up leaky-view problem using qualifiers that reference only one-side of join-loop inside of view definition.
>
> The point of this scenario is criteria to distribute qualifiers of scanning-plan distributed in distribute_qual_to_rels(). If and when a qualifiers that reference only one-side of join-loop, the optimizer may distribute this qualifier into inside of the join-loop, even if it goes over the boundary of a subquery expanded from a view for row-level security.
> This behavior allows us to reference whole of one-side of join-loop using functions with side-effects.
> The solution is quite simple; it prohibits to distribute qualifiers over the boundary of subquery, however, performance cost is unignorable, because it also disables to utilize obviously indexable qualifiers such as (id=123), so this patch requires users a hint whether a particular view is for row-level security, or not.
>
> This patch newly adds "CREATE SECURITY VIEW" statement that marks a flag to show this view was defined for row-level security purpose. This flag shall be stored as reloptions.
> If this flag was set, the optimizer does not distribute qualifiers over the boundary of subqueries expanded from security views, except for obviously safe qualifiers.
> (Right now, we consider built-in indexable operators are safe, but it might be arguable.)
>
> It fixes up the scenario [2] in the bellow descriprions.
>
> --------
> The background of the leaky-view problem is well summarized at:
>  http://wiki.postgresql.org/wiki/RLS
>
> We had discussed several scenarios in v9.1 development cycle, and the last developer meeting. We almost concluded the following criteria to characterize whether a leak-view scenario is problematic to be fixed, or not.
>  * If unprived user can directly reference contents of invisible tuples, it is a problem to be fixed.
>  * As long as contents of invisible tuples are consumed by internal stuff (eg, index-access method), it is not a problem to be fixed.
>
> Thus, the scenario [1] and [2] are problematic to be fixed, but [3] and [4] are not. So, I'll try to fix up these two scenario with the patch part-1 amd part-2.
>
> [1] unexpected reorder of functions with tiny-cost and side-effects
>
> Qualifiers of WHERE or JOIN ... IN clause shall be sorted by estimated cost, not depth of nest level. Thus, this logic can make order reversal when user-given qualifier has smaller cost than qualifiers to perform as security policy inside of view.
> In the result, these qualifiers can reference both of visible and invisible tuples prior to the filtering by row-level security policy of the view. Thus, this behavior can be used to leak contents of invisible tuples.
>
>
> [2] unexpected push-down of functions with side-effect into join-loop
>
> If arguments of qualifier being appended on outside of join-loop references only one-side of the join-loop, it is a good strategy to distribute this qualifier into inside of the join-loop to minimize number of tuples to be joined, from the viewpoint of performance.
> However, it also makes order reversal when the join-loop is a part of view definition that should perform row-level security policy. Then, these exogenetic qualifiers may be executed prior to the filtering by row-level security policy of the view. Thus, this behavior can be used to leak contents of invisible tuple.
>
>
> [3] estimation of hidden value using iteration of PK/FK proves
>
> Due to the nature of PK/FK constraints, we can infer existence of key values being stored within invisible tuple, even if we never allows users to reference contents of invisible tuples.
> We commonly call this type of information leaks "covert-channel", and it is basically impossible to prevent according to the previous security research, however, its risk is also relatively small because of slow bandwidth to leak.
> We already made consensus this scenario is not a problem to be fixed.
>
>
> [4] estimation of hidden value using statistics
>
> One example was selectivity-estimator function; that may reference statistical information delivered from the tables have invisible tuples for optimization. Here are two points to be considered. The one is purely internal stuff may be able to reference invisible tuples, however, it is not a problem as long as it does not leak them into end-users; such as index access methods. The second is statistical or other form of date delivered from invisible tuples. We can set up a table that contains data delivered from invisible tuples using row-level triggers, however, it is quite a matter of database administration. Unless owner of tables set up such a leakable configuration, other users cannot reference them.
>
> Thanks,
> --
> NEC Europe Ltd, SAP Global Competence Center
> KaiGai Kohei <kohei(dot)kaigai(at)emea(dot)nec(dot)com>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sun, Jul 03, 2011 at 11:33:38AM +0200, Kohei KaiGai wrote:
> The attached patches are revised version.
>
> The part-0 provides 'security_barrier' option for view definition, and performs
> as a common basis of part-1 and part-2 patches.
> Syntax is extended as follows:
>
> CREATE VIEW view_name [WITH (param [=value])] AS query;
>
> We can also turn on/off this security_barrier setting by ALTER TABLE with
> SET/RESET options.
>
> The part-1 patch enforces the qualifiers originally located under the security
> barrier view to be launched prior to ones supplied on upper level.
> The differences from the previous version is this barrier become conditional,
> not always. So, existing optimization will be applied without any changes
> onto non-security-barrier views.

I tested various query trees I considered interesting, and this version had
sound semantics for all of them. I have one suggestion for CREATE OR REPLACE
VIEW semantics, plus various cosmetic comments.

These patches are unified diffs, rather than project-standard context diffs.

Part 0:
> --- a/doc/src/sgml/ref/create_view.sgml
> +++ b/doc/src/sgml/ref/create_view.sgml
> @@ -22,6 +22,7 @@ PostgreSQL documentation
> <refsynopsisdiv>
> <synopsis>
> CREATE [ OR REPLACE ] [ TEMP | TEMPORARY ] VIEW <replaceable class="PARAMETER">name</replaceable> [ ( <replaceable class="PARAMETER">column_name</replaceable> [, ...] ) ]
> + [ WITH ( parameter [= value] [, ... ] ) ]

This needs a bit more markup; see the corresponding case in create_table.sgml.

alter_view.sgml also needs an update. Incidentally, we should use ALTER VIEW
SET OPTION when referring to setting this for a view. ALTER TABLE SET OPTION
will also support views, since that's the general pattern for tablecmds.c type
checks, but that's largely an implementation detail.

> --- a/src/backend/commands/view.c
> +++ b/src/backend/commands/view.c
> @@ -97,7 +97,8 @@ isViewOnTempTable_walker(Node *node, void *context)
> *---------------------------------------------------------------------
> */
> static Oid
> -DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
> +DefineVirtualRelation(const RangeVar *relation, List *tlist,
> + bool replace, List *options)
> {
> Oid viewOid,
> namespaceId;

This hunk and the hunk for the function's caller get rejects due to another
recent signature change.

> @@ -167,6 +168,8 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
> {
> Relation rel;
> TupleDesc descriptor;
> + List *atcmds = NIL;
> + AlterTableCmd *atcmd;
>
> /*
> * Yes. Get exclusive lock on the existing view ...
> @@ -211,14 +214,11 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
> */
> if (list_length(attrList) > rel->rd_att->natts)
> {
> - List *atcmds = NIL;
> ListCell *c;
> int skip = rel->rd_att->natts;
>
> foreach(c, attrList)
> {
> - AlterTableCmd *atcmd;
> -
> if (skip > 0)
> {
> skip--;
> @@ -229,10 +229,24 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
> atcmd->def = (Node *) lfirst(c);
> atcmds = lappend(atcmds, atcmd);
> }
> - AlterTableInternal(viewOid, atcmds, true);
> }
>
> /*
> + * If optional parameters are specified, we must set options
> + * using ALTER TABLE SET OPTION internally.

I think CREATE OR REPLACE VIEW should replace the option list, while ALTER
VIEW SET OPTION should retain its current behavior. That is, this should
leave the view with no options set:

create or replace view v0(n) with (security_barrier) as values (1), (2), (3), (4);
select reloptions from pg_class where oid = 'v0'::regclass;
create or replace view v0(n) as values (4), (3), (2), (1);
select reloptions from pg_class where oid = 'v0'::regclass;

> + */
> + if (list_length(options) > 0)
> + {
> + atcmd = makeNode(AlterTableCmd);
> + atcmd->subtype = AT_SetRelOptions;
> + atcmd->def = options;

This line produces a warning:

view.c: In function `DefineVirtualRelation':
view.c:240: warning: assignment from incompatible pointer type

> +
> + atcmds = lappend(atcmds, atcmd);
> + }
> + if (atcmds != NIL)
> + AlterTableInternal(viewOid, atcmds, true);
> +
> + /*
> * Seems okay, so return the OID of the pre-existing view.
> */
> relation_close(rel, NoLock); /* keep the lock! */

Part 1:
> --- a/src/backend/optimizer/plan/planner.c
> +++ b/src/backend/optimizer/plan/planner.c

> @@ -2993,6 +3001,131 @@ get_column_info_for_window(PlannerInfo *root, WindowClause *wc, List *tlist,
> }
> }
>
> +/*
> + * mark_qualifiers_depth
> + *
> + * It marks depth field of the each expression nodes that eventually
> + * invokes functions, to track the original nest-level. On the evaluation
> + * of qualifiers within WHERE or JOIN ... ON clauses during relation scans,
> + * these items shall be reordered according to the nest-level and estimated
> + * cost.
> + * The optimizer may pull-up simple sub-queries or join clause, and
> + * qualifiers to filter out tuples shall be mixed with ones in upper-
> + * level. Thus, we need to track the original nest-level of qualifiers
> + * to prevent reverse of order in evaluation, because some of qualifiers
> + * can have side-effects that allows to leak supplied argument to outside.
> + * It can be abused to break row-level security using a user defined function
> + * with very small estimated cost, so nest level of qualifiers originated
> + * from is used as a criteria, rather than estimated cost, to decide order
> + * to evaluate qualifiers.
> + */
> +static bool
> +mark_qualifiers_depth_walker(Node *node, void *context)
> +{
> + int depth = *((int *)(context));
> +
> + if (node == NULL)
> + return false;
> + if (IsA(node, FuncExpr))
> + {
> + FuncExpr *exp = (FuncExpr *)node;
> +
> + exp->depth = depth | (exp->depth & 1);

Why did these change from plain "exp->depth = depth;" of the last version?
Since no core code sets a 1-bit in a depth value, I assume it must be related
to your future-use design for that bit. If so: could an external module
realistically take advantage of this? If yes, then a mere comment is in
order. If not, I think we should remove this (and the incrementing by 2) and
add it again in the future patch that makes use thereof.

> --- a/src/test/regress/sql/select_views.sql
> +++ b/src/test/regress/sql/select_views.sql
> @@ -8,3 +8,42 @@ SELECT * FROM street;
> SELECT name, #thepath FROM iexit ORDER BY 1, 2;
>
> SELECT * FROM toyemp WHERE name = 'sharon';
> +
> +--
> +-- Test for leaky-view
> +--
> +
> +CREATE USER alice;
> +CREATE FUNCTION f_leak(text, text)
> + RETURNS bool LANGUAGE 'plpgsql'
> + COST 0.00000001
> + AS 'begin raise notice ''% => %'', $1, $2; return true; end';
> +CREATE TABLE credit_cards (
> + name text,
> + number text,
> + expired text
> +);
> +INSERT INTO credit_cards VALUES ('alice', '1111-2222-3333-4444', 'Aug-2012'),
> + ('bob', '5555-6666-7777-8888', 'Nov-2016'),
> + ('eve', '9801-2345-6789-0123', 'Jan-2018');
> +CREATE VIEW your_credit_normal AS
> + SELECT * FROM credit_cards WHERE name = getpgusername();
> +CREATE VIEW your_credit_secure WITH (security_barrier) AS
> + SELECT * FROM credit_cards WHERE name = getpgusername();
> +
> +GRANT SELECT ON your_credit_normal TO public;
> +GRANT SELECT ON your_credit_secure TO public;
> +-- run leaky view
> +SET SESSION AUTHORIZATION alice;
> +
> +SELECT * FROM your_credit_normal WHERE f_leak(number,expired);
> +EXPLAIN (COSTS OFF) SELECT * FROM your_credit_normal WHERE f_leak(number,expired);
> +
> +SELECT * FROM your_credit_secure WHERE f_leak(number,expired);
> +EXPLAIN (COSTS OFF) SELECT * FROM your_credit_secure WHERE f_leak(number,expired);
> +
> +\c -
> +-- cleanups
> +DROP ROLE IF EXISTS alice;
> +DROP FUNCTION IF EXISTS f_leak(text);
> +DROP TABLE IF EXISTS credit_cards CASCADE;

Keep the view around. That way, pg_dump tests of the regression database will
test the dumping of this view option. (Your pg_dump support for this feature
does work fine, though.)

Thanks,
nm

Thanks for your detailed viewing and testing.

The attached patches are revised version.

Part-0)
* The patch was re-generated using context diff, instead of unified diff
* Documentation on ALTER VIEW was added
* Behavior of CREATE OR REPLACE VIEW was revised; when we replace
an existing view, reloption shall be reset, even if a particular
value was set.
* And, cosmetic changes; eliminate warnings due to lack of type cast.

Part-1)
* I removed the code to increment depth by 2, and preserve the latest bit,
because we have no module to utilize this mechanism right now.

Thanks,

2011/7/5 Noah Misch <noah(at)2ndquadrant(dot)com>:
> On Sun, Jul 03, 2011 at 11:33:38AM +0200, Kohei KaiGai wrote:
>> The attached patches are revised version.
>>
>> The part-0 provides 'security_barrier' option for view definition, and performs
>> as a common basis of part-1 and part-2 patches.
>> Syntax is extended as follows:
>>
>>   CREATE VIEW view_name [WITH (param [=value])] AS query;
>>
>> We can also turn on/off this security_barrier setting by ALTER TABLE with
>> SET/RESET options.
>>
>> The part-1 patch enforces the qualifiers originally located under the security
>> barrier view to be launched prior to ones supplied on upper level.
>> The differences from the previous version is this barrier become conditional,
>> not always. So, existing optimization will be applied without any changes
>> onto non-security-barrier views.
>
> I tested various query trees I considered interesting, and this version had
> sound semantics for all of them.  I have one suggestion for CREATE OR REPLACE
> VIEW semantics, plus various cosmetic comments.
>
> These patches are unified diffs, rather than project-standard context diffs.
>
> Part 0:
>> --- a/doc/src/sgml/ref/create_view.sgml
>> +++ b/doc/src/sgml/ref/create_view.sgml
>> @@ -22,6 +22,7 @@ PostgreSQL documentation
>>   <refsynopsisdiv>
>>  <synopsis>
>>  CREATE [ OR REPLACE ] [ TEMP | TEMPORARY ] VIEW <replaceable class="PARAMETER">name</replaceable> [ ( <replaceable class="PARAMETER">column_name</replaceable> [, ...] ) ]
>> +    [ WITH ( parameter [= value] [, ... ] ) ]
>
> This needs a bit more markup; see the corresponding case in create_table.sgml.
>
> alter_view.sgml also needs an update.  Incidentally, we should use ALTER VIEW
> SET OPTION when referring to setting this for a view.  ALTER TABLE SET OPTION
> will also support views, since that's the general pattern for tablecmds.c type
> checks, but that's largely an implementation detail.
>
>> --- a/src/backend/commands/view.c
>> +++ b/src/backend/commands/view.c
>> @@ -97,7 +97,8 @@ isViewOnTempTable_walker(Node *node, void *context)
>>   *---------------------------------------------------------------------
>>   */
>>  static Oid
>> -DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
>> +DefineVirtualRelation(const RangeVar *relation, List *tlist,
>> +                                       bool replace, List *options)
>>  {
>>       Oid                     viewOid,
>>                               namespaceId;
>
> This hunk and the hunk for the function's caller get rejects due to another
> recent signature change.
>
>> @@ -167,6 +168,8 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
>>       {
>>               Relation        rel;
>>               TupleDesc       descriptor;
>> +             List       *atcmds = NIL;
>> +             AlterTableCmd *atcmd;
>>
>>               /*
>>                * Yes.  Get exclusive lock on the existing view ...
>> @@ -211,14 +214,11 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
>>                */
>>               if (list_length(attrList) > rel->rd_att->natts)
>>               {
>> -                     List       *atcmds = NIL;
>>                       ListCell   *c;
>>                       int                     skip = rel->rd_att->natts;
>>
>>                       foreach(c, attrList)
>>                       {
>> -                             AlterTableCmd *atcmd;
>> -
>>                               if (skip > 0)
>>                               {
>>                                       skip--;
>> @@ -229,10 +229,24 @@ DefineVirtualRelation(const RangeVar *relation, List *tlist, bool replace)
>>                               atcmd->def = (Node *) lfirst(c);
>>                               atcmds = lappend(atcmds, atcmd);
>>                       }
>> -                     AlterTableInternal(viewOid, atcmds, true);
>>               }
>>
>>               /*
>> +              * If optional parameters are specified, we must set options
>> +              * using ALTER TABLE SET OPTION internally.
>
> I think CREATE OR REPLACE VIEW should replace the option list, while ALTER
> VIEW SET OPTION should retain its current behavior.  That is, this should
> leave the view with no options set:
>
> create or replace view v0(n) with (security_barrier) as values (1), (2), (3), (4);
> select reloptions from pg_class where oid = 'v0'::regclass;
> create or replace view v0(n) as values (4), (3), (2), (1);
> select reloptions from pg_class where oid = 'v0'::regclass;
>
>> +              */
>> +             if (list_length(options) > 0)
>> +             {
>> +                     atcmd = makeNode(AlterTableCmd);
>> +                     atcmd->subtype = AT_SetRelOptions;
>> +                     atcmd->def = options;
>
> This line produces a warning:
>
> view.c: In function `DefineVirtualRelation':
> view.c:240: warning: assignment from incompatible pointer type
>
>> +
>> +                     atcmds = lappend(atcmds, atcmd);
>> +             }
>> +             if (atcmds != NIL)
>> +                     AlterTableInternal(viewOid, atcmds, true);
>> +
>> +             /*
>>                * Seems okay, so return the OID of the pre-existing view.
>>                */
>>               relation_close(rel, NoLock);    /* keep the lock! */
>
> Part 1:
>> --- a/src/backend/optimizer/plan/planner.c
>> +++ b/src/backend/optimizer/plan/planner.c
>
>> @@ -2993,6 +3001,131 @@ get_column_info_for_window(PlannerInfo *root, WindowClause *wc, List *tlist,
>>       }
>>  }
>>
>> +/*
>> + * mark_qualifiers_depth
>> + *
>> + * It marks depth field of the each expression nodes that eventually
>> + * invokes functions, to track the original nest-level. On the evaluation
>> + * of qualifiers within WHERE or JOIN ... ON clauses during relation scans,
>> + * these items shall be reordered according to the nest-level and estimated
>> + * cost.
>> + * The optimizer may pull-up simple sub-queries or join clause, and
>> + * qualifiers to filter out tuples shall be mixed with ones in upper-
>> + * level. Thus, we need to track the original nest-level of qualifiers
>> + * to prevent reverse of order in evaluation, because some of qualifiers
>> + * can have side-effects that allows to leak supplied argument to outside.
>> + * It can be abused to break row-level security using a user defined function
>> + * with very small estimated cost, so nest level of qualifiers originated
>> + * from is used as a criteria, rather than estimated cost, to decide order
>> + * to evaluate qualifiers.
>> + */
>> +static bool
>> +mark_qualifiers_depth_walker(Node *node, void *context)
>> +{
>> +     int             depth = *((int *)(context));
>> +
>> +     if (node == NULL)
>> +             return false;
>> +     if (IsA(node, FuncExpr))
>> +     {
>> +             FuncExpr   *exp = (FuncExpr *)node;
>> +
>> +             exp->depth = depth | (exp->depth & 1);
>
> Why did these change from plain "exp->depth = depth;" of the last version?
> Since no core code sets a 1-bit in a depth value, I assume it must be related
> to your future-use design for that bit.  If so: could an external module
> realistically take advantage of this?  If yes, then a mere comment is in
> order.  If not, I think we should remove this (and the incrementing by 2) and
> add it again in the future patch that makes use thereof.
>
>> --- a/src/test/regress/sql/select_views.sql
>> +++ b/src/test/regress/sql/select_views.sql
>> @@ -8,3 +8,42 @@ SELECT * FROM street;
>>  SELECT name, #thepath FROM iexit ORDER BY 1, 2;
>>
>>  SELECT * FROM toyemp WHERE name = 'sharon';
>> +
>> +--
>> +-- Test for leaky-view
>> +--
>> +
>> +CREATE USER alice;
>> +CREATE FUNCTION f_leak(text, text)
>> +        RETURNS bool LANGUAGE 'plpgsql'
>> +        COST 0.00000001
>> +        AS 'begin raise notice ''% => %'', $1, $2; return true; end';
>> +CREATE TABLE credit_cards (
>> +    name   text,
>> +    number text,
>> +    expired    text
>> +);
>> +INSERT INTO credit_cards VALUES ('alice', '1111-2222-3333-4444', 'Aug-2012'),
>> +                                ('bob',   '5555-6666-7777-8888', 'Nov-2016'),
>> +                                ('eve',   '9801-2345-6789-0123', 'Jan-2018');
>> +CREATE VIEW your_credit_normal AS
>> +    SELECT * FROM credit_cards WHERE name = getpgusername();
>> +CREATE VIEW your_credit_secure WITH (security_barrier) AS
>> +    SELECT * FROM credit_cards WHERE name = getpgusername();
>> +
>> +GRANT SELECT ON your_credit_normal TO public;
>> +GRANT SELECT ON your_credit_secure TO public;
>> +-- run leaky view
>> +SET SESSION AUTHORIZATION alice;
>> +
>> +SELECT * FROM your_credit_normal WHERE f_leak(number,expired);
>> +EXPLAIN (COSTS OFF) SELECT * FROM your_credit_normal WHERE f_leak(number,expired);
>> +
>> +SELECT * FROM your_credit_secure WHERE f_leak(number,expired);
>> +EXPLAIN (COSTS OFF) SELECT * FROM your_credit_secure WHERE f_leak(number,expired);
>> +
>> +\c -
>> +-- cleanups
>> +DROP ROLE IF EXISTS alice;
>> +DROP FUNCTION IF EXISTS f_leak(text);
>> +DROP TABLE IF EXISTS credit_cards CASCADE;
>
> Keep the view around.  That way, pg_dump tests of the regression database will
> test the dumping of this view option.  (Your pg_dump support for this feature
> does work fine, though.)
>
> Thanks,
> nm
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
> *** a/src/backend/commands/view.c
> --- b/src/backend/commands/view.c

> --- 227,257 ----
> atcmd->def = (Node *) lfirst(c);
> atcmds = lappend(atcmds, atcmd);
> }
> }
>
> /*
> + * If optional parameters are specified, we must set options
> + * using ALTER TABLE SET OPTION internally.
> + */
> + if (list_length(options) > 0)
> + {
> + atcmd = makeNode(AlterTableCmd);
> + atcmd->subtype = AT_SetRelOptions;
> + atcmd->def = (List *)options;
> +
> + atcmds = lappend(atcmds, atcmd);
> + }
> + else
> + {
> + atcmd = makeNode(AlterTableCmd);
> + atcmd->subtype = AT_ResetRelOptions;
> + atcmd->def = (Node *) list_make1(makeDefElem("security_barrier",
> + NULL));
> + }
> + if (atcmds != NIL)
> + AlterTableInternal(viewOid, atcmds, true);
> +
> + /*
> * Seems okay, so return the OID of the pre-existing view.
> */
> relation_close(rel, NoLock); /* keep the lock! */

That gets the job done for today, but DefineVirtualRelation() should not need
to know all view options by name to simply replace the existing list with a
new one. I don't think you can cleanly use the ALTER TABLE SET/RESET code for
this. Instead, compute an option list similar to how DefineRelation() does so
at tablecmds.c:491, then update pg_class.

> 2011/7/5 Noah Misch <noah(at)2ndquadrant(dot)com>:
> > On Sun, Jul 03, 2011 at 11:33:38AM +0200, Kohei KaiGai wrote:
> >> --- a/src/test/regress/sql/select_views.sql
> >> +++ b/src/test/regress/sql/select_views.sql

> >> +-- cleanups
> >> +DROP ROLE IF EXISTS alice;
> >> +DROP FUNCTION IF EXISTS f_leak(text);
> >> +DROP TABLE IF EXISTS credit_cards CASCADE;
> >
> > Keep the view around.  That way, pg_dump tests of the regression database will
> > test the dumping of this view option.  (Your pg_dump support for this feature
> > does work fine, though.)

The latest version of part 1 still drops everything here.

2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
> On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
>> *** a/src/backend/commands/view.c
>> --- b/src/backend/commands/view.c
>
>> --- 227,257 ----
>>                               atcmd->def = (Node *) lfirst(c);
>>                               atcmds = lappend(atcmds, atcmd);
>>                       }
>>               }
>>
>>               /*
>> +              * If optional parameters are specified, we must set options
>> +              * using ALTER TABLE SET OPTION internally.
>> +              */
>> +             if (list_length(options) > 0)
>> +             {
>> +                     atcmd = makeNode(AlterTableCmd);
>> +                     atcmd->subtype = AT_SetRelOptions;
>> +                     atcmd->def = (List *)options;
>> +
>> +                     atcmds = lappend(atcmds, atcmd);
>> +             }
>> +             else
>> +             {
>> +                     atcmd = makeNode(AlterTableCmd);
>> +                     atcmd->subtype = AT_ResetRelOptions;
>> +                     atcmd->def = (Node *) list_make1(makeDefElem("security_barrier",
>> +                                                                                                              NULL));
>> +             }
>> +             if (atcmds != NIL)
>> +                     AlterTableInternal(viewOid, atcmds, true);
>> +
>> +             /*
>>                * Seems okay, so return the OID of the pre-existing view.
>>                */
>>               relation_close(rel, NoLock);    /* keep the lock! */
>
> That gets the job done for today, but DefineVirtualRelation() should not need
> to know all view options by name to simply replace the existing list with a
> new one.  I don't think you can cleanly use the ALTER TABLE SET/RESET code for
> this.  Instead, compute an option list similar to how DefineRelation() does so
> at tablecmds.c:491, then update pg_class.
>
My opinion is ALTER TABLE SET/RESET code should be enhanced to accept
an operation to reset all the existing options, rather than tricky
updates of pg_class.
How about an idea to add AT_ResetAllRelOptions for internal use only?

I'll fix up the regression test also.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Thu, Jul 7, 2011 at 10:56 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept
> an operation to reset all the existing options, rather than tricky
> updates of pg_class.
> How about an idea to add AT_ResetAllRelOptions for internal use only?
>
> I'll fix up the regression test also.

On an only semi-related note, ISTM that you may as well merge parts 0,
1, and 2 into a single patch, since there is no way we are going to
apply any of them without the others. I suggest closing one of the
CommitFest entries and revising the other one to point to the
consolidated patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote:
> 2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
> > On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
> >> *** a/src/backend/commands/view.c
> >> --- b/src/backend/commands/view.c
> >
> >> --- 227,257 ----
> >>                               atcmd->def = (Node *) lfirst(c);
> >>                               atcmds = lappend(atcmds, atcmd);
> >>                       }
> >>               }
> >>
> >>               /*
> >> +              * If optional parameters are specified, we must set options
> >> +              * using ALTER TABLE SET OPTION internally.
> >> +              */
> >> +             if (list_length(options) > 0)
> >> +             {
> >> +                     atcmd = makeNode(AlterTableCmd);
> >> +                     atcmd->subtype = AT_SetRelOptions;
> >> +                     atcmd->def = (List *)options;
> >> +
> >> +                     atcmds = lappend(atcmds, atcmd);
> >> +             }
> >> +             else
> >> +             {
> >> +                     atcmd = makeNode(AlterTableCmd);
> >> +                     atcmd->subtype = AT_ResetRelOptions;
> >> +                     atcmd->def = (Node *) list_make1(makeDefElem("security_barrier",
> >> +                                                                                                              NULL));
> >> +             }
> >> +             if (atcmds != NIL)
> >> +                     AlterTableInternal(viewOid, atcmds, true);
> >> +
> >> +             /*
> >>                * Seems okay, so return the OID of the pre-existing view.
> >>                */
> >>               relation_close(rel, NoLock);    /* keep the lock! */
> >
> > That gets the job done for today, but DefineVirtualRelation() should not need
> > to know all view options by name to simply replace the existing list with a
> > new one.  I don't think you can cleanly use the ALTER TABLE SET/RESET code for
> > this.  Instead, compute an option list similar to how DefineRelation() does so
> > at tablecmds.c:491, then update pg_class.
> >
> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept
> an operation to reset all the existing options, rather than tricky
> updates of pg_class.

The pg_class update has ~20 lines of idiomatic code; see tablecmds.c:7931-7951.

> How about an idea to add AT_ResetAllRelOptions for internal use only?

If some operation is purely internal and does not otherwise benefit from the
ALTER TABLE infrastructure, there's no benefit in involving ALTER TABLE.
DefineVirtualRelation() uses ALTER TABLE to add columns because all that code
needs to exist anyway. You could make a plain function to do the update that
gets called from both ATExecSetRelOptions() and DefineVirtualRelation().

Thanks,
nm

On Sun, Jul 03, 2011 at 11:41:47AM +0200, Kohei KaiGai wrote:
> The simplified version of fix-leaky-view patch. The part of reloptions
> for views got splitted out
> into the part-0 patch, so it needs to be applied prior to this patch.
> Rest of logic to prevent unexpected pushing down across security
> barrier is not changed.
>
> Thanks,
>
> 2011/6/6 Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com>:
> > This patch enables to fix up leaky-view problem using qualifiers that reference only one-side of join-loop inside of view definition.
> >
> > The point of this scenario is criteria to distribute qualifiers of scanning-plan distributed in distribute_qual_to_rels(). If and when a qualifiers that reference only one-side of join-loop, the optimizer may distribute this qualifier into inside of the join-loop, even if it goes over the boundary of a subquery expanded from a view for row-level security.
> > This behavior allows us to reference whole of one-side of join-loop using functions with side-effects.
> > The solution is quite simple; it prohibits to distribute qualifiers over the boundary of subquery, however, performance cost is unignorable, because it also disables to utilize obviously indexable qualifiers such as (id=123), so this patch requires users a hint whether a particular view is for row-level security, or not.
> >
> > This patch newly adds "CREATE SECURITY VIEW" statement that marks a flag to show this view was defined for row-level security purpose. This flag shall be stored as reloptions.
> > If this flag was set, the optimizer does not distribute qualifiers over the boundary of subqueries expanded from security views, except for obviously safe qualifiers.
> > (Right now, we consider built-in indexable operators are safe, but it might be arguable.)

I took a moderately-detailed look at this patch. This jumped out:

> --- a/src/backend/optimizer/util/clauses.c
> +++ b/src/backend/optimizer/util/clauses.c

> +static bool
> +contain_leakable_functions_walker(Node *node, void *context)
> +{
> + if (node == NULL)
> + return false;
> +
> + if (IsA(node, FuncExpr))
> + {
> + /*
> + * Right now, we have no way to distinguish safe functions with
> + * leakable ones, so, we treat all the function call possibly
> + * leakable.
> + */
> + return true;
> + }
> + else if (IsA(node, OpExpr))
> + {
> + OpExpr *expr = (OpExpr *) node;
> +
> + /*
> + * Right now, we assume operators implemented by built-in functions
> + * are not leakable, so it does not need to prevent optimization.
> + */
> + set_opfuncid(expr);
> + if (get_func_lang(expr->opfuncid) != INTERNALlanguageId)
> + return true;
> + /* else fall through to check args */
> + }

Any user can do this:

CREATE OPERATOR !-! (PROCEDURE = int4in, RIGHTARG = cstring);
SELECT !-! 'foo';

Making a distinction based simply on the call being an operator vs. a function
is a dead end. I see these options:

1. The user defining a security view can be assumed to trust the operator class
members of indexes defined on the tables he references. Keep track of which
those are and treat only them as non-leakable. This covers many interesting
cases, but it's probably tricky to implement and/or costly at runtime.

2. Add a pg_proc flag indicating whether the function is known leak-free.
Simple, but tedious and perhaps error-prone.

3. Trust operators owned by PGUID. This is simple and probably covers the
essential cases, but it's an ugly hack.

4. Trust nothing as leak-free. Simple; performance will be unattractive.

There are probably others.

2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
> On Sun, Jul 03, 2011 at 11:41:47AM +0200, Kohei KaiGai wrote:
>> The simplified version of fix-leaky-view patch. The part of reloptions
>> for views got splitted out
>> into the part-0 patch, so it needs to be applied prior to this patch.
>> Rest of logic to prevent unexpected pushing down across security
>> barrier is not changed.
>>
>> Thanks,
>>
>> 2011/6/6 Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com>:
>> > This patch enables to fix up leaky-view problem using qualifiers that reference only one-side of join-loop inside of view definition.
>> >
>> > The point of this scenario is criteria to distribute qualifiers of scanning-plan distributed in distribute_qual_to_rels(). If and when a qualifiers that reference only one-side of join-loop, the optimizer may distribute this qualifier into inside of the join-loop, even if it goes over the boundary of a subquery expanded from a view for row-level security.
>> > This behavior allows us to reference whole of one-side of join-loop using functions with side-effects.
>> > The solution is quite simple; it prohibits to distribute qualifiers over the boundary of subquery, however, performance cost is unignorable, because it also disables to utilize obviously indexable qualifiers such as (id=123), so this patch requires users a hint whether a particular view is for row-level security, or not.
>> >
>> > This patch newly adds "CREATE SECURITY VIEW" statement that marks a flag to show this view was defined for row-level security purpose. This flag shall be stored as reloptions.
>> > If this flag was set, the optimizer does not distribute qualifiers over the boundary of subqueries expanded from security views, except for obviously safe qualifiers.
>> > (Right now, we consider built-in indexable operators are safe, but it might be arguable.)
>
> I took a moderately-detailed look at this patch.  This jumped out:
>
>> --- a/src/backend/optimizer/util/clauses.c
>> +++ b/src/backend/optimizer/util/clauses.c
>
>> +static bool
>> +contain_leakable_functions_walker(Node *node, void *context)
>> +{
>> +     if (node == NULL)
>> +             return false;
>> +
>> +     if (IsA(node, FuncExpr))
>> +     {
>> +             /*
>> +              * Right now, we have no way to distinguish safe functions with
>> +              * leakable ones, so, we treat all the function call possibly
>> +              * leakable.
>> +              */
>> +             return true;
>> +     }
>> +     else if (IsA(node, OpExpr))
>> +     {
>> +             OpExpr *expr = (OpExpr *) node;
>> +
>> +             /*
>> +              * Right now, we assume operators implemented by built-in functions
>> +              * are not leakable, so it does not need to prevent optimization.
>> +              */
>> +             set_opfuncid(expr);
>> +             if (get_func_lang(expr->opfuncid) != INTERNALlanguageId)
>> +                     return true;
>> +             /* else fall through to check args */
>> +     }
>
> Any user can do this:
>
>        CREATE OPERATOR !-! (PROCEDURE = int4in, RIGHTARG = cstring);
>        SELECT !-! 'foo';
>
As I mentioned at the source code comments, this ad-hoc assumption was
come from we have no way to distinguish a non-leaky function from others.
So, I definitely love the approach (2), because only trusted function creator
can determine whether it is possible leaky or not.

> Making a distinction based simply on the call being an operator vs. a function
> is a dead end.  I see these options:
>
> 1. The user defining a security view can be assumed to trust the operator class
> members of indexes defined on the tables he references.  Keep track of which
> those are and treat only them as non-leakable.  This covers many interesting
> cases, but it's probably tricky to implement and/or costly at runtime.
>
It requires DBA massive amount of detailed knowledge about functions underlying
operators used in a view. I don't think it is a realistic assumption.

> 2. Add a pg_proc flag indicating whether the function is known leak-free.
> Simple, but tedious and perhaps error-prone.
>
+1

> 3. Trust operators owned by PGUID.  This is simple and probably covers the
> essential cases, but it's an ugly hack.
>
Some of built-in functions are also leaky. For example, int4div raise an error
when we try to divid a particular value by zero.

> 4. Trust nothing as leak-free.  Simple; performance will be unattractive.
>
-1, Because of performance perspective.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On 08.07.2011 11:03, Kohei KaiGai wrote:
> 2011/7/7 Noah Misch<noah(at)2ndquadrant(dot)com>:
>> Making a distinction based simply on the call being an operator vs. a function
>> is a dead end. I see these options:
>>
>> 1. The user defining a security view can be assumed to trust the operator class
>> members of indexes defined on the tables he references. Keep track of which
>> those are and treat only them as non-leakable. This covers many interesting
>> cases, but it's probably tricky to implement and/or costly at runtime.
>>
> It requires DBA massive amount of detailed knowledge about functions underlying
> operators used in a view. I don't think it is a realistic assumption.
>
>> 2. Add a pg_proc flag indicating whether the function is known leak-free.
>> Simple, but tedious and perhaps error-prone.
>>
> +1

IMHO the situation from DBA's point of view is exactly opposite. Option
two requires deep knowledge of this leaky views issue. The DBA needs to
inspect any function he wants to mark as leak-free closely, and
understand that innocent-looking things like casts can cause leaks. That
is not feasible in practice. Option 1, however, requires no such
knowledge. Operators used in indexes are already expected to not throw
errors, or you would get errors when inserting certain values to the
table, for example.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
> On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote:
>> 2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
>> > On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
>> >> *** a/src/backend/commands/view.c
>> >> --- b/src/backend/commands/view.c
>> >
>> >> --- 227,257 ----
>> >>                               atcmd->def = (Node *) lfirst(c);
>> >>                               atcmds = lappend(atcmds, atcmd);
>> >>                       }
>> >>               }
>> >>
>> >>               /*
>> >> +              * If optional parameters are specified, we must set options
>> >> +              * using ALTER TABLE SET OPTION internally.
>> >> +              */
>> >> +             if (list_length(options) > 0)
>> >> +             {
>> >> +                     atcmd = makeNode(AlterTableCmd);
>> >> +                     atcmd->subtype = AT_SetRelOptions;
>> >> +                     atcmd->def = (List *)options;
>> >> +
>> >> +                     atcmds = lappend(atcmds, atcmd);
>> >> +             }
>> >> +             else
>> >> +             {
>> >> +                     atcmd = makeNode(AlterTableCmd);
>> >> +                     atcmd->subtype = AT_ResetRelOptions;
>> >> +                     atcmd->def = (Node *) list_make1(makeDefElem("security_barrier",
>> >> +                                                                                                              NULL));
>> >> +             }
>> >> +             if (atcmds != NIL)
>> >> +                     AlterTableInternal(viewOid, atcmds, true);
>> >> +
>> >> +             /*
>> >>                * Seems okay, so return the OID of the pre-existing view.
>> >>                */
>> >>               relation_close(rel, NoLock);    /* keep the lock! */
>> >
>> > That gets the job done for today, but DefineVirtualRelation() should not need
>> > to know all view options by name to simply replace the existing list with a
>> > new one.  I don't think you can cleanly use the ALTER TABLE SET/RESET code for
>> > this.  Instead, compute an option list similar to how DefineRelation() does so
>> > at tablecmds.c:491, then update pg_class.
>> >
>> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept
>> an operation to reset all the existing options, rather than tricky
>> updates of pg_class.
>
> The pg_class update has ~20 lines of idiomatic code; see tablecmds.c:7931-7951.
>
Even if idiomatic, another part of DefineVirtualRelation() uses
AlterTableInternal().
I think a common way is more straightforward.

So, how about an idea to add a function that pull-out existing options
from syscache,
and merge with the supplied options list prior to AlterTableInternal()?

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

2011/7/8 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> On 08.07.2011 11:03, Kohei KaiGai wrote:
>>
>> 2011/7/7 Noah Misch<noah(at)2ndquadrant(dot)com>:
>>>
>>> Making a distinction based simply on the call being an operator vs. a
>>> function
>>> is a dead end.  I see these options:
>>>
>>> 1. The user defining a security view can be assumed to trust the operator
>>> class
>>> members of indexes defined on the tables he references.  Keep track of
>>> which
>>> those are and treat only them as non-leakable.  This covers many
>>> interesting
>>> cases, but it's probably tricky to implement and/or costly at runtime.
>>>
>> It requires DBA massive amount of detailed knowledge about functions
>> underlying
>> operators used in a view. I don't think it is a realistic assumption.
>>
>>> 2. Add a pg_proc flag indicating whether the function is known leak-free.
>>> Simple, but tedious and perhaps error-prone.
>>>
>> +1
>
> IMHO the situation from DBA's point of view is exactly opposite. Option two
> requires deep knowledge of this leaky views issue. The DBA needs to inspect
> any function he wants to mark as leak-free closely, and understand that
> innocent-looking things like casts can cause leaks. That is not feasible in
> practice. Option 1, however, requires no such knowledge. Operators used in
> indexes are already expected to not throw errors, or you would get errors
> when inserting certain values to the table, for example.
>
I might misread his description at first.
Hmm. If we introduce DBA the scenario and the condition to push down qualifiers,
it may be possible to explain more simply.

A challenge of this approach is to determine what qualifier shall be
used to index
accesses in the stage of distribute_qual_to_rels(); prior to the
optimizer's selection
of access methods.
Do you have any good idea, or suggestion?

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

The attached patch is a revised one; that utilizes untransformRelOptions()
to construct a list of DefElem to be supplied into AT_ResetRelOptions
commands. It enabled me to implement more compact as I expected.

How about this approach to reset existing reloptions?

I'll consolidate part-0, 1 and 2 patches after we make fix the direction to
distinguish leaky qualifiers from others, in the thread of part-2.
Right now, I'm considering the right way to choose qualifiers to be
transformed into index scans.

Thanks,

2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
> On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote:
>> 2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
>> > On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
>> >> *** a/src/backend/commands/view.c
>> >> --- b/src/backend/commands/view.c
>> >
>> >> --- 227,257 ----
>> >>                               atcmd->def = (Node *) lfirst(c);
>> >>                               atcmds = lappend(atcmds, atcmd);
>> >>                       }
>> >>               }
>> >>
>> >>               /*
>> >> +              * If optional parameters are specified, we must set options
>> >> +              * using ALTER TABLE SET OPTION internally.
>> >> +              */
>> >> +             if (list_length(options) > 0)
>> >> +             {
>> >> +                     atcmd = makeNode(AlterTableCmd);
>> >> +                     atcmd->subtype = AT_SetRelOptions;
>> >> +                     atcmd->def = (List *)options;
>> >> +
>> >> +                     atcmds = lappend(atcmds, atcmd);
>> >> +             }
>> >> +             else
>> >> +             {
>> >> +                     atcmd = makeNode(AlterTableCmd);
>> >> +                     atcmd->subtype = AT_ResetRelOptions;
>> >> +                     atcmd->def = (Node *) list_make1(makeDefElem("security_barrier",
>> >> +                                                                                                              NULL));
>> >> +             }
>> >> +             if (atcmds != NIL)
>> >> +                     AlterTableInternal(viewOid, atcmds, true);
>> >> +
>> >> +             /*
>> >>                * Seems okay, so return the OID of the pre-existing view.
>> >>                */
>> >>               relation_close(rel, NoLock);    /* keep the lock! */
>> >
>> > That gets the job done for today, but DefineVirtualRelation() should not need
>> > to know all view options by name to simply replace the existing list with a
>> > new one.  I don't think you can cleanly use the ALTER TABLE SET/RESET code for
>> > this.  Instead, compute an option list similar to how DefineRelation() does so
>> > at tablecmds.c:491, then update pg_class.
>> >
>> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept
>> an operation to reset all the existing options, rather than tricky
>> updates of pg_class.
>
> The pg_class update has ~20 lines of idiomatic code; see tablecmds.c:7931-7951.
>
>> How about an idea to add AT_ResetAllRelOptions for internal use only?
>
> If some operation is purely internal and does not otherwise benefit from the
> ALTER TABLE infrastructure, there's no benefit in involving ALTER TABLE.
> DefineVirtualRelation() uses ALTER TABLE to add columns because all that code
> needs to exist anyway.  You could make a plain function to do the update that
> gets called from both ATExecSetRelOptions() and DefineVirtualRelation().
>
> Thanks,
> nm
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Fri, Jul 8, 2011 at 4:18 AM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> IMHO the situation from DBA's point of view is exactly opposite. Option two
> requires deep knowledge of this leaky views issue. The DBA needs to inspect
> any function he wants to mark as leak-free closely, and understand that
> innocent-looking things like casts can cause leaks. That is not feasible in
> practice. Option 1, however, requires no such knowledge. Operators used in
> indexes are already expected to not throw errors, or you would get errors
> when inserting certain values to the table, for example.

But, IMHO, the chance of the DBA wanting to set this flag is
miniscule. I think that 99.9% of DBAs will be perfectly happy to just
use whatever set we mark as built-ins. And an explicit pg_proc flag
gives us a lot more flexibility.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jul 08, 2011 at 09:20:46AM +0100, Kohei KaiGai wrote:
> 2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
> > On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote:
> >> 2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
> >> > On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:

> >> > That gets the job done for today, but DefineVirtualRelation() should not need
> >> > to know all view options by name to simply replace the existing list with a
> >> > new one. ?I don't think you can cleanly use the ALTER TABLE SET/RESET code for
> >> > this. ?Instead, compute an option list similar to how DefineRelation() does so
> >> > at tablecmds.c:491, then update pg_class.
> >> >
> >> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept
> >> an operation to reset all the existing options, rather than tricky
> >> updates of pg_class.
> >
> > The pg_class update has ~20 lines of idiomatic code; see tablecmds.c:7931-7951.
> >
> Even if idiomatic, another part of DefineVirtualRelation() uses
> AlterTableInternal().
> I think a common way is more straightforward.

The fact that we use ALTER TABLE ADD COLUMN in DefineVirtualRelation() is not
itself cause to use ALTER TABLE SET (...) nearby. We should do so only if it
brings some advantage, like simpler or more-robust code. I'm not seeing either
advantage. Those can be points of style, so perhaps I have the poor taste here.

> So, how about an idea to add a function that pull-out existing options
> from syscache,
> and merge with the supplied options list prior to AlterTableInternal()?

It seems wrong to me to trawl through the view's existing option list just to
replace it completely with a new option list. Again, it's subjective. If you'd
like to proceed with this and let the committer decide, that's fine with me.

Thanks,
nm

On Fri, Jul 08, 2011 at 10:09:54AM +0100, Kohei KaiGai wrote:
> 2011/7/8 Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>:
> > On 08.07.2011 11:03, Kohei KaiGai wrote:
> >>
> >> 2011/7/7 Noah Misch<noah(at)2ndquadrant(dot)com>:
> >>>
> >>> Making a distinction based simply on the call being an operator vs. a
> >>> function
> >>> is a dead end. ?I see these options:
> >>>
> >>> 1. The user defining a security view can be assumed to trust the operator
> >>> class
> >>> members of indexes defined on the tables he references. ?Keep track of
> >>> which
> >>> those are and treat only them as non-leakable. ?This covers many
> >>> interesting
> >>> cases, but it's probably tricky to implement and/or costly at runtime.
> >>>
> >> It requires DBA massive amount of detailed knowledge about functions
> >> underlying
> >> operators used in a view. I don't think it is a realistic assumption.
> >>
> >>> 2. Add a pg_proc flag indicating whether the function is known leak-free.
> >>> Simple, but tedious and perhaps error-prone.
> >>>
> >> +1
> >
> > IMHO the situation from DBA's point of view is exactly opposite. Option two
> > requires deep knowledge of this leaky views issue. The DBA needs to inspect
> > any function he wants to mark as leak-free closely, and understand that
> > innocent-looking things like casts can cause leaks. That is not feasible in
> > practice. Option 1, however, requires no such knowledge. Operators used in
> > indexes are already expected to not throw errors, or you would get errors
> > when inserting certain values to the table, for example.
> >
> I might misread his description at first.
> Hmm. If we introduce DBA the scenario and the condition to push down qualifiers,
> it may be possible to explain more simply.
>
> A challenge of this approach is to determine what qualifier shall be
> used to index
> accesses in the stage of distribute_qual_to_rels(); prior to the
> optimizer's selection
> of access methods.
> Do you have any good idea, or suggestion?

Note that it does not matter whether we're actually doing an index scan -- a seq
scan with a filter using only leakproof operators is equally acceptable. What I
had in mind was to enumerate all operators in operator classes of indexes below
each security view. Those become the leak-free operators for that security
view. If the operator for an OpExpr is considered leak-free by all sources of
its operands, then we may push it down. That's purely a high-level sketch: I
haven't considered implementation concerns in any detail. The resulting
behavior could be surprising: adding an index may change a plan without the new
plan actually using the index.

I lean toward favoring the pg_proc flag. Functions like "texteq" will be taken
as leakproof even if no involved table has an index on a text column. It works
for functions that will never take a place in an operator class, like
length(text). When a user reports a qualifier not getting pushed down, the
answer is much more satisfying: "Run 'CREATE OR REPLACE FUNCTION
... I_DONT_LEAK' as a superuser." Compare to "Define an operator class that
includes the function, if needed, and create an otherwise-useless index." The
main disadvantage I see is the loss of policy locality. Only a superuser (or
maybe database owner?) can create or modify declared-leakproof functions, and
that decision applies throughout the database. However, I think the other
advantages clearly outweigh that loss.

Incidentally, whichever policy we choose here can also loosen the constraints on
qualifier order (part 1 of your original submission).

On Fri, Jul 8, 2011 at 4:57 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
> Note that it does not matter whether we're actually doing an index scan -- a seq
> scan with a filter using only leakproof operators is equally acceptable.  What I
> had in mind was to enumerate all operators in operator classes of indexes below
> each security view.  Those become the leak-free operators for that security
> view.  If the operator for an OpExpr is considered leak-free by all sources of
> its operands, then we may push it down.  That's purely a high-level sketch: I
> haven't considered implementation concerns in any detail.  The resulting
> behavior could be surprising: adding an index may change a plan without the new
> plan actually using the index.
>
> I lean toward favoring the pg_proc flag.  Functions like "texteq" will be taken
> as leakproof even if no involved table has an index on a text column.  It works
> for functions that will never take a place in an operator class, like
> length(text).  When a user reports a qualifier not getting pushed down, the
> answer is much more satisfying: "Run 'CREATE OR REPLACE FUNCTION
> ... I_DONT_LEAK' as a superuser."  Compare to "Define an operator class that
> includes the function, if needed, and create an otherwise-useless index."  The
> main disadvantage I see is the loss of policy locality.  Only a superuser (or
> maybe database owner?) can create or modify declared-leakproof functions, and
> that decision applies throughout the database.  However, I think the other
> advantages clearly outweigh that loss.

This strikes me as a fairly compelling refutation of Heikki's proposed approach.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/7/8 Noah Misch <noah(at)2ndquadrant(dot)com>:
> On Fri, Jul 08, 2011 at 09:20:46AM +0100, Kohei KaiGai wrote:
>> 2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
>> > On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote:
>> >> 2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
>> >> > On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
>
>> >> > That gets the job done for today, but DefineVirtualRelation() should not need
>> >> > to know all view options by name to simply replace the existing list with a
>> >> > new one. ?I don't think you can cleanly use the ALTER TABLE SET/RESET code for
>> >> > this. ?Instead, compute an option list similar to how DefineRelation() does so
>> >> > at tablecmds.c:491, then update pg_class.
>> >> >
>> >> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept
>> >> an operation to reset all the existing options, rather than tricky
>> >> updates of pg_class.
>> >
>> > The pg_class update has ~20 lines of idiomatic code; see tablecmds.c:7931-7951.
>> >
>> Even if idiomatic, another part of DefineVirtualRelation() uses
>> AlterTableInternal().
>> I think a common way is more straightforward.
>
> The fact that we use ALTER TABLE ADD COLUMN in DefineVirtualRelation() is not
> itself cause to use ALTER TABLE SET (...) nearby.  We should do so only if it
> brings some advantage, like simpler or more-robust code.  I'm not seeing either
> advantage.  Those can be points of style, so perhaps I have the poor taste here.
>
The attached patch is a revised version according to the approach that updates
pg_class system catalog before AlterTableInternal().
It invokes the new ResetViewOptions when rel->rd_options is not null, and it set
null on the pg_class.reloptions of the view and increments command counter.

Rest of stuffs are not changed from the v5.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

2011/7/9 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Jul 8, 2011 at 4:57 PM, Noah Misch <noah(at)2ndquadrant(dot)com> wrote:
>> Note that it does not matter whether we're actually doing an index scan -- a seq
>> scan with a filter using only leakproof operators is equally acceptable.  What I
>> had in mind was to enumerate all operators in operator classes of indexes below
>> each security view.  Those become the leak-free operators for that security
>> view.  If the operator for an OpExpr is considered leak-free by all sources of
>> its operands, then we may push it down.  That's purely a high-level sketch: I
>> haven't considered implementation concerns in any detail.  The resulting
>> behavior could be surprising: adding an index may change a plan without the new
>> plan actually using the index.
>>
>> I lean toward favoring the pg_proc flag.  Functions like "texteq" will be taken
>> as leakproof even if no involved table has an index on a text column.  It works
>> for functions that will never take a place in an operator class, like
>> length(text).  When a user reports a qualifier not getting pushed down, the
>> answer is much more satisfying: "Run 'CREATE OR REPLACE FUNCTION
>> ... I_DONT_LEAK' as a superuser."  Compare to "Define an operator class that
>> includes the function, if needed, and create an otherwise-useless index."  The
>> main disadvantage I see is the loss of policy locality.  Only a superuser (or
>> maybe database owner?) can create or modify declared-leakproof functions, and
>> that decision applies throughout the database.  However, I think the other
>> advantages clearly outweigh that loss.
>
> This strikes me as a fairly compelling refutation of Heikki's proposed approach.
>
OK, I'll try to modify the patch according to the flag of pg_proc design.
As long as the default of user-defined function is off, and we provide
built-in functions
with appropriate configurations, it seems to me the burden of DBA is
quite limited.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
> The attached patch is a revised version according to the approach that updates
> pg_class system catalog before AlterTableInternal().
> It invokes the new ResetViewOptions when rel->rd_options is not null, and it set
> null on the pg_class.reloptions of the view and increments command counter.

> + /*
> + * ResetViewOptions
> + *
> + * It clears all the reloptions prior to replacing
> + */
> + static void
> + ResetViewOptions(Oid viewOid)
> + {
> + Relation pg_class;
> + HeapTuple oldtup;
> + HeapTuple newtup;
> + Datum values[Natts_pg_class];
> + bool nulls[Natts_pg_class];
> + bool replaces[Natts_pg_class];
> +
> + pg_class = heap_open(RelationRelationId, RowExclusiveLock);
> +
> + oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));

Use SearchSysCacheCopy1, since you're modifying the tuple.

> + if (!HeapTupleIsValid(oldtup))
> + elog(ERROR, "cache lookup failed for relation %u", viewOid);
> +
> + memset(values, 0, sizeof(values));
> + memset(nulls, false, sizeof(nulls));
> + memset(replaces, false, sizeof(replaces));
> +
> + replaces[Anum_pg_class_reloptions - 1] = true;
> + nulls[Anum_pg_class_reloptions - 1] = true;
> +
> + newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
> + values, nulls, replaces);
> + simple_heap_update(pg_class, &newtup->t_self, newtup);
> +
> + CatalogUpdateIndexes(pg_class, newtup);
> +
> + ReleaseSysCache(oldtup);
> +
> + heap_close(pg_class, RowExclusiveLock);
> +
> + CommandCounterIncrement();

Why is a CCI necessary?

> + }

In any event, we seem to be converging on a version of parts 0 and 1 that are
ready for committer. However, Robert contends that this will not be committed
separately from part 2. Unless someone wishes to contest that, I suggest we
mark this Returned with Feedback and let the CF entry for part 2 subsume its
future development. Does that sound reasonable?

Thanks,
nm

2011/7/9 Noah Misch <noah(at)2ndquadrant(dot)com>:
> On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
>> The attached patch is a revised version according to the approach that updates
>> pg_class system catalog before AlterTableInternal().
>> It invokes the new ResetViewOptions when rel->rd_options is not null, and it set
>> null on the pg_class.reloptions of the view and increments command counter.
>
>> + /*
>> +  * ResetViewOptions
>> +  *
>> +  * It clears all the reloptions prior to replacing
>> +  */
>> + static void
>> + ResetViewOptions(Oid viewOid)
>> + {
>> +     Relation        pg_class;
>> +     HeapTuple       oldtup;
>> +     HeapTuple       newtup;
>> +     Datum           values[Natts_pg_class];
>> +     bool            nulls[Natts_pg_class];
>> +     bool            replaces[Natts_pg_class];
>> +
>> +     pg_class = heap_open(RelationRelationId, RowExclusiveLock);
>> +
>> +     oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));
>
> Use SearchSysCacheCopy1, since you're modifying the tuple.
>
The heap_modify_tuple() allocates a new tuple as a copy of old tuple.
No need to worry about.

>> +     if (!HeapTupleIsValid(oldtup))
>> +             elog(ERROR, "cache lookup failed for relation %u", viewOid);
>> +
>> +     memset(values, 0, sizeof(values));
>> +     memset(nulls, false, sizeof(nulls));
>> +     memset(replaces, false, sizeof(replaces));
>> +
>> +     replaces[Anum_pg_class_reloptions - 1] = true;
>> +     nulls[Anum_pg_class_reloptions - 1] = true;
>> +
>> +     newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
>> +                                                        values, nulls, replaces);
>> +     simple_heap_update(pg_class, &newtup->t_self, newtup);
>> +
>> +     CatalogUpdateIndexes(pg_class, newtup);
>> +
>> +     ReleaseSysCache(oldtup);
>> +
>> +     heap_close(pg_class, RowExclusiveLock);
>> +
>> +     CommandCounterIncrement();
>
> Why is a CCI necessary?
>
ATExecSetRelOptions() reference the view to be updated using syscache,
however, this update will not become visible without CCI.
In the result, it will reference old tuple, then get an error because
it tries to
update already updated tuple.

>> + }
>
> In any event, we seem to be converging on a version of parts 0 and 1 that are
> ready for committer.  However, Robert contends that this will not be committed
> separately from part 2.  Unless someone wishes to contest that, I suggest we
> mark this Returned with Feedback and let the CF entry for part 2 subsume its
> future development.  Does that sound reasonable?
>
At least, it seems to me we don't need to tackle to this matter from
the beginning
on the next commit fest again.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sat, Jul 09, 2011 at 10:52:33AM +0200, Kohei KaiGai wrote:
> 2011/7/9 Noah Misch <noah(at)2ndquadrant(dot)com>:
> > On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
> >> The attached patch is a revised version according to the approach that updates
> >> pg_class system catalog before AlterTableInternal().
> >> It invokes the new ResetViewOptions when rel->rd_options is not null, and it set
> >> null on the pg_class.reloptions of the view and increments command counter.
> >
> >> + /*
> >> +  * ResetViewOptions
> >> +  *
> >> +  * It clears all the reloptions prior to replacing
> >> +  */
> >> + static void
> >> + ResetViewOptions(Oid viewOid)
> >> + {
> >> +     Relation        pg_class;
> >> +     HeapTuple       oldtup;
> >> +     HeapTuple       newtup;
> >> +     Datum           values[Natts_pg_class];
> >> +     bool            nulls[Natts_pg_class];
> >> +     bool            replaces[Natts_pg_class];
> >> +
> >> +     pg_class = heap_open(RelationRelationId, RowExclusiveLock);
> >> +
> >> +     oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));
> >
> > Use SearchSysCacheCopy1, since you're modifying the tuple.
> >
> The heap_modify_tuple() allocates a new tuple as a copy of old tuple.
> No need to worry about.

Ah, yes. Sorry for the noise.

> >> +     if (!HeapTupleIsValid(oldtup))
> >> +             elog(ERROR, "cache lookup failed for relation %u", viewOid);
> >> +
> >> +     memset(values, 0, sizeof(values));
> >> +     memset(nulls, false, sizeof(nulls));
> >> +     memset(replaces, false, sizeof(replaces));
> >> +
> >> +     replaces[Anum_pg_class_reloptions - 1] = true;
> >> +     nulls[Anum_pg_class_reloptions - 1] = true;
> >> +
> >> +     newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
> >> +                                                        values, nulls, replaces);
> >> +     simple_heap_update(pg_class, &newtup->t_self, newtup);
> >> +
> >> +     CatalogUpdateIndexes(pg_class, newtup);
> >> +
> >> +     ReleaseSysCache(oldtup);
> >> +
> >> +     heap_close(pg_class, RowExclusiveLock);
> >> +
> >> +     CommandCounterIncrement();
> >
> > Why is a CCI necessary?
> >
> ATExecSetRelOptions() reference the view to be updated using syscache,
> however, this update will not become visible without CCI.
> In the result, it will reference old tuple, then get an error because
> it tries to
> update already updated tuple.

Okay, thanks.

On 2011-07-09 09:14, Kohei KaiGai wrote:
> OK, I'll try to modify the patch according to the flag of pg_proc design.
> As long as the default of user-defined function is off, and we provide
> built-in functions
> with appropriate configurations, it seems to me the burden of DBA is
> quite limited.

A different solution to the leaky view problem could be to check access
to a tuple at or near the heaptuple visibility level, in addition to
adding tuple access filter conditions to the query. This would have both
the possible performance benefits of the query rewriting solution, as
the everything is filtered before further processing at the heaptuple
visibility level. Fixing leaky views is not needed because they don't
exist in this case, the code is straightforward, and there's less change
of future security bugs by either misconfiguration of leakproof
functions or code that might introduce another leak path.

regards,

--
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data

2011/7/20 Yeb Havinga <yebhavinga(at)gmail(dot)com>:
> On 2011-07-09 09:14, Kohei KaiGai wrote:
>>
>> OK, I'll try to modify the patch according to the flag of pg_proc design.
>> As long as the default of user-defined function is off, and we provide
>> built-in functions
>> with appropriate configurations, it seems to me the burden of DBA is
>> quite limited.
>
> A different solution to the leaky view problem could be to check access to a
> tuple at or near the heaptuple visibility level, in addition to adding tuple
> access filter conditions to the query. This would have both the possible
> performance benefits of the query rewriting solution, as the everything is
> filtered before further processing at the heaptuple visibility level. Fixing
> leaky views is not needed because they don't exist in this case, the code is
> straightforward, and there's less change of future security bugs by either
> misconfiguration of leakproof functions or code that might introduce another
> leak path.
>
I'm not fun with this approach. The harderst one to find out a solution is
a way to distinguish qualifiers of security policy and others.
Leaky functions looks like a harmless function, them the optimizer will
distribute them onto particular scan plans.
If it was executed on the visibility check of tuples, same problem will be
reproduced. So, I'm still fun with a flag of pg_proc catalog and idea of
security barrier.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Wed, Jul 20, 2011 at 09:02:59AM +0200, Yeb Havinga wrote:
> On 2011-07-09 09:14, Kohei KaiGai wrote:
>> OK, I'll try to modify the patch according to the flag of pg_proc design.
>> As long as the default of user-defined function is off, and we provide
>> built-in functions
>> with appropriate configurations, it seems to me the burden of DBA is
>> quite limited.
>
> A different solution to the leaky view problem could be to check access
> to a tuple at or near the heaptuple visibility level, in addition to
> adding tuple access filter conditions to the query. This would have both
> the possible performance benefits of the query rewriting solution, as
> the everything is filtered before further processing at the heaptuple
> visibility level. Fixing leaky views is not needed because they don't
> exist in this case, the code is straightforward, and there's less change
> of future security bugs by either misconfiguration of leakproof
> functions or code that might introduce another leak path.

The SQL-level semantics of the view define the access rules in question. How
would you translate that into tests to apply at a lower level?

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

On 2011-07-20 16:06, Noah Misch wrote:
>
> The SQL-level semantics of the view define the access rules in question. How
> would you translate that into tests to apply at a lower level?
I assumed the leaky view thread was about row level security, not about
access rules to views, since it was mentioned at the RLS wiki page for
se-pgsql. Sorry for the confusion.

regards,
Yeb

On 2011-07-20 16:15, Yeb Havinga wrote:
> On 2011-07-20 16:06, Noah Misch wrote:
>>
>> The SQL-level semantics of the view define the access rules in
>> question. How
>> would you translate that into tests to apply at a lower level?
> I assumed the leaky view thread was about row level security, not
> about access rules to views, since it was mentioned at the RLS wiki
> page for se-pgsql. Sorry for the confusion.
Had to digg a bit for the wiki, it was this one :
http://wiki.postgresql.org/wiki/RLS#Issue:_A_leaky_VIEWs_for_RLS

regards,
Yeb

On Wed, Jul 20, 2011 at 04:23:10PM +0200, Yeb Havinga wrote:
> On 2011-07-20 16:15, Yeb Havinga wrote:
>> On 2011-07-20 16:06, Noah Misch wrote:
>>>
>>> The SQL-level semantics of the view define the access rules in
>>> question. How
>>> would you translate that into tests to apply at a lower level?
>> I assumed the leaky view thread was about row level security, not
>> about access rules to views, since it was mentioned at the RLS wiki
>> page for se-pgsql. Sorry for the confusion.
> Had to digg a bit for the wiki, it was this one :
> http://wiki.postgresql.org/wiki/RLS#Issue:_A_leaky_VIEWs_for_RLS

It is about row-level security, broadly. These patches close the hazard
described in the latter half of this page:
http://www.postgresql.org/docs/9.0/static/rules-privileges.html

In the example given there, "phone NOT LIKE '412%'" is the (row-level) access
rule that needs to apply before any possibly-leaky function sees the tuple.

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