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

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

From: Abhijit Menon-Sen <ams(at)toroid(dot)org>
To: pgsql-hackers(at)postgresql(dot)org
Cc: stef(at)memberwebs(dot)com
Subject: Re: pg_hba.conf: samehost and samenet [REVIEW]
Date: 2009-09-20 03:59:29
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
(This is my review of the latest version of Stef Walter's samehost/net
patch, posted on 2009-09-17. See
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

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 */
> + 	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);
        return foreach_ifaddr_ifconf(callback, cb_data);

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

-- ams

In response to


pgsql-hackers by date

Next:From: Andrew GierthDate: 2009-09-20 08:03:18
Subject: Re: updated hstore patch
Previous:From: Robert HaasDate: 2009-09-20 03:22:37
Subject: Re: Anonymous code blocks

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