Re: security hooks on object creation

The attached patch provides plugin modules a hook just after object
creation time. In typical use cases, it enables to assign default
security labels on object creation by the external security providers.

As Robert suggested before, it provides a generic purpose main hook.
It takes an enum of ObjectAccessType which informs plugins what kind
of accesses are required, and identifier of the object to be referenced.
But, in this version, no additional information, such as new name in
ALTER xxx RENAME TO, are not supported.

The ObjectAccessType is defined as follows:

typedef enum ObjectAccessType {
OAT_POST_CREATE, /* Post creation fixups; such as security labeling */
} ObjectAccessType;

We will support more complete kind of access types in the future version,
however, we focus on default labeling rather than DDL permissions right
now, so only OAT_POST_CREATE is defined here.
Perhaps, we will add OAT_ALTER, OAT_DROP, OAT_COMMENT and so on.

In this patch, I put hooks on the place just after creation of database
objects that we can assign security labels. (schema, relation, attribute,
procedure, language, type, large object)

However, I didn't touch or move CommandCounterIncrement() yet, although
we had a long discussion MVCC visibility of new object.
Because I'm not clear whether it is really preferable to inject CCIs
onto random points such as TypeCreate() or ProcedureCreate() under
development of the version killed by myself.
(In other words, it was simply ugly...)

At least, we can see the new entries with SnapshotSelf, although we will
pay performance penalty. If so, it is an idea not to touch anything
related to CCIs.
The purpose of post creation hooks are assignment of default security
labels, not DDL permissions. So, it is not a bad idea not to touch
routines related to CCIs in the earlier version of external security
provider.

In this patch, we put InvokeObjectAccessHook0 on the following functions.

- heap_create_with_catalog() for relations/attributes
- ATExecAddColumn() for attributes
- NamespaceCreate() for schemas
- ProcedureCreate() for aggregates/functions
- TypeCreate() and TypeShellMake() for types
- create_proc_lang() for procedural languages
- inv_create() for large objects

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

2010/11/9 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> The attached patch provides plugin modules a hook just after object
> creation time. In typical use cases, it enables to assign default
> security labels on object creation by the external security providers.

It looks like "DDL Trigger" on other database products.
Do we need to consider both security hooks and DDL triggers now?
Or, is it enough to design DLL triggers after the hooks are merged?
Low-level hooks might be better for security providers because
SQL-level triggers could be uninstall by superusers.

--
Itagaki Takahiro

(2010/11/09 20:34), Itagaki Takahiro wrote:
> 2010/11/9 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> The attached patch provides plugin modules a hook just after object
>> creation time. In typical use cases, it enables to assign default
>> security labels on object creation by the external security providers.
>
> It looks like "DDL Trigger" on other database products.
> Do we need to consider both security hooks and DDL triggers now?
> Or, is it enough to design DLL triggers after the hooks are merged?
> Low-level hooks might be better for security providers because
> SQL-level triggers could be uninstall by superusers.
>
An interesting viewpoint. Does the DDL trigger allow us to do something
on CREATE/ALTER/DROP command?

One thing we need to pay attention is that CREATE command is an exception
from any other DDL commands, because the database object to be modified
does not exist before the actual works. So, I'm saying we need both of
prep/post creation hooks in the world of complete features.
Meanwhile, I don't think we need security hooks post ALTER/DROP commands.
Thus, we will put security hooks next to the existing permission checks,
not after the actual works of these commands.
Is it reasonable for DDL triggers (if it has something like BEFORE/AFTER)?

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

2010/11/9 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> The attached patch provides plugin modules a hook just after object
> creation time. In typical use cases, it enables to assign default
> security labels on object creation by the external security providers.
>
> As Robert suggested before, it provides a generic purpose main hook.
> It takes an enum of ObjectAccessType which informs plugins what kind
> of accesses are required, and identifier of the object to be referenced.
> But, in this version, no additional information, such as new name in
> ALTER xxx RENAME TO, are not supported.
>
> The ObjectAccessType is defined as follows:
>
>  typedef enum ObjectAccessType {
>    OAT_POST_CREATE,    /* Post creation fixups; such as security labeling */
>  } ObjectAccessType;
>
> We will support more complete kind of access types in the future version,
> however, we focus on default labeling rather than DDL permissions right
> now, so only OAT_POST_CREATE is defined here.
> Perhaps, we will add OAT_ALTER, OAT_DROP, OAT_COMMENT and so on.
>
> In this patch, I put hooks on the place just after creation of database
> objects that we can assign security labels. (schema, relation, attribute,
> procedure, language, type, large object)
>
> However, I didn't touch or move CommandCounterIncrement() yet, although
> we had a long discussion MVCC visibility of new object.
> Because I'm not clear whether it is really preferable to inject CCIs
> onto random points such as TypeCreate() or ProcedureCreate() under
> development of the version killed by myself.
> (In other words, it was simply ugly...)
>
> At least, we can see the new entries with SnapshotSelf, although we will
> pay performance penalty. If so, it is an idea not to touch anything
> related to CCIs.
> The purpose of post creation hooks are assignment of default security
> labels, not DDL permissions. So, it is not a bad idea not to touch
> routines related to CCIs in the earlier version of external security
> provider.
>
> In this patch, we put InvokeObjectAccessHook0 on the following functions.
>
> - heap_create_with_catalog() for relations/attributes
> - ATExecAddColumn() for attributes
> - NamespaceCreate() for schemas
> - ProcedureCreate() for aggregates/functions
> - TypeCreate() and TypeShellMake() for types
> - create_proc_lang() for procedural languages
> - inv_create() for large objects

I think you ought to try to arrange to avoid the overhead of a
function call in the common case where nobody's using the hook.
That's why I originally suggested making InvokeObjectAccessHook() a
macro around the actual function call.

I don't want to refer to this as a framework for enhanced security
providers. Let's stick with the term "object access hook". Calling
it an enhanced security provider overspecifies; it could equally well
be used for, say, logging.

Is there any compelling reason not to apply this to every object type
in the system (e.g. all the ones COMMENT can apply to)? I don't see
any reason to restrict it to the set of objects to which it's sensible
to apply security labels.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

(2010/11/10 13:06), Robert Haas wrote:
>> In this patch, we put InvokeObjectAccessHook0 on the following functions.
>>
>> - heap_create_with_catalog() for relations/attributes
>> - ATExecAddColumn() for attributes
>> - NamespaceCreate() for schemas
>> - ProcedureCreate() for aggregates/functions
>> - TypeCreate() and TypeShellMake() for types
>> - create_proc_lang() for procedural languages
>> - inv_create() for large objects
>
> I think you ought to try to arrange to avoid the overhead of a
> function call in the common case where nobody's using the hook.
> That's why I originally suggested making InvokeObjectAccessHook() a
> macro around the actual function call.
>
Hmm. Although I have little preference here, the penalty to call
an empty function (when no plugins are installed) is not visible,
because frequency of DDL commands are not high.
Even so, is it necessary to replace them by macros?

> I don't want to refer to this as a framework for enhanced security
> providers. Let's stick with the term "object access hook". Calling
> it an enhanced security provider overspecifies; it could equally well
> be used for, say, logging.
>
OK. As Itagaki-san also pointed out, we may be able to utilize the hooks
in other purposes. Although I designed it in similar manner with security
label provider, I'll revise it like as other hooks doing.

> Is there any compelling reason not to apply this to every object type
> in the system (e.g. all the ones COMMENT can apply to)? I don't see
> any reason to restrict it to the set of objects to which it's sensible
> to apply security labels.
>
Because I thought too many hooks within one patch gives burden to reviewers,
so I restricted it on a part of object classes in this version.
However,it is not a compelling reason.
OK, I'll try to revise the patch soon.

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

On Wed, Nov 10, 2010 at 8:33 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> (2010/11/10 13:06), Robert Haas wrote:
>>>
>>> In this patch, we put InvokeObjectAccessHook0 on the following functions.
>>>
>>> - heap_create_with_catalog() for relations/attributes
>>> - ATExecAddColumn() for attributes
>>> - NamespaceCreate() for schemas
>>> - ProcedureCreate() for aggregates/functions
>>> - TypeCreate() and TypeShellMake() for types
>>> - create_proc_lang() for procedural languages
>>> - inv_create() for large objects
>>
>> I think you ought to try to arrange to avoid the overhead of a
>> function call in the common case where nobody's using the hook.
>> That's why I originally suggested making InvokeObjectAccessHook() a
>> macro around the actual function call.
>>
> Hmm. Although I have little preference here, the penalty to call
> an empty function (when no plugins are installed) is not visible,
> because frequency of DDL commands are not high.
> Even so, is it necessary to replace them by macros?

It's a fair point. I'm open to other opinions but my vote is to shove
a macro in there. A pointer test is cheaper than a function call, and
doesn't really complicate things much.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

