Skip site navigation (1) Skip section navigation (2)

Re: Parsing of pg_hba.conf and authentication inconsistencies

From: "Brendan Jurd" <direvus(at)gmail(dot)com>
To: "Magnus Hagander" <magnus(at)hagander(dot)net>
Cc: "Bruce Momjian" <bruce(at)momjian(dot)us>, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "Josh Berkus" <josh(at)agliodbs(dot)com>, "Gregory Stark" <stark(at)enterprisedb(dot)com>, "PG Hackers" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Parsing of pg_hba.conf and authentication inconsistencies
Date: 2008-09-11 20:53:55
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
Hi Magnus,

Josh has assigned your patch to me for an initial review.

First up I'd like to say that this is a really nice upgrade.
Shielding a running server from reloading a bogus conf file makes a
whole lot of sense.

On Sun, Aug 17, 2008 at 1:47 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Attached is the patch I have so far. The only "extra" it adds over today
> is that it allows the use of "ident" authentication without explicitly
> specifying "sameuser" when you want that.

Nice.  I'd expect that custom ident maps are pretty rare, so this
seems like a good move.

> Other than that, it moves code around to do the parsing in the
> postmaster and the maching in the backend. This means that now if there
> is a syntax error in the file on a reload, we just keep the old file
> around still letting people log into the database. If there is a syntax
> error on server startup, it's FATAL of course, since we can't run
> without any kind of pg_hba.
> It also changes a couple of error cases to explicitly state that support
> for a certain auth method isn't compiled in, rather than just call it a
> syntax error.

The patch applied cleanly to HEAD, compiled fine on amd64 gentoo and
the features appeared to work as advertised.  I tried putting various
kinds of junk into my pg_hba.conf file and reloading/restarting the
postmaster to see how it would react.  As expected, on a reload I got
a parse error and a WARNING letting me know that the new configuration
was not loaded.  On a restart, I got the same parse error, and a FATAL
indicating that the postmaster couldn't start.

I also checked that it was safe to omit "sameuser" in an "ident"
record, and again that worked as expected.

The patch doesn't include any updates to documentation.  I humbly
suggest that the change to the ident map (making "sameuser" the
default) should be mentioned both in the Manual itself, and in the
sample conf file comments.  Here are a few sites that may be in want
of an update:

 * src/backend/libpq/pg_ident.conf.sample:33 - "Instead, use the
special map name "sameuser" in pg_hba.conf"

 * doc/src/sgml/client-auth.sgml:512 has a sample "ident sameuser" record

 * doc/src/sgml/client-auth.sgml:931 - "There is a predefined ident
map <literal>sameuser</literal>"

The section on Ident Authentication might need some broader rewording
to indicate that the map argument is now optional.

The new error messages are good, but I wonder if they could be
improved by using DETAIL segments.  The guidelines on error message
style say that the primary message should be terse and make a
reasonable effort to fit on one line.  For example, this:

    LOG:  invalid IP address "a.b.c.d" in file
"/home/direvus/src/postgres/inst/data/pg_hba.conf" line 77: Name or
service not known

Could be rewritten as something like this:

    LOG:  invalid IP address "a.b.c.d" in auth config: Name or service not known
    DETAIL: In file "/home/direvus/src/postgres/inst/data/pg_hba.conf", line 77

That's all the feedback I have for the moment.  I hope you find my
comments helpful.


In response to


pgsql-hackers by date

Next:From: Tom LaneDate: 2008-09-11 21:06:02
Subject: Re: Commitfest patches mostly assigned ... status
Previous:From: Tom LaneDate: 2008-09-11 20:33:26
Subject: Re: Copy column storage parameters on CREATE TABLE LIKE/INHERITS

Privacy Policy | About PostgreSQL
Copyright © 1996-2017 The PostgreSQL Global Development Group