IPV4 addresses on IPV6 machines in pg_hba.conf

On Wed, Sep 03, 2003 at 07:19:16PM +0200, Andreas Pflug wrote:

This was discussed in [HACKERS] TCP/IP with 7.4 beta2 broken?

I created a patch to hba.c which uses IPV4 entries as IPV6 entries if
running on a IPV6 system (which is detected from a port coming in as
AF_INET6).

You're assuming all systems have an AF_INET6 constant, which is
not the case. Please make use of HAVE_IPV6.

Can't directly see anything else wrong with it.

Kurt

Kurt Roeckx wrote:

You're assuming all systems have an AF_INET6 constant, which is
not the case. Please make use of HAVE_IPV6.

Can't directly see anything else wrong with it.

Here's the patch with HAVE_IPV6 conditional compiling.

Regards,
Andreas

Andreas,

You should check that the CIDR mask is a valid integer. You would need
to use strtol() rather than atoi() to do that. Perhaps this should be
hoisted out of ip.c:SockAddr_cidr_mask() and put in hba.c.

Sorry, I should have checked this carefully earlier.

andrew

Andreas Pflug wrote:

Andrew Dunstan wrote:

Andreas,

You should check that the CIDR mask is a valid integer. You would need
to use strtol() rather than atoi() to do that. Perhaps this should be
hoisted out of ip.c:SockAddr_cidr_mask() and put in hba.c.

Right, I added this.

Regards,
Andreas

Andreas Pflug <pgadmin@pse-consulting.de> writes:

Andrew Dunstan wrote:

You should check that the CIDR mask is a valid integer. You would need
to use strtol() rather than atoi() to do that. Perhaps this should be
hoisted out of ip.c:SockAddr_cidr_mask() and put in hba.c.

Right, I added this.

I thought this was still really messy, so I modified it to use a
separate "promote v4 address to v6" subroutine. I've applied the
attached patch (plus docs). It's not very well tested since I don't
have an IPv6 setup here; please check that it does what you want.

regards, tom lane

*** src/backend/libpq/hba.c.orig	Fri Sep  5 10:35:54 2003
--- src/backend/libpq/hba.c	Fri Sep  5 16:24:41 2003
***************
*** 673,685 ****
  		if (cidr_slash)
  			*cidr_slash = '/';

- 		if (file_ip_addr->ai_family != port->raddr.addr.ss_family)
- 		{
- 			/* Wrong address family. */
- 			freeaddrinfo_all(hints.ai_family, file_ip_addr);
- 			return;
- 		}
- 
  		/* Get the netmask */
  		if (cidr_slash)
  		{
--- 673,678 ----
***************
*** 703,708 ****
--- 696,723 ----

  			if (file_ip_addr->ai_family != mask->ss_family)
  				goto hba_syntax;
+ 		}
+ 
+ 		if (file_ip_addr->ai_family != port->raddr.addr.ss_family)
+ 		{
+ 			/*
+ 			 * 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.
+ 			 */
+ #ifdef HAVE_IPV6
+ 			if (file_ip_addr->ai_family == AF_INET &&
+ 				port->raddr.addr.ss_family == AF_INET6)
+ 			{
+ 				promote_v4_to_v6_addr((struct sockaddr_storage *) file_ip_addr->ai_addr);
+ 				promote_v4_to_v6_mask(mask);
+ 			}
+ 			else
+ #endif /* HAVE_IPV6 */
+ 			{
+ 				freeaddrinfo_all(hints.ai_family, file_ip_addr);
+ 				return;
+ 			}
  		}

  		/* Read the rest of the line. */
*** src/backend/libpq/ip.c.orig	Sun Aug  3 23:00:36 2003
--- src/backend/libpq/ip.c	Fri Sep  5 16:24:42 2003
***************
*** 34,40 ****
  #endif
  #include <arpa/inet.h>
  #include <sys/file.h>
! #endif

#include "libpq/ip.h"

--- 34,41 ----
  #endif
  #include <arpa/inet.h>
  #include <sys/file.h>
! 
! #endif /* !defined(_MSC_VER) && !defined(__BORLANDC__) */

#include "libpq/ip.h"

***************
*** 265,273 ****
--- 266,281 ----

return 0;
}
+
#endif /* HAVE_UNIX_SOCKETS */

+ /*
+  * rangeSockAddr - is addr within the subnet specified by netaddr/netmask ?
+  *
+  * Note: caller must already have verified that all three addresses are
+  * in the same address family; and AF_UNIX addresses are not supported.
+  */
  int
  rangeSockAddr(const struct sockaddr_storage * addr,
  			  const struct sockaddr_storage * netaddr,
***************
*** 287,292 ****
--- 295,333 ----
  		return 0;
  }

+ static int
+ rangeSockAddrAF_INET(const struct sockaddr_in * addr,
+ 					 const struct sockaddr_in * netaddr,
+ 					 const struct sockaddr_in * netmask)
+ {
+ 	if (((addr->sin_addr.s_addr ^ netaddr->sin_addr.s_addr) &
+ 		 netmask->sin_addr.s_addr) == 0)
+ 		return 1;
+ 	else
+ 		return 0;
+ }
+ 
+ 
+ #ifdef HAVE_IPV6
+ static int
+ rangeSockAddrAF_INET6(const struct sockaddr_in6 * addr,
+ 					  const struct sockaddr_in6 * netaddr,
+ 					  const struct sockaddr_in6 * netmask)
+ {
+ 	int			i;
+ 
+ 	for (i = 0; i < 16; i++)
+ 	{
+ 		if (((addr->sin6_addr.s6_addr[i] ^ netaddr->sin6_addr.s6_addr[i]) &
+ 			 netmask->sin6_addr.s6_addr[i]) != 0)
+ 			return 0;
+ 	}
+ 
+ 	return 1;
+ }
+ 
+ #endif
+ 
  /*
   *	SockAddr_cidr_mask - make a network mask of the appropriate family
   *	  and required number of significant bits
***************
*** 358,391 ****
  	return 0;
  }

! static int
! rangeSockAddrAF_INET(const struct sockaddr_in * addr, const struct sockaddr_in * netaddr,
! const struct sockaddr_in * netmask)
{
! if (((addr->sin_addr.s_addr ^ netaddr->sin_addr.s_addr) &
! netmask->sin_addr.s_addr) == 0)
! return 1;
! else
! return 0;
! }

! #ifdef HAVE_IPV6
! static int
! rangeSockAddrAF_INET6(const struct sockaddr_in6 * addr,
! const struct sockaddr_in6 * netaddr,
! const struct sockaddr_in6 * netmask)
{
int i;

! for (i = 0; i < 16; i++)
! {
! if (((addr->sin6_addr.s6_addr[i] ^ netaddr->sin6_addr.s6_addr[i]) &
! netmask->sin6_addr.s6_addr[i]) != 0)
! return 0;
! }

! return 1;
}

! #endif
--- 399,472 ----
  	return 0;
  }

!
! #ifdef HAVE_IPV6
!
! /*
! * promote_v4_to_v6_addr --- convert an AF_INET addr to AF_INET6, using
! * the standard convention for IPv4 addresses mapped into IPv6 world
! *
! * The passed addr is modified in place. Note that we only worry about
! * setting the fields that rangeSockAddr will look at.
! */
! void
! promote_v4_to_v6_addr(struct sockaddr_storage * addr)
{
! struct sockaddr_in addr4;
! struct sockaddr_in6 addr6;
! uint32 s_addr;

+ 	memcpy(&addr4, addr, sizeof(addr4));
+ 	s_addr = ntohl(addr4.sin_addr.s_addr);

! memset(&addr6, 0, sizeof(addr6));
!
! addr6.sin6_family = AF_INET6;
!
! addr6.sin6_addr.s6_addr[10] = 0xff;
! addr6.sin6_addr.s6_addr[11] = 0xff;
! addr6.sin6_addr.s6_addr[12] = (s_addr >> 24) & 0xFF;
! addr6.sin6_addr.s6_addr[13] = (s_addr >> 16) & 0xFF;
! addr6.sin6_addr.s6_addr[14] = (s_addr >> 8) & 0xFF;
! addr6.sin6_addr.s6_addr[15] = (s_addr) & 0xFF;
!
! memcpy(addr, &addr6, sizeof(addr6));
! }
!
! /*
! * promote_v4_to_v6_mask --- convert an AF_INET netmask to AF_INET6, using
! * the standard convention for IPv4 addresses mapped into IPv6 world
! *
! * This must be different from promote_v4_to_v6_addr because we want to
! * set the high-order bits to 1's not 0's.
! *
! * The passed addr is modified in place. Note that we only worry about
! * setting the fields that rangeSockAddr will look at.
! */
! void
! promote_v4_to_v6_mask(struct sockaddr_storage * addr)
{
+ struct sockaddr_in addr4;
+ struct sockaddr_in6 addr6;
+ uint32 s_addr;
int i;

! memcpy(&addr4, addr, sizeof(addr4));
! s_addr = ntohl(addr4.sin_addr.s_addr);

! memset(&addr6, 0, sizeof(addr6));
!
! addr6.sin6_family = AF_INET6;
!
! for (i = 0; i < 12; i++)
! addr6.sin6_addr.s6_addr[i] = 0xff;
!
! addr6.sin6_addr.s6_addr[12] = (s_addr >> 24) & 0xFF;
! addr6.sin6_addr.s6_addr[13] = (s_addr >> 16) & 0xFF;
! addr6.sin6_addr.s6_addr[14] = (s_addr >> 8) & 0xFF;
! addr6.sin6_addr.s6_addr[15] = (s_addr) & 0xFF;
!
! memcpy(addr, &addr6, sizeof(addr6));
}

! #endif /* HAVE_IPV6 */
*** src/include/libpq/ip.h.orig	Sun Aug  3 23:01:33 2003
--- src/include/libpq/ip.h	Fri Sep  5 16:24:36 2003
***************
*** 33,38 ****
--- 33,43 ----
  extern int SockAddr_cidr_mask(struct sockaddr_storage ** mask,
  				   char *numbits, int family);

+ #ifdef HAVE_IPV6
+ extern void promote_v4_to_v6_addr(struct sockaddr_storage * addr);
+ extern void promote_v4_to_v6_mask(struct sockaddr_storage * addr);
+ #endif
+ 
  #ifdef	HAVE_UNIX_SOCKETS
  #define IS_AF_UNIX(fam) ((fam) == AF_UNIX)
  #else

Tom Lane wrote:

I thought this was still really messy, so I modified it to use a
separate "promote v4 address to v6" subroutine. I've applied the
attached patch (plus docs). It's not very well tested since I don't
have an IPv6 setup here; please check that it does what you want.

It SEGVs. Reason is that the memcpy of the promote_v4_to_v6_XXX
functions assumes that the sockaddr_storage is large enough to hold an
IPV6 address, which appears to be not true. Since the struct isn't
created by a plain malloc() and returned by free(), but assembled by
getaddrinfo() according to the family's requirement, I don't see a way
how to fix this. IMHO the struct needs to be created officially by
getaddrinfo(), which will lead more or less the the same solution I
posted previously.

Regards,
Andreas

Andreas Pflug <pgadmin@pse-consulting.de> writes:

Tom Lane wrote:

I thought this was still really messy, so I modified it to use a
separate "promote v4 address to v6" subroutine. I've applied the
attached patch (plus docs). It's not very well tested since I don't
have an IPv6 setup here; please check that it does what you want.

It SEGVs. Reason is that the memcpy of the promote_v4_to_v6_XXX
functions assumes that the sockaddr_storage is large enough to hold an
IPV6 address, which appears to be not true.

Drat. That didn't happen when I forced it through that path here, but
I guess it could if getaddrinfo is trying to be conservative about the
amount of memory in its return value. I'll rework it.

IMHO the struct needs to be created officially by
getaddrinfo(), which will lead more or less the the same solution I
posted previously.

No, we just need to use datastructures that are under our control for
the values that will get passed to rangeSockAddr. A few more lines...

regards, tom lane

Tom Lane wrote:

IMHO the struct needs to be created officially by
getaddrinfo(), which will lead more or less the the same solution I
posted previously.

No, we just need to use datastructures that are under our control for
the values that will get passed to rangeSockAddr. A few more lines...

Hm,

are you sure it's not just for beauty's sake? While talking about
beauty: that setting of *cidr_slash to '/' and 0 doesn't look too
esthetic...

Regards,
Andreas

Andreas Pflug <pgadmin@pse-consulting.de> writes:

are you sure it's not just for beauty's sake?

