Re: security hook on table creation

This patch allows external security providers to check privileges
to create a new relation and to inform the security labels to be
assigned on the new one.

This hook is put on DefineRelation() and OpenIntoRel(), but isn't
put on Boot_CreateStmt, create_toast_table() and make_new_heap(),
although they actually create a relation, because I assume these
are the cases when we don't need individual checks and security
labels.

DefineIndex() also creates a new relation (RELKIND_INDEX), but
it seems to me the PG implementation considers indexes are a part
of properties of tables, because it always has same owner-id.
So, it shall be checked at the upcoming ALTER TABLE hook, instead.

The ESP plugins can return a list of security labels of the new
relation, columns and relation-type. If multiple ESPs are installed,
it shall append its security labels on the security labels of the
secondary plugin.
The list shall be a list of SecLabelItem as follows:
typedef struct
{
ObjectAddress object;
const char *tag;
const char *seclabel;
} SecLabelItem;
OID of them are decided in heap_create_with_catalog(), so ESP cannot
know what OID shall be supplied at the point where it makes access
control decision.
So, the core routine fix up the SecLabelItem::object->objectId later,
if InvalidOid is given. I think it is a reasonable idea rather than
putting one more hook to store security labels after the table creation.

Please also note that this patch depends on the security label support
patch that I submitted before.

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

2010/9/1 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> This patch allows external security providers to check privileges
> to create a new relation and to inform the security labels to be
> assigned on the new one.

Review:

I took a brief look at this patch tonight and I think it's on the
wrong track. There's no reason for the hook function to return the
list of security labels and then have the core code turn around and
apply them to the object. If the hook function wants to label the
object, it can just as easily call SetSecurityLabel() itself.

It seems to me that there is a general pattern to the hooks that are
needed here. For each object type for which we wish to have MAC
integration, you need the ability to get control when the object is
created and again when the object is dropped. You might want to deny
the operation, apply labels to the newly created object, do some
logging, or whatever. So it strikes me that you could have a hook
function with a signature like this:

typedef void (*object_access_hook_type)(ObjectType objtype, Oid oid,
int subid, ObjectAccessType op);

...where ObjectAccessType is an enum.

Then you could do something like this:

#define InvokeObjectAccessHook(objtype, oid, subid, op) \
if (object_access_hook != NULL) \
object_access_hook(objtype, oid, subid, op);

Then you can sprinkle calls to that macro in strategically chosen
places to trap create, drop, comment, security label, ... whatever the
object gets manipulated in a way that something like SE-Linux is apt
to care about. So ObjectAccessType can have values like OAT_CREATE,
OAT_DROP, OAT_COMMENT, OAT_SECURITY_LABEL, ...

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

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

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

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

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

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

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

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

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

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

On Wed, Sep 29, 2010 at 6:38 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> I don't think it is an option to move the hook after the pollution
> of system catalogs, although we can pull out any information about
> the new relation from syscache.

Why not?

> Sorry, it seems to me the idea simplifies the issue too much to implement
> access control features correctly.
> For example, we need to provide security modules the supplied label on
> the SECURITY LABEL hook, so it has to take one more argument at least.
> For example, we will need to provide them OID of the new schema on
> the ALTER TABLE SET SCHEMA at least too.
>  :

So what? The patch you submitted doesn't provide the OID of the new
schema when someone does ALTER TABLE SET SCHEMA, either. I proposed a
design which was much more general than what you submitted, and you're
now complaining that it's not general enough. It's unrealistic to
think you're going to solve every problem with one patch. Moreover,
it's far from obvious that you actually do need the details that
you're proposing anyway. Are you really going to write an SE-Linux
policy that allows people to change the schema of table A to schema B
but not schema C? Or that allows a hypothetical smack plugin to label
a given object with one label but not another? And if so, are those
absolutely must-have features for the first version or are those
things that would be nice to have in version 3 or 4?

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

Excerpts from KaiGai Kohei's message of mié sep 29 06:38:09 -0400 2010:

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

Why not? The relation is not yet visible to other transactions until
the creation is committed, so you can apply security labels after
populating the catalogs and there's no security leak.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

(2010/09/29 22:00), Robert Haas wrote:
> On Wed, Sep 29, 2010 at 6:38 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I don't think it is an option to move the hook after the pollution
>> of system catalogs, although we can pull out any information about
>> the new relation from syscache.
>
> Why not?
>
All the existing security checks applies before modifying system catalogs.

At least, I cannot find out any constructive reason why we try to apply
permission checks on object creation time with different manner towards
the existing privilege mechanism...

>> Sorry, it seems to me the idea simplifies the issue too much to implement
>> access control features correctly.
>> For example, we need to provide security modules the supplied label on
>> the SECURITY LABEL hook, so it has to take one more argument at least.
>> For example, we will need to provide them OID of the new schema on
>> the ALTER TABLE SET SCHEMA at least too.
>> :
>
> So what? The patch you submitted doesn't provide the OID of the new
> schema when someone does ALTER TABLE SET SCHEMA, either. I proposed a
> design which was much more general than what you submitted, and you're
> now complaining that it's not general enough. It's unrealistic to
> think you're going to solve every problem with one patch.

Sorry, I never said one patch with enough generic hook solves everything.

By contraries, I think the proposed prototype of the hook cannot inform
the plugins anything except for OID and event type, even if necessary.
Some of permission checks needs its specific prototype to inform extra
information rather than OIDs; such as new label in SECURITY LABEL hook,
new schema in upcoming ALTER TABLE SET SCHEMA, and so on...

Of course, we can implement some of permission checks with OID of the
target object and event type collectly. E,g. I cannot image any extra
information to check permission on COMMENT. I never deny it.

> Moreover,
> it's far from obvious that you actually do need the details that
> you're proposing anyway. Are you really going to write an SE-Linux
> policy that allows people to change the schema of table A to schema B
> but not schema C? Or that allows a hypothetical smack plugin to label
> a given object with one label but not another? And if so, are those
> absolutely must-have features for the first version or are those
> things that would be nice to have in version 3 or 4?
>

In your proposition, prototype of the security hook has four arguments:
ObjectType, oid, subid and ObjectAccessType, doesn't it?

When user tries to change the schema of table from A to B, we can know
the current schema of the table using syscache, but we need to inform
the plugin that B is the new schema, because we have no way to pull out
what schema was provided by the user.

As LookupCreationNamespace() checks CREATE permission on the new schema,
SELinux also want to check permission on the new schema, not only old one.
So, I concerned about the prototype does not inform about new schema that
user provided using ALTER TABLE SET SCHEMA statement.

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

KaiGai,

