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 <kai...@ak.jp.nec.com>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
