Re: pg_hba.conf: samehost and samenet [REVIEW]

(This is my review of the latest version of Stef Walter's samehost/net
patch, posted on 2009-09-17. See
http://archives.postgresql.org/message-id/4AB28043.3050109@memberwebs.com
for the original message.)

The patch applies and builds cleanly, and the samehost/samenet keywords
in pg_hba.conf work as advertised. I have no particular opinion on the
desirability of the feature, but I can see it would be useful in some
circumstances, as Stef described.

I think the patch is more or less ready, but I have a few minor
comments:

First, it needs to be reformatted to not use a space before the opening
parentheses in (some) function calls and definitions.

> *** a/doc/src/sgml/client-auth.sgml
> --- b/doc/src/sgml/client-auth.sgml
> [...]
>
> + <para>Instead of an <replaceable>CIDR-address</replaceable>, you can specify
> + the values <literal>samehost</literal> or <literal>samenet</literal>. To
> + match any address on the subnets connected to the local machine, specify
> + <literal>samenet</literal>. By specifying <literal>samehost</literal>, any
> + addresses present on the network interfaces of local machine will match.
> + </para>
> +

I'd suggest something like the following instead:

<para>Instead of a <replaceable>CIDR-address</replaceable>, you can
specify <literal>samehost</literal> to match any of the server's own
IP addresses, or <literal>samenet</literal> to match any address in
a subnet that the server belongs to.

> *** a/src/backend/libpq/hba.c
> --- b/src/backend/libpq/hba.c
> [...]
>
> + else if (addr->sa_family == AF_INET &&
> + raddr->addr.ss_family == AF_INET6)
> + {
> + /*
> + * Wrong address family. We allow only one case: if the file
> + * has IPv4 and the port is IPv6, promote the file address to
> + * IPv6 and try to match that way.
> + */

How about this instead:

If we're listening on IPv6 but the file specifies an IPv4 address to
match against, we promote the latter also to an IPv6 address before
trying to match the client's address.

(The comment is repeated elsewhere.)

> + int
> + pg_foreach_ifaddr(PgIfAddrCallback callback, void * cb_data)
> + {
> + #ifdef WIN32
> + return foreach_ifaddr_win32(callback, cb_data);
> + #else /* !WIN32 */
> + #ifdef HAVE_GETIFADDRS
> + return foreach_ifaddr_getifaddrs(callback, cb_data);
> + #else /* !HAVE_GETIFADDRS */
> + return foreach_ifaddr_ifconf(callback, cb_data);
> + #endif
> + #endif /* !WIN32 */
> + }

First, writing it this way is less noisy:

#ifdef WIN32
return foreach_ifaddr_win32(callback, cb_data);
#elif defined(HAVE_GETIFADDRS)
return foreach_ifaddr_getifaddrs(callback, cb_data);
#else
return foreach_ifaddr_ifconf(callback, cb_data);
#endif

Second, I can't see that it makes very much difference, but why do it
this way at all? You could just have each of the three #ifdef blocks
define a function named pg_foreach_ifaddr() and be done with it. No
need for a fourth function.

> *** a/src/backend/libpq/pg_hba.conf.sample
> --- b/src/backend/libpq/pg_hba.conf.sample
> [...]
>
> + # You can also specify "samehost" to limit connections to those from addresses
> + # of the local machine. Or you can specify "samenet" to limit connections
> + # to addresses on the subnets of the local network.

This should be reworded to match the documentation change suggested
above.

-- ams