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@ak.jp.nec.com>

Search Discussions

  • Robert Haas at Sep 28, 2010 at 3:57 am

    2010/9/1 KaiGai Kohei <kaigai@ak.jp.nec.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
  • KaiGai Kohei at Sep 29, 2010 at 10:38 am
    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@ak.jp.nec.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@kaigai.gr.jp>
  • Robert Haas at Sep 29, 2010 at 1:00 pm

    On Wed, Sep 29, 2010 at 6:38 AM, KaiGai Kohei 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
  • KaiGai Kohei at Sep 29, 2010 at 1:59 pm

    (2010/09/29 22:00), Robert Haas wrote:
    On Wed, Sep 29, 2010 at 6:38 AM, KaiGai Koheiwrote:
    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@kaigai.gr.jp>
  • Stephen Frost at Sep 29, 2010 at 3:14 pm
    KaiGai,

    * KaiGai Kohei (kaigai@kaigai.gr.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
  • Robert Haas at Sep 29, 2010 at 3:37 pm

    On Wed, Sep 29, 2010 at 9:59 AM, KaiGai Kohei wrote:
    (2010/09/29 22:00), Robert Haas wrote:
    On Wed, Sep 29, 2010 at 6:38 AM, KaiGai Koheiwrote:
    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
  • KaiGai Kohei at Sep 30, 2010 at 1:07 am

    (2010/09/30 0:36), Robert Haas wrote:
    On Wed, Sep 29, 2010 at 9:59 AM, KaiGai Koheiwrote:
    (2010/09/29 22:00), Robert Haas wrote:
    On Wed, Sep 29, 2010 at 6:38 AM, KaiGai Koheiwrote:
    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@ak.jp.nec.com>
  • Robert Haas at Sep 30, 2010 at 1:28 am

    2010/9/29 KaiGai Kohei <kaigai@ak.jp.nec.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
  • KaiGai Kohei at Sep 30, 2010 at 2:19 am

    (2010/09/30 10:28), Robert Haas wrote:
    2010/9/29 KaiGai Kohei<kaigai@ak.jp.nec.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@ak.jp.nec.com>
  • Robert Haas at Sep 30, 2010 at 6:14 pm

    2010/9/29 KaiGai Kohei <kaigai@ak.jp.nec.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
  • KaiGai Kohei at Oct 1, 2010 at 1:01 am

    (2010/10/01 3:09), Robert Haas wrote:
    2010/9/29 KaiGai Kohei<kaigai@ak.jp.nec.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@ak.jp.nec.com>
  • Robert Haas at Oct 1, 2010 at 2:23 am

    On Sep 30, 2010, at 9:01 PM, KaiGai Kohei wrote:
    (2010/10/01 3:09), Robert Haas wrote:
    2010/9/29 KaiGai Kohei<kaigai@ak.jp.nec.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
  • KaiGai Kohei at Oct 5, 2010 at 9:03 am

    (2010/10/01 11:23), Robert Haas wrote:
    On Sep 30, 2010, at 9:01 PM, KaiGai Koheiwrote:
    (2010/10/01 3:09), Robert Haas wrote:
    2010/9/29 KaiGai Kohei<kaigai@ak.jp.nec.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@ak.jp.nec.com>
  • Robert Haas at Oct 6, 2010 at 9:02 pm

    2010/10/5 KaiGai Kohei <kaigai@ak.jp.nec.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
  • Alvaro Herrera at Oct 6, 2010 at 9:21 pm

    Excerpts from Robert Haas's message of mié oct 06 17:02:22 -0400 2010:
    2010/10/5 KaiGai Kohei <kaigai@ak.jp.nec.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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • KaiGai Kohei at Oct 7, 2010 at 12:37 am

    (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@ak.jp.nec.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@ak.jp.nec.com>
  • Robert Haas at Oct 7, 2010 at 3:21 pm

    On Wed, Oct 6, 2010 at 5:21 PM, Alvaro Herrera wrote:
    Excerpts from Robert Haas's message of mié oct 06 17:02:22 -0400 2010:
    2010/10/5 KaiGai Kohei <kaigai@ak.jp.nec.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
  • KaiGai Kohei at Oct 8, 2010 at 12:40 am

    (2010/10/08 0:21), Robert Haas wrote:
    On Wed, Oct 6, 2010 at 5:21 PM, Alvaro Herrera
    wrote:
    Excerpts from Robert Haas's message of mié oct 06 17:02:22 -0400 2010:
    2010/10/5 KaiGai Kohei<kaigai@ak.jp.nec.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@ak.jp.nec.com>
  • KaiGai Kohei at Oct 12, 2010 at 1:59 am
    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
    wrote:
    Excerpts from Robert Haas's message of mié oct 06 17:02:22 -0400 2010:
    2010/10/5 KaiGai Kohei<kaigai@ak.jp.nec.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@ak.jp.nec.com>
  • Robert Haas at Oct 12, 2010 at 2:36 am

    On Mon, Oct 11, 2010 at 9:58 PM, KaiGai Kohei 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
  • KaiGai Kohei at Oct 12, 2010 at 3:02 am

    (2010/10/12 11:35), Robert Haas wrote:
    On Mon, Oct 11, 2010 at 9:58 PM, KaiGai Koheiwrote:
    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@ak.jp.nec.com>
  • Robert Haas at Oct 12, 2010 at 11:59 am

    2010/10/11 KaiGai Kohei <kaigai@ak.jp.nec.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
  • KaiGai Kohei at Oct 12, 2010 at 1:21 pm

    (2010/10/12 20:59), Robert Haas wrote:
    2010/10/11 KaiGai Kohei<kaigai@ak.jp.nec.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@kaigai.gr.jp>
  • Robert Haas at Oct 12, 2010 at 2:09 pm

    On Tue, Oct 12, 2010 at 9:20 AM, KaiGai Kohei 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
  • KaiGai Kohei at Oct 13, 2010 at 1:05 am

    (2010/10/12 23:09), Robert Haas wrote:
    On Tue, Oct 12, 2010 at 9:20 AM, KaiGai Koheiwrote:
    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@ak.jp.nec.com>
  • Stephen Frost at Oct 15, 2010 at 1:04 pm
    KaiGai,

    * KaiGai Kohei (kaigai@kaigai.gr.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
  • KaiGai Kohei at Oct 18, 2010 at 1:09 am

    (2010/10/15 22:04), Stephen Frost wrote:
    KaiGai,

    * KaiGai Kohei (kaigai@kaigai.gr.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@ak.jp.nec.com>
  • Robert Haas at Oct 18, 2010 at 2:14 pm

    2010/10/17 KaiGai Kohei (2010/10/15 22:04), Stephen Frost wrote:
    KaiGai,

    * KaiGai Kohei (kaigai@kaigai.gr.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
  • KaiGai Kohei at Oct 19, 2010 at 4:34 am

    (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@ak.jp.nec.com>
  • KaiGai Kohei at Oct 7, 2010 at 12:32 am

    (2010/10/07 6:02), Robert Haas wrote:
    2010/10/5 KaiGai Kohei<kaigai@ak.jp.nec.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@ak.jp.nec.com>
  • Alvaro Herrera at Sep 29, 2010 at 1:59 pm

    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@ak.jp.nec.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@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedSep 2, '10 at 12:38a
activeOct 19, '10 at 4:34a
posts32
users5
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase