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

On Wed, 30 Apr 2008, KaiGai Kohei wrote:

> [1/4] sepostgresql-pgace-8.4devel-3-r739.patch
> provides PGACE (PostgreSQL Access Control Extension) framework.
> http://sepgsql.googlecode.com/files/sepostgresql-pgace-8.4devel-3-r739.patch

For those overwhelmed by sheer volume here, this is the patch to start
with, because it's got all the core changes to the server. I'm also in
the camp that would like to see this feature added, but rather than just
giving it a +1 I started looking at it.

The overall code is nice: easy to understand, structured modularly. I
have some concerns though. The first two things that jump out at me on an
initial review appear right from the beginning for those who want to take
a look:

-I'm a bit unnerved by both the performance and reliability implications
from how the security check calls are done in every case, even if there is
no SELinux support included. Those checks are sitting in some pretty low
level tuple and heap calls.

The approach taken here is to put all the "#ifdef" logic into the
underlying ACE interface (see patch [2/4]), so that the caller doesn't
have to care. If SELinux support is off then the calls turns into

void x(y) {} or
bool a(b) { return true; }

This is a very clean design, but it's putting extra (possibly optimized
away) calls into a lot of places. While it would be uglier, it might make
sense to put that on/off logic in all the places where the calls are made,
so that when you turn SELinux support off most of the code really does go
completely away rather than just turning into stubs.

-The only error reporting and handling method used is "elog(ERROR,...".
That seems a bit heavy handed for something that can be expected to happen
all the time.

If I understand this correctly, when you're scanning a table with 1000
rows where you're only allowed to see 50% of them, that's going to be 500
call to elog(), one for each tuple you can't see. Having a tuple get
screened out isn't really an error per se, and while I can see how
sensitive installs would want those all reported there are others where
this volume of log activity would be too much. Just because someone with
classified clearance is looking at a big table that also has a lot of
secret info in it, not all installs will want a million errors reported
just because there's data that person can't see available.

At a minimum, this needs some finer log control, and maybe a rethinking
altogether of how to handle error cases.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD