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

On the WiKi of CommitFest:Sep,
http://wiki.postgresql.org/wiki/CommitFest:2008-09

The entry of SE-PostgreSQL points a message when I submitted older version
of our patch set. But the latest ones are listed on another message.

Please add a link to the following message for our convenience:
http://archives.postgresql.org/pgsql-hackers/2008-07/msg01365.php

BTW, I can respond to reviewer's comments and update/rework our patches
on this August, although SE-PostgreSQL is moved to the CommitFest:Sep.
We welcome any early comments, if possible.

Thanks,

KaiGai Kohei wrote:
> 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(at)ak(dot)jp(dot)nec(dot)com>