Re: New patch for Column-level privileges

The attached patch is proof of the concept.

It can be applied on the latest CVS HEAD with colprivs_2009011001.diff.gz.
- It unpatches unnecessary updates at parser/parse_expr.c, parser/parse_node.c
and parser/parse_relation.c.
- It adds markColumnForSelectPriv() to mark proper rte->cols_sel for
the given Var node. It is invoked from scanRTEForColumn(), expandRelAttrs()
and transformWholeRowRef().
- The markColumnForSelectPriv() uses walker function internally, because
there is no guarantee all the entities within RangeTblEntry->joinaliasvars
are Var type node. However, it is used to walks on a single Var node, not
whole of Query tree, so I think its cost is small enough.

==== Results:
postgres=# CREATE TABLE t1 (a int, b text, c bool);
CREATE TABLE
postgres=# CREATE TABLE t2 (x int, y text, z bool);
CREATE TABLE
postgres=# GRANT SELECT (a,b) ON t1 TO ymj;
GRANT
postgres=# GRANT SELECT (x,y) ON t2 TO ymj;
GRANT
postgres=# \c - ymj
psql (8.4devel)
You are now connected to database "postgres" as user "ymj".
postgres=> SELECT a,b FROM t1;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
a | b
---+---
(0 rows)

postgres=> SELECT * FROM t1;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.c required: 0002 allowed: 0000
ERROR: permission denied for relation t1

postgres=> SELECT a FROM t1 order by c;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.c required: 0002 allowed: 0000
ERROR: permission denied for relation t1

postgres=> SELECT t1 FROM t1;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.c required: 0002 allowed: 0000
ERROR: permission denied for relation t1

postgres=> SELECT 'abcd' FROM t1;
DEBUG: pg_attribute_aclmask: t1.tableoid required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.cmax required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.xmax required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.cmin required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.xmin required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.ctid required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
?column?
----------
(0 rows)

postgres=> SELECT b,y FROM t1 JOIN t2 ON a=x;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.x required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.y required: 0002 allowed: 0002
b | y
---+---
(0 rows)

postgres=> SELECT b, (SELECT y FROM t2 WHERE x=a) FROM t1;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.x required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.y required: 0002 allowed: 0002
b | ?column?
---+----------
(0 rows)

Thanks,

KaiGai Kohei wrote:
> Tom Lane wrote:
>> I'm thinking make_var is not the place to do this. The places that are
>> supposed to be taking care of permissions are the ones that do this:
>>
>> /* Require read access --- see comments in setTargetTable() */
>> rte->requiredPerms |= ACL_SELECT;
>
> Indeed, it seems to me appropriate policy, because CLP feature is
> a part of SQL standard access control model.
>
>
> The rte->requiredPerms is set on the following places:
>
> (1) transformLockingClause() and markQueryForLocking()
> It adds ACL_SELECT_FOR_UPDATE for listed relations.
> In this case, column-level priv feature checks ACL_UPDATE for each
> columns on rte->cols_sel, doesn't it?
> So, I think it is unnecessary to care about anything here.
>
> (2) setTargetTable()
> It is invoked from transformInsertStmt(), transformUpdateStmt() and
> transformDeleteStmt(). I thnk it is a proper place to set rte->cols_mod,
> but the caller does not initialize Query->targetList yet, when it is
> invoked.
> So, I think we need put a function call to set cols_mod on caller side
> based on Query->resultRelation and Query->targetList.
>
> (3) scanRTEForColumn()
> Stephen's original one patched here, but it does not handle RTE_JOIN
> well, so it made a matter on table-joins.
> Please revert a code to mark rte->cols_sel, with proper handling for
> RTE_JOIN cases.
>
> (4) addRangeTableEntry()
> Purpose of the function is to translate RangeVar node to RangeTblEntry
> node listed on FROM clause and so on.
> I think we don't need to add any column references here.
> When the translated relation used for column-references, it is a case
> that rte->cols_sel is empty.
>
> (5) addRangeTableEntryForRelation()
> It is similar to addRangeTableEntry().
> I think we don't need to add something here.
>
> (6) ExpandColumnRefStar() and ExpandAllTables()
> They invoke expandRelAttrs() to handle "SELECT * FROM t;" case.
> I think here is a proper point to mark refered columns.
> Please note that here is no guarantee that given RangeTblEntry has
> RTE_RELATION.
>
> Thus, we need the following reworking in my opinion.
> (a) Implement a function to set rte->cols_sel and rte->cols_mod correctly,
> even if the given rte has RTE_JOIN.
> (b) Put a function to set rte->cols_mod on the caller of setTargetTable().
> (c) Put a function to set rte->cols_sel on scanRTEForColumn(), expandRelAttrs()
> and transformWholeRowRef().
>
>> It's possible that we've missed some --- in particular, right at the
>> moment I am not sure that whole-row Vars are handled properly.
>
> Is transformWholeRowRef() a proper place to handle it?
> If given RangeTblEntry has RTE_JOIN, it has to resolve it and set
> its source's cols_sel.
>
>> And maybe we could refactor a little bit to save some code.
>> But those are basically the same places that ought to be adding
>> bits to the column bitmaps.
>

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>