(2010/08/07 0:02), Robert Haas wrote:
At PGCon, we discussed the possibility that a minimal SE-PostgreSQL
implementation would need little more than a hook in
ExecCheckRTPerms() [which we've since added] and a security label
facility [for which KaiGai has submitted a patch]. I actually sat
down to write the security label patch myself while we were in Ottawa,
but quickly ran into difficulties: while the hook we have now can't do
anything useful with objects other than relations, it's pretty clear
from previous discussions on this topic that the demand for labels on
other kinds of objects is not going to go away. Rather than adding
additional syntax to every object type in the system (some of which
don't even have ALTER commands at present), I suggested basing the
syntax on the existing COMMENT syntax. After some discussion[1], we
seem to have settled on the following:
SECURITY LABEL [ FOR<provider> ] ON<object class> <object name> IS '<label>';
At present, there are some difficulties with generalizing this syntax
to other object types. As I found out when I initially set out to
write this patch, it'd basically require duplicating all of comment.c,
which is an unpleasant prospect, because that file is big and crufty;
it has a large amount of internal duplication. Furthermore, the
existing locking mechanism that we're using for comments is known to
be inadequate[2]. Dropping a comment while someone else is in the
midst of commenting on it leaves orphaned comments lying around in
pg_(sh)description that could later be inherited by a new object.
That's a minor nuisance for comments and would be nice to fix, but is
obviously a far larger problem for security labels, where even a small
chance of randomly mislabeling an object is no good.
So I wrote a patch. The attached patch factors out all of the code in
comment.c that is responsible for translating parser representations
into a new file parser/parse_object.c, leaving just the
comment-specific stuff in commands/comment.c. It also adds
appropriate locking, so that concurrent COMMENT/DROP scenarios don't
leave behind leftovers. It's a fairly large patch, but the changes
are extremely localized: comment.c gets a lot smaller, and
parse_object.c gets bigger by a slightly smaller amount.
Any comments? (ha ha ha...)
[1] http://archives.postgresql.org/pgsql-hackers/2010-07/msg01328.php
[2] http://archives.postgresql.org/pgsql-hackers/2010-07/msg00351.php
Thanks for your efforts.
I believe the get_object_address() enables to implement security
label features on various kind of database objects.
I tried to look at the patch. Most part is fine, but I found out
two issues.
On the object_exists(), when we verify existence of a large object,
it needs to scan pg_largeobject_metadata, instead of pg_largeobject.
When we implement pg_largeobject_metadata catalog, we decided to set
LargeObjectRelationId on object.classId due to the backend compatibility.
| /*
| * For object types that have a relevant syscache, we use it; for
| * everything else, we'll have to do an index-scan. This switch
| * sets either the cache to be used for the syscache lookup, or the
| * index to be used for the index scan.
| */
| switch (address.classId)
| {
| case RelationRelationId:
| cache = RELOID;
| break;
| :
| case LargeObjectRelationId:
| indexoid = LargeObjectMetadataOidIndexId;
| break;
| :
| }
|
| /* Found a syscache? */
| if (cache != -1)
| return SearchSysCacheExists1(cache,
ObjectIdGetDatum(address.objectId));
|
| /* No syscache, so examine the table directly. */
| Assert(OidIsValid(indexoid));
| ScanKeyInit(&skey[0], ObjectIdAttributeNumber, BTEqualStrategyNumber,
| F_OIDEQ, ObjectIdGetDatum(address.objectId));
| rel = heap_open(address.classId, AccessShareLock);
^^^^^^^^^^^^^^^ <- It tries to open pg_largeobject
| sd = systable_beginscan(rel, indexoid, true, SnapshotNow, 1, skey);
| found = HeapTupleIsValid(systable_getnext(sd));
| systable_endscan(sd);
| heap_close(rel, AccessShareLock);
| return found;
| }
Although no caller invokes get_object_address() with lockmode = NoLock,
isn't it necessary to skip locking if NoLock was given.
| /*
| * If we're dealing with a relation or attribute, then the relation is
| * already locked. If we're dealing with any other type of object, we need
| * to lock it and then verify that it still exists.
| */
| if (address.classId != RelationRelationId)
| {
| if (IsSharedRelation(address.classId))
| LockSharedObject(address.classId, address.objectId, 0, lockmode);
| else
| LockDatabaseObject(address.classId, address.objectId, 0, lockmode);
| /* Did it go away while we were waiting for the lock? */
| if (!object_exists(address))
| elog(ERROR, "cache lookup failed for class %u object %u subobj %d",
| address.classId, address.objectId, address.objectSubId);
| }
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
