Thread: [v9.2] Fix Leaky View Problem

Hi,

The attached patches try to tackle our long-standing leaky view problem, and were revised according to the discussion
wehad in the commit-fest 1st. 

We already knew two scenarios to leak contents of invisible tuples using functions with side-effects; such as error
messagescontaining its arguments. 

The first one was the order of execution of qualifiers within a scan plan. Query optimizer shall pull-up simple
sub-queriesinto inner-join due to the performance gain, however, it possibly cause a problem that functions supplied at
outsideof the sub-query is launched earlier than functions come from inside of the sub-query; depending on the cost
estimation.In the result, it allows users to reference contents of invisible tuples (to be filtered by view), if they
providea function with side-effects as a part of WHERE clause. 

The second one was the distribution policy of qualifiers. In the case when a view (that intends row-level security)
containsJOIN clause, we hope the qualifiers supplied from outside of the view to be launched after the table join,
becausethe view may filter out some of tuples during checks of its join condition. However, the current query optimizer
willdistribute a qualifier that reference only one-side of the join into inside of the join-loop to minimize number of
tuplesto be joined. In the result, it also allows users to reference contents of invisible tuples. 

In the commit-fest 1st, we made a consensus that a part of views should perform as "security barrier" that enables to
preventunexpected push-down and execution order of qualifiers; being marked by creator of the view. 
And, we also made a consensus obviously secure functions should be allowed to push-down across security barrier; to
minimizeunnecessary performance damages. 

The part-1 patch implements SQL enhancement stuffs; (1) add reloption support on RELKIND_VIEW with "security_barrier"
boolvariable (2) add pg_proc.proleaky flag to show whether the function is possibly leaky, or not. 
The (2) is new stuff from the revision in commit-fest 1st. It enables to supply "NOLEAKY" option on CREATE FUNCTION
statement,then the function is allowed to distribute across security barrier. Only superuser can set this option. 

  Example)
    CREATE FUNCTION safe_func(text) RETURNS bool
        LANGUAGE 'C' NOLEAKY AS '$libdir/safe_lib', 'safe_func';
                     ^^^^^^^
A patch to add a new field into pg_proc always takes a large chunk, so the attached proctrans.php is the script I used
toappend a new field to the existing functions. Right now, I marked it true (= possibly leaky), because we need to have
adiscussion what shall be none-leaky functions in the default. 

The part-2 patch is same as we had discussed in the commit fest. Here is not updates except for rebasing to the latest
tree.It enables to remember the nest level of the qualifier being originally used, and utilize it to sort order of the
qualifiers.

The part-3 patch was a bit revised, although its basic idea has not been changed.
It enables to prevent qualifiers come from outside of security barrier being pushed down into inside of the security
barrier,even if it references only a part of relations within the sub-query expanded from a view with
"security_barrier"flag. 

Thanks,
--
NEC Europe Ltd, SAP Global Competence Center
KaiGai Kohei <kohei.kaigai@emea.nec.com>

2011/9/7 Thom Brown <thom@linux.com>:
> On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:
>>
>> The (2) is new stuff from the revision in commit-fest 1st. It enables to
>> supply "NOLEAKY" option on CREATE FUNCTION statement, then the function is
>> allowed to distribute across security barrier. Only superuser can set this
>> option.
>
> "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin English.
>  Also, it could be read as "Don't allow leaks in this function".  Could we
> instead use something like TRUSTED or something akin to it being allowed to
> do more than safer functions?  It then describes its level of behaviour
> rather than what it promises not to do.
>
Thanks for your comment. I'm not a native English specker, so it is helpful.

"TRUSTED" sounds meaningful for me, however, it is confusable with a concept
of "trusted procedure" in label-based MAC. It is not only SELinux,
Oracle's label
based security also uses this term to mean a procedure that switches user's
credential during its execution. http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm

So, how about "CREDIBLE", instead of "TRUSTED"?

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown <thom@linux.com> wrote:
> On 7 September 2011 14:34, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> 2011/9/7 Thom Brown <thom@linux.com>:
>> > On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:
>> >>
>> >> The (2) is new stuff from the revision in commit-fest 1st. It enables
>> >> to
>> >> supply "NOLEAKY" option on CREATE FUNCTION statement, then the function
>> >> is
>> >> allowed to distribute across security barrier. Only superuser can set
>> >> this
>> >> option.
>> >
>> > "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin
>> > English.
>> >  Also, it could be read as "Don't allow leaks in this function".  Could
>> > we
>> > instead use something like TRUSTED or something akin to it being allowed
>> > to
>> > do more than safer functions?  It then describes its level of behaviour
>> > rather than what it promises not to do.
>> >
>> Thanks for your comment. I'm not a native English specker, so it is
>> helpful.
>>
>> "TRUSTED" sounds meaningful for me, however, it is confusable with a
>> concept
>> of "trusted procedure" in label-based MAC. It is not only SELinux,
>> Oracle's label
>> based security also uses this term to mean a procedure that switches
>> user's
>> credential during its execution.
>>
>>  http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm
>>
>> So, how about "CREDIBLE", instead of "TRUSTED"?
>
> I can't say I'm keen on that alternative, but I'm probably not the one to
> participate in bike-shedding here, so I'll leave comment to you hackers. :)

I think TRUSTED actually does a reasonably good job capturing what
we're after here, although I do share a bit of KaiGai's nervousness
about terminological confusion.  Still, I'd be inclined to go that way
if we can't come up with anything better.  CREDIBLE is definitely the
wrong idea: that means "believable", which sounds more like a
statement about the function's results than about its side-effects.  I
thought about TACITURN, since we need the error messages to not be
excessively informative, but that doesn't do a good job characterizing
the hazard created by side-effects, or the potential for abuse due to
- for example - deliberate division by zero.  I also thought about
PURE, which is a term that's sometimes used to describe code that
throws no errors and has no side effects, and comes pretty close to
our actual requirement here, but doesn't necessarily convey that a
security concern is involved.  Yet another idea would be to use a
variant of TRUSTED, such as TRUSTWORTHY, just to avoid confusion with
the idea of a trusted procedure, but I'm not that excited about that
idea despite have no real specific gripe with it other than length.
So at the moment I am leaning toward TRUSTED.

Anyone else want to bikeshed?

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

Robert Haas <robertmhaas@gmail.com> wrote:
> Anyone else want to bikeshed?
I'm not sure they beat TRUSTED, but:
SECURE
OPAQUE
-Kevin

2011/9/7 Robert Haas <robertmhaas@gmail.com>:
> On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown <thom@linux.com> wrote:
>> On 7 September 2011 14:34, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>> 2011/9/7 Thom Brown <thom@linux.com>:
>>> > On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:
>>> >>
>>> >> The (2) is new stuff from the revision in commit-fest 1st. It enables
>>> >> to
>>> >> supply "NOLEAKY" option on CREATE FUNCTION statement, then the function
>>> >> is
>>> >> allowed to distribute across security barrier. Only superuser can set
>>> >> this
>>> >> option.
>>> >
>>> > "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin
>>> > English.
>>> >  Also, it could be read as "Don't allow leaks in this function".  Could
>>> > we
>>> > instead use something like TRUSTED or something akin to it being allowed
>>> > to
>>> > do more than safer functions?  It then describes its level of behaviour
>>> > rather than what it promises not to do.
>>> >
>>> Thanks for your comment. I'm not a native English specker, so it is
>>> helpful.
>>>
>>> "TRUSTED" sounds meaningful for me, however, it is confusable with a
>>> concept
>>> of "trusted procedure" in label-based MAC. It is not only SELinux,
>>> Oracle's label
>>> based security also uses this term to mean a procedure that switches
>>> user's
>>> credential during its execution.
>>>
>>>  http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm
>>>
>>> So, how about "CREDIBLE", instead of "TRUSTED"?
>>
>> I can't say I'm keen on that alternative, but I'm probably not the one to
>> participate in bike-shedding here, so I'll leave comment to you hackers. :)
>
> I think TRUSTED actually does a reasonably good job capturing what
> we're after here, although I do share a bit of KaiGai's nervousness
> about terminological confusion.  Still, I'd be inclined to go that way
> if we can't come up with anything better.  CREDIBLE is definitely the
> wrong idea: that means "believable", which sounds more like a
> statement about the function's results than about its side-effects.  I
> thought about TACITURN, since we need the error messages to not be
> excessively informative, but that doesn't do a good job characterizing
> the hazard created by side-effects, or the potential for abuse due to
> - for example - deliberate division by zero.  I also thought about
> PURE, which is a term that's sometimes used to describe code that
> throws no errors and has no side effects, and comes pretty close to
> our actual requirement here, but doesn't necessarily convey that a
> security concern is involved.  Yet another idea would be to use a
> variant of TRUSTED, such as TRUSTWORTHY, just to avoid confusion with
> the idea of a trusted procedure, but I'm not that excited about that
> idea despite have no real specific gripe with it other than length.
> So at the moment I am leaning toward TRUSTED.
>
> Anyone else want to bikeshed?
>
I also become leaning toward TRUSTED, although we still have a bit risk of
terminology confusion, because I assume it is quite rare case to set this
option by DBA and we will able to expect DBAs who try to this option have
correct knowledge about background of the leaky-view problem.

I'll submit the patch again.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On 2011-09-07 16:02, Kevin Grittner wrote:
> Robert Haas<robertmhaas@gmail.com>  wrote:
>
>> Anyone else want to bikeshed?
>
> I'm not sure they beat TRUSTED, but:
>
> SECURE
> OPAQUE

SAVE

-- Yeb

On Wed, Sep 07, 2011 at 02:09:15PM +0100, Thom Brown wrote:
> On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:
> 
> > The (2) is new stuff from the revision in commit-fest 1st. It enables to
> > supply "NOLEAKY" option on CREATE FUNCTION statement, then the function is
> > allowed to distribute across security barrier. Only superuser can set this
> > option.
> >
> 
> "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin English.
>  Also, it could be read as "Don't allow leaks in this function".  Could we
> instead use something like TRUSTED or something akin to it being allowed to
> do more than safer functions?  It then describes its level of behaviour
> rather than what it promises not to do.

I liked NOLEAKY for its semantics, though I probably would have spelled it
"LEAKPROOF".  PostgreSQL will trust the function to implement a specific,
relatively-unintuitive security policy.  We want the function implementers to
read that policy closely and not rely on any intuition they have about the
"trusted" term of art.  Our use of TRUSTED in CREATE LANGUAGE is more
conventional, I think, as is the trusted nature of SECURITY DEFINER.  In that
vein, folks who actually need SECURITY DEFINER might first look at TRUSTED;
NOLEAKY would not attract the same unwarranted attention.

Noah Misch <noah@leadboat.com> writes:
> I liked NOLEAKY for its semantics, though I probably would have spelled it
> "LEAKPROOF".  PostgreSQL will trust the function to implement a specific,
> relatively-unintuitive security policy.  We want the function implementers to
> read that policy closely and not rely on any intuition they have about the
> "trusted" term of art.  Our use of TRUSTED in CREATE LANGUAGE is more
> conventional, I think, as is the trusted nature of SECURITY DEFINER.  In that
> vein, folks who actually need SECURITY DEFINER might first look at TRUSTED;
> NOLEAKY would not attract the same unwarranted attention.

I agree that TRUSTED is a pretty bad choice here because of the high
probability that people will think it means something else than what
it really means.  LEAKPROOF isn't too bad.
        regards, tom lane

2011/9/7 Tom Lane <tgl@sss.pgh.pa.us>:
> Noah Misch <noah@leadboat.com> writes:
>> I liked NOLEAKY for its semantics, though I probably would have spelled it
>> "LEAKPROOF".  PostgreSQL will trust the function to implement a specific,
>> relatively-unintuitive security policy.  We want the function implementers to
>> read that policy closely and not rely on any intuition they have about the
>> "trusted" term of art.  Our use of TRUSTED in CREATE LANGUAGE is more
>> conventional, I think, as is the trusted nature of SECURITY DEFINER.  In that
>> vein, folks who actually need SECURITY DEFINER might first look at TRUSTED;
>> NOLEAKY would not attract the same unwarranted attention.
>
> I agree that TRUSTED is a pretty bad choice here because of the high
> probability that people will think it means something else than what
> it really means.  LEAKPROOF isn't too bad.
>
It seems to me LEAKPROOF is never confusable for everyone, and
no conflicts with other concept, although it was not in my vocaburary.

