Re: Parsing of pg_hba.conf and authentication inconsistencies

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.

Cheers,
BJ