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

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>