Re: [PATCH] SE-PgSQL/lite rev.2163

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.

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.

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:

- Less code.
- Easier to maintain.

With the current design, every time someone makes a change to how DAC
permissions are checked, they have to think about the proper sepgsql
treatment as well.  That seems like a recipe for security bugs.

...Robert