Horton

Thread

Commits

  1. In libpq, don't look up all the hostnames at once.

  1. libpq should not look up all host addresses at once

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-09T15:05:02Z

    Whilst fooling with the patch for CVE-2018-10915, I got annoyed by
the fact that connectDBStart() does the DNS lookups for all supplied
hostnames at once, and fails if any of them are bad.  It was reasonable
to do the lookup there when we only allowed one hostname, but now that
"host" can be a list, this is really pretty stupid.  The whole point
of allowing multiple hostnames is redundancy and avoiding a single
point of failure; but the way this is written, *each* of your servers'
DNS servers is a single point of failure all by itself.  If any one of
them is down, you don't connect.  Plus, in the normal case where you
successfully connect to something before the very last host in the list,
the extra DNS lookups are wasted --- and DNS lookups aren't that cheap,
since they typically involve a network round trip.

So I think what this code should do is (1) look up each hostname as it
needs it, not all at once, and (2) proceed on to the next hostname
if it gets a DNS lookup failure, not fail the whole connection attempt
immediately.  As attached.

I'm tempted to call this a back-patchable bug fix, because the existing
behavior basically negates the entire value of the multi-hostname feature
once you consider the possibility of DNS server failures.  But given the
lack of field complaints, maybe that's an overreaction.

I'll put this in the September fest for review.

			regards, tom lane

  2. Re: libpq should not look up all host addresses at once

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-08-09T16:42:08Z

    On 2018-Aug-09, Tom Lane wrote:

> Whilst fooling with the patch for CVE-2018-10915, I got annoyed by
> the fact that connectDBStart() does the DNS lookups for all supplied
> hostnames at once, and fails if any of them are bad.  It was reasonable
> to do the lookup there when we only allowed one hostname, but now that
> "host" can be a list, this is really pretty stupid.  The whole point
> of allowing multiple hostnames is redundancy and avoiding a single
> point of failure; but the way this is written, *each* of your servers'
> DNS servers is a single point of failure all by itself.  If any one of
> them is down, you don't connect.  Plus, in the normal case where you
> successfully connect to something before the very last host in the list,
> the extra DNS lookups are wasted --- and DNS lookups aren't that cheap,
> since they typically involve a network round trip.

I'm not very familiar with the libpq code structure, but I think
connectDBStart() is not used for synchronous connections, only
asynchronous, is that correct?  If that's the case, then perhaps the
reason this hasn't been more widely reported is simple that those two
features (async conns and multihost conninfo strings) are just not very
commonly used ... and even less so with failing DNS setups.

> I'm tempted to call this a back-patchable bug fix, because the existing
> behavior basically negates the entire value of the multi-hostname feature
> once you consider the possibility of DNS server failures.  But given the
> lack of field complaints, maybe that's an overreaction.

Well, since it's broken, then I don't think it serves anybody very well.
I vote to backpatch it to 10.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  3. Re: libpq should not look up all host addresses at once

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-09T17:08:20Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Aug-09, Tom Lane wrote:
>> Whilst fooling with the patch for CVE-2018-10915, I got annoyed by
>> the fact that connectDBStart() does the DNS lookups for all supplied
>> hostnames at once, and fails if any of them are bad.  It was reasonable
>> to do the lookup there when we only allowed one hostname, but now that
>> "host" can be a list, this is really pretty stupid.

> I'm not very familiar with the libpq code structure, but I think
> connectDBStart() is not used for synchronous connections, only
> asynchronous, is that correct?

No, it's used for all connections.  It's trivial to demonstrate the
problem in, eg, psql:

$ psql "host=localhost"
[ connects OK ]
$ psql "host=localhost,bogus"
psql: could not translate host name "bogus" to address: Name or service not known

>> I'm tempted to call this a back-patchable bug fix, because the existing
>> behavior basically negates the entire value of the multi-hostname feature
>> once you consider the possibility of DNS server failures.  But given the
>> lack of field complaints, maybe that's an overreaction.

> Well, since it's broken, then I don't think it serves anybody very well.
> I vote to backpatch it to 10.

The only reason not to is that it involves a change in the contents of
struct PGconn.  But I suppose we could put the added fields at the end
in v10, if we're worried about clients looking into that struct (not
that they should be, but ...)

			regards, tom lane

  4. Re: libpq should not look up all host addresses at once

    Chapman Flack <chap@anastigmatix.net> — 2018-08-09T18:18:36Z

    On 08/09/2018 11:05 AM, Tom Lane wrote:

> So I think what this code should do is (1) look up each hostname as it
> needs it, not all at once, and (2) proceed on to the next hostname
> if it gets a DNS lookup failure, not fail the whole connection attempt
> immediately.  As attached.

Would it be worth the complexity to be a little async about it,
fling a few DNS requests out, and try the hosts in the order the
responses come back?

-Chap

  5. Re: libpq should not look up all host addresses at once

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-09T18:32:58Z

    Chapman Flack <chap@anastigmatix.net> writes:
> On 08/09/2018 11:05 AM, Tom Lane wrote:
>> So I think what this code should do is (1) look up each hostname as it
>> needs it, not all at once, and (2) proceed on to the next hostname
>> if it gets a DNS lookup failure, not fail the whole connection attempt
>> immediately.  As attached.

> Would it be worth the complexity to be a little async about it,
> fling a few DNS requests out, and try the hosts in the order the
> responses come back?

It would be nice if an async connection request didn't have to block during
DNS lookups ... but I don't know of any portable library API for async DNS
requests, and it's most certainly not worth the trouble for us to write
our own version of getaddrinfo(3).

In practice, I think the async connection mode is mostly a legacy API
at this point anyway; surely most people who need that sort of behavior
are handling it nowadays by invoking libpq on a separate thread.  So I
just don't see it being worth a huge amount of work and maintenance
effort to get that to happen.  (Having said that, at least moving the
lookup from connectDBStart into PQconnectPoll, as this patch does,
is a step in the right direction.)

Now that I think about it, there may be some text in the libpq docs
claiming that the lookup happens in PQconnectStart not PQconnectPoll;
that would need adjustment.

			regards, tom lane

  6. Re: libpq should not look up all host addresses at once

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-08-11T10:18:48Z

    Hello Tom,

> [...]
>
> So I think what this code should do is (1) look up each hostname as it
> needs it, not all at once, and (2) proceed on to the next hostname
> if it gets a DNS lookup failure, not fail the whole connection attempt
> immediately.  As attached.

A quick test, and very quick glance at the code.

"git apply" gives "error: patch with only garbage at line 3". Patch 
applies with "patch -p1".

Patch compiles, global "make check" ok, although I'm unsure whether the
feature is actually tested somewhere. I think not:-(

As you noted in another message, a small doc update should be needed.

Patch works as expected: I tried with failing dns queries, multiple ips 
some of which or all failing connection attempts...

About the behavior from psql point of view:

* if dns works, error messages are only printed if all attempts failed:

  sh> ./psql "host=127.0.0.17,local2.coelho.net"
  psql: could not connect to server: Connection refused
         Is the server running on host "127.0.0.17" and accepting
         TCP/IP connections on port 5432?
  could not connect to server: Connection refused
         Is the server running on host "local2.coelho.net" (127.0.0.3) and accepting
         TCP/IP connections on port 5432?
  could not connect to server: Connection refused
         Is the server running on host "local2.coelho.net" (127.0.0.2) and accepting
         TCP/IP connections on port 5432?

But nothing shows if one succeeds at some point. I understand that libpq 
is doing its job, but I'm wondering whether this is the best behavior.

Maybe the user would like to know that attempts are made and are failing? 
This would suggest some callback mecanism so that the client is informed 
of in progress issues? Maybe this is for future work.

Maybe psql should show these as warnings?

* when the password is required, there is no way to know for which host/ip 
it is:

  sh> psql "host=127.0.0.17,local2.coelho.net,local.coelho.net"
  Password for user fabien:

* once connected, \conninfo shows only a partial information:

  psql> \conninfo
  You are connected to database "postgres" as user "fabien" on host
  "local3.coelho.net" at port "5432".

But local3 is 127.0.0.4 and 127.0.0.1, which one is it?

* Code

atoi("5432+1") == 5432, so the port syntax check is loose, really.

I'd consider wrapping some of the logic. I'd check the port first, then 
move the host resolution stuff into a function.

I would be fine with backpatching, as the current behavior is not somehow 
broken.

-- 
Fabien.

  7. Re: libpq should not look up all host addresses at once

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-08-11T12:15:44Z

    > * when the password is required, there is no way to know for which host/ip it 
> is:
>
> sh> psql "host=127.0.0.17,local2.coelho.net,local.coelho.net"
> Password for user fabien:

In the same vein on a wrong password:

  sh> psql "host=no-such-host,local2.coelho.net,local3.coelho.net"
  Password for user fabien:
  psql: could not translate host name "no-such-host" to address: Name or service not known
  could not connect to server: Connection refused
         Is the server running on host "local2.coelho.net" (127.0.0.2) and accepting
         TCP/IP connections on port 5432?
  could not connect to server: Connection refused
         Is the server running on host "local2.coelho.net" (127.0.0.3) and accepting
         TCP/IP connections on port 5432?
  FATAL:  password authentication failed for user "fabien"

The Fatal error does not really say for which host/ip the password fail.

Basically, with multiple hostnames and ips per hostname, error messages
need to be more precise.

-- 
Fabien.

  8. Re: libpq should not look up all host addresses at once

    Noah Misch <noah@leadboat.com> — 2018-08-12T06:26:15Z

    On Thu, Aug 09, 2018 at 02:32:58PM -0400, Tom Lane wrote:
> Chapman Flack <chap@anastigmatix.net> writes:
> > On 08/09/2018 11:05 AM, Tom Lane wrote:
> >> So I think what this code should do is (1) look up each hostname as it
> >> needs it, not all at once, and (2) proceed on to the next hostname
> >> if it gets a DNS lookup failure, not fail the whole connection attempt
> >> immediately.  As attached.
> 
> > Would it be worth the complexity to be a little async about it,
> > fling a few DNS requests out, and try the hosts in the order the
> > responses come back?
> 
> It would be nice if an async connection request didn't have to block during
> DNS lookups ... but I don't know of any portable library API for async DNS
> requests

https://c-ares.haxx.se/ is the async DNS API that I've experienced.  It works
well if you really need such a thing, but I haven't needed it in libpq.

  9. Re: libpq should not look up all host addresses at once

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-13T14:39:55Z

    Fabien COELHO <coelho@cri.ensmp.fr> writes:
> About the behavior from psql point of view:

> * if dns works, error messages are only printed if all attempts failed:
> But nothing shows if one succeeds at some point. I understand that libpq 
> is doing its job, but I'm wondering whether this is the best behavior.

Yeah, this is the behavior that was established by the multi-host patch
to begin with.  This patch just extends that to treat DNS failures the
same way as we already treated other connection problems.

> Maybe the user would like to know that attempts are made and are failing? 
> This would suggest some callback mecanism so that the client is informed 
> of in progress issues? Maybe this is for future work.

Well, the application can already tell that if it wishes to, by noting
whether PQhost/PQport return the values for the first alternative or
later ones.  Not sure that we need anything more.

> * when the password is required, there is no way to know for which host/ip 
> it is:

Again, I'm not here to re-litigate API decisions that were made in
connection with the multi-host patch.  What was decided (and documented)
at that point was that if you don't want to have the same password for all
the hosts in question, you need to use ~/.pgpass to supply per-host
passwords.

In practice, I'm not sure this matters too much.  It's hard to conceive of
a practical use-case in which all the target hosts aren't interchangeable
from the application's/user's standpoint.  That's why there's little or
no provision for varying the other conn parameters per-host.  We could
imagine future extensions to libpq to allow some or all of the rest of
them to be comma-separated lists, but I'm content to wait for a compelling
use-case to be shown before doing that work.

> atoi("5432+1") == 5432, so the port syntax check is loose, really.

libpq has always parsed port parameters that way.  Tightening it now
is not likely to earn us any thanks.

			regards, tom lane

  10. Re: libpq should not look up all host addresses at once

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-13T14:42:40Z

    Fabien COELHO <coelho@cri.ensmp.fr> writes:
> In the same vein on a wrong password:

>   sh> psql "host=no-such-host,local2.coelho.net,local3.coelho.net"
>   Password for user fabien:
>   psql: could not translate host name "no-such-host" to address: Name or service not known
>   could not connect to server: Connection refused
>          Is the server running on host "local2.coelho.net" (127.0.0.2) and accepting
>          TCP/IP connections on port 5432?
>   could not connect to server: Connection refused
>          Is the server running on host "local2.coelho.net" (127.0.0.3) and accepting
>          TCP/IP connections on port 5432?
>   FATAL:  password authentication failed for user "fabien"

> The Fatal error does not really say for which host/ip the password fail.

Yup, but that's not the province of this patch to improve.  See

https://www.postgresql.org/message-id/25918.1533918960@sss.pgh.pa.us

for one that is trying to improve it.

			regards, tom lane

  11. Re: libpq should not look up all host addresses at once

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-08-13T15:37:55Z

    >> The Fatal error does not really say for which host/ip the password fail.
>
> Yup, but that's not the province of this patch to improve.  See
>
> https://www.postgresql.org/message-id/25918.1533918960@sss.pgh.pa.us
>
> for one that is trying to improve it.

Yep, I gathered that afterwards.

-- 
Fabien.

  12. Re: libpq should not look up all host addresses at once

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-13T19:46:34Z

    Fabien COELHO <coelho@cri.ensmp.fr> writes:
> Patch compiles, global "make check" ok, although I'm unsure whether the
> feature is actually tested somewhere. I think not:-(

Yeah, it's hard to test this stuff without either opening up security
hazards or making unwarranted assumptions about the local network setup.
I think that the standard regression tests only use Unix-socket
communication (except on Windows) for exactly that reason, and that makes
it hard to do anything much about regression-testing this feature.

> As you noted in another message, a small doc update should be needed.

Check.  Proposed doc patch attached.  (Only the last hunk is actually
specific to this patch, the rest is cleanup that I noticed while looking
around for possibly-relevant text.)

> I'd consider wrapping some of the logic. I'd check the port first, then 
> move the host resolution stuff into a function.

Don't really see the value of either ...

			regards, tom lane

  13. Re: libpq should not look up all host addresses at once

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-08-14T10:24:32Z

    Hello Tom,

>> As you noted in another message, a small doc update should be needed.
>
> Check.  Proposed doc patch attached.  (Only the last hunk is actually
> specific to this patch, the rest is cleanup that I noticed while looking
> around for possibly-relevant text.)

Doc build is ok.

Some comments that you may not find all useful, please accept my apology, 
it just really shows that I read your prose in some detail:-)

The mention of possible reverse dns queries has been removed... but I do 
not think there was any before? There could be if only hostaddr is 
provided but a hostname is required by some auth, but it does not seem to 
be the case according to the documentation.

I read the rational of the host/hostaddr artificial mapping. I cannot say 
I'm thrilled with the result: I do not really see a setting where avoiding 
a DNS query is required but which still needs a hostname for auth... If 
you have GSSAPI or SSPI then you have an underlying network, in which a 
dns query should be fine.

Moreover the feature implementation is misleading as hostaddr changes the 
behavior of host when both are provided. Also, "hosts" accepts ips anyway 
(although is not documented).

STM that we should have had only "host" for specifying the target (name, 
ip, path), and if really necessary an ugly "authname" directive to provide 
names on the side. I know, too late.

I think that in the string format host=foo:5433,bla:5432 could be 
accepted as it is in the URL format.

Some of the changes are not directly related to the multi host feature, 
and should/could be backpatched further?

>> I'd consider wrapping some of the logic. I'd check the port first, then
>> move the host resolution stuff into a function.
>
> Don't really see the value of either ...

What I see is that the host logic is rather lengthy and specific (it 
depends on the connection type -- name, ip, socket including a so nice 
#ifdef), distinct from the port stuff which is dealt with quite quickcly.

By separating the port & host and putting the lengthy stuff in a function 
the scope of the for loop on potential connections would be easier to read 
and understand, and would not require to see CHT_ cases to understand what 
is being done for managing "host" variants, which is a mere detail.

-- 
Fabien.

  14. Re: libpq should not look up all host addresses at once

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-08-14T17:36:56Z

    Fabien COELHO <coelho@cri.ensmp.fr> writes:
> The mention of possible reverse dns queries has been removed... but I do 
> not think there was any before?

Yeah, that's why I took it out.  If there ever were any reverse lookups in
libpq, we got rid of them AFAICS, so this statement in the docs seemed just
unnecessarily confusing to me.

> I read the rational of the host/hostaddr artificial mapping. I cannot say 
> I'm thrilled with the result: I do not really see a setting where avoiding 
> a DNS query is required but which still needs a hostname for auth... If 
> you have GSSAPI or SSPI then you have an underlying network, in which a 
> dns query should be fine.

The point is that you might wish to avoid a DNS query for speed reasons,
not because it's "required" to do so.  Or, perhaps, you did the DNS
query already using some async DNS library, and you want to pass the
result on to PQconnectStart so it will not block.

> STM that we should have had only "host" for specifying the target (name, 
> ip, path), and if really necessary an ugly "authname" directive to provide 
> names on the side. I know, too late.
> I think that in the string format host=foo:5433,bla:5432 could be 
> accepted as it is in the URL format.

I'm uninterested in these proposals, and they certainly are not something
to include in this particular patch even if I were.

Also, it's too late to redefine the meaning of colon in a host string,
since as you mentioned that could already be an IPv6 address.

>>> I'd consider wrapping some of the logic. I'd check the port first, then
>>> move the host resolution stuff into a function.

>> Don't really see the value of either ...

> What I see is that the host logic is rather lengthy and specific (it 
> depends on the connection type -- name, ip, socket including a so nice 
> #ifdef), distinct from the port stuff which is dealt with quite quickcly.
> By separating the port & host and putting the lengthy stuff in a function 
> the scope of the for loop on potential connections would be easier to read 
> and understand, and would not require to see CHT_ cases to understand what 
> is being done for managing "host" variants, which is a mere detail.

I still don't see the value of it.  This code was inlined into the calling
function before, and it still is with this patch --- just a different
calling function.  Maybe there's an argument for a wholesale refactoring
of PQconnectPoll into smaller pieces, but that's not a task for this patch
either.  (I'm not really convinced it'd be an improvement anyway, at least
not enough of one to justify creating so much back-patching pain.)

			regards, tom lane

  15. Re: libpq should not look up all host addresses at once

    Garick Hamlin <ghamlin@isc.upenn.edu> — 2018-08-14T19:18:32Z

    On Tue, Aug 14, 2018 at 12:24:32PM +0200, Fabien COELHO wrote:
> 
> Hello Tom,
> 
> >>As you noted in another message, a small doc update should be needed.
> >
> >Check.  Proposed doc patch attached.  (Only the last hunk is actually
> >specific to this patch, the rest is cleanup that I noticed while looking
> >around for possibly-relevant text.)
> 
> Doc build is ok.
> 
> Some comments that you may not find all useful, please accept my apology, it
> just really shows that I read your prose in some detail:-)
> 
> The mention of possible reverse dns queries has been removed... but I do not
> think there was any before? There could be if only hostaddr is provided but
> a hostname is required by some auth, but it does not seem to be the case
> according to the documentation.
> 
> I read the rational of the host/hostaddr artificial mapping. I cannot say
> I'm thrilled with the result: I do not really see a setting where avoiding a
> DNS query is required but which still needs a hostname for auth... If you
> have GSSAPI or SSPI then you have an underlying network, in which a dns
> query should be fine.

FWIW, I think this is useful even it will be uncommon to use.  I run
some HA services here and I find I use this kind of functionality all
the time to test if a standby node functioning properly.  openssh 
GSSAPIServerIdentity does this.  curl does this via '--resolve'.  In
both cases one can check the name authenticates properly via TLS or
GSSAPI while connecting to an IP that is not production.  

The IP might float via VRRP or EIP in AWS, or it might be a service
local OOB network and the frontend might be a load balancer like haproxy.

FWIW, I am not using this for PG today, but this kind of feature is
definitely nice to have for alarming and HA.  It lets proper analysis
happen.  This way not everyone to be called when the local DNS resolver
fails and just the DNS-people can get the 2am call.

Anyway, if it's not a big burden, I suggest you keep it, IIUC.
This kind of thing is really handy especially since today's cloudy-stuff
means one often gets all-the-nat whether one wants it or not.

Garick

  16. Re: libpq should not look up all host addresses at once

    Nico Williams <nico@cryptonector.com> — 2018-08-14T20:18:03Z

    On Tue, Aug 14, 2018 at 03:18:32PM -0400, Garick Hamlin wrote:
> On Tue, Aug 14, 2018 at 12:24:32PM +0200, Fabien COELHO wrote:
> > I read the rational of the host/hostaddr artificial mapping. I cannot say
> > I'm thrilled with the result: I do not really see a setting where avoiding a
> > DNS query is required but which still needs a hostname for auth... If you
> > have GSSAPI or SSPI then you have an underlying network, in which a dns
> > query should be fine.
> 
> FWIW, I think this is useful even it will be uncommon to use.  I run
> some HA services here and I find I use this kind of functionality all
> the time to test if a standby node functioning properly.  openssh 
> GSSAPIServerIdentity does this.  curl does this via '--resolve'.  In
> both cases one can check the name authenticates properly via TLS or
> GSSAPI while connecting to an IP that is not production.  

+1

curl's --resolve is a fantastic diagnostic tool.  I wish it also allowed
changing the destination port as well.

While I'm at it, I strongly prefer using postgresql: URIs to any other
way to specify connect info, and I think PG should do more to encourage
their use -- perhaps even deprecating the alternatives.

Nico
--

  17. Re: libpq should not look up all host addresses at once

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-08-14T20:53:48Z

    Hello Garick,

>> I read the rational of the host/hostaddr artificial mapping. I cannot say
>> I'm thrilled with the result: I do not really see a setting where avoiding a
>> DNS query is required but which still needs a hostname for auth... If you
>> have GSSAPI or SSPI then you have an underlying network, in which a dns
>> query should be fine.
>
> FWIW, I think this is useful even it will be uncommon to use.  I run
> some HA services here and I find I use this kind of functionality all
> the time to test if a standby node functioning properly.

Ok, I understand that you want to locally override DNS results for 
testing purposes. Interesting use case.

> Anyway, if it's not a big burden, I suggest you keep it, IIUC.

I would not suggest to remove the feature, esp if there is a reasonable 
use case.

> This kind of thing is really handy especially since today's cloudy-stuff
> means one often gets all-the-nat whether one wants it or not.

Yep, NAT, another possible use case.

So it is a useful feature for specialized use cases! I just lack 
imagination:-) Thanks for the explanations.

Maybe I'll try to just improve the the documentation.

-- 
Fabien.

  18. Re: libpq should not look up all host addresses at once

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-08-15T07:07:05Z

    Hello,

>>>> I'd consider [...]
>>> Don't really see the value of either ...
>> What I see is [...].
> I still don't see the value of it.  [...].

Ok. I suggested to consider, you considered and rejected, fine with me!

-- 
Fabien.