Re: Updates of SE-PostgreSQL 8.4devel patches (r1403)

Alvaro Herrera wrote:
> KaiGai Kohei wrote:
>> I updated patch set of SE-PostgreSQL and related stuff (r1403).
>>
>> [1/5] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1403.patch
>
> Random observations:

Thanks for your comment!

> heapam.c: you've got a bunch of elog(ERROR) calls in there that should
> be ereport(ERROR), and should probably have a errcode() on them too.
> Also the message should be worded like this:
> "could not insert tuple on \"%s\" due to security reasons"
> or something like that. I mean: do not use C function names in error
> messages; quote the table name.
>
> tuptoaster.c: same problem
>
> heap.c: typo on line 1062, says "el", should say "rel"

Fixed,
http://code.google.com/p/sepgsql/source/detail?r=1404

IIRC, Tom pointed out we should apply ereport() for user visible error
messages, not elog(), on CommitFest:May, but I missed to fix them.

> pgace.h: you have a bunch of "static inline" functions in here. As far
> as I know this doesn't work in compilers other than GCC :-( See
> pg_list.h (list_head) for an example. I think we can tolerate this for
> the three functions in pg_list.h because they are so few and so tiny,
> but I'm not sure about PGACE because they are a large lot. On the other
> hand, turning them to real functions would be a performance hit.

It was the original implementation of PGACE hooks about 700 revisions ago. :-)
http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/src/backend/security/pgaceHooks.c?r=739

On CommitFest:May, I got a comment to replace them as inline functions,
so they are moved to src/inclide/security/pgace.h.
At that time, it turned on/off a security feature using compile-time
option, so I guess reviewer concerned about cost to invoke empty function
when no enhanced security feature is available.

However, I also think what you pointed out is fair enough.
Now pgace hooks allows 'pgace_feature' option to choose an enhanced security
feature on startup time, not a compile time, so the assumption was changed.
I think they should be reverted to real functions again.

Is there any other opinion?

> The pgace worker process ... do your postmaster.c changes work when
> pgace is disabled? I think it tries to start the worker on every
> iteration.

It is incorrect. When enhanced security is disabled, pgaceStartupWorkerProcess()
does nothing and always returns (pid_t) 0. It means there is no worker process.

> Maybe it needs more smarts in postmaster.c so that when
> !sepgsqlIsEnabled() it just doesn't try to start it up.

When user chooses selinux as 'pgace_feature', and sepgsqlIsEnabled()
returns true, it tries to start it up.
Yes, it is already done.

> Also, I think
> there should be a separate function to tell whether a particular
> PGACE_FEATURE actually needs a worker process; right now the only
> feature (SELinux) does need it, but is this the case for them all?

Basically, I think the core code should not invoke functions of
enhanced security features directly, as far as possible.
(GUC options and reloptions are exceptions.)

It is worthwhile that pgaceXXXX() wraps and turns on/off all the
security features, even if SELinux is currently the only guest,
because it enables to separate security related code so well.

> I didn't delve into many details in the avc, the worker, or anything
> much here actually -- I just skimmed randomly. This is a really huge
> patch; sorry I do not have time right now to review it.

Never mind it.
I think your comments help us to improve SE-PostgreSQL more.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>