Re: [PATCH] Reworks for Access Control facilities (r2311)

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>