Thread: [PATCH] Reworks for Access Control facilities (r2311)
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>
Attachment
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>
Attachment
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
* 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
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=31e6bde46c0594c6f8bb82606c49f93295f1195c#patch8 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>
* 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
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 namespacebut 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/dependency.c#959 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. Itallows 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>
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
* 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
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>
>>>>> 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> 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); }
* 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
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
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(); -->
onlyMAC 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>
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
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>
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 existingobject */ 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>
* 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
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
* 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
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>