If no better idea anymore, I'll submit the patch again; with LEAKPROOF keyword.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On 09/07/2011 12:05 PM, Tom Lane wrote:
>
> I agree that TRUSTED is a pretty bad choice here because of the high
> probability that people will think it means something else than what
> it really means.

Agreed.

> LEAKPROOF isn't too bad.
>
>             

It's fairly opaque unless you know the context, although that might be 
said of some of our other terms too. Someone coming across it is going 
to think "What would it leak?"

cheers

andrew

2011/9/7 Andrew Dunstan <andrew@dunslane.net>:
>> LEAKPROOF isn't too bad.
>>
>>
>
> It's fairly opaque unless you know the context, although that might be said
> of some of our other terms too. Someone coming across it is going to think
> "What would it leak?"
>
It is introduced in the documentation. I'll add a point to this
chapter in the introduction of this keyword.

http://developer.postgresql.org/pgdocs/postgres/rules-privileges.html

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Andrew Dunstan <andrew@dunslane.net> writes:
> On 09/07/2011 12:05 PM, Tom Lane wrote:
>> LEAKPROOF isn't too bad.

> It's fairly opaque unless you know the context, although that might be 
> said of some of our other terms too. Someone coming across it is going 
> to think "What would it leak?"

Well, the whole point is that we want people to RTFM instead of assuming
they know what it means ...
        regards, tom lane

I updated the patches of fix-leaky-view problem, according to the
previous discussion.
The "NOLEAKY" option was replaced by "LEAKPROOF" option, and several regression
test cases were added. Rest of stuffs are unchanged.

For convenience of reviewer, below is summary of these patches:

The Part-1 implements corresponding SQL syntax stuffs which are
"security_barrier"
reloption of views, and "LEAKPROOF" option on creation of functions to be stored
new pg_proc.proleakproof field.

The Part-2 tries to tackles a leaky-view scenarios by functions with
very tiny cost
estimation value. It was same one we had discussed in the commitfest-1st.
It prevents to launch functions earlier than ones come from inside of views with
"security_barrier" option.

The Part-3 tries to tackles a leaky-view scenarios by functions that references
one side of join loop. It prevents to distribute qualifiers including
functions without
"leakproof" attribute into relations across security-barrier.

Thanks,

2011/9/7 Kohei KaiGai <kaigai@kaigai.gr.jp>:
> 2011/9/7 Tom Lane <tgl@sss.pgh.pa.us>:
>> Noah Misch <noah@leadboat.com> writes:
>>> I liked NOLEAKY for its semantics, though I probably would have spelled it
>>> "LEAKPROOF".  PostgreSQL will trust the function to implement a specific,
>>> relatively-unintuitive security policy.  We want the function implementers to
>>> read that policy closely and not rely on any intuition they have about the
>>> "trusted" term of art.  Our use of TRUSTED in CREATE LANGUAGE is more
>>> conventional, I think, as is the trusted nature of SECURITY DEFINER.  In that
>>> vein, folks who actually need SECURITY DEFINER might first look at TRUSTED;
>>> NOLEAKY would not attract the same unwarranted attention.
>>
>> I agree that TRUSTED is a pretty bad choice here because of the high
>> probability that people will think it means something else than what
>> it really means.  LEAKPROOF isn't too bad.
>>
> It seems to me LEAKPROOF is never confusable for everyone, and
> no conflicts with other concept, although it was not in my vocaburary.
>
> If no better idea anymore, I'll submit the patch again; with LEAKPROOF keyword.
>
> Thanks,
> --
> KaiGai Kohei <kaigai@kaigai.gr.jp>
>



--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> I updated the patches of fix-leaky-view problem, according to the
> previous discussion.
> The "NOLEAKY" option was replaced by "LEAKPROOF" option, and several regression
> test cases were added. Rest of stuffs are unchanged.

You have a leftover reference to NOLEAKY.

> For convenience of reviewer, below is summary of these patches:
>
> The Part-1 implements corresponding SQL syntax stuffs which are
> "security_barrier"
> reloption of views, and "LEAKPROOF" option on creation of functions to be stored
> new pg_proc.proleakproof field.

The way you have this implemented, we just blow away all view options
whenever we do CREATE OR REPLACE VIEW.  Is that the behavior we want?
If a security_barrier view gets accidentally turned into a
non-security_barrier view, doesn't that create a security_hole?

I'm also wondering if the way you're using ResetViewOptions() is the
right way to handle this anyhow.  Isn't that going to update pg_class
twice?  I guess that's probably harmless from a performance
standpoint, but wouldn't it be better not to?  I guess we could define
something like AT_ReplaceRelOptions to handle this case.

The documentation in general is not nearly adequate, at least IMHO.

I'm a bit nervous about storing security_barrier in the RTE.  What
happens to stored rules if the security_barrier option gets change
later?

More when I've had more time to look at this...

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

On Fri, Sep 23, 2011 at 06:25:01PM -0400, Robert Haas wrote:
> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> > The Part-1 implements corresponding SQL syntax stuffs which are
> > "security_barrier"
> > reloption of views, and "LEAKPROOF" option on creation of functions to be stored
> > new pg_proc.proleakproof field.
>
> The way you have this implemented, we just blow away all view options
> whenever we do CREATE OR REPLACE VIEW.  Is that the behavior we want?
> If a security_barrier view gets accidentally turned into a
> non-security_barrier view, doesn't that create a security_hole?

I think CREATE OR REPLACE needs to keep meaning just that, never becoming
"replace some characteristics, merge others".  The consequence is less than
delightful here, but I don't have an idea that avoids this problem without
running afoul of some previously-raised design constraint.

