Thread: [PATCH] ACE Framework - Database, Schema
Stephen, The attached two patches are the first pieces of split out from the previous large access control reworks patch. The pgsql-ace-01-database-8.5devel-r2475.patch contains nigh security hooks related to global initialization and databases. The pgsql-ace-02-schema-8.5devel-r2475.patch contains the six security hooks related to schema objects. Note that these are not simple replacement for pg_xxx_aclcheck() and pg_xxx_ownercheck(). For example, DefineRelation() calls pg_namespace_aclcheck() with ACL_CREATE. This check shall be abstracted in the pgsql-ace-0x-relation patch, so I don't touch them yet. Also note that these patches don't support any security label. So, ace_xxx_create() is declared as void function, although it has to return a security label to be assigned. But these hooks are deployed on where we can easily support security label management, so later patch will fix it. The previous patch is too large to review. Is this scale confortable to review? $ diffstat pgsql-ace-01-database-8.5devel-r2475.patch backend/Makefile | 2 backend/catalog/aclchk.c | 68 +++++++! backend/commands/comment.c | 5 backend/commands/dbcommands.c | 154 +--------!!!!!!!!! backend/commands/indexcmds.c | 6 backend/security/Makefile | 10 + backend/security/ace/Makefile | 11 + backend/security/ace/ace_database.c | 285 ++++++++++++++++++++++++++++++++++++ backend/security/ace/ace_misc.c | 23 ++ backend/utils/adt/dbsize.c | 9 backend/utils/init/postinit.c | 17 !! include/security/ace.h | 39 ++++ 12 files changed, 445 insertions(+), 63 deletions(-), 121 modifications(!) $ diffstat pgsql-ace-02-schema-8.5devel-r2475.patch backend/catalog/aclchk.c | 15 +! backend/catalog/namespace.c | 42 ++---!! backend/commands/comment.c | 4 backend/commands/schemacmds.c | 57 -!!!!!!!!! backend/security/ace/Makefile | 2 backend/security/ace/ace_schema.c | 200 ++++++++++++++++++++++++++++++++++++++ backend/tcop/fastpath.c | 6 ! include/security/ace.h | 14 ++ 8 files changed, 234 insertions(+), 25 deletions(-), 81 modifications(!) -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
2009/12/13 KaiGai Kohei <kaigai@kaigai.gr.jp>: > The previous patch is too large to review. > Is this scale confortable to review? The scale is fine. But the content is not. So I am faced with a bit of a dilemma. If I start enumerating specific reasons why it's not OK, then it's going to take more time than I really want to put into this project. If I don't, then I may be accused of hiding the ball. What I'm hoping is that there are other interested people on this mailing list (or not on this mailing list, maybe in the security community) who have the time and the ability to help you fix some of the issues here so that we can then have a serious design discussion. Just to name a few really obvious problems (I only looked at the 01-database patch): 1. We have been talking for several days about the need to make the initial patch in this area strictly a code cleanup patch. Is this cleaner than the code that it is replacing? Is it even making an attempt to conform to that mandate? 2. What will happen when someone runs pgindent against this? Perhaps you only intended this to be a starting point for discussion, in which case that's fine, but I don't think I can really contribute anything useful until it gets a little further along. Thanks, ...Robert
I read through the database patch a little more. Here are some further thoughts. ace_database_create() calls have_createdb_privilege(), but of course what ace_database_create() is supposed to be checking is whether you have privileges to create the DB. This is extremely confusing. The calling sequence for ace_database_create() seems to indicate a failure of abstraction. The srcIsTemplate argument seems quite problematic. It is one of many values that are returned by get_db_info(), and it's passed to ace_database_create() because the current PG model happens to care. It seems like a near-certainty that future security models will care about something else. (Incidentally, the interface to the existing get_db_info function seems to me to be rather poor. By the time we get up to 10 out parameters, I think we ought to be using a structure. This might be material for a standalone patch.) I don't understand the modifications to restrict_and_check_grant(). It seems to me that this is going in the wrong direction, although since I don't understand the motivation for the change, I might be wrong. If we have a piece of code that centralizes permissions checking now, splitting that up into a bunch of ace_*_grant() functions and duplicating it doesn't seem like an improvement. ace_database_alter() appears to never be called with more than one argument that is non-NULL/InvalidOid, which is usually a sign that the interface is wrong. There are two calls to this function where, apparently, we're checking alter database permissions without specifying any details whatsoever about exactly what's going to get altered. That sounds like we don't really know what things we want to pass across the interface layer, so we're just passing the ones that we happen to care about at the moment. It certainly looks like we need separate functions for alter, rename, and alter-set. In some cases it might make sense to pass the parsenode as an argument rather than trying to decide which bits of data contained within it are sufficiently interesting to be worth sending over. Tom objected to ace_database_calculate_size() before. It seems to me that in order to keep this managable we have to settle on a finite set of permissions and stick with it. For example, in this case, we might decide that in EVERY security model, calculating the database size requires the same permissions as connect. The individual security models might want to make different decisions about how to check that permission, but they all have to treat it the same way they would treat an actual connection attempt. It seems to me that if every security model is allowed to introduce not only new logic for making checks but also new kinds of checks, we might as well give up on designing an interface layer. (Similarly, skipping back to ace_database_create() for a minute ... since I asked for a patch that only deals with one object type, I won't now complain about having gotten one. But I will note that I think where ace_database_create() calls pg_tablespace_aclchk() it probably eventually needs to call some ace_tablespace_...() function. Although frankly I'm not sure which one. Would ace_tablespace_create() mean permission to create a tablespace or permission to create tables within that tablespace?) security/ace.h, like a good number of other things in this patch, does not conform to project indentation style. All of the parameter blocks (parameter : description) don't either; unless I'm mistaken, pgindent will do something awful to those. The comments generally need some editing help from a native English speaker, and I spotted at least two typos (defatult for default, provides for providers). They also make reference to multiple security providers, which is not within the purview of this patch. I am not very happy with the ace_<object-type>_<operation> naming convention. Most PG functions are named a little more descriptively than that. Like I can pretty much guess that AlterDatabase() is going to alter a database, and get_db_info() is going to get info about a DB, and ExecHashJoin() is going to execute a hash join. It's not possible to look at ace_database_create() and have any idea what the function does, unless you have the preexisting knowledge that ACE stands for "access control extensions", and then you still don't know what the function does because "access control extensions database create" is a sentence with no verb. Granted, we have some poorly named functions in the system already, but I'd like to not increase the number. Apart from all of the above, I still believe that for a first phase of this we should just be looking to centralize access control decisions without promising to ever do anything more, and "access control extensions" doesn't send that message. I don't know exactly what the best way to structure this is but I think putting the word "check" somewhere in there would be a good start (or maybe the already-used-elsewhere abbreviation "chk"). Again, I want to provide what help I can here, but this is a massive project, so help is really needed. ...Robert
Robert, Thanks for your detailed comments.
> Just to name a few really obvious problems (I only looked at the
> 01-database patch):
>
> 1. We have been talking for several days about the need to make the
> initial patch in this area strictly a code cleanup patch. Is this
> cleaner than the code that it is replacing? Is it even making an
> attempt to conform to that mandate?
Even if it is unclear whether the current form is more clear than the
current inlined pg_xxx_aclcheck() form, or not, it will obviously
provide a set of common entry points for upcoming enhanced security
providers.
Eventually, it is more clear than enumeration of #ifdef ... #endif
blocks for SELinux, Smack, Solaris-TX and others.
> 2. What will happen when someone runs pgindent against this?
>
> Perhaps you only intended this to be a starting point for discussion,
> in which case that's fine, but I don't think I can really contribute
> anything useful until it gets a little further along.
If someone runs pgindent, I'll have to fix up a series of patches
mechanically to resolve conflictions, if necessary.
Indeed, this framework does not provide any additional user visible
features by itself, but necessary to accept upcoming anything useful.
Robert Haas wrote:
> I read through the database patch a little more. Here are some
> further thoughts.
>
> ace_database_create() calls have_createdb_privilege(), but of course
> what ace_database_create() is supposed to be checking is whether you
> have privileges to create the DB. This is extremely confusing. The
> calling sequence for ace_database_create() seems to indicate a failure
> of abstraction. The srcIsTemplate argument seems quite problematic.
> It is one of many values that are returned by get_db_info(), and it's
> passed to ace_database_create() because the current PG model happens
> to care. It seems like a near-certainty that future security models
> will care about something else. (Incidentally, the interface to the
> existing get_db_info function seems to me to be rather poor. By the
> time we get up to 10 out parameters, I think we ought to be using a
> structure. This might be material for a standalone patch.)
The have_createdb_privilege() is a copy & paste from the dbcommand.c.
It lookups AUTHOID syscache and returns pg_authid.rolcreatedb, but
indeed, its name is confusable. I'll consider another name.
The srcIsTemplate might be pulled out from DATABASEOID syscache
using the given srcDatOid. I guess the reason why get_db_info()
is it tries to resolve dbname -> db_oid and fetch its properties
in same time. But OID of the source database is already given,
so security provider (including the default PG) can lookup this
as neccessity.
> I don't understand the modifications to restrict_and_check_grant().
> It seems to me that this is going in the wrong direction, although
> since I don't understand the motivation for the change, I might be
> wrong. If we have a piece of code that centralizes permissions
> checking now, splitting that up into a bunch of ace_*_grant()
> functions and duplicating it doesn't seem like an improvement.
From perspective of the enhanced security providers, GRANT/REVOKE is
a kind of modification of properties on a certain database object.
So, it has a motivation to check GRANT/REVOKE using its access control
rules.
If we don't modify restrict_and_check_grant(), the caller (aclchk.c)
correctly handles the default PG checks, so rest of enhanced security
providers will apply additional checks in the ace_*_grant().
In this case, we keep ace_*_grant() empty at the moment, and it shall
be a special purpose hook for enhanced security providers.
I don't have any preference on either of them.
> ace_database_alter() appears to never be called with more than one
> argument that is non-NULL/InvalidOid, which is usually a sign that the
> interface is wrong. There are two calls to this function where,
> apparently, we're checking alter database permissions without
> specifying any details whatsoever about exactly what's going to get
> altered. That sounds like we don't really know what things we want to
> pass across the interface layer, so we're just passing the ones that
> we happen to care about at the moment. It certainly looks like we
> need separate functions for alter, rename, and alter-set. In some
> cases it might make sense to pass the parsenode as an argument rather
> than trying to decide which bits of data contained within it are
> sufficiently interesting to be worth sending over.
It makes sense for me. I'll separate it as follows:
- ace_database_alter_rename() ... ALTER DATABASE xxx RENAME TO
- ace_database_alter_owner() ... ALTER DATABASE xxx OWNER TO
- ace_database_alter() ... ALTER DATABASE with other options
When we support security label on database, it will also necessary:
- ace_database_alter_relabel()
> Tom objected to ace_database_calculate_size() before. It seems to me
> that in order to keep this managable we have to settle on a finite set
> of permissions and stick with it. For example, in this case, we might
> decide that in EVERY security model, calculating the database size
> requires the same permissions as connect. The individual security
> models might want to make different decisions about how to check that
> permission, but they all have to treat it the same way they would
> treat an actual connection attempt. It seems to me that if every
> security model is allowed to introduce not only new logic for making
> checks but also new kinds of checks, we might as well give up on
> designing an interface layer.
It againsts to the purpose of the framework. It provides a common set
of access controls for multiple security providers, but it does not
enforce the way to make its access control decision.
It seems to me ace_database_calculate_size() might be a bad name
because of too specific naming for a certin functionality.
We can also consider this hook checks permission to obtain attributes
of a certain database.
So, we can also generalize it as follows:
void ace_database_getattr(Oid datOid);
It checks permission to reference attributes (not contents) of a certain
database. The database size is a part of attribute, and the default PG
model checks ACL_CONNECT permission for the purpose.
Any other upcoming enhanced security provider may make its decision based
on different basis. But there is no matter.
> (Similarly, skipping back to ace_database_create() for a minute ...
> since I asked for a patch that only deals with one object type, I
> won't now complain about having gotten one. But I will note that I
> think where ace_database_create() calls pg_tablespace_aclchk() it
> probably eventually needs to call some ace_tablespace_...() function.
> Although frankly I'm not sure which one. Would
> ace_tablespace_create() mean permission to create a tablespace or
> permission to create tables within that tablespace?)
ace_tablespace_create() means permission to create a tablespace itself.
It shall be checked on CREATE TABLESPACE statement, not an alternative
of pg_tablespace_aclcheck(..., ACL_CREATE).
In the default PG model, it makes its access control decision based on
the following rules:- Current database user must has pg_authid.rolsuper or pg_authid.rolcreatedb- If not superuser,
currentdatabase user must have membership of the owner of the new database.- If source database is not template, the
currentdatabase user must have ownership of the new database.- If an explicit tablespace is given, the current
databaseuser must have CREATE privilege on the target tablespace.
It is the way to make access control decision in the default PG model.
However, any other enhanced security providers can apply thier rules.
E.g, SE-PgSQL tries to assign a default security label on a new database,
and checks permission to create a new database (db_database:{create})
between the client and the database with this label.
This framework provides a set of common entrypoints for various kind of
security providers, but does not enforce a certain security model.
> security/ace.h, like a good number of other things in this patch, does
> not conform to project indentation style. All of the parameter blocks
> (parameter : description) don't either; unless I'm mistaken, pgindent
> will do something awful to those. The comments generally need some
> editing help from a native English speaker, and I spotted at least two
> typos (defatult for default, provides for providers). They also make
> reference to multiple security providers, which is not within the
> purview of this patch.
I'll pay my efforts as possible as I can.
> I am not very happy with the ace_<object-type>_<operation> naming
> convention. Most PG functions are named a little more descriptively
> than that. Like I can pretty much guess that AlterDatabase() is going
> to alter a database, and get_db_info() is going to get info about a
> DB, and ExecHashJoin() is going to execute a hash join. It's not
> possible to look at ace_database_create() and have any idea what the
> function does, unless you have the preexisting knowledge that ACE
> stands for "access control extensions", and then you still don't know
> what the function does because "access control extensions database
> create" is a sentence with no verb. Granted, we have some poorly
> named functions in the system already, but I'd like to not increase
> the number. Apart from all of the above, I still believe that for a
> first phase of this we should just be looking to centralize access
> control decisions without promising to ever do anything more, and
> "access control extensions" doesn't send that message. I don't know
> exactly what the best way to structure this is but I think putting the
> word "check" somewhere in there would be a good start (or maybe the
> already-used-elsewhere abbreviation "chk").
OK. Indeed, it is not a verb.
However, the "check" does not means by itself what should be checked.
It may be permission, integrity, syntax or others.
What about "acechk_*" to mean access control extension checks <something>
to be <action>'ed? For example, acechk_database_create() means "access
control extension checks <database> to be <created>".
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
2009/12/13 KaiGai Kohei <kaigai@ak.jp.nec.com>: >> Just to name a few really obvious problems (I only looked at the >> 01-database patch): >> >> 1. We have been talking for several days about the need to make the >> initial patch in this area strictly a code cleanup patch. Is this >> cleaner than the code that it is replacing? Is it even making an >> attempt to conform to that mandate? > > Even if it is unclear whether the current form is more clear than the > current inlined pg_xxx_aclcheck() form, or not, it will obviously > provide a set of common entry points for upcoming enhanced security > providers. > Eventually, it is more clear than enumeration of #ifdef ... #endif > blocks for SELinux, Smack, Solaris-TX and others. Right, but it will also not get committed. :-( You seem to still be failing to understand the approach that Stephen and I have been discussing for the last several days... but at any rate, I think that it is possible to do this in a way that is more clear (or at least, AS clear) as the current method. Stephen is also working on a concept patch that I'm eager to see, and I would like to give him a little time to get that together before we spend too much more time working on this. >> I don't understand the modifications to restrict_and_check_grant(). >> It seems to me that this is going in the wrong direction, although >> since I don't understand the motivation for the change, I might be >> wrong. If we have a piece of code that centralizes permissions >> checking now, splitting that up into a bunch of ace_*_grant() >> functions and duplicating it doesn't seem like an improvement. > > From perspective of the enhanced security providers, GRANT/REVOKE is > a kind of modification of properties on a certain database object. > So, it has a motivation to check GRANT/REVOKE using its access control > rules. > If we don't modify restrict_and_check_grant(), the caller (aclchk.c) > correctly handles the default PG checks, so rest of enhanced security > providers will apply additional checks in the ace_*_grant(). > In this case, we keep ace_*_grant() empty at the moment, and it shall > be a special purpose hook for enhanced security providers. > I don't have any preference on either of them. [...] > When we support security label on database, it will also necessary: > > - ace_database_alter_relabel() This seems like bad design to me. I don't see why SE-Linux would get a vote on whether a user is allowed to change the DAC permissions, any more than DAC gets a vote on whether an SE-Linux relabel operation can go through. If it does, then this attempt to create a framework is not going to succeed, because every new provider will require hooks to let every exist provider muck with what it does, and they'll all have to have complete knowledge of each other's internals. > It seems to me ace_database_calculate_size() might be a bad name > because of too specific naming for a certin functionality. That's exactly the problem. I'm not sure if I believe in the particular solution you've offered here, but it's certainly better than the way it is now. I think this needs more thought from more people. > What about "acechk_*" to mean access control extension checks <something> > to be <action>'ed? For example, acechk_database_create() means "access > control extension checks <database> to be <created>". That fails to address all of my comments, so, -1 from me. ...Robert
Robert Haas wrote:
> 2009/12/13 KaiGai Kohei <kaigai@ak.jp.nec.com>:
>>> Just to name a few really obvious problems (I only looked at the
>>> 01-database patch):
>>>
>>> 1. We have been talking for several days about the need to make the
>>> initial patch in this area strictly a code cleanup patch. Is this
>>> cleaner than the code that it is replacing? Is it even making an
>>> attempt to conform to that mandate?
>> Even if it is unclear whether the current form is more clear than the
>> current inlined pg_xxx_aclcheck() form, or not, it will obviously
>> provide a set of common entry points for upcoming enhanced security
>> providers.
>> Eventually, it is more clear than enumeration of #ifdef ... #endif
>> blocks for SELinux, Smack, Solaris-TX and others.
>
> Right, but it will also not get committed. :-(
The framework will be necessary to get them committed.
Which is an egg, and which is a chicken? :-(
> You seem to still be failing to understand the approach that Stephen
> and I have been discussing for the last several days... but at any
> rate, I think that it is possible to do this in a way that is more
> clear (or at least, AS clear) as the current method. Stephen is also
> working on a concept patch that I'm eager to see, and I would like to
> give him a little time to get that together before we spend too much
> more time working on this.
Hmm... In my hope, I'd like to work on these patches during the Christmas
vacation terms for US people. So, it seems to me we need to decide the
direction in this week.
Stephen, sorry for the prod.
>>> I don't understand the modifications to restrict_and_check_grant().
>>> It seems to me that this is going in the wrong direction, although
>>> since I don't understand the motivation for the change, I might be
>>> wrong. If we have a piece of code that centralizes permissions
>>> checking now, splitting that up into a bunch of ace_*_grant()
>>> functions and duplicating it doesn't seem like an improvement.
>> From perspective of the enhanced security providers, GRANT/REVOKE is
>> a kind of modification of properties on a certain database object.
>> So, it has a motivation to check GRANT/REVOKE using its access control
>> rules.
>> If we don't modify restrict_and_check_grant(), the caller (aclchk.c)
>> correctly handles the default PG checks, so rest of enhanced security
>> providers will apply additional checks in the ace_*_grant().
>> In this case, we keep ace_*_grant() empty at the moment, and it shall
>> be a special purpose hook for enhanced security providers.
>> I don't have any preference on either of them.
> [...]
>> When we support security label on database, it will also necessary:
>>
>> - ace_database_alter_relabel()
>
> This seems like bad design to me. I don't see why SE-Linux would get
> a vote on whether a user is allowed to change the DAC permissions, any
> more than DAC gets a vote on whether an SE-Linux relabel operation can
> go through. If it does, then this attempt to create a framework is
> not going to succeed, because every new provider will require hooks to
> let every exist provider muck with what it does, and they'll all have
> to have complete knowledge of each other's internals.
From the DAC perspective, a relabel operation is setting an attribute
of a certain database, such as ALTER DATABASE ... SET xxx. So, it is
quite natural to check ownership of the target database like other
ALTER DATABASE stuff, not only SELinux relabeling checks.
I intend to use the *_alter_relabel() hook for any other label-based
security providers, not only SELinux. The caller, syntax support and
the default PG checks don't need to know details in any other security
providers. (It is also reason why I prefer one enhanced provider at
the same time, because they can share syntax support such as SECURITY
LABEL ('...') option.)
In my image, it shall be implemented as follows:
ALTER DATABASE <datname> SECURITY LABEL ( '<new label>' ); | V Value * ace_database_alter_relabel(Oid datOid,
Value*newLabel) { if (pg_database_ownercheck(datOid, GetUserId()) aclcheck_error(...);
switch (ace_provider) { #if HAVE_SELINUX case ACE_PROVIDER_SELINUX: if (sepgsql_is_enabled())
return sepgsql_database_alter_relabel(...); break; #endif #if HAVE_SMACK case ACE_PROVIDER_SMACK:
if(smack_is_enabled()) return smack_database_alter_relabel(...); break; #endif default:
break; }
ereport(ERROR, "No label-based MAC is now enabled"); return NULL; } | V The caller update pg_database
systemcatalog with the returned security label.
However, it is not necessary to conclude at the moment.
We can add it later.
>> It seems to me ace_database_calculate_size() might be a bad name
>> because of too specific naming for a certin functionality.
>
> That's exactly the problem. I'm not sure if I believe in the
> particular solution you've offered here, but it's certainly better
> than the way it is now. I think this needs more thought from more
> people.
From my previous large patch, the following functions might have similar
arguments:
* ac_relation_get_transaction_id() The currtid_byreloid() and currtid_byrelname() are the only caller, and checks
ACL_SELECTpermission on the given relation. We can consider its purpose is to check references to attribute of a
certainrelation.
* ac_relation_copy_definition() The transformInhRelation() is the only caller (name is confusable because it handles
LIKEclause in CREATE TABLE statement), and checks ACL_SELECT permission on the given relation. We can consider its
purposeis to check references to attribute of a certain relation.
-> These two checks apply identical permissions for similar purpose from more generalized perspective. It seems to me
thesecan be replaced by *_relation_getattr().
* ac_database_calculate_size() The calculate_database_size() is the only caller, and checks ACL_CONNECT permission on
thegiven database. We can consider its purpose is to return users attribute of a certain database.
-> ditto, can be replaced by *_database_getattr()
* ac_tablespace_calculate_size() The calculate_tablespace_size() is the only caller, and checks ACL_CREAET permission
onthe given tablespace. We can consider its purpose is to return users attribute of a certain tablespace.
-> ditto, can be replaced by *_tablespace_getattr()
>> What about "acechk_*" to mean access control extension checks <something>
>> to be <action>'ed? For example, acechk_database_create() means "access
>> control extension checks <database> to be <created>".
>
> That fails to address all of my comments, so, -1 from me.
OK, should be check_<object class>_<action>()?
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
2009/12/14 KaiGai Kohei <kaigai@ak.jp.nec.com>: > Robert Haas wrote: >> 2009/12/13 KaiGai Kohei <kaigai@ak.jp.nec.com>: >>>> Just to name a few really obvious problems (I only looked at the >>>> 01-database patch): >>>> >>>> 1. We have been talking for several days about the need to make the >>>> initial patch in this area strictly a code cleanup patch. Is this >>>> cleaner than the code that it is replacing? Is it even making an >>>> attempt to conform to that mandate? >>> Even if it is unclear whether the current form is more clear than the >>> current inlined pg_xxx_aclcheck() form, or not, it will obviously >>> provide a set of common entry points for upcoming enhanced security >>> providers. >>> Eventually, it is more clear than enumeration of #ifdef ... #endif >>> blocks for SELinux, Smack, Solaris-TX and others. >> >> Right, but it will also not get committed. :-( > > The framework will be necessary to get them committed. > Which is an egg, and which is a chicken? :-( We've been around that path a few times, but that's not my point here.Doing the framework first makes a lot of sense; theproblem is that we just had a design discussion regarding that framework and you've chosen to do something other than what was discussed. Did you not read that discussion? Did you not understand it? Unfortunately, what this project has turned into is a very long series of patch submissions that all basically have the same problems. The code is messy. It doesn't conform to project style. It embeds undiscussed design assumptions that the community does not endorse. It has poorly factored interfaces that may serve SE-PostgreSQL adequately for the moment but are likely to require constant rejiggering as new problems arise. It is filled with shims that don't accomplish anything useful and other places where useful shims are left out. Now, it may be that even if you respond to all of the comments and fix all of the issues that people are concerned about, the patch still won't get committed. As you know, Tom is very skeptical that the user-base for this feature is large enough to warrant the work that it will take. But my point is that you seem to be very deliberately not fixing those problems, and I don't see how we're going to make any progress that way. Many of the problems that I found in my review of this patch were covered in design discussions that happened this week, and yet the patch shows almost no evidence of those design discussions. Frankly, I find that kind of depressing. ...Robert
(2009/12/14 20:48), Robert Haas wrote: > 2009/12/14 KaiGai Kohei<kaigai@ak.jp.nec.com>: >> Robert Haas wrote: >>> 2009/12/13 KaiGai Kohei<kaigai@ak.jp.nec.com>: >>>>> Just to name a few really obvious problems (I only looked at the >>>>> 01-database patch): >>>>> >>>>> 1. We have been talking for several days about the need to make the >>>>> initial patch in this area strictly a code cleanup patch. Is this >>>>> cleaner than the code that it is replacing? Is it even making an >>>>> attempt to conform to that mandate? >>>> Even if it is unclear whether the current form is more clear than the >>>> current inlined pg_xxx_aclcheck() form, or not, it will obviously >>>> provide a set of common entry points for upcoming enhanced security >>>> providers. >>>> Eventually, it is more clear than enumeration of #ifdef ... #endif >>>> blocks for SELinux, Smack, Solaris-TX and others. >>> >>> Right, but it will also not get committed. :-( >> >> The framework will be necessary to get them committed. >> Which is an egg, and which is a chicken? :-( > > We've been around that path a few times, but that's not my point here. > Doing the framework first makes a lot of sense; the problem is that > we just had a design discussion regarding that framework and you've > chosen to do something other than what was discussed. Did you not > read that discussion? Did you not understand it? Please point out, if my understanding is incorrect from the discussion in a few days. * As a draft of the discussion, I have to split out the access control reworks patch in the 2nd CF per object classes. * This framework supports only the default PG privileges at the moment. * The way to host enhanced security providers are not decided. (Maybe #ifdef ... #endif block, Maybe function pointer) * It is not decided how many security labels are assigned on a database object. (Maybe 1:1, Maybe 1:n) I don't intend to go to something undecided, but, might understand something incorrectly or not be able to follow the discussion enough. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Mon, Dec 14, 2009 at 7:30 AM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote: > (2009/12/14 20:48), Robert Haas wrote: >> >> 2009/12/14 KaiGai Kohei<kaigai@ak.jp.nec.com>: >>> >>> Robert Haas wrote: >>>> >>>> 2009/12/13 KaiGai Kohei<kaigai@ak.jp.nec.com>: >>>>>> >>>>>> Just to name a few really obvious problems (I only looked at the >>>>>> 01-database patch): >>>>>> >>>>>> 1. We have been talking for several days about the need to make the >>>>>> initial patch in this area strictly a code cleanup patch. Is this >>>>>> cleaner than the code that it is replacing? Is it even making an >>>>>> attempt to conform to that mandate? >>>>> >>>>> Even if it is unclear whether the current form is more clear than the >>>>> current inlined pg_xxx_aclcheck() form, or not, it will obviously >>>>> provide a set of common entry points for upcoming enhanced security >>>>> providers. >>>>> Eventually, it is more clear than enumeration of #ifdef ... #endif >>>>> blocks for SELinux, Smack, Solaris-TX and others. >>>> >>>> Right, but it will also not get committed. :-( >>> >>> The framework will be necessary to get them committed. >>> Which is an egg, and which is a chicken? :-( >> >> We've been around that path a few times, but that's not my point here. >> Doing the framework first makes a lot of sense; the problem is that >> we just had a design discussion regarding that framework and you've >> chosen to do something other than what was discussed. Did you not >> read that discussion? Did you not understand it? > > Please point out, if my understanding is incorrect from the discussion > in a few days. > > * As a draft of the discussion, I have to split out the access control > reworks patch in the 2nd CF per object classes. > * This framework supports only the default PG privileges at the moment. > * The way to host enhanced security providers are not decided. > (Maybe #ifdef ... #endif block, Maybe function pointer) > * It is not decided how many security labels are assigned on a database > object. (Maybe 1:1, Maybe 1:n) > > I don't intend to go to something undecided, but, might understand > something incorrectly or not be able to follow the discussion enough. Hmm... all of those things are true, but it seems to leave quite a bit out. ...Robert
(2009/12/14 21:34), Robert Haas wrote: > On Mon, Dec 14, 2009 at 7:30 AM, KaiGai Kohei<kaigai@kaigai.gr.jp> wrote: >> (2009/12/14 20:48), Robert Haas wrote: >>> >>> 2009/12/14 KaiGai Kohei<kaigai@ak.jp.nec.com>: >>>> >>>> Robert Haas wrote: >>>>> >>>>> 2009/12/13 KaiGai Kohei<kaigai@ak.jp.nec.com>: >>>>>>> >>>>>>> Just to name a few really obvious problems (I only looked at the >>>>>>> 01-database patch): >>>>>>> >>>>>>> 1. We have been talking for several days about the need to make the >>>>>>> initial patch in this area strictly a code cleanup patch. Is this >>>>>>> cleaner than the code that it is replacing? Is it even making an >>>>>>> attempt to conform to that mandate? >>>>>> >>>>>> Even if it is unclear whether the current form is more clear than the >>>>>> current inlined pg_xxx_aclcheck() form, or not, it will obviously >>>>>> provide a set of common entry points for upcoming enhanced security >>>>>> providers. >>>>>> Eventually, it is more clear than enumeration of #ifdef ... #endif >>>>>> blocks for SELinux, Smack, Solaris-TX and others. >>>>> >>>>> Right, but it will also not get committed. :-( >>>> >>>> The framework will be necessary to get them committed. >>>> Which is an egg, and which is a chicken? :-( >>> >>> We've been around that path a few times, but that's not my point here. >>> Doing the framework first makes a lot of sense; the problem is that >>> we just had a design discussion regarding that framework and you've >>> chosen to do something other than what was discussed. Did you not >>> read that discussion? Did you not understand it? >> >> Please point out, if my understanding is incorrect from the discussion >> in a few days. >> >> * As a draft of the discussion, I have to split out the access control >> reworks patch in the 2nd CF per object classes. >> * This framework supports only the default PG privileges at the moment. >> * The way to host enhanced security providers are not decided. >> (Maybe #ifdef ... #endif block, Maybe function pointer) >> * It is not decided how many security labels are assigned on a database >> object. (Maybe 1:1, Maybe 1:n) >> >> I don't intend to go to something undecided, but, might understand >> something incorrectly or not be able to follow the discussion enough. > > Hmm... all of those things are true, but it seems to leave quite a bit out. Since I had to look many messages in a day, my concentration for each messages might not be enough. I'll try to check it again. At least, I don't intend to ignore our discussion. -- KaiGai Kohei <kaigai@kaigai.gr.jp>