Re: [PATCH] Reworks for Access Control facilities (r2311)
От | KaiGai Kohei |
---|---|
Тема | Re: [PATCH] Reworks for Access Control facilities (r2311) |
Дата | |
Msg-id | 4ABC136A.90903@ak.jp.nec.com обсуждение исходный текст |
Ответ на | [PATCH] Reworks for Access Control facilities (r2311) (KaiGai Kohei <kaigai@ak.jp.nec.com>) |
Ответы |
Re: [PATCH] Reworks for Access Control facilities (r2311)
(Robert Haas <robertmhaas@gmail.com>)
Re: [PATCH] Reworks for Access Control facilities (r2311) (Stephen Frost <sfrost@snowman.net>) |
Список | pgsql-hackers |
I noticed that the previous patch (r2311) fails to apply on the CVS HEAD. The attached patch is only rebased to the latest CVS HEAD, without any other changes. BTW, I raised a few issues. Do you have any opinions? * deployment of the source code The current patch implements all the access control abstractions at the src/backend/security/access_control.c. Its size is about 4,500 lines which includs source comments. It is an approach to sort out a series of functionalities into a single big file, such as aclchk.c. One other approach is to put these codes in the short many files deployed in a directory, such as backend/catalog/*. Which is the preferable in PostgreSQL? If the later one is preferable, I can reorganize the access control abstraction functions as follows, instead of the access_control.c. src/backend/security/ac/ac_database.c ac_schema.c ac_relation.c : * pg_class_ownercheck() in EnableDisableRule() As I mentioned in the another message, pg_class_ownercheck() in the EnableDisableRule() is redundant. The EnableDisableRule() is called from ATExecEnableDisableRule() only, and it is also called from the ATExecCmd() with AT_EnableRule, AT_EnableAlwaysRule, AT_EnableReplicaRule and AT_DisableRule. In this path, ATPrepCmd() already calls ATSimplePermissions() which also calls pg_class_ownercheck() for the target. I don't think it is necessary to check ownership of the relation twice. My opinion is to remove the checks from the EnableDisableRule() and the ac_rule_toggle() is also removed from the patch. It does not have any compatibility issue. Any comments? KaiGai Kohei wrote: > The attached patch is an updated version of reworks for access control > facilities, and a short introduction to help reviewing not-obvious part > of the patch. > > List of updates: > - Rebased to the latest CVS HEAD. > - bugfix: ac_opfamily_create() was not called on DefineOpClass(). > - bugfix: ac_trigger_create() was moved to the proper point. > - Add more source code comments. > > == Purpose == > > The purpose of this patch is to provide a common abstraction layer for > various kind of upcoming access control facilities. > Access control is to make an access control decision whether the give > access can be allowed, or not. The way to decide it depends on the > security model. For example, when we create a new table, it should be > checked whether the client has enough privilege to create a new one. > The native database privilege stuff checks ACL_CREATE permission on > the namespace to be placed. It is the way to make a decision on access > controls. > > Now PostgreSQL has only one access control mechanism, and its routines > to make access control decisions are invoked from all over the backend > implementation. (Please count pg_xxx_aclcheck() and pg_xxx_ownercheck().) > It also means we need to put a similar number of invocations of security > hooks, when we implement a new security mechanism in addition to the > native one. > > The common abstraction layer performs as entry points for each security > features (including the native database privilege), and enables to minimize > the impact when a new security feature is added. > > == Implementation == > > Basically, this patch replaces a series of pg_xxx_aclcheck() and > pg_xxx_ownercheck() invocations by the new abstraction function. > The abstraction functions invokes equivalent acl/owner check routines, > and it allows us to put other security checks within the abstraction > functions. > > For example, when we want to check client's privilege to execute a certain > function, pg_proc_aclcheck(ACL_EXECUTE) was called from several points. > This patch replaces it by ac_proc_execute(procOid) which internally invokes > pg_proc_aclcheck() as follows: > > ------------------------------------------------ > *** 1029,1040 **** > init_fcache(Oid foid, FuncExprState *fcache, > MemoryContext fcacheCxt, bool needDescForSets) > { > - AclResult aclresult; > - > /* Check permission to call function */ > ! aclresult = pg_proc_aclcheck(foid, GetUserId(), ACL_EXECUTE); > ! if (aclresult != ACLCHECK_OK) > ! aclcheck_error(aclresult, ACL_KIND_PROC, get_func_name(foid)); > > /* > * Safety check on nargs. Under normal circumstances this should never > --- 1029,1036 ---- > init_fcache(Oid foid, FuncExprState *fcache, > MemoryContext fcacheCxt, bool needDescForSets) > { > /* Check permission to call function */ > ! ac_proc_execute(foid, GetUserId()); > > /* > * Safety check on nargs. Under normal circumstances this should never > ------------------------------------------------ > > And, > > ------------------------------------------------ > + /* > + * ac_proc_execute > + * > + * It checks privilege to execute a certain function. > + * > + * Note that it should be checked on the function runtime. > + * Some of DDL statements requires ACL_EXECUTE on creation time, such as > + * CreateConversionCommand(), however, these are individually checked on > + * the ac_xxx_create() hook. > + * > + * [Params] > + * proOID : OID of the function to be executed > + * roleOid : OID of the database role to be evaluated > + */ > + void > + ac_proc_execute(Oid proOid, Oid roleOid) > + { > + AclResult aclresult; > + > + aclresult = pg_proc_aclcheck(proOid, roleOid, ACL_EXECUTE); > + if (aclresult != ACLCHECK_OK) > + aclcheck_error(aclresult, ACL_KIND_PROC, get_func_name(proOid)); > + } > ------------------------------------------------ > > It is obvious that we can add invocations of different kind of security > checks at the tail of this abstraction function. It also can be a check > based on MAC security policy. > > All the abstraction functions have the following convention. > > ac_<object class>_<type of action>([args ...]); > > The object class shows what kind of database object is checked. For example, > here is "relation", "proc", "database" and so on. Most of then are equivalent > to the name of system catalog. > > The type of action shows what kind of action is required on the object. > It is specific for each object classes, but the "_create", "_alter" and > "_drop" are common for each object classes. (A few object classes without > its ALTER statement does not have "_alter" function.) > > Most of abstraction functions will be obvious what does it replaces. > > However, "ac_*_create" function tends not to be obvious, because some of > them requires multiple permissions to create a new database object. > For example, when we create a new function, the following permissions are > checked in the native database privilege mechanism. > > - ACL_CREATE on the namespace. > - ACL_USAGE on the procedural language, or superuser privilege if untrusted. > - Ownership of the procedure to be replaced, if CREATE OR REPLACE is used. > > The original implementation checks these permissions at the different points, > but this patch consolidated them within a ac_proc_create() function. > > The "_alter" function may also seems a bit not-obvious, because required > permission depends on the alter option. > For example, ALTER TABLE ... RENAME TO statement required ACL_CREATE on > the current namespace, not only ownership of the target table. So, it > should be checked conditionally, if the ALTER TABLE statement tries to > alter the name. > In the typical "_alter" functions, it checks ownership of the target object, > and it also checks ACL_CREATE on the namespace if renamed, and it also checks > role membership and ACL_CREATE on the namespace with the new owner if owner > changed. > > The restrict_and_check_grant() has checked client's privilege to grant/revoke > something on the given database object, and dropped permissions being > available to grant/revoke. > It is separated into two parts. The earlier part is replaced by > the ac_xxx_grant() function to check client's privilege to grant/revoke it. > The later part is still common to drop unavailable permissions to grant/reveke, > as follows: > > ------------------------------------------------ > *************** ExecGrant_Function(InternalGrant *istmt) > *** 1562,1577 **** > old_acl, ownerId, > &grantorId, &avail_goptions); > > /* > * Restrict the privileges to what we can actually grant, and emit the > * standards-mandated warning and error messages. > */ > this_privileges = > ! restrict_and_check_grant(istmt->is_grant, avail_goptions, > ! istmt->all_privs, istmt->privileges, > ! funcId, grantorId, ACL_KIND_PROC, > ! NameStr(pg_proc_tuple->proname), > ! 0, NULL); > > /* > * Generate new ACL. > --- 1508,1524 ---- > old_acl, ownerId, > &grantorId, &avail_goptions); > > + /* Permission check to grant/revoke */ > + ac_proc_grant(funcId, grantorId, avail_goptions); > + > /* > * Restrict the privileges to what we can actually grant, and emit the > * standards-mandated warning and error messages. > */ > this_privileges = > ! restrict_grant(istmt->is_grant, avail_goptions, > ! istmt->all_privs, istmt->privileges, > ! NameStr(pg_proc_tuple->proname)); > > /* > * Generate new ACL. > ------------------------------------------------ > > > == Needs any comments == > > Currently, all the abstraction functions are placed at > the src/backend/security/access_control.c, but it has about 4400 lines. > > It might be preferable to deploy these abstraction functions categorized > by object classes, because it may help to lookup abstraction functions, > as follows: > > src/backend/security/ac/ac_database.c > /ac_schema.c > /ac_relation.c > : > > Which is preferable for reviewers? > We still have a day by the start of the CF#2. > > Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
Вложения
В списке pgsql-hackers по дате отправления: