Grantor name gets lost when grantor role dropped

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

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

* 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

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.

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.

* 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

* 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