Re: pg_hba.conf: samehost and samenet [REVIEW] - Mailing list pgsql-hackers

(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
anyof the server's own   IP addresses, or <literal>samenet</literal> to match any address in   a subnet that the server
belongsto.
 

> *** 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
anIPv6 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