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 00:34:38 |
Message-ID: | 4BFC6C9E.3050505@ak.jp.nec.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
(2010/05/25 21:44), Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> OK, the attached patch reworks it according to the way.
>
> Reviewing this patch, there are a whole slew of problems.
>
> #1: REALLY BIG ISSUE- Insufficient comment updates. You've changed
> function definitions in a pretty serious way as well as moved some code
> around such that some of the previous comments don't make sense. You
> have got to update comments when you're writing a patch. Indeed, the
> places I see a changes in comments are when you've removed what appears
> to still be valid and appropriate comments, or places where you've added
> comments which are just blatently wrong with the submitted patch.
Hmm. I'll revise/add the comment around the patched code.
> #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?
> #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?)
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.
It means the ExecCheckRTPerms() is caller of acl functions, not acl
function itself, isn't it?
In other words, I wonder we should categorize a function X which calls
A and (optionally) B as a part of A.
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 .
> #4: As mentioned previously, the hook (which should be added in a
> separate patch anyway) makes more sense to me to be in
> ExecCheckRTPerms(), not ExecCheckRTEPerms(). This also means that we
> need to be calling ExecCheckRTPerms() from DoCopy and
> RI_Initial_Check(), to make sure that the hook gets called. To that
> end, I wouldn't even expose ExecCheckRTEPerms() outside of acl.c. Also,
> there should be a big comment about not using or calling
> ExecCheckRTEPerms() directly outside of ExecCheckRTPerms() since the
> hook would then be skipped.
I don't have any differences in preference between ExecCheckRTPerms()
and ExecCheckRTEPerms(), except for DoCopy() and RI_Initial_Check()
have to call the checker function with list_make1(&rte), instead of &rte.
> #5: In DoCopy, you can remove relPerms and remainingPerms, but I'd
> probably leave required_access up near the top and then just use it to
> set rte->required_access directly rather than moving that bit deep down
> into the function.
OK,
> #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?
> #7: I'd move the conditional if (is_from) into the foreach which is
> building the columnsSet and eliminate the need for columnsSet; I don't
> see that it's really adding much here.
OK,
> #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?
Thanks,
--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
From | Date | Subject | |
---|---|---|---|
Next Message | KaiGai Kohei | 2010-05-26 00:52:13 | Re: ExecutorCheckPerms() hook |
Previous Message | Jan Urbański | 2010-05-25 23:16:55 | Re: tsvector pg_stats seems quite a bit off. |