(2010/11/11 3:00), Robert Haas wrote:
> On Wed, Nov 10, 2010 at 8:33 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> (2010/11/10 13:06), Robert Haas wrote:
>>>>
>>>> In this patch, we put InvokeObjectAccessHook0 on the following functions.
>>>>
>>>> - heap_create_with_catalog() for relations/attributes
>>>> - ATExecAddColumn() for attributes
>>>> - NamespaceCreate() for schemas
>>>> - ProcedureCreate() for aggregates/functions
>>>> - TypeCreate() and TypeShellMake() for types
>>>> - create_proc_lang() for procedural languages
>>>> - inv_create() for large objects
>>>
>>> I think you ought to try to arrange to avoid the overhead of a
>>> function call in the common case where nobody's using the hook.
>>> That's why I originally suggested making InvokeObjectAccessHook() a
>>> macro around the actual function call.
>>>
>> Hmm. Although I have little preference here, the penalty to call
>> an empty function (when no plugins are installed) is not visible,
>> because frequency of DDL commands are not high.
>> Even so, is it necessary to replace them by macros?
>
> It's a fair point. I'm open to other opinions but my vote is to shove
> a macro in there. A pointer test is cheaper than a function call, and
> doesn't really complicate things much.
>
Since I have no strong preference function call here, so, I'll revise my
patch according to your vote.

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

I revised my patch according to the prior suggestions.

Invocation of the hooks is encapsulated within macro, not function:

+ #define InvokeObjectAccessHook0(access,classId,objectId,subId) \
+ do { \
+ if (object_access_hook) \
+ (*object_access_hook)((access),(classId),(objectId),(subId)); \
+ } while(0)

The "0" of tail means that it does not takes any arguments except for
object-ids, like syscache code.
It will reduce impact when we want to add arguments of the hooks.

In the previous version, it just support seven object classes that is
allowed to assign security labels. But, Robert pointed out the purpose
of post-creation hook is limited to security labeling. So, I expand its
coverage into all the commentable object classes.

- relations: heap_create_with_catalog()
- constraint: CreateConstraintEntry()
- conversion: ConversionCreate()
- schema: NamespaceCreate()
- operator: OperatorCreate() and OperatorShellMake()
- procedure: ProcedureCreate()
- type: TypeCreate() and TypeShellMake()
- database: createdb()
- cast: CreateCast()
- opfamily: CreateOpFamily()
- opclass: DefineOpClass()
- language: create_proc_lang()
- attribute: ATExecAddColumn()
- tablespace: CreateTableSpace()
- trigger: CreateTrigger()
- ts_parser: DefineTSParser()
- ts_dict: DefineTSDictionary()
- ts_template: DefineTSTemplate()
- ts_config: DefineTSConfiguration()
- role: CreateRole()
- rule: InsertRule()
- largeobject: inv_create()

The post-creation hooks are put on the place just after adding dependency
of the new object, if the object class uses dependency mechanism.
I believe it will be a clear guidance for the future maintenance works.

Thanks,

(2010/11/11 7:41), KaiGai Kohei wrote:
> (2010/11/11 3:00), Robert Haas wrote:
>> On Wed, Nov 10, 2010 at 8:33 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> (2010/11/10 13:06), Robert Haas wrote:
>>>>>
>>>>> In this patch, we put InvokeObjectAccessHook0 on the following functions.
>>>>>
>>>>> - heap_create_with_catalog() for relations/attributes
>>>>> - ATExecAddColumn() for attributes
>>>>> - NamespaceCreate() for schemas
>>>>> - ProcedureCreate() for aggregates/functions
>>>>> - TypeCreate() and TypeShellMake() for types
>>>>> - create_proc_lang() for procedural languages
>>>>> - inv_create() for large objects
>>>>
>>>> I think you ought to try to arrange to avoid the overhead of a
>>>> function call in the common case where nobody's using the hook.
>>>> That's why I originally suggested making InvokeObjectAccessHook() a
>>>> macro around the actual function call.
>>>>
>>> Hmm. Although I have little preference here, the penalty to call
>>> an empty function (when no plugins are installed) is not visible,
>>> because frequency of DDL commands are not high.
>>> Even so, is it necessary to replace them by macros?
>>
>> It's a fair point. I'm open to other opinions but my vote is to shove
>> a macro in there. A pointer test is cheaper than a function call, and
>> doesn't really complicate things much.
>>
> Since I have no strong preference function call here, so, I'll revise my
> patch according to your vote.
>
> Thanks,

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

(2010/11/12 19:34), KaiGai Kohei wrote:
> I revised my patch according to the prior suggestions.
>
I'm sorry. I revised my patch, but not attached.

Please see this attached one.

Thanks,

> Invocation of the hooks is encapsulated within macro, not function:
>
> + #define InvokeObjectAccessHook0(access,classId,objectId,subId) \
> + do { \
> + if (object_access_hook) \
> + (*object_access_hook)((access),(classId),(objectId),(subId)); \
> + } while(0)
>
> The "0" of tail means that it does not takes any arguments except for
> object-ids, like syscache code.
> It will reduce impact when we want to add arguments of the hooks.
>
>
> In the previous version, it just support seven object classes that is
> allowed to assign security labels. But, Robert pointed out the purpose
> of post-creation hook is limited to security labeling. So, I expand its
> coverage into all the commentable object classes.
>
> - relations: heap_create_with_catalog()
> - constraint: CreateConstraintEntry()
> - conversion: ConversionCreate()
> - schema: NamespaceCreate()
> - operator: OperatorCreate() and OperatorShellMake()
> - procedure: ProcedureCreate()
> - type: TypeCreate() and TypeShellMake()
> - database: createdb()
> - cast: CreateCast()
> - opfamily: CreateOpFamily()
> - opclass: DefineOpClass()
> - language: create_proc_lang()
> - attribute: ATExecAddColumn()
> - tablespace: CreateTableSpace()
> - trigger: CreateTrigger()
> - ts_parser: DefineTSParser()
> - ts_dict: DefineTSDictionary()
> - ts_template: DefineTSTemplate()
> - ts_config: DefineTSConfiguration()
> - role: CreateRole()
> - rule: InsertRule()
> - largeobject: inv_create()
>
> The post-creation hooks are put on the place just after adding dependency
> of the new object, if the object class uses dependency mechanism.
> I believe it will be a clear guidance for the future maintenance works.
>
> Thanks,
>
> (2010/11/11 7:41), KaiGai Kohei wrote:
>> (2010/11/11 3:00), Robert Haas wrote:
>>> On Wed, Nov 10, 2010 at 8:33 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> (2010/11/10 13:06), Robert Haas wrote:
>>>>>>
>>>>>> In this patch, we put InvokeObjectAccessHook0 on the following functions.
>>>>>>
>>>>>> - heap_create_with_catalog() for relations/attributes
>>>>>> - ATExecAddColumn() for attributes
>>>>>> - NamespaceCreate() for schemas
>>>>>> - ProcedureCreate() for aggregates/functions
>>>>>> - TypeCreate() and TypeShellMake() for types
>>>>>> - create_proc_lang() for procedural languages
>>>>>> - inv_create() for large objects
>>>>>
>>>>> I think you ought to try to arrange to avoid the overhead of a
>>>>> function call in the common case where nobody's using the hook.
>>>>> That's why I originally suggested making InvokeObjectAccessHook() a
>>>>> macro around the actual function call.
>>>>>
>>>> Hmm. Although I have little preference here, the penalty to call
>>>> an empty function (when no plugins are installed) is not visible,
>>>> because frequency of DDL commands are not high.
>>>> Even so, is it necessary to replace them by macros?
>>>
>>> It's a fair point. I'm open to other opinions but my vote is to shove
>>> a macro in there. A pointer test is cheaper than a function call, and
>>> doesn't really complicate things much.
>>>
>> Since I have no strong preference function call here, so, I'll revise my
>> patch according to your vote.
>>
>> Thanks,
>
>

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

2010/11/12 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> (2010/11/12 19:34), KaiGai Kohei wrote:
>> I revised my patch according to the prior suggestions.
>>
> I'm sorry. I revised my patch, but not attached.
>
> Please see this attached one.

I'm satisfied with this approach, although I intend to change
InvokeObjectAccessHook0 to simply InvokeObjectAccessHook before
committing it; and correct your use of AttributeRelationId to
RelationRelationId for consistency with the rest of the code. What
I'm not quite sure about is where to put the definitions you've added
to a new file utils/hooks.h; I don't feel that's a very appropriate
location. It's tempting to put them in utils/acl.h just because this
is vaguely access-control related and that header is already included
in most of the right places, but maybe that's too much of a stretch;
or perhaps catalog/catalog.h, although that doesn't feel quite right
either. If we are going to add a new header file, I still don't like
utils/hooks.h much - it's considerably more generic than can be
justified by its contents.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Thanks for your reviewing, and sorry for the late reply.
I've not been available for a few days.

(2010/11/22 12:11), Robert Haas wrote:
> 2010/11/12 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> (2010/11/12 19:34), KaiGai Kohei wrote:
>>> I revised my patch according to the prior suggestions.
>>>
>> I'm sorry. I revised my patch, but not attached.
>>
>> Please see this attached one.
>
> I'm satisfied with this approach, although I intend to change
> InvokeObjectAccessHook0 to simply InvokeObjectAccessHook before
> committing it;

