Re: [HACKERS] Reworks for Access Control facilities (r2363)
KaiGai Kohei wrote: When we create a new object, we can provide an explicit security context to be assigned on the new object, instead of the default one. To get started, do we really need that feature? It would make for a significantly smaller patch if there was no explicit security labels on objects. On the other hand, the default PG model allows to bypass checks on certain objects. For example, column-level privileges are only checked when a user does not have enough permissions on the target table. If SELECT a,b FROM t is given, pg_attribute_aclcheck() may not invoked when user has needed privileges on the table t. Hmm, I see. Yes, it does seem like we'd need to change such permission checks to accommodate both models. I'm not clear why we need to rework the permission checks here. DAC and MAC perform orthogonally and independently. DAC allows to override column-level privileges by table-level privileges according to the default PG's model. It seems to me fine. On the other hand, MAC checks both of permissions. It is also fine. I meant we need to refactor the code doing the permission checks. The existing checks are doing the right thing for DAC, but as you point out, if the MAC checks are within pg_*_aclcheck() functions, pg_attribute_aclcheck() needs to be called even if you have privilege on the table. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
Heikki Linnakangas wrote: KaiGai Kohei wrote: When we create a new object, we can provide an explicit security context to be assigned on the new object, instead of the default one. To get started, do we really need that feature? It would make for a significantly smaller patch if there was no explicit security labels on objects. The importance of the feature is relatively minor than MAC itself. So, I can agree to omit code corresponding to statement support from the first patch. (IIRC, about 300-400 lines can be reduced.) But it will be necessary feature at the next step, because DBA cannot create a special purpose table without statement support. For example, if security policy allows DBA to create read-writable table (in default) and read-only table. He cannot set up read-only table without explicit security label support. On the other hand, the default PG model allows to bypass checks on certain objects. For example, column-level privileges are only checked when a user does not have enough permissions on the target table. If SELECT a,b FROM t is given, pg_attribute_aclcheck() may not invoked when user has needed privileges on the table t. Hmm, I see. Yes, it does seem like we'd need to change such permission checks to accommodate both models. I'm not clear why we need to rework the permission checks here. DAC and MAC perform orthogonally and independently. DAC allows to override column-level privileges by table-level privileges according to the default PG's model. It seems to me fine. On the other hand, MAC checks both of permissions. It is also fine. I meant we need to refactor the code doing the permission checks. The existing checks are doing the right thing for DAC, but as you point out, if the MAC checks are within pg_*_aclcheck() functions, pg_attribute_aclcheck() needs to be called even if you have privilege on the table. I think we already learned refactoring DAC checks need widespread code changes and pushes a burden to reviewers. In this case, I think the point just after invocation of ExecCheckRTEPerms() in ExecCheckRTPerms() is the best point to put SE-PgSQL's checks. Needless to say, its specification should be clearly documented. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
Heikki Linnakangas wrote:
KaiGai Kohei wrote:
1) creation of a database object
In SELinux model, when a user tries to create a new object (not limited
to database object, like a file or socket), a default security context
is assigned on the new object, then SELinux checks whether the user has
privileges to create a new object labeled with the security context, or not.
When we create a new table, the default PG model checks ACL_CREATE privilege
on the namespace which is supposed to own the new table. DefineRelation()
invokes pg_namespace_aclcheck() with OID of the namespace, but we cannot
see any properties of the new table from inside of pg_namespace_aclcheck().
It checks permissions on the couple of a user and a namespace.
On the other hand, SE-PG model follows the above principle. When we create
a new table, SE-PG compute a default security context to be assigned on,
then it checks the security policy whether the user is allowed to create
a new table labeled with the context, or not.
It checks permissions on the couple of a user and a new table itself.
I don't think I buy that argument. Can't we simply decide that in
PostgreSQL, the granularity is different, and you can only create
policies governing creation of objects on the basis of schema+user
combination, not on the properties of the new object. AFAICS it wouldn't
violate the principle of Mandatory Access Control.
No, it violates the principle.
I omitted a case for simplification of explanations.
When we create a new object, we can provide an explicit security context
to be assigned on the new object, instead of the default one.
In this case, SELinux checks privilege to create the object with the
given security context. (If it is disallowed, this creation will be
failed.)
If we check MAC permission to create a new object based on a couple
of user and schema which owns the new one, it also allows users to
create a new object with arbitrary security context, because this
check is not applied on security context of the new object itself.
It is a reason why SELinux is MAC. It never allows to create a new
object with a violated security context. The only way to control
this policy is to check privileges on the pair of user and the new
object. Thus, SELinux defines its permission to create a new object
on various kind of objects; not limited to database objects such
as files, sockets, IPC, x-window and so on.
2) AND-condition for all the privileges
When a certain action requires multiple permissions at one time,
the principle of SELinux is that all the permissions have to be checked.
If one of them is not allowed, it disallows the required action.
In other word, all the conditions are chained by AND.
This principle enables us to analyze the data flows between users and
resources with the security policy, without implementation details.
If a certain permission (e.g db_table:{select}) can override any other
permission (e.g db_column:{select}), it also implicitly means a possibility
of infotmation leaks/manipulations, even if the security policy said this
user cannot read a data from the column.
On the other hand, the default PG model allows to bypass checks on
certain objects. For example, column-level privileges are only checked
when a user does not have enough permissions on the target table.
If SELECT a,b FROM t is given, pg_attribute_aclcheck() may not invoked
when user has needed privileges on the table t.
Hmm, I see. Yes, it does seem like we'd need to change such permission
checks to accommodate both models.
I'm not clear why we need to rework the permission checks here.
DAC and MAC perform orthogonally and independently.
DAC allows to override column-level privileges by table-level privileges
according to the default PG's model. It seems to me fine.
On the other hand, MAC checks both of permissions. It is also fine.
3) superuser is not an exception of access control.
It is the similar issue to the 2).
Yeah.
The following code is a part of AlterFunctionOwner_internal().
/* Superusers can always do it */
if (!superuser())
{
/* Otherwise, must be owner of the existing object */
if (!pg_proc_ownercheck(procOid, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC,
NameStr(procForm-proname));
/* Must be able to become new owner */
check_is_member_of_role(GetUserId(), newOwnerId);
/* New owner must have CREATE privilege on namespace */
aclresult = pg_namespace_aclcheck(procForm-pronamespace,
newOwnerId,
ACL_CREATE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
get_namespace_name(procForm-pronamespace));
}
From perspective of the default PG model, this code
Re: [HACKERS] Reworks for Access Control facilities (r2363)
Robert Haas wrote: On Sat, Oct 17, 2009 at 9:53 AM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: This raises an important point: We need *developer documentation* on how to write SE-Pgsql compliant permission checks. Not only for authors of 3rd party modules but for developers of PostgreSQL itself. Point 2) above needs to be emphasized, it's a big change in the way permission checks have to be programmed. One that I hadn't realized before. I haven't been paying much attention, but neither is most other developers, so we need clear documentation. This is a good point. All throughout these discussions, there has been a concern that whatever is implemented here will be unmaintainable because we don't have any committers who are familiar with the ins and outs of SE-Linux and MAC (and not too many other community members interested in the topic, either). So some developer documentation seems like it might help. On the other hand, KaiGai has made several attempts at documentation and several attempts at patches and we're not really any closer to having SE-PostgreSQL in core than we were a year ago. I think that's partly because KaiGai tried to bite off far too much initially (still?), partly because of technical problems with the patches, partly because the intersection of people who are experts in PostgreSQL and people who are experts in MAC seems to be empty, and partly because, as much as people sorta kinda like this feature, nobody other than KaiGai has really been willing to step up and pour into this project the kind of resources that it will likely require to be successful. I have to admit that I'm kind of giving up hope. We seem to be going in circles, and I don't think anything new is being said on this thread that hasn't been said before. We may not be always able to find out the right way to the mountain summit. Indeed, it seems that we returned to the original design which deploys SE-PgSQL's hooks on the strategic points. But there is a significant improvement. We learned several designs which we already tried were on the rocky path, although they look like an easy path at first. I agrre to the Heikki's suggestion. Not only user documentation, we need another documentation from the viewpoint of developer, which describes what permissions are defined, what is the purpose of SE-PgSQL's hooks and when/where these are called. Thanks, BTW, as I noted in the last message, I have to allocate my activities to Japan Linux Symposium in this week. So, responses may delay, Sorry. http://events.linuxfoundation.org/events/japan-linux-symposium/schedule -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
KaiGai Kohei wrote:
1) creation of a database object
In SELinux model, when a user tries to create a new object (not limited
to database object, like a file or socket), a default security context
is assigned on the new object, then SELinux checks whether the user has
privileges to create a new object labeled with the security context, or not.
When we create a new table, the default PG model checks ACL_CREATE privilege
on the namespace which is supposed to own the new table. DefineRelation()
invokes pg_namespace_aclcheck() with OID of the namespace, but we cannot
see any properties of the new table from inside of pg_namespace_aclcheck().
It checks permissions on the couple of a user and a namespace.
On the other hand, SE-PG model follows the above principle. When we create
a new table, SE-PG compute a default security context to be assigned on,
then it checks the security policy whether the user is allowed to create
a new table labeled with the context, or not.
It checks permissions on the couple of a user and a new table itself.
I don't think I buy that argument. Can't we simply decide that in
PostgreSQL, the granularity is different, and you can only create
policies governing creation of objects on the basis of schema+user
combination, not on the properties of the new object. AFAICS it wouldn't
violate the principle of Mandatory Access Control.
2) AND-condition for all the privileges
When a certain action requires multiple permissions at one time,
the principle of SELinux is that all the permissions have to be checked.
If one of them is not allowed, it disallows the required action.
In other word, all the conditions are chained by AND.
This principle enables us to analyze the data flows between users and
resources with the security policy, without implementation details.
If a certain permission (e.g db_table:{select}) can override any other
permission (e.g db_column:{select}), it also implicitly means a possibility
of infotmation leaks/manipulations, even if the security policy said this
user cannot read a data from the column.
On the other hand, the default PG model allows to bypass checks on
certain objects. For example, column-level privileges are only checked
when a user does not have enough permissions on the target table.
If SELECT a,b FROM t is given, pg_attribute_aclcheck() may not invoked
when user has needed privileges on the table t.
Hmm, I see. Yes, it does seem like we'd need to change such permission
checks to accommodate both models.
3) superuser is not an exception of access control.
It is the similar issue to the 2).
Yeah.
The following code is a part of AlterFunctionOwner_internal().
/* Superusers can always do it */
if (!superuser())
{
/* Otherwise, must be owner of the existing object */
if (!pg_proc_ownercheck(procOid, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC,
NameStr(procForm-proname));
/* Must be able to become new owner */
check_is_member_of_role(GetUserId(), newOwnerId);
/* New owner must have CREATE privilege on namespace */
aclresult = pg_namespace_aclcheck(procForm-pronamespace,
newOwnerId,
ACL_CREATE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
get_namespace_name(procForm-pronamespace));
}
From perspective of the default PG model, this code perfectly correct.
Both of pg_proc_ownercheck() and pg_namespace_aclcheck() always returns
ACLCHECK_OK, so these invocations are bypassable.
However, if SE-PG's hooks are deployed on pg_xxx_aclcheck() routines,
it means that we cannot check correct MAC permissions when a client is
allowed to apply superuser privilege.
Please remind that SELinux requires AND-condition for all the privileges
required to a certain action. When a root user tries to read a certain
file without DAC permisions, it requires both of capability:{dac_override}
and file:{read} permissions in operating system.
We need to ask ourselves, is that a realistic goal, given how widespread
such if (superuser()) calls are? And more imporantly, unless you
sprinkle additional fine-grained permission checks to all the places
that currently just check if (superuser()), it will be possible to
circumvent the system with LOAD or any of the other commands that are
inherently dangerous. We don't want such additional fine-grained
permissions, not for now at least.
Seems a lot simpler and also easier to understand if there's a single
superuser privilege that trumps all other permission checks.
If we look at the SE-PgSQL project on the greater scale, it also can be
considered as an efforts to add MAC checks on the module which applied
its own access controls, but bypassed
Re: [HACKERS] Reworks for Access Control facilities (r2363)
On Sat, Oct 17, 2009 at 9:53 AM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: This raises an important point: We need *developer documentation* on how to write SE-Pgsql compliant permission checks. Not only for authors of 3rd party modules but for developers of PostgreSQL itself. Point 2) above needs to be emphasized, it's a big change in the way permission checks have to be programmed. One that I hadn't realized before. I haven't been paying much attention, but neither is most other developers, so we need clear documentation. This is a good point. All throughout these discussions, there has been a concern that whatever is implemented here will be unmaintainable because we don't have any committers who are familiar with the ins and outs of SE-Linux and MAC (and not too many other community members interested in the topic, either). So some developer documentation seems like it might help. On the other hand, KaiGai has made several attempts at documentation and several attempts at patches and we're not really any closer to having SE-PostgreSQL in core than we were a year ago. I think that's partly because KaiGai tried to bite off far too much initially (still?), partly because of technical problems with the patches, partly because the intersection of people who are experts in PostgreSQL and people who are experts in MAC seems to be empty, and partly because, as much as people sorta kinda like this feature, nobody other than KaiGai has really been willing to step up and pour into this project the kind of resources that it will likely require to be successful. I have to admit that I'm kind of giving up hope. We seem to be going in circles, and I don't think anything new is being said on this thread that hasn't been said before. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
KaiGai Kohei wrote: The purpose of this patch is to provide function entrypoints for the upcoming SE-PostgreSQL feature, because I got a few comments that we hesitate to put sepgsql_xxx() hooks on the main routines directly in the first commit fest. In addition, I already tried to put SE-PG hooks within pg_xxx_aclchecks() in this CF, but it was failed due to the differences in the security models. Can you elaborate that? It might well be that you need to adapt the SE-PostgreSQL security model to the one that's there already. Putting SE-PG hooks into existing pg_xxx_aclcheck functions is the only low-impact way I can see to implement SE-PostgreSQL. * There are two special-purpose shims, ac_database_calculate_size and ac_tablespace_calculate_size, that got added for the benefit of utils/adt/dbsize.c. What if that code were still in contrib? How is it different from a lot of the code that is in contrib now, eg dblink or pgrowlocks, to say nothing of third-party modules? Presuming that the shim layer can know explicitly about each individual permission-checking requirement is a dead-end design. Back to the definition of access controls (or reference monitor). It prevents violated accesses launched by user's requests (SQL). It is not a job to protect something from malicious internal modules. The issue isn't malicious modules, but modules that have pg_xxx_aclcheck calls in them and haven't been modified to do SE-pgsql checks like you modified all the backend code. As the patch stands, they would perform just the regular acl checks and bypass SE-pgsql. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
2009/10/16 KaiGai Kohei kai...@ak.jp.nec.com: . In addition, I already tried to put SE-PG hooks within pg_xxx_aclchecks() in this CF, but it was failed due to the differences in the security models. I thought the last discussion ended with a pretty strong conclusion that we didn't want differences in the security models. The first step is to add hooks which don't change the security model at all, just allow people to control the existing checks from their SE configuration. Only as a second step we would look into making incremental changes to the postgres security model to add support for privileges SE users might expect to find, eventually possibly including per-row permissions. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
Greg Stark gsst...@mit.edu writes: The first step is to add hooks which don't change the security model at all, just allow people to control the existing checks from their SE configuration. This is in fact what the presented patch is meant to do. The issue is about whether the hook placement is sane/useful/extensible. The main problem I've got with the design is that it doesn't appear to work for privilege checks made by add-on modules. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
On Fri, Oct 16, 2009 at 12:45 PM, Greg Stark gsst...@mit.edu wrote: 2009/10/16 KaiGai Kohei kai...@ak.jp.nec.com: . In addition, I already tried to put SE-PG hooks within pg_xxx_aclchecks() in this CF, but it was failed due to the differences in the security models. I thought the last discussion ended with a pretty strong conclusion that we didn't want differences in the security models. The first step is to add hooks which don't change the security model at all, just allow people to control the existing checks from their SE configuration. Only as a second step we would look into making incremental changes to the postgres security model to add support for privileges SE users might expect to find, eventually possibly including per-row permissions. I think we sort of came to the conclusion that even a basic implementation of SE-PostgreSQL might have some requirements that didn't quite square with the existing PostgreSQL security model. The charter of this patch AIUI was to refactor things so that they were square up, but I the patch is substantially more complex and invasive than what I thought would be necessary and it's not clear that it solves the problem. Rather than refactoring the existing checks to provide a cleaner abstraction layer, it seems to provide a layer that, if it's anything, is just a place-holder for an SE-PostgreSQL implementation, and there's no guarantee that it's adequate even for that, much less for anything else we might want to do. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
Heikki Linnakangas wrote:
KaiGai Kohei wrote:
The purpose of this patch is to provide function entrypoints for the
upcoming SE-PostgreSQL feature, because I got a few comments that we
hesitate to put sepgsql_xxx() hooks on the main routines directly in
the first commit fest. In addition, I already tried to put SE-PG hooks
within pg_xxx_aclchecks() in this CF, but it was failed due to the
differences in the security models.
Can you elaborate that? It might well be that you need to adapt the
SE-PostgreSQL security model to the one that's there already. Putting
SE-PG hooks into existing pg_xxx_aclcheck functions is the only
low-impact way I can see to implement SE-PostgreSQL.
We can show several examples that pg_xxx_aclcheck() routines are not
suitable to implement SELinux's security model.
Please note that it is not a defect of the default PG's security model
needless to say. It is just a different in standpoints.
1) creation of a database object
In SELinux model, when a user tries to create a new object (not limited
to database object, like a file or socket), a default security context
is assigned on the new object, then SELinux checks whether the user has
privileges to create a new object labeled with the security context, or not.
When we create a new table, the default PG model checks ACL_CREATE privilege
on the namespace which is supposed to own the new table. DefineRelation()
invokes pg_namespace_aclcheck() with OID of the namespace, but we cannot
see any properties of the new table from inside of pg_namespace_aclcheck().
It checks permissions on the couple of a user and a namespace.
On the other hand, SE-PG model follows the above principle. When we create
a new table, SE-PG compute a default security context to be assigned on,
then it checks the security policy whether the user is allowed to create
a new table labeled with the context, or not.
It checks permissions on the couple of a user and a new table itself.
The caller does not provide enough information to the pg_xxx_aclcheck(),
so we decided to create an abstraction layer which can provide enough
informations to both of security models. Then, the ac_xxx_*() routines
were implemented.
2) AND-condition for all the privileges
When a certain action requires multiple permissions at one time,
the principle of SELinux is that all the permissions have to be checked.
If one of them is not allowed, it disallows the required action.
In other word, all the conditions are chained by AND.
This principle enables us to analyze the data flows between users and
resources with the security policy, without implementation details.
If a certain permission (e.g db_table:{select}) can override any other
permission (e.g db_column:{select}), it also implicitly means a possibility
of infotmation leaks/manipulations, even if the security policy said this
user cannot read a data from the column.
On the other hand, the default PG model allows to bypass checks on
certain objects. For example, column-level privileges are only checked
when a user does not have enough permissions on the target table.
If SELECT a,b FROM t is given, pg_attribute_aclcheck() may not invoked
when user has needed privileges on the table t.
3) superuser is not an exception of access control.
It is the similar issue to the 2).
The following code is a part of AlterFunctionOwner_internal().
/* Superusers can always do it */
if (!superuser())
{
/* Otherwise, must be owner of the existing object */
if (!pg_proc_ownercheck(procOid, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC,
NameStr(procForm-proname));
/* Must be able to become new owner */
check_is_member_of_role(GetUserId(), newOwnerId);
/* New owner must have CREATE privilege on namespace */
aclresult = pg_namespace_aclcheck(procForm-pronamespace,
newOwnerId,
ACL_CREATE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
get_namespace_name(procForm-pronamespace));
}
From perspective of the default PG model, this code perfectly correct.
Both of pg_proc_ownercheck() and pg_namespace_aclcheck() always returns
ACLCHECK_OK, so these invocations are bypassable.
However, if SE-PG's hooks are deployed on pg_xxx_aclcheck() routines,
it means that we cannot check correct MAC permissions when a client is
allowed to apply superuser privilege.
Please remind that SELinux requires AND-condition for all the privileges
required to a certain action. When a root user tries to read a certain
file without DAC permisions, it requires both of capability:{dac_override}
and file:{read} permissions in operating system.
These are a part of reasons why we had to design such a large patch.
If we stick around the common entrypoints of access
Re: [HACKERS] Reworks for Access Control facilities (r2363)
Greg Stark wrote: 2009/10/16 KaiGai Kohei kai...@ak.jp.nec.com: . In addition, I already tried to put SE-PG hooks within pg_xxx_aclchecks() in this CF, but it was failed due to the differences in the security models. I thought the last discussion ended with a pretty strong conclusion that we didn't want differences in the security models. It is not a fact. Because the SE-PG patch is a bit large to review, I got a suggestion to implement a part of permissions checks which can be invoked from the pg_xxx_aclcheck() without any breaks for SELinux's security model, at the first step. In other word, I tried to implement only union part of the security models. The first step is to add hooks which don't change the security model at all, just allow people to control the existing checks from their SE configuration. Only as a second step we would look into making incremental changes to the postgres security model to add support for privileges SE users might expect to find, eventually possibly including per-row permissions. I already did it on the first CF... However, most of permission checks had gone at the first step. It was commented it is same as checks nothing. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
KaiGai Kohei kai...@ak.jp.nec.com writes: [ patch r2363 ] I promised I would review this today, but I just can't make myself do it in any detail. This is too large, too ugly, and it is going in a direction that I do not like or want to spend any of my time on. The overwhelming impression after a brief read-through is that the code has been hacked apart with a chainsaw and reassembled into a Frankenstein's monster --- it's alive, but man is it ugly. Code comments that refer to something above or below are still there, but the referent is no longer close enough for that to be a reasonable way of referring to it. It's impossible to follow what's going on or why, either in the shim functions or in the callers --- in the original coding there was context for the aclcheck calls, now there isn't. I don't have any confidence that this is a sane way to proceed forward. The shim layer knows everything about everything --- there may still be a few backend .h files it doesn't include, but that's not for lack of trying. The direction this is heading in is an unmaintainable giant-bowl-of-spaghetti security module, rather than something that can be divided into understandable parts. And I don't think it's really removed any complexity from the callers, nor do I believe that it's going to be a useful basis for imposing a different security policy than the one we have now. Two specific examples of why not: * The skip permissions checks arguments that have been added to various random functions suggest strongly that the factoring still isn't right --- I especially don't believe in that in the context of performDeletion and friends. * There are two special-purpose shims, ac_database_calculate_size and ac_tablespace_calculate_size, that got added for the benefit of utils/adt/dbsize.c. What if that code were still in contrib? How is it different from a lot of the code that is in contrib now, eg dblink or pgrowlocks, to say nothing of third-party modules? Presuming that the shim layer can know explicitly about each individual permission-checking requirement is a dead-end design. Maybe if I weren't burned out after a month of CommitFesting, I could muster a more positive reaction, but right now I just can't summon any enthusiasm for this. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
Tom Lane wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: [ patch r2363 ] I promised I would review this today, but I just can't make myself do it in any detail. This is too large, too ugly, and it is going in a direction that I do not like or want to spend any of my time on. The overwhelming impression after a brief read-through is that the code has been hacked apart with a chainsaw and reassembled into a Frankenstein's monster --- it's alive, but man is it ugly. Code comments that refer to something above or below are still there, but the referent is no longer close enough for that to be a reasonable way of referring to it. It's impossible to follow what's going on or why, either in the shim functions or in the callers --- in the original coding there was context for the aclcheck calls, now there isn't. Quite frankly, I felt disappointed that we have to repeat these kind of design level issues again. :-( The purpose of this patch is to provide function entrypoints for the upcoming SE-PostgreSQL feature, because I got a few comments that we hesitate to put sepgsql_xxx() hooks on the main routines directly in the first commit fest. In addition, I already tried to put SE-PG hooks within pg_xxx_aclchecks() in this CF, but it was failed due to the differences in the security models. Then, we made a direction to add an abstraction layer for the purpose of access controls which can be available both of DAC and MAC. Apart from this patch, we need to consider the preferable way to host additional security models in PostgreSQL again. I don't have any confidence that this is a sane way to proceed forward. Indeed, ac_xxx_*() routines needs a large scale changes for the core. However, we didn't have any other way, if both of security model have to use common entry points. In the original design, I put sepgsqlCheckXXX() hooks which does not affect anything if disabled on the main routines, and it works well. I would like to consider why reviewers felt these hooks are (possibly) hard to maintain again. One reason was the hooks reflected individual SELinux permissions. e.g) sepgsqlCheckProcedureInstall(Oid procOid); It checks user's privilege to use a certain function as a system internal stuff which is executed on runtime without individual execution permission checks, like an implementation of conversion. However, it may need future contributors to understand the intention why the hooks were deployed here, without enough knowledge about SELinux. In other word, the hooks represented how SELinux makes its decision (method), not what SELinux make its decision on (purpose). Instead of this ac_xxx_*() routines and previous sepgsqlCheckXXX() routines, I would like to propose SE-PG hooks which reflects the purpose of security checks. e.g) sepgsql_relation_create(char *relName, Oid namespace_oid, ...); It internally compute the default security context of the new table and checks permission on the table itself and the namespace to be created on. The series of checks consists of a permission check to create a new table in totally. I think it is not a major issue whether this patch is applied, or not. What is important is to point out the right direction to host SELinux security model correctly. At the PGcon2008 keynote, Bruce talked that our road to the summit is similar to a bendy road. It means our development does not always go into the right direction, but we are certainly getting near to the summit. I've tried several approaches for more than two years, but I cannot feel we are getting near to the summit yet. At least, we need to decide where we should go on the next at the end of this commit fest. The shim layer knows everything about everything --- there may still be a few backend .h files it doesn't include, but that's not for lack of trying. The direction this is heading in is an unmaintainable giant-bowl-of-spaghetti security module, rather than something that can be divided into understandable parts. And I don't think it's really removed any complexity from the callers, nor do I believe that it's going to be a useful basis for imposing a different security policy than the one we have now. Two specific examples of why not: * The skip permissions checks arguments that have been added to various random functions suggest strongly that the factoring still isn't right --- I especially don't believe in that in the context of performDeletion and friends. The reason why we needed to put permission checks on the routines in dependency.c is that we cannot know what objects are dropped due to the cascaded deletion. But some of purely internal stuffs (such as cleaning up temporary objects) uses the routines without any necessity of permission checks. So, the flag to control permission check is necessary. * There are two special-purpose shims, ac_database_calculate_size and ac_tablespace_calculate_size, that got added for the benefit of utils/adt/dbsize.c. What if
Re: [HACKERS] Reworks for Access Control facilities (r2363)
On Thu, Oct 15, 2009 at 1:22 PM, Tom Lane t...@sss.pgh.pa.us wrote: Maybe if I weren't burned out after a month of CommitFesting, I could muster a more positive reaction, but right now I just can't summon any enthusiasm for this. Based on this review, I am marking this patch Rejected. For what it's worth, I took a quick look at this just to see if I had any reason to disagree with your conclusions. I don't. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
Robert Haas wrote: On Thu, Oct 15, 2009 at 1:22 PM, Tom Lane t...@sss.pgh.pa.us wrote: Maybe if I weren't burned out after a month of CommitFesting, I could muster a more positive reaction, but right now I just can't summon any enthusiasm for this. Based on this review, I am marking this patch Rejected. Basically, I need to agree in spite of Stephen's efforts. For what it's worth, I took a quick look at this just to see if I had any reason to disagree with your conclusions. I don't. Sorry, please make clear the your conclusions? Does it mean that Tom's comment that this reworking does not go into the right direction? Or, my comment on the last message? Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
2009/10/13 KaiGai Kohei kai...@ak.jp.nec.com: The attached patch is a revised one with the following updates: Despite two fairly explicit requests, this patch (and, with the exception of ECPG, only this patch) has not yet been reviewed by a committer. http://archives.postgresql.org/pgsql-hackers/2009-10/msg00591.php http://archives.postgresql.org/pgsql-hackers/2009-10/msg00652.php Are any of the committers willing to take a look at this? Tom? Alvaro, maybe? Bruce? ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
Robert Haas robertmh...@gmail.com writes: Are any of the committers willing to take a look at this? Tom? I do plan to look at it tomorrow. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
Robert Haas wrote: 2009/10/13 KaiGai Kohei kai...@ak.jp.nec.com: The attached patch is a revised one with the following updates: Despite two fairly explicit requests, this patch (and, with the exception of ECPG, only this patch) has not yet been reviewed by a committer. http://archives.postgresql.org/pgsql-hackers/2009-10/msg00591.php http://archives.postgresql.org/pgsql-hackers/2009-10/msg00652.php Are any of the committers willing to take a look at this? Tom? Alvaro, maybe? Bruce? In actually, I cannot believe this patch to be perfectly commitable by the 15-Oct due to the remaining time, but it is necessary to be comittable at the head of the next commit fest. In other word, I strongly want to continue the discussion and revising the patch, even if it will be actually commited at the 15-Nov. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reworks for Access Control facilities (r2363)
On Wed, Oct 14, 2009 at 9:19 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Are any of the committers willing to take a look at this? Tom? I do plan to look at it tomorrow. Oh, great. You've done an impressive job slogging through a bunch of big, complex patches in the last week. Thanks, ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
