Re: WIP: Column-level Privileges

Greetings,

First, a quick digression into what I've gleaned from the spec in
terms of implementation, from SQL2003:

-- Table-level grants. The spec basically says that a table-level
-- grant is as if the same grant was done on all existing and future
-- columns. (I looked at GRANT and ALTER ADD COLUMN definitions to
-- discover that)
GRANT SELECT, INSERT ON tab1 TO role1;

-- Column-level grants. These apply when accessing the column, and
-- are independent of table-level grants. If you have a
-- column-level grant to a table, you can access that column even if
-- you do not have table-level rights to same table.
GRANT SELECT (col1, col2), INSERT (col1, col2) ON tab1 TO role2;

-- Column-level revokes. Mostly what you would expect, but be aware
-- that table-level grants 'with admin option' apply when revoking
-- column-level grants on that table.
REVOKE SELECT (col1, col2), INSERT (col1, col2) ON tab1 FROM role2;

-- Table-level revokes. A table-level revoke of a given privilege
-- applies to all columns as well. So if you have INSERT on col1
-- and I do a 'REVOKE INSERT ON tab1 FROM you;', your column-level
-- permissions will also be removed.
REVOKE SELECT, INSERT ON tab1 FROM role1;

One of the implications from all of this is that we're going to have
to keep column-level and table-level permissions seperate and
distinct from each other. An upshot from this is that it probably
more closely matches what people expect from how we've handled things
in the past. Also, as long as we do table-level checks first, people
who don't use column-level permissions shouldn't see much of
performance hit. It'll be a bit slower on failure cases because it'll
do per-column checks for ACLs which aren't defined. I'm on the fence
as to if that's a big enough concern to warrant a flag in pg_class.

Attached is the current state of the patch. The grammer/parser
changes have been tested and seem to work reasonably well so far. The
aclchk.c changes are probably broken at the moment. I've been
frustrated with implementing what the spec calls for and what it means
for the code. Specifically, I don't like the number of nested loops
to get the per-privilege column lists into per-column ACL masks, and
dealing with multiple relations at a time. I also don't like the
amount of what seems like mostly duplicated code between the
table-level permissions handling and the column-level handling, but I
might be a little pedantic there. Working the column-level
permissions into other parts of the code has also proven challenging
since you need a (Oid,AttrNumber) combination to identify a column
instead of just an Oid like everything expects.

I've yet to even start in on the changes necessary to get the column
information down to the executor, unfortunately.

Comments welcome, apologies for it not being ready by 9/1. I'm
planning to continue working on it tomorrow, and throughout September
as opportunity allows (ie: when Josh isn't keeping me busy).

Thanks,

Stephen

Greetings,

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> Comments welcome, apologies for it not being ready by 9/1. I'm
> planning to continue working on it tomorrow, and throughout September
> as opportunity allows (ie: when Josh isn't keeping me busy).

Here is an updated patch. I've added column-level privilege information
to the psql output as an additional column for \dp, but it depends on
the array_accum() aggregate which isn't included (yet). This output
matches more what I would expect, but I'm open to comments.

An example of what it looks like is here, for the next few days:
http://pgsql.privatepaste.com/871z3IheMr

I havn't tested everything, but basic column-level grant/revoke should
work now. This includes: grammer, parser, catalog changes. It also
properly does the 'revoke all column-level when called as a table-level
revoke' SQL spec requirement. It does not yet have proper dependency
handling.

Next I'm planning to work on adding some regression tests for
grant/revoke commands and catalog updates and make sure those all work
correctly. Then I'll probably go through the dependency handling, and
last will be working on the changes to implement the permission checks.

Thanks,

Stephen

Hello Stephen,

Stephen Frost wrote:
> Comments welcome, apologies for it not being ready by 9/1. I'm
> planning to continue working on it tomorrow, and throughout September
> as opportunity allows (ie: when Josh isn't keeping me busy).

I'm trying to review this patch. I could at least compile it
successfully for now.

There have already been some comments from Tom [1]. You've mostly
answered that you are going to fix these issues, but I'm pretty unclear
on what the current status is (of patch 09/02). Can you please elaborate
on what's done and what not?

As this is still marked as WIP on the wiki page: what needs to be done
until you consider it done?

Regards

Markus Wanner

[1]: Comments from Tom:
http://archives.postgresql.org/pgsql-patches/2008-05/msg00111.php

Hi Markus,

* Markus Wanner (markus(at)bluegap(dot)ch) wrote:
> Stephen Frost wrote:
>> Comments welcome, apologies for it not being ready by 9/1. I'm
>> planning to continue working on it tomorrow, and throughout September
>> as opportunity allows (ie: when Josh isn't keeping me busy).
>
> I'm trying to review this patch. I could at least compile it
> successfully for now.

That's a start at least. :)

> There have already been some comments from Tom [1]. You've mostly
> answered that you are going to fix these issues, but I'm pretty unclear
> on what the current status is (of patch 09/02). Can you please elaborate
> on what's done and what not?

I would suggest you review the updated patch (linked off the wiki page)
here:
http://archives.postgresql.org/message-id/20080902221823.GL16005@tamriel.snowman.net

It includes my comments about what's done and what's not. I reiterated
much of it here as well:
http://archives.postgresql.org/pgsql-hackers/2008-09/msg00263.php

> As this is still marked as WIP on the wiki page: what needs to be done
> until you consider it done?

As mentioned in above, regression tests, documentation updates,
dependency handling, and actually implementing the permission checks all
remain. What I'm looking for feedback on are the changes to the
grammer, parser, catalog changes, psql output, etc.

Thanks,

Stephen

Hi,

Stephen Frost wrote:
> I would suggest you review the updated patch (linked off the wiki page)
> here:
> http://archives.postgresql.org/message-id/20080902221823.GL16005@tamriel.snowman.net

That's the patch I've been talking about: file
colprivs_wip.20080902.diff.gz, dated Sept, 2nd.

> It includes my comments about what's done and what's not. I reiterated
> much of it here as well:
> http://archives.postgresql.org/pgsql-hackers/2008-09/msg00263.php

Uh.. I've read that message as well, but didn't find it overly clear on
what you were referring to.

> As mentioned in above, regression tests, documentation updates,
> dependency handling, and actually implementing the permission checks all
> remain. What I'm looking for feedback on are the changes to the
> grammer, parser, catalog changes, psql output, etc.

Aha, good. So I'm going to (try to) check these things and comment.

Regards

Markus Wanner

Hi,

Markus Wanner wrote:
>> As mentioned in above, regression tests, documentation updates,
>> dependency handling, and actually implementing the permission checks all
>> remain. What I'm looking for feedback on are the changes to the
>> grammer, parser, catalog changes, psql output, etc.
>
> Aha, good. So I'm going to (try to) check these things and comment.

Sorry, this took way longer than planned.

The grammar and parser changes look fine to me. You've added a 'Priv'
parser node which now stores a privilege string (like 'REFERENCES',
'SELECT' or 'CREATE') as well as a list of affected columns.

I've been wondering about the use of 'ColId' instead of all the other
options (i.e. 'UPDATE', 'DELETE', 'TRUNCATE', ...). But that has
obviously been there before. Checking it is deferred to later giving an
"unrecognized privilege type" error. I'm wondering why this is done that
way. Seems to be related to some unreserved_keywords vs col_name_keyword
vs reserved_keywords issue.

However, the following is certainly bogus and needs to be prevented by
the parser or later privilege type checking code:

testdb=# GRANT TRUNCATE (single_col) ON test TO malory;
GRANT

Otherwise the syntax seems to match what my SQL 2008 draft is telling.
MySQL does it the same way as well.

The catalog changes have been discussed with Tom.

Some privilege regression tests currently fail with your patch, but I
think that's expected.

Documentation and new regression tests for column level privileges are
still missing. If you want, Stephen, I can work on that.

Given the known-unfinished state of this patch I'm moving it to the
November commit fest. Hope that's fine with you. I'm glad to help and
review as updated patch, no matter what the commit fest state is.

Regards

Markus Wanner

Markus,

* Markus Wanner (markus(at)bluegap(dot)ch) wrote:
> Sorry, this took way longer than planned.

Beleive me, I understand. :)

> testdb=# GRANT TRUNCATE (single_col) ON test TO malory;
> GRANT

This has been fixed in the attached patch.

> Some privilege regression tests currently fail with your patch, but I
> think that's expected.

All regression tests should pass now.

> Documentation and new regression tests for column level privileges are
> still missing. If you want, Stephen, I can work on that.

If you could work on the documentation, that'd be great! I've updated
the regression tests to include some testing of the column level
privileges. Feel free to suggest or add to them though, and if you find
anything not working as you'd expect, please let me know!

There are a few outstanding items that I can think of-

The error-reporting could be better (eg, give the specific column that
you don't have rights on, rather than just saying you don't have rights
on the relation), but I wasn't sure if people would be happy with the
change to existing error messages that would imply. Basically, rather
than getting told you don't have rights on the relation, you would be
told you don't have rights on the first column in the relation that you
don't have the necessary rights on. It's a simple change, if people are
agreeable to it. Having it give the table-level message only when there
aren't column-level privileges is possible, but makes the code rather
ugly..

Documentation, of course.

More testing, more review, etc, etc, making sure everything is working
as expected, more complex queries than what I've done to make sure
things happen correctly. Tom has me rather nervous based on his
previous comments about the rewriter/optimizer causing problems, and I
barely touched them.. I also wonder if you could use joins or something
to extract information about columns you're not supposed to have access
to, or where clauses, etc..

Anyhow, updated patch attached.

Thanks,

Stephen

Markus, et al,

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> I also wonder if you could use joins or something
> to extract information about columns you're not supposed to have access
> to, or where clauses, etc..

welp, I've done some additional testing and there's good news and bad, I
suppose. The good news is that when relations are join'd, they go
through expandRelation, which adds all the columns in that relation to
the 'required' set, so you have to have rights to all columns on a table
to join against it in the normal way.

On the other hand, you can just select out the columns you have access
to in a subquery and then join against *that* and it works. updates
with where clauses and inserts-with-selects seem to work correctly
though, which is nice. A case I just realized might be an issue is
doing a 'select 1 from x;' where you have *no* rights on x, or any
columns in it, would still get you the rowcount. That might not be too
hard to fix though, I'll look into it tomorrow sometime.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> ... A case I just realized might be an issue is
> doing a 'select 1 from x;' where you have *no* rights on x, or any
> columns in it, would still get you the rowcount.

Well, if you have table-level select on x, I would expect that to work,
even if your privs on every column of x are revoked. If the patch
doesn't get this right then it needs more work ...

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > ... A case I just realized might be an issue is
> > doing a 'select 1 from x;' where you have *no* rights on x, or any
> > columns in it, would still get you the rowcount.
>
> Well, if you have table-level select on x, I would expect that to work,
> even if your privs on every column of x are revoked. If the patch
> doesn't get this right then it needs more work ...

Table-level select on x is equivilant to having column-level select on
every column, per the spec. The issue here, that I'm planning to fix
shortly, is that you could get a rowcount without having table-level or
column-level select rights on the table.

Thanks,

Stephen

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > ... A case I just realized might be an issue is
> > doing a 'select 1 from x;' where you have *no* rights on x, or any
> > columns in it, would still get you the rowcount.
>
> Well, if you have table-level select on x, I would expect that to work,
> even if your privs on every column of x are revoked. If the patch
> doesn't get this right then it needs more work ...

Attached patch has this fixed and still passes all regression tests,
etc.

Thanks,

Stephen

Hello Stephen,

Stephen Frost wrote:
> This has been fixed in the attached patch.

Cool, thanks.

> If you could work on the documentation, that'd be great!

I'll give it a try.

Regards

Markus Wanner

Hello Stephen,

Stephen Frost wrote:
> Attached patch has this fixed and still passes all regression tests,
> etc.

Do you have an up-to-date patch laying around? The current one conflicts
with some CVS tip changes.

I didn't get around writing some docu, yet. Sorry.

Regards

Markus Wanner

Markus,

* Markus Wanner (markus(at)bluegap(dot)ch) wrote:
> Stephen Frost wrote:
> > Attached patch has this fixed and still passes all regression tests,
> > etc.
>
> Do you have an up-to-date patch laying around? The current one conflicts
> with some CVS tip changes.

No, not yet. I suspect the array_agg patch is conflicting (which is a
good thing, really) and the addition of array_agg by my patch can now be
removed. Testing should be done to ensure nothing changed wrt output,
of course, but I'm not too worried.

I can probably update it this weekend, depending on how things are going
with the newborn (she's only 4 days old at this point, after all.. :).

Thanks,

Stephen

Stephen Frost wrote:
> Markus,
>
> * Markus Wanner (markus(at)bluegap(dot)ch) wrote:
> > Stephen Frost wrote:
> > > Attached patch has this fixed and still passes all regression tests,
> > > etc.
> >
> > Do you have an up-to-date patch laying around? The current one conflicts
> > with some CVS tip changes.
>
> No, not yet. I suspect the array_agg patch is conflicting (which is a
> good thing, really) and the addition of array_agg by my patch can now be
> removed. Testing should be done to ensure nothing changed wrt output,
> of course, but I'm not too worried.

Yes, it has a conflict with the array_agg patch.

I had some time on my hands today so I stole the part of the patch that
dealt with pg_attribute tuples, tinkered with it a bit, and committed
it.

So now your patch conflicts with more stuff :-(

I'll probably fix both things and submit an updated version tomorrow.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:

> I'll probably fix both things and submit an updated version tomorrow.

Here it is. This applies cleanly to current CVS HEAD and passes the
regression tests. Apart from fixing the conflicts, I updated psql's \z
with the new array aggregate, and changed the Schema_pg_* declarations
in pg_attribute.h to contain initializators for attacl (I'm not sure
that { 0 } is the correct initializator, but it seems better than
omitting it completely). Also tacked _null_ at the end of the DATA
lines.

I didn't check the rest of the code, so don't count this as a review.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>
> > I'll probably fix both things and submit an updated version tomorrow.
>
> Here it is.

Really attached this time.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:

> I didn't check the rest of the code, so don't count this as a review.

I had a look at aclchk.c and didn't like your change to
objectNamesToOids; seems rather baroque. I changed it per the attached
patch.

Moreover I didn't very much like the way aclcheck_error_col is dealing
with two or one % escapes. I think you should have a separate routine
for the column case, and prepend a dummy string to no_priv_msg.

Why is there a InternalGrantStmt.rel_level? Doesn't it suffice to
check whether col_privs is NIL?

Is there enough common code in ExecGrant_Relation to justify the way you
have it? Can the common be refactored in a better way that separates
the two cases more clearly?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro,

* Alvaro Herrera (alvherre(at)commandprompt(dot)com) wrote:
> I had a look at aclchk.c and didn't like your change to
> objectNamesToOids; seems rather baroque. I changed it per the attached
> patch.

I've incorporated this change.

> Moreover I didn't very much like the way aclcheck_error_col is dealing
> with two or one % escapes. I think you should have a separate routine
> for the column case, and prepend a dummy string to no_priv_msg.

I can do this, not really a big deal.

> Why is there a InternalGrantStmt.rel_level? Doesn't it suffice to
> check whether col_privs is NIL?

No, a single statement can include both relation-level and column-level
permission changes. The rel_level flag is there to indicate if there
are any relation-level changes. Nothing else indicates that.

> Is there enough common code in ExecGrant_Relation to justify the way you
> have it? Can the common be refactored in a better way that separates
> the two cases more clearly?

I've looked at this a couple of times and I've not been able to see a
good way to do that. I agree that there's alot of common code and it
seems like there should be a way to factor it out, but there are a
number of differences that make it difficult. If you see something I'm
missing, please let me know.

Thanks,

Stephen

On Tue, Nov 25, 2008 at 4:03 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Alvaro Herrera (alvherre(at)commandprompt(dot)com) wrote:
>> I had a look at aclchk.c and didn't like your change to
>> objectNamesToOids; seems rather baroque. I changed it per the attached
>> patch.
>
> I've incorporated this change.
>
>> Moreover I didn't very much like the way aclcheck_error_col is dealing
>> with two or one % escapes. I think you should have a separate routine
>> for the column case, and prepend a dummy string to no_priv_msg.
>
> I can do this, not really a big deal.

Stephen,

Are you sending an updated patch with these minor changes?

...Robert