Re: Proposal of SE-PostgreSQL patches [try#2]

Thanks for your reviewing.

Peter Eisentraut wrote:
> Am Donnerstag, 26. Juni 2008 schrieb KaiGai Kohei:
>> The following patch set (r926) are updated one toward the latest CVS head,
>> and contains some fixes in security policy and documentation.
>
> OK, I have quickly read through these patches. They look very nice, so I am
> optimistic we can get through this.
>
> First of all, now would be a good time if someone out there really wants to
> object to this feature in general. It will probably always be a niche
> feature. But all the code is hidden behind ifdefs or other constructs that a
> compiler can easily hide away (or we can make it so, at least).
>
> Here is a presentation from PGCon on SE-PostgreSQL:
> http://www.pgcon.org/2008/schedule/events/77.en.html
>
> Are there any comments yet from the (Trusted)Solaris people that wanted to
> evaluate this approach for compatibility with their approach?

In this April, I had a face-to-face meeting with Trusted Solaris people
to discuss SE-PostgreSQL and PGACE framework, and concluded that PGACE
framework provides enough facilities for both operating systems.

I modified several hooks from CommitFest:May, however, its fundamental
structures are unchanged.

> In general, are we OK with the syntax CONTEXT = '...'? I would rather see
> something like SECURITY CONTEXT '...'. There are a lot of contexts, after
> all.

If we change it, I prefer SECURITY_CONTEXT = '...' style, because it enables
to represent the left hand with a single token and make PGACE hooks simpler.
I also agree the CONTEXT has widespread meanings and to be changed here.

> This will also add a system column called security_context. I think that is
> OK.

Thanks,

> In the pg_dump patch:
>
> spelling mistake "tuen on/off"

I'll fix it soon.

> Evil coding style: if (strcmp(SELINUX_SYSATTR_NAME, security_sysattr_name)) --
> compare the result with 0 please.

OK, I'll fix it and check my implementations in other place.

> The above code appears to assume that security_sysattr_name never changes, but
> then why do we need a GUC parameter to show it?

The purpose of the GUC parameter is to identify the kind of security feature
if enabled. It can be changed by other security features, and it will show us
different value.

> Might want to change the option name --enable-selinux to something
> like --security-context.
>
> In general, we might want to not name things selinux_* but instead
> sepostgresql_* or security_* or security_context_*. Or maybe PGACE?

The pgace_* scheme is an attractive idea, although the server process
has to provide a bit more hints (like the name of security system column
and the kind of objects exported with security attribute) pg_dump to
support various kind of security features with smallest implementation.

If not so, I prefer the combination of --security-context and sepostgresql_*.

> On the default policy:
>
> Should this really be a contrib module? Considering that it would be a core
> feature that is not really usable without a policy.

It is correct, SE-PostgreSQL feature always need its security policy.
Do you think "src/backend/security/sepgsql/policy" is better?

> Please change all the sepgsql_* things to sepostgresql_*, considering that you
> are using both already, so we shouldn't have both forms of names.

We can convert them from sepostgresql_* to sepgsql_* easily, because the longer
forms are not used as a part of identifier in security context.
But we have a possible matter in changing from sepgsql_* to sepostgresql_*.

Here is a news from SELinux community:
http://marc.info/?l=selinux&m=121501336024075&w=2

It shows most part of the SE-PostgreSQL policy module got merged into
the upstreamed at selinux-policy-3.4.2 or later. It contains identifier
with sepgsql_* stuff, so it has a possible matter when users upgrade
his security policy.

If their database is labeled as sepostgresql_*, it will lose rules
defined in the policy when users upgrade selinux-policy package to
the latest one.

> Documentation:
>
> Looks good for a start, but we will probably want to write more later.

Do you think what kind of information should be added?
I intentionally omitted descriptions about SELinux itself,
because it is a documentation of PostgreSQL, not OS.

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