Re: ExecutorCheckPerms() hook

(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>