Re: [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(at)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(at)ak(dot)jp(dot)nec(dot)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.

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(at)ak(dot)jp(dot)nec(dot)com>

2009/9/24 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>