security label support, part.2

2010/7/14 KaiGai Kohei <kaigai@ak.jp.nec.com>:

The attached patch is a part of efforts to support security label
on database objects.

This is similar to what I had in mind as a design for this feature,
but I have some gripes:

1. I am inclined to suggest the syntax SECURITY LABEL ON ... IS ...,
following COMMENT ON (it's also somewhat similar to the GRANT syntax).
While the hook in ExecCheckRTPerms() will only allow meaningful
permissions checks on the use of relations, there will presumably be
ongoing demand to add additional hooks to cover other types of
objects, and I'd rather add label support all at once rather than
bit-by-bit. I also think that the SECURITY LABEL syntax is more
future-proof; we don't need to worry about conflicts in other parts of
the grammar.

2. Similarly, the design of the hook in secabel.h is way too
short-sighted and won't scale to any other object type. We don't need
or want one hook per object type here. Use an ObjectAddress.

3. I am firmly of the view that we want to allow multiple security
providers. I think the way this should work here is that we should
keep a list somewhere of security providers known to the system, which
loadable modules should add themselves to. Each such security
provider should be represented by a struct containing, at least, a
name and a function that gets called on relabel. The labels should be
stored in the catalog. That way there is never any possibility of one
security provider getting a label that was originally applied by some
other security provider. If we were storing these labels in pg_class
or pg_attribute or similar, the storage cost for this might be worth
worrying about, but as this is a separate catalog, it's not.

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

Thanks for your reviewing.

(2010/07/23 0:54), Robert Haas wrote:

2010/7/14 KaiGai Kohei<kaigai@ak.jp.nec.com>:

The attached patch is a part of efforts to support security label
on database objects.

This is similar to what I had in mind as a design for this feature,
but I have some gripes:

1. I am inclined to suggest the syntax SECURITY LABEL ON ... IS ...,
following COMMENT ON (it's also somewhat similar to the GRANT syntax).
While the hook in ExecCheckRTPerms() will only allow meaningful
permissions checks on the use of relations, there will presumably be
ongoing demand to add additional hooks to cover other types of
objects, and I'd rather add label support all at once rather than
bit-by-bit. I also think that the SECURITY LABEL syntax is more
future-proof; we don't need to worry about conflicts in other parts of
the grammar.

Hmm. Indeed, we cannot deny the possibility to conflict with other part
in the future, if we use ALTER xxx statement here.

But, right now, we have no statement that starts in noun, rather than verb.
The "comment" is a noun, but "comment on" is a phrasal-verb, isn't it?

How about RELABEL <object> TO <label>, instead?

The "relabel" is a transitive-verb, we don't need "ON" between RELABEL
and <object>. And, it tries to change a property of the object, so
it seems to me "TO" is more appropriate than "IS".

2. Similarly, the design of the hook in secabel.h is way too
short-sighted and won't scale to any other object type. We don't need
or want one hook per object type here. Use an ObjectAddress.

I think the relation type is an exceptional object class, because of
the recursion due to the table inheritances.
For other object classes, I also think one security hook which takes
ObjectAddress as an argument is enough to implement.

So, I expect we need two hooks on relabeling eventually.
(One for relation, one for other object classes)

3. I am firmly of the view that we want to allow multiple security
providers. I think the way this should work here is that we should
keep a list somewhere of security providers known to the system, which
loadable modules should add themselves to. Each such security
provider should be represented by a struct containing, at least, a
name and a function that gets called on relabel. The labels should be
stored in the catalog. That way there is never any possibility of one
security provider getting a label that was originally applied by some
other security provider. If we were storing these labels in pg_class
or pg_attribute or similar, the storage cost for this might be worth
worrying about, but as this is a separate catalog, it's not.

What I'm worrying about is that we cannot estimate amount of works when
we expand the concept to row-level security. We will need to revise the
implementation, if individual user tuples have its security label in the
future version.
If we don't support multiple labels right now, it will not be feature
degradation, even if it will be hard to implement multiple label support
for each user tuples. :(

I don't deny worth of multiple security providers concurrently, however,
I doubt whether it should be supported from the beginning, or not.
It seems to me it is not too late after we can find out the way to label
individual user tuples.

Thanks,
--
KaiGai Kohei <kaigai@ak.jp.nec.com>

2010/7/22 KaiGai Kohei <kaigai@ak.jp.nec.com>:

Thanks for your reviewing.

1. I am inclined to suggest the syntax SECURITY LABEL ON ... IS ...,
following COMMENT ON (it's also somewhat similar to the GRANT syntax).
  While the hook in ExecCheckRTPerms() will only allow meaningful
permissions checks on the use of relations, there will presumably be
ongoing demand to add additional hooks to cover other types of
objects, and I'd rather add label support all at once rather than
bit-by-bit.  I also think that the SECURITY LABEL syntax is more
future-proof; we don't need to worry about conflicts in other parts of
the grammar.

Hmm. Indeed, we cannot deny the possibility to conflict with other part
in the future, if we use ALTER xxx statement here.

But, right now, we have no statement that starts in noun, rather than verb.
The "comment" is a noun, but "comment on" is a phrasal-verb, isn't it?

How about RELABEL <object> TO <label>, instead?

Well, I like SECURITY LABEL better because it's more clear about what
kind of label we're talking about, but if there's consensus on some
other option it's OK with me. Actually, we need to work the security
provider name in there too, I think, so perhaps SECURITY LABEL FOR
provider ON object IS labeltext. I realize it's slightly odd
grammatically, but it's no worse than the COMMENT syntax.

2. Similarly, the design of the hook in secabel.h is way too
short-sighted and won't scale to any other object type.  We don't need
or want one hook per object type here.  Use an ObjectAddress.

I think the relation type is an exceptional object class, because of
the recursion due to the table inheritances.
For other object classes, I also think one security hook which takes
ObjectAddress as an argument is enough to implement.

So, I expect we need two hooks on relabeling eventually.
(One for relation, one for other object classes)

Please explain in more detail.

3. I am firmly of the view that we want to allow multiple security
providers.  I think the way this should work here is that we should
keep a list somewhere of security providers known to the system, which
loadable modules should add themselves to.  Each such security
provider should be represented by a struct containing, at least, a
name and a function that gets called on relabel.  The labels should be
stored in the catalog.  That way there is never any possibility of one
security provider getting a label that was originally applied by some
other security provider.  If we were storing these labels in pg_class
or pg_attribute or similar, the storage cost for this might be worth
worrying about, but as this is a separate catalog, it's not.

What I'm worrying about is that we cannot estimate amount of works when
we expand the concept to row-level security. We will need to revise the
implementation, if individual user tuples have its security label in the
future version.
If we don't support multiple labels right now, it will not be feature
degradation, even if it will be hard to implement multiple label support
for each user tuples. :(

I think it is 100% clear that row-level security will require
completely separate infrastructure, and therefore I'm not even a tiny
bit worried about this. :-)

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

(2010/07/23 10:05), Robert Haas wrote:

2010/7/22 KaiGai Kohei<kaigai@ak.jp.nec.com>:

Thanks for your reviewing.

1. I am inclined to suggest the syntax SECURITY LABEL ON ... IS ...,
following COMMENT ON (it's also somewhat similar to the GRANT syntax).
While the hook in ExecCheckRTPerms() will only allow meaningful
permissions checks on the use of relations, there will presumably be
ongoing demand to add additional hooks to cover other types of
objects, and I'd rather add label support all at once rather than
bit-by-bit. I also think that the SECURITY LABEL syntax is more
future-proof; we don't need to worry about conflicts in other parts of
the grammar.

Hmm. Indeed, we cannot deny the possibility to conflict with other part
in the future, if we use ALTER xxx statement here.

But, right now, we have no statement that starts in noun, rather than verb.
The "comment" is a noun, but "comment on" is a phrasal-verb, isn't it?

How about RELABEL<object> TO<label>, instead?

Well, I like SECURITY LABEL better because it's more clear about what
kind of label we're talking about, but if there's consensus on some
other option it's OK with me. Actually, we need to work the security
provider name in there too, I think, so perhaps SECURITY LABEL FOR
provider ON object IS labeltext. I realize it's slightly odd
grammatically, but it's no worse than the COMMENT syntax.

The "FOR <provider>" clause should be optional. I expect most use
cases installs only one security provider, rather than multiple.

If no explicit <provider> is specified, all the security providers
check the given security label. If two or more providers are here,
of course, either of them will raise an error, because they have
different label formats. It is right.

Anyway, I'd like to implement according to the idea.

SECURITY LABEL [FOR <provider>] ON <object> IS <label>;

2. Similarly, the design of the hook in secabel.h is way too
short-sighted and won't scale to any other object type. We don't need
or want one hook per object type here. Use an ObjectAddress.

I think the relation type is an exceptional object class, because of
the recursion due to the table inheritances.
For other object classes, I also think one security hook which takes
ObjectAddress as an argument is enough to implement.

So, I expect we need two hooks on relabeling eventually.
(One for relation, one for other object classes)

Please explain in more detail.

For relations, one SECURITY LABEL statement may relabel multiple tables
when it has child tables, if ONLY clause was not given.
So, we need to obtain oids to be relabeled using find_all_inheritors(),
and need to ask providers whether it allows, or not.
But, obviously, it is specific for relations.

For other object class, the target object to be relabeled is identified
by <object> in SECURITY LABEL statement. It will be parsed by the upcoming
parse_object.c feature, then it solves the object name to ObjectAddress.
So, we can apply access controls after setting up the ObjectAddress with
common hooks for object classes except for relations, like:

void check_object_relabel(ObjectAddress object, const char *new_label);

3. I am firmly of the view that we want to allow multiple security
providers. I think the way this should work here is that we should
keep a list somewhere of security providers known to the system, which
loadable modules should add themselves to. Each such security
provider should be represented by a struct containing, at least, a
name and a function that gets called on relabel. The labels should be
stored in the catalog. That way there is never any possibility of one
security provider getting a label that was originally applied by some
other security provider. If we were storing these labels in pg_class
or pg_attribute or similar, the storage cost for this might be worth
worrying about, but as this is a separate catalog, it's not.

What I'm worrying about is that we cannot estimate amount of works when
we expand the concept to row-level security. We will need to revise the
implementation, if individual user tuples have its security label in the
future version.
If we don't support multiple labels right now, it will not be feature
degradation, even if it will be hard to implement multiple label support
for each user tuples. :(

I think it is 100% clear that row-level security will require
completely separate infrastructure, and therefore I'm not even a tiny
bit worried about this. :-)

Hmm. Are you saying we may degrade the feature when we switch to the
completely separate infrastructure? Is it preferable??

Thanks,
--
KaiGai Kohei <kaigai@ak.jp.nec.com>

2010/7/22 KaiGai Kohei <kaigai@ak.jp.nec.com>:

Well, I like SECURITY LABEL better because it's more clear about what
kind of label we're talking about, but if there's consensus on some
other option it's OK with me.  Actually, we need to work the security
provider name in there too, I think, so perhaps SECURITY LABEL FOR
provider ON object IS labeltext.  I realize it's slightly odd
grammatically, but it's no worse than the COMMENT syntax.

The "FOR <provider>" clause should be optional. I expect most use
cases installs only one security provider, rather than multiple.

If no explicit <provider> is specified, all the security providers
check the given security label. If two or more providers are here,
of course, either of them will raise an error, because they have
different label formats. It is right.

Hmm. How about if there's just one provider loaded, you can omit it,
but if you fail to specify it and there's >1 loaded, we just throw an
error saying you didn't specify whose label it is.

So, I expect we need two hooks on relabeling eventually.
(One for relation, one for other object classes)

Please explain in more detail.

For relations, one SECURITY LABEL statement may relabel multiple tables
when it has child tables, if ONLY clause was not given.
So, we need to obtain oids to be relabeled using find_all_inheritors(),
and need to ask providers whether it allows, or not.
But, obviously, it is specific for relations.

For other object class, the target object to be relabeled is identified
by <object> in SECURITY LABEL statement. It will be parsed by the upcoming
parse_object.c feature, then it solves the object name to ObjectAddress.
So, we can apply access controls after setting up the ObjectAddress with
common hooks for object classes except for relations, like:

 void check_object_relabel(ObjectAddress object, const char *new_label);

So just construct an ObjectAddress for each relation and call the
check function once for each.

I think it is 100% clear that row-level security will require
completely separate infrastructure, and therefore I'm not even a tiny
bit worried about this.  :-)

Hmm. Are you saying we may degrade the feature when we switch to the
completely separate infrastructure? Is it preferable??

Uh... no, not really. I'm saying that I don't think we're backing
ourselves into a corner. What makes you think we are?

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

(2010/07/23 12:56), Robert Haas wrote:

2010/7/22 KaiGai Kohei<kaigai@ak.jp.nec.com>:

Well, I like SECURITY LABEL better because it's more clear about what
kind of label we're talking about, but if there's consensus on some
other option it's OK with me. Actually, we need to work the security
provider name in there too, I think, so perhaps SECURITY LABEL FOR
provider ON object IS labeltext. I realize it's slightly odd
grammatically, but it's no worse than the COMMENT syntax.

The "FOR<provider>" clause should be optional. I expect most use
cases installs only one security provider, rather than multiple.

If no explicit<provider> is specified, all the security providers
check the given security label. If two or more providers are here,
of course, either of them will raise an error, because they have
different label formats. It is right.

Hmm. How about if there's just one provider loaded, you can omit it,
but if you fail to specify it and there's>1 loaded, we just throw an
error saying you didn't specify whose label it is.

Perhaps, we need to return the caller a state whether one provider checked
the given label at least, or not.

If invalid <provider> was specified so nobody checked it, nobody returns
the caller a state of "checked", then it raises an error to notice invalid
security provider.

If valid <provider> was specified, only specified provider checks the given
label, and returns the caller a state of "it was checked by xxxx".

If it was omitted, all the providers try to check the given label, but it
has mutually different format, so one of providers will raise an error at
least.

It means we have to specify the provider when two or more providers are
loaded, but not necessary when just one provider.

So, I expect we need two hooks on relabeling eventually.
(One for relation, one for other object classes)

Please explain in more detail.

For relations, one SECURITY LABEL statement may relabel multiple tables
when it has child tables, if ONLY clause was not given.
So, we need to obtain oids to be relabeled using find_all_inheritors(),
and need to ask providers whether it allows, or not.
But, obviously, it is specific for relations.

For other object class, the target object to be relabeled is identified
by<object> in SECURITY LABEL statement. It will be parsed by the upcoming
parse_object.c feature, then it solves the object name to ObjectAddress.
So, we can apply access controls after setting up the ObjectAddress with
common hooks for object classes except for relations, like:

void check_object_relabel(ObjectAddress object, const char *new_label);

So just construct an ObjectAddress for each relation and call the
check function once for each.

OK, I'll revise it.

I think it is 100% clear that row-level security will require
completely separate infrastructure, and therefore I'm not even a tiny
bit worried about this. :-)

Hmm. Are you saying we may degrade the feature when we switch to the
completely separate infrastructure? Is it preferable??

Uh... no, not really. I'm saying that I don't think we're backing
ourselves into a corner. What makes you think we are?

Sorry, meaning of the last question was unclear for me.... Is it a idiom?

Thanks,
--
KaiGai Kohei <kaigai@ak.jp.nec.com>

2010/7/23 KaiGai Kohei <kaigai@ak.jp.nec.com>:

Hmm.  How about if there's just one provider loaded, you can omit it,
but if you fail to specify it and there's>1 loaded, we just throw an
error saying you didn't specify whose label it is.

Perhaps, we need to return the caller a state whether one provider checked
the given label at least, or not.

Return to the caller? This is an SQL command. You either get an
error, or you don't.

If it was omitted, all the providers try to check the given label, but it
has mutually different format, so one of providers will raise an error at
least.

Yeah, but it won't be a very clear error, and what if you have, say, a
provider that allows arbitrary strings as labels? Since this is a
security feature, I think it's a pretty bad idea to allow the user to
do anything that might be ambiguous.

It means we have to specify the provider when two or more providers are
loaded, but not necessary when just one provider.

But that should be fine. Loading multiple providers should, as you
say, be fairly rare.

I think it is 100% clear that row-level security will require
completely separate infrastructure, and therefore I'm not even a tiny
bit worried about this.  :-)

Hmm. Are you saying we may degrade the feature when we switch to the
completely separate infrastructure? Is it preferable??

Uh... no, not really.  I'm saying that I don't think we're backing
ourselves into a corner.  What makes you think we are?

Sorry, meaning of the last question was unclear for me.... Is it a idiom?

I don't understand why we wouldn't be able to support multiple
providers for row-level security. Why do you think that's a problem?

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

* Robert Haas (robertmhaas@gmail.com) wrote:

I don't understand why we wouldn't be able to support multiple
providers for row-level security. Why do you think that's a problem?

My guess would be that he's concerned about only having space in the
tuple header for 1 label. I see two answers- only allow 1 provider for
a given relation (doesn't strike me as a horrible limitation), or handle
labels as extra columns where you could have more than one.

Thanks,

Stephen

On Fri, Jul 23, 2010 at 8:32 AM, Stephen Frost <sfrost@snowman.net> wrote:

* Robert Haas (robertmhaas@gmail.com) wrote:

I don't understand why we wouldn't be able to support multiple
providers for row-level security.  Why do you think that's a problem?

My guess would be that he's concerned about only having space in the
tuple header for 1 label.  I see two answers- only allow 1 provider for
a given relation (doesn't strike me as a horrible limitation), or handle
labels as extra columns where you could have more than one.

I think we've been pretty clear in previous discussions that any
row-level security implementation should be a general one, and
SE-Linux or whatever can integrate with that to do what it needs to
do. So I'm pretty sure we'll be using regular columns rather than
cramming anything into the tuple header. There are pretty substantial
performance benefits to such an implementation, as well.

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

(2010/07/23 20:44), Robert Haas wrote:

2010/7/23 KaiGai Kohei<kaigai@ak.jp.nec.com>:

Hmm. How about if there's just one provider loaded, you can omit it,
but if you fail to specify it and there's>1 loaded, we just throw an
error saying you didn't specify whose label it is.

Perhaps, we need to return the caller a state whether one provider checked
the given label at least, or not.

Return to the caller? This is an SQL command. You either get an
error, or you don't.

Ahh, I was talked about relationship between the core PG code and ESP module.
It means the security hook returns a state which informs the core PG code
whether one provider checked the given label, then the core PG code can
decide whether it raise an actual error to users, or not.

In other words, I'd like to suggest the security hook which returns a tag
of ESP module, as follows:

const char *
check_object_relabel_hook(const ObjectAddress *object,
const char *provider,
const char *seclabel);

The second argument reflects "FOR <provider>" clause. It informs ESP modules
what provider is specified. If omitted, it will be NULL.

Then, ESP module which checked the given security label must return its tag.
Maybe, "selinux", if SE-PostgreSQL. Or, NULL will be returned if nobody
checked it. If NULL or incorrect tag is returned, the core PG code can know
the given seclabel is not checked/validated, then it will raise an error to
users.

Elsewhere, the validated label will be stored with the returned tag.
It enables to recognize what label is validated by SELinux, and what label
is not.

If it was omitted, all the providers try to check the given label, but it
has mutually different format, so one of providers will raise an error at
least.

Yeah, but it won't be a very clear error, and what if you have, say, a
provider that allows arbitrary strings as labels? Since this is a
security feature, I think it's a pretty bad idea to allow the user to
do anything that might be ambiguous.

It is provider's job to validate the given security label.
So, if we install such a security module which accept arbitrary strings
as label, the core PG code also need to believe the ESP module.

But the arbitrary label will be tagged with something other than "selinux",
so it does not confuse other module, according to the above idea.

It means we have to specify the provider when two or more providers are
loaded, but not necessary when just one provider.

But that should be fine. Loading multiple providers should, as you
say, be fairly rare.

I think it is 100% clear that row-level security will require
completely separate infrastructure, and therefore I'm not even a tiny
bit worried about this. :-)

Hmm. Are you saying we may degrade the feature when we switch to the
completely separate infrastructure? Is it preferable??

Uh... no, not really. I'm saying that I don't think we're backing
ourselves into a corner. What makes you think we are?

Sorry, meaning of the last question was unclear for me.... Is it a idiom?

I don't understand why we wouldn't be able to support multiple
providers for row-level security. Why do you think that's a problem?

I don't have any clear reason why we wouldn't be able to support multiple
labels on user tuples, but it is intangible anxiety, because I have not
implemented it as a working example yet.
(So, I never think it is impossible.)

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Fri, Jul 23, 2010 at 8:59 AM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote:

(2010/07/23 20:44), Robert Haas wrote:

2010/7/23 KaiGai Kohei<kaigai@ak.jp.nec.com>:

Hmm.  How about if there's just one provider loaded, you can omit it,
but if you fail to specify it and there's>1 loaded, we just throw an
error saying you didn't specify whose label it is.

Perhaps, we need to return the caller a state whether one provider
checked
the given label at least, or not.

Return to the caller?  This is an SQL command.  You either get an
error, or you don't.

Ahh, I was talked about relationship between the core PG code and ESP
module.
It means the security hook returns a state which informs the core PG code
whether one provider checked the given label, then the core PG code can
decide whether it raise an actual error to users, or not.

In other words, I'd like to suggest the security hook which returns a tag
of ESP module, as follows:

 const char *
 check_object_relabel_hook(const ObjectAddress *object,
                           const char *provider,
                           const char *seclabel);

I don't think that's a very good design. What I had in mind was a
simple API for security providers to register themselves (including
their names), and then the core code will only call the relevant
security provider. I did try to explain this in point #3 of my
original review.

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

The attached patches are revised ones, as follows.

* A new SECURITY LABEL statement replaced the previous ALTER TABLE statement
with SECURITY LABEL TO option. It has the following syntax.

SECURITY LABEL [ FOR <provider> ] ON <object class> <object name> IS '<label>';

E.g) SECURITY LABEL ON TABLE t1 IS 'system_u:object_r:sepgsql_table_t:s0';

* It supports multiple security providers to assign its security label on
a database object. The pg_seclabel catalog was modified as follows:

CATALOG(pg_seclabel,3037) BKI_WITHOUT_OIDS
{
Oid reloid; /* OID of table containing the object */
Oid objoid; /* OID of the object itself */
int4 subid; /* column number, or 0 if not used */
+ text tag; /* identifier of external security provider */
text label; /* security label of the object */
} FormData_pg_seclabel;

The new 'tag' field identifies which security provider manages this
security label. For example, SE-PostgreSQL uses "selinux" for its
identifier.

* The security hook to check relabeling become to be registered using
register_object_relabel_hook() which takes a tag of ESP module and
a function pointer to the security hook.
ExecSecLabelStmt() picks up an appropriate security hook, then it
shall be invoked even if multiple modules are loaded.

* Add _copySecLabelStmt() on nodes/copyfuncs.c and _equalSecLabelStmt()
on nodes/equalfuncs.c, because I forgot to add them, although new
parsenode (SecLabelStmt) was added.

* Add descriptions about pg_seclabel catalog and SECURITY LABEL statement
on the documentation.

Thanks,

(2010/07/23 22:36), Robert Haas wrote:

On Fri, Jul 23, 2010 at 8:59 AM, KaiGai Kohei<kaigai@kaigai.gr.jp> wrote:

(2010/07/23 20:44), Robert Haas wrote:

2010/7/23 KaiGai Kohei<kaigai@ak.jp.nec.com>:

Hmm. How about if there's just one provider loaded, you can omit it,
but if you fail to specify it and there's>1 loaded, we just throw an
error saying you didn't specify whose label it is.

Perhaps, we need to return the caller a state whether one provider
checked
the given label at least, or not.

Return to the caller? This is an SQL command. You either get an
error, or you don't.

Ahh, I was talked about relationship between the core PG code and ESP
module.
It means the security hook returns a state which informs the core PG code
whether one provider checked the given label, then the core PG code can
decide whether it raise an actual error to users, or not.

In other words, I'd like to suggest the security hook which returns a tag
of ESP module, as follows:

const char *
check_object_relabel_hook(const ObjectAddress *object,
const char *provider,
const char *seclabel);

I don't think that's a very good design. What I had in mind was a
simple API for security providers to register themselves (including
their names), and then the core code will only call the relevant
security provider. I did try to explain this in point #3 of my
original review.

--
KaiGai Kohei <kaigai@ak.jp.nec.com>

2010/7/26 KaiGai Kohei <kaigai@ak.jp.nec.com>:

The attached patches are revised ones, as follows.

I think this is pretty good, and I'm generally in favor of committing
it. Some concerns:

1. Since nobody has violently objected to the comment.c refactoring
patch I recently proposed, I'm hopeful that can go in. And if that's
the case, then I'd prefer to see that committed first, and then rework
this to use that code. That would eliminate some code here, and it
would also make it much easier to support labels on other types of
objects.

2. Some of this code refers to "local" security labels. I'm not sure
what's "local" about them - aren't they just security labels? On a
related note, I don't like the trivial wrappers you have here, with
DeleteSecurityLabel around DeleteLocalSecLabel, SetSecurityLabel
around SetLocalSecLabel, etc. Just collapse these into a single set
of functions.

3. Is it really appropriate for ExecRelationSecLabel() to have an
"Exec" prefix? I don't think so.

4. Please get rid of the nkeys++ stuff in DeleteLocalSecLabel() and
just use fixed offsets as we do everywhere else.

5. Why do we think that the relabel hook needs to be passed the number
of expected parents?

6. What are we doing about the assignment of initial security labels?
I had initially thought that perhaps new objects would just start out
unlabeled, and the user would be responsible for labeling them as
needed. But maybe we can do better. Perhaps we should extend the
security provider hook API with a function that gets called when a
labellable object gets created, and let each loaded security provider
return any label it would like attached. Even if we don't do that
now, esp_relabel_hook_entry needs to be renamed to something more
generic; we will certainly want to add more fields to that structure
later.

7. I think we need to write and include in the fine documentation some
"big picture" documentation about enhanced security providers. Of
course, we have to decide what we want to say. But the SECURITY LABEL
documentation is just kind of hanging out there in space right now; it
needs to connect to a broad introduction to the subject.

8. Generally, the English in both the comments and documentation needs
work. However, we can address that problem when we're closer to
commit.

I am going to mark this Returned with Feedback because I don't believe
it's realistic to get the comment code committed in the next week,
rework this patch, and then get this patch committed also. However,
I'm feeling pretty good about this effort and I think we're making
good progress toward getting this done.

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

Thanks for your reviewing.

On Mon, 9 Aug 2010 16:02:12 -0400
Robert Haas <robertmhaas@gmail.com> wrote:

2010/7/26 KaiGai Kohei <kaigai@ak.jp.nec.com>:

The attached patches are revised ones, as follows.

I think this is pretty good, and I'm generally in favor of committing
it. Some concerns:

1. Since nobody has violently objected to the comment.c refactoring
patch I recently proposed, I'm hopeful that can go in. And if that's
the case, then I'd prefer to see that committed first, and then rework
this to use that code. That would eliminate some code here, and it
would also make it much easier to support labels on other types of
objects.

It seems to me fair enough. This parse_object.c can also provide
a facility to resolve the name of object to be labeled.

2. Some of this code refers to "local" security labels. I'm not sure
what's "local" about them - aren't they just security labels? On a
related note, I don't like the trivial wrappers you have here, with
DeleteSecurityLabel around DeleteLocalSecLabel, SetSecurityLabel
around SetLocalSecLabel, etc. Just collapse these into a single set
of functions.

In the feature, I plan to assign security labels on the shared database
objects such as pg_database. The "local" is a contradistinction
towards these "shared" objects.

3. Is it really appropriate for ExecRelationSecLabel() to have an
"Exec" prefix? I don't think so.

I don't have any preferences about

4. Please get rid of the nkeys++ stuff in DeleteLocalSecLabel() and
just use fixed offsets as we do everywhere else.

OK, I'll fix it.

5. Why do we think that the relabel hook needs to be passed the number
of expected parents?

We need to prevent relabeling on inherited relations/columns from
the multiple origin, like ALTER RENAME TO.
It requires to pass the expected parents into the provider, or
to check it in the caller.

6. What are we doing about the assignment of initial security labels?
I had initially thought that perhaps new objects would just start out
unlabeled, and the user would be responsible for labeling them as
needed. But maybe we can do better. Perhaps we should extend the
security provider hook API with a function that gets called when a
labellable object gets created, and let each loaded security provider
return any label it would like attached. Even if we don't do that
now, esp_relabel_hook_entry needs to be renamed to something more
generic; we will certainly want to add more fields to that structure
later.

Starting with "unlabeled" is wrong, because it does not distinguish
an object created by security sensitive users and insensitive users,
for example. It is similar to relation's relowner is InvalidOid.

I plan the security provider hook on the creation time works two things.
1. It checks user's privilege to create this object.
2. It returns security labels to be assigned.

Then, the caller assigns these returned labels on the new object,
if one or more valid labels are returned.

7. I think we need to write and include in the fine documentation some
"big picture" documentation about enhanced security providers. Of
course, we have to decide what we want to say. But the SECURITY LABEL
documentation is just kind of hanging out there in space right now; it
needs to connect to a broad introduction to the subject.

OK, I'll try to describe with appropriate granularity.
Do we need an independent section in addition to the introduction of
SECURITY LABEL syntax?

8. Generally, the English in both the comments and documentation needs
work. However, we can address that problem when we're closer to
commit.

OK

BTW, I'll go on the area where internet unconnectable during next
two days. Perhaps, my reply will run late.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

2010/8/9 <kaigai@kaigai.gr.jp>:

2. Some of this code refers to "local" security labels.  I'm not sure
what's "local" about them - aren't they just security labels?  On a
related note, I don't like the trivial wrappers you have here, with
DeleteSecurityLabel around DeleteLocalSecLabel, SetSecurityLabel
around SetLocalSecLabel, etc.  Just collapse these into a single set
of functions.

In the feature, I plan to assign security labels on the shared database
objects such as pg_database. The "local" is a contradistinction
towards these "shared" objects.

Oh, I see. I don't think that's entirely clear: and in any event it
seems a bit premature, since we're not at that point yet. Let's just
get rid of this stuff for now as I suggested.

5. Why do we think that the relabel hook needs to be passed the number
of expected parents?

We need to prevent relabeling on inherited relations/columns from
the multiple origin, like ALTER RENAME TO.
It requires to pass the expected parents into the provider, or
to check it in the caller.

Please explain further. I don't understand.

6. What are we doing about the assignment of initial security labels?
I had initially thought that perhaps new objects would just start out
unlabeled, and the user would be responsible for labeling them as
needed.  But maybe we can do better.  Perhaps we should extend the
security provider hook API with a function that gets called when a
labellable object gets created, and let each loaded security provider
return any label it would like attached.  Even if we don't do that
now, esp_relabel_hook_entry needs to be renamed to something more
generic; we will certainly want to add more fields to that structure
later.

Starting with "unlabeled" is wrong, because it does not distinguish
an object created by security sensitive users and insensitive users,
for example. It is similar to relation's relowner is InvalidOid.

I plan the security provider hook on the creation time works two things.
1. It checks user's privilege to create this object.
2. It returns security labels to be assigned.

Then, the caller assigns these returned labels on the new object,
if one or more valid labels are returned.

OK, let's give that a try and see how it looks. I don't think that's
in this version of the patch, right?

7. I think we need to write and include in the fine documentation some
"big picture" documentation about enhanced security providers.  Of
course, we have to decide what we want to say.  But the SECURITY LABEL
documentation is just kind of hanging out there in space right now; it
needs to connect to a broad introduction to the subject.

OK, I'll try to describe with appropriate granularity.
Do we need an independent section in addition to the introduction of
SECURITY LABEL syntax?

I think so. I suggest a new chapter called "Enhanced Security
Providers" just after "Database Roles and Privileges".

BTW, I'll go on the area where internet unconnectable during next
two days. Perhaps, my reply will run late.

No problem.

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

(2010/08/10 8:39), Robert Haas wrote:

2010/8/9<kaigai@kaigai.gr.jp>:

2. Some of this code refers to "local" security labels. I'm not sure
what's "local" about them - aren't they just security labels? On a
related note, I don't like the trivial wrappers you have here, with
DeleteSecurityLabel around DeleteLocalSecLabel, SetSecurityLabel
around SetLocalSecLabel, etc. Just collapse these into a single set
of functions.

In the feature, I plan to assign security labels on the shared database
objects such as pg_database. The "local" is a contradistinction
towards these "shared" objects.

Oh, I see. I don't think that's entirely clear: and in any event it
seems a bit premature, since we're not at that point yet. Let's just
get rid of this stuff for now as I suggested.

OK. We can add this supportanytime we need it.

5. Why do we think that the relabel hook needs to be passed the number
of expected parents?

We need to prevent relabeling on inherited relations/columns from
the multiple origin, like ALTER RENAME TO.
It requires to pass the expected parents into the provider, or
to check it in the caller.

Please explain further. I don't understand.

Yep, rte->requiredPerms of inherited relations are cleared on the
expand_inherited_rtentry() since the v9.0, so we cannot know what
kind of accesses are required on the individual child relations.
It needs the inherited relations/columns being labeled with same
security label of their parent, because SE-PgSQL always makes same
access control decision on same security labels.

Thus, we want to check whether the relabeling operation breaks the
uniqueness of security label within a certain inheritance tree, or not.

Here is the logic to check relabeling on relation/column.
http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/hooks.c#254
It checks two things.
1) The given relation/column must be the origin of inheritance tree
when expected_parents = 0.
2) The given relation/column must not belong to multiple inheritance
tree.

So, the hook need to provide the expected_parents for each relations/columns.

6. What are we doing about the assignment of initial security labels?
I had initially thought that perhaps new objects would just start out
unlabeled, and the user would be responsible for labeling them as
needed. But maybe we can do better. Perhaps we should extend the
security provider hook API with a function that gets called when a
labellable object gets created, and let each loaded security provider
return any label it would like attached. Even if we don't do that
now, esp_relabel_hook_entry needs to be renamed to something more
generic; we will certainly want to add more fields to that structure
later.

Starting with "unlabeled" is wrong, because it does not distinguish
an object created by security sensitive users and insensitive users,
for example. It is similar to relation's relowner is InvalidOid.

I plan the security provider hook on the creation time works two things.
1. It checks user's privilege to create this object.
2. It returns security labels to be assigned.

Then, the caller assigns these returned labels on the new object,
if one or more valid labels are returned.

OK, let's give that a try and see how it looks. I don't think that's
in this version of the patch, right?

Yes, this version of the patch didn't support labeling on creation time
of database objects. It shall be added in separated patch.

7. I think we need to write and include in the fine documentation some
"big picture" documentation about enhanced security providers. Of
course, we have to decide what we want to say. But the SECURITY LABEL
documentation is just kind of hanging out there in space right now; it
needs to connect to a broad introduction to the subject.

