Re: [PATCH] SE-PgSQL/lite rev.2163 - Mailing list pgsql-hackers

2009/7/12 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> Robert, thanks for your comments.
>
> Robert Haas wrote:
>> 2009/7/10 KaiGai Kohei <kaigai@ak.jp.nec.com>:
>>> The SE-PostgreSQL patches are updated as follows:
>>>
>>> [1/5] http://sepgsql.googlecode.com/files/sepgsql-01-sysatt-8.5devel-r2163.patch
>>> [2/5] http://sepgsql.googlecode.com/files/sepgsql-02-core-8.5devel-r2163.patch
>>> [3/5] http://sepgsql.googlecode.com/files/sepgsql-03-gram-8.5devel-r2163.patch
>>> [4/5] http://sepgsql.googlecode.com/files/sepgsql-04-tests-8.5devel-r2163.patch
>>> [5/5] http://sepgsql.googlecode.com/files/sepgsql-05-docs-8.5devel-r2163.patch
>>>
>>> List of updates:
>>> * Patch set was organized to a few ones which provides only core features.
>>> * Code base was upgraded to the latest CVS HEAD.
>>> * Some of features in the fullset edition were separated, to focus on
>>>  the core feature of SE-PostgreSQL at the first commit fest.
>>
>> I took a look at this a little bit.  It looks as if you are still
>> treating the Security Label as a special attribute of the tuple, which
>> seems completely unnecessary given that this patch set is not
>> attempting to implement row-level security.  It seems to me that all
>> you should need is regular old columns pg_class.relseclabel,
>> pg_attribute.attseclabel, etc; it also seems to me that would simplify
>> the code.
>
> Are you saying that whole of the pg_security mechanism also should be
> postponed to the second commit fest, not only system column's definitions?

I'm not sure what you mean by "the whole of the pg_security
mechanism".  But I believe there was negative feedback previously
about the idea of sandwhiching the security label into the tuple
header instead of making it an ordinary (non-system) column in a
system catalog table.

>> As a general comment, I don't have much confidence that your original
>> design for row-level security is a good one.  I think that needs to be
>> thought about very carefully before anything is implemented.  I note
>> that your original design called for a system catalog lookup on each
>> row returned, which is basically saying that all security-label
>> filtering will be implemented as a nested loop with inner index scan.
>> It seems to me that if we ever implement row-level security (which is
>> far from a sure thing) we're certainly going to want to allow the
>> planner to make other decisions, such as sorting the tuples by
>> security label and performing a merge join against the pg_security
>> catalog, or hashing the pg_security catalog and performing a hash
>> join, or using an index on the security label column to perform a
>> bitmap index scan, or any other technique that the planner already
>> knows how to consider.  I say all this not to encourage you to spend
>> more time on row-level security now (because I think that would be
>> premature) but to encourage you to abandon all the design decisions in
>> this patch that are presuming a particular design for row-level
>> security and focus on making the features that are actually in this
>> patch set as lean and robust as possible.
>
> I'm confusing a bit for the comments.
> The row-level access control stuff (which is managed in my repository
> now) does not need to look up the pg_security system catalog every time.
> I guess you believe SE-PgSQL looks up the system catalog to fetch security
> label in text form, and calls in-kernel SELinux to make its decision for
> every tuples. However, it is incorrect.
>
> The userspace avc (security/sepgsql/avc.c) routines enable to cache access
> control decisions recently used, and return its result for the required
> pair of security identifier (not a text form) and type of actions (like
> db_table:{select}), if it hits a cache entries.
> It means SE-PgSQL can return its decisions using only security identifier
> in most cases.

Perhaps you're not making a kernel call for each row (good!), but I
think you are still assuming that you're going to fetch the rows
first, without any sepgsql-specific decision making, and then perform
an operation of some kind on each row to see whether to filter it.  If
so, what I'm saying is that's bad.

Suppose we have two security contexts A and B, and two users Alice and
Bob.  Alice can see only tuples in security context A, and Bob can see
only tuples in security context B.  If I create an index on the
security ID of a table with row-level security, (a) then will it work?
and (b) if one of the users issues a command like "select * for
table", will the planner consider a bitmap-index-scan using that index
rather than a sequential scan of the entire table?

> I think what you suggested is useful, if SE-PgSQL needs security label
> in text form on making its decision. However, it seems to me your comments
> bases on incorrect assumption. Could you point to me, if I incorrectly
> understood the intentions of your comments.
>
>> Another problem that I have with this patch set is that it STILL
>> doesn't have parity with the DAC permissions scheme (despite previous
>> requests to make it have parity).  For example, you're checking
>> privileges like db_column:{drop}, which have no analogue in our
>> current permissions scheme.  I think this has been discussed several
>> times before, and it seems that you still haven't chosen to fully take
>> that advice, which I suspect is going to be an absolute bar to getting
>> this committed.  (I am not a committer, of course, so take whatever I
>> say with a grain of salt, but that's my opinion for what it's worth.)
>> It seems to me that if you have REAL parity with the existing
>> permissions scheme, it shouldn't be necessary to add additional,
>> separate sepgsql checks in every place currently has a standard
>> permissions check.  Instead, you should be able to put all of the
>> logic into functions like pg_class_aclmask().  This will be:
>
> I moved several security hooks to the pg_xxx_aclmask() because these
> permissions to be checked in same places.
> However, both of security models also have different definitions.
> For example, when we create a new table, dac checks ACL_CREATE on
> the namespace (it may be equivalent to db_schema:{add_object}),
> but MAC checks db_table:{create} permission on the new table itself
> labeled with default or user given one.

So why do it differently?  I don't see why you have to invent a whole
new paradigm here.

> For the security hooks we can move to pg_xxx_aclmask(), I can agree
> to move them as possible as we can, such as sepgsqlCheclProcedureExecute()
> invoked from pg_proc_aclmask(). But, I concluded rest of security hooks
> are hard to integrate with pg_xxxx_aclmask() mechanism.

...Robert