ExecutorCheckPerms() hook
in [PgSql]
|
From: KaiGai Kohei on 26 May 2010 02:50 The attached patch is a revised one for DML permission checks. List of updates: - Source code comments in the patched functions were revised. - ExecCheckRTPerms() and ExecCheckRTEPerms() were moved to aclchk.c, and renamed to chkpriv_relation_perms() and chkpriv_rte_perms(). - It took the 2nd argument (bool abort) that is a hint of behavior on access violations. - It also returns AclResult, instead of bool. - I assumed RI_Initial_Check() is not broken, right now. So, this patch just reworks DML permission checks without any bugfixes. - The ESP hook were moved to ExecCheckRTPerms() from ExecCheckRTEPerms(). - At DoCopy() and RI_Initial_Check() call the checker function with list_make1(&rte), instead of &rte. - In DoCopy(), required_access is used to store either ACL_SELECT or ACL_INSERT; initialized at head of the function. - In DoCopy(), it initialize selectedCols or modifiedCol of RTE depending on if (is_from), instead of columnsSet. ToDo: - makeRangeTblEntry() stuff to allocate a RTE node with given parameter is not yet. Thanks, (2010/05/26 12:04), KaiGai Kohei wrote: > (2010/05/26 11:12), Stephen Frost wrote: >> * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: >>>> #2: REALLY BIG ISSUE- You've added ExecutorCheckPerms_hook as part of >>>> this patch- don't, we're in feature-freeze right now and should not be >>>> adding hooks at this time. >>> >>> The patch is intended to submit for the v9.1 development, not v9.0, isn't it? >> >> That really depends on if this is actually fixing a bug in the existing >> code or not. I'm on the fence about that at the moment, to be honest. >> I was trying to find if we expliitly say that SELECT rights are needed >> to reference a column but wasn't able to. If every code path is >> expecting that, then perhaps we should just document it that way and >> move on. In that case, all these changes would be for 9.1. If we >> decide the current behavior is a bug, it might be something which could >> be fixed in 9.0 and maybe back-patched. > > Ahh, because I found out an independent problem during the discussion, > it made us confused. Please make clear this patch does not intend to > fix the bug. > > If we decide it is an actual bug to be fixed/informed, I also agree > it should be worked in a separated patch. > > Well, rest of discussion should be haven in different thread. > >> In *either* case, given that one is a 'clean-up' patch and the other is >> 'new functionality', they should be independent *anyway*. Small >> incremental changes that don't break things when applied is what we're >> shooting for here. > > Agreed. > >>>> #3: You didn't move ExecCheckRTPerms() and ExecCheckRTEPerms() to >>>> utils/acl and instead added executor/executor.h to rt_triggers.c. >>>> I don't particularly like that. I admit that DoCopy() already knew >>>> about the executor, and if that were the only case outside of the >>>> executor where ExecCheckRTPerms() was getting called it'd probably be >>>> alright, but we already have another place that wants to use it, so >>>> let's move it to a more appropriate place. >>> >>> Sorry, I'm a bit confused. >>> It seemed to me you suggested to utilize ExecCheckRTPerms() rather than >>> moving its logic anywhere, so I kept it here. (Was it misunderstand?) >> >> I'm talking about moving the whole function (all 3 lines of it) to >> somewhere else and then reworking the function to be more appropriate >> based on it's new location (including renaming and changing arguments >> and return values, as appropriate). > > OK, I agreed. > >>> If so, but, I doubt utils/acl is the best placeholder of the moved >>> ExecCheckRTPerms(), because the checker function calls both of the >>> default acl functions and a optional external security function. >> >> Can you explain why you think that having a function in utils/acl (eg: >> include/utils/acl.h and backend/utils/aclchk.c) which calls default acl >> functions and an allows for an external hook would be a bad idea? >> >>> It means the ExecCheckRTPerms() is caller of acl functions, not acl >>> function itself, isn't it? >> >> It's providing a higher-level service, sure, but there's nothing >> particularly interesting or special about what it's doing in this case, >> and, we need it in multiple places. Why duplicate it? > > If number of the checker functions is only a reason why we move > ExecCheckRTPerms() into the backend/utils/aclchk.c right now, I > don't have any opposition. > When it reaches to a dozen, we can consider new location. Right? > > Sorry, the name of pg_rangetbl_aclcheck() was misleading for me. > >>> I agreed the checker function is not a part of executor, but it is >>> also not a part of acl functions in my opinion. >>> >>> If it is disinclined to create a new directory to deploy the checker >>> function, my preference is src/backend/utils/adt/security.c and >>> src/include/utils/security.h . >> >> We don't need a new directory or file for one function, as Robert >> already pointed out. > > OK, let's consider when aclchk.c holds a dozen of checker functions. > >>>> #6: I havn't checked yet, but if there are other things in an RTE which >>>> would make sense in the DoCopy case, beyond just what's needed for the >>>> permissions checking, and which wouldn't be 'correct' with a NULL'd >>>> value, I would set those. Yes, we're building the RTE to check >>>> permissions, but we don't want someone downstream to be suprised when >>>> they make a change to something in the permissions checking and discover >>>> that a value in RTE they expected to be there wasn't valid. Even more >>>> so, if there are function helpers which can be used to build an RTE, we >>>> should be using them. The same goes for RI_Initial_Check(). >>> >>> Are you saying something like makeFuncExpr()? >>> I basically agree. However, should it be done in this patch? >> >> Actually, I mean looking for, and using, things like >> markRTEForSelectPriv() and addRangeTableEntry() or >> addRangeTableEntryForRelation(). > > OK, I'll make it in separated patch. > >>>> #8: When moving ExecCheckRTPerms(), you should rename it to be more like >>>> the other function calls in acl.h Perhaps pg_rangetbl_aclcheck()? >>>> Also, it should return an actual AclResult instead of just true/false. >>> >>> See the comments in #3. >>> And, if the caller has to handle aclcheck_error(), user cannot distinguish >>> access violation errors between the default PG permission and any other >>> external security stuff, isn't it? >> >> I'm not suggesting that the caller handle aclcheck_error().. >> ExecCheckRTPerms() could just as easily have a flag which indicates if >> it will call aclcheck_error() or not, and if not, to return an >> AclResult to the caller. That flag could then be passed to >> ExecCheckRTEPerms() as you have it now. > > Sorry, the name of pg_rangetbl_aclcheck() is also misleading for me. > It makes me an impression that it always returns AclResult and caller handles > it appropriately, like pg_class_aclcheck(). > > What you explained seems to me same as what I plan now. > > Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com>
From: Stephen Frost on 26 May 2010 06:54 Tom, * Tom Lane (tgl(a)sss.pgh.pa.us) wrote: > I haven't dug in the SQL spec to see if that addresses > the point, but it wouldn't bother me in the least to insist that > both REFERENCES and SELECT privilege are required to create an FK. Ok. If we require REFERENCES and SELECT privs to create an FK then I think the question is: when is the path in RI_Initial_Check not able to be used (hence, why do we need a fall-back)? My guess would be: role X has: Primary table: SELECT, REFERENCES Foreign table: REFERENCES This doesn't make much sense either though, because X has to own the foreign table. postgres=> alter table fk_tbl add foreign key (x) references pk_tbl; ERROR: must be owner of relation fk_tbl So, the only situation, it seems, where the fall-back method has to be used is when X owns the table but doesn't have SELECT rights on it. Maybe it's just me, but that seems pretty silly. If we require: Primary table: SELECT, REFERENCES Foreign table: OWNER, SELECT, REFERENCES Then it seems like we should be able to eliminate the fall-back method and just use the RI_Initial_Check approach. What am I missing here? :/ > In any case, RI_Initial_Check isn't broken, because if it can't do > the SELECTs it just falls back to a slower method. It's arguable > that the FK triggers themselves are assuming more than they should > about permissions, but I don't think that RI_Initial_Check can be > claimed to be buggy. RI_Initial_Check is at least missing an optimization to support column-level priviledges. If we say that REFERENCES alone is allowed to create a FK, then the fall-back method is broken because it depends on SELECT rights on the primary. To be honest, I'm guessing that the reason there's so much confusion around this is that the spec probably says you don't need SELECT rights (I havn't checked though), and at some point in the distant past we handled that correctly with the fall-back method and that has since been broken by other changes (possibly to plug the hole the fall-back method was using). I'll try to go deipher the spec so we can at least have something more interesting to discuss (if we agree with doing it how the spec says or not :). Thanks, Stephen
From: Tom Lane on 26 May 2010 09:50 KaiGai Kohei <kaigai(a)ak.jp.nec.com> writes: > Hmm. If both REFERENCES and SELECT privilege are required to create > a new FK constraint, why RI_Initial_Check() need to check SELECT > permission prior to SPI_execute()? > It eventually checks SELECT privilege during execution of the secondary > query. It is unclear for me why we need to provide a slower fallback. Because the queries inside the triggers are done with a different current userid. 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: Stephen Frost on 26 May 2010 11:09 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. 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. - 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. - 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. - I'd move chkpriv_relation_perms above chkpriv_rte_perms, it's what we expect people to use, after all. - 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. - 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? - 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. - 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. - Have you checked if there are any bad side-effects from calling ri_FetchConstraintInfo before doing the permissions checking? - 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'. All pretty minor things that I'd probably just fix myself if I was going to be committing it (not that I have that option ;). Thanks, Stephen
From: Stephen Frost on 26 May 2010 11:10
* Tom Lane (tgl(a)sss.pgh.pa.us) wrote: > Because the queries inside the triggers are done with a different > current userid. Indeed, I figured that out eventually too. Sorry it took so long. :/ Thanks, Stephen |