Обсуждение: Grantor name gets lost when grantor role dropped
I am sending this email on behalf of Russel Smith. He discovered this bug and his description follows: Verified on 8.2.3 on Fedora Core 6 Verified on 8.1.3 on RHEL4, custom compile. (I can't control the update to 8.1.8) The output of an empty role name would possibly not be a problem, but when you are doing a dump and restore, pg_dumpall dumps invalid syntax as below; GRANT "postgres" TO "test_role" GRANTED BY ""; We either need to rethink the way we handle grantor information and when it's valid. Or we need to at least allow dump/restore to work as expected when a dropped role granted privileges to other users. To add to my woes when investigating this, GRANTED BY syntax is not included in the 8.2 documentation at all. It's not listed as valid syntax, and there are no comments saying what it does. The self contained test case to produce this is below; Regards Russell Smith psql postgres < bug.sql 2>&1 > output.txt CREATE ROLE test_role NOSUPERUSER INHERIT NOCREATEDB NOCREATEROLE; CREATE ROLE invalid_grantor SUPERUSER INHERIT NOCREATEDB NOCREATEROLE; SET ROLE invalid_grantor; GRANT "postgres" TO "test_role"; SET ROLE postgres; select * from pg_roles; select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid LEFT JOIN pg_roles gr ON gr.oid = grantor; DROP ROLE invalid_grantor; select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid LEFT JOIN pg_roles gr ON gr.oid = grantor; DROP ROLE test_role;
Jeff Davis wrote: > CREATE ROLE test_role > NOSUPERUSER INHERIT NOCREATEDB NOCREATEROLE; > > CREATE ROLE invalid_grantor > SUPERUSER INHERIT NOCREATEDB NOCREATEROLE; > > SET ROLE invalid_grantor; > GRANT "postgres" TO "test_role"; > SET ROLE postgres; > > select * from pg_roles; > > select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid > LEFT JOIN pg_roles gr ON gr.oid = grantor; > > DROP ROLE invalid_grantor; > > select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid > LEFT JOIN pg_roles gr ON gr.oid = grantor; > > DROP ROLE test_role; The problem here is that we allowed the drop of invalid_grantor. We are missing a shared dependency on it. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Jeff Davis wrote: > > >> CREATE ROLE test_role >> NOSUPERUSER INHERIT NOCREATEDB NOCREATEROLE; >> >> CREATE ROLE invalid_grantor >> SUPERUSER INHERIT NOCREATEDB NOCREATEROLE; >> >> SET ROLE invalid_grantor; >> GRANT "postgres" TO "test_role"; >> SET ROLE postgres; >> >> select * from pg_roles; >> >> select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid >> LEFT JOIN pg_roles gr ON gr.oid = grantor; >> >> DROP ROLE invalid_grantor; >> >> select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid >> LEFT JOIN pg_roles gr ON gr.oid = grantor; >> >> DROP ROLE test_role; >> > > The problem here is that we allowed the drop of invalid_grantor. We are > missing a shared dependency on it. > So does this make a todo item? But this still leaves the concerns about you can currently get the database into an invalid state that can't be dumped and restored.
Russell Smith wrote: > Alvaro Herrera wrote: > >Jeff Davis wrote: > > > > > >>CREATE ROLE test_role > >> NOSUPERUSER INHERIT NOCREATEDB NOCREATEROLE; > >> > >>CREATE ROLE invalid_grantor > >> SUPERUSER INHERIT NOCREATEDB NOCREATEROLE; > >> > >>SET ROLE invalid_grantor; > >>GRANT "postgres" TO "test_role"; > >>SET ROLE postgres; > >> > >>select * from pg_roles; > >> > >>select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members > >>LEFT JOIN pg_roles ur ON roleid = oid > >>LEFT JOIN pg_roles gr ON gr.oid = grantor; > >> > >>DROP ROLE invalid_grantor; > >> > >>select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members > >>LEFT JOIN pg_roles ur ON roleid = oid > >>LEFT JOIN pg_roles gr ON gr.oid = grantor; > >> > >>DROP ROLE test_role; > >> > > > >The problem here is that we allowed the drop of invalid_grantor. We are > >missing a shared dependency on it. > > > So does this make a todo item? > > But this still leaves the concerns about you can currently get the > database into an invalid state that can't be dumped and restored. Correct, which makes it a bug (==> needs fixed) rather than a todo item. We now have a problem because there may already be databases that are undumpable. We might need to provide a workaround for people with such a database. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Russell Smith wrote: > >> Alvaro Herrera wrote: >> >>> Jeff Davis wrote: >>> >>> >>> >>>> CREATE ROLE test_role >>>> NOSUPERUSER INHERIT NOCREATEDB NOCREATEROLE; >>>> >>>> CREATE ROLE invalid_grantor >>>> SUPERUSER INHERIT NOCREATEDB NOCREATEROLE; >>>> >>>> SET ROLE invalid_grantor; >>>> GRANT "postgres" TO "test_role"; >>>> SET ROLE postgres; >>>> >>>> select * from pg_roles; >>>> >>>> select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members >>>> LEFT JOIN pg_roles ur ON roleid = oid >>>> LEFT JOIN pg_roles gr ON gr.oid = grantor; >>>> >>>> DROP ROLE invalid_grantor; >>>> >>>> select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members >>>> LEFT JOIN pg_roles ur ON roleid = oid >>>> LEFT JOIN pg_roles gr ON gr.oid = grantor; >>>> >>>> DROP ROLE test_role; >>>> >>>> >>> The problem here is that we allowed the drop of invalid_grantor. We are >>> missing a shared dependency on it. >>> >>> >> So does this make a todo item? >> >> But this still leaves the concerns about you can currently get the >> database into an invalid state that can't be dumped and restored. >> > > Correct, which makes it a bug (==> needs fixed) rather than a todo item. > > We now have a problem because there may already be databases that are > undumpable. We might need to provide a workaround for people with such > a database. > Well, given GRANTED BY is not documented, it should be reasonable to alter pg_dumpall to remove GRANTED BY in cases where the role doesn't resolve. That is not an unsafe change as it should never happen if the depend data is in place. It will also allow any currently undumpable databases to be dumpable again. A simple and totally untested or compiled patch is below; Index: pg_dumpall.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_dumpall.c,v retrieving revision 1.90 diff -c -r1.90 pg_dumpall.c *** pg_dumpall.c 10 Feb 2007 14:58:55 -0000 1.90 --- pg_dumpall.c 18 Apr 2007 07:41:44 -0000 *************** *** 724,730 **** fprintf(OPF, " TO %s", fmtId(member)); if (*option == 't') fprintf(OPF, " WITH ADMIN OPTION"); ! fprintf(OPF, " GRANTED BY %s;\n", fmtId(grantor)); } PQclear(res); --- 724,739 ---- fprintf(OPF, " TO %s", fmtId(member)); if (*option == 't') fprintf(OPF, " WITH ADMIN OPTION"); ! /* ! * It's possible due to a lack of shared dependency tracking ! * of grantor that this parameter and be an empty string. ! * In that case, we don't dump the grantor and the grant ! * will be granted by the user who imports the roles. ! */ ! if (strlen(grantor) != 0) ! fprintf(OPF, " GRANTED BY %s", fmtId(grantor)); ! ! fprintf(OPF, ";\n"); } PQclear(res);
Russell Smith wrote: >>>>> CREATE ROLE test_role >>>>> NOSUPERUSER INHERIT NOCREATEDB NOCREATEROLE; >>>>> >>>>> CREATE ROLE invalid_grantor >>>>> SUPERUSER INHERIT NOCREATEDB NOCREATEROLE; >>>>> >>>>> SET ROLE invalid_grantor; >>>>> GRANT "postgres" TO "test_role"; >>>>> SET ROLE postgres; >>>>> >>>>> select * from pg_roles; >>>>> >>>>> select pg_auth_members.*, ur.rolname, gr.rolname from >>>>> pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid >>>>> LEFT JOIN pg_roles gr ON gr.oid = grantor; >>>>> >>>>> DROP ROLE invalid_grantor; >>>>> >>>>> select pg_auth_members.*, ur.rolname, gr.rolname from >>>>> pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid >>>>> LEFT JOIN pg_roles gr ON gr.oid = grantor; >>>>> >>>>> DROP ROLE test_role; >>>>> >>> So does this make a todo item? >>> >>> But this still leaves the concerns about you can currently get the >>> database into an invalid state that can't be dumped and restored. >>> >> >> Correct, which makes it a bug (==> needs fixed) rather than a todo item. >> >> We now have a problem because there may already be databases that are >> undumpable. We might need to provide a workaround for people with such >> a database. >> [snip] As I am not a frequent reporter of bugs, what happens now? It's been a week since I wrote my last message and I'm unsure of whether anything has, or is going to happen about this bug report. Alvaro has said it's a "bug", so it needs fixing. But that has not translated into somebody saying they will look at it. I've also had no comments on the possible patch I attached in my last email. I understand people are busy, but as a follower of pg development, from this experience I can see who some new people get the feeling of not being looked after. Even if something is happening, there is little flow of information. I would greatly appreciate an update, even though this is not a server crash, it does allow creation of dumps that cannot be restored. They can be alter to be restored, but I didn't think that was the point. Regards Russell Smith
Russell Smith wrote: > As I am not a frequent reporter of bugs, what happens now? It's been a > week since I wrote my last message and I'm unsure of whether anything > has, or is going to happen about this bug report. Alvaro has said it's > a "bug", so it needs fixing. But that has not translated into somebody > saying they will look at it. I've also had no comments on the possible > patch I attached in my last email. Sorry. It's on my to-do list and I'll fix it shortly. I am very sorry that the fix did not make it into 8.2.4. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Russell Smith wrote: > > >> As I am not a frequent reporter of bugs, what happens now? It's been a >> week since I wrote my last message and I'm unsure of whether anything >> has, or is going to happen about this bug report. Alvaro has said it's >> a "bug", so it needs fixing. But that has not translated into somebody >> saying they will look at it. I've also had no comments on the possible >> patch I attached in my last email. >> > > Sorry. It's on my to-do list and I'll fix it shortly. I am very sorry > that the fix did not make it into 8.2.4. > Thanks for the update it's most appreciated. I'm not so worried it didn't make it into 8.2.4, just wanted to make sure it had made it to somebodies todo list. I did post a pg_dumpall fix to make it so you can dump existing invalid databases. Regards Russell
Jeff Davis wrote: > GRANT "postgres" TO "test_role" GRANTED BY ""; > > We either need to rethink the way we handle grantor information and when it's valid. > Or we need to at least allow dump/restore to work as expected when a dropped role > granted privileges to other users. I've been staring at this for a while. Upon first reading it, I thought that it would be simply a matter of adding pg_shdepend entries for the pg_auth_members rows. This starts sounding suspicious the moment you consider that there will be one pg_shdepend entry for each role granted, that is, a lot. The second problem with this idea is that it's not at all possible, because pg_shdepend entries can reference an object by OID, but pg_auth_members rows don't have OIDs. So the most we could do is add entries for pg_authid. So I'm currently considering the following alternatives: 1. do nothing at all with pg_shdepend. Upon role deletion, seqscan pg_auth_members and reject the drop altogether if there is a role granted to another which mentions the to-be-dropped role ID as grantor. This is easiest in terms of code (it's even mentioned in the comments in DropRole). 2. record one pg_shdepend entry for each role that has granted something to each role (unless the grantor is the same role being granted, in which case we needn't record anything). So if role A grants Z and X to C, and role B grants Y and W to C, C now has access to W, Y, X and Z and there are two pg_shdepend entries: C -> A C -> B So dropping a role would be disallowed automatically without any code changes, with the checkSharedDependencies() call that's already in DropRole. Adding a role membership would require a bit more work, because we'd first need to check that there's not already a pg_shdepend entry for that combination. Removing a role membership also becomes more work; we need to check that no other grant depends on the same grantor before removing the entry. Note that I'm considering that this alternative requires adding a GRANTOR symbol to the SharedDependencyType, which probably rules this out for backpatching. Comments? I'm leaning towards implementing (2). The patch for pg_dumpall would also be needed. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes: > So I'm currently considering the following alternatives: > 1. do nothing at all with pg_shdepend. Upon role deletion, seqscan > pg_auth_members and reject the drop altogether if there is a role > granted to another which mentions the to-be-dropped role ID as grantor. > This is easiest in terms of code (it's even mentioned in the comments in > DropRole). > 2. record one pg_shdepend entry for each role that has granted something > to each role (unless the grantor is the same role being granted, in > which case we needn't record anything). So if role A grants Z and X to > C, and role B grants Y and W to C, C now has access to W, Y, X and Z and > there are two pg_shdepend entries: > C -> A > C -> B > So dropping a role would be disallowed automatically without any code > changes, with the checkSharedDependencies() call that's already in > DropRole. Adding a role membership would require a bit more work, > because we'd first need to check that there's not already a pg_shdepend > entry for that combination. Removing a role membership also becomes > more work; we need to check that no other grant depends on the same > grantor before removing the entry. Both of these have got race conditions ... not but what the dependency code has got race condition problems already, but maybe we should try to avoid introducing more? I haven't got any better ideas though. Why is it that we record grantor at all? One could argue that granting membership in a role is done on behalf of that role and there's no real need to remember exactly who did it. regards, tom lane
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > So I'm currently considering the following alternatives: > > > 1. do nothing at all with pg_shdepend. Upon role deletion, seqscan > > pg_auth_members and reject the drop altogether if there is a role > > granted to another which mentions the to-be-dropped role ID as grantor. > > This is easiest in terms of code (it's even mentioned in the comments in > > DropRole). > > > 2. record one pg_shdepend entry for each role that has granted something > > to each role (unless the grantor is the same role being granted, in > > which case we needn't record anything). So if role A grants Z and X to > > C, and role B grants Y and W to C, C now has access to W, Y, X and Z and > > there are two pg_shdepend entries: > > C -> A > > C -> B > > So dropping a role would be disallowed automatically without any code > > changes, with the checkSharedDependencies() call that's already in > > DropRole. Adding a role membership would require a bit more work, > > because we'd first need to check that there's not already a pg_shdepend > > entry for that combination. Removing a role membership also becomes > > more work; we need to check that no other grant depends on the same > > grantor before removing the entry. > > Both of these have got race conditions ... not but what the dependency > code has got race condition problems already, but maybe we should try > to avoid introducing more? I haven't got any better ideas though. I couldn't parse this paragraph very well. However I'm not sure why you say the dependency code has got race conditions? We do lock the object before checking the dependencies, so it's not possible to add a new dependency while we're dropping the object. .. right? I'm going to have a look at it again. > Why is it that we record grantor at all? One could argue that granting > membership in a role is done on behalf of that role and there's no real > need to remember exactly who did it. I think you should ask Stephen Frost about that -- added to CC. If the grantor bit is not important, then what we should do is just omit emitting the GRANTED BY part in pg_dumpall, which fixes this report. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes: > Tom Lane wrote: >> Both of these have got race conditions ... not but what the dependency >> code has got race condition problems already, but maybe we should try >> to avoid introducing more? I haven't got any better ideas though. > I couldn't parse this paragraph very well. However I'm not sure why you > say the dependency code has got race conditions? We do lock the object > before checking the dependencies, so it's not possible to add a new > dependency while we're dropping the object. Sorry, I was thinking of the regular dependency code, which has open bug report(s) based on exactly the fact that there's no such locking. shdepend may be OK, since it's fundamentally only dealing in roles. >> Why is it that we record grantor at all? One could argue that granting >> membership in a role is done on behalf of that role and there's no real >> need to remember exactly who did it. > I think you should ask Stephen Frost about that -- added to CC. > If the grantor bit is not important, then what we should do is just omit > emitting the GRANTED BY part in pg_dumpall, which fixes this report. It's at least something we should reflect on before sweating hard to make it work... regards, tom lane
Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Alvaro Herrera
Дата:
Tom Lane wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: > >> Why is it that we record grantor at all? One could argue that granting > >> membership in a role is done on behalf of that role and there's no real > >> need to remember exactly who did it. > > > I think you should ask Stephen Frost about that -- added to CC. > > > If the grantor bit is not important, then what we should do is just omit > > emitting the GRANTED BY part in pg_dumpall, which fixes this report. > > It's at least something we should reflect on before sweating hard to > make it work... I took a look, and concluded that the only bit of code that uses the grantor at all is pg_dumpall. Does this means we can remove it altogether? In back branches, we would take out the pg_dumpall code. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Re: Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Stephen Frost
Дата:
* Alvaro Herrera (alvherre@commandprompt.com) wrote: > I took a look, and concluded that the only bit of code that uses the > grantor at all is pg_dumpall. > > Does this means we can remove it altogether? In back branches, we would > take out the pg_dumpall code. I don't have time right at the moment (leaving shortly and will be gone all weekend) but what I would do is check the SQL standard, especially the information schema, for any requirement to track the grantor. Much of what I did was based on the standard so that may have been the instigation for tracking grantor. Though, even without that, we track the grantor of most other grants (possibly all currently?) and it seems like a useful bit of information for DBAs to be able to know who granted what to whom. I can't say I've used it though, personally. Of course, I'll be pretty unhappy if a day comes when I do need it and it's not there. :) Thanks, Stephen
Re: Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Alvaro Herrera
Дата:
Stephen Frost wrote: > I don't have time right at the moment (leaving shortly and will be gone > all weekend) but what I would do is check the SQL standard, especially > the information schema, for any requirement to track the grantor. Much > of what I did was based on the standard so that may have been the > instigation for tracking grantor. Hmm. I had forgotten the information schema. I just checked: the only view using pg_auth_members is APPLICABLE_ROLES, and that one doesn't display the grantor column. > Though, even without that, we track > the grantor of most other grants (possibly all currently?) and it seems > like a useful bit of information for DBAs to be able to know who granted > what to whom. I note that the grantor of ACLs are listed separately, for example in COLUMN_PRIVILEGES, ROLE_COLUMN_GRANTS, etc. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Re: Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Alvaro Herrera
Дата:
Alvaro Herrera wrote: > Stephen Frost wrote: > > > I don't have time right at the moment (leaving shortly and will be gone > > all weekend) but what I would do is check the SQL standard, especially > > the information schema, for any requirement to track the grantor. Much > > of what I did was based on the standard so that may have been the > > instigation for tracking grantor. > > Hmm. I had forgotten the information schema. I just checked: the only > view using pg_auth_members is APPLICABLE_ROLES, and that one doesn't > display the grantor column. This section of the standard is relevant: 4.34.3 Roles Each grant is represented and identified by a role authorization descriptor. A role authorization descriptor includes: — The role name of the role. — The <authorization identifier> of the grantor. — The <authorization identifier> of the grantee. — An indication of whether or not the role was granted with the WITH ADMIN OPTION and hence is grantable. ... continues reading the spec ... Ah, here it is, 12.7 <revoke statement>. It says that if role revokes another role from a third role, it will only remove the privileges that were granted by him, not someone else. That is, if roles A and B grant a role Z to C, and then role A revokes Z from C, then role C continues to have the role Z because of the grant B gave. So we have a problem here, because this alvherre=# create role a; CREATE ROLE alvherre=# create role b; CREATE ROLE alvherre=# create role z admin a, b; CREATE ROLE alvherre=# create role c; CREATE ROLE alvherre=# set session authorization a; SET alvherre=> grant z to c; GRANT ROLE alvherre=> set session authorization b; SET alvherre=> grant z to c; NOTICE: role "c" is already a member of role "z" should not emit any noise, but instead add another grant of Z to C with grantor B. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Re: Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Stephen Frost
Дата:
* Alvaro Herrera (alvherre@commandprompt.com) wrote: > Ah, here it is, 12.7 <revoke statement>. It says that if role revokes > another role from a third role, it will only remove the privileges that > were granted by him, not someone else. Hmm. I'm not sure, but that may have been a case where it was generally decided that the spec was somewhat braindead in this fashion (it seems so in my personal view of this, honestly...). To issue a revoke and have it not work would be kind of concerning. If we do end up following this path we should emit a warning (at least...) if the user still has the rights which are being revoked, even if through someone else. Perhaps that also implies that tracking the grantor is unnecessary. Thanks, Stephen
Stephen Frost <sfrost@snowman.net> writes: > Hmm. I'm not sure, but that may have been a case where it was generally > decided that the spec was somewhat braindead in this fashion (it seems > so in my personal view of this, honestly...). To issue a revoke and > have it not work would be kind of concerning. If we do end up following > this path we should emit a warning (at least...) if the user still has > the rights which are being revoked, even if through someone else. That's not how it works for rights on ordinary objects. regards, tom lane
Re: Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Stephen Frost
Дата:
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > Hmm. I'm not sure, but that may have been a case where it was generally > > decided that the spec was somewhat braindead in this fashion (it seems > > so in my personal view of this, honestly...). To issue a revoke and > > have it not work would be kind of concerning. If we do end up following > > this path we should emit a warning (at least...) if the user still has > > the rights which are being revoked, even if through someone else. > > That's not how it works for rights on ordinary objects. Not quite sure which bit you're referring to here.. On 8.1, at least, we ignore a grant which has a matching right and target: sfrost=> set role u1; sfrost=> \dp Access privileges for database "sfrost"Schema | Name | Type | Access privileges --------+------+-------+-------------------------sfrost | test | table | {u1=arwdRxt/u1,u3=r/u1} (1 row) sfrost=> reset role; RESET sfrost=> set role u2; SET sfrost=> grant select on test to u3; GRANT sfrost=> \dp Access privileges for database "sfrost"Schema | Name | Type | Access privileges --------+------+-------+-------------------------sfrost | test | table | {u1=arwdRxt/u1,u3=r/u1} (1 row) Additionally, any user with ownership rights on the table in question can revoke the rights of a user. Still as u2: sfrost=> revoke select on test from u3; REVOKE sfrost=> \dp Access privileges for database "sfrost"Schema | Name | Type | Access privileges --------+------+-------+-------------------sfrost | test | table | {u1=arwdRxt/u1} (1 row) If you're saying we don't currently warn if a revoke leaves the priviledges in-tact for the right and target, I'm not sure you can currently get in a state where it'd be possible to run into that. Either you have the rights to remove the grant on the object (you're an 'owner' of it), in which case the grant will be removed if it exists (based on the right and target, regardless of who granted it), or you don't, in which case you get a permission denied ERROR outright. If regular object permissions were ever changed to require the grantor to be the revoker, I would want a warning in the case described for regular objects as well. If you're saying we don't currently require that the grantor be the revoker on regular objects, I would agree. :) Thanks, Stephen
Stephen Frost <sfrost@snowman.net> writes: > If you're saying we don't currently warn if a revoke leaves the > priviledges in-tact for the right and target, I'm not sure you can > currently get in a state where it'd be possible to run into that. I'm thinking of the case that comes up periodically where newbies think that revoking a right from a particular user overrides a grant to PUBLIC of the same right. regards, tom lane
Re: Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Alvaro Herrera
Дата:
Based on the discussion so far, it seems to me that the sane course of action is to continue to register the grantor, because the standard mandates that it should be there; but ignore the parts where we revoke selectively, because that's a stupid thing to do. So we do deviate, if slightly. So we will have pg_dumpall do nothing special if the grantor has gone away since granting the privilege. That is, exactly the patch that was submitted, no new code needs to be written. (Maybe a mention in the "compatibility" section of REVOKE is warranted, though I'm not sure). Does anyone object to this course of action? -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Re: Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Stephen Frost
Дата:
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > If you're saying we don't currently warn if a revoke leaves the > > priviledges in-tact for the right and target, I'm not sure you can > > currently get in a state where it'd be possible to run into that. > > I'm thinking of the case that comes up periodically where newbies think > that revoking a right from a particular user overrides a grant to PUBLIC > of the same right. Technically, the grant to public is a different target from the target of the revoke in such a case. Following the spec would mean that even when the grant and the revoke target is the same (unless you're the original grantor) the right won't be removed. I'm not against adding a warning in the case you describe though, but I don't see it being as necessary for that case. What the spec describes is, at least in my view, much more counter-intuitive than how PG currently works. Thanks, Stephen
Re: Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Russell Smith
Дата:
Stephen Frost wrote: > * Tom Lane (tgl@sss.pgh.pa.us) wrote: > >> Stephen Frost <sfrost@snowman.net> writes: >> >>> If you're saying we don't currently warn if a revoke leaves the >>> priviledges in-tact for the right and target, I'm not sure you can >>> currently get in a state where it'd be possible to run into that. >>> >> I'm thinking of the case that comes up periodically where newbies think >> that revoking a right from a particular user overrides a grant to PUBLIC >> of the same right. >> > > Technically, the grant to public is a different target from the target > of the revoke in such a case. Following the spec would mean that even > when the grant and the revoke target is the same (unless you're the > original grantor) the right won't be removed. I'm not against adding a > warning in the case you describe though, but I don't see it being as > necessary for that case. What the spec describes is, at least in my > view, much more counter-intuitive than how PG currently works. > > > If we were to follow the spec, I would expect that it would be possible for the object owner to revoke privileges no matter what role granted them. It need not be the default, but as an object owner, I'd expect to be able to say that I want all privileges for a role revoked, no matter who granted them. 8.2 docs state this on the revoke page: -- REVOKE can also be done by a role that is not the owner of the affected object, but is a member of the role that owns the object, or is a member of a role that holds privileges WITH GRANT OPTION on the object. In this case the command is performed as though it were issued by the containing role that actually owns the object or holds the privileges WITH GRANT OPTION. For example, if table t1 is owned by role g1, of which role u1 is a member, then u1 can revoke privileges on t1 that are recorded as being granted by g1. This would include grants made by u1 as well as by other members of role g1. If the role executing REVOKE holds privileges indirectly via more than one role membership path, it is unspecified which containing role will be used to perform the command. In such cases it is best practice to use SET ROLE to become the specific role you want to do the REVOKE as. Failure to do so may lead to revoking privileges other than the ones you intended, or not revoking anything at all. -- Paragraph 1 implies that we are meeting the standard now. I think paragraph two is stating that if you are a member of multiple roles which could have granted privileges, then you don't know which one you are revoking. Makes sense if we are implementing the SQL standard. Does this mean we were intending to be SQL compliant when we wrote the documentation? I also note that 8.1 says the same thing in its documentation. My possible suggestion is; 1. Implement the standard for revoking only your privileges by default. 2. Allow the object owner to revoke privileges assigned by any role, as if you drop and recreate the object you can achieve this anyway. Regards Russell Smith
Re: Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Alvaro Herrera
Дата:
So the discussion died again with nothing being decided. I see we have several choices: 1. implement the standard, per Russell suggestion below 2. decide that the standard is braindead and just omit dumping the grantor when it's no longer available, but don't remove pg_auth_members.grantor 3. decide that the standard is braindead and remove pg_auth_members.grantor Which do people feel should be implemented? I can do whatever we decide; if no one has a strong opinion on the matter, my opinion is we do (2) which is the easiest. Russell Smith wrote: > My possible suggestion is; > 1. Implement the standard for revoking only your privileges by default. > 2. Allow the object owner to revoke privileges assigned by any role, as > if you drop and recreate the object you can achieve this anyway. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Re: Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Alvaro Herrera
Дата:
Alvaro Herrera wrote: > 2. decide that the standard is braindead and just omit dumping the > grantor when it's no longer available, but don't remove > pg_auth_members.grantor > > Which do people feel should be implemented? I can do whatever we > decide; if no one has a strong opinion on the matter, my opinion is we > do (2) which is the easiest. Here is a patch implementing this idea, vaguely based on Russell's. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Вложения
Re: Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Russell Smith
Дата:
Alvaro Herrera wrote: > Alvaro Herrera wrote: > > >> 2. decide that the standard is braindead and just omit dumping the >> grantor when it's no longer available, but don't remove >> pg_auth_members.grantor >> >> Which do people feel should be implemented? I can do whatever we >> decide; if no one has a strong opinion on the matter, my opinion is we >> do (2) which is the easiest. >> > > Here is a patch implementing this idea, vaguely based on Russell's. > I haven't had time to finalize my research about this, but the admin option with revoke doesn't appear to work as expected. Here is my sample SQL for 8.2.4 create table test (x integer); \z create role test1 noinherit; create role test2 noinherit; grant select on test to test1 with grant option; grant select on test to test2; \z test set role test1; revoke select on test from test2; \z test set role test2; select * from test; reset role; revoke all on test from test2; revoke all on test from test1; drop role test2; drop role test1; drop table test; \q The privilege doesn't appear to be revoked by test1 from test2. I'm not sure if this is related, but I wanted to bring it up in light of the options we have for grantor.
Re: Removing pg_auth_members.grantor (was Grantor name gets lost when grantor role dropped)
От
Alvaro Herrera
Дата:
Russell Smith wrote: > Alvaro Herrera wrote: > >Alvaro Herrera wrote: > > > > > >>2. decide that the standard is braindead and just omit dumping the > >> grantor when it's no longer available, but don't remove > >> pg_auth_members.grantor > >> > >>Which do people feel should be implemented? I can do whatever we > >>decide; if no one has a strong opinion on the matter, my opinion is we > >>do (2) which is the easiest. > > > >Here is a patch implementing this idea, vaguely based on Russell's. > > I haven't had time to finalize my research about this, but the admin > option with revoke doesn't appear to work as expected. > > Here is my sample SQL for 8.2.4 > > create table test (x integer); > \z > create role test1 noinherit; > create role test2 noinherit; > grant select on test to test1 with grant option; > grant select on test to test2; > \z test > set role test1; > revoke select on test from test2; > \z test > set role test2; > select * from test; > reset role; > revoke all on test from test2; > revoke all on test from test1; > drop role test2; > drop role test1; > drop table test; > \q > > > The privilege doesn't appear to be revoked by test1 from test2. I'm not > sure if this is related, but I wanted to bring it up in light of the > options we have for grantor. Humm, but the privilege was not granted by test1, but by the user you were using initially. The docs state in a note that A user can only revoke privileges that were granted directly bythat user. I understand that this would apply to the grantor stuff being discussed in this thread as well, but I haven't seen anyone arguing that we should implement that for GRANT ROLE (and I asked three times if people felt it was important and nobody answered). -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera wrote: > Alvaro Herrera wrote: > > > 2. decide that the standard is braindead and just omit dumping the > > grantor when it's no longer available, but don't remove > > pg_auth_members.grantor > > > > Which do people feel should be implemented? I can do whatever we > > decide; if no one has a strong opinion on the matter, my opinion is we > > do (2) which is the easiest. > > Here is a patch implementing this idea, vaguely based on Russell's. Applied to CVS HEAD, 8.2 and 8.1. If we want to start tracking the grantor as a shared dependency, and have REVOKE work per spec (i.e. only revoke the privileges actually granted by the role executing REVOKE), those are separate patches (and they should be applied only to HEAD). This patch merely fixes the fact that pg_dumpall failed to work for busted databases. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Alvaro Herrera wrote: > >> Alvaro Herrera wrote: >> >> >>> 2. decide that the standard is braindead and just omit dumping the >>> grantor when it's no longer available, but don't remove >>> pg_auth_members.grantor >>> >>> Which do people feel should be implemented? I can do whatever we >>> decide; if no one has a strong opinion on the matter, my opinion is we >>> do (2) which is the easiest. >>> >> Here is a patch implementing this idea, vaguely based on Russell's. >> > > Applied to CVS HEAD, 8.2 and 8.1. > > If we want to start tracking the grantor as a shared dependency, and > have REVOKE work per spec (i.e. only revoke the privileges actually > granted by the role executing REVOKE), those are separate patches (and > they should be applied only to HEAD). This patch merely fixes the fact > that pg_dumpall failed to work for busted databases. > > Should there also be a doc patch for this, the document descriptions seemed different to what is actually implemented. I'll check that before I make any further comments. Russell
Alvaro Herrera wrote: > Russell Smith wrote: > >> Alvaro Herrera wrote: >> >>> Alvaro Herrera wrote: >>> >>> >>> >>>> 2. decide that the standard is braindead and just omit dumping the >>>> grantor when it's no longer available, but don't remove >>>> pg_auth_members.grantor >>>> >>>> Which do people feel should be implemented? I can do whatever we >>>> decide; if no one has a strong opinion on the matter, my opinion is we >>>> do (2) which is the easiest. >>>> >>> Here is a patch implementing this idea, vaguely based on Russell's. >>> >> I haven't had time to finalize my research about this, but the admin >> option with revoke doesn't appear to work as expected. >> >> Here is my sample SQL for 8.2.4 >> >> create table test (x integer); >> \z >> create role test1 noinherit; >> create role test2 noinherit; >> grant select on test to test1 with grant option; >> grant select on test to test2; >> \z test >> set role test1; >> revoke select on test from test2; >> \z test >> set role test2; >> select * from test; >> reset role; >> revoke all on test from test2; >> revoke all on test from test1; >> drop role test2; >> drop role test1; >> drop table test; >> \q >> >> >> The privilege doesn't appear to be revoked by test1 from test2. I'm not >> sure if this is related, but I wanted to bring it up in light of the >> options we have for grantor. >> > > Humm, but the privilege was not granted by test1, but by the user you > were using initially. The docs state in a note that > > A user can only revoke privileges that were granted directly by > that user. > > I understand that this would apply to the grantor stuff being discussed > in this thread as well, but I haven't seen anyone arguing that we should > implement that for GRANT ROLE (and I asked three times if people felt it > was important and nobody answered). > > Well, I would vote for implementing this in GRANT ROLE. I wish to use it in my security model. I don't think the spec is brain dead when you understand what it's trying to achieve. Example: 2 Groups of administrators who are allowed to grant a role to users of the system App_Admin_G1 App_Admin_G2 App_User SET ROLE App_Admin_G1 GRANT App_User TO "Fred"; SET ROLE App_Admin_G2 GRANT App_User TO "John"; SET ROLE App_Admin_G1 REVOKE App_User FROM "John"; As App_Admin_G1 did not grant App_User rights to John, he should not be able to take them away. I currently have a situation where I would like to be able to do the above. I have two separate departments who might grant privileges for the same application to the same user. One department administrator should not be able to revoke the privileges set by the other one. I would expect superusers to be able to revoke from anybody, or the "owner". I'm not sure what the owner is when we talk about granting roles. Regards Russell Smith
Is there a TODO here? --------------------------------------------------------------------------- Russell Smith wrote: > Alvaro Herrera wrote: > > Russell Smith wrote: > > > >> Alvaro Herrera wrote: > >> > >>> Alvaro Herrera wrote: > >>> > >>> > >>> > >>>> 2. decide that the standard is braindead and just omit dumping the > >>>> grantor when it's no longer available, but don't remove > >>>> pg_auth_members.grantor > >>>> > >>>> Which do people feel should be implemented? I can do whatever we > >>>> decide; if no one has a strong opinion on the matter, my opinion is we > >>>> do (2) which is the easiest. > >>>> > >>> Here is a patch implementing this idea, vaguely based on Russell's. > >>> > >> I haven't had time to finalize my research about this, but the admin > >> option with revoke doesn't appear to work as expected. > >> > >> Here is my sample SQL for 8.2.4 > >> > >> create table test (x integer); > >> \z > >> create role test1 noinherit; > >> create role test2 noinherit; > >> grant select on test to test1 with grant option; > >> grant select on test to test2; > >> \z test > >> set role test1; > >> revoke select on test from test2; > >> \z test > >> set role test2; > >> select * from test; > >> reset role; > >> revoke all on test from test2; > >> revoke all on test from test1; > >> drop role test2; > >> drop role test1; > >> drop table test; > >> \q > >> > >> > >> The privilege doesn't appear to be revoked by test1 from test2. I'm not > >> sure if this is related, but I wanted to bring it up in light of the > >> options we have for grantor. > >> > > > > Humm, but the privilege was not granted by test1, but by the user you > > were using initially. The docs state in a note that > > > > A user can only revoke privileges that were granted directly by > > that user. > > > > I understand that this would apply to the grantor stuff being discussed > > in this thread as well, but I haven't seen anyone arguing that we should > > implement that for GRANT ROLE (and I asked three times if people felt it > > was important and nobody answered). > > > > > Well, I would vote for implementing this in GRANT ROLE. I wish to use > it in my security model. I don't think the spec is brain dead when you > understand what it's trying to achieve. > > Example: > > 2 Groups of administrators who are allowed to grant a role to users of > the system > > App_Admin_G1 > App_Admin_G2 > App_User > > SET ROLE App_Admin_G1 > GRANT App_User TO "Fred"; > SET ROLE App_Admin_G2 > GRANT App_User TO "John"; > SET ROLE App_Admin_G1 > REVOKE App_User FROM "John"; > > As App_Admin_G1 did not grant App_User rights to John, he should not be > able to take them away. > > I currently have a situation where I would like to be able to do the > above. I have two separate departments who might grant privileges for > the same application to the same user. One department administrator > should not be able to revoke the privileges set by the other one. > > I would expect superusers to be able to revoke from anybody, or the > "owner". I'm not sure what the owner is when we talk about granting roles. > > Regards > > Russell Smith > > > ---------------------------(end of broadcast)--------------------------- > TIP 5: don't forget to increase your free space map settings -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > > Is there a TODO here? Yes, I think so: * Implement the SQL standard mechanism whereby REVOKE ROLE only revokes the privilege as granted by the invoking role, andnot those granted by other roles -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Added to TODO: --------------------------------------------------------------------------- Alvaro Herrera wrote: > Bruce Momjian wrote: > > > > Is there a TODO here? > > Yes, I think so: > > * Implement the SQL standard mechanism whereby REVOKE ROLE only revokes > the privilege as granted by the invoking role, and not those granted > by other roles > > > -- > Alvaro Herrera http://www.CommandPrompt.com/ > The PostgreSQL Company - Command Prompt, Inc. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +