Skip site navigation (1) Skip section navigation (2)

Re: ExecutorCheckPerms() hook

From: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: ExecutorCheckPerms() hook
Date: 2010-05-26 06:50:56
Message-ID: 4BFCC4D0.5090301@ak.jp.nec.com (view raw or flat)
Thread:
Lists: 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(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>

Attachment: dml_reworks_kaigai.3.patch
Description: text/x-patch (18.3 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Takahiro ItagakiDate: 2010-05-26 07:32:56
Subject: Re: fillfactor gets set to zero for toast tables
Previous:From: Simon RiggsDate: 2010-05-26 06:33:10
Subject: Re: Synchronization levels in SR

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group