pgsql: Define INADDR_NONE on Solaris when it's missing.
Log Message:
-----------
Define INADDR_NONE on Solaris when it's missing. Per a couple of buildfarm
members complaining.
Modified Files:
--------------
pgsql/src/include/port:
solaris.h (r1.17 -> r1.18)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/port/solaris.h?r1=1.17&r2=1.18)
mha@postgresql.org (Magnus Hagander) writes:
Log Message:
-----------
Define INADDR_NONE on Solaris when it's missing. Per a couple of buildfarm
members complaining.
This seems likely to break as much as it fixes, since there's no very
good reason to assume that whatever header should define INADDR_NONE
has been included before the os.h header file has been read.
Possibly more to the point, where are we using INADDR_NONE anyway?
regards, tom lane
On Thu, Jan 28, 2010 at 16:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
mha@postgresql.org (Magnus Hagander) writes:
Log Message:
-----------
Define INADDR_NONE on Solaris when it's missing. Per a couple of buildfarm
members complaining.This seems likely to break as much as it fixes, since there's no very
good reason to assume that whatever header should define INADDR_NONE
has been included before the os.h header file has been read.
Hmm. Where would you suggest it goes?
The addition of such a define is in a lot of places on the net as
fixing just this issue, and was also recommended by Zdenek as the fix
for Solaris. But I can agree it may be in the wrong place :-)
Possibly more to the point, where are we using INADDR_NONE anyway?
In the RADIUS code.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
On Thu, Jan 28, 2010 at 16:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Possibly more to the point, where are we using INADDR_NONE anyway?
In the RADIUS code.
Oh, that's why it isn't in my tree and has zero portability track record ...
I think what this shows is we should look for a way to avoid using
INADDR_NONE. What's your grounds for believing it's portable at all?
In the Single Unix Spec I only see INADDR_ANY and INADDR_BROADCAST
defined.
regards, tom lane
On Thu, Jan 28, 2010 at 17:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Thu, Jan 28, 2010 at 16:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Possibly more to the point, where are we using INADDR_NONE anyway?
In the RADIUS code.
Oh, that's why it isn't in my tree and has zero portability track record ...
I think what this shows is we should look for a way to avoid using
INADDR_NONE. What's your grounds for believing it's portable at all?
In the Single Unix Spec I only see INADDR_ANY and INADDR_BROADCAST
defined.
Um, I don't think I have any specific grounds for it, other than
having seen it in a lot of other software :-)
From some more googling
(http://www.opengroup.org/onlinepubs/000095399/functions/inet_addr.html),
it says it will return (in_addr_t)(-1), though, so maybe we should
just move that #ifdef out to some global place?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
On Thu, Jan 28, 2010 at 17:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think what this shows is we should look for a way to avoid using
INADDR_NONE.
From some more googling
(http://www.opengroup.org/onlinepubs/000095399/functions/inet_addr.html),
it says it will return (in_addr_t)(-1), though, so maybe we should
just move that #ifdef out to some global place?
Given the way that's written, I think we should just compare the result
to (in_addr_t)(-1), and not assume there's any macro provided for that.
However, now that I know the real issue is you're using inet_addr, I
would like to know why you're not using inet_aton instead; or even
better, something that also copes with IPv6.
regards, tom lane
On Thu, Jan 28, 2010 at 17:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Thu, Jan 28, 2010 at 17:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think what this shows is we should look for a way to avoid using
INADDR_NONE.From some more googling
(http://www.opengroup.org/onlinepubs/000095399/functions/inet_addr.html),
it says it will return (in_addr_t)(-1), though, so maybe we should
just move that #ifdef out to some global place?Given the way that's written, I think we should just compare the result
to (in_addr_t)(-1), and not assume there's any macro provided for that.
Well, that doesn't match all other platforms..
However, now that I know the real issue is you're using inet_addr, I
would like to know why you're not using inet_aton instead; or even
better, something that also copes with IPv6.
"Path of least resistance?"
Which method would you suggest?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
On Thu, Jan 28, 2010 at 17:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
However, now that I know the real issue is you're using inet_addr, I
would like to know why you're not using inet_aton instead; or even
better, something that also copes with IPv6.
"Path of least resistance?"
Which method would you suggest?
I haven't actually read the RADIUS patch, but generally we rely on
pg_getaddrinfo_all to interpret strings representing IP addresses.
Is there a reason not to use that?
regards, tom lane
On Thu, Jan 28, 2010 at 21:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Thu, Jan 28, 2010 at 17:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
However, now that I know the real issue is you're using inet_addr, I
would like to know why you're not using inet_aton instead; or even
better, something that also copes with IPv6."Path of least resistance?"
Which method would you suggest?
I haven't actually read the RADIUS patch, but generally we rely on
pg_getaddrinfo_all to interpret strings representing IP addresses.
Is there a reason not to use that?
I don't think so. I'll look it over.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
2010/1/28 Magnus Hagander <magnus@hagander.net>:
On Thu, Jan 28, 2010 at 21:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Thu, Jan 28, 2010 at 17:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
However, now that I know the real issue is you're using inet_addr, I
would like to know why you're not using inet_aton instead; or even
better, something that also copes with IPv6."Path of least resistance?"
Which method would you suggest?
I haven't actually read the RADIUS patch, but generally we rely on
pg_getaddrinfo_all to interpret strings representing IP addresses.
Is there a reason not to use that?I don't think so. I'll look it over.
Here's what I came up with. Works well on the platforms I've tried,
but I haven't tried on a non-ipv6 capable one yet (need to find one..)
I'll also remove the defines from solaris.h when applying it.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachments:
radius_addr.patchapplication/octet-stream; name=radius_addr.patchDownload
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 2a9625f..47ba2a8 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -1375,8 +1375,8 @@ ldapserver=ldap.example.net ldapprefix="cn=" ldapsuffix=", dc=example, dc=net"
<term><literal>radiusserver</literal></term>
<listitem>
<para>
- The IP address of the RADIUS server to connect to. This must
- be an IPV4 address and not a hostname. This parameter is required.
+ The name or IP address of the RADIUS server to connect to.
+ This parameter is required.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 36be782..c8bf16b 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2521,8 +2521,16 @@ CheckRADIUSAuth(Port *port)
uint8 encryptedpassword[RADIUS_VECTOR_LENGTH];
int packetlength;
pgsocket sock;
+#ifdef HAVE_IPV6
+ struct sockaddr_in6 localaddr;
+ struct sockaddr_in6 remoteaddr;
+#else
struct sockaddr_in localaddr;
struct sockaddr_in remoteaddr;
+#endif
+ struct addrinfo hint;
+ struct addrinfo *serveraddrs;
+ char portstr[128];
ACCEPT_TYPE_ARG3 addrsize;
fd_set fdset;
struct timeval timeout;
@@ -2549,17 +2557,22 @@ CheckRADIUSAuth(Port *port)
if (port->hba->radiusport == 0)
port->hba->radiusport = 1812;
- memset(&remoteaddr, 0, sizeof(remoteaddr));
- remoteaddr.sin_family = AF_INET;
- remoteaddr.sin_addr.s_addr = inet_addr(port->hba->radiusserver);
- if (remoteaddr.sin_addr.s_addr == INADDR_NONE)
+ MemSet(&hint, 0, sizeof(hint));
+ hint.ai_socktype = SOCK_DGRAM;
+ hint.ai_family = AF_UNSPEC;
+ snprintf(portstr, sizeof(portstr), "%d", port->hba->radiusport);
+
+ r = pg_getaddrinfo_all(port->hba->radiusserver, portstr, &hint, &serveraddrs);
+ if (r || !serveraddrs)
{
ereport(LOG,
- (errmsg("RADIUS server '%s' is not a valid IP address",
- port->hba->radiusserver)));
+ (errmsg("could not translate RADIUS server name '%s' to address: %s",
+ port->hba->radiusserver, gai_strerror(r))));
+ if (serveraddrs)
+ pg_freeaddrinfo_all(hint.ai_family, serveraddrs);
return STATUS_ERROR;
}
- remoteaddr.sin_port = htons(port->hba->radiusport);
+ /* XXX: add support for multiple returned addresses? */
if (port->hba->radiusidentifier && port->hba->radiusidentifier[0])
identifier = port->hba->radiusidentifier;
@@ -2633,34 +2646,46 @@ CheckRADIUSAuth(Port *port)
packetlength = packet->length;
packet->length = htons(packet->length);
- sock = socket(AF_INET, SOCK_DGRAM, 0);
+ sock = socket(serveraddrs[0].ai_family, SOCK_DGRAM, 0);
if (sock < 0)
{
ereport(LOG,
(errmsg("could not create RADIUS socket: %m")));
+ pg_freeaddrinfo_all(hint.ai_family, serveraddrs);
return STATUS_ERROR;
}
memset(&localaddr, 0, sizeof(localaddr));
- localaddr.sin_family = AF_INET;
+#ifdef HAVE_IPV6
+ localaddr.sin6_family = serveraddrs[0].ai_family;
+ localaddr.sin6_addr = in6addr_any;
+#else
+ localaddr.sin_family = serveraddrs[0].ai_family;
localaddr.sin_addr.s_addr = INADDR_ANY;
+#endif
if (bind(sock, (struct sockaddr *) &localaddr, sizeof(localaddr)))
{
ereport(LOG,
(errmsg("could not bind local RADIUS socket: %m")));
closesocket(sock);
+ pg_freeaddrinfo_all(hint.ai_family, serveraddrs);
return STATUS_ERROR;
}
if (sendto(sock, radius_buffer, packetlength, 0,
- (struct sockaddr *) &remoteaddr, sizeof(remoteaddr)) < 0)
+ serveraddrs[0].ai_addr, serveraddrs[0].ai_addrlen) < 0)
{
ereport(LOG,
(errmsg("could not send RADIUS packet: %m")));
closesocket(sock);
+ pg_freeaddrinfo_all(hint.ai_family, serveraddrs);
return STATUS_ERROR;
}
+ /* Don't need the server address anymore */
+ pg_freeaddrinfo_all(hint.ai_family, serveraddrs);
+
+ /* Wait for a response */
timeout.tv_sec = RADIUS_TIMEOUT;
timeout.tv_usec = 0;
FD_ZERO(&fdset);
@@ -2705,11 +2730,21 @@ CheckRADIUSAuth(Port *port)
closesocket(sock);
+#ifdef HAVE_IPV6
+ if (remoteaddr.sin6_port != htons(port->hba->radiusport))
+#else
if (remoteaddr.sin_port != htons(port->hba->radiusport))
+#endif
{
+#ifdef HAVE_IPV6
+ ereport(LOG,
+ (errmsg("RADIUS response was sent from incorrect port: %i",
+ ntohs(remoteaddr.sin6_port))));
+#else
ereport(LOG,
(errmsg("RADIUS response was sent from incorrect port: %i",
ntohs(remoteaddr.sin_port))));
+#endif
return STATUS_ERROR;
}
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index dd7ad5c..927b394 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1167,16 +1167,25 @@ parse_hba_line(List *line, int line_num, HbaLine *parsedline)
else if (strcmp(token, "radiusserver") == 0)
{
REQUIRE_AUTH_OPTION(uaRADIUS, "radiusserver", "radius");
- if (inet_addr(c) == INADDR_NONE)
+
+ MemSet(&hints, 0, sizeof(hints));
+ hints.ai_socktype = SOCK_DGRAM;
+ hints.ai_family = AF_UNSPEC;
+
+ ret = pg_getaddrinfo_all(c, NULL, &hints, &gai_result);
+ if (ret || !gai_result)
{
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("invalid RADIUS server IP address: \"%s\"", c),
+ errmsg("could not translate RADIUS server name '%s' to address: %s",
+ c, gai_strerror(ret)),
errcontext("line %d of configuration file \"%s\"",
line_num, HbaFileName)));
+ if (gai_result)
+ pg_freeaddrinfo_all(hints.ai_family, gai_result);
return false;
-
}
+ pg_freeaddrinfo_all(hints.ai_family, gai_result);
parsedline->radiusserver = pstrdup(c);
}
else if (strcmp(token, "radiusport") == 0)
Magnus Hagander <magnus@hagander.net> writes:
Here's what I came up with. Works well on the platforms I've tried,
but I haven't tried on a non-ipv6 capable one yet (need to find one..)
Hmm, well, I have an ipv6-ignorant HPUX box at hand. I do not have a
radius server though. Are you only concerned about whether it compiles,
or do you want actual testing?
regards, tom lane
2010/2/1 Magnus Hagander <magnus@hagander.net>:
2010/1/28 Magnus Hagander <magnus@hagander.net>:
On Thu, Jan 28, 2010 at 21:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
On Thu, Jan 28, 2010 at 17:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
However, now that I know the real issue is you're using inet_addr, I
would like to know why you're not using inet_aton instead; or even
better, something that also copes with IPv6."Path of least resistance?"
Which method would you suggest?
I haven't actually read the RADIUS patch, but generally we rely on
pg_getaddrinfo_all to interpret strings representing IP addresses.
Is there a reason not to use that?I don't think so. I'll look it over.
Here's what I came up with. Works well on the platforms I've tried,
but I haven't tried on a non-ipv6 capable one yet (need to find one..)
I'll also remove the defines from solaris.h when applying it.
Applied with some adjustments needed for non-ipv6 platforms.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/