Thread: Re: [COMMITTERS] pgsql: Add a hook in ExecCheckRTPerms().
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
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
* 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
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
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.comPostgreSQL Development, 24x7 Support, Training and Services
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.comPostgreSQL Development, 24x7 Support, Training and Services
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.comPostgreSQL Development, 24x7 Support, Training and Services
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.comPostgreSQL Development, 24x7 Support, Training and Services
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
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
* 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
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
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.comPostgreSQL Development, 24x7 Support, Training and Services
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
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
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.comPostgreSQL Development, 24x7 Support, Training and Services
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
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
Attachment
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
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 saidpatch. If Simon writes one and submits it with some results, we'll consider it on the merits. I think that's all Simonis asking for, and I don't think anyone is seriously arguing anything to the contrary. Like Tom, I'm skeptical thatthere is much performance to be found here, but if I'm wrong, I'm happy to have someone demonstrate it. ...Robert
(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>