Re: ExecutorCheckPerms() hook

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> > #2: REALLY BIG ISSUE- You've added ExecutorCheckPerms_hook as part of
> > this patch- don't, we're in feature-freeze right now and should not be
> > adding hooks at this time.
>
> The patch is intended to submit for the v9.1 development, not v9.0, isn't it?

That really depends on if this is actually fixing a bug in the existing
code or not. I'm on the fence about that at the moment, to be honest.
I was trying to find if we expliitly say that SELECT rights are needed
to reference a column but wasn't able to. If every code path is
expecting that, then perhaps we should just document it that way and
move on. In that case, all these changes would be for 9.1. If we
decide the current behavior is a bug, it might be something which could
be fixed in 9.0 and maybe back-patched.

In *either* case, given that one is a 'clean-up' patch and the other is
'new functionality', they should be independent *anyway*. Small
incremental changes that don't break things when applied is what we're
shooting for here.

> > #3: You didn't move ExecCheckRTPerms() and ExecCheckRTEPerms() to
> > utils/acl and instead added executor/executor.h to rt_triggers.c.
> > I don't particularly like that. I admit that DoCopy() already knew
> > about the executor, and if that were the only case outside of the
> > executor where ExecCheckRTPerms() was getting called it'd probably be
> > alright, but we already have another place that wants to use it, so
> > let's move it to a more appropriate place.
>
> Sorry, I'm a bit confused.
> It seemed to me you suggested to utilize ExecCheckRTPerms() rather than
> moving its logic anywhere, so I kept it here. (Was it misunderstand?)

I'm talking about moving the whole function (all 3 lines of it) to
somewhere else and then reworking the function to be more appropriate
based on it's new location (including renaming and changing arguments
and return values, as appropriate).

> If so, but, I doubt utils/acl is the best placeholder of the moved
> ExecCheckRTPerms(), because the checker function calls both of the
> default acl functions and a optional external security function.

Can you explain why you think that having a function in utils/acl (eg:
include/utils/acl.h and backend/utils/aclchk.c) which calls default acl
functions and an allows for an external hook would be a bad idea?

> It means the ExecCheckRTPerms() is caller of acl functions, not acl
> function itself, isn't it?

It's providing a higher-level service, sure, but there's nothing
particularly interesting or special about what it's doing in this case,
and, we need it in multiple places. Why duplicate it?

> I agreed the checker function is not a part of executor, but it is
> also not a part of acl functions in my opinion.
>
> If it is disinclined to create a new directory to deploy the checker
> function, my preference is src/backend/utils/adt/security.c and
> src/include/utils/security.h .

We don't need a new directory or file for one function, as Robert
already pointed out.

> > #6: I havn't checked yet, but if there are other things in an RTE which
> > would make sense in the DoCopy case, beyond just what's needed for the
> > permissions checking, and which wouldn't be 'correct' with a NULL'd
> > value, I would set those. Yes, we're building the RTE to check
> > permissions, but we don't want someone downstream to be suprised when
> > they make a change to something in the permissions checking and discover
> > that a value in RTE they expected to be there wasn't valid. Even more
> > so, if there are function helpers which can be used to build an RTE, we
> > should be using them. The same goes for RI_Initial_Check().
>
> Are you saying something like makeFuncExpr()?
> I basically agree. However, should it be done in this patch?

Actually, I mean looking for, and using, things like
markRTEForSelectPriv() and addRangeTableEntry() or
addRangeTableEntryForRelation().

> > #8: When moving ExecCheckRTPerms(), you should rename it to be more like
> > the other function calls in acl.h Perhaps pg_rangetbl_aclcheck()?
> > Also, it should return an actual AclResult instead of just true/false.
>
> See the comments in #3.
> And, if the caller has to handle aclcheck_error(), user cannot distinguish
> access violation errors between the default PG permission and any other
> external security stuff, isn't it?

I'm not suggesting that the caller handle aclcheck_error()..
ExecCheckRTPerms() could just as easily have a flag which indicates if
it will call aclcheck_error() or not, and if not, to return an
AclResult to the caller. That flag could then be passed to
ExecCheckRTEPerms() as you have it now.

Thanks,

Stephen