Re: [HACKERS] [PATCH] Largeobject Access Controls (r2460)
KaiGai Kohei kai...@ak.jp.nec.com wrote: Apart from SELinux, it is quite natural to apply any access controls on binary data. If we could not have any valid access controls, users will not want to store their sensitive information, such as confidential PDF files, as a large object. Absolutely. The historical security issues for large objects immediately eliminated them as a possible place to store PDF files. -Kevin -- 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] [PATCH] Largeobject Access Controls (r2460)
I just looked over the latest version of this patch and it seems to satisfy all the issues suggested by the initial review. This looks like it's ready for a committer from a quality perspective and I'm going to mark it as such. I have a guess what some of the first points of discussion are going to be though, so might as well raise them here. This patch is 2.8K lines of code that's in a lot of places: a mix of full new functions, tweaks to existing ones, docs, regression tests, it's a well structured but somewhat heavy bit of work. One obvious questions is whether there's enough demand for access controls on large objects to justify adding the complexity involved to do so. A second thing I'm concerned about is what implications this change would have for in-place upgrades. If there's demand and it's not going to cause upgrade issues, then we just need to find a committer willing to chew on it. I think those are the main hurdles left for this patch. -- Greg Smith2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.com -- 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] [PATCH] Largeobject Access Controls (r2460)
Greg Smith wrote: I just looked over the latest version of this patch and it seems to satisfy all the issues suggested by the initial review. This looks like it's ready for a committer from a quality perspective and I'm going to mark it as such. Thanks for your efforts. I have a guess what some of the first points of discussion are going to be though, so might as well raise them here. This patch is 2.8K lines of code that's in a lot of places: a mix of full new functions, tweaks to existing ones, docs, regression tests, it's a well structured but somewhat heavy bit of work. One obvious questions is whether there's enough demand for access controls on large objects to justify adding the complexity involved to do so. At least, it is a todo item in the community: http://wiki.postgresql.org/wiki/Todo#Binary_Data Apart from SELinux, it is quite natural to apply any access controls on binary data. If we could not have any valid access controls, users will not want to store their sensitive information, such as confidential PDF files, as a large object. A second thing I'm concerned about is what implications this change would have for in-place upgrades. If there's demand and it's not going to cause upgrade issues, then we just need to find a committer willing to chew on it. I think those are the main hurdles left for this patch. I guess we need to create an empty entry with a given OID into the pg_largeobject_metadata for each large objects when we try to upgrade in-place from 8.4.x or earlier release to the upcoming release. However, no format changes in the pg_largeobject catalog, including an empty large object, so I guess we need a small amount of additional support in pg_dump to create empty metadata. I want any suggestion about here. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- 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] [PATCH] Largeobject Access Controls (r2460)
On Sun, Dec 6, 2009 at 11:19 PM, Greg Smith g...@2ndquadrant.com wrote: I just looked over the latest version of this patch and it seems to satisfy all the issues suggested by the initial review. This looks like it's ready for a committer from a quality perspective and I'm going to mark it as such. yes. i have just finished my tests and seems like the patch is working just fine... BTW, seems like KaiGai miss this comment in src/backend/catalog/pg_largeobject.c when renaming the parameter * large_object_privilege_checks is not refered here, i still doesn't like the name but we have changed it a lot of times so if anyone has a better idea now is when you have to speak -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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] [PATCH] Largeobject Access Controls (r2460)
Jaime Casanova wrote: On Sun, Dec 6, 2009 at 11:19 PM, Greg Smith g...@2ndquadrant.com wrote: I just looked over the latest version of this patch and it seems to satisfy all the issues suggested by the initial review. This looks like it's ready for a committer from a quality perspective and I'm going to mark it as such. yes. i have just finished my tests and seems like the patch is working just fine... BTW, seems like KaiGai miss this comment in src/backend/catalog/pg_largeobject.c when renaming the parameter * large_object_privilege_checks is not refered here, i still doesn't like the name but we have changed it a lot of times so if anyone has a better idea now is when you have to speak Oops, it should be fixed to lo_compat_privileges. This comment also have version number issue, so I fixed it as follows: BEFORE: /* * large_object_privilege_checks is not refered here, * because it is a compatibility option, but we don't * have ALTER LARGE OBJECT prior to the v8.5.0. */ AFTER: /* * The 'lo_compat_privileges' is not checked here, because we * don't have any access control features in the 8.4.x series * or earlier release. * So, it is not a place we can define a compatible behavior. */ Nothing are changed in other codes, including something corresponding to in-place upgrading. I'm waiting for suggestion. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com sepgsql-02-blob-8.5devel-r2461.patch.gz Description: application/gzip -- 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] [PATCH] Largeobject Access Controls (r2432)
2009/11/12 KaiGai Kohei kai...@ak.jp.nec.com: The attached patch is a revised version of large object privileges based on the feedbacks at the last commit fest. please update the patch, it's giving an error when 'make check' is trying to create template1 in initdb: creating template1 database in /home/postgres/pg_releases/pgsql/src/test/regress/./tmp_check/data/base/1 ... TRAP: FailedAssertion(!(reln-md_fd[forkNum] == ((void *)0)), File: md.c, Line: 254) child process was terminated by signal 6: Aborted Meanwhile, i will make some comments: This manual will be specific for 8.5 so i think all mentions to the version should be removed for example; +In this version, a large object has OID of its owner, access permissions +and OID of the largeobject itself. + Prior to the version 8.5.x release does not have any privilege checks on + large objects. the parameter name (large_object_privilege_checks) is confusing enough that we have to make this statements to clarify... let's think in a better less confuse name + Please note that it is not equivalent to disable all the security + checks corresponding to large objects. + For example, the literallo_import()/literal and + literallo_export/literal need superuser privileges independent + from this setting as prior versions were doing. this will not be off by default? it should be for compatibility reasons... i remember there was a discussion about that but can't remember the conclusion Mmm... One of them? the first? + The one is literalSELECT/literal. + Even if a transaction modified access rights and commit it, it is + not invisible from other transaction which already opened the large + object. The other one, the second + The other is literalUPDATE/literal. it seems there is an are that should not be there :) + + These functions are originally requires database superuser privilege, + and it allows to bypass the default database privilege checks, so + we don't need to check an obvious test twice. a typo, obviously +For largeo bjects, this privilege also allows to read from +the target large object. We have two versions of these functions one that a recieve an SnapShot parameter and other that don't... what is the rationale of this? AFAIU, the one that doesn't receive SnapShot is calling the other one with SnapShotNow, can't we simply call it that way and drop the version of the functions that doesn't have that parameter? + pg_largeobject_aclmask(Oid lobj_oid, Oid roleid, + AclMode mask, AclMaskHow how) + pg_largeobject_aclcheck(Oid lobj_oid, Oid roleid, AclMode mode) -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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] [PATCH] Largeobject Access Controls (r2432)
On Thu, Dec 3, 2009 at 12:49 PM, Jaime Casanova jcasa...@systemguards.com.ec wrote: This manual will be specific for 8.5 so i think all mentions to the version should be removed Not sure I agree on this point. We have similar mentions elsewhere. ...Robert -- 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] [PATCH] Largeobject Access Controls (r2432)
Robert Haas wrote: On Thu, Dec 3, 2009 at 12:49 PM, Jaime Casanova jcasa...@systemguards.com.ec wrote: This manual will be specific for 8.5 so i think all mentions to the version should be removed Not sure I agree on this point. We have similar mentions elsewhere. In this particular example, it's bad form because it's even possible that 8.5 will actually be 9.0. You don't want to refer to a version number that doesn't even exist for sure yet, lest it leave a loose end that needs to be cleaned up later if that number is changed before release. Rewriting in terms like in earlier versions... instead is one approach. Then people will have to manually scan earlier docs to sort that out, I know I end up doing that all the time. If you want to keep the note specific, saying in 8.4 and earlier versions [old behavior] is better than before 8.5 [old behavior] because it only mentions version numbers that are historical rather than future. -- Greg Smith2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.com
Re: [HACKERS] [PATCH] Largeobject Access Controls (r2432)
On Thu, Dec 3, 2009 at 1:23 PM, Greg Smith g...@2ndquadrant.com wrote: Robert Haas wrote: On Thu, Dec 3, 2009 at 12:49 PM, Jaime Casanova jcasa...@systemguards.com.ec wrote: This manual will be specific for 8.5 so i think all mentions to the version should be removed Not sure I agree on this point. We have similar mentions elsewhere. In this particular example, it's bad form because it's even possible that 8.5 will actually be 9.0. You don't want to refer to a version number that doesn't even exist for sure yet, lest it leave a loose end that needs to be cleaned up later if that number is changed before release. Rewriting in terms like in earlier versions... instead is one approach. Then people will have to manually scan earlier docs to sort that out, I know I end up doing that all the time. If you want to keep the note specific, saying in 8.4 and earlier versions [old behavior] is better than before 8.5 [old behavior] because it only mentions version numbers that are historical rather than future. Ah, yes, I like In 8.4 and earlier versions, or maybe earlier releases. Compare: http://www.postgresql.org/docs/8.4/static/sql-copy.html#AEN55855 http://www.postgresql.org/docs/8.4/static/runtime-config-logging.html#GUC-LOG-FILENAME ...Robert -- 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] [PATCH] Largeobject Access Controls (r2432)
Robert Haas robertmh...@gmail.com writes: On Thu, Dec 3, 2009 at 1:23 PM, Greg Smith g...@2ndquadrant.com wrote: In this particular example, it's bad form because it's even possible that 8.5 will actually be 9.0. You don't want to refer to a version number that doesn't even exist for sure yet, lest it leave a loose end that needs to be cleaned up later if that number is changed before release. Ah, yes, I like In 8.4 and earlier versions, or maybe earlier releases. Compare: Please do *not* resort to awkward constructions just to avoid one mention of the current version number. If we did decide to call the next version 9.0, the search-and-replace effort involved is not going to be measurably affected by any one usage. There are plenty already. (I did the work when we decided to call 7.5 8.0, so I know whereof I speak.) 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] [PATCH] Largeobject Access Controls (r2432)
On Thu, Dec 3, 2009 at 2:25 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Thu, Dec 3, 2009 at 1:23 PM, Greg Smith g...@2ndquadrant.com wrote: In this particular example, it's bad form because it's even possible that 8.5 will actually be 9.0. You don't want to refer to a version number that doesn't even exist for sure yet, lest it leave a loose end that needs to be cleaned up later if that number is changed before release. Ah, yes, I like In 8.4 and earlier versions, or maybe earlier releases. Compare: Please do *not* resort to awkward constructions just to avoid one mention of the current version number. If we did decide to call the next version 9.0, the search-and-replace effort involved is not going to be measurably affected by any one usage. There are plenty already. (I did the work when we decided to call 7.5 8.0, so I know whereof I speak.) I agree that search and replace isn't that hard, but I don't find the proposed construction awkward, and we have various uses of it in the docs already. Actually the COPY one is not quite clear whether it means = 7.3 or 7.3. I think we're just aiming for consistency here as much as anything. ...Robert -- 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] [PATCH] Largeobject Access Controls (r2432)
Robert Haas robertmh...@gmail.com writes: I agree that search and replace isn't that hard, but I don't find the proposed construction awkward, and we have various uses of it in the docs already. Actually the COPY one is not quite clear whether it means = 7.3 or 7.3. I think we're just aiming for consistency here as much as anything. Well, the problem is that = 8.4 is confusing as to whether it includes 8.4.n. You and I know that it does because we know we don't make feature changes in minor releases, but this is not necessarily obvious to everyone. 8.5 is much less ambiguous. 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] [PATCH] Largeobject Access Controls (r2432)
On Thu, Dec 3, 2009 at 3:33 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: I agree that search and replace isn't that hard, but I don't find the proposed construction awkward, and we have various uses of it in the docs already. Actually the COPY one is not quite clear whether it means = 7.3 or 7.3. I think we're just aiming for consistency here as much as anything. Well, the problem is that = 8.4 is confusing as to whether it includes 8.4.n. You and I know that it does because we know we don't make feature changes in minor releases, but this is not necessarily obvious to everyone. 8.5 is much less ambiguous. Ah. I would not have considered that, but it does make sense. ...Robert -- 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] [PATCH] Largeobject Access Controls (r2432)
Robert Haas wrote: I agree that search and replace isn't that hard, but I don't find the proposed construction awkward, and we have various uses of it in the docs already. Actually the COPY one is not quite clear whether it means = 7.3 or 7.3. Yeah, I wouldn't have suggested it if it made the wording particularly difficult in the process. I don't know what your issue with the COPY one is: The following syntax was used before PostgreSQL version 7.3 and is still supported I can't parse that as anything other than 7.3; now sure how can someone read that to be =? In any case, the two examples you gave are certainly good for showing the standard practices used here. Specific version numbers are strewn all about, and if there's commits mentioning 8.5 already in there one more won't hurt. -- Greg Smith2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.com -- 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] [PATCH] Largeobject Access Controls (r2432)
Jaime Casanova wrote: 2009/11/12 KaiGai Kohei kai...@ak.jp.nec.com: The attached patch is a revised version of large object privileges based on the feedbacks at the last commit fest. please update the patch, it's giving an error when 'make check' is trying to create template1 in initdb: creating template1 database in /home/postgres/pg_releases/pgsql/src/test/regress/./tmp_check/data/base/1 ... TRAP: FailedAssertion(!(reln-md_fd[forkNum] == ((void *)0)), File: md.c, Line: 254) child process was terminated by signal 6: Aborted I could not reproduce it. Could you run make clean, then make check? Various kind of patches are merged under the commit fest, so some of them changes definition of structures. If *.o files are already built based on older definitions, it may refers incorrect addresses. Meanwhile, i will make some comments: This manual will be specific for 8.5 so i think all mentions to the version should be removed for example; +In this version, a large object has OID of its owner, access permissions +and OID of the largeobject itself. + Prior to the version 8.5.x release does not have any privilege checks on + large objects. The conclusion is unclear for me. Is In the 8.4.x and prior release, ... an ambiguous expression? ^ the parameter name (large_object_privilege_checks) is confusing enough that we have to make this statements to clarify... let's think in a better less confuse name + Please note that it is not equivalent to disable all the security + checks corresponding to large objects. + For example, the literallo_import()/literal and + literallo_export/literal need superuser privileges independent + from this setting as prior versions were doing. In the last commit fest, it was named largeobject_compat_acl, but it is not preferable for Tom Lane, so he suggested to rename it into large_object_privilege_checks. Other candidates: - lo_compat_privileges (- my preference in this four) - large_object_compat_privs - lo_compat_access_control - large_object_compat_ac I think _compat_ should be contained to emphasize it is a compatibility option. this will not be off by default? it should be for compatibility reasons... i remember there was a discussion about that but can't remember the conclusion IIRC, we have no discussion about its default value, although similar topics were discussed: * what should be checked on creation of a large object? - No need to check permission on its creation. It allows everyone to create a new large object like current implementation. (Also note that this behavior may be changed in the future.) * DELETE should be checked on deletion of a large object? - No. PgSQL checks ownership of the database objects on its deletion such as DROP TABLE. The DELETE permission is checked when we delete contents of a certain database object, not the database object itself. Mmm... One of them? the first? + The one is literalSELECT/literal. + Even if a transaction modified access rights and commit it, it is + not invisible from other transaction which already opened the large + object. The other one, the second + The other is literalUPDATE/literal. I have no arguments about English expression. BTW, The one is ..., the other is ... was a statement on textbook to introduce two things. :-) it seems there is an are that should not be there :) + + These functions are originally requires database superuser privilege, + and it allows to bypass the default database privilege checks, so + we don't need to check an obvious test twice. a typo, obviously +For largeo bjects, this privilege also allows to read from +the target large object. Thanks, I see. We have two versions of these functions one that a recieve an SnapShot parameter and other that don't... what is the rationale of this? AFAIU, the one that doesn't receive SnapShot is calling the other one with SnapShotNow, can't we simply call it that way and drop the version of the functions that doesn't have that parameter? + pg_largeobject_aclmask(Oid lobj_oid, Oid roleid, + AclMode mask, AclMaskHow how) + pg_largeobject_aclcheck(Oid lobj_oid, Oid roleid, AclMode mode) We have no reason other than cosmetic rationale. In the current implementation, all the caller of pg_largeobejct_aclcheck_*() needs to provides correct snapshot including SnapshotNow when read-writable. When pg_aclmask() calls pg_largeobject_aclmask(), it is the only case that caller assumes SnapshotNow shall be applied implicitly. On the other hand, all the case when pg_largeobject_ownercheck() is called, the caller assumes SnapshotNow is applied, so we don't have multiple versions. So, I'll reorganize these APIs as follows: - pg_largeobject_aclmask_snapshot() - pg_largeobject_aclcheck_snapshot() - pg_largeobject_ownercheck()
Re: [HACKERS] [PATCH] Largeobject Access Controls (r2432)
KaiGai Kohei kai...@ak.jp.nec.com wrote: creating template1 database in /home/postgres/pg_releases/pgsql/src/test/regress/./tmp_check/data/base/1 ... TRAP: FailedAssertion(!(reln-md_fd[forkNum] == ((void *)0)), File: md.c, Line: 254) child process was terminated by signal 6: Aborted I could not reproduce it. I had the same trap before when I mistakenly used duplicated oids. Don't you add a new catalog with existing oids? src/include/catalog/duplicate_oids might be a help. Regards, --- ITAGAKI Takahiro NTT Open Source Software Center -- 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] [PATCH] Largeobject Access Controls (r2432)
Itagaki Takahiro wrote: KaiGai Kohei kai...@ak.jp.nec.com wrote: creating template1 database in /home/postgres/pg_releases/pgsql/src/test/regress/./tmp_check/data/base/1 ... TRAP: FailedAssertion(!(reln-md_fd[forkNum] == ((void *)0)), File: md.c, Line: 254) child process was terminated by signal 6: Aborted I could not reproduce it. I had the same trap before when I mistakenly used duplicated oids. Don't you add a new catalog with existing oids? src/include/catalog/duplicate_oids might be a help. Thanks, Bingo! toasting.h:DECLARE_TOAST(pg_trigger, 2336, 2337); pg_largeobject_metadata.h:CATALOG(pg_largeobject_metadata,2336) -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Largeobject Access Controls (r2460)
The attached patch is an updated revision of Largeobject Access Controls. List of updates: * rebased to the latest CVS HEAD * SGML documentation fixes: - The future version number was replaced as: In the 8.4.x series and earlier release, ... - Other strange English representations and typoes were fixed. * Fixed OID conflicts in system catalog definition. The new TOAST relation for pg_trigger used same OID number with pg_largeobject_metadata. * Fixed incorrect error code in pg_largeobject_ownercheck(). It raised _UNDEFINED_FUNCTION, but should be _UNDEFINED_OBJECT. * Renamed GUC parameter to lo_compat_privileges from large_object_privilege_checks. * pg_largeobject_aclmask() and pg_largeobject_aclcheck(), not take an argument of snapshot, were removed. Currently, the caller provide an appropriate snapshot them. Thanks, Jaime Casanova wrote: 2009/11/12 KaiGai Kohei kai...@ak.jp.nec.com: The attached patch is a revised version of large object privileges based on the feedbacks at the last commit fest. please update the patch, it's giving an error when 'make check' is trying to create template1 in initdb: creating template1 database in /home/postgres/pg_releases/pgsql/src/test/regress/./tmp_check/data/base/1 ... TRAP: FailedAssertion(!(reln-md_fd[forkNum] == ((void *)0)), File: md.c, Line: 254) child process was terminated by signal 6: Aborted Meanwhile, i will make some comments: This manual will be specific for 8.5 so i think all mentions to the version should be removed for example; +In this version, a large object has OID of its owner, access permissions +and OID of the largeobject itself. + Prior to the version 8.5.x release does not have any privilege checks on + large objects. the parameter name (large_object_privilege_checks) is confusing enough that we have to make this statements to clarify... let's think in a better less confuse name + Please note that it is not equivalent to disable all the security + checks corresponding to large objects. + For example, the literallo_import()/literal and + literallo_export/literal need superuser privileges independent + from this setting as prior versions were doing. this will not be off by default? it should be for compatibility reasons... i remember there was a discussion about that but can't remember the conclusion Mmm... One of them? the first? + The one is literalSELECT/literal. + Even if a transaction modified access rights and commit it, it is + not invisible from other transaction which already opened the large + object. The other one, the second + The other is literalUPDATE/literal. it seems there is an are that should not be there :) + + These functions are originally requires database superuser privilege, + and it allows to bypass the default database privilege checks, so + we don't need to check an obvious test twice. a typo, obviously +For largeo bjects, this privilege also allows to read from +the target large object. We have two versions of these functions one that a recieve an SnapShot parameter and other that don't... what is the rationale of this? AFAIU, the one that doesn't receive SnapShot is calling the other one with SnapShotNow, can't we simply call it that way and drop the version of the functions that doesn't have that parameter? + pg_largeobject_aclmask(Oid lobj_oid, Oid roleid, + AclMode mask, AclMaskHow how) + pg_largeobject_aclcheck(Oid lobj_oid, Oid roleid, AclMode mode) -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com sepgsql-02-blob-8.5devel-r2460.patch.gz Description: application/gzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Largeobject Access Controls (r2432)
The attached patch is a revised version of large object privileges based on the feedbacks at the last commit fest. List of updates: * Rebased to the latest CVS HEAD * Add pg_largeobject_aclcheck_snapshot() interface. In the read-only access mode, large object feature uses query's snaphot, not only SnapshotNow. This behavior also should be applied on accesses to its metadata. When it makes its access control decision, it has to fetch database acls from the system catalog. In this time, we scan the pg_largeobject_metadata with a snapshot of large object descriptor. (Note that it is SnapshotNow in read-write mode.) It enables to resolve the matter when access rights are changed during large objects are opened. * Add pg_dump support. * Replace all the largeobject by large object from user visible messages and documentation. * Remove ac_largeobject_*() routines because we decided not to share same entry points between DAC and MAC. * Add a description of large object privileges in SGML. * Add a definition of pg_largeobject_metadata in SGML. * \lo_list command supports both of v8.5 and prior version. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com sepgsql-02-blob-8.5devel-r2432.patch.gz Description: application/gzip -- 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] [PATCH] Largeobject access controls
KaiGai Kohei wrote: I have to focus on my patches with highest priority in CommitFest, but it may be possible to help reviewing the proposed patches in the off-fest season. It is illegal/undesirable for the process? No, that's absolutely fine. During commitfests patch review is needed the most, but please do help whenever you have the time. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- 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] [PATCH] Largeobject access controls
2009/10/15 Heikki Linnakangas heikki.linnakan...@enterprisedb.com: KaiGai Kohei wrote: I have to focus on my patches with highest priority in CommitFest, but it may be possible to help reviewing the proposed patches in the off-fest season. It is illegal/undesirable for the process? No, that's absolutely fine. During commitfests patch review is needed the most, but please do help whenever you have the time. I agree. I would also mention that reviewing a patch can often be done in a few hours, and there are certainly periods of downtime during a CommitFest when your own patches won't need attention. Of course, a major patch like Hot Standby needs a lot more reviewing, but that's an exceptional case, and there's really no need for you to bite off something that ambitious. Just reviewing 1 or 2 small patches per CommitFest would be much appreciated, if it's something you can manage. Reviewing patches at other times is very helpful too. Many times patch authors complain about not getting feedback between CommitFests, so anything you can pick up and give feedback on helps move the project forward (and also decreases the amount that has to get done during the next CommitFest). Thanks, ...Robert -- 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] [PATCH] Largeobject access controls
KaiGai Kohei kai...@ak.jp.nec.com writes: Tom Lane wrote: The most serious problem is that you ripped out myLargeObjectExists(), apparently because you didn't understand what it's there for. The reason it's there is to ensure that pg_dump runs don't fail. pg_dump is expected to produce a consistent dump of all large objects that existed at the time of its transaction snapshot. With this code, pg_dump would get a large object doesn't exist error on any LO that is deleted between the time of the snapshot and the time pg_dump actually tries to dump it --- which could be quite a large window in a large database. The origin of this matter is the basis of existence was changed. Our current basis is whether pg_largeobject has one or more data chunk for the given loid in the correct snapshot, or not. The newer one is whether pg_largeobject_metadata has the entry for the given loid in the SnapshotNow, or not. Which is wrong. You can certainly switch your attention as to which catalog to look in, but you can't change the snapshot that the test is referenced to. The newer basis is same as other database objects, such as functions. But why pg_dump performs correctly for these database objects? The reason pg_dump works reasonably well is that it takes locks on tables, and the other objects it dumps (such as functions) have indivisible one-row representations in the catalogs. They're either there when pg_dump looks, or they're not. What you would have here is that pg_dump would see LO data chunks when it looks into pg_largeobject using its transaction snapshot, and then its attempts to open those LO OIDs would fail because the metadata is not there anymore according to SnapshotNow. There are certainly plenty of corner-case issues in pg_dump; I've complained before that it's generally a bad idea to be migrating pg_dump functionality into internal backend routines that tend to rely on SnapshotNow. But if we change LO dumping like this, it's not going to be a corner case --- it's going to be a common race-condition failure with a window measured in many minutes if not hours. That's more than sufficient reason to reject this patch, because the added functionality isn't worth it. And pg_dump isn't even the only client that depends on the assumption that a read-only open LO is static. Moving on to lesser but still significant problems, you probably already guessed my next one: there's no pg_dump support. Hmm. I planed to add support to the pg_dump next to the serve-side enhancement. We do not commit system changes that lack necessary pg_dump support. There are some things you can leave till later, but pg_dump is not negotiable. (Otherwise, testers would be left with databases they can't dump/reload across the next forced initdb.) 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] [PATCH] Largeobject access controls
On Wed, Oct 14, 2009 at 12:02 PM, Tom Lane t...@sss.pgh.pa.us wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: Tom Lane wrote: The most serious problem is that you ripped out myLargeObjectExists(), apparently because you didn't understand what it's there for. The reason it's there is to ensure that pg_dump runs don't fail. pg_dump is expected to produce a consistent dump of all large objects that existed at the time of its transaction snapshot. With this code, pg_dump would get a large object doesn't exist error on any LO that is deleted between the time of the snapshot and the time pg_dump actually tries to dump it --- which could be quite a large window in a large database. The origin of this matter is the basis of existence was changed. Our current basis is whether pg_largeobject has one or more data chunk for the given loid in the correct snapshot, or not. The newer one is whether pg_largeobject_metadata has the entry for the given loid in the SnapshotNow, or not. Which is wrong. You can certainly switch your attention as to which catalog to look in, but you can't change the snapshot that the test is referenced to. The newer basis is same as other database objects, such as functions. But why pg_dump performs correctly for these database objects? The reason pg_dump works reasonably well is that it takes locks on tables, and the other objects it dumps (such as functions) have indivisible one-row representations in the catalogs. They're either there when pg_dump looks, or they're not. What you would have here is that pg_dump would see LO data chunks when it looks into pg_largeobject using its transaction snapshot, and then its attempts to open those LO OIDs would fail because the metadata is not there anymore according to SnapshotNow. There are certainly plenty of corner-case issues in pg_dump; I've complained before that it's generally a bad idea to be migrating pg_dump functionality into internal backend routines that tend to rely on SnapshotNow. But if we change LO dumping like this, it's not going to be a corner case --- it's going to be a common race-condition failure with a window measured in many minutes if not hours. That's more than sufficient reason to reject this patch, because the added functionality isn't worth it. And pg_dump isn't even the only client that depends on the assumption that a read-only open LO is static. Moving on to lesser but still significant problems, you probably already guessed my next one: there's no pg_dump support. Hmm. I planed to add support to the pg_dump next to the serve-side enhancement. We do not commit system changes that lack necessary pg_dump support. There are some things you can leave till later, but pg_dump is not negotiable. (Otherwise, testers would be left with databases they can't dump/reload across the next forced initdb.) As part of closing out this CommitFest, I am marking this patch as Returned with Feedback. Because the amount of work that remains to be done is so substantial, that might have been a good idea anyway, but since the clock is expiring the point is moot. ...Robert -- 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] [PATCH] Largeobject access controls
Tom Lane wrote: The newer basis is same as other database objects, such as functions. But why pg_dump performs correctly for these database objects? The reason pg_dump works reasonably well is that it takes locks on tables, and the other objects it dumps (such as functions) have indivisible one-row representations in the catalogs. They're either there when pg_dump looks, or they're not. What you would have here is that pg_dump would see LO data chunks when it looks into pg_largeobject using its transaction snapshot, and then its attempts to open those LO OIDs would fail because the metadata is not there anymore according to SnapshotNow. It needs to break down the matter more simple. This patch adds the pg_largeobject_metadata catalog, and a certain entry within the catalog is refered by multiple entries within pg_largeobject. At the viewpoint of data model, it is equivalent that any pg_largeobject tuples which share same LOID always have common copy of the metadata. (If we tries to implement this actually, it needs to keep consistency of the part of metadata for each writer operations!) In the current design, when we access a certain large object with read-only mode, query's snapshot is used, not always SnapshotNow. If we assume any data chunk has its metadata part, the metadata should be also refered from the query's snapshot. In fact, this patch stores the part of metadata within pg_largeobject_metadata. But it seems to me the principle is that same snapshot which used for data chunks should be applied to scan the metadata. So, I can agree the following approach: So I'm kind of inclined to say that the least evil solution is to apply the permissions check using the query-snapshot-time metadata row. It's definitely a debatable question though. We'd also want to make sure that the aclcheck code doesn't fail if it finds a nonexistent role ID in the ACL. In this approach, we cannot apply the current pg_largeobject_aclcheck() because it does not have an argument to deliver the preferable snapshot. So, we need to extract acl field from the tuple being visible from the appropriate snapshot, then calls aclmask() routine. The aclmask() just compares identifiers in numeris representation, so it seems to me this routine does not raise an error if it finds a nonexistent role ID from the viewpoint of SnapshotNow. aclmask(const Acl *acl, Oid roleid, Oid ownerId, AclMode mask, AclMaskHow how) It requires five arguments. The acl and ownerId can be fetched from the metadata with query's snapshot. The roleid can be given by GetUserId(). The mask and how are constant. An I missing something? We do not commit system changes that lack necessary pg_dump support. There are some things you can leave till later, but pg_dump is not negotiable. (Otherwise, testers would be left with databases they can't dump/reload across the next forced initdb.) OK, I'll add support pg_dump soon. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- 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] [PATCH] Largeobject access controls
Robert Haas wrote: On Wed, Oct 14, 2009 at 12:02 PM, Tom Lane t...@sss.pgh.pa.us wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: Tom Lane wrote: The most serious problem is that you ripped out myLargeObjectExists(), apparently because you didn't understand what it's there for. The reason it's there is to ensure that pg_dump runs don't fail. pg_dump is expected to produce a consistent dump of all large objects that existed at the time of its transaction snapshot. With this code, pg_dump would get a large object doesn't exist error on any LO that is deleted between the time of the snapshot and the time pg_dump actually tries to dump it --- which could be quite a large window in a large database. The origin of this matter is the basis of existence was changed. Our current basis is whether pg_largeobject has one or more data chunk for the given loid in the correct snapshot, or not. The newer one is whether pg_largeobject_metadata has the entry for the given loid in the SnapshotNow, or not. Which is wrong. You can certainly switch your attention as to which catalog to look in, but you can't change the snapshot that the test is referenced to. The newer basis is same as other database objects, such as functions. But why pg_dump performs correctly for these database objects? The reason pg_dump works reasonably well is that it takes locks on tables, and the other objects it dumps (such as functions) have indivisible one-row representations in the catalogs. They're either there when pg_dump looks, or they're not. What you would have here is that pg_dump would see LO data chunks when it looks into pg_largeobject using its transaction snapshot, and then its attempts to open those LO OIDs would fail because the metadata is not there anymore according to SnapshotNow. There are certainly plenty of corner-case issues in pg_dump; I've complained before that it's generally a bad idea to be migrating pg_dump functionality into internal backend routines that tend to rely on SnapshotNow. But if we change LO dumping like this, it's not going to be a corner case --- it's going to be a common race-condition failure with a window measured in many minutes if not hours. That's more than sufficient reason to reject this patch, because the added functionality isn't worth it. And pg_dump isn't even the only client that depends on the assumption that a read-only open LO is static. Moving on to lesser but still significant problems, you probably already guessed my next one: there's no pg_dump support. Hmm. I planed to add support to the pg_dump next to the serve-side enhancement. We do not commit system changes that lack necessary pg_dump support. There are some things you can leave till later, but pg_dump is not negotiable. (Otherwise, testers would be left with databases they can't dump/reload across the next forced initdb.) As part of closing out this CommitFest, I am marking this patch as Returned with Feedback. Because the amount of work that remains to be done is so substantial, that might have been a good idea anyway, but since the clock is expiring the point is moot. Could you wait for the next 24 hours please? It is unproductive to break off the discussion for a month, even if the patch will be actually commited at the December. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- 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] [PATCH] Largeobject access controls
2009/10/14 KaiGai Kohei kai...@ak.jp.nec.com: Robert Haas wrote: On Wed, Oct 14, 2009 at 12:02 PM, Tom Lane t...@sss.pgh.pa.us wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: Tom Lane wrote: The most serious problem is that you ripped out myLargeObjectExists(), apparently because you didn't understand what it's there for. The reason it's there is to ensure that pg_dump runs don't fail. pg_dump is expected to produce a consistent dump of all large objects that existed at the time of its transaction snapshot. With this code, pg_dump would get a large object doesn't exist error on any LO that is deleted between the time of the snapshot and the time pg_dump actually tries to dump it --- which could be quite a large window in a large database. The origin of this matter is the basis of existence was changed. Our current basis is whether pg_largeobject has one or more data chunk for the given loid in the correct snapshot, or not. The newer one is whether pg_largeobject_metadata has the entry for the given loid in the SnapshotNow, or not. Which is wrong. You can certainly switch your attention as to which catalog to look in, but you can't change the snapshot that the test is referenced to. The newer basis is same as other database objects, such as functions. But why pg_dump performs correctly for these database objects? The reason pg_dump works reasonably well is that it takes locks on tables, and the other objects it dumps (such as functions) have indivisible one-row representations in the catalogs. They're either there when pg_dump looks, or they're not. What you would have here is that pg_dump would see LO data chunks when it looks into pg_largeobject using its transaction snapshot, and then its attempts to open those LO OIDs would fail because the metadata is not there anymore according to SnapshotNow. There are certainly plenty of corner-case issues in pg_dump; I've complained before that it's generally a bad idea to be migrating pg_dump functionality into internal backend routines that tend to rely on SnapshotNow. But if we change LO dumping like this, it's not going to be a corner case --- it's going to be a common race-condition failure with a window measured in many minutes if not hours. That's more than sufficient reason to reject this patch, because the added functionality isn't worth it. And pg_dump isn't even the only client that depends on the assumption that a read-only open LO is static. Moving on to lesser but still significant problems, you probably already guessed my next one: there's no pg_dump support. Hmm. I planed to add support to the pg_dump next to the serve-side enhancement. We do not commit system changes that lack necessary pg_dump support. There are some things you can leave till later, but pg_dump is not negotiable. (Otherwise, testers would be left with databases they can't dump/reload across the next forced initdb.) As part of closing out this CommitFest, I am marking this patch as Returned with Feedback. Because the amount of work that remains to be done is so substantial, that might have been a good idea anyway, but since the clock is expiring the point is moot. Could you wait for the next 24 hours please? It is unproductive to break off the discussion for a month, even if the patch will be actually commited at the December. Well, I don't know that I have the deciding vote here. I don't have any super-powers to make Tom - or anyone else - review the next version of this patch for this CommitFest, the next CommitFest, or at all. In my ideal world, Tom would review every patch in his area of expertise the day it came in and commit it right away, especially the ones that implement features I happen to like. But unless I offer to pay Tom's salary, and maybe not even then, that isn't going to happen. Backing up a little bit, my understanding of the CommitFest process is that it is a time for everyone to stop working on their own patches and help review and commit the patches of others. Because everyone wants to get back to their own work, we try very hard to wrap CommitFests up in one month so that everyone can then have a full month to do their own work before the next CommitFest starts. I don't see any reason to believe that this patch is so important that we should make an exception to that policy on its behalf, and I think it's unfair of you to ask. Tom, I, and others have almost entirely put aside our own development work for the last month to focus on this CommitFest. You haven't offered to help, are now asking us to put aside our work for even longer for your benefit. Furthermore, you haven't offered to help with either of the previous two CommitFests in which I've been involved either, despite having submitted large, complex patches that required substantial reviewing effort. All that having been said, I have no intention of or desire to foreclose discussion on this patch, or any desire to
Re: [HACKERS] [PATCH] Largeobject access controls
Robert Haas wrote: 2009/10/14 KaiGai Kohei kai...@ak.jp.nec.com: Robert Haas wrote: On Wed, Oct 14, 2009 at 12:02 PM, Tom Lane t...@sss.pgh.pa.us wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: Tom Lane wrote: The most serious problem is that you ripped out myLargeObjectExists(), apparently because you didn't understand what it's there for. The reason it's there is to ensure that pg_dump runs don't fail. pg_dump is expected to produce a consistent dump of all large objects that existed at the time of its transaction snapshot. With this code, pg_dump would get a large object doesn't exist error on any LO that is deleted between the time of the snapshot and the time pg_dump actually tries to dump it --- which could be quite a large window in a large database. The origin of this matter is the basis of existence was changed. Our current basis is whether pg_largeobject has one or more data chunk for the given loid in the correct snapshot, or not. The newer one is whether pg_largeobject_metadata has the entry for the given loid in the SnapshotNow, or not. Which is wrong. You can certainly switch your attention as to which catalog to look in, but you can't change the snapshot that the test is referenced to. The newer basis is same as other database objects, such as functions. But why pg_dump performs correctly for these database objects? The reason pg_dump works reasonably well is that it takes locks on tables, and the other objects it dumps (such as functions) have indivisible one-row representations in the catalogs. They're either there when pg_dump looks, or they're not. What you would have here is that pg_dump would see LO data chunks when it looks into pg_largeobject using its transaction snapshot, and then its attempts to open those LO OIDs would fail because the metadata is not there anymore according to SnapshotNow. There are certainly plenty of corner-case issues in pg_dump; I've complained before that it's generally a bad idea to be migrating pg_dump functionality into internal backend routines that tend to rely on SnapshotNow. But if we change LO dumping like this, it's not going to be a corner case --- it's going to be a common race-condition failure with a window measured in many minutes if not hours. That's more than sufficient reason to reject this patch, because the added functionality isn't worth it. And pg_dump isn't even the only client that depends on the assumption that a read-only open LO is static. Moving on to lesser but still significant problems, you probably already guessed my next one: there's no pg_dump support. Hmm. I planed to add support to the pg_dump next to the serve-side enhancement. We do not commit system changes that lack necessary pg_dump support. There are some things you can leave till later, but pg_dump is not negotiable. (Otherwise, testers would be left with databases they can't dump/reload across the next forced initdb.) As part of closing out this CommitFest, I am marking this patch as Returned with Feedback. Because the amount of work that remains to be done is so substantial, that might have been a good idea anyway, but since the clock is expiring the point is moot. Could you wait for the next 24 hours please? It is unproductive to break off the discussion for a month, even if the patch will be actually commited at the December. Well, I don't know that I have the deciding vote here. I don't have any super-powers to make Tom - or anyone else - review the next version of this patch for this CommitFest, the next CommitFest, or at all. In my ideal world, Tom would review every patch in his area of expertise the day it came in and commit it right away, especially the ones that implement features I happen to like. But unless I offer to pay Tom's salary, and maybe not even then, that isn't going to happen. I'm sorry for this a little bit emotional response. Anyway, what I wanted to say was my wish that I wanted to conclude or make a direction for the issue (what snapshot should be applied.) It is unclear now, so I would like to continue the discussion a bit more. Backing up a little bit, my understanding of the CommitFest process is that it is a time for everyone to stop working on their own patches and help review and commit the patches of others. Because everyone wants to get back to their own work, we try very hard to wrap CommitFests up in one month so that everyone can then have a full month to do their own work before the next CommitFest starts. I don't see any reason to believe that this patch is so important that we should make an exception to that policy on its behalf, and I think it's unfair of you to ask. Tom, I, and others have almost entirely put aside our own development work for the last month to focus on this CommitFest. You haven't offered to help, are now asking us to put aside our work for even longer for your benefit. Furthermore, you haven't
Re: [HACKERS] [PATCH] Largeobject access controls
KaiGai Kohei kai...@ak.jp.nec.com writes: The attached patch is the revised one for largeobejct access controls, because it conflicted to the GRANT/REVOKE ON ALL xxx IN SCHEMA. I started to look through this patch with the hope of committing it, but found out that it's not really ready. The most serious problem is that you ripped out myLargeObjectExists(), apparently because you didn't understand what it's there for. The reason it's there is to ensure that pg_dump runs don't fail. pg_dump is expected to produce a consistent dump of all large objects that existed at the time of its transaction snapshot. With this code, pg_dump would get a large object doesn't exist error on any LO that is deleted between the time of the snapshot and the time pg_dump actually tries to dump it --- which could be quite a large window in a large database. The reason this is a significant problem and not just an easily fixable oversight is that it's not entirely clear what to do instead. We can certainly make the pure existence test use the query snapshot instead of SnapshotNow, but what about the implied permissions tests? Should we attempt to make them run using the version of the LO's ACL found in the query-snapshot-time metadata row? The trouble with that is it might refer to roles that don't exist anymore, perhaps resulting in failures down inside the ACL checking routines. It would be safer to rely on the current metadata row contents, but then we have the question of whether to allow the access if the row doesn't exist according to SnapshotNow. Now these issues arise to some extent already in pg_dump, but the current window for failure is quite short because it obtains access share locks on all the tables it will dump at the start of the run. With large objects the window in which things could have changed is very much longer. Of course, in the cases that people are most concerned about, pg_dump is running as superuser and so the actual ACL contents don't really matter anyway, so long as we don't fail outright before getting to the check. So I'm kind of inclined to say that the least evil solution is to apply the permissions check using the query-snapshot-time metadata row. It's definitely a debatable question though. We'd also want to make sure that the aclcheck code doesn't fail if it finds a nonexistent role ID in the ACL. Moving on to lesser but still significant problems, you probably already guessed my next one: there's no pg_dump support. If the system tracks owner and ACL for large objects, pg_dump *must* be prepared to dump that information. It'd also be worthwhile to teach pg_dump that in servers = 8.5, it can look in the metadata catalog to make the list of LO OIDs instead of having to do a SELECT DISTINCT from pg_largeobject. I notice that the patch decides to change the pg_description classoid for LO comments from pg_largeobject's OID to pg_largeobject_metadata's. This will break existing clients that look at pg_description (eg, pg_dump and psql, plus any other clients that have any intelligence about comments, for instance it probably breaks pgAdmin). And there's just not a lot of return that I can see. I agree that using pg_largeobject_metadata would be more consistent given the new catalog layout, but I'm inclined to think we should stick to the old convention on compatibility grounds. Given that choice, for consistency we'd better also use pg_largeobject's OID not pg_largeobject_metadata's in pg_shdepend entries for LOs. In the category of lesser issues that have still got to be fixed: * You need to pay more attention to updating comments. For example your changes to LargeObjectCreate render its header comment a complete lie, but you didn't change the comment. (And what is the purpose of renaming it to CreateLargeObject, and similarly for the adjacent routines? You didn't change the API meaningfully, so there is no reason to break calling code that way.) * The documentation needs work too, eg a new system catalog requires a page in catalogs.sgml, and I'll bet there's a few references to large objects and/or permissions that need to be updated. * largeobject is not an English word. The occurrences in user-visible messages and documentation must get changed to large object. I've got mixed emotions even about using it in code identifiers, although we certainly aren't going to rename pg_largeobject, so anything that's named in parallel to that should stay as-is. In the same vein, acl is not a word we want to expose to users, so largeobject_check_acl is doubly bad as a GUC variable name. Perhaps large_object_privilege_checks would do. * I'm not really happy with the ac_largeobject_foo shim layer, and would frankly prefer to rip it out and put those tests inline. It's poorly thought out IMO --- eg, what the heck is that cascade boolean --- and doesn't look like any of the rest of the Postgres code stylistically, and it makes the calling code look different from similar
Re: [HACKERS] [PATCH] Largeobject access controls
Tom Lane wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: The attached patch is the revised one for largeobejct access controls, because it conflicted to the GRANT/REVOKE ON ALL xxx IN SCHEMA. I started to look through this patch with the hope of committing it, but found out that it's not really ready. The most serious problem is that you ripped out myLargeObjectExists(), apparently because you didn't understand what it's there for. The reason it's there is to ensure that pg_dump runs don't fail. pg_dump is expected to produce a consistent dump of all large objects that existed at the time of its transaction snapshot. With this code, pg_dump would get a large object doesn't exist error on any LO that is deleted between the time of the snapshot and the time pg_dump actually tries to dump it --- which could be quite a large window in a large database. The reason this is a significant problem and not just an easily fixable oversight is that it's not entirely clear what to do instead. We can certainly make the pure existence test use the query snapshot instead of SnapshotNow, but what about the implied permissions tests? Should we attempt to make them run using the version of the LO's ACL found in the query-snapshot-time metadata row? The trouble with that is it might refer to roles that don't exist anymore, perhaps resulting in failures down inside the ACL checking routines. It would be safer to rely on the current metadata row contents, but then we have the question of whether to allow the access if the row doesn't exist according to SnapshotNow. Now these issues arise to some extent already in pg_dump, but the current window for failure is quite short because it obtains access share locks on all the tables it will dump at the start of the run. With large objects the window in which things could have changed is very much longer. Of course, in the cases that people are most concerned about, pg_dump is running as superuser and so the actual ACL contents don't really matter anyway, so long as we don't fail outright before getting to the check. So I'm kind of inclined to say that the least evil solution is to apply the permissions check using the query-snapshot-time metadata row. It's definitely a debatable question though. We'd also want to make sure that the aclcheck code doesn't fail if it finds a nonexistent role ID in the ACL. The origin of this matter is the basis of existence was changed. Our current basis is whether pg_largeobject has one or more data chunk for the given loid in the correct snapshot, or not. The newer one is whether pg_largeobject_metadata has the entry for the given loid in the SnapshotNow, or not. The newer basis is same as other database objects, such as functions. But why pg_dump performs correctly for these database objects? In my understanding, because it reads the system catalog directly in the query snapshot. So, it will be visible, if concurrent transaction dropped a function to be backed up. Now, pg_dump uses libpq's large object interface which internally uses loread()/lowrite(). If pg_dump fetches data chunks from the system catalog, it seems to me the matter is reasonably solvable. My assumption is that you're not talking about a generic situation when a certain database object is unavailable even if we can find it within the system catalog, apart from large object backups. For example, we can easily produce a similar behavior when we tries to use a function which can be found in pg_proc, but concurrent transaction already removed it. I don't believe PostgreSQL guarantees equivalence between the visibility of system catalog and the availability of the related database object. So, is it the simplest approach to patch on the pg_dump? Moving on to lesser but still significant problems, you probably already guessed my next one: there's no pg_dump support. If the system tracks owner and ACL for large objects, pg_dump *must* be prepared to dump that information. It'd also be worthwhile to teach pg_dump that in servers = 8.5, it can look in the metadata catalog to make the list of LO OIDs instead of having to do a SELECT DISTINCT from pg_largeobject. Hmm. I planed to add support to the pg_dump next to the serve-side enhancement. If both of patches are necessary soon, I'll include it in this phase. I notice that the patch decides to change the pg_description classoid for LO comments from pg_largeobject's OID to pg_largeobject_metadata's. This will break existing clients that look at pg_description (eg, pg_dump and psql, plus any other clients that have any intelligence about comments, for instance it probably breaks pgAdmin). And there's just not a lot of return that I can see. I agree that using pg_largeobject_metadata would be more consistent given the new catalog layout, but I'm inclined to think we should stick to the old convention on compatibility grounds. Given that choice, for consistency
Re: [HACKERS] [PATCH] Largeobject access controls
The attached patch is the revised one for largeobejct access controls, because it conflicted to the GRANT/REVOKE ON ALL xxx IN SCHEMA. No other changes are here. Please apply this one, instead of the older revision (r2354). Thanks, $ diffstat /home/kaigai/RPMS/SOURCES/sepgsql-02-blob-8.5devel-r2362.patch.gz doc/src/sgml/config.sgml | 28 + doc/src/sgml/ref/allfiles.sgml|1 doc/src/sgml/ref/alter_large_object.sgml | 75 doc/src/sgml/ref/grant.sgml |8 doc/src/sgml/ref/revoke.sgml |6 doc/src/sgml/reference.sgml |1 src/backend/catalog/Makefile |6 src/backend/catalog/aclchk.c | 294 ++ src/backend/catalog/dependency.c | 14 src/backend/catalog/pg_largeobject.c | 406 + src/backend/catalog/pg_shdepend.c |5 src/backend/commands/alter.c |5 src/backend/commands/comment.c|7 src/backend/commands/tablecmds.c |1 src/backend/libpq/be-fsstubs.c| 49 +- src/backend/parser/gram.y | 21 + src/backend/storage/large_object/inv_api.c| 108 +---! src/backend/tcop/utility.c|3 src/backend/utils/adt/acl.c |5 src/backend/utils/misc/guc.c | 10 src/backend/utils/misc/postgresql.conf.sample |1 src/bin/psql/large_obj.c | 10 src/include/catalog/dependency.h |1 src/include/catalog/indexing.h|3 src/include/catalog/pg_largeobject.h |4 src/include/catalog/pg_largeobject_metadata.h | 68 src/include/nodes/parsenodes.h|1 src/include/utils/acl.h |6 src/test/regress/expected/privileges.out | 206 + src/test/regress/expected/sanity_check.out|3 src/test/regress/sql/privileges.sql | 84 + 31 files changed, 1167 insertions(+), 80 deletions(-), 193 modifications(!) KaiGai Kohei wrote: Tom Lane wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: I rebased the largeobject access controls patch to the CVS HEAD because of the patch confliction to the default ACL patch. Quick comment on this --- I think that using a syscache for large objects is probably not a good idea. There is no provision in the catcache code for limiting the cache size anymore, and that means that anybody who touches a large number of large objects is going to blow out memory. We removed the old cache limit code because that seemed most sensible for the use of the caches for regular catalog objects, but I don't think LOs will have the same characteristics with respect to either number of objects or locality of access. The attached patch is the revised largeobject access controls. It replaced any usage of system cache for pg_largeobject_metadata. In this patch, we basically follow the manner in the pg_tablespace which also does not have its own system cache. For example, it needs to open the pg_largeobject_metadata relation with AccessShareLock when pg_largeobject_aclcheck() is called, as pg_tablespace_aclcheck() doing. $ diffstat sepgsql-02-blob-8.5devel-r2354.patch.gz doc/src/sgml/config.sgml | 28 + doc/src/sgml/ref/allfiles.sgml|1 doc/src/sgml/ref/alter_large_object.sgml | 75 doc/src/sgml/ref/grant.sgml |8 doc/src/sgml/ref/revoke.sgml |6 doc/src/sgml/reference.sgml |1 src/backend/catalog/Makefile |6 src/backend/catalog/aclchk.c | 294 ++ src/backend/catalog/dependency.c | 14 src/backend/catalog/pg_largeobject.c | 406 + src/backend/catalog/pg_shdepend.c |5 src/backend/commands/alter.c |5 src/backend/commands/comment.c|7 src/backend/commands/tablecmds.c |1 src/backend/libpq/be-fsstubs.c| 49 +- src/backend/parser/gram.y | 20 + src/backend/storage/large_object/inv_api.c| 108 +---! src/backend/tcop/utility.c|3 src/backend/utils/adt/acl.c |5 src/backend/utils/misc/guc.c | 10 src/backend/utils/misc/postgresql.conf.sample |1 src/bin/psql/large_obj.c | 10 src/include/catalog/dependency.h |1 src/include/catalog/indexing.h|3 src/include/catalog/pg_largeobject.h |4 src/include/catalog/pg_largeobject_metadata.h | 68 src/include/nodes/parsenodes.h|1 src/include/utils/acl.h |6
Re: [HACKERS] [PATCH] Largeobject access controls
KaiGai Kohei kai...@ak.jp.nec.com writes: I rebased the largeobject access controls patch to the CVS HEAD because of the patch confliction to the default ACL patch. Quick comment on this --- I think that using a syscache for large objects is probably not a good idea. There is no provision in the catcache code for limiting the cache size anymore, and that means that anybody who touches a large number of large objects is going to blow out memory. We removed the old cache limit code because that seemed most sensible for the use of the caches for regular catalog objects, but I don't think LOs will have the same characteristics with respect to either number of objects or locality of access. 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] [PATCH] Largeobject access controls
Tom Lane wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: I rebased the largeobject access controls patch to the CVS HEAD because of the patch confliction to the default ACL patch. Quick comment on this --- I think that using a syscache for large objects is probably not a good idea. There is no provision in the catcache code for limiting the cache size anymore, and that means that anybody who touches a large number of large objects is going to blow out memory. We removed the old cache limit code because that seemed most sensible for the use of the caches for regular catalog objects, but I don't think LOs will have the same characteristics with respect to either number of objects or locality of access. Are you talking about syscache.c? I added a syscache entry for pg_largeobject_metadata, not pg_largeobject which contains data chunks. The pg_largeobject_metadata is a smaller catalog than most of system catalogs, such as pg_class. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] [PATCH] Largeobject access controls
KaiGai Kohei escribió: I added a syscache entry for pg_largeobject_metadata, not pg_largeobject which contains data chunks. The pg_largeobject_metadata is a smaller catalog than most of system catalogs, such as pg_class. The point is not the size of the cache entries, but the number of them. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- 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] [PATCH] Largeobject access controls
Alvaro Herrera wrote: KaiGai Kohei escribió: I added a syscache entry for pg_largeobject_metadata, not pg_largeobject which contains data chunks. The pg_largeobject_metadata is a smaller catalog than most of system catalogs, such as pg_class. The point is not the size of the cache entries, but the number of them. If so, I'll replace any routines which use LARGEOBJECTOID cache. Please wait for the revised patch. Is there any other comment? -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] [PATCH] Largeobject access controls
Tom Lane wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: I rebased the largeobject access controls patch to the CVS HEAD because of the patch confliction to the default ACL patch. Quick comment on this --- I think that using a syscache for large objects is probably not a good idea. There is no provision in the catcache code for limiting the cache size anymore, and that means that anybody who touches a large number of large objects is going to blow out memory. We removed the old cache limit code because that seemed most sensible for the use of the caches for regular catalog objects, but I don't think LOs will have the same characteristics with respect to either number of objects or locality of access. The attached patch is the revised largeobject access controls. It replaced any usage of system cache for pg_largeobject_metadata. In this patch, we basically follow the manner in the pg_tablespace which also does not have its own system cache. For example, it needs to open the pg_largeobject_metadata relation with AccessShareLock when pg_largeobject_aclcheck() is called, as pg_tablespace_aclcheck() doing. $ diffstat sepgsql-02-blob-8.5devel-r2354.patch.gz doc/src/sgml/config.sgml | 28 + doc/src/sgml/ref/allfiles.sgml|1 doc/src/sgml/ref/alter_large_object.sgml | 75 doc/src/sgml/ref/grant.sgml |8 doc/src/sgml/ref/revoke.sgml |6 doc/src/sgml/reference.sgml |1 src/backend/catalog/Makefile |6 src/backend/catalog/aclchk.c | 294 ++ src/backend/catalog/dependency.c | 14 src/backend/catalog/pg_largeobject.c | 406 + src/backend/catalog/pg_shdepend.c |5 src/backend/commands/alter.c |5 src/backend/commands/comment.c|7 src/backend/commands/tablecmds.c |1 src/backend/libpq/be-fsstubs.c| 49 +- src/backend/parser/gram.y | 20 + src/backend/storage/large_object/inv_api.c| 108 +---! src/backend/tcop/utility.c|3 src/backend/utils/adt/acl.c |5 src/backend/utils/misc/guc.c | 10 src/backend/utils/misc/postgresql.conf.sample |1 src/bin/psql/large_obj.c | 10 src/include/catalog/dependency.h |1 src/include/catalog/indexing.h|3 src/include/catalog/pg_largeobject.h |4 src/include/catalog/pg_largeobject_metadata.h | 68 src/include/nodes/parsenodes.h|1 src/include/utils/acl.h |6 src/test/regress/expected/privileges.out | 206 + src/test/regress/expected/sanity_check.out|3 src/test/regress/sql/privileges.sql | 84 + 31 files changed, 1166 insertions(+), 80 deletions(-), 193 modifications(!) -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com sepgsql-02-blob-8.5devel-r2354.patch.gz Description: application/gzip -- 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] [PATCH] Largeobject access controls
I rebased the largeobject access controls patch to the CVS HEAD
because of the patch confliction to the default ACL patch.
The only difference was a switch-case statement was moved from
shdepDropOwned() to RemoveRoleFromObjectACL(), so we had to
change the point to be patched.
I don't think this change needs whole of reviewing again.
Actual changes are as follows:
$ diff -up r2333.patch r2353.patch
snip
+*** base/src/backend/catalog/aclchk.c Tue Oct 6 08:45:40 2009
+--- blob/src/backend/catalog/aclchk.c Tue Oct 6 09:44:51 2009
@@ -310,9 +310,21 @@ diff -Nrpc base/src/backend/catalog/aclc
case ACL_OBJECT_NAMESPACE:
foreach(cell, objnames)
{
+*** RemoveRoleFromObjectACL(Oid roleid, Oid
+*** 1156,1161
+--- 1184,1192
+ case LanguageRelationId:
+ istmt.objtype = ACL_OBJECT_LANGUAGE;
+ break;
++ case LargeObjectMetadataRelationId:
++ istmt.objtype = ACL_OBJECT_LARGEOBJECT;
++ break;
+ case NamespaceRelationId:
+ istmt.objtype = ACL_OBJECT_NAMESPACE;
+ break;
*** ExecGrant_Language(InternalGrant *istmt)
snip
-*** base/src/backend/catalog/pg_shdepend.c Thu Jun 18 10:20:52 2009
blob/src/backend/catalog/pg_shdepend.c Thu Sep 24 10:46:38 2009
+*** base/src/backend/catalog/pg_shdepend.c Tue Oct 6 08:45:40 2009
+--- blob/src/backend/catalog/pg_shdepend.c Tue Oct 6 09:44:51 2009
***
-*** 24,29
24,30
- #include catalog/pg_conversion.h
+*** 25,30
+--- 25,31
#include catalog/pg_database.h
+ #include catalog/pg_default_acl.h
#include catalog/pg_language.h
+ #include catalog/pg_largeobject_metadata.h
#include catalog/pg_namespace.h
#include catalog/pg_operator.h
#include catalog/pg_proc.h
-*** shdepDropOwned(List *roleids, DropBehavi
-*** 1210,1215
1211,1219
- case LanguageRelationId:
- istmt.objtype = ACL_OBJECT_LANGUAGE;
- break;
-+ case LargeObjectMetadataRelationId:
-+ istmt.objtype = ACL_OBJECT_LARGEOBJECT;
-+ break;
- case NamespaceRelationId:
- istmt.objtype = ACL_OBJECT_NAMESPACE;
- break;
*** shdepReassignOwned(List *roleids, Oid ne
-*** 1365,1370
1369,1378
+*** 1332,1337
+--- 1333,1342
AlterLanguageOwner_oid(sdepForm-objid, newrole);
break;
@@ -1178,9 +1178,9 @@ diff -Nrpc base/src/backend/catalog/pg_s
+ AlterLargeObjectOwner(sdepForm-objid, newrole);
+ break;
+
- default:
- elog(ERROR, unexpected classid %d, sdepForm-classid);
- break;
+ case DefaultAclRelationId:
+ /*
+* Ignore default ACLs; they should be handled by
diff -Nrpc base/src/backend/commands/alter.c blob/src/backend/commands/alter.c
*** base/src/backend/commands/alter.c Sat Jan 3 13:01:35 2009
--- blob/src/backend/commands/alter.c Thu Sep 24 10:46:38 2009
--
OSS Platform Development Division, NEC
KaiGai Kohei kai...@ak.jp.nec.com
sepgsql-02-blob-8.5devel-r2353.patch.gz
Description: application/gzip
--
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] [PATCH] Largeobject access controls
2009/9/24 KaiGai Kohei kai...@ak.jp.nec.com: The attached patch is revised from the previous revision at the following points: - The largeobject_compat_acl is renamed to largeobject_check_acl. Its default is on, and turning it off means the largeobject stuff performs in compatible mode for the v8.4.x or prior releases. - Notification messages were eliminated at the compatible mode. It always allows to bypass ACL checks without any warnings. a few minor points: + For example, the literallo_import()/literal and + literallo_export/literal need superuser privileges independent + from this setting, as if the prior version doing. that should read as prior versions were doing? and you're still using pg_largeobject_meta in some comments in src/include/catalog/pg_largeobject_metadata.h besides that it looks good to me... i will mark the patch as ready for committer -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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] [PATCH] Largeobject access controls
Thanks for your comments. Jaime Casanova wrote: 2009/9/24 KaiGai Kohei kai...@ak.jp.nec.com: The attached patch is revised from the previous revision at the following points: - The largeobject_compat_acl is renamed to largeobject_check_acl. Its default is on, and turning it off means the largeobject stuff performs in compatible mode for the v8.4.x or prior releases. - Notification messages were eliminated at the compatible mode. It always allows to bypass ACL checks without any warnings. a few minor points: + For example, the literallo_import()/literal and + literallo_export/literal need superuser privileges independent + from this setting, as if the prior version doing. that should read as prior versions were doing? Yes. It seems to me same meanings, but it is unnatural for you, isn't it? and you're still using pg_largeobject_meta in some comments in src/include/catalog/pg_largeobject_metadata.h Fixed, The attached patch is revised based on the comments. Below is the diffset from the previous revision (r2328). [kai...@saba ~]$ diff -u r2328.patch r2333.patch --- r2328.patch 2009-09-28 16:37:19.0 +0900 +++ r2333.patch 2009-09-28 16:36:55.0 +0900 @@ -1,6 +1,6 @@ diff -Nrpc base/doc/src/sgml/config.sgml blob/doc/src/sgml/config.sgml *** base/doc/src/sgml/config.sgml Thu Sep 24 08:43:31 2009 blob/doc/src/sgml/config.sgml Fri Sep 25 09:00:55 2009 +--- blob/doc/src/sgml/config.sgml Mon Sep 28 16:32:50 2009 *** dynamic_library_path = 'C:\tools\postgre *** 4797,4802 --- 4797,4830 @@ -27,7 +27,7 @@ + checks corresponding to largeobjects. + For example, the literallo_import()/literal and + literallo_export/literal need superuser privileges independent -+ from this setting, as if the prior version doing. ++ from this setting as prior versions were doing. +/para +para + It is literalon/literal by default. @@ -1990,21 +1990,21 @@ DECLARE_UNIQUE_INDEX(pg_namespace_oid_index, 2685, on pg_namespace using btree(oid oid_ops)); diff -Nrpc base/src/include/catalog/pg_largeobject_metadata.h blob/src/include/catalog/pg_largeobject_metadata.h *** base/src/include/catalog/pg_largeobject_metadata.h Thu Jan 1 09:00:00 1970 blob/src/include/catalog/pg_largeobject_metadata.h Fri Sep 25 09:00:55 2009 +--- blob/src/include/catalog/pg_largeobject_metadata.h Mon Sep 28 16:31:11 2009 *** *** 0 --- 1,67 + /*- + * -+ * pg_largeobject_meta.h -+ * definition of the system largeobject_meta relation (pg_largeobject_meta) ++ * pg_largeobject_metadata.h ++ * definition of the system largeobject_metadata relation (pg_largeobject_metadata) + * along with the relation's initial contents. + * + * + * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * -+ * $PostgreSQL: pgsql/src/include/catalog/pg_largeobject_meta.h,v 1.24 2009/01/01 17:23:57 momjian Exp $ ++ * $PostgreSQL: pgsql/src/include/catalog/pg_largeobject_metadata.h,v 1.24 2009/01/01 17:23:57 momjian Exp $ + * + * NOTES + * the genbki.sh script reads this file and generates .bki @@ -2012,14 +2012,14 @@ + * + *- + */ -+ #ifndef PG_LARGEOBJECT_META_H -+ #define PG_LARGEOBJECT_META_H ++ #ifndef PG_LARGEOBJECT_METADATA_H ++ #define PG_LARGEOBJECT_METADATA_H + + #include catalog/genbki.h + + /* -+ * pg_largeobject definition. cpp turns this into -+ * typedef struct FormData_pg_largeobject_meta ++ * pg_largeobject_metadata definition. cpp turns this into ++ * typedef struct FormData_pg_largeobject_metadata + * + */ + #define LargeObjectMetadataRelationId 2336 @@ -2060,7 +2060,7 @@ + extern void ac_largeobject_export(Oid loid, const char *filename); + extern void ac_largeobject_import(Oid loid, const char *filename); + -+ #endif /* PG_LARGEOBJECT_META_H */ ++ #endif /* PG_LARGEOBJECT_METADATA_H */ diff -Nrpc base/src/include/nodes/parsenodes.h blob/src/include/nodes/parsenodes.h *** base/src/include/nodes/parsenodes.hThu Sep 24 08:43:31 2009 --- blob/src/include/nodes/parsenodes.hThu Sep 24 09:04:49 2009 -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com sepgsql-02-blob-8.5devel-r2333.patch.gz Description: application/gzip -- 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] [PATCH] Largeobject access controls
2009/9/24 KaiGai Kohei kai...@ak.jp.nec.com: The attached patch is revised from the previous revision at the following points: Jaime, Do you think this is Ready for Committer review at this point? If so, please mark it that way; otherwise, what do you think are the outstanding issues? ...Robert -- 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] [PATCH] Largeobject access controls
2009/9/24 KaiGai Kohei kai...@ak.jp.nec.com: Example) postgres=# SET SESSION AUTHORIZATION ymj; SET postgres= SELECT loread(lo_open(16453, x'4'::int), 20); ERROR: permission denied for largeobject 16453 postgres=# SET largeobject_compat_acl = on; enables compatible mode SET (Only superuser can set it) postgres=# SET SESSION AUTHORIZATION ymj; SET postgres= SELECT loread(lo_open(16453, x'4'::int), 20); NOTICE: permission denied for largeobject 16453 dose not prevent it i'm not really sure the warnings are worth the trouble but if you want to do it then the NOTICE version should use another message... i'm not comfortable with a permission denied that is simply ignored... -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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] [PATCH] Largeobject access controls
Jaime Casanova wrote: 2009/9/23 KaiGai Kohei kai...@ak.jp.nec.com: Jaime, KaiGai Kohei wrote: | ALTER LARGE OBJECT is working, but now that we can change the owner of | a LO we should be able to see who the actual owner is... i mean we | should add an owner column in \dl for psql (maybe \dl+) and maybe an | lo_owner() function. | | I would like to buy your idea at the revised patch. Now we don't have xxx_owner() function for other database objects, such as tables, procedures and so on. good point, but we have has_xx_privileges() family of functions but i think we can add them later if needed... Yes, the has_xx_privileges() family should be added later or soon. Anyway, what I wanted to say is we have no special functions to show owner of the database objects. Jaime Casanova wrote: Do you think the largeobject_compat_acl is a meaningful name, instead? maybe something like largeobject_security_controls? It is important to contain a term of compat which means compatible, because this GUC does not disables all the security checks. The v8.4.x checks superuser privilege on using lo_import()/lo_export(). It is also checked in this patch, even if the GUC is turned on. The purpose of the GUC is to provide compatible behavior, not to provide a stuff to turn on/off all the security features in largeobjects. that's why the section in the postgresql.conf is called VERSION/PLATFORM COMPATIBILITY and the subsection Previous PostgreSQL Versions we have other compatibilty GUC and no ones of those has the term compat in it... Indeed, I put the largeobject_compat_acl in the compatibility section, but no other GUCs have compat prefix/suffix. It seems to me fair enough. So, I still prefer the largeobject_compat_acl. maybe enhanced_largeobjects_checks or enhanced_lo_checks or make the GUC an enum and name it largeobject_control_checks with posible values basic and enhanced But, isn't the enhanced tumid expression? It just applies native database privilege mechanism on largeobjects, as if it does on other objects. An other alternative is largeobject_check_acl. What's your feeling? Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- 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] [PATCH] Largeobject access controls
Jaime Casanova wrote: 2009/9/24 KaiGai Kohei kai...@ak.jp.nec.com: Example) postgres=# SET SESSION AUTHORIZATION ymj; SET postgres= SELECT loread(lo_open(16453, x'4'::int), 20); ERROR: permission denied for largeobject 16453 postgres=# SET largeobject_compat_acl = on; enables compatible mode SET (Only superuser can set it) postgres=# SET SESSION AUTHORIZATION ymj; SET postgres= SELECT loread(lo_open(16453, x'4'::int), 20); NOTICE: permission denied for largeobject 16453 dose not prevent it i'm not really sure the warnings are worth the trouble but if you want to do it then the NOTICE version should use another message... i'm not comfortable with a permission denied that is simply ignored... It is not a significant issue whether the compatible mode allows users to bypass ACL checks with or without any notifications. Which is the preferable one? 1. It always generates notifications whenever access violations. 2. It generates notifications at the first violation only. 3. It never generates notifications. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- 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] [PATCH] Largeobject access controls
The attached patch is revised from the previous revision at the following points: - The largeobject_compat_acl is renamed to largeobject_check_acl. Its default is on, and turning it off means the largeobject stuff performs in compatible mode for the v8.4.x or prior releases. - Notification messages were eliminated at the compatible mode. It always allows to bypass ACL checks without any warnings. Thanks, $ diffstat sepgsql-02-blob-8.5devel-r2328.patch.gz doc/src/sgml/config.sgml | 28 ++ doc/src/sgml/ref/allfiles.sgml|1 doc/src/sgml/ref/alter_large_object.sgml | 75 + doc/src/sgml/ref/grant.sgml |8 doc/src/sgml/ref/revoke.sgml |6 doc/src/sgml/reference.sgml |1 src/backend/catalog/Makefile |6 src/backend/catalog/aclchk.c | 249 ++ src/backend/catalog/dependency.c | 14 + src/backend/catalog/pg_largeobject.c | 357 +++!! src/backend/catalog/pg_shdepend.c |8 src/backend/commands/alter.c |5 src/backend/commands/comment.c| 11 src/backend/commands/tablecmds.c |1 src/backend/libpq/be-fsstubs.c| 49 +-- src/backend/parser/gram.y | 20 + src/backend/storage/large_object/inv_api.c| 115 ++---!!! src/backend/tcop/utility.c|3 src/backend/utils/adt/acl.c |5 src/backend/utils/cache/syscache.c| 13 src/backend/utils/misc/guc.c | 10 src/backend/utils/misc/postgresql.conf.sample |1 src/bin/psql/large_obj.c | 10 src/include/catalog/dependency.h |1 src/include/catalog/indexing.h|3 src/include/catalog/pg_largeobject_metadata.h | 67 src/include/nodes/parsenodes.h|1 src/include/utils/acl.h |6 src/include/utils/syscache.h |1 src/test/regress/expected/privileges.out | 206 +++ src/test/regress/expected/sanity_check.out|3 src/test/regress/sql/privileges.sql | 85 ++ 32 files changed, 976 insertions(+), 77 deletions(-), 316 modifications(!) KaiGai Kohei wrote: Jaime Casanova wrote: 2009/9/23 KaiGai Kohei kai...@ak.jp.nec.com: Jaime, KaiGai Kohei wrote: | ALTER LARGE OBJECT is working, but now that we can change the owner of | a LO we should be able to see who the actual owner is... i mean we | should add an owner column in \dl for psql (maybe \dl+) and maybe an | lo_owner() function. | | I would like to buy your idea at the revised patch. Now we don't have xxx_owner() function for other database objects, such as tables, procedures and so on. good point, but we have has_xx_privileges() family of functions but i think we can add them later if needed... Yes, the has_xx_privileges() family should be added later or soon. Anyway, what I wanted to say is we have no special functions to show owner of the database objects. Jaime Casanova wrote: Do you think the largeobject_compat_acl is a meaningful name, instead? maybe something like largeobject_security_controls? It is important to contain a term of compat which means compatible, because this GUC does not disables all the security checks. The v8.4.x checks superuser privilege on using lo_import()/lo_export(). It is also checked in this patch, even if the GUC is turned on. The purpose of the GUC is to provide compatible behavior, not to provide a stuff to turn on/off all the security features in largeobjects. that's why the section in the postgresql.conf is called VERSION/PLATFORM COMPATIBILITY and the subsection Previous PostgreSQL Versions we have other compatibilty GUC and no ones of those has the term compat in it... Indeed, I put the largeobject_compat_acl in the compatibility section, but no other GUCs have compat prefix/suffix. It seems to me fair enough. So, I still prefer the largeobject_compat_acl. maybe enhanced_largeobjects_checks or enhanced_lo_checks or make the GUC an enum and name it largeobject_control_checks with posible values basic and enhanced But, isn't the enhanced tumid expression? It just applies native database privilege mechanism on largeobjects, as if it does on other objects. An other alternative is largeobject_check_acl. What's your feeling? Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com sepgsql-02-blob-8.5devel-r2328.patch.gz Description: application/gzip -- 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] [PATCH] Largeobject access controls
Jaime, KaiGai Kohei wrote: | ALTER LARGE OBJECT is working, but now that we can change the owner of | a LO we should be able to see who the actual owner is... i mean we | should add an owner column in \dl for psql (maybe \dl+) and maybe an | lo_owner() function. | | I would like to buy your idea at the revised patch. Now we don't have xxx_owner() function for other database objects, such as tables, procedures and so on. I agree to enhance \dl command for psql, however, it seems to me that using SELECT from system catalogs are normal manner in pgsql, instead of lo_owner() function. Jaime Casanova wrote: Do you think the largeobject_compat_acl is a meaningful name, instead? maybe something like largeobject_security_controls? It is important to contain a term of compat which means compatible, because this GUC does not disables all the security checks. The v8.4.x checks superuser privilege on using lo_import()/lo_export(). It is also checked in this patch, even if the GUC is turned on. The purpose of the GUC is to provide compatible behavior, not to provide a stuff to turn on/off all the security features in largeobjects. So, I still prefer the largeobject_compat_acl. Now, I'm revising the patch as follows: - pg_largeobject_meta is renamed to pg_largeobject_metadata - The GUC of largeobject_compat_dac is renamed to largeobject_compat_acl - psql supports \dl to show owner of the largeobject - add documentation for the GUC, and add it to the postgresql.conf.sample Any comments? Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- 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] [PATCH] Largeobject access controls
2009/9/23 KaiGai Kohei kai...@ak.jp.nec.com: Now, I'm revising the patch as follows: - pg_largeobject_meta is renamed to pg_largeobject_metadata - The GUC of largeobject_compat_dac is renamed to largeobject_compat_acl - psql supports \dl to show owner of the largeobject - add documentation for the GUC, and add it to the postgresql.conf.sample I still don't like the idea of having a GUC that turns off a substantial part of the security system. Am I the only one? ...Robert -- 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] [PATCH] Largeobject access controls
Robert Haas wrote: 2009/9/23 KaiGai Kohei kai...@ak.jp.nec.com: Now, I'm revising the patch as follows: - pg_largeobject_meta is renamed to pg_largeobject_metadata - The GUC of largeobject_compat_dac is renamed to largeobject_compat_acl - psql supports \dl to show owner of the largeobject - add documentation for the GUC, and add it to the postgresql.conf.sample I still don't like the idea of having a GUC that turns off a substantial part of the security system. Am I the only one? I also think you are right from the viewpoint of the security. Smaller number of pitfall on configuration is basically better. However, we already released v8.4.x or prior versions without ACL checks on largeobjects, so it is necessary to pay attentions for existing SQLs which expect no ACL checks on largeobject accesses. The purpose of the GUC is to provide users compatible bahaviors on largeobjects. BTW, here is one idea. When the largeobject_compat_acl is turned on, it allows to bypass ACL checks, but it generates warning message for violated accesses. User can notice his SQL should be fixed at the v8.5.x or later. (It is similar to the permissive-mode in SELinux.) Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- 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] [PATCH] Largeobject access controls
Now, I'm revising the patch as follows: - pg_largeobject_meta is renamed to pg_largeobject_metadata - The GUC of largeobject_compat_dac is renamed to largeobject_compat_acl - psql supports \dl to show owner of the largeobject - add documentation for the GUC, and add it to the postgresql.conf.sample Here is the revised patch. The \dl command in psql is enhanced as follows: postgres=# \dl Large objects ID | Owner | Description ---++- 16448 | kaigai | 16449 | kaigai | test large object 1 16450 | ymj| 16451 | ymj| 16452 | ymj| test large object 2 16453 | tak| 16454 | tak| (7 rows) The functionality of largeobject_compat_acl (which was named as largeobject_compat_dac at the previous patch) is changed a bit. Its default is 'off'. When it is turned on, access control features on largeobjects performs with the compatible mode. It also checks access permissions on largeobjects, but its results are ignored with notification messages to inform access violation. It means the v8.5.x provides access control on largeobjects in default, although it also provides compatible mode. However, it should be informed to users their SQL to be revised. Example) postgres=# SET SESSION AUTHORIZATION ymj; SET postgres= SELECT loread(lo_open(16453, x'4'::int), 20); ERROR: permission denied for largeobject 16453 postgres=# SET largeobject_compat_acl = on; enables compatible mode SET (Only superuser can set it) postgres=# SET SESSION AUTHORIZATION ymj; SET postgres= SELECT loread(lo_open(16453, x'4'::int), 20); NOTICE: permission denied for largeobject 16453 dose not prevent it loread \x255044462d312e350d0a25b5b5b5b50d0a312030 (1 row) Thanks, $ diffstat sepgsql-02-blob-8.5devel-r2327.patch.gz doc/src/sgml/config.sgml | 25 + doc/src/sgml/ref/allfiles.sgml|1 doc/src/sgml/ref/alter_large_object.sgml | 75 + doc/src/sgml/ref/grant.sgml |8 doc/src/sgml/ref/revoke.sgml |6 doc/src/sgml/reference.sgml |1 src/backend/catalog/Makefile |6 src/backend/catalog/aclchk.c | 249 ++ src/backend/catalog/dependency.c | 14 + src/backend/catalog/pg_largeobject.c | 354 ++-! src/backend/catalog/pg_shdepend.c |8 src/backend/commands/alter.c |5 src/backend/commands/comment.c| 11 src/backend/commands/tablecmds.c |1 src/backend/libpq/be-fsstubs.c| 49 +-- src/backend/parser/gram.y | 20 + src/backend/storage/large_object/inv_api.c| 115 ++---!!! src/backend/tcop/utility.c|3 src/backend/utils/adt/acl.c |5 src/backend/utils/cache/syscache.c| 13 src/backend/utils/misc/guc.c | 10 src/backend/utils/misc/postgresql.conf.sample |1 src/bin/psql/large_obj.c | 10 src/include/catalog/dependency.h |1 src/include/catalog/indexing.h|3 src/include/catalog/pg_largeobject_metadata.h | 67 src/include/nodes/parsenodes.h|1 src/include/utils/acl.h |6 src/include/utils/syscache.h |1 src/test/regress/expected/privileges.out | 204 ++ src/test/regress/expected/sanity_check.out|3 src/test/regress/sql/privileges.sql | 83 ++ 32 files changed, 966 insertions(+), 73 deletions(-), 320 modifications(!) -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com sepgsql-02-blob-8.5devel-r2327.patch.gz Description: application/gzip -- 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] [PATCH] Largeobject access controls
2009/9/23 KaiGai Kohei kai...@ak.jp.nec.com: Jaime, KaiGai Kohei wrote: | ALTER LARGE OBJECT is working, but now that we can change the owner of | a LO we should be able to see who the actual owner is... i mean we | should add an owner column in \dl for psql (maybe \dl+) and maybe an | lo_owner() function. | | I would like to buy your idea at the revised patch. Now we don't have xxx_owner() function for other database objects, such as tables, procedures and so on. good point, but we have has_xx_privileges() family of functions but i think we can add them later if needed... Jaime Casanova wrote: Do you think the largeobject_compat_acl is a meaningful name, instead? maybe something like largeobject_security_controls? It is important to contain a term of compat which means compatible, because this GUC does not disables all the security checks. The v8.4.x checks superuser privilege on using lo_import()/lo_export(). It is also checked in this patch, even if the GUC is turned on. The purpose of the GUC is to provide compatible behavior, not to provide a stuff to turn on/off all the security features in largeobjects. that's why the section in the postgresql.conf is called VERSION/PLATFORM COMPATIBILITY and the subsection Previous PostgreSQL Versions we have other compatibilty GUC and no ones of those has the term compat in it... So, I still prefer the largeobject_compat_acl. maybe enhanced_largeobjects_checks or enhanced_lo_checks or make the GUC an enum and name it largeobject_control_checks with posible values basic and enhanced -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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] [PATCH] Largeobject access controls
Jaime, Thanks for your reviewing. Jaime Casanova wrote: 2009/9/6 KaiGai Kohei kai...@ak.jp.nec.com: The attached patch is an update of largeobject access controls. it applies fine (just 3 succeded hunks), compiles and passes regression tests... ALTER LARGE OBJECT is working, but now that we can change the owner of a LO we should be able to see who the actual owner is... i mean we should add an owner column in \dl for psql (maybe \dl+) and maybe an lo_owner() function. I would like to buy your idea at the revised patch. the GRANTs (and REVOKEs) work just fine too... Another question is what we want that only the LO owner can delete it (via lo_unlink)? It is the standard manner in PostgreSQL. The native database privilege mechanism checks ownership of the database object when a user tries to drop the object. I don't think we have good reason that only largeobejct has its own principle. Please note that it is different from ACL_DELETE permission, because it does not control privilege to drop the table itself. It allows a certain user to delete tuples within the table. another one, is it possible for us to have a CREATE LARGE OBJECT statement? the reason for this is: 1) it is a little ugly to use the OID in ALTER/GRANT/REVOKE statements, in a create statement we can assign a name to the LO 2) it could be more consistent with other ALTER/GRANT/REVOKE that acts over objects created with CREATE while large objects are created via lo_import() which makes obvious that are just records in meta-data table (hope this is understandable) It is not difficult to implement the named-largeobejct. However, the matter is whether it is really wanted feature to decorate a largeobject, or not. At least, access controls on largeobjects is a todo item, but symbolic identifier on largeobject is not the one. BTW, we already have COMMENT ON LARGE OBJECT loid statement now. How should it be handled, if a largeobject has its symbolic identifier? It adds a new guc variable to turn on/off compatible behavior in largeobejct access controls. largeobject_compat_dac = [on | off] (default: off) If the variable is turned on, all the new access control features on largeobjects are bypassed, as if v8.4.x or prior did. the GUC works as intended but i don't like the name, it is not very meaningful for those of us that doesn't know what DAC or MAC are... Do you think the largeobject_compat_acl is a meaningful name, instead? another point, you really have to put the GUC in the postgresql.conf file if you hope people know about it ;) it is not documented either I'll add a description about the GUC at the document. Is it also necessary to add postgresql.conf.sample?? About the code... - I don't like the name pg_largeobject_meta why not pg_largeobject_acl (put here any other name you like)? or there was a reason for that choose? My preference is a pair of pg_largeobject and pg_largeobject_data (which has an identical structure to the current pg_largeobject). However, it seems to me the pg_largeobject_acl is an incorrect name, because it also contains the owner identifier which is a part of metadata, but not an acl. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] [PATCH] Largeobject access controls
On Tue, Sep 22, 2009 at 7:23 AM, KaiGai Kohei kai...@kaigai.gr.jp wrote: another one, is it possible for us to have a CREATE LARGE OBJECT statement? the reason for this is: 1) it is a little ugly to use the OID in ALTER/GRANT/REVOKE statements, in a create statement we can assign a name to the LO 2) it could be more consistent with other ALTER/GRANT/REVOKE that acts over objects created with CREATE while large objects are created via lo_import() which makes obvious that are just records in meta-data table (hope this is understandable) It is not difficult to implement the named-largeobejct. However, the matter is whether it is really wanted feature to decorate a largeobject, or not. yeah! i don't think this will be implemented soon nor that you had to do it... just want to mention it for later discussion because it seems like natural evolution of the feature It adds a new guc variable to turn on/off compatible behavior in largeobejct access controls. largeobject_compat_dac = [on | off] (default: off) If the variable is turned on, all the new access control features on largeobjects are bypassed, as if v8.4.x or prior did. the GUC works as intended but i don't like the name, it is not very meaningful for those of us that doesn't know what DAC or MAC are... Do you think the largeobject_compat_acl is a meaningful name, instead? maybe something like largeobject_security_controls? another point, you really have to put the GUC in the postgresql.conf file if you hope people know about it ;) it is not documented either I'll add a description about the GUC at the document. Is it also necessary to add postgresql.conf.sample?? i think so, it's a compatibility issue so it must be easily findable (don't know if that word actually exists, though :) About the code... - I don't like the name pg_largeobject_meta why not pg_largeobject_acl (put here any other name you like)? or there was a reason for that choose? My preference is a pair of pg_largeobject and pg_largeobject_data (which has an identical structure to the current pg_largeobject). However, it seems to me the pg_largeobject_acl is an incorrect name, because it also contains the owner identifier which is a part of metadata, but not an acl. have anyone better ideas about the name? if not, then go with pg_largeobject_meta -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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] [PATCH] Largeobject access controls
On Tue, Sep 22, 2009 at 1:29 PM, Jaime Casanova jcasa...@systemguards.com.ec wrote: have anyone better ideas about the name? if not, then go with pg_largeobject_meta I don't think there's anything wrong with calling it metadata. That seems to leave the door open to future things we might want to do, without restricting it to security-related settings or whatever. But I don't see any reason to abbreviate it either - I'd go with pg_largeobject_metadata rather than just pg_largeobject_meta. ...Robert -- 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] [PATCH] Largeobject access controls
On Tue, Sep 22, 2009 at 12:53 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Sep 22, 2009 at 1:29 PM, Jaime Casanova jcasa...@systemguards.com.ec wrote: have anyone better ideas about the name? if not, then go with pg_largeobject_meta I don't think there's anything wrong with calling it metadata. That seems to leave the door open to future things we might want to do, without restricting it to security-related settings or whatever. But I don't see any reason to abbreviate it either - I'd go with pg_largeobject_metadata rather than just pg_largeobject_meta. ok! metadata is self explanatory meta is not... at least not for someone that his mother tongue is spanish and think of meta like in goal ;) -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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] [PATCH] Largeobject access controls
2009/9/6 KaiGai Kohei kai...@ak.jp.nec.com: The attached patch is an update of largeobject access controls. it applies fine (just 3 succeded hunks), compiles and passes regression tests... ALTER LARGE OBJECT is working, but now that we can change the owner of a LO we should be able to see who the actual owner is... i mean we should add an owner column in \dl for psql (maybe \dl+) and maybe an lo_owner() function. the GRANTs (and REVOKEs) work just fine too... Another question is what we want that only the LO owner can delete it (via lo_unlink)? another one, is it possible for us to have a CREATE LARGE OBJECT statement? the reason for this is: 1) it is a little ugly to use the OID in ALTER/GRANT/REVOKE statements, in a create statement we can assign a name to the LO 2) it could be more consistent with other ALTER/GRANT/REVOKE that acts over objects created with CREATE while large objects are created via lo_import() which makes obvious that are just records in meta-data table (hope this is understandable) It adds a new guc variable to turn on/off compatible behavior in largeobejct access controls. largeobject_compat_dac = [on | off] (default: off) If the variable is turned on, all the new access control features on largeobjects are bypassed, as if v8.4.x or prior did. the GUC works as intended but i don't like the name, it is not very meaningful for those of us that doesn't know what DAC or MAC are... another point, you really have to put the GUC in the postgresql.conf file if you hope people know about it ;) it is not documented either About the code... - I don't like the name pg_largeobject_meta why not pg_largeobject_acl (put here any other name you like)? or there was a reason for that choose? -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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] [PATCH] Largeobject access controls
Jamie, * Robert Haas (robertmh...@gmail.com) wrote: Jaime Casanova jcasa...@systemguards.com.ec - Largeobject access controls How is the review for this coming? Do you have any thoughts regarding the new GUC? Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Largeobject access controls
On Fri, Sep 18, 2009 at 8:29 PM, Stephen Frost sfr...@snowman.net wrote: Jamie, How is the review for this coming? Do you have any thoughts regarding the new GUC? Hi, sorry... these have been hard days... i'm just starting reviewing -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- 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] [PATCH] Largeobject access controls
The attached patch is an update of largeobject access controls. It adds a new guc variable to turn on/off compatible behavior in largeobejct access controls. largeobject_compat_dac = [on | off] (default: off) If the variable is turned on, all the new access control features on largeobjects are bypassed, as if v8.4.x or prior did. (Note that lo_import()/lo_export() checks client's superuser privilege independent from the guc setting, because it has been checked prior to the v8.4.x.) [kai...@saba blob]$ psql postgres psql (8.5devel) Type help for help. postgres=# SET SESSION AUTHORIZATION ymj; SET postgres= SELECT loread(lo_open(1234, x'4'::int), 100); ERROR: permission denied for largeobject 1234 postgres= \c - psql (8.5devel) You are now connected to database postgres. postgres=# SET largeobject_compat_dac TO on;-- set compatible largeobject SET postgres=# SET SESSION AUTHORIZATION ymj; SET postgres= SELECT loread(lo_open(1234, x'4'::int), 100); loread \x (1 row) KaiGai Kohei wrote: Robert Haas wrote: 2009/9/3 KaiGai Kohei kai...@ak.jp.nec.com: KaiGai Kohei wrote: Alvaro Herrera wrote: Tom Lane wrote: KaiGai Kohei kai...@kaigai.gr.jp writes: BTW, currently, the default ACL of largeobject allows anything for owner and nothing for world. Do you have any comment for the default behavior? Mph. I think the backlash will be too great. You have to leave the default behavior the same as it is now, ie, world access. BTW as a default it is pretty bad. Should we have a GUC var to set the default LO permissions? It seems to me a reasonable idea in direction. However, it might be better to add a GUC variable to turn on/off LO permission feature, not only default permissions. It allows us to control whether the privilege mechanism should perform in backward compatible, or not. Now we have two options: 1. A GUC variable to set the default largeobject permissions. SET largeobject_default_acl = [ ro | rw | none ] - ro : read-only - rw : read-writable - none : nothing It can control the default acl which is applied when NULL is set on the pg_largeobject_meta.lomacl. However, lo_unlink() checks ownership on the largeobject, so it is not enough compatible with v8.4.x or prior. 2. A GUC veriable to turn on/off largeobject permissions. SET largeobject_compat_dac = [ on | off ] When the variable is turned on, largeobject dac permission check is not applied as the v8.4.x or prior version did. So, the variable is named compat which means compatible behavior. It also does not check ownership on lo_unlink(). My preference is the second approach. What's your opinion? I prefer the first. There's little harm in letting users set the default permissions for themselves, but a GUC that controls system-wide behavior will have to be something only superusers can money with, and that seems like it will reduce usability. I don't intend to allow session users to set up their default acl. Both operation should be always system-wide. If a normal user can change the default acl, it is also equivalent he can grant all permissions to public on all the largeobject with its acl being NULL. Note that PostgreSQL does not set up a certain ACLs on its creation time, so NULL is assigned. The default ACL means an alternarive set of permissions, when it is NULL. Why couldn't lo_unlink() just check write privilege? Because it is inconsistent behavior. PostgreSQL checks its ownership on dropping a certain database objects, such as tabls, procedures and so on. It seems to me quite strange, if only largeobject checks writer permission to drop itself. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com sepgsql-02-blob-8.5devel-r2283.patch.gz Description: application/gzip -- 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] [PATCH] Largeobject access controls
KaiGai Kohei wrote: Alvaro Herrera wrote: Tom Lane wrote: KaiGai Kohei kai...@kaigai.gr.jp writes: BTW, currently, the default ACL of largeobject allows anything for owner and nothing for world. Do you have any comment for the default behavior? Mph. I think the backlash will be too great. You have to leave the default behavior the same as it is now, ie, world access. BTW as a default it is pretty bad. Should we have a GUC var to set the default LO permissions? It seems to me a reasonable idea in direction. However, it might be better to add a GUC variable to turn on/off LO permission feature, not only default permissions. It allows us to control whether the privilege mechanism should perform in backward compatible, or not. Now we have two options: 1. A GUC variable to set the default largeobject permissions. SET largeobject_default_acl = [ ro | rw | none ] - ro : read-only - rw : read-writable - none : nothing It can control the default acl which is applied when NULL is set on the pg_largeobject_meta.lomacl. However, lo_unlink() checks ownership on the largeobject, so it is not enough compatible with v8.4.x or prior. 2. A GUC veriable to turn on/off largeobject permissions. SET largeobject_compat_dac = [ on | off ] When the variable is turned on, largeobject dac permission check is not applied as the v8.4.x or prior version did. So, the variable is named compat which means compatible behavior. It also does not check ownership on lo_unlink(). My preference is the second approach. What's your opinion? Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- 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] [PATCH] Largeobject access controls
2009/9/3 KaiGai Kohei kai...@ak.jp.nec.com: KaiGai Kohei wrote: Alvaro Herrera wrote: Tom Lane wrote: KaiGai Kohei kai...@kaigai.gr.jp writes: BTW, currently, the default ACL of largeobject allows anything for owner and nothing for world. Do you have any comment for the default behavior? Mph. I think the backlash will be too great. You have to leave the default behavior the same as it is now, ie, world access. BTW as a default it is pretty bad. Should we have a GUC var to set the default LO permissions? It seems to me a reasonable idea in direction. However, it might be better to add a GUC variable to turn on/off LO permission feature, not only default permissions. It allows us to control whether the privilege mechanism should perform in backward compatible, or not. Now we have two options: 1. A GUC variable to set the default largeobject permissions. SET largeobject_default_acl = [ ro | rw | none ] - ro : read-only - rw : read-writable - none : nothing It can control the default acl which is applied when NULL is set on the pg_largeobject_meta.lomacl. However, lo_unlink() checks ownership on the largeobject, so it is not enough compatible with v8.4.x or prior. 2. A GUC veriable to turn on/off largeobject permissions. SET largeobject_compat_dac = [ on | off ] When the variable is turned on, largeobject dac permission check is not applied as the v8.4.x or prior version did. So, the variable is named compat which means compatible behavior. It also does not check ownership on lo_unlink(). My preference is the second approach. What's your opinion? I prefer the first. There's little harm in letting users set the default permissions for themselves, but a GUC that controls system-wide behavior will have to be something only superusers can money with, and that seems like it will reduce usability. Why couldn't lo_unlink() just check write privilege? ...Robert -- 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] [PATCH] Largeobject access controls
Robert Haas wrote: 2009/9/3 KaiGai Kohei kai...@ak.jp.nec.com: KaiGai Kohei wrote: Alvaro Herrera wrote: Tom Lane wrote: KaiGai Kohei kai...@kaigai.gr.jp writes: BTW, currently, the default ACL of largeobject allows anything for owner and nothing for world. Do you have any comment for the default behavior? Mph. I think the backlash will be too great. You have to leave the default behavior the same as it is now, ie, world access. BTW as a default it is pretty bad. Should we have a GUC var to set the default LO permissions? It seems to me a reasonable idea in direction. However, it might be better to add a GUC variable to turn on/off LO permission feature, not only default permissions. It allows us to control whether the privilege mechanism should perform in backward compatible, or not. Now we have two options: 1. A GUC variable to set the default largeobject permissions. SET largeobject_default_acl = [ ro | rw | none ] - ro : read-only - rw : read-writable - none : nothing It can control the default acl which is applied when NULL is set on the pg_largeobject_meta.lomacl. However, lo_unlink() checks ownership on the largeobject, so it is not enough compatible with v8.4.x or prior. 2. A GUC veriable to turn on/off largeobject permissions. SET largeobject_compat_dac = [ on | off ] When the variable is turned on, largeobject dac permission check is not applied as the v8.4.x or prior version did. So, the variable is named compat which means compatible behavior. It also does not check ownership on lo_unlink(). My preference is the second approach. What's your opinion? I prefer the first. There's little harm in letting users set the default permissions for themselves, but a GUC that controls system-wide behavior will have to be something only superusers can money with, and that seems like it will reduce usability. I don't intend to allow session users to set up their default acl. Both operation should be always system-wide. If a normal user can change the default acl, it is also equivalent he can grant all permissions to public on all the largeobject with its acl being NULL. Note that PostgreSQL does not set up a certain ACLs on its creation time, so NULL is assigned. The default ACL means an alternarive set of permissions, when it is NULL. Why couldn't lo_unlink() just check write privilege? Because it is inconsistent behavior. PostgreSQL checks its ownership on dropping a certain database objects, such as tabls, procedures and so on. It seems to me quite strange, if only largeobject checks writer permission to drop itself. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- 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] [PATCH] Largeobject access controls
The attached patch is the revised version of largeobject access controls.
It reverts pg_largeobject system catalog, and adds new pg_largeobject_meta
system catalog to store the owner identifier and its ACLs.
The definition of pg_largeobject_meta:
#define LargeObjectMetaRelationId 2336
CATALOG(pg_largeobject_meta,2336)
{
Oid lomowner; /* OID of the largeobject owner */
aclitem lomacl[1]; /* access permissions */
} FormData_pg_largeobject_meta;
The pg_largeobject system catalog is still used to store data chunks of
largeobjects, and its pg_largeobject.loid is associated with OID of the
pg_largeobject_meta system catalog.
* It also supports case handling in DROP ROLE and REASSIGN/DROP OWNED
using existing dependency mechanism.
* A new ALTER LARGE OBJECT oid OWNER TO user statement was added.
* Permission checks on creation of largeobjects are dropped. It implicitly
allows everyone to create a new largeobject.
(CREATE USER LARGEOBJECT/NOLARGEOBJECT is also dropped.)
* The default ACL allows public to read/write new largeobjects as long as
owner does not revoke permissions. (MEMO: It might be configurable
using GUC whether the default allows public to read/write, or not.)
[Performance measurement]
We measured the time to execute \lo_import with two large files (the one
is well compressible, the other is not so) and \lo_export them.
In the result, it seems to me there are no significant regression here.
* Environment
CPU: Pentium4 3.20GHz
Mem: 512MB
Kernel: 2.6.30-6.fc12.i586
PostgreSQL configuration: all parameters are in default.
* Base PostgreSQL
- Import/Export an uncompressible file
[kai...@saba ~]$ time -p psql postgres -c '\lo_import 512MB_Rnd'
lo_import 16386
real 132.33
user 1.01
sys 5.06
[kai...@saba ~]$ time -p psql postgres -c '\lo_export 16386 /dev/null'
lo_export
real 77.57
user 0.79
sys 3.76
- Import/Export well compressible file
[kai...@saba ~]$ time -p psql postgres -c '\lo_import 512MB_Zero'
lo_import 16387
real 45.84
user 0.91
sys 5.38
[kai...@saba ~]$ time -p psql postgres -c '\lo_export 16387 /dev/null'
lo_export
real 13.51
user 0.62
sys 2.98
* with Largeobject access control patch
- Import/Export an uncompressible file
[kai...@saba ~]$ time -p psql postgres -c '\lo_import 512MB_Rnd'
lo_import 16384
real 132.49
user 1.13
sys 5.10
[kai...@saba ~]$ time -p psql postgres -c '\lo_export 16384 /dev/null'
lo_export
real 76.14
user 0.81
sys 3.63
- Import/Export well compressible file
[kai...@saba ~]$ time -p psql postgres -c '\lo_import 512MB_Zero'
lo_import 16385
real 44.21
user 0.91
sys 5.51
[kai...@saba ~]$ time -p psql postgres -c '\lo_export 16385 /dev/null'
lo_export
real 14.27
user 0.66
sys 3.11
Thanks,
[kai...@saba blob]$ diffstat sepgsql-02-blob-8.5devel-r2272.patch.gz
doc/src/sgml/ref/allfiles.sgml |1
doc/src/sgml/ref/alter_large_object.sgml | 75
doc/src/sgml/ref/grant.sgml|8
doc/src/sgml/ref/revoke.sgml |6
doc/src/sgml/reference.sgml|1
src/backend/catalog/Makefile |6
src/backend/catalog/aclchk.c | 247 ++
src/backend/catalog/dependency.c | 14 +
src/backend/catalog/pg_largeobject.c | 270 +!!!
src/backend/catalog/pg_shdepend.c |8
src/backend/commands/alter.c |5
src/backend/commands/comment.c | 14 !
src/backend/commands/tablecmds.c |1
src/backend/libpq/be-fsstubs.c | 49 ++--
src/backend/parser/gram.y | 20 ++
src/backend/storage/large_object/inv_api.c | 115 +++-
src/backend/tcop/utility.c |3
src/backend/utils/adt/acl.c|5
src/backend/utils/cache/syscache.c | 13 +
src/include/catalog/dependency.h |1
src/include/catalog/indexing.h |3
src/include/catalog/pg_largeobject_meta.h | 66 +++
src/include/nodes/parsenodes.h |1
src/include/utils/acl.h|6
src/include/utils/syscache.h |1
src/test/regress/expected/privileges.out | 162 +
src/test/regress/expected/sanity_check.out |3
src/test/regress/sql/privileges.sql| 65 ++
28 files changed, 859 insertions(+), 73 deletions(-), 237 modifications(!)
--
OSS Platform Development Division, NEC
KaiGai Kohei kai...@ak.jp.nec.com
sepgsql-02-blob-8.5devel-r2272.patch.gz
Description: application/gzip
--
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] [PATCH] Largeobject access controls
Tom Lane wrote: KaiGai Kohei kai...@kaigai.gr.jp writes: BTW, currently, the default ACL of largeobject allows anything for owner and nothing for world. Do you have any comment for the default behavior? Mph. I think the backlash will be too great. You have to leave the default behavior the same as it is now, ie, world access. BTW as a default it is pretty bad. Should we have a GUC var to set the default LO permissions? -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- 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] [PATCH] Largeobject access controls
Alvaro Herrera wrote: Tom Lane wrote: KaiGai Kohei kai...@kaigai.gr.jp writes: BTW, currently, the default ACL of largeobject allows anything for owner and nothing for world. Do you have any comment for the default behavior? Mph. I think the backlash will be too great. You have to leave the default behavior the same as it is now, ie, world access. BTW as a default it is pretty bad. Should we have a GUC var to set the default LO permissions? It seems to me a reasonable idea in direction. However, it might be better to add a GUC variable to turn on/off LO permission feature, not only default permissions. It allows us to control whether the privilege mechanism should perform in backward compatible, or not. -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- 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] [PATCH] Largeobject access controls
Itagaki Takahiro wrote:
KaiGai Kohei kai...@ak.jp.nec.com wrote:
The pg_largeobject system catalog is reworked to manage its metadata.
CATALOG(pg_largeobject,2613)
{
Oid loowner;/* OID of the owner */
Oid lochunk;/* OID of the data chunks */
aclitem loacl[1]; /* access permissions */
} FormData_pg_largeobject;
Actual data chunks are stored in the toast relation of pg_largeobject,
and its chunk_id is stored in the pg_largeobject.lochunk.
A bit acrobatic, but insteresting idea.
I have some random comments:
* Do you measure performance of the new LO implementation?
AFAIK, Users expect performance benefits to LO; TOASTed
bytea slows down when the size of data is 100KB or greater
even if they don't use random seeks.
Not yet. Can you recommend commonly-used workload?
* We might take care of DROP ROLE and REASSIGN/DROP OWNED.
Or, we might have large object without owner.
It might require full-scanning of pg_largeobject table,
but we can accept it because the size of pg_largeobject
will be smaller; we have actual data out of the table.
I think it should be implemented using the dependency mechanism.
It requires full-scanning on the pg_shdepend tables, but it has
been accepted.
* Don't we also need ALTER LARGE OBJECT oid OWNER TO user
for consistency?
Agreed. It will be also necessary to implement REASSIGN OWNED.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei kai...@ak.jp.nec.com
--
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] [PATCH] Largeobject access controls
KaiGai Kohei kai...@ak.jp.nec.com writes: The attached patch provides access control features on largeobject. This patch adds the ownership and two permissions (SELECT and UPDATE) on largeobjects. The two permissions controls reader and writer accesses to the largeobejcts. What about DELETE permissions? Should we track that separately from UPDATE? The CREATE USER/ROLE statement got a new option: LARGEOBJECT/NOLARGEOBJECT. It enables to controls whether the user can create a largeobject, or not. I don't think this is necessary or appropriate. The pg_largeobject system catalog is reworked to manage its metadata. Actual data chunks are stored in the toast relation of pg_largeobject, This seems like a very confusing design, and one that (a) breaks existing code to no purpose, (b) will greatly complicate in-place upgrade. Instead of abusing a toast relation to do something nonstandard, keep pg_largeobject as it is now and add a new, separate catalog that carries ownership and permissions info for each LO OID. 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] [PATCH] Largeobject access controls
Tom Lane wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: The attached patch provides access control features on largeobject. This patch adds the ownership and two permissions (SELECT and UPDATE) on largeobjects. The two permissions controls reader and writer accesses to the largeobejcts. What about DELETE permissions? Should we track that separately from UPDATE? PostgreSQL checks ownership of the database object when user tries to drop it. This patch also add pg_largeobject_ownercheck() on lo_unlink(). The CREATE USER/ROLE statement got a new option: LARGEOBJECT/NOLARGEOBJECT. It enables to controls whether the user can create a largeobject, or not. I don't think this is necessary or appropriate. What should control privilege to create a new largeobject? Or, it implicitly allows everyone to create a new one? The pg_largeobject system catalog is reworked to manage its metadata. Actual data chunks are stored in the toast relation of pg_largeobject, This seems like a very confusing design, and one that (a) breaks existing code to no purpose, (b) will greatly complicate in-place upgrade. Instead of abusing a toast relation to do something nonstandard, keep pg_largeobject as it is now and add a new, separate catalog that carries ownership and permissions info for each LO OID. It was the original design just before the first commit fest. :-) I don't have any reason to oppose to it. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] [PATCH] Largeobject access controls
KaiGai Kohei kai...@kaigai.gr.jp writes: Tom Lane wrote: What about DELETE permissions? Should we track that separately from UPDATE? PostgreSQL checks ownership of the database object when user tries to drop it. This patch also add pg_largeobject_ownercheck() on lo_unlink(). Oh, okay, that will do fine. The CREATE USER/ROLE statement got a new option: LARGEOBJECT/NOLARGEOBJECT. It enables to controls whether the user can create a largeobject, or not. I don't think this is necessary or appropriate. What should control privilege to create a new largeobject? Or, it implicitly allows everyone to create a new one? We have not had any requests to keep people from creating LOs, so I think we can just implicitly allow everyone. If we were going to try to manage it, I don't think a role attribute is a very good solution. It's not grantable or inheritable, it can't be managed per-database, etc. So I'd leave this out until there's some popular demand. 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] [PATCH] Largeobject access controls
The CREATE USER/ROLE statement got a new option: LARGEOBJECT/NOLARGEOBJECT. It enables to controls whether the user can create a largeobject, or not. I don't think this is necessary or appropriate. What should control privilege to create a new largeobject? Or, it implicitly allows everyone to create a new one? We have not had any requests to keep people from creating LOs, so I think we can just implicitly allow everyone. If we were going to try to manage it, I don't think a role attribute is a very good solution. It's not grantable or inheritable, it can't be managed per-database, etc. So I'd leave this out until there's some popular demand. OK, I'll keep the current behavior (it allows everyone to create it). BTW, currently, the default ACL of largeobject allows anything for owner and nothing for world. Do you have any comment for the default behavior? Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] [PATCH] Largeobject access controls
KaiGai Kohei kai...@kaigai.gr.jp writes: BTW, currently, the default ACL of largeobject allows anything for owner and nothing for world. Do you have any comment for the default behavior? Mph. I think the backlash will be too great. You have to leave the default behavior the same as it is now, ie, world access. 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] [PATCH] Largeobject access controls
KaiGai Kohei wrote: BTW, currently, the default ACL of largeobject allows anything for owner and nothing for world. Do you have any comment for the default behavior? Backwards compatibility would say that the world should be able to at least read the object. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Largeobject access controls
The attached patch provides access control features on largeobject.
This patch adds the ownership and two permissions (SELECT and UPDATE) on
largeobjects. The two permissions controls reader and writer accesses to
the largeobejcts. Only owner can unlink the largeobject which is owned by.
It also add a new attribute on the database role to control whether he
can create a new largeobject, or not. Because largeobject is not stored
within a certain namespace, we cannot control its creation using CREATE
permission.
The CREATE USER/ROLE statement got a new option: LARGEOBJECT/NOLARGEOBJECT.
It enables to controls whether the user can create a largeobject, or not.
The default is LARGEOBJECT which means user can create them.
This attribute is stored within pg_authid.rollargeobject defined as bool.
The pg_largeobject system catalog is reworked to manage its metadata.
CATALOG(pg_largeobject,2613)
{
Oid loowner;/* OID of the owner */
Oid lochunk;/* OID of the data chunks */
aclitem loacl[1]; /* access permissions */
} FormData_pg_largeobject;
Actual data chunks are stored in the toast relation of pg_largeobject,
and its chunk_id is stored in the pg_largeobject.lochunk.
As I noted before, there are several difficulties to implement partially
writable varlena type, so it uses the its toast relation just as a storage
to store its data chunks.
The GRANT/REVOKE statement also support largeobject, as follows:
GRANT SELECT ON LARGE OBJECT 1234 TO kaigai;
It follows the matter when COMMENT ON statement specifies a large object.
Thanks,
(Example)
postgres=# CREATE USER dog;-- user can create largeobjects in default
CREATE ROLE
postgres=# CREATE USER cat NOLARGEOBJECT;
CREATE ROLE
postgres=# \c - dog
psql (8.5devel)
You are now connected to database postgres as user dog.
postgres= SELECT lo_create(123);
lo_create
---
123
(1 row)
postgres= SELECT lo_create(100);
lo_create
---
100
(1 row)
postgres= GRANT SELECT ON LARGE OBJECT 123 TO cat;
GRANT
postgres= \c - cat
psql (8.5devel)
You are now connected to database postgres as user cat.
postgres= SELECT lo_create(456);
ERROR: permission denied to create largeobject
postgres= SELECT loread(lo_open(123, x'4'::int), 100);
loread
\x
(1 row)
postgres= SELECT lowrite(lo_open(123, x'2'::int), 'abcdefg');
ERROR: permission denied for largeobject 123
postgres= SELECT lo_unlink(123);
ERROR: must be owner of largeobject 123
===
[kai...@saba ~]$ diffstat sepgsql-02-blob-8.5devel-r2264.patch.gz
doc/src/sgml/ref/create_role.sgml | 13 +
doc/src/sgml/ref/create_user.sgml |1
doc/src/sgml/ref/grant.sgml|8
doc/src/sgml/ref/revoke.sgml |6
src/backend/catalog/aclchk.c | 246
src/backend/catalog/dependency.c | 14 +
src/backend/catalog/pg_largeobject.c | 139 +!!
src/backend/catalog/pg_shdepend.c |4
src/backend/commands/comment.c | 10
src/backend/commands/tablecmds.c |1
src/backend/commands/user.c| 32 ++
src/backend/libpq/be-fsstubs.c | 141 ++-
src/backend/parser/gram.y | 26 +!
src/backend/storage/large_object/inv_api.c | 344 ---
src/backend/utils/adt/acl.c|4
src/backend/utils/cache/syscache.c | 13 +
src/include/catalog/dependency.h |1
src/include/catalog/indexing.h |4
src/include/catalog/pg_authid.h| 14 !
src/include/catalog/pg_largeobject.h | 17 !
src/include/catalog/toasting.h | 10
src/include/nodes/parsenodes.h |1
src/include/parser/kwlist.h|2
src/include/utils/acl.h|6
src/include/utils/syscache.h |1
src/test/regress/expected/privileges.out | 202 +
src/test/regress/input/largeobject.source |7
src/test/regress/output/largeobject.source | 10
src/test/regress/sql/privileges.sql| 75 ++
29 files changed, 857 insertions(+), 107 deletions(-), 388 modifications(!)
--
OSS Platform Development Division, NEC
KaiGai Kohei kai...@ak.jp.nec.com
sepgsql-02-blob-8.5devel-r2264.patch.gz
Description: application/gzip
--
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] [PATCH] Largeobject access controls
KaiGai Kohei kai...@ak.jp.nec.com wrote:
The pg_largeobject system catalog is reworked to manage its metadata.
CATALOG(pg_largeobject,2613)
{
Oid loowner;/* OID of the owner */
Oid lochunk;/* OID of the data chunks */
aclitem loacl[1]; /* access permissions */
} FormData_pg_largeobject;
Actual data chunks are stored in the toast relation of pg_largeobject,
and its chunk_id is stored in the pg_largeobject.lochunk.
A bit acrobatic, but insteresting idea.
I have some random comments:
* Do you measure performance of the new LO implementation?
AFAIK, Users expect performance benefits to LO; TOASTed
bytea slows down when the size of data is 100KB or greater
even if they don't use random seeks.
* We might take care of DROP ROLE and REASSIGN/DROP OWNED.
Or, we might have large object without owner.
It might require full-scanning of pg_largeobject table,
but we can accept it because the size of pg_largeobject
will be smaller; we have actual data out of the table.
* Don't we also need ALTER LARGE OBJECT oid OWNER TO user
for consistency?
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
