Обсуждение: 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@snowman.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@bluegap.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@bluegap.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@snowman.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@snowman.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@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.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@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.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@bluegap.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@bluegap.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@commandprompt.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@snowman.net> wrote: > * Alvaro Herrera (alvherre@commandprompt.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