On Sat, Sep 24, 2011 at 5:37 PM, Noah Misch <noah@leadboat.com> wrote:
> On Fri, Sep 23, 2011 at 06:25:01PM -0400, Robert Haas wrote:
>> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> > The Part-1 implements corresponding SQL syntax stuffs which are
>> > "security_barrier"
>> > reloption of views, and "LEAKPROOF" option on creation of functions to be stored
>> > new pg_proc.proleakproof field.
>>
>> The way you have this implemented, we just blow away all view options
>> whenever we do CREATE OR REPLACE VIEW.  Is that the behavior we want?
>> If a security_barrier view gets accidentally turned into a
>> non-security_barrier view, doesn't that create a security_hole?
>
> I think CREATE OR REPLACE needs to keep meaning just that, never becoming
> "replace some characteristics, merge others".  The consequence is less than
> delightful here, but I don't have an idea that avoids this problem without
> running afoul of some previously-raised design constraint.

Hmm, you might be right, although 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.

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

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

2011/9/24 Robert Haas <robertmhaas@gmail.com>:
> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> I updated the patches of fix-leaky-view problem, according to the
>> previous discussion.
>> The "NOLEAKY" option was replaced by "LEAKPROOF" option, and several regression
>> test cases were added. Rest of stuffs are unchanged.
>
> You have a leftover reference to NOLEAKY.
>
Oops, I'll fix it.

>> For convenience of reviewer, below is summary of these patches:
>>
>> The Part-1 implements corresponding SQL syntax stuffs which are
>> "security_barrier"
>> reloption of views, and "LEAKPROOF" option on creation of functions to be stored
>> new pg_proc.proleakproof field.
>
> The way you have this implemented, we just blow away all view options
> whenever we do CREATE OR REPLACE VIEW.  Is that the behavior we want?
> If a security_barrier view gets accidentally turned into a
> non-security_barrier view, doesn't that create a security_hole?
>
> I'm also wondering if the way you're using ResetViewOptions() is the
> right way to handle this anyhow.  Isn't that going to update pg_class
> twice?  I guess that's probably harmless from a performance
> standpoint, but wouldn't it be better not to?  I guess we could define
> something like AT_ReplaceRelOptions to handle this case.
>
IIRC, we had a discussion that the behavior should follow the case
when a view is newly defined, even if it would be replaced actually.
However, it seems to me consistent way to keep existing setting
as long as user does not provide new option explicitly.
If so, I think AT_ReplaceRelOptions enables to simplify the code
to implement such a behavior.

> The documentation in general is not nearly adequate, at least IMHO.
>
Do you think the description is poor to introduce the behavior changes
corresponding to security_barrier option?
OK, I'll try to update the documentation.

> I'm a bit nervous about storing security_barrier in the RTE.  What
> happens to stored rules if the security_barrier option gets change
> later?
>
The rte->security_barrier is evaluated when a query referencing security
views get expanded. So, rte->security_barrier is not stored to catalog.
Even if security_barrier option was changed after PREPARE statement
with references to security view, our existing mechanism re-evaluate
the query on EXECUTE, thus, it shall be executed as we expected.
(As an aside, I didn't know this mechanism and surprised at EXECUTE
works correctly, even if VIEW definition was changed after PREPARE.)

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Sun, Sep 25, 2011 at 3:25 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> I'm a bit nervous about storing security_barrier in the RTE.  What
>> happens to stored rules if the security_barrier option gets change
>> later?
>>
> The rte->security_barrier is evaluated when a query referencing security
> views get expanded. So, rte->security_barrier is not stored to catalog.

I think it is.  If you create a view that involves an RTE, the node
tree is going to get stored in pg_rewrite.ev_action.  And it's going
to include the security_barrier attribute, because you added outfuncs
support for it...

No?

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

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@leadboat.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@gmail.com>:
> On Sun, Sep 25, 2011 at 3:25 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>> I'm a bit nervous about storing security_barrier in the RTE.  What
>>> happens to stored rules if the security_barrier option gets change
>>> later?
>>>
>> The rte->security_barrier is evaluated when a query referencing security
>> views get expanded. So, rte->security_barrier is not stored to catalog.
>
> I think it is.  If you create a view that involves an RTE, the node
> tree is going to get stored in pg_rewrite.ev_action.  And it's going
> to include the security_barrier attribute, because you added outfuncs
> support for it...
>
> No?
>
IIUC, nested views are also expanded when user's query gets rewritten.
Thus, rte->security_barrier shall be set based on the latest configuration
of the view.
I injected an elog(NOTICE, ...) to confirm the behavior, when security_barrier
flag was set on rte->security_barrier at ApplyRetrieveRule().

postgres=# CREATE VIEW v1 WITH (security_barrier=true) AS SELECT *
FROM t1 WHERE a % 2 = 0;
CREATE VIEW
postgres=# CREATE VIEW v2 WITH (security_barrier=true) AS SELECT a + a
AS aa, b FROM v1;
CREATE VIEW

postgres=# SELECT * FROM v2;
NOTICE:  security barrier set on v1
NOTICE:  security barrier set on v2aa |  b
----+----- 4 | bbb
(1 row)

postgres=# ALTER TABLE v1 SET (security_barrier=false);
ALTER TABLE
postgres=# SELECT * FROM v2;
NOTICE:  security barrier set on v2aa |  b
----+----- 4 | bbb
(1 row)

postgres=# ALTER TABLE v1 SET (security_barrier=true);
ALTER TABLE
postgres=# SELECT * FROM v2;
NOTICE:  security barrier set on v1
NOTICE:  security barrier set on v2aa |  b
----+----- 4 | bbb
(1 row)

It seems to me the rte->security_barrier flag is correctly set, even
if underlying view's option was changed later.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

2011/9/26 Robert Haas <robertmhaas@gmail.com>:
> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah@leadboat.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@kaigai.gr.jp>

