Re: security hook on table creation

(2010/09/30 10:28), Robert Haas wrote:
> 2010/9/29 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> But with that exception,
>>> they seemed to think that coarse-grained permissions would be fine for
>>> a basic implementation: perhaps even just install something in
>>> ProcessUtility_hook and bounce DDL across the board, so long as it's
>>> controlled by reference to the security policy rather than by DAC. I
>>> think we can do better than that in a pretty short period of time if
>>> we avoid getting side-tracked, but the key is that we have to avoid
>>> getting side-tracked.
>>>
>> In this approach, we eventually need to deploy the hooks on object creation
>> as we are currently working on. So, I don't think using ProcessUtility_hook
>> for coarse-grained permissions is a right direction.
>
> Well, it may be the easiest way to do certain things. For example, if
> you wanted to control access to a command such as LOAD (which
> presumably you do since otherwise a loadable module could trivially
> subvert the security policy), it's unclear to me that there's any need
> for a new hook; ProcessUtility_hook might very well be the best way to
> tackle that. We need to consider the best way to handle each case.
> In some cases, all of the necessary information may not be available
> when ProcessUtility_hook is called, but where it is, we shouldn't
> reinvent the wheel.
>
In the ideal world, I want to put a new hook to control LOAD command,
because the given library name is not expanded at ProcessUtility_hook
time (and expand_dynamic_library_name() is a static function), so
we cannot know enough information to apply fine-grained control;
such as per-libraries-control.

So, right-now, all we can do in coarse-grained permissions are to
prohibit LOAD command always when SE-PgSQL is installed.
(For more detail, it is not perfect because we can overwrite the
'local_shared_libraries' setting using connection string.)

However, we understood we need to prioritize our upcoming works,
and I think the security hooks on table creation has the highest
priority than others.

> With respect to this patch, I think we are on the same page now, with
> possibly some disagreement about how far it makes sense to go with
> this that needn't concern us for the present. I'm going to mark this
> patch Returned with Feedback, because we need to move on to other
> patches that are closer to being committable.
>
OK. I'll refactor my patch set.

| #define InvokeObjectAccessHook(objtype, oid, subid, op) \
| if (object_access_hook != NULL) \
| object_access_hook(objtype, oid, subid, op);

One my preference is functions, rather than macros, because we
need a *.c file somewhere to put pointer variable of the hook
and it will become a good place to describe source code comments
of the hook.

In addition, I want to give these entrypoints its name which
represents an appropriate purpose of the hook, rather than
a uniformed one.

Example:

/*
* This hook is ...
*/
object_access_hook_type object_access_hook = NULL;

/*
* check_relation_create
*
* This hook is invoked when ...
:
:
* If violated, guest of the hook can raise an error.
*/
void
check_relation_create(Oid oid)
{
if (object_access_hook != NULL)
object_access_hook(OBJECT_TABLE, oid, OAT_CREATE);
}

Thanks,
--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>