Horton
Menu

Thread

  1. pgsql: Add a hook in ExecCheckRTPerms().

    Robert Haas <rhaas@postgresql.org> — 2010-07-09T14:06:01Z

    Log Message:
-----------
Add a hook in ExecCheckRTPerms().

This hook allows a loadable module to gain control when table permissions
are checked.  It is expected to be used by an eventual SE-PostgreSQL
implementation, but there are other possible applications as well.  A
sample contrib module can be found in the archives at:

http://archives.postgresql.org/pgsql-hackers/2010-05/msg01095.php

Robert Haas and Stephen Frost

Modified Files:
--------------
    pgsql/src/backend/executor:
        execMain.c (r1.349 -> r1.350)
        (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/executor/execMain.c?r1=1.349&r2=1.350)
    pgsql/src/include/executor:
        executor.h (r1.168 -> r1.169)
        (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/executor/executor.h?r1=1.168&r2=1.169)

  2. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Simon Riggs <simon@2ndquadrant.com> — 2010-07-09T14:51:27Z

    On Fri, 2010-07-09 at 14:06 +0000, Robert Haas wrote:
> Log Message:
> -----------
> Add a hook in ExecCheckRTPerms().
> 
> This hook allows a loadable module to gain control when table permissions
> are checked.  It is expected to be used by an eventual SE-PostgreSQL
> implementation, but there are other possible applications as well.  A
> sample contrib module can be found in the archives at:
> 
> http://archives.postgresql.org/pgsql-hackers/2010-05/msg01095.php
> 

The loadable module doesn't "gain control" here it simplify kicks-in
after, and in addition to, normal checking. That just means you have the
option of failing for additional reasons.

We're not passing in any form of context other than the rangetable so
what additional reasons could there be? This is of no use to anything
that uses object labelling. We're not even at the part of the executor
where we would be able to identify objects yet, so I can't see what
value this brings. Though I am certainly in favour in general terms of
simple changes to enhance security configuration features.

Strangely, I was looking into removing the ExecCheckRTPerms check
altogether by forcing plan invalidation when permissions are updated.
That would be a performance tweak that would render this change useless.

-- 
 Simon Riggs           www.2ndQuadrant.com

  3. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Robert Haas <robertmhaas@gmail.com> — 2010-07-09T15:07:30Z

    On Fri, Jul 9, 2010 at 10:51 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> The loadable module doesn't "gain control" here it simplify kicks-in
> after, and in addition to, normal checking. That just means you have the
> option of failing for additional reasons.

True.  We could change it so that the normal checking is bypassed if
the hook is installed, and leave it up to the hook whether to call the
standard checks as well, but I don't think there's much of a use case
for that.

> We're not passing in any form of context other than the rangetable so
> what additional reasons could there be? This is of no use to anything
> that uses object labelling. We're not even at the part of the executor
> where we would be able to identify objects yet, so I can't see what
> value this brings. Though I am certainly in favour in general terms of
> simple changes to enhance security configuration features.

Well, KaiGai Kohei already posted a proof-of-concept patch showing how
this could be used by a simple SE-PostgreSQL implementation.  Since we
don't have a security labelling facility yet, he used the comment on
the relation to store the security label (there are other ways it
could be done too, of course).

> Strangely, I was looking into removing the ExecCheckRTPerms check
> altogether by forcing plan invalidation when permissions are updated.
> That would be a performance tweak that would render this change useless.

Huh.  Obviously, I would have refrained from committing the patch had
I known that it was going to conflict with work someone else was doing
in this area, at least until we reached consensus on which way to go
with it, but since you didn't post about it on -hackers, I had no idea
that was the case.  Sounds like you should probably post your proposal
and we can discuss what to do in general and also with respect to this
hook.

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

  4. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Stephen Frost <sfrost@snowman.net> — 2010-07-09T15:09:43Z

    * Simon Riggs (simon@2ndQuadrant.com) wrote:
