Re: [PATCH] ACE Framework - Database, Schema - Mailing list pgsql-hackers

2009/12/13 KaiGai Kohei <kaigai@ak.jp.nec.com>:
>> Just to name a few really obvious problems (I only looked at the
>> 01-database patch):
>>
>> 1. We have been talking for several days about the need to make the
>> initial patch in this area strictly a code cleanup patch.  Is this
>> cleaner than the code that it is replacing?  Is it even making an
>> attempt to conform to that mandate?
>
> Even if it is unclear whether the current form is more clear than the
> current inlined pg_xxx_aclcheck() form, or not, it will obviously
> provide a set of common entry points for upcoming enhanced security
> providers.
> Eventually, it is more clear than enumeration of #ifdef ... #endif
> blocks for SELinux, Smack, Solaris-TX and others.

Right, but it will also not get committed.  :-(

You seem to still be failing to understand the approach that Stephen
and I have been discussing for the last several days...  but at any
rate, I think that it is possible to do this in a way that is more
clear (or at least, AS clear) as the current method.  Stephen is also
working on a concept patch that I'm eager to see, and I would like to
give him a little time to get that together before we spend too much
more time working on this.

>> I don't understand the modifications to restrict_and_check_grant().
>> It seems to me that this is going in the wrong direction, although
>> since I don't understand the motivation for the change, I might be
>> wrong.  If we have a piece of code that centralizes permissions
>> checking now, splitting that up into a bunch of ace_*_grant()
>> functions and duplicating it doesn't seem like an improvement.
>
> From perspective of the enhanced security providers, GRANT/REVOKE is
> a kind of modification of properties on a certain database object.
> So, it has a motivation to check GRANT/REVOKE using its access control
> rules.
> If we don't modify restrict_and_check_grant(), the caller (aclchk.c)
> correctly handles the default PG checks, so rest of enhanced security
> providers will apply additional checks in the ace_*_grant().
> In this case, we keep ace_*_grant() empty at the moment, and it shall
> be a special purpose hook for enhanced security providers.
> I don't have any preference on either of them.
[...]
> When we support security label on database, it will also necessary:
>
> - ace_database_alter_relabel()

This seems like bad design to me.  I don't see why SE-Linux would get
a vote on whether a user is allowed to change the DAC permissions, any
more than DAC gets a vote on whether an SE-Linux relabel operation can
go through.  If it does, then this attempt to create a framework is
not going to succeed, because every new provider will require hooks to
let every exist provider muck with what it does, and they'll all have
to have complete knowledge of each other's internals.

> It seems to me ace_database_calculate_size() might be a bad name
> because of too specific naming for a certin functionality.

That's exactly the problem.  I'm not sure if I believe in the
particular solution you've offered here, but it's certainly better
than the way it is now.  I think this needs more thought from more
people.

> What about "acechk_*" to mean access control extension checks <something>
> to be <action>'ed? For example, acechk_database_create() means "access
> control extension checks <database> to be <created>".

That fails to address all of my comments, so, -1 from me.

...Robert