Re: ExecutorCheckPerms() hook

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> Stephen, thanks for comments.
>
> The attached three patches are the revised and divided ones.
>
> A: add makeRangeTblEntry()

Ok, didn't actually expect that. Guess my suggestion would have been to
just use makeNode() since there wasn't anything more appropriate already.
Still, I don't really have a problem with your makeRangeTblEntry() and
you certainly found quite a few places to use it.

> B: major reworks of DML permission checks

No serious issues with this that I saw either. I definitely think it's
cleaner using makeRangeTblEntry() and check_relation_privileges().

> C: add an ESP hook on the DML permission checks

This also looks good to me, though I don't know that you really need the
additional comment in execMain.c about the hook. I would make sure that
you have a comment around check_rte_privileges() which says not to call
it directly because you'll bypass the hook (and potentially cause a
security leak by doing so). Don't recall seeing that, apologies if it
was there.

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

Yeah, that's alright. I'm on the fence about using 'relation' or using
'rangetbl' there, but certainly whomever commits this could trivially
change it to whatever they prefer.

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

Maybe 'ereport_on_violation'? I dunno, guess one isn't really better
than the other. You need to go back and fix the comment though- you
still say 'abort' there.

> > - 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.

Ok.

> > - 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.

Fair enough.

> BTW, I wonder whether acl.h is a correct place to explain about the hook,
> although I added comments for the hook.

Guess I don't really see a problem putting the comments there. By the
way, have we got a place where we actually document the hooks we support
somewhere in the official documentation..? If so, that should certainly
be updated too..

> 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.

I believe the goal will be to avoid reaching a dozen hooks for this.

All-in-all, I'm pretty happy with these. Couple minor places which
could use some copy editing, but that's about it.

Next, we need to get the security label catalog and the grammar to
support it implemented and then from that an SELinux module should
be pretty easy to implement. Based on the discussions at PGCon, Robert
is working on the security label catalog and grammar. The current plan
is to have a catalog similar to pg_depend, to minimize impact to the
rest of the backend and to those who aren't interested in using security
labels. Of course, there will also need to be hooks there for an
external module to enforce restrictions associated with changing labels
on various objects in the system.

Thanks,

Stephen