On Mon, Sep 26, 2011 at 6:28 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> 2011/9/26 Robert Haas <robertmhaas@gmail.com>:
>> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah@leadboat.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@gmail.com>:
> On Mon, Sep 26, 2011 at 6:28 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> 2011/9/26 Robert Haas <robertmhaas@gmail.com>:
>>> On Sun, Sep 25, 2011 at 10:38 PM, Noah Misch <noah@leadboat.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@kaigai.gr.jp>

On Mon, Sep 26, 2011 at 9:51 AM, Kohei KaiGai <kaigai@kaigai.gr.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 Mon, Sep 26, 2011 at 5:58 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> I think it is.  If you create a view that involves an RTE, the node
>> tree is going to get stored in pg_rewrite.ev_action.  And it's going
>> to include the security_barrier attribute, because you added outfuncs
>> support for it...
>>
>> No?
>>
> IIUC, nested views are also expanded when user's query gets rewritten.
> Thus, rte->security_barrier shall be set based on the latest configuration
> of the view.
> I injected an elog(NOTICE, ...) to confirm the behavior, when security_barrier
> flag was set on rte->security_barrier at ApplyRetrieveRule().

Hmm, OK.  I am still not convinced that this is the right approach.
Normally, we don't cache anything in the RangeTblEntry that might
change between plan time and execution time.  Those things are
normally stored in the RelOptInfo - why not do the same here?

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

2011/9/26 Robert Haas <robertmhaas@gmail.com>:
> On Mon, Sep 26, 2011 at 5:58 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>> I think it is.  If you create a view that involves an RTE, the node
>>> tree is going to get stored in pg_rewrite.ev_action.  And it's going
>>> to include the security_barrier attribute, because you added outfuncs
>>> support for it...
>>>
>>> No?
>>>
>> IIUC, nested views are also expanded when user's query gets rewritten.
>> Thus, rte->security_barrier shall be set based on the latest configuration
>> of the view.
>> I injected an elog(NOTICE, ...) to confirm the behavior, when security_barrier
>> flag was set on rte->security_barrier at ApplyRetrieveRule().
>
> Hmm, OK.  I am still not convinced that this is the right approach.
> Normally, we don't cache anything in the RangeTblEntry that might
> change between plan time and execution time.  Those things are
> normally stored in the RelOptInfo - why not do the same here?
>
The point is that a sub-query come from a particular view does not
keep the information what view originally stored the sub-query when
it was passed to the executor stage.
PostgreSQL handles a view as just a sub-query after the rewriter stage.

One possible idea not to store the flag in RangeTblEntry is to utilize
rte->relid to show the relation-id of the source view, when rtekind is
RTE_SUBQUERY; that enables to pull the security_barrier flag in
executor stage.
However, the interface to reference reloptions are designed to pull
this information with Relation pointer, rather than lsyscache, so
I implemented this revision with a new rte->security_barrier member.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
> However, the interface to reference reloptions are designed to pull
> this information with Relation pointer, rather than lsyscache, so
> I implemented this revision with a new rte->security_barrier member.

This approach will guarantee that we can never implement an ALTER VIEW
(or CREATE OR REPLACE VIEW) option that changes the state of the flag.
I don't think that's a good idea.
        regards, tom lane

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
> Sorry, are you saying the current (in other words, rte->security_barrier
> stores the state of reloption) approach is not a good idea?

Yes.  I think the same as Robert: the way to handle this is to store it
in RelOptInfo for the duration of planning, and pull it from the
catalogs during planner startup (cf plancat.c).
        regards, tom lane

2011/9/26 Tom Lane <tgl@sss.pgh.pa.us>:
> Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
>> However, the interface to reference reloptions are designed to pull
>> this information with Relation pointer, rather than lsyscache, so
>> I implemented this revision with a new rte->security_barrier member.
>
> This approach will guarantee that we can never implement an ALTER VIEW
> (or CREATE OR REPLACE VIEW) option that changes the state of the flag.
> I don't think that's a good idea.
>
Sorry, are you saying the current (in other words, rte->security_barrier
stores the state of reloption) approach is not a good idea?

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

2011/9/26 Tom Lane <tgl@sss.pgh.pa.us>:
> Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
>> Sorry, are you saying the current (in other words, rte->security_barrier
>> stores the state of reloption) approach is not a good idea?
>
> Yes.  I think the same as Robert: the way to handle this is to store it
> in RelOptInfo for the duration of planning, and pull it from the
> catalogs during planner startup (cf plancat.c).
>
Hmm. If so, it seems to me worthwhile to investigate an alternative
approach that stores relation-id of the view on rte->relid if rtekind is
RTE_SUBQUERY and pull the "security_barrier" flag from the catalog
during planner stage.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> The Part-2 tries to tackles a leaky-view scenarios by functions with
> very tiny cost
> estimation value. It was same one we had discussed in the commitfest-1st.
> It prevents to launch functions earlier than ones come from inside of views with
> "security_barrier" option.
>
> The Part-3 tries to tackles a leaky-view scenarios by functions that references
> one side of join loop. It prevents to distribute qualifiers including
> functions without
> "leakproof" attribute into relations across security-barrier.

I took a little more of a look at this today.  It has major problems.

First, I get compiler warnings (which you might want to trap in the
future by creating src/Makefile.custom with COPT=-Werror when
compiling).

Second, the regression tests fail on the select_views test.

Third, it appears that the part2 patch works by adding an additional
traversal of the entire query tree to standard_planner().  I don't
think we want to add overhead to the common case where no security
views are in use, or at least it had better be very small - so this
doesn't seem acceptable to me.

Here are some simple benchmarking with pgbench -S (scale factor 10,
shared_buffers=400MB, MacBook Pro laptop) with and without this stack
of patches.  These aren't clear-cut enough to make me absolutely sure
that this patch causes a noticeable performance regression, but I
think it does, and I'm not at all sure that this is the worst case:

results.kaigai.1:tps = 9359.908769 (including connections establishing)
results.kaigai.1:tps = 9366.317857 (including connections establishing)
results.kaigai.1:tps = 9413.593349 (including connections establishing)
results.master.1:tps = 9444.494510 (including connections establishing)
results.master.1:tps = 9400.486860 (including connections establishing)
results.master.1:tps = 9472.220529 (including connections establishing)

