Re: [v9.2] Fix Leaky View Problem

Robert Haas 09/25/11 10:58 AM >>>

> I'm not sure we've been 100% consistent about that, since we
> previously made CREATE OR REPLACE LANGUAGE not replace the owner
> with the current user.

I think we've been consistent in *not* changing security on an
object when it is replaced.

test=# create user someoneelse;
CREATE ROLE
test=# create user yetanother;
CREATE ROLE
test=# create function one() returns int language sql as 'select 1;';
CREATE FUNCTION
test=# alter function one() owner to someoneelse;
ALTER FUNCTION
test=# revoke execute on function one() from public;
REVOKE
test=# create or replace function one() returns int language plpgsql as
$$begin return 1; end;$$;
CREATE FUNCTION
test=# \df+ one()
List of
functions
Schema | Name | Result data type | Argument data types | Type |
Volatility | Owner | Language | Source code | Description

--------+------+------------------+---------------------+--------+------------+-------------+----------+----------------------+-------------
public | one | integer | | normal |
volatile | someoneelse | plpgsql | begin return 1; end; |
(1 row)

test=# set role yetanother;
SET
test=> select one();
ERROR: permission denied for function one

-Kevin

On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote:
> Robert Haas 09/25/11 10:58 AM >>>
>
> > I'm not sure we've been 100% consistent about that, since we
> > previously made CREATE OR REPLACE LANGUAGE not replace the owner
> > with the current user.
>
> I think we've been consistent in *not* changing security on an
> object when it is replaced.

> [CREATE OR REPLACE FUNCTION does not change proowner or proacl]

Good point. C-O-R VIEW also preserves column default values. I believe we are
consistent to the extent that everything possible to specify in each C-O-R
statement gets replaced outright. The preserved characteristics *require*
commands like GRANT, COMMENT and ALTER VIEW to set in the first place.

The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to
SECURITY INVOKER if it's not specified each time. That default is safe, though,
while the proposed default of security_barrier=false is unsafe.

Thanks,
nm

On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote:
>> Robert Haas  09/25/11 10:58 AM >>>
>>
>> > I'm not sure we've been 100% consistent about that, since we
>> > previously made CREATE OR REPLACE LANGUAGE not replace the owner
>> > with the current user.
>>
>> I think we've been consistent in *not* changing security on an
>> object when it is replaced.
>
>> [CREATE OR REPLACE FUNCTION does not change proowner or proacl]
>
> Good point.  C-O-R VIEW also preserves column default values.  I believe we are
> consistent to the extent that everything possible to specify in each C-O-R
> statement gets replaced outright.  The preserved characteristics *require*
> commands like GRANT, COMMENT and ALTER VIEW to set in the first place.
>
> The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to
> SECURITY INVOKER if it's not specified each time.  That default is safe, though,
> while the proposed default of security_barrier=false is unsafe.

Even though I normally take the opposite position, I still like the
idea of dedicated syntax for this feature. Not knowing what view
options we might end up with in the future, I hate having to decide on
what the general behavior ought to be. But it would be easy to decide
that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves
the security flag set; it would be consistent with what we're doing
with owner and acl information and wouldn't back us into any
unpleasant decisions down the road.

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

2011/9/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote:
>>> Robert Haas  09/25/11 10:58 AM >>>
>>>
>>> > I'm not sure we've been 100% consistent about that, since we
>>> > previously made CREATE OR REPLACE LANGUAGE not replace the owner
>>> > with the current user.
>>>
>>> I think we've been consistent in *not* changing security on an
>>> object when it is replaced.
>>
>>> [CREATE OR REPLACE FUNCTION does not change proowner or proacl]
>>
>> Good point.  C-O-R VIEW also preserves column default values.  I believe we are
>> consistent to the extent that everything possible to specify in each C-O-R
>> statement gets replaced outright.  The preserved characteristics *require*
>> commands like GRANT, COMMENT and ALTER VIEW to set in the first place.
>>
>> The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to
>> SECURITY INVOKER if it's not specified each time.  That default is safe, though,
>> while the proposed default of security_barrier=false is unsafe.
>
> Even though I normally take the opposite position, I still like the
> idea of dedicated syntax for this feature.  Not knowing what view
> options we might end up with in the future, I hate having to decide on
> what the general behavior ought to be.  But it would be easy to decide
> that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves
> the security flag set; it would be consistent with what we're doing
> with owner and acl information and wouldn't back us into any
> unpleasant decisions down the road.
>
Does the CREATE SECURITY VIEW statement mean a synonym of
CREATE VIEW ... WITH (security_barrier=true) ?

If so, it seems to me reasonable to keep the configuration when user
provides no explicit option.

1) an explicit WITH(security_barrier=true) / CREATE SECURITY VIEW
-> It always turns on a security_barrier option.

2) an explicit WITH(security_barrier=false)
-> It always turns off security_barrier option.

3) no explicit option / CREATE VIEW
-> Keep existing configuration, although inconsist with SECURITY DEFINER

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

On Mon, Sep 26, 2011 at 6:28 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2011/9/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>>> On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote:
>>>> Robert Haas  09/25/11 10:58 AM >>>
>>>>
>>>> > I'm not sure we've been 100% consistent about that, since we
>>>> > previously made CREATE OR REPLACE LANGUAGE not replace the owner
>>>> > with the current user.
>>>>
>>>> I think we've been consistent in *not* changing security on an
>>>> object when it is replaced.
>>>
>>>> [CREATE OR REPLACE FUNCTION does not change proowner or proacl]
>>>
>>> Good point.  C-O-R VIEW also preserves column default values.  I believe we are
>>> consistent to the extent that everything possible to specify in each C-O-R
>>> statement gets replaced outright.  The preserved characteristics *require*
>>> commands like GRANT, COMMENT and ALTER VIEW to set in the first place.
>>>
>>> The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to
>>> SECURITY INVOKER if it's not specified each time.  That default is safe, though,
>>> while the proposed default of security_barrier=false is unsafe.
>>
>> Even though I normally take the opposite position, I still like the
>> idea of dedicated syntax for this feature.  Not knowing what view
>> options we might end up with in the future, I hate having to decide on
>> what the general behavior ought to be.  But it would be easy to decide
>> that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves
>> the security flag set; it would be consistent with what we're doing
>> with owner and acl information and wouldn't back us into any
>> unpleasant decisions down the road.
>>
> Does the CREATE SECURITY VIEW statement mean a synonym of
> CREATE VIEW ... WITH (security_barrier=true) ?
>
> If so, it seems to me reasonable to keep the configuration when user
> provides no explicit option.
>
> 1) an explicit WITH(security_barrier=true) / CREATE SECURITY VIEW
>  -> It always turns on a security_barrier option.
>
> 2) an explicit WITH(security_barrier=false)
>  -> It always turns off security_barrier option.
>
> 3) no explicit option / CREATE VIEW
>  -> Keep existing configuration, although inconsist with SECURITY DEFINER

No, you're missing my point completely. If we use a flexible options
syntax here, then we have to decide on what behavior CREATE OR REPLACE
should have for all future options, without knowing what they are yet,
or what behavior will be appropriate.

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

2011/9/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Sep 26, 2011 at 6:28 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 2011/9/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>>>> On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote:
>>>>> Robert Haas  09/25/11 10:58 AM >>>
>>>>>
>>>>> > I'm not sure we've been 100% consistent about that, since we
>>>>> > previously made CREATE OR REPLACE LANGUAGE not replace the owner
>>>>> > with the current user.
>>>>>
>>>>> I think we've been consistent in *not* changing security on an
>>>>> object when it is replaced.
>>>>
>>>>> [CREATE OR REPLACE FUNCTION does not change proowner or proacl]
>>>>
>>>> Good point.  C-O-R VIEW also preserves column default values.  I believe we are
>>>> consistent to the extent that everything possible to specify in each C-O-R
>>>> statement gets replaced outright.  The preserved characteristics *require*
>>>> commands like GRANT, COMMENT and ALTER VIEW to set in the first place.
>>>>
>>>> The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to
>>>> SECURITY INVOKER if it's not specified each time.  That default is safe, though,
>>>> while the proposed default of security_barrier=false is unsafe.
>>>
>>> Even though I normally take the opposite position, I still like the
>>> idea of dedicated syntax for this feature.  Not knowing what view
>>> options we might end up with in the future, I hate having to decide on
>>> what the general behavior ought to be.  But it would be easy to decide
>>> that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves
>>> the security flag set; it would be consistent with what we're doing
>>> with owner and acl information and wouldn't back us into any
>>> unpleasant decisions down the road.
>>>
>> Does the CREATE SECURITY VIEW statement mean a synonym of
>> CREATE VIEW ... WITH (security_barrier=true) ?
>>
>> If so, it seems to me reasonable to keep the configuration when user
>> provides no explicit option.
>>
>> 1) an explicit WITH(security_barrier=true) / CREATE SECURITY VIEW
>>  -> It always turns on a security_barrier option.
>>
>> 2) an explicit WITH(security_barrier=false)
>>  -> It always turns off security_barrier option.
>>
>> 3) no explicit option / CREATE VIEW
>>  -> Keep existing configuration, although inconsist with SECURITY DEFINER
>
> No, you're missing my point completely.  If we use a flexible options
> syntax here, then we have to decide on what behavior CREATE OR REPLACE
> should have for all future options, without knowing what they are yet,
> or what behavior will be appropriate.
>
Hmm. Indeed, it seems to me fair enough reason.

In this syntax case, the only way to clear the security_barrier flag
is to drop view
once, then create a view, isn't it?
And, is the security_barrier flag still stored within reloptions field?

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

On Mon, Sep 26, 2011 at 9:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> No, you're missing my point completely.  If we use a flexible options
>> syntax here, then we have to decide on what behavior CREATE OR REPLACE
>> should have for all future options, without knowing what they are yet,
>> or what behavior will be appropriate.
>>
> Hmm. Indeed, it seems to me fair enough reason.
>
> In this syntax case, the only way to clear the security_barrier flag
> is to drop view
> once, then create a view, isn't it?

I was imagining we'd have ALTER VIEW .. [NO] SECURITY or something like that.

> And, is the security_barrier flag still stored within reloptions field?

No. That would be missing the point.

But keep in mind no one else has endorsed my reasoning on this one as yet...

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

On Sun, Sep 25, 2011 at 11:22:56PM -0400, Robert Haas wrote:
> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote:
> >> Robert Haas ?09/25/11 10:58 AM >>>
> >>
> >> > I'm not sure we've been 100% consistent about that, since we
> >> > previously made CREATE OR REPLACE LANGUAGE not replace the owner
> >> > with the current user.
> >>
> >> I think we've been consistent in *not* changing security on an
> >> object when it is replaced.
> >
> >> [CREATE OR REPLACE FUNCTION does not change proowner or proacl]
> >
> > Good point. ?C-O-R VIEW also preserves column default values. ?I believe we are
> > consistent to the extent that everything possible to specify in each C-O-R
> > statement gets replaced outright. ?The preserved characteristics *require*
> > commands like GRANT, COMMENT and ALTER VIEW to set in the first place.
> >
> > The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to
> > SECURITY INVOKER if it's not specified each time. ?That default is safe, though,
> > while the proposed default of security_barrier=false is unsafe.
>
> Even though I normally take the opposite position, I still like the
> idea of dedicated syntax for this feature. Not knowing what view
> options we might end up with in the future, I hate having to decide on
> what the general behavior ought to be. But it would be easy to decide
> that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves
> the security flag set; it would be consistent with what we're doing
> with owner and acl information and wouldn't back us into any
> unpleasant decisions down the road.

