Re: [v9.2] sepgsql's DROP Permission checks

2012/1/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Jan 19, 2012 at 3:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 2012/1/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> In sepgsql side, it determines a case to apply permission checks
>>>> according to the contextual information; that is same technique
>>>> when we implemented create permission.
>>>> Thus, it could checks db_xxx:{drop} permission correctly.
>>>
>>> Why do we need the contextual information in this case?  Why
>>> can't/shouldn't the decision be made solely on the basis of what
>>> object is targeted?
>>>
>> Several code-paths to remove a particular objects are not appropriate
>> to apply permission checks. For example...
>>
>> [1] Cleanup of temporary objects
>> on_shmem_eixt() registers RemoveTempRelationsCallback(), then
>> it eventually calls deleteWhatDependsOn() that have an invocation
>> of deleteOneObject().
>> This code path is just an internal cleanup process, not related to
>> permission of the client.
>>
>> [2] Cleanup of transient table at VACUUM FULL/CLUSTER command
>> rebuild_relation() creates a temporary table with make_new_heap(),
>> then it copies the contents of original table according to the order of
>> index, and calls finish_heap_swap() that swaps relation files and
>> removes the temporary table using performDeletion().
>> This code actually create and drop a table, however, it is quite
>> internal design and not related to permission of the client.
>>
>> So, I think sepgsql should only applied to permission checks
>> object deletion invoked by user's operations, such as DropStmt.
>
> I agree with that theory, but isn't this method of implementing that a
> pretty horrible kludge?  For example, if you'd implemented it this way
> for 9.1, the recent drop-statement refactoring would have broken it.
> Or if, in the future, we add another type of statement that can drop
> things, this code will still compile just fine but will no longer work
> correctly.  ISTM that we need a way to either (1) not call the hook at
> all unless the operation is user-initiated, or (2) call the hook, but
> pass a flag indicating what sort of operation this is?
>
Yes. This approach requires to continue code revising on sepgsql-side
also for each major release.
I think the approach (1) raise an issue similar to what we discussed
when sepgsql implemented create permission; we have to know
details of extension module to determine whether the hook should
be called, or not. My preference is (2) that is more reasonable.

> Let's imagine another possible use of this hook: we want to emit some
> kind of log message every time a database object gets dropped.  I
> think that's a plausible use-case, and in that case what we'd want is:
> (1) VACUUM FULL or CLUSTER shouldn't call the hook at all, (2) cleanup
> of temporary objects should probably call the hook, but ideally with a
> flag to indicate that it's an internal (DB-initiated) operation, and
> (3) user activity should definitely call the hook.
>
> I'm not sure how we can cleanly get that behavior, but ISTM that's
> what we want...
>
I think the case (1) should also call the hook but with a flag that indicate
database internal stuff, because make_new_heap() calls OAT_POST_CREATE
hook, thus, we need to give extensions a chance to cleanup, if it did
something on this timing.

I'd like to propose to utilize DropBehavior argument of performDeletion()
to inform dependency.c its invoked context.
If we have a new flag DROP_INTERNAL that can be OR-masked with
existing DROP_RESTRICT or DROP_CASCADE, it can be used to
inform the current context, then, it shall be used to the flag to the hook.
It seems to me this approach is minimum invasive.

In addition, we may have a case when extension want to know whether
the deletion is cascaded, or explicitly specified by users. If they want to
implement same security model on this hook.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>