Prev: [COMMITTERS] pgsql: Stamp HEAD as 9.1devel.
Next: [HACKERS] FYI: Ubuntu 10.04 lucid strange segfault
From: Simon Riggs on 9 Jul 2010 10:51
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


--
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: Robert Haas on 9 Jul 2010 11:07
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.

> 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

--
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 11:09
* 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.

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
From: Tom Lane on 9 Jul 2010 11:09
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.

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

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


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

 |  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