Thread: [v9.2] Object access hooks with arguments support (v1)
The attached patch is a draft to support arguments in addition to OAT_* enum and object identifiers. The existing object_access_hook enables loadable modules to acquire control when objects are referenced. The first guest of this hook is contrib/sepgsql for assignment of default security label on newly created objects. Right now, OAT_POST_CREATE is the all supported object access type. However, we plan to enhance this mechanism onto other widespread purpose; such as comprehensive DDL permissions supported by loadable modules. This patch is a groundwork to utilize this hook for object creation permission checks, not only default labeling. At the v9.1 development cycle, I proposed an idea that defines both OAT_CREATE hook prior to system catalog modification and OAT_POST_CREATE hook as currently we have. This design enables to check permission next to the existing pg_xxx_aclcheck() or pg_xxx_ownercheck(), and raise an error before system catalog updates. However, it was painful to deliver private datum set on OAT_CREATE to the OAT_POST_CREATE due to the code complexity. The other idea tried to do all the necessary things in OAT_POST_CREATE hook, and it had been merged, because loadable modules can pull properties of the new object from system catalogs by the supplied object identifiers. Thus, contrib/sepgsql assigns a default security label on new object using OAT_POST_CREATE hook. However, I have two concern on the existing hook to implement permission check for object creation. The first one is the entry of system catalog is not visible using SnaphotNow, so we had to open and scan system catalog again, instead of syscache mechanism. The second one is more significant. A part of information to make access control decision is not available within contents of the system catalog entries. For example, we hope to skip permission check when heap_create_with_catalog() was launched by make_new_heap() because the new relation is just used to swap later. Thus, I'd like to propose to support argument of object_access_hook to inform the loadable modules additional contextual information on its invocations; to solve these concerns. Regarding to the first concern, fortunately, most of existing OAT_POST_CREATE hook is deployed just after insert or update of system catalogs, but before CCI. So, it is helpful for the loadable modules to deliver Form_pg_xxxx data to reference properties of the new object, instead of open and scan the catalog again. In the draft patch, I enhanced OAT_POST_CREATE hook commonly to take an argument that points to the Form_pg_xxxx data of the new object. Regarding to the second concern, I added a few contextual information as second or later arguments in a part of object classes. Right now, I hope the following contextual information shall be provided to OAT_POST_CREATE hook to implement permission checks of object creation. * pg_class - TupleDesc structure of the new relation I want to reference of pg_attribute, not only pg_class. * pg_class - A flag to show whether the relation is defined for rebuilding, or not. I want not to apply permission check in the case when it is invoked from make_new_heap(), because it just create a dummy table as a part of internal process. All the necessary permission checks should be done at ALTER TABLE or CLUSTER. * pg_class - A flag to show whether the relation is created with SELECT INTO, or not. I want to check "insert" permission of the new table, created by SELECT INTO, because DML hook is not available to check this case. * pg_type - A flag to show whether the type is defined as implicit array, or not. I want not to apply permission check on creation of implicit array type. * pg_database - Oid of the source (template) database. I want to fetch security label of the source database to compute a default label of the new database. * pg_trigger - A flag to show whether the trigger is used to FK constraint, or not. I want not to apply permission check on creation of FK constraint. It should be done in ALTER TABLE level. Sorry for this long explanation. Right now, I tend to consider it is the best way to implement permission checks on object creation with least invasive way. Thanks, Any comments welcome. -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
I noticed that the previous revision does not provide any way to inform the modules name of foreign server, even if foreign table was created, on the OAT_POST_CREATE hook. So, I modified the invocation at heap_create_with_catalog to deliver this information to the modules. Rest of parts were unchanged, except for rebasing to the latest git master. 2011/8/28 Kohei KaiGai <kaigai@kaigai.gr.jp>: > The attached patch is a draft to support arguments in addition to > OAT_* enum and object identifiers. > > The existing object_access_hook enables loadable modules to acquire > control when objects are referenced. The first guest of this hook is > contrib/sepgsql for assignment of default security label on newly > created objects. Right now, OAT_POST_CREATE is the all supported > object access type. However, we plan to enhance this mechanism onto > other widespread purpose; such as comprehensive DDL permissions > supported by loadable modules. > > This patch is a groundwork to utilize this hook for object creation > permission checks, not only default labeling. > At the v9.1 development cycle, I proposed an idea that defines both > OAT_CREATE hook prior to system catalog modification and > OAT_POST_CREATE hook as currently we have. This design enables to > check permission next to the existing pg_xxx_aclcheck() or > pg_xxx_ownercheck(), and raise an error before system catalog updates. > However, it was painful to deliver private datum set on OAT_CREATE to > the OAT_POST_CREATE due to the code complexity. > > The other idea tried to do all the necessary things in OAT_POST_CREATE > hook, and it had been merged, because loadable modules can pull > properties of the new object from system catalogs by the supplied > object identifiers. Thus, contrib/sepgsql assigns a default security > label on new object using OAT_POST_CREATE hook. > However, I have two concern on the existing hook to implement > permission check for object creation. The first one is the entry of > system catalog is not visible using SnaphotNow, so we had to open and > scan system catalog again, instead of syscache mechanism. The second > one is more significant. A part of information to make access control > decision is not available within contents of the system catalog > entries. For example, we hope to skip permission check when > heap_create_with_catalog() was launched by make_new_heap() because the > new relation is just used to swap later. > > Thus, I'd like to propose to support argument of object_access_hook to > inform the loadable modules additional contextual information on its > invocations; to solve these concerns. > > Regarding to the first concern, fortunately, most of existing > OAT_POST_CREATE hook is deployed just after insert or update of system > catalogs, but before CCI. So, it is helpful for the loadable modules > to deliver Form_pg_xxxx data to reference properties of the new > object, instead of open and scan the catalog again. > In the draft patch, I enhanced OAT_POST_CREATE hook commonly to take > an argument that points to the Form_pg_xxxx data of the new object. > > Regarding to the second concern, I added a few contextual information > as second or later arguments in a part of object classes. Right now, I > hope the following contextual information shall be provided to > OAT_POST_CREATE hook to implement permission checks of object > creation. > > * pg_class - TupleDesc structure of the new relation > I want to reference of pg_attribute, not only pg_class. > > * pg_class - A flag to show whether the relation is defined for > rebuilding, or not. > I want not to apply permission check in the case when it is invoked > from make_new_heap(), because it just create a dummy table as a part > of internal process. All the necessary permission checks should be > done at ALTER TABLE or CLUSTER. > And, name of the foreign server being used by the foreign table being created just a moment before. > * pg_class - A flag to show whether the relation is created with > SELECT INTO, or not. > I want to check "insert" permission of the new table, created by > SELECT INTO, because DML hook is not available to check this case. > > * pg_type - A flag to show whether the type is defined as implicit > array, or not. > I want not to apply permission check on creation of implicit array type. > > * pg_database - Oid of the source (template) database. > I want to fetch security label of the source database to compute a > default label of the new database. > > * pg_trigger - A flag to show whether the trigger is used to FK > constraint, or not. > I want not to apply permission check on creation of FK constraint. It > should be done in ALTER TABLE level. > > Sorry for this long explanation. Right now, I tend to consider it is > the best way to implement permission checks on object creation with > least invasive way. > > Thanks, Any comments welcome. > -- > KaiGai Kohei <kaigai@kaigai.gr.jp> > -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
BTW, I remember that I was suggested the object-access-hooks to acquire controls around changes of system catalogs are also useful to implement clustering features, not only enhanced security features, when I had a talk at PGcon2001. It might be my mistake that I categorized this patch at the "security" topic. If someone volunteers to review this patch from the different viewpoint, not only security features, it is quite helpful for us. Thanks, 2011/9/29 Kohei KaiGai <kaigai@kaigai.gr.jp>: > I noticed that the previous revision does not provide any way to inform > the modules name of foreign server, even if foreign table was created, > on the OAT_POST_CREATE hook. > So, I modified the invocation at heap_create_with_catalog to deliver > this information to the modules. > > Rest of parts were unchanged, except for rebasing to the latest git > master. > > 2011/8/28 Kohei KaiGai <kaigai@kaigai.gr.jp>: >> The attached patch is a draft to support arguments in addition to >> OAT_* enum and object identifiers. >> >> The existing object_access_hook enables loadable modules to acquire >> control when objects are referenced. The first guest of this hook is >> contrib/sepgsql for assignment of default security label on newly >> created objects. Right now, OAT_POST_CREATE is the all supported >> object access type. However, we plan to enhance this mechanism onto >> other widespread purpose; such as comprehensive DDL permissions >> supported by loadable modules. >> >> This patch is a groundwork to utilize this hook for object creation >> permission checks, not only default labeling. >> At the v9.1 development cycle, I proposed an idea that defines both >> OAT_CREATE hook prior to system catalog modification and >> OAT_POST_CREATE hook as currently we have. This design enables to >> check permission next to the existing pg_xxx_aclcheck() or >> pg_xxx_ownercheck(), and raise an error before system catalog updates. >> However, it was painful to deliver private datum set on OAT_CREATE to >> the OAT_POST_CREATE due to the code complexity. >> >> The other idea tried to do all the necessary things in OAT_POST_CREATE >> hook, and it had been merged, because loadable modules can pull >> properties of the new object from system catalogs by the supplied >> object identifiers. Thus, contrib/sepgsql assigns a default security >> label on new object using OAT_POST_CREATE hook. >> However, I have two concern on the existing hook to implement >> permission check for object creation. The first one is the entry of >> system catalog is not visible using SnaphotNow, so we had to open and >> scan system catalog again, instead of syscache mechanism. The second >> one is more significant. A part of information to make access control >> decision is not available within contents of the system catalog >> entries. For example, we hope to skip permission check when >> heap_create_with_catalog() was launched by make_new_heap() because the >> new relation is just used to swap later. >> >> Thus, I'd like to propose to support argument of object_access_hook to >> inform the loadable modules additional contextual information on its >> invocations; to solve these concerns. >> >> Regarding to the first concern, fortunately, most of existing >> OAT_POST_CREATE hook is deployed just after insert or update of system >> catalogs, but before CCI. So, it is helpful for the loadable modules >> to deliver Form_pg_xxxx data to reference properties of the new >> object, instead of open and scan the catalog again. >> In the draft patch, I enhanced OAT_POST_CREATE hook commonly to take >> an argument that points to the Form_pg_xxxx data of the new object. >> >> Regarding to the second concern, I added a few contextual information >> as second or later arguments in a part of object classes. Right now, I >> hope the following contextual information shall be provided to >> OAT_POST_CREATE hook to implement permission checks of object >> creation. >> >> * pg_class - TupleDesc structure of the new relation >> I want to reference of pg_attribute, not only pg_class. >> >> * pg_class - A flag to show whether the relation is defined for >> rebuilding, or not. >> I want not to apply permission check in the case when it is invoked >> from make_new_heap(), because it just create a dummy table as a part >> of internal process. All the necessary permission checks should be >> done at ALTER TABLE or CLUSTER. >> > And, name of the foreign server being used by the foreign table > being created just a moment before. > >> * pg_class - A flag to show whether the relation is created with >> SELECT INTO, or not. >> I want to check "insert" permission of the new table, created by >> SELECT INTO, because DML hook is not available to check this case. >> >> * pg_type - A flag to show whether the type is defined as implicit >> array, or not. >> I want not to apply permission check on creation of implicit array type. >> >> * pg_database - Oid of the source (template) database. >> I want to fetch security label of the source database to compute a >> default label of the new database. >> >> * pg_trigger - A flag to show whether the trigger is used to FK >> constraint, or not. >> I want not to apply permission check on creation of FK constraint. It >> should be done in ALTER TABLE level. >> >> Sorry for this long explanation. Right now, I tend to consider it is >> the best way to implement permission checks on object creation with >> least invasive way. >> >> Thanks, Any comments welcome. >> -- >> KaiGai Kohei <kaigai@kaigai.gr.jp> >> > -- > KaiGai Kohei <kaigai@kaigai.gr.jp> > -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Thu, Sep 29, 2011 at 4:52 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> I noticed that the previous revision does not provide any way to inform
> the modules name of foreign server, even if foreign table was created,
> on the OAT_POST_CREATE hook.
> So, I modified the invocation at heap_create_with_catalog to deliver
> this information to the modules.
>
> Rest of parts were unchanged, except for rebasing to the latest git
> master.
I've never really been totally sanguine with the idea of making object
access hooks take arguments, and all of my general concerns seem to
apply to the way you've set this patch up. In particular:
1. Type safety goes out the window. What you're essentially proposing
here is that we should have one hook function can be used for almost
anything at all and can be getting up to 9 arguments of any type
whatsoever. The hook will need to know how to interpret all of its
arguments depending on the context in which it was called. The
compiler will be totally unable to do any static type-checking, so
there will be absolutely no warning if the interface is changed
incompatibly on either side. Instead, you'll probably just crash the
database server at runtime.
2. The particular choice of data being passed to the object access
hooks appears capricious and arbitrary. Here's an example:
/* Post creation hook for new relation */
- InvokeObjectAccessHook(OAT_POST_CREATE, RelationRelationId, relid, 0);
+ InvokeObjectAccessHook4(OAT_POST_CREATE,
+
RelationRelationId, relid, 0,
+
PointerGetDatum(new_rel_tup),
+
PointerGetDatum(tupdesc),
+
BoolGetDatum(is_select_into),
+
CStringGetDatum(fdw_srvname));
Now, I am sure that you have good reasons for wanting to pass those
particular things to the object access hook rather than any other
local variable or argument that might happen to be lying around at
this point in the code, but they are not documented. If someone adds
a new argument to this function, or removes an argument that's being
passed, they will have no idea what to do about this. Moreover, if
you did document it, I think it would boil down to "this is what
sepgsql happens to need", and I don't think that's an acceptable
answer. We have repeatedly refused to adopt that approach in the
past.
(This particular case is worse than average, because new_rel_tup is
coming from InsertPgClassTuple, which therefore has to be modified,
along with AddNewRelationTuple and index_create, to get the tuple back
up to the call site where, apparently, it is needed.)
I am not exactly sure what the right way to solve this problem is, but
I don't think this is it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert,
I agree with it is a reasonable argument that compiler cannot raise warnings
if all the arguments are delivered as Datum. In fact, I also tried to implement
this feature with InvokeObjectAccessHook() defined as function.
The first needed point to be improved is that we hope compiler to raise
warnnings if and when number or type of arguments are changed in the
future version.
If so, how about an idea to define data types to inform modules the context
information in, such as:
struct ObjectAccessInfoData { ObjectAccessType oa_type; ObjectAddress oa_address; union
{ struct { HeapTuple newtuple; TupleDesc tupdesc; /* only if create a
newrelation */ : } post_create; /* additional info for OAT_POST_CREATE event */
} }
Even if its invocation shall be wrapped up by macro, compiler can
raise warnings when caller set values on the ObjectAccessInfoData
structure.
It will also help the second points partially. The objectaccess.h will perform
a placeholder to describe specification of the argument. That clearly informs
developers what informations are available on the hook and what was lacked.
> Moreover, if
> you did document it, I think it would boil down to "this is what
> sepgsql happens to need", and I don't think that's an acceptable
> answer. ?We have repeatedly refused to adopt that approach in the
> past.
>
Unfortunately, I still don't have a clear answer for this point.
However, in general terms, it is impossible to design any interface without
knowledge of its usage more or less.
We have several other hooks being available to loadable modules,
but I don't believe that we designed these hooks without knowledge
of use-cases, more or less.
At least, this proposition enables modules being informed using
commonly used data type (such as HeapTuple, TupleDesc), compared
to the past approach that tried to push all the necessary information
into argument list individually.
I think the answer of this matter is on the middle-of-position between
"we should not know anything about sepgsql" and "we should know
everything about sepgsql", neither of them....
> (This particular case is worse than average, because new_rel_tup is
> coming from InsertPgClassTuple, which therefore has to be modified,
> along with AddNewRelationTuple and index_create, to get the tuple back
> up to the call site where, apparently, it is needed.)
>
It might be a wrong design. All we want to inform was stored within
new_rel_desc in heap_create_with_catalog(). So, I should design the
hook to deliver new_rel_desc, instead of HeapTuple itself that need to
pull up from InsertPgClassTuple.
Thanks,
2011/10/12 Robert Haas <robertmhaas@gmail.com>:
> On Thu, Sep 29, 2011 at 4:52 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> I noticed that the previous revision does not provide any way to inform
>> the modules name of foreign server, even if foreign table was created,
>> on the OAT_POST_CREATE hook.
>> So, I modified the invocation at heap_create_with_catalog to deliver
>> this information to the modules.
>>
>> Rest of parts were unchanged, except for rebasing to the latest git
>> master.
>
> I've never really been totally sanguine with the idea of making object
> access hooks take arguments, and all of my general concerns seem to
> apply to the way you've set this patch up. In particular:
>
> 1. Type safety goes out the window. What you're essentially proposing
> here is that we should have one hook function can be used for almost
> anything at all and can be getting up to 9 arguments of any type
> whatsoever. The hook will need to know how to interpret all of its
> arguments depending on the context in which it was called. The
> compiler will be totally unable to do any static type-checking, so
> there will be absolutely no warning if the interface is changed
> incompatibly on either side. Instead, you'll probably just crash the
> database server at runtime.
>
> 2. The particular choice of data being passed to the object access
> hooks appears capricious and arbitrary. Here's an example:
>
> /* Post creation hook for new relation */
> - InvokeObjectAccessHook(OAT_POST_CREATE, RelationRelationId, relid, 0);
> + InvokeObjectAccessHook4(OAT_POST_CREATE,
> +
> RelationRelationId, relid, 0,
> +
> PointerGetDatum(new_rel_tup),
> +
> PointerGetDatum(tupdesc),
> +
> BoolGetDatum(is_select_into),
> +
> CStringGetDatum(fdw_srvname));
>
> Now, I am sure that you have good reasons for wanting to pass those
> particular things to the object access hook rather than any other
> local variable or argument that might happen to be lying around at
> this point in the code, but they are not documented. If someone adds
> a new argument to this function, or removes an argument that's being
> passed, they will have no idea what to do about this. Moreover, if
> you did document it, I think it would boil down to "this is what
> sepgsql happens to need", and I don't think that's an acceptable
> answer. We have repeatedly refused to adopt that approach in the
> past.
>
> (This particular case is worse than average, because new_rel_tup is
> coming from InsertPgClassTuple, which therefore has to be modified,
> along with AddNewRelationTuple and index_create, to get the tuple back
> up to the call site where, apparently, it is needed.)
>
> I am not exactly sure what the right way to solve this problem is, but
> I don't think this is it.
>
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
On Thu, Oct 13, 2011 at 6:48 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> struct ObjectAccessInfoData {
> ObjectAccessType oa_type;
> ObjectAddress oa_address;
> union {
> struct {
> HeapTuple newtuple;
> TupleDesc tupdesc; /* only if create a new relation */
> :
> } post_create; /* additional info for OAT_POST_CREATE event */
> }
> }
That's possibly an improvement over just passing everything opaquely,
but it still doesn't seem very good. I mean, this is C, not Perl or
Python. Anything where you pass a bunch of arguments of indeterminate
type and hope that the person you're calling can figure it out is
inherently pretty dangerous. I had it in mind that the object access
hook mechanism could serve as a simple and generic way of letting an
external module know that an event of a certain type has occurred on a
certain object, and to let that module gain control. But if we have
to pass a raft of arguments in, then it's not generic any more - it's
just a bunch of things that are probably really need to be separate
hooks shoved into a single function.
>> Moreover, if
>> you did document it, I think it would boil down to "this is what
>> sepgsql happens to need", and I don't think that's an acceptable
>> answer. ?We have repeatedly refused to adopt that approach in the
>> past.
>>
> Unfortunately, I still don't have a clear answer for this point.
> However, in general terms, it is impossible to design any interface without
> knowledge of its usage more or less.
Well, that's true. But the hook also has to be maintainable. ISTM
that there's no reasonable way for someone making a modification to
the code to know whether the additional local variable that they just
added to the function should be passed to the hook, or not. Here, at
least as far as I can see, there's no guiding principle that would
enable future contributors to make an intelligent decision about that.And if someone wanted to write another client for
thehook, it's not
very obvious whether the particular things you've chosen to pass here
would be relevant or not. I think if you look over the code you'll
find that there's nowhere else that we have a hook that looks anything
like what you're proposing here.
> At least, this proposition enables modules being informed using
> commonly used data type (such as HeapTuple, TupleDesc), compared
> to the past approach that tried to push all the necessary information
> into argument list individually.
That does seem like a good direction to go in, but you're still
passing a lot of other stuff in there. I guess my feeling is that if
we can't boil down the argument list to a short list of things that
are more-or-less common to all the call sites, we probably need to
rethink the whole design.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
2011/10/18 Robert Haas <robertmhaas@gmail.com>:
> On Thu, Oct 13, 2011 at 6:48 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> struct ObjectAccessInfoData {
>> ObjectAccessType oa_type;
>> ObjectAddress oa_address;
>> union {
>> struct {
>> HeapTuple newtuple;
>> TupleDesc tupdesc; /* only if create a new relation */
>> :
>> } post_create; /* additional info for OAT_POST_CREATE event */
>> }
>> }
>
> That's possibly an improvement over just passing everything opaquely,
> but it still doesn't seem very good. I mean, this is C, not Perl or
> Python. Anything where you pass a bunch of arguments of indeterminate
> type and hope that the person you're calling can figure it out is
> inherently pretty dangerous. I had it in mind that the object access
> hook mechanism could serve as a simple and generic way of letting an
> external module know that an event of a certain type has occurred on a
> certain object, and to let that module gain control. But if we have
> to pass a raft of arguments in, then it's not generic any more - it's
> just a bunch of things that are probably really need to be separate
> hooks shoved into a single function.
>
I got an inspiration of this idea from APIs of foreign-data-wrapper that
trusts values of each fields that contains function pointers.
Indeed, we may have a risk of backend crash, if we try to run modules
built towards v9.1 on the pgsql-v9.2devel. However, it is not avoidable,
unless we rewrite whole of the code by Java?, Python? or something? :-(
At least, we don't promise binary compatible across major versions?
If so, I don't think dangerousness is a reasonable reason why the hook
does not take any additional arguments, as long as auther of modules
can be noticed by compiler warnings.
The point to be focused on is how do we implement the target feature
(DDL permissions by sepgsql in my case) with simpleness.
>>> Moreover, if
>>> you did document it, I think it would boil down to "this is what
>>> sepgsql happens to need", and I don't think that's an acceptable
>>> answer. ?We have repeatedly refused to adopt that approach in the
>>> past.
>>>
>> Unfortunately, I still don't have a clear answer for this point.
>> However, in general terms, it is impossible to design any interface without
>> knowledge of its usage more or less.
>
> Well, that's true. But the hook also has to be maintainable. ISTM
> that there's no reasonable way for someone making a modification to
> the code to know whether the additional local variable that they just
> added to the function should be passed to the hook, or not. Here, at
> least as far as I can see, there's no guiding principle that would
> enable future contributors to make an intelligent decision about that.
> And if someone wanted to write another client for the hook, it's not
> very obvious whether the particular things you've chosen to pass here
> would be relevant or not. I think if you look over the code you'll
> find that there's nowhere else that we have a hook that looks anything
> like what you're proposing here.
>
In the case when the second client of the hook is proposed, a straight-
forward approach is to expand the above ObjectAccessInfoData to
satisfy the requirement of new one, if existing arguments are not enough.
Because existing modules ignore the new fields, it is harmless.
Linux adopts this approch to host multiple security modules without
confusion, although one makes decision by security label and one
other makes decision by pathname of files; they require different
information on the point to make its decision, because they ignore
unrelevant arguments; such as pathname in selinux.
I remember you worry about the number of arguments getting increased
infinity, however, it is an extreme situation. We are not an A.I to commit
proposed patches automatically, so auther of modules (including me)
will explain why the new arguments are additionally needed here, like as
pathname based security module did in Linux kernel development.
>> At least, this proposition enables modules being informed using
>> commonly used data type (such as HeapTuple, TupleDesc), compared
>> to the past approach that tried to push all the necessary information
>> into argument list individually.
>
> That does seem like a good direction to go in, but you're still
> passing a lot of other stuff in there. I guess my feeling is that if
> we can't boil down the argument list to a short list of things that
> are more-or-less common to all the call sites, we probably need to
> rethink the whole design.
>
Honestly, the arguments of object_access_hook is a method to implement
security model of sepgsql, not a purpose of mine. So, an alternative idea is
quite welcome to implement sepgsql.
However, I think it is less invasive way than other ideas right now.
For example, I hope sepgsql to perform as follows when user create a new table.
- It computes a default security label that needs Oid of the namespace.
- It checks db_table:{create} permission on the security label being computed.
- In addition, it checks db_table:{insert} permission when SELECT INTO
- Also, it checks these permissions on columns being newly created.
- However, It does not check permissions when the tables are internally created, such as toast tables or temporary
relationscreated by make_new_heap().
Although table creation is the most complex case, it allows us to implement
with just three additional aguments. I tried to implement to deploy separate
hooks on the code path on DefineRelation and OpenIntoRel, but I didn't feel
it is enough simple than the latest approach.
Unfortunately, I have no idea to find out these "contextual information" being
commonly used for all the object classes, so I tried to define my example
ObjectAccessInfoData structure with optional fields depending on event
type and object class.
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
On Tue, Oct 18, 2011 at 11:25 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> For example, I hope sepgsql to perform as follows when user create a new table.
> - It computes a default security label that needs Oid of the namespace.
> - It checks db_table:{create} permission on the security label being computed.
> - In addition, it checks db_table:{insert} permission when SELECT INTO
> - Also, it checks these permissions on columns being newly created.
> - However, It does not check permissions when the tables are internally created,
> such as toast tables or temporary relations created by make_new_heap().
That's superficially reasonable, but I don't feel good about passing
is_select_info to the object access hook here. If insert permission
is required there, then it ought to be getting checked by selinux at
the same place that it's getting checked for at the DAC level. If we
get into a situation where sepgsql is checking permissions using some
system that's totally different from what we do for DAC, I think
that's going to produce confusing and illogical results. This is
ground we've been over before. Similarly with fdw_srvname. The only
excuse for passing that is to handle a permissions check for foreign
data wrappers that isn't being done for DAC: if there is a DAC
permissions check, then the MAC one should be done there also, not
someplace totally different.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
2011/10/18 Robert Haas <robertmhaas@gmail.com>:
> On Tue, Oct 18, 2011 at 11:25 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> For example, I hope sepgsql to perform as follows when user create a new table.
>> - It computes a default security label that needs Oid of the namespace.
>> - It checks db_table:{create} permission on the security label being computed.
>> - In addition, it checks db_table:{insert} permission when SELECT INTO
>> - Also, it checks these permissions on columns being newly created.
>> - However, It does not check permissions when the tables are internally created,
>> such as toast tables or temporary relations created by make_new_heap().
>
> That's superficially reasonable, but I don't feel good about passing
> is_select_info to the object access hook here. If insert permission
> is required there, then it ought to be getting checked by selinux at
> the same place that it's getting checked for at the DAC level. If we
> get into a situation where sepgsql is checking permissions using some
> system that's totally different from what we do for DAC, I think
> that's going to produce confusing and illogical results. This is
> ground we've been over before. Similarly with fdw_srvname. The only
> excuse for passing that is to handle a permissions check for foreign
> data wrappers that isn't being done for DAC: if there is a DAC
> permissions check, then the MAC one should be done there also, not
> someplace totally different.
>
If you are suggesting DAC and MAC permissions should be checked
on the same place like as we already doing at ExecCheckRTPerms(),
I'd like to agree with the suggestion, rather than all the checks within
object_access_hook, although it will take code reworking around
access controls.
In the example table creation, heap_create_with_catalog() is invoked
by 5 routines, however, 3 of them are just internal usages, so it is not
preferable to apply permission checks on table creation....
If we may have a hook on table creation next to the place currently
we checks permission on the namespace being created, it is quite
helpful to implement sepgsql.
BTW, it seems to me SELECT INTO should also check insert permission
on DAC level, because it become an incorrect assumption that owner of
table has insert permission on its creation time.
postgres=# ALTER DEFAULT PRIVILEGES FOR USER tak GRANT select ON TABLES TO tak;
ALTER DEFAULT PRIVILEGES
postgres=# \ddp Default access privilegesOwner | Schema | Type | Access privileges
-------+--------+-------+-------------------tak | | table | tak=r/tak
(1 row)
postgres=# SET SESSION AUTHORIZATION tak;
SET
postgres=> SELECT * INTO t1 FROM (VALUES(1,'aaa'), (2,'bbb'), (3,'ccc')) AS v;
SELECT 3
postgres=> SELECT * FROM t1;column1 | column2
---------+--------- 1 | aaa 2 | bbb 3 | ccc
(3 rows)
postgres=> INSERT INTO t1 VALUES (4,'ddd');
ERROR: permission denied for relation t1
Oops!
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
On Tue, Oct 18, 2011 at 1:23 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > If you are suggesting DAC and MAC permissions should be checked > on the same place like as we already doing at ExecCheckRTPerms(), > I'd like to agree with the suggestion, rather than all the checks within > object_access_hook, although it will take code reworking around > access controls. Agreed. > In the example table creation, heap_create_with_catalog() is invoked > by 5 routines, however, 3 of them are just internal usages, so it is not > preferable to apply permission checks on table creation.... Some wit once made the remark that if a function has 10 arguments, it has 11 arguments, meaning that functions with very large numbers of arguments typically indicate a poor choice of abstraction boundary. That's pretty much what I think is going on with heap_create_with_catalog(). I think maybe some refactoring is in order there, though I am not sure quite what. > If we may have a hook on table creation next to the place currently > we checks permission on the namespace being created, it is quite > helpful to implement sepgsql. Makes sense. > BTW, it seems to me SELECT INTO should also check insert permission > on DAC level, because it become an incorrect assumption that owner of > table has insert permission on its creation time. > > postgres=# ALTER DEFAULT PRIVILEGES FOR USER tak GRANT select ON TABLES TO tak; > ALTER DEFAULT PRIVILEGES > postgres=# \ddp > Default access privileges > Owner | Schema | Type | Access privileges > -------+--------+-------+------------------- > tak | | table | tak=r/tak > (1 row) > > postgres=# SET SESSION AUTHORIZATION tak; > SET > postgres=> SELECT * INTO t1 FROM (VALUES(1,'aaa'), (2,'bbb'), (3,'ccc')) AS v; > SELECT 3 > postgres=> SELECT * FROM t1; > column1 | column2 > ---------+--------- > 1 | aaa > 2 | bbb > 3 | ccc > (3 rows) > > postgres=> INSERT INTO t1 VALUES (4,'ddd'); > ERROR: permission denied for relation t1 > > Oops! You could make the argument that there's no real security hole there, because the user could give himself those same privileges right back. You could also make the argument that a SELECT .. INTO is not the same as an INSERT and therefore INSERT privileges shouldn't be checked. I think there are valid arguments on both sides, but my main point is that we shouldn't have core do it one way and sepgsql do it the other way. That's going to be impossible to maintain and doesn't really make any logical sense anyway. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/10/18 Robert Haas <robertmhaas@gmail.com>: >> In the example table creation, heap_create_with_catalog() is invoked >> by 5 routines, however, 3 of them are just internal usages, so it is not >> preferable to apply permission checks on table creation.... > > Some wit once made the remark that if a function has 10 arguments, it > has 11 arguments, meaning that functions with very large numbers of > arguments typically indicate a poor choice of abstraction boundary. > That's pretty much what I think is going on with > heap_create_with_catalog(). I think maybe some refactoring is in > order there, though I am not sure quite what. > Sorry, are you complained about the number of arguments in heap_create_with_catalog? or hooks of security checks? If we try to rework access control logic around DefineRelation and OpenIntoRel to host both of DAC and MAC, it will takes a long arguments list because an entry of pg_class catalog is not constructed at the timing, so we need to inform the routine all the parameters of new relation required to both DAC and MAC. Thus, it takes a long argument list, however, number of arguments of heap_create_with_catalog will not increased so much. (Maybe, it additionally requires a private data to deliver security labels to be assigned on the new relation.) If we relocate logics of DAC into heap_create_with_catalog(), it will need additional two arguments, though it has 19 arguments right now. It is not a strange idea to inform routines that modified catalogs whether this context needs permission checks, or not, because CreateTrigger has a flag of "isInternal" to skip permission check. >> BTW, it seems to me SELECT INTO should also check insert permission >> on DAC level, because it become an incorrect assumption that owner of >> table has insert permission on its creation time. : > You could make the argument that there's no real security hole there, > because the user could give himself those same privileges right back. > You could also make the argument that a SELECT .. INTO is not the same > as an INSERT and therefore INSERT privileges shouldn't be checked. I > think there are valid arguments on both sides, but my main point is > that we shouldn't have core do it one way and sepgsql do it the other > way. That's going to be impossible to maintain and doesn't really > make any logical sense anyway. > My point is that we should minimize the code complexity, redundancy or others between DAC and MAC implementation, however, it is not possible to get their different to zero due to the differences of their standpoint. Yes, I never say SELECT INTO without DAC checks cause actual security hole, because owner can change its access permissions by himself, unlike MAC. Please suppose that there are two security labels: confidential table (TC) and public table (PT). Typical MAC policy (including selinux) does not allow users who can read from tables with TC to write out data into tables with PT, because PT is readable for public as literal, so confidential data may be leaked to public domain in the result. It is a significant characteristic of MAC; called as data-flow-control. So, it damages significant part of its worth, if MAC could not distinct CREATE TABLE from SELECT INTO in PostgreSQL, and it is the reason why I strongly requires a flag of contextual information here. Although you say it is not possible to maintain, doesn't the above introduction help us to understand nothing why MAC needs to distinct SELECT INTO from CREATE TABLE?, and why it needs a flag to distinct two cases? Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Wed, Oct 19, 2011 at 6:18 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > 2011/10/18 Robert Haas <robertmhaas@gmail.com>: >>> In the example table creation, heap_create_with_catalog() is invoked >>> by 5 routines, however, 3 of them are just internal usages, so it is not >>> preferable to apply permission checks on table creation.... >> >> Some wit once made the remark that if a function has 10 arguments, it >> has 11 arguments, meaning that functions with very large numbers of >> arguments typically indicate a poor choice of abstraction boundary. >> That's pretty much what I think is going on with >> heap_create_with_catalog(). I think maybe some refactoring is in >> order there, though I am not sure quite what. >> > Sorry, are you complained about the number of arguments in > heap_create_with_catalog? or hooks of security checks? I'm just saying that heap_create_with_catalog() is big and complex and does a lot of different things. The fact it does security checks even though it's also sometimes called as an internal function strikes me as a modularity violation. Normally I would expect to have a high-level routine (which is invoked more or less directly from SQL) that does security checks and then calls a low-level routine (that actually does the work) if they pass. Other parts of the code that want to perform the same operation without the security checks can call the low-level function directly. But that's not the way it's set up here. > Yes, I never say SELECT INTO without DAC checks cause actual > security hole, because owner can change its access permissions by > himself, unlike MAC. > Please suppose that there are two security labels: confidential table > (TC) and public table (PT). Typical MAC policy (including selinux) > does not allow users who can read from tables with TC to write out > data into tables with PT, because PT is readable for public as literal, > so confidential data may be leaked to public domain in the result. > > It is a significant characteristic of MAC; called as data-flow-control. > So, it damages significant part of its worth, if MAC could not distinct > CREATE TABLE from SELECT INTO in PostgreSQL, and it is the > reason why I strongly requires a flag of contextual information here. > > Although you say it is not possible to maintain, doesn't the above > introduction help us to understand nothing why MAC needs to > distinct SELECT INTO from CREATE TABLE?, and why it needs > a flag to distinct two cases? Sure. But what is going to happen when someone needs to modify that code in a year or two? In order to understand what to do with the object access hook, they're going to need to understand all those details you just mentioned. And those details do not exist in the patch as submitted. Worse, let's suppose we'd committed a patch like the one you have here before foreign tables went in. Then, whoever added foreign tables would have needed to know to add the foreign table server name to the object access hook invocation, because apparently sepgsql needs that. How would they know they needed to do that? I've committed practically all of the sepgsql-related patches, and I would not have known that, so it seems likely to me that other people adding new functionality to the server wouldn't know it either. When someone comes along in another year or two and adds materialized views, will they need to pass some additional data to the object access hook? Probably, but I bet you're the only one who can quickly figure out what it is. That's no good. We're not going to make changes to PostgreSQL core that only you can maintain, and that are likely to be broken by future commits. At this point I feel pretty good that someone can look at the stuff that we've done so far with SECURITY LABEL and the object access hooks, and if they add a new object type, they can make those things apply to the new object type too by copying what's already there, without making any reference to the sepgsql code. There's a clear abstraction boundary, and people who are working on one side of the boundary do not need to understand the details of what happens on the other side of the boundary. In this particular case, I think it might be reasonable to change the DAC behavior, so that a CREATE TABLE AS SELECT or SELECT INTO requires insert privileges on the new table as well as permission to create it in the first place. I don't particularly see any reason to require different privileges for CREATE TABLE followed by INSERT .. SELECT than what we require when the two commands are rolled into one. Prior to 9.0, this case couldn't arise, because we didn't have default privileges, so I'm inclined to think that the way it works now is a historical accident rather than a deliberate design decision. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> When someone comes along in another year or two and adds materialized > views, will they need to pass some additional data to the object > access hook? Probably, but I bet you're the only one who can quickly > figure out what it is. That's no good. We're not going to make > changes to PostgreSQL core that only you can maintain, and that are > likely to be broken by future commits. At this point I feel pretty > good that someone can look at the stuff that we've done so far with > SECURITY LABEL and the object access hooks, and if they add a new > object type, they can make those things apply to the new object type > too by copying what's already there, without making any reference to > the sepgsql code. There's a clear abstraction boundary, and people > who are working on one side of the boundary do not need to understand > the details of what happens on the other side of the boundary. > I had checked my older implementation based on 8.4.x or 9.0.x that includes all the features that I want to implement. At least, it does not require so much different information from ones needed by DAC model, although SELECT INTO was an exception. It might be quite natural because both works similar things. For example, sepgsql required Oid of source database to compute default security label on new database at createdb(). It was used to permission checks in DAC model also. For another example, sepgsql also required Oids of type-functions to check permissions on them at DefineType(). It was also used to DAC model except for these checks were commented out by #ifdef NOT_USED because of superuser() was already checked. So, how do we launch this efforts according to the principles: - Hooks being used to security checks also should be deployed around existing DAC checks. - The delivered arguments should not be model specific. I don't have clear idea to rework existing routines like as I proposed long before; that wrap-up a series of DAC checks and entrypoint of MAC hooks into a single function. A straightforward idea is to deploy object-access-hook around existing DAC checks with new OAT_* label, such as OAT_CREATE. In the case of relation creation, it shall be DefineRelation() and OpenIntoRel() to bypass "internal" invocation of heap_create_with_catalog(). Please tell me if we have different idea of code reworking to simplify deployment of the hooks. > In this particular case, I think it might be reasonable to change the > DAC behavior, so that a CREATE TABLE AS SELECT or SELECT INTO requires > insert privileges on the new table as well as permission to create it > in the first place. I don't particularly see any reason to require > different privileges for CREATE TABLE followed by INSERT .. SELECT > than what we require when the two commands are rolled into one. Prior > to 9.0, this case couldn't arise, because we didn't have default > privileges, so I'm inclined to think that the way it works now is a > historical accident rather than a deliberate design decision. > It will help this mismatch between DAC and MAC. I'll submit it as a separate patch to handle this behavior. Probably, all we need to do here is invocation of ExecCheckRTPerms(). Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Fri, Oct 21, 2011 at 12:44 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > I had checked my older implementation based on 8.4.x or 9.0.x that > includes all the features that I want to implement. > At least, it does not require so much different information from ones > needed by DAC model, although SELECT INTO was an exception. > It might be quite natural because both works similar things. OK. It seems like it might be helpful to put together a list of all of the things you want to check permissions on, maybe on a wiki page somewhere, and indicate in there which ones are done and which ones are not done, and what additional information you think is needed in each case, and flag any MAC/DAC discrepancies that you are concerned about. I think that might help us reach agreement on the best way forward. Also, then, as we get things committed, we can track progress versus that roadmap. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/10/21 Robert Haas <robertmhaas@gmail.com>: > On Fri, Oct 21, 2011 at 12:44 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> I had checked my older implementation based on 8.4.x or 9.0.x that >> includes all the features that I want to implement. >> At least, it does not require so much different information from ones >> needed by DAC model, although SELECT INTO was an exception. >> It might be quite natural because both works similar things. > > OK. It seems like it might be helpful to put together a list of all > of the things you want to check permissions on, maybe on a wiki page > somewhere, and indicate in there which ones are done and which ones > are not done, and what additional information you think is needed in > each case, and flag any MAC/DAC discrepancies that you are concerned > about. I think that might help us reach agreement on the best way > forward. Also, then, as we get things committed, we can track > progress versus that roadmap. > I tried to summarize permission checks of DAC/MAC on several object classes that are allowed to assign security label right now. http://wiki.postgresql.org/index.php?title=SEPostgreSQL/Permissions In most of checks, required contextual information by SELinux are commonly used to DAC also, as listed. On the creation of relations, SELinux requires relkind and TupleDesc to identify relation type and columns. Unlike a flag of select-into, I'm not sure whether it is a model specific and hard to manage without knowledge about sepgsql internal. In some cases, DAC uses superuser privilege as a substitute for individual permission checks, such as DefineType(). It seems to me its original implementation checked CREATE permission of the namespace and ownership of the functions that implements type input, output or others, then, we commented out these code because superuser is obviously allowed these checks. However, MAC does not have concept of superuser, so, SELinux wants to check db_procedure:{install} for each functions, thus, it requires oid of the functions to be used for the new type. Of course, we can see here is no difference between DAC and MAC, if we consider DAC implicitly allows these checks by superuser(). I think it is a good start to implement first ddl permissions on creation of the seven object classes as listed; also to proof the concept; 1) we put permission checks around existing DAC checks, 2) we deliver contextual data (basically) used in existing DAC also. I guess DROP or some of ALTER code reworking should be done prior to deploy object_access_hook around their permission checks, to minimize maintain efforts. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Tue, Nov 1, 2011 at 1:32 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > I tried to summarize permission checks of DAC/MAC on several object classes > that are allowed to assign security label right now. > http://wiki.postgresql.org/index.php?title=SEPostgreSQL/Permissions > > In most of checks, required contextual information by SELinux are commonly > used to DAC also, as listed. What's up with this: "a flag to inform whether CASCADE or RESTRICT" That doesn't seem like it should be needed. We should consider whether CREATE TABLE should be considered to consist of creating a table and then n attributes, rather than trying to shove the attribute information wholesale into the create table check. > I guess DROP or some of ALTER code reworking should be done prior to > deploy object_access_hook around their permission checks, to minimize > maintain efforts. +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2011/11/1 Robert Haas <robertmhaas@gmail.com>: > On Tue, Nov 1, 2011 at 1:32 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> I tried to summarize permission checks of DAC/MAC on several object classes >> that are allowed to assign security label right now. >> http://wiki.postgresql.org/index.php?title=SEPostgreSQL/Permissions >> >> In most of checks, required contextual information by SELinux are commonly >> used to DAC also, as listed. > > What's up with this: > > "a flag to inform whether CASCADE or RESTRICT" > > That doesn't seem like it should be needed. > Ah, it is needed to determine the scope of objects to be deleted. If a table being referenced by views, deletion of this table with cascade involves the views, even if these are owned by others. DAC does not check permissions to drop on depending objects, so, it is a headache for me. However, the worth of drop permission checks on involved objects is not perfectly clear, because the user is allowed to drop the table being reference by views at least in above example. If so, views to reference non-existent table is nonsense. I'm not 100% sure why existing DAC does not check permissions on depending objects even if these are owned by other users. I'd like to know the reason of this behavior. > We should consider whether CREATE TABLE should be considered to > consist of creating a table and then n attributes, rather than trying > to shove the attribute information wholesale into the create table > check. > I don't see tangible difference between them except for simpleness of implementation. One point we should consider is the timing to store security label of table and columns. If creation checks of table/columns are consolidated into one hook, we can check permissions to create them and write-back default security labels of them into one private datum to be delivered to post-creation hook. If creation checks of table/columns are separated into individual invocation, the later column-checks needs security label of the new table as contextual information, however, it requires us to know internals of sepgsql why it needs table's label to check permission to create a column, because both of checks shall be done DefineRelation / OpenIntoRel prior to heap_create_with_catalog that invokes post-creation hook. So, I think it works better with the approach that also delivers TupleDesc on creation of relation checks, rather than separated invocations. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
I'm under works on the prep-object-creation hook with contextual
arguments that are
commonly used to existing DAC checks.
One heahache of mine is that some object classes takes long distance to collect
all the information being necessary on creation check; such as DefineType().
In this function, it checks superuser privilege (and CREATE on the namespace
implicitly), then it makes a shell type or a base type after name
resolve of type
handlers (that also checks ownership of functions implicitly). It
means the place
where we can put a prep-creation hook is restricted to the point next to all the
oid of type-handlers are looked-up, however, it is too late to apply checks
before TypeShellMake().
I hope to make clear the matter again. The reason why we didn't want to put
permission check on OAT_POST_CREATE hook is that it takes random
flags (like "is_select_into") on its argument to avoid unnecessary permission
checks.
If sepgsql would apply permission checks db_procedure:{install} on the
OAT_POST_CREATE hook based on the funcion-oid within new entry of
system catalog, we can relocate OAT_PREP_CREATE hook more conceptually
right place, such as just after the pg_namespace_aclcheck() of DefineType().
On the other hand, we may need to answer why these information are NOT
delivered on the OAT_PREP_CREATE hook without knowledge of sepgsql
internal.
Please some ideas.
Thanks,
2011/11/1 Kohei KaiGai <kaigai@kaigai.gr.jp>:
> 2011/11/1 Robert Haas <robertmhaas@gmail.com>:
>> On Tue, Nov 1, 2011 at 1:32 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>> I tried to summarize permission checks of DAC/MAC on several object classes
>>> that are allowed to assign security label right now.
>>> http://wiki.postgresql.org/index.php?title=SEPostgreSQL/Permissions
>>>
>>> In most of checks, required contextual information by SELinux are commonly
>>> used to DAC also, as listed.
>>
>> What's up with this:
>>
>> "a flag to inform whether CASCADE or RESTRICT"
>>
>> That doesn't seem like it should be needed.
>>
> Ah, it is needed to determine the scope of objects to be deleted.
> If a table being referenced by views, deletion of this table with cascade
> involves the views, even if these are owned by others.
> DAC does not check permissions to drop on depending objects,
> so, it is a headache for me.
>
> However, the worth of drop permission checks on involved objects
> is not perfectly clear, because the user is allowed to drop the table
> being reference by views at least in above example. If so, views to
> reference non-existent table is nonsense.
> I'm not 100% sure why existing DAC does not check permissions
> on depending objects even if these are owned by other users.
> I'd like to know the reason of this behavior.
>
>> We should consider whether CREATE TABLE should be considered to
>> consist of creating a table and then n attributes, rather than trying
>> to shove the attribute information wholesale into the create table
>> check.
>>
> I don't see tangible difference between them except for simpleness of
> implementation. One point we should consider is the timing to store
> security label of table and columns.
>
> If creation checks of table/columns are consolidated into one hook,
> we can check permissions to create them and write-back default
> security labels of them into one private datum to be delivered to
> post-creation hook.
>
> If creation checks of table/columns are separated into individual
> invocation, the later column-checks needs security label of the
> new table as contextual information, however, it requires us to
> know internals of sepgsql why it needs table's label to check
> permission to create a column, because both of checks shall
> be done DefineRelation / OpenIntoRel prior to heap_create_with_catalog
> that invokes post-creation hook.
>
> So, I think it works better with the approach that also delivers
> TupleDesc on creation of relation checks, rather than separated
> invocations.
>
> Thanks,
> --
> KaiGai Kohei <kaigai@kaigai.gr.jp>
>
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
On Mon, Nov 7, 2011 at 12:20 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
> If sepgsql would apply permission checks db_procedure:{install} on the
> OAT_POST_CREATE hook based on the funcion-oid within new entry of
> system catalog, we can relocate OAT_PREP_CREATE hook more conceptually
> right place, such as just after the pg_namespace_aclcheck() of DefineType().
> On the other hand, we may need to answer why these information are NOT
> delivered on the OAT_PREP_CREATE hook without knowledge of sepgsql
> internal.
I'm having a hard time understanding this.
Can you strip this patch down so it just applies to a single object
type (tables, maybe, or functions, or whatever you like) and then
submit the corresponding sepgsql changes with it? Just as a demo
patch, so I can understand where you're trying to go with this.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
2011/11/8 Robert Haas <robertmhaas@gmail.com>:
> On Mon, Nov 7, 2011 at 12:20 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> If sepgsql would apply permission checks db_procedure:{install} on the
>> OAT_POST_CREATE hook based on the funcion-oid within new entry of
>> system catalog, we can relocate OAT_PREP_CREATE hook more conceptually
>> right place, such as just after the pg_namespace_aclcheck() of DefineType().
>> On the other hand, we may need to answer why these information are NOT
>> delivered on the OAT_PREP_CREATE hook without knowledge of sepgsql
>> internal.
>
> I'm having a hard time understanding this.
>
> Can you strip this patch down so it just applies to a single object
> type (tables, maybe, or functions, or whatever you like) and then
> submit the corresponding sepgsql changes with it? Just as a demo
> patch, so I can understand where you're trying to go with this.
>
I tried to strip the previous patch into small portion per object types.
Part-1 for database, Part-2 for schema, Part-3 for relations, and
Part-4 for functions.
The basic idea is the prep-creation hook informs the sepgsql module
properties of the new object being constructed. According to the
previous discussion, all the arguments are commonly used to existing
DAC checks also. Then, this hook allows to write back its private
opaque to be delivered to the post-creation hook; likely used to deliver
security label of the new object to be assigned.
However, I become unsure whether it is a good idea to put prep-creation
hook on all the object, because it takes many boring interface changes
to deliver private datum, and we're probably able to implement similar
stuff with post-creation hook except for a few object types.
I guess the following (A) and (B) are the only case that needs prep-
creation hooks for permission checks. Elsewhere, sepgsql will be
able to make its decision based on the entry of system catalogs
on post-creation hook.
(A) In the case when we want to apply checks based on information
that is not contained within system catalogs.
E.g, Oid of source database on CREATE DATABASE. Existing DAC
checks ownership of the database, if not template.
(B) In the case when we want to distinguish code path between user's
query and system internal stuff.
E.g, heap_create_with_catalog() is also called by make_new_heap()
as a part of ALTER TABLE, but it is quite internal stuff, so not suitable
to apply permission checks here.
It seems to me, using post-creation hooks makes the patch mode simple
to implement permission checks; except for above two object types.
So, I'd like to adopt approach to put prep-creation hooks on limited number
of object types, not symmetric with post-creation hook.
How about your opinion about this?
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>