* KaiGai Kohei (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
> All the existing security checks applies before modifying system catalogs.
>
> At least, I cannot find out any constructive reason why we try to apply
> permission checks on object creation time with different manner towards
> the existing privilege mechanism...

The reason to do it was pretty clear- makes the code flow alot nicer and
make more sense. The existing checks aren't really doing the same thing
as this one, so I don't see that as a really good reason to contort the
code. The impression you gave is that you had a security concern
associated with this, if that's the case, please articulate what that
concern is and we can then address it. If you concern is just about
code clarity and flow, I think I'd have to vote with Robert on this one.

Thanks,

Stephen

On Wed, Sep 29, 2010 at 9:59 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> (2010/09/29 22:00), Robert Haas wrote:
>>
>> On Wed, Sep 29, 2010 at 6:38 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp>  wrote:
>>>
>>> I don't think it is an option to move the hook after the pollution
>>> of system catalogs, although we can pull out any information about
>>> the new relation from syscache.
>>
>> Why not?
>>
> All the existing security checks applies before modifying system catalogs.
>
> At least, I cannot find out any constructive reason why we try to apply
> permission checks on object creation time with different manner towards
> the existing privilege mechanism...

The reason would be so that you can apply security labels if you so
desire. If you choose to throw an error instead, transaction abort
will clean everything up. We could have two hooks - one earlier when
we're checking DAC permisisons, and a second one later to apply
labels, but I don't see that there's enough of a gain from that to be
worth the additional complexity. It's still simpler than your
proposed design, though.

>> So what?  The patch you submitted doesn't provide the OID of the new
>> schema when someone does ALTER TABLE SET SCHEMA, either.  I proposed a
>> design which was much more general than what you submitted, and you're
>> now complaining that it's not general enough.  It's unrealistic to
>> think you're going to solve every problem with one patch.
>
> Sorry, I never said one patch with enough generic hook solves everything.
>
> By contraries, I think the proposed prototype of the hook cannot inform
> the plugins anything except for OID and event type, even if necessary.

That is true. But ISTM that it will handle a remarkably large number
of cases very well. We could of course do more later, either by
adding additional hooks or by adding capabilities to this one.
However, you'd first need to make a convincing argument that those
capabilities are important.

> Some of permission checks needs its specific prototype to inform extra
> information rather than OIDs; such as new label in SECURITY LABEL hook,
> new schema in upcoming ALTER TABLE SET SCHEMA, and so on...
>
> Of course, we can implement some of permission checks with OID of the
> target object and event type collectly. E,g. I cannot image any extra
> information to check permission on COMMENT. I never deny it.

Why not? If you're going to prohibit another plugin from relabeling
an object based on the provider and label, why not allow or disallow
comments based on the content of the comment? A general problem with
your designs from the very beginning is that they involve the enhanced
security provider needing to know about absolutely everything that
goes on in core, and visca versa. That's unmaintainable and we're not
doing it.

Incidentally, wanting to know the label that some other security
provider might try to assign to an object is a crystal-clear example
of moving the goal-posts. You had a hook for that (which I removed)
in the security label patch, and it didn't provide the label anyway.
How can it be a requirement now if it wasn't two weeks ago? You need
to stay focused on coming up with simple, easy-to-understand hooks
that ideally have use case cases that are broader than security, but
at least that are broadly applicable to security rather than very
narrowly tailored to extremely specific things which you want to do.

I think that the remit of this patch should be to add hooks for CREATE
and DROP to every single object type in the system that are generic
and can be used for any purpose, whether security related or
otherwise, with room for extension to additional operations in future
patches.

>> Moreover,
>> it's far from obvious that you actually do need the details that
>> you're proposing anyway.  Are you really going to write an SE-Linux
>> policy that allows people to change the schema of table A to schema B
>> but not schema C?  Or that allows a hypothetical smack plugin to label
>> a given object with one label but not another?  And if so, are those
>> absolutely must-have features for the first version or are those
>> things that would be nice to have in version 3 or 4?
>
> In your proposition, prototype of the security hook has four arguments:
> ObjectType, oid, subid and ObjectAccessType, doesn't it?

Yes.

> When user tries to change the schema of table from A to B, we can know
> the current schema of the table using syscache, but we need to inform
> the plugin that B is the new schema, because we have no way to pull out
> what schema was provided by the user.
>
> As LookupCreationNamespace() checks CREATE permission on the new schema,
> SELinux also want to check permission on the new schema, not only old one.
> So, I concerned about the prototype does not inform about new schema that
> user provided using ALTER TABLE SET SCHEMA statement.

You're not answering my question. Are you going to write an SE-Linux
policy that allows table A to be moved to schema B but not to schema
C? And if so, is that an essential feature for the first version or
something that can be added later?

My understanding from the conversation at BWPUG is that this is not
something that Josh Brindle and David Quigley are concerned about.
Hooks on object creation are important for type transitions, so that
you can automatically assign labels rather than forcing users to apply
them by hand; the fact that we can also the entire CREATE operation to
get bounced from the same hook is a bonus. But with that exception,
they seemed to think that coarse-grained permissions would be fine for
a basic implementation: perhaps even just install something in
ProcessUtility_hook and bounce DDL across the board, so long as it's
controlled by reference to the security policy rather than by DAC. I
think we can do better than that in a pretty short period of time if
we avoid getting side-tracked, but the key is that we have to avoid
getting side-tracked.

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

(2010/09/30 0:36), Robert Haas wrote:
> On Wed, Sep 29, 2010 at 9:59 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> (2010/09/29 22:00), Robert Haas wrote:
>>>
>>> On Wed, Sep 29, 2010 at 6:38 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>>
>>>> I don't think it is an option to move the hook after the pollution
>>>> of system catalogs, although we can pull out any information about
>>>> the new relation from syscache.
>>>
>>> Why not?
>>>
>> All the existing security checks applies before modifying system catalogs.
>>
>> At least, I cannot find out any constructive reason why we try to apply
>> permission checks on object creation time with different manner towards
>> the existing privilege mechanism...
>
> The reason would be so that you can apply security labels if you so
> desire. If you choose to throw an error instead, transaction abort
> will clean everything up. We could have two hooks - one earlier when
> we're checking DAC permisisons, and a second one later to apply
> labels, but I don't see that there's enough of a gain from that to be
> worth the additional complexity. It's still simpler than your
> proposed design, though.
>
Hmm. My scheme of consideration might be affected by kernel programming
which does not have any transaction rollback, so I though it needed all
the checks before doing anything.

>>> So what? The patch you submitted doesn't provide the OID of the new
>>> schema when someone does ALTER TABLE SET SCHEMA, either. I proposed a
>>> design which was much more general than what you submitted, and you're
>>> now complaining that it's not general enough. It's unrealistic to
>>> think you're going to solve every problem with one patch.
>>
>> Sorry, I never said one patch with enough generic hook solves everything.
>>
>> By contraries, I think the proposed prototype of the hook cannot inform
>> the plugins anything except for OID and event type, even if necessary.
>
> That is true. But ISTM that it will handle a remarkably large number
> of cases very well. We could of course do more later, either by
> adding additional hooks or by adding capabilities to this one.
> However, you'd first need to make a convincing argument that those
> capabilities are important.
>
I now understand you are never suggesting a set of forever-fixed interfaces.
If we can fix up prototype of the security hooks later, I have less concern
for the approach.
It seems to me I misunderstood the intention of your proposition, sorry.

>> Some of permission checks needs its specific prototype to inform extra
>> information rather than OIDs; such as new label in SECURITY LABEL hook,
>> new schema in upcoming ALTER TABLE SET SCHEMA, and so on...
>>
>> Of course, we can implement some of permission checks with OID of the
>> target object and event type collectly. E,g. I cannot image any extra
>> information to check permission on COMMENT. I never deny it.
>
> Why not? If you're going to prohibit another plugin from relabeling
> an object based on the provider and label, why not allow or disallow
> comments based on the content of the comment? A general problem with
> your designs from the very beginning is that they involve the enhanced
> security provider needing to know about absolutely everything that
> goes on in core, and visca versa. That's unmaintainable and we're not
> doing it.
>
Indeed, we can assume such a security module which also checks content
of the comment (aside from its effectivity).

> Incidentally, wanting to know the label that some other security
> provider might try to assign to an object is a crystal-clear example
> of moving the goal-posts. You had a hook for that (which I removed)
> in the security label patch, and it didn't provide the label anyway.
> How can it be a requirement now if it wasn't two weeks ago? You need
> to stay focused on coming up with simple, easy-to-understand hooks
> that ideally have use case cases that are broader than security, but
> at least that are broadly applicable to security rather than very
> narrowly tailored to extremely specific things which you want to do.
>
The concern was also based on my misunderstanding.
I've agreed to the small-startup approach, so I believe this hook can
be eventually fixed up.

> I think that the remit of this patch should be to add hooks for CREATE
> and DROP to every single object type in the system that are generic
> and can be used for any purpose, whether security related or
> otherwise, with room for extension to additional operations in future
> patches.
>
I agree.

>>> Moreover,
>>> it's far from obvious that you actually do need the details that
>>> you're proposing anyway. Are you really going to write an SE-Linux
>>> policy that allows people to change the schema of table A to schema B
>>> but not schema C? Or that allows a hypothetical smack plugin to label
>>> a given object with one label but not another? And if so, are those
>>> absolutely must-have features for the first version or are those
>>> things that would be nice to have in version 3 or 4?
>>
>> In your proposition, prototype of the security hook has four arguments:
>> ObjectType, oid, subid and ObjectAccessType, doesn't it?
>
> Yes.
>
>> When user tries to change the schema of table from A to B, we can know
>> the current schema of the table using syscache, but we need to inform
>> the plugin that B is the new schema, because we have no way to pull out
>> what schema was provided by the user.
>>
>> As LookupCreationNamespace() checks CREATE permission on the new schema,
>> SELinux also want to check permission on the new schema, not only old one.
>> So, I concerned about the prototype does not inform about new schema that
>> user provided using ALTER TABLE SET SCHEMA statement.
>
> You're not answering my question. Are you going to write an SE-Linux
> policy that allows table A to be moved to schema B but not to schema
> C? And if so, is that an essential feature for the first version or
> something that can be added later?
>
Ah, Sorry.
Yes, I (eventually) want to provide this kind of the policy, but I think
its priority is not first, indeed.

> My understanding from the conversation at BWPUG is that this is not
> something that Josh Brindle and David Quigley are concerned about.
> Hooks on object creation are important for type transitions, so that
> you can automatically assign labels rather than forcing users to apply
> them by hand; the fact that we can also the entire CREATE operation to
> get bounced from the same hook is a bonus.
>
Yes, the hooks on object creation time are important, rather than others.

> But with that exception,
> they seemed to think that coarse-grained permissions would be fine for
> a basic implementation: perhaps even just install something in
> ProcessUtility_hook and bounce DDL across the board, so long as it's
> controlled by reference to the security policy rather than by DAC. I
> think we can do better than that in a pretty short period of time if
> we avoid getting side-tracked, but the key is that we have to avoid
> getting side-tracked.
>
In this approach, we eventually need to deploy the hooks on object creation
as we are currently working on. So, I don't think using ProcessUtility_hook
for coarse-grained permissions is a right direction.

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

2010/9/29 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> But with that exception,
>> they seemed to think that coarse-grained permissions would be fine for
>> a basic implementation: perhaps even just install something in
>> ProcessUtility_hook and bounce DDL across the board, so long as it's
>> controlled by reference to the security policy rather than by DAC.  I
>> think we can do better than that in a pretty short period of time if
>> we avoid getting side-tracked, but the key is that we have to avoid
>> getting side-tracked.
>>
> In this approach, we eventually need to deploy the hooks on object creation
> as we are currently working on. So, I don't think using ProcessUtility_hook
> for coarse-grained permissions is a right direction.

Well, it may be the easiest way to do certain things. For example, if
you wanted to control access to a command such as LOAD (which
presumably you do since otherwise a loadable module could trivially
subvert the security policy), it's unclear to me that there's any need
for a new hook; ProcessUtility_hook might very well be the best way to
tackle that. We need to consider the best way to handle each case.
In some cases, all of the necessary information may not be available
when ProcessUtility_hook is called, but where it is, we shouldn't
reinvent the wheel.

With respect to this patch, I think we are on the same page now, with
possibly some disagreement about how far it makes sense to go with
this that needn't concern us for the present. I'm going to mark this
patch Returned with Feedback, because we need to move on to other
patches that are closer to being committable.

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

(2010/09/30 10:28), Robert Haas wrote:
> 2010/9/29 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> But with that exception,
>>> they seemed to think that coarse-grained permissions would be fine for
>>> a basic implementation: perhaps even just install something in
>>> ProcessUtility_hook and bounce DDL across the board, so long as it's
>>> controlled by reference to the security policy rather than by DAC. I
>>> think we can do better than that in a pretty short period of time if
>>> we avoid getting side-tracked, but the key is that we have to avoid
>>> getting side-tracked.
>>>
>> In this approach, we eventually need to deploy the hooks on object creation
>> as we are currently working on. So, I don't think using ProcessUtility_hook
>> for coarse-grained permissions is a right direction.
>
> Well, it may be the easiest way to do certain things. For example, if
> you wanted to control access to a command such as LOAD (which
> presumably you do since otherwise a loadable module could trivially
> subvert the security policy), it's unclear to me that there's any need
> for a new hook; ProcessUtility_hook might very well be the best way to
> tackle that. We need to consider the best way to handle each case.
> In some cases, all of the necessary information may not be available
> when ProcessUtility_hook is called, but where it is, we shouldn't
> reinvent the wheel.
>
In the ideal world, I want to put a new hook to control LOAD command,
because the given library name is not expanded at ProcessUtility_hook
time (and expand_dynamic_library_name() is a static function), so
we cannot know enough information to apply fine-grained control;
such as per-libraries-control.

So, right-now, all we can do in coarse-grained permissions are to
prohibit LOAD command always when SE-PgSQL is installed.
(For more detail, it is not perfect because we can overwrite the
'local_shared_libraries' setting using connection string.)

However, we understood we need to prioritize our upcoming works,
and I think the security hooks on table creation has the highest
priority than others.

> With respect to this patch, I think we are on the same page now, with
> possibly some disagreement about how far it makes sense to go with
> this that needn't concern us for the present. I'm going to mark this
> patch Returned with Feedback, because we need to move on to other
> patches that are closer to being committable.
>
OK. I'll refactor my patch set.

| #define InvokeObjectAccessHook(objtype, oid, subid, op) \
| if (object_access_hook != NULL) \
| object_access_hook(objtype, oid, subid, op);

One my preference is functions, rather than macros, because we
need a *.c file somewhere to put pointer variable of the hook
and it will become a good place to describe source code comments
of the hook.

In addition, I want to give these entrypoints its name which
represents an appropriate purpose of the hook, rather than
a uniformed one.

Example:

/*
* This hook is ...
*/
object_access_hook_type object_access_hook = NULL;

/*
* check_relation_create
*
* This hook is invoked when ...
:
:
* If violated, guest of the hook can raise an error.
*/
void
check_relation_create(Oid oid)
{
if (object_access_hook != NULL)
object_access_hook(OBJECT_TABLE, oid, OAT_CREATE);
}

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

2010/9/29 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> In addition, I want to give these entrypoints its name which
> represents an appropriate purpose of the hook, rather than
> a uniformed one.

It sounds like you're proposing to create a vast number of hooks
rather than just one. If we have ~20 object types in the system,
that's 40 hooks just for create and drop, and then many more to handle
comment, alter (perhaps in various flavors), etc. I'm pretty
unexcited about that. The main hook function can always dispatch
internally if it so desires, but I don't see any benefit to forcing
people to write the code that way.

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

(2010/10/01 3:09), Robert Haas wrote:
> 2010/9/29 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> In addition, I want to give these entrypoints its name which
>> represents an appropriate purpose of the hook, rather than
>> a uniformed one.
>
> It sounds like you're proposing to create a vast number of hooks
> rather than just one. If we have ~20 object types in the system,
> that's 40 hooks just for create and drop, and then many more to handle
> comment, alter (perhaps in various flavors), etc. I'm pretty
> unexcited about that. The main hook function can always dispatch
> internally if it so desires, but I don't see any benefit to forcing
> people to write the code that way.
>
What I proposed is to create just one hook and wrapper functions
with appropriate name; that calls the hook with appropriate parameters,
such as SearchSysCache1, 2, 3 and 4.

However, the reason why I proposed the wrapper functions is mainly from
a sense of beauty at the code. So, I choose the term of 'my preference'.
Well, at first, I'll try to work on as you suggested.

---
BTW, as an aside, the SearchSysCacheX() interface also inspired me.
If the hook function can deliver a few Datum values depending on object
types and event types, it may allows the main hook to handle most of
security checks, even if we need to add various flavors.

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

On Sep 30, 2010, at 9:01 PM, KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> (2010/10/01 3:09), Robert Haas wrote:
>> 2010/9/29 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> In addition, I want to give these entrypoints its name which
>>> represents an appropriate purpose of the hook, rather than
>>> a uniformed one.
>>
>> It sounds like you're proposing to create a vast number of hooks
>> rather than just one. If we have ~20 object types in the system,
>> that's 40 hooks just for create and drop, and then many more to handle
>> comment, alter (perhaps in various flavors), etc. I'm pretty
>> unexcited about that. The main hook function can always dispatch
>> internally if it so desires, but I don't see any benefit to forcing
>> people to write the code that way.
>>
> What I proposed is to create just one hook and wrapper functions
> with appropriate name; that calls the hook with appropriate parameters,
> such as SearchSysCache1, 2, 3 and 4.

Seems like you'd end up creating a lot of macros that were only used once.

> BTW, as an aside, the SearchSysCacheX() interface also inspired me.
> If the hook function can deliver a few Datum values depending on object
> types and event types, it may allows the main hook to handle most of
> security checks, even if we need to add various flavors.

Good idea. Let's leave that out for the first version of this, though.

...Robert

(2010/10/01 11:23), Robert Haas wrote:
> On Sep 30, 2010, at 9:01 PM, KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>> (2010/10/01 3:09), Robert Haas wrote:
>>> 2010/9/29 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>>> In addition, I want to give these entrypoints its name which
>>>> represents an appropriate purpose of the hook, rather than
>>>> a uniformed one.
>>>
>>> It sounds like you're proposing to create a vast number of hooks
>>> rather than just one. If we have ~20 object types in the system,
>>> that's 40 hooks just for create and drop, and then many more to handle
>>> comment, alter (perhaps in various flavors), etc. I'm pretty
>>> unexcited about that. The main hook function can always dispatch
>>> internally if it so desires, but I don't see any benefit to forcing
>>> people to write the code that way.
>>>
>> What I proposed is to create just one hook and wrapper functions
>> with appropriate name; that calls the hook with appropriate parameters,
>> such as SearchSysCache1, 2, 3 and 4.
>
> Seems like you'd end up creating a lot of macros that were only used once.
>

I began to revise the security hooks, but I could find a few cases that does
not work with the approach of post-creation security hooks.
If we try to fix up the core PG routine to become suitable for the post-creation
security hooks, it shall need to put more CommandCounterIncrement() on various
places, so it made me doubtful whether this approach gives really minimum impact
to the core PG routine, or not.

See the following analysis:

Now we support to assign security label on the seven object classes enumerated
at ExecSecLabelStmt().

Some of object classes have CommandCounterIncrement() just after the routine
to create a new object. For example, DefineRelation() calls it just after the
heap_create_with_catalog(), so the new relation entry is visible for plugin
modules using SearchSysCache(), as long as we put the post-creation security
hook aftere the CommandCounterIncrement().

However, we also have a few headache cases.
DefineType() creates a new type object and its array type, but it does not
call CommandCounterIncrement() by the end of this function, so the new type
entries are not visible from the plugin modules, even if we put a security
hook at tail of the DefineType().
DefineFunction() also has same matter. It create a new procedure object,
but it also does not call CommandCounterIncrement() by the end of this
function, except for the case when ProcedureCreate() invokes language
validator function.

During the discussion, we talked about which is more preferable design
(A) having both of prep/post creation hooks, or (B) having only post
creation hook. My original proposition is similar to the idea (A), but
we could not find out a good reason to justify (A) rather than simple
(B) a week ago.
However, again, it seems to me the idea (A) becomes better, because it
enables to avoid random injection of CommandCounterIncrement() in the
future.

So, I'd like to reconsider the idea with two hooks on creation time.

The issue of prep creation hook was that it needs per object class
definitions because we cannot pull-out information from SysCache
before the new database object being visible.
But it does not mean we eventually need (# of object classes) x (# of
access types) hooks.

E.g, it may be possible to design creation of relation as follows:

DefineRelation(...)
{
/* DAC permission checks here */
:
/* MAC permission checks; it returns its private data */
opaque = check_relation_create(...<relation parameters>...);
:
/* insertion into pg_class catalog */
relationId = heap_create_with_catalog(...);
:
/* assign security labels on the new object */
InvokeObjectAccessHook(OBJECT_TABLE, OAT_POST_CREATE,
relationId, 0, opaque);
}

In this design, we can share the post-creation hook with any other object
classes, so all we need to provide for each object classes are just prep
creation hooks. It does not seem to me explosion of security hooks.

Could you give me your opinion to handle these problematic code paths?

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

2010/10/5 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> I began to revise the security hooks, but I could find a few cases that does
> not work with the approach of post-creation security hooks.
> If we try to fix up the core PG routine to become suitable for the post-creation
> security hooks, it shall need to put more CommandCounterIncrement() on various
> places, so it made me doubtful whether this approach gives really minimum impact
> to the core PG routine, or not.

We definitely don't want to add CCIs without a good reason.

> Some of object classes have CommandCounterIncrement() just after the routine
> to create a new object. For example, DefineRelation() calls it just after the
> heap_create_with_catalog(), so the new relation entry is visible for plugin
> modules using SearchSysCache(), as long as we put the post-creation security
> hook aftere the CommandCounterIncrement().
>
> However, we also have a few headache cases.
> DefineType() creates a new type object and its array type, but it does not
> call CommandCounterIncrement() by the end of this function, so the new type
> entries are not visible from the plugin modules, even if we put a security
> hook at tail of the DefineType().
> DefineFunction() also has same matter. It create a new procedure object,
> but it also does not call CommandCounterIncrement() by the end of this
> function, except for the case when ProcedureCreate() invokes language
> validator function.

So I guess the first question here is why it's important to be able to
see the new entry. I am thinking that you want it so that, for
example, you can fetch the namespace OID to perform an SE-Linux type
transition. Is that right?

Maybe we need a variant of InvokeObjectAccessHook that does a CCI only
if a hook is present. I can't see that we're going to want to pay for
that unconditionally, but it shouldn't surprise anyone that loading an
enhanced security provider comes with some overhead.

> E.g, it may be possible to design creation of relation as follows:
>
> DefineRelation(...)
> {
>    /* DAC permission checks here */
>        :
>    /* MAC permission checks; it returns its private data */
>    opaque = check_relation_create(...<relation parameters>...);
>        :
>    /* insertion into pg_class catalog */
>    relationId = heap_create_with_catalog(...);
>        :
>    /* assign security labels on the new object */
>    InvokeObjectAccessHook(OBJECT_TABLE, OAT_POST_CREATE,
>                           relationId, 0, opaque);
> }

I'd like to try to avoid that, if we can. That's going to make this
code far more complex to understand and maintain.

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

Excerpts from Robert Haas's message of mié oct 06 17:02:22 -0400 2010:
> 2010/10/5 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:

> > However, we also have a few headache cases.
> > DefineType() creates a new type object and its array type, but it does not
> > call CommandCounterIncrement() by the end of this function, so the new type
> > entries are not visible from the plugin modules, even if we put a security
> > hook at tail of the DefineType().
> > DefineFunction() also has same matter. It create a new procedure object,
> > but it also does not call CommandCounterIncrement() by the end of this
> > function, except for the case when ProcedureCreate() invokes language
> > validator function.
>
> So I guess the first question here is why it's important to be able to
> see the new entry. I am thinking that you want it so that, for
> example, you can fetch the namespace OID to perform an SE-Linux type
> transition. Is that right?

I'm not sure that there's any point trying to optimize these to the
point of avoiding CommandCounterIncrement. Surely DefineType et al are
not performance-sensitive operations.

> Maybe we need a variant of InvokeObjectAccessHook that does a CCI only
> if a hook is present.

The problem I see with this idea is that it becomes a lot harder to
track down whether it ocurred or not for any given operation.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

(2010/10/07 6:02), Robert Haas wrote:
> 2010/10/5 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> I began to revise the security hooks, but I could find a few cases that does
>> not work with the approach of post-creation security hooks.
>> If we try to fix up the core PG routine to become suitable for the post-creation
>> security hooks, it shall need to put more CommandCounterIncrement() on various
>> places, so it made me doubtful whether this approach gives really minimum impact
>> to the core PG routine, or not.
>
> We definitely don't want to add CCIs without a good reason.
>
>> Some of object classes have CommandCounterIncrement() just after the routine
>> to create a new object. For example, DefineRelation() calls it just after the
>> heap_create_with_catalog(), so the new relation entry is visible for plugin
>> modules using SearchSysCache(), as long as we put the post-creation security
>> hook aftere the CommandCounterIncrement().
>>
>> However, we also have a few headache cases.
>> DefineType() creates a new type object and its array type, but it does not
>> call CommandCounterIncrement() by the end of this function, so the new type
>> entries are not visible from the plugin modules, even if we put a security
>> hook at tail of the DefineType().
>> DefineFunction() also has same matter. It create a new procedure object,
>> but it also does not call CommandCounterIncrement() by the end of this
>> function, except for the case when ProcedureCreate() invokes language
>> validator function.
>
> So I guess the first question here is why it's important to be able to
> see the new entry. I am thinking that you want it so that, for
> example, you can fetch the namespace OID to perform an SE-Linux type
> transition. Is that right?
>
We assumed that namespace OID can be fetched from the entry of pg_class,
so we thought the common InvokeObjectAccessHook() dose not need to take
many arguments, because we can pull out corresponding properties of new
object (such as namespace OID) from SysCache using OID of new object.

So, InvokeObjectAccessHook() must deliver OID of the namespace, rather
than OID of the new object, if it is not visible.

> Maybe we need a variant of InvokeObjectAccessHook that does a CCI only
> if a hook is present. I can't see that we're going to want to pay for
> that unconditionally, but it shouldn't surprise anyone that loading an
> enhanced security provider comes with some overhead.
>
The reason why we tried to move the object creation hooks into post
object creation was to reduce number of security hooks and burden of
code maintenance.

However, it seems to me paying attention for object visibility gives
us more burden rather than we have multiple creation hooks.

>> E.g, it may be possible to design creation of relation as follows:
>>
>> DefineRelation(...)
>> {
>> /* DAC permission checks here */
>> :
>> /* MAC permission checks; it returns its private data */
>> opaque = check_relation_create(...<relation parameters>...);
>> :
>> /* insertion into pg_class catalog */
>> relationId = heap_create_with_catalog(...);
>> :
>> /* assign security labels on the new object */
>> InvokeObjectAccessHook(OBJECT_TABLE, OAT_POST_CREATE,
>> relationId, 0, opaque);
>> }
>
> I'd like to try to avoid that, if we can. That's going to make this
> code far more complex to understand and maintain.
>
Against our feeling, I consider the idea of two hooks help us to
understand and maintain security hooks in the future.
Because the place where we should put the prep-creation hook is
just after the default privilege checks, it is quite obvious.

If we would have only post-creation hook, is it obvious where we
should put the security hook on function creation, for example?

In the case when we have two hooks, obviously, the prep-creation
hook will be after the DAC checks, and the post-creation hook will
be after the insert/update of system catalogs.
It seems to me quite easy to understand and maintain.

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

(2010/10/07 6:21), Alvaro Herrera wrote:
> Excerpts from Robert Haas's message of mié oct 06 17:02:22 -0400 2010:
>> 2010/10/5 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>
>>> However, we also have a few headache cases.
>>> DefineType() creates a new type object and its array type, but it does not
>>> call CommandCounterIncrement() by the end of this function, so the new type
>>> entries are not visible from the plugin modules, even if we put a security
>>> hook at tail of the DefineType().
>>> DefineFunction() also has same matter. It create a new procedure object,
>>> but it also does not call CommandCounterIncrement() by the end of this
>>> function, except for the case when ProcedureCreate() invokes language
>>> validator function.
>>
>> So I guess the first question here is why it's important to be able to
>> see the new entry. I am thinking that you want it so that, for
>> example, you can fetch the namespace OID to perform an SE-Linux type
>> transition. Is that right?
>
> I'm not sure that there's any point trying to optimize these to the
> point of avoiding CommandCounterIncrement. Surely DefineType et al are
> not performance-sensitive operations.
>
Performance optimization is not the point here.

If we need to call CommandCounterIncrement() before invocation of security
hooks, we also need to analyze its side-effect and to confirm it is harmless.
Even if it is harmless, I think it gives us more burden of code maintenance
than the idea of two hooks on creation.

>> Maybe we need a variant of InvokeObjectAccessHook that does a CCI only
>> if a hook is present.
>
> The problem I see with this idea is that it becomes a lot harder to
> track down whether it ocurred or not for any given operation.
>
Yes, it is a burden of code maintenance.

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

On Wed, Oct 6, 2010 at 5:21 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Robert Haas's message of mié oct 06 17:02:22 -0400 2010:
>> 2010/10/5 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>
>> > However, we also have a few headache cases.
>> > DefineType() creates a new type object and its array type, but it does not
>> > call CommandCounterIncrement() by the end of this function, so the new type
>> > entries are not visible from the plugin modules, even if we put a security
>> > hook at tail of the DefineType().
>> > DefineFunction() also has same matter. It create a new procedure object,
>> > but it also does not call CommandCounterIncrement() by the end of this
>> > function, except for the case when ProcedureCreate() invokes language
>> > validator function.
>>
>> So I guess the first question here is why it's important to be able to
>> see the new entry.  I am thinking that you want it so that, for
>> example, you can fetch the namespace OID to perform an SE-Linux type
>> transition.  Is that right?
>
> I'm not sure that there's any point trying to optimize these to the
> point of avoiding CommandCounterIncrement.  Surely DefineType et al are
> not performance-sensitive operations.

OK, fair enough. Let's just do it unconditionally then.

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

(2010/10/08 0:21), Robert Haas wrote:
> On Wed, Oct 6, 2010 at 5:21 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>> Excerpts from Robert Haas's message of mié oct 06 17:02:22 -0400 2010:
>>> 2010/10/5 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>
>>>> However, we also have a few headache cases.
>>>> DefineType() creates a new type object and its array type, but it does not
>>>> call CommandCounterIncrement() by the end of this function, so the new type
>>>> entries are not visible from the plugin modules, even if we put a security
>>>> hook at tail of the DefineType().
>>>> DefineFunction() also has same matter. It create a new procedure object,
>>>> but it also does not call CommandCounterIncrement() by the end of this
>>>> function, except for the case when ProcedureCreate() invokes language
>>>> validator function.
>>>
>>> So I guess the first question here is why it's important to be able to
>>> see the new entry. I am thinking that you want it so that, for
>>> example, you can fetch the namespace OID to perform an SE-Linux type
>>> transition. Is that right?
>>
>> I'm not sure that there's any point trying to optimize these to the
>> point of avoiding CommandCounterIncrement. Surely DefineType et al are
>> not performance-sensitive operations.
>
> OK, fair enough. Let's just do it unconditionally then.
>
I guess we will need to have such kind of discussion whenever we touch
the code around security hooks in the future, if hooks would not be put
next to the existing DAC checks.
Although we need to define several hooks on object creation in addition
to the main hook, separating it into two parts helps us to understand
and maintenance.

| In the case when we have two hooks, obviously, the prep-creation
| hook will be after the DAC checks, and the post-creation hook will
| be after the insert/update of system catalogs.

I still don't think such a simple principle overs our capability, and
also don't think it is more complex than matters around the idea of
only post-creation hooks, such as CommandCounterIncrement().

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

It seems to me the conclusion of this discussion is unclear.

We commonly try to find out an approach that minimize code complexity
to understand and maintain, so the point of issue is clear, but we
still don't reach same conclusion, because both of two ideas have merits
and demerits each other.

* Prep/Post-creation hook
merits: simple principle to deploy security hooks. The prep-creation
hook shall be after existing DAC checks, and the post-creation hook
shall be after modification of system catalogs.
demerits: several specific security hooks are necessary, in addition
to the main security hook.

* Only post-creation hook
merits: small number of security hook definitions.
demerits: at least, new entries of system catalog must be visible
when we invoke the security hooks, so some cases require us to
add new CCIs on invocations, but it requires us to verify it is
harmless (whenever we will touch the code around security hooks
in the future).

In my viewpoint, MVCC is one of the most complex things in PG.
So, in fact, I also missed the possibility that the gust of security
hook cannot reference the entry of new database object, when the idea
of post-creation hook was suggested.
If we have a strong (and implicit) restriction about places where
we should deploy the security hooks on, I don't think it enables to
minimize our task to understand and maintain (in the future), although
line of change set is a bit smaller now.

So, I think the idea of two hooks on creation is better.
It tries to put prep-creation hook according to the manner of existing
DAC checks, and post-creation hook is next to modification of system
catalogs with same visibility of the main entries.
It seems to me this simple principle enables to minimize our task to
understand and maintain.

Thanks,

(2010/10/08 9:39), KaiGai Kohei wrote:
> (2010/10/08 0:21), Robert Haas wrote:
>> On Wed, Oct 6, 2010 at 5:21 PM, Alvaro Herrera
>> <alvherre(at)commandprompt(dot)com> wrote:
>>> Excerpts from Robert Haas's message of mié oct 06 17:02:22 -0400 2010:
>>>> 2010/10/5 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>>
>>>>> However, we also have a few headache cases.
>>>>> DefineType() creates a new type object and its array type, but it does not
>>>>> call CommandCounterIncrement() by the end of this function, so the new type
>>>>> entries are not visible from the plugin modules, even if we put a security
>>>>> hook at tail of the DefineType().
>>>>> DefineFunction() also has same matter. It create a new procedure object,
>>>>> but it also does not call CommandCounterIncrement() by the end of this
>>>>> function, except for the case when ProcedureCreate() invokes language
>>>>> validator function.
>>>>
>>>> So I guess the first question here is why it's important to be able to
>>>> see the new entry. I am thinking that you want it so that, for
>>>> example, you can fetch the namespace OID to perform an SE-Linux type
>>>> transition. Is that right?
>>>
>>> I'm not sure that there's any point trying to optimize these to the
>>> point of avoiding CommandCounterIncrement. Surely DefineType et al are
>>> not performance-sensitive operations.
>>
>> OK, fair enough. Let's just do it unconditionally then.
>>
> I guess we will need to have such kind of discussion whenever we touch
> the code around security hooks in the future, if hooks would not be put
> next to the existing DAC checks.
> Although we need to define several hooks on object creation in addition
> to the main hook, separating it into two parts helps us to understand
> and maintenance.
>
> | In the case when we have two hooks, obviously, the prep-creation
> | hook will be after the DAC checks, and the post-creation hook will
> | be after the insert/update of system catalogs.
>
> I still don't think such a simple principle overs our capability, and
> also don't think it is more complex than matters around the idea of
> only post-creation hooks, such as CommandCounterIncrement().
>
> Thanks,

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

On Mon, Oct 11, 2010 at 9:58 PM, KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> It seems to me the conclusion of this discussion is unclear.
>
> We commonly try to find out an approach that minimize code complexity
> to understand and maintain, so the point of issue is clear, but we
> still don't reach same conclusion, because both of two ideas have merits
> and demerits each other.
>
> * Prep/Post-creation hook
>  merits: simple principle to deploy security hooks. The prep-creation
>    hook shall be after existing DAC checks, and the post-creation hook
>    shall be after modification of system catalogs.
>  demerits: several specific security hooks are necessary, in addition
>    to the main security hook.
>
> * Only post-creation hook
>  merits: small number of security hook definitions.
>  demerits: at least, new entries of system catalog must be visible
>    when we invoke the security hooks, so some cases require us to
>    add new CCIs on invocations, but it requires us to verify it is
>    harmless (whenever we will touch the code around security hooks
>    in the future).
>
> In my viewpoint, MVCC is one of the most complex things in PG.
> So, in fact, I also missed the possibility that the gust of security
> hook cannot reference the entry of new database object, when the idea
> of post-creation hook was suggested.
> If we have a strong (and implicit) restriction about places where
> we should deploy the security hooks on, I don't think it enables to
> minimize our task to understand and maintain (in the future), although
> line of change set is a bit smaller now.
>
> So, I think the idea of two hooks on creation is better.
> It tries to put prep-creation hook according to the manner of existing
> DAC checks, and post-creation hook is next to modification of system
> catalogs with same visibility of the main entries.
> It seems to me this simple principle enables to minimize our task to
> understand and maintain.

I don't understand what problem you think having two hooks will solve.

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

(2010/10/12 11:35), Robert Haas wrote:
> On Mon, Oct 11, 2010 at 9:58 PM, KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>> It seems to me the conclusion of this discussion is unclear.
>>
>> We commonly try to find out an approach that minimize code complexity
>> to understand and maintain, so the point of issue is clear, but we
>> still don't reach same conclusion, because both of two ideas have merits
>> and demerits each other.
>>
>> * Prep/Post-creation hook
>> merits: simple principle to deploy security hooks. The prep-creation
>> hook shall be after existing DAC checks, and the post-creation hook
>> shall be after modification of system catalogs.
>> demerits: several specific security hooks are necessary, in addition
>> to the main security hook.
>>
>> * Only post-creation hook
>> merits: small number of security hook definitions.
>> demerits: at least, new entries of system catalog must be visible
>> when we invoke the security hooks, so some cases require us to
>> add new CCIs on invocations, but it requires us to verify it is
>> harmless (whenever we will touch the code around security hooks
>> in the future).
>>
>> In my viewpoint, MVCC is one of the most complex things in PG.
>> So, in fact, I also missed the possibility that the gust of security
>> hook cannot reference the entry of new database object, when the idea
>> of post-creation hook was suggested.
>> If we have a strong (and implicit) restriction about places where
>> we should deploy the security hooks on, I don't think it enables to
>> minimize our task to understand and maintain (in the future), although
>> line of change set is a bit smaller now.
>>
>> So, I think the idea of two hooks on creation is better.
>> It tries to put prep-creation hook according to the manner of existing
>> DAC checks, and post-creation hook is next to modification of system
>> catalogs with same visibility of the main entries.
>> It seems to me this simple principle enables to minimize our task to
>> understand and maintain.
>
> I don't understand what problem you think having two hooks will solve.
>
It enables us to put security hooks independent from MVCC visibility of
the new database object. If we pay attention for visibility of new database
object, it seems to me amount of things to understand and maintain will be
increased, although MVCC visibility is fundamentally unrelated stuff from
viewpoint of the access control.

In the idea of two hooks, the prep-creation hook shall be invoked in same
visibility of existing permission checks, and the post-creation hook shall
be invoked in same visibility of simple_heap_* operations.
I think it enables to reduce amount of things to understand and maintain,
because the scope we should pay attention become small, if we can put
security hooks independent from MVCC visibility.

Perhaps, the problem may be intangible, but I don't think it is fair
enough if we have to pay attention about MVCC visibility of plugin
modules whenever we try to apply a patch around creation hooks.

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

2010/10/11 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> It enables us to put security hooks independent from MVCC visibility of
> the new database object. If we pay attention for visibility of new database
> object, it seems to me amount of things to understand and maintain will be
> increased, although MVCC visibility is fundamentally unrelated stuff from
> viewpoint of the access control.
>
> In the idea of two hooks, the prep-creation hook shall be invoked in same
> visibility of existing permission checks, and the post-creation hook shall
> be invoked in same visibility of simple_heap_* operations.
> I think it enables to reduce amount of things to understand and maintain,
> because the scope we should pay attention become small, if we can put
> security hooks independent from MVCC visibility.
>
> Perhaps, the problem may be intangible, but I don't think it is fair
> enough if we have to pay attention about MVCC visibility of plugin
> modules whenever we try to apply a patch around creation hooks.

This may be nothing more than a matter of opinion, but it seems to me
that what you're proposing makes this vastly more complicated for no
particular benefit. Instead of having one hook that can cover a wide
variety of use cases, you're going to need individual hooks for each
object type plus the ability to pass data between them. And the point
of this, apparently, is so that you can avoid using the standard
syscache functions that the entire backend uses for retrieving
information about objects and instead extract it in some other way;
and/or avoid having to deal with the MVCC properties of which the rest
of the backend must be aware. Maybe somebody else has a different
opinion, but I just can't get even a little excited about that.

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

(2010/10/12 20:59), Robert Haas wrote:
> 2010/10/11 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> It enables us to put security hooks independent from MVCC visibility of
>> the new database object. If we pay attention for visibility of new database
>> object, it seems to me amount of things to understand and maintain will be
>> increased, although MVCC visibility is fundamentally unrelated stuff from
>> viewpoint of the access control.
>>
>> In the idea of two hooks, the prep-creation hook shall be invoked in same
>> visibility of existing permission checks, and the post-creation hook shall
>> be invoked in same visibility of simple_heap_* operations.
>> I think it enables to reduce amount of things to understand and maintain,
>> because the scope we should pay attention become small, if we can put
>> security hooks independent from MVCC visibility.
>>
>> Perhaps, the problem may be intangible, but I don't think it is fair
>> enough if we have to pay attention about MVCC visibility of plugin
>> modules whenever we try to apply a patch around creation hooks.
>
> This may be nothing more than a matter of opinion, but it seems to me
> that what you're proposing makes this vastly more complicated for no
> particular benefit. Instead of having one hook that can cover a wide
> variety of use cases, you're going to need individual hooks for each
> object type plus the ability to pass data between them.

In the broad outline, I also agree with one main security which can
cover most of use cases. However, the only difference is that I'm
saying we should handle prep-creation case as exception of the main
hook.
As I introduced before, the idea of two hooks makes obvious where
we should put the security hooks; it is next to the existing DAC
checking. It is the best guideline, even if we will touch the code
around object creation in the future version.

If the creation-hook would be put on the place far from existing
DAC checks, what provides us a guideline to deploy security hooks?
It seems to me the idea of only post-creation hook try to lose
this kind of benefit instead of half dozen of exceptions.

I think MVCC visibility is just an actualization of the matters.
The point is that we can be released from the task to consider
where is the right place for security hooks, as long as it will
be placed next to the existing DAC checks.
It seems to me its simplicity of design is unignorable benefit.

> And the point
> of this, apparently, is so that you can avoid using the standard
> syscache functions that the entire backend uses for retrieving
> information about objects and instead extract it in some other way;
> and/or avoid having to deal with the MVCC properties of which the rest
> of the backend must be aware.

It just means it is not impossible...
However, it requires the plugin modules need to know everything;
such as what is visible/invisible. It seems to me too closely-
coupled interface.

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

On Tue, Oct 12, 2010 at 9:20 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> As I introduced before, the idea of two hooks makes obvious where
> we should put the security hooks; it is next to the existing DAC
> checking. It is the best guideline, even if we will touch the code
> around object creation in the future version.
>
> If the creation-hook would be put on the place far from existing
> DAC checks, what provides us a guideline to deploy security hooks?
> It seems to me the idea of only post-creation hook try to lose
> this kind of benefit instead of half dozen of exceptions.
>
> I think MVCC visibility is just an actualization of the matters.
> The point is that we can be released from the task to consider
> where is the right place for security hooks, as long as it will
> be placed next to the existing DAC checks.
> It seems to me its simplicity of design is unignorable benefit.

In either design, you have to decide where to put the post-creation
hook. In your design, you ALSO need to decide where to put the
pre-creation hook. Deciding where to put the pre-creation hook may be
simple, but it is not as simple as not having it at all.

A possibly legitimate reason to have a pre-creation hook is to prevent
users from consuming more excessive system resources by repeatedly
performing operations that SE-Linux will end up denying, but only
after they're basically complete.

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

(2010/10/12 23:09), Robert Haas wrote:
> On Tue, Oct 12, 2010 at 9:20 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> As I introduced before, the idea of two hooks makes obvious where
>> we should put the security hooks; it is next to the existing DAC
>> checking. It is the best guideline, even if we will touch the code
>> around object creation in the future version.
>>
>> If the creation-hook would be put on the place far from existing
>> DAC checks, what provides us a guideline to deploy security hooks?
>> It seems to me the idea of only post-creation hook try to lose
>> this kind of benefit instead of half dozen of exceptions.
>>
>> I think MVCC visibility is just an actualization of the matters.
>> The point is that we can be released from the task to consider
>> where is the right place for security hooks, as long as it will
>> be placed next to the existing DAC checks.
>> It seems to me its simplicity of design is unignorable benefit.
>
> In either design, you have to decide where to put the post-creation
> hook. In your design, you ALSO need to decide where to put the
> pre-creation hook. Deciding where to put the pre-creation hook may be
> simple, but it is not as simple as not having it at all.
>
If the post-creation hook have two tasks (access control and fix-up
security labels) concurrently, things we need to consider and assess
is not equal to the case when we only fix-up security labels on the
post-creation hooks.
MVCC visibility is a typical example. Elsewhere, we need to check up
various things (such as completeness of the hook coverage, side-effects
of CommandCounterIncrement(), ...) without reliable guidelines.

I'm saying we can go through an easy way, as long as we design these
hooks according to a simple principle based on the existing features.

* pre-creation hooks (for access control) shall be put next to the
existing DAC checks.
* post-creation hooks (for fix-up security label) shall be put next
to the simple_heap_*(). Because OID and labels to be assigned are
already given, we don't need to consider such a complex things.

Even if we need to decide the place of two hooks, it seems to me
still simpler than security hooks from the scratch.

> A possibly legitimate reason to have a pre-creation hook is to prevent
> users from consuming more excessive system resources by repeatedly
> performing operations that SE-Linux will end up denying, but only
> after they're basically complete.
>
We had similar discussion before when I tried to work on something
related to table-inheritance.
MergeAttributes() checks ownership of the parent table appeared in
the INHERITS() clause, then it immediately raises an error even if
one of them was not owned by the current user, because it allows
users to prevent accesses unprivileged tables, if we check these
ownership at once later.

I learned existing privilege-checks are located with their reasons.
So, it seems to me a good strategy to follow on existing design.

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

KaiGai,

* KaiGai Kohei (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
> However, it requires the plugin modules need to know everything;
> such as what is visible/invisible. It seems to me too closely-
> coupled interface.

I agree with Robert on this one. We're not trying to design a wholly
independent security module system for any project to pick up and use
here. We're discussing hooks to go into PostgreSQL to support a
PostgreSQL security module. In other words, I don't think we need to
worry over if the PG-SELinux security module could be re-used for
another project or is too "PG specific". If it's *not* very PG
specific then something is wrong.

The issues we're talking about with regard to MVCC, visibility, etc,
would all be applicable to any serious database anyway.

Thanks,

Stephen

(2010/10/15 22:04), Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
>> However, it requires the plugin modules need to know everything;
>> such as what is visible/invisible. It seems to me too closely-
>> coupled interface.
>
> I agree with Robert on this one. We're not trying to design a wholly
> independent security module system for any project to pick up and use
> here. We're discussing hooks to go into PostgreSQL to support a
> PostgreSQL security module. In other words, I don't think we need to
> worry over if the PG-SELinux security module could be re-used for
> another project or is too "PG specific". If it's *not* very PG
> specific then something is wrong.
>
> The issues we're talking about with regard to MVCC, visibility, etc,
> would all be applicable to any serious database anyway.
>
Sorry for this delayed reply, because I've not been internet connectable
for a couple of days.

What we are always talking about is a PG specific security module, not
universal ones for any other RDBMS.

Please imagine a scenario that I'm concerning about, as follows:

If and when we will release a minor version up (E.g: 9.1.3 -> 9.1.4)
which contains hot-fixes around the object creation code and its security
hook, it may affect MVCC visibility to the guest of the security hook.
In this (bad) case, the security module would lose compatibility across
the minor version up. I said it as "security module need to know everything".
To avoid this, we will need to become paying attention what will be happen
on the security hooks whenever we apply these bug fixes. So, I'm saying it
will become a burden of management in the future.

If MVCC visibility always would match with existing permission checks,
we don't need to pay special attention for these things, do we?

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

2010/10/17 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> (2010/10/15 22:04), Stephen Frost wrote:
>> KaiGai,
>>
>> * KaiGai Kohei (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
>>> However, it requires the plugin modules need to know everything;
>>> such as what is visible/invisible. It seems to me too closely-
>>> coupled interface.
>>
>> I agree with Robert on this one.  We're not trying to design a wholly
>> independent security module system for any project to pick up and use
>> here.  We're discussing hooks to go into PostgreSQL to support a
>> PostgreSQL security module.  In other words, I don't think we need to
>> worry over if the PG-SELinux security module could be re-used for
>> another project or is too "PG specific".  If it's *not* very PG
>> specific then something is wrong.
>>
>> The issues we're talking about with regard to MVCC, visibility, etc,
>> would all be applicable to any serious database anyway.
>>
> Sorry for this delayed reply, because I've not been internet connectable
> for a couple of days.
>
> What we are always talking about is a PG specific security module, not
> universal ones for any other RDBMS.
>
> Please imagine a scenario that I'm concerning about, as follows:
>
> If and when we will release a minor version up (E.g: 9.1.3 -> 9.1.4)
> which contains hot-fixes around the object creation code and its security
> hook, it may affect MVCC visibility to the guest of the security hook.
> In this (bad) case, the security module would lose compatibility across
> the minor version up. I said it as "security module need to know everything".
> To avoid this, we will need to become paying attention what will be happen
> on the security hooks whenever we apply these bug fixes. So, I'm saying it
> will become a burden of management in the future.
>
> If MVCC visibility always would match with existing permission checks,
> we don't need to pay special attention for these things, do we?

It seems to me that you're worrying about the wrong set of problems.
The original purpose of what I proposed was to let you set a security
context on a new object at creation time, not to provide fine-grained
DDL access control. I thought perhaps we could kill two birds with
one stone, but if not, let's take three steps back and assume that DDL
permissions will be controlled using a coarse-grained check installed
in ProcessUtility_hook, so that by the time these hooks get installed
they have no job except to apply any necessary label.

But as to your question, nothing whatever excuses you from needing to
worry about MVCC. The entire backend is littered with things that
have to worry about MVCC. Whether there's a concern for any
particular bit of code depends heavily on what that code does, but "I
put it in the same place so I needn't worry" is only true if you're
doing the same thing, which you may not be. Nor is it correct to
suppose that doing permissions checking the way you're proposing is
going to somehow be free of code maintenance concerns. Indeed, many
of the patches you've submitted in this area have been rejected
precisely because they would make the code quite difficult to maintain
- for example, by passing bits of no obvious relevance to the hooks.

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

(2010/10/18 23:14), Robert Haas wrote:
>> If MVCC visibility always would match with existing permission checks,
>> we don't need to pay special attention for these things, do we?
>
> It seems to me that you're worrying about the wrong set of problems.
> The original purpose of what I proposed was to let you set a security
> context on a new object at creation time, not to provide fine-grained
> DDL access control. I thought perhaps we could kill two birds with
> one stone, but if not, let's take three steps back and assume that DDL
> permissions will be controlled using a coarse-grained check installed
> in ProcessUtility_hook, so that by the time these hooks get installed
> they have no job except to apply any necessary label.
>
Ah, yes, the original or primary purpose was automatic assignment of
security labels at creation time. Access controls on CREATE statement
is a second bird, indeed.
I agree with the idea to separate things corresponding to DDL permission
checks right now. I'll focus on the creation-time hooks to fix up
security label (or something depending on security models) on the
next commit fest.

At least, ProcessUtility_hook is too coarse-grained to implement full-
set of functionalities that we expect, so we will have a discussion about
what is the preferable design of access control hooks in the future.
But it is not right now.

> But as to your question, nothing whatever excuses you from needing to
> worry about MVCC. The entire backend is littered with things that
> have to worry about MVCC. Whether there's a concern for any
> particular bit of code depends heavily on what that code does, but "I
> put it in the same place so I needn't worry" is only true if you're
> doing the same thing, which you may not be. Nor is it correct to
> suppose that doing permissions checking the way you're proposing is
> going to somehow be free of code maintenance concerns. Indeed, many
> of the patches you've submitted in this area have been rejected
> precisely because they would make the code quite difficult to maintain
> - for example, by passing bits of no obvious relevance to the hooks.
>
Hmm. Indeed, we have many things which need to be carefully implemented,
not only access control stuff. Perhaps, it is right. So, author of plugin
modules need to pay attention even if minor updates.

However, MVCC visibility is one of the issues we need to pay attentions.
As you mentioned about, I have a bad memory about difficulty to maintain
the code. The existing permission checks are reviewed by larger number of
eyeballs than author of enhanced security modules.
So, I believe it eventually reduce burden to maintain, and enables to
overlook code-paths that bypass the upcoming DDL permission checks.

Anyway, let's have a discussion when we put security hooks for DDL checks.
But the next patch will focus on assignment of security label at object
creation.

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