[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(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

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(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>
>>
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>

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

On Thu, Sep 29, 2011 at 4:52 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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 new relation */
:
} 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(at)gmail(dot)com>:
> On Thu, Sep 29, 2011 at 4:52 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Thu, Oct 13, 2011 at 6:48 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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 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.

> 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(at)gmail(dot)com>:
> On Thu, Oct 13, 2011 at 6:48 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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 relations created 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(at)kaigai(dot)gr(dot)jp>

On Tue, Oct 18, 2011 at 11:25 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)gmail(dot)com>:
> On Tue, Oct 18, 2011 at 11:25 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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 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!
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Tue, Oct 18, 2011 at 1:23 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)gmail(dot)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(at)kaigai(dot)gr(dot)jp>

On Wed, Oct 19, 2011 at 6:18 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2011/10/18 Robert Haas <robertmhaas(at)gmail(dot)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(at)kaigai(dot)gr(dot)jp>

On Fri, Oct 21, 2011 at 12:44 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)gmail(dot)com>:
> On Fri, Oct 21, 2011 at 12:44 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Tue, Nov 1, 2011 at 1:32 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)gmail(dot)com>:
> On Tue, Nov 1, 2011 at 1:32 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>:
> 2011/11/1 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Tue, Nov 1, 2011 at 1:32 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>
>

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

On Mon, Nov 7, 2011 at 12:20 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)gmail(dot)com>:
> On Mon, Nov 7, 2011 at 12:20 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>