Re: [0/4] Proposal of SE-PostgreSQL patches - Mailing list pgsql-hackers

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@gregsmith.com http://www.gregsmith.com Baltimore, MD