Re: security label support, revised

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

It adds statement support to manage security label of relations.
Right now, object labeling except for relations/columns are not
supported, because the DML permission hook is the only chance to
apply access control decision of ESP module.

It has the following syntax:
ALTER TABLE <relation_expr> [ALTER [COLUMN] <colmu_name>]
SECURITY LABEL TO '<label>';

I believe Robert's refactoring on COMMENT ON code also helps to
implement security label support for various kind of object classes.
However, we need to handle relabeling on the tables particularly
because of table's inheritances, unlike any other object classes.
So, I considered we can make progress these works in progress, then
we can integrated them later.

Example:
postgres=# CREATE TABLE t (a int, b text);
CREATE TABLE
postgres=# ALTER TABLE t SECURITY LABEL TO 'system_u:object_r:sepgsql_table_t:s0';
ALTER TABLE
postgres=# ALTER TABLE t ALTER a SECURITY LABEL TO 'system_u:object_r:sepgsql_table_t:s0';
ALTER TABLE
postgres=# ALTER TABLE t ALTER b SECURITY LABEL TO 'system_u:object_r:sepgsql_table_t:s0:c1';
ALTER TABLE

[kaigai(at)saba ~]$ runcon -l s0 psql postgres
psql (9.1devel)
Type "help" for help.

postgres=# set client_min_messages = log;
SET
postgres=# SELECT * FROM t;
LOG: SELinux: denied { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0 tcontext=system_u:object_r:sepgsql_table_t:s0:c1 tclass=db_column name=t.b
ERROR: SELinux: security policy violation
postgres=# SELECT a FROM t;
a
---
(0 rows)

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

2010/7/14 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

2010/7/22 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

2010/7/22 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

2010/7/23 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)gmail(dot)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(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)kaigai(dot)gr(dot)jp>

On Fri, Jul 23, 2010 at 8:59 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> (2010/07/23 20:44), Robert Haas wrote:
>>
>> 2010/7/23 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)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(at)kaigai(dot)gr(dot)jp> wrote:
>> (2010/07/23 20:44), Robert Haas wrote:
>>>
>>> 2010/7/23 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

2010/7/26 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)gmail(dot)com> wrote:

> 2010/7/26 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)kaigai(dot)gr(dot)jp>

2010/8/9 <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

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

2010/8/14 KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>:
> (2010/08/15 9:16), Stephen Frost wrote:
>> * KaiGai Kohei (kaigai(at)kaigai(dot)gr(dot)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

(2010/08/15 9:55), Robert Haas wrote:
> 2010/8/14 KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp>:
>> (2010/08/15 9:16), Stephen Frost wrote:
>>> * KaiGai Kohei (kaigai(at)kaigai(dot)gr(dot)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.
>
Indeed, our existing mechanism allows to assign individual privileges
on child tables, even if it is in a certain inheritance hierarchy.

The purpose of this restriction is to ensure an access control decision
using parent's label being also consistent on child tables.
If we control accesses on child tables using child's label, no need to
restrict an identical label within an entire inheritance hierarchy.
But it needs to provide the original rte->requiredPerms of child tables.
Now it is cleared at expand_inherited_rtentry(), so we have no way to
control accesses on child tables using child's label. :(

From viewpoint of MAC, both of the following SQLs should be denied,
when accesses on parent_tbl is allowed, but child_tbl is denied.

1) SELECT * FROM parent_tbl;

2) SELECT * FROM child_tbl;

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

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> The purpose of this restriction is to ensure an access control decision
> using parent's label being also consistent on child tables.

Robert and I understand the concern that you have. The answer, at least
for now, is that we don't agree with you. PG doesn't consider child
tables to be independent objects when they're being accessed through the
parent. As such, they don't have their own permissions checking.

> If we control accesses on child tables using child's label, no need to
> restrict an identical label within an entire inheritance hierarchy.
> But it needs to provide the original rte->requiredPerms of child tables.
> Now it is cleared at expand_inherited_rtentry(), so we have no way to
> control accesses on child tables using child's label. :(

If you want to argue that we should care about the childs permissions,
or do something different with regard to inheritance, then you need to
make that argument for all of PG, not just try to do what you think is
right in the security definer framework.

> >From viewpoint of MAC, both of the following SQLs should be denied,
> when accesses on parent_tbl is allowed, but child_tbl is denied.

KaiGai, this is not a MAC vs. DAC difference. This is a question of
"what is an object" and if a child table is really an independent object
from a parent table. In PG, we've decided they're not. We should
probably do more to make that clearer in PG, rather than have different
parts of the system treat them differently.

Thanks,

Stephen

>Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> PG doesn't consider child tables to be independent objects when
> they're being accessed through the parent. As such, they don't
> have their own permissions checking.

I've been thinking about this from the perspective of possible
eventual use by the Wisconsin Courts, and want to throw out my
perspective on long-term direction here, without venturing any
opinion on the immediate issue.

It wouldn't be totally insane for the courts to some day use
inheritance to deal with court cases. All court cases have much in
common and have the same basic structure, but specific case types
need to store some additional information. If we did that, we would
want different permissions on different case types -- for example,
juvenile cases are not open to the public as many case types are.
We would also need the ability to revoke public permissions on
specific rows, as judges can seal cases or various pieces of
information on a case (like the address of a stalker victim).

The point being, we would want a structure something like (picking a
few of our case types):

Case
\_ ChargeableCase
\_ FelonyCase
\_ MisdemeanorCase
\_ CivilForfeitureCase
\_ JuvenileCase
\_ NonchargableCase
\_ CivilCase
\_ SmallClaimsCase
\_ ProbateCase
\_ MentalCommitmentCase

Just because most of these case types are a matter of public record
and subject to open records law disclosure requests (which we
largely avoid by putting what we can on the web site), juvenile and
mental commitment cases are confidential; unless you need to handle
something related to such a case to support its progress through the
courts, you're not supposed to see anything beyond such sketchy
information as the existence of the case number, a filing date, and
a caption where names are replaced by initials (e.g., "In the
interest of E.B.") -- and even that information is held back from
the web site because of possible "data mining" attacks.

Many of the features KaiGai has discussed would fit nicely with
court requirements -- and might even be prerequisites for
considering moving security to the database level. Mandating
identical security for all tables in a hierarchy would be a problem.
We'd want to be able to `grant select on "Case" to public` and then
`revoke select on "JuvenileCase", "MentalCommitmentCase" from
public` and have those cases disappear from selects against the
ancestor levels unless the user has the appropriate permission. Or
less convenient, but still feasible, would be to grant nothing at
the ancestor levels, and grant what is appropriate at each child
level and have that affect the results of a query against the
ancestor.

Of course, if one was really careful, this could all be done by
adding views with appropriate permissions and blocking access to the
underlying ancestor tables, but that seems like a lot more work and
rather more error prone.

-Kevin

* Kevin Grittner (Kevin(dot)Grittner(at)wicourts(dot)gov) wrote:
> Many of the features KaiGai has discussed would fit nicely with
> court requirements -- and might even be prerequisites for
> considering moving security to the database level. Mandating
> identical security for all tables in a hierarchy would be a problem.

What you're describing isn't how inheiritance used to work in PG anyway,
so it's not really like we've made things worse. What used to happen is
that if your query against the parent table happened to hit a table you
didn't have access to, it'd fail outright with a permissions error, not
just skip over the things you didn't have access to. That certainly
wasn't ideal.

I think what you're really looking for is RLS (Row-Level Security),
which I think we would want to implement independently of the
inheiritance system (though it'd have to work with it, of course).
That's certainly something that I think would be great to have in PG and
would ideally be something which would address both of your "sometimes
everything is public except rows which look like X" and "all of these
types are non-public" situations.

I don't believe it's something that could be addressed *only* by
inheiritance though, in any case.

Thanks,

Stephen

(2010/08/16 22:14), Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> The purpose of this restriction is to ensure an access control decision
>> using parent's label being also consistent on child tables.
>
> Robert and I understand the concern that you have. The answer, at least
> for now, is that we don't agree with you. PG doesn't consider child
> tables to be independent objects when they're being accessed through the
> parent. As such, they don't have their own permissions checking.
>
>> If we control accesses on child tables using child's label, no need to
>> restrict an identical label within an entire inheritance hierarchy.
>> But it needs to provide the original rte->requiredPerms of child tables.
>> Now it is cleared at expand_inherited_rtentry(), so we have no way to
>> control accesses on child tables using child's label. :(
>
> If you want to argue that we should care about the childs permissions,
> or do something different with regard to inheritance, then you need to
> make that argument for all of PG, not just try to do what you think is
> right in the security definer framework.
>
>> > From viewpoint of MAC, both of the following SQLs should be denied,
>> when accesses on parent_tbl is allowed, but child_tbl is denied.
>
> KaiGai, this is not a MAC vs. DAC difference. This is a question of
> "what is an object" and if a child table is really an independent object
> from a parent table. In PG, we've decided they're not. We should
> probably do more to make that clearer in PG, rather than have different
> parts of the system treat them differently.
>
Ahh, yes, the question is "what is an object", not a "MAC vs DAC".

Indeed, PG does not try to handle child table as an independent object
from a parent table. However, if so, it seems to me strange that we can
assign individual ownership and access privileges on child tables.

If we stand on the perspective that child tables are a part of the
parent table, isn't it necessary to keep same ownership and access
privileges between parent and children? It seems to me the current
implementation is in the halfway from the perspective of child
tables as independent object to the perspective of child tables as
a part of parent table.

If PG can keep consistency of ownership and access privileges between
parent and children, it is quite natural we keep consistency of labels,
isn't it?

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

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> Ahh, yes, the question is "what is an object", not a "MAC vs DAC".
>
> Indeed, PG does not try to handle child table as an independent object
> from a parent table. However, if so, it seems to me strange that we can
> assign individual ownership and access privileges on child tables.

I tend to agree. Perhaps we should bring up, in an independent thread,
the question of if that really makes sense or if we should do something
to prevent it (or at least issue a warning when we detect it).

> If we stand on the perspective that child tables are a part of the
> parent table, isn't it necessary to keep same ownership and access
> privileges between parent and children? It seems to me the current
> implementation is in the halfway from the perspective of child
> tables as independent object to the perspective of child tables as
> a part of parent table.

I tend to agree- PG isn't doing this as cleanly as it should.

> If PG can keep consistency of ownership and access privileges between
> parent and children, it is quite natural we keep consistency of labels,
> isn't it?

Yes, but that's something that should be dealt with in PG and not as
part of an external security infrastructure. For example, PG could just
force that the same label is applied to a child table when it's made a
child of a particular parent, perhaps with a check to the external
security system to see if there's a problem changing whatever label is
on the child table before it's changed to be that of the parent, but
once it's a child of the parent (if that operation was permitted and was
successful), it no longer has its own 'identity'.

Let's not build the complication of dealing with inheiritance/child
relations into the external security system when we're in the middle of
trying to get rid of that distinction in core PG though.

Thanks,

Stephen

(2010/08/17 9:51), Stephen Frost wrote:
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> Ahh, yes, the question is "what is an object", not a "MAC vs DAC".
>>
>> Indeed, PG does not try to handle child table as an independent object
>> from a parent table. However, if so, it seems to me strange that we can
>> assign individual ownership and access privileges on child tables.
>
> I tend to agree. Perhaps we should bring up, in an independent thread,
> the question of if that really makes sense or if we should do something
> to prevent it (or at least issue a warning when we detect it).
>
>> If we stand on the perspective that child tables are a part of the
>> parent table, isn't it necessary to keep same ownership and access
>> privileges between parent and children? It seems to me the current
>> implementation is in the halfway from the perspective of child
>> tables as independent object to the perspective of child tables as
>> a part of parent table.
>
> I tend to agree- PG isn't doing this as cleanly as it should.
>
>> If PG can keep consistency of ownership and access privileges between
>> parent and children, it is quite natural we keep consistency of labels,
>> isn't it?
>
> Yes, but that's something that should be dealt with in PG and not as
> part of an external security infrastructure. For example, PG could just
> force that the same label is applied to a child table when it's made a
> child of a particular parent, perhaps with a check to the external
> security system to see if there's a problem changing whatever label is
> on the child table before it's changed to be that of the parent, but
> once it's a child of the parent (if that operation was permitted and was
> successful), it no longer has its own 'identity'.
>
> Let's not build the complication of dealing with inheiritance/child
> relations into the external security system when we're in the middle of
> trying to get rid of that distinction in core PG though.
>
I also agree the idea that PG core handles the recursion of inheritance
hierarchy and enforce same label between them. The reason why I handled
it within the module is the core does not enforce same labels.

OK, I'll rid 'expected_parents' argument from the security hook on
relabeling. Right now, it is incomplete, but should be fixed up in
the future.

In addition, I'll also post a design proposal to keep consistency of
ownership and access privileges between parent and children.
Please also wait for a while.

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

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> Indeed, PG does not try to handle child table as an independent object
>> from a parent table. However, if so, it seems to me strange that we can
>> assign individual ownership and access privileges on child tables.

> I tend to agree. Perhaps we should bring up, in an independent thread,
> the question of if that really makes sense or if we should do something
> to prevent it (or at least issue a warning when we detect it).

The reason there is still some value in setting permissions state on a
child table is that that controls what happens when you address the
child table directly, rather than implicitly by querying its parent.

regards, tom lane

(2010/08/17 11:58), Tom Lane wrote:
> Stephen Frost<sfrost(at)snowman(dot)net> writes:
>> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>>> Indeed, PG does not try to handle child table as an independent object
>>> from a parent table. However, if so, it seems to me strange that we can
>>> assign individual ownership and access privileges on child tables.
>
>> I tend to agree. Perhaps we should bring up, in an independent thread,
>> the question of if that really makes sense or if we should do something
>> to prevent it (or at least issue a warning when we detect it).
>
> The reason there is still some value in setting permissions state on a
> child table is that that controls what happens when you address the
> child table directly, rather than implicitly by querying its parent.
>
However, isn't it strange if we stand on the perspective that child table
is a part of parent object? It means an object have multiple properties
depending on the context.
If we want to allow someone to reference a part of the table (= child table),
I think VIEW is more appropriate and flexible tool.

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

On tis, 2010-08-17 at 13:00 +0900, KaiGai Kohei wrote:
> However, isn't it strange if we stand on the perspective that child
> table is a part of parent object? It means an object have multiple
> properties depending on the context.

Such is the nature of inheritance, isn't it?

(2010/08/17 13:28), Peter Eisentraut wrote:
> On tis, 2010-08-17 at 13:00 +0900, KaiGai Kohei wrote:
>> However, isn't it strange if we stand on the perspective that child
>> table is a part of parent object? It means an object have multiple
>> properties depending on the context.
>
> Such is the nature of inheritance, isn't it?
>
Yep, it will return different set of user data depending on the table
queried, when we reference either parent or child table.
But it seems to me too stretch interpretation to apply this behavior
on metadata of the tables also.

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

>Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> Let's not build the complication of dealing with inheiritance/
> child relations into the external security system when we're in
> the middle of trying to get rid of that distinction in core PG
> though.

I didn't realize we were trying to do that. I know we're working on
making partitioning easier, but there hasn't been a decision to stop
supporting other uses of inheritance, has there?

-Kevin

* Kevin Grittner (Kevin(dot)Grittner(at)wicourts(dot)gov) wrote:
> >Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
> > Let's not build the complication of dealing with inheiritance/
> > child relations into the external security system when we're in
> > the middle of trying to get rid of that distinction in core PG
> > though.
>
> I didn't realize we were trying to do that. I know we're working on
> making partitioning easier, but there hasn't been a decision to stop
> supporting other uses of inheritance, has there?

No.. and I'm not sure we ever would. What we *have* done is removed
all permissions checking on child tables when a parent is being
queried..

Stephen

>Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> No.. and I'm not sure we ever would. What we *have* done is
> removed all permissions checking on child tables when a parent is
> being queried..

OK, that clarifies things. Thanks.

So, essentially that means that you need to set all ancestor levels
to something at least as strict as the intersection of all the
permissions on lower levels to avoid exposing something through an
ancestor which is restricted in a descendant. And you'd better
trust the owner of any table you extend, because they can bypass any
attempt to restrict access to the table you create which extends
theirs.

I hope those security implications are well documented.

-Kevin

On Tue, Aug 17, 2010 at 1:50 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> No..  and I'm not sure we ever would.  What we *have* done is removed
> all permissions checking on child tables when a parent is being
> queried..

Yeah. I'm not totally sure that is sensible for a MAC environment.
Heck, it's arguably incorrect (though perhaps quite convenient) in a
DAC environment. Anyway, I wonder if it would be sensible to try to
adjust the structure of the DAC permissions checks so enhanced
security providers can make their own decision about how to handle
this case.

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, Aug 17, 2010 at 1:50 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > No..  and I'm not sure we ever would.  What we *have* done is removed
> > all permissions checking on child tables when a parent is being
> > queried..
>
> Yeah. I'm not totally sure that is sensible for a MAC environment.
> Heck, it's arguably incorrect (though perhaps quite convenient) in a
> DAC environment. Anyway, I wonder if it would be sensible to try to
> adjust the structure of the DAC permissions checks so enhanced
> security providers can make their own decision about how to handle
> this case.

To be honest, I don't really like the way this is done at all. I'd
rather have it such that if and when a table is made a child of another
table, it should inherit the permissions of the parent and be kept that
way, or it should be completely independent (which is the situation we
used to have), or, last resort, we should complain when they don't
match.

Or we could just not error when we hit a child table that the caller
doesn't have access to (but also not return the records). The problem
is that we've got different users that want to use inheiritance for very
different purposes and we havn't got a way to address all of them. I do
worry that we're going to regret making the change to not check
permissions on child tables, but at the same time, any query which would
have been impacted by that would have failed, so that really begs the
question of "do people really use/want different permissions on child
tables than the parent?". I tend to think 'no', and would rather force
them and keep them the same, but maybe that's just me..

Thanks,

Stephen

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Aug 17, 2010 at 1:50 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> No.. and I'm not sure we ever would. What we *have* done is removed
>> all permissions checking on child tables when a parent is being
>> queried..

> Yeah. I'm not totally sure that is sensible for a MAC environment.
> Heck, it's arguably incorrect (though perhaps quite convenient) in a
> DAC environment.

IIRC, the reason we did it was that we decided the SQL spec requires it.
So there's not a lot of point in debating the issue, unless you can
convince us we misread the spec.

regards, tom lane

(2010/08/18 3:07), Robert Haas wrote:
> On Tue, Aug 17, 2010 at 1:50 PM, Stephen Frost<sfrost(at)snowman(dot)net> wrote:
>> No.. and I'm not sure we ever would. What we *have* done is removed
>> all permissions checking on child tables when a parent is being
>> queried..
>
> Yeah. I'm not totally sure that is sensible for a MAC environment.
> Heck, it's arguably incorrect (though perhaps quite convenient) in a
> DAC environment. Anyway, I wonder if it would be sensible to try to
> adjust the structure of the DAC permissions checks so enhanced
> security providers can make their own decision about how to handle
> this case.
>
As long as we handle child tables in consistent way, here is no matter
for a MAC environment also. As Stephen mentioned, the question was
"what is an object". So, I want child tables being either a part of
parent table or an independent object from its parent.
In the first case, child tables need to have same security properties
(ownership, access privileges, security labels) with its parent.
In the later case, we need to check permissions on child tables also
when we query on the parent, but it is an old perspective now.

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

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > Yeah. I'm not totally sure that is sensible for a MAC environment.
> > Heck, it's arguably incorrect (though perhaps quite convenient) in a
> > DAC environment.
>
> IIRC, the reason we did it was that we decided the SQL spec requires it.
> So there's not a lot of point in debating the issue, unless you can
> convince us we misread the spec.

I've not gone through the spec with regard to this (yet..), but I think
we need to consider the whole 'principle of least surprise' here,
regardless of what the spec says. For one thing, this isn't how older
versions of PG behaved and while I doubt anyone intended to rely on that
behavior, it makes me nervous that someone, somewhere, unintentionally
relies on it.

What I'm thinking of is something like a warning if the permissions on
the child don't match those of the parent when the relationship is
created, or maybe forcibly setting the permissions on the child (with a
NOTICE), so it's at least clear what is going on. Or perhaps, set the
permissions on the child only if it doesn't have permissions (with the
NOTICE), and issue a WARNING if the child already has permissions set.
Perhaps also a WARNING if someone changes the permissions on a child
after the relationship has been created too, but let it happen in case
someone really wants it..

I dunno. None of the above makes me feel very comfortable from a
security perspective because I'm concerned any of the above could too
easily be overlooked by someone upgrading. It also doesn't really
address the concern that, at some point, a child table could have
different permissions than a parent table. If we forcibly set the
permissions on the child, or ERROR'd if the permissions weren't either
the same or empty on the child, and then ERROR'd if someone tried to
change the child's permissions later, I'd be happier.

I don't really want to force people doing routine partition additions
to have to set the permissions on the child before adding it, but I
also don't want to have to figure out "are these two sets of permissions
identical", since that's not really trivial to determine. We do have
default permissions now though, so maybe requiring the permissions be
the same from the get-go is the right idea. Of course, if we change the
permissions on the child when the inheiritance relationship is created,
we'll need to update those perms every time the parents perms are
changed.

Just my 2c.

Thanks,

Stephen

(2010/08/18 9:04), Stephen Frost wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> Robert Haas<robertmhaas(at)gmail(dot)com> writes:
>>> Yeah. I'm not totally sure that is sensible for a MAC environment.
>>> Heck, it's arguably incorrect (though perhaps quite convenient) in a
>>> DAC environment.
>>
>> IIRC, the reason we did it was that we decided the SQL spec requires it.
>> So there's not a lot of point in debating the issue, unless you can
>> convince us we misread the spec.
>
> I've not gone through the spec with regard to this (yet..), but I think
> we need to consider the whole 'principle of least surprise' here,
> regardless of what the spec says. For one thing, this isn't how older
> versions of PG behaved and while I doubt anyone intended to rely on that
> behavior, it makes me nervous that someone, somewhere, unintentionally
> relies on it.
>
I believed that table inheritance is a unique feature in PostgreSQL.
Does the SQL spec really mention about whether a child table is an
independent table object, or not?
Or, are you talking about the behavior that parent's permission also
controls accesses on child tables? If so, all of us already agreed.

> What I'm thinking of is something like a warning if the permissions on
> the child don't match those of the parent when the relationship is
> created, or maybe forcibly setting the permissions on the child (with a
> NOTICE), so it's at least clear what is going on. Or perhaps, set the
> permissions on the child only if it doesn't have permissions (with the
> NOTICE), and issue a WARNING if the child already has permissions set.
> Perhaps also a WARNING if someone changes the permissions on a child
> after the relationship has been created too, but let it happen in case
> someone really wants it..
>
> I dunno. None of the above makes me feel very comfortable from a
> security perspective because I'm concerned any of the above could too
> easily be overlooked by someone upgrading. It also doesn't really
> address the concern that, at some point, a child table could have
> different permissions than a parent table. If we forcibly set the
> permissions on the child, or ERROR'd if the permissions weren't either
> the same or empty on the child, and then ERROR'd if someone tried to
> change the child's permissions later, I'd be happier.
>
I also think ERROR should be raised when user tries to assign different
security properties on child tables from its parent. At least, I think
it should be configurable using a guc variable.
If WARNING/NOTICE, we can easily break consistency of the permissions...

> I don't really want to force people doing routine partition additions
> to have to set the permissions on the child before adding it, but I
> also don't want to have to figure out "are these two sets of permissions
> identical", since that's not really trivial to determine. We do have
> default permissions now though, so maybe requiring the permissions be
> the same from the get-go is the right idea. Of course, if we change the
> permissions on the child when the inheiritance relationship is created,
> we'll need to update those perms every time the parents perms are
> changed.
>
I also think it is a good idea to copy permissions from the parent when
we try to define an inheritance relationship. It obviously reduces user's
routine task on defining many of child tables. It seems to me a case when
we provide a NOTICE to users, if permissions of child table is overwritten.

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

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> I believed that table inheritance is a unique feature in PostgreSQL.

It's actually not..

> Does the SQL spec really mention about whether a child table is an
> independent table object, or not?

The SQL spec does discuss 'subtables' and inheiritance. It also does
describe some information under 'Access Rules' regarding these
sub-tables (check the 'table definition' clause). I've been looking at
them and trying to make some sense out of what I see.

Thanks,

Stephen

2010/8/17 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> I dunno.  None of the above makes me feel very comfortable from a
>> security perspective because I'm concerned any of the above could too
>> easily be overlooked by someone upgrading.  It also doesn't really
>> address the concern that, at some point, a child table could have
>> different permissions than a parent table.  If we forcibly set the
>> permissions on the child, or ERROR'd if the permissions weren't either
>> the same or empty on the child, and then ERROR'd if someone tried to
>> change the child's permissions later, I'd be happier.
>>
> I also think ERROR should be raised when user tries to assign different
> security properties on child tables from its parent. At least, I think
> it should be configurable using a guc variable.

If C1, C2, and C3 inherit from P, it's perfectly reasonable to grant
permissions to X on C1 and C2, Y on C3, and Z on C1, C2, C3, and P. I
don't think we should disallow that. Sure, it's possible to do things
that are less sane, but if we put ourselves in the business of
removing useful functionality because it might be misused, we'll put
ourselves out of business.

Having said that, I'm not sure that the same arguments really hold
water in the world of label based security. Suppose we have
compartmentalized security: P is a table of threats, with C1
containing data on nukes, C2 containing data on terrorists, and C3
containing data on foreign militaries. If we create a label for each
of these threat types, we can apply that label to the corresponding
table; but what label shall we assign P? Logically, the label for P
should be set up in such a fashion that the only people who can read P
are those who can read C1, C2, and C3 anyway, but who is to say that
such a label exists? Even if KaiGai's intended implementation of
SE-PostgreSQL supports construction of such a label, who is to say
that EVERY conceivable labeling system will also do so? In fact, it
seems to me that it might be far more reasonable, in a case like this,
to ignore the *parent* label and look only at each *child* label,
which to me is an argument that we should set this up so as to allow
individual users of this hook to do as they like.

It's also worth pointing out that the hook in ExecCheckRTPerms() does
not presuppose label-based security. It could be used to implement
some other policy altogether, which only strengthens the argument that
we can't know how the user of the hook wants to handle these cases.

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

(2010/08/18 12:02), Robert Haas wrote:
> 2010/8/17 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> I dunno. None of the above makes me feel very comfortable from a
>>> security perspective because I'm concerned any of the above could too
>>> easily be overlooked by someone upgrading. It also doesn't really
>>> address the concern that, at some point, a child table could have
>>> different permissions than a parent table. If we forcibly set the
>>> permissions on the child, or ERROR'd if the permissions weren't either
>>> the same or empty on the child, and then ERROR'd if someone tried to
>>> change the child's permissions later, I'd be happier.
>>>
>> I also think ERROR should be raised when user tries to assign different
>> security properties on child tables from its parent. At least, I think
>> it should be configurable using a guc variable.
>
> If C1, C2, and C3 inherit from P, it's perfectly reasonable to grant
> permissions to X on C1 and C2, Y on C3, and Z on C1, C2, C3, and P. I
> don't think we should disallow that. Sure, it's possible to do things
> that are less sane, but if we put ourselves in the business of
> removing useful functionality because it might be misused, we'll put
> ourselves out of business.
>
Hmm. If C1, C2 and C3 are defined to store information for different
categories, but shares common data structure, indeed, it is useful.

> Having said that, I'm not sure that the same arguments really hold
> water in the world of label based security. Suppose we have
> compartmentalized security: P is a table of threats, with C1
> containing data on nukes, C2 containing data on terrorists, and C3
> containing data on foreign militaries. If we create a label for each
> of these threat types, we can apply that label to the corresponding
> table; but what label shall we assign P? Logically, the label for P
> should be set up in such a fashion that the only people who can read P
> are those who can read C1, C2, and C3 anyway, but who is to say that
> such a label exists?

Right. If access privileges on P implicitly allow accesses on children,
the P must have a label that only allows people who can access both of
the children. However, in SELinux at least, here is no guarantee that
we can always find out such a label in the security policy installed. :(

> Even if KaiGai's intended implementation of
> SE-PostgreSQL supports construction of such a label, who is to say
> that EVERY conceivable labeling system will also do so? In fact, it
> seems to me that it might be far more reasonable, in a case like this,
> to ignore the *parent* label and look only at each *child* label,
> which to me is an argument that we should set this up so as to allow
> individual users of this hook to do as they like.
>
It will be helpful. If we can check each children's label, no need
to restrict an identical security label within a certain inheritance
hierarchy. Of course, other security module may check permissions
in different basic, but it seems to me characteristics.

> It's also worth pointing out that the hook in ExecCheckRTPerms() does
> not presuppose label-based security. It could be used to implement
> some other policy altogether, which only strengthens the argument that
> we can't know how the user of the hook wants to handle these cases.
>
If rte->requiredPerms would not be cleared, the user of the hook will
be able to check access rights on the child tables, as they like.
How about an idea to add a new flag in RangeTblEntry which shows where
the RangeTblEntry came from, instead of clearing requiredPerms?
If the flag is true, I think ExecCheckRTEPerms() can simply skip checks
on the child tables.

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

2010/8/18 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> It's also worth pointing out that the hook in ExecCheckRTPerms() does
>> not presuppose label-based security.  It could be used to implement
>> some other policy altogether, which only strengthens the argument that
>> we can't know how the user of the hook wants to handle these cases.
>>
> If rte->requiredPerms would not be cleared, the user of the hook will
> be able to check access rights on the child tables, as they like.
> How about an idea to add a new flag in RangeTblEntry which shows where
> the RangeTblEntry came from, instead of clearing requiredPerms?
> If the flag is true, I think ExecCheckRTEPerms() can simply skip checks
> on the child tables.

Something along those lines might work, although I haven't yet
scrutinized the code well enough to have a real clear opinion on what
the best way of dealing with this is.

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

Robert,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> If C1, C2, and C3 inherit from P, it's perfectly reasonable to grant
> permissions to X on C1 and C2, Y on C3, and Z on C1, C2, C3, and P. I
> don't think we should disallow that. Sure, it's possible to do things
> that are less sane, but if we put ourselves in the business of
> removing useful functionality because it might be misused, we'll put
> ourselves out of business.
>
> Having said that, I'm not sure that the same arguments really hold
> water in the world of label based security. Suppose we have
> compartmentalized security: P is a table of threats, with C1
> containing data on nukes, C2 containing data on terrorists, and C3
> containing data on foreign militaries. If we create a label for each
> of these threat types, we can apply that label to the corresponding
> table; but what label shall we assign P? Logically, the label for P
> should be set up in such a fashion that the only people who can read P
> are those who can read C1, C2, and C3 anyway, but who is to say that
> such a label exists? Even if KaiGai's intended implementation of
> SE-PostgreSQL supports construction of such a label, who is to say
> that EVERY conceivable labeling system will also do so?

I don't see why using labels in the second case changes anything.
Consider roles. If you only had a role that could see threats, a role
that could see nukes, and a role that could see terrorists, but no role
that could see all of them, it's the same problem. Additionally, this
kind of problem *isn't* typically addressed with the semantics or the
structure of inheiritance- it's done with row-level security and is
completely orthogonal to the inheiritance issue.

Imagine a new table, C4, is added to P and the admin configures it such
that only the 'view_c4' role has access to that child table directly.
Now, Z can see what's in C4 through P, even though Z doesn't have access
to C4. In the old system, if Z's query happened to hit C4, the whole
query would fail but at least Z wouldn't see any C4 data. Other queries
on P done by Z would be fine, so long as they didn't hit C4.

> In fact, it
> seems to me that it might be far more reasonable, in a case like this,
> to ignore the *parent* label and look only at each *child* label,
> which to me is an argument that we should set this up so as to allow
> individual users of this hook to do as they like.

I think it'd be more reasonable to do this for inheiritance in general,
but the problem is that people use it for partitioning, and there is a
claim out there that it's against what the SQL spec says. The folks
using inheiritance for partitioning would probably prefer to not have to
deal with setting up the permissions on the child tables. I think
that's less of an issue now, but I didn't like the previous behavior
where certain queries would work and certain queries wouldn't work
against the parent table, either.

> It's also worth pointing out that the hook in ExecCheckRTPerms() does
> not presuppose label-based security. It could be used to implement
> some other policy altogether, which only strengthens the argument that
> we can't know how the user of the hook wants to handle these cases.

This comes back around, in my view, to the distinction between really
using inheiritance for inheiritance, vs using it for partitioning. If
it's used for partitioning (which certainly seems to be the vast
majority of the cases I've seen it used) then I think it should really
be considered and viewed as a single object to the authentication
system. I don't suppose we're going to get rid of inheiritance for
inheiritance any time soon though.

In the end, I'm thinking that if the external security module wants to
enforce a check against all the children of a parent, they could quite
possibly handle that already and do it in such a way that it won't break
depending on the specific query. To wit, it could query the catalog to
determine if the current table is a parent of any children, and if so,
go check the labels/permissions/etc on those children. I'd much rather
have something where the permissions check either succeeds or fails
against the parent, depending on the permissions of the parent and its
children, than on what the query is itself and what conditionals are
applied to it.

Thanks,

Stephen

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> If rte->requiredPerms would not be cleared, the user of the hook will
> be able to check access rights on the child tables, as they like.

This would only be the case for those children which are being touched
in the current query, which would depend on what conditionals are
applied, what the current setting of check_constraints is, and possibly
other factors. I do *not* like this approach.

> How about an idea to add a new flag in RangeTblEntry which shows where
> the RangeTblEntry came from, instead of clearing requiredPerms?
> If the flag is true, I think ExecCheckRTEPerms() can simply skip checks
> on the child tables.

How about the external module just checks if the current object being
queried has parents, and if so, goes and checks the
labels/permissions/etc on those children? That way the query either
always fails or never fails for a given caller, rather than sometimes
working and sometimes not depending on the query.

Thanks,

Stephen

On Wed, Aug 18, 2010 at 8:49 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> In the end, I'm thinking that if the external security module wants to
> enforce a check against all the children of a parent, they could quite
> possibly handle that already and do it in such a way that it won't break
> depending on the specific query.  To wit, it could query the catalog to
> determine if the current table is a parent of any children, and if so,
> go check the labels/permissions/etc on those children.  I'd much rather
> have something where the permissions check either succeeds or fails
> against the parent, depending on the permissions of the parent and its
> children, than on what the query is itself and what conditionals are
> applied to it.

Interesting idea. Again, I haven't read the code, but seems worth
further investigation, at least.

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

(2010/08/18 21:52), Stephen Frost wrote:
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> If rte->requiredPerms would not be cleared, the user of the hook will
>> be able to check access rights on the child tables, as they like.
>
> This would only be the case for those children which are being touched
> in the current query, which would depend on what conditionals are
> applied, what the current setting of check_constraints is, and possibly
> other factors. I do *not* like this approach.
>
Indeed, the planner might omit scan on the children which are not obviously
referenced, but I'm not certain whether its RangeTblEntry would be also
removed from the PlannedStmt->rtable, or not.

>> How about an idea to add a new flag in RangeTblEntry which shows where
>> the RangeTblEntry came from, instead of clearing requiredPerms?
>> If the flag is true, I think ExecCheckRTEPerms() can simply skip checks
>> on the child tables.
>
> How about the external module just checks if the current object being
> queried has parents, and if so, goes and checks the
> labels/permissions/etc on those children? That way the query either
> always fails or never fails for a given caller, rather than sometimes
> working and sometimes not depending on the query.
>
Hmm, this idea may be feasible. The RangeTblEntry->inh flag of the parent
will give us a hint whether we also should check labels on its children.

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

>>> How about an idea to add a new flag in RangeTblEntry which shows where
>>> the RangeTblEntry came from, instead of clearing requiredPerms?
>>> If the flag is true, I think ExecCheckRTEPerms() can simply skip checks
>>> on the child tables.
>>
>> How about the external module just checks if the current object being
>> queried has parents, and if so, goes and checks the
>> labels/permissions/etc on those children? That way the query either
>> always fails or never fails for a given caller, rather than sometimes
>> working and sometimes not depending on the query.
>>
> Hmm, this idea may be feasible. The RangeTblEntry->inh flag of the parent
> will give us a hint whether we also should check labels on its children.
>

http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/relation.c#293

At least, it seems to me this logic works as expected.

postgres=# CREATE TABLE tbl_p (a int, b text);
CREATE TABLE
postgres=# CREATE TABLE tbl_1 (check (a < 100)) inherits (tbl_p);
CREATE TABLE
postgres=# CREATE TABLE tbl_2 (check (a >= 100 and a < 200)) inherits (tbl_p);
CREATE TABLE
postgres=# CREATE TABLE tbl_3 (check (a >= 300)) inherits (tbl_p);
CREATE TABLE
postgres=# SECURITY LABEL on TABLE tbl_p IS 'system_u:object_r:sepgsql_table_t:s0';
SECURITY LABEL
postgres=# SECURITY LABEL on COLUMN tbl_p.a IS 'system_u:object_r:sepgsql_table_t:s0';
SECURITY LABEL
postgres=# SECURITY LABEL on COLUMN tbl_p.b IS 'system_u:object_r:sepgsql_table_t:s0';
SECURITY LABEL

postgres=# set sepgsql_debug_audit = on;
SET

postgres=# SELECT a FROM ONLY tbl_p WHERE a = 150;
LOG: SELinux: allowed { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_table name=tbl_p
STATEMENT: SELECT a FROM ONLY tbl_p WHERE a = 150;
LOG: SELinux: allowed { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_column name=tbl_p.a
STATEMENT: SELECT a FROM ONLY tbl_p WHERE a = 150;
a
---
(0 rows)

-> ONLY tbl_p was not expanded

postgres=# SELECT a FROM tbl_p WHERE a = 150;
LOG: SELinux: allowed { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_table name=tbl_p
STATEMENT: SELECT a FROM tbl_p WHERE a = 150;
LOG: SELinux: allowed { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_column name=tbl_p.a
STATEMENT: SELECT a FROM tbl_p WHERE a = 150;
LOG: SELinux: allowed { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_table name=tbl_1
STATEMENT: SELECT a FROM tbl_p WHERE a = 150;
LOG: SELinux: allowed { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_column name=tbl_1.a
STATEMENT: SELECT a FROM tbl_p WHERE a = 150;
LOG: SELinux: allowed { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_table name=tbl_2
STATEMENT: SELECT a FROM tbl_p WHERE a = 150;
LOG: SELinux: allowed { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_column name=tbl_2.a
STATEMENT: SELECT a FROM tbl_p WHERE a = 150;
LOG: SELinux: allowed { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_table name=tbl_3
STATEMENT: SELECT a FROM tbl_p WHERE a = 150;
LOG: SELinux: allowed { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_column name=tbl_3.a
STATEMENT: SELECT a FROM tbl_p WHERE a = 150;
a
---
(0 rows)

-> tbl_p was expanded to tbl_1, tbl_2 and tbl_3

postgres=# set sepgsql_debug_audit = off;
SET
postgres=# EXPLAIN SELECT a FROM tbl_p WHERE a = 150;
QUERY PLAN
------------------------------------------------------------------------
Result (cost=0.00..50.75 rows=12 width=4)
-> Append (cost=0.00..50.75 rows=12 width=4)
-> Seq Scan on tbl_p (cost=0.00..25.38 rows=6 width=4)
Filter: (a = 150)
-> Seq Scan on tbl_2 tbl_p (cost=0.00..25.38 rows=6 width=4)
Filter: (a = 150)
(6 rows)

-> Actually, it does not scan tbl_1 and tbl_3 due to the a = 150.

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

On tis, 2010-08-17 at 20:04 -0400, Stephen Frost wrote:
> What I'm thinking of is something like a warning if the permissions on
> the child don't match those of the parent when the relationship is
> created, or maybe forcibly setting the permissions on the child (with
> a
> NOTICE), so it's at least clear what is going on. Or perhaps, set the
> permissions on the child only if it doesn't have permissions (with the
> NOTICE), and issue a WARNING if the child already has permissions set.
> Perhaps also a WARNING if someone changes the permissions on a child
> after the relationship has been created too, but let it happen in case
> someone really wants it..

I think there are perfectly good reasons to have different permissions
on parent and child tables. I don't see any reason to monkey around
with that.

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> I think there are perfectly good reasons to have different permissions
> on parent and child tables. I don't see any reason to monkey around
> with that.

Even though the permissions on the child table aren't invovled at all if
queried through the parent..? The parent implicitly adds to the set of
privileges which are granted on the child, but that's not clear at all
from the permissions visible on the child. That's principally what I'm
complaining about here.

Thanks,

Stephen

On sön, 2010-08-22 at 15:08 -0400, Stephen Frost wrote:
> * Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> > I think there are perfectly good reasons to have different permissions
> > on parent and child tables. I don't see any reason to monkey around
> > with that.
>
> Even though the permissions on the child table aren't invovled at all if
> queried through the parent..? The parent implicitly adds to the set of
> privileges which are granted on the child, but that's not clear at all
> from the permissions visible on the child. That's principally what I'm
> complaining about here.

Perhaps this is a user interface issue then. Maybe the fact that a
table is inherited from another one needs to be shown closer to
whereever the permissions are listed.

* Peter Eisentraut (peter_e(at)gmx(dot)net) wrote:
> On sön, 2010-08-22 at 15:08 -0400, Stephen Frost wrote:
> > Even though the permissions on the child table aren't invovled at all if
> > queried through the parent..? The parent implicitly adds to the set of
> > privileges which are granted on the child, but that's not clear at all
> > from the permissions visible on the child. That's principally what I'm
> > complaining about here.
>
> Perhaps this is a user interface issue then. Maybe the fact that a
> table is inherited from another one needs to be shown closer to
> whereever the permissions are listed.

That's a nice idea, except that we've got a pretty well defined API
regarding how to determine what the privileges on a table are, and many
different UIs which use it. Fixing it in psql (if it needs to be..
iirc, \d or \d+ may already show it) doesn't really address the problem,
in my view.

Thanks,

Stephen

>>>> 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,
>

Now I'm under describing the new chapter.
http://git.postgresql.org/gitweb?p=users/kaigai/sepgsql.git;a=blob;f=doc/src/sgml/esp.sgml;hb=devel/seclabel

However, I'm wondering whether the topic about security hooks and some
others are appropriate for the "III. Server Administration" part.

Perhaps, it is a good idea a new section at the last of "Database Roles
and Privileges" which introduce a fact that PostgreSQL allows plugins
to make access control decision, and a new chapter in the "VII. Internals"
part.

How about the idea?

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

2010/8/25 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>>>> 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,
>>
>
> Now I'm under describing the new chapter.
> http://git.postgresql.org/gitweb?p=users/kaigai/sepgsql.git;a=blob;f=doc/src/sgml/esp.sgml;hb=devel/seclabel
>
> However, I'm wondering whether the topic about security hooks and some
> others are appropriate for the "III. Server Administration" part.
>
> Perhaps, it is a good idea a new section at the last of "Database Roles
> and Privileges" which introduce a fact that PostgreSQL allows plugins
> to make access control decision, and a new chapter in the "VII. Internals"
> part.
>
> How about the idea?

Well, I prefer what I suggested, but of course I'm biased.

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

The attached patch is a revised version of security label support.

summary of changes:
* The logic to translate object-name to object-id was rewritten
with the new get_object_address().

* Right now, it does not support labeling on shared database object
(ie; pg_database), so wrapper functions to XXXLocalSecLabel() were
removed.

* The restriction of same security label within whole of inheritance
tree has gone. And, the 'num_parents' argument was also removed
from the security hook.

* ExecRelationSecLabel() was also removed, although you suggested
to rename it, because it translate the supplied relation name
into relation id and handled child tables, but it get unnecessary.

* The chapter of 'External Security Provider' was added.
It introduces overview of ESP concept and MAC features.
Perhaps, other structures of chapters are more preferable,
but I also think we need a draft at the begining of discussion.

* The '--security-label' option was added to pg_dump/pg_dumpall;
it allows to include security label of objects in the archives.
The '--no-security-label' option was also added to pg_restore;
it allows to skip security labels, even if the archive contains
security labels.

Thanks,

(2010/08/10 5:02), Robert Haas wrote:
> 2010/7/26 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)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.
>

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

Robert, although you suggested that only one ESP module shall be
invoked on relabeling an object before, and I agreed this design
at that time, but I noticed that we cannot implement the following
behavior correctly.

SELinux defines these permissions corresponding to table relabeling.
- db_table:{setattr}
It is necessary to change *any* properties of the table.
Security label is one of properties of the table, so, needs to be
checked on relabeling, not only ALTER TABLE and so on.
- db_table:{relabelfrom relabelto}
It is neccesary to change label of the table.
User must have {relabelfrom} permission on the older security label,
and {relabelto} permission on the new security label.

If and when multiple ESP modules are installed, we need to consider
the way to handle SECURITY LABEL statement for other modules.
When user tries to relabel security label of a table managed by
something except for selinux, it is not relevant to {relabelfrom
relabelto} permission in selinux, but we want to check {setattr}
permission on the operation, because it is a property of the table,
although not a security label in selinux.

In the current patch, the core PG (ExecSecurityLabel()) invokes only
one hook that matches with the given "FOR <esp>" option.
However, I reconsidered this hook should be simply invoked like other
hooks. Then, the ESP module decides whether ignores or handles the
invocation, and also decides to call secondary hook when multiple
modules are loaded. If so, selinux module can check {setattr} and
also calls secondary modules.

In the previous discussion, I missed the possibility of the case
when we want to check relabeling a security label managed by other
ESP. But it might be also necessary to call all the modules which
want to get control on SECURITY LABEL statement, apart from whether
it manages the supplied security label, or not.

Thanks,

(2010/08/31 15:27), KaiGai Kohei wrote:
> The attached patch is a revised version of security label support.
>
> summary of changes:
> * The logic to translate object-name to object-id was rewritten
> with the new get_object_address().
>
> * Right now, it does not support labeling on shared database object
> (ie; pg_database), so wrapper functions to XXXLocalSecLabel() were
> removed.
>
> * The restriction of same security label within whole of inheritance
> tree has gone. And, the 'num_parents' argument was also removed
> from the security hook.
>
> * ExecRelationSecLabel() was also removed, although you suggested
> to rename it, because it translate the supplied relation name
> into relation id and handled child tables, but it get unnecessary.
>
> * The chapter of 'External Security Provider' was added.
> It introduces overview of ESP concept and MAC features.
> Perhaps, other structures of chapters are more preferable,
> but I also think we need a draft at the begining of discussion.
>
> * The '--security-label' option was added to pg_dump/pg_dumpall;
> it allows to include security label of objects in the archives.
> The '--no-security-label' option was also added to pg_restore;
> it allows to skip security labels, even if the archive contains
> security labels.
>
> Thanks,
>
> (2010/08/10 5:02), Robert Haas wrote:
>> 2010/7/26 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)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.
>>
>
>
>
>
>

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

2010/9/13 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> Robert, although you suggested that only one ESP module shall be
> invoked on relabeling an object before, and I agreed this design
> at that time, but I noticed that we cannot implement the following
> behavior correctly.
>
> SELinux defines these permissions corresponding to table relabeling.
> - db_table:{setattr}
>  It is necessary to change *any* properties of the table.
>  Security label is one of properties of the table, so, needs to be
>  checked on relabeling, not only ALTER TABLE and so on.
> - db_table:{relabelfrom relabelto}
>  It is neccesary to change label of the table.
>  User must have {relabelfrom} permission on the older security label,
>  and {relabelto} permission on the new security label.
>
> If and when multiple ESP modules are installed, we need to consider
> the way to handle SECURITY LABEL statement for other modules.
> When user tries to relabel security label of a table managed by
> something except for selinux, it is not relevant to {relabelfrom
> relabelto} permission in selinux, but we want to check {setattr}
> permission on the operation, because it is a property of the table,
> although not a security label in selinux.
>
> In the current patch, the core PG (ExecSecurityLabel()) invokes only
> one hook that matches with the given "FOR <esp>" option.
> However, I reconsidered this hook should be simply invoked like other
> hooks. Then, the ESP module decides whether ignores or handles the
> invocation, and also decides to call secondary hook when multiple
> modules are loaded. If so, selinux module can check {setattr} and
> also calls secondary modules.
>
> In the previous discussion, I missed the possibility of the case
> when we want to check relabeling a security label managed by other
> ESP. But it might be also necessary to call all the modules which
> want to get control on SECURITY LABEL statement, apart from whether
> it manages the supplied security label, or not.

Maybe. The whole point of MAC is that the security policy itself is
capable of enforcing all of the necessary protections. It shouldn't
be necessary for it to control what DAC or other MAC providers do,
should it? At any rate, they'll probably treat it quite a bit
differently than a change of their own label. I think it might be
cleaner to fold that in under some of the DDL permissions checking and
refactoring which we know still needs to be done, rather than cramming
it in here. Note that presumably COMMENT ON would need similar
treatment, and there may be other things.

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

(2010/09/13 20:46), Robert Haas wrote:
> 2010/9/13 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> Robert, although you suggested that only one ESP module shall be
>> invoked on relabeling an object before, and I agreed this design
>> at that time, but I noticed that we cannot implement the following
>> behavior correctly.
>>
>> SELinux defines these permissions corresponding to table relabeling.
>> - db_table:{setattr}
>> It is necessary to change *any* properties of the table.
>> Security label is one of properties of the table, so, needs to be
>> checked on relabeling, not only ALTER TABLE and so on.
>> - db_table:{relabelfrom relabelto}
>> It is neccesary to change label of the table.
>> User must have {relabelfrom} permission on the older security label,
>> and {relabelto} permission on the new security label.
>>
>> If and when multiple ESP modules are installed, we need to consider
>> the way to handle SECURITY LABEL statement for other modules.
>> When user tries to relabel security label of a table managed by
>> something except for selinux, it is not relevant to {relabelfrom
>> relabelto} permission in selinux, but we want to check {setattr}
>> permission on the operation, because it is a property of the table,
>> although not a security label in selinux.
>>
>> In the current patch, the core PG (ExecSecurityLabel()) invokes only
>> one hook that matches with the given "FOR<esp>" option.
>> However, I reconsidered this hook should be simply invoked like other
>> hooks. Then, the ESP module decides whether ignores or handles the
>> invocation, and also decides to call secondary hook when multiple
>> modules are loaded. If so, selinux module can check {setattr} and
>> also calls secondary modules.
>>
>> In the previous discussion, I missed the possibility of the case
>> when we want to check relabeling a security label managed by other
>> ESP. But it might be also necessary to call all the modules which
>> want to get control on SECURITY LABEL statement, apart from whether
>> it manages the supplied security label, or not.
>
> Maybe. The whole point of MAC is that the security policy itself is
> capable of enforcing all of the necessary protections. It shouldn't
> be necessary for it to control what DAC or other MAC providers do,
> should it?

Yes, what we should do here is that DAC and any MACs make their own
decision individually, then PG eventually prevents user's request if
one of them denied at least.

> At any rate, they'll probably treat it quite a bit
> differently than a change of their own label. I think it might be
> cleaner to fold that in under some of the DDL permissions checking and
> refactoring which we know still needs to be done, rather than cramming
> it in here.

Yes, if and when MAC-X and MAC-Y are installed, it is significant event
for MAC-X to change X's label, so MAC-X may need to check special
permissions. But it is a common event for MAC-Y and DAC, so they checks
an appropriate permission to change one of the properties. Hoever, it
does not mean we should not give any chance MAC-Y and DAC to check something.

I'll revise my patch within a couple of days.

Thanks,

> Note that presumably COMMENT ON would need similar
> treatment, and there may be other things.
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Mon, Sep 13, 2010 at 8:38 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> Yes, if and when MAC-X and MAC-Y are installed, it is significant event
> for MAC-X to change X's label, so MAC-X may need to check special
> permissions. But it is a common event for MAC-Y and DAC, so they checks
> an appropriate permission to change one of the properties. Hoever, it
> does not mean we should not give any chance MAC-Y and DAC to check
> something.
>
> I'll revise my patch within a couple of days.

I have a feeling we are talking past each other.

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

(2010/09/13 21:57), Robert Haas wrote:
> On Mon, Sep 13, 2010 at 8:38 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> Yes, if and when MAC-X and MAC-Y are installed, it is significant event
>> for MAC-X to change X's label, so MAC-X may need to check special
>> permissions. But it is a common event for MAC-Y and DAC, so they checks
>> an appropriate permission to change one of the properties. Hoever, it
>> does not mean we should not give any chance MAC-Y and DAC to check
>> something.
>>
>> I'll revise my patch within a couple of days.
>
> I have a feeling we are talking past each other.
>
Perhaps, we might discuss about this topic before, but it's unclear for me.

The attached patch is a revised version, but a bit difference from what
I introduced yesterday.

The commands/seclabel.c still keeps the list of a pair of esp tag and
its security hook on relabeling, but it was modified to invoke all the
registered hooks with/without the supplied security label.

The guest of the hook has the following prototype:

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

When user tries to change the security label owned by other ESP,
the hook shall be invoked with NULL as the 'seclabel' argument,
because it does not need to know the new label itself.
(Perhaps, a flag as 3rd argument is more preferable.)

If we would implement it as a simple hook chain, like other existing
hooks, it is not easy to put the logic that allows to omit FOR clause
when only one ESP is install, on the core PG routine, because here is
no way to count number of installed ESPs. :-(

Code example of ESP module at:
http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/label.c#214

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

2010/9/14 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> The attached patch is a revised version, but a bit difference from what
> I introduced yesterday.

I am working through this patch and fixing a variety of things things
that seem to need fixing. Please hang tight and don't send any new
versions for now.

Thanks,

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

On Thu, Sep 16, 2010 at 11:12 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 2010/9/14 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> The attached patch is a revised version, but a bit difference from what
>> I introduced yesterday.
>
> I am working through this patch and fixing a variety of things things
> that seem to need fixing.  Please hang tight and don't send any new
> versions for now.

There's no particularly good way to say this, so I'm just going to
spit it out: this patch was a real mess. In particular, there are a
huge number of cases where the identifier names were poorly chosen,
which I have mostly gone through and fixed now. There may yet be some
arguable and/or wrong cases remaining, and it's certainly possible
that not everyone may agree with all the choices I made, but it's
certainly a lot better than it was. I also had to rewrite pretty much
all of the documentation, comments, and error messages. I reorganized
a fair amount of the code, too; and ripped out a bunch of stuff that
looked irrelevant. In theory, this was supposed to be patterned off
the COMMENT code, but there were various changes which mostly did not
seem like improvements to me, and which in at least one case were
plain wrong.

Most of the contents of the new documentation section on external
security providers seemed irrelevant to me, which I guess I can only
blame myself for since I was the one who asked for that section to be
created, and I didn't specify what it should contain all that well. I
took a try at rewriting it to be more on-topic, but it didn't amount
to much so I ended up just ripping that part out completely.

For a couple of reasons, I decided that it made sense to broaden the
set of objects to which the SECURITY LABEL command can apply. My
meeting with the NSA folks at BWPUG more or less convinced me that
we're not going to get very far with this unless we have suitable
hooks for additional permissions-checking when functions are executed
or large objects are accessed, so I added labels for those, as well as
for types, schemas, and procedural languages. It is possible that we
need more than that, but supporting all of these rather than just
relations and attributes requires only fairly trivial code changes,
and I'd like to have at least a month or two go by before I have to
look at another patch in this area. It's worth noting that labels on
schemas can be useful even if we don't have a hook for schema-related
permissions checking, once we have hooks to set labels at object
creation time: the label for a newly assigned table can be a function
of the user's label and the schema's label.

I removed the crock that let one label provider veto another label
provider's label. I understand that MAC will require a control there,
but (as I said before) that's not the right way to do it. Let's leave
that as material for a separate patch that solves the whole problem
well instead of 5% of it poorly.

I think the backend code here is now in pretty good shape, but there
are still a number of things that need to be fixed. The pg_dump
support is broken at the moment, because of the change to the set of
objects that can be labeled. I also don't think it's right to dump
security labels only when asked to do so. I think that the option
should be --no-security-label in pg_dump(all) just as it is in
pg_restore. Also, the pg_dump support for security labels should
really reuse the existing design for comments, rather than inventing a
new and less efficient method, unless there is some really compelling
reason why the method used for comments won't work. Please send a
reworked patch for just this directory (src/bin/pg_dump).

There are a few other problems. First, there's no psql support of any
kind. Now, this is kind of a corner-case feature: so maybe we don't
really need it. And as I mentioned on another thread, there aren't a
lot of good letters left for backslash-d commands. So I'd be just as
happy to add a system view along the lines I previously proposed for
comments and call it good. Alternatively, or in addition, we could
add a \d command after all. The best way forward is debatable, but we
certainly need *something*, because interpreting the pg_seclabel
catalog by hand is not for the faint of heart. Second, there are no
regression tests. It's a bit tricky to think about how to crack that
nut because this feature is somewhat unusual in that it can't be used
without loading an appropriate loadable module. I'm wondering if we
can ship a "dummy_seclabel" contrib module that can be loaded during
the regression test run and then run various tests using that, but I'm
not quite sure what the best way to set that up is. SECURITY LABEL is
a core feature, so it would be nice to test it in the core regression
tests... but maybe that's too complicated to get working, and we
should just test it from the contrib module.

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

Robert,

First off, thanks alot for working on this. My apologies for not having
time to help out. A few minor comments:

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> Most of the contents of the new documentation section on external
> security providers seemed irrelevant to me, which I guess I can only
> blame myself for since I was the one who asked for that section to be
> created, and I didn't specify what it should contain all that well. I
> took a try at rewriting it to be more on-topic, but it didn't amount
> to much so I ended up just ripping that part out completely.

Do we have a place where we actually document hooks today..? Seems like
we should and that'd be a good place to put the few necessary comments
regarding these.

> There are a few other problems. First, there's no psql support of any
> kind. Now, this is kind of a corner-case feature: so maybe we don't
> really need it. And as I mentioned on another thread, there aren't a
> lot of good letters left for backslash-d commands.

One thought would be to add it to \dp or have a \dp+.

> So I'd be just as
> happy to add a system view along the lines I previously proposed for
> comments and call it good.

I think that regardless of psql and \d, we should have a sensible system
view for it.

> Second, there are no
> regression tests. It's a bit tricky to think about how to crack that
> nut because this feature is somewhat unusual in that it can't be used
> without loading an appropriate loadable module. I'm wondering if we
> can ship a "dummy_seclabel" contrib module that can be loaded during
> the regression test run and then run various tests using that, but I'm
> not quite sure what the best way to set that up is. SECURITY LABEL is
> a core feature, so it would be nice to test it in the core regression
> tests... but maybe that's too complicated to get working, and we
> should just test it from the contrib module.

The first set of regression tests could simply run the SECURITY LABEL
commands and then check the results in the catalog. If some kind of
psql support is included, it could test that also. That doesn't check
that the hooks are called at the right time and with the right data, so
I agree with the suggestion to have dummy contrib modules (or something)
to do that generically for all our hooks, but I don't think we've got
anything like that today..? If we do, then we should model it off
whatever's there now. Perhaps we can look at how to do it
comprehensively for all hooks..

Thanks,

Stephen

On Thu, Sep 23, 2010 at 10:21 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> Most of the contents of the new documentation section on external
>> security providers seemed irrelevant to me, which I guess I can only
>> blame myself for since I was the one who asked for that section to be
>> created, and I didn't specify what it should contain all that well.  I
>> took a try at rewriting it to be more on-topic, but it didn't amount
>> to much so I ended up just ripping that part out completely.
>
> Do we have a place where we actually document hooks today..?  Seems like
> we should and that'd be a good place to put the few necessary comments
> regarding these.

We do not. Whether or not we should, I'm not sure.

>> There are a few other problems.  First, there's no psql support of any
>> kind.  Now, this is kind of a corner-case feature: so maybe we don't
>> really need it.  And as I mentioned on another thread, there aren't a
>> lot of good letters left for backslash-d commands.
>
> One thought would be to add it to \dp or have a \dp+.

That only works for table-ish things, though.

>> So I'd be just as
>> happy to add a system view along the lines I previously proposed for
>> comments and call it good.
>
> I think that regardless of psql and \d, we should have a sensible system
> view for it.

That's fine with me. The one I wrote for comments can probably be
adapted pretty easily.

>> Second, there are no
>> regression tests.  It's a bit tricky to think about how to crack that
>> nut because this feature is somewhat unusual in that it can't be used
>> without loading an appropriate loadable module.  I'm wondering if we
>> can ship a "dummy_seclabel" contrib module that can be loaded during
>> the regression test run and then run various tests using that, but I'm
>> not quite sure what the best way to set that up is.  SECURITY LABEL is
>> a core feature, so it would be nice to test it in the core regression
>> tests...  but maybe that's too complicated to get working, and we
>> should just test it from the contrib module.
>
> The first set of regression tests could simply run the SECURITY LABEL
> commands and then check the results in the catalog.  If some kind of
> psql support is included, it could test that also.  That doesn't check
> that the hooks are called at the right time and with the right data, so
> I agree with the suggestion to have dummy contrib modules (or something)
> to do that generically for all our hooks, but I don't think we've got
> anything like that today..?  If we do, then we should model it off
> whatever's there now.  Perhaps we can look at how to do it
> comprehensively for all hooks..

The point is that SECURITY LABEL, as coded, will fail utterly unless
there is a label provider loaded. So you can't actually run it and
check the results in the catalog without loading a contrib module.

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> The point is that SECURITY LABEL, as coded, will fail utterly unless
> there is a label provider loaded. So you can't actually run it and
> check the results in the catalog without loading a contrib module.

Urgh, yes, point. Well, we could test that it errors out correctly. :)

Another thought might be to allow the "check if a module is loaded
before doing things" to be a postgresql.conf option that is disabled in
the regression testing.. If you can modify postgresql.conf you can
remove the module anyway.. Interesting question as to if we should
auto-fail queries against objects which have labels when no security
module is loaded. Have we discussed that yet?

Thanks,

Stephen

On Thu, Sep 23, 2010 at 2:06 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> The point is that SECURITY LABEL, as coded, will fail utterly unless
>> there is a label provider loaded.  So you can't actually run it and
>> check the results in the catalog without loading a contrib module.
>
> Urgh, yes, point.  Well, we could test that it errors out correctly. :)

Indeed.

> Another thought might be to allow the "check if a module is loaded
> before doing things" to be a postgresql.conf option that is disabled in
> the regression testing.. If you can modify postgresql.conf you can
> remove the module anyway..

That might work, although I'm not sure whether it's any easier that
getting a contrib module to run during the regression tests. I think
we're testing LOAD in there already somewhere, so...

> Interesting question as to if we should
> auto-fail queries against objects which have labels when no security
> module is loaded.  Have we discussed that yet?

My feeling is that we should do what the existing code does, namely,
bounce the request immediately if the relevant label provider can't be
found. It isn't as if people can't modify the labels anyway in that
case, by messing with pg_seclabel by hand, but I don't really see the
need to spend extra code trying to make this work sensibly when I'm
not sure there's any real sensible behavior. I think that people who
write these modules will need to include a mechanism to disable
checking, hedged about with some appropriate protections. Isn't that
what SE-Linux permissive mode is for? (And you could possibly have a
similar concept within the module, just local to PG, driven off a GUC;
of course the assign_hook can ask SE-Linux whether it's OK to enable
PG-only permissive mode.)

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

Robert, thanks for your reviewing and revising.

(2010/09/23 13:28), Robert Haas wrote:
> On Thu, Sep 16, 2010 at 11:12 PM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>> 2010/9/14 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> The attached patch is a revised version, but a bit difference from what
>>> I introduced yesterday.
>>
>> I am working through this patch and fixing a variety of things things
>> that seem to need fixing. Please hang tight and don't send any new
>> versions for now.
>
> There's no particularly good way to say this, so I'm just going to
> spit it out: this patch was a real mess. In particular, there are a
> huge number of cases where the identifier names were poorly chosen,
> which I have mostly gone through and fixed now. There may yet be some
> arguable and/or wrong cases remaining, and it's certainly possible
> that not everyone may agree with all the choices I made, but it's
> certainly a lot better than it was. I also had to rewrite pretty much
> all of the documentation, comments, and error messages. I reorganized
> a fair amount of the code, too; and ripped out a bunch of stuff that
> looked irrelevant. In theory, this was supposed to be patterned off
> the COMMENT code, but there were various changes which mostly did not
> seem like improvements to me, and which in at least one case were
> plain wrong.
>
Thanks for this revising that I didn't found out.

> Most of the contents of the new documentation section on external
> security providers seemed irrelevant to me, which I guess I can only
> blame myself for since I was the one who asked for that section to be
> created, and I didn't specify what it should contain all that well. I
> took a try at rewriting it to be more on-topic, but it didn't amount
> to much so I ended up just ripping that part out completely.
>
One headache thing when I tried to describe the new documentation section
was what we should describe here, because whole of the chapters in Server
Administration are on the standpoint of users, not developers.

Under the previous discussion, I suggested to move the most of descriptions
about external security providers into chapters in Internals, except for
a mention about a fact we have external security provider at the tail of
Database Roles and Privileges. How about the idea?
Perhaps, the chapters in Internals are appropriate place to introduce
specification of security hooks.

I also think it is irrelevant to describe label based mandatory access
control in the PgSQL documentation. It should be moved to the package of
SE-PgSQL.

> For a couple of reasons, I decided that it made sense to broaden the
> set of objects to which the SECURITY LABEL command can apply. My
> meeting with the NSA folks at BWPUG more or less convinced me that
> we're not going to get very far with this unless we have suitable
> hooks for additional permissions-checking when functions are executed
> or large objects are accessed, so I added labels for those, as well as
> for types, schemas, and procedural languages. It is possible that we
> need more than that, but supporting all of these rather than just
> relations and attributes requires only fairly trivial code changes,
> and I'd like to have at least a month or two go by before I have to
> look at another patch in this area. It's worth noting that labels on
> schemas can be useful even if we don't have a hook for schema-related
> permissions checking, once we have hooks to set labels at object
> creation time: the label for a newly assigned table can be a function
> of the user's label and the schema's label.
>
I agree this enhancement. Early or late, security labels of these objects
become eventually necessary to apply permission checks rather than relations
and attributes.

> I removed the crock that let one label provider veto another label
> provider's label. I understand that MAC will require a control there,
> but (as I said before) that's not the right way to do it. Let's leave
> that as material for a separate patch that solves the whole problem
> well instead of 5% of it poorly.
>
OK, I'll revise this matter later.

> I think the backend code here is now in pretty good shape, but there
> are still a number of things that need to be fixed. The pg_dump
> support is broken at the moment, because of the change to the set of
> objects that can be labeled. I also don't think it's right to dump
> security labels only when asked to do so. I think that the option
> should be --no-security-label in pg_dump(all) just as it is in
> pg_restore.

OK, I'll fix up the specification.

> Also, the pg_dump support for security labels should
> really reuse the existing design for comments, rather than inventing a
> new and less efficient method, unless there is some really compelling
> reason why the method used for comments won't work. Please send a
> reworked patch for just this directory (src/bin/pg_dump).
>
I intended to follow on the existing design for comments.
Could you suggest me how should it be fixed up the design?

Because of the --no-security-label option, we need to dump security
labels in a separated section from comments. So, we cannot pack them
into "COMMENT" sections.

> There are a few other problems. First, there's no psql support of any
> kind. Now, this is kind of a corner-case feature: so maybe we don't
> really need it. And as I mentioned on another thread, there aren't a
> lot of good letters left for backslash-d commands. So I'd be just as
> happy to add a system view along the lines I previously proposed for
> comments and call it good. Alternatively, or in addition, we could
> add a \d command after all. The best way forward is debatable, but we
> certainly need *something*, because interpreting the pg_seclabel
> catalog by hand is not for the faint of heart.

Do you suggest the new system views should be defined for each supported
object classes, such as pg_largeobject_seclabel? It seems to me a bit
inflation of number of system views.
My preference is psql's \d commands at first.

> Second, there are no
> regression tests. It's a bit tricky to think about how to crack that
> nut because this feature is somewhat unusual in that it can't be used
> without loading an appropriate loadable module. I'm wondering if we
> can ship a "dummy_seclabel" contrib module that can be loaded during
> the regression test run and then run various tests using that, but I'm
> not quite sure what the best way to set that up is. SECURITY LABEL is
> a core feature, so it would be nice to test it in the core regression
> tests... but maybe that's too complicated to get working, and we
> should just test it from the contrib module.
>

As you suggested in the following topic, I think it is the best way to
use LOAD command at the head of regression test (or just after SECURITY
LABEL command being failed due to no modules).
I'll add a dummy module into contrib, and regression test for labels.

Right now, I don't have any complaint about the patch to the backend
you revised, so I'd like to submit the next patch as an incremental
one to the seclabel-v4.patch, except for src/bin/pg_dump.

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

2010/9/23 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> Most of the contents of the new documentation section on external
>> security providers seemed irrelevant to me, which I guess I can only
>> blame myself for since I was the one who asked for that section to be
>> created, and I didn't specify what it should contain all that well.  I
>> took a try at rewriting it to be more on-topic, but it didn't amount
>> to much so I ended up just ripping that part out completely.
>>
> One headache thing when I tried to describe the new documentation section
> was what we should describe here, because whole of the chapters in Server
> Administration are on the standpoint of users, not developers.
>
> Under the previous discussion, I suggested to move the most of descriptions
> about external security providers into chapters in Internals, except for
> a mention about a fact we have external security provider at the tail of
> Database Roles and Privileges. How about the idea?
> Perhaps, the chapters in Internals are appropriate place to introduce
> specification of security hooks.

Perhaps. I know that in the past we have not documented hook
functions, and I'm thinking that there may be people (in particular,
possibly Tom) who have strong feelings about keeping it that way.
Even if that's not the case, once we do start documenting the hooks,
we will presumably need to document all of them, and that may be more
of a project than I really want to get into right now, especially if I
will have to do much of the work myself. I'd be perfectly ecstatic if
a committable patch spontaneously materialized, but...

>> Also, the pg_dump support for security labels should
>> really reuse the existing design for comments, rather than inventing a
>> new and less efficient method, unless there is some really compelling
>> reason why the method used for comments won't work.  Please send a
>> reworked patch for just this directory (src/bin/pg_dump).
>>
> I intended to follow on the existing design for comments.
> Could you suggest me how should it be fixed up the design?

dumpComment calls findComments calls collectComments, which dumps all
the comments in one query (not one query per object).

> Because of the --no-security-label option, we need to dump security
> labels in a separated section from comments. So, we cannot pack them
> into "COMMENT" sections.

I'm not proposing that - I just want to avoid sending so many database
queries, if that's possible.

>> There are a few other problems.  First, there's no psql support of any
>> kind.  Now, this is kind of a corner-case feature: so maybe we don't
>> really need it.  And as I mentioned on another thread, there aren't a
>> lot of good letters left for backslash-d commands.  So I'd be just as
>> happy to add a system view along the lines I previously proposed for
>> comments and call it good.  Alternatively, or in addition, we could
>> add a \d command after all.  The best way forward is debatable, but we
>> certainly need *something*, because interpreting the pg_seclabel
>> catalog by hand is not for the faint of heart.
>
> Do you suggest the new system views should be defined for each supported
> object classes, such as pg_largeobject_seclabel? It seems to me a bit
> inflation of number of system views.
> My preference is psql's \d commands at first.

Please see http://archives.postgresql.org/pgsql-hackers/2010-09/msg01080.php

> Right now, I don't have any complaint about the patch to the backend
> you revised, so I'd like to submit the next patch as an incremental
> one to the seclabel-v4.patch, except for src/bin/pg_dump.

Yes, an incremental diff would be preferable, thanks.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Perhaps. I know that in the past we have not documented hook
> functions, and I'm thinking that there may be people (in particular,
> possibly Tom) who have strong feelings about keeping it that way.
> Even if that's not the case, once we do start documenting the hooks,
> we will presumably need to document all of them, and that may be more
> of a project than I really want to get into right now, especially if I
> will have to do much of the work myself. I'd be perfectly ecstatic if
> a committable patch spontaneously materialized, but...

I wouldn't say I have strong feelings about it; but most of the hooks
we've put in so far are things that you really had better be prepared to
read the source code if you want to exploit them. Does anyone want to
write and maintain SGML documentation specifying a complete API for
ProcessUtility, for example?

One of the powerful advantages of being an open source project is that
"use the source, Luke" is a perfectly reasonable approach to documenting
some things. I think hook functions are one.

regards, tom lane

(2010/09/24 11:53), Robert Haas wrote:
> 2010/9/23 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> Most of the contents of the new documentation section on external
>>> security providers seemed irrelevant to me, which I guess I can only
>>> blame myself for since I was the one who asked for that section to be
>>> created, and I didn't specify what it should contain all that well. I
>>> took a try at rewriting it to be more on-topic, but it didn't amount
>>> to much so I ended up just ripping that part out completely.
>>>
>> One headache thing when I tried to describe the new documentation section
>> was what we should describe here, because whole of the chapters in Server
>> Administration are on the standpoint of users, not developers.
>>
>> Under the previous discussion, I suggested to move the most of descriptions
>> about external security providers into chapters in Internals, except for
>> a mention about a fact we have external security provider at the tail of
>> Database Roles and Privileges. How about the idea?
>> Perhaps, the chapters in Internals are appropriate place to introduce
>> specification of security hooks.
>
> Perhaps. I know that in the past we have not documented hook
> functions, and I'm thinking that there may be people (in particular,
> possibly Tom) who have strong feelings about keeping it that way.
> Even if that's not the case, once we do start documenting the hooks,
> we will presumably need to document all of them, and that may be more
> of a project than I really want to get into right now, especially if I
> will have to do much of the work myself. I'd be perfectly ecstatic if
> a committable patch spontaneously materialized, but...
>
If so, maybe, we should keep the scale of documentation to a minimum,
then, rest of the detailed specifications of hooks are kept as source
code comments.
Because authors of ESP obviously reference source code, the comments
will provide them enough information.

>>> Also, the pg_dump support for security labels should
>>> really reuse the existing design for comments, rather than inventing a
>>> new and less efficient method, unless there is some really compelling
>>> reason why the method used for comments won't work. Please send a
>>> reworked patch for just this directory (src/bin/pg_dump).
>>>
>> I intended to follow on the existing design for comments.
>> Could you suggest me how should it be fixed up the design?
>
> dumpComment calls findComments calls collectComments, which dumps all
> the comments in one query (not one query per object).
>
>> Because of the --no-security-label option, we need to dump security
>> labels in a separated section from comments. So, we cannot pack them
>> into "COMMENT" sections.
>
> I'm not proposing that - I just want to avoid sending so many database
> queries, if that's possible.
>
Ahh, Thanks. It makes me clear.
I'll revise dumpSecLabel to call findSecLabels and collectSecLabels on
the first call.

>>> There are a few other problems. First, there's no psql support of any
>>> kind. Now, this is kind of a corner-case feature: so maybe we don't
>>> really need it. And as I mentioned on another thread, there aren't a
>>> lot of good letters left for backslash-d commands. So I'd be just as
>>> happy to add a system view along the lines I previously proposed for
>>> comments and call it good. Alternatively, or in addition, we could
>>> add a \d command after all. The best way forward is debatable, but we
>>> certainly need *something*, because interpreting the pg_seclabel
>>> catalog by hand is not for the faint of heart.
>>
>> Do you suggest the new system views should be defined for each supported
>> object classes, such as pg_largeobject_seclabel? It seems to me a bit
>> inflation of number of system views.
>> My preference is psql's \d commands at first.
>
> Please see http://archives.postgresql.org/pgsql-hackers/2010-09/msg01080.php
>
OK, I'll emulate this approach at first.

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

2010/9/23 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> Please see http://archives.postgresql.org/pgsql-hackers/2010-09/msg01080.php
>>
> OK, I'll emulate this approach at first.

Don't worry about this part - I will do this myself. If you can just
fix the pg_dump stuff, I think we will be in pretty good shape.

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

(2010/09/24 20:56), Robert Haas wrote:
> 2010/9/23 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> Please see http://archives.postgresql.org/pgsql-hackers/2010-09/msg01080.php
>>>
>> OK, I'll emulate this approach at first.
>
> Don't worry about this part - I will do this myself. If you can just
> fix the pg_dump stuff, I think we will be in pretty good shape.
>
Ahh, I already did this part at the today's afternoon:
http://bit.ly/9kOsnx

And, the pg_dump stuff has been just implemented(, but not tested yet):
http://bit.ly/a0eVfL

If you prefer to keep the patch small, I'll revert the system_views.sql
in the next patch.

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

On Fri, Sep 24, 2010 at 8:54 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> (2010/09/24 20:56), Robert Haas wrote:
>>
>> 2010/9/23 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>>>
>>>> Please see
>>>> http://archives.postgresql.org/pgsql-hackers/2010-09/msg01080.php
>>>>
>>> OK, I'll emulate this approach at first.
>>
>> Don't worry about this part - I will do this myself.  If you can just
>> fix the pg_dump stuff, I think we will be in pretty good shape.
>>
> Ahh, I already did this part at the today's afternoon:
>  http://bit.ly/9kOsnx
>
> And, the pg_dump stuff has been just implemented(, but not tested yet):
>  http://bit.ly/a0eVfL
>  If you prefer to keep the patch small, I'll revert the system_views.sql
> in the next patch.

It probably doesn't matter much - it'll likely take me about the same
amount of time to check your work as it would to do it myself, so it's
pretty much six of one, half a dozen of the other.

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

The attached patch can be applied on the Robert's seclabel-v4.patch.

It contains the following stuffs.
* The "dummy_esp" module and regression test for SECURITY LABEL statement.
This module allows only four labels: "unclassified", "classified",
"secret" and "top secret". The later two labels can be set by only
superusers. The new regression test uses this "dummy_esp" module to
find out future regression in SECURITY LABEL statement.
* A minimum description about external security provider at the tail
of Database Roles and Privileges chapter.
* Add pg_seclabels system view
* Revising pg_dump/pg_dumpall
- '--security-label' was replaced by '--no-security-label'
- implemented according to the manner in comments.
findSecLabels() and collectSecLabels() are added to reduce number of
SQL queries, in addition to dumpSecLabel().

Thanks,

(2010/09/24 21:58), Robert Haas wrote:
> On Fri, Sep 24, 2010 at 8:54 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> (2010/09/24 20:56), Robert Haas wrote:
>>>
>>> 2010/9/23 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>>>>
>>>>> Please see
>>>>> http://archives.postgresql.org/pgsql-hackers/2010-09/msg01080.php
>>>>>
>>>> OK, I'll emulate this approach at first.
>>>
>>> Don't worry about this part - I will do this myself. If you can just
>>> fix the pg_dump stuff, I think we will be in pretty good shape.
>>>
>> Ahh, I already did this part at the today's afternoon:
>> http://bit.ly/9kOsnx
>>
>> And, the pg_dump stuff has been just implemented(, but not tested yet):
>> http://bit.ly/a0eVfL
>> If you prefer to keep the patch small, I'll revert the system_views.sql
>> in the next patch.
>
> It probably doesn't matter much - it'll likely take me about the same
> amount of time to check your work as it would to do it myself, so it's
> pretty much six of one, half a dozen of the other.
>

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

On Sat, Sep 25, 2010 at 7:04 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> * The "dummy_esp" module and regression test for SECURITY LABEL statement.
>  This module allows only four labels: "unclassified", "classified",
>  "secret" and "top secret". The later two labels can be set by only
>  superusers. The new regression test uses this "dummy_esp" module to
>  find out future regression in SECURITY LABEL statement.
> * A minimum description about external security provider at the tail
>  of Database Roles and Privileges  chapter.
> * Add pg_seclabels system view
> * Revising pg_dump/pg_dumpall
>  - '--security-label' was replaced by '--no-security-label'
>  - implemented according to the manner in comments.
>    findSecLabels() and collectSecLabels() are added to reduce number of
>    SQL queries, in addition to dumpSecLabel().

Thanks, this looks like mostly good stuff. Here's a new version of
the patch with some bug fixes, additional regression tests, and other
cleanup. I think this is about ready to commit. I didn't adopt your
documentation and I renamed your contrib module from dummy_esp to
dummy_seclabel, but the rest I took more or less as you had it. For
now, I don't want to use the term "external security provider" because
that's not really what this provides - it just provides labels. I
initially thought that an external security provider would really turn
out to be a concept that was somewhat embedded in the system, but on
further reflection I don't think that's the case. I think what we're
going to end up with is a collection of hooks that might happen to be
useful for security-related things, but not necessarily just those.
Anyway, I feel that it's a bit premature to document too much about
what this might do someday; the documentation already in the patch is
adequate to explain what it does now.

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

(2010/09/27 11:49), Robert Haas wrote:
> On Sat, Sep 25, 2010 at 7:04 AM, KaiGai Kohei<kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> * The "dummy_esp" module and regression test for SECURITY LABEL statement.
>> This module allows only four labels: "unclassified", "classified",
>> "secret" and "top secret". The later two labels can be set by only
>> superusers. The new regression test uses this "dummy_esp" module to
>> find out future regression in SECURITY LABEL statement.
>> * A minimum description about external security provider at the tail
>> of Database Roles and Privileges chapter.
>> * Add pg_seclabels system view
>> * Revising pg_dump/pg_dumpall
>> - '--security-label' was replaced by '--no-security-label'
>> - implemented according to the manner in comments.
>> findSecLabels() and collectSecLabels() are added to reduce number of
>> SQL queries, in addition to dumpSecLabel().
>
> Thanks, this looks like mostly good stuff. Here's a new version of
> the patch with some bug fixes, additional regression tests, and other
> cleanup. I think this is about ready to commit.

Thanks for your reviewing and cleaning-up.

> I didn't adopt your
> documentation and I renamed your contrib module from dummy_esp to
> dummy_seclabel, but the rest I took more or less as you had it.

Fair enough. I intended the name of "dummy_esp" to host any other
upcoming regression tests corresponding to security hooks, but
right now it just provides dummy labels.

> For
> now, I don't want to use the term "external security provider" because
> that's not really what this provides - it just provides labels. I
> initially thought that an external security provider would really turn
> out to be a concept that was somewhat embedded in the system, but on
> further reflection I don't think that's the case. I think what we're
> going to end up with is a collection of hooks that might happen to be
> useful for security-related things, but not necessarily just those.

Right, the "security provider" plugin which uses the collection of
security hooks will provides access control decision, but we don't
force anything in the way to make decisions.
Someone may provide label based security, but other one may provide
non-label based security.
All we can say is that guest of the hook on SECURITY LABEL provides
security label, but it is unclear whether it also provides any access
control, or not.
I also think the "label provider" is a fair enough naming.

> Anyway, I feel that it's a bit premature to document too much about
> what this might do someday; the documentation already in the patch is
> adequate to explain what it does now.
>
I agreed. It is quite internal stuff how security hooks should perform
on interactions with plugin modules, so it might be premature.

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

On Mon, Sep 27, 2010 at 4:05 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> Thanks, this looks like mostly good stuff.  Here's a new version of
>> the patch with some bug fixes, additional regression tests, and other
>> cleanup.  I think this is about ready to commit.
>
> Thanks for your reviewing and cleaning-up.

I found and fixed a few more issues and committed this. The pg_dump
support had a few escaping bugs, and I added tab completion support
for psql. Considering the size of the patch, it seems likely that
there are some issues we both overlooked, but this is as solid as I
can make it for right now.

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

On Mon, 27 Sep 2010 21:07:33 -0400
Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I found and fixed a few more issues and committed this. The pg_dump
> support had a few escaping bugs, and I added tab completion support
> for psql. Considering the size of the patch, it seems likely that
> there are some issues we both overlooked, but this is as solid as I
> can make it for right now.
Some OIDs used in SECURITY LABEL patch have already been used for
some functions such as pg_stat_get_xact_numscans().

The src/include/catalog/duplicate_oids script reports that 3037 ~
3040 are used two or more times.

Though all regression tests finish successfully, should this be
fixed ?

regards,
--
Shigeru Hanada

On Tue, Sep 28, 2010 at 3:57 AM, Shigeru HANADA
<hanada(at)metrosystems(dot)co(dot)jp> wrote:
> On Mon, 27 Sep 2010 21:07:33 -0400
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I found and fixed a few more issues and committed this.  The pg_dump
>> support had a few escaping bugs, and I added tab completion support
>> for psql.  Considering the size of the patch, it seems likely that
>> there are some issues we both overlooked, but this is as solid as I
>> can make it for right now.
> Some OIDs used in SECURITY LABEL patch have already been used for
> some functions such as pg_stat_get_xact_numscans().
>
> The src/include/catalog/duplicate_oids script reports that 3037 ~
> 3040 are used two or more times.
>
> Though all regression tests finish successfully, should this be
> fixed ?

Woops. Thanks for the report, fixed. I wish we had a regression test
that would catch these mistakes. It's easy to forget to run this
script.

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

On Tue, Sep 28, 2010 at 13:50, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Sep 28, 2010 at 3:57 AM, Shigeru HANADA
> <hanada(at)metrosystems(dot)co(dot)jp> wrote:
>> On Mon, 27 Sep 2010 21:07:33 -0400
>> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> I found and fixed a few more issues and committed this.  The pg_dump
>>> support had a few escaping bugs, and I added tab completion support
>>> for psql.  Considering the size of the patch, it seems likely that
>>> there are some issues we both overlooked, but this is as solid as I
>>> can make it for right now.
>> Some OIDs used in SECURITY LABEL patch have already been used for
>> some functions such as pg_stat_get_xact_numscans().
>>
>> The src/include/catalog/duplicate_oids script reports that 3037 ~
>> 3040 are used two or more times.
>>
>> Though all regression tests finish successfully, should this be
>> fixed ?
>
> Woops.  Thanks for the report, fixed.  I wish we had a regression test
> that would catch these mistakes.  It's easy to forget to run this
> script.

Could we run the script as part of the regression tests? :-)

Or if not, could we have the buildfarm run it?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On tis, 2010-09-28 at 13:53 +0200, Magnus Hagander wrote:
> On Tue, Sep 28, 2010 at 13:50, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Tue, Sep 28, 2010 at 3:57 AM, Shigeru HANADA
> >> The src/include/catalog/duplicate_oids script reports that 3037 ~
> >> 3040 are used two or more times.
> >>
> >> Though all regression tests finish successfully, should this be
> >> fixed ?
> >
> > Woops. Thanks for the report, fixed. I wish we had a regression test
> > that would catch these mistakes. It's easy to forget to run this
> > script.
>
> Could we run the script as part of the regression tests? :-)
>
> Or if not, could we have the buildfarm run it?

I think it should actually be run as part of the regular build. Ever
since I started using git and developing like 10 features at once, and
other people doing the same, the chances of (hidden) OID conflicts is
growing immensely.

On Tue, Sep 28, 2010 at 8:03 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On tis, 2010-09-28 at 13:53 +0200, Magnus Hagander wrote:
>> On Tue, Sep 28, 2010 at 13:50, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> > On Tue, Sep 28, 2010 at 3:57 AM, Shigeru HANADA
>> >> The src/include/catalog/duplicate_oids script reports that 3037 ~
>> >> 3040 are used two or more times.
>> >>
>> >> Though all regression tests finish successfully, should this be
>> >> fixed ?
>> >
>> > Woops.  Thanks for the report, fixed.  I wish we had a regression test
>> > that would catch these mistakes.  It's easy to forget to run this
>> > script.
>>
>> Could we run the script as part of the regression tests? :-)
>>
>> Or if not, could we have the buildfarm run it?
>
> I think it should actually be run as part of the regular build.  Ever
> since I started using git and developing like 10 features at once, and
> other people doing the same, the chances of (hidden) OID conflicts is
> growing immensely.

Injecting nonessential checks into the build process doesn't seem like
a good idea to me. Typing 'make' should just do the build. If you
want to check for breakage, well, that's what 'make check' is for.

Another angle on this problem is that, at least AFAICT, the duplicate
OIDs are completely harmless so long as they are in different
catalogs. And if they are in the same catalog, then initdb will fail
(and shame on you if you don't notice that). Long, long ago
pg_description was indexed just by object-OID, so duplicates would be
a problem, but that hasn't been the case since 2001, and I'm not aware
of anything else that relies on OIDs being globally unique either. So
maybe we should decide that we just don't care about this any more.
It seems a little silly since we're in no danger of running out of
OIDs any time soon, but if there's no practical reason to do it...

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

On tis, 2010-09-28 at 09:22 -0400, Robert Haas wrote:
> > I think it should actually be run as part of the regular build.
> Ever
> > since I started using git and developing like 10 features at once,
> and
> > other people doing the same, the chances of (hidden) OID conflicts
> is
> > growing immensely.
>
> Injecting nonessential checks into the build process doesn't seem like
> a good idea to me. Typing 'make' should just do the build. If you
> want to check for breakage, well, that's what 'make check' is for.

I don't feel strongly either way, but if we want to philosophize for a
minute, all these -W option on the compiler command line are
nonessential checks. ;-) I suppose 'make check' should test whether the
code behaves correctly rather than checking whether the code was
structured consistently to begin with.

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Another angle on this problem is that, at least AFAICT, the duplicate
> OIDs are completely harmless so long as they are in different
> catalogs. And if they are in the same catalog, then initdb will fail
> (and shame on you if you don't notice that). Long, long ago
> pg_description was indexed just by object-OID, so duplicates would be
> a problem, but that hasn't been the case since 2001, and I'm not aware
> of anything else that relies on OIDs being globally unique either. So
> maybe we should decide that we just don't care about this any more.

No, we shouldn't. The reason we still have the policy of global
uniqueness of manually-assigned OIDs is that those OIDs frequently
need to be copied in multiple places (eg, operators may need to be
entered in pg_amop). It gets a lot easier to make mistakes, and
harder to check for mistakes, if the OIDs aren't unique.

The duplicate_oids script is just something that committers are supposed
to know to run when applying a patch that messes with the catalogs.
That's a sufficiently small minority of patches that I don't see the
attraction of trying to wire it into every build, nor every regression
test. Maybe the landscape is changing to the point where we can't trust
committers to get this right, but I haven't seen evidence of that. You
certainly won't forget it again soon ;-)

regards, tom lane

On Tue, Sep 28, 2010 at 11:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Another angle on this problem is that, at least AFAICT, the duplicate
>> OIDs are completely harmless so long as they are in different
>> catalogs.  And if they are in the same catalog, then initdb will fail
>> (and shame on you if you don't notice that).  Long, long ago
>> pg_description was indexed just by object-OID, so duplicates would be
>> a problem, but that hasn't been the case since 2001, and I'm not aware
>> of anything else that relies on OIDs being globally unique either.  So
>> maybe we should decide that we just don't care about this any more.
>
> No, we shouldn't.  The reason we still have the policy of global
> uniqueness of manually-assigned OIDs is that those OIDs frequently
> need to be copied in multiple places (eg, operators may need to be
> entered in pg_amop).  It gets a lot easier to make mistakes, and
> harder to check for mistakes, if the OIDs aren't unique.

Yeah, I guess that's true.

> The duplicate_oids script is just something that committers are supposed
> to know to run when applying a patch that messes with the catalogs.
> That's a sufficiently small minority of patches that I don't see the
> attraction of trying to wire it into every build, nor every regression
> test.  Maybe the landscape is changing to the point where we can't trust
> committers to get this right, but I haven't seen evidence of that.  You
> certainly won't forget it again soon ;-)

Maybe so, but the more steps there are that have to be manually
remembered, the harder it gets. Run the regression tests, check for
duplicate OIDs, bump catversion, bump WAL version, bump pg_dump
archive version, bump pg_control version, check for unnecessary header
includes, audit trailing whitespace, look for leftovers that don't
need to be committed. If we can combine a step or two, it just makes
life easier.

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

On Tue, Sep 28, 2010 at 3:22 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Sep 28, 2010 at 8:03 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > On tis, 2010-09-28 at 13:53 +0200, Magnus Hagander wrote:
> >> On Tue, Sep 28, 2010 at 13:50, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> >> > On Tue, Sep 28, 2010 at 3:57 AM, Shigeru HANADA
> >> >> The src/include/catalog/duplicate_oids script reports that 3037 ~
> >> >> 3040 are used two or more times.
> >> >>
> >> >> Though all regression tests finish successfully, should this be
> >> >> fixed ?
> >> >
> >> > Woops. Thanks for the report, fixed. I wish we had a regression test
> >> > that would catch these mistakes. It's easy to forget to run this
> >> > script.
> >>
> >> Could we run the script as part of the regression tests? :-)
> >>
> >> Or if not, could we have the buildfarm run it?
> >
> > I think it should actually be run as part of the regular build. Ever
> > since I started using git and developing like 10 features at once, and
> > other people doing the same, the chances of (hidden) OID conflicts is
> > growing immensely.
>
> Injecting nonessential checks into the build process doesn't seem like
> a good idea to me. Typing 'make' should just do the build. If you
> want to check for breakage, well, that's what 'make check' is for.
>
>
How about having git-hooks execute the script on specific action of our
choice? pre-commit hook sounds like a good place to check for this.

Regards,

(I don't have the complete context of this thread, so please ignore my
ignorance)
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com

singh(dot)gurjeet(at){ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

Robert Haas wrote:
> On Tue, Sep 28, 2010 at 3:57 AM, Shigeru HANADA
> <hanada(at)metrosystems(dot)co(dot)jp> wrote:
> > On Mon, 27 Sep 2010 21:07:33 -0400
> > Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >> I found and fixed a few more issues and committed this. ?The pg_dump
> >> support had a few escaping bugs, and I added tab completion support
> >> for psql. ?Considering the size of the patch, it seems likely that
> >> there are some issues we both overlooked, but this is as solid as I
> >> can make it for right now.
> > Some OIDs used in SECURITY LABEL patch have already been used for
> > some functions such as pg_stat_get_xact_numscans().
> >
> > The src/include/catalog/duplicate_oids script reports that 3037 ~
> > 3040 are used two or more times.
> >
> > Though all regression tests finish successfully, should this be
> > fixed ?
>
> Woops. Thanks for the report, fixed. I wish we had a regression test
> that would catch these mistakes. It's easy to forget to run this
> script.

Attached it the script I use for checks that eventually calls
src/tools/pgtest.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +