Обсуждение: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

VGhlIGZvbGxvd2luZyBidWcgaGFzIGJlZW4gbG9nZ2VkIG9uIHRoZSB3ZWJz
aXRlOgoKQnVnIHJlZmVyZW5jZTogICAgICAxNDI0MgpMb2dnZWQgYnk6ICAg
ICAgICAgIE5hdGhhbiBCb3NzYXJ0CkVtYWlsIGFkZHJlc3M6ICAgICAgYm9z
c2FydG5AYW1hem9uLmNvbQpQb3N0Z3JlU1FMIHZlcnNpb246IDkuNS4yCk9w
ZXJhdGluZyBzeXN0ZW06ICAgNjQtYml0IExpbnV4CkRlc2NyaXB0aW9uOiAg
ICAgICAgCgpJdCBpcyBwb3NzaWJsZSB0byBtb2RpZnkgdGhlICJyb2xlIiBz
ZXR0aW5nIGluIHNldGNvbmZpZyBpbiB0aGUKcGdfZGJfcm9sZV9zZXR0aW5n
IHRhYmxlIHN1Y2ggdGhhdCBpdCBwb2ludHMgdG8gYSBub25leGlzdGVudCBy
b2xlLiAgV2hlbgp0aGlzIGlzIHRoZSBjYXNlLCByZXN0b3JpbmcgdGhlIG91
dHB1dCBvZiBwZ19kdW1wYWxsIHdpbGwgZmFpbCBkdWUgdG8gdGhlCm1pc3Np
bmcgcm9sZS4NCiANClN0ZXBzIHRvIHJlcHJvZHVjZToNCiANCjEuIEFzIHN1
cGVydXNlciwgZXhlY3V0ZSAiY3JlYXRlIHJvbGUgZm9vIHdpdGggbG9naW4g
cGFzc3dvcmQgJ3Rlc3QnIg0KMi4gQXMgZm9vLCBleGVjdXRlICJhbHRlciBy
b2xlIGZvbyBzZXQgcm9sZSA9ICdmb28nIg0KMy4gQXMgc3VwZXJ1c2VyLCBl
eGVjdXRlICJhbHRlciByb2xlIGZvbyByZW5hbWUgdG8gYmFyIg0KICAgICAg
ICBhLiBBdCB0aGlzIHBvaW50LCB0aGUgc2V0Y29uZmlnIGVudHJ5IGluIHBn
X2RiX3JvbGVfc2V0dGluZyBmb3IKJ2Jhcicgd2lsbCBjb250YWluICd7cm9s
ZT1mb299Jywgd2hpY2ggbm8gbG9uZ2VyIGV4aXN0cw0KNC4gRXhlY3V0ZSBw
Z191cGdyYWRlIHdpdGggdGhlIHJlY29tbWVuZGVkIHN0ZXBzIGluCmh0dHBz
Oi8vd3d3LnBvc3RncmVzcWwub3JnL2RvY3MvY3VycmVudC9zdGF0aWMvcGd1
cGdyYWRlLmh0bWwNCiANCkR1cmluZyBwZ191cGdyYWRlIChtb3JlIHNwZWNp
ZmljYWxseSwgZHVyaW5nIHRoZSByZXN0b3JlIG9mIHRoZSBvdXRwdXQgZnJv
bQpwZ19kdW1wYWxsKSwgdGhlICJBTFRFUiBST0xFICJiYXIiIFNFVCAicm9s
ZSIgVE8gJ2ZvbyciIGNvbW1hbmQgZ2VuZXJhdGVkCndpbGwgZmFpbCB3aXRo
ICJFUlJPUjogcm9sZSAiZm9vIiBkb2VzIG5vdCBleGlzdCIuDQogDQpUaGlz
IGlzc3VlIHdhcyBpZGVudGlmaWVkIGJ5IEpvcmRhbiBMYW5nZSBhbmQgTmF0
aGFuIEJvc3NhcnQuDQoKCg==

A simple fix for this issue would be to temporarily create the role just for the ALTER ROLE SET command generated by
pg_dumpall. I've attached a patch that does this.
 
 
Nathan

bossartn@amazon.com writes:
> It is possible to modify the "role" setting in setconfig in the
> pg_db_role_setting table such that it points to a nonexistent role.  When
> this is the case, restoring the output of pg_dumpall will fail due to the
> missing role.

> Steps to reproduce:

> 1. As superuser, execute "create role foo with login password 'test'"
> 2. As foo, execute "alter role foo set role = 'foo'"
> 3. As superuser, execute "alter role foo rename to bar"
>         a. At this point, the setconfig entry in pg_db_role_setting for
> 'bar' will contain '{role=foo}', which no longer exists
> 4. Execute pg_upgrade with the recommended steps in
> https://www.postgresql.org/docs/current/static/pgupgrade.html

> During pg_upgrade (more specifically, during the restore of the output from
> pg_dumpall), the "ALTER ROLE "bar" SET "role" TO 'foo'" command generated
> will fail with "ERROR: role "foo" does not exist".

This does not seem like particularly a bug to me.  Once you rename the
role, the ALTER ROLE SET setting is broken already, and it's on your head
to fix that, not pg_upgrade's.  (Or in other words, promising that
pg_upgrade will succeed in already-malfunctioning installations seems
to me like a slope we'd better not start down.)

I am kind of wondering why we allow that parameter to be set in ALTER
ROLE/DATABASE SET at all, though; especially by unprivileged users.
Is this example based on a real use-case, and if so what is it?

            regards, tom lane

On Monday, July 11, 2016, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> bossartn@amazon.com <javascript:;> writes:
> > It is possible to modify the "role" setting in setconfig in the
> > pg_db_role_setting table such that it points to a nonexistent role.  When
> > this is the case, restoring the output of pg_dumpall will fail due to the
> > missing role.
>
> > Steps to reproduce:
>
> > 1. As superuser, execute "create role foo with login password 'test'"
> > 2. As foo, execute "alter role foo set role = 'foo'"
> > 3. As superuser, execute "alter role foo rename to bar"
> >         a. At this point, the setconfig entry in pg_db_role_setting for
> > 'bar' will contain '{role=foo}', which no longer exists
> > 4. Execute pg_upgrade with the recommended steps in
> > https://www.postgresql.org/docs/current/static/pgupgrade.html
>
> > During pg_upgrade (more specifically, during the restore of the output
> from
> > pg_dumpall), the "ALTER ROLE "bar" SET "role" TO 'foo'" command generated
> > will fail with "ERROR: role "foo" does not exist".
>
> This does not seem like particularly a bug to me.  Once you rename the
> role, the ALTER ROLE SET setting is broken already, and it's on your head
> to fix that, not pg_upgrade's.  (Or in other words, promising that
> pg_upgrade will succeed in already-malfunctioning installations seems
> to me like a slope we'd better not start down.)
>
>
I'm at a loss to understand what this does when it isn't broken.  Assuming
valid grants does the user become the assigned role upon session startup?
Is the behavior even defined in the docs or is this, as suspected below,
and malfunctioning implementation detail?

I am kind of wondering why we allow that parameter to be set in ALTER
> ROLE/DATABASE SET at all, though; especially by unprivileged users.
> Is this example based on a real use-case, and if so what is it?
>

See also:

https://www.postgresql.org/message-id/20150212193420.2590.49423@wrigleys.postgresql.org

Upon further reflection we require custom variables to be namespaced.
There is not, to my knowledge, a documented guc variable named "role".

Something seems off here, but I cannot put my finger on it, even if the
error in question is one we cannot reasonably avoid.

David J.

"David G. Johnston" <david.g.johnston@gmail.com> writes:
>> bossartn@amazon.com <javascript:;> writes:
>>> 2. As foo, execute "alter role foo set role = 'foo'"

> I'm at a loss to understand what this does when it isn't broken.  Assuming
> valid grants does the user become the assigned role upon session startup?

As written it does nothing much.  But "SET ROLE" is defined by the SQL
standard, and what I'd expect this to do is execute an implicit SET ROLE
at login.  Whether that's a good idea is pretty debatable, though, thus
my question whether we'd not be better off to forbid this.

            regards, tom lane

>>>>> "David" == David G Johnston <david.g.johnston@gmail.com> writes:

 >> > 2. As foo, execute "alter role foo set role = 'foo'"

 David> I'm at a loss to understand what this does when it isn't broken.

ALTER ROLE foo SET role = bar;   will have the effect that when 'foo'
logs in, it will behave as if foo had immediately executed the command
SET ROLE bar;

I don't think this is documented but it has obvious uses.

--
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> I don't think this is documented but it has obvious uses.

Does it?  If the named role is the same as the actual role, then it's
useless.  If they're different, it seems at best confusing.  In the
context of ALTER DATABASE SET, it seems both confusing and possibly
a security hazard.

            regards, tom lane

I cannot speak to any real use-cases beyond easily altering current_user ev=
ery time you start a session.

Besides disallowing this parameter in ALTER ROLE SET statements entirely, p=
erhaps pg_dumpall could simply skip it for missing roles?  I cannot think o=
f a use-case for maintaining an invalid setting here.

Nathan

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 > Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
 >> I don't think this is documented but it has obvious uses.

 Tom> Does it?

For ALTER ROLE, there's actually a question that comes up not all that
infrequently on irc: "how do I arrange things so that what user 'foo'
does, by default, ends up owned by group role 'bar'"

I'm pretty sure I have never actually suggested that anyone do it this
way (because I had no idea it worked until I tried it just now), but I
can see the use case.

 Tom> If the named role is the same as the actual role, then it's
 Tom> useless.  If they're different, it seems at best confusing.  In
 Tom> the context of ALTER DATABASE SET, it seems both confusing and
 Tom> possibly a security hazard.

It _appears_ to silently fail if the user logging in is not actually a
member of the specified role. I have not looked at the code.

--
Andrew (irc:RhodiumToad)

On Mon, Jul 11, 2016 at 7:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> "David G. Johnston" <david.g.johnston@gmail.com> writes:
> >> bossartn@amazon.com <javascript:;> writes:
> >>> 2. As foo, execute "alter role foo set role =3D 'foo'"
>
> > I'm at a loss to understand what this does when it isn't broken.
> Assuming
> > valid grants does the user become the assigned role upon session startu=
p?
>
> As written it does nothing much.  But "SET ROLE" is defined by the SQL
> standard, and what I'd expect this to do is execute an implicit SET ROLE
> at login.  Whether that's a good idea is pretty debatable, though, thus
> my question whether we'd not be better off to forbid this
> =E2=80=8B.
>

=E2=80=8BSo, I just tested and we indeed do (9.6beta-2=E2=80=8B) make the e=
ffective role
the value associated with the "ROLE" configuration variable associated to
the user.

CREATE ROLE loginrole WITH LOGIN PASSWORD 'password';
CREATE ROLE grouprole;
GRANT grouprole TO loginrole
ALTER ROLE loginrole SET ROLE TO grouprole
=E2=80=8Bpsql -U loginrole postgres
SELECT current_role; --> grouprole=E2=80=8B

=E2=80=8B=E2=80=8BI'd say that the expression "SET ROLE" as defined does no=
t match any of
the valid variations of ALTER ROLE that we've documented (i.e., ROLE is not
a "configuration_parameter").  We should document it explicitly.

ALTER ROLE { role_specification | ALL } [ IN DATABASE database_name ] SET
ROLE { TO | =3D } role_identifier
Does the standard provide guidance on the syntax for the equivalent of
"RESET ROLE"?

"The role_identifier in the <third> variant is used in conjunction with the
SET ROLE SQL command and causes a newly connected session to switch to the
named role.  If for some reason the named role no longer exists - which can
happen if it is renamed or removed - future attempts to login will provoke
a WARNING and the original user will remain active.  Note that this role
change happens after all configuration_parameters for the original role
have been setup - no configurations attached to the target role are applied=
.

role_identifier:
The name of an existing role

I would also take a page from the search_path GUC and ignore any attempt to
associate an undefined role - or at worse make it a warning.  In fact, we
already do this during the login attempt.  We should extend the forgiveness
to here as well.

DROP ROLE grouprole; -- OK
psql -U loginrole postgres
=E2=80=8BWARNING:\s\srole "group=E2=80=8Brole" does not exist
=E2=80=8BSELECT current_role; --> loginrole=E2=80=8B


In hindsight we probably could do better if we didn't treat "ROLE" like any
other configuration parameter.  I'm not sure what incremental improvements
could be made. Would a record in pg_depend that is set/cleared upon
invoking of ROLE-related commands?  Would that be sufficient?

David J.

On Mon, Jul 11, 2016 at 7:36 PM, Andrew Gierth <andrew@tao11.riddles.org.uk=
>
wrote:

>  Tom> If the named role is the same as the actual role, then it's
>  Tom> useless.  If they're different, it seems at best confusing.  In
>  Tom> the context of ALTER DATABASE SET, it seems both confusing and
>  Tom> possibly a security hazard.
>
> It _appears_ to silently fail if the user logging in is not actually a
> member of the specified role. I have not looked at the code.
>

=E2=80=8BWARNING:\s\spermission denied to set role "grouprole"
=E2=80=8B
David J.

On Mon, Jul 11, 2016 at 8:05 PM, David G. Johnston <
david.g.johnston@gmail.com> wrote:

> On Mon, Jul 11, 2016 at 7:36 PM, Andrew Gierth <
> andrew@tao11.riddles.org.uk> wrote:
>
>>  Tom> If the named role is the same as the actual role, then it's
>>  Tom> useless.  If they're different, it seems at best confusing.  In
>>  Tom> the context of ALTER DATABASE SET, it seems both confusing and
>>  Tom> possibly a security hazard.
>>
>> It _appears_ to silently fail if the user logging in is not actually a
>> member of the specified role. I have not looked at the code.
>>
>
> =E2=80=8BWARNING:\s\spermission denied to set role "grouprole"
>
>
=E2=80=8BFun times...
[up-thread commands still in effect]
ALTER DATABASE postgres SET ROLE loginrole2;
psql -U loginrole postgres
WARNING:  permission denied to set role "grouprole"
WARNING:  permission denied to set role "loginrole2"
postgres=3D>

Note the code comment at about: =E2=80=8Bsrc/backend/commands/user.c@478-47=
9

"
Although it will work to say
=E2=80=8B =E2=80=8B
ALTER ROLE role ROLE rolenames", we don't document it.
=E2=80=8B"

While that's good to know the specific syntax in the comment is invalid on
its face.  It also doesn't say "why" we don't document it nor why it needs
to be accepted.  I'd say at this point the why is immaterial though.

I'm still in favor of documenting both commands and reducing our parental
involvement here.

In light of the above double-warning I'm concerned that "precedence" isn't
happening correctly here - but that could be an implementation artifact
(the more specific combination is executed second so that it ends up
overriding any settings attempted to be set by the less specific
=E2=80=8Bconfiguration).  In this case, though, the failed attempt to set t=
he
db+role setting would have resulted in the role setting taking effect if it
was valid.  I don't recall us making this distinction clear in the
documentation.

Tangentially, I'm not sure what, if anything, to do with 18.1 given this
knowledge.  18.1 was written assuming that the GUC variation of these
commands cannot fail and thus it is safe to execute the DATABASE version
followed by a ROLE specific version followed by a DATABASE+ROLE version.
This seems correct on its face and as I said up-thread this whole ROLE
business isn't really a configuration variable even though it is
shoe-horned into that infrastructure.=E2=80=8B  I'm inclined to leave well =
enough
alone.

David J.

Sorry...I keep trying to dig deeper and keep discovering/realizing stuff.

On Mon, Jul 11, 2016 at 10:08 PM, David G. Johnston <
david.g.johnston@gmail.com> wrote:

> =E2=80=8B
>>
> =E2=80=8BFun times...
> [up-thread commands still in effect]
> ALTER DATABASE postgres SET ROLE loginrole2;
> psql -U loginrole postgres
> WARNING:  permission denied to set role "grouprole"
> WARNING:  permission denied to set role "loginrole2"
> postgres=3D>
>
> In light of the above double-warning I'm concerned that "precedence" isn'=
t
> happening correctly here - but that could be an implementation artifact
> (the more specific combination is executed second so that it ends up
> overriding any settings attempted to be set by the less specific
> =E2=80=8Bconfiguration).  In this case, though, the failed attempt to set=
 the
> db+role setting would have resulted in the role setting taking effect if =
it
> was valid.  I don't recall us making this distinction clear in the
> documentation.
>
>
Actually, apparently the system realizes =E2=80=8Bits attempt to SET ROLE <=
role-set
value> failed and proceeded to attempt to "SET ROLE <db-set value>" -
assuming the visible order is reflective of reality.  So it does have the
necessary smarts and also fall-back-try-again logic.

The rest of the documentation observations stand.

David J.

FYI, Tom just applied a patch to properly detect/fix this issue.

---------------------------------------------------------------------------

On Mon, Jul 11, 2016 at 10:15:38PM -0400, David G. Johnston wrote:
> Sorry...I keep trying to dig deeper and keep discovering/realizing stuff.
> 
> On Mon, Jul 11, 2016 at 10:08 PM, David G. Johnston <david.g.johnston@gmail.com
> > wrote:
> 
>         ​
> 
>     ​Fun times...
>     [up-thread commands still in effect]
>     ALTER DATABASE postgres SET ROLE loginrole2;
>     psql -U loginrole postgres
>     WARNING:  permission denied to set role "grouprole"
>     WARNING:  permission denied to set role "loginrole2"
>     postgres=>
> 
>     In light of the above double-warning I'm concerned that "precedence" isn't
>     happening correctly here - but that could be an implementation artifact
>     (the more specific combination is executed second so that it ends up
>     overriding any settings attempted to be set by the less specific
>     ​configuration).  In this case, though, the failed attempt to set the
>     db+role setting would have resulted in the role setting taking effect if it
>     was valid.  I don't recall us making this distinction clear in the
>     documentation.
>    
> 
> 
> Actually, apparently the system realizes ​its attempt to SET ROLE <role-set
> value> failed and proceeded to attempt to "SET ROLE <db-set value>" - assuming
> the visible order is reflective of reality.  So it does have the necessary
> smarts and also fall-back-try-again logic.
> 
> The rest of the documentation observations stand.
> 
> David J.
> 

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.