In the light of these problems, it doesn't seem worthwhile for me to
spend any more time on this right now: it looks to me like this needs
a lot more work before it can be considered for commit.  I will mark
it Waiting on Author for now, but I think Returned with Feedback might
be more appropriate.  This needs more than light cleanup; it needs
much more rigorous testing, both as to correctness and performance,
and at least a partial redesign.

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

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
> One possible idea not to store the flag in RangeTblEntry is to utilize
> rte->relid to show the relation-id of the source view, when rtekind is
> RTE_SUBQUERY; that enables to pull the security_barrier flag in
> executor stage.

Maybe I'm confused here, but what does the executor need the information
for?  I thought this was a planner problem.
        regards, tom lane

2011/9/26 Tom Lane <tgl@sss.pgh.pa.us>:
> Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
>> One possible idea not to store the flag in RangeTblEntry is to utilize
>> rte->relid to show the relation-id of the source view, when rtekind is
>> RTE_SUBQUERY; that enables to pull the security_barrier flag in
>> executor stage.
>
> Maybe I'm confused here, but what does the executor need the information
> for?  I thought this was a planner problem.
>
Sorry, "planner" was what I wanted to say.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

2011/9/26 Robert Haas <robertmhaas@gmail.com>:
> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> The Part-2 tries to tackles a leaky-view scenarios by functions with
>> very tiny cost
>> estimation value. It was same one we had discussed in the commitfest-1st.
>> It prevents to launch functions earlier than ones come from inside of views with
>> "security_barrier" option.
>>
>> The Part-3 tries to tackles a leaky-view scenarios by functions that references
>> one side of join loop. It prevents to distribute qualifiers including
>> functions without
>> "leakproof" attribute into relations across security-barrier.
>
> I took a little more of a look at this today.  It has major problems.
>
> First, I get compiler warnings (which you might want to trap in the
> future by creating src/Makefile.custom with COPT=-Werror when
> compiling).
>
> Second, the regression tests fail on the select_views test.
>
> Third, it appears that the part2 patch works by adding an additional
> traversal of the entire query tree to standard_planner().  I don't
> think we want to add overhead to the common case where no security
> views are in use, or at least it had better be very small - so this
> doesn't seem acceptable to me.
>
The reason why I put a walker routine on the head of standard_planner()
was that previous revision of this patch tracked strict depth of sub-queries,
not a number of times to go through security barrier.
The policy to count-up depth of qualifier was changed according to Noad's
suggestion is commit-fest 1st, however, the suitable position to mark the
depth value was kept.
I'll try to revise the suitable position to track the depth value. It seems to
me one candidate is pull_up_subqueries during its recursive call, because
this patch originally set FromExpr->security_barrier here.

In addition to the two points you mentioned above, I'll update this patch
as follows:
* Use CREATE [SECURITY] VIEW statement, instead of reloptions. the flag shall be stored within a new attribute of
pg_class,and it shall be kept when an existing view getting replaced. 

* Utilize RangeTblEntry->relid, instead of rte->security_barrier, and the flag shall be pulled from the catalog on
plannerstage. 

* Documentation and Regression test.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

I updated the patches according to the suggestion.
And most of documentation and regression test changes are consolidated
to the part-4 patch.

* CREATE SECURITY VIEW statement, instead of reloptions. (Part-1)

I added this new statement, instead of reloptions support on views.
The "security_barrier" information gets being stored on
pg_class.relissecbarrier field newly added.
However, in my opinion, the previous implementation was simpler from
the viewpoint of code,
because it allows to utilize existing facilities to support reloptions.
As long as we definitely inform users the reloptions are preserved
even if a view is replaced,
my preference is utilization of reloption...

* RangeTblEntry->security_barrier has gone.

The security_barrier field of RangeTblEntry was removed, and
rte->relid also gets utilized to track
what view was originally referenced instead of the sub-query.
The rte->relid enables to pull "security_barrier" flag from the
catalog, if and when a sub-query is
originally referenced as a view.

* Performance improvement

I removed mark_qualifiers_depth that walks on whole of the Query tree
from the head of
standard_planner. Instead of this, I put this routine at pull_up_subqueries().
This design change enabled to avoid unneeded tree-walking when
security view was not
appeared in the query, because Expr nodes are initialized by 0 on
makeNode(), so we
don't need to mark the "depth" field by zero.
The subquery_depth is incremented when we pull-up a sub-query that
come from security
view, and its initial value is 0. So, we don't need to mark depth
unless we pull up a sub-query
come from the security view.

+       /*
+        * Mark the sub-query depth of qualifiers to determine the original
+        * level of them, if necessary. Expr->depth is initialized to zero,
+        * so we don't need to walk on the expression tree, if security view
+        * was not used.
+        */
+       if (subquery_depth > 0)
+           mark_qualifiers_depth(f->quals, subquery_depth);

I have no idea why the patched version records better results,
although it might be within
the mergin of statistical errors. Could you reproduce it in your environment?
- Xeon E5504 (2.00GHz), shared_buffers=512, scaling factor=10
- Using pgbench -S -c 4 -T 15 postgres

* Git master + all the patches
tps = 6414.525599 (excluding connections establishing)
tps = 6422.960691 (excluding connections establishing)
tps = 6239.301706 (excluding connections establishing)
tps = 6406.008424 (excluding connections establishing)
tps = 6361.722286 (excluding connections establishing)

* Git master
tps = 6141.622043 (excluding connections establishing)
tps = 6243.385064 (excluding connections establishing)
tps = 6266.548213 (excluding connections establishing)
tps = 6020.004101 (excluding connections establishing)
tps = 6210.104070 (excluding connections establishing)

* Compiler warnning
I fixed them using Makefile.custom.

* Regression test
I noticed there are two select_view*.out files in regtest/expected directory,
and regress.diff compared results/select_view.out and
expected/select_view_1.out.
So, I copied my results/select_view.out to expected/select_view_1.out,
then we have no unexpected regression test errors.
However, it seems to me quite confusable.