> The loadable module doesn't "gain control" here it simplify kicks-in
> after, and in addition to, normal checking. That just means you have the
> option of failing for additional reasons.

Right, additional checks (such as the label) can be done.

> We're not passing in any form of context other than the rangetable so
> what additional reasons could there be? This is of no use to anything
> that uses object labelling. We're not even at the part of the executor
> where we would be able to identify objects yet, so I can't see what
> value this brings. 

I'm a bit confused by this.  By this point, we've fully planned out the
query, looked up info about all the objects involved, and the module can
now go look up any other information about them that it needs.  It can
also use info like what the current user is and information about the
connection.

There was actually a proof-of-concept module created by KaiGai to do all
of this with SELinux using the existing COMMENT tables, I'm pretty sure
we would have heard a bit earlier if it was useless.

> Strangely, I was looking into removing the ExecCheckRTPerms check
> altogether by forcing plan invalidation when permissions are updated.
> That would be a performance tweak that would render this change useless.

I don't see how you could remove ExecCheckRTPerms..?  It's what handles
all permissions checking for DML (like, making sure you have SELECT
rights on the table you're trying to query).  I could see forcing plan
invalidation when permissions are updated, sure, but that doesn't mean
you can stop doing them altogether anywhere.  Where would you move the
permissions checking to?  Wherever it is, this hook would just need to
follow.  

I don't know that you could (or that I'd be comfortable with) move the
permissions checking to the planner and then rely on plan invalidation
on permission changes, but if you did, just make sure the hook is
included in the decision about allowing the query.

	Thanks,

		Stephen

  5. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-07-09T15:09:50Z

    Simon Riggs <simon@2ndQuadrant.com> writes:
> Strangely, I was looking into removing the ExecCheckRTPerms check
> altogether by forcing plan invalidation when permissions are updated.
> That would be a performance tweak that would render this change useless.

That seems both pointless and wrong.  Permissions checks should happen
at execution time not plan time.

			regards, tom lane

  6. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Simon Riggs <simon@2ndquadrant.com> — 2010-07-09T16:36:28Z

    On Fri, 2010-07-09 at 11:07 -0400, Robert Haas wrote:

> > Strangely, I was looking into removing the ExecCheckRTPerms check
> > altogether by forcing plan invalidation when permissions are updated.
> > That would be a performance tweak that would render this change useless.
> 
> Huh.  Obviously, I would have refrained from committing the patch had
> I known that it was going to conflict with work someone else was doing
> in this area, at least until we reached consensus on which way to go
> with it, but since you didn't post about it on -hackers, I had no idea
> that was the case.  Sounds like you should probably post your proposal
> and we can discuss what to do in general and also with respect to this
> hook.

Sorry, yes, you couldn't possibly know I was looking at that. I just
meant it was strange those things overlapped.

-- 
 Simon Riggs           www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and Services

  7. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Simon Riggs <simon@2ndquadrant.com> — 2010-07-09T16:51:48Z

    On Fri, 2010-07-09 at 11:09 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > Strangely, I was looking into removing the ExecCheckRTPerms check
> > altogether by forcing plan invalidation when permissions are updated.
> > That would be a performance tweak that would render this change useless.
> 
> That seems both pointless and wrong.  Permissions checks should happen
> at execution time not plan time.

Agreed that permission checks should logically be applied at execution
time. I am proposing a performance optimisation, not a change in
behaviour.

Permissions are set much less frequently than plans and connections, so
when the only permission check is at table level it makes more sense to
optimistically assume that permission checks will never change for a
plan and cache the result of the permission check. That way we need only
check permissions once at plan time rather than checking them every
single execution.

The only extra code to do this would be to invalidate plans when
permissions change for a table. That doesn't seem hard or invasive.

The proposed performance enhancement would be very useful since we have
to check permissions of functions, views, tables and every other aspect.
We could spend a while quantifying that overhead, though "non-zero" is
all we need to know.

-- 
 Simon Riggs           www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and Services

  8. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Simon Riggs <simon@2ndquadrant.com> — 2010-07-09T17:07:52Z

    On Fri, 2010-07-09 at 11:07 -0400, Robert Haas wrote:
> On Fri, Jul 9, 2010 at 10:51 AM, Simon Riggs <simon@2ndquadrant.com>
> wrote:
> > The loadable module doesn't "gain control" here it simplify kicks-in
> > after, and in addition to, normal checking. That just means you have
> the
> > option of failing for additional reasons.
> 
> True.  We could change it so that the normal checking is bypassed if
> the hook is installed, and leave it up to the hook whether to call the
> standard checks as well, but I don't think there's much of a use case
> for that.

With respect, there doesn't seem to be much use case anyway. I'm sorry
to be expressing that opinion now; been away for a while. I am somewhat
amazed that Tom isn't dancing on your head for proposing it though.

> > We're not passing in any form of context other than the rangetable
> so
> > what additional reasons could there be? This is of no use to
> anything
> > that uses object labelling. We're not even at the part of the
> executor
> > where we would be able to identify objects yet, so I can't see what
> > value this brings. Though I am certainly in favour in general terms
> of
> > simple changes to enhance security configuration features.
> 
> Well, KaiGai Kohei already posted a proof-of-concept patch showing how
> this could be used by a simple SE-PostgreSQL implementation.  Since we
> don't have a security labelling facility yet, he used the comment on
> the relation to store the security label (there are other ways it
> could be done too, of course).

What's the difference between that and a GRANT command? GRANT is
designed to allow privileges to be defined at table level. I don't see
how a plugin whose only API input is a rangetable and which executes
before any tuples have been touched can possibly add value here.

KaiGai's had an uphill task here and I don't wish to be part of slowing
him down. I'm not seeing how this moves label security forwards in any
measurable way.

Tom's test of a useful plugin has been one where a useful contrib module
gets added at the same time. I don't think a useful plugin has been
demonstrated or produced, as yet.

-- 
 Simon Riggs           www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and Services

  9. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Simon Riggs <simon@2ndquadrant.com> — 2010-07-09T17:12:12Z

    On Fri, 2010-07-09 at 11:09 -0400, Stephen Frost wrote:
