Re: Fallout from PQhost() semantics changes - Mailing list pgsql-hackers

Traditionally (prior to v10), PQhost() returned the "host" connection
parameter if that was nonempty, otherwise the default host name
(DEFAULT_PGSOCKET_DIR or "localhost" depending on platform).

That got whacked around to a state of brokenness in v10 (which I'll return
to in a bit), and then commit 1944cdc98 fixed it to return the active
host's connhost[].host string if nonempty, else the connhost[].hostaddr
string if nonempty, else an empty string.  Together with the fact that the
default host name gets inserted into connhost[].host if neither option is
supplied, that's compatible with the traditional behavior when host is
supplied or when both options are omitted.  It's not the same when only
hostaddr is supplied.  This change is generally a good thing: returning
the default host name is pretty misleading if hostaddr actually points at
some remote server.  However, it seems that insufficient attention was
paid to whether *every* call site is OK with it.

In particular, libpq has several internal calls to PQhost() to get the
host name to be compared to a server SSL certificate, or for comparable
usages in GSS and SSPI authentication.  These changes mean that sometimes
we will be comparing the server's numeric address, not its hostname,
to the server auth information.  I do not think that was the intention;
it's certainly in direct contradiction to our documentation, which clearly
says that the host name parameter and nothing else is used for this
purpose.  It's not clear to me if this could amount to a security problem,
but at the least it's wrongly documented.

What I think we should do about it is change those internal calls to
fetch connhost[].host directly instead of going through PQhost(), as
in the attached libpq-internal-PQhost-usage-1.patch.  This will restore
the semantics to what they were pre-v10, including erroring out when
hostaddr is supplied without host.

I also noted that psql's \conninfo code takes it upon itself to substitute
the value of the hostaddr parameter, if used, for the result of PQhost().
This is entirely wrong/unhelpful if multiple host targets were specified;
moreover, that patch failed to account for the very similar connection
info printout in do_connect().  Given the change in PQhost's behavior
I think it'd be fine to just drop that complexity and print PQhost's
result without any editorialization, as in the attached
psql-conninfo-PQhost-usage-1.patch.

I would also like to make the case for back-patching 1944cdc98 into v10.
I'm not sure why that wasn't done to begin with, because v10's PQhost()
is just completely broken for cases involving a hostaddr specification:

    if (!conn)
        return NULL;
    if (conn->connhost != NULL &&
        conn->connhost[conn->whichhost].type != CHT_HOST_ADDRESS)
        return conn->connhost[conn->whichhost].host;
    else if (conn->pghost != NULL && conn->pghost[0] != '\0')
        return conn->pghost;
    else
    {
#ifdef HAVE_UNIX_SOCKETS
        return DEFAULT_PGSOCKET_DIR;
#else
        return DefaultHost;
#endif
    }

In the CHT_HOST_ADDRESS case, it will either give back the raw host
parameter (again, wrong if multiple hosts are targeted) or give back
DEFAULT_PGSOCKET_DIR/DefaultHost if the host wasn't specified.
Ignoring the brokenness for multiple target hosts, you could argue
that that's compatible with pre-v10 behavior ... but it's still pretty
misleading to give back DefaultHost, much less DEFAULT_PGSOCKET_DIR,
for a remote connection.  (There's at least some chance that the
hostaddr is actually 127.0.0.1 or ::1.  There is no chance that
DEFAULT_PGSOCKET_DIR is an appropriate description.)

Given that we whacked around v10 libpq's behavior for some related corner
cases earlier this week, I think it'd be OK to change this in v10.
If we do, it'd make sense to back-patch psql-conninfo-PQhost-usage-1.patch
into v10 as well.  I think that libpq-internal-PQhost-usage-1.patch should
be back-patched to v10 in any case, since whether or not you want to live
with the existing behavior of PQhost() in v10, it's surely not appropriate
for comparing to server SSL certificates.

In fact, I think there's probably a good case for doing something
comparable to libpq-internal-PQhost-usage-1.patch all the way back.
In exactly what scenario is it sane to be comparing "/tmp" or
"localhost" to a server's SSL certificate?