Thanks,

2011/9/27 Kohei KaiGai <kaigai@kaigai.gr.jp>:
> 2011/9/26 Robert Haas <robertmhaas@gmail.com>:
>> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>> The Part-2 tries to tackles a leaky-view scenarios by functions with
>>> very tiny cost
>>> estimation value. It was same one we had discussed in the commitfest-1st.
>>> It prevents to launch functions earlier than ones come from inside of views with
>>> "security_barrier" option.
>>>
>>> The Part-3 tries to tackles a leaky-view scenarios by functions that references
>>> one side of join loop. It prevents to distribute qualifiers including
>>> functions without
>>> "leakproof" attribute into relations across security-barrier.
>>
>> I took a little more of a look at this today.  It has major problems.
>>
>> First, I get compiler warnings (which you might want to trap in the
>> future by creating src/Makefile.custom with COPT=-Werror when
>> compiling).
>>
>> Second, the regression tests fail on the select_views test.
>>
>> Third, it appears that the part2 patch works by adding an additional
>> traversal of the entire query tree to standard_planner().  I don't
>> think we want to add overhead to the common case where no security
>> views are in use, or at least it had better be very small - so this
>> doesn't seem acceptable to me.
>>
> The reason why I put a walker routine on the head of standard_planner()
> was that previous revision of this patch tracked strict depth of sub-queries,
> not a number of times to go through security barrier.
> The policy to count-up depth of qualifier was changed according to Noad's
> suggestion is commit-fest 1st, however, the suitable position to mark the
> depth value was kept.
> I'll try to revise the suitable position to track the depth value. It seems to
> me one candidate is pull_up_subqueries during its recursive call, because
> this patch originally set FromExpr->security_barrier here.
>
> In addition to the two points you mentioned above, I'll update this patch
> as follows:
> * Use CREATE [SECURITY] VIEW statement, instead of reloptions.
>  the flag shall be stored within a new attribute of pg_class, and it shall
>  be kept when an existing view getting replaced.
>
> * Utilize RangeTblEntry->relid, instead of rte->security_barrier, and the
>  flag shall be pulled from the catalog on planner stage.
>
> * Documentation and Regression test.
>
> Thanks,
> --
> KaiGai Kohei <kaigai@kaigai.gr.jp>
>



--
KaiGai Kohei <kaigai@kaigai.gr.jp>

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@leadboat.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@leadboat.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@leadboat.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@kaigai.gr.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@leadboat.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@kaigai.gr.jp>

On Sat, Oct 08, 2011 at 09:11:08AM +0200, Kohei KaiGai wrote:
> 2011/10/8 Noah Misch <noah@leadboat.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@leadboat.com>:
> On Sat, Oct 08, 2011 at 09:11:08AM +0200, Kohei KaiGai wrote:
>> 2011/10/8 Noah Misch <noah@leadboat.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@kaigai.gr.jp>

On Sun, Oct 9, 2011 at 11:50 AM, Kohei KaiGai <kaigai@kaigai.gr.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
becomesa 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.00rows=1 width=39) (actual
 
time=0.022..2.758 rows=1 loops=1)  Filter: (a = 1)  Rows Removed by Filter: 9999Total 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);
QUERYPLAN
 
-----------------------------------------------------------------------------------------------------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: 9999Total runtime:
126.565ms
 