I prefer the previous UI (WITH (security_barrier=...)) to this proposal, albeit
for diffuse reasons. Both kinds of views can have the consequence of granting
new access to data. One kind leaks tuples to untrustworthy code whenever it's
convenient for performance, and the other does not. A "non-security view" would
not mimic either of these objects; it would be a mere subquery macro. Using
WITH (...) syntax attached to the CREATE VIEW command better evokes the
similarity between the alternatives we're actually offering. I also find it
mildly odd letting CREATE OR REPLACE VIEW update an object originating with
CREATE SECURITY VIEW.

Unqualified CREATE VIEW will retain no redeeming value apart from backward
compatibility; new applications with any concern for database-level security
should use only security_barrier=true and mark functions LEAKPROOF as needed.

nm

2011/9/30 Noah Misch <noah(at)leadboat(dot)com>:
> On Sun, Sep 25, 2011 at 11:22:56PM -0400, Robert Haas wrote:
>> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> > On Sun, Sep 25, 2011 at 11:22:03AM -0500, Kevin Grittner wrote:
>> >> Robert Haas ?09/25/11 10:58 AM >>>
>> >>
>> >> > I'm not sure we've been 100% consistent about that, since we
>> >> > previously made CREATE OR REPLACE LANGUAGE not replace the owner
>> >> > with the current user.
>> >>
>> >> I think we've been consistent in *not* changing security on an
>> >> object when it is replaced.
>> >
>> >> [CREATE OR REPLACE FUNCTION does not change proowner or proacl]
>> >
>> > Good point. ?C-O-R VIEW also preserves column default values. ?I believe we are
>> > consistent to the extent that everything possible to specify in each C-O-R
>> > statement gets replaced outright. ?The preserved characteristics *require*
>> > commands like GRANT, COMMENT and ALTER VIEW to set in the first place.
>> >
>> > The analogue I had in mind is SECURITY DEFINER, which C-O-R FUNCTION reverts to
>> > SECURITY INVOKER if it's not specified each time. ?That default is safe, though,
>> > while the proposed default of security_barrier=false is unsafe.
>>
>> Even though I normally take the opposite position, I still like the
>> idea of dedicated syntax for this feature.  Not knowing what view
>> options we might end up with in the future, I hate having to decide on
>> what the general behavior ought to be.  But it would be easy to decide
>> that CREATE SECURITY VIEW followed by CREATE OR REPLACE VIEW leaves
>> the security flag set; it would be consistent with what we're doing
>> with owner and acl information and wouldn't back us into any
>> unpleasant decisions down the road.
>
> I prefer the previous UI (WITH (security_barrier=...)) to this proposal, albeit
> for diffuse reasons.  Both kinds of views can have the consequence of granting
> new access to data.  One kind leaks tuples to untrustworthy code whenever it's
> convenient for performance, and the other does not.  A "non-security view" would
> not mimic either of these objects; it would be a mere subquery macro.  Using
> WITH (...) syntax attached to the CREATE VIEW command better evokes the
> similarity between the alternatives we're actually offering.  I also find it
> mildly odd letting CREATE OR REPLACE VIEW update an object originating with
> CREATE SECURITY VIEW.
>
My preference is still also WITH(security_barrier=...) syntax.

The arguable point was the behavior when a view is replaced without
explicit WITH clause;
whether we should consider it was specified a default value, or we
should consider it means
the option is preserved.
If we stand on the viewpoint that object's attribute related to
security (such as ownership,
acl, label, ...) should be preserved, the security barrier also shall
be preserved.
On the other hand, we can never know what options will be added in the
future, right now.
Thus, we may need to sort out options related to security and not at
DefineVirtualRelation().

However, do we need to limit type of the options to be preserved to
security related?
It is the first case that object with arbitrary options can be replaced.
It seems to me we have no matter, even if we determine object's
options are preserved
unless an explicit new value is provided.

Any other ideas?

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

On Sun, Oct 02, 2011 at 07:16:33PM +0200, Kohei KaiGai wrote:
> My preference is still also WITH(security_barrier=...) syntax.
>
> The arguable point was the behavior when a view is replaced without
> explicit WITH clause;
> whether we should consider it was specified a default value, or we
> should consider it means
> the option is preserved.
> If we stand on the viewpoint that object's attribute related to
> security (such as ownership,
> acl, label, ...) should be preserved, the security barrier also shall
> be preserved.
> On the other hand, we can never know what options will be added in the
> future, right now.
> Thus, we may need to sort out options related to security and not at
> DefineVirtualRelation().
>
> However, do we need to limit type of the options to be preserved to
> security related?
> It is the first case that object with arbitrary options can be replaced.
> It seems to me we have no matter, even if we determine object's
> options are preserved
> unless an explicit new value is provided.

Currently, you can predict how CREATE OR REPLACE affects a given object
characteristic with a simple rule: if the CREATE OR REPLACE statement can
specify a characteristic, we don't preserve its existing value. Otherwise, we
do preserve it. Let's not depart from that rule.

Applying that rule to the proposed syntax, it shall not preserve the existing
security_barrier value. I think that is acceptable. If it's not acceptable, we
need a different syntax -- perhaps CREATE SECURITY VIEW.

> Any other ideas?

Suppose we permitted pushdown of unsafe predicates when the user can read the
involved columns anyway, a generalization of the idea from the first paragraph
of [1]. Would that, along with LEAKPROOF, provide enough strategies for shoring
up performance to justify removing unsafe views entirely?

nm

[1] http://archives.postgresql.org/message-id/AANLkTil1n2qWDD7IZlgBVt2IFL29rWfVkSseL9b9r2Fi@mail.gmail.com

2011/10/8 Noah Misch <noah(at)leadboat(dot)com>:
> On Sun, Oct 02, 2011 at 07:16:33PM +0200, Kohei KaiGai wrote:
>> My preference is still also WITH(security_barrier=...) syntax.
>>
>> The arguable point was the behavior when a view is replaced without
>> explicit WITH clause;
>> whether we should consider it was specified a default value, or we
>> should consider it means
>> the option is preserved.
>> If we stand on the viewpoint that object's attribute related to
>> security (such as ownership,
>> acl, label, ...) should be preserved, the security barrier also shall
>> be preserved.
>> On the other hand, we can never know what options will be added in the
>> future, right now.
>> Thus, we may need to sort out options related to security and not at
>> DefineVirtualRelation().
>>
>> However, do we need to limit type of the options to be preserved to
>> security related?
>> It is the first case that object with arbitrary options can be replaced.
>> It seems to me we have no matter, even if we determine object's
>> options are preserved
>> unless an explicit new value is provided.
>
> Currently, you can predict how CREATE OR REPLACE affects a given object
> characteristic with a simple rule: if the CREATE OR REPLACE statement can
> specify a characteristic, we don't preserve its existing value.  Otherwise, we
> do preserve it.  Let's not depart from that rule.
>
> Applying that rule to the proposed syntax, it shall not preserve the existing
> security_barrier value.  I think that is acceptable.  If it's not acceptable, we
> need a different syntax -- perhaps CREATE SECURITY VIEW.
>
No. It also preserves the security-barrier flag, when we replace a view without
SECURITY option. The only difference is that we have no way to turn off
security-barrier flag explicitly, right now.

The major reason why I prefer reloptions rather than "SECURITY" option is
that allows to reuse the existing capability to store a property of relation.
It seems to me both of syntax enables to achieve the rule to preserve flags
when a view is replaced.

>> Any other ideas?
>
> Suppose we permitted pushdown of unsafe predicates when the user can read the
> involved columns anyway, a generalization of the idea from the first paragraph
> of [1].  Would that, along with LEAKPROOF, provide enough strategies for shoring
> up performance to justify removing unsafe views entirely?
>
The problem was that we do all the access control decision at the
executor stage,
but planner has to make a plan prior to execution. So, it was also reason why we
have tried to add LEAKPROOF flag to functions.

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

On Sat, Oct 08, 2011 at 09:11:08AM +0200, Kohei KaiGai wrote:
> 2011/10/8 Noah Misch <noah(at)leadboat(dot)com>:
> > On Sun, Oct 02, 2011 at 07:16:33PM +0200, Kohei KaiGai wrote:
> >> My preference is still also WITH(security_barrier=...) syntax.
> >>
> >> The arguable point was the behavior when a view is replaced without
> >> explicit WITH clause;
> >> whether we should consider it was specified a default value, or we
> >> should consider it means
> >> the option is preserved.
> >> If we stand on the viewpoint that object's attribute related to
> >> security (such as ownership,
> >> acl, label, ...) should be preserved, the security barrier also shall
> >> be preserved.
> >> On the other hand, we can never know what options will be added in the
> >> future, right now.
> >> Thus, we may need to sort out options related to security and not at
> >> DefineVirtualRelation().
> >>
> >> However, do we need to limit type of the options to be preserved to
> >> security related?
> >> It is the first case that object with arbitrary options can be replaced.
> >> It seems to me we have no matter, even if we determine object's
> >> options are preserved
> >> unless an explicit new value is provided.
> >
> > Currently, you can predict how CREATE OR REPLACE affects a given object
> > characteristic with a simple rule: if the CREATE OR REPLACE statement can
> > specify a characteristic, we don't preserve its existing value. ?Otherwise, we
> > do preserve it. ?Let's not depart from that rule.
> >
> > Applying that rule to the proposed syntax, it shall not preserve the existing
> > security_barrier value. ?I think that is acceptable. ?If it's not acceptable, we
> > need a different syntax -- perhaps CREATE SECURITY VIEW.
> >
> No. It also preserves the security-barrier flag, when we replace a view without
> SECURITY option. The only difference is that we have no way to turn off
> security-barrier flag explicitly, right now.
>
> The major reason why I prefer reloptions rather than "SECURITY" option is
> that allows to reuse the existing capability to store a property of relation.
> It seems to me both of syntax enables to achieve the rule to preserve flags
> when a view is replaced.

Yes, there are no technical barriers to implementing either behavior with either
syntax. However, CREATE OR REPLACE VIEW ... WITH (...) has a precedent guiding
its behavior: if a CREATE OR REPLACE statement can specify a characteristic, we
don't preserve its existing value.

> >> Any other ideas?
> >
> > Suppose we permitted pushdown of unsafe predicates when the user can read the
> > involved columns anyway, a generalization of the idea from the first paragraph
> > of [1]. ?Would that, along with LEAKPROOF, provide enough strategies for shoring
> > up performance to justify removing unsafe views entirely?
> >
> The problem was that we do all the access control decision at the
> executor stage,
> but planner has to make a plan prior to execution. So, it was also reason why we
> have tried to add LEAKPROOF flag to functions.

Yes; we'd need to invalidate relevant plans in response to anything that changes
access control decisions. GRANT and ALTER ... OWNER TO already do that, but
we'd need to cover pg_authid/pg_auth_members changes, SET ROLE, SET SESSION
AUTHORIZATION, and probably a few other things. That might be a substantial
project in its own right.

