Horton

Thread

Commits

  1. Fix some inappropriately-disallowed uses of ALTER ROLE/DATABASE SET.

  1. BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    Nathan Bossart <bossartn@amazon.com> — 2016-07-11T22:36:41Z

    The following bug has been logged on the website:

Bug reference:      14242
Logged by:          Nathan Bossart
Email address:      bossartn@amazon.com
PostgreSQL version: 9.5.2
Operating system:   64-bit Linux
Description:        

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 issue was identified by Jordan Lange and Nathan Bossart.

  2. Re: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    Nathan Bossart <bossartn@amazon.com> — 2016-07-11T22:48:08Z

    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

  3. Re: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-07-11T22:49:06Z

    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

  4. Re: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    David G. Johnston <david.g.johnston@gmail.com> — 2016-07-11T23:03:22Z

    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.

  5. Re: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-07-11T23:12:49Z

    "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

  6. Re: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2016-07-11T23:18:06Z

    >>>>> "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)

  7. Re: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    Nathan Bossart <bossartn@amazon.com> — 2016-07-11T23:22:02Z

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

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

Nathan

  8. Re: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-07-11T23:22:12Z

    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

  9. Re: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2016-07-11T23:36:05Z

    >>>>> "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)

  10. Re: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    David G. Johnston <david.g.johnston@gmail.com> — 2016-07-12T00:01:30Z

    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 = '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
> ​.
>

​So, I just tested and we indeed do (9.6beta-2​) make the effective 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
​psql -U loginrole postgres
SELECT current_role; --> grouprole​

​​I'd say that the expression "SET ROLE" as defined does not 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 | = } 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
​WARNING:\s\srole "group​role" does not exist
​SELECT current_role; --> loginrole​


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.

  11. Re: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    David G. Johnston <david.g.johnston@gmail.com> — 2016-07-12T00:05:40Z

    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.
>

​WARNING:\s\spermission denied to set role "grouprole"
​
David J.

  12. Re: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    David G. Johnston <david.g.johnston@gmail.com> — 2016-07-12T02:08:54Z

    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.
>>
>
> ​WARNING:\s\spermission denied to set role "grouprole"
>
>
​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=>

Note the code comment at about: ​src/backend/commands/user.c@478-479

"
Although it will work to say
​ ​
ALTER ROLE role ROLE rolenames", we don't document it.
​"

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
​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.

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.​  I'm inclined to leave well enough
alone.

David J.

  13. Re: BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    David G. Johnston <david.g.johnston@gmail.com> — 2016-07-12T02:15:38Z

    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.

  14. Re: [BUGS] BUG #14242: Role with a setconfig "role" setting to a nonexistent role causes pg_upgrade to fail

    Bruce Momjian <bruce@momjian.us> — 2021-04-14T20:13:43Z

    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.