Re: More robust pg_hba.conf parsing/error logging

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello

The origin of this petition is an error produced today by a user on one
of our systems. Because of this error many users lost access to their
databases.

Problem:
- --------
If you define in pg_hba.conf a database or a user value with 'ALL'
instead of 'all', you will lose access to *all* databases involved. The
reload process will not report anything about 'ALL' been an invalid
value and the new pg_hba.conf will be reloaded.

This is the only thing in the log file:
"LOG: received SIGHUP, reloading configuration files"

Solution:
- ---------
Or change internally all uppercase to lowercase so users can define
values in pg_hba.conf with uppercase characters.

Or throw an error saying 'ALL' is not a valid value and *not* reload the
pg_hba.conf file. This is already done if you use uppercase when you
define connection type or authentication method.

regards,
- --
Rafael Martinez, <r(dot)m(dot)guerrero(at)usit(dot)uio(dot)no>
Center for Information Technology Services
University of Oslo, Norway

PGP Public Key: http://folk.uio.no/rafael/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.7 (GNU/Linux)

iD8DBQFKp7GVBhuKQurGihQRAhCZAJ9y5BhdWbrpJeW12g/rJ6yRfgubgACglYC3
wkG1cHESexmSZ48/Fc63vU4=
=a46y
-----END PGP SIGNATURE-----

Rafael Martinez wrote:
>
> Or throw an error saying 'ALL' is not a valid value and *not* reload the
> pg_hba.conf file.
>

But it's not invalid. It would designate a database or user named "ALL".
That might be a silly thing to do, but that's another question.

cheers

andrew

Rafael Martinez wrote:

> Problem:
> - --------
> If you define in pg_hba.conf a database or a user value with 'ALL'
> instead of 'all', you will lose access to *all* databases involved. The
> reload process will not report anything about 'ALL' been an invalid
> value and the new pg_hba.conf will be reloaded.
>
> This is the only thing in the log file:
> "LOG: received SIGHUP, reloading configuration files"

Aye, that's surprising. I think the correct fix here is to change the
strcmp comparisons to pg_strcasecmp() in several places in hba.c.

(BTW the business about appending newlines to special tokens in
next_token() seems ugly and underdocumented.)

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Andrew Dunstan wrote:

> Rafael Martinez wrote:
> >
> >Or throw an error saying 'ALL' is not a valid value and *not* reload the
> >pg_hba.conf file.
>
> But it's not invalid. It would designate a database or user named
> "ALL". That might be a silly thing to do, but that's another
> question.

Surely if you want to designate a database named ALL you should use
quotes, same as if you wanted to designate a database named all (see my
other followup).

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Andrew Dunstan wrote:
>
>
>> Rafael Martinez wrote:
>>
>>> Or throw an error saying 'ALL' is not a valid value and *not* reload the
>>> pg_hba.conf file.
>>>
>> But it's not invalid. It would designate a database or user named
>> "ALL". That might be a silly thing to do, but that's another
>> question.
>>
>
> Surely if you want to designate a database named ALL you should use
> quotes, same as if you wanted to designate a database named all (see my
> other followup).
>
>

OK, but if we move to using pg_strcasecmp() that would be a behaviour
change, so I think we couldn't do it before 8.5, in case someone is
relying on it. It will affect any dbname or username in mixed or upper
case, not just ALL, won't it?

cheers

andrew

Andrew Dunstan wrote:
>
>
> Rafael Martinez wrote:
>>
>> Or throw an error saying 'ALL' is not a valid value and *not* reload the
>> pg_hba.conf file.
>
>
> But it's not invalid. It would designate a database or user named "ALL".
> That might be a silly thing to do, but that's another question.
>

Shouldn't 'all' be a reserved word?, it has a special meaning when used
in pg_hba.conf.

regards
--
Rafael Martinez, <r(dot)m(dot)guerrero(at)usit(dot)uio(dot)no>
Center for Information Technology Services
University of Oslo, Norway

PGP Public Key: http://folk.uio.no/rafael/

Andrew Dunstan wrote:

> Alvaro Herrera wrote:

> >Surely if you want to designate a database named ALL you should use
> >quotes, same as if you wanted to designate a database named all (see my
> >other followup).
>
> OK, but if we move to using pg_strcasecmp() that would be a
> behaviour change, so I think we couldn't do it before 8.5, in case
> someone is relying on it.

Yeah, I think so. It doesn't seem like this is backpatchable (I lean
towards doubting that anyone is using a database named ALL, but still).

> It will affect any dbname or username in mixed or upper case, not just
> ALL, won't it?

No, I am suggesting to change only the comparisons to the literals
"all", "sameuser", "samegroup" and "samerole".

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Rafael Martinez wrote:

