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

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 31ade0b..eb4c4ca 100644
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
*************** ClientAuthentication(Port *port)
*** 425,439 ****
                                     NI_NUMERICHOST);

  #define HOSTNAME_LOOKUP_DETAIL(port) \
!                 (port->remote_hostname                  \
!                  ? (port->remote_hostname_resolv == +1                    \
!                     ? errdetail_log("Client IP address resolved to \"%s\", forward lookup matches.",
port->remote_hostname)\ 
!                     : (port->remote_hostname_resolv == 0                \
!                        ? errdetail_log("Client IP address resolved to \"%s\", forward lookup not checked.",
port->remote_hostname)\ 
!                        : (port->remote_hostname_resolv == -1            \
!                           ? errdetail_log("Client IP address resolved to \"%s\", forward lookup does not match.",
port->remote_hostname)\ 
!                           : 0)))                                        \
!                  : 0)

                  if (am_walsender)
                  {
--- 425,449 ----
                                     NI_NUMERICHOST);

  #define HOSTNAME_LOOKUP_DETAIL(port) \
!                 (port->remote_hostname ? \
!                  (port->remote_hostname_resolv == +1 ? \
!                   errdetail_log("Client IP address resolved to \"%s\", forward lookup matches.", \
!                                 port->remote_hostname) : \
!                   port->remote_hostname_resolv == 0 ? \
!                   errdetail_log("Client IP address resolved to \"%s\", forward lookup not checked.", \
!                                 port->remote_hostname) : \
!                   port->remote_hostname_resolv == -1 ? \
!                   errdetail_log("Client IP address resolved to \"%s\", forward lookup does not match.", \
!                                 port->remote_hostname) : \
!                   port->remote_hostname_resolv == -2 ? \
!                   errdetail_log("Could not translate client host name \"%s\" to IP address: %s.", \
!                                 port->remote_hostname, \
!                                 gai_strerror(port->remote_hostname_errcode)) : \
!                   0) \
!                  : (port->remote_hostname_resolv == -2 ? \
!                     errdetail_log("Client IP address could not be resolved to a host name: %s.", \
!                                   gai_strerror(port->remote_hostname_errcode)) : \
!                     0))

                  if (am_walsender)
                  {
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 77434f4..83dd147 100644
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
*************** check_hostname(hbaPort *port, const char
*** 592,626 ****
      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) != 0)
              return false;

          port->remote_hostname = pstrdup(remote_hostname);
      }

      if (!hostname_match(hostname, port->remote_hostname))
          return false;

!     /* Lookup IP from host name and check against original IP */
!
      if (port->remote_hostname_resolv == +1)
          return true;
-     if (port->remote_hostname_resolv == -1)
-         return false;

      ret = getaddrinfo(port->remote_hostname, NULL, NULL, &gai_result);
      if (ret != 0)
!         ereport(ERROR,
!                 (errmsg("could not translate host name \"%s\" to address: %s",
!                         port->remote_hostname, gai_strerror(ret))));

      found = false;
      for (gai = gai_result; gai; gai = gai->ai_next)
--- 592,638 ----
      int            ret;
      bool        found;

+     /* Quick out if remote host name already known bad */
+     if (port->remote_hostname_resolv < 0)
+         return false;
+
      /* Lookup remote host name if not already done */
      if (!port->remote_hostname)
      {
          char        remote_hostname[NI_MAXHOST];

!         ret = pg_getnameinfo_all(&port->raddr.addr, port->raddr.salen,
!                                  remote_hostname, sizeof(remote_hostname),
!                                  NULL, 0,
!                                  NI_NAMEREQD);
!         if (ret != 0)
!         {
!             /* remember failure; don't complain in the postmaster log yet */
!             port->remote_hostname_resolv = -2;
!             port->remote_hostname_errcode = ret;
              return false;
+         }

          port->remote_hostname = pstrdup(remote_hostname);
      }

+     /* Now see if remote host name matches this pg_hba line */
      if (!hostname_match(hostname, port->remote_hostname))
          return false;

!     /* If we already verified the forward lookup, we're done */
      if (port->remote_hostname_resolv == +1)
          return true;

+     /* Lookup IP from host name and check against original IP */
      ret = getaddrinfo(port->remote_hostname, NULL, NULL, &gai_result);
      if (ret != 0)
!     {
!         /* remember failure; don't complain in the postmaster log yet */
!         port->remote_hostname_resolv = -2;
!         port->remote_hostname_errcode = ret;
!         return false;
!     }

      found = false;
      for (gai = gai_result; gai; gai = gai->ai_next)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index e9072b7..41556a4 100644
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*************** BackendInitialize(Port *port)
*** 3952,3959 ****
       */
      port->remote_host = strdup(remote_host);
      port->remote_port = strdup(remote_port);
-     if (log_hostname)
-         port->remote_hostname = port->remote_host;

      /*
       * Ready to begin client interaction.  We will give up and exit(1) after a
--- 3952,3957 ----
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 270e8fb..546d88c 100644
*** a/src/include/libpq/libpq-be.h
--- b/src/include/libpq/libpq-be.h
*************** extern int    ssl_renegotiation_limit;
*** 104,109 ****
--- 104,123 ----
   * still available when a backend is running (see MyProcPort).    The data
   * it points to must also be malloc'd, or else palloc'd in TopMemoryContext,
   * so that it survives into PostgresMain execution!
+  *
+  * remote_hostname is set if we did a successful reverse lookup of the
+  * client's IP address during authentication.  remote_hostname_resolv tracks
+  * the state of hostname verification:
+  *    +1 = remote_hostname is known to resolve to client's IP address
+  *    -1 = remote_hostname is known NOT to resolve to client's IP address
+  *     0 = we have not done the forward DNS lookup yet
+  *    -2 = there was an error in name resolution
+  * If reverse lookup of the client IP address fails, remote_hostname will be
+  * left NULL while remote_hostname_resolv is set to -2.  If reverse lookup
+  * succeeds but forward lookup fails, remote_hostname_resolv is also set to -2
+  * (the case is distinguishable because remote_hostname isn't NULL).  In
+  * either of the -2 cases, remote_hostname_errcode saves the lookup return
+  * code for possible later use with gai_strerror.
   */

  typedef struct Port
*************** typedef struct Port
*** 116,127 ****
      char       *remote_host;    /* name (or ip addr) of remote host */
      char       *remote_hostname;/* name (not ip addr) of remote host, if
                                   * available */
!     int            remote_hostname_resolv; /* +1 = remote_hostname is known to
!                                          * resolve to client's IP address; -1
!                                          * = remote_hostname is known NOT to
!                                          * resolve to client's IP address; 0 =
!                                          * we have not done the forward DNS
!                                          * lookup yet */
      char       *remote_port;    /* text rep of remote port */
      CAC_state    canAcceptConnections;    /* postmaster connection status */

--- 130,137 ----
      char       *remote_host;    /* name (or ip addr) of remote host */
      char       *remote_hostname;/* name (not ip addr) of remote host, if
                                   * available */
!     int            remote_hostname_resolv; /* see above */
!     int            remote_hostname_errcode;        /* see above */
      char       *remote_port;    /* text rep of remote port */
      CAC_state    canAcceptConnections;    /* postmaster connection status */