Re: SE-PostgreSQL Specifications

KaiGai,

* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> For the recent a few days, I've worked to write and edit
> the specification (partially copied from the draft of user
> documentation) for the development purpose.
>
>  http://wiki.postgresql.org/wiki/SEPostgreSQL_Development

Thanks for doing this.  I've taken a quick glance through it and will
review it more later today.  The next step, I believe, is to document
the changes which need to be made to PG, at a high level, to support
this specification.

I think the initial goal should be to make changes mainly to aclchk.c to
add the SELinux hooks.  I'd like to hear from some committers about this
idea: I think we should also be looking to move more of the permissions
checking which is done in other parts of the code (eg:
ATExecChangeOwner()) into aclchk.c, which is where I would argue it
belongs.  Doing so will make it much easier to add other hooks, should
the need arise, and would minimize the amount of code 'touched' to add
SELinux support.

Strategy for code changes: Patch #1: Move permissions checks currently implemented in other parts           of the code
(eg:tablecmds.c:ATExecChangeOwner()) into        aclchk.c. Patch #2: Change the aclchk.c function APIs, if necessary,
toadd           additional information required for SELinux (eg: the 'old'        owner). Patch #3: Add SELinux hooks
intoaclchk.c functions. 

This initial round, again, should focus on just those
controls/permissions which PostgreSQL already supports.  At this time,
do not stress over finding every "if(superuser())" and moving it to
aclchk.c.  Let's deal with the "clear" situations, such as tablecmds.c
lines 6322-6342 (that entire block, including the if(!superuser()),
should be ripped out and made a function in aclchk.c which is called
with necessary args).  We can go back and "clean up" the other places
where we have explicit superuser() checks later, if necessary.

I've finally had a chance to look through the last set of proposed
patches some, and I've noticed some other issues that need to be
addressed:

- Let's avoid the changes to heaptuple.c for now.  That's really for much later down the road when we implement
row-levelsecurity. 
- I would expect the dependency system to be used to handle deleting things from pg_security, etc, when an object has
beendeleted (eg: a table was dropped). 
- Conversly, when new entries need to be added to pg_security, they should be done at the same level as other items
beingadded to the catalog.  In places like createdb(), where it's all one big function, I would recommend splitting out
theexisting catalog update into a separate function, which then makes it much clearer that the code is doing: update
pg_databasetable, update pg_security table, update pg_dependency table.  That should be done as a separate patch, of
course. Remember, we're trying to make small incremental changes that make sense by themselves but at the same time
willreduce the effort required to add SELinux later. 
- In terms of cacheing the results, is there any way we could use SysCache for this?  And just handle the cacheing in
thehooks, etc, from aclchk.c?  I dislike having the security labels added to every tuple until we actually implement
rowlevel security.  It adds alot of unnecessary complexity to the initial implementation. Note that I'm referring to
theresults from the kernel/syscalls, I realize you're using SysCache properly for the entries in pg_security already,
andthat's good. 

Guess that's a start on the implementation design, which I feel is the
next step after specification.  Perhaps you could start a wiki page on
it which includes my comments?  I'm in meetings for the next couple of
hours, but will resume looking at this after lunch, US/Eastern time.
Thanks,
    Stephen