Re: [0/4] Proposal of SE-PostgreSQL patches

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> I updated the series of SE-PostgreSQL patches for the latest pgsql-8.4devel tree.

I tried to do a bit of testing of this, but it does not work on current
Fedora 8, because the policy module doesn't build:

[tgl(at)rh2 sepgsql-policy]$ make
make -f /usr/share/selinux/devel/Makefile NAME=targeted
make[1]: Entering directory `/home/tgl/sepgsql/contrib/sepgsql-policy'
Compiling targeted sepostgresql module
/usr/bin/checkmodule: loading policy configuration from tmp/sepostgresql.tmp
sepostgresql.te:349:ERROR 'syntax error' at token 'corenet_tcp_recvfrom_labeled' on line 5675:
# NOTE: These changes are to be merged in the later releases.
corenet_tcp_recvfrom_labeled(sepgsql_server_type, sepgsql_client_type)
/usr/bin/checkmodule: error(s) encountered while parsing configuration
make[1]: *** [tmp/sepostgresql.mod] Error 1
make[1]: Leaving directory `/home/tgl/sepgsql/contrib/sepgsql-policy'
make: *** [sepostgresql.pp] Error 2
[tgl(at)rh2 sepgsql-policy]$

In the meantime, here are some random comments after my failed test
build and a very fast scan through the patch:

The patch tries to re-add ipcclean to the source tree --- probably a
merge error?

autoconf complains about the description-free AC_DEFINEs

Doesn't compile warning-free if selinux not enabled ... for that matter,
it doesn't compile warning-free if selinux IS enabled.

No documentation updates whatsoever :-(

About half of the patch seems to be conditional on
#ifdef SECURITY_SYSATTR_NAME
and the other half on
#ifdef HAVE_SELINUX
This seems bizarre: is there really any chance that there are two
independently usable chunks of code here? And why is it conditional
on a macro that is a field name, rather than conditional on a feature
macro? That is, I'd expect to see something like
#ifdef ENABLE_SEPOSTGRES
throughout.

For that matter, what is the point of treating SECURITY_SYSATTR_NAME
as a configurable thing in the first place? I can hardly imagine a
worse idea than a security-critical column having different names in
different installations.

The patch hasn't got a mode in which SELinux support is compiled in but
not active. This is a good way to ensure that no one will ever ship
standard RPMs with the feature compiled in, because they will be entirely
nonfunctional for people who aren't interested in setting up SELinux.
I think you need an "enable_sepostgres" GUC, or something like that.
(Of course, the overhead of the per-row security column would probably
discourage anyone from wanting to use such a configuration anyway,
so maybe the point is moot.)

sepgsql-policy has got usability problems:
* It should pay attention to the configured installation PREFIX instead of
hardwiring a couple of random possible installation locations
* It can only support the build machine's SELINUXTYPE --- how am I supposed
to produce RPMs that support all available types?

The contents and use of sepgsqlGetTupleName() make it look like the
entire security scheme is based on object name alone. That doesn't
even account for schemas, let alone overloaded function names. Please
tell me this is not as broken-by-design as it looks.

I occasionally tell people "try to make the patch look like it's always
been there". This is pretty far from meeting that goal. Random bits
of code that are commented "PGACE:" are obviously not intended to just
fit in. You've generally ignored the Postgres code layout conventions
(pgindent will help to some extent but it's far from a panacea) and our
commenting conventions --- eg, hardly any of the functions have header
comments, and the ones that do follow conventions seen noplace in the
Postgres code, like using "@" on parameter names. In general the number
and quality of the comments is far below the standard for Postgres code,
and the lack of any implementation documentation isn't helping.

Another big problem, which I understand your motivation for but that
doesn't make the code any less ugly, is that you've got trivial bits
of code that're separated by two(!) levels of hook calls from where
they're actually being used. Not only does that make it unreadable
but the files that actually do the work combine bits of code that
should be scattered across a lot of modules, causing those files to
be just horrid from a modularity standpoint --- they've got their
fingers stuck in everyplace. If you want this to get applied you need
to start thinking of it as an integral part of the code, not an add-on.

Some other bits of add-on-itis:
* If you need a dedicated LWLock, declare it in lwlock.h.
* If you need a node type, declare it in nodes.h (T_SEvalItem is utterly
broken)

Why in the world would you have security restrictions associated with
TOAST tuples? Seems like all the interesting restrictions should be
on the parent table.

Don't randomly invent your own style of management of a postmaster
child process. For one thing, this code doesn't cope with either
unexpected death of the postmaster or unexpected death of the child.
If you need another child, manage it in postmaster.c the same way
the other children are managed.

The code in hooks.c looks suspiciously not-HOT-aware, eg use of
ItemIdIsUsed() for what probably needs to be ItemIdIsNormal().
(Not that this code ought to be fetching the tuple for itself
in the first place --- probable big performance loss there...)

pgaceHooks.c seems to be a useless layer of indirection
--- lose it all, and inline into callers instead.

Is the hard-wired shmem cache size really adequate? Why are you
using such a cache in shared memory at all, rather than backend-local?
The locking implications likely take away more performance than you
save by not having each session need to load up its cache.
(We don't use shared catalog caches, in general.)

If we're going to support assignment to system columns, we probably
want a general solution that will work for OID not only the security
column. Also, I'm unconvinced that setting resjunk = true for such
targetlist entries is a good idea.

The whole "early security" business looks like a mess :-(. I suspect
you should rip all that out of the backend and add a step to initdb
that fills in those tables.

The idea of input functions that alter system tables scares me.

elog() should not be used for user-facing errors. I couldn't easily
tell just which of the messages are likely to be seen by users and
which ones should be "can't happen" cases, but certainly there are
a whole lot of these that need to be ereport()s. Likely there need
to be some new ERRCODEs too.

__lookupRelationForm() strikes me as a complete crock. It probably
means that you've selected the wrong places to call its callers from.
This gets back to the point above that adding additional fetches of
tuples isn't good for performance anyway.

Don't use identifiers with a leading double-underscore. These are
reserved for system-private identifiers according to the C standard.

Use of a function in genbki.sh is very likely not portable.

use of flock() is probably not portable and even less probably necessary.

Declaring a function foo() rather than foo(void) is poor style ---
at least some compilers will complain about that.

I see a lot of "Copyright 2007 KaiGai Kohei" notices. Are you
willing to assign those copyrights to the Postgres Global Development
Group? If not, we'll at least need statements along the line of
"Distributed under the PostgreSQL license".

regards, tom lane