Re: leaky views, yet again

(2010/10/05 18:01), Itagaki Takahiro wrote:
> 2010/9/6 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>> The attached patch tackles both of the above two points, and allows to
>> control this deoptimization when CREATE SECURITY VIEW is used.
> 
> I'll send a review for the patch.
> 
Thanks for your reviewing.

> Contents&  Purpose
> ==================
> The patch adds a new SECURITY option for VIEWs. Views defined with
> CREATE SECURITY
> VIEW command are not merged with external WHERE-clauses including
> security-leakable
> functions in queries. However, since internal functions and operators are not
> considered as leakable functions, the planner still works well for
> security views
> unless we use user-defined functions in the WHERE-clause.
> 
> Initial Run
> ===========
> * Because of the delayed review, the patch has one hunk:
>     1 out of 5 hunks FAILED -- saving rejects to file
> src/backend/commands/tablecmds.c.rej
>    but it is not serious at all, and can be easily fixed.
> 
The patch was submitted a month ago. I'll fix it.

> * It can be compiled, but has two warnings:
> rewriteHandler.c: In function 'MarkFuncExprDepthWalker':
> rewriteHandler.c:1155: warning: cast from pointer to integer of different size
> rewriteHandler.c:1227: warning: cast to pointer from integer of different size
> 
> They should be harmless, but casting intptr_t is often used to avoid warnings:
>    - L1155: (int) (intptr_t) context;
>    - L1227: (void *) (intptr_t) (depth + 1)
> 
Thanks for the good idea.

> * It passes all regression tests, but doesn't have own new tests.
> 
OK, I'll add it. Perhaps, EXPLAIN enables to show us differences between
security views and others.

> Remaining works
> ===============
> The patch might include only core code, but I'll let you know additional
> sub-works are still required to complete the feature. Also, I've not measured
> the performance yet, though performance might not be so critical for the patch.
> 
> * Regression tests and documentation for the feature are required.
> * pg_dump must support to dump SECURITY VIEWs.
>    They are dumped as normal views for now.
> * pg_views can have "security" column.
> * psql's \dv can show security attributes of views.
> 
All are fair enough for me.

> Questions
> =========
>> postgres=# EXPLAIN select * from v1 where f_malicious(b) and a = 2;
>>   ==>  We assume operators implemented with built-in functions are safe.
>>       So, we don't prevent this pushing-down, if the clause does not
>>       contains something leakable.
> 
> The term "built-in functions" means functions written in INTERNAL language
> here. But we also have SQL functions by default. Some of them are just a
> wrapper to internal functions. I'm not sure the checking of INTERNAL language
> is the best way for the purpose. Did you compare it with other methods?
> For example, "oid<  FirstNormalObjectId" looks workable for me.
> 
Hmm. I'm not sure they can be used for index-scans. If these operators are not
corresponding to index-scans, I want to keep the logic to check INTERNAL language,
because these have obviously no side effects (= not leakable anything).

kaigai=# select oprname, oprleft::regtype, oprright::regtype, prosrc            from pg_operator o join pg_proc p on
o.oprcode= p.oid where prolang <> 12;oprname |        oprleft         |          oprright           |
prosrc
---------+------------------------+-----------------------------+------------------------------------@>      | path
             | point                       | select pg_catalog.on_ppath($2, $1)+       | time without time zone | date
                     | select ($2 + $1)+       | time with time zone    | date                        | select ($2 +
$1)+      | bigint                 | inet                        | select $2 + $1+       | interval               |
timewithout time zone      | select $2 + $1+       | interval               | date                        | select $2 +
$1+      | interval               | time with time zone         | select $2 + $1+       | interval               |
timestampwithout time zone | select $2 + $1+       | interval               | timestamp with time zone    | select $2 +
$1+      | integer                | date                        | select $2 + $1||      | text                   |
anynonarray                | select $1 || $2::pg_catalog.text||      | anynonarray            | text
   | select $1::pg_catalog.text || $2~       | path                   | point                       | select
pg_catalog.on_ppath($2,$1)
 
(13 rows)

>> Instead of modifying the structure of pg_class, this patch stores a flag of
>> security view on the reloption. So, it needed to modify routines about
>> reloptions because it is the first case to store reloptions of views.
> 
> Why did you need to extend StdRdOptions strucuture? Since other fields in
> the structure are not used by views at all, I think adding a new structure
>    struct ViewOptions { vl_len, security_view }
> would be better than extending StdRdOptions.
> 
Hmm. It might be better than ad-hoc enhancement of StdRdOptions.
BTW, which is more preference to store the flag of security view:
reloption of the view or a new bool variable in the pg_class?

I tried to store this flag within reloptions to minimize the patch
size, but it seems to me reloption support for views makes the patch
size larger in the result.

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