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 (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
