Re: host name support in pg_hba.conf

(2010/10/06 10:21), KaiGai Kohei wrote:
> I'll check the patch for more details, please wait for a few days.

I could find out some matters in this patch, independent from the
discussion of localhost. (About pg_hba.conf.sample, I'm sorry for
the missuggestion. Please fix up it according to Tom's comment.)

* The logic is still unclear for me.

The check_hostname() immediately returns with false, if the resolved
remote hostname is NOT matched with the hostname described in pg_hba.conf.

| +static bool
| +check_hostname(hbaPort *port, const char *hostname)
| +{
| + struct addrinfo *gai_result, *gai;
| + int ret;
| + bool found;
| +
| + /* Lookup remote host name if not already done */
| + if (!port->remote_hostname)
| + {
| + char remote_hostname[NI_MAXHOST];
| +
| + if (pg_getnameinfo_all(&port->raddr.addr, port->raddr.salen,
| + remote_hostname, sizeof(remote_hostname),
| + NULL, 0,
| + 0))
| + return false;
| +
| + port->remote_hostname = strdup(remote_hostname);
| + }
| +
| + if (strcmp(port->remote_hostname, hostname) != 0)
| + return false;
| +
| + /* Lookup IP from host name and check against original IP */

However, it seems to me you expected an opposite behavior.

If the resolved hostname is matched with the hostname described
in pg_hba.conf, we can consider this HbaLine to be a suitable
configuration without any fallbacks. Right?
It so, it should be as follows:

if (strcmp(port->remote_hostname, hostname) == 0)
return true;

In addition, we should go the rest of fallback code on mismatch
cases only, don't we?

* Why getnameinfo() in the fallback loop?

At the fallback code when the hostname was matched (I believe this code
is intended to handle the case when hostname was NOT matched.) calls
getnameinfo() for each candidate of remote addresses.
But its result is not referenced by anybody. Is it really necessary?

| + found = false;
| + for (gai = gai_result; gai; gai = gai->ai_next)
| + {
| + char hostinfo[NI_MAXHOST];
| +
| + getnameinfo(gai->ai_addr, gai->ai_addrlen,
| + hostinfo, sizeof(hostinfo),
| + NULL, 0,
| + NI_NUMERICHOST);
| +
| + if (gai->ai_addr->sa_family == port->raddr.addr.ss_family)
| + {
| + if (gai->ai_addr->sa_family == AF_INET)
| + {
| + if (ipv4eq((struct sockaddr_in *) gai->ai_addr,
| + (struct sockaddr_in *) &port->raddr.addr))
| + {
| + found = true;
| + break;
| + }
| + }
| + else if (gai->ai_addr->sa_family == AF_INET6)
| + {
| + if (ipv6eq((struct sockaddr_in6 *) gai->ai_addr,
| + (struct sockaddr_in6 *) &port->raddr.addr))
| + {
| + found = true;
| + break;
| + }
| + }
| + }
| + }
| +
| + if (gai_result)
| + freeaddrinfo(gai_result);
| +
| + return found;
| +}

* Slash ('/') after the hostname

At the parse_hba_line(), the parsed token which contains either
hostname or cidr address is sliced into two parts on the first '/'
character, if exist.
Then, even if cidr_slash is not NULL, it shall be ignored when
top-half of the token is hostname, not numeric address.

| else
| {
| /* IP and netmask are specified */
| parsedline->ip_cmp_method = ipCmpMask;
|
| /* need a modifiable copy of token */
| token = pstrdup(token);
|
| /* Check if it has a CIDR suffix and if so isolate it */
| cidr_slash = strchr(token, '/');
| if (cidr_slash)
| *cidr_slash = '\0';
:
| ret = pg_getaddrinfo_all(token, NULL, &hints, &gai_result);
| - if (ret || !gai_result)
| + if (ret == 0 && gai_result)
| + memcpy(&parsedline->addr, gai_result->ai_addr,
| + gai_result->ai_addrlen);
| + else if (ret == EAI_NONAME)
| + parsedline->hostname = token;
| + else
| {

It allows the following configuration works without any errors.
(In fact, it works for me.)

# IPv4/6 local connections:
host all all kaigai.myhome.cx/today_is_sunny trust

It seems to me, we should raise an error, if both of cidr_slash and
parsedline->hostname are not NULL.

Thanks,
--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>