Thread: BUG #15006: "make check" error if current user is "user"

The following bug has been logged on the website:

Bug reference:      15006
Logged by:          Sergey Burladyan
Email address:      eshkinkot@gmail.com
PostgreSQL version: 10.1
Operating system:   Debian GNU/Linux 9 (stretch) in docker
Description:

I run tests in docker with current system user "user" and "make check" stop
with error:
...
     updatable_views          ... ok
     rolenames                ... FAILED
     roleattributes           ... ok
...

==== /home/user/postgresql-10.1/src/test/regress/regression.diffs ====

*** /home/user/postgresql-10.1/src/test/regress/expected/rolenames.out  Tue
Nov  7 00:46:52 2017
--- /home/user/postgresql-10.1/src/test/regress/results/rolenames.out   Thu
Jan 11 12:51:19 2018
***************
*** 42,47 ****
--- 42,48 ----
  CREATE ROLE "current_user";
  CREATE ROLE "session_user";
  CREATE ROLE "user";
+ ERROR:  role "user" already exists
  CREATE ROLE current_user; -- error
  ERROR:  CURRENT_USER cannot be used as a role name here
  LINE 1: CREATE ROLE current_user;
***************
*** 950,952 ****
--- 951,954 ----
  DROP OWNED BY regress_testrol0, "Public", "current_user",
regress_testrol1, regress_testrol2, regress_testrolx CASCADE;
  DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2,
regress_testrolx;
  DROP ROLE "Public", "None", "current_user", "session_user", "user";
+ ERROR:  current user cannot be dropped

======================================================================

On 01/11/2018 01:53 PM, PG Bug reporting form wrote:
> The following bug has been logged on the website:
> 
> Bug reference:      15006
> Logged by:          Sergey Burladyan
> Email address:      eshkinkot@gmail.com
> PostgreSQL version: 10.1
> Operating system:   Debian GNU/Linux 9 (stretch) in docker
> Description:        
> 
> I run tests in docker with current system user "user" and "make check" stop
> with error:
> ...
>      updatable_views          ... ok
>      rolenames                ... FAILED
>      roleattributes           ... ok
> ...
> 

Hmmm ... I'm having this issue too (my distribution uses "user"
internally for the default user). Over time I managed to convince myself
it's not really a PostgreSQL bug, but rather a strange choice of OS user
name. And I've been simply ignoring this particular regression failure
on my laptop.

But maybe we could/should fix it anyway? Most regression tests switched
to roles prefixed with regress_* so why not to do the same here?

Of course, a particularly strange distribution could still pick a
conflicting OS name, but that seems rather unlikely, I guess.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Wed, Jan 17, 2018 at 2:58 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com>
> wrote:
>> But maybe we could/should fix it anyway? Most regression tests switched
>> to roles prefixed with regress_* so why not to do the same here?

> The point of the test seems to be to ensure that the special system
> keywords, when quoted, are allowed to be used for role names.

Exactly.  Changing the names ruins the point of the test.

> So the
> choice is to make the test conditional (if the role previously exists
> neither create or drop it - and since it existed it doesn't seem like its a
> problem to create it anyway) or to simply not bother testing "user"
> figuring that the other two roles suffice for testing this behavior.

I wouldn't have a big problem with just dropping this whole test stanza.
It's an out-and-out violation of our rule against not creating rolenames
not starting with "regress_", and it's not testing anything that seems
especially likely to break.

            regards, tom lane

On Wed, Jan 17, 2018 at 07:21:03PM -0500, Tom Lane wrote:
> I wouldn't have a big problem with just dropping this whole test stanza.
> It's an out-and-out violation of our rule against not creating rolenames
> not starting with "regress_", and it's not testing anything that seems
> especially likely to break.

Perhaps a stupid question. What's the point behind the logic to forbid a
double-quoted "public" but to authorize a double-quoted "user"? The
whole thing looks inconsistent to me.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> Perhaps a stupid question. What's the point behind the logic to forbid a
> double-quoted "public" but to authorize a double-quoted "user"? The
> whole thing looks inconsistent to me.

Good question.  There may be some backwards-compatibility considerations
here, but still this is just plain inconsistent:

regression=# create user public;
ERROR:  role name "public" is reserved
LINE 1: create user public;
                    ^
regression=# create user "public";
ERROR:  role name "public" is reserved
LINE 1: create user "public";
                    ^
regression=# create user user;  
ERROR:  syntax error at or near "user"
LINE 1: create user user;
                    ^
regression=# create user "user";
CREATE ROLE
regression=# create user current_user;
ERROR:  CURRENT_USER cannot be used as a role name here
LINE 1: create user current_user;
                    ^
regression=# create user "current_user";
CREATE ROLE

