pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[PATCH] Reworks for Access Control facilities (r2251)
50+ messages / 8 participants
[nested] [flat]
* [PATCH] Reworks for Access Control facilities (r2251)
@ 2009-08-25 02:10 KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 1 reply; 50+ messages in thread
From: KaiGai Kohei @ 2009-08-25 02:10 UTC (permalink / raw)
To: sfrost@snowman.net; +Cc: pgsql-hackers
The attached patch reworks access control facilities in PostgreSQL.
The current implementation does not have well separation in what
to be controled and how to be controled. For example, when we create
a new table, it requires users ACL_CREATE on the namespace and
ACL_CREATE on the tablespace if necessary. These checks are methods
to control whether he can create a new table, or not.
This patch provides an abstraction layer of access controls to
separate what to be controlsed and how to be controled.
The abstraction layer is a set of functions to implement what
to be controled.
For example, ac_relation_create() checks user's privilege to
create a new table. It internally calls pg_namespace_aclcheck()
and pg_tablespace_aclcheck() to make its access control decision
based on the security model in database ACLs.
This abstraction layer functions have the following naming convension.
ac_<object type>_<action>(args, ...)
e.g) void ac_proc_execute(Oid proOid, Oid roleOid)
It checks privilege to execute a certain procedure with
the given database role. The caller gives all the necessary
informations to make its decision.
It replaces all the pg_xxx_aclcheck() and pg_xxx_ownercheck() invocations
from the backend implementations, except for security/access_control.c.
In this patch, these are used as helper functions to implement access
control logic (in other word, how to be controled), invoked from the
access control functions.
These ac_xxx_xxx() routines will be entrypoints to invoke additional
security checks (SE-PostgreSQL), rather than sepgsqlXXXX() hooks around
the backend implementation.
Thanks,
[kaigai@saba pgsec]$ diffstat sepgsql-01-base-8.5devel-r2251.patch.gz
backend/Makefile | 2
backend/catalog/aclchk.c | 218 !
backend/catalog/namespace.c | 53
backend/catalog/pg_aggregate.c | 12
backend/catalog/pg_conversion.c | 33
backend/catalog/pg_operator.c | 42
backend/catalog/pg_proc.c | 15
backend/catalog/pg_shdepend.c | 8
backend/catalog/pg_type.c | 25
backend/commands/aggregatecmds.c | 42
backend/commands/alter.c | 66
backend/commands/analyze.c | 5
backend/commands/cluster.c | 9
backend/commands/comment.c | 120
backend/commands/conversioncmds.c | 71
backend/commands/copy.c | 40
backend/commands/dbcommands.c | 160 !
backend/commands/foreigncmds.c | 144
backend/commands/functioncmds.c | 123
backend/commands/indexcmds.c | 120
backend/commands/lockcmds.c | 17
backend/commands/opclasscmds.c | 223 !
backend/commands/operatorcmds.c | 70
backend/commands/proclang.c | 56
backend/commands/schemacmds.c | 60
backend/commands/sequence.c | 38
backend/commands/tablecmds.c | 427 -!
backend/commands/tablespace.c | 46
backend/commands/trigger.c | 41
backend/commands/tsearchcmds.c | 176 !
backend/commands/typecmds.c | 136 !
backend/commands/vacuum.c | 3
backend/commands/view.c | 7
backend/executor/execMain.c | 203 !
backend/executor/execQual.c | 16
backend/executor/nodeAgg.c | 24
backend/executor/nodeMergejoin.c | 8
backend/executor/nodeWindowAgg.c | 24
backend/optimizer/util/clauses.c | 6
backend/parser/parse_utilcmd.c | 13
backend/rewrite/rewriteDefine.c | 10
backend/rewrite/rewriteRemove.c | 6
backend/security/Makefile | 10
backend/security/access_control.c | 4290 ++++++++++++++++++++++++++++++++++++++
backend/tcop/fastpath.c | 15
backend/tcop/utility.c | 74
backend/utils/adt/dbsize.c | 25
backend/utils/adt/ri_triggers.c | 24
backend/utils/adt/tid.c | 18
backend/utils/init/postinit.c | 14
include/catalog/pg_proc_fn.h | 1
include/commands/defrem.h | 1
include/utils/security.h | 337 ++
53 files changed, 5027 insertions(+), 924 deletions(-), 1776 modifications(!)
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Attachments:
[application/gzip] sepgsql-01-base-8.5devel-r2251.patch.gz (71.5K, ../../4A93480C.707@ak.jp.nec.com/2-sepgsql-01-base-8.5devel-r2251.patch.gz)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* [PATCH] Reworks for Access Control facilities (r2251)
@ 2009-08-25 02:24 KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 1 reply; 50+ messages in thread
From: KaiGai Kohei @ 2009-08-25 02:24 UTC (permalink / raw)
To: sfrost@snowman.net; +Cc: pgsql-hackers
The following url is a patch to rework access control facilities in PostgreSQL.
http://sepgsql.googlecode.com/files/sepgsql-01-base-8.5devel-r2251.patch.gz
The current implementation does not have well separation in what
to be controled and how to be controled. For example, when we create
a new table, it requires users ACL_CREATE on the namespace and
ACL_CREATE on the tablespace if necessary. These checks are methods
to control whether he can create a new table, or not.
This patch provides an abstraction layer of access controls to
separate what to be controlsed and how to be controled.
The abstraction layer is a set of functions to implement what
to be controled.
For example, ac_relation_create() checks user's privilege to
create a new table. It internally calls pg_namespace_aclcheck()
and pg_tablespace_aclcheck() to make its access control decision
based on the security model in database ACLs.
This abstraction layer functions have the following naming convension.
ac_<object type>_<action>(args, ...)
e.g) void ac_proc_execute(Oid proOid, Oid roleOid)
It checks privilege to execute a certain procedure with
the given database role. The caller gives all the necessary
informations to make its decision.
It replaces all the pg_xxx_aclcheck() and pg_xxx_ownercheck() invocations
from the backend implementations, except for security/access_control.c.
In this patch, these are used as helper functions to implement access
control logic (in other word, how to be controled), invoked from the
access control functions.
These ac_xxx_xxx() routines will be entrypoints to invoke additional
security checks (SE-PostgreSQL), rather than sepgsqlXXXX() hooks around
the backend implementation.
Thanks,
$ diffstat sepgsql-01-base-8.5devel-r2251.patch.gz
backend/Makefile | 2
backend/catalog/aclchk.c | 218 !
backend/catalog/namespace.c | 53
backend/catalog/pg_aggregate.c | 12
backend/catalog/pg_conversion.c | 33
backend/catalog/pg_operator.c | 42
backend/catalog/pg_proc.c | 15
backend/catalog/pg_shdepend.c | 8
backend/catalog/pg_type.c | 25
backend/commands/aggregatecmds.c | 42
backend/commands/alter.c | 66
backend/commands/analyze.c | 5
backend/commands/cluster.c | 9
backend/commands/comment.c | 120
backend/commands/conversioncmds.c | 71
backend/commands/copy.c | 40
backend/commands/dbcommands.c | 160 !
backend/commands/foreigncmds.c | 144
backend/commands/functioncmds.c | 123
backend/commands/indexcmds.c | 120
backend/commands/lockcmds.c | 17
backend/commands/opclasscmds.c | 223 !
backend/commands/operatorcmds.c | 70
backend/commands/proclang.c | 56
backend/commands/schemacmds.c | 60
backend/commands/sequence.c | 38
backend/commands/tablecmds.c | 427 -!
backend/commands/tablespace.c | 46
backend/commands/trigger.c | 41
backend/commands/tsearchcmds.c | 176 !
backend/commands/typecmds.c | 136 !
backend/commands/vacuum.c | 3
backend/commands/view.c | 7
backend/executor/execMain.c | 203 !
backend/executor/execQual.c | 16
backend/executor/nodeAgg.c | 24
backend/executor/nodeMergejoin.c | 8
backend/executor/nodeWindowAgg.c | 24
backend/optimizer/util/clauses.c | 6
backend/parser/parse_utilcmd.c | 13
backend/rewrite/rewriteDefine.c | 10
backend/rewrite/rewriteRemove.c | 6
backend/security/Makefile | 10
backend/security/access_control.c | 4290 ++++++++++++++++++++++++++++++++++++++
backend/tcop/fastpath.c | 15
backend/tcop/utility.c | 74
backend/utils/adt/dbsize.c | 25
backend/utils/adt/ri_triggers.c | 24
backend/utils/adt/tid.c | 18
backend/utils/init/postinit.c | 14
include/catalog/pg_proc_fn.h | 1
include/commands/defrem.h | 1
include/utils/security.h | 337 ++
53 files changed, 5027 insertions(+), 924 deletions(-), 1776 modifications(!)
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2251)
@ 2009-08-25 02:54 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 1 reply; 50+ messages in thread
From: KaiGai Kohei @ 2009-08-25 02:54 UTC (permalink / raw)
To: pgsql-hackers; +Cc: sfrost@snowman.net
KaiGai Kohei wrote:
> The following url is a patch to rework access control facilities in PostgreSQL.
>
> http://sepgsql.googlecode.com/files/sepgsql-01-base-8.5devel-r2251.patch.gz
IIRC, the limitation of attachment was 40kb, so I resent it using a pointing URL
instead of attachment, sorry for same messages.
BTW, was it expanded?
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2251)
@ 2009-08-25 03:16 Alvaro Herrera <alvherre@commandprompt.com>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 0 replies; 50+ messages in thread
From: Alvaro Herrera @ 2009-08-25 03:16 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: pgsql-hackers
KaiGai Kohei wrote:
> KaiGai Kohei wrote:
> > The following url is a patch to rework access control facilities in PostgreSQL.
> >
> > http://sepgsql.googlecode.com/files/sepgsql-01-base-8.5devel-r2251.patch.gz
>
> IIRC, the limitation of attachment was 40kb, so I resent it using a pointing URL
> instead of attachment, sorry for same messages.
Actually the message with the big attachment was delivered and is on the
archives:
http://archives.postgresql.org/message-id/4A93480C.707@ak.jp.nec.com
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
^ permalink raw reply [nested|flat] 50+ messages in thread
* [PATCH] Reworks for Access Control facilities (r2277)
@ 2009-09-03 06:50 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 0 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-09-03 06:50 UTC (permalink / raw)
To: sfrost@snowman.net; +Cc: pgsql-hackers
The attached patch is updated one for reworks of access control
facilities in PostgreSQL.
List of updates:
- Base tree was updated to the latest CVS HEAD
- ac_role_xxx() routines were added, which was forgotten to include.
- Code cleanups. A few compile time warnings were eliminated.
- Add source code comments to introduce why the security checks are
modified at some points.
BTW, IIRC, I remember you mentioned as follows:
| We're not afraid of making large scale changes to core. It just needs
| to be done incrementally and done *first*, before adding features and
| code which depends on it. We have to fix PG *first*, in this regard,
| before we can even start to look at SELinux hooks, etc.
This patch is indeed pure PostgreSQL feature, but a bit large.
| 54 files changed, 5313 insertions(+), 937 deletions(-), 1875 modifications(!)
I'm afraid of the patch is ignored due to the scale of changeset again.
But we also need to get succeeded to merge the access control reworks
at the next commit fest, because we have only three commit fest remained
and the last one automatically reject large patches, so I have to submit
SE-PostgreSQL/lite patch on the CF-Nov.
Is there any good idea to help reviewers?
Or, do you think it is enough to review in the CF-Sep?
One possible idea is to separate the current single patch into the several
shorter frames. For example, the first patch reworks access controls
corresponding to relations into ac_relation_xxxx(), the second patch
reworks access controls corresponding to procedures into ac_proc_xxx(),
and so on....
But, I'm not sure whether it is really helpful to review the patch.
(Needless to say, if we can conclude it is helpfull, I'll rework for
the patch prior to the beggining of the CF-Sep.)
Thanks,
KaiGai Kohei wrote:
> The attached patch reworks access control facilities in PostgreSQL.
>
> The current implementation does not have well separation in what
> to be controled and how to be controled. For example, when we create
> a new table, it requires users ACL_CREATE on the namespace and
> ACL_CREATE on the tablespace if necessary. These checks are methods
> to control whether he can create a new table, or not.
>
> This patch provides an abstraction layer of access controls to
> separate what to be controlsed and how to be controled.
> The abstraction layer is a set of functions to implement what
> to be controled.
> For example, ac_relation_create() checks user's privilege to
> create a new table. It internally calls pg_namespace_aclcheck()
> and pg_tablespace_aclcheck() to make its access control decision
> based on the security model in database ACLs.
>
> This abstraction layer functions have the following naming convension.
>
> ac_<object type>_<action>(args, ...)
>
> e.g) void ac_proc_execute(Oid proOid, Oid roleOid)
> It checks privilege to execute a certain procedure with
> the given database role. The caller gives all the necessary
> informations to make its decision.
>
> It replaces all the pg_xxx_aclcheck() and pg_xxx_ownercheck() invocations
> from the backend implementations, except for security/access_control.c.
> In this patch, these are used as helper functions to implement access
> control logic (in other word, how to be controled), invoked from the
> access control functions.
>
> These ac_xxx_xxx() routines will be entrypoints to invoke additional
> security checks (SE-PostgreSQL), rather than sepgsqlXXXX() hooks around
> the backend implementation.
>
> Thanks,
[kaigai@saba trunk]$ diffstat sepgsql-01-base-8.5devel-r2277.patch.gz
backend/Makefile | 2
backend/catalog/aclchk.c | 220 !
backend/catalog/namespace.c | 66
backend/catalog/pg_aggregate.c | 12
backend/catalog/pg_conversion.c | 33
backend/catalog/pg_operator.c | 42
backend/catalog/pg_proc.c | 22
backend/catalog/pg_shdepend.c | 18
backend/catalog/pg_type.c | 25
backend/commands/aggregatecmds.c | 42
backend/commands/alter.c | 72
backend/commands/analyze.c | 5
backend/commands/cluster.c | 9
backend/commands/comment.c | 125
backend/commands/conversioncmds.c | 71
backend/commands/copy.c | 40
backend/commands/dbcommands.c | 160 !
backend/commands/foreigncmds.c | 144
backend/commands/functioncmds.c | 127
backend/commands/indexcmds.c | 120
backend/commands/lockcmds.c | 17
backend/commands/opclasscmds.c | 223 !
backend/commands/operatorcmds.c | 70
backend/commands/proclang.c | 56
backend/commands/schemacmds.c | 60
backend/commands/sequence.c | 38
backend/commands/tablecmds.c | 355 -
backend/commands/tablespace.c | 46
backend/commands/trigger.c | 41
backend/commands/tsearchcmds.c | 176 !
backend/commands/typecmds.c | 137 !
backend/commands/user.c | 183 !
backend/commands/vacuum.c | 5
backend/commands/view.c | 7
backend/executor/execMain.c | 203 !
backend/executor/execQual.c | 16
backend/executor/nodeAgg.c | 24
backend/executor/nodeMergejoin.c | 8
backend/executor/nodeWindowAgg.c | 24
backend/optimizer/util/clauses.c | 6
backend/parser/parse_utilcmd.c | 13
backend/rewrite/rewriteDefine.c | 10
backend/rewrite/rewriteRemove.c | 6
backend/security/Makefile | 10
backend/security/access_control.c | 4513 ++++++++++++++++++++++++++++++++++++++
backend/tcop/fastpath.c | 15
backend/tcop/utility.c | 74
backend/utils/adt/dbsize.c | 25
backend/utils/adt/ri_triggers.c | 24
backend/utils/adt/tid.c | 18
backend/utils/init/postinit.c | 16
include/catalog/pg_proc_fn.h | 1
include/commands/defrem.h | 1
include/utils/security.h | 349 ++
54 files changed, 5313 insertions(+), 937 deletions(-), 1875 modifications(!)
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Attachments:
[application/gzip] sepgsql-01-base-8.5devel-r2277.patch.gz (75.4K, ../../4A9F6734.9020705@ak.jp.nec.com/2-sepgsql-01-base-8.5devel-r2277.patch.gz)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-14 06:12 KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 1 reply; 50+ messages in thread
From: KaiGai Kohei @ 2009-09-14 06:12 UTC (permalink / raw)
To: pgsql-hackers
The attached patch is an updated version of reworks for access control
facilities, and a short introduction to help reviewing not-obvious part
of the patch.
List of updates:
- Rebased to the latest CVS HEAD.
- bugfix: ac_opfamily_create() was not called on DefineOpClass().
- bugfix: ac_trigger_create() was moved to the proper point.
- Add more source code comments.
== Purpose ==
The purpose of this patch is to provide a common abstraction layer for
various kind of upcoming access control facilities.
Access control is to make an access control decision whether the give
access can be allowed, or not. The way to decide it depends on the
security model. For example, when we create a new table, it should be
checked whether the client has enough privilege to create a new one.
The native database privilege stuff checks ACL_CREATE permission on
the namespace to be placed. It is the way to make a decision on access
controls.
Now PostgreSQL has only one access control mechanism, and its routines
to make access control decisions are invoked from all over the backend
implementation. (Please count pg_xxx_aclcheck() and pg_xxx_ownercheck().)
It also means we need to put a similar number of invocations of security
hooks, when we implement a new security mechanism in addition to the
native one.
The common abstraction layer performs as entry points for each security
features (including the native database privilege), and enables to minimize
the impact when a new security feature is added.
== Implementation ==
Basically, this patch replaces a series of pg_xxx_aclcheck() and
pg_xxx_ownercheck() invocations by the new abstraction function.
The abstraction functions invokes equivalent acl/owner check routines,
and it allows us to put other security checks within the abstraction
functions.
For example, when we want to check client's privilege to execute a certain
function, pg_proc_aclcheck(ACL_EXECUTE) was called from several points.
This patch replaces it by ac_proc_execute(procOid) which internally invokes
pg_proc_aclcheck() as follows:
------------------------------------------------
*** 1029,1040 ****
init_fcache(Oid foid, FuncExprState *fcache,
MemoryContext fcacheCxt, bool needDescForSets)
{
- AclResult aclresult;
-
/* Check permission to call function */
! aclresult = pg_proc_aclcheck(foid, GetUserId(), ACL_EXECUTE);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_PROC, get_func_name(foid));
/*
* Safety check on nargs. Under normal circumstances this should never
--- 1029,1036 ----
init_fcache(Oid foid, FuncExprState *fcache,
MemoryContext fcacheCxt, bool needDescForSets)
{
/* Check permission to call function */
! ac_proc_execute(foid, GetUserId());
/*
* Safety check on nargs. Under normal circumstances this should never
------------------------------------------------
And,
------------------------------------------------
+ /*
+ * ac_proc_execute
+ *
+ * It checks privilege to execute a certain function.
+ *
+ * Note that it should be checked on the function runtime.
+ * Some of DDL statements requires ACL_EXECUTE on creation time, such as
+ * CreateConversionCommand(), however, these are individually checked on
+ * the ac_xxx_create() hook.
+ *
+ * [Params]
+ * proOID : OID of the function to be executed
+ * roleOid : OID of the database role to be evaluated
+ */
+ void
+ ac_proc_execute(Oid proOid, Oid roleOid)
+ {
+ AclResult aclresult;
+
+ aclresult = pg_proc_aclcheck(proOid, roleOid, ACL_EXECUTE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_PROC, get_func_name(proOid));
+ }
------------------------------------------------
It is obvious that we can add invocations of different kind of security
checks at the tail of this abstraction function. It also can be a check
based on MAC security policy.
All the abstraction functions have the following convention.
ac_<object class>_<type of action>([args ...]);
The object class shows what kind of database object is checked. For example,
here is "relation", "proc", "database" and so on. Most of then are equivalent
to the name of system catalog.
The type of action shows what kind of action is required on the object.
It is specific for each object classes, but the "_create", "_alter" and
"_drop" are common for each object classes. (A few object classes without
its ALTER statement does not have "_alter" function.)
Most of abstraction functions will be obvious what does it replaces.
However, "ac_*_create" function tends not to be obvious, because some of
them requires multiple permissions to create a new database object.
For example, when we create a new function, the following permissions are
checked in the native database privilege mechanism.
- ACL_CREATE on the namespace.
- ACL_USAGE on the procedural language, or superuser privilege if untrusted.
- Ownership of the procedure to be replaced, if CREATE OR REPLACE is used.
The original implementation checks these permissions at the different points,
but this patch consolidated them within a ac_proc_create() function.
The "_alter" function may also seems a bit not-obvious, because required
permission depends on the alter option.
For example, ALTER TABLE ... RENAME TO statement required ACL_CREATE on
the current namespace, not only ownership of the target table. So, it
should be checked conditionally, if the ALTER TABLE statement tries to
alter the name.
In the typical "_alter" functions, it checks ownership of the target object,
and it also checks ACL_CREATE on the namespace if renamed, and it also checks
role membership and ACL_CREATE on the namespace with the new owner if owner
changed.
The restrict_and_check_grant() has checked client's privilege to grant/revoke
something on the given database object, and dropped permissions being
available to grant/revoke.
It is separated into two parts. The earlier part is replaced by
the ac_xxx_grant() function to check client's privilege to grant/revoke it.
The later part is still common to drop unavailable permissions to grant/reveke,
as follows:
------------------------------------------------
*************** ExecGrant_Function(InternalGrant *istmt)
*** 1562,1577 ****
old_acl, ownerId,
&grantorId, &avail_goptions);
/*
* Restrict the privileges to what we can actually grant, and emit the
* standards-mandated warning and error messages.
*/
this_privileges =
! restrict_and_check_grant(istmt->is_grant, avail_goptions,
! istmt->all_privs, istmt->privileges,
! funcId, grantorId, ACL_KIND_PROC,
! NameStr(pg_proc_tuple->proname),
! 0, NULL);
/*
* Generate new ACL.
--- 1508,1524 ----
old_acl, ownerId,
&grantorId, &avail_goptions);
+ /* Permission check to grant/revoke */
+ ac_proc_grant(funcId, grantorId, avail_goptions);
+
/*
* Restrict the privileges to what we can actually grant, and emit the
* standards-mandated warning and error messages.
*/
this_privileges =
! restrict_grant(istmt->is_grant, avail_goptions,
! istmt->all_privs, istmt->privileges,
! NameStr(pg_proc_tuple->proname));
/*
* Generate new ACL.
------------------------------------------------
== Needs any comments ==
Currently, all the abstraction functions are placed at
the src/backend/security/access_control.c, but it has about 4400 lines.
It might be preferable to deploy these abstraction functions categorized
by object classes, because it may help to lookup abstraction functions,
as follows:
src/backend/security/ac/ac_database.c
/ac_schema.c
/ac_relation.c
:
Which is preferable for reviewers?
We still have a day by the start of the CF#2.
Thanks,
[kaigai@saba ~]$ diffstat sepgsql-01-base-8.5devel-r2311.patch.gz
backend/Makefile | 2
backend/catalog/aclchk.c | 220 !
backend/catalog/dependency.c | 15
backend/catalog/namespace.c | 66
backend/catalog/pg_aggregate.c | 12
backend/catalog/pg_conversion.c | 33
backend/catalog/pg_operator.c | 42
backend/catalog/pg_proc.c | 22
backend/catalog/pg_shdepend.c | 18
backend/catalog/pg_type.c | 25
backend/commands/aggregatecmds.c | 42
backend/commands/alter.c | 72
backend/commands/analyze.c | 5
backend/commands/cluster.c | 9
backend/commands/comment.c | 125
backend/commands/conversioncmds.c | 71
backend/commands/copy.c | 40
backend/commands/dbcommands.c | 160 !
backend/commands/foreigncmds.c | 144
backend/commands/functioncmds.c | 127
backend/commands/indexcmds.c | 120
backend/commands/lockcmds.c | 17
backend/commands/opclasscmds.c | 236 !
backend/commands/operatorcmds.c | 70
backend/commands/proclang.c | 56
backend/commands/schemacmds.c | 60
backend/commands/sequence.c | 38
backend/commands/tablecmds.c | 355 -
backend/commands/tablespace.c | 46
backend/commands/trigger.c | 41
backend/commands/tsearchcmds.c | 176 !
backend/commands/typecmds.c | 137 !
backend/commands/user.c | 183 !
backend/commands/vacuum.c | 5
backend/commands/view.c | 7
backend/executor/execMain.c | 203 !
backend/executor/execQual.c | 16
backend/executor/nodeAgg.c | 24
backend/executor/nodeMergejoin.c | 8
backend/executor/nodeWindowAgg.c | 24
backend/optimizer/util/clauses.c | 6
backend/parser/parse_utilcmd.c | 13
backend/rewrite/rewriteDefine.c | 10
backend/rewrite/rewriteRemove.c | 6
backend/security/Makefile | 10
backend/security/access_control.c | 4472 ++++++++++++++++++++++++++++++++++++++
backend/tcop/fastpath.c | 15
backend/tcop/utility.c | 74
backend/utils/adt/dbsize.c | 25
backend/utils/adt/ri_triggers.c | 24
backend/utils/adt/tid.c | 18
backend/utils/init/postinit.c | 15
include/catalog/pg_proc_fn.h | 1
include/commands/defrem.h | 1
include/utils/security.h | 348 ++
55 files changed, 5294 insertions(+), 922 deletions(-), 1894 modifications(!)
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Attachments:
[application/gzip] sepgsql-01-base-8.5devel-r2311.patch.gz (75.3K, ../../4AADDEBD.6000406@ak.jp.nec.com/2-sepgsql-01-base-8.5devel-r2311.patch.gz)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-25 00:48 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 2 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-09-25 00:48 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: pgsql-hackers
I noticed that the previous patch (r2311) fails to apply on the CVS HEAD.
The attached patch is only rebased to the latest CVS HEAD, without any
other changes.
BTW, I raised a few issues. Do you have any opinions?
* deployment of the source code
The current patch implements all the access control abstractions at the
src/backend/security/access_control.c. Its size is about 4,500 lines
which includs source comments.
It is an approach to sort out a series of functionalities into a single
big file, such as aclchk.c. One other approach is to put these codes in
the short many files deployed in a directory, such as backend/catalog/*.
Which is the preferable in PostgreSQL?
If the later one is preferable, I can reorganize the access control
abstraction functions as follows, instead of the access_control.c.
src/backend/security/ac/ac_database.c
ac_schema.c
ac_relation.c
:
* pg_class_ownercheck() in EnableDisableRule()
As I mentioned in the another message, pg_class_ownercheck() in the
EnableDisableRule() is redundant.
The EnableDisableRule() is called from ATExecEnableDisableRule() only,
and it is also called from the ATExecCmd() with AT_EnableRule,
AT_EnableAlwaysRule, AT_EnableReplicaRule and AT_DisableRule.
In this path, ATPrepCmd() already calls ATSimplePermissions() which
also calls pg_class_ownercheck() for the target.
I don't think it is necessary to check ownership of the relation twice.
My opinion is to remove the checks from the EnableDisableRule() and
the ac_rule_toggle() is also removed from the patch.
It does not have any compatibility issue.
Any comments?
KaiGai Kohei wrote:
> The attached patch is an updated version of reworks for access control
> facilities, and a short introduction to help reviewing not-obvious part
> of the patch.
>
> List of updates:
> - Rebased to the latest CVS HEAD.
> - bugfix: ac_opfamily_create() was not called on DefineOpClass().
> - bugfix: ac_trigger_create() was moved to the proper point.
> - Add more source code comments.
>
> == Purpose ==
>
> The purpose of this patch is to provide a common abstraction layer for
> various kind of upcoming access control facilities.
> Access control is to make an access control decision whether the give
> access can be allowed, or not. The way to decide it depends on the
> security model. For example, when we create a new table, it should be
> checked whether the client has enough privilege to create a new one.
> The native database privilege stuff checks ACL_CREATE permission on
> the namespace to be placed. It is the way to make a decision on access
> controls.
>
> Now PostgreSQL has only one access control mechanism, and its routines
> to make access control decisions are invoked from all over the backend
> implementation. (Please count pg_xxx_aclcheck() and pg_xxx_ownercheck().)
> It also means we need to put a similar number of invocations of security
> hooks, when we implement a new security mechanism in addition to the
> native one.
>
> The common abstraction layer performs as entry points for each security
> features (including the native database privilege), and enables to minimize
> the impact when a new security feature is added.
>
> == Implementation ==
>
> Basically, this patch replaces a series of pg_xxx_aclcheck() and
> pg_xxx_ownercheck() invocations by the new abstraction function.
> The abstraction functions invokes equivalent acl/owner check routines,
> and it allows us to put other security checks within the abstraction
> functions.
>
> For example, when we want to check client's privilege to execute a certain
> function, pg_proc_aclcheck(ACL_EXECUTE) was called from several points.
> This patch replaces it by ac_proc_execute(procOid) which internally invokes
> pg_proc_aclcheck() as follows:
>
> ------------------------------------------------
> *** 1029,1040 ****
> init_fcache(Oid foid, FuncExprState *fcache,
> MemoryContext fcacheCxt, bool needDescForSets)
> {
> - AclResult aclresult;
> -
> /* Check permission to call function */
> ! aclresult = pg_proc_aclcheck(foid, GetUserId(), ACL_EXECUTE);
> ! if (aclresult != ACLCHECK_OK)
> ! aclcheck_error(aclresult, ACL_KIND_PROC, get_func_name(foid));
>
> /*
> * Safety check on nargs. Under normal circumstances this should never
> --- 1029,1036 ----
> init_fcache(Oid foid, FuncExprState *fcache,
> MemoryContext fcacheCxt, bool needDescForSets)
> {
> /* Check permission to call function */
> ! ac_proc_execute(foid, GetUserId());
>
> /*
> * Safety check on nargs. Under normal circumstances this should never
> ------------------------------------------------
>
> And,
>
> ------------------------------------------------
> + /*
> + * ac_proc_execute
> + *
> + * It checks privilege to execute a certain function.
> + *
> + * Note that it should be checked on the function runtime.
> + * Some of DDL statements requires ACL_EXECUTE on creation time, such as
> + * CreateConversionCommand(), however, these are individually checked on
> + * the ac_xxx_create() hook.
> + *
> + * [Params]
> + * proOID : OID of the function to be executed
> + * roleOid : OID of the database role to be evaluated
> + */
> + void
> + ac_proc_execute(Oid proOid, Oid roleOid)
> + {
> + AclResult aclresult;
> +
> + aclresult = pg_proc_aclcheck(proOid, roleOid, ACL_EXECUTE);
> + if (aclresult != ACLCHECK_OK)
> + aclcheck_error(aclresult, ACL_KIND_PROC, get_func_name(proOid));
> + }
> ------------------------------------------------
>
> It is obvious that we can add invocations of different kind of security
> checks at the tail of this abstraction function. It also can be a check
> based on MAC security policy.
>
> All the abstraction functions have the following convention.
>
> ac_<object class>_<type of action>([args ...]);
>
> The object class shows what kind of database object is checked. For example,
> here is "relation", "proc", "database" and so on. Most of then are equivalent
> to the name of system catalog.
>
> The type of action shows what kind of action is required on the object.
> It is specific for each object classes, but the "_create", "_alter" and
> "_drop" are common for each object classes. (A few object classes without
> its ALTER statement does not have "_alter" function.)
>
> Most of abstraction functions will be obvious what does it replaces.
>
> However, "ac_*_create" function tends not to be obvious, because some of
> them requires multiple permissions to create a new database object.
> For example, when we create a new function, the following permissions are
> checked in the native database privilege mechanism.
>
> - ACL_CREATE on the namespace.
> - ACL_USAGE on the procedural language, or superuser privilege if untrusted.
> - Ownership of the procedure to be replaced, if CREATE OR REPLACE is used.
>
> The original implementation checks these permissions at the different points,
> but this patch consolidated them within a ac_proc_create() function.
>
> The "_alter" function may also seems a bit not-obvious, because required
> permission depends on the alter option.
> For example, ALTER TABLE ... RENAME TO statement required ACL_CREATE on
> the current namespace, not only ownership of the target table. So, it
> should be checked conditionally, if the ALTER TABLE statement tries to
> alter the name.
> In the typical "_alter" functions, it checks ownership of the target object,
> and it also checks ACL_CREATE on the namespace if renamed, and it also checks
> role membership and ACL_CREATE on the namespace with the new owner if owner
> changed.
>
> The restrict_and_check_grant() has checked client's privilege to grant/revoke
> something on the given database object, and dropped permissions being
> available to grant/revoke.
> It is separated into two parts. The earlier part is replaced by
> the ac_xxx_grant() function to check client's privilege to grant/revoke it.
> The later part is still common to drop unavailable permissions to grant/reveke,
> as follows:
>
> ------------------------------------------------
> *************** ExecGrant_Function(InternalGrant *istmt)
> *** 1562,1577 ****
> old_acl, ownerId,
> &grantorId, &avail_goptions);
>
> /*
> * Restrict the privileges to what we can actually grant, and emit the
> * standards-mandated warning and error messages.
> */
> this_privileges =
> ! restrict_and_check_grant(istmt->is_grant, avail_goptions,
> ! istmt->all_privs, istmt->privileges,
> ! funcId, grantorId, ACL_KIND_PROC,
> ! NameStr(pg_proc_tuple->proname),
> ! 0, NULL);
>
> /*
> * Generate new ACL.
> --- 1508,1524 ----
> old_acl, ownerId,
> &grantorId, &avail_goptions);
>
> + /* Permission check to grant/revoke */
> + ac_proc_grant(funcId, grantorId, avail_goptions);
> +
> /*
> * Restrict the privileges to what we can actually grant, and emit the
> * standards-mandated warning and error messages.
> */
> this_privileges =
> ! restrict_grant(istmt->is_grant, avail_goptions,
> ! istmt->all_privs, istmt->privileges,
> ! NameStr(pg_proc_tuple->proname));
>
> /*
> * Generate new ACL.
> ------------------------------------------------
>
>
> == Needs any comments ==
>
> Currently, all the abstraction functions are placed at
> the src/backend/security/access_control.c, but it has about 4400 lines.
>
> It might be preferable to deploy these abstraction functions categorized
> by object classes, because it may help to lookup abstraction functions,
> as follows:
>
> src/backend/security/ac/ac_database.c
> /ac_schema.c
> /ac_relation.c
> :
>
> Which is preferable for reviewers?
> We still have a day by the start of the CF#2.
>
> Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Attachments:
[application/gzip] sepgsql-01-base-8.5devel-r2328.patch.gz (75.2K, ../../4ABC136A.90903@ak.jp.nec.com/2-sepgsql-01-base-8.5devel-r2328.patch.gz)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-27 16:56 Robert Haas <robertmhaas@gmail.com>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
1 sibling, 0 replies; 50+ messages in thread
From: Robert Haas @ 2009-09-27 16:56 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: Stephen Frost <sfrost@snowman.net>; pgsql-hackers
2009/9/24 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> I noticed that the previous patch (r2311) fails to apply on the CVS HEAD.
> The attached patch is only rebased to the latest CVS HEAD, without any
> other changes.
Stephen,
Are you planning to post a review for this? We are 12 days into the
CommitFest so we need to give KaiGai some feedback soon.
Thanks,
...Robert
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-29 01:33 Stephen Frost <sfrost@snowman.net>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
1 sibling, 1 reply; 50+ messages in thread
From: Stephen Frost @ 2009-09-29 01:33 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: pgsql-hackers
* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> BTW, I raised a few issues. Do you have any opinions?
Certainly, though they're my opinions and I don't know if the committers
will agree, but I suspect they will.
> * deployment of the source code
>
> The current patch implements all the access control abstractions at the
> src/backend/security/access_control.c. Its size is about 4,500 lines
> which includs source comments.
> It is an approach to sort out a series of functionalities into a single
> big file, such as aclchk.c. One other approach is to put these codes in
> the short many files deployed in a directory, such as backend/catalog/*.
> Which is the preferable in PostgreSQL?
A single, larger file, as implemented, is preferable, I believe.
> * pg_class_ownercheck() in EnableDisableRule()
>
> As I mentioned in the another message, pg_class_ownercheck() in the
> EnableDisableRule() is redundant.
>
> The EnableDisableRule() is called from ATExecEnableDisableRule() only,
> and it is also called from the ATExecCmd() with AT_EnableRule,
> AT_EnableAlwaysRule, AT_EnableReplicaRule and AT_DisableRule.
> In this path, ATPrepCmd() already calls ATSimplePermissions() which
> also calls pg_class_ownercheck() for the target.
>
> I don't think it is necessary to check ownership of the relation twice.
> My opinion is to remove the checks from the EnableDisableRule() and
> the ac_rule_toggle() is also removed from the patch.
> It does not have any compatibility issue.
>
> Any comments?
I agree that we don't need to check the ownership twice. You might
check if there was some history to having both checks (perhaps there was
another code path before which didn't check before calling
EnableDisableRule()?). I'd feel alot more comfortable removing the
check if we can show why it was there originally as well as why it's not
needed now.
I'm working on a more comprehensive review, but wanted to answer these
questions first.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (196B, ../../20090929013337.GK17756@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-29 02:19 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 0 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-09-29 02:19 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: pgsql-hackers
Stephen Frost wrote:
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> BTW, I raised a few issues. Do you have any opinions?
>
> Certainly, though they're my opinions and I don't know if the committers
> will agree, but I suspect they will.
Thanks for your comments.
>> * deployment of the source code
>>
>> The current patch implements all the access control abstractions at the
>> src/backend/security/access_control.c. Its size is about 4,500 lines
>> which includs source comments.
>> It is an approach to sort out a series of functionalities into a single
>> big file, such as aclchk.c. One other approach is to put these codes in
>> the short many files deployed in a directory, such as backend/catalog/*.
>> Which is the preferable in PostgreSQL?
>
> A single, larger file, as implemented, is preferable, I believe.
OK,
>> * pg_class_ownercheck() in EnableDisableRule()
>>
>> As I mentioned in the another message, pg_class_ownercheck() in the
>> EnableDisableRule() is redundant.
>>
>> The EnableDisableRule() is called from ATExecEnableDisableRule() only,
>> and it is also called from the ATExecCmd() with AT_EnableRule,
>> AT_EnableAlwaysRule, AT_EnableReplicaRule and AT_DisableRule.
>> In this path, ATPrepCmd() already calls ATSimplePermissions() which
>> also calls pg_class_ownercheck() for the target.
>>
>> I don't think it is necessary to check ownership of the relation twice.
>> My opinion is to remove the checks from the EnableDisableRule() and
>> the ac_rule_toggle() is also removed from the patch.
>> It does not have any compatibility issue.
>>
>> Any comments?
>
> I agree that we don't need to check the ownership twice. You might
> check if there was some history to having both checks (perhaps there was
> another code path before which didn't check before calling
> EnableDisableRule()?). I'd feel alot more comfortable removing the
> check if we can show why it was there originally as well as why it's not
> needed now.
I checked history of the repository, and this commit adds EnableDisableRule().
* Changes pg_trigger and extend pg_rewrite in order to allow triggers and
Jan Wieck [Mon, 19 Mar 2007 23:38:32 +0000 (23:38 +0000)]
http://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=31e6bde46c0594c6f8bb82606c49f93295f...
The corresponding AT_xxxx command calls ATSimplePermissions() from ATPrepCmd(),
and ATExecCmd() is the only caller from the begining.
It seems to me this redundant check does not have any explicit reason.
I think it is harmless to remove this pg_class_ownercheck() from here.
> I'm working on a more comprehensive review, but wanted to answer these
> questions first.
Thanks for your efforts.
I'm looking forward to see rest of the comments.
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-29 10:54 Stephen Frost <sfrost@snowman.net>
0 siblings, 2 replies; 50+ messages in thread
From: Stephen Frost @ 2009-09-29 10:54 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; KaiGai Kohei <kaigai@kaigai.gr.jp>; Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers
* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> Could you post any review comments, even if it is not comprehensive yet?
In general, you don't need to preface your comments with 'MEMO:'. I
would encourage removing that. You might use 'FIXME:' instead, if it is
something which needs to be corrected in the future. Additionally,
please think about PG as a whole project. Talking about 'native
privilege mechanism' implies there are 'native' and 'foreign' ones. I
would recommend using the term 'default' instead. Also, rather than
saying it is "not a correct way" when referring to decisions made about
the permissions checks for the 'default' mechanism, just say it's the
default way and that other modules might not implement it the same.
Saying it's "not a correct way" implies a problem with the existing
code. If that's true, then it should be addressed separatly from this
patch.
Specifically, in LookupExplicitNamespace, you added:
---------------------
MEMO: The native privilege mechanism always allows everyone to apply
ACL_USAGE permission on the temporary namespaces implicitly, so it was
omitted to check the obvious one here. But it is a heuristic, not a
correct way.
---------------------
My recommendation for how to reword this, if it's accurate, is:
---------------------
By default, everyone is permitted ACL_USAGE on temporary namespaces
implicitly, so a check for it was omitted here. Other security models
may wish to implement a check, so call ac_schema_search() to check.
---------------------
You might also provide a specific example of where and why this check
matters. I'm not entirely convinced it's necessary or makes sense, to
be honest..
Also, does it make sense to still have LookupCreationNamespace? Given
its charter and the changes, is it still different from
LookupExplicitNamespace?
I would recommend adding back the comments above the calls to
ac_*_grant() which explicitly say:
---------------------
If we found no grant options, consider whether to issue a hard error.
Per spec, having any privilege at all on the object will get you by
here.
---------------------
The issue here is to make it clear to any callers that ac_*_grant() does
not check if everything being attempted will succeed but rather if
any one thing will. Same thing with the comments above the ac_*_grant()
functions themselves.
I'm not sure that the comment in restrict_grant() regarding this is
really necessary, considering that post-change there shouldn't be a
pg_aclmask() anymore, so referring to it in a comment seems odd.
In general, I like what you've done with splitting
restrict_and_check_grant() up.
Regarding your comment in dependency.c about objects which are dropped
due to dependencies:
Have you already developed the code to resolve this issue? Is there
additional information required to check if the object can be dropped?
Should the ac_object_drop() just call the regular drop routines? But
they'd have to have a flag added which indicates if it's a dependency
drop or not, right? If the regular drop routine isn't called, then what
would ac_object_drop() use to determine if the drop is permitted or not?
My concern here is that we don't want to develop a new API and then
immediately discover that it doesn't cover the cases we need it for.
If you have already developed an ac_object_drop() which would work for
existing PG and would be easily changed to support SE, I would recommend
you include it in this patch.
I don't find the comment regarding what happened with FindConversion to
be nearly descriptive enough. Can you elaborate on why the check wasn't
necessary and has now been removed? If it really isn't needed, why have
that function at all?
Regarding OperatorCreate, it looks like there may be some functional
changes. Specifically, get_other_operator() could now be called even if
a given user doesn't own the operator shell he's trying to fill out. It
could then error-out with the "operator cannot be its own negator or
sort operator" error. I'm not sure that's a big deal at all, but it's a
difference which we might want to document as expected. This assumes,
of course, that such a situation could really happen. If I'm missing
something and that's not possible, let me know.
The new 'permission' argument to ProcedureCreate should be documented in
the function definition, or at least where it's used, as to what it's
for and why it's needed and why ac_proc_create is conditional on it.
Again, regarding shdepend, if you have the code already which works with
the default permissions model for PG, you should include it in this
patch as part of the API. I realize this contradicts a bit what I said
earlier, but the main concern here is making sure that it will actually
work for SEPG. If you vouch that it will, then perhaps we don't need to
add them now, but I would probably also remove the comments then. One
or the other.. Let's not add comments which will immediately be
removed, assuming everything goes well.
There is a similar change in CreateConversionCommand. Again, I don't
think it's a big deal, but I wonder if we should make a decision about
if permission checks should be first or last and then be consistant
about it. My gut feeling is that we should be doing them first and
doing all of them, if at all possible.. There are a couple of other
places like this.
I'm also concerned about the inconsistancy regarding if the roleOid is
passed into the function of if GetUserId() is used. I would recommend
being consistant with this. Either GetUserId() is always 'good enough',
or it's not, and we should require the roleOid to be passed into all of
the ac_* functions. I'm tending towards the latter, since it appears to
be necessary in some cases. If there is some division of the ac_*
functions where it's consistant within a division, that might be
alright, but it should be discussed somewhere.
I've glanced through the rest and in general I feel like it's starting
to look good. Thanks for your efforts towards this. I'd really like to
see this get committed eventually.
Thanks!
Stephen
Attachments:
[application/pgp-signature] signature.asc (196B, ../../20090929105431.GO17756@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-29 11:08 KaiGai Kohei <kaigai@kaigai.gr.jp>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 1 reply; 50+ messages in thread
From: KaiGai Kohei @ 2009-09-29 11:08 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
Stephen, thanks for your comments.
Stephen Frost wrote:
>> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> >> Could you post any review comments, even if it is not comprehensive yet?
>>
>> In general, you don't need to preface your comments with 'MEMO:'. I
>> would encourage removing that. You might use 'FIXME:' instead, if it is
>> something which needs to be corrected in the future. Additionally,
>> please think about PG as a whole project. Talking about 'native
>> privilege mechanism' implies there are 'native' and 'foreign' ones. I
>> would recommend using the term 'default' instead.
OK, indeed, we don't have such a manner something like 'MEMO:' and 'FIXME:'.
I could not find an appropriate name for the current database acl mechanism.
OK, I'll revise the source code comment using the "default".
>> Also, rather than
>> saying it is "not a correct way" when referring to decisions made about
>> the permissions checks for the 'default' mechanism, just say it's the
>> default way and that other modules might not implement it the same.
>> Saying it's "not a correct way" implies a problem with the existing
>> code. If that's true, then it should be addressed separatly from this
>> patch.
>>
>> Specifically, in LookupExplicitNamespace, you added:
>>
>> ---------------------
>> MEMO: The native privilege mechanism always allows everyone to apply
>> ACL_USAGE permission on the temporary namespaces implicitly, so it was
>> omitted to check the obvious one here. But it is a heuristic, not a
>> correct way.
>> ---------------------
>>
>> My recommendation for how to reword this, if it's accurate, is:
>> ---------------------
>> By default, everyone is permitted ACL_USAGE on temporary namespaces
>> implicitly, so a check for it was omitted here. Other security models
>> may wish to implement a check, so call ac_schema_search() to check.
>> ---------------------
OK, indeed, it might be an inappropriate representation.
>> You might also provide a specific example of where and why this check
>> matters. I'm not entirely convinced it's necessary or makes sense, to
>> be honest..
By the default, it is 100% correct to omit checks here.
But it can make an issue, when we port SE-PG.
SELinux has its working mode (Enforcing or Permissive). On the permissive
mode, it also check security policy but does not prevent anything except
for generating access violation logs. We can toggle the working mode at the
runtime, so it is necessary to consider the following scenario.
1. A client tries to create a temporary table, the security policy does not
allow him to search his temporary namespace but it can be bypassed due to
the permissive mode.
2. Security administrator toggles it to enforcing mode.
3. The client tries to use the temporary table, but the security policy does
not allow him to search the temporary namespace, and it shall be enforced
in this time.
>> Also, does it make sense to still have LookupCreationNamespace? Given
>> its charter and the changes, is it still different from
>> LookupExplicitNamespace?
These have a bit of difference. The LookupCreationNamespace() can set up
a new temporary namespace, if "pg_temp" is given but the temporary namespace
it not still set up. LookupExplicitNamespace() does not set it up.
>> I would recommend adding back the comments above the calls to
>> ac_*_grant() which explicitly say:
>>
>> ---------------------
>> If we found no grant options, consider whether to issue a hard error.
>> Per spec, having any privilege at all on the object will get you by
>> here.
>> ---------------------
>>
>> The issue here is to make it clear to any callers that ac_*_grant() does
>> not check if everything being attempted will succeed but rather if
>> any one thing will. Same thing with the comments above the ac_*_grant()
>> functions themselves.
OK, I'll add this comments.
>> I'm not sure that the comment in restrict_grant() regarding this is
>> really necessary, considering that post-change there shouldn't be a
>> pg_aclmask() anymore, so referring to it in a comment seems odd.
Hmm, it may not be necessary after the code reviewing. I'll remove it.
>> In general, I like what you've done with splitting
>> restrict_and_check_grant() up.
Thanks,
>> Regarding your comment in dependency.c about objects which are dropped
>> due to dependencies:
>>
>> Have you already developed the code to resolve this issue? Is there
>> additional information required to check if the object can be dropped?
>> Should the ac_object_drop() just call the regular drop routines? But
>> they'd have to have a flag added which indicates if it's a dependency
>> drop or not, right? If the regular drop routine isn't called, then what
>> would ac_object_drop() use to determine if the drop is permitted or not?
>>
>> My concern here is that we don't want to develop a new API and then
>> immediately discover that it doesn't cover the cases we need it for.
>> If you have already developed an ac_object_drop() which would work for
>> existing PG and would be easily changed to support SE, I would recommend
>> you include it in this patch.
At the SE-PgSQL v8.4.x series, I already added checks on deleteOneObject().
http://code.google.com/p/sepgsql/source/browse/branches/pgsql-8.4.x/sepgsql/src/backend/catalog/depe...
I added a new boolean argument for the function to inform whether the
deletion should be checked, or not. In some cases, it is necessary to
bypass permission checks on cascaded deletion, such as cleaning up
database objects within temporary namespace.
It is not difficult the caller to distinguish whether the given deletion
is cascaded or not.
The findDependentObjects() returns a list of dependency objects.
Any entries to be removed in cascade deletion does not have DEPFLAG_ORIGINAL
flag in the ObjectAddressExtra structure. If the object has the flag, it is
already checked, so we don't need to check permission at the deleteOneObject()
twice.
>> Should the ac_object_drop() just call the regular drop routines?
In this patch, ac_xxx_drop() has dacSkip flag to bypass permission
checks on cascaded deletion. Perhaps, it might be necessary to put
two different ac_ routine to check object deletion.
The one is deployed on regular drop routines, such as ac_relation_drop().
The other is deloped on dependency drop routine, such as ac_object_drop().
If SE-PG feature checks all the drop permission at the ac_object_drop()
(except for objects which does not use dependency deletion), it is not
necessary to distinguish whether the given object is original or cascaded.
>> I don't find the comment regarding what happened with FindConversion to
>> be nearly descriptive enough. Can you elaborate on why the check wasn't
>> necessary and has now been removed? If it really isn't needed, why have
>> that function at all?
http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
I'll add a comment about the reason why this check was simply eliminated.
It already checks permission when we create a new conversion, and it
is enough for the purpose.
BTW, I wonder why ACL_EXECUTE is checked on creation of conversions,
because it is equivalent to allow everyone to execute the conversion
function. It seems to me ownership is more appropriate check.
>> Regarding OperatorCreate, it looks like there may be some functional
>> changes. Specifically, get_other_operator() could now be called even if
>> a given user doesn't own the operator shell he's trying to fill out. It
>> could then error-out with the "operator cannot be its own negator or
>> sort operator" error. I'm not sure that's a big deal at all, but it's a
>> difference which we might want to document as expected. This assumes,
>> of course, that such a situation could really happen. If I'm missing
>> something and that's not possible, let me know.
When the get_other_operator() returns valid OID of the negator and
commutator, these OIDs are delivered to ac_operator_create(), then
ac_operator_create() calls pg_oper_ownercheck() to check ownership
on the negator and commutator.
I think it keeps compatibility in behavior.
>> The new 'permission' argument to ProcedureCreate should be documented in
>> the function definition, or at least where it's used, as to what it's
>> for and why it's needed and why ac_proc_create is conditional on it.
I'll add the following comment. Is it natural?
----
/*
* The 'permission' argument should be false, when an internal stuff
* tries to define a new obviously safe function. It allows to bypass
* security checks to create a new function.
* Otherwise, it should be true.
*/
----
>> Again, regarding shdepend, if you have the code already which works with
>> the default permissions model for PG, you should include it in this
>> patch as part of the API. I realize this contradicts a bit what I said
>> earlier, but the main concern here is making sure that it will actually
>> work for SEPG. If you vouch that it will, then perhaps we don't need to
>> add them now, but I would probably also remove the comments then. One
>> or the other.. Let's not add comments which will immediately be
>> removed, assuming everything goes well.
I guess your requirement is to proof the ac_object_drop() is implementable
with reasonable changes.
Yes, in the default permission model for PG, it does not check anything
here. So, if we add ac_object_drop() here, it should be an empty function
or it calls ac_xxx_drop() with dacSkip equals true which means do nothing.
In my current preference, I would like to remove dacSkip flag from the
ac_xxx_drop() routines and we don't call it from the ac_object_drop().
And, ac_object_drop() only calls MAC routines, as the SE-PG v8.4 doing.
However, either of them are possible.
>> There is a similar change in CreateConversionCommand. Again, I don't
>> think it's a big deal, but I wonder if we should make a decision about
>> if permission checks should be first or last and then be consistant
>> about it. My gut feeling is that we should be doing them first and
>> doing all of them, if at all possible.. There are a couple of other
>> places like this.
The default PG model requires two privilege to create a new conversion.
The one is ACL_CREATE on the namespace, the other is ACL_EXECUTE on
the conversion procedure. The caller (CreateConversionCommand) has to
resolve both of objects identified by human readable name, prior to
the ac_conversion_create() invocation.
In the current implementation, these permissions are checked on the
separated timing just after obtaining OID of namespace and procedure.
>> My gut feeling is that we should be doing them first and
>> doing all of them, if at all possible
The 'doing all of them' will contain resolving database object identified
by its name. It has to be done prior to the security checks.
The security checks raises an error on access violations, and it aborts
whole of the current transaction. So, I don't think it is a big issue
whether it should be put at the first or last.
It is only necessary to provide enough information to make decision for
the ac_*() routines.
>> I'm also concerned about the inconsistancy regarding if the roleOid is
>> passed into the function of if GetUserId() is used. I would recommend
>> being consistant with this. Either GetUserId() is always 'good enough',
>> or it's not, and we should require the roleOid to be passed into all of
>> the ac_* functions. I'm tending towards the latter, since it appears to
>> be necessary in some cases. If there is some division of the ac_*
>> functions where it's consistant within a division, that might be
>> alright, but it should be discussed somewhere.
The ac_proc_execute() is the only case which the caller has to give
roleOid instead of GetUserId(), because the default PG model requires
to check ACL_EXECUTE permission on the pair of owner of the aggregate
function and trans/final function of the aggregate.
In other case, GetUserId() is good enough.
Is it really necessary to add roleOid argument for all the hooks?
>> I've glanced through the rest and in general I feel like it's starting
>> to look good. Thanks for your efforts towards this. I'd really like to
>> see this get committed eventually.
Thanks for your reviewing. It is great heplful to improve the project.
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-29 13:41 Robert Haas <robertmhaas@gmail.com>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 0 replies; 50+ messages in thread
From: Robert Haas @ 2009-09-29 13:41 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; KaiGai Kohei <kaigai@kaigai.gr.jp>; pgsql-hackers
On Tue, Sep 29, 2009 at 6:54 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> Could you post any review comments, even if it is not comprehensive yet?
>
> In general, you don't need to preface your comments with 'MEMO:'. Â I
> would encourage removing that. Â You might use 'FIXME:' instead, if it is
> something which needs to be corrected in the future. Â Additionally,
> please think about PG as a whole project. Â Talking about 'native
> privilege mechanism' implies there are 'native' and 'foreign' ones. Â I
> would recommend using the term 'default' instead.
Or maybe 'standard'?
...Robert
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-29 17:30 Stephen Frost <sfrost@snowman.net>
parent: KaiGai Kohei <kaigai@kaigai.gr.jp>
0 siblings, 1 reply; 50+ messages in thread
From: Stephen Frost @ 2009-09-29 17:30 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@kaigai.gr.jp>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
* KaiGai Kohei (kaigai@kaigai.gr.jp) wrote:
> Stephen Frost wrote:
> >> You might also provide a specific example of where and why this check
> >> matters. I'm not entirely convinced it's necessary or makes sense, to
> >> be honest..
>
> By the default, it is 100% correct to omit checks here.
>
> But it can make an issue, when we port SE-PG.
> SELinux has its working mode (Enforcing or Permissive). On the permissive
> mode, it also check security policy but does not prevent anything except
> for generating access violation logs. We can toggle the working mode at the
> runtime, so it is necessary to consider the following scenario.
The scenario you outline could happen without SE-PG, couldn't it?
Specifically, if a user makes a connection, creates a temporary table,
and then their rights to create temporary tables are revoked? What
should happen in that instance though? Should they continue to have
access to the tables they've created? Should they be dropped
immediately?
I'm not convinced this case is handled sufficiently.. We at least need
to think about what we want to happen and document it accordingly.
> >> Should the ac_object_drop() just call the regular drop routines?
>
> In this patch, ac_xxx_drop() has dacSkip flag to bypass permission
> checks on cascaded deletion. Perhaps, it might be necessary to put
> two different ac_ routine to check object deletion.
I'm don't think that's necessary. Having the flag is appropriate, what
I'm wondering about is why you say in the comment that calling
ac_object_drop() should be done, but then you don't call it. Even if
it doesn't do anything in the default case, I think the call should be
included. The point is that it should be part of the API and
implemented and ready to go for when SE-PG is added. I don't like the
idea of having it commented out now, but then uncommenting it when SE-PG
is added.
> >> I don't find the comment regarding what happened with FindConversion to
> >> be nearly descriptive enough. Can you elaborate on why the check wasn't
> >> necessary and has now been removed? If it really isn't needed, why have
> >> that function at all?
>
> http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
>
> I'll add a comment about the reason why this check was simply eliminated.
Could these kind of changes be done as a separate patch? Perhaps one
which should be applied first? It's alot easier to review if we can
have:
- patches to fix things in PG (such as the above)
- patch to add in ac_* model
This would allow us to much more easily prove to ourselves that the ac_*
model doesn't break anything. As this is security related code, we
really need to be comfortable that we're not doing anything to break the
security of the system.
> BTW, I wonder why ACL_EXECUTE is checked on creation of conversions,
> because it is equivalent to allow everyone to execute the conversion
> function. It seems to me ownership is more appropriate check.
This is, again, something which should be discussed and dealt with
separately from the ac_* model implementation.
> >> Regarding OperatorCreate, it looks like there may be some functional
> >> changes. Specifically, get_other_operator() could now be called even if
> >> a given user doesn't own the operator shell he's trying to fill out. It
> >> could then error-out with the "operator cannot be its own negator or
> >> sort operator" error. I'm not sure that's a big deal at all, but it's a
> >> difference which we might want to document as expected. This assumes,
> >> of course, that such a situation could really happen. If I'm missing
> >> something and that's not possible, let me know.
>
> When the get_other_operator() returns valid OID of the negator and
> commutator, these OIDs are delivered to ac_operator_create(), then
> ac_operator_create() calls pg_oper_ownercheck() to check ownership
> on the negator and commutator.
> I think it keeps compatibility in behavior.
My concern is just that there might now be an error message seen be the
user first regarding the negator instead of a permissions error that
would have been seen first before, for the same situation.
I doubt this is a problem, really, but we should at least bring up any
changes in behaviour like this and get agreement that they're
acceptable, even if it's just the particulars of error-messages (since
they could possibly contain information that shouldn't be available...).
> >> The new 'permission' argument to ProcedureCreate should be documented in
> >> the function definition, or at least where it's used, as to what it's
> >> for and why it's needed and why ac_proc_create is conditional on it.
>
> I'll add the following comment. Is it natural?
> ----
> /*
> * The 'permission' argument should be false, when an internal stuff
> * tries to define a new obviously safe function. It allows to bypass
> * security checks to create a new function.
> * Otherwise, it should be true.
> */
> ----
I don't really like referring to 'internal stuff'.. How about:
----------------
The 'permission' argument specifies if permissions checking will be
done. This allows bypassing security checks when they're not necessary,
such as being called from internal routines where the checks have
already been done or they're clearly not required.
In general, this argument should be 'true' to ensure that appropriate
permissions checks are done.
----------------
Something like that.. Of course, it needs to be correct, and you're
more familiar with that code, so if that comment isn't correct, please
change it.
> >> Again, regarding shdepend, if you have the code already which works with
> >> the default permissions model for PG, you should include it in this
> >> patch as part of the API. I realize this contradicts a bit what I said
> >> earlier, but the main concern here is making sure that it will actually
> >> work for SEPG. If you vouch that it will, then perhaps we don't need to
> >> add them now, but I would probably also remove the comments then. One
> >> or the other.. Let's not add comments which will immediately be
> >> removed, assuming everything goes well.
>
> I guess your requirement is to proof the ac_object_drop() is implementable
> with reasonable changes.
> Yes, in the default permission model for PG, it does not check anything
> here. So, if we add ac_object_drop() here, it should be an empty function
> or it calls ac_xxx_drop() with dacSkip equals true which means do nothing.
I like the latter of those (call ac_xxx_drop() with dacSkip set to
true). We might even consider changing the name of 'dacSkip' to be
'depDrop' or similar. This would indicate that the function is being
called to drop an object due to a dependency, which is really the only
case we want to skip permissions checking in the default model (is this
correct?). That then makes it clear that other models (such as SE-PG)
can change the behaviour of permissions checking on objects dropped due
to dependencies.
> In my current preference, I would like to remove dacSkip flag from the
> ac_xxx_drop() routines and we don't call it from the ac_object_drop().
> And, ac_object_drop() only calls MAC routines, as the SE-PG v8.4 doing.
We could do this instead. Does it duplicate alot of code from the
ac_xxx_drop() routines though? If not, then I agree with this approach.
I still feel we should include calling ac_object_drop() in this patch
since it's clearly a hook we want to include in the API.
Again, it boils down to if there is sufficient information to implement
the controls we want. How about this- if we wanted (in some future PG)
to give users the option of "check permissions on objects dropped due to
dependencies" with regular PG, could we do that in ac_object_drop()
without alot of extra code? Or would we have to go add the dacSkip (my
depDrop) flag everywhere then? It seems like we could just have that
option checked in ac_object_drop() and have it set depDrop accordingly
for the other functions.
> >> There is a similar change in CreateConversionCommand. Again, I don't
> >> think it's a big deal, but I wonder if we should make a decision about
> >> if permission checks should be first or last and then be consistant
> >> about it. My gut feeling is that we should be doing them first and
> >> doing all of them, if at all possible.. There are a couple of other
> >> places like this.
>
> The default PG model requires two privilege to create a new conversion.
> The one is ACL_CREATE on the namespace, the other is ACL_EXECUTE on
> the conversion procedure. The caller (CreateConversionCommand) has to
> resolve both of objects identified by human readable name, prior to
> the ac_conversion_create() invocation.
>
> In the current implementation, these permissions are checked on the
> separated timing just after obtaining OID of namespace and procedure.
Checking immediately after resolving OIDs seems fine to me. Clearly,
that has to be done and it's really closer to 'parsing' than actually
doing anything.
> >> My gut feeling is that we should be doing them first and
> >> doing all of them, if at all possible
>
> The 'doing all of them' will contain resolving database object identified
> by its name. It has to be done prior to the security checks.
Right, that's fine and make sense.
> The security checks raises an error on access violations, and it aborts
> whole of the current transaction. So, I don't think it is a big issue
> whether it should be put at the first or last.
The issue is that, in my view, it's better to get 'permission denied'
earlier than some other error. I would be frustrated to work out how to
create a particular object only to then be given a 'permission denied'
right at the end. Some things have to be done first to make a decision
about permissions, such as name->OID resolution, but the real 'work' and
any issues related to that should be dealt with after permissions
checking.
> It is only necessary to provide enough information to make decision for
> the ac_*() routines.
Right.. Can we be consistant about having the permissions check done as
soon as we have enough information to call the ac_*() routines then? I
believe that's true in most cases, but not all, today. Any of those
changes which change behaviour should be discussed in some way (such as
an email to -hackers as you did for FindConversion()).
> The ac_proc_execute() is the only case which the caller has to give
> roleOid instead of GetUserId(), because the default PG model requires
> to check ACL_EXECUTE permission on the pair of owner of the aggregate
> function and trans/final function of the aggregate.
> In other case, GetUserId() is good enough.
>
> Is it really necessary to add roleOid argument for all the hooks?
Ok.. It's just ac_proc_execute(), and really only in certain
circumstances, right? Most 'regular' usage of ac_proc_execute() still
uses GetUserId()? Perhaps we could address this by having the argument
called 'aggOid' instead, and pass 'InvalidOid' when it's not an
aggregate, and use GetUserId() in that case inside ac_proc_execute().
We could also change it to be ac_proc_execute() and
ac_proc_agg_execute(). That might be cleaner.
What do you think?
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (196B, ../../20090929173049.GP17756@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-30 02:09 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 2 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-09-30 02:09 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
Stephen Frost wrote:
> * KaiGai Kohei (kaigai@kaigai.gr.jp) wrote:
>> Stephen Frost wrote:
>>>> You might also provide a specific example of where and why this check
>>>> matters. I'm not entirely convinced it's necessary or makes sense, to
>>>> be honest..
>> By the default, it is 100% correct to omit checks here.
>>
>> But it can make an issue, when we port SE-PG.
>> SELinux has its working mode (Enforcing or Permissive). On the permissive
>> mode, it also check security policy but does not prevent anything except
>> for generating access violation logs. We can toggle the working mode at the
>> runtime, so it is necessary to consider the following scenario.
>
> The scenario you outline could happen without SE-PG, couldn't it?
> Specifically, if a user makes a connection, creates a temporary table,
> and then their rights to create temporary tables are revoked? What
> should happen in that instance though? Should they continue to have
> access to the tables they've created? Should they be dropped
> immediately?
The permission to be checked here is ACL_USAGE, not ACL_CREATE_TEMP.
So, the default PG model does not prevent to access his temporary
tables, even if ACL_CREATE_TEMP is revoked after the creation.
In this case, he cannot create new temporay tables any more due to
the lack of ACL_CREATE_TEMP. But it does not prevent accesses to the
temporary objects because these are already created.
> I'm not convinced this case is handled sufficiently.. We at least need
> to think about what we want to happen and document it accordingly.
It is a special case when pg_namespace_aclmask() is called towards
a temporary namespace. It always returns ACL_USAGE bit at least
independently from its acl setting. (ACL_CREATE bit depends on the
ACL_CREATE_TEMP privilege on the current database.)
Please see the pg_namespace_aclmask(). Its source code comment
describes it and it always makes its decision to allow to search
temporary namespace.
I guess it is the reason why pg_namespace_aclcheck() is not called
towards the temporary namespace, and it's right for the default PG
model.
But SE-PG model concerns the assumption.
>>>> Should the ac_object_drop() just call the regular drop routines?
>> In this patch, ac_xxx_drop() has dacSkip flag to bypass permission
>> checks on cascaded deletion. Perhaps, it might be necessary to put
>> two different ac_ routine to check object deletion.
>
> I'm don't think that's necessary. Having the flag is appropriate, what
> I'm wondering about is why you say in the comment that calling
> ac_object_drop() should be done, but then you don't call it. Even if
> it doesn't do anything in the default case, I think the call should be
> included. The point is that it should be part of the API and
> implemented and ready to go for when SE-PG is added. I don't like the
> idea of having it commented out now, but then uncommenting it when SE-PG
> is added.
The reason why I commented as ac_object_drop() should be done is that
SE-PG model also need to apply checks on cascaded objects, not only
the original one, as you mentioned. And I expected that code only used
in SE-PG will be suggested to remove from the first patch.
The dacSkip flag is a bit different issue.
In some cases, cascaded deletion is not completely equivalent to the
regular deletion.
For example, the default PG model checks ownership of the relation
when we create/drop a trigger object. The PE-PG model also follow
the manner, so it also checks db_table:{setattr} permission.
When we drop a table, it automatically drops triggers which depends
on the table. In this case, if ac_object_drop() calls ac_trigger_drop()
with dacSkip=true, SE-PG checks db_table:{setattr} first, then it also
checks db_table:{drop} later. It is not incorrect, but redundant.
But, it is not a fundamental differences between approaches.
For example, we can skip SE-PG's check on triggers when dacSkip=true.
In this case, the name of variable might be a bit strange.
It is a matter of preference finally.
>>>> I don't find the comment regarding what happened with FindConversion to
>>>> be nearly descriptive enough. Can you elaborate on why the check wasn't
>>>> necessary and has now been removed? If it really isn't needed, why have
>>>> that function at all?
>> http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
>>
>> I'll add a comment about the reason why this check was simply eliminated.
>
> Could these kind of changes be done as a separate patch? Perhaps one
> which should be applied first? It's alot easier to review if we can
> have:
>
> - patches to fix things in PG (such as the above)
> - patch to add in ac_* model
I think we can apply these kind of eliminations earlier or later.
These checks might be redundant or unnecessary, but harmless.
As far as the reworks patch does not touch them, it does not affect
to our discussion.
> This would allow us to much more easily prove to ourselves that the ac_*
> model doesn't break anything. As this is security related code, we
> really need to be comfortable that we're not doing anything to break the
> security of the system.
OK, I'll separate this part.
>> BTW, I wonder why ACL_EXECUTE is checked on creation of conversions,
>> because it is equivalent to allow everyone to execute the conversion
>> function. It seems to me ownership is more appropriate check.
>
> This is, again, something which should be discussed and dealt with
> separately from the ac_* model implementation.
Ahn, it is just my opinion when I saw the code.
I have no intention to change the behavior in this patch.
Is the source code comment also unconfortable?
>>>> Regarding OperatorCreate, it looks like there may be some functional
>>>> changes. Specifically, get_other_operator() could now be called even if
>>>> a given user doesn't own the operator shell he's trying to fill out. It
>>>> could then error-out with the "operator cannot be its own negator or
>>>> sort operator" error. I'm not sure that's a big deal at all, but it's a
>>>> difference which we might want to document as expected. This assumes,
>>>> of course, that such a situation could really happen. If I'm missing
>>>> something and that's not possible, let me know.
>> When the get_other_operator() returns valid OID of the negator and
>> commutator, these OIDs are delivered to ac_operator_create(), then
>> ac_operator_create() calls pg_oper_ownercheck() to check ownership
>> on the negator and commutator.
>> I think it keeps compatibility in behavior.
>
> My concern is just that there might now be an error message seen be the
> user first regarding the negator instead of a permissions error that
> would have been seen first before, for the same situation.
>
> I doubt this is a problem, really, but we should at least bring up any
> changes in behaviour like this and get agreement that they're
> acceptable, even if it's just the particulars of error-messages (since
> they could possibly contain information that shouldn't be available...).
I don't think it is a problem.
The default PG model (also SE-PG model) does not support to hide existence
of a certain database objects, even if client does not have access permissions.
For example, a client can SELECT from the pg_class which include relations
stored within a certain namespace without ACL_USAGE.
The default PG model prevent to resolve the relation name in the schema,
but it is different from to hide its existent.
>>>> The new 'permission' argument to ProcedureCreate should be documented in
>>>> the function definition, or at least where it's used, as to what it's
>>>> for and why it's needed and why ac_proc_create is conditional on it.
>> I'll add the following comment. Is it natural?
>> ----
>> /*
>> * The 'permission' argument should be false, when an internal stuff
>> * tries to define a new obviously safe function. It allows to bypass
>> * security checks to create a new function.
>> * Otherwise, it should be true.
>> */
>> ----
>
> I don't really like referring to 'internal stuff'.. How about:
>
> ----------------
> The 'permission' argument specifies if permissions checking will be
> done. This allows bypassing security checks when they're not necessary,
> such as being called from internal routines where the checks have
> already been done or they're clearly not required.
> In general, this argument should be 'true' to ensure that appropriate
> permissions checks are done.
> ----------------
>
> Something like that.. Of course, it needs to be correct, and you're
> more familiar with that code, so if that comment isn't correct, please
> change it.
I would like to use the revised comment.
>>>> Again, regarding shdepend, if you have the code already which works with
>>>> the default permissions model for PG, you should include it in this
>>>> patch as part of the API. I realize this contradicts a bit what I said
>>>> earlier, but the main concern here is making sure that it will actually
>>>> work for SEPG. If you vouch that it will, then perhaps we don't need to
>>>> add them now, but I would probably also remove the comments then. One
>>>> or the other.. Let's not add comments which will immediately be
>>>> removed, assuming everything goes well.
>> I guess your requirement is to proof the ac_object_drop() is implementable
>> with reasonable changes.
>> Yes, in the default permission model for PG, it does not check anything
>> here. So, if we add ac_object_drop() here, it should be an empty function
>> or it calls ac_xxx_drop() with dacSkip equals true which means do nothing.
>
> I like the latter of those (call ac_xxx_drop() with dacSkip set to
> true). We might even consider changing the name of 'dacSkip' to be
> 'depDrop' or similar. This would indicate that the function is being
> called to drop an object due to a dependency, which is really the only
> case we want to skip permissions checking in the default model (is this
> correct?). That then makes it clear that other models (such as SE-PG)
> can change the behaviour of permissions checking on objects dropped due
> to dependencies.
As I noted above. It is not a significant design issue.
I prefere 'cascade' more than 'depDrop' as the name of flag variable.
> which is really the only
> case we want to skip permissions checking in the default model (is this
> correct?).
If you saying the case is only MAC should be applied but no DAC,
it is correct.
I think four other cases that both of MAC/DAC should be bypassed.
- when temporary database objects are cleaned up on session closing.
- when ATRewriteTables() cleans up a temporary relation.
- when CLUSTER command cleans up a temporary relation.
- when autovacuum found an orphan temp table, and drop it.
In this case, ac_object_drop() should not be called anyway,
as if ac_proc_create() is not called when caller does not want.
>> In my current preference, I would like to remove dacSkip flag from the
>> ac_xxx_drop() routines and we don't call it from the ac_object_drop().
>> And, ac_object_drop() only calls MAC routines, as the SE-PG v8.4 doing.
>
> We could do this instead. Does it duplicate alot of code from the
> ac_xxx_drop() routines though? If not, then I agree with this approach.
> I still feel we should include calling ac_object_drop() in this patch
> since it's clearly a hook we want to include in the API.
The only difference is where sepgsql_xxx_drop() is called from.
If we have the 'dacSkip' (or 'depDrop' or 'cascade') flag,
the sepgsql_proc_drop() will be called from the ac_proc_drop().
Otherwise, ac_object_drop() calls sepgsql_object_drop(), then
it calls sepgsql_proc_drop().
It needs micro adjustment for several object classes, such as
trigger objects, but it is not a fundamental issue.
At first, I follows your preference. (ac_object_drop() commented out
and it calls ac_xxx_drop() with DAC bypassable flag.)
> Again, it boils down to if there is sufficient information to implement
> the controls we want. How about this- if we wanted (in some future PG)
> to give users the option of "check permissions on objects dropped due to
> dependencies" with regular PG, could we do that in ac_object_drop()
> without alot of extra code? Or would we have to go add the dacSkip (my
> depDrop) flag everywhere then? It seems like we could just have that
> option checked in ac_object_drop() and have it set depDrop accordingly
> for the other functions.
If we expect such kind of future enhancement, it may be more flexible
to deliver a flag value.
>>>> There is a similar change in CreateConversionCommand. Again, I don't
>>>> think it's a big deal, but I wonder if we should make a decision about
>>>> if permission checks should be first or last and then be consistant
>>>> about it. My gut feeling is that we should be doing them first and
>>>> doing all of them, if at all possible.. There are a couple of other
>>>> places like this.
>> The default PG model requires two privilege to create a new conversion.
>> The one is ACL_CREATE on the namespace, the other is ACL_EXECUTE on
>> the conversion procedure. The caller (CreateConversionCommand) has to
>> resolve both of objects identified by human readable name, prior to
>> the ac_conversion_create() invocation.
>>
>> In the current implementation, these permissions are checked on the
>> separated timing just after obtaining OID of namespace and procedure.
>
> Checking immediately after resolving OIDs seems fine to me. Clearly,
> that has to be done and it's really closer to 'parsing' than actually
> doing anything.
Please note that what factors are used depends on the security model
for the same required action, such as CREATE CONVERSION.
If we put ac_*() functions after the name->OID resolution, it breaks
the purpose of security abstraction layer.
In this example, it makes access control decision to create a new
conversion, and raises an error if violated.
But the way to make the decision is encapsulated from the caller.
A configuration may make decision with the default PG model.
An other configuration may make decision with the default PG and SE-PG
model. The caller has to provide information to the ac_*() functions,
but it should not depend on a certain security model.
>> The security checks raises an error on access violations, and it aborts
>> whole of the current transaction. So, I don't think it is a big issue
>> whether it should be put at the first or last.
>
> The issue is that, in my view, it's better to get 'permission denied'
> earlier than some other error. I would be frustrated to work out how to
> create a particular object only to then be given a 'permission denied'
> right at the end. Some things have to be done first to make a decision
> about permissions, such as name->OID resolution, but the real 'work' and
> any issues related to that should be dealt with after permissions
> checking.
It is necessary to categolize the work into two cases.
The one is a work without any side-effect. For example, constructing
a memory object, looking up system caches and so on.
The other is a work with updating system catalogs.
I think the security checks can be reordered within the earlier operations,
but should not move to the later of the updating system catalogs, even if
an accee violation error cancels the updates.
>> It is only necessary to provide enough information to make decision for
>> the ac_*() routines.
>
> Right.. Can we be consistant about having the permissions check done as
> soon as we have enough information to call the ac_*() routines then? I
> believe that's true in most cases, but not all, today. Any of those
> changes which change behaviour should be discussed in some way (such as
> an email to -hackers as you did for FindConversion()).
At least, the current implementation deploys ac_*() routines as soon as
the caller have enough information to provide it.
In the result, it had to be moved just before simple_heap_insert() in
some cases.
I'd like to discuss issues related to FindConversion() and EnableDisableRules()
in other patch. And, ac_*() routines don't care about them in this stage.
>> The ac_proc_execute() is the only case which the caller has to give
>> roleOid instead of GetUserId(), because the default PG model requires
>> to check ACL_EXECUTE permission on the pair of owner of the aggregate
>> function and trans/final function of the aggregate.
>> In other case, GetUserId() is good enough.
>>
>> Is it really necessary to add roleOid argument for all the hooks?
>
> Ok.. It's just ac_proc_execute(), and really only in certain
> circumstances, right? Most 'regular' usage of ac_proc_execute() still
> uses GetUserId()? Perhaps we could address this by having the argument
> called 'aggOid' instead, and pass 'InvalidOid' when it's not an
> aggregate, and use GetUserId() in that case inside ac_proc_execute().
> We could also change it to be ac_proc_execute() and
> ac_proc_agg_execute(). That might be cleaner.
>
> What do you think?
Hmm, it may be considerable.
The ac_aggregate_execute(Oid aggOid) may be preferable for the naming
convension.
This check should be put on the ExecInitAgg() and ExecInitWindowAgg().
The current window-func code checks permission on the finalfn/transfn
at the initialize_peragg() called from the ExecInitWindowAgg().
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-30 05:32 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
1 sibling, 1 reply; 50+ messages in thread
From: KaiGai Kohei @ 2009-09-30 05:32 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
>>>>> I don't find the comment regarding what happened with FindConversion to
>>>>> be nearly descriptive enough. Can you elaborate on why the check wasn't
>>>>> necessary and has now been removed? If it really isn't needed, why have
>>>>> that function at all?
>>> http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
>>>
>>> I'll add a comment about the reason why this check was simply eliminated.
>> Could these kind of changes be done as a separate patch? Perhaps one
>> which should be applied first? It's alot easier to review if we can
>> have:
>>
>> - patches to fix things in PG (such as the above)
>> - patch to add in ac_* model
>
> I think we can apply these kind of eliminations earlier or later.
> These checks might be redundant or unnecessary, but harmless.
> As far as the reworks patch does not touch them, it does not affect
> to our discussion.
The attached patch eliminates permission checks in FindConversion()
and EnableDisableRule(), because these are nonsense or redundant.
It is an separated issue from the ac_*() routines.
For now, we decided not to touch these stuffs in the access control
reworks patch. So, we can discuss about these fixes as a different
topic.
See the corresponding messages:
http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
http://archives.postgresql.org/message-id/4ABC136A.90903@ak.jp.nec.com
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Attachments:
[text/x-patch] remove-unneeded-checks.patch (3.7K, ../../4AC2ED74.2030409@ak.jp.nec.com/2-remove-unneeded-checks.patch)
download | inline diff:
Index: base/src/backend/rewrite/rewriteDefine.c
===================================================================
*** base/src/backend/rewrite/rewriteDefine.c (revision 2336)
--- base/src/backend/rewrite/rewriteDefine.c (working copy)
*************** EnableDisableRule(Relation rel, const ch
*** 671,677 ****
{
Relation pg_rewrite_desc;
Oid owningRel = RelationGetRelid(rel);
- Oid eventRelationOid;
HeapTuple ruletup;
bool changed = false;
--- 671,676 ----
*************** EnableDisableRule(Relation rel, const ch
*** 690,702 ****
rulename, get_rel_name(owningRel))));
/*
! * Verify that the user has appropriate permissions.
*/
- eventRelationOid = ((Form_pg_rewrite) GETSTRUCT(ruletup))->ev_class;
- Assert(eventRelationOid == owningRel);
- if (!pg_class_ownercheck(eventRelationOid, GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
- get_rel_name(eventRelationOid));
/*
* Change ev_enabled if it is different from the desired new state.
--- 689,704 ----
rulename, get_rel_name(owningRel))));
/*
! * At the prior release, we had a permission check here on
! * a relation on which the given rule is configured.
! * If user does not have ownership on the relation, it raises
! * an error and aborts current transaction.
! * But this check was redundant. ATExecCmd() is the only caller
! * of EnableDisableRule(), and ATPrepCmd() already checks
! * ownership of the target relation ATSimplePermissions().
! *
! * Therefore, we removed this permission check at v8.5.
*/
/*
* Change ev_enabled if it is different from the desired new state.
Index: base/src/backend/catalog/pg_conversion.c
===================================================================
*** base/src/backend/catalog/pg_conversion.c (revision 2336)
--- base/src/backend/catalog/pg_conversion.c (working copy)
***************
*** 24,30 ****
#include "catalog/pg_proc.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
- #include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/rel.h"
--- 24,29 ----
*************** FindDefaultConversion(Oid name_space, in
*** 219,246 ****
Oid
FindConversion(const char *conname, Oid connamespace)
{
! HeapTuple tuple;
! Oid procoid;
! Oid conoid;
! AclResult aclresult;
!
! /* search pg_conversion by connamespace and conversion name */
! tuple = SearchSysCache(CONNAMENSP,
! PointerGetDatum(conname),
! ObjectIdGetDatum(connamespace),
! 0, 0);
! if (!HeapTupleIsValid(tuple))
! return InvalidOid;
!
! procoid = ((Form_pg_conversion) GETSTRUCT(tuple))->conproc;
! conoid = HeapTupleGetOid(tuple);
!
! ReleaseSysCache(tuple);
!
! /* Check we have execute rights for the function */
! aclresult = pg_proc_aclcheck(procoid, GetUserId(), ACL_EXECUTE);
! if (aclresult != ACLCHECK_OK)
! return InvalidOid;
!
! return conoid;
}
--- 218,237 ----
Oid
FindConversion(const char *conname, Oid connamespace)
{
! /*
! * At the prior release, we had a permission check here
! * on the conversion function. If user does not have
! * ACL_EXECUTE right on the function, the caller performs
! * as if the required conversion is not exist.
! * However, it is nonsense. FindConversion() is only called
! * from the DDL code patch, such as ALTER CONVERSION, so
! * we already apply enough checks on its creation time, and
! * no interfaces are provided to change conversion function.
! *
! * Therefore, we removed this permission check at v8.5.
! */
! return GetSysCacheOid(CONNAMENSP,
! PointerGetDatum(conname),
! ObjectIdGetDatum(connamespace),
! 0, 0);
}
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-30 10:59 Stephen Frost <sfrost@snowman.net>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
1 sibling, 1 reply; 50+ messages in thread
From: Stephen Frost @ 2009-09-30 10:59 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> Stephen Frost wrote:
> > The scenario you outline could happen without SE-PG, couldn't it?
> > Specifically, if a user makes a connection, creates a temporary table,
> > and then their rights to create temporary tables are revoked? What
> > should happen in that instance though? Should they continue to have
> > access to the tables they've created? Should they be dropped
> > immediately?
>
> The permission to be checked here is ACL_USAGE, not ACL_CREATE_TEMP.
> So, the default PG model does not prevent to access his temporary
> tables, even if ACL_CREATE_TEMP is revoked after the creation.
> In this case, he cannot create new temporay tables any more due to
> the lack of ACL_CREATE_TEMP. But it does not prevent accesses to the
> temporary objects because these are already created.
What I'm failing to understand is why SE-PG would be changing this then.
In general, the pg_temp stuff is a bit of a 'wart', as I understand it,
and is there mainly to be a place to put temporary tables. The fact
that it's a schema, which we use in other cases to implement access
controls, feels more like a side-effect of it being a schema than
something really necessary.
> > I'm not convinced this case is handled sufficiently.. We at least need
> > to think about what we want to happen and document it accordingly.
>
> It is a special case when pg_namespace_aclmask() is called towards
> a temporary namespace. It always returns ACL_USAGE bit at least
> independently from its acl setting. (ACL_CREATE bit depends on the
> ACL_CREATE_TEMP privilege on the current database.)
>
> Please see the pg_namespace_aclmask(). Its source code comment
> describes it and it always makes its decision to allow to search
> temporary namespace.
> I guess it is the reason why pg_namespace_aclcheck() is not called
> towards the temporary namespace, and it's right for the default PG
> model.
>
> But SE-PG model concerns the assumption.
The temporary namespace is just there to hold temporary tables which
have been created. It's not intended to be a point of access control.
I understand that there may be some cases where SE-PG wants to control
access when the default PG model doesn't, but this feels like a case
where the PG model doesn't because it's an implementation detail that's
really intended to be hidden from the user.
> The reason why I commented as ac_object_drop() should be done is that
> SE-PG model also need to apply checks on cascaded objects, not only
> the original one, as you mentioned. And I expected that code only used
> in SE-PG will be suggested to remove from the first patch.
I can understand that, and I think I even said that myself previously.
Things have changed a bit since then though, we're trying to develop a
generalized API. As I said before, I could go either way on this:
Either drop the comment, or add the function call. I'm leaning towards
adding the function call here since it can be done as part of the
generalized API and doesn't add alot of complexity. Removing the
comment is also an option, but that's not my preference.
> The dacSkip flag is a bit different issue.
> In some cases, cascaded deletion is not completely equivalent to the
> regular deletion.
>
> For example, the default PG model checks ownership of the relation
> when we create/drop a trigger object. The PE-PG model also follow
> the manner, so it also checks db_table:{setattr} permission.
> When we drop a table, it automatically drops triggers which depends
> on the table. In this case, if ac_object_drop() calls ac_trigger_drop()
> with dacSkip=true, SE-PG checks db_table:{setattr} first, then it also
> checks db_table:{drop} later. It is not incorrect, but redundant.
>
> But, it is not a fundamental differences between approaches.
> For example, we can skip SE-PG's check on triggers when dacSkip=true.
> In this case, the name of variable might be a bit strange.
That was part of the reason I was suggesting changing the name to
'depDrop' for 'dependency Drop', that would clear up the confusion about
it being dac or SE, etc.
> >> BTW, I wonder why ACL_EXECUTE is checked on creation of conversions,
> >> because it is equivalent to allow everyone to execute the conversion
> >> function. It seems to me ownership is more appropriate check.
> >
> > This is, again, something which should be discussed and dealt with
> > separately from the ac_* model implementation.
>
> Ahn, it is just my opinion when I saw the code.
> I have no intention to change the behavior in this patch.
> Is the source code comment also unconfortable?
I could probably go either way on this. In any case, it should be a
separate discussion in a separate thread, if we really want to discuss
it.
> > My concern is just that there might now be an error message seen be the
> > user first regarding the negator instead of a permissions error that
> > would have been seen first before, for the same situation.
> >
> > I doubt this is a problem, really, but we should at least bring up any
> > changes in behaviour like this and get agreement that they're
> > acceptable, even if it's just the particulars of error-messages (since
> > they could possibly contain information that shouldn't be available...).
>
> I don't think it is a problem.
> The default PG model (also SE-PG model) does not support to hide existence
> of a certain database objects, even if client does not have access permissions.
> For example, a client can SELECT from the pg_class which include relations
> stored within a certain namespace without ACL_USAGE.
> The default PG model prevent to resolve the relation name in the schema,
> but it is different from to hide its existent.
I know it doesn't hide existence of major database objects. Depending
on the situation, there might be other information that could be leaked.
I realize that's not the case here, but I still want to catch and
document any behavioral changes, even if it's clear they shouldn't be an
issue.
> I would like to use the revised comment.
Feel free to..
> > I like the latter of those (call ac_xxx_drop() with dacSkip set to
> > true). We might even consider changing the name of 'dacSkip' to be
> > 'depDrop' or similar. This would indicate that the function is being
> > called to drop an object due to a dependency, which is really the only
> > case we want to skip permissions checking in the default model (is this
> > correct?). That then makes it clear that other models (such as SE-PG)
> > can change the behaviour of permissions checking on objects dropped due
> > to dependencies.
>
> As I noted above. It is not a significant design issue.
>
> I prefere 'cascade' more than 'depDrop' as the name of flag variable.
That would be fine with me.
> > which is really the only
> > case we want to skip permissions checking in the default model (is this
> > correct?).
>
> If you saying the case is only MAC should be applied but no DAC,
> it is correct.
>
> I think four other cases that both of MAC/DAC should be bypassed.
> - when temporary database objects are cleaned up on session closing.
> - when ATRewriteTables() cleans up a temporary relation.
> - when CLUSTER command cleans up a temporary relation.
> - when autovacuum found an orphan temp table, and drop it.
>
> In this case, ac_object_drop() should not be called anyway,
> as if ac_proc_create() is not called when caller does not want.
Ok.
> >> In my current preference, I would like to remove dacSkip flag from the
> >> ac_xxx_drop() routines and we don't call it from the ac_object_drop().
> >> And, ac_object_drop() only calls MAC routines, as the SE-PG v8.4 doing.
> >
> > We could do this instead. Does it duplicate alot of code from the
> > ac_xxx_drop() routines though? If not, then I agree with this approach.
> > I still feel we should include calling ac_object_drop() in this patch
> > since it's clearly a hook we want to include in the API.
>
> The only difference is where sepgsql_xxx_drop() is called from.
> If we have the 'dacSkip' (or 'depDrop' or 'cascade') flag,
> the sepgsql_proc_drop() will be called from the ac_proc_drop().
> Otherwise, ac_object_drop() calls sepgsql_object_drop(), then
> it calls sepgsql_proc_drop().
>
> It needs micro adjustment for several object classes, such as
> trigger objects, but it is not a fundamental issue.
>
> At first, I follows your preference. (ac_object_drop() commented out
> and it calls ac_xxx_drop() with DAC bypassable flag.)
>
> > Again, it boils down to if there is sufficient information to implement
> > the controls we want. How about this- if we wanted (in some future PG)
> > to give users the option of "check permissions on objects dropped due to
> > dependencies" with regular PG, could we do that in ac_object_drop()
> > without alot of extra code? Or would we have to go add the dacSkip (my
> > depDrop) flag everywhere then? It seems like we could just have that
> > option checked in ac_object_drop() and have it set depDrop accordingly
> > for the other functions.
>
> If we expect such kind of future enhancement, it may be more flexible
> to deliver a flag value.
Yes, that's what I'm starting to think too, especially now that you've
explained how sepgsql_object_drop() would work. I'd rather it go
through the ac_* API and the sepgsql_proc_drop() be called from there
with the flag than have a separate path.
> >> In the current implementation, these permissions are checked on the
> >> separated timing just after obtaining OID of namespace and procedure.
> >
> > Checking immediately after resolving OIDs seems fine to me. Clearly,
> > that has to be done and it's really closer to 'parsing' than actually
> > doing anything.
>
> Please note that what factors are used depends on the security model
> for the same required action, such as CREATE CONVERSION.
> If we put ac_*() functions after the name->OID resolution, it breaks
> the purpose of security abstraction layer.
>
> In this example, it makes access control decision to create a new
> conversion, and raises an error if violated.
> But the way to make the decision is encapsulated from the caller.
> A configuration may make decision with the default PG model.
> An other configuration may make decision with the default PG and SE-PG
> model. The caller has to provide information to the ac_*() functions,
> but it should not depend on a certain security model.
I agree that what the caller provides shouldn't depend on the security
model. I wasn't suggesting that it would. The name->OID resolution I
was referring to was for things like getting the namespace OID, not
getting the OID for the new conversion being created. Does that clear
up the confusion here?
> >> The security checks raises an error on access violations, and it aborts
> >> whole of the current transaction. So, I don't think it is a big issue
> >> whether it should be put at the first or last.
> >
> > The issue is that, in my view, it's better to get 'permission denied'
> > earlier than some other error. I would be frustrated to work out how to
> > create a particular object only to then be given a 'permission denied'
> > right at the end. Some things have to be done first to make a decision
> > about permissions, such as name->OID resolution, but the real 'work' and
> > any issues related to that should be dealt with after permissions
> > checking.
>
> It is necessary to categolize the work into two cases.
> The one is a work without any side-effect. For example, constructing
> a memory object, looking up system caches and so on.
> The other is a work with updating system catalogs.
>
> I think the security checks can be reordered within the earlier operations,
> but should not move to the later of the updating system catalogs, even if
> an accee violation error cancels the updates.
Right, I was just suggesting moving it to immediately after the work
that does not have any side-effect. It must be checked before any
system catalog changes are done. Sorry for the confusion.
> >> It is only necessary to provide enough information to make decision for
> >> the ac_*() routines.
> >
> > Right.. Can we be consistant about having the permissions check done as
> > soon as we have enough information to call the ac_*() routines then? I
> > believe that's true in most cases, but not all, today. Any of those
> > changes which change behaviour should be discussed in some way (such as
> > an email to -hackers as you did for FindConversion()).
>
> At least, the current implementation deploys ac_*() routines as soon as
> the caller have enough information to provide it.
Good. We should document where that changed behaviour though, but also
document that this is a policy that we're going to try and stick with in
the future (running ac_*() as soon as there is enough information to).
> In the result, it had to be moved just before simple_heap_insert() in
> some cases.
>
> I'd like to discuss issues related to FindConversion() and EnableDisableRules()
> in other patch. And, ac_*() routines don't care about them in this stage.
Ok.
> >> The ac_proc_execute() is the only case which the caller has to give
> >> roleOid instead of GetUserId(), because the default PG model requires
> >> to check ACL_EXECUTE permission on the pair of owner of the aggregate
> >> function and trans/final function of the aggregate.
> >> In other case, GetUserId() is good enough.
> >>
> >> Is it really necessary to add roleOid argument for all the hooks?
> >
> > Ok.. It's just ac_proc_execute(), and really only in certain
> > circumstances, right? Most 'regular' usage of ac_proc_execute() still
> > uses GetUserId()? Perhaps we could address this by having the argument
> > called 'aggOid' instead, and pass 'InvalidOid' when it's not an
> > aggregate, and use GetUserId() in that case inside ac_proc_execute().
> > We could also change it to be ac_proc_execute() and
> > ac_proc_agg_execute(). That might be cleaner.
> >
> > What do you think?
>
> Hmm, it may be considerable.
>
> The ac_aggregate_execute(Oid aggOid) may be preferable for the naming
> convension.
> This check should be put on the ExecInitAgg() and ExecInitWindowAgg().
> The current window-func code checks permission on the finalfn/transfn
> at the initialize_peragg() called from the ExecInitWindowAgg().
Right.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (196B, ../../20090930105911.GS17756@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-09-30 12:23 Stephen Frost <sfrost@snowman.net>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 1 reply; 50+ messages in thread
From: Stephen Frost @ 2009-09-30 12:23 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
KaiGai,
* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> The attached patch eliminates permission checks in FindConversion()
> and EnableDisableRule(), because these are nonsense or redundant.
>
> It is an separated issue from the ac_*() routines.
> For now, we decided not to touch these stuffs in the access control
> reworks patch. So, we can discuss about these fixes as a different
> topic.
>
> See the corresponding messages:
> http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
> http://archives.postgresql.org/message-id/4ABC136A.90903@ak.jp.nec.com
Thanks. To make sure it gets picked up, you might respond to Tom's
message above with this same email. Just a thought.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (196B, ../../20090930122347.GT17756@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-10-01 01:09 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 1 reply; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-01 01:09 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
Stephen Frost wrote:
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> Stephen Frost wrote:
>>> The scenario you outline could happen without SE-PG, couldn't it?
>>> Specifically, if a user makes a connection, creates a temporary table,
>>> and then their rights to create temporary tables are revoked? What
>>> should happen in that instance though? Should they continue to have
>>> access to the tables they've created? Should they be dropped
>>> immediately?
>> The permission to be checked here is ACL_USAGE, not ACL_CREATE_TEMP.
>> So, the default PG model does not prevent to access his temporary
>> tables, even if ACL_CREATE_TEMP is revoked after the creation.
>> In this case, he cannot create new temporay tables any more due to
>> the lack of ACL_CREATE_TEMP. But it does not prevent accesses to the
>> temporary objects because these are already created.
>
> What I'm failing to understand is why SE-PG would be changing this then.
> In general, the pg_temp stuff is a bit of a 'wart', as I understand it,
> and is there mainly to be a place to put temporary tables. The fact
> that it's a schema, which we use in other cases to implement access
> controls, feels more like a side-effect of it being a schema than
> something really necessary.
>
>>> I'm not convinced this case is handled sufficiently.. We at least need
>>> to think about what we want to happen and document it accordingly.
>> It is a special case when pg_namespace_aclmask() is called towards
>> a temporary namespace. It always returns ACL_USAGE bit at least
>> independently from its acl setting. (ACL_CREATE bit depends on the
>> ACL_CREATE_TEMP privilege on the current database.)
>>
>> Please see the pg_namespace_aclmask(). Its source code comment
>> describes it and it always makes its decision to allow to search
>> temporary namespace.
>> I guess it is the reason why pg_namespace_aclcheck() is not called
>> towards the temporary namespace, and it's right for the default PG
>> model.
>>
>> But SE-PG model concerns the assumption.
>
> The temporary namespace is just there to hold temporary tables which
> have been created. It's not intended to be a point of access control.
> I understand that there may be some cases where SE-PG wants to control
> access when the default PG model doesn't, but this feels like a case
> where the PG model doesn't because it's an implementation detail that's
> really intended to be hidden from the user.
It is a good point.
PostgreSQL implements temporary database object feature using a special
schema (pg_temp_*), but it is an implementation details indeed.
As you mentioned, it is a schema in the fact. It may be harmful for
simplicity of the security model to use exception cases.
But it is a perspective from developers, not users.
For example, how many people pay mention that network layer is implemented
as a pseudo filesystem in Linux? You are saying such a thing, correct?
At least, I don't think it is an issue corresponding to security risk.
The reason why SE-PG want to check permission to search a certain schema
including temporary one is a simplicity of access control model.
Yes, it is reasonable both of MAC/DAC to handle temporary schema as
an exception of access controls on schemas.
>>>> BTW, I wonder why ACL_EXECUTE is checked on creation of conversions,
>>>> because it is equivalent to allow everyone to execute the conversion
>>>> function. It seems to me ownership is more appropriate check.
>>> This is, again, something which should be discussed and dealt with
>>> separately from the ac_* model implementation.
>> Ahn, it is just my opinion when I saw the code.
>> I have no intention to change the behavior in this patch.
>> Is the source code comment also unconfortable?
>
> I could probably go either way on this. In any case, it should be a
> separate discussion in a separate thread, if we really want to discuss
> it.
OK,
>>> My concern is just that there might now be an error message seen be the
>>> user first regarding the negator instead of a permissions error that
>>> would have been seen first before, for the same situation.
>>>
>>> I doubt this is a problem, really, but we should at least bring up any
>>> changes in behaviour like this and get agreement that they're
>>> acceptable, even if it's just the particulars of error-messages (since
>>> they could possibly contain information that shouldn't be available...).
>> I don't think it is a problem.
>> The default PG model (also SE-PG model) does not support to hide existence
>> of a certain database objects, even if client does not have access permissions.
>> For example, a client can SELECT from the pg_class which include relations
>> stored within a certain namespace without ACL_USAGE.
>> The default PG model prevent to resolve the relation name in the schema,
>> but it is different from to hide its existent.
>
> I know it doesn't hide existence of major database objects. Depending
> on the situation, there might be other information that could be leaked.
> I realize that's not the case here, but I still want to catch and
> document any behavioral changes, even if it's clear they shouldn't be an
> issue.
I agree that it should be documented.
Where should I document them on? I guess the purpose of the description
is to inform these behavior changes for users, not only developers.
The official documentation sgml? wiki.postgresql.org? or, source code
comments are enough?
>>> which is really the only
>>> case we want to skip permissions checking in the default model (is this
>>> correct?).
>> If you saying the case is only MAC should be applied but no DAC,
>> it is correct.
>>
>> I think four other cases that both of MAC/DAC should be bypassed.
>> - when temporary database objects are cleaned up on session closing.
>> - when ATRewriteTables() cleans up a temporary relation.
>> - when CLUSTER command cleans up a temporary relation.
>> - when autovacuum found an orphan temp table, and drop it.
>>
>> In this case, ac_object_drop() should not be called anyway,
>> as if ac_proc_create() is not called when caller does not want.
>
> Ok.
Sorry, I missed a point.
The shdepDropOwned() launched using DROP OWNED BY statement is a case that
we have no DAC for each database objects but MAC should be applied on them.
We can consider it as a variety of cascaded deletion, so ac_object_drop()
should be also put here.
>>>> In the current implementation, these permissions are checked on the
>>>> separated timing just after obtaining OID of namespace and procedure.
>>> Checking immediately after resolving OIDs seems fine to me. Clearly,
>>> that has to be done and it's really closer to 'parsing' than actually
>>> doing anything.
>> Please note that what factors are used depends on the security model
>> for the same required action, such as CREATE CONVERSION.
>> If we put ac_*() functions after the name->OID resolution, it breaks
>> the purpose of security abstraction layer.
>>
>> In this example, it makes access control decision to create a new
>> conversion, and raises an error if violated.
>> But the way to make the decision is encapsulated from the caller.
>> A configuration may make decision with the default PG model.
>> An other configuration may make decision with the default PG and SE-PG
>> model. The caller has to provide information to the ac_*() functions,
>> but it should not depend on a certain security model.
>
> I agree that what the caller provides shouldn't depend on the security
> model. I wasn't suggesting that it would. The name->OID resolution I
> was referring to was for things like getting the namespace OID, not
> getting the OID for the new conversion being created. Does that clear
> up the confusion here?
Sorry, confusable description.
What I would like to say is something like:
CreateXXXX()
{
namespaceId = LookupCreationNamespace();
ac_xxx_create_namespace(); --> only one use it, but other doesn't use?
:
tablespaceId = get_tablespace_oid();
ac_xxx_create_tablespace(); --> only DAC use it?
:
ac_xxx_create(); --> only MAC use it?
:
values[ ... ] = ObjectIdGetDatum(namespaceId);
values[ ... ] = ObjectIdGetDatum(tablespaceId);
simple_heap_insert();
:
}
When we create a new object X which needs OID of namespace and tablespace,
these names have to be resolved prior to updating system catalogs.
If we put ac_*() routines for each name resolution, it implicitly assumes
a certain security model and the ac_*() routine getting nonsense.
>>>> It is only necessary to provide enough information to make decision for
>>>> the ac_*() routines.
>>> Right.. Can we be consistant about having the permissions check done as
>>> soon as we have enough information to call the ac_*() routines then? I
>>> believe that's true in most cases, but not all, today. Any of those
>>> changes which change behaviour should be discussed in some way (such as
>>> an email to -hackers as you did for FindConversion()).
>> At least, the current implementation deploys ac_*() routines as soon as
>> the caller have enough information to provide it.
>
> Good. We should document where that changed behaviour though, but also
> document that this is a policy that we're going to try and stick with in
> the future (running ac_*() as soon as there is enough information to).
This policy focuses on developers, so it is enough to be source code comments?
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-10-01 02:17 Stephen Frost <sfrost@snowman.net>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 1 reply; 50+ messages in thread
From: Stephen Frost @ 2009-10-01 02:17 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
KaiGai,
* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> Yes, it is reasonable both of MAC/DAC to handle temporary schema as
> an exception of access controls on schemas.
Great.
> > I know it doesn't hide existence of major database objects. Depending
> > on the situation, there might be other information that could be leaked.
> > I realize that's not the case here, but I still want to catch and
> > document any behavioral changes, even if it's clear they shouldn't be an
> > issue.
>
> I agree that it should be documented.
> Where should I document them on? I guess the purpose of the description
> is to inform these behavior changes for users, not only developers.
> The official documentation sgml? wiki.postgresql.org? or, source code
> comments are enough?
What I would suggest is having a README or similar which accompanies the
patch. This could then be included by reference in the commit message,
or directly in the commit depending on what the committer prefers. Or,
it could just go into the mailing list and commitfest archives. The
point is to make sure the committer understands and isn't suprised when
reviewing the changes and comes across places where the code changes
result in a behaviour change.
If the changes are significant enough (and I don't think they will be,
to be honest..), they should be included by the committer in the commit
message and then picked up by Bruce, et al, when the release notes are
developed. I don't believe it needs to be in the formal PG
documentation, unless there's something documented there today which is
changing (very unlikely..).
> The shdepDropOwned() launched using DROP OWNED BY statement is a case that
> we have no DAC for each database objects but MAC should be applied on them.
> We can consider it as a variety of cascaded deletion, so ac_object_drop()
> should be also put here.
Right, that makes sense to me.
> > I agree that what the caller provides shouldn't depend on the security
> > model. I wasn't suggesting that it would. The name->OID resolution I
> > was referring to was for things like getting the namespace OID, not
> > getting the OID for the new conversion being created. Does that clear
> > up the confusion here?
>
> Sorry, confusable description.
>
> What I would like to say is something like:
>
> CreateXXXX()
> {
> namespaceId = LookupCreationNamespace();
> ac_xxx_create_namespace(); --> only one use it, but other doesn't use?
> :
> tablespaceId = get_tablespace_oid();
> ac_xxx_create_tablespace(); --> only DAC use it?
> :
> ac_xxx_create(); --> only MAC use it?
> :
> values[ ... ] = ObjectIdGetDatum(namespaceId);
> values[ ... ] = ObjectIdGetDatum(tablespaceId);
> simple_heap_insert();
> :
> }
>
> When we create a new object X which needs OID of namespace and tablespace,
> these names have to be resolved prior to updating system catalogs.
> If we put ac_*() routines for each name resolution, it implicitly assumes
> a certain security model and the ac_*() routine getting nonsense.
No, no. What I was suggesting and what I think we already do in most
places (but not everywhere and it's not really a policy) is this:
CreateXXXX()
{
namespaceId = LookupCreationNamespace();
tablespaceId = get_tablespace_oid();
ac_xxx_create();
:
values[ ... ] = ObjectIdGetDatum(namespaceId);
values[ ... ] = ObjectIdGetDatum(tablespaceId);
simple_heap_insert();
:
}
Which I think is what you're doing with this, it just might be a change
from what was done before when there were multiple permission checks
done.
> >> At least, the current implementation deploys ac_*() routines as soon as
> >> the caller have enough information to provide it.
> >
> > Good. We should document where that changed behaviour though, but also
> > document that this is a policy that we're going to try and stick with in
> > the future (running ac_*() as soon as there is enough information to).
>
> This policy focuses on developers, so it is enough to be source code comments?
Source code comments would be good for this. The only other place I
could think of it going would be on the developer part of the wiki.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (196B, ../../20091001021701.GY17756@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-10-01 03:09 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 0 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-01 03:09 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
Stephen Frost wrote:
>>> I know it doesn't hide existence of major database objects. Depending
>>> on the situation, there might be other information that could be leaked.
>>> I realize that's not the case here, but I still want to catch and
>>> document any behavioral changes, even if it's clear they shouldn't be an
>>> issue.
>> I agree that it should be documented.
>> Where should I document them on? I guess the purpose of the description
>> is to inform these behavior changes for users, not only developers.
>> The official documentation sgml? wiki.postgresql.org? or, source code
>> comments are enough?
>
> What I would suggest is having a README or similar which accompanies the
> patch. This could then be included by reference in the commit message,
> or directly in the commit depending on what the committer prefers. Or,
> it could just go into the mailing list and commitfest archives. The
> point is to make sure the committer understands and isn't suprised when
> reviewing the changes and comes across places where the code changes
> result in a behaviour change.
>
> If the changes are significant enough (and I don't think they will be,
> to be honest..), they should be included by the committer in the commit
> message and then picked up by Bruce, et al, when the release notes are
> developed. I don't believe it needs to be in the formal PG
> documentation, unless there's something documented there today which is
> changing (very unlikely..).
It may be good idea to put src/backend/security/README.
I'm not clear whether the commit message or release note is appropriate.
(it is unnoticeable if commit message, it is too details for release note.)
> No, no. What I was suggesting and what I think we already do in most
> places (but not everywhere and it's not really a policy) is this:
>
> CreateXXXX()
> {
> namespaceId = LookupCreationNamespace();
> tablespaceId = get_tablespace_oid();
> ac_xxx_create();
> :
> values[ ... ] = ObjectIdGetDatum(namespaceId);
> values[ ... ] = ObjectIdGetDatum(tablespaceId);
> simple_heap_insert();
> :
> }
>
> Which I think is what you're doing with this, it just might be a change
> from what was done before when there were multiple permission checks
> done.
Ahh, it was a communication bug. we talked same thing.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-10-01 03:10 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 1 reply; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-01 03:10 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> The attached patch eliminates permission checks in FindConversion()
>> and EnableDisableRule(), because these are nonsense or redundant.
>>
>> It is an separated issue from the ac_*() routines.
>> For now, we decided not to touch these stuffs in the access control
>> reworks patch. So, we can discuss about these fixes as a different
>> topic.
>>
>> See the corresponding messages:
>> http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
>> http://archives.postgresql.org/message-id/4ABC136A.90903@ak.jp.nec.com
>
> Thanks. To make sure it gets picked up, you might respond to Tom's
> message above with this same email. Just a thought.
The following message was my reply.
http://archives.postgresql.org/pgsql-hackers/2009-08/msg01420.php
Now I'm removing something with behavior change such as above patch,
and eliminating comments to be discussed in other thread.
I would like to quote them not to forget them away.
* ACL_CREATE checks on renaming type
When we rename an existing type, it checks ownership of the target
type, but ACL_CREATE on the namespace in which the type is stored,
although it is checked on renaming any other database objects stored
within a certain namespace, such as tables, functions and so on.
Is it an intended behavior?
* Ownership checks on REASSIGN OWNED BY statement
When we use the REASSIGN OWNER BY statement, it tries to change
ownership of the database objects which are owned by the target
role. It internally calls routine to change the ownership such
as AlterFunctionOwner_oid().
The routine depends on the class of objects, and they have a code
to check privilege to change ownership when it is actually changed
as follows:
But the code path corresponding to relations and types does not
have such kind of checks. IMO, similar check should be deployed.
-- at the AlterFunctionOwner_internal()
if (procForm->proowner != newOwnerId)
{
Datum repl_val[Natts_pg_proc];
bool repl_null[Natts_pg_proc];
bool repl_repl[Natts_pg_proc];
Acl *newAcl;
Datum aclDatum;
bool isNull;
HeapTuple newtuple;
/* 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));
}
:
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-10-01 03:17 Stephen Frost <sfrost@snowman.net>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 1 reply; 50+ messages in thread
From: Stephen Frost @ 2009-10-01 03:17 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers
* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> Stephen Frost wrote:
> > Thanks. To make sure it gets picked up, you might respond to Tom's
> > message above with this same email. Just a thought.
>
> The following message was my reply.
> http://archives.postgresql.org/pgsql-hackers/2009-08/msg01420.php
Right, but now there's actually a patch to go with it.. Just thinking
that Tom might pick up on it more easily if the patch was sent to that
thread, that's all. Of course, he seems to know all anyway, so it's
entirely likely that I'm just being silly.
> Now I'm removing something with behavior change such as above patch,
> and eliminating comments to be discussed in other thread.
> I would like to quote them not to forget them away.
That's fine. You'll start new threads with different subject lines for
them, right? That way other people will see the specific issues and
comment if they want to. I expect few people are actually following
this very long thread at this level. :)
THanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (196B, ../../20091001031729.GB17756@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* [PATCH] Reworks for Access Control facilities (r2350)
@ 2009-10-02 06:44 KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 1 reply; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-02 06:44 UTC (permalink / raw)
To: sfrost@snowman.net; +Cc: robertmhaas@gmail.com; pgsql-hackers; kaigai@kaigai.gr.jp
The attached patch is a revised version based on the previous
discussions at:
http://archives.postgresql.org/message-id/20090929105431.GO17756@tamriel.snowman.net
http://archives.postgresql.org/message-id/4AC1EA9E.3080907@kaigai.gr.jp
http://archives.postgresql.org/message-id/20090929173049.GP17756@tamriel.snowman.net
http://archives.postgresql.org/message-id/4AC2BDD0.7050906@ak.jp.nec.com
http://archives.postgresql.org/message-id/20090930105911.GS17756@tamriel.snowman.net
http://archives.postgresql.org/message-id/4AC40133.4080509@ak.jp.nec.com
Please review the new revision, Thanks,
* List of updates
- code base was updated to the latest CVS HEAD.
- reverted changes on FindConversion() and EnableDisableRule().
these changes are discussed in the different topics.
- removed uncertain comment at the restrict_grant().
- added comment about SQL specifications for each ac_xxx_grant().
- eliminate MEMO: and FIXME: prefix
- moved ac_language_create() prior to the CreateProcedure() because
it may update the pg_proc system catalog.
- removed ac_schema_search() invocations when the target namespace is
obviously temporary namespace. And, added a comment to bypass checks
for both of DAC/MAC on temporary namespaces.
- uncommented "ac_object_drop() should be here", and added actual
ac_object_drop() at the performDeletion() and performMultipleDeletion().
The 'permission' argument was added to these functions.
- uncommented "ac_attribute_xxxx() should be here", and put actual
ac_attribute_create() and ac_attribute_drop() calls here.
- ac_aggregate_execute() function was added.
- add a memo for minor behavior changes at src/backend/security/README
(It is a initial description, so needs more brushing up)
$ diffstat sepgsql-01-base-8.5devel-r2350.patch.gz
backend/Makefile | 2
backend/catalog/aclchk.c | 254 !
backend/catalog/dependency.c | 31
backend/catalog/heap.c | 2
backend/catalog/namespace.c | 54
backend/catalog/pg_aggregate.c | 12
backend/catalog/pg_operator.c | 42
backend/catalog/pg_proc.c | 29
backend/catalog/pg_shdepend.c | 13
backend/catalog/pg_type.c | 25
backend/commands/aggregatecmds.c | 44
backend/commands/alter.c | 78
backend/commands/analyze.c | 5
backend/commands/cluster.c | 11
backend/commands/comment.c | 125
backend/commands/conversioncmds.c | 73
backend/commands/copy.c | 40
backend/commands/dbcommands.c | 160 !
backend/commands/foreigncmds.c | 150
backend/commands/functioncmds.c | 132
backend/commands/indexcmds.c | 120
backend/commands/lockcmds.c | 17
backend/commands/opclasscmds.c | 246 !
backend/commands/operatorcmds.c | 72
backend/commands/proclang.c | 63
backend/commands/schemacmds.c | 62
backend/commands/sequence.c | 38
backend/commands/tablecmds.c | 370 -
backend/commands/tablespace.c | 46
backend/commands/trigger.c | 43
backend/commands/tsearchcmds.c | 182 !
backend/commands/typecmds.c | 143 !
backend/commands/user.c | 183 !
backend/commands/vacuum.c | 5
backend/commands/view.c | 7
backend/executor/execMain.c | 208 !
backend/executor/execQual.c | 16
backend/executor/nodeAgg.c | 38
backend/executor/nodeMergejoin.c | 8
backend/executor/nodeWindowAgg.c | 42
backend/optimizer/util/clauses.c | 6
backend/parser/parse_utilcmd.c | 13
backend/postmaster/autovacuum.c | 2
backend/rewrite/rewriteDefine.c | 5
backend/rewrite/rewriteRemove.c | 8
backend/security/Makefile | 10
backend/security/README | 294 ++
backend/security/access_control.c | 4593 ++++++++++++++++++++++++++++++++++++++
backend/tcop/fastpath.c | 15
backend/tcop/utility.c | 74
backend/utils/adt/dbsize.c | 25
backend/utils/adt/ri_triggers.c | 24
backend/utils/adt/tid.c | 18
backend/utils/init/postinit.c | 15
include/catalog/dependency.h | 4
include/catalog/pg_proc_fn.h | 1
include/commands/defrem.h | 1
include/utils/security.h | 348 ++
58 files changed, 5747 insertions(+), 914 deletions(-), 1986 modifications(!)
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Attachments:
[application/gzip] sepgsql-01-base-8.5devel-r2350.patch.gz (80.9K, ../../4AC5A14F.6050306@ak.jp.nec.com/2-sepgsql-01-base-8.5devel-r2350.patch.gz)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-10-05 20:59 Robert Haas <robertmhaas@gmail.com>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 1 reply; 50+ messages in thread
From: Robert Haas @ 2009-10-05 20:59 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; KaiGai Kohei <kaigai@kaigai.gr.jp>; pgsql-hackers
On Wed, Sep 30, 2009 at 11:17 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> Stephen Frost wrote:
>> > Thanks. Â To make sure it gets picked up, you might respond to Tom's
>> > message above with this same email. Â Just a thought.
>>
>> The following message was my reply.
>> Â http://archives.postgresql.org/pgsql-hackers/2009-08/msg01420.php
>
> Right, but now there's actually a patch to go with it.. Â Just thinking
> that Tom might pick up on it more easily if the patch was sent to that
> thread, that's all. Â Of course, he seems to know all anyway, so it's
> entirely likely that I'm just being silly.
>
>> Now I'm removing something with behavior change such as above patch,
>> and eliminating comments to be discussed in other thread.
>> I would like to quote them not to forget them away.
>
> That's fine. Â You'll start new threads with different subject lines for
> them, right? Â That way other people will see the specific issues and
> comment if they want to. Â I expect few people are actually following
> this very long thread at this level. :)
So what's the status of this patch currently?
...Robert
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-10-05 21:57 Stephen Frost <sfrost@snowman.net>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 50+ messages in thread
From: Stephen Frost @ 2009-10-05 21:57 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; KaiGai Kohei <kaigai@kaigai.gr.jp>; pgsql-hackers
* Robert Haas (robertmhaas@gmail.com) wrote:
> So what's the status of this patch currently?
I'll be reviewing the updates shortly. After that, I'd like a committer
to review it.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (196B, ../../20091005215734.GR17756@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2311)
@ 2009-10-06 02:32 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Stephen Frost <sfrost@snowman.net>
0 siblings, 0 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-06 02:32 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: Robert Haas <robertmhaas@gmail.com>; KaiGai Kohei <kaigai@kaigai.gr.jp>; pgsql-hackers
Stephen Frost wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> So what's the status of this patch currently?
>
> I'll be reviewing the updates shortly. After that, I'd like a committer
> to review it.
Do you think this version also should rework an invocation of
pg_namespace_aclcheck() newly added due to the default ACL feature?
IMO, we don't need to hurry-up to catch up these new features just
now, because we have only a week in this commit fest.
It is desirable to improve the patch being commitable earlier.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2350)
@ 2009-10-12 02:46 Stephen Frost <sfrost@snowman.net>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 2 replies; 50+ messages in thread
From: Stephen Frost @ 2009-10-12 02:46 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: robertmhaas@gmail.com; pgsql-hackers; kaigai@kaigai.gr.jp
KaiGai,
* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> Please review the new revision, Thanks,
In general, I'm pretty happy with this revision. You still have a
number of places where you have comments about code which does not exist
any more. For example, the comments about the check being removed from
LookupCreationNamespace. I would recommend pulling out those comments
and instead having a comment at the top of the function that says
"namespace creation permission checks are handled in the individual
object ac_*_create() routines".
I don't like having comments that are about code which was removed.
Some of these could be moved to the README if they aren't there already
and they really need to be kept.
There are some other grammatical and spelling issues in the comments,
but I don't believe any of this should hold this patch up from being
ready for committer. At a minimum, I think this really needs to have a
committer comment on it to ensure we're going in the right direction.
I'd be happy to continue working with KaiGai to review his changes going
forward, either with the next set of SE-PG patches or reworking this one
if necessary.
Thanks,
Stephen
Attachments:
[application/pgp-signature] signature.asc (196B, ../../20091012024604.GW17756@tamriel.snowman.net/2-signature.asc)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: [PATCH] Reworks for Access Control facilities (r2350)
@ 2009-10-13 01:17 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 0 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-13 01:17 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: robertmhaas@gmail.com; pgsql-hackers; kaigai@kaigai.gr.jp
Stephen, Thanks for your reviewing comments, although you have busy days.
Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> Please review the new revision, Thanks,
>
> In general, I'm pretty happy with this revision. You still have a
> number of places where you have comments about code which does not exist
> any more. For example, the comments about the check being removed from
> LookupCreationNamespace. I would recommend pulling out those comments
> and instead having a comment at the top of the function that says
> "namespace creation permission checks are handled in the individual
> object ac_*_create() routines".
>
> I don't like having comments that are about code which was removed.
> Some of these could be moved to the README if they aren't there already
> and they really need to be kept.
OK, I'll check and revise these commenting issues soon.
Please wait for a couple of days at most.
> There are some other grammatical and spelling issues in the comments,
> but I don't believe any of this should hold this patch up from being
> ready for committer. At a minimum, I think this really needs to have a
> committer comment on it to ensure we're going in the right direction.
> I'd be happy to continue working with KaiGai to review his changes going
> forward, either with the next set of SE-PG patches or reworking this one
> if necessary.
>
> Thanks,
>
> Stephen
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Reworks for Access Control facilities (r2363)
@ 2009-10-14 03:07 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Stephen Frost <sfrost@snowman.net>
1 sibling, 2 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-14 03:07 UTC (permalink / raw)
To: Stephen Frost <sfrost@snowman.net>; +Cc: robertmhaas@gmail.com; pgsql-hackers; kaigai@kaigai.gr.jp
The attached patch is a revised one with the following updates:
- rebased to the latest CVS HEAD
- eliminated comments about code which already removed, such as "we had
ACL_xxx checks here, but it is moved to ac_xxx_create()", and some of
notifications are moved to the README.
(comments about LookupCreationNamespace() and CheckRelationOwnership())
- removed ac_relation_permission() invocation from OpenIntoRel()
because the default PG model uses the perspective CREATE TABLE AS is
an atomic operation, due to the defaultACL thread.
(It is already talked with Stephen, and agreed.)
- fixed two bugs:
* ac_index_create() didn't bypass checks on bootstraping mode.
* ac_schema_alter() didn't checks ACL_CREATE on changing owner.
Thanks,
Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> Please review the new revision, Thanks,
>
> In general, I'm pretty happy with this revision. You still have a
> number of places where you have comments about code which does not exist
> any more. For example, the comments about the check being removed from
> LookupCreationNamespace. I would recommend pulling out those comments
> and instead having a comment at the top of the function that says
> "namespace creation permission checks are handled in the individual
> object ac_*_create() routines".
>
> I don't like having comments that are about code which was removed.
> Some of these could be moved to the README if they aren't there already
> and they really need to be kept.
>
> There are some other grammatical and spelling issues in the comments,
> but I don't believe any of this should hold this patch up from being
> ready for committer. At a minimum, I think this really needs to have a
> committer comment on it to ensure we're going in the right direction.
> I'd be happy to continue working with KaiGai to review his changes going
> forward, either with the next set of SE-PG patches or reworking this one
> if necessary.
>
> Thanks,
>
> Stephen
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Attachments:
[application/gzip] sepgsql-01-base-8.5devel-r2363.patch.gz (81.0K, ../../4AD54082.9050001@ak.jp.nec.com/2-sepgsql-01-base-8.5devel-r2363.patch.gz)
download
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-15 01:08 Robert Haas <robertmhaas@gmail.com>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
1 sibling, 2 replies; 50+ messages in thread
From: Robert Haas @ 2009-10-15 01:08 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: Stephen Frost <sfrost@snowman.net>; pgsql-hackers; kaigai@kaigai.gr.jp
2009/10/13 KaiGai Kohei <kaigai@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
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-15 01:19 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 50+ messages in thread
From: Tom Lane @ 2009-10-15 01:19 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; kaigai@kaigai.gr.jp
Robert Haas <robertmhaas@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
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-15 01:21 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 0 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-15 01:21 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Stephen Frost <sfrost@snowman.net>; pgsql-hackers; kaigai@kaigai.gr.jp
Robert Haas wrote:
> 2009/10/13 KaiGai Kohei <kaigai@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 <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-15 02:13 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 50+ messages in thread
From: Robert Haas @ 2009-10-15 02:13 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; kaigai@kaigai.gr.jp
On Wed, Oct 14, 2009 at 9:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@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
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-15 17:22 Tom Lane <tgl@sss.pgh.pa.us>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
1 sibling, 2 replies; 50+ messages in thread
From: Tom Lane @ 2009-10-15 17:22 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: Stephen Frost <sfrost@snowman.net>; robertmhaas@gmail.com; pgsql-hackers; kaigai@kaigai.gr.jp
KaiGai Kohei <kaigai@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
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-16 01:31 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 2 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-16 01:31 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Stephen Frost <sfrost@snowman.net>; robertmhaas@gmail.com; pgsql-hackers; kaigai@kaigai.gr.jp
Tom Lane wrote:
> KaiGai Kohei <kaigai@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 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 loadable kernel module is a good analogy. It is allowed anything
because kernel module can access directly without system-call invocations.
However, OS checks whether the admin has an appropriate privilege, or not,
when the kernel module tries to be loaded.
It is also DBA's decision whether he allows to load a third-party module,
or not.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-16 01:56 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 50+ messages in thread
From: Robert Haas @ 2009-10-16 01:56 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; kaigai@kaigai.gr.jp
On Thu, Oct 15, 2009 at 1:22 PM, Tom Lane <tgl@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
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-16 02:13 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 0 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-16 02:13 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; kaigai@kaigai.gr.jp
Robert Haas wrote:
> On Thu, Oct 15, 2009 at 1:22 PM, Tom Lane <tgl@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 <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-16 09:37 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
1 sibling, 1 reply; 50+ messages in thread
From: Heikki Linnakangas @ 2009-10-16 09:37 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Stephen Frost <sfrost@snowman.net>; robertmhaas@gmail.com; pgsql-hackers; kaigai@kaigai.gr.jp
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
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-16 16:45 Greg Stark <gsstark@mit.edu>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
1 sibling, 3 replies; 50+ messages in thread
From: Greg Stark @ 2009-10-16 16:45 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Stephen Frost <sfrost@snowman.net>; robertmhaas@gmail.com; pgsql-hackers; kaigai@kaigai.gr.jp
2009/10/16 KaiGai Kohei <kaigai@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
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-16 17:33 Tom Lane <tgl@sss.pgh.pa.us>
parent: Greg Stark <gsstark@mit.edu>
2 siblings, 0 replies; 50+ messages in thread
From: Tom Lane @ 2009-10-16 17:33 UTC (permalink / raw)
To: Greg Stark <gsstark@mit.edu>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; Stephen Frost <sfrost@snowman.net>; robertmhaas@gmail.com; pgsql-hackers; kaigai@kaigai.gr.jp
Greg Stark <gsstark@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
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-16 17:49 Robert Haas <robertmhaas@gmail.com>
parent: Greg Stark <gsstark@mit.edu>
2 siblings, 0 replies; 50+ messages in thread
From: Robert Haas @ 2009-10-16 17:49 UTC (permalink / raw)
To: Greg Stark <gsstark@mit.edu>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; Tom Lane <tgl@sss.pgh.pa.us>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers; kaigai@kaigai.gr.jp
On Fri, Oct 16, 2009 at 12:45 PM, Greg Stark <gsstark@mit.edu> wrote:
> 2009/10/16 KaiGai Kohei <kaigai@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
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-17 04:28 KaiGai Kohei <kaigai@kaigai.gr.jp>
parent: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
0 siblings, 1 reply; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-17 04:28 UTC (permalink / raw)
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; Tom Lane <tgl@sss.pgh.pa.us>; Stephen Frost <sfrost@snowman.net>; robertmhaas@gmail.com; pgsql-hackers
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 controls, such kind of
reworks are necessary. The current pg_xxx_aclcheck() routines are designed
for the database ACL model. It works fine and correctly.
However, it is not suitable to host the SELinux's model within the hooks.
>>> * 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.
OK, I can understand what he wanted to say.
In the kernel module cases, we can find out modules which provide its own
checks based on user/group identifier, so it also means using the loadable
module allows to bypass MAC checks. However, the loadable module is loaded
at first, and the administrator (or init script) is allowed to load such
a loadable module which bypasses MAC checks.
In other word, the security policy basically controls whole of the usage
of these modules. Needless to say, we can add security hooks if necessary.
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 MAC checks.
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-17 04:37 KaiGai Kohei <kaigai@kaigai.gr.jp>
parent: Greg Stark <gsstark@mit.edu>
2 siblings, 0 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-17 04:37 UTC (permalink / raw)
To: Greg Stark <gsstark@mit.edu>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; Tom Lane <tgl@sss.pgh.pa.us>; Stephen Frost <sfrost@snowman.net>; robertmhaas@gmail.com; pgsql-hackers
Greg Stark wrote:
> 2009/10/16 KaiGai Kohei <kaigai@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 <kaigai@kaigai.gr.jp>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-17 13:53 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
parent: KaiGai Kohei <kaigai@kaigai.gr.jp>
0 siblings, 2 replies; 50+ messages in thread
From: Heikki Linnakangas @ 2009-10-17 13:53 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@kaigai.gr.jp>; +Cc: KaiGai Kohei <kaigai@ak.jp.nec.com>; Tom Lane <tgl@sss.pgh.pa.us>; Stephen Frost <sfrost@snowman.net>; robertmhaas@gmail.com; pgsql-hackers
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 MAC checks.
Yeah, it seems like any external modules need to be modified or at least
verified to comply with the MAC requirements. Your point 2) about
whether permissions are ANDed or ORred together seem to be the key here.
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.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-18 01:12 Robert Haas <robertmhaas@gmail.com>
parent: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
1 sibling, 1 reply; 50+ messages in thread
From: Robert Haas @ 2009-10-18 01:12 UTC (permalink / raw)
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; KaiGai Kohei <kaigai@ak.jp.nec.com>; Tom Lane <tgl@sss.pgh.pa.us>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers
On Sat, Oct 17, 2009 at 9:53 AM, Heikki Linnakangas
<heikki.linnakangas@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
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-19 03:59 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
1 sibling, 1 reply; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-19 03:59 UTC (permalink / raw)
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; Tom Lane <tgl@sss.pgh.pa.us>; Stephen Frost <sfrost@snowman.net>; robertmhaas@gmail.com; pgsql-hackers
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 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.
It may be an ideal goal, but it is far from what we tries to do.
Some of actions can be allowed without any MAC checks more than
"if (superuser())" which internally checks SE-PgSQL's permission
to perform superuser in DAC.
The most significant purpose of MAC is to control data-flows between
processes via shared resources (such as files, databases).
So, SELinux/SE-PgSQL primarily focuses on the operation to access
database objects. An important thing is to make clear the priority
what actions should be also checked by MAC, not only signle superuser
privilege.
I don't believe a single patch which add checks on various kind of
database objects is acceptable within a reasonable time-frame.
But we can adopt incremental approach. It is not necessary to put
SE-PgSQL's hooks near the all of "if (superuser())" at beginning.
>> 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 MAC checks.
>
> Yeah, it seems like any external modules need to be modified or at least
> verified to comply with the MAC requirements. Your point 2) about
> whether permissions are ANDed or ORred together seem to be the key here.
>
> 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.
Yes, I also think we need a documentation from developer viewpoint.
(not only user documentation)
I think it should contains the following items.
* overview and architecture
(including differences from the default PG's model?)
* what permissions are defined in SELinux model
* when/where they should be checked
* specification of SE-PgSQL hooks
What item should be described in the developer documentation any other?
In generally, what I want to describe may not match with what people want
to know.
Thanks,
BTW, I have to allocate my activity on Japan Linux Symposium in this week.
So, my response may be delayed. Sorry.
http://events.linuxfoundation.org/events/japan-linux-symposium/schedule
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-19 04:21 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 0 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-19 04:21 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; KaiGai Kohei <kaigai@kaigai.gr.jp>; Tom Lane <tgl@sss.pgh.pa.us>; Stephen Frost <sfrost@snowman.net>; pgsql-hackers
Robert Haas wrote:
> On Sat, Oct 17, 2009 at 9:53 AM, Heikki Linnakangas
> <heikki.linnakangas@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 <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-19 07:01 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
parent: KaiGai Kohei <kaigai@ak.jp.nec.com>
0 siblings, 1 reply; 50+ messages in thread
From: Heikki Linnakangas @ 2009-10-19 07:01 UTC (permalink / raw)
To: KaiGai Kohei <kaigai@ak.jp.nec.com>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; Tom Lane <tgl@sss.pgh.pa.us>; Stephen Frost <sfrost@snowman.net>; robertmhaas@gmail.com; pgsql-hackers
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
^ permalink raw reply [nested|flat] 50+ messages in thread
* Re: Reworks for Access Control facilities (r2363)
@ 2009-10-19 07:27 KaiGai Kohei <kaigai@ak.jp.nec.com>
parent: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
0 siblings, 0 replies; 50+ messages in thread
From: KaiGai Kohei @ 2009-10-19 07:27 UTC (permalink / raw)
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; +Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>; Tom Lane <tgl@sss.pgh.pa.us>; Stephen Frost <sfrost@snowman.net>; robertmhaas@gmail.com; pgsql-hackers
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 <kaigai@ak.jp.nec.com>
^ permalink raw reply [nested|flat] 50+ messages in thread
end of thread, other threads:[~2009-10-19 07:27 UTC | newest]
Thread overview: 50+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2009-08-25 02:10 [PATCH] Reworks for Access Control facilities (r2251) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-09-03 06:50 ` [PATCH] Reworks for Access Control facilities (r2277) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-08-25 02:24 [PATCH] Reworks for Access Control facilities (r2251) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-08-25 02:54 ` KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-08-25 03:16 ` Alvaro Herrera <alvherre@commandprompt.com>
2009-09-14 06:12 [PATCH] Reworks for Access Control facilities (r2311) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-09-25 00:48 ` Re: [PATCH] Reworks for Access Control facilities (r2311) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-09-27 16:56 ` Re: [PATCH] Reworks for Access Control facilities (r2311) Robert Haas <robertmhaas@gmail.com>
2009-09-29 01:33 ` Re: [PATCH] Reworks for Access Control facilities (r2311) Stephen Frost <sfrost@snowman.net>
2009-09-29 02:19 ` Re: [PATCH] Reworks for Access Control facilities (r2311) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-09-29 10:54 Re: [PATCH] Reworks for Access Control facilities (r2311) Stephen Frost <sfrost@snowman.net>
2009-09-29 11:08 ` Re: [PATCH] Reworks for Access Control facilities (r2311) KaiGai Kohei <kaigai@kaigai.gr.jp>
2009-09-29 17:30 ` Re: [PATCH] Reworks for Access Control facilities (r2311) Stephen Frost <sfrost@snowman.net>
2009-09-30 02:09 ` Re: [PATCH] Reworks for Access Control facilities (r2311) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-09-30 05:32 ` Re: [PATCH] Reworks for Access Control facilities (r2311) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-09-30 12:23 ` Re: [PATCH] Reworks for Access Control facilities (r2311) Stephen Frost <sfrost@snowman.net>
2009-10-01 03:10 ` Re: [PATCH] Reworks for Access Control facilities (r2311) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-10-01 03:17 ` Re: [PATCH] Reworks for Access Control facilities (r2311) Stephen Frost <sfrost@snowman.net>
2009-10-05 20:59 ` Re: [PATCH] Reworks for Access Control facilities (r2311) Robert Haas <robertmhaas@gmail.com>
2009-10-05 21:57 ` Re: [PATCH] Reworks for Access Control facilities (r2311) Stephen Frost <sfrost@snowman.net>
2009-10-06 02:32 ` Re: [PATCH] Reworks for Access Control facilities (r2311) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-09-30 10:59 ` Re: [PATCH] Reworks for Access Control facilities (r2311) Stephen Frost <sfrost@snowman.net>
2009-10-01 01:09 ` Re: [PATCH] Reworks for Access Control facilities (r2311) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-10-01 02:17 ` Re: [PATCH] Reworks for Access Control facilities (r2311) Stephen Frost <sfrost@snowman.net>
2009-10-01 03:09 ` Re: [PATCH] Reworks for Access Control facilities (r2311) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-09-29 13:41 ` Re: [PATCH] Reworks for Access Control facilities (r2311) Robert Haas <robertmhaas@gmail.com>
2009-10-02 06:44 [PATCH] Reworks for Access Control facilities (r2350) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-10-12 02:46 ` Re: [PATCH] Reworks for Access Control facilities (r2350) Stephen Frost <sfrost@snowman.net>
2009-10-13 01:17 ` Re: [PATCH] Reworks for Access Control facilities (r2350) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-10-14 03:07 ` Reworks for Access Control facilities (r2363) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-10-15 01:08 ` Re: Reworks for Access Control facilities (r2363) Robert Haas <robertmhaas@gmail.com>
2009-10-15 01:19 ` Re: Reworks for Access Control facilities (r2363) Tom Lane <tgl@sss.pgh.pa.us>
2009-10-15 02:13 ` Re: Reworks for Access Control facilities (r2363) Robert Haas <robertmhaas@gmail.com>
2009-10-15 01:21 ` Re: Reworks for Access Control facilities (r2363) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-10-15 17:22 ` Re: Reworks for Access Control facilities (r2363) Tom Lane <tgl@sss.pgh.pa.us>
2009-10-16 01:31 ` Re: Reworks for Access Control facilities (r2363) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-10-16 09:37 ` Re: Reworks for Access Control facilities (r2363) Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
2009-10-17 04:28 ` Re: Reworks for Access Control facilities (r2363) KaiGai Kohei <kaigai@kaigai.gr.jp>
2009-10-17 13:53 ` Re: Reworks for Access Control facilities (r2363) Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
2009-10-18 01:12 ` Re: Reworks for Access Control facilities (r2363) Robert Haas <robertmhaas@gmail.com>
2009-10-19 04:21 ` Re: Reworks for Access Control facilities (r2363) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-10-19 03:59 ` Re: Reworks for Access Control facilities (r2363) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-10-19 07:01 ` Re: Reworks for Access Control facilities (r2363) Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
2009-10-19 07:27 ` Re: Reworks for Access Control facilities (r2363) KaiGai Kohei <kaigai@ak.jp.nec.com>
2009-10-16 16:45 ` Re: Reworks for Access Control facilities (r2363) Greg Stark <gsstark@mit.edu>
2009-10-16 17:33 ` Re: Reworks for Access Control facilities (r2363) Tom Lane <tgl@sss.pgh.pa.us>
2009-10-16 17:49 ` Re: Reworks for Access Control facilities (r2363) Robert Haas <robertmhaas@gmail.com>
2009-10-17 04:37 ` Re: Reworks for Access Control facilities (r2363) KaiGai Kohei <kaigai@kaigai.gr.jp>
2009-10-16 01:56 ` Re: Reworks for Access Control facilities (r2363) Robert Haas <robertmhaas@gmail.com>
2009-10-16 02:13 ` Re: Reworks for Access Control facilities (r2363) KaiGai Kohei <kaigai@ak.jp.nec.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox