Column-Level Privileges

All,

Attached is an updated patch for column-level privileges. This is a
very minor change to use get_rel_name(), so the main point of this is
to update the general community on the status of the patch. It was
pointed out to me that some folks havn't been able to follow along and
so aren't sure of the status.

Overall, I feel this patch is definitely ready for another review.
Markus, Alvaro, as the official CommitFest Reviewers, I'd really like
your feedback on this version. Comments are welcome from others too,
of course.

Changes since the November patch:

- column-level privileges are now respected during JOINs, including
NATURAL JOINs and JOINs with USING clauses, per the SQL
specification, many thanks to KaiGai and Tom for that!
- Regression tests have been added and are reasonably extensive, but
more testing is always good, please test and comment if you're
interested in this capability!
- Documentation has been added
- psql and pg_dump, support has been added
- The code has been cleaned up, bits of code refactored into seperate
functions (pg_attribute_aclcheck_all, ExecGrant_Attribute), follows
coding practices better
- execMain is now cleaner in how it handles permissions that aren't
applicable to columns
- Dependency handling has been fixed
- Interfaces have been made cleaner between acl.c and aclchk.c,
aclchk.c no longer knows the deep innards of an Acl
- Comments added about impact of attacl being added to pg_attribute
- Priv node renamed to AccessPriv
- RTE variables cols_sel and cols_mod changed to Bitmapsets
- outfuncs support added for AccessPriv
- readfuncs now support Bitmapsets
- error-handling for GRANT CREATE (col1) improved
- system columns now handled when explicitly requested
- Other minor changes/bug fixes

I'm not aware of any outstanding issues at this point. The changes
recently have been pretty minor, and I really feel like things have
settled down a great deal with this patch.

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> Attached is an updated patch for column-level privileges.

I'm working on getting this committed. I've run into a major stumbling
block in the parse-time marking of columns for SELECT privileges: the
do-it-as-the-Vars-get-transformed approach basically doesn't work as-is.
The problem comes from whole-row Vars referencing JOIN relations.
We already talked about the need to mark column 0 as referenced for a
whole-row Var, but if the Var is referencing a join then the correct
thing is to recursively mark column 0 of the two input relations.
(The existing patch simply crashes in this case ...)

The problem is that there is no reliable way to identify the two input
relations given only the join RTE. The normal thing we do is to dig
in the query jointree for the JoinExpr, but during parse analysis the
jointree is still being built and we don't have access to it :-(.
The failure case would be where an upper JOIN/ON clause contains a
whole-row reference to a contained JOIN relation.

I considered a couple of alternatives:

* Modify the recursion in transformFromClauseItem so that we can somehow
get at the partially-built jointree for the current join item. Ugly
and probably fragile.

* Depend on the join's joinaliasvars list to contain references to both
sides of the join. This fails in various corner cases, for instance LEFT
NATURAL JOIN where every column of the righthand rel is a common column.

* Add the left and right input RT indexes to join RTEs, so that we can
get hold of them without needing to search the jointree. Kind of
annoying to do this just for the one usage, mainly because there's a lot
of infrastructure needed to update such entries during rewriter/planner
manipulations. (Which would all be 100% useless anyway if the fields
are only examined by the parser, but I dislike the idea of having RTE
fields that aren't valid after parse time ...)

On the whole I think we have to go back to the original plan of
recursively searching the query's expressions after we've finished all
the transformations (and have a completed jointree to refer to). This
is slightly annoying on the grounds of adding parsing overhead that's
completely useless unless per-column privileges are in use. On the
other hand, none of the workable alternatives are exactly overhead-free
either.

Comments?

regards, tom lane

... btw, what is the reasoning behind the special cases for SELECT FOR
UPDATE in execMain.c?

/* Check if this is SELECT-FOR-UPDATE and handle
* accordingly. */
if(remainingPerms & ACL_UPDATE &&
pg_attribute_aclcheck_all(relOid, userid,
ACL_UPDATE, ACLMASK_ALL) != ACLCHECK_OK)
aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS,
get_rel_name(relOid));

If there actually is a need to treat SELECT FOR UPDATE specially, then
this code is quite wrong because it will also fire on a plain UPDATE
(assuming the UPDATE reads any existing column values, which it usually
would). Offhand though I don't see why we can't just use code that is
symmetric with the SELECT case: if requiredPerms includes UPDATE but
there are no columns called out for UPDATE, then allow it if we have
UPDATE on any column.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> On the whole I think we have to go back to the original plan of
> recursively searching the query's expressions after we've finished all
> the transformations (and have a completed jointree to refer to). This
> is slightly annoying on the grounds of adding parsing overhead that's
> completely useless unless per-column privileges are in use. On the
> other hand, none of the workable alternatives are exactly overhead-free
> either.
>
> Comments?

Honestly, I like this approach. There is some additional overhead
during parsing, but it seems cleaner and more robust. Also, hopefully
in most cases where people are concerned about parse time they're
preparing their queries. If it's warrented, we could try doing
benchmarks to see how bad the impact is and if we need to do something
different. It doesn't strike me as likely to be a significant amount of
overhead though.

Thanks,

Stephen

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> ... btw, what is the reasoning behind the special cases for SELECT FOR
> UPDATE in execMain.c?

Basically, because the original logic allowed SELECT-FOR-UPDATE if you
only had SELECT rights, which wasn't right.

> If there actually is a need to treat SELECT FOR UPDATE specially, then
> this code is quite wrong because it will also fire on a plain UPDATE
> (assuming the UPDATE reads any existing column values, which it usually
> would). Offhand though I don't see why we can't just use code that is
> symmetric with the SELECT case: if requiredPerms includes UPDATE but
> there are no columns called out for UPDATE, then allow it if we have
> UPDATE on any column.

I agree, this makes alot more sense to me.

Thanks,

Stephen

Stephen Frost wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> On the whole I think we have to go back to the original plan of
>> recursively searching the query's expressions after we've finished all
>> the transformations (and have a completed jointree to refer to). This
>> is slightly annoying on the grounds of adding parsing overhead that's
>> completely useless unless per-column privileges are in use. On the
>> other hand, none of the workable alternatives are exactly overhead-free
>> either.
>>
>> Comments?
>
> Honestly, I like this approach. There is some additional overhead
> during parsing, but it seems cleaner and more robust. Also, hopefully
> in most cases where people are concerned about parse time they're
> preparing their queries. If it's warrented, we could try doing
> benchmarks to see how bad the impact is and if we need to do something
> different. It doesn't strike me as likely to be a significant amount of
> overhead though.

I agree with Stephen's opinion.
Indeed, the walker approach requires additional steps during query
parsing, but the code obviousness is a significant factor from the
point of view of security.

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

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> Stephen Frost wrote:
>> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>>> On the whole I think we have to go back to the original plan of
>>> recursively searching the query's expressions after we've finished all
>>> the transformations (and have a completed jointree to refer to).
>>
>> Honestly, I like this approach.

> I agree with Stephen's opinion.
> Indeed, the walker approach requires additional steps during query
> parsing, but the code obviousness is a significant factor from the
> point of view of security.

On looking closer, though, it's *still* messy and unobvious :-(.
There is no single place in the parser where we have the complete
multi-level query tree available in a convenient form for this sort of
postprocessing.

I've thought of a less painful variant of my third option: instead of
making a permanent addition to RangeTblEntry, we can have a transient
data structure attached to ParseState that lets us find the JoinExpr
nodes for already-parsed joins. I'm going to try that next.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> On looking closer, though, it's *still* messy and unobvious :-(.
> There is no single place in the parser where we have the complete
> multi-level query tree available in a convenient form for this sort of
> postprocessing.

That's unfortunate. :/

> I've thought of a less painful variant of my third option: instead of
> making a permanent addition to RangeTblEntry, we can have a transient
> data structure attached to ParseState that lets us find the JoinExpr
> nodes for already-parsed joins. I'm going to try that next.

Sounds reasonable. I'd be happy to help if there's anything useful I
can do at this point.

Thanks,

Stephen

Stephen Frost wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> On looking closer, though, it's *still* messy and unobvious :-(.
>> There is no single place in the parser where we have the complete
>> multi-level query tree available in a convenient form for this sort of
>> postprocessing.
>
> That's unfortunate. :/
>
>> I've thought of a less painful variant of my third option: instead of
>> making a permanent addition to RangeTblEntry, we can have a transient
>> data structure attached to ParseState that lets us find the JoinExpr
>> nodes for already-parsed joins. I'm going to try that next.
>
> Sounds reasonable. I'd be happy to help if there's anything useful I
> can do at this point.

I also think it can be a reasonable approach.

However, as an aside, it will not be a help for SE-PostgreSQL, because
it checks Query tree *after* it passed through the rewriter stage, so
ParseState is already released. :-(

http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/src/backend/security/sepgsql/proxy.c#395

QueryRewrite()
-> pgacePostQueryRewrite()
-> sepgsqlPostQueryRewrite()
-> walkQueryHelper()
-> walkVarHelper()
-> wholeRefJoinWalker()

Yes, it is an optional facility and we assume performance is not first
priority for SE-PostgreSQL users. However, if its duration of life has
been expanded to the tail of rewriter, I would be also happy.

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

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> Attached is an updated patch for column-level privileges.

Applied with revisions. The main externally visible change is that I
implemented per-column REFERENCES privilege, since that's required by
spec. I did some heavy revision of the parsing support too, as per
previous dicussions, and editorial cleanup and bugfixing elsewhere.

There are still some significant loose ends though:

* Some of the information_schema views are specified to respond to
per-column privileges; the column_privileges and columns views
certainly need work now to meet spec, and there might be others.

* It might be appropriate to let the pg_stats view expose stats for
columns you have select privilege for, even if you haven't got it
across the whole table.

* We probably ought to invent has_column_privilege SQL functions
analogous to has_table_privilege; this is not just for completeness,
but is probably necessary to finish the above items.

* ISTM that COPY with a column list should succeed if you have
SELECT or INSERT privilege on just the mentioned columns.

* Perhaps it would be appropriate to let LOCK TABLE succeed if you have
proper permissions on at least one column of the table. However, it's
bad enough that LOCK TABLE examines permissions before locking the table
now; I don't think it ought to be grovelling through the columns without
lock. So this might be a place to leave well enough alone.

regards, tom lane

On Thu, Jan 22, 2009 at 3:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> * We probably ought to invent has_column_privilege SQL functions
> analogous to has_table_privilege; this is not just for completeness,
> but is probably necessary to finish the above items.
>

+1

> * ISTM that COPY with a column list should succeed if you have
> SELECT or INSERT privilege on just the mentioned columns.
>

+1

> * Perhaps it would be appropriate to let LOCK TABLE succeed if you have
> proper permissions on at least one column of the table. However, it's
> bad enough that LOCK TABLE examines permissions before locking the table
> now; I don't think it ought to be grovelling through the columns without
> lock. So this might be a place to leave well enough alone.
>

+1

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Applied with revisions. The main externally visible change is that I
> implemented per-column REFERENCES privilege, since that's required by
> spec. I did some heavy revision of the parsing support too, as per
> previous dicussions, and editorial cleanup and bugfixing elsewhere.

Great! Glad to hear it, and thanks for the updates and handling
REFERENCES.

> There are still some significant loose ends though:
[...]

I'll work on these and plan to finish them by Monday.

> * Perhaps it would be appropriate to let LOCK TABLE succeed if you have
> proper permissions on at least one column of the table. However, it's
> bad enough that LOCK TABLE examines permissions before locking the table
> now; I don't think it ought to be grovelling through the columns without
> lock. So this might be a place to leave well enough alone.

Agreed.

Thanks,

Stephen

BTW, something else I'd meant to bring up for discussion is whether
anyone likes the formatting of column privileges in \dp:

regression=# create table foo(bar int, baz int);
CREATE TABLE
regression=# grant select on foo to joe;
GRANT
regression=# grant insert(bar), update(baz) on foo to joe;
GRANT
regression=# \dp foo
Access privileges
Schema | Name | Type | Access privileges | Column access privileges
--------+------+-------+---------------------------+--------------------------
public | foo | table | postgres=arwdDxt/postgres | bar:
: joe=r/postgres : joe=a/postgres
: baz:
: joe=w/postgres
(1 row)

(The colons after the column names are something I added on my own
authority to Stephen's original.)

This seems a bit ASCII-art-ish to me; it certainly wouldn't be readily
parsable by programs. Now that's not really the design goal for \d
output, and I don't have a better suggestion offhand, but still...
anyone got a better idea?

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> BTW, something else I'd meant to bring up for discussion is whether
> anyone likes the formatting of column privileges in \dp:

Well, I kinda like it, but that's not an entirely unbiased opinion. ;)

> Access privileges
> Schema | Name | Type | Access privileges | Column access privileges
> --------+------+-------+---------------------------+--------------------------
> public | foo | table | postgres=arwdDxt/postgres | bar:
> : joe=r/postgres : joe=a/postgres
> : baz:
> : joe=w/postgres
> (1 row)
>
> (The colons after the column names are something I added on my own
> authority to Stephen's original.)

sure, makes sense.

> This seems a bit ASCII-art-ish to me; it certainly wouldn't be readily
> parsable by programs. Now that's not really the design goal for \d
> output, and I don't have a better suggestion offhand, but still...
> anyone got a better idea?

One thing that just occured to me is that we could, should we want to,
move the column-level privs over into the 'Access privileges' column by
just adding them on after the table-level privs. We would want to make
sure the table-level privs come first and maybe have some seperator to
indicate that the following are column-level privs.

That might make the display nicer on 80-col systems, though personally
I like larger windows. :)

A couple of things I didn't particularly like:
I don't like having to have a separate command to show column-level
privs, and I don't really like displaying the column-level privs after
the regular \dp output for tables.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> One thing that just occured to me is that we could, should we want to,
> move the column-level privs over into the 'Access privileges' column by
> just adding them on after the table-level privs. We would want to make
> sure the table-level privs come first and maybe have some seperator to
> indicate that the following are column-level privs.

> That might make the display nicer on 80-col systems, though personally
> I like larger windows. :)

Well, the examples I've looked at fit in 80 columns, but it's true that
all the identifiers involved were pretty short. The alternative I think
you're suggesting is

Access privileges
Schema | Name | Type | Access privileges
--------+------+-------+---------------------------
public | foo | table | postgres=arwdDxt/postgres
: joe=r/postgres
: bar:
: joe=a/postgres
: baz:
: joe=w/postgres
(1 row)

which is definitely more compact horizontally, but I think it's harder
to follow. It's also *less* compact vertically, which is not a
negligible consideration either.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Well, the examples I've looked at fit in 80 columns, but it's true that
> all the identifiers involved were pretty short. The alternative I think
> you're suggesting is

Yeah, I see that now. I guess you'd need a column identifier wider than
'Column Access Privileges' or so, which is almost asking for trouble
already, or a combination of grantee+privs+grantor greater than around
the same, which would require rolenames of >9 characters for grantee
and grantor, which is probably not that common. The new stuff added to
split the ACL across lines is pretty nice.

> which is definitely more compact horizontally, but I think it's harder
> to follow. It's also *less* compact vertically, which is not a
> negligible consideration either.

yea, I'd rather we provide more information on a given row than add
additional rows, but I also tend to run my DB-work terminals at
220x70 or so, which seems to indicate I'm in the minority. :)

Thanks,

Stephen

On Thu, Jan 22, 2009 at 07:03:45PM -0500, Tom Lane wrote:
> BTW, something else I'd meant to bring up for discussion is whether
> anyone likes the formatting of column privileges in \dp:
>
> regression=# create table foo(bar int, baz int);
> CREATE TABLE
> regression=# grant select on foo to joe;
> GRANT
> regression=# grant insert(bar), update(baz) on foo to joe;
> GRANT
> regression=# \dp foo
> Access privileges
> Schema | Name | Type | Access privileges | Column access privileges
> --------+------+-------+---------------------------+--------------------------
> public | foo | table | postgres=arwdDxt/postgres | bar:
> : joe=r/postgres : joe=a/postgres
> : baz:
> : joe=w/postgres
> (1 row)
>
> (The colons after the column names are something I added on my own
> authority to Stephen's original.)
>
> This seems a bit ASCII-art-ish to me; it certainly wouldn't be
> readily parsable by programs. Now that's not really the design goal
> for \d output, and I don't have a better suggestion offhand, but
> still... anyone got a better idea?

Apart from enclosing braces and commas in between, it looks like JSON.
Maybe adding those in would help :)

regression=# \dp foo
Access privileges
Schema | Name | Type | Access privileges | Column access privileges
--------+------+-------+---------------------------+--------------------------
public | foo | table | postgres=arwdDxt/postgres | {
: joe=r/postgres : bar:
: joe=a/postgres,
: baz:
: joe=w/postgres
: }
(1 row)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Tom, et al,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> There are still some significant loose ends though:

Apologies for not having this finished already, been kind of caught up
in some discussions. :)

> * Some of the information_schema views are specified to respond to
> per-column privileges; the column_privileges and columns views
> certainly need work now to meet spec, and there might be others.

Done.

> * It might be appropriate to let the pg_stats view expose stats for
> columns you have select privilege for, even if you haven't got it
> across the whole table.

Done.

> * We probably ought to invent has_column_privilege SQL functions
> analogous to has_table_privilege; this is not just for completeness,
> but is probably necessary to finish the above items.

Done.

> * ISTM that COPY with a column list should succeed if you have
> SELECT or INSERT privilege on just the mentioned columns.

Currently working on this one, doesn't look too bad, but I'm not going
to get it finished tonight. Once I've got this done, hopefully
tomorrow, I'll send in a patch against HEAD for all of the above.

Thanks,

Stephen

Tom, all,

In the attached patch-

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> * Some of the information_schema views are specified to respond to
> per-column privileges; the column_privileges and columns views
> certainly need work now to meet spec, and there might be others.

Done.

> * It might be appropriate to let the pg_stats view expose stats for
> columns you have select privilege for, even if you haven't got it
> across the whole table.

Done.

> * We probably ought to invent has_column_privilege SQL functions
> analogous to has_table_privilege; this is not just for completeness,
> but is probably necessary to finish the above items.

Done.

> * ISTM that COPY with a column list should succeed if you have
> SELECT or INSERT privilege on just the mentioned columns.

Done.

> * Perhaps it would be appropriate to let LOCK TABLE succeed if you have
> proper permissions on at least one column of the table. However, it's
> bad enough that LOCK TABLE examines permissions before locking the table
> now; I don't think it ought to be grovelling through the columns without
> lock. So this might be a place to leave well enough alone.

Left alone.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> * Some of the information_schema views are specified to respond to
>> per-column privileges; the column_privileges and columns views
>> certainly need work now to meet spec, and there might be others.

> Done.

I looked through the spec a bit. If I'm reading it right, these
views should show columns that you have either table-level or
column-level privilege for:
column_privileges
columns
key_column_usage
role_column_grants

What's more, these views should show you tables/views that you have
column privilege on any column of, even if you haven't got any
full-table privileges:
tables
table_constraints
table_privileges
views

I thought about handling the tests for the latter by exposing
pg_attribute_aclcheck_all() as a function named something like
has_any_column_privilege(). However, that would amount to forcing a
nestloop-with-inner-indexscan join to pg_attribute for any table you
didn't have full-table privileges for; also it would bloat the syscache
in a database with lots of tables. It might be better to expose that
join explicitly and let the planner try to decide what to do. OTOH
I don't think the planner would be very smart about avoiding the join
if you do have full-table privileges. Either way you slice it it could
be really slow :-(

Comments, better ideas? Does anyone think I misread the spec?

regards, tom lane

On Tue, Feb 3, 2009 at 7:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>>> * Some of the information_schema views are specified to respond to
>>> per-column privileges; the column_privileges and columns views
>>> certainly need work now to meet spec, and there might be others.
>
>> Done.
>
> I looked through the spec a bit. If I'm reading it right, these
> views should show columns that you have either table-level or
> column-level privilege for:
> column_privileges
> columns
> key_column_usage
> role_column_grants
>
> What's more, these views should show you tables/views that you have
> column privilege on any column of, even if you haven't got any
> full-table privileges:
> tables
> table_constraints
> table_privileges
> views
>
> I thought about handling the tests for the latter by exposing
> pg_attribute_aclcheck_all() as a function named something like
> has_any_column_privilege(). However, that would amount to forcing a
> nestloop-with-inner-indexscan join to pg_attribute for any table you
> didn't have full-table privileges for; also it would bloat the syscache
> in a database with lots of tables. It might be better to expose that
> join explicitly and let the planner try to decide what to do. OTOH
> I don't think the planner would be very smart about avoiding the join
> if you do have full-table privileges. Either way you slice it it could
> be really slow :-(
>
> Comments, better ideas? Does anyone think I misread the spec?

Perhaps there should be a column in pg_class that indicates, in
essence, whether there is any point in checking pg_attribute. Call it
relaclpartial aclitem[]. Whenever user U has column-level privileges
P and Q on a subset of the columns in the table, put {U=PQ} in
relaclpartial.

...Robert

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> [ column privs cleanup patch ]

Applied with revisions, as per previous messages.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > [ column privs cleanup patch ]
>
> Applied with revisions, as per previous messages.

Great, thanks!

Stephen