Обсуждение: 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@ak.jp.nec.com>
Вложения
2010/11/9 KaiGai Kohei <kaigai@ak.jp.nec.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@ak.jp.nec.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@kaigai.gr.jp>
2010/11/9 KaiGai Kohei <kaigai@ak.jp.nec.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@kaigai.gr.jp>
On Wed, Nov 10, 2010 at 8:33 AM, KaiGai Kohei <kaigai@kaigai.gr.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@kaigai.gr.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@kaigai.gr.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@kaigai.gr.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@ak.jp.nec.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@kaigai.gr.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@ak.jp.nec.com>
Вложения
2010/11/12 KaiGai Kohei <kaigai@ak.jp.nec.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@ak.jp.nec.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@ak.jp.nec.com>
2010/11/23 KaiGai Kohei <kaigai@ak.jp.nec.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@ak.jp.nec.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@ak.jp.nec.com>
Вложения
2010/11/25 KaiGai Kohei <kaigai@ak.jp.nec.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