> > Strangely, I was looking into removing the ExecCheckRTPerms check
> > altogether by forcing plan invalidation when permissions are
> updated.
> > That would be a performance tweak that would render this change
> useless.
> 
> I don't see how you could remove ExecCheckRTPerms..?  It's what
> handles
> all permissions checking for DML (like, making sure you have SELECT
> rights on the table you're trying to query).  I could see forcing plan
> invalidation when permissions are updated, sure, but that doesn't mean
> you can stop doing them altogether anywhere.  Where would you move the
> permissions checking to?  

I apologise, when I said removing the check altogether, I meant removing
from the executor path.

-- 
 Simon Riggs           www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and Services

  10. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Stephen Frost <sfrost@snowman.net> — 2010-07-09T17:19:31Z

    Simon,

* Simon Riggs (simon@2ndQuadrant.com) wrote:
> With respect, there doesn't seem to be much use case anyway. I'm sorry
> to be expressing that opinion now; been away for a while. I am somewhat
> amazed that Tom isn't dancing on your head for proposing it though.

I believe it's because we've been through it with him already and
explained how and why it's useful.

> > Well, KaiGai Kohei already posted a proof-of-concept patch showing how
> > this could be used by a simple SE-PostgreSQL implementation.  Since we
> > don't have a security labelling facility yet, he used the comment on
> > the relation to store the security label (there are other ways it
> > could be done too, of course).
> 
> What's the difference between that and a GRANT command? GRANT is
> designed to allow privileges to be defined at table level. I don't see
> how a plugin whose only API input is a rangetable and which executes
> before any tuples have been touched can possibly add value here.

The *hook*'s API is just a range-table- the plugin has access to a huge
amount of additional information.  I don't think it makes sense to try
to limit that in some fashion by dictating what other information the
hook is allowed to gather.  Even if we did, it wouldn't be everything
the hook needs since we're not going to collect the SE-Linux label from
the kernel in the main backend code.

The difference between this and a GRANT command would be the whole DAC
vs MAC discussion and overall label-based security (this is *not* the
same as *row-level* security).

> KaiGai's had an uphill task here and I don't wish to be part of slowing
> him down. I'm not seeing how this moves label security forwards in any
> measurable way.

I'm afraid we're talking about different things here.

> Tom's test of a useful plugin has been one where a useful contrib module
> gets added at the same time. I don't think a useful plugin has been
> demonstrated or produced, as yet.

We have a plugin which *could* be used to allow SE-Linux in the backend
today- but it uses the COMMENT system, which isn't exactly ideal.

Robert's already working on a patch which will add the ability to track
actual labels in the catalog (using some new catalog tables which are
similar to the comment tables, which is why it's not done yet, it's
waiting on the get_xxx_oid() infrastructure which will greatly simplify
both), which could then be queried by the module.  There will be a hook
there as well, which will get called whenever someone tries to modify or
add a label to an object.  We've also discussed new syntax for
supporting those catalogs (ALTER SECURITY LABEL ON TABLE x TO y, or
something like that, it's in the archives).

So, no, it's not done today, but I'm certainly hopeful this will all get
into 9.1 and will allow label-based security for DML with SELinux (and
maybe Smack too) and this is a necessary step along the way.

	Thanks,

		Stephen

  11. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-07-09T17:21:54Z

    Simon Riggs <simon@2ndQuadrant.com> writes:
> On Fri, 2010-07-09 at 11:09 -0400, Tom Lane wrote:
>> Simon Riggs <simon@2ndQuadrant.com> writes:
>>> Strangely, I was looking into removing the ExecCheckRTPerms check
>>> altogether by forcing plan invalidation when permissions are updated.
>>> That would be a performance tweak that would render this change useless.
>> 
>> That seems both pointless and wrong.  Permissions checks should happen
>> at execution time not plan time.

> Agreed that permission checks should logically be applied at execution
> time. I am proposing a performance optimisation, not a change in
> behaviour.

Except that it *is* a change in behavior: the first check will occur too
soon.

The fact that we're interested in adding plugin permissions checking
pretty much destroys the idea anyway.  You cannot assume that a plan
cache invalidation will happen for any change in external state that
a plugin might be consulting.

> The proposed performance enhancement would be very useful since we have
> to check permissions of functions, views, tables and every other aspect.
> We could spend a while quantifying that overhead, though "non-zero" is
> all we need to know.

No, it's not all we need to know.  If you can't prove the overhead
involved here is significant, we should not be expending effort and
creating subtle behavioral changes in pursuit of a minor optimization.

			regards, tom lane

  12. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Stephen Frost <sfrost@snowman.net> — 2010-07-09T17:28:26Z

    * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > Agreed that permission checks should logically be applied at execution
> > time. I am proposing a performance optimisation, not a change in
> > behaviour.
> 
> Except that it *is* a change in behavior: the first check will occur too
> soon.

Yeah, I have to say that I don't see any way you could avoid the
behaviour change from doing this.  Specifically, you can prepare plans
today that you don't have access to run and then run them later after
you've been granted the permission.

I'm not saying that's a huge use-case or anything, but moving the checks
to planner time would definitely change the behavior.  No clue if any of
this is covered in the SQL spec.

> The fact that we're interested in adding plugin permissions checking
> pretty much destroys the idea anyway.  You cannot assume that a plan
> cache invalidation will happen for any change in external state that
> a plugin might be consulting.

Yeah, this would certainly be a problem too, unless we kept the plugin
hook in the executor and only used the "optimization" for the stock PG
checks.

> > The proposed performance enhancement would be very useful since we have
> > to check permissions of functions, views, tables and every other aspect.
> > We could spend a while quantifying that overhead, though "non-zero" is
> > all we need to know.
> 
> No, it's not all we need to know.  If you can't prove the overhead
> involved here is significant, we should not be expending effort and
> creating subtle behavioral changes in pursuit of a minor optimization.

Agreed.

	Thanks,

		Stephen

  13. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Robert Haas <robertmhaas@gmail.com> — 2010-07-09T17:33:26Z

    On Fri, Jul 9, 2010 at 1:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> On Fri, 2010-07-09 at 11:09 -0400, Tom Lane wrote:
>>> Simon Riggs <simon@2ndQuadrant.com> writes:
>>>> Strangely, I was looking into removing the ExecCheckRTPerms check
>>>> altogether by forcing plan invalidation when permissions are updated.
>>>> That would be a performance tweak that would render this change useless.
>>>
>>> That seems both pointless and wrong.  Permissions checks should happen
>>> at execution time not plan time.
>
>> Agreed that permission checks should logically be applied at execution
>> time. I am proposing a performance optimisation, not a change in
>> behaviour.
>
> Except that it *is* a change in behavior: the first check will occur too
> soon.

You might be able to get around this by doing the first check on first
use of the plan and then going and marking all the plans as needing a
recheck whenever a permissions change happens.  Whether the
performance savings are sufficient to justify such a thing is another
matter.

> The fact that we're interested in adding plugin permissions checking
> pretty much destroys the idea anyway.  You cannot assume that a plan
> cache invalidation will happen for any change in external state that
> a plugin might be consulting.

This is certainly true, but I also wonder what SE-PostgreSQL plans to
do about this.  Taking this to its logical exteme, the system security
policy could change in mid-query - and while you'd like to think that
the system would stop emitting tuples on a dime, that's probably not
too feasible in practice.  I am assuming that SE-PostgreSQL will want
to do some kind of caching, but I wonder how one decides what to cache
and for how long, and whether there's any mechanism for propagating
cache invalidations.

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

  14. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Simon Riggs <simon@2ndquadrant.com> — 2010-07-09T17:38:52Z

    On Fri, 2010-07-09 at 13:21 -0400, Tom Lane wrote:
> > Agreed that permission checks should logically be applied at
> execution
> > time. I am proposing a performance optimisation, not a change in
> > behaviour.
> 
> Except that it *is* a change in behavior: the first check will occur
> too
> soon.

Sooner matters why? We already have a lock on the table at plan time so
there cannot be a concurrent GRANT against a plan-then-execute
transaction. Later transactions would invalidate and replan.

> The fact that we're interested in adding plugin permissions checking
> pretty much destroys the idea anyway.  You cannot assume that a plan
> cache invalidation will happen for any change in external state that
> a plugin might be consulting.

Plugin can still be executed at appropriate time, its mostly absent and
so cheap. I guess we can keep plugin whatever else I attempt.

> > The proposed performance enhancement would be very useful since we
> have
> > to check permissions of functions, views, tables and every other
> aspect.
> > We could spend a while quantifying that overhead, though "non-zero"
> is
> > all we need to know.
> 
> No, it's not all we need to know.  If you can't prove the overhead
> involved here is significant, we should not be expending effort and
> creating subtle behavioral changes in pursuit of a minor optimization.

OK, will gather evidence.

-- 
 Simon Riggs           www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and Services

  15. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Stephen Frost <sfrost@snowman.net> — 2010-07-09T17:44:26Z

    Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> This is certainly true, but I also wonder what SE-PostgreSQL plans to
> do about this.  Taking this to its logical exteme, the system security
> policy could change in mid-query - and while you'd like to think that
> the system would stop emitting tuples on a dime, that's probably not
> too feasible in practice.  I am assuming that SE-PostgreSQL will want
> to do some kind of caching, but I wonder how one decides what to cache
> and for how long, and whether there's any mechanism for propagating
> cache invalidations.

Yes, SE-PG will be doing cacheing and this exact problem has already
been addressed (KaiGai's original SE-PG patches included cacheing,
actually).  It's also not something that's unique to PG in any way.

	Thanks,

		Stephen

  16. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-07-09T18:01:11Z

    Simon Riggs <simon@2ndQuadrant.com> writes:
> On Fri, 2010-07-09 at 13:21 -0400, Tom Lane wrote:
>> Except that it *is* a change in behavior: the first check will occur
>> too soon.

> Sooner matters why?

Consider PREPARE followed only later by EXECUTE.  Your proposal would
make the PREPARE fail outright, when it currently does not.

			regards, tom lane

  17. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Simon Riggs <simon@2ndquadrant.com> — 2010-07-09T18:17:36Z

    On Fri, 2010-07-09 at 14:01 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > On Fri, 2010-07-09 at 13:21 -0400, Tom Lane wrote:
> >> Except that it *is* a change in behavior: the first check will occur
> >> too soon.
> 
> > Sooner matters why?
> 
> Consider PREPARE followed only later by EXECUTE.  Your proposal would
> make the PREPARE fail outright, when it currently does not.

Just to avoid wasted investigation: are you saying that is important
behaviour that is essential we retain in PostgreSQL, or will you hear
evidence that supporting that leads to a performance decrease elsewhere?

-- 
 Simon Riggs           www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and Services

  18. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-07-09T21:21:54Z

    Simon Riggs <simon@2ndQuadrant.com> writes:
> On Fri, 2010-07-09 at 14:01 -0400, Tom Lane wrote:
>> Consider PREPARE followed only later by EXECUTE.  Your proposal would
>> make the PREPARE fail outright, when it currently does not.

> Just to avoid wasted investigation: are you saying that is important
> behaviour that is essential we retain in PostgreSQL, or will you hear
> evidence that supporting that leads to a performance decrease elsewhere?

Well, I think that that problem makes moving the checks into the planner
a nonstarter.  But as somebody pointed out upthread, you could still get
what you want by keeping a flag saying "permission checks have been
done" so the executor could skip the checks on executions after the
first.  I'd still want to see some evidence showing that it's worth
troubling over though.  Premature optimization being the root of all
evil, and all that.  (In this case, the hazard we expose ourselves to
seems to be security holes due to missed resets of the flag.)

			regards, tom lane

  19. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Simon Riggs <simon@2ndquadrant.com> — 2010-07-11T07:58:18Z

    On Fri, 2010-07-09 at 17:21 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > On Fri, 2010-07-09 at 14:01 -0400, Tom Lane wrote:
> >> Consider PREPARE followed only later by EXECUTE.  Your proposal would
> >> make the PREPARE fail outright, when it currently does not.
> 
> > Just to avoid wasted investigation: are you saying that is important
> > behaviour that is essential we retain in PostgreSQL, or will you hear
> > evidence that supporting that leads to a performance decrease elsewhere?
> 
> Well, I think that that problem makes moving the checks into the planner
> a nonstarter.  But as somebody pointed out upthread, you could still get
> what you want by keeping a flag saying "permission checks have been
> done" so the executor could skip the checks on executions after the
> first.  

Seems like best idea.

> I'd still want to see some evidence showing that it's worth
> troubling over though.  Premature optimization being the root of all
> evil, and all that.  (In this case, the hazard we expose ourselves to
> seems to be security holes due to missed resets of the flag.)

If we did this it would be to add one line to the code 

	if (!perms_ok)

That doesn't seem to fall into the category of evil optimization to me.
I've seen you recode other parts of the executor stating reasons like
"shave another few cycles off the main path" and that seems the case
here. We shouldn't need to debate the consequences of Amhdahls law each
time.


Attached is a script to allow pgbench to be used to measure difference
between superuser and a typical privilege model for the pgbench tables.

-- 
 Simon Riggs           www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and Services

  20. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-07-11T15:44:55Z

    Simon Riggs <simon@2ndQuadrant.com> writes:
> On Fri, 2010-07-09 at 17:21 -0400, Tom Lane wrote:
>> I'd still want to see some evidence showing that it's worth
>> troubling over though.  Premature optimization being the root of all
>> evil, and all that.  (In this case, the hazard we expose ourselves to
>> seems to be security holes due to missed resets of the flag.)

> If we did this it would be to add one line to the code 
> 	if (!perms_ok)

> That doesn't seem to fall into the category of evil optimization to me.

The problem I foresee is not in the testing of the flag, it's in the
setting/resetting of it.  It's a reliability penalty not a performance
penalty --- and any mistakes would count as security issues.

Now it may be that you can offer a convincing argument that no such
mistake/oversight is likely.  But you haven't even tried to make that
case.  Even if you can show that the risk is small, it's not going to
be zero, so we have to trade it off against a demonstrated performance
improvement.

			regards, tom lane

  21. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Robert Haas <robertmhaas@gmail.com> — 2010-07-11T16:11:07Z

    On Jul 11, 2010, at 10:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> On Fri, 2010-07-09 at 17:21 -0400, Tom Lane wrote:
>>> I'd still want to see some evidence showing that it's worth
>>> troubling over though.  Premature optimization being the root of all
>>> evil, and all that.  (In this case, the hazard we expose ourselves to
>>> seems to be security holes due to missed resets of the flag.)
> 
>> If we did this it would be to add one line to the code 
>>    if (!perms_ok)
> 
>> That doesn't seem to fall into the category of evil optimization to me.
> 
> The problem I foresee is not in the testing of the flag, it's in the
> setting/resetting of it.  It's a reliability penalty not a performance
> penalty --- and any mistakes would count as security issues.
> 
> Now it may be that you can offer a convincing argument that no such
> mistake/oversight is likely.  But you haven't even tried to make that
> case.  Even if you can show that the risk is small, it's not going to
> be zero, so we have to trade it off against a demonstrated performance
> improvement.

There's no point in going back and forth here until we have a patch and the results of some performance testing using said patch. If Simon writes one and submits it with some results, we'll consider it on the merits. I think that's all Simon is asking for, and I don't think anyone is seriously arguing anything to the contrary. Like Tom, I'm skeptical that there is much performance to be found here, but if I'm wrong, I'm happy to have someone demonstrate it.

...Robert

  22. Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-07-12T06:45:30Z

    (2010/07/10 2:12), Simon Riggs wrote:
> On Fri, 2010-07-09 at 11:09 -0400, Stephen Frost wrote:
>>> Strangely, I was looking into removing the ExecCheckRTPerms check
>>> altogether by forcing plan invalidation when permissions are
>> updated.
>>> That would be a performance tweak that would render this change
>> useless.
>>
>> I don't see how you could remove ExecCheckRTPerms..?  It's what
>> handles
>> all permissions checking for DML (like, making sure you have SELECT
>> rights on the table you're trying to query).  I could see forcing plan
>> invalidation when permissions are updated, sure, but that doesn't mean
>> you can stop doing them altogether anywhere.  Where would you move the
>> permissions checking to?
> 
> I apologise, when I said removing the check altogether, I meant removing
> from the executor path.
> 
The Linux kernel has a facility that notify userspace applications when
the security policy is changed. This message is delivered using netlink
socket from the kernel. Once we received it, then SE-PG's access control
decision cache will be invalidated. Even if it was invalidated in mid-query,
please note that the older policy was valid when we make access control
decision.

If and when we have cached plans being already permission checked,
I'd like the core PG to ask external securities whether it is still
valid from the perspective of external security policy. If already
invalid, we can check permissions again on the cached plan.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>