Re: SE-PostgreSQL/Lite Review

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> (1) Whether the framework should host the default PG model, not only
> enhanced security features, or not?
> This patch tried to host both of the default PG model and SELinux.
> But, the default PG model does not have same origin with label-based
> mandatory access controls, such as SELinux, from a historical angle.
> So, this patch needed many of unobvious changes to the core routines.

I certainly understand what you're saying here. However, I feel that
these changes to the core of PG are good, and are taking PG in the right
direction to have a much clearer and better implemented security
infrastructure. The fact that you've found a number of odd cases
(duplicate checking, odd failure cases due to checks being done at
various parts of the process, etc) is a testement that this is bringing
the PG code forward even without the addition of SELinux support.

I realize that makes it more work for you and I don't envy you that.

> (2) Whether the framework should be comprehensive, or not?
> This patch tried to replace all the default PG checks in the core
> routines from the begining. It required to review many of unobvious
> changes in the core routine. Because I've been repeatedly pointed out
> to keep changeset smaller, I'm frightened for the direction.
> The coverage of the latest SE-PgSQL/Lite patch is only databases,
> schemas, tables and columns. I think it is a good start to deploy
> security hooks on the routines related to these objects, rather than
> comprehensive support from the begining.

I don't believe we will get support to commit a framework which is
intended to eventually support the PG model (following from my answer to
#1) in a partial form. Perhaps it would be good to split the patch up
on a "per-object-type" basis, to make it simpler/easier to review (as
in, patch #1: database_ac.c + changes to core for it; patch #2:
table_ac.c + changes to core for it, etc; earlier patches assumed to be
already done in later patches), but the assumption will almost certainly
be that they will all be committed to wonderful CVS at the same time.

> (3) In the future, whether we should allow multiple enhanced securities
> at the same time, or not?
> It was not clear in the previous commit fest. But, in fact, Linux
> kernel does not support multiple security features in same time,
> because it just makes security label management complex. (It also
> have another reasons, but just now unlear.)
> I don't think we have any good reason to activate multiple enhanced
> securities at same time.

To be honest, I feel that this will just fall out once we get through
doing #1. I've just heard from Casey (author of the SMACK security
manager, an alternative to SELinux) that the PGACE framework (which is
what we suggested he look at) would be easily modified to support SMACK.
I feel the same would be true of what we're talking about in #1. If you
disagree with that, please let me know.

> I think most of folks can agree with (2) and (3). But I expect opinion
> is divided at (1).
>
> For example, if we host the default PG checks to create a new database,
> all the existing checks shall be moved as follows:
>
> http://code.google.com/p/sepgsql/source/browse/trunk/pgsec/src/backend/security/access_control.c?r=2273#1430
>
> In addition, it has to return the security context to be assigned on
> the new database, when it hosts SELinux.
> Even if this rework is limited to four kind of object classes, I'm
> worry about it overs an acceptable complexity.

I feel that what was at issue is that the complexity of
"access_control.c" was far too great- because *everything* was in that
one file. Splitting access_control.c into smaller files would make them
more managable, and if we implement them all in a similar manner using a
similar policy for function names, arguments, etc, then once the first
object type is reviewed, the others will go more easily for the reviwer.

If the security framework does *not* support the SQL model, I think
we'll get push back from the community that it clearly couldn't be used
by any other security manager generically without alot of potential
changes and additional hooks that could end up bringing us all the way
to being capable of supporting the SQL model anyway. I'm certainly
willing to hear core indicate otherwise though.

Thanks again, KaiGai.

Stephen