Re: Parsing of pg_hba.conf and authentication inconsistencies - Mailing list 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@hagander.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