2011/10/8 Noah Misch <noah(at)leadboat(dot)com>:
> On Sat, Oct 08, 2011 at 09:11:08AM +0200, Kohei KaiGai wrote:
>> 2011/10/8 Noah Misch <noah(at)leadboat(dot)com>:
>> > On Sun, Oct 02, 2011 at 07:16:33PM +0200, Kohei KaiGai wrote:
>> >> My preference is still also WITH(security_barrier=...) syntax.
>> >>
>> >> The arguable point was the behavior when a view is replaced without
>> >> explicit WITH clause;
>> >> whether we should consider it was specified a default value, or we
>> >> should consider it means
>> >> the option is preserved.
>> >> If we stand on the viewpoint that object's attribute related to
>> >> security (such as ownership,
>> >> acl, label, ...) should be preserved, the security barrier also shall
>> >> be preserved.
>> >> On the other hand, we can never know what options will be added in the
>> >> future, right now.
>> >> Thus, we may need to sort out options related to security and not at
>> >> DefineVirtualRelation().
>> >>
>> >> However, do we need to limit type of the options to be preserved to
>> >> security related?
>> >> It is the first case that object with arbitrary options can be replaced.
>> >> It seems to me we have no matter, even if we determine object's
>> >> options are preserved
>> >> unless an explicit new value is provided.
>> >
>> > Currently, you can predict how CREATE OR REPLACE affects a given object
>> > characteristic with a simple rule: if the CREATE OR REPLACE statement can
>> > specify a characteristic, we don't preserve its existing value. ?Otherwise, we
>> > do preserve it. ?Let's not depart from that rule.
>> >
>> > Applying that rule to the proposed syntax, it shall not preserve the existing
>> > security_barrier value. ?I think that is acceptable. ?If it's not acceptable, we
>> > need a different syntax -- perhaps CREATE SECURITY VIEW.
>> >
>> No. It also preserves the security-barrier flag, when we replace a view without
>> SECURITY option. The only difference is that we have no way to turn off
>> security-barrier flag explicitly, right now.
>>
>> The major reason why I prefer reloptions rather than "SECURITY" option is
>> that allows to reuse the existing capability to store a property of relation.
>> It seems to me both of syntax enables to achieve the rule to preserve flags
>> when a view is replaced.
>
> Yes, there are no technical barriers to implementing either behavior with either
> syntax.  However, CREATE OR REPLACE VIEW ... WITH (...) has a precedent guiding
> its behavior: if a CREATE OR REPLACE statement can specify a characteristic, we
> don't preserve its existing value.
>
I tried to refactor the patches based on the interface of WITH (...)
and usage of
pg_class.reloptions, although here is no functionality changes; including the
behavior when a view is replaced.

My preference is WITH (...) interface, however, it is not a strong one.
So, I hope either of versions being reviewed.

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

On Sun, Oct 9, 2011 at 11:50 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> I tried to refactor the patches based on the interface of WITH (...)
> and usage of
> pg_class.reloptions, although here is no functionality changes; including the
> behavior when a view is replaced.
>
> My preference is WITH (...) interface, however, it is not a strong one.
> So, I hope either of versions being reviewed.

I spent some more time looking at this, and I guess I'm pretty unsold
on the whole approach. In the part 2 patch, for example, we're doing
this:

+static bool
+mark_qualifiers_depth_walker(Node *node, void *context)
+{
+ int depth = *((int *)(context));
+
+ if (node == NULL)
+ return false;
+
+ if (IsA(node, FuncExpr))
+ {
+ ((FuncExpr *)node)->depth = depth;
+ }
+ else if (IsA(node, OpExpr))
+ {
+ ((OpExpr *)node)->depth = depth;
+ }
+ else if (IsA(node, DistinctExpr))
+ {
+ ((DistinctExpr *)node)->depth = depth;
+ }
+ else if (IsA(node, ScalarArrayOpExpr))
+ {
+ ((ScalarArrayOpExpr *)node)->depth = depth;
+ }
+ else if (IsA(node, CoerceViaIO))
+ {
+ ((CoerceViaIO *)node)->depth = depth;
+ }
+ else if (IsA(node, ArrayCoerceExpr))
+ {
+ ((ArrayCoerceExpr *)node)->depth = depth;
+ }
+ else if (IsA(node, NullIfExpr))
+ {
+ ((NullIfExpr *)node)->depth = depth;
+ }
+ else if (IsA(node, RowCompareExpr))
+ {
+ ((RowCompareExpr *)node)->depth = depth;
+ }
+ return expression_tree_walker(node,
mark_qualifiers_depth_walker, context);
+}

It seems really ugly to me to suppose that we need to add a depth
field to every single one of these node types. If you've missed one,
then we have a security hole. If someone else adds another node type
later that requires this field and doesn't add it, we have a security
hole. And since all of these depth fields are going to make their way
into stored rules, those security holes will require an initdb to fix.
Ouch! And what happens if the security view becomes a non-security
view or visca versa? Now all of those stored depth fields are out of
date. Maybe you can argue that we can just patch that up when we
reload them, but that seems to me to miss the point. If the data in a
stored rule can get out of date, then it shouldn't be stored there in
the first place.

Tom may have a better feeling on this than I do, but my gut feeling
here is that this whole approach is letting the cat out of the bag and
then trying to stuff it back in. I don't think that's going to be
very reliable, and more than that, I don't like our chances of having
confidence in its reliability. I feel like the heart of what we're
doing here ought to be preventing the subquery from getting flattened.
For example:

rhaas=# create table secret (a int, b text);
CREATE TABLE
rhaas=# insert into secret select g, random()::text||random()::text
from generate_series(1,10000) g;
INSERT 0 10000
rhaas=# create view window_on_secret as select * from secret where a = 1;
CREATE VIEW
rhaas=# create table leak (a int, b text);
CREATE TABLE
rhaas=# create or replace function snarf(a int, b text) returns
boolean as $$begin insert into leak values ($1, $2); return true;
end$$ language plpgsql cost
0.00000000000000000000000000000000000000001;
CREATE FUNCTION
rhaas=# explain analyze select * from window_on_secret;
QUERY PLAN
---------------------------------------------------------------------------------------------------
Seq Scan on secret (cost=0.00..209.00 rows=1 width=39) (actual
time=0.022..2.758 rows=1 loops=1)
Filter: (a = 1)
Rows Removed by Filter: 9999
Total runtime: 2.847 ms
(4 rows)

rhaas=# select * from leak;
a | b
---+---
(0 rows)

rhaas=# explain analyze select * from window_on_secret where snarf(a, b);
QUERY PLAN
-----------------------------------------------------------------------------------------------------
Seq Scan on secret (cost=0.00..209.00 rows=1 width=39) (actual
time=0.671..126.521 rows=1 loops=1)
Filter: (snarf(a, b) AND (a = 1))
Rows Removed by Filter: 9999
Total runtime: 126.565 ms
(4 rows)

Woops! I've stolen the whole table. But look what happens when I
change the definition of window_on_secret so that it can't be
flattened:

rhaas=# truncate leak;
TRUNCATE TABLE
rhaas=# create or replace view window_on_secret as select * from
secret where a = 1 limit 1000000000;
CREATE VIEW
rhaas=# explain analyze select * from window_on_secret where snarf(a, b);
QUERY PLAN
------------------------------------------------------------------------------------------------------------------
Subquery Scan on window_on_secret (cost=0.00..209.01 rows=1
width=39) (actual time=0.320..2.348 rows=1 loops=1)
Filter: snarf(window_on_secret.a, window_on_secret.b)
-> Limit (cost=0.00..209.00 rows=1 width=39) (actual
time=0.016..2.043 rows=1 loops=1)
-> Seq Scan on secret (cost=0.00..209.00 rows=1 width=39)
(actual time=0.014..2.034 rows=1 loops=1)
Filter: (a = 1)
Rows Removed by Filter: 9999
Total runtime: 2.434 ms
(7 rows)

rhaas=# select * from leak;
a | b
---+----------------------------------
1 | 0.60352857504040.928101760800928
(1 row)

If we make security views work like this, then we don't need to have
one mechanism to sort quals by depth and another to prevent them from
being pushed down through joins. It all just works. Now, there is
one problem: if snarf() were a non-leaky function rather than a
maliciously crafted one, it still wouldn't get pushed down:

rhaas=# explain analyze select * from window_on_secret where b = 'not so hot';
QUERY PLAN
------------------------------------------------------------------------------------------------------------------
Subquery Scan on window_on_secret (cost=0.00..209.01 rows=1
width=39) (actual time=2.080..2.080 rows=0 loops=1)
Filter: (window_on_secret.b = 'not so hot'::text)
Rows Removed by Filter: 1
-> Limit (cost=0.00..209.00 rows=1 width=39) (actual
time=0.014..2.077 rows=1 loops=1)
-> Seq Scan on secret (cost=0.00..209.00 rows=1 width=39)
(actual time=0.013..2.075 rows=1 loops=1)
Filter: (a = 1)
Rows Removed by Filter: 9999
Total runtime: 2.131 ms
(8 rows)