OK, I'll try to describe with appropriate granularity.
Do we need an independent section in addition to the introduction of
SECURITY LABEL syntax?

I think so. I suggest a new chapter called "Enhanced Security
Providers" just after "Database Roles and Privileges".

OK,

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

* KaiGai Kohei (kaigai@kaigai.gr.jp) wrote:

Yep, rte->requiredPerms of inherited relations are cleared on the
expand_inherited_rtentry() since the v9.0, so we cannot know what
kind of accesses are required on the individual child relations.

This is really a PG issue and decision, in my view. We're moving more
and more towards a decision that inherited relations are really just the
same relation but broken up per tables (ala "true" partitioning). As
such, PG has chosen to view them as the same wrt permissions checking.
I don't think we should make a different decision for security labels.
If you don't want people who have access to the parent to have access to
the children, then you shouldn't be making them children.

Thanks,

Stephen

(2010/08/15 9:16), Stephen Frost wrote:

* KaiGai Kohei (kaigai@kaigai.gr.jp) wrote:

Yep, rte->requiredPerms of inherited relations are cleared on the
expand_inherited_rtentry() since the v9.0, so we cannot know what
kind of accesses are required on the individual child relations.

This is really a PG issue and decision, in my view. We're moving more
and more towards a decision that inherited relations are really just the
same relation but broken up per tables (ala "true" partitioning). As
such, PG has chosen to view them as the same wrt permissions checking.
I don't think we should make a different decision for security labels.
If you don't want people who have access to the parent to have access to
the children, then you shouldn't be making them children.

No, what I want to do is people have identical access rights on both of
the parent and children. If they have always same label, SE-PgSQL always
makes same access control decision. This behavior is suitable to the
standpoint that inherited relations are really just the same relation
of the parent. For this purpose, I want to enforce a unique label on
a certain inheritance tree.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

2010/8/14 KaiGai Kohei <kaigai@kaigai.gr.jp>:

(2010/08/15 9:16), Stephen Frost wrote:

* KaiGai Kohei (kaigai@kaigai.gr.jp) wrote:

Yep, rte->requiredPerms of inherited relations are cleared on the
expand_inherited_rtentry() since the v9.0, so we cannot know what
kind of accesses are required on the individual child relations.

This is really a PG issue and decision, in my view.  We're moving more
and more towards a decision that inherited relations are really just the
same relation but broken up per tables (ala "true" partitioning).  As
such, PG has chosen to view them as the same wrt permissions checking.
I don't think we should make a different decision for security labels.
If you don't want people who have access to the parent to have access to
the children, then you shouldn't be making them children.

No, what I want to do is people have identical access rights on both of
the parent and children. If they have always same label, SE-PgSQL always
makes same access control decision. This behavior is suitable to the
standpoint that inherited relations are really just the same relation
of the parent. For this purpose, I want to enforce a unique label on
a certain inheritance tree.

This seems like a bad idea to me, too. I think it's arguable whether
access to the children should be controlled by the parent's label or
the child's label, but enforcing that the entire inheritance hierarchy
is identically labeled seems like a pointless restriction. As Stephen
points out, it's also wildly inconsistent with the way we currently
handle it.

There's also the problem that the hooks we're talking about here are
inadequate to support such a restriction anyway. You'd need some kind
of a hook in ALTER TABLE ... [NO] INHERIT, at a minimum. As has been
mentioned many times before in reviewing many generations of
SE-PostgreSQL patches, we're not going to get into the business of
re-engineering our security architecture just because you would have
designed it differently. Inventing something that's randomly
different will not only make the code ugly and hard to maintain; it
will also be confusing and difficult to manage for end-users.

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