Re: ExecutorCheckPerms() hook

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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>