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

Hello,

The following patches are updated ones of SE-PostgreSQL toward the HEAD of CVS.

[1/4] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r958.patch
[2/4] http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r958.patch
[3/4] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r958.patch
[4/4] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r958.patch

By the way,
http://wiki.postgresql.org/wiki/CommitFest:2008-07

| Peter says: checking with Solaris engineers about compatibility with
| Solaris TX; will continue review throughout August


Jon, do you have anything to comment about PGACE security framework?
(Show the src/include/security/pgace.h)

It has been reworked a bit from this April when we had an offline meeting,
but I think its impact is not significant for its portability.

It can provide its guest subsystem (like SE-PostgreSQL) a series of hooks to
make a decision and facilities to manage text represented security attribute
of database objects.
IIUC, we concluded these foundations are also enough to port SolarisTX feature.

If you find out something lacks/conflicts, please tell me.

Thanks,


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