What I didn't like about your last patch was the close coupling of the
CIDR/netmask processing to the v4-to-v6 conversion; as Andrew pointed
out, you were hacking into hba.c functionality that overlapped with
SockAddr_cidr_mask. Doing the conversion after we've collected the
netmask seems a lot cleaner to me. Also, this way keeps a fairly decent
separation of interests between hba.c (parsing the hba.conf syntax) and
ip.c (messing with address representations).

While talking about beauty: that setting of *cidr_slash to '/' and 0
doesn't look too esthetic...

It is ugly (and I didn't write it ;-)). But if we palloc'd a modified
version of the token we'd have to remember to pfree it, so it nets out
to about the same amount of code either way I think. If you wanna try
to clean it up more, be my guest ...

regards, tom lane

Tom Lane said:

Andreas Pflug <pgadmin@pse-consulting.de> writes:

are you sure it's not just for beauty's sake?

What I didn't like about your last patch was the close coupling of the

CIDR/netmask processing to the v4-to-v6 conversion; as Andrew pointed
out, you were hacking into hba.c functionality that overlapped with
SockAddr_cidr_mask. Doing the conversion after we've collected the
netmask seems a lot cleaner to me. Also, this way keeps a fairly decent
separation of interests between hba.c (parsing the hba.conf syntax) and
ip.c (messing with address representations).

While talking about beauty: that setting of *cidr_slash to '/' and 0

doesn't look too esthetic...

It is ugly (and I didn't write it ;-)). But if we palloc'd a modified

version of the token we'd have to remember to pfree it, so it nets out to
about the same amount of code either way I think. If you wanna try to
clean it up more, be my guest ...

I wrote it :-) The reason is that alone of the tokens in this file
address/mask is a composite. I agree it is a bit ugly. In fact, this
whole function is ugly and getting uglier and needs recasting. I intend
to have a go at that, since I am partly responsible, but not in the
present cycle.

Nobody objected when the original patch from Kurt (including my bit) was
submitted, though, so it's a bit late to complain now about aesthetics.

cheers

andrew

Tom Lane wrote:

Andreas Pflug <pgadmin@pse-consulting.de> writes:

are you sure it's not just for beauty's sake?

What I didn't like about your last patch was the close coupling of the
CIDR/netmask processing to the v4-to-v6 conversion; as Andrew pointed
out, you were hacking into hba.c functionality that overlapped with
SockAddr_cidr_mask. Doing the conversion after we've collected the
netmask seems a lot cleaner to me. Also, this way keeps a fairly decent
separation of interests between hba.c (parsing the hba.conf syntax) and
ip.c (messing with address representations).

While talking about beauty: that setting of *cidr_slash to '/' and 0
doesn't look too esthetic...

It is ugly (and I didn't write it ;-)). But if we palloc'd a modified
version of the token we'd have to remember to pfree it, so it nets out
to about the same amount of code either way I think. If you wanna try
to clean it up more, be my guest ...

Would you like me to conditionally add the IPv6 line to pg_hba.conf from
initdb now?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian said:

Would you like me to conditionally add the IPv6 line to pg_hba.conf
from initdb now?

As a matter of taste and possibly due to some paranoia I would prefer to
see it added unconditionally to pg_hba.conf and conditionally commented
out in initdb. (The paranoia is what says to me that it is better to have
hidden removal of something useless than hidden addition of something
potentially dangerous).

cheers

andrew

andrew@dunslane.net wrote:

Bruce Momjian said:

Would you like me to conditionally add the IPv6 line to pg_hba.conf
from initdb now?

As a matter of taste and possibly due to some paranoia I would prefer to
see it added unconditionally to pg_hba.conf and conditionally commented
out in initdb. (The paranoia is what says to me that it is better to have
hidden removal of something useless than hidden addition of something
potentially dangerous).

So have initdb coditionally comment it out in data/pg_hba.conf --- I can
do that.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Would you like me to conditionally add the IPv6 line to pg_hba.conf from
initdb now?

I was going to tackle that tomorrow, but if you wanna do it, go for it.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Would you like me to conditionally add the IPv6 line to pg_hba.conf from
initdb now?

I was going to tackle that tomorrow, but if you wanna do it, go for it.

No, go ahead. I wasn't sure if you were waiting for me so I asked.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073