Re: security hook on table creation

Thanks for your reviewing, and sorry for delayed responding due to
the LinuxCon Japan for a couple of days.

(2010/09/28 12:57), Robert Haas wrote:
> 2010/9/1 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> This patch allows external security providers to check privileges
>> to create a new relation and to inform the security labels to be
>> assigned on the new one.
>
> Review:
>
> I took a brief look at this patch tonight and I think it's on the
> wrong track. There's no reason for the hook function to return the
> list of security labels and then have the core code turn around and
> apply them to the object. If the hook function wants to label the
> object, it can just as easily call SetSecurityLabel() itself.
>
However, it is not actually easy, because we cannot know OID of
the new table before invocation of heap_create_with_catalog().
So, we needed to return a list of security labels to caller of
the hook, then the core core calls SetSecurityLabel() with newly
assigned OID.

I don't think it is an option to move the hook after the pollution
of system catalogs, although we can pull out any information about
the new relation from syscache.

> It seems to me that there is a general pattern to the hooks that are
> needed here. For each object type for which we wish to have MAC
> integration, you need the ability to get control when the object is
> created and again when the object is dropped. You might want to deny
> the operation, apply labels to the newly created object, do some
> logging, or whatever. So it strikes me that you could have a hook
> function with a signature like this:
>
> typedef void (*object_access_hook_type)(ObjectType objtype, Oid oid,
> int subid, ObjectAccessType op);
>
> ...where ObjectAccessType is an enum.
>
> Then you could do something like this:
>
> #define InvokeObjectAccessHook(objtype, oid, subid, op) \
> if (object_access_hook != NULL) \
> object_access_hook(objtype, oid, subid, op);
>
> Then you can sprinkle calls to that macro in strategically chosen
> places to trap create, drop, comment, security label, ... whatever the
> object gets manipulated in a way that something like SE-Linux is apt
> to care about. So ObjectAccessType can have values like OAT_CREATE,
> OAT_DROP, OAT_COMMENT, OAT_SECURITY_LABEL, ...
>
Sorry, it seems to me the idea simplifies the issue too much to implement
access control features correctly.
For example, we need to provide security modules the supplied label on
the SECURITY LABEL hook, so it has to take one more argument at least.
For example, we will need to provide them OID of the new schema on
the ALTER TABLE SET SCHEMA at least too.
:

So, we need to inform the security modules some more extra information
rather than OID of the objects to be referenced.

> I would like to mark this patch Returned with Feedback, because I
> think the above suggestions are going to amount to a complete rewrite.
>
It is too early.

Please consider again the reason why we needed to return a list of
security labels to be assigned on the new relation

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>