(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
Scanon 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.00rows=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: 9999Total
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
Scanon 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: 9999Total
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@gmail.com>:
> On Sun, Oct 9, 2011 at 11:50 AM, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.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@kaigai.gr.jp> wrote:
> 2011/10/10 Robert Haas <robertmhaas@gmail.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@kaigai.gr.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);
QUERYPLAN 
------------------------------------------------------------------------------------Merge Join  (cost=381.80..9597.97
rows=586624width=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              ->
SeqScan 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.56rows=15510 width=44)        ->  Merge Join  (cost=210.95..489.78 rows=15510 width=44)
MergeCond: (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=1230width=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@gmail.com>:
> On Mon, Oct 10, 2011 at 4:28 PM, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai <kaigai@kaigai.gr.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@gmail.com> writes:
> On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai <kaigai@kaigai.gr.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@sss.pgh.pa.us>:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

On Wed, Oct 19, 2011 at 6:35 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> 2011/10/19 Tom Lane <tgl@sss.pgh.pa.us>:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai <kaigai@kaigai.gr.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@gmail.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@sss.pgh.pa.us>:
> Robert Haas <robertmhaas@gmail.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@kaigai.gr.jp>

On Fri, Oct 21, 2011 at 10:36 AM, Kohei KaiGai <kaigai@kaigai.gr.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@gmail.com>:
> On Fri, Oct 21, 2011 at 10:36 AM, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

On Wed, Nov 2, 2011 at 7:34 AM, Kohei KaiGai <kaigai@kaigai.gr.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@gmail.com>:
> On Wed, Nov 2, 2011 at 7:34 AM, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

Kohei KaiGai <kaigai@kaigai.gr.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@sss.pgh.pa.us>:
> Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.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@kaigai.gr.jp>:
> 2011/11/2 Tom Lane <tgl@sss.pgh.pa.us>:
>> Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>



--
KaiGai Kohei <kaigai@kaigai.gr.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@kaigai.gr.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@kaigai.gr.jp>:
>> 2011/11/2 Tom Lane <tgl@sss.pgh.pa.us>:
>>> Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>
>
>
>
> --
> KaiGai Kohei <kaigai@kaigai.gr.jp>



--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Mon, Sep 26, 2011 at 3:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
>> Sorry, are you saying the current (in other words, rte->security_barrier
>> stores the state of reloption) approach is not a good idea?
>
> Yes.  I think the same as Robert: the way to handle this is to store it
> in RelOptInfo for the duration of planning, and pull it from the
> catalogs during planner startup (cf plancat.c).

Having looked at this more, I'm starting to believe KaiGai has this
part right after all.  The trouble is that the rewriter does this:

    /*
     * Now, plug the view query in as a subselect, replacing the relation's
     * original RTE.
     */
    rte = rt_fetch(rt_index, parsetree->rtable);
    rte->rtekind = RTE_SUBQUERY;
    rte->relid = InvalidOid;
    rte->subquery = rule_action;
    rte->inh = false;           /* must not be set for a subquery */

In other words, by the time the planner comes along and tries to
decide whether or not it should flatten this subquery, the view has
already been rewritten into a subquery - and that subquery is in most
respects indistinguishable from a subquery that the user wrote
directly.  There is one difference: the permission check that would
have been done against the view gets attached to the OLD entry in the
subquery's range table.  It would probably be possible to make this
work by having the code paths that need to know whether or not a given
subquery originated from a security-barrier-enabled view do that same
trick: peek down into the OLD entry in the subquery rangetable,
extract the view OID from there, and go check its reloptions.  But
that seems awfully complicated and error-prone, hence my feeling that
just flagging the subquery explicitly is probably a better approach.

One other possibility that comes to mind is that, instead of adding
"bool security_view" to the RTE, we could instead add a new RTEKind,
something like RTE_SECURITY_VIEW.  That would mean going through and
finding all the places that refer to RTE_SUBQUERY and adjusting them
to handle RTE_SECURITY_VIEW in either the same way or differently as
may be appropriate.  The possible advantage of this approach is that
it doesn't bloat the RTE structure (and stored rules that use it) with
an additional attribute that (I think) will always be false - because
security_barrier can only be set on a subquery RTE after rewriting has
happened, and stored rules are haven't been rewritten yet.  It might
also force people to think a bit more carefully about how security
views should be handled during future code changes, which could also
be viewed as a plus.

I'm attaching my current version of KaiGai's patch (with substantial
cleanup of the comments and documentation, and some other changes) for
reference.

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

Robert Haas <robertmhaas@gmail.com> writes:
> Having looked at this more, I'm starting to believe KaiGai has this
> part right after all.

Yeah, you have a point.  The rewriter is intentionally trying to make an
expanded view look just the same as an in-line SELECT-in-FROM, and we
need it to be easier to distinguish them.

> One other possibility that comes to mind is that, instead of adding
> "bool security_view" to the RTE, we could instead add a new RTEKind,
> something like RTE_SECURITY_VIEW.  That would mean going through and
> finding all the places that refer to RTE_SUBQUERY and adjusting them
> to handle RTE_SECURITY_VIEW in either the same way or differently as
> may be appropriate.  The possible advantage of this approach is that
> it doesn't bloat the RTE structure (and stored rules that use it) with
> an additional attribute that (I think) will always be false - because
> security_barrier can only be set on a subquery RTE after rewriting has
> happened, and stored rules are haven't been rewritten yet.  It might
> also force people to think a bit more carefully about how security
> views should be handled during future code changes, which could also
> be viewed as a plus.

Hmm.  The question is whether the places where we need to care about
this would naturally be looking at RTEKind anyway.  If they are, or many
are, then I think this might be a good idea.  However if a lot of the
action is elsewhere then I don't know if we get much leverage from the
new RTEKind.  I haven't read the patch lately so can't opine on that.
        regards, tom lane

On Wed, Dec 7, 2011 at 1:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> One other possibility that comes to mind is that, instead of adding
>> "bool security_view" to the RTE, we could instead add a new RTEKind,
>> something like RTE_SECURITY_VIEW.  That would mean going through and
>> finding all the places that refer to RTE_SUBQUERY and adjusting them
>> to handle RTE_SECURITY_VIEW in either the same way or differently as
>> may be appropriate.  The possible advantage of this approach is that
>> it doesn't bloat the RTE structure (and stored rules that use it) with
>> an additional attribute that (I think) will always be false - because
>> security_barrier can only be set on a subquery RTE after rewriting has
>> happened, and stored rules are haven't been rewritten yet.  It might
>> also force people to think a bit more carefully about how security
>> views should be handled during future code changes, which could also
>> be viewed as a plus.
>
> Hmm.  The question is whether the places where we need to care about
> this would naturally be looking at RTEKind anyway.  If they are, or many
> are, then I think this might be a good idea.  However if a lot of the
> action is elsewhere then I don't know if we get much leverage from the
> new RTEKind.  I haven't read the patch lately so can't opine on that.

*reads through the code*

It looks to me like most places that look at RTE_SUBQUERY really have
no reason to care about this. So probably it's just as well to have a
separate flag for it.

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

On Sat, Dec 3, 2011 at 3:19 AM, Kohei KaiGai <kaigai@kaigai.gr.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@gmail.com>:
> On Sat, Dec 3, 2011 at 3:19 AM, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

On Thu, Dec 8, 2011 at 5:17 PM, Kohei KaiGai <kaigai@kaigai.gr.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@gmail.com>:
> On Thu, Dec 8, 2011 at 5:17 PM, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

2011/12/10 Kohei KaiGai <kaigai@kaigai.gr.jp>:
> 2011/12/9 Robert Haas <robertmhaas@gmail.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@kaigai.gr.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@kaigai.gr.jp>:
> 2011/12/10 Kohei KaiGai <kaigai@kaigai.gr.jp>:
>> 2011/12/9 Robert Haas <robertmhaas@gmail.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@kaigai.gr.jp>

--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Mon, Dec 12, 2011 at 12:00 PM, Kohei KaiGai <kaigai@kaigai.gr.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@gmail.com>:
> On Mon, Dec 12, 2011 at 12:00 PM, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai <kaigai@kaigai.gr.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@gmail.com>:
> On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

On Tue, Jan 3, 2012 at 12:11 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> 2011/12/23 Robert Haas <robertmhaas@gmail.com>:
>> On Fri, Dec 23, 2011 at 5:56 AM, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai <kaigai@kaigai.gr.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@gmail.com> wrote:
> On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai <kaigai@kaigai.gr.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.janes@gmail.com>:
> On Tue, Jan 17, 2012 at 7:08 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Sun, Jan 8, 2012 at 10:32 AM, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>

On Sun, Jan 22, 2012 at 5:57 AM, Kohei KaiGai <kaigai@kaigai.gr.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