OK. We have no other object-access-type which takes any arguments
right now. It is quite cosmetic things, so we may be able to add
the number of arguments later, such as SysCache.

> and correct your use of AttributeRelationId to
> RelationRelationId for consistency with the rest of the code.

Oops, it was my bug. I'll fix it.

> What
> I'm not quite sure about is where to put the definitions you've added
> to a new file utils/hooks.h; I don't feel that's a very appropriate
> location. It's tempting to put them in utils/acl.h just because this
> is vaguely access-control related and that header is already included
> in most of the right places, but maybe that's too much of a stretch;
> or perhaps catalog/catalog.h, although that doesn't feel quite right
> either. If we are going to add a new header file, I still don't like
> utils/hooks.h much - it's considerably more generic than can be
> justified by its contents.
>
I don't think utils/acl.h is long-standing right place, because we
intended not to restrict the purpose of this hooks to access controls
as you mentioned.

I think somewhere under the catalog/ directory is a good idea because
it hooks events that user wants (eventually) to modify system catalogs.
How about catalog/hooks.h, instead of utils/hooks.h?

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

2010/11/23 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> What
>> I'm not quite sure about is where to put the definitions you've added
>> to a new file utils/hooks.h; I don't feel that's a very appropriate
>> location.  It's tempting to put them in utils/acl.h just because this
>> is vaguely access-control related and that header is already included
>> in most of the right places, but maybe that's too much of a stretch;
>> or perhaps catalog/catalog.h, although that doesn't feel quite right
>> either.  If we are going to add a new header file, I still don't like
>> utils/hooks.h much - it's considerably more generic than can be
>> justified by its contents.
>>
> I don't think utils/acl.h is long-standing right place, because we
> intended not to restrict the purpose of this hooks to access controls
> as you mentioned.
>
> I think somewhere under the catalog/ directory is a good idea because
> it hooks events that user wants (eventually) to modify system catalogs.
> How about catalog/hooks.h, instead of utils/hooks.h?

Well, if we're going to create a new header file for this, I think it
should be called something like catalog/objectaccess.h, rather than
just hooks.h. But I'd rather reuse something that's already there,
all things being equal.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

The attached patch is a revised patch.

- The utils/hooks.h was renamed to catalog/objectaccess.h
- Numeric in the tail of InvokeObjectAccessHook0() has gone.
- Fixed bug in ATExecAddColumn; it gave AttributeRelationId
to the hook instead of RelationRelationId.

In addition, I found that we didn't put post-creation hook
on foreign data wrapper, foreign server and user mapping
exceptionally. So, I put this hook around their command
handler like any other object classes.

Thanks,

(2010/11/24 12:07), Robert Haas wrote:
> 2010/11/23 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> What
>>> I'm not quite sure about is where to put the definitions you've added
>>> to a new file utils/hooks.h; I don't feel that's a very appropriate
>>> location. It's tempting to put them in utils/acl.h just because this
>>> is vaguely access-control related and that header is already included
>>> in most of the right places, but maybe that's too much of a stretch;
>>> or perhaps catalog/catalog.h, although that doesn't feel quite right
>>> either. If we are going to add a new header file, I still don't like
>>> utils/hooks.h much - it's considerably more generic than can be
>>> justified by its contents.
>>>
>> I don't think utils/acl.h is long-standing right place, because we
>> intended not to restrict the purpose of this hooks to access controls
>> as you mentioned.
>>
>> I think somewhere under the catalog/ directory is a good idea because
>> it hooks events that user wants (eventually) to modify system catalogs.
>> How about catalog/hooks.h, instead of utils/hooks.h?
>
> Well, if we're going to create a new header file for this, I think it
> should be called something like catalog/objectaccess.h, rather than
> just hooks.h. But I'd rather reuse something that's already there,
> all things being equal.
>

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

2010/11/25 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> The attached patch is a revised patch.
>
> - The utils/hooks.h was renamed to catalog/objectaccess.h
> - Numeric in the tail of InvokeObjectAccessHook0() has gone.
> - Fixed bug in ATExecAddColumn; it gave AttributeRelationId
>  to the hook instead of RelationRelationId.
>
> In addition, I found that we didn't put post-creation hook
> on foreign data wrapper, foreign server and user mapping
> exceptionally. So, I put this hook around their command
> handler like any other object classes.

Committed with minor, mostly cosmetic adjustments.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company