Re: leaky views, yet again

(2010/10/05 18:01), Itagaki Takahiro wrote:
> 2010/9/6 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)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 | time without time zone | select $2 + $1
+ | interval | date | select $2 + $1
+ | interval | time with time zone | select $2 + $1
+ | interval | timestamp without 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(at)ak(dot)jp(dot)nec(dot)com>