Обсуждение: security label support, part.2
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@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:c1tclass=db_column name=t.b ERROR: SELinux: security policy violation postgres=# SELECT a FROM t; a --- (0 rows) Thanks, -- KaiGai Kohei <kaigai@ak.jp.nec.com>
Вложения
2010/7/14 KaiGai Kohei <kaigai@ak.jp.nec.com>: > The attached patch is a part of efforts to support security label > on database objects. This is similar to what I had in mind as a design for this feature, but I have some gripes: 1. I am inclined to suggest the syntax SECURITY LABEL ON ... IS ..., following COMMENT ON (it's also somewhat similar to the GRANT syntax).While the hook in ExecCheckRTPerms() will only allowmeaningful permissions checks on the use of relations, there will presumably be ongoing demand to add additional hooks to cover other types of objects, and I'd rather add label support all at once rather than bit-by-bit. I also think that the SECURITY LABEL syntax is more future-proof; we don't need to worry about conflicts in other parts of the grammar. 2. Similarly, the design of the hook in secabel.h is way too short-sighted and won't scale to any other object type. We don't need or want one hook per object type here. Use an ObjectAddress. 3. I am firmly of the view that we want to allow multiple security providers. I think the way this should work here is that we should keep a list somewhere of security providers known to the system, which loadable modules should add themselves to. Each such security provider should be represented by a struct containing, at least, a name and a function that gets called on relabel. The labels should be stored in the catalog. That way there is never any possibility of one security provider getting a label that was originally applied by some other security provider. If we were storing these labels in pg_class or pg_attribute or similar, the storage cost for this might be worth worrying about, but as this is a separate catalog, it's not. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Thanks for your reviewing. (2010/07/23 0:54), Robert Haas wrote: > 2010/7/14 KaiGai Kohei<kaigai@ak.jp.nec.com>: >> The attached patch is a part of efforts to support security label >> on database objects. > > This is similar to what I had in mind as a design for this feature, > but I have some gripes: > > 1. I am inclined to suggest the syntax SECURITY LABEL ON ... IS ..., > following COMMENT ON (it's also somewhat similar to the GRANT syntax). > While the hook in ExecCheckRTPerms() will only allow meaningful > permissions checks on the use of relations, there will presumably be > ongoing demand to add additional hooks to cover other types of > objects, and I'd rather add label support all at once rather than > bit-by-bit. I also think that the SECURITY LABEL syntax is more > future-proof; we don't need to worry about conflicts in other parts of > the grammar. > Hmm. Indeed, we cannot deny the possibility to conflict with other part in the future, if we use ALTER xxx statement here. But, right now, we have no statement that starts in noun, rather than verb. The "comment" is a noun, but "comment on" is a phrasal-verb, isn't it? How about RELABEL <object> TO <label>, instead? The "relabel" is a transitive-verb, we don't need "ON" between RELABEL and <object>. And, it tries to change a property of the object, so it seems to me "TO" is more appropriate than "IS". > 2. Similarly, the design of the hook in secabel.h is way too > short-sighted and won't scale to any other object type. We don't need > or want one hook per object type here. Use an ObjectAddress. > I think the relation type is an exceptional object class, because of the recursion due to the table inheritances. For other object classes, I also think one security hook which takes ObjectAddress as an argument is enough to implement. So, I expect we need two hooks on relabeling eventually. (One for relation, one for other object classes) > 3. I am firmly of the view that we want to allow multiple security > providers. I think the way this should work here is that we should > keep a list somewhere of security providers known to the system, which > loadable modules should add themselves to. Each such security > provider should be represented by a struct containing, at least, a > name and a function that gets called on relabel. The labels should be > stored in the catalog. That way there is never any possibility of one > security provider getting a label that was originally applied by some > other security provider. If we were storing these labels in pg_class > or pg_attribute or similar, the storage cost for this might be worth > worrying about, but as this is a separate catalog, it's not. > What I'm worrying about is that we cannot estimate amount of works when we expand the concept to row-level security. We will need to revise the implementation, if individual user tuples have its security label in the future version. If we don't support multiple labels right now, it will not be feature degradation, even if it will be hard to implement multiple label support for each user tuples. :( I don't deny worth of multiple security providers concurrently, however, I doubt whether it should be supported from the beginning, or not. It seems to me it is not too late after we can find out the way to label individual user tuples. Thanks, -- KaiGai Kohei <kaigai@ak.jp.nec.com>
2010/7/22 KaiGai Kohei <kaigai@ak.jp.nec.com>: > Thanks for your reviewing. >> 1. I am inclined to suggest the syntax SECURITY LABEL ON ... IS ..., >> following COMMENT ON (it's also somewhat similar to the GRANT syntax). >> While the hook in ExecCheckRTPerms() will only allow meaningful >> permissions checks on the use of relations, there will presumably be >> ongoing demand to add additional hooks to cover other types of >> objects, and I'd rather add label support all at once rather than >> bit-by-bit. I also think that the SECURITY LABEL syntax is more >> future-proof; we don't need to worry about conflicts in other parts of >> the grammar. >> > Hmm. Indeed, we cannot deny the possibility to conflict with other part > in the future, if we use ALTER xxx statement here. > > But, right now, we have no statement that starts in noun, rather than verb. > The "comment" is a noun, but "comment on" is a phrasal-verb, isn't it? > > How about RELABEL <object> TO <label>, instead? Well, I like SECURITY LABEL better because it's more clear about what kind of label we're talking about, but if there's consensus on some other option it's OK with me. Actually, we need to work the security provider name in there too, I think, so perhaps SECURITY LABEL FOR provider ON object IS labeltext. I realize it's slightly odd grammatically, but it's no worse than the COMMENT syntax. >> 2. Similarly, the design of the hook in secabel.h is way too >> short-sighted and won't scale to any other object type. We don't need >> or want one hook per object type here. Use an ObjectAddress. >> > I think the relation type is an exceptional object class, because of > the recursion due to the table inheritances. > For other object classes, I also think one security hook which takes > ObjectAddress as an argument is enough to implement. > > So, I expect we need two hooks on relabeling eventually. > (One for relation, one for other object classes) Please explain in more detail. >> 3. I am firmly of the view that we want to allow multiple security >> providers. I think the way this should work here is that we should >> keep a list somewhere of security providers known to the system, which >> loadable modules should add themselves to. Each such security >> provider should be represented by a struct containing, at least, a >> name and a function that gets called on relabel. The labels should be >> stored in the catalog. That way there is never any possibility of one >> security provider getting a label that was originally applied by some >> other security provider. If we were storing these labels in pg_class >> or pg_attribute or similar, the storage cost for this might be worth >> worrying about, but as this is a separate catalog, it's not. >> > What I'm worrying about is that we cannot estimate amount of works when > we expand the concept to row-level security. We will need to revise the > implementation, if individual user tuples have its security label in the > future version. > If we don't support multiple labels right now, it will not be feature > degradation, even if it will be hard to implement multiple label support > for each user tuples. :( I think it is 100% clear that row-level security will require completely separate infrastructure, and therefore I'm not even a tiny bit worried about this. :-) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
(2010/07/23 10:05), Robert Haas wrote: > 2010/7/22 KaiGai Kohei<kaigai@ak.jp.nec.com>: >> Thanks for your reviewing. >>> 1. I am inclined to suggest the syntax SECURITY LABEL ON ... IS ..., >>> following COMMENT ON (it's also somewhat similar to the GRANT syntax). >>> While the hook in ExecCheckRTPerms() will only allow meaningful >>> permissions checks on the use of relations, there will presumably be >>> ongoing demand to add additional hooks to cover other types of >>> objects, and I'd rather add label support all at once rather than >>> bit-by-bit. I also think that the SECURITY LABEL syntax is more >>> future-proof; we don't need to worry about conflicts in other parts of >>> the grammar. >>> >> Hmm. Indeed, we cannot deny the possibility to conflict with other part >> in the future, if we use ALTER xxx statement here. >> >> But, right now, we have no statement that starts in noun, rather than verb. >> The "comment" is a noun, but "comment on" is a phrasal-verb, isn't it? >> >> How about RELABEL<object> TO<label>, instead? > > Well, I like SECURITY LABEL better because it's more clear about what > kind of label we're talking about, but if there's consensus on some > other option it's OK with me. Actually, we need to work the security > provider name in there too, I think, so perhaps SECURITY LABEL FOR > provider ON object IS labeltext. I realize it's slightly odd > grammatically, but it's no worse than the COMMENT syntax. > The "FOR <provider>" clause should be optional. I expect most use cases installs only one security provider, rather than multiple. If no explicit <provider> is specified, all the security providers check the given security label. If two or more providers are here, of course, either of them will raise an error, because they have different label formats. It is right. Anyway, I'd like to implement according to the idea. SECURITY LABEL [FOR <provider>] ON <object> IS <label>; >>> 2. Similarly, the design of the hook in secabel.h is way too >>> short-sighted and won't scale to any other object type. We don't need >>> or want one hook per object type here. Use an ObjectAddress. >>> >> I think the relation type is an exceptional object class, because of >> the recursion due to the table inheritances. >> For other object classes, I also think one security hook which takes >> ObjectAddress as an argument is enough to implement. >> >> So, I expect we need two hooks on relabeling eventually. >> (One for relation, one for other object classes) > > Please explain in more detail. > For relations, one SECURITY LABEL statement may relabel multiple tables when it has child tables, if ONLY clause was not given. So, we need to obtain oids to be relabeled using find_all_inheritors(), and need to ask providers whether it allows, or not. But, obviously, it is specific for relations. For other object class, the target object to be relabeled is identified by <object> in SECURITY LABEL statement. It will be parsed by the upcoming parse_object.c feature, then it solves the object name to ObjectAddress. So, we can apply access controls after setting up the ObjectAddress with common hooks for object classes except for relations, like: void check_object_relabel(ObjectAddress object, const char *new_label); >>> 3. I am firmly of the view that we want to allow multiple security >>> providers. I think the way this should work here is that we should >>> keep a list somewhere of security providers known to the system, which >>> loadable modules should add themselves to. Each such security >>> provider should be represented by a struct containing, at least, a >>> name and a function that gets called on relabel. The labels should be >>> stored in the catalog. That way there is never any possibility of one >>> security provider getting a label that was originally applied by some >>> other security provider. If we were storing these labels in pg_class >>> or pg_attribute or similar, the storage cost for this might be worth >>> worrying about, but as this is a separate catalog, it's not. >>> >> What I'm worrying about is that we cannot estimate amount of works when >> we expand the concept to row-level security. We will need to revise the >> implementation, if individual user tuples have its security label in the >> future version. >> If we don't support multiple labels right now, it will not be feature >> degradation, even if it will be hard to implement multiple label support >> for each user tuples. :( > > I think it is 100% clear that row-level security will require > completely separate infrastructure, and therefore I'm not even a tiny > bit worried about this. :-) > Hmm. Are you saying we may degrade the feature when we switch to the completely separate infrastructure? Is it preferable?? Thanks, -- KaiGai Kohei <kaigai@ak.jp.nec.com>
2010/7/22 KaiGai Kohei <kaigai@ak.jp.nec.com>: >> Well, I like SECURITY LABEL better because it's more clear about what >> kind of label we're talking about, but if there's consensus on some >> other option it's OK with me. Actually, we need to work the security >> provider name in there too, I think, so perhaps SECURITY LABEL FOR >> provider ON object IS labeltext. I realize it's slightly odd >> grammatically, but it's no worse than the COMMENT syntax. >> > The "FOR <provider>" clause should be optional. I expect most use > cases installs only one security provider, rather than multiple. > > If no explicit <provider> is specified, all the security providers > check the given security label. If two or more providers are here, > of course, either of them will raise an error, because they have > different label formats. It is right. Hmm. How about if there's just one provider loaded, you can omit it, but if you fail to specify it and there's >1 loaded, we just throw an error saying you didn't specify whose label it is. >>> So, I expect we need two hooks on relabeling eventually. >>> (One for relation, one for other object classes) >> >> Please explain in more detail. >> > For relations, one SECURITY LABEL statement may relabel multiple tables > when it has child tables, if ONLY clause was not given. > So, we need to obtain oids to be relabeled using find_all_inheritors(), > and need to ask providers whether it allows, or not. > But, obviously, it is specific for relations. > > For other object class, the target object to be relabeled is identified > by <object> in SECURITY LABEL statement. It will be parsed by the upcoming > parse_object.c feature, then it solves the object name to ObjectAddress. > So, we can apply access controls after setting up the ObjectAddress with > common hooks for object classes except for relations, like: > > void check_object_relabel(ObjectAddress object, const char *new_label); So just construct an ObjectAddress for each relation and call the check function once for each. >> I think it is 100% clear that row-level security will require >> completely separate infrastructure, and therefore I'm not even a tiny >> bit worried about this. :-) >> > Hmm. Are you saying we may degrade the feature when we switch to the > completely separate infrastructure? Is it preferable?? Uh... no, not really. I'm saying that I don't think we're backing ourselves into a corner. What makes you think we are? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
(2010/07/23 12:56), Robert Haas wrote: > 2010/7/22 KaiGai Kohei<kaigai@ak.jp.nec.com>: >>> Well, I like SECURITY LABEL better because it's more clear about what >>> kind of label we're talking about, but if there's consensus on some >>> other option it's OK with me. Actually, we need to work the security >>> provider name in there too, I think, so perhaps SECURITY LABEL FOR >>> provider ON object IS labeltext. I realize it's slightly odd >>> grammatically, but it's no worse than the COMMENT syntax. >>> >> The "FOR<provider>" clause should be optional. I expect most use >> cases installs only one security provider, rather than multiple. >> >> If no explicit<provider> is specified, all the security providers >> check the given security label. If two or more providers are here, >> of course, either of them will raise an error, because they have >> different label formats. It is right. > > Hmm. How about if there's just one provider loaded, you can omit it, > but if you fail to specify it and there's>1 loaded, we just throw an > error saying you didn't specify whose label it is. > Perhaps, we need to return the caller a state whether one provider checked the given label at least, or not. If invalid <provider> was specified so nobody checked it, nobody returns the caller a state of "checked", then it raises an error to notice invalid security provider. If valid <provider> was specified, only specified provider checks the given label, and returns the caller a state of "it was checked by xxxx". If it was omitted, all the providers try to check the given label, but it has mutually different format, so one of providers will raise an error at least. It means we have to specify the provider when two or more providers are loaded, but not necessary when just one provider. >>>> So, I expect we need two hooks on relabeling eventually. >>>> (One for relation, one for other object classes) >>> >>> Please explain in more detail. >>> >> For relations, one SECURITY LABEL statement may relabel multiple tables >> when it has child tables, if ONLY clause was not given. >> So, we need to obtain oids to be relabeled using find_all_inheritors(), >> and need to ask providers whether it allows, or not. >> But, obviously, it is specific for relations. >> >> For other object class, the target object to be relabeled is identified >> by<object> in SECURITY LABEL statement. It will be parsed by the upcoming >> parse_object.c feature, then it solves the object name to ObjectAddress. >> So, we can apply access controls after setting up the ObjectAddress with >> common hooks for object classes except for relations, like: >> >> void check_object_relabel(ObjectAddress object, const char *new_label); > > So just construct an ObjectAddress for each relation and call the > check function once for each. > OK, I'll revise it. >>> I think it is 100% clear that row-level security will require >>> completely separate infrastructure, and therefore I'm not even a tiny >>> bit worried about this. :-) >>> >> Hmm. Are you saying we may degrade the feature when we switch to the >> completely separate infrastructure? Is it preferable?? > > Uh... no, not really. I'm saying that I don't think we're backing > ourselves into a corner. What makes you think we are? > Sorry, meaning of the last question was unclear for me.... Is it a idiom? Thanks, -- KaiGai Kohei <kaigai@ak.jp.nec.com>
2010/7/23 KaiGai Kohei <kaigai@ak.jp.nec.com>: >> Hmm. How about if there's just one provider loaded, you can omit it, >> but if you fail to specify it and there's>1 loaded, we just throw an >> error saying you didn't specify whose label it is. >> > Perhaps, we need to return the caller a state whether one provider checked > the given label at least, or not. Return to the caller? This is an SQL command. You either get an error, or you don't. > If it was omitted, all the providers try to check the given label, but it > has mutually different format, so one of providers will raise an error at > least. Yeah, but it won't be a very clear error, and what if you have, say, a provider that allows arbitrary strings as labels? Since this is a security feature, I think it's a pretty bad idea to allow the user to do anything that might be ambiguous. > It means we have to specify the provider when two or more providers are > loaded, but not necessary when just one provider. But that should be fine. Loading multiple providers should, as you say, be fairly rare. >>>> I think it is 100% clear that row-level security will require >>>> completely separate infrastructure, and therefore I'm not even a tiny >>>> bit worried about this. :-) >>>> >>> Hmm. Are you saying we may degrade the feature when we switch to the >>> completely separate infrastructure? Is it preferable?? >> >> Uh... no, not really. I'm saying that I don't think we're backing >> ourselves into a corner. What makes you think we are? >> > Sorry, meaning of the last question was unclear for me.... Is it a idiom? I don't understand why we wouldn't be able to support multiple providers for row-level security. Why do you think that's a problem? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
* Robert Haas (robertmhaas@gmail.com) wrote: > I don't understand why we wouldn't be able to support multiple > providers for row-level security. Why do you think that's a problem? My guess would be that he's concerned about only having space in the tuple header for 1 label. I see two answers- only allow 1 provider for a given relation (doesn't strike me as a horrible limitation), or handle labels as extra columns where you could have more than one. Thanks, Stephen
On Fri, Jul 23, 2010 at 8:32 AM, Stephen Frost <sfrost@snowman.net> wrote: > * Robert Haas (robertmhaas@gmail.com) wrote: >> I don't understand why we wouldn't be able to support multiple >> providers for row-level security. Why do you think that's a problem? > > My guess would be that he's concerned about only having space in the > tuple header for 1 label. I see two answers- only allow 1 provider for > a given relation (doesn't strike me as a horrible limitation), or handle > labels as extra columns where you could have more than one. I think we've been pretty clear in previous discussions that any row-level security implementation should be a general one, and SE-Linux or whatever can integrate with that to do what it needs to do. So I'm pretty sure we'll be using regular columns rather than cramming anything into the tuple header. There are pretty substantial performance benefits to such an implementation, as well. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
(2010/07/23 20:44), Robert Haas wrote: > 2010/7/23 KaiGai Kohei<kaigai@ak.jp.nec.com>: >>> Hmm. How about if there's just one provider loaded, you can omit it, >>> but if you fail to specify it and there's>1 loaded, we just throw an >>> error saying you didn't specify whose label it is. >>> >> Perhaps, we need to return the caller a state whether one provider checked >> the given label at least, or not. > > Return to the caller? This is an SQL command. You either get an > error, or you don't. > Ahh, I was talked about relationship between the core PG code and ESP module. It means the security hook returns a state which informs the core PG code whether one provider checked the given label, then the core PG code can decide whether it raise an actual error to users, or not. In other words, I'd like to suggest the security hook which returns a tag of ESP module, as follows: const char * check_object_relabel_hook(const ObjectAddress *object, const char *provider, const char *seclabel); The second argument reflects "FOR <provider>" clause. It informs ESP modules what provider is specified. If omitted, it will be NULL. Then, ESP module which checked the given security label must return its tag. Maybe, "selinux", if SE-PostgreSQL. Or, NULL will be returned if nobody checked it. If NULL or incorrect tag is returned, the core PG code can know the given seclabel is not checked/validated, then it will raise an error to users. Elsewhere, the validated label will be stored with the returned tag. It enables to recognize what label is validated by SELinux, and what label is not. >> If it was omitted, all the providers try to check the given label, but it >> has mutually different format, so one of providers will raise an error at >> least. > > Yeah, but it won't be a very clear error, and what if you have, say, a > provider that allows arbitrary strings as labels? Since this is a > security feature, I think it's a pretty bad idea to allow the user to > do anything that might be ambiguous. > It is provider's job to validate the given security label. So, if we install such a security module which accept arbitrary strings as label, the core PG code also need to believe the ESP module. But the arbitrary label will be tagged with something other than "selinux", so it does not confuse other module, according to the above idea. >> It means we have to specify the provider when two or more providers are >> loaded, but not necessary when just one provider. > > But that should be fine. Loading multiple providers should, as you > say, be fairly rare. > >>>>> I think it is 100% clear that row-level security will require >>>>> completely separate infrastructure, and therefore I'm not even a tiny >>>>> bit worried about this. :-) >>>>> >>>> Hmm. Are you saying we may degrade the feature when we switch to the >>>> completely separate infrastructure? Is it preferable?? >>> >>> Uh... no, not really. I'm saying that I don't think we're backing >>> ourselves into a corner. What makes you think we are? >>> >> Sorry, meaning of the last question was unclear for me.... Is it a idiom? > > I don't understand why we wouldn't be able to support multiple > providers for row-level security. Why do you think that's a problem? > I don't have any clear reason why we wouldn't be able to support multiple labels on user tuples, but it is intangible anxiety, because I have not implemented it as a working example yet. (So, I never think it is impossible.) Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Fri, Jul 23, 2010 at 8:59 AM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote: > (2010/07/23 20:44), Robert Haas wrote: >> >> 2010/7/23 KaiGai Kohei<kaigai@ak.jp.nec.com>: >>>> >>>> Hmm. How about if there's just one provider loaded, you can omit it, >>>> but if you fail to specify it and there's>1 loaded, we just throw an >>>> error saying you didn't specify whose label it is. >>>> >>> Perhaps, we need to return the caller a state whether one provider >>> checked >>> the given label at least, or not. >> >> Return to the caller? This is an SQL command. You either get an >> error, or you don't. >> > Ahh, I was talked about relationship between the core PG code and ESP > module. > It means the security hook returns a state which informs the core PG code > whether one provider checked the given label, then the core PG code can > decide whether it raise an actual error to users, or not. > > In other words, I'd like to suggest the security hook which returns a tag > of ESP module, as follows: > > const char * > check_object_relabel_hook(const ObjectAddress *object, > const char *provider, > const char *seclabel); I don't think that's a very good design. What I had in mind was a simple API for security providers to register themselves (including their names), and then the core code will only call the relevant security provider. I did try to explain this in point #3 of my original review. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
The attached patches are revised ones, as follows. * A new SECURITY LABEL statement replaced the previous ALTER TABLE statement with SECURITY LABEL TO option. It has the following syntax. SECURITY LABEL [ FOR <provider> ] ON <object class> <object name> IS '<label>'; E.g) SECURITY LABEL ON TABLE t1 IS 'system_u:object_r:sepgsql_table_t:s0'; * It supports multiple security providers to assign its security label on a database object. The pg_seclabel catalog was modified as follows: CATALOG(pg_seclabel,3037) BKI_WITHOUT_OIDS { Oid reloid; /* OID of table containing the object */ Oid objoid; /* OID of the object itself */ int4 subid; /* column number, or 0 if not used */ + text tag; /* identifier of external security provider */ text label; /* security label of the object */ } FormData_pg_seclabel; The new 'tag' field identifies which security provider manages this security label. For example, SE-PostgreSQL uses "selinux" for its identifier. * The security hook to check relabeling become to be registered using register_object_relabel_hook() which takes a tag of ESP module and a function pointer to the security hook. ExecSecLabelStmt() picks up an appropriate security hook, then it shall be invoked even if multiple modules are loaded. * Add _copySecLabelStmt() on nodes/copyfuncs.c and _equalSecLabelStmt() on nodes/equalfuncs.c, because I forgot to add them, although new parsenode (SecLabelStmt) was added. * Add descriptions about pg_seclabel catalog and SECURITY LABEL statement on the documentation. Thanks, (2010/07/23 22:36), Robert Haas wrote: > On Fri, Jul 23, 2010 at 8:59 AM, KaiGai Kohei<kaigai@kaigai.gr.jp> wrote: >> (2010/07/23 20:44), Robert Haas wrote: >>> >>> 2010/7/23 KaiGai Kohei<kaigai@ak.jp.nec.com>: >>>>> >>>>> Hmm. How about if there's just one provider loaded, you can omit it, >>>>> but if you fail to specify it and there's>1 loaded, we just throw an >>>>> error saying you didn't specify whose label it is. >>>>> >>>> Perhaps, we need to return the caller a state whether one provider >>>> checked >>>> the given label at least, or not. >>> >>> Return to the caller? This is an SQL command. You either get an >>> error, or you don't. >>> >> Ahh, I was talked about relationship between the core PG code and ESP >> module. >> It means the security hook returns a state which informs the core PG code >> whether one provider checked the given label, then the core PG code can >> decide whether it raise an actual error to users, or not. >> >> In other words, I'd like to suggest the security hook which returns a tag >> of ESP module, as follows: >> >> const char * >> check_object_relabel_hook(const ObjectAddress *object, >> const char *provider, >> const char *seclabel); > > I don't think that's a very good design. What I had in mind was a > simple API for security providers to register themselves (including > their names), and then the core code will only call the relevant > security provider. I did try to explain this in point #3 of my > original review. > -- KaiGai Kohei <kaigai@ak.jp.nec.com>
Вложения
2010/7/26 KaiGai Kohei <kaigai@ak.jp.nec.com>: > The attached patches are revised ones, as follows. I think this is pretty good, and I'm generally in favor of committing it. Some concerns: 1. Since nobody has violently objected to the comment.c refactoring patch I recently proposed, I'm hopeful that can go in. And if that's the case, then I'd prefer to see that committed first, and then rework this to use that code. That would eliminate some code here, and it would also make it much easier to support labels on other types of objects. 2. Some of this code refers to "local" security labels. I'm not sure what's "local" about them - aren't they just security labels? On a related note, I don't like the trivial wrappers you have here, with DeleteSecurityLabel around DeleteLocalSecLabel, SetSecurityLabel around SetLocalSecLabel, etc. Just collapse these into a single set of functions. 3. Is it really appropriate for ExecRelationSecLabel() to have an "Exec" prefix? I don't think so. 4. Please get rid of the nkeys++ stuff in DeleteLocalSecLabel() and just use fixed offsets as we do everywhere else. 5. Why do we think that the relabel hook needs to be passed the number of expected parents? 6. What are we doing about the assignment of initial security labels? I had initially thought that perhaps new objects would just start out unlabeled, and the user would be responsible for labeling them as needed. But maybe we can do better. Perhaps we should extend the security provider hook API with a function that gets called when a labellable object gets created, and let each loaded security provider return any label it would like attached. Even if we don't do that now, esp_relabel_hook_entry needs to be renamed to something more generic; we will certainly want to add more fields to that structure later. 7. I think we need to write and include in the fine documentation some "big picture" documentation about enhanced security providers. Of course, we have to decide what we want to say. But the SECURITY LABEL documentation is just kind of hanging out there in space right now; it needs to connect to a broad introduction to the subject. 8. Generally, the English in both the comments and documentation needs work. However, we can address that problem when we're closer to commit. I am going to mark this Returned with Feedback because I don't believe it's realistic to get the comment code committed in the next week, rework this patch, and then get this patch committed also. However, I'm feeling pretty good about this effort and I think we're making good progress toward getting this done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Thanks for your reviewing. On Mon, 9 Aug 2010 16:02:12 -0400 Robert Haas <robertmhaas@gmail.com> wrote: > 2010/7/26 KaiGai Kohei <kaigai@ak.jp.nec.com>: > > The attached patches are revised ones, as follows. > > I think this is pretty good, and I'm generally in favor of committing > it. Some concerns: > > 1. Since nobody has violently objected to the comment.c refactoring > patch I recently proposed, I'm hopeful that can go in. And if that's > the case, then I'd prefer to see that committed first, and then rework > this to use that code. That would eliminate some code here, and it > would also make it much easier to support labels on other types of > objects. > It seems to me fair enough. This parse_object.c can also provide a facility to resolve the name of object to be labeled. > 2. Some of this code refers to "local" security labels. I'm not sure > what's "local" about them - aren't they just security labels? On a > related note, I don't like the trivial wrappers you have here, with > DeleteSecurityLabel around DeleteLocalSecLabel, SetSecurityLabel > around SetLocalSecLabel, etc. Just collapse these into a single set > of functions. > In the feature, I plan to assign security labels on the shared database objects such as pg_database. The "local" is a contradistinction towards these "shared" objects. > 3. Is it really appropriate for ExecRelationSecLabel() to have an > "Exec" prefix? I don't think so. > I don't have any preferences about > 4. Please get rid of the nkeys++ stuff in DeleteLocalSecLabel() and > just use fixed offsets as we do everywhere else. > OK, I'll fix it. > 5. Why do we think that the relabel hook needs to be passed the number > of expected parents? > We need to prevent relabeling on inherited relations/columns from the multiple origin, like ALTER RENAME TO. It requires to pass the expected parents into the provider, or to check it in the caller. > 6. What are we doing about the assignment of initial security labels? > I had initially thought that perhaps new objects would just start out > unlabeled, and the user would be responsible for labeling them as > needed. But maybe we can do better. Perhaps we should extend the > security provider hook API with a function that gets called when a > labellable object gets created, and let each loaded security provider > return any label it would like attached. Even if we don't do that > now, esp_relabel_hook_entry needs to be renamed to something more > generic; we will certainly want to add more fields to that structure > later. > Starting with "unlabeled" is wrong, because it does not distinguish an object created by security sensitive users and insensitive users, for example. It is similar to relation's relowner is InvalidOid. I plan the security provider hook on the creation time works two things. 1. It checks user's privilege to create this object. 2. It returns security labels to be assigned. Then, the caller assigns these returned labels on the new object, if one or more valid labels are returned. > 7. I think we need to write and include in the fine documentation some > "big picture" documentation about enhanced security providers. Of > course, we have to decide what we want to say. But the SECURITY LABEL > documentation is just kind of hanging out there in space right now; it > needs to connect to a broad introduction to the subject. > OK, I'll try to describe with appropriate granularity. Do we need an independent section in addition to the introduction of SECURITY LABEL syntax? > 8. Generally, the English in both the comments and documentation needs > work. However, we can address that problem when we're closer to > commit. > OK BTW, I'll go on the area where internet unconnectable during next two days. Perhaps, my reply will run late. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
2010/8/9 <kaigai@kaigai.gr.jp>: >> 2. Some of this code refers to "local" security labels. I'm not sure >> what's "local" about them - aren't they just security labels? On a >> related note, I don't like the trivial wrappers you have here, with >> DeleteSecurityLabel around DeleteLocalSecLabel, SetSecurityLabel >> around SetLocalSecLabel, etc. Just collapse these into a single set >> of functions. >> > In the feature, I plan to assign security labels on the shared database > objects such as pg_database. The "local" is a contradistinction > towards these "shared" objects. Oh, I see. I don't think that's entirely clear: and in any event it seems a bit premature, since we're not at that point yet. Let's just get rid of this stuff for now as I suggested. >> 5. Why do we think that the relabel hook needs to be passed the number >> of expected parents? >> > We need to prevent relabeling on inherited relations/columns from > the multiple origin, like ALTER RENAME TO. > It requires to pass the expected parents into the provider, or > to check it in the caller. Please explain further. I don't understand. >> 6. What are we doing about the assignment of initial security labels? >> I had initially thought that perhaps new objects would just start out >> unlabeled, and the user would be responsible for labeling them as >> needed. But maybe we can do better. Perhaps we should extend the >> security provider hook API with a function that gets called when a >> labellable object gets created, and let each loaded security provider >> return any label it would like attached. Even if we don't do that >> now, esp_relabel_hook_entry needs to be renamed to something more >> generic; we will certainly want to add more fields to that structure >> later. >> > Starting with "unlabeled" is wrong, because it does not distinguish > an object created by security sensitive users and insensitive users, > for example. It is similar to relation's relowner is InvalidOid. > > I plan the security provider hook on the creation time works two things. > 1. It checks user's privilege to create this object. > 2. It returns security labels to be assigned. > > Then, the caller assigns these returned labels on the new object, > if one or more valid labels are returned. OK, let's give that a try and see how it looks. I don't think that's in this version of the patch, right? >> 7. I think we need to write and include in the fine documentation some >> "big picture" documentation about enhanced security providers. Of >> course, we have to decide what we want to say. But the SECURITY LABEL >> documentation is just kind of hanging out there in space right now; it >> needs to connect to a broad introduction to the subject. >> > OK, I'll try to describe with appropriate granularity. > Do we need an independent section in addition to the introduction of > SECURITY LABEL syntax? I think so. I suggest a new chapter called "Enhanced Security Providers" just after "Database Roles and Privileges". > BTW, I'll go on the area where internet unconnectable during next > two days. Perhaps, my reply will run late. No problem. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
(2010/08/10 8:39), Robert Haas wrote: > 2010/8/9<kaigai@kaigai.gr.jp>: >>> 2. Some of this code refers to "local" security labels. I'm not sure >>> what's "local" about them - aren't they just security labels? On a >>> related note, I don't like the trivial wrappers you have here, with >>> DeleteSecurityLabel around DeleteLocalSecLabel, SetSecurityLabel >>> around SetLocalSecLabel, etc. Just collapse these into a single set >>> of functions. >>> >> In the feature, I plan to assign security labels on the shared database >> objects such as pg_database. The "local" is a contradistinction >> towards these "shared" objects. > > Oh, I see. I don't think that's entirely clear: and in any event it > seems a bit premature, since we're not at that point yet. Let's just > get rid of this stuff for now as I suggested. > OK. We can add this supportanytime we need it. >>> 5. Why do we think that the relabel hook needs to be passed the number >>> of expected parents? >>> >> We need to prevent relabeling on inherited relations/columns from >> the multiple origin, like ALTER RENAME TO. >> It requires to pass the expected parents into the provider, or >> to check it in the caller. > > Please explain further. I don't understand. > Yep, rte->requiredPerms of inherited relations are cleared on the expand_inherited_rtentry() since the v9.0, so we cannot know what kind of accesses are required on the individual child relations. It needs the inherited relations/columns being labeled with same security label of their parent, because SE-PgSQL always makes same access control decision on same security labels. Thus, we want to check whether the relabeling operation breaks the uniqueness of security label within a certain inheritance tree, or not. Here is the logic to check relabeling on relation/column. http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/hooks.c#254 It checks two things. 1) The given relation/column must be the origin of inheritance tree when expected_parents = 0. 2) The given relation/column must not belong to multiple inheritance tree. So, the hook need to provide the expected_parents for each relations/columns. >>> 6. What are we doing about the assignment of initial security labels? >>> I had initially thought that perhaps new objects would just start out >>> unlabeled, and the user would be responsible for labeling them as >>> needed. But maybe we can do better. Perhaps we should extend the >>> security provider hook API with a function that gets called when a >>> labellable object gets created, and let each loaded security provider >>> return any label it would like attached. Even if we don't do that >>> now, esp_relabel_hook_entry needs to be renamed to something more >>> generic; we will certainly want to add more fields to that structure >>> later. >>> >> Starting with "unlabeled" is wrong, because it does not distinguish >> an object created by security sensitive users and insensitive users, >> for example. It is similar to relation's relowner is InvalidOid. >> >> I plan the security provider hook on the creation time works two things. >> 1. It checks user's privilege to create this object. >> 2. It returns security labels to be assigned. >> >> Then, the caller assigns these returned labels on the new object, >> if one or more valid labels are returned. > > OK, let's give that a try and see how it looks. I don't think that's > in this version of the patch, right? > Yes, this version of the patch didn't support labeling on creation time of database objects. It shall be added in separated patch. >>> 7. I think we need to write and include in the fine documentation some >>> "big picture" documentation about enhanced security providers. Of >>> course, we have to decide what we want to say. But the SECURITY LABEL >>> documentation is just kind of hanging out there in space right now; it >>> needs to connect to a broad introduction to the subject. >>> >> OK, I'll try to describe with appropriate granularity. >> Do we need an independent section in addition to the introduction of >> SECURITY LABEL syntax? > > I think so. I suggest a new chapter called "Enhanced Security > Providers" just after "Database Roles and Privileges". > OK, Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
* KaiGai Kohei (kaigai@kaigai.gr.jp) wrote: > Yep, rte->requiredPerms of inherited relations are cleared on the > expand_inherited_rtentry() since the v9.0, so we cannot know what > kind of accesses are required on the individual child relations. This is really a PG issue and decision, in my view. We're moving more and more towards a decision that inherited relations are really just the same relation but broken up per tables (ala "true" partitioning). As such, PG has chosen to view them as the same wrt permissions checking. I don't think we should make a different decision for security labels. If you don't want people who have access to the parent to have access to the children, then you shouldn't be making them children. Thanks, Stephen
(2010/08/15 9:16), Stephen Frost wrote: > * KaiGai Kohei (kaigai@kaigai.gr.jp) wrote: >> Yep, rte->requiredPerms of inherited relations are cleared on the >> expand_inherited_rtentry() since the v9.0, so we cannot know what >> kind of accesses are required on the individual child relations. > > This is really a PG issue and decision, in my view. We're moving more > and more towards a decision that inherited relations are really just the > same relation but broken up per tables (ala "true" partitioning). As > such, PG has chosen to view them as the same wrt permissions checking. > I don't think we should make a different decision for security labels. > If you don't want people who have access to the parent to have access to > the children, then you shouldn't be making them children. > No, what I want to do is people have identical access rights on both of the parent and children. If they have always same label, SE-PgSQL always makes same access control decision. This behavior is suitable to the standpoint that inherited relations are really just the same relation of the parent. For this purpose, I want to enforce a unique label on a certain inheritance tree. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
2010/8/14 KaiGai Kohei <kaigai@kaigai.gr.jp>: > (2010/08/15 9:16), Stephen Frost wrote: >> * KaiGai Kohei (kaigai@kaigai.gr.jp) wrote: >>> Yep, rte->requiredPerms of inherited relations are cleared on the >>> expand_inherited_rtentry() since the v9.0, so we cannot know what >>> kind of accesses are required on the individual child relations. >> >> This is really a PG issue and decision, in my view. We're moving more >> and more towards a decision that inherited relations are really just the >> same relation but broken up per tables (ala "true" partitioning). As >> such, PG has chosen to view them as the same wrt permissions checking. >> I don't think we should make a different decision for security labels. >> If you don't want people who have access to the parent to have access to >> the children, then you shouldn't be making them children. >> > No, what I want to do is people have identical access rights on both of > the parent and children. If they have always same label, SE-PgSQL always > makes same access control decision. This behavior is suitable to the > standpoint that inherited relations are really just the same relation > of the parent. For this purpose, I want to enforce a unique label on > a certain inheritance tree. This seems like a bad idea to me, too. I think it's arguable whether access to the children should be controlled by the parent's label or the child's label, but enforcing that the entire inheritance hierarchy is identically labeled seems like a pointless restriction. As Stephen points out, it's also wildly inconsistent with the way we currently handle it. There's also the problem that the hooks we're talking about here are inadequate to support such a restriction anyway. You'd need some kind of a hook in ALTER TABLE ... [NO] INHERIT, at a minimum. As has been mentioned many times before in reviewing many generations of SE-PostgreSQL patches, we're not going to get into the business of re-engineering our security architecture just because you would have designed it differently. Inventing something that's randomly different will not only make the code ugly and hard to maintain; it will also be confusing and difficult to manage for end-users. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
(2010/08/15 9:55), Robert Haas wrote: > 2010/8/14 KaiGai Kohei<kaigai@kaigai.gr.jp>: >> (2010/08/15 9:16), Stephen Frost wrote: >>> * KaiGai Kohei (kaigai@kaigai.gr.jp) wrote: >>>> Yep, rte->requiredPerms of inherited relations are cleared on the >>>> expand_inherited_rtentry() since the v9.0, so we cannot know what >>>> kind of accesses are required on the individual child relations. >>> >>> This is really a PG issue and decision, in my view. We're moving more >>> and more towards a decision that inherited relations are really just the >>> same relation but broken up per tables (ala "true" partitioning). As >>> such, PG has chosen to view them as the same wrt permissions checking. >>> I don't think we should make a different decision for security labels. >>> If you don't want people who have access to the parent to have access to >>> the children, then you shouldn't be making them children. >>> >> No, what I want to do is people have identical access rights on both of >> the parent and children. If they have always same label, SE-PgSQL always >> makes same access control decision. This behavior is suitable to the >> standpoint that inherited relations are really just the same relation >> of the parent. For this purpose, I want to enforce a unique label on >> a certain inheritance tree. > > This seems like a bad idea to me, too. I think it's arguable whether > access to the children should be controlled by the parent's label or > the child's label, but enforcing that the entire inheritance hierarchy > is identically labeled seems like a pointless restriction. As Stephen > points out, it's also wildly inconsistent with the way we currently > handle it. > > There's also the problem that the hooks we're talking about here are > inadequate to support such a restriction anyway. You'd need some kind > of a hook in ALTER TABLE ... [NO] INHERIT, at a minimum. As has been > mentioned many times before in reviewing many generations of > SE-PostgreSQL patches, we're not going to get into the business of > re-engineering our security architecture just because you would have > designed it differently. Inventing something that's randomly > different will not only make the code ugly and hard to maintain; it > will also be confusing and difficult to manage for end-users. > 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@ak.jp.nec.com>
KaiGai, * KaiGai Kohei (kaigai@ak.jp.nec.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@snowman.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.Grittner@wicourts.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@ak.jp.nec.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@ak.jp.nec.com>
* KaiGai Kohei (kaigai@ak.jp.nec.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@ak.jp.nec.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@ak.jp.nec.com>
Stephen Frost <sfrost@snowman.net> writes: > * KaiGai Kohei (kaigai@ak.jp.nec.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@snowman.net> writes: >> * KaiGai Kohei (kaigai@ak.jp.nec.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@ak.jp.nec.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@ak.jp.nec.com>
>Stephen Frost <sfrost@snowman.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
>Stephen Frost <sfrost@snowman.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
* Kevin Grittner (Kevin.Grittner@wicourts.gov) wrote: > >Stephen Frost <sfrost@snowman.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
On Tue, Aug 17, 2010 at 1:50 PM, Stephen Frost <sfrost@snowman.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@gmail.com) wrote: > On Tue, Aug 17, 2010 at 1:50 PM, Stephen Frost <sfrost@snowman.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@gmail.com> writes: > On Tue, Aug 17, 2010 at 1:50 PM, Stephen Frost <sfrost@snowman.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@snowman.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@ak.jp.nec.com>
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Robert Haas <robertmhaas@gmail.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@sss.pgh.pa.us) wrote: >> Robert Haas<robertmhaas@gmail.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@ak.jp.nec.com>
KaiGai, * KaiGai Kohei (kaigai@ak.jp.nec.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@ak.jp.nec.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@ak.jp.nec.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@ak.jp.nec.com>
2010/8/18 KaiGai Kohei <kaigai@ak.jp.nec.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@gmail.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@ak.jp.nec.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@snowman.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@ak.jp.nec.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@ak.jp.nec.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.c1023tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_tablename=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.c1023tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_columnname=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.c1023tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_tablename=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.c1023tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_columnname=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.c1023tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_tablename=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.c1023tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_columnname=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.c1023tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_tablename=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.c1023tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_columnname=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.c1023tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_tablename=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.c1023tcontext=system_u:object_r:sepgsql_table_t:s0 tclass=db_columnname=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.75rows=12 width=4) -> Append (cost=0.00..50.75 rows=12 width=4) -> Seq Scan on tbl_p (cost=0.00..25.38rows=6 width=4) Filter: (a = 150) -> Seq Scan on tbl_2 tbl_p (cost=0.00..25.38rows=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@ak.jp.nec.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@gmx.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@gmx.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@gmx.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@ak.jp.nec.com>
2010/8/25 KaiGai Kohei <kaigai@ak.jp.nec.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@ak.jp.nec.com>: >> The attached patches are revised ones, as follows. > > I think this is pretty good, and I'm generally in favor of committing > it. Some concerns: > > 1. Since nobody has violently objected to the comment.c refactoring > patch I recently proposed, I'm hopeful that can go in. And if that's > the case, then I'd prefer to see that committed first, and then rework > this to use that code. That would eliminate some code here, and it > would also make it much easier to support labels on other types of > objects. > > 2. Some of this code refers to "local" security labels. I'm not sure > what's "local" about them - aren't they just security labels? On a > related note, I don't like the trivial wrappers you have here, with > DeleteSecurityLabel around DeleteLocalSecLabel, SetSecurityLabel > around SetLocalSecLabel, etc. Just collapse these into a single set > of functions. > > 3. Is it really appropriate for ExecRelationSecLabel() to have an > "Exec" prefix? I don't think so. > > 4. Please get rid of the nkeys++ stuff in DeleteLocalSecLabel() and > just use fixed offsets as we do everywhere else. > > 5. Why do we think that the relabel hook needs to be passed the number > of expected parents? > > 6. What are we doing about the assignment of initial security labels? > I had initially thought that perhaps new objects would just start out > unlabeled, and the user would be responsible for labeling them as > needed. But maybe we can do better. Perhaps we should extend the > security provider hook API with a function that gets called when a > labellable object gets created, and let each loaded security provider > return any label it would like attached. Even if we don't do that > now, esp_relabel_hook_entry needs to be renamed to something more > generic; we will certainly want to add more fields to that structure > later. > > 7. I think we need to write and include in the fine documentation some > "big picture" documentation about enhanced security providers. Of > course, we have to decide what we want to say. But the SECURITY LABEL > documentation is just kind of hanging out there in space right now; it > needs to connect to a broad introduction to the subject. > > 8. Generally, the English in both the comments and documentation needs > work. However, we can address that problem when we're closer to > commit. > > I am going to mark this Returned with Feedback because I don't believe > it's realistic to get the comment code committed in the next week, > rework this patch, and then get this patch committed also. However, > I'm feeling pretty good about this effort and I think we're making > good progress toward getting this done. > -- KaiGai Kohei <kaigai@ak.jp.nec.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 thetable, 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} permissionon 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@ak.jp.nec.com>: >>> The attached patches are revised ones, as follows. >> >> I think this is pretty good, and I'm generally in favor of committing >> it. Some concerns: >> >> 1. Since nobody has violently objected to the comment.c refactoring >> patch I recently proposed, I'm hopeful that can go in. And if that's >> the case, then I'd prefer to see that committed first, and then rework >> this to use that code. That would eliminate some code here, and it >> would also make it much easier to support labels on other types of >> objects. >> >> 2. Some of this code refers to "local" security labels. I'm not sure >> what's "local" about them - aren't they just security labels? On a >> related note, I don't like the trivial wrappers you have here, with >> DeleteSecurityLabel around DeleteLocalSecLabel, SetSecurityLabel >> around SetLocalSecLabel, etc. Just collapse these into a single set >> of functions. >> >> 3. Is it really appropriate for ExecRelationSecLabel() to have an >> "Exec" prefix? I don't think so. >> >> 4. Please get rid of the nkeys++ stuff in DeleteLocalSecLabel() and >> just use fixed offsets as we do everywhere else. >> >> 5. Why do we think that the relabel hook needs to be passed the number >> of expected parents? >> >> 6. What are we doing about the assignment of initial security labels? >> I had initially thought that perhaps new objects would just start out >> unlabeled, and the user would be responsible for labeling them as >> needed. But maybe we can do better. Perhaps we should extend the >> security provider hook API with a function that gets called when a >> labellable object gets created, and let each loaded security provider >> return any label it would like attached. Even if we don't do that >> now, esp_relabel_hook_entry needs to be renamed to something more >> generic; we will certainly want to add more fields to that structure >> later. >> >> 7. I think we need to write and include in the fine documentation some >> "big picture" documentation about enhanced security providers. Of >> course, we have to decide what we want to say. But the SECURITY LABEL >> documentation is just kind of hanging out there in space right now; it >> needs to connect to a broad introduction to the subject. >> >> 8. Generally, the English in both the comments and documentation needs >> work. However, we can address that problem when we're closer to >> commit. >> >> I am going to mark this Returned with Feedback because I don't believe >> it's realistic to get the comment code committed in the next week, >> rework this patch, and then get this patch committed also. However, >> I'm feeling pretty good about this effort and I think we're making >> good progress toward getting this done. >> > > > > > -- KaiGai Kohei <kaigai@ak.jp.nec.com>
2010/9/13 KaiGai Kohei <kaigai@ak.jp.nec.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@ak.jp.nec.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@kaigai.gr.jp>
On Mon, Sep 13, 2010 at 8:38 AM, KaiGai Kohei <kaigai@kaigai.gr.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@kaigai.gr.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@ak.jp.nec.com>
Вложения
2010/9/14 KaiGai Kohei <kaigai@ak.jp.nec.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@gmail.com> wrote: > 2010/9/14 KaiGai Kohei <kaigai@ak.jp.nec.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@gmail.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@snowman.net> wrote: > * Robert Haas (robertmhaas@gmail.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@gmail.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@snowman.net> wrote: > * Robert Haas (robertmhaas@gmail.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@gmail.com> wrote: >> 2010/9/14 KaiGai Kohei<kaigai@ak.jp.nec.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@ak.jp.nec.com>
2010/9/23 KaiGai Kohei <kaigai@ak.jp.nec.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@gmail.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@ak.jp.nec.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@ak.jp.nec.com>
2010/9/23 KaiGai Kohei <kaigai@ak.jp.nec.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@ak.jp.nec.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@kaigai.gr.jp>
On Fri, Sep 24, 2010 at 8:54 AM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote: > (2010/09/24 20:56), Robert Haas wrote: >> >> 2010/9/23 KaiGai Kohei<kaigai@ak.jp.nec.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@kaigai.gr.jp> wrote: >> (2010/09/24 20:56), Robert Haas wrote: >>> >>> 2010/9/23 KaiGai Kohei<kaigai@ak.jp.nec.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@kaigai.gr.jp>
Вложения
On Sat, Sep 25, 2010 at 7:04 AM, KaiGai Kohei <kaigai@kaigai.gr.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@kaigai.gr.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@kaigai.gr.jp>
On Mon, Sep 27, 2010 at 4:05 AM, KaiGai Kohei <kaigai@kaigai.gr.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@gmail.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@metrosystems.co.jp> wrote: > On Mon, 27 Sep 2010 21:07:33 -0400 > Robert Haas <robertmhaas@gmail.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@gmail.com> wrote: > On Tue, Sep 28, 2010 at 3:57 AM, Shigeru HANADA > <hanada@metrosystems.co.jp> wrote: >> On Mon, 27 Sep 2010 21:07:33 -0400 >> Robert Haas <robertmhaas@gmail.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@gmail.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@gmx.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@gmail.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@gmail.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@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.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@gmail.com> wrote:
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.
On Tue, Sep 28, 2010 at 8:03 AM, Peter Eisentraut <peter_e@gmx.net> wrote:Injecting nonessential checks into the build process doesn't seem like
> On tis, 2010-09-28 at 13:53 +0200, Magnus Hagander wrote:
>> On Tue, Sep 28, 2010 at 13:50, Robert Haas <robertmhaas@gmail.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.
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.gurjeet@{ 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@metrosystems.co.jp> wrote: > > On Mon, 27 Sep 2010 21:07:33 -0400 > > Robert Haas <robertmhaas@gmail.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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + #!/bin/bash . traprm [ "$1" = "-q" ] && QUIET="Y" if [ ! -f COPYRIGHT ] then cd /pgtop || exit 1 fi chown -R postgres . # skip for <= PG 8.1, SGML has tabs if ! grep 'AC_INIT(\[PostgreSQL\]' configure.in | egrep -q '\[(6\.|7\.|8\.[01])' then echo "Checking SGML" cd doc/src/sgml gmake check > $TMP/0 2>&1 if grep -q 'Error' < $TMP/0 then echo "SGML error" cat $TMP/0 exit 1 fi gmake check-tabs # Run only at night to check for HISTORY build problems # in HISTORY.html. if [ ! -t 0 ] then gmake HISTORY.html > $TMP/0 2>&1 if grep -q 'Error' < $TMP/0 then echo "SGML error" cat $TMP/0 exit 1 fi fi # fails on /bin/sh cd - fi echo "Checking duplicate oids" cd src/include/catalog duplicate_oids > $TMP/0 if [ -s $TMP/0 ] then echo "Duplicate system oids" cat $TMP/0 exit 1 fi cd - # supress assembler warning (aspg /pg/tools/pgtest "$@"; echo "$?" > $TMP/ret) | # use only one grep so we don't buffer output egrep -v ': Warning: using `%|^SPI.c:.*: warning: |^ppport.h:[0-9][0-9]*: warning: |^/usr/libdata/perl5/5.00503/i386-bsdos/CORE/patchlevel.h|plperl.c:.*:warning: (implicit|passing)|variable .fast. might beclobbered|warning: unused variable .yyg.' rm -fr src/test/regress/tmp_check [ ! "$QUIET" ] && bell exit `cat $TMP/ret`