Re: Proposal of SE-PostgreSQL patches [try#2] - Mailing list pgsql-hackers

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@ak.jp.nec.com>