Обсуждение: 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
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
> ...
>
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. 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. Changing it to a unlikely-to-conflict name doesn't help since that, I'd presume, is already being covered by some either sequence of statements.
If these tests can use pl/pgsql and DO-blocks the conditional execution of this one should be straight-forward to incorporate...
David J.
"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