I can see the point of disallowing a user named "public", because
otherwise syntax like GRANT some-privilege TO PUBLIC is just a trap
for the unwary DBA, one that could have bad security consequences.
But it's not clear to me why the same logic doesn't apply to "user",
"current_user", or "session_user", all of which are equally conflatable
with a built-in meaning in some security-relevant contexts.

BTW, you might think that those wildly different phrasings of essentially
the same error come from different places in the code, but no, they are
from adjacent lines in gram.y.  WTF?  This seems to be deliberately
anti-consistent.

And, to return to the original scenario, if you do

initdb -U public

it goes through just fine ... so our defenses against having such a
username have a hole in them.

Probably the OP would not be very happy if the outcome of this discussion
is that "initdb -U user" fails, but I am not seeing a principled reason
why that should be allowed.

            regards, tom lane

On Wed, Jan 17, 2018 at 09:15:06PM -0500, Tom Lane wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> Perhaps a stupid question. What's the point behind the logic to forbid a
>> double-quoted "public" but to authorize a double-quoted "user"? The
>> whole thing looks inconsistent to me.
>
> Good question.  There may be some backwards-compatibility considerations
> here, but still this is just plain inconsistent:
>
> <snip>
>
> I can see the point of disallowing a user named "public", because
> otherwise syntax like GRANT some-privilege TO PUBLIC is just a trap
> for the unwary DBA, one that could have bad security consequences.
> But it's not clear to me why the same logic doesn't apply to "user",
> "current_user", or "session_user", all of which are equally conflatable
> with a built-in meaning in some security-relevant contexts.

Just forgot to mention that double-quoted user names with upper-case
characters are similarly allowed should still be allowed, like:
=# CREATE ROLE "Public";
CREATE ROLE
=# CREATE ROLE "pG_as";
CREATE ROLE
So those are correctly handled now.

Worth noting also this bit (from IsReservedName), which looks correct to
me:
=# CREATE ROLE "pg_aB";
ERROR:  42939: role name "pg_aB" is reserved
DETAIL:  Role names starting with "pg_" are reserved.

> BTW, you might think that those wildly different phrasings of essentially
> the same error come from different places in the code, but no, they are
> from adjacent lines in gram.y.  WTF?  This seems to be deliberately
> anti-consistent.

Same reaction here :)
I would have expected all the checks to be in user.c and at parsing
level.

> Probably the OP would not be very happy if the outcome of this discussion
> is that "initdb -U user" fails, but I am not seeing a principled reason
> why that should be allowed.

Me neither.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Wed, Jan 17, 2018 at 09:15:06PM -0500, Tom Lane wrote:
>> Probably the OP would not be very happy if the outcome of this discussion
>> is that "initdb -U user" fails, but I am not seeing a principled reason
>> why that should be allowed.

> Me neither.

Alternatively, we could consider removing all these artificial
restrictions on what a username can be, and just expecting the DBA
to know what he's doing.  But what we've got right now is weirdly
inconsistent.

            regards, tom lane

On 01/17/2018 06:51 PM, Tom Lane wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Wed, Jan 17, 2018 at 09:15:06PM -0500, Tom Lane wrote:
>>> Probably the OP would not be very happy if the outcome of this discussion
>>> is that "initdb -U user" fails, but I am not seeing a principled reason
>>> why that should be allowed.
>
>> Me neither.
>
> Alternatively, we could consider removing all these artificial
> restrictions on what a username can be, and just expecting the DBA
> to know what he's doing.  But what we've got right now is weirdly
> inconsistent.

+1

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Tom Lane wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
> > On Wed, Jan 17, 2018 at 09:15:06PM -0500, Tom Lane wrote:
> >> Probably the OP would not be very happy if the outcome of this discussion
> >> is that "initdb -U user" fails, but I am not seeing a principled reason
> >> why that should be allowed.
> 
> > Me neither.
> 
> Alternatively, we could consider removing all these artificial
> restrictions on what a username can be, and just expecting the DBA
> to know what he's doing.  But what we've got right now is weirdly
> inconsistent.

Have at it.

This all comes from
https://www.postgresql.org/message-id/20141010.172740.88258644.horiguchi.kyotaro@lab.ntt.co.jp
which ended up as commit 31eae6028eca4365e7165f5f33fee1ed0486aee0 (with
a couple of fixups afterwards).

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:

> Of course, a particularly strange distribution could still pick a
> conflicting OS name, but that seems rather unlikely, I guess.

Why the tests depend and use current OS user name? I think it's not very
good (if you does not test exactly that, of course).

If you enforce user name for temporary initdb cluster and use this
name for connection in tests, then you does not depends on OS name at
all, something like this (see attachment).

With this patch "make check-world" run successfully with OS name "user"
and "current_user".

-- 
Sergey Burladyan