Re: ExecutorCheckPerms() hook
От | KaiGai Kohei |
---|---|
Тема | Re: ExecutorCheckPerms() hook |
Дата | |
Msg-id | 4BFCC4D0.5090301@ak.jp.nec.com обсуждение исходный текст |
Ответ на | Re: ExecutorCheckPerms() hook (KaiGai Kohei <kaigai@ak.jp.nec.com>) |
Ответы |
Re: ExecutorCheckPerms() hook
(Stephen Frost <sfrost@snowman.net>)
|
Список | pgsql-hackers |
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@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@ak.jp.nec.com>
Вложения
В списке pgsql-hackers по дате отправления: