Re: ExecutorCheckPerms() hook - Mailing list pgsql-hackers

Stephen, thanks for comments.

The attached three patches are the revised and divided ones.

 A: add makeRangeTblEntry()
 B: major reworks of DML permission checks
 C: add an ESP hook on the DML permission checks

(2010/05/27 0:09), Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> The attached patch is a revised one for DML permission checks.
>
> This is certainly alot better.
>
>> ToDo:
>> - makeRangeTblEntry() stuff to allocate a RTE node with given parameter
>>    is not yet.
>
> I'd certainly like to see the above done, or to understand why it can't
> be if that turns out to be the case.

The patch-A tries to implement makeRangeTblEntry() which takes only rtekind
as argument right now.
Other fields are initialized to zero, using makeNode().

> A couple of other comments, all pretty minor things:
>
> - I'd still rather see the hook itself in another patch, but given that
>    we've determined that none of this is going to go into 9.0, it's not
>    as big a deal.

OK, I divided the ESP hook part into the patch-C.

> - The hook definition in aclchk.c should really be at the top of that
>    file.  We've been pretty consistant about putting hooks at the top of
>    files instead of deep down in the file, this should also follow that
>    scheme.

OK, I moved it.

> - Some of the comments at the top of chkpriv_rte_perms probably make
>    sense to move up to where it's called from execMain.c.  Specifically,
>    the comments about the other RTE types (function, join, subquery).
>    I'd probably change the comment in chkpriv_rte_perms to be simpler-
>    "This is only used for checking plain relation permissions, nothing
>    else is checked here", and also have that same comment around
>    chkpriv_relation_perms, both in aclchk.c and in acl.h.

OK, I edited the comment as follows:

|   /*
|    * Do permissions checks. The check_relation_privileges() checks access
|    * permissions for all relations listed in a range table, but does not
|    * check anything for other RTE types (function, join, subquery, ...).
|    * Function RTEs are checked by init_fcache when the function is prepared
|    * for execution. Join, subquery, and special RTEs need no checks.
|    */

> - I'd move chkpriv_relation_perms above chkpriv_rte_perms, it's what we
>    expect people to use, after all.

OK, I reordered it.

> - Don't particularly like the function names.  How about
>    relation_privilege_check?  Or rangetbl_privilege_check?  We don't use
>    'perms' much (uh, at all?) in function names, and even if we did, it'd
>    be redundant and not really help someone understand what the function
>    is doing.

IIRC, Robert suggested that a verb should lead the function name.
So, I renamed it into check_relation_privileges() and check_rte_privileges().

> - I don't really like having 'abort' as the variable name for the 2nd
>    argument.  I'm not finding an obvious convention right now, but maybe
>    something like "error_on_failure" instead?

The 'failure' may make an impression of generic errors not only permission denied.
How about 'error_on_violation'?

> - In DoCopy, some comments about what you're doing there to set up for
>    calling chkpriv_relation_perms would be good (like the comment you
>    removed- /* We don't have table permissions, check per-column
>    permissions */, updated to for something like "build an RTE with the
>    columns referenced marked to check for necessary privileges").
>    Additionally, it might be worth considering if having an RTE built
>    farther up in DoCopy would make sense and would then be usable for
>    other bits in DoCopy.

I edited the comments as follows:

| /*
|  * Check relation permissions.
|  * We built an RTE with the relation and columns to be accessed
|  * to check for necessary privileges in the common way.
|  */

> - In RI_Initial_Check, why not build up an actual list of RTEs and just
>    call chkpriv_relation_perms once?  Also, you should add comments
>    there, again, about what you're doing and why.  If you can use another
>    function to build the actual RTE, this will probably fall out more
>    sensibly too.

Good catch! I fixed the invocation of checker function with list_make2().

And, I edited the comments as follows:

| /*
|  * We built a pair of RTEs of FK/PK relations and columns referenced
|  * in the test query to check necessary permission in the common way.
|  */

> - Have you checked if there are any bad side-effects from calling
>    ri_FetchConstraintInfo before doing the permissions checking?

The ri_FetchConstraintInfo() only references SysCaches to set up given
local variable without any other locks except for ones acquired by syscache.c.

> - The hook in acl.h should be separated out and brought to the top and
>    documented independently as to exactly where the hook is and what it
>    can be used for, along with what the arguments mean, etc.  Similairly,
>    chkpriv_relation_perms should really have a short comment for it about
>    what it's for.  Something more than 'security checker function'.

OK, at the patch-C, I moved the definition of the hook into the first half
of acl.h, but it needs to be declared after the AclResult definition.

BTW, I wonder whether acl.h is a correct place to explain about the hook,
although I added comments for the hook.
I think we should add a series of explanation about ESP hooks in the internal
section of the documentation, when the number of hooks reaches a dozen for
example.

Thanks,
--
KaiGai Kohei <kaigai@ak.jp.nec.com>