ExecutorCheckPerms() hook
in [PgSql]
|
From: KaiGai Kohei on 26 May 2010 21:45 Stephen, thanks for comments. The attached three patches are the revised and divided ones. A: add makeRangeTblEntry() B: major reworks of DML permission checks C: add an ESP hook on the DML permission checks (2010/05/27 0:09), Stephen Frost wrote: > KaiGai, > > * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: >> The attached patch is a revised one for DML permission checks. > > This is certainly alot better. > >> ToDo: >> - makeRangeTblEntry() stuff to allocate a RTE node with given parameter >> is not yet. > > I'd certainly like to see the above done, or to understand why it can't > be if that turns out to be the case. The patch-A tries to implement makeRangeTblEntry() which takes only rtekind as argument right now. Other fields are initialized to zero, using makeNode(). > A couple of other comments, all pretty minor things: > > - I'd still rather see the hook itself in another patch, but given that > we've determined that none of this is going to go into 9.0, it's not > as big a deal. OK, I divided the ESP hook part into the patch-C. > - The hook definition in aclchk.c should really be at the top of that > file. We've been pretty consistant about putting hooks at the top of > files instead of deep down in the file, this should also follow that > scheme. OK, I moved it. > - Some of the comments at the top of chkpriv_rte_perms probably make > sense to move up to where it's called from execMain.c. Specifically, > the comments about the other RTE types (function, join, subquery). > I'd probably change the comment in chkpriv_rte_perms to be simpler- > "This is only used for checking plain relation permissions, nothing > else is checked here", and also have that same comment around > chkpriv_relation_perms, both in aclchk.c and in acl.h. OK, I edited the comment as follows: | /* | * Do permissions checks. The check_relation_privileges() checks access | * permissions for all relations listed in a range table, but does not | * check anything for other RTE types (function, join, subquery, ...). | * Function RTEs are checked by init_fcache when the function is prepared | * for execution. Join, subquery, and special RTEs need no checks. | */ > - I'd move chkpriv_relation_perms above chkpriv_rte_perms, it's what we > expect people to use, after all. OK, I reordered it. > - Don't particularly like the function names. How about > relation_privilege_check? Or rangetbl_privilege_check? We don't use > 'perms' much (uh, at all?) in function names, and even if we did, it'd > be redundant and not really help someone understand what the function > is doing. IIRC, Robert suggested that a verb should lead the function name. So, I renamed it into check_relation_privileges() and check_rte_privileges(). > - I don't really like having 'abort' as the variable name for the 2nd > argument. I'm not finding an obvious convention right now, but maybe > something like "error_on_failure" instead? The 'failure' may make an impression of generic errors not only permission denied. How about 'error_on_violation'? > - In DoCopy, some comments about what you're doing there to set up for > calling chkpriv_relation_perms would be good (like the comment you > removed- /* We don't have table permissions, check per-column > permissions */, updated to for something like "build an RTE with the > columns referenced marked to check for necessary privileges"). > Additionally, it might be worth considering if having an RTE built > farther up in DoCopy would make sense and would then be usable for > other bits in DoCopy. I edited the comments as follows: | /* | * Check relation permissions. | * We built an RTE with the relation and columns to be accessed | * to check for necessary privileges in the common way. | */ > - In RI_Initial_Check, why not build up an actual list of RTEs and just > call chkpriv_relation_perms once? Also, you should add comments > there, again, about what you're doing and why. If you can use another > function to build the actual RTE, this will probably fall out more > sensibly too. Good catch! I fixed the invocation of checker function with list_make2(). And, I edited the comments as follows: | /* | * We built a pair of RTEs of FK/PK relations and columns referenced | * in the test query to check necessary permission in the common way. | */ > - Have you checked if there are any bad side-effects from calling > ri_FetchConstraintInfo before doing the permissions checking? The ri_FetchConstraintInfo() only references SysCaches to set up given local variable without any other locks except for ones acquired by syscache.c. > - The hook in acl.h should be separated out and brought to the top and > documented independently as to exactly where the hook is and what it > can be used for, along with what the arguments mean, etc. Similairly, > chkpriv_relation_perms should really have a short comment for it about > what it's for. Something more than 'security checker function'. OK, at the patch-C, I moved the definition of the hook into the first half of acl.h, but it needs to be declared after the AclResult definition. BTW, I wonder whether acl.h is a correct place to explain about the hook, although I added comments for the hook. I think we should add a series of explanation about ESP hooks in the internal section of the documentation, when the number of hooks reaches a dozen for example. Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com>
From: Stephen Frost on 26 May 2010 22:15 KaiGai, * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: > Stephen, thanks for comments. > > The attached three patches are the revised and divided ones. > > A: add makeRangeTblEntry() Ok, didn't actually expect that. Guess my suggestion would have been to just use makeNode() since there wasn't anything more appropriate already. Still, I don't really have a problem with your makeRangeTblEntry() and you certainly found quite a few places to use it. > B: major reworks of DML permission checks No serious issues with this that I saw either. I definitely think it's cleaner using makeRangeTblEntry() and check_relation_privileges(). > C: add an ESP hook on the DML permission checks This also looks good to me, though I don't know that you really need the additional comment in execMain.c about the hook. I would make sure that you have a comment around check_rte_privileges() which says not to call it directly because you'll bypass the hook (and potentially cause a security leak by doing so). Don't recall seeing that, apologies if it was there. > IIRC, Robert suggested that a verb should lead the function name. > So, I renamed it into check_relation_privileges() and check_rte_privileges(). Yeah, that's alright. I'm on the fence about using 'relation' or using 'rangetbl' there, but certainly whomever commits this could trivially change it to whatever they prefer. > The 'failure' may make an impression of generic errors not only permission denied. > How about 'error_on_violation'? Maybe 'ereport_on_violation'? I dunno, guess one isn't really better than the other. You need to go back and fix the comment though- you still say 'abort' there. > > - Have you checked if there are any bad side-effects from calling > > ri_FetchConstraintInfo before doing the permissions checking? > > The ri_FetchConstraintInfo() only references SysCaches to set up given > local variable without any other locks except for ones acquired by syscache.c. Ok. > > - The hook in acl.h should be separated out and brought to the top and > > documented independently as to exactly where the hook is and what it > > can be used for, along with what the arguments mean, etc. Similairly, > > chkpriv_relation_perms should really have a short comment for it about > > what it's for. Something more than 'security checker function'. > > OK, at the patch-C, I moved the definition of the hook into the first half > of acl.h, but it needs to be declared after the AclResult definition. Fair enough. > BTW, I wonder whether acl.h is a correct place to explain about the hook, > although I added comments for the hook. Guess I don't really see a problem putting the comments there. By the way, have we got a place where we actually document the hooks we support somewhere in the official documentation..? If so, that should certainly be updated too.. > I think we should add a series of explanation about ESP hooks in the internal > section of the documentation, when the number of hooks reaches a dozen for > example. I believe the goal will be to avoid reaching a dozen hooks for this. All-in-all, I'm pretty happy with these. Couple minor places which could use some copy editing, but that's about it. Next, we need to get the security label catalog and the grammar to support it implemented and then from that an SELinux module should be pretty easy to implement. Based on the discussions at PGCon, Robert is working on the security label catalog and grammar. The current plan is to have a catalog similar to pg_depend, to minimize impact to the rest of the backend and to those who aren't interested in using security labels. Of course, there will also need to be hooks there for an external module to enforce restrictions associated with changing labels on various objects in the system. Thanks, Stephen
From: KaiGai Kohei on 26 May 2010 23:00 Stephen, >> The 'failure' may make an impression of generic errors not only permission denied. >> How about 'error_on_violation'? > > Maybe 'ereport_on_violation'? I dunno, guess one isn't really better > than the other. You need to go back and fix the comment though- you > still say 'abort' there. I have no preference between 'error_on_violation' and 'ereport_on_violation'. OK, I fixed it. >> BTW, I wonder whether acl.h is a correct place to explain about the hook, >> although I added comments for the hook. > > Guess I don't really see a problem putting the comments there. By the > way, have we got a place where we actually document the hooks we support > somewhere in the official documentation..? If so, that should certainly > be updated too.. I could not find Executor hooks from doc/src/sgml using grep. If so, it might be worth to list them on the wikipage. >> I think we should add a series of explanation about ESP hooks in the internal >> section of the documentation, when the number of hooks reaches a dozen for >> example. > > I believe the goal will be to avoid reaching a dozen hooks for this. Maybe, all we need to hook on DML permissions is only this one. > All-in-all, I'm pretty happy with these. Couple minor places which > could use some copy editing, but that's about it. > > Next, we need to get the security label catalog and the grammar to > support it implemented and then from that an SELinux module should > be pretty easy to implement. Based on the discussions at PGCon, Robert > is working on the security label catalog and grammar. The current plan > is to have a catalog similar to pg_depend, to minimize impact to the > rest of the backend and to those who aren't interested in using security > labels. Pg_depend? not pg_description/pg_shdescription? I basically agree with the idea that minimizes damages to the existing schema of system catalogs, but I cannot imagine something like pg_depend well. I'd like to post a new thread to discuss the security label support. OK? > Of course, there will also need to be hooks there for an > external module to enforce restrictions associated with changing labels > on various objects in the system. Yes, the user given has to be validated by ESP. Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com>
From: KaiGai Kohei on 14 Jun 2010 04:27 I attached three patches for the effort. Each patch tries to tackle one theme, so it is not unreasonable. But the ESP security hook patch (quite tiny) depends on the DML permission refactoring patch (relatively larger). So, Robert suggested me to reconsider the dependency of these patches. The attached patch shall be applied on the head of the git repository. It just adds a security hook on ExecCheckRTPerms() as Robert suggested at first. Of course, it does not allow to acquire the control on COPY TO/FROM and RI_Initial_Check(). It will be refactored in the following patches. Thanks, (2010/05/27 12:00), KaiGai Kohei wrote: > Stephen, > >>> The 'failure' may make an impression of generic errors not only permission denied. >>> How about 'error_on_violation'? >> >> Maybe 'ereport_on_violation'? I dunno, guess one isn't really better >> than the other. You need to go back and fix the comment though- you >> still say 'abort' there. > > I have no preference between 'error_on_violation' and 'ereport_on_violation'. > OK, I fixed it. > >>> BTW, I wonder whether acl.h is a correct place to explain about the hook, >>> although I added comments for the hook. >> >> Guess I don't really see a problem putting the comments there. By the >> way, have we got a place where we actually document the hooks we support >> somewhere in the official documentation..? If so, that should certainly >> be updated too.. > > I could not find Executor hooks from doc/src/sgml using grep. > If so, it might be worth to list them on the wikipage. > >>> I think we should add a series of explanation about ESP hooks in the internal >>> section of the documentation, when the number of hooks reaches a dozen for >>> example. >> >> I believe the goal will be to avoid reaching a dozen hooks for this. > > Maybe, all we need to hook on DML permissions is only this one. > >> All-in-all, I'm pretty happy with these. Couple minor places which >> could use some copy editing, but that's about it. >> >> Next, we need to get the security label catalog and the grammar to >> support it implemented and then from that an SELinux module should >> be pretty easy to implement. Based on the discussions at PGCon, Robert >> is working on the security label catalog and grammar. The current plan >> is to have a catalog similar to pg_depend, to minimize impact to the >> rest of the backend and to those who aren't interested in using security >> labels. > > Pg_depend? not pg_description/pg_shdescription? > > I basically agree with the idea that minimizes damages to the existing schema > of system catalogs, but I cannot imagine something like pg_depend well. > > I'd like to post a new thread to discuss the security label support. OK? > >> Of course, there will also need to be hooks there for an >> external module to enforce restrictions associated with changing labels >> on various objects in the system. > > Yes, the user given has to be validated by ESP. > > Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com>
From: Robert Haas on 14 Jun 2010 07:54
2010/6/14 KaiGai Kohei <kaigai(a)ak.jp.nec.com>: > I attached three patches for the effort. > Each patch tries to tackle one theme, so it is not unreasonable. > > But the ESP security hook patch (quite tiny) depends on the DML permission > refactoring patch (relatively larger). So, Robert suggested me to reconsider > the dependency of these patches. > > The attached patch shall be applied on the head of the git repository. > It just adds a security hook on ExecCheckRTPerms() as Robert suggested > at first. > Of course, it does not allow to acquire the control on COPY TO/FROM and > RI_Initial_Check(). It will be refactored in the following patches. This is essentially the same patch that I wrote and posted several weeks ago, with changes to the comments and renaming of the identifiers. Are you trying to represent it as your own work? With all due respect, I intend to imply my own version. Please make your other proposed patches apply on top of that. -- 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 |