I don't have a clear idea what to do about that, and maybe it's an
intractable problem, but I feel like once we've flattened the
subquery, it's a whole lot harder to prevent bad stuff from happening,
because now the bits that started inside the security view are all
over the place. Somebody previously raised the issue of what happen
when there are multiple security views involved in the same query. I
don't see how the depth-based approach can possibly deal with that
case correctly. Let's suppose that in the test case above,
window_on_secret was created as a security view. Now, the bad guy
comes along and creates a security view that uses with some
maliciously crafted qual, and then joins that view against
window_on_secret. IIUC, the quals from both views are going to have
depth = 1, so from the planner's point of view it ought to be OK to
interchange them - which it's not. Now, in the normal course of
events it won't matter, because the quals in window_on_secret are
going to apply to the "secret" table, and the quals in the other view
are going to apply only to whatever view that table ranges over. But
there's already at least one case in which that might not be true: if
the equivalence class machinery throws a constant into the same bucket
as a column in window_on_secret, it will feel free to add a qual
comparing that value to the associated constant using the appropriate
operator, and that qual could then (presumably) get reordered with the
qual from the security view. I'm not 100% sure that it's possible to
construct a security breach this way, but I'm definitely not 100% sure
that it isn't. And future changes to the way we make deductions based
on equivalence classes (like deducing implied equalities, something
that's been requested more than once) could open up further
possibilities for mischief. We could maybe hunt all of those down but
it seems rather error-prone to me, and not very future-proof.

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

2011/10/10 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Oct 9, 2011 at 11:50 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I tried to refactor the patches based on the interface of WITH (...)
>> and usage of
>> pg_class.reloptions, although here is no functionality changes; including the
>> behavior when a view is replaced.
>>
>> My preference is WITH (...) interface, however, it is not a strong one.
>> So, I hope either of versions being reviewed.
>
> I spent some more time looking at this, and I guess I'm pretty unsold
> on the whole approach.  In the part 2 patch, for example, we're doing
> this:
>
> +static bool
> +mark_qualifiers_depth_walker(Node *node, void *context)
> +{
> +       int             depth = *((int *)(context));
> +
... <snip> ...
> +       else if (IsA(node, RowCompareExpr))
> +       {
> +               ((RowCompareExpr *)node)->depth = depth;
> +       }
> +       return expression_tree_walker(node,
> mark_qualifiers_depth_walker, context);
> +}
>
> It seems really ugly to me to suppose that we need to add a depth
> field to every single one of these node types.  If you've missed one,
> then we have a security hole.  If someone else adds another node type
> later that requires this field and doesn't add it, we have a security
> hole.  And since all of these depth fields are going to make their way
> into stored rules, those security holes will require an initdb to fix.
>
Indeed, I have to admit this disadvantage from the perspective of code
maintenance, because it had also been a tough work for me to track
the depth field in this patch.

> If we make security views work like this, then we don't need to have
> one mechanism to sort quals by depth and another to prevent them from
> being pushed down through joins.  It all just works.  Now, there is
> one problem: if snarf() were a non-leaky function rather than a
> maliciously crafted one, it still wouldn't get pushed down:
>
Rather than my original design, I'm learning to the idea to keep
sub-queries come from security views; without flatten, because
of its straightforwardness.

> If we make security views work like this, then we don't need to have
> one mechanism to sort quals by depth and another to prevent them from
> being pushed down through joins.  It all just works.  Now, there is
> one problem: if snarf() were a non-leaky function rather than a
> maliciously crafted one, it still wouldn't get pushed down:
>
I agreed. We have been on the standpoint that tries to prevent
leakable functions to reference a portion of join-tree being already
flatten, however, it has been a tough work.
It seems to me it is much simple approach that enables to push
down only non-leaky functions into inside of sub-queries.

An idea is to add a hack on distribute_qual_to_rels() to relocate
a qualifier into inside of the sub-query, when it references only
a particular sub-query being come from a security view, and
when the sub-query satisfies is_simple_subquery(), for example.

Anyway, I'll try to tackle this long standing problem with this
approach in the next commit-fest.

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

On Sun, Oct 09, 2011 at 05:50:52PM +0200, Kohei KaiGai wrote:
> [patch v4]

Each revision of this patch yielded a 1.2 MiB email. Please gzip attachments
this large. The two revisions you sent in September constituted 18% of the
pgsql-hackers bits for the month, and the next-largest message was only 315
KiB. Your mailer also picks base64 for textual attachments, needlessly
inflating them by 37%.

At the same time, the patch is large because it rewrites every line in
pg_proc.h. Especially since it leaves proleakproof = 'f' for _all_ rows,
consider instead using an approach like this:
http://archives.postgresql.org/message-id/20110611211304.GB21098@tornado.leadboat.com

These patches were not context diffs.

Thanks,
nm

On 10 October 2011 21:28, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2011/10/10 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> It seems really ugly to me to suppose that we need to add a depth
>> field to every single one of these node types.  If you've missed one,
>> then we have a security hole.  If someone else adds another node type
>> later that requires this field and doesn't add it, we have a security
>> hole.  And since all of these depth fields are going to make their way
>> into stored rules, those security holes will require an initdb to fix.
>>
> Indeed, I have to admit this disadvantage from the perspective of code
> maintenance, because it had also been a tough work for me to track
> the depth field in this patch.

Would you consider putting the depth field directly into a generic
superclass node, such as the "Expr" node? Perhaps that approach would
be neater.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Oct 10, 2011 at 4:28 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> I agreed. We have been on the standpoint that tries to prevent
> leakable functions to reference a portion of join-tree being already
> flatten, however, it has been a tough work.
> It seems to me it is much simple approach that enables to push
> down only non-leaky functions into inside of sub-queries.
>
> An idea is to add a hack on distribute_qual_to_rels() to relocate
> a qualifier into inside of the sub-query, when it references only
> a particular sub-query being come from a security view, and
> when the sub-query satisfies is_simple_subquery(), for example.

If you can make this work, I think it could be a pretty sweet plannner
optimization even apart from the implications for security views.
Consider a query of this form:

A LEFT JOIN B LEFT JOIN C

where B is a view defined as:

B1 JOIN B2 JOIN B3 LEFT JOIN B4 LEFT JOIN B5

Now let's suppose that from_collapse_limit/join_collapse_limit are set
low enough that we decline to fold these subproblems together. If
there happens to be a qual B.x = 1, where B.x is really B1.x, then the
generated plan sucks, because it will basically lose the ability to
filter B1 early, very possibly on, say, a unique index. Or at least a
highly selective index. If we could allow the B.x qual to trickle
down inside of the subquery, we'd get a much better plan. Of course,
it's still not as good as flattening, because it won't allow us to
consider as many possible join orders - but the whole point of having
from_collapse_limit/join_collapse_limit in the first place is that we
can't consider all the join orders without having planning time and
memory usage balloon wildly out of control. And in many real-world
cases, I think that this would probably mitigate the effects of
exceeding from_collapse_limit/join_collapse_limit quite a bit.

In order to make it work, though, you'd need to arrange things so that
we distribute quals to rels in the parent query, then let some of them
filter down into the subquery, then distribute quals to rels in the
subquery (possibly adjusting RTE indexes?), then finish planning the
subquery, then finish planning the parent query. Not sure how
possible/straightforward that is.

It's probably a good idea to deal with this part first, because if you
can't make it work then the whole approach is in trouble. I'm almost
imagining that we could break this into three independent patches,
like this:

1. Let quals percolate down into subqueries.
2. Add the notion of a security view, which prevents flattening and
disables the optimization of patch #1
3. Add the notion of a leakproof function, which can benefit from the
optimization of #1 even when the view involved is a security view as
introduced in #2

Unlike the way you have it now, I think those patches could be
independently committable.

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

Hi Robert,

I'm a bit confusing about this sentence.

> If you can make this work, I think it could be a pretty sweet plannner
> optimization even apart from the implications for security views.
> Consider a query of this form:
>
> A LEFT JOIN B LEFT JOIN C
>
> where B is a view defined as:
>
> B1 JOIN B2 JOIN B3 LEFT JOIN B4 LEFT JOIN B5
>
> Now let's suppose that from_collapse_limit/join_collapse_limit are set
> low enough that we decline to fold these subproblems together.  If
> there happens to be a qual B.x = 1, where B.x is really B1.x, then the
> generated plan sucks, because it will basically lose the ability to
> filter B1 early, very possibly on, say, a unique index.  Or at least a
> highly selective index.
>

I tried to reproduce the scenario with enough small from/join_collapse_limit
(typically 1), but it allows to push down qualifiers into the least scan plan.

E.g)
mytest=# SET from_collapse_limit = 1;
mytest=# SET join_collapse_limit = 1;
mytest=# CREATE VIEW B AS SELECT B1.* FROM B1,B2,B3 WHERE B1.x = B2.x
AND B2.x = B3.x;
mytest=# EXPLAIN SELECT * FROM A,B,C WHERE A.x=B.x AND B.x=C.x AND f_leak(B.y);
QUERY PLAN
------------------------------------------------------------------------------------
Merge Join (cost=381.80..9597.97 rows=586624 width=108)
Merge Cond: (a.x = b1.x)
-> Merge Join (cost=170.85..290.46 rows=7564 width=72)
Merge Cond: (a.x = c.x)
-> Sort (cost=85.43..88.50 rows=1230 width=36)
Sort Key: a.x
-> Seq Scan on a (cost=0.00..22.30 rows=1230 width=36)
-> Sort (cost=85.43..88.50 rows=1230 width=36)
Sort Key: c.x
-> Seq Scan on c (cost=0.00..22.30 rows=1230 width=36)
-> Materialize (cost=210.95..528.56 rows=15510 width=44)
-> Merge Join (cost=210.95..489.78 rows=15510 width=44)
Merge Cond: (b1.x = b3.x)
-> Merge Join (cost=125.52..165.40 rows=2522 width=40)
Merge Cond: (b1.x = b2.x)
-> Sort (cost=40.09..41.12 rows=410 width=36)
Sort Key: b1.x
-> Seq Scan on b1 (cost=0.00..22.30
rows=410 width=36)
Filter: f_leak(y)
-> Sort (cost=85.43..88.50 rows=1230 width=4)
Sort Key: b2.x
-> Seq Scan on b2 (cost=0.00..22.30
rows=1230 width=4)
-> Sort (cost=85.43..88.50 rows=1230 width=4)
Sort Key: b3.x
-> Seq Scan on b3 (cost=0.00..22.30 rows=1230 width=4)
(25 rows)

In this example, f_leak() takes an argument come from B1 table within B view,
and it was correctly distributed to SeqScan on B1.

From perspective of the code, the *_collapse_limit affects the contents of
joinlist being returned from deconstruct_jointree() whether its sub-portion is
flatten, or not.
However, the qualifiers are distributed on distribute_restrictinfo_to_rels() to
RelOptInfo based on its dependency of relations being referenced by
arguments. Thus, the above f_leak() was distributed to B1, not B, because
its arguments come from only B1.

I agree with the following approach to tackle this problem in 100%.
However, I'm unclear how from/join_collapse_limit affects to keep
sub-queries unflatten. It seems to me it is determined based on
the result of is_simple_subquery().

> 1. Let quals percolate down into subqueries.
> 2. Add the notion of a security view, which prevents flattening and
> disables the optimization of patch #1
> 3. Add the notion of a leakproof function, which can benefit from the
> optimization of #1 even when the view involved is a security view as
> introduced in #2
>

Thanks,

2011/10/11 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Oct 10, 2011 at 4:28 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I agreed. We have been on the standpoint that tries to prevent
>> leakable functions to reference a portion of join-tree being already
>> flatten, however, it has been a tough work.
>> It seems to me it is much simple approach that enables to push
>> down only non-leaky functions into inside of sub-queries.
>>
>> An idea is to add a hack on distribute_qual_to_rels() to relocate
>> a qualifier into inside of the sub-query, when it references only
>> a particular sub-query being come from a security view, and
>> when the sub-query satisfies is_simple_subquery(), for example.
>
> If you can make this work, I think it could be a pretty sweet plannner
> optimization even apart from the implications for security views.
> Consider a query of this form:
>
> A LEFT JOIN B LEFT JOIN C
>
> where B is a view defined as:
>
> B1 JOIN B2 JOIN B3 LEFT JOIN B4 LEFT JOIN B5
>
> Now let's suppose that from_collapse_limit/join_collapse_limit are set
> low enough that we decline to fold these subproblems together.  If
> there happens to be a qual B.x = 1, where B.x is really B1.x, then the
> generated plan sucks, because it will basically lose the ability to
> filter B1 early, very possibly on, say, a unique index.  Or at least a
> highly selective index.  If we could allow the B.x qual to trickle
> down inside of the subquery, we'd get a much better plan.  Of course,
> it's still not as good as flattening, because it won't allow us to
> consider as many possible join orders - but the whole point of having
> from_collapse_limit/join_collapse_limit in the first place is that we
> can't consider all the join orders without having planning time and
> memory usage balloon wildly out of control.  And in many real-world
> cases, I think that this would probably mitigate the effects of
> exceeding from_collapse_limit/join_collapse_limit quite a bit.
>
> In order to make it work, though, you'd need to arrange things so that
> we distribute quals to rels in the parent query, then let some of them
> filter down into the subquery, then distribute quals to rels in the
> subquery (possibly adjusting RTE indexes?), then finish planning the
> subquery, then finish planning the parent query.  Not sure how
> possible/straightforward that is.
>
> It's probably a good idea to deal with this part first, because if you
> can't make it work then the whole approach is in trouble.  I'm almost
> imagining that we could break this into three independent patches,
> like this:
>
> 1. Let quals percolate down into subqueries.
> 2. Add the notion of a security view, which prevents flattening and
> disables the optimization of patch #1
> 3. Add the notion of a leakproof function, which can benefit from the
> optimization of #1 even when the view involved is a security view as
> introduced in #2
>
> Unlike the way you have it now, I think those patches could be
> independently committable.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

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

On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> Hi Robert,
>
> I'm a bit confusing about this sentence.
>
>> If you can make this work, I think it could be a pretty sweet plannner
>> optimization even apart from the implications for security views.
>> Consider a query of this form:
>>
>> A LEFT JOIN B LEFT JOIN C
>>
>> where B is a view defined as:
>>
>> B1 JOIN B2 JOIN B3 LEFT JOIN B4 LEFT JOIN B5
>>
>> Now let's suppose that from_collapse_limit/join_collapse_limit are set
>> low enough that we decline to fold these subproblems together.  If
>> there happens to be a qual B.x = 1, where B.x is really B1.x, then the
>> generated plan sucks, because it will basically lose the ability to
>> filter B1 early, very possibly on, say, a unique index.  Or at least a
>> highly selective index.
>>
>
> I tried to reproduce the scenario with enough small from/join_collapse_limit
> (typically 1), but it allows to push down qualifiers into the least scan plan.

Hmm, you're right. LIMIT 1000000000 prevents qual pushdown, but
hitting from_collapse_limit/join_collapse_limit apparently doesn't. I
could have sworn I've seen this work the other way, but I guess not.

> E.g)
> mytest=# SET from_collapse_limit = 1;
> mytest=# SET join_collapse_limit = 1;
> mytest=# CREATE VIEW B AS SELECT B1.* FROM B1,B2,B3 WHERE B1.x = B2.x
> AND B2.x = B3.x;
> mytest=# EXPLAIN SELECT * FROM A,B,C WHERE A.x=B.x AND B.x=C.x AND f_leak(B.y);

