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

Hi,

I updated the series of patches, as follows:

[1/4] Core facilities of PGACE/SE-PostgreSQL
http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r953.patch

[2/4] "--security-context" option of pg_dump/pg_dumpall
http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r953.patch

[3/4] Default security policy for SE-PostgreSQL
http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r953.patch

[4/4] Documentation updates
http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r953.patch

List of updates:
* "--enable-selinux" option of pg_dump/pg_dumpall are replaced
by "--security-context" option.
* A new GUC parameter of "pgace_security_feature" is added to show
what feature is worked on PGACE security framework.
* pg_ace_dumpXXXX() hooks are added to src/bin/pg_dump/pg_ace_dump.h
to abstract security attribute dumping effort.
* An extended syntax of CONTEXT = '...' is replaced by
SECURITY_CONTEXT = '...'.
* The sources of security policy module are moved from contrib/ to
src/backend/security/sepgsql/policy.
* The prefix of interfaces in the default security policy are changed
to sepgsql_*() from sepostgresql_*()
* Using integer value as a condition is replaced as follows:
if (!strcmp(..)) -> if (strcmp(...) == 0)
* Two potential bug fixes:
1. Unconditional Assert() in sepgsql_avc_reclaim().
2. relkind checks are lacked in sepgsqlSetDefaultContext().

The patch of core facilities is unchanged expect for the new GUC parameters
and above two minor bug fixes.

And I have a question. Is it legal to use a pointer value as a condition,
like `while (!pointer) ...' ?

Thanks for youe reviewing.

KaiGai Kohei wrote:
> 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>