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

Greg Smith wrote:
> 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:

Thanks for your attention & reviewing.

> -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.

Your concern is fair enough. I indeed used inline function to avoid unnecessary
invokation in the previous version, as follows:
http://code.google.com/p/sepgsql/source/browse/branches/pgsql-8.2.x/sepgsql/src/include/security/pgace.h

However, I changed this design, because it requires any security modules
have to provides its implementation for any hooks, including just a stub.
In the current design, author of another security modules can leave unused
hooks unchanged. It improves code maintenancability.

But an inspiration hit me just now.
If we declare any hooks as static inline functions which includes #ifdef
block, it can resolve these matters as follows:
---- EXAMPLE ----
static inline bool pgaceHeapTupleInsert(Relation rel, HeapTuple tuple,
bool is_internal, bool with_returning) {
#ifdef HAVE_SELINUX
if (sepgsqlIsEnabled())
return sepgsqlHeapTupleInsert(rel, tuple, is_internal, with_returning);
#endif
return true;
}
----------------
Is it reasonable idea?

> -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.

SE-PostgreSQL invokes "elog(ERROR,...", when a given query tries to access
violated columns, tables and so on, but not for each tuple.

In row-level access controls, it filters violated tuples without aborting
query execution. However, "elog(NOTICE,..." can be invoked, if the security
policy requires to generate access denied logs on row-level.

> 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.

The security policy can control whether access denied logs should be
printed, or not.
In the default security policy provided by the patch [4/4], row-level
access denied logs are disabled to avoid flood of logs as you noticed.

If you want to turn on/off row-level logs, use the following command:
# setsebool -P sepgsql_enable_audittuple=(1|0)
It changes the internal state of security policy, and SE-PostgreSQL
works according to this one.

> 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.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>