This I wouldn't expect to have any effect anyway, because you're using
the ad-hoc join syntax rather than explicit join syntax. But I tried
it with explicit join syntax and it seems to only constrain the join
order, not prevent qual pushdown.

> I agree with the following approach to tackle this problem in 100%.
> However, I'm unclear how from/join_collapse_limit affects to keep
> sub-queries unflatten. It seems to me it is determined based on
> the result of is_simple_subquery().

I think you are right, but I'm not sure it's right to hack
is_simple_subquery() directly. Perhaps what we want to do is modify
pull_up_subquery()?

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I tried to reproduce the scenario with enough small from/join_collapse_limit
>> (typically 1), but it allows to push down qualifiers into the least scan plan.

> Hmm, you're right. LIMIT 1000000000 prevents qual pushdown, but
> hitting from_collapse_limit/join_collapse_limit apparently doesn't. I
> could have sworn I've seen this work the other way, but I guess not.

No, the collapse_limit variables are entirely unrelated to subquery
flattening, or to qual pushdown for that matter. They only restrict the
number of join paths we consider. And we will attempt to push down
quals into an unflattened subquery, too, if it looks safe. See
subquery_is_pushdown_safe, qual_is_pushdown_safe, etc in allpaths.c.

regards, tom lane

2011/10/19 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> I tried to reproduce the scenario with enough small from/join_collapse_limit
>>> (typically 1), but it allows to push down qualifiers into the least scan plan.
>
>> Hmm, you're right.  LIMIT 1000000000 prevents qual pushdown, but
>> hitting from_collapse_limit/join_collapse_limit apparently doesn't.  I
>> could have sworn I've seen this work the other way, but I guess not.
>
> No, the collapse_limit variables are entirely unrelated to subquery
> flattening, or to qual pushdown for that matter.  They only restrict the
> number of join paths we consider.  And we will attempt to push down
> quals into an unflattened subquery, too, if it looks safe.  See
> subquery_is_pushdown_safe, qual_is_pushdown_safe, etc in allpaths.c.
>
I tried to observe the behavior with a bit modification of is_simple_subquery
that become to return 'false' always.
(It is a simulation if and when a view with security_barrier would be given.)

The expected behavior is to keep sub-query without flatten.
However, the externally provided qualifiers are correctly pushed down.

Do we need to focus on the code around above functions rather than
distribute_qual_to_rels, to prevent undesirable pushing-down across
security barrier?

postgres=# CREATE VIEW v1 AS SELECT * FROM t1 WHERE a > 100;
CREATE VIEW
postgres=# CREATE VIEW v2 AS SELECT * FROM t2 JOIN t3 ON x = s;
CREATE VIEW
postgres=# EXPLAIN SELECT * FROM v1 WHERE b = 'bbb';
QUERY PLAN
----------------------------------------------------
Seq Scan on t1 (cost=0.00..28.45 rows=2 width=36)
Filter: ((a > 100) AND (b = 'bbb'::text))
(2 rows)
postgres=# EXPLAIN SELECT * FROM v2 WHERE t = 'ttt';
QUERY PLAN
----------------------------------------------------------------
Hash Join (cost=25.45..52.73 rows=37 width=72)
Hash Cond: (t2.x = t3.s)
-> Seq Scan on t2 (cost=0.00..22.30 rows=1230 width=36)
-> Hash (cost=25.38..25.38 rows=6 width=36)
-> Seq Scan on t3 (cost=0.00..25.38 rows=6 width=36)
Filter: (t = 'ttt'::text)
(6 rows)

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

On Wed, Oct 19, 2011 at 6:35 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2011/10/19 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> I tried to reproduce the scenario with enough small from/join_collapse_limit
>>>> (typically 1), but it allows to push down qualifiers into the least scan plan.
>>
>>> Hmm, you're right.  LIMIT 1000000000 prevents qual pushdown, but
>>> hitting from_collapse_limit/join_collapse_limit apparently doesn't.  I
>>> could have sworn I've seen this work the other way, but I guess not.
>>
>> No, the collapse_limit variables are entirely unrelated to subquery
>> flattening, or to qual pushdown for that matter.  They only restrict the
>> number of join paths we consider.  And we will attempt to push down
>> quals into an unflattened subquery, too, if it looks safe.  See
>> subquery_is_pushdown_safe, qual_is_pushdown_safe, etc in allpaths.c.
>>
> I tried to observe the behavior with a bit modification of is_simple_subquery
> that become to return 'false' always.
> (It is a simulation if and when a view with security_barrier would be given.)
>
> The expected behavior is to keep sub-query without flatten.
> However, the externally provided qualifiers are correctly pushed down.
>
> Do we need to focus on the code around above functions rather than
> distribute_qual_to_rels, to prevent undesirable pushing-down across
> security barrier?
>
> postgres=# CREATE VIEW v1 AS SELECT * FROM t1 WHERE a > 100;
> CREATE VIEW
> postgres=# CREATE VIEW v2 AS SELECT * FROM t2 JOIN t3 ON x = s;
> CREATE VIEW
> postgres=# EXPLAIN SELECT * FROM v1 WHERE b = 'bbb';
>                     QUERY PLAN
> ----------------------------------------------------
>  Seq Scan on t1  (cost=0.00..28.45 rows=2 width=36)
>   Filter: ((a > 100) AND (b = 'bbb'::text))
> (2 rows)
> postgres=# EXPLAIN SELECT * FROM v2 WHERE t = 'ttt';
>                           QUERY PLAN
> ----------------------------------------------------------------
>  Hash Join  (cost=25.45..52.73 rows=37 width=72)
>   Hash Cond: (t2.x = t3.s)
>   ->  Seq Scan on t2  (cost=0.00..22.30 rows=1230 width=36)
>   ->  Hash  (cost=25.38..25.38 rows=6 width=36)
>         ->  Seq Scan on t3  (cost=0.00..25.38 rows=6 width=36)
>               Filter: (t = 'ttt'::text)
> (6 rows)

Well, there's clearly some way to prevent pushdown from happening,
because sticking a LIMIT in there does the trick...

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Well, there's clearly some way to prevent pushdown from happening,
> because sticking a LIMIT in there does the trick...

I already pointed you at subquery_is_pushdown_safe ...

regards, tom lane

So, I will split the patch into two parts as follows, in the next commit fest.

Part-1) Views with security_barrier reloption

The part-1 portion provides views "security_barrier" reloption; that enables
to keep sub-queries unflatten in the prepjoin.c stage.
In addition, these sub-queries (that originally come from views with
"security_barrier" option) don't allow to push down qualifiers from upper
level. It shall prevent both of the problematic scenarios.

Part-2) Functions with leakproof attribute

The part-2 portion provides functions "leakproof" attribute; that enables
to push down leakproof functions into sub-queries, even if it originally
come from security views.
It shall minimize performance damages when we use view for row-level
security purpose.

2011/10/19 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Well, there's clearly some way to prevent pushdown from happening,
>> because sticking a LIMIT in there does the trick...
>
> I already pointed you at subquery_is_pushdown_safe ...
>
>                        regards, tom lane
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Fri, Oct 21, 2011 at 10:36 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> So, I will split the patch into two parts as follows, in the next commit fest.
>
> Part-1) Views with security_barrier reloption
>
> The part-1 portion provides views "security_barrier" reloption; that enables
> to keep sub-queries unflatten in the prepjoin.c stage.
> In addition, these sub-queries (that originally come from views with
> "security_barrier" option) don't allow to push down qualifiers from upper
> level. It shall prevent both of the problematic scenarios.
>
> Part-2) Functions with leakproof attribute
>
> The part-2 portion provides functions "leakproof" attribute; that enables
> to push down leakproof functions into sub-queries, even if it originally
> come from security views.
> It shall minimize performance damages when we use view for row-level
> security purpose.

Sounds reasonable.

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

The attached patches are fixes to the leaky-view problem; a
prerequisite to implement
row-level security; that consists of two portion.

Part-1)
It adds WITH(options...) clause on view definition, and disallow to
make sub-queries
flatten, if this sub-query is originated a particular view with
"security_barrier" reloption.
In addition, it also disallow to push-down qualifiers across
security-barrier, thus, we
will have a way to guarantee order to launch qualifiers; that has been
headache for us
to achieve row-level security using view (or possibly similar feature).

Part-2)
It adds "leakproof" attribute to functions; that means functions are
obviously leakproof
to the supplied arguments, and only superuser can set.
If a qualifier is consists of functions with "leakproof" only, the
query planner handles it
as an exception of the security-barrier. A typical case is WHERE x =
100; that shall
promote the given scan plan from sequential to index in many cases.
It requires the part-1 being applied prior to this patch, and
compressed by gzip due to
the size of patch (mostly pg_proc.h).

The following examples shows how these features works:

postgres=# CREATE VIEW v1 AS SELECT * FROM t1 WHERE a % 2 =0;
CREATE VIEW
postgres=# CREATE VIEW v1s WITH (security_barrier) AS SELECT * FROM t1
WHERE a % 2 =0;
CREATE VIEW
postgres=# CREATE VIEW v2 AS SELECT * FROM t1 JOIN t2 ON a = x WHERE a % 2 = 0;
CREATE VIEW
postgres=# CREATE VIEW v2s WITH (security_barrier) AS SELECT * FROM t1
JOIN t2 ON a = x W
HERE a % 2 = 0;
CREATE VIEW
postgres=# CREATE OR REPLACE FUNCTION f_leak(text) RETURNS bool
LANGUAGE plpgsql COST 0.0001
AS 'BEGIN RAISE notice ''f_leak => %'', $1; RETURN true;
END';CREATE FUNCTION
Without security_barrier
------------------------------------
postgres=# SELECT * FROM v1 WHERE f_leak(b);NOTICE:  f_leak =>
aaaNOTICE:  f_leak => bbbNOTICE:  f_leak => cccNOTICE:  f_leak => ddd
a |  b---+----- 2 | bbb 4 | ddd(2 rows)
postgres=# EXPLAIN SELECT * FROM v1 WHERE f_leak(b);
QUERY PLAN
----------------------------------------------------
Seq Scan on t1 (cost=0.00..28.45 rows=2 width=36)
Filter: (f_leak(b) AND ((a % 2) = 0))
(2 rows)

postgres=# SELECT * FROM v2 WHERE f_leak(y);
NOTICE: f_leak => xxx
NOTICE: f_leak => yyy
NOTICE: f_leak => zzz
NOTICE: f_leak => xyz
a | b | x | y
---+-----+---+-----
2 | bbb | 2 | xxx
4 | ddd | 4 | zzz
(2 rows)

postgres=# EXPLAIN SELECT * FROM v2 WHERE f_leak(y);
QUERY PLAN
----------------------------------------------------------------
Hash Join (cost=28.52..52.38 rows=2 width=72)
Hash Cond: (t2.x = t1.a)
-> Seq Scan on t2 (cost=0.00..22.30 rows=410 width=36)
Filter: f_leak(y)
-> Hash (cost=28.45..28.45 rows=6 width=36)
-> Seq Scan on t1 (cost=0.00..28.45 rows=6 width=36)
Filter: ((a % 2) = 0)
(7 rows)

With security_barrier
-------------------------------
postgres=# SELECT * FROM v1s WHERE f_leak(b);NOTICE:  f_leak =>
bbbNOTICE:  f_leak => ddd a |  b---+----- 2 | bbb 4 | ddd(2 rows)
postgres=# EXPLAIN SELECT * FROM v1s WHERE f_leak(b);
QUERY PLAN
----------------------------------------------------------
Subquery Scan on v1s (cost=0.00..28.51 rows=2 width=36)
Filter: f_leak(v1s.b)
-> Seq Scan on t1 (cost=0.00..28.45 rows=6 width=36)
Filter: ((a % 2) = 0)
(4 rows)

postgres=# SELECT * FROM v2s WHERE f_leak(y);
NOTICE: f_leak => xxx
NOTICE: f_leak => zzz
a | b | x | y
---+-----+---+-----
2 | bbb | 2 | xxx
4 | ddd | 4 | zzz
(2 rows)

postgres=# EXPLAIN SELECT * FROM v2s WHERE f_leak(y);
QUERY PLAN
----------------------------------------------------------------------
Subquery Scan on v2s (cost=28.52..55.56 rows=2 width=72)
Filter: f_leak(v2s.y)
-> Hash Join (cost=28.52..55.50 rows=6 width=72)
Hash Cond: (t2.x = t1.a)
-> Seq Scan on t2 (cost=0.00..22.30 rows=1230 width=36)
-> Hash (cost=28.45..28.45 rows=6 width=36)
-> Seq Scan on t1 (cost=0.00..28.45 rows=6 width=36)
Filter: ((a % 2) = 0)
(8 rows)

Leakproof function is exceptionally allowed to be pushed down
------------------------------------------------------------------------------------------

postgres=# SELECT * FROM v2s WHERE f_leak(y) AND a = 2;
NOTICE: f_leak => xxx
a | b | x | y
---+-----+---+-----
2 | bbb | 2 | xxx
(1 row)

(*) int4eq is set as a leakproof function in the default.

postgres=# EXPLAIN SELECT * FROM v2s WHERE f_leak(y) AND a = 2;
QUERY PLAN
-------------------------------------------------------------------------------
Subquery Scan on v2s (cost=0.00..16.56 rows=1 width=72)
Filter: f_leak(v2s.y)
-> Nested Loop (cost=0.00..16.55 rows=1 width=72)
-> Index Scan using t1_pkey on t1 (cost=0.00..8.27 rows=1 width=36)
Index Cond: (a = 2)
Filter: ((a % 2) = 0)
-> Index Scan using t2_pkey on t2 (cost=0.00..8.27 rows=1 width=36)
Index Cond: (x = 2)
(8 rows)

Thanks,

2011/10/21 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Oct 21, 2011 at 10:36 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> So, I will split the patch into two parts as follows, in the next commit fest.
>>
>> Part-1) Views with security_barrier reloption
>>
>> The part-1 portion provides views "security_barrier" reloption; that enables
>> to keep sub-queries unflatten in the prepjoin.c stage.
>> In addition, these sub-queries (that originally come from views with
>> "security_barrier" option) don't allow to push down qualifiers from upper
>> level. It shall prevent both of the problematic scenarios.
>>
>> Part-2) Functions with leakproof attribute
>>
>> The part-2 portion provides functions "leakproof" attribute; that enables
>> to push down leakproof functions into sub-queries, even if it originally
>> come from security views.
>> It shall minimize performance damages when we use view for row-level
>> security purpose.
>
> Sounds reasonable.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

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

On Wed, Nov 2, 2011 at 7:34 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> [ new patch, with example query plans ]

I like the look of those query plans.

Redefining the RangeTblEntry's relid field to be valid for either a
table or a subquery that originated from a view seems problematic to
me, though. For one thing, it's hard to say how much other code
assumes that field to be valid only for a table. For example, you
didn't update _readRangeTblEntry(), and I wouldn't bet on that being
the only place that needs fixing. For another thing, instead of
changing the meaning of the relid field, you could just leave that
alone and instead add a "bool security_barrier field" that caches the
answer; ApplyRetrieveRule() has the Relation object and could set that
field appropriately, and then subquery_was_security_barrier() wouldn't
need a syscache lookup.

Now, the obvious objection is that the security-barrier attribute
might change between the time the RTE is created and the time that it
gets used. But if that's a danger, then presumably the whole view
could also change, in which case the Query object would be pointing to
the wrong data anyway. I'm not sure I fully understand the details
here, but it seems like it ought to be safe to cache the
security_barrier attribute any place it's safe to cache the Query
itself. It certainly doesn't seem right to think that we might end up
using a new value of the security_barrier attribute with an old query,
or the other way around. So something seems funky here.

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

2011/11/2 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Wed, Nov 2, 2011 at 7:34 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> [ new patch, with example query plans ]
>
> I like the look of those query plans.
>
> Redefining the RangeTblEntry's relid field to be valid for either a
> table or a subquery that originated from a view seems problematic to
> me, though.  For one thing, it's hard to say how much other code
> assumes that field to be valid only for a table.  For example, you
> didn't update _readRangeTblEntry(), and I wouldn't bet on that being
> the only place that needs fixing.  For another thing, instead of
> changing the meaning of the relid field, you could just leave that
> alone and instead add a "bool security_barrier field" that caches the
> answer; ApplyRetrieveRule() has the Relation object and could set that
> field appropriately, and then subquery_was_security_barrier() wouldn't
> need a syscache lookup.
>
> Now, the obvious objection is that the security-barrier attribute
> might change between the time the RTE is created and the time that it
> gets used.  But if that's a danger, then presumably the whole view
> could also change, in which case the Query object would be pointing to
> the wrong data anyway.  I'm not sure I fully understand the details
> here, but it seems like it ought to be safe to cache the
> security_barrier attribute any place it's safe to cache the Query
> itself.  It certainly doesn't seem right to think that we might end up
> using a new value of the security_barrier attribute with an old query,
> or the other way around.  So something seems funky here.
>
The reason why I redefined the relid of RangeTblEntry is to avoid
the problem when security_barrier attribute get changed by concurrent
transactions between rewriter and planenr stage.

Of course, I'm not 100% sure whether we have a routine that assumes
valid relid of RangeTblEntry is regular table, or not, although we could
run the regression test correctly.

As I examined before, updates of the issued pg_class shall invalidate
prepared statements that assumed a particular security_barrier
(maybe, PlanCacheRelCallback does this work?), so it is unavailable
to use old plans based on old view definition.
If we want to avoid syscache lookup on subquery_was_security_barrier(),
I think it is a feasible idea to hold the value of security_barrier within RTE.

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

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> The reason why I redefined the relid of RangeTblEntry is to avoid
> the problem when security_barrier attribute get changed by concurrent
> transactions between rewriter and planenr stage.

This is complete nonsense. If the information is being injected into
the querytree by the rewriter, it's sufficient to assume that it's up to
date. Were it not so, we'd have problems with CREATE OR REPLACE RULE,
too.

regards, tom lane

2011/11/2 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> The reason why I redefined the relid of RangeTblEntry is to avoid
>> the problem when security_barrier attribute get changed by concurrent
>> transactions between rewriter and planenr stage.
>
> This is complete nonsense.  If the information is being injected into
> the querytree by the rewriter, it's sufficient to assume that it's up to
> date.  Were it not so, we'd have problems with CREATE OR REPLACE RULE,
> too.
>
I revised the patches to revert redefinition in relid of RangeTblEntry, and
add a flag of "security_barrier".
I seems to work fine, even if view's property was changed between
rewriter and planner stage.

postgres=# CREATE VIEW v1 WITH (security_barrier) AS SELECT * FROM t1
WHERE a % 2 = 0;
CREATE VIEW
postgres=# PREPARE p1 AS SELECT * FROM v1 WHERE f_leak(b);
PREPARE
postgres=# EXECUTE p1;
NOTICE: f_leak => bbb
NOTICE: f_leak => ddd
a | b
---+-----
2 | bbb
4 | ddd
(2 rows)

postgres=# ALTER VIEW v1 SET (security_barrier=false);
ALTER VIEW
postgres=# EXECUTE p1;
NOTICE: f_leak => aaa
NOTICE: f_leak => bbb
NOTICE: f_leak => ccc
NOTICE: f_leak => ddd
NOTICE: f_leak => eee
a | b
---+-----
2 | bbb
4 | ddd
(2 rows)

postgres=# ALTER VIEW v1 SET (security_barrier=true);
ALTER VIEW
postgres=# EXECUTE p1;
NOTICE: f_leak => bbb
NOTICE: f_leak => ddd
a | b
---+-----
2 | bbb
4 | ddd
(2 rows)

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

I rebased my patch set. New functions in pg_proc.h prevented to apply
previous revision cleanly. Here is no functional changes.

2011/11/3 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2011/11/2 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>>> The reason why I redefined the relid of RangeTblEntry is to avoid
>>> the problem when security_barrier attribute get changed by concurrent
>>> transactions between rewriter and planenr stage.
>>
>> This is complete nonsense.  If the information is being injected into
>> the querytree by the rewriter, it's sufficient to assume that it's up to
>> date.  Were it not so, we'd have problems with CREATE OR REPLACE RULE,
>> too.
>>
> I revised the patches to revert redefinition in relid of RangeTblEntry, and
> add a flag of "security_barrier".
> I seems to work fine, even if view's property was changed between
> rewriter and planner stage.
>
> postgres=# CREATE VIEW v1 WITH (security_barrier) AS SELECT * FROM t1
> WHERE a % 2 = 0;
> CREATE VIEW
> postgres=# PREPARE p1 AS SELECT * FROM v1 WHERE f_leak(b);
> PREPARE
> postgres=# EXECUTE p1;
> NOTICE:  f_leak => bbb
> NOTICE:  f_leak => ddd
>  a |  b
> ---+-----
>  2 | bbb
>  4 | ddd
> (2 rows)
>
> postgres=# ALTER VIEW v1 SET (security_barrier=false);
> ALTER VIEW
> postgres=# EXECUTE p1;
> NOTICE:  f_leak => aaa
> NOTICE:  f_leak => bbb
> NOTICE:  f_leak => ccc
> NOTICE:  f_leak => ddd
> NOTICE:  f_leak => eee
>  a |  b
> ---+-----
>  2 | bbb
>  4 | ddd
> (2 rows)
>
> postgres=# ALTER VIEW v1 SET (security_barrier=true);
> ALTER VIEW
> postgres=# EXECUTE p1;
> NOTICE:  f_leak => bbb
> NOTICE:  f_leak => ddd
>  a |  b
> ---+-----
>  2 | bbb
>  4 | ddd
> (2 rows)
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

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

Sorry, patches were not attached. Again.

I rebased my patch set. New functions in pg_proc.h prevented to apply
previous revision cleanly. Here is no functional changes.

Thanks,

2011/12/3 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> I rebased my patch set. New functions in pg_proc.h prevented to apply
> previous revision cleanly. Here is no functional changes.
>
> 2011/11/3 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>> 2011/11/2 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>>>> The reason why I redefined the relid of RangeTblEntry is to avoid
>>>> the problem when security_barrier attribute get changed by concurrent
>>>> transactions between rewriter and planenr stage.
>>>
>>> This is complete nonsense.  If the information is being injected into
>>> the querytree by the rewriter, it's sufficient to assume that it's up to
>>> date.  Were it not so, we'd have problems with CREATE OR REPLACE RULE,
>>> too.
>>>
>> I revised the patches to revert redefinition in relid of RangeTblEntry, and
>> add a flag of "security_barrier".
>> I seems to work fine, even if view's property was changed between
>> rewriter and planner stage.
>>
>> postgres=# CREATE VIEW v1 WITH (security_barrier) AS SELECT * FROM t1
>> WHERE a % 2 = 0;
>> CREATE VIEW
>> postgres=# PREPARE p1 AS SELECT * FROM v1 WHERE f_leak(b);
>> PREPARE
>> postgres=# EXECUTE p1;
>> NOTICE:  f_leak => bbb
>> NOTICE:  f_leak => ddd
>>  a |  b
>> ---+-----
>>  2 | bbb
>>  4 | ddd
>> (2 rows)
>>
>> postgres=# ALTER VIEW v1 SET (security_barrier=false);
>> ALTER VIEW
>> postgres=# EXECUTE p1;
>> NOTICE:  f_leak => aaa
>> NOTICE:  f_leak => bbb
>> NOTICE:  f_leak => ccc
>> NOTICE:  f_leak => ddd
>> NOTICE:  f_leak => eee
>>  a |  b
>> ---+-----
>>  2 | bbb
>>  4 | ddd
>> (2 rows)
>>
>> postgres=# ALTER VIEW v1 SET (security_barrier=true);
>> ALTER VIEW
>> postgres=# EXECUTE p1;
>> NOTICE:  f_leak => bbb
>> NOTICE:  f_leak => ddd
>>  a |  b
>> ---+-----
>>  2 | bbb
>>  4 | ddd
>> (2 rows)
>>
>> Thanks,
>> --
>> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>
>
>
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

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

