Обсуждение: "pg_xxx" role name restriction not applied to bootstrap superuser?

I noticed that opossum's latest buildfarm run failed, evidently because
it was set up with the user running the buildfarm named "pg_buildfarmer":

http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=opossum&dt=2016-05-03%2018%3A43%3A31

That caused the bootstrap superuser's name to be "pg_buildfarmer".
initdb didn't complain about this, but it was impossible to log in:

016-05-04 02:29:00.820 BST [5729505c.26:1] LOG:  connection received: host=[local]
2016-05-04 02:29:00.927 BST [5729505c.26:2] LOG:  connection authorized: user=pg_buildfarmer database=postgres
2016-05-04 02:29:00.932 BST [5729505c.26:3] FATAL:  invalid value for parameter "session_authorization":
"pg_buildfarmer"
2016-05-04 02:29:02.260 BST [5729505e.4396:1] LOG:  connection received: host=[local]
2016-05-04 02:29:02.328 BST [5729505e.4396:2] LOG:  connection authorized: user=pg_buildfarmer database=postgres
2016-05-04 02:29:02.333 BST [5729505e.4396:3] FATAL:  invalid value for parameter "session_authorization":
"pg_buildfarmer"
2016-05-04 02:29:03.662 BST [5729505f.57cd:1] LOG:  connection received: host=[local]
2016-05-04 02:29:03.731 BST [5729505f.57cd:2] LOG:  connection authorized: user=pg_buildfarmer database=postgres
2016-05-04 02:29:03.735 BST [5729505f.57cd:3] FATAL:  invalid value for parameter "session_authorization":
"pg_buildfarmer"

I tried to duplicate this failure just now, and could not.  Evidently,
the failures opossum shows above come from the "cannot set role to pg_xxx"
checks you had in check_session_authorization, which are gone as of commit
a89505fd2.  So in principle opossum should succeed if it were to try
again today with the same environment.

So this seems like another reason why removing those checks was an
improvement, but I'm left with a policy question: should initdb disallow
bootstrap superuser names like "pg_xxx"?  This doesn't seem quite
open-and-shut.  On the one hand, if we leave it as-is, then people might
be blindsided by future additions of built-in roles.  On the other,
if we forbid the case, it seems noticeably more likely that we'll break
existing setups, because "pg_something" doesn't seem like a terribly
unlikely choice for the name of the Postgres OS user.  (Certainly
opossum's owner would have to fix it, so that's one example out of a
not very large sample space of buildfarm users...)  Allowing a potential
conflict for the bootstrap superuser is a much narrower conflict risk
than any-old-user, so maybe it's okay to leave it as is.

Also, the failure mode if you do get an actual, rather than hypothetical,
conflict against a built-in role name isn't all that nice:

$ initdb -U pg_signal_backend
...
running bootstrap script ... FATAL:  could not create unique index "pg_authid_rolname_index"
DETAIL:  Key (rolname)=(pg_signal_backend) is duplicated.
...

While it's not hard to interpret this if you already know that
"pg_signal_backend" is a reserved role name, an explicit failure message
saying that the bootstrap superuser name can't begin with "pg_" would be
more user-friendly.  So that's a point in favor of having initdb reject
the case.

On the whole I lean to adding a restriction, but only weakly.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> So this seems like another reason why removing those checks was an
> improvement, but I'm left with a policy question: should initdb disallow
> bootstrap superuser names like "pg_xxx"?  This doesn't seem quite
> open-and-shut.  On the one hand, if we leave it as-is, then people might
> be blindsided by future additions of built-in roles.  On the other,
> if we forbid the case, it seems noticeably more likely that we'll break
> existing setups, because "pg_something" doesn't seem like a terribly
> unlikely choice for the name of the Postgres OS user.  (Certainly
> opossum's owner would have to fix it, so that's one example out of a
> not very large sample space of buildfarm users...)  Allowing a potential
> conflict for the bootstrap superuser is a much narrower conflict risk
> than any-old-user, so maybe it's okay to leave it as is.

On the whole, I'd vote to treat the bootstrap user as a normal role and
therefore have the same restriction in place for that user also.  As was
mentioned previously, it's already impossible to create schemas which
start with 'pg_', so you couldn't have a 'pg_buildfarmer' schema.  I
realize that, for the buildfarm, that's not an issue, but that's a bit
of a special case.

> Also, the failure mode if you do get an actual, rather than hypothetical,
> conflict against a built-in role name isn't all that nice:
>
> $ initdb -U pg_signal_backend
> ...
> running bootstrap script ... FATAL:  could not create unique index "pg_authid_rolname_index"
> DETAIL:  Key (rolname)=(pg_signal_backend) is duplicated.
> ...
>
> While it's not hard to interpret this if you already know that
> "pg_signal_backend" is a reserved role name, an explicit failure message
> saying that the bootstrap superuser name can't begin with "pg_" would be
> more user-friendly.  So that's a point in favor of having initdb reject
> the case.
>
> On the whole I lean to adding a restriction, but only weakly.

Agreed.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> ... but I'm left with a policy question: should initdb disallow
>> bootstrap superuser names like "pg_xxx"?

> On the whole, I'd vote to treat the bootstrap user as a normal role and
> therefore have the same restriction in place for that user also.

If we're going to enforce such a restriction, I think it would be
a good thing for it to be in place in beta1.
        regards, tom lane

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> ... but I'm left with a policy question: should initdb disallow
>> bootstrap superuser names like "pg_xxx"?

> On the whole, I'd vote to treat the bootstrap user as a normal role and
> therefore have the same restriction in place for that user also.

If we're going to enforce such a restriction, I think it would be
a good thing for it to be in place in beta1.

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> ... but I'm left with a policy question: should initdb disallow
> >> bootstrap superuser names like "pg_xxx"?
>
> > On the whole, I'd vote to treat the bootstrap user as a normal role and
> > therefore have the same restriction in place for that user also.
>
> If we're going to enforce such a restriction, I think it would be
> a good thing for it to be in place in beta1.

Makes sense.

Patch attached.  I'll push this in a bit, barring objections.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> If we're going to enforce such a restriction, I think it would be
>> a good thing for it to be in place in beta1.

> Makes sense.
> Patch attached.  I'll push this in a bit, barring objections.

Three minor wording quibbles:

* s/reserved/disallowed/ maybe?  Not 100% sold on this.

* s/can not/cannot/

* use double quotes not single around pg_
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> If we're going to enforce such a restriction, I think it would be
> >> a good thing for it to be in place in beta1.
>
> > Makes sense.
> > Patch attached.  I'll push this in a bit, barring objections.
>
> Three minor wording quibbles:
>
> * s/reserved/disallowed/ maybe?  Not 100% sold on this.
>
> * s/can not/cannot/
>
> * use double quotes not single around pg_

Blargh.  Missed this before pushing, sorry.

Will fix.

Thanks!

Stephen

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> If we're going to enforce such a restriction, I think it would be
> >> a good thing for it to be in place in beta1.
>
> > Makes sense.
> > Patch attached.  I'll push this in a bit, barring objections.
>
> Three minor wording quibbles:
>
> * s/reserved/disallowed/ maybe?  Not 100% sold on this.
>
> * s/can not/cannot/
>
> * use double quotes not single around pg_

Pushed those changes and also emailed the buildfarm owner directly
regarding the change.

Thanks!

Stephen