Re: SE-PostgreSQL Specifications

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)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, to add
additional information required for SELinux (eg: the 'old'
owner).
Patch #3: Add SELinux hooks into aclchk.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-level security.
- I would expect the dependency system to be used to handle deleting
things from pg_security, etc, when an object has been deleted (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 being added to the
catalog. In places like createdb(), where it's all one big function,
I would recommend splitting out the existing catalog update into a
separate function, which then makes it much clearer that the code is
doing: update pg_database table, 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 will reduce 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 the hooks, etc,
from aclchk.c? I dislike having the security labels added to
every tuple until we actually implement row level security. It adds
alot of unnecessary complexity to the initial implementation.
Note that I'm referring to the results from the kernel/syscalls, I
realize you're using SysCache properly for the entries in pg_security
already, and that'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