> Shouldn't 'all' be a reserved word?, it has a special meaning when used
> in pg_hba.conf.

No, it works fine with a line like this:

local "all" all md5

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Rafael Martinez wrote:
>
>> Shouldn't 'all' be a reserved word?, it has a special meaning when used
>> in pg_hba.conf.
>
> No, it works fine with a line like this:
>
> local "all" all md5
>

Ok, then "all" and "ALL" should be valid values but not all and ALL
(without "")

regards
--
Rafael Martinez, <r(dot)m(dot)guerrero(at)usit(dot)uio(dot)no>
Center for Information Technology Services
University of Oslo, Norway

PGP Public Key: http://folk.uio.no/rafael/

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Andrew Dunstan wrote:
>> It will affect any dbname or username in mixed or upper case, not just
>> ALL, won't it?

> No, I am suggesting to change only the comparisons to the literals
> "all", "sameuser", "samegroup" and "samerole".

Hmm. These words are effectively keywords, so +1 for treating them
case-insensitively, as we do in SQL. But I wonder whether there isn't
an argument for making the comparisons of role and database names
behave more like SQL, too --- that is FOO matches foo but not "FOO".

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Andrew Dunstan wrote:
> >> It will affect any dbname or username in mixed or upper case, not just
> >> ALL, won't it?
>
> > No, I am suggesting to change only the comparisons to the literals
> > "all", "sameuser", "samegroup" and "samerole".
>
> Hmm. These words are effectively keywords, so +1 for treating them
> case-insensitively, as we do in SQL. But I wonder whether there isn't
> an argument for making the comparisons of role and database names
> behave more like SQL, too --- that is FOO matches foo but not "FOO".

In general, I think that sounds like a good idea. At the same time, I
wouldn't be against changing the specific 'ALL' special-case comparison
in 8.4.2, using the argument that not many people have moved to it yet
and it's pretty far out there for an 'ALL' database to exist anyway..

Might be too much for a point-release. :/

Just my 2c.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> In general, I think that sounds like a good idea. At the same time, I
> wouldn't be against changing the specific 'ALL' special-case comparison
> in 8.4.2, using the argument that not many people have moved to it yet
> and it's pretty far out there for an 'ALL' database to exist anyway..

I don't think this is back-patch material. We've had what, one
complaint in twelve years? The odds of causing a problem seem
higher than the odds of preventing one.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Andrew Dunstan wrote:
> >> It will affect any dbname or username in mixed or upper case, not just
> >> ALL, won't it?
>
> > No, I am suggesting to change only the comparisons to the literals
> > "all", "sameuser", "samegroup" and "samerole".

What happened to this idea?

> Hmm. These words are effectively keywords, so +1 for treating them
> case-insensitively, as we do in SQL. But I wonder whether there isn't
> an argument for making the comparisons of role and database names
> behave more like SQL, too --- that is FOO matches foo but not "FOO".

And this one?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
+ If your life is a hard drive, Christ can be your backup. +

On Tue, Feb 23, 2010 at 12:22 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Tom Lane wrote:
>> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> > Andrew Dunstan wrote:
>> >> It will affect any dbname or username in mixed or upper case, not just
>> >> ALL, won't it?
>>
>> > No, I am suggesting to change only the comparisons to the literals
>> > "all", "sameuser", "samegroup" and "samerole".
>
> What happened to this idea?
>
>> Hmm.  These words are effectively keywords, so +1 for treating them
>> case-insensitively, as we do in SQL.  But I wonder whether there isn't
>> an argument for making the comparisons of role and database names
>> behave more like SQL, too --- that is FOO matches foo but not "FOO".
>
> And this one?

Nobody's implemented them? I'd suggest adding these to the TODO.

...Robert

Robert Haas wrote:
> On Tue, Feb 23, 2010 at 12:22 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Tom Lane wrote:
> >> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> >> > Andrew Dunstan wrote:
> >> >> It will affect any dbname or username in mixed or upper case, not just
> >> >> ALL, won't it?
> >>
> >> > No, I am suggesting to change only the comparisons to the literals
> >> > "all", "sameuser", "samegroup" and "samerole".
> >
> > What happened to this idea?
> >
> >> Hmm. ?These words are effectively keywords, so +1 for treating them
> >> case-insensitively, as we do in SQL. ?But I wonder whether there isn't
> >> an argument for making the comparisons of role and database names
> >> behave more like SQL, too --- that is FOO matches foo but not "FOO".
> >
> > And this one?
>
> Nobody's implemented them? I'd suggest adding these to the TODO.

Added:

|Process pg_hba.conf keywords as case-insensitive

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
+ If your life is a hard drive, Christ can be your backup. +