Skip site navigation (1) Skip section navigation (2)

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

From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [v9.2] sepgsql's DROP Permission checks
Date: 2012-01-22 14:54:08
Message-ID: CADyhKSXUeF8NeK5BemgP=+i9Vn0j4K-E9ekUYiKBVzQARm2_UQ@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
I tried to implement based on the idea to call object-access-hook with
flag; that
informs extensions contexts of objects being removed.
Because I missed DROP_RESTRICT and DROP_CASCADE are enum type,
this patch added performInternalDeletion() instead of OR-masked DROP_INTERNAL.
All its difference from performDeletion() is a flag (OAT_DROP_FLAGS_INTERNAL)
shall be delivered to extension module. I replaced several performDeletion() by
performInternalDeletion() that clean-up objects due to internal stuff.

How about this approach?

Thanks,

2012/1/21 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 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>

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

Attachment: pgsql-v9.2-drop-permissions.v3.patch
Description: application/octet-stream (55.2 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Kohei KaiGaiDate: 2012-01-22 15:48:09
Subject: PG-Strom - A GPU optimized asynchronous executor module
Previous:From: Giuseppe SucameliDate: 2012-01-22 13:22:06
Subject: Different error messages executing CREATE TABLE or ALTER TABLE to create a column "xmin"

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group