ExecutorCheckPerms() hook
in [PgSql]
|
From: Stephen Frost on 25 May 2010 22:12 * KaiGai Kohei (kaigai(a)ak.jp.nec.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. 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. > > #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). > 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? > 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. > > #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(). > > #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. Thanks, Stephen
From: Stephen Frost on 25 May 2010 22:16 * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: > 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. That may be the case. I'm certainly more concerned with a bug in the existing code than any new code that we're working on. The question is- is this actually a user-visible bug? Or do we require that a user creating an FK needs SELECT rights on the primary table? If so, it's still a bug, but at that point it's a bug in our documentation where we don't mention that SELECT rights are also needed. Anyone know what the SQL spec says about this (if anything...)? > However, it is an independent issue right now, so I kept it as is. Uh, I don't really see it as independent.. If we have a bug there that we need to fix, and it's because we have two different bits of code trying to do the same checking, we should fix it be eliminating the duplicate checking, imv. > 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? That's certainly a nice thought, but given the complexity in ALTER TABLE, in particular with regard to permissions checking, I have no idea if what it's doing is intentional or wrong. Thanks, Stephen
From: KaiGai Kohei on 25 May 2010 23:04 (2010/05/26 11:12), Stephen Frost wrote: > * KaiGai Kohei (kaigai(a)ak.jp.nec.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(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: Tom Lane on 25 May 2010 23:17 Stephen Frost <sfrost(a)snowman.net> writes: > That may be the case. I'm certainly more concerned with a bug in the > existing code than any new code that we're working on. The question is- > is this actually a user-visible bug? Or do we require that a user > creating an FK needs SELECT rights on the primary table? If so, it's > still a bug, but at that point it's a bug in our documentation where we > don't mention that SELECT rights are also needed. Having an FK to another table certainly allows at least an indirect form of SELECT, because you can determine whether any given key exists in the PK table by seeing if you're allowed to insert a referencing row. I haven't dug in the SQL spec to see if that addresses the point, but it wouldn't bother me in the least to insist that both REFERENCES and SELECT privilege are required to create an FK. In any case, RI_Initial_Check isn't broken, because if it can't do the SELECTs it just falls back to a slower method. It's arguable that the FK triggers themselves are assuming more than they should about permissions, but I don't think that RI_Initial_Check can be claimed to be buggy. regards, tom lane -- 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 23:39
(2010/05/26 12:17), Tom Lane wrote: > Stephen Frost<sfrost(a)snowman.net> writes: >> That may be the case. I'm certainly more concerned with a bug in the >> existing code than any new code that we're working on. The question is- >> is this actually a user-visible bug? Or do we require that a user >> creating an FK needs SELECT rights on the primary table? If so, it's >> still a bug, but at that point it's a bug in our documentation where we >> don't mention that SELECT rights are also needed. > > Having an FK to another table certainly allows at least an indirect > form of SELECT, because you can determine whether any given key > exists in the PK table by seeing if you're allowed to insert a > referencing row. I haven't dug in the SQL spec to see if that addresses > the point, but it wouldn't bother me in the least to insist that > both REFERENCES and SELECT privilege are required to create an FK. > > In any case, RI_Initial_Check isn't broken, because if it can't do > the SELECTs it just falls back to a slower method. It's arguable > that the FK triggers themselves are assuming more than they should > about permissions, but I don't think that RI_Initial_Check can be > claimed to be buggy. Hmm. If both REFERENCES and SELECT privilege are required to create a new FK constraint, why RI_Initial_Check() need to check SELECT permission prior to SPI_execute()? It eventually checks SELECT privilege during execution of the secondary query. It is unclear for me why we need to provide a slower fallback. 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 |