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

Re: New patch for Column-level privileges

From: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, sfrost(at)snowman(dot)net
Cc: Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)ec>, Markus Wanner <markus(at)bluegap(dot)ch>, Alex Hunsaker <badalex(at)gmail(dot)com>, PostgreSQL-development Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: New patch for Column-level privileges
Date: 2009-01-13 08:54:09
Message-ID: 496C56B1.8020708@ak.jp.nec.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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>

Attachment: colprivs_fix_kaigai.20090113.patch
Description: text/x-patch (9.1 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Fujii MasaoDate: 2009-01-13 09:03:39
Subject: Re: Multiplexing SUGUSR1
Previous:From: Fujii MasaoDate: 2009-01-13 08:44:55
Subject: Re: Synch Rep v5

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