Re: [PATCH] Largeobject Access Controls (r2432)

Jaime Casanova wrote:
> 2009/11/12 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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 <literal>lo_import()</literal> and
> + <literal>lo_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 <literal>SELECT</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 <literal>UPDATE</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()

Thanks, please wait for revised revision.
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>