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

Robert, thanks for your comments.

Robert Haas wrote:
> 2009/7/10 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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?

> 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.

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.

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.

Thanks,

> - 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

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>