ExecutorCheckPerms() hook
in [PgSql]
|
From: Stephen Frost on 25 May 2010 06:28 KaiGai, * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: > OK, the attached patch reworks it according to the way. I havn't looked at it yet, but the hook was added to ExecCheckRTPerms(), not RTE. This was for two main reasons- it seemed simpler to us and it meant that any security module implemented would have access to essentially everything we know the query is going to use all at once (instead of on a per-range-table basis). That could be particularly useful if you wanted to, say, enforce a constraint that says "no two tables of different labels shall ever be used in the same query at the same time" (perhaps with some caveats on that, etc). Could you change this patch to use ExecCheckRTPerms() instead? > * ExecCheckRTEPerms() becomes to take 2nd argument the caller to suggest > behavior on access violation. The 'abort' argument is true, it raises > an error using aclcheck_error() or ereport(). Otherwise, it returns > false immediately without rest of checks. > > * DoCopy() and RI_Initial_Check() were reworked to call ExecCheckRTEPerms() > with locally built RangeTblEntry. Does this change fix the issue you had in RI_Initial_Check()? Thanks, Stephen
From: Stephen Frost on 25 May 2010 08:44 KaiGai, * KaiGai Kohei (kaigai(a)ak.jp.nec.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. #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. #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. #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. #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. #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(). #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. #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. Thanks, Stephen
From: Stephen Frost on 25 May 2010 09:59 * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: > * DoCopy() and RI_Initial_Check() were reworked to call ExecCheckRTEPerms() > with locally built RangeTblEntry. Maybe I missed it somewhere, but we still need to address the case where the user doesn't have those SELECT permissions that we're looking for in RI_Initial_Check(), right? KaiGai, your patch should be addressing that in a similar fashion.. Thanks, Stephen
From: KaiGai Kohei on 25 May 2010 20:34 (2010/05/25 21:44), Stephen Frost wrote: > KaiGai, > > * KaiGai Kohei (kaigai(a)ak.jp.nec.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(a)ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: KaiGai Kohei on 25 May 2010 20:52
(2010/05/25 22:59), Stephen Frost wrote: > * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: >> * DoCopy() and RI_Initial_Check() were reworked to call ExecCheckRTEPerms() >> with locally built RangeTblEntry. > > Maybe I missed it somewhere, but we still need to address the case where > the user doesn't have those SELECT permissions that we're looking for in > RI_Initial_Check(), right? KaiGai, your patch should be addressing that > in a similar fashion.. The reason why user must have SELECT privileges on the PK/FK tables is the validateForeignKeyConstraint() entirely calls SPI_execute() to verify FK constraints can be established between two tables (even if fallback path). And, the reason why RI_Initial_Check() now calls pg_class_aclcheck() is to try to avoid unexpected access violation error because of SPI_execute(). However, the fallback path also calls SPI_execute() entirely, so I concluded the permission checks in RI_Initial_Check() is nonsense. However, it is an independent issue right now, so I kept it as is. The origin of the matter is that we applies unnecessary permission checks, although it is purely internal use and user was already checked to execute whole of ALTER TABLE statement. Right? Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |