[PATCH] Largeobject Access Controls (r2432)

2009/11/12 KaiGai Kohei <kaigai@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 <literal>lo_import()</literal> and
+         <literal>lo_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 <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>.

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

On Thu, Dec 3, 2009 at 12:49 PM, Jaime Casanova
<jcasanov@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

Robert Haas wrote:

On Thu, Dec 3, 2009 at 12:49 PM, Jaime Casanova
<jcasanov@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 Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.com

On Thu, Dec 3, 2009 at 1:23 PM, Greg Smith <greg@2ndquadrant.com> wrote:

Robert Haas wrote:
On Thu, Dec 3, 2009 at 12:49 PM, Jaime Casanova
<jcasanov@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

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Dec 3, 2009 at 1:23 PM, Greg Smith <greg@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

On Thu, Dec 3, 2009 at 2:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Dec 3, 2009 at 1:23 PM, Greg Smith <greg@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

Robert Haas <robertmhaas@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

On Thu, Dec 3, 2009 at 3:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@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

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 Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.com

Jaime Casanova wrote:

2009/11/12 KaiGai Kohei <kaigai@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 <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@ak.jp.nec.com>

KaiGai Kohei <kaigai@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

Itagaki Takahiro wrote:

KaiGai Kohei <kaigai@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 <kaigai@ak.jp.nec.com>

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 <kaigai@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 <literal>lo_import()</literal> and
+         <literal>lo_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 <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>.

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 <kaigai@ak.jp.nec.com>

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 Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.com

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 <kaigai@ak.jp.nec.com>

On Sun, Dec 6, 2009 at 11:19 PM, Greg Smith <greg@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

Jaime Casanova wrote:

On Sun, Dec 6, 2009 at 11:19 PM, Greg Smith <greg@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 <kaigai@ak.jp.nec.com>

KaiGai Kohei <kaigai@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

Hi, I'm reviewing LO-AC patch.

KaiGai Kohei <kaigai@ak.jp.nec.com> wrote:

Nothing are changed in other codes, including something corresponding to
in-place upgrading. I'm waiting for suggestion.

I have a question about the behavior -- the patch adds ownership
management of large objects. Non-privileged users cannot read, write,
or drop othere's LOs. But they can read the contents of large object
when they read pg_catalog.pg_largeobject directly. Even if the patch
is applied, we still allow "SELECT * FROM pg_largeobject" ...right?

This issue might be solved by the core SE-PgSQL patch,
but what should we do fo now?

Other changes in the patch seem to be reasonable.

"GRANT/REVOKE ON LARGE OBJECT <number>" might be hard to use if used alone,
but we can use the commands as dynamic SQLs in DO statements if we want to
grant or revoke privileges in bulk.

"SELECT oid FROM pg_largeobject_metadata" is used in some places instead of
"SELECT DISTINCT loid FROM pg_largeobject". They return the same result,
but the former will be faster because we don't use DISTINCT. pg_dump will
be slightly accelerated by the new query.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center