Re: [HACKERS] ExecutorCheckPerms() hook
(2010/07/22 10:04), Stephen Frost wrote: > * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: >> We can find out a similar case in CreateTrigger(). >> If I was granted TRIGGER privilege on a certain table, I can create a new >> trigger on the table without its ownership. More commonly, it allows us >> to modify a certain property of the table without its ownership. > > TRIGGER is hardly the same as REFERENCES. If we invented a new priv, it > would be more like 'FK_CREATE'. > >> Perhaps, if SQL permission would be more fine-grained, for example, >> "RENAME" permission might control RENAME TO statement, rather than >> its ownership. > > This wouldn't actually be any more fine-grained, it'd just be adding > rights on to an existing priv, which I think is a wholly *bad* idea. > >> What is the reason why we check its ownership here, although we already >> have REFERENCES permission to control ADD FOREIGN KEY? > > REFERENCES is needed on the REFERENCED table, ownership is needed on the > REFERENCING table. They're not the same.. > > We only allow owners of objects to change the structure of those > objects. Adding a FK to another table doesn't really change the > structure of the referenced table. Adding a FK does though, imv. > However, existing ATAddForeignKeyConstraint() checks REFERENCES permission on both of the referencing and referenced table/columns. Is it unexpected behavior??? It is an agreeable interpretation that we need ownership on the referencing table because creating a new FK equals to change a certain property of the referencing table. If so, why REFERENCES permissions are necessary on the referencing side, not only referenced side? Thanks, -- KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
* Robert Haas (robertmh...@gmail.com) wrote: > On Wed, Jul 21, 2010 at 9:02 PM, Stephen Frost wrote: > > Errr, no. If I grant you REFERENCES on my table, it means you can > > create a FK to it from some other table. > > Well, in that case, we should fix the fine documentation: > >To create a foreign key constraint, it is >necessary to have this privilege on both the referencing and >referenced columns. The privilege may be granted for all columns >of a table, or just specific columns. Technically that's true.. You just *also* have to own the referencing table. :) I agree though, if my claims are correct (which I'd like to think they are, but perusing the SQL spec just now didn't make it as abundently clear as I would have hoped...), and it's how PG acts today anyway, we should definitely fix the docs. Also, we do document that to use ALTER TABLE you have to own the table you're calling ALTER TABLE on, and obviously if you're calling CREATE TABLE you're "owner" of the object.. Have we got another way to add a FK to an existing table? If so, we should make sure they're all consistant in any case. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
On Wed, Jul 21, 2010 at 9:02 PM, Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: >> I think the relevant case might be where ymj owns fk_tbl but not >> pk_tbl, and has REFERENCES but not SELECT on pk_tbl. >> >> Come to think of it, I wonder if REFERENCES on fk_tbl ought to be >> sufficient to create a foreign key. Currently, it requires ownership: >> >> rhaas=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); >> ERROR: must be owner of relation fk_tbl > > Errr, no. If I grant you REFERENCES on my table, it means you can > create a FK to it from some other table. Well, in that case, we should fix the fine documentation: To create a foreign key constraint, it is necessary to have this privilege on both the referencing and referenced columns. The privilege may be granted for all columns of a table, or just specific columns. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote: > We can find out a similar case in CreateTrigger(). > If I was granted TRIGGER privilege on a certain table, I can create a new > trigger on the table without its ownership. More commonly, it allows us > to modify a certain property of the table without its ownership. TRIGGER is hardly the same as REFERENCES. If we invented a new priv, it would be more like 'FK_CREATE'. > Perhaps, if SQL permission would be more fine-grained, for example, > "RENAME" permission might control RENAME TO statement, rather than > its ownership. This wouldn't actually be any more fine-grained, it'd just be adding rights on to an existing priv, which I think is a wholly *bad* idea. > What is the reason why we check its ownership here, although we already > have REFERENCES permission to control ADD FOREIGN KEY? REFERENCES is needed on the REFERENCED table, ownership is needed on the REFERENCING table. They're not the same.. We only allow owners of objects to change the structure of those objects. Adding a FK to another table doesn't really change the structure of the referenced table. Adding a FK does though, imv. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
* Robert Haas (robertmh...@gmail.com) wrote: > I think the relevant case might be where ymj owns fk_tbl but not > pk_tbl, and has REFERENCES but not SELECT on pk_tbl. > > Come to think of it, I wonder if REFERENCES on fk_tbl ought to be > sufficient to create a foreign key. Currently, it requires ownership: > > rhaas=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); > ERROR: must be owner of relation fk_tbl Errr, no. If I grant you REFERENCES on my table, it means you can create a FK to it from some other table. That's very different from saying you can create a FK *on* my table. Put another way- you can prevent me from deleting data in my table if you have a FK to it, but you can prevent me from *inserting* data into my table if you can create a FK on it. Those are two distinct and different things and I definitely don't believe we should have 1 permission be used for both. Also, REFERENCES is in the spec, and I don't believe you could interprete it to letting people create FKs on tables they have REFERENCES on, afaik. I don't believe it's how other RDBMS' are either, but I have to admit to not having tested yet. Let's not add things to an SQL-defined priviledge or we'll end up seriously suprising people coming from standard-conforming databases, and in a security way. All that being said, having more fine-grained control over what can be done through an ALTER TABLE command is a neat idea, but it's also a pretty huge can of worms. I'd rather spend time figuring out the somewhat smaller set of things which are superuser only right now, and creating a way to have just non-superuser roles which can do those things (where it makes sense, anyway). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
(2010/07/22 8:45), Robert Haas wrote: > 2010/5/24 KaiGai Kohei: >> (2010/05/24 22:18), Robert Haas wrote: >>> 2010/5/24 KaiGai Kohei: BTW, I guess the reason why permissions on attributes are not checked here is that we missed it at v8.4 development. >>> >>> That's a little worrying. Can you construct and post a test case >>> where this results in a user-visible failure in CVS HEAD? >> >> Sorry, after more detailed consideration, it seems to me the permission >> checks in RI_Initial_Check() and its fallback mechanism are nonsense. >> >> See the following commands. >> >> postgres=# CREATE USER ymj; >> CREATE ROLE >> postgres=# CREATE TABLE pk_tbl (a int primary key, b text); >> NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index >> "pk_tbl_pkey" for table "pk_tbl" >> CREATE TABLE >> postgres=# CREATE TABLE fk_tbl (x int, y text); >> CREATE TABLE >> postgres=# ALTER TABLE pk_tbl OWNER TO ymj; >> ALTER TABLE >> postgres=# ALTER TABLE fk_tbl OWNER TO ymj; >> ALTER TABLE >> postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; >> REVOKE >> postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj; >> GRANT >> >> At that time, the 'ymj' has ownership and REFERENCES permissions on >> both of pk_tbl and fk_tbl. In this case, RI_Initial_Check() shall return >> and the fallback-seqscan will run. But, >> >> postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); >> ERROR: permission denied for relation pk_tbl >> CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE "a" >> OPERATOR(pg_catalog.=) $1 FOR SHARE OF x" >> >> From more careful observation of the code, the >> validateForeignKeyConstraint() >> also calls RI_FKey_check_ins() for each scanned tuples, but it also execute >> SELECT statement using SPI_*() interface internally. >> >> In other words, both of execution paths entirely require SELECT permission >> to validate new FK constraint. > > I think the relevant case might be where ymj owns fk_tbl but not > pk_tbl, and has REFERENCES but not SELECT on pk_tbl. > > Come to think of it, I wonder if REFERENCES on fk_tbl ought to be > sufficient to create a foreign key. Currently, it requires ownership: > > rhaas=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); > ERROR: must be owner of relation fk_tbl > +1. We can find out a similar case in CreateTrigger(). If I was granted TRIGGER privilege on a certain table, I can create a new trigger on the table without its ownership. More commonly, it allows us to modify a certain property of the table without its ownership. Perhaps, if SQL permission would be more fine-grained, for example, "RENAME" permission might control RENAME TO statement, rather than its ownership. What is the reason why we check its ownership here, although we already have REFERENCES permission to control ADD FOREIGN KEY? Thanks, -- KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
2010/5/24 KaiGai Kohei : > (2010/05/24 22:18), Robert Haas wrote: >> 2010/5/24 KaiGai Kohei: >>> BTW, I guess the reason why permissions on attributes are not checked here >>> is >>> that we missed it at v8.4 development. >> >> That's a little worrying. Can you construct and post a test case >> where this results in a user-visible failure in CVS HEAD? > > Sorry, after more detailed consideration, it seems to me the permission > checks in RI_Initial_Check() and its fallback mechanism are nonsense. > > See the following commands. > > postgres=# CREATE USER ymj; > CREATE ROLE > postgres=# CREATE TABLE pk_tbl (a int primary key, b text); > NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "pk_tbl_pkey" > for table "pk_tbl" > CREATE TABLE > postgres=# CREATE TABLE fk_tbl (x int, y text); > CREATE TABLE > postgres=# ALTER TABLE pk_tbl OWNER TO ymj; > ALTER TABLE > postgres=# ALTER TABLE fk_tbl OWNER TO ymj; > ALTER TABLE > postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; > REVOKE > postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj; > GRANT > > At that time, the 'ymj' has ownership and REFERENCES permissions on > both of pk_tbl and fk_tbl. In this case, RI_Initial_Check() shall return > and the fallback-seqscan will run. But, > > postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); > ERROR: permission denied for relation pk_tbl > CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE "a" > OPERATOR(pg_catalog.=) $1 FOR SHARE OF x" > > From more careful observation of the code, the validateForeignKeyConstraint() > also calls RI_FKey_check_ins() for each scanned tuples, but it also execute > SELECT statement using SPI_*() interface internally. > > In other words, both of execution paths entirely require SELECT permission > to validate new FK constraint. I think the relevant case might be where ymj owns fk_tbl but not pk_tbl, and has REFERENCES but not SELECT on pk_tbl. Come to think of it, I wonder if REFERENCES on fk_tbl ought to be sufficient to create a foreign key. Currently, it requires ownership: rhaas=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); ERROR: must be owner of relation fk_tbl -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
On Thu, May 20, 2010 at 1:33 PM, Tom Lane wrote: >> As for committing it, I would definitely like to commit the actual >> hook. If we want the hook without the contrib module that's OK with >> me, although I generally feel it's useful to have examples of how >> hooks can be used, which is why I took the time to produce a working >> example. > > +1 on committing the hook. As for the contrib module, it doesn't strike > me that there's much of a use-case for it as is. I think it's enough > that it's available in the -hackers archives. OK, done that way. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
(2010/06/14 21:35), Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: >> This is essentially the same patch that I wrote and posted several >> weeks ago, with changes to the comments and renaming of the >> identifiers. Are you trying to represent it as your own work? > > Ehh, I doubt it. He had included your patch in another patch that he > was working, which I then reviewed and asked him to update/change, and > I think part of that was me asking that he keep the hook patch split > out. He then split it out of his patch rather than just going back to > yours. > >> With all due respect, I intend to imply my own version. Please make >> your other proposed patches apply on top of that. > > This strikes me as a case of "gee, won't git help here?". Perhaps we > can use this as an opportunity to show how git can help. Then again, > it's not exactly a huge patch. :) > The patch provides the same functionality with what you wrote and posted several weeks ago, but different from identifiers and comments. During the discussion, I was suggested that 'ExecutorCheckPerms_hook' is not an appropriate naming on the refactored DML permission check routine, because it is not still a part of the executor. So, I changed your original proposition. When ExecCheckRTPerms() was refactored to a common DML permission checker function, is the hook also renamed to more appropriately? If so, I don't have any opposition to go back to the original one. Thanks, -- KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
* Robert Haas (robertmh...@gmail.com) wrote: > This is essentially the same patch that I wrote and posted several > weeks ago, with changes to the comments and renaming of the > identifiers. Are you trying to represent it as your own work? Ehh, I doubt it. He had included your patch in another patch that he was working, which I then reviewed and asked him to update/change, and I think part of that was me asking that he keep the hook patch split out. He then split it out of his patch rather than just going back to yours. > With all due respect, I intend to imply my own version. Please make > your other proposed patches apply on top of that. This strikes me as a case of "gee, won't git help here?". Perhaps we can use this as an opportunity to show how git can help. Then again, it's not exactly a huge patch. :) Thanks, Stephen (who won't mention the impetus for the hook being put here in the first place.. ;) signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
2010/6/14 KaiGai Kohei : > I attached three patches for the effort. > Each patch tries to tackle one theme, so it is not unreasonable. > > But the ESP security hook patch (quite tiny) depends on the DML permission > refactoring patch (relatively larger). So, Robert suggested me to reconsider > the dependency of these patches. > > The attached patch shall be applied on the head of the git repository. > It just adds a security hook on ExecCheckRTPerms() as Robert suggested > at first. > Of course, it does not allow to acquire the control on COPY TO/FROM and > RI_Initial_Check(). It will be refactored in the following patches. This is essentially the same patch that I wrote and posted several weeks ago, with changes to the comments and renaming of the identifiers. Are you trying to represent it as your own work? With all due respect, I intend to imply my own version. Please make your other proposed patches apply on top of that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
I attached three patches for the effort. Each patch tries to tackle one theme, so it is not unreasonable. But the ESP security hook patch (quite tiny) depends on the DML permission refactoring patch (relatively larger). So, Robert suggested me to reconsider the dependency of these patches. The attached patch shall be applied on the head of the git repository. It just adds a security hook on ExecCheckRTPerms() as Robert suggested at first. Of course, it does not allow to acquire the control on COPY TO/FROM and RI_Initial_Check(). It will be refactored in the following patches. Thanks, (2010/05/27 12:00), KaiGai Kohei wrote: > Stephen, > >>> The 'failure' may make an impression of generic errors not only permission >>> denied. >>> How about 'error_on_violation'? >> >> Maybe 'ereport_on_violation'? I dunno, guess one isn't really better >> than the other. You need to go back and fix the comment though- you >> still say 'abort' there. > > I have no preference between 'error_on_violation' and 'ereport_on_violation'. > OK, I fixed it. > >>> BTW, I wonder whether acl.h is a correct place to explain about the hook, >>> although I added comments for the hook. >> >> Guess I don't really see a problem putting the comments there. By the >> way, have we got a place where we actually document the hooks we support >> somewhere in the official documentation..? If so, that should certainly >> be updated too.. > > I could not find Executor hooks from doc/src/sgml using grep. > If so, it might be worth to list them on the wikipage. > >>> I think we should add a series of explanation about ESP hooks in the >>> internal >>> section of the documentation, when the number of hooks reaches a dozen for >>> example. >> >> I believe the goal will be to avoid reaching a dozen hooks for this. > > Maybe, all we need to hook on DML permissions is only this one. > >> All-in-all, I'm pretty happy with these. Couple minor places which >> could use some copy editing, but that's about it. >> >> Next, we need to get the security label catalog and the grammar to >> support it implemented and then from that an SELinux module should >> be pretty easy to implement. Based on the discussions at PGCon, Robert >> is working on the security label catalog and grammar. The current plan >> is to have a catalog similar to pg_depend, to minimize impact to the >> rest of the backend and to those who aren't interested in using security >> labels. > > Pg_depend? not pg_description/pg_shdescription? > > I basically agree with the idea that minimizes damages to the existing schema > of system catalogs, but I cannot imagine something like pg_depend well. > > I'd like to post a new thread to discuss the security label support. OK? > >> Of course, there will also need to be hooks there for an >> external module to enforce restrictions associated with changing labels >> on various objects in the system. > > Yes, the user given has to be validated by ESP. > > Thanks, -- KaiGai Kohei pgsql-v9.1-add-dml-hook.1.patch Description: application/octect-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
Stephen, >> The 'failure' may make an impression of generic errors not only permission >> denied. >> How about 'error_on_violation'? > > Maybe 'ereport_on_violation'? I dunno, guess one isn't really better > than the other. You need to go back and fix the comment though- you > still say 'abort' there. I have no preference between 'error_on_violation' and 'ereport_on_violation'. OK, I fixed it. >> BTW, I wonder whether acl.h is a correct place to explain about the hook, >> although I added comments for the hook. > > Guess I don't really see a problem putting the comments there. By the > way, have we got a place where we actually document the hooks we support > somewhere in the official documentation..? If so, that should certainly > be updated too.. I could not find Executor hooks from doc/src/sgml using grep. If so, it might be worth to list them on the wikipage. >> I think we should add a series of explanation about ESP hooks in the internal >> section of the documentation, when the number of hooks reaches a dozen for >> example. > > I believe the goal will be to avoid reaching a dozen hooks for this. Maybe, all we need to hook on DML permissions is only this one. > All-in-all, I'm pretty happy with these. Couple minor places which > could use some copy editing, but that's about it. > > Next, we need to get the security label catalog and the grammar to > support it implemented and then from that an SELinux module should > be pretty easy to implement. Based on the discussions at PGCon, Robert > is working on the security label catalog and grammar. The current plan > is to have a catalog similar to pg_depend, to minimize impact to the > rest of the backend and to those who aren't interested in using security > labels. Pg_depend? not pg_description/pg_shdescription? I basically agree with the idea that minimizes damages to the existing schema of system catalogs, but I cannot imagine something like pg_depend well. I'd like to post a new thread to discuss the security label support. OK? > Of course, there will also need to be hooks there for an > external module to enforce restrictions associated with changing labels > on various objects in the system. Yes, the user given has to be validated by ESP. Thanks, -- KaiGai Kohei *** a/src/backend/catalog/aclchk.c --- b/src/backend/catalog/aclchk.c *** *** 135,140 static AclMode pg_aclmask(AclObjectKind objkind, Oid table_oid, AttrNumber attnu --- 135,144 static AclResult check_rte_privileges(RangeTblEntry *rte, bool ereport_on_violation); + /* + * External security provider hooks + */ + check_relation_privileges_hook_type check_relation_privileges_hook = NULL; #ifdef ACLDEBUG static void *** *** 4730,4735 get_user_default_acl(GrantObjectType objtype, Oid ownerId, Oid nsp_oid) --- 4734,4742 * It checks access permissions for all relations listed in a range table. * If violated, it raises an error or returns false depending on the 'abort' * argument. + * It also invokes an external security provide to check the permissions. + * If it is available, both of the default PG checks and external checks + * have to allow the required accesses for the relations. */ AclResult check_relation_privileges(List *rangeTable, bool ereport_on_violation) *** *** 4746,4751 check_relation_privileges(List *rangeTable, bool ereport_on_violation) --- 4753,4763 if (retval != ACLCHECK_OK) return retval; } + + /* External security provider invocation */ + if (check_relation_privileges_hook) + retval = (*check_relation_privileges_hook)(rangeTable, + ereport_on_violation); return retval; } *** a/src/backend/executor/execMain.c --- b/src/backend/executor/execMain.c *** *** 466,471 InitPlan(QueryDesc *queryDesc, int eflags) --- 466,475 * check anything for other RTE types (function, join, subquery, ...). * Function RTEs are checked by init_fcache when the function is prepared * for execution. Join, subquery, and special RTEs need no checks. + * + * If available, it also invokes an external security provider. In this + * case, both of the default PG checks and the external checks have to + * allow the required accesses on the relation */ check_relation_privileges(rangeTable, true); *** a/src/include/utils/acl.h --- b/src/include/utils/acl.h *** *** 196,201 typedef enum AclObjectKind --- 196,216 MAX_ACL_KIND/* MUST BE LAST */ } AclObjectKind; + /* + * External security provider hooks + */ + + /* + * check_relation_privileges_hook + * It allows an ESP to get control at check_relation_privileges(). + * A list of RangeTblEntry to be referenced and a flag to inform preferable + * bahavior on access violations. + * The ESP shall return ACLCHECK_OK if it allows the required access. + * Elsewhere, it raises an error or return other AclResult statu
Re: [HACKERS] ExecutorCheckPerms() hook
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: > Stephen, thanks for comments. > > The attached three patches are the revised and divided ones. > > A: add makeRangeTblEntry() Ok, didn't actually expect that. Guess my suggestion would have been to just use makeNode() since there wasn't anything more appropriate already. Still, I don't really have a problem with your makeRangeTblEntry() and you certainly found quite a few places to use it. > B: major reworks of DML permission checks No serious issues with this that I saw either. I definitely think it's cleaner using makeRangeTblEntry() and check_relation_privileges(). > C: add an ESP hook on the DML permission checks This also looks good to me, though I don't know that you really need the additional comment in execMain.c about the hook. I would make sure that you have a comment around check_rte_privileges() which says not to call it directly because you'll bypass the hook (and potentially cause a security leak by doing so). Don't recall seeing that, apologies if it was there. > IIRC, Robert suggested that a verb should lead the function name. > So, I renamed it into check_relation_privileges() and check_rte_privileges(). Yeah, that's alright. I'm on the fence about using 'relation' or using 'rangetbl' there, but certainly whomever commits this could trivially change it to whatever they prefer. > The 'failure' may make an impression of generic errors not only permission > denied. > How about 'error_on_violation'? Maybe 'ereport_on_violation'? I dunno, guess one isn't really better than the other. You need to go back and fix the comment though- you still say 'abort' there. > > - Have you checked if there are any bad side-effects from calling > >ri_FetchConstraintInfo before doing the permissions checking? > > The ri_FetchConstraintInfo() only references SysCaches to set up given > local variable without any other locks except for ones acquired by syscache.c. Ok. > > - The hook in acl.h should be separated out and brought to the top and > >documented independently as to exactly where the hook is and what it > >can be used for, along with what the arguments mean, etc. Similairly, > >chkpriv_relation_perms should really have a short comment for it about > >what it's for. Something more than 'security checker function'. > > OK, at the patch-C, I moved the definition of the hook into the first half > of acl.h, but it needs to be declared after the AclResult definition. Fair enough. > BTW, I wonder whether acl.h is a correct place to explain about the hook, > although I added comments for the hook. Guess I don't really see a problem putting the comments there. By the way, have we got a place where we actually document the hooks we support somewhere in the official documentation..? If so, that should certainly be updated too.. > I think we should add a series of explanation about ESP hooks in the internal > section of the documentation, when the number of hooks reaches a dozen for > example. I believe the goal will be to avoid reaching a dozen hooks for this. All-in-all, I'm pretty happy with these. Couple minor places which could use some copy editing, but that's about it. Next, we need to get the security label catalog and the grammar to support it implemented and then from that an SELinux module should be pretty easy to implement. Based on the discussions at PGCon, Robert is working on the security label catalog and grammar. The current plan is to have a catalog similar to pg_depend, to minimize impact to the rest of the backend and to those who aren't interested in using security labels. Of course, there will also need to be hooks there for an external module to enforce restrictions associated with changing labels on various objects in the system. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
Stephen, thanks for comments. The attached three patches are the revised and divided ones. A: add makeRangeTblEntry() B: major reworks of DML permission checks C: add an ESP hook on the DML permission checks (2010/05/27 0:09), Stephen Frost wrote: > KaiGai, > > * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: >> The attached patch is a revised one for DML permission checks. > > This is certainly alot better. > >> ToDo: >> - makeRangeTblEntry() stuff to allocate a RTE node with given parameter >>is not yet. > > I'd certainly like to see the above done, or to understand why it can't > be if that turns out to be the case. The patch-A tries to implement makeRangeTblEntry() which takes only rtekind as argument right now. Other fields are initialized to zero, using makeNode(). > A couple of other comments, all pretty minor things: > > - I'd still rather see the hook itself in another patch, but given that >we've determined that none of this is going to go into 9.0, it's not >as big a deal. OK, I divided the ESP hook part into the patch-C. > - The hook definition in aclchk.c should really be at the top of that >file. We've been pretty consistant about putting hooks at the top of >files instead of deep down in the file, this should also follow that >scheme. OK, I moved it. > - Some of the comments at the top of chkpriv_rte_perms probably make >sense to move up to where it's called from execMain.c. Specifically, >the comments about the other RTE types (function, join, subquery). >I'd probably change the comment in chkpriv_rte_perms to be simpler- >"This is only used for checking plain relation permissions, nothing >else is checked here", and also have that same comment around >chkpriv_relation_perms, both in aclchk.c and in acl.h. OK, I edited the comment as follows: | /* |* Do permissions checks. The check_relation_privileges() checks access |* permissions for all relations listed in a range table, but does not |* check anything for other RTE types (function, join, subquery, ...). |* Function RTEs are checked by init_fcache when the function is prepared |* for execution. Join, subquery, and special RTEs need no checks. |*/ > - I'd move chkpriv_relation_perms above chkpriv_rte_perms, it's what we >expect people to use, after all. OK, I reordered it. > - Don't particularly like the function names. How about >relation_privilege_check? Or rangetbl_privilege_check? We don't use >'perms' much (uh, at all?) in function names, and even if we did, it'd >be redundant and not really help someone understand what the function >is doing. IIRC, Robert suggested that a verb should lead the function name. So, I renamed it into check_relation_privileges() and check_rte_privileges(). > - I don't really like having 'abort' as the variable name for the 2nd >argument. I'm not finding an obvious convention right now, but maybe >something like "error_on_failure" instead? The 'failure' may make an impression of generic errors not only permission denied. How about 'error_on_violation'? > - In DoCopy, some comments about what you're doing there to set up for >calling chkpriv_relation_perms would be good (like the comment you >removed- /* We don't have table permissions, check per-column >permissions */, updated to for something like "build an RTE with the >columns referenced marked to check for necessary privileges"). >Additionally, it might be worth considering if having an RTE built >farther up in DoCopy would make sense and would then be usable for >other bits in DoCopy. I edited the comments as follows: | /* | * Check relation permissions. | * We built an RTE with the relation and columns to be accessed | * to check for necessary privileges in the common way. | */ > - In RI_Initial_Check, why not build up an actual list of RTEs and just >call chkpriv_relation_perms once? Also, you should add comments >there, again, about what you're doing and why. If you can use another >function to build the actual RTE, this will probably fall out more >sensibly too. Good catch! I fixed the invocation of checker function with list_make2(). And, I edited the comments as follows: | /* | * We built a pair of RTEs of FK/PK relations and columns referenced | * in the test query to check necessary permission in the common way. | */ > - Have you checked if there are any bad side-effects from calling >ri_FetchConstraintInfo before doing the permissions checking? The ri_FetchConstraintInfo() only references SysCaches to set up given local variable without any other locks except for ones acquired by syscache.c. > - The hook in acl.h should be separated out and brought to the top and >documented independently as to exactly where the hook is and what it >can be used for, along with what the arguments mean, etc. Similairly, >chkpriv_relation_perms should real
Re: [HACKERS] ExecutorCheckPerms() hook
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Because the queries inside the triggers are done with a different > current userid. Indeed, I figured that out eventually too. Sorry it took so long. :/ Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: > The attached patch is a revised one for DML permission checks. This is certainly alot better. > ToDo: > - makeRangeTblEntry() stuff to allocate a RTE node with given parameter > is not yet. I'd certainly like to see the above done, or to understand why it can't be if that turns out to be the case. A couple of other comments, all pretty minor things: - I'd still rather see the hook itself in another patch, but given that we've determined that none of this is going to go into 9.0, it's not as big a deal. - The hook definition in aclchk.c should really be at the top of that file. We've been pretty consistant about putting hooks at the top of files instead of deep down in the file, this should also follow that scheme. - Some of the comments at the top of chkpriv_rte_perms probably make sense to move up to where it's called from execMain.c. Specifically, the comments about the other RTE types (function, join, subquery). I'd probably change the comment in chkpriv_rte_perms to be simpler- "This is only used for checking plain relation permissions, nothing else is checked here", and also have that same comment around chkpriv_relation_perms, both in aclchk.c and in acl.h. - I'd move chkpriv_relation_perms above chkpriv_rte_perms, it's what we expect people to use, after all. - Don't particularly like the function names. How about relation_privilege_check? Or rangetbl_privilege_check? We don't use 'perms' much (uh, at all?) in function names, and even if we did, it'd be redundant and not really help someone understand what the function is doing. - I don't really like having 'abort' as the variable name for the 2nd argument. I'm not finding an obvious convention right now, but maybe something like "error_on_failure" instead? - In DoCopy, some comments about what you're doing there to set up for calling chkpriv_relation_perms would be good (like the comment you removed- /* We don't have table permissions, check per-column permissions */, updated to for something like "build an RTE with the columns referenced marked to check for necessary privileges"). Additionally, it might be worth considering if having an RTE built farther up in DoCopy would make sense and would then be usable for other bits in DoCopy. - In RI_Initial_Check, why not build up an actual list of RTEs and just call chkpriv_relation_perms once? Also, you should add comments there, again, about what you're doing and why. If you can use another function to build the actual RTE, this will probably fall out more sensibly too. - Have you checked if there are any bad side-effects from calling ri_FetchConstraintInfo before doing the permissions checking? - The hook in acl.h should be separated out and brought to the top and documented independently as to exactly where the hook is and what it can be used for, along with what the arguments mean, etc. Similairly, chkpriv_relation_perms should really have a short comment for it about what it's for. Something more than 'security checker function'. All pretty minor things that I'd probably just fix myself if I was going to be committing it (not that I have that option ;). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
KaiGai Kohei writes: > 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. Because the queries inside the triggers are done with a different current userid. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: > Yes, it is entirely separate issue. I don't intend to argue whether > we can assume the default PG permission allows owner to SELECT on > the table, or not. This actually isn't a separate issue. It's the whole crux of it, as a matter of fact. So, wrt the standard, you do NOT need SELECT rights on a table to create an FK against it. You only need references. PG handles this correctly. This error: > >>postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl > >> (a); > >>ERROR: permission denied for relation pk_tbl > >>CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE > >> "a" OPERATOR(pg_catalog.=) $1 FOR SHARE OF x" Is due to the *owner* of pk_tbl not having SELECT rights- a situation we're not generally expecting to see. What's happening is that prior to the above being run, we've switched user over to the owner of the table to perform this check. This script illustrates what I'm talking about: CREATE USER pk_owner; CREATE USER fk_owner; GRANT pk_owner TO sfrost; GRANT fk_owner TO sfrost; SET ROLE pk_owner; CREATE TABLE pk_tbl (a int primary key, b text); INSERT INTO pk_tbl VALUES (1,'aaa'), (2,'bbb'), (3,'ccc'); GRANT REFERENCES ON pk_tbl TO fk_owner; SET ROLE fk_owner; CREATE TABLE fk_tbl (x int, y text); INSERT INTO fk_tbl VALUES (1,'xxx'), (2,'yyy'), (3,'zzz'); ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); ALTER TABLE fk_tbl DROP CONSTRAINT fk_tbl_x_fkey; SET ROLE pk_owner; REVOKE ALL ON pk_tbl FROM pk_owner; SET ROLE fk_owner; ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); ERROR: permission denied for relation pk_tbl CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE "a" OPERATOR(pg_catalog.=) $1 FOR SHARE OF x" This does make me think (as I've thought in the past..) that we really should say *who* doesn't have that permission. We run into the same problem with views- they're run as the owner of the view, so you can get a permission denied error trying to select from the view when you clearly have select rights on the view itself. As it turns out, the check in RI_Initial_Check() to provide the speed-up is if the current role can just SELECT against the PK table- in which case, you can run the check as the FK user and not have to change user. We can't just switch to the PK user and run the same query though, because that user might not have any rights on the FK table. So, we end up taking the slow path, which fires off the FK trigger that's been set up on the fk_tbl but which runs as the owner of the pk_tbl. So, long-and-short, I don't see that we have a bug in any of this. I do think we should allow RI_Initial_Check() to run if it has the necessary column-level permissions, and we should remove the duplicate permissions-checking code in favor of using the same code the executor will, which happens to also be where the new hook we're talking about is. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: > 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. Ok. If we require REFERENCES and SELECT privs to create an FK then I think the question is: when is the path in RI_Initial_Check not able to be used (hence, why do we need a fall-back)? My guess would be: role X has: Primary table: SELECT, REFERENCES Foreign table: REFERENCES This doesn't make much sense either though, because X has to own the foreign table. postgres=> alter table fk_tbl add foreign key (x) references pk_tbl; ERROR: must be owner of relation fk_tbl So, the only situation, it seems, where the fall-back method has to be used is when X owns the table but doesn't have SELECT rights on it. Maybe it's just me, but that seems pretty silly. If we require: Primary table: SELECT, REFERENCES Foreign table: OWNER, SELECT, REFERENCES Then it seems like we should be able to eliminate the fall-back method and just use the RI_Initial_Check approach. What am I missing here? :/ > 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. RI_Initial_Check is at least missing an optimization to support column-level priviledges. If we say that REFERENCES alone is allowed to create a FK, then the fall-back method is broken because it depends on SELECT rights on the primary. To be honest, I'm guessing that the reason there's so much confusion around this is that the spec probably says you don't need SELECT rights (I havn't checked though), and at some point in the distant past we handled that correctly with the fall-back method and that has since been broken by other changes (possibly to plug the hole the fall-back method was using). I'll try to go deipher the spec so we can at least have something more interesting to discuss (if we agree with doing it how the spec says or not :). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
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 (kai...@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 >>
Re: [HACKERS] ExecutorCheckPerms() hook
(2010/05/26 12:17), Tom Lane wrote: > Stephen Frost 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
Stephen Frost 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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
(2010/05/26 11:12), Stephen Frost wrote: > * KaiGai Kohei (kai...@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 sh
Re: [HACKERS] ExecutorCheckPerms() hook
* KaiGai Kohei (kai...@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 signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
* KaiGai Kohei (kai...@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 signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
(2010/05/25 22:59), Stephen Frost wrote: > * KaiGai Kohei (kai...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
(2010/05/25 21:44), Stephen Frost wrote: > KaiGai, > > * KaiGai Kohei (kai...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
* KaiGai Kohei (kai...@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 signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
KaiGai, * KaiGai Kohei (kai...@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 signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
KaiGai, * KaiGai Kohei (kai...@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 signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
(2010/05/25 12:19), Robert Haas wrote:
> On Mon, May 24, 2010 at 9:27 PM, Stephen Frost wrote:
>> * KaiGai Kohei (kai...@ak.jp.nec.com) wrote:
>>> We have two options; If the checker function takes the list of
>>> RangeTblEntry,
>>> it will be comfortable to ExecCheckRTPerms(), but not DoCopy(). Inversely,
>>> if the checker function takes arguments in my patch, it will be comfortable
>>> to DoCopy(), but ExecCheckRTPerms().
>>>
>>> In my patch, it takes 6 arguments, but we can reference all of them from
>>> the given RangeTblEntry. On the other hand, if DoCopy() has to set up
>>> a pseudo RangeTblEntry to call checker function, it entirely needs to set
>>> up similar or a bit large number of variables.
>>
>> I don't know that it's really all that difficult to set up an RT in
>> DoCopy or RI_Initial_Check(). In my opinion, those are the strange or
>> corner cases- not the Executor code, through which all 'regular' DML is
>> done. It makes me wonder if COPY shouldn't have been implemented using
>> the Executor instead, but that's, again, a completely separate topic.
>> It wasn't, but it wants to play like it operates in the same kind of way
>> as INSERT, so it needs to pick up the slack.
>
> I think this approach is definitely worth investigating. KaiGai, can
> you please work up what the patch would look like if we do it this
> way?
OK, the attached patch reworks it according to the way.
* 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.
Thanks,
--
KaiGai Kohei
*** a/src/backend/commands/copy.c
--- b/src/backend/commands/copy.c
***
*** 21,26
--- 21,27
#include
#include "access/heapam.h"
+ #include "access/sysattr.h"
#include "access/xact.h"
#include "catalog/namespace.h"
#include "catalog/pg_type.h"
***
*** 37,43
#include "rewrite/rewriteHandler.h"
#include "storage/fd.h"
#include "tcop/tcopprot.h"
- #include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
--- 38,43
***
*** 725,733 DoCopy(const CopyStmt *stmt, const char *queryString)
List *force_notnull = NIL;
bool force_quote_all = false;
bool format_specified = false;
- AclMode required_access = (is_from ? ACL_INSERT : ACL_SELECT);
- AclMode relPerms;
- AclMode remainingPerms;
ListCell *option;
TupleDesc tupDesc;
int num_phys_attrs;
--- 725,730
***
*** 988,993 DoCopy(const CopyStmt *stmt, const char *queryString)
--- 985,995
if (stmt->relation)
{
+ RangeTblEntry rte;
+ Bitmapset *columnsSet = NULL;
+ List *attnums;
+ ListCell *cur;
+
Assert(!stmt->query);
cstate->queryDesc = NULL;
***
*** 998,1026 DoCopy(const CopyStmt *stmt, const char *queryString)
tupDesc = RelationGetDescr(cstate->rel);
/* Check relation permissions. */
! relPerms = pg_class_aclmask(RelationGetRelid(cstate->rel), GetUserId(),
! required_access, ACLMASK_ALL);
! remainingPerms = required_access & ~relPerms;
! if (remainingPerms != 0)
{
! /* We don't have table permissions, check per-column permissions */
! List *attnums;
! ListCell *cur;
!
! attnums = CopyGetAttnums(tupDesc, cstate->rel, attnamelist);
! foreach(cur, attnums)
! {
! int attnum = lfirst_int(cur);
! if (pg_attribute_aclcheck(RelationGetRelid(cstate->rel),
! attnum,
! GetUserId(),
! remainingPerms) != ACLCHECK_OK)
! aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS,
! RelationGetRelationName(cstate->rel));
! }
}
/* check read-only transaction */
if (XactReadOnly && is_from && !cstate->rel->rd_islocaltemp)
PreventCommandIfReadOnly("COPY FROM");
--- 1000,1025
tupDesc = RelationGetDescr(cstate->rel);
/* Check relation permissions. */
! attnums = CopyGetAttnums(tupDesc, cstate->rel, attnamelist);
! foreach (cur, attnums)
{
! int attnum = lfirst_int(cur) - FirstLowInvalidHeapAttributeNumber;
! columnsSet = bms_add_member(columnsSet, attnum);
}
+ memset(&rte, 0, sizeof(rte));
+ rte.type = T_RangeTblEntry;
+ rte.rtekind = RTE_RELATION;
+ rte.relid = RelationGetRelid(cstate->rel);
+ rte.requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
+ if (is_from)
+ rte.modifiedCols = columnsSet;
+ else
+ rte.selectedCols = columnsSet;
+
+ ExecCheckRTEPerms(&rte, true);
+
/* check read-only transaction */
if (XactReadOnly && is_from && !cstate->rel->rd_islocaltemp)
PreventCommandIfReadOnly("COPY FROM");
*** a/src/backend/executor/execMain.c
--- b/src/bac
Re: [HACKERS] ExecutorCheckPerms() hook
Stephen Frost writes: > ... It makes me wonder if COPY shouldn't have been implemented using > the Executor instead, but that's, again, a completely separate topic. > It wasn't, but it wants to play like it operates in the same kind of way > as INSERT, so it needs to pick up the slack. FWIW, we've shifted COPY more towards using executor support over the years. I'm pretty sure that it didn't originally use the executor's index-entry-insertion infrastructure, for instance. Building an RT entry seems like a perfectly sane thing to do in order to make it use the executor's permissions infrastructure. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
On Mon, May 24, 2010 at 9:27 PM, Stephen Frost wrote: > * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: >> We have two options; If the checker function takes the list of RangeTblEntry, >> it will be comfortable to ExecCheckRTPerms(), but not DoCopy(). Inversely, >> if the checker function takes arguments in my patch, it will be comfortable >> to DoCopy(), but ExecCheckRTPerms(). >> >> In my patch, it takes 6 arguments, but we can reference all of them from >> the given RangeTblEntry. On the other hand, if DoCopy() has to set up >> a pseudo RangeTblEntry to call checker function, it entirely needs to set >> up similar or a bit large number of variables. > > I don't know that it's really all that difficult to set up an RT in > DoCopy or RI_Initial_Check(). In my opinion, those are the strange or > corner cases- not the Executor code, through which all 'regular' DML is > done. It makes me wonder if COPY shouldn't have been implemented using > the Executor instead, but that's, again, a completely separate topic. > It wasn't, but it wants to play like it operates in the same kind of way > as INSERT, so it needs to pick up the slack. I think this approach is definitely worth investigating. KaiGai, can you please work up what the patch would look like if we do it this way? Thanks, -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
2010/5/24 KaiGai Kohei : > I think we need a new SPI_*() interface which allows to run the given query > without any permission checks, because these queries are purely internal > stuff, > so we can trust the query is harmless. [...] > I'm afraid of that the src/backend/catalog/aclchk.c will become overcrowding > in the future. If it is ugly to deploy checker functions in separated > dir/files, > I think it is an idea to put it on the execMain.c, instead of > ExecCheckRTEPerms(). Both of these are bad ideas, for reasons Stephen Frost has articulated well. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
(2010/05/25 10:27), Stephen Frost wrote: > * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: >> We have two options; If the checker function takes the list of RangeTblEntry, >> it will be comfortable to ExecCheckRTPerms(), but not DoCopy(). Inversely, >> if the checker function takes arguments in my patch, it will be comfortable >> to DoCopy(), but ExecCheckRTPerms(). >> >> In my patch, it takes 6 arguments, but we can reference all of them from >> the given RangeTblEntry. On the other hand, if DoCopy() has to set up >> a pseudo RangeTblEntry to call checker function, it entirely needs to set >> up similar or a bit large number of variables. > > I don't know that it's really all that difficult to set up an RT in > DoCopy or RI_Initial_Check(). In my opinion, those are the strange or > corner cases- not the Executor code, through which all 'regular' DML is > done. It makes me wonder if COPY shouldn't have been implemented using > the Executor instead, but that's, again, a completely separate topic. > It wasn't, but it wants to play like it operates in the same kind of way > as INSERT, so it needs to pick up the slack. > Yes, it is not difficult to set up. The reason why I prefer the checker function takes 6 arguments are that DoCopy() / RI_Initial_Check() has to set up RangeTblEntry in addition to Bitmap set, but we don't have any other significant reason. OK, let's add a hook in the ExecCheckRTPerms(). * RI_Initial_Check() >> >> It seems to me the permission checks in RI_Initial_Check() is a bit ad-hoc. > > I agree with this- my proposal would address this in a way whih would be > guaranteed to be consistant: by using the same code path to do both > checks. I'm still not thrilled with how RI_Initial_Check() works, but > rewriting that isn't part of this. I agree to ignore the problem right now. It implicitly assume the owner has SELECT privilege on the FK/PK tables, so the minimum SELinux module also implicitly assume the client has similar permissions on it. >> In this case, we should execute the secondary query without permission >> checks, >> because the permissions of ALTER TABLE is already checked, and we can trust >> the given query is harmless. > > I dislike the idea of providing a new SPI interfance (on the face of > it), and also dislike the idea of having a "skip all permissions > checking" option for anything which resembles SPI. I would rather ask > the question of if it really makes sense to use SPI to check FKs as > they're being added, but we're not going to solve that issue here. Apart from the topic of this thread, I guess it allows us to utilize query optimization and cascaded triggers to implement FK constraints with minimum code size. Thanks -- KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
(2010/05/25 10:13), Stephen Frost wrote: > KaiGai, > > * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: >>postgres=# ALTER TABLE pk_tbl OWNER TO ymj; >>ALTER TABLE >>postgres=# ALTER TABLE fk_tbl OWNER TO ymj; >>ALTER TABLE >>postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; >>REVOKE >>postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj; >>GRANT >> >> At that time, the 'ymj' has ownership and REFERENCES permissions on >> both of pk_tbl and fk_tbl. In this case, RI_Initial_Check() shall return >> and the fallback-seqscan will run. But, > > ymj may be considered an 'owner' on that table, but in this case, it > doesn't have SELECT rights on it. Now, you might argue that we should > assume that the owner has SELECT rights (since they're granted by > default), even if they've been revoked, but that's a whole separate > issue. Yes, it is entirely separate issue. I don't intend to argue whether we can assume the default PG permission allows owner to SELECT on the table, or not. >>postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); >>ERROR: permission denied for relation pk_tbl >>CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE "a" >> OPERATOR(pg_catalog.=) $1 FOR SHARE OF x" > > I think you've got another issue here that's not related. Perhaps > something wrong with a patch you've applied? Otherwise, what version of > PG is this? Using 8.2, 8.3, 8.4 and a recent git checkout, I get: > > postgres=# CREATE USER ymj; > CREATE ROLE > postgres=# CREATE TABLE pk_tbl (a int primary key, b text); > NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "pk_tbl_pkey" > for table "pk_tbl" > CREATE TABLE > postgres=# CREATE TABLE fk_tbl (x int, y text); > CREATE TABLE > postgres=# ALTER TABLE pk_tbl OWNER TO ymj; > ALTER TABLE > postgres=# ALTER TABLE fk_tbl OWNER TO ymj; > ALTER TABLE > postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; > REVOKE > postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj; > GRANT > postgres=# SET ROLE ymj; > SET > postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); > ALTER TABLE > postgres=> Sorry, I missed to copy & paste INSERT statement just after CREATE TABLE. The secondary RI_FKey_check_ins() is invoked during the while() loop using heap_getnext(), so it is not called for empty table. For correctness, postgres=# CREATE USER ymj; CREATE ROLE postgres=# CREATE TABLE pk_tbl (a int primary key, b text); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "pk_tbl_pkey" for table "pk_tbl" CREATE TABLE postgres=# CREATE TABLE fk_tbl (x int, y text); CREATE TABLE | postgres=# INSERT INTO pk_tbl VALUES (1,'aaa'), (2,'bbb'), (3,'ccc'); | INSERT 0 3 | postgres=# INSERT INTO fk_tbl VALUES (1,'xxx'), (2,'yyy'), (3,'zzz'); | INSERT 0 3 postgres=# ALTER TABLE pk_tbl OWNER TO ymj; ALTER TABLE postgres=# ALTER TABLE fk_tbl OWNER TO ymj; ALTER TABLE postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; REVOKE postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj; GRANT postgres=# SET ROLE ymj; SET postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); ERROR: permission denied for relation pk_tbl CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE "a" OPERATOR(pg_catalog.=) $1 FOR SHARE OF x" I could reproduce it on the 8.4.4, but didn't try on the prior releases. Thanks, -- KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote: > We have two options; If the checker function takes the list of RangeTblEntry, > it will be comfortable to ExecCheckRTPerms(), but not DoCopy(). Inversely, > if the checker function takes arguments in my patch, it will be comfortable > to DoCopy(), but ExecCheckRTPerms(). > > In my patch, it takes 6 arguments, but we can reference all of them from > the given RangeTblEntry. On the other hand, if DoCopy() has to set up > a pseudo RangeTblEntry to call checker function, it entirely needs to set > up similar or a bit large number of variables. I don't know that it's really all that difficult to set up an RT in DoCopy or RI_Initial_Check(). In my opinion, those are the strange or corner cases- not the Executor code, through which all 'regular' DML is done. It makes me wonder if COPY shouldn't have been implemented using the Executor instead, but that's, again, a completely separate topic. It wasn't, but it wants to play like it operates in the same kind of way as INSERT, so it needs to pick up the slack. > As I replied in the earlier message, it may be an idea to rename and change > the definition of ExecCheckRTEPerms() without moving it anywhere. And, again, I don't see that as a good idea at all. > >> * RI_Initial_Check() > > It seems to me the permission checks in RI_Initial_Check() is a bit ad-hoc. I agree with this- my proposal would address this in a way whih would be guaranteed to be consistant: by using the same code path to do both checks. I'm still not thrilled with how RI_Initial_Check() works, but rewriting that isn't part of this. > In this case, we should execute the secondary query without permission checks, > because the permissions of ALTER TABLE is already checked, and we can trust > the given query is harmless. I dislike the idea of providing a new SPI interfance (on the face of it), and also dislike the idea of having a "skip all permissions checking" option for anything which resembles SPI. I would rather ask the question of if it really makes sense to use SPI to check FKs as they're being added, but we're not going to solve that issue here. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: > postgres=# ALTER TABLE pk_tbl OWNER TO ymj; > ALTER TABLE > postgres=# ALTER TABLE fk_tbl OWNER TO ymj; > ALTER TABLE > postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; > REVOKE > postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj; > GRANT > > At that time, the 'ymj' has ownership and REFERENCES permissions on > both of pk_tbl and fk_tbl. In this case, RI_Initial_Check() shall return > and the fallback-seqscan will run. But, ymj may be considered an 'owner' on that table, but in this case, it doesn't have SELECT rights on it. Now, you might argue that we should assume that the owner has SELECT rights (since they're granted by default), even if they've been revoked, but that's a whole separate issue. > postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); > ERROR: permission denied for relation pk_tbl > CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE "a" > OPERATOR(pg_catalog.=) $1 FOR SHARE OF x" I think you've got another issue here that's not related. Perhaps something wrong with a patch you've applied? Otherwise, what version of PG is this? Using 8.2, 8.3, 8.4 and a recent git checkout, I get: postgres=# CREATE USER ymj; CREATE ROLE postgres=# CREATE TABLE pk_tbl (a int primary key, b text); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "pk_tbl_pkey" for table "pk_tbl" CREATE TABLE postgres=# CREATE TABLE fk_tbl (x int, y text); CREATE TABLE postgres=# ALTER TABLE pk_tbl OWNER TO ymj; ALTER TABLE postgres=# ALTER TABLE fk_tbl OWNER TO ymj; ALTER TABLE postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; REVOKE postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj; GRANT postgres=# SET ROLE ymj; SET postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); ALTER TABLE postgres=> > I think we need a new SPI_*() interface which allows to run the given query > without any permission checks, because these queries are purely internal > stuff, > so we can trust the query is harmless. > Is there any other idea? Yeah, I don't see that going anywhere... > I'm afraid of that the src/backend/catalog/aclchk.c will become overcrowding > in the future. If it is ugly to deploy checker functions in separated > dir/files, > I think it is an idea to put it on the execMain.c, instead of > ExecCheckRTEPerms(). No, this is not a service of the executor, putting it in execMain.c does not make any sense. > It also suggest us where the checker functions should be deployed on the > upcoming > DDL reworks. In similar way, we will deploy them on > src/backend/command/pg_database > for example? We'll worry about DDL when we get there. It won't be any time soon. I would strongly recommend that you concentrate on building an SELinux module using the hook function that Robert wrote or none of this is going to end up going anywhere. If and when we find other places which handle DML and need adjustment, we can identify how to handle them at that time. Hopefully by the time we're comfortable with DML, some of the DDL permissions checking rework will have been done and how to move forward with allowing external security modules to handle that will be clear. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
(2010/05/25 4:11), Stephen Frost wrote: > * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: >> I'd like to point out two more points are necessary to be considered >> for DML permission checks in addition to ExecCheckRTPerms(). >> >> * DoCopy() >> >> Although DoCopy() is called from standard_ProcessUtility(), it performs >> as DML statement, rather than DDL. It check ACL_SELECT or ACL_INSERT on >> the copied table or attributes, similar to what ExecCheckRTEPerms() doing. > > Rather than construct a complicated API for this DML activity, why don't > we just make ExecCheckRTPerms available for DoCopy to use? It seems > like we could move ExecCheckRTPerms() to acl.c without too much trouble. > acl.h already includes parsenodes.h and has knowledge of RangeVar's. > Once DoCopy is using that, this issue resolves itself with the hook that > Robert already wrote up. We have two options; If the checker function takes the list of RangeTblEntry, it will be comfortable to ExecCheckRTPerms(), but not DoCopy(). Inversely, if the checker function takes arguments in my patch, it will be comfortable to DoCopy(), but ExecCheckRTPerms(). In my patch, it takes 6 arguments, but we can reference all of them from the given RangeTblEntry. On the other hand, if DoCopy() has to set up a pseudo RangeTblEntry to call checker function, it entirely needs to set up similar or a bit large number of variables. As I replied in the earlier message, it may be an idea to rename and change the definition of ExecCheckRTEPerms() without moving it anywhere. >> * RI_Initial_Check() >> >> RI_Initial_Check() is a function called on ALTER TABLE command to add FK >> constraints between two relations. The permission to execute this ALTER TABLE >> command itself is checked on ATPrepCmd() and ATAddForeignKeyConstraint(), >> so it does not affect anything on the DML permission reworks. > > I'm not really thrilled with how RI_Initial_Check() does it's own > permissions checking and then calls SPI expecting things to 'just work'. > Not sure if there's some way we could handle failure from SPI, or, if it > was changed to call ExecCheckRTPerms() instead, how it would handle > failure cases from there. One possible solution would be to have an > additional option to ExecCheckRTPerms() which asks it to just check and > return false if there's a problem, rather than unconditionally calling > aclcheck_error() whenever it finds a problem. > > Using the same function for both the initial check in RI_Initial_Check() > and then from SPI would eliminate issues where those two checks disagree > for some reason, which would be good in the general case. Sorry, I missed the fallback path also needs SELECT permissions because validateForeignKeyConstraint() calls RI_FKey_check_ins() which entirely tries to execute SELECT statement using SPI_*() interface. But, it is a separate issue from the DML permission reworks. It seems to me the permission checks in RI_Initial_Check() is a bit ad-hoc. What we really want to do here is validation of the new FK constraints. So, the validateForeignKeyConstraint() intends to provide a fall-back code when table-level permission is denied, doesn't it? In this case, we should execute the secondary query without permission checks, because the permissions of ALTER TABLE is already checked, and we can trust the given query is harmless. >> BTW, I guess the reason why permissions on attributes are not checked here is >> that we missed it at v8.4 development. > > Indeed, but at the same time, this looks to be a 'fail-safe' situation. > Basically, this is checking table-level permissions, which, if you have, > gives you sufficient rights to SELECT against the table (any column). > What this isn't doing is allowing the option of column-level permissions > to be sufficient for this requirement. That's certainly an oversight in > the column-level permissions handling (sorry about that), but it's not > horrible- there's a workaround if RI_Initial_Check returns false already > anyway. Yes, it is harmless expect for performances in a corner-case. If user have table-level permissions, it does not need to check column- level permissions, even if it is implemented. Thanks, -- KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
(2010/05/24 22:18), Robert Haas wrote: > 2010/5/24 KaiGai Kohei: >> BTW, I guess the reason why permissions on attributes are not checked here is >> that we missed it at v8.4 development. > > That's a little worrying. Can you construct and post a test case > where this results in a user-visible failure in CVS HEAD? Sorry, after more detailed consideration, it seems to me the permission checks in RI_Initial_Check() and its fallback mechanism are nonsense. See the following commands. postgres=# CREATE USER ymj; CREATE ROLE postgres=# CREATE TABLE pk_tbl (a int primary key, b text); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "pk_tbl_pkey" for table "pk_tbl" CREATE TABLE postgres=# CREATE TABLE fk_tbl (x int, y text); CREATE TABLE postgres=# ALTER TABLE pk_tbl OWNER TO ymj; ALTER TABLE postgres=# ALTER TABLE fk_tbl OWNER TO ymj; ALTER TABLE postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; REVOKE postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj; GRANT At that time, the 'ymj' has ownership and REFERENCES permissions on both of pk_tbl and fk_tbl. In this case, RI_Initial_Check() shall return and the fallback-seqscan will run. But, postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); ERROR: permission denied for relation pk_tbl CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE "a" OPERATOR(pg_catalog.=) $1 FOR SHARE OF x" >From more careful observation of the code, the validateForeignKeyConstraint() also calls RI_FKey_check_ins() for each scanned tuples, but it also execute SELECT statement using SPI_*() interface internally. In other words, both of execution paths entirely require SELECT permission to validate new FK constraint. I think we need a new SPI_*() interface which allows to run the given query without any permission checks, because these queries are purely internal stuff, so we can trust the query is harmless. Is there any other idea? >> The attached patch provides a common checker function of DML, and modifies >> ExecCheckRTPerms(), CopyTo() and RI_Initial_Check() to call the checker >> function instead of individual ACL checks. > > This looks pretty sane to me, although I have not done a full review. > I am disinclined to create a whole new directory for it. I think the > new function should go in src/backend/catalog/aclchk.c and be declared > in src/include/utils/acl.h. If that sounds reasonable to you, please > revise and post an updated patch. > I'm afraid of that the src/backend/catalog/aclchk.c will become overcrowding in the future. If it is ugly to deploy checker functions in separated dir/files, I think it is an idea to put it on the execMain.c, instead of ExecCheckRTEPerms(). It also suggest us where the checker functions should be deployed on the upcoming DDL reworks. In similar way, we will deploy them on src/backend/command/pg_database for example? Thanks, -- KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote: > I'd like to point out two more points are necessary to be considered > for DML permission checks in addition to ExecCheckRTPerms(). > > * DoCopy() > > Although DoCopy() is called from standard_ProcessUtility(), it performs > as DML statement, rather than DDL. It check ACL_SELECT or ACL_INSERT on > the copied table or attributes, similar to what ExecCheckRTEPerms() doing. Rather than construct a complicated API for this DML activity, why don't we just make ExecCheckRTPerms available for DoCopy to use? It seems like we could move ExecCheckRTPerms() to acl.c without too much trouble. acl.h already includes parsenodes.h and has knowledge of RangeVar's. Once DoCopy is using that, this issue resolves itself with the hook that Robert already wrote up. > * RI_Initial_Check() > > RI_Initial_Check() is a function called on ALTER TABLE command to add FK > constraints between two relations. The permission to execute this ALTER TABLE > command itself is checked on ATPrepCmd() and ATAddForeignKeyConstraint(), > so it does not affect anything on the DML permission reworks. I'm not really thrilled with how RI_Initial_Check() does it's own permissions checking and then calls SPI expecting things to 'just work'. Not sure if there's some way we could handle failure from SPI, or, if it was changed to call ExecCheckRTPerms() instead, how it would handle failure cases from there. One possible solution would be to have an additional option to ExecCheckRTPerms() which asks it to just check and return false if there's a problem, rather than unconditionally calling aclcheck_error() whenever it finds a problem. Using the same function for both the initial check in RI_Initial_Check() and then from SPI would eliminate issues where those two checks disagree for some reason, which would be good in the general case. > BTW, I guess the reason why permissions on attributes are not checked here is > that we missed it at v8.4 development. Indeed, but at the same time, this looks to be a 'fail-safe' situation. Basically, this is checking table-level permissions, which, if you have, gives you sufficient rights to SELECT against the table (any column). What this isn't doing is allowing the option of column-level permissions to be sufficient for this requirement. That's certainly an oversight in the column-level permissions handling (sorry about that), but it's not horrible- there's a workaround if RI_Initial_Check returns false already anyway. Basically, if you are using column-level privs, and you have necessary rights to do this w/ those permissions (but don't have table-level rights), it's not going to be as fast as it could be. > The most part of the checker function is cut & paste from ExecCheckRTEPerms(), > but its arguments are modified for easy invocation from other functions. As mentioned above, it seems like this would be better the other way- have the callers build RangeTbl's and then call ExecCheckRTPerms(). It feels like that approach might be more 'future-proof' as well. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] ExecutorCheckPerms() hook
2010/5/24 KaiGai Kohei : > BTW, I guess the reason why permissions on attributes are not checked here is > that we missed it at v8.4 development. That's a little worrying. Can you construct and post a test case where this results in a user-visible failure in CVS HEAD? > The attached patch provides a common checker function of DML, and modifies > ExecCheckRTPerms(), CopyTo() and RI_Initial_Check() to call the checker > function instead of individual ACL checks. This looks pretty sane to me, although I have not done a full review. I am disinclined to create a whole new directory for it. I think the new function should go in src/backend/catalog/aclchk.c and be declared in src/include/utils/acl.h. If that sounds reasonable to you, please revise and post an updated patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
(2010/05/21 1:14), Robert Haas wrote:
> In yesterday's development meeting, we talked about the possibility of
> a basic SE-PostgreSQL implementation that checks permissions only for
> DML. Greg Smith offered the opinion that this could provide much of
> the benefit of SE-PostgreSQL for many users, while being much simpler.
> In fact, SE-PostgreSQL would need to get control in just one place:
> ExecCheckRTPerms. This morning, Stephen Frost and I worked up a quick
> patch showing how we could add a hook here to let a hypothetical
> SE-PostgreSQL module get control in the relevant place. The attached
> patch also includes a toy contrib module showing how it could be used
> to enforce arbitrary security policy.
>
> I don't think that this by itself would be quite enough framework for
> a minimal SE-PostgreSQL implementation - for that, you'd probably need
> an object-labeling facility in core which SE-PostgreSQL could leverage
> - or else some other way to determine which the label associated with
> a given object - but I think that plus this would be enough.
I'd like to point out two more points are necessary to be considered
for DML permission checks in addition to ExecCheckRTPerms().
* DoCopy()
Although DoCopy() is called from standard_ProcessUtility(), it performs
as DML statement, rather than DDL. It check ACL_SELECT or ACL_INSERT on
the copied table or attributes, similar to what ExecCheckRTEPerms() doing.
* RI_Initial_Check()
RI_Initial_Check() is a function called on ALTER TABLE command to add FK
constraints between two relations. The permission to execute this ALTER TABLE
command itself is checked on ATPrepCmd() and ATAddForeignKeyConstraint(),
so it does not affect anything on the DML permission reworks.
When we add a new FK constraint, both of the existing FK and PK relations have
to satify the new constraint. So, RI_Initial_Check() tries to check whether the
PK relation has corresponding tuples to FK relation, or not.
Then, it tries to execute a secondary query using SPI_*() functions, if no
access violations are expected. Otherwise, it scans the FK relation with
per tuple checks sequentionally (see, validateForeignKeyConstraint()), but slow.
If we have an external security provider which will deny accesses on the FK/PK
relation, but the default PG checks allows it, the RI_Initial_Check() tries to
execute secondary SELECT statement, then it raises an access violation error,
although we are already allowed to execute ALTER TABLE statement.
Therefore, we also need to check DML permissions at RI_Initial_Check() to avoid
unexpected access violation error, prior to the secondary query.
BTW, I guess the reason why permissions on attributes are not checked here is
that we missed it at v8.4 development.
The attached patch provides a common checker function of DML, and modifies
ExecCheckRTPerms(), CopyTo() and RI_Initial_Check() to call the checker
function instead of individual ACL checks.
The most part of the checker function is cut & paste from ExecCheckRTEPerms(),
but its arguments are modified for easy invocation from other functions.
extern bool check_dml_permissions(Oid relOid,
Oid userId,
AclMode requiredPerms,
Bitmapset *selCols,
Bitmapset *modCols,
bool abort);
Thanks,
--
KaiGai Kohei
*** a/src/backend/commands/copy.c
--- b/src/backend/commands/copy.c
***
*** 21,26
--- 21,27
#include
#include "access/heapam.h"
+ #include "access/sysattr.h"
#include "access/xact.h"
#include "catalog/namespace.h"
#include "catalog/pg_type.h"
***
*** 41,46
--- 42,48
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
+ #include "utils/security.h"
#include "utils/snapmgr.h"
***
*** 725,733 DoCopy(const CopyStmt *stmt, const char *queryString)
List *force_notnull = NIL;
bool force_quote_all = false;
bool format_specified = false;
- AclMode required_access = (is_from ? ACL_INSERT : ACL_SELECT);
- AclMode relPerms;
- AclMode remainingPerms;
ListCell *option;
TupleDesc tupDesc;
int num_phys_attrs;
--- 727,732
***
*** 988,993 DoCopy(const CopyStmt *stmt, const char *queryString)
--- 987,996
if (stmt->relation)
{
+ Bitmapset *columnsSet = NULL;
+ List *attnums;
+ ListCell *cur;
+
Assert(!stmt->query);
cstate->queryDesc = NULL;
***
*** 998,1026 DoCopy(const CopyStmt *stmt, const char *queryString)
tupDesc = RelationGetDescr(cstate->rel);
/* Check relation permissions. */
! relPerms = pg_class_aclmask(RelationGetRelid(cstate->rel), GetUserId(),
! required_access, ACLMASK_ALL);
! remainingPerms = required_access & ~relPerms;
! if (remainingPerms != 0)
{
! /* We don'
Re: [HACKERS] ExecutorCheckPerms() hook
Robert Haas writes: > On Thu, May 20, 2010 at 12:32 PM, Tom Lane wrote: >> Hm, I think you need to ignore RT entries that have no requiredPerms >> bits set. (Not that it matters too much, unless you were proposing to >> actually commit this contrib module.) > Well, that's an easy change - just out of curiosity, how do we end up > with RT entries with no requiredPerm bits set? Inheritance child tables look like that now, per the discussion awhile back that a SELECT on the parent shouldn't require any particular permission on the individual child tables. IIRC there are some other cases involving views too, but those are probably just optimizations (ie not do duplicate permissions checks) rather than something that would result in a user-visible behavioral issue. > As for committing it, I would definitely like to commit the actual > hook. If we want the hook without the contrib module that's OK with > me, although I generally feel it's useful to have examples of how > hooks can be used, which is why I took the time to produce a working > example. +1 on committing the hook. As for the contrib module, it doesn't strike me that there's much of a use-case for it as is. I think it's enough that it's available in the -hackers archives. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
On Thu, May 20, 2010 at 12:32 PM, Tom Lane wrote: > Robert Haas writes: >> In yesterday's development meeting, we talked about the possibility of >> a basic SE-PostgreSQL implementation that checks permissions only for >> DML. Greg Smith offered the opinion that this could provide much of >> the benefit of SE-PostgreSQL for many users, while being much simpler. >> In fact, SE-PostgreSQL would need to get control in just one place: >> ExecCheckRTPerms. This morning, Stephen Frost and I worked up a quick >> patch showing how we could add a hook here to let a hypothetical >> SE-PostgreSQL module get control in the relevant place. The attached >> patch also includes a toy contrib module showing how it could be used >> to enforce arbitrary security policy. > > Hm, I think you need to ignore RT entries that have no requiredPerms > bits set. (Not that it matters too much, unless you were proposing to > actually commit this contrib module.) Well, that's an easy change - just out of curiosity, how do we end up with RT entries with no requiredPerm bits set? As for committing it, I would definitely like to commit the actual hook. If we want the hook without the contrib module that's OK with me, although I generally feel it's useful to have examples of how hooks can be used, which is why I took the time to produce a working example. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExecutorCheckPerms() hook
Robert Haas writes: > In yesterday's development meeting, we talked about the possibility of > a basic SE-PostgreSQL implementation that checks permissions only for > DML. Greg Smith offered the opinion that this could provide much of > the benefit of SE-PostgreSQL for many users, while being much simpler. > In fact, SE-PostgreSQL would need to get control in just one place: > ExecCheckRTPerms. This morning, Stephen Frost and I worked up a quick > patch showing how we could add a hook here to let a hypothetical > SE-PostgreSQL module get control in the relevant place. The attached > patch also includes a toy contrib module showing how it could be used > to enforce arbitrary security policy. Hm, I think you need to ignore RT entries that have no requiredPerms bits set. (Not that it matters too much, unless you were proposing to actually commit this contrib module.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] ExecutorCheckPerms() hook
In yesterday's development meeting, we talked about the possibility of a basic SE-PostgreSQL implementation that checks permissions only for DML. Greg Smith offered the opinion that this could provide much of the benefit of SE-PostgreSQL for many users, while being much simpler. In fact, SE-PostgreSQL would need to get control in just one place: ExecCheckRTPerms. This morning, Stephen Frost and I worked up a quick patch showing how we could add a hook here to let a hypothetical SE-PostgreSQL module get control in the relevant place. The attached patch also includes a toy contrib module showing how it could be used to enforce arbitrary security policy. I don't think that this by itself would be quite enough framework for a minimal SE-PostgreSQL implementation - for that, you'd probably need an object-labeling facility in core which SE-PostgreSQL could leverage - or else some other way to determine which the label associated with a given object - but I think that plus this would be enough. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company executor_check_perms.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
