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,
