Re: [PATCH] ACE Framework - Database, Schema

Robert Haas wrote:
> 2009/12/13 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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. :-(

The framework will be necessary to get them committed.
Which is an egg, and which is a chicken? :-(

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

Hmm... In my hope, I'd like to work on these patches during the Christmas
vacation terms for US people. So, it seems to me we need to decide the
direction in this week.

Stephen, sorry for the prod.

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

From the DAC perspective, a relabel operation is setting an attribute
of a certain database, such as ALTER DATABASE ... SET xxx. So, it is
quite natural to check ownership of the target database like other
ALTER DATABASE stuff, not only SELinux relabeling checks.

I intend to use the *_alter_relabel() hook for any other label-based
security providers, not only SELinux. The caller, syntax support and
the default PG checks don't need to know details in any other security
providers. (It is also reason why I prefer one enhanced provider at
the same time, because they can share syntax support such as SECURITY
LABEL ('...') option.)

In my image, it shall be implemented as follows:

ALTER DATABASE <datname> SECURITY LABEL ( '<new label>' );
|
V
Value *
ace_database_alter_relabel(Oid datOid, Value *newLabel)
{
if (pg_database_ownercheck(datOid, GetUserId())
aclcheck_error(...);

switch (ace_provider)
{
#if HAVE_SELINUX
case ACE_PROVIDER_SELINUX:
if (sepgsql_is_enabled())
return sepgsql_database_alter_relabel(...);
break;
#endif
#if HAVE_SMACK
case ACE_PROVIDER_SMACK:
if (smack_is_enabled())
return smack_database_alter_relabel(...);
break;
#endif
default:
break;
}

ereport(ERROR, "No label-based MAC is now enabled");
return NULL;
}
|
V
The caller update pg_database system catalog with the returned
security label.

However, it is not necessary to conclude at the moment.
We can add it later.

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

From my previous large patch, the following functions might have similar
arguments:

* ac_relation_get_transaction_id()
The currtid_byreloid() and currtid_byrelname() are the only caller, and
checks ACL_SELECT permission on the given relation.
We can consider its purpose is to check references to attribute of
a certain relation.

* ac_relation_copy_definition()
The transformInhRelation() is the only caller (name is confusable because
it handles LIKE clause in CREATE TABLE statement), and checks ACL_SELECT
permission on the given relation.
We can consider its purpose is to check references to attribute of
a certain relation.

-> These two checks apply identical permissions for similar purpose from
more generalized perspective.
It seems to me these can be replaced by *_relation_getattr().

* ac_database_calculate_size()
The calculate_database_size() is the only caller, and checks ACL_CONNECT
permission on the given database.
We can consider its purpose is to return users attribute of a certain
database.

-> ditto, can be replaced by *_database_getattr()

* ac_tablespace_calculate_size()
The calculate_tablespace_size() is the only caller, and checks ACL_CREAET
permission on the given tablespace.
We can consider its purpose is to return users attribute of a certain
tablespace.

-> ditto, can be replaced by *_tablespace_getattr()

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

OK, should be check_<object class>_<action>()?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>