Re: [BUGS] BUG #9518: temporary login failure - "missing pg_hba entry"

I wrote:
>> IOW, it looks to me like intermittent failures in the reverse DNS lookup
>> could disable matching by hostname, and nothing would be said in the
>> postmaster log. Why is there no complaint if check_hostname's call to
>> pg_getnameinfo_all (line 600 in HEAD) fails?

> After sleeping on it, I think probably the reason it is like that is a
> desire to not clutter the postmaster log if there are some legitimate
> clients without rDNS entries. That is, suppose pg_hba.conf has

> host foo.bar.com ...
> host 192.168.168.1 ...

> and you've not bothered to create a reverse-DNS entry for 192.168.168.1.
> We will try (and fail) to look up the rDNS entry while considering the
> foo.bar.com line. We certainly don't want a failure there to prevent us
> from reaching the 192.168.168.1 line, and we don't really want to clutter
> the postmaster log with a bleat about it, either. Hence the lack of any
> error logging in the existing code. (The later cross-check on whether
> the forward DNS matches does have an error report, which maybe isn't such
> a great thing either from this standpoint.)

> The problem of course is that if the rDNS failure prevents us from
> matching to *any* line, we exit with no error more helpful than
> "missing pg_hba entry", which is not very desirable in this case.

> I guess we could do something like remember the fact that we tried and
> failed to do an rDNS lookup, and report it as DETAIL in the eventual
> "missing pg_hba entry" report. Not quite sure if it's worth the trouble
> --- any thoughts?

> Another objection to the code as it stands is that if there are multiple
> pg_hba lines containing hostnames, we'll repeat the failing rDNS lookup
> at each one. This is at best a huge waste of cycles (multiple network
> roundtrips, if the DNS server isn't local), and at worst inconsistent
> if things actually are intermittent and a later lookup attempt succeeds.
> I think we want to fix it to be sure that there's exactly one rDNS lookup
> attempt, occurring at the first line with a hostname.

Attached is a draft patch to deal with these issues. While testing it,
I realized that the existing code is wrong in a couple more ways than
I'd thought. In the first place, it does not specify the NI_NAMEREQD
flag to getnameinfo, which means that if getnameinfo can't resolve a
hostname for the client IP address, it'll just quietly return the numeric
address instead of complaining. That is certainly not what we want here.
In the second place, postmaster.c happily forced the result of its own
getnameinfo call into port->remote_hostname, despite the lack of
NI_NAMEREQD in that call, not to mention the fact that it would do so even
if that call had failed outright (not that that's too likely without
NI_NAMEREQD, I guess).

Unfortunately, I don't think we want to add NI_NAMEREQD to postmaster.c's
call, since then we'd get nothing usable at all for port->remote_host for
a client without an rDNS record. This means that we can't realistically
piggyback on that call to set port->remote_hostname. I thought about
trying to detect whether the returned string was a numeric IP address or
not, but that doesn't look so easy, because IPv6 addresses can contain
hex characters. So for instance a simple strspn character-set check
wouldn't be able to tell that "abc123.de" wasn't numeric. So I just
stripped out that code in the attached patch.

I'm tempted to back-patch this, because AFAICS the misbehaviors mentioned
here are flat-out bugs, independently of whether the error logging is
improved or not. Changing struct Port in back branches is a bit risky,
but we could put the added field at the end in the back branches.

Thoughts?

regards, tom lane