Prev: [COMMITTERS] pgsql: Stamp HEAD as 9.1devel.
Next: [HACKERS] FYI: Ubuntu 10.04 lucid strange segfault
From: Simon Riggs on 9 Jul 2010 12:51
On Fri, 2010-07-09 at 11:09 -0400, Tom Lane wrote:
> Simon Riggs <simon(a)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


--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Simon Riggs on 9 Jul 2010 13:07
On Fri, 2010-07-09 at 11:07 -0400, Robert Haas wrote:
> On Fri, Jul 9, 2010 at 10:51 AM, Simon Riggs <simon(a)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


--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Simon Riggs on 9 Jul 2010 13:12
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


--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Stephen Frost on 9 Jul 2010 13:19
Simon,

* Simon Riggs (simon(a)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
From: Tom Lane on 9 Jul 2010 13:21
Simon Riggs <simon(a)2ndQuadrant.com> writes:
> On Fri, 2010-07-09 at 11:09 -0400, Tom Lane wrote:
>> Simon Riggs <simon(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5
Prev: [COMMITTERS] pgsql: Stamp HEAD as 9.1devel.
Next: [HACKERS] FYI: Ubuntu 10.04 lucid strange segfault