On Sat, Dec 3, 2011 at 3:19 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> I rebased my patch set. New functions in pg_proc.h prevented to apply
> previous revision cleanly. Here is no functional changes.

I was thinking that my version of this (attached to an email from
earlier today) might be about ready to commit. But while I was
trolling through the archives on this problem trying to figure out who
to credit, I found an old complaint of Tom's that we never fixed, and
which represents a security exposure for this patch:

rhaas=# create table foo (a integer);
CREATE TABLE
rhaas=# insert into foo select generate_series(1,10);
INSERT 0 10
rhaas=# insert into foo values (1);
INSERT 0 1
rhaas=# analyze foo;
ANALYZE
rhaas=# create view safe_foo with (security_barrier) as select * from
foo where a > 5;
CREATE VIEW
rhaas=# grant select on safe_foo to bob;
GRANT

Secure in the knowledge that Bob will only be able to see rows where a
is 6 or higher, we go to bed. But Bob finds a way to outsmart us:

rhaas=> create or replace function leak(integer,integer) returns
boolean as $$begin raise notice 'leak % %', $1, $2; return false;
end$$ language plpgsql;
CREATE FUNCTION
rhaas=> create operator !! (procedure = leak, leftarg = integer,
rightarg = integer, restrict = eqsel);
CREATE OPERATOR
rhaas=> explain select * from safe_foo where a !! 0;
NOTICE: leak 1 0
QUERY PLAN
-------------------------------------------------------------
Subquery Scan on safe_foo (cost=0.00..2.70 rows=1 width=4)
Filter: (safe_foo.a !! 0)
-> Seq Scan on foo (cost=0.00..1.14 rows=6 width=4)
Filter: (a > 5)
(4 rows)

OOPS. The *executor* has been persuaded not to apply the
possibly-nefarious operator !! to the data until after applying the
security-critical qual "a > 5". But the *planner* has no such
compunctions, and has cheerfully leaked the most common value in the
table, which the user wasn't supposed to see. I guess it's hopeless
to suppose that we're going to completely conceal the list of MCVs
from the user, since it might change the plan - and even if
ProcessUtility_hook or somesuch is used to disable EXPLAIN, the user
can still try to ferret out the MCVs via a timing attack. That having
been said, the above behavior doesn't sit well with me: letting the
user probe for MCVs via a timing attack or a plan change is one thing;
printing them out on request is a little bit too convenient for my
taste. :-(

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

2011/12/8 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sat, Dec 3, 2011 at 3:19 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I rebased my patch set. New functions in pg_proc.h prevented to apply
>> previous revision cleanly. Here is no functional changes.
>
> I was thinking that my version of this (attached to an email from
> earlier today) might be about ready to commit.  But while I was
> trolling through the archives on this problem trying to figure out who
> to credit, I found an old complaint of Tom's that we never fixed, and
> which represents a security exposure for this patch:
>
> rhaas=# create table foo (a integer);
> CREATE TABLE
> rhaas=# insert into foo select generate_series(1,10);
> INSERT 0 10
> rhaas=# insert into foo values (1);
> INSERT 0 1
> rhaas=# analyze foo;
> ANALYZE
> rhaas=# create view safe_foo with (security_barrier) as select * from
> foo where a > 5;
> CREATE VIEW
> rhaas=# grant select on safe_foo to bob;
> GRANT
>
> Secure in the knowledge that Bob will only be able to see rows where a
> is 6 or higher, we go to bed.  But Bob finds a way to outsmart us:
>
> rhaas=> create or replace function leak(integer,integer) returns
> boolean as $$begin raise notice 'leak % %', $1, $2; return false;
> end$$ language plpgsql;
> CREATE FUNCTION
> rhaas=> create operator !! (procedure = leak, leftarg = integer,
> rightarg = integer, restrict = eqsel);
> CREATE OPERATOR
> rhaas=> explain select * from safe_foo where a !! 0;
> NOTICE:  leak 1 0
>                         QUERY PLAN
> -------------------------------------------------------------
>  Subquery Scan on safe_foo  (cost=0.00..2.70 rows=1 width=4)
>   Filter: (safe_foo.a !! 0)
>   ->  Seq Scan on foo  (cost=0.00..1.14 rows=6 width=4)
>         Filter: (a > 5)
> (4 rows)
>
> OOPS.  The *executor* has been persuaded not to apply the
> possibly-nefarious operator !! to the data until after applying the
> security-critical qual "a > 5".  But the *planner* has no such
> compunctions, and has cheerfully leaked the most common value in the
> table, which the user wasn't supposed to see.  I guess it's hopeless
> to suppose that we're going to completely conceal the list of MCVs
> from the user, since it might change the plan - and even if
> ProcessUtility_hook or somesuch is used to disable EXPLAIN, the user
> can still try to ferret out the MCVs via a timing attack.  That having
> been said, the above behavior doesn't sit well with me: letting the
> user probe for MCVs via a timing attack or a plan change is one thing;
> printing them out on request is a little bit too convenient for my
> taste.  :-(
>
Sorry, I missed this scenario, and have not investigated this code path
in detail yet.

My first impression remind me an idea that I proposed before, even
though it got negative response due to user visible changes.
It requires superuser privilege to create new operators, since we
assume superuser does not set up harmful configuration.

I still think it is an idea. Or, maybe, we can adopt a bit weaker restriction;
functions being used to operators must have "leakproof" property.

Is it worthful to have a discussion again?

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

On Thu, Dec 8, 2011 at 5:17 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> My first impression remind me an idea that I proposed before, even
> though it got negative response due to user visible changes.
> It requires superuser privilege to create new operators, since we
> assume superuser does not set up harmful configuration.

I don't think that's acceptable from a usability point of view; this
feature is important, but not important enough to go start ripping out
other features that people are already using, like non-superuser
operators. I'm also pretty skeptical that it would fix the problem,
because the superuser might fail to realize that creating an operator
was going to create this type of security exposure. After all, you
and I also failed to realize that, so it's obviously a fairly subtle
problem.

I feel like there must be some logic in the planner somewhere that is
"looking through" the subquery RTE and figuring out that safe_foo.a is
really the same variable as foo.a, and which therefore feels entitled
to apply !!'s selectivity estimator to foo.a's statistics. If that's
the case, it might be possible to handicap that logic so that when the
security_barrier flag is set, it doesn't do that, and instead treats
safe_foo.a as a black box. That would, obviously, degrade the quality
of complex plans involving security views, but I think we should focus
on getting something that meets our security goals first and then try
to improve performance later.

(For example, I am fairly certain that only a superuser can install a
new selectivity estimator; so perhaps we could allow selectivity
estimators to be signaled with the information that a security view
interposes or not, and then they can make an estimator-specific
decision on how to punt; but on the other hand that might be a stupid
idea; so for step #1 let's just figure out how to batten down the
hatches.)

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

2011/12/9 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Dec 8, 2011 at 5:17 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> My first impression remind me an idea that I proposed before, even
>> though it got negative response due to user visible changes.
>> It requires superuser privilege to create new operators, since we
>> assume superuser does not set up harmful configuration.
>
> I don't think that's acceptable from a usability point of view; this
> feature is important, but not important enough to go start ripping out
> other features that people are already using, like non-superuser
> operators.  I'm also pretty skeptical that it would fix the problem,
> because the superuser might fail to realize that creating an operator
> was going to create this type of security exposure.  After all, you
> and I also failed to realize that, so it's obviously a fairly subtle
> problem.
>
OK, I agree with your opinion. It may stand on a fiction story that
superuser understand all effects and risk of his operations.
If this assumption get broken, system's security is also broken.

> I feel like there must be some logic in the planner somewhere that is
> "looking through" the subquery RTE and figuring out that safe_foo.a is
> really the same variable as foo.a, and which therefore feels entitled
> to apply !!'s selectivity estimator to foo.a's statistics.  If that's
> the case, it might be possible to handicap that logic so that when the
> security_barrier flag is set, it doesn't do that, and instead treats
> safe_foo.a as a black box.  That would, obviously, degrade the quality
> of complex plans involving security views, but I think we should focus
> on getting something that meets our security goals first and then try
> to improve performance later.
>
I tried to investigate the code around size-estimation, and it seems to
me here is two candidates to put this type of checks.

The one is examine_simple_variable() that is used to pull a datum
from statistic information, but it locates on the code path restriction
estimator of operators; so user controlable, although it requires least
code changes just after if (rte->rtekind == RTE_SUBQUERY).
The other is clause_selectivity(). Its code path is not user controlable,
so we can apply necessary checks to prevent qualifier that reference
variable come from sub-query with security-barrier.

In my sense, clause_selectivity() is better place to apply this type of
checks. But, on the other hand, it provides get_relation_stats_hook
to allow extensions to control references to statistic data.
So, I wonder whether the PG core assumes this routine covers
all the code path here?

In addition, I also consider the case when we add a functionality that
forcibly adds restriction on WHERE clause of regular tables in the
future version, like:
SELECT * FROM t WHERE a > 0;
==> SELECT * FROM t WHERE sepgsql_policy(selinux_label) AND a > 0;
Probably, same solution will be available to avoid unintentional references
to pg_statistic; as long as security_barrier is set on rte of regular tables.

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

2011/12/10 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2011/12/9 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> I feel like there must be some logic in the planner somewhere that is
>> "looking through" the subquery RTE and figuring out that safe_foo.a is
>> really the same variable as foo.a, and which therefore feels entitled
>> to apply !!'s selectivity estimator to foo.a's statistics.  If that's
>> the case, it might be possible to handicap that logic so that when the
>> security_barrier flag is set, it doesn't do that, and instead treats
>> safe_foo.a as a black box.  That would, obviously, degrade the quality
>> of complex plans involving security views, but I think we should focus
>> on getting something that meets our security goals first and then try
>> to improve performance later.
>>
> I tried to investigate the code around size-estimation, and it seems to
> me here is two candidates to put this type of checks.
>
> The one is examine_simple_variable() that is used to pull a datum
> from statistic information, but it locates on the code path restriction
> estimator of operators; so user controlable, although it requires least
> code changes just after if (rte->rtekind == RTE_SUBQUERY).
> The other is clause_selectivity(). Its code path is not user controlable,
> so we can apply necessary checks to prevent qualifier that reference
> variable come from sub-query with security-barrier.
>
> In my sense, clause_selectivity() is better place to apply this type of
> checks. But, on the other hand, it provides get_relation_stats_hook
> to allow extensions to control references to statistic data.
> So, I wonder whether the PG core assumes this routine covers
> all the code path here?
>
The attached patch adds checks around invocation of selectivity
estimator functions, and it changes behavior of the estimator, if the
supplied operator tries to touch variables come from security-barrier
relations.
Then, it fixes the problem you mentioned.

postgres=# explain select * from safe_foo where a !! 0;
QUERY PLAN
-------------------------------------------------------------
Subquery Scan on safe_foo (cost=0.00..2.70 rows=3 width=4)
Filter: (safe_foo.a !! 0)
-> Seq Scan on foo (cost=0.00..1.14 rows=6 width=4)
Filter: (a > 5)
(4 rows)

However, I'm still a bit skeptical on my patch, because it still allows
to invoke estimator function when operator's argument does not
touch values originated from security-barrier relation.
In the case when oprrest or oprjoin are implemented without our
regular convention (E.g, it anyway reference whole of statistical data),
it will break this solution.

Of course, it is an assumption that we cannot prevent any attack
using binary modules, so we need to say "use it your own risk" if
people tries to use extensional modules. And, we also need to
keep the built-in code secure.

Some of built-in estimator functions (such as eqsel) provides
a feature that invokes operator function with arguments originated
from pg_statistics table. It didn't harmless, however, we got understand
that this logic can be used to break row-level security.
So, I begin to consider the routines to be revised are some of built-in
functions to be used for estimator functions; such as eqsel and ...
These function eventually reference statistical data at examine_variable.

It might be a better approach to add checks whether invocation of
the supplied operator possibly leaks contents to be invisible.
It seems to me the Idea of the attached patch depends on something
internal stuff of existing built-in estimator functions...

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

The attached patches are cut-off version based on the latest Robert's
updates. The "v8.regtest" adds regression test cases on variable
leaky-view scenarios with/without security-barrier property.

The "v8.option-1" add checks around restriction_selectivity, and prevent
to invoke estimator function if Var node touches relation with security-
barrier attribute.

The "v8.option-2" add checks around examine_simple_variable, and
prevent to reference statistical data, if Var node tries to reference
relation with security-barrier attribute.
(And, it shall be marked as "leakproof")

I initially thought restriction_selectivity called by clause_selectivity is
the best point to add checks, however, I reconsidered it might not be
the origin of this problem.
As long as user-defined functions acquires control on selectivity
estimation of operators, same problems can be re-produced;
if someone tries to reference unrelated data within estimator.

This scenario is normally prevented, because only superuser can define
a function that can bypass permission checks to reference internal data
structures; using "untrusted" procedural-language.
If my conclusion is right, what we should fix up is built-in estimators side,
and we should enforce estimator function being "leakproof", even though
we still allow unprivileged users to define operators.

Thanks,

2011/12/11 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2011/12/10 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>> 2011/12/9 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> I feel like there must be some logic in the planner somewhere that is
>>> "looking through" the subquery RTE and figuring out that safe_foo.a is
>>> really the same variable as foo.a, and which therefore feels entitled
>>> to apply !!'s selectivity estimator to foo.a's statistics.  If that's
>>> the case, it might be possible to handicap that logic so that when the
>>> security_barrier flag is set, it doesn't do that, and instead treats
>>> safe_foo.a as a black box.  That would, obviously, degrade the quality
>>> of complex plans involving security views, but I think we should focus
>>> on getting something that meets our security goals first and then try
>>> to improve performance later.
>>>
>> I tried to investigate the code around size-estimation, and it seems to
>> me here is two candidates to put this type of checks.
>>
>> The one is examine_simple_variable() that is used to pull a datum
>> from statistic information, but it locates on the code path restriction
>> estimator of operators; so user controlable, although it requires least
>> code changes just after if (rte->rtekind == RTE_SUBQUERY).
>> The other is clause_selectivity(). Its code path is not user controlable,
>> so we can apply necessary checks to prevent qualifier that reference
>> variable come from sub-query with security-barrier.
>>
>> In my sense, clause_selectivity() is better place to apply this type of
>> checks. But, on the other hand, it provides get_relation_stats_hook
>> to allow extensions to control references to statistic data.
>> So, I wonder whether the PG core assumes this routine covers
>> all the code path here?
>>
> The attached patch adds checks around invocation of selectivity
> estimator functions, and it changes behavior of the estimator, if the
> supplied operator tries to touch variables come from security-barrier
> relations.
> Then, it fixes the problem you mentioned.
>
>  postgres=# explain select * from safe_foo where a !! 0;
>                           QUERY PLAN
>  -------------------------------------------------------------
>   Subquery Scan on safe_foo  (cost=0.00..2.70 rows=3 width=4)
>     Filter: (safe_foo.a !! 0)
>     ->  Seq Scan on foo  (cost=0.00..1.14 rows=6 width=4)
>           Filter: (a > 5)
>  (4 rows)
>
> However, I'm still a bit skeptical on my patch, because it still allows
> to invoke estimator function when operator's argument does not
> touch values originated from security-barrier relation.
> In the case when oprrest or oprjoin are implemented without our
> regular convention (E.g, it anyway reference whole of statistical data),
> it will break this solution.
>
> Of course, it is an assumption that we cannot prevent any attack
> using binary modules, so we need to say "use it your own risk" if
> people tries to use extensional modules. And, we also need to
> keep the built-in code secure.
>
> Some of built-in estimator functions (such as eqsel) provides
> a feature that invokes operator function with arguments originated
> from pg_statistics table. It didn't harmless, however, we got understand
> that this logic can be used to break row-level security.
> So, I begin to consider the routines to be revised are some of built-in
> functions to be used for estimator functions; such as eqsel and ...
> These function eventually reference statistical data at examine_variable.
>
> It might be a better approach to add checks whether invocation of
> the supplied operator possibly leaks contents to be invisible.
> It seems to me the Idea of the attached patch depends on something
> internal stuff of existing built-in estimator functions...
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

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

On Mon, Dec 12, 2011 at 12:00 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> The "v8.option-2" add checks around examine_simple_variable, and
> prevent to reference statistical data, if Var node tries to reference
> relation with security-barrier attribute.

I adopted this approach, and committed this.

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

2011/12/22 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Dec 12, 2011 at 12:00 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> The "v8.option-2" add checks around examine_simple_variable, and
>> prevent to reference statistical data, if Var node tries to reference
>> relation with security-barrier attribute.
>
> I adopted this approach, and committed this.
>
Thanks for your help and efforts.

I'd like the regression test on select_view test being committed also
to detect unexpected changed in the future. How about it?

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

On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> I'd like the regression test on select_view test being committed also
> to detect unexpected changed in the future. How about it?

Can you resend that as a separate patch? I remember there were some
things I didn't like about it, but I don't remember what they were at
the moment...

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

2011/12/23 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I'd like the regression test on select_view test being committed also
>> to detect unexpected changed in the future. How about it?
>
> Can you resend that as a separate patch?  I remember there were some
> things I didn't like about it, but I don't remember what they were at
> the moment...
>
Sorry for this late response.

The attached one is patch of the regression test that checks scenario
of malicious function with/without security_barrier option.

I guess you concerned about that expected/select_views_1.out is
patched, not expected/select_views.out.
I'm not sure the reason why regression test script tries to make diff
between results/select_views and expected/select_views_1.out.

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

On Tue, Jan 3, 2012 at 12:11 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2011/12/23 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> I'd like the regression test on select_view test being committed also
>>> to detect unexpected changed in the future. How about it?
>>
>> Can you resend that as a separate patch?  I remember there were some
>> things I didn't like about it, but I don't remember what they were at
>> the moment...
>>
> Sorry for this late response.
>
> The attached one is patch of the regression test that checks scenario
> of malicious function with/without security_barrier option.
>
> I guess you concerned about that expected/select_views_1.out is
> patched, not expected/select_views.out.
> I'm not sure the reason why regression test script tries to make diff
> between results/select_views and expected/select_views_1.out.

select_views.out and select_views_1.out are alternate expected output
files. The regression tests pass if the actual output matches either
one. Thus, you have to patch both.

BTW, can you also resubmit the leakproof stuff as a separate patch for
the last CF? Want to make sure we get that into 9.2, if at all
possible.

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

>> I guess you concerned about that expected/select_views_1.out is
>> patched, not expected/select_views.out.
>> I'm not sure the reason why regression test script tries to make diff
>> between results/select_views and expected/select_views_1.out.
>
> select_views.out and select_views_1.out are alternate expected output
> files.  The regression tests pass if the actual output matches either
> one.  Thus, you have to patch both.
>
It was new for me. The attached patch updates both of the expected
files, however, I'm not certain whether select_view.out is suitable, or
not, because my results/select_view.out matched with expected/select_view_1.out.

> BTW, can you also resubmit the leakproof stuff as a separate patch for
> the last CF?  Want to make sure we get that into 9.2, if at all
> possible.
>
Yes, it shall be attached on the next message.

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

On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> I guess you concerned about that expected/select_views_1.out is
>>> patched, not expected/select_views.out.
>>> I'm not sure the reason why regression test script tries to make diff
>>> between results/select_views and expected/select_views_1.out.
>>
>> select_views.out and select_views_1.out are alternate expected output
>> files.  The regression tests pass if the actual output matches either
>> one.  Thus, you have to patch both.
>>
> It was new for me. The attached patch updates both of the expected
> files, however, I'm not certain whether select_view.out is suitable, or
> not, because my results/select_view.out matched with expected/select_view_1.out.

Committed. We'll see what the buildfarm thinks.

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

On Tue, Jan 17, 2012 at 7:08 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> I guess you concerned about that expected/select_views_1.out is
>>>> patched, not expected/select_views.out.
>>>> I'm not sure the reason why regression test script tries to make diff
>>>> between results/select_views and expected/select_views_1.out.
>>>
>>> select_views.out and select_views_1.out are alternate expected output
>>> files.  The regression tests pass if the actual output matches either
>>> one.  Thus, you have to patch both.
>>>
>> It was new for me. The attached patch updates both of the expected
>> files, however, I'm not certain whether select_view.out is suitable, or
>> not, because my results/select_view.out matched with expected/select_view_1.out.
>
> Committed.  We'll see what the buildfarm thinks.

This passes installcheck initially. Then upon second invocation of
installcheck, it fails.

It creates the role "alice", and doesn't clean it up. On next
invocation alice already exists and cases a failure in test
select_views.

Cheers,

Jeff

2012/1/21 Jeff Janes <jeff(dot)janes(at)gmail(dot)com>:
> On Tue, Jan 17, 2012 at 7:08 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>>> I guess you concerned about that expected/select_views_1.out is
>>>>> patched, not expected/select_views.out.
>>>>> I'm not sure the reason why regression test script tries to make diff
>>>>> between results/select_views and expected/select_views_1.out.
>>>>
>>>> select_views.out and select_views_1.out are alternate expected output
>>>> files.  The regression tests pass if the actual output matches either
>>>> one.  Thus, you have to patch both.
>>>>
>>> It was new for me. The attached patch updates both of the expected
>>> files, however, I'm not certain whether select_view.out is suitable, or
>>> not, because my results/select_view.out matched with expected/select_view_1.out.
>>
>> Committed.  We'll see what the buildfarm thinks.
>
> This passes installcheck initially.  Then upon second invocation of
> installcheck, it fails.
>
> It creates the role "alice", and doesn't clean it up.  On next
> invocation alice already exists and cases a failure in test
> select_views.
>
Thanks for your pointing out.

The attached patch adds cleaning-up part of object being defined
within this test;
includes user "alice".

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

On Sun, Jan 22, 2012 at 5:57 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> This passes installcheck initially.  Then upon second invocation of
>> installcheck, it fails.
>>
>> It creates the role "alice", and doesn't clean it up.  On next
>> invocation alice already exists and cases a failure in test
>> select_views.
>>
> Thanks for your pointing out.
>
> The attached patch adds cleaning-up part of object being defined
> within this test;
> includes user "alice".

Urp. I failed to notice this patch and committed a different fix for
the problem pointed out by Jeff. I'm inclined to think it's OK to
leave the non-shared objects behind - among other things, if people
are testing pg_upgrade using the regression database, this will help
to ensure that pg_dump is handling security_barrier views correctly.

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