dns: exclude loopback link from the single lookup resolution

[puskin94]

dns: the lookup of a single address should not be returned if the result is a loopback link

Fixes: #51732

As described in the original issue, I am having problems writing working automated tests:

if /etc/hosts is modified to have this entry fe80::1 ipv6_loopback tests are working just fine, but of course it is not a valid solution. I tried to mock the inner workings of the actual dns resolved but I was not able to make it work fine for my test case.

two questions:

1. does anybody know how to fix the "broken" test?
2. are the new API (the returned values) fine?

[nodejs-github-bot]

Review requested:

• @nodejs/net

[codecov]

Codecov Report

Attention: Patch coverage is 71.42857% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.10%. Comparing base (e0634f5) to head (1400cbd).
Report is 2 commits behind head on main.

Files	Patch %	Lines
lib/dns.js	72.72%	2 Missing and 1 partial ⚠️
lib/internal/dns/promises.js	70.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54249      +/-   ##
==========================================
- Coverage   87.10%   87.10%   -0.01%     
==========================================
  Files         647      647              
  Lines      181739   181758      +19     
  Branches    34887    34889       +2     
==========================================
+ Hits       158310   158318       +8     
- Misses      16738    16741       +3     
- Partials     6691     6699       +8     

Files	Coverage Δ
lib/dns.js	97.91% <72.72%> (-0.75%)	⬇️
lib/internal/dns/promises.js	97.61% <70.00%> (-0.68%)	⬇️

... and 28 files with indirect coverage changes

[mcollina]

How is the new API different from
the old one?

[puskin94]

How is the new API different from the old one?

if the result of the single lookup is a loopback address:

before:

const response = await dnsPromises.lookup('ipv6_loopback', { verbatim: true });
console.log(response); // { address: 'fe80::1', family: 6 }

util.promisify(dns.lookup)('ipv6_loopback').then(response => {
    console.log(response); // { address: 'fe80::1', family: 6 }
});

after:

const response = await dnsPromises.lookup('ipv6_loopback', { verbatim: true });
console.log(response); // { }

util.promisify(dns.lookup)('ipv6_loopback').then(response => {
    console.log(response); // { }
});

to be honest, considering we are completely filtering out any loopback address in this case, and the result being empty, I would be more in favor of returning a ENOTFOUND , like the address cannot be resolved at all

[pimterry]

Thanks for the explanation and for working on this @puskin94. I don't think this is the correct behaviour quite yet though. This technically solves #51732, but it will cause lots of other issues, as it seems to make it impossible to look up DNS names for link-local addresses.

There's two specific things I think we should still support:

• If there is a hostname that resolves to a single link-local IPv6 IP then it should still be possible to resolve that.
• If there is a hostname that resolves to multiple IPs including some link-local results, it should definitely still be possible to get the full list when passing all: true, including those results.

This PR currently breaks both of those I think. A hostname resolving to these IPs is unusual, but AFAICT it's not invalid in any sense from a DNS perspective, so we shouldn't pretend it doesn't exist. We shouldn't return ENOTFOUND either - it should be possible to still look up these addresses for other use cases.

To resolve #51732, we just need to ensure that that when you call server.listen(port, hostname) the server prefers non-link-local addresses for the hostname somehow, if there is one available. Blocking all those addresses from lookups technically works, but there are better approaches. I can see two plausible options:

• If you call dns.lookup (without all: true, to get just one value) and there are multiple values, maybe the selection logic should change to prefer non-link-local IPs.
• Alternatively, maybe we shouldn't change DNS at all, and instead this should just be a change within net.listen to look up all addresses for the given hostname and prefer non-link-local IPs when there is one available (probably best to still try to use the link-local IP if it's the only IP returned, I think? Just because it makes the error message in the failure case a little clearer).

That first option is very likely a breaking change (although it's a bit unclear how consistent the current DNS result selection is anyway) and the latter is probably not (since it sounds like this currently always fails anyway?) but both will need some thought to check there's no other consequences. Changing the server listen logic is the cleaner/preferable choice I think, assuming there's no issues with that, unless we have a good reason to be sure that this preference would be useful in DNS lookup selection in most other contexts.

Does that make sense?

[puskin94]

there's no other consequences. Changing the server listen logic is the cl

Hi @pimterry ! Thanks for the super detailed explaination 😃
I aree with you on pretty much everything, but I have 2 points to make:

1. you said that changing the server.listen is probably not a breaking change, while I think it is, we might have people out there relying on this exact behavior right now, we just don't know
2. if I manage to change server.listen's API (this is my second contribution to Node, I am still getting lost in the humongous codebase) it will not directly solve the reported issue. We actually don't know what the OP wanted to do and he directly reported that dns.lookup was returning a loopback address, we are not sure he needed that to be used with server.listen

All this said, I agree with both your domain analysis and implementation specs 😄
I will start working on it ASAP!

Thanks again 😃

[pimterry]

you said that changing the server.listen is probably not a breaking change, while I think it is, we might have people out there relying on this exact behavior right now, we just don't know

Yes, it's not totally clear, it does need some thought & testing.

If server.listen on a link-local address does always fail, then I think fixing this is probably not a breaking change. We don't formally define the hostname preference order anywhere anyway (AFAICT) and I would hope that nobody's code is depending on intentionally starting servers that will intentionally fail to start, but only because the hostname has two IPs but the first one is link-local. In practice, I think anything that currently listens to a hostname like this is broken right now (i.e. the failure is a bug, and shouldn't happen) and so this is just a bugfix.

That's just if listening on link-local addresses does indeed always fail. I haven't intentionally ever used them in practice myself though, so that needs confirmation. If there are practical cases in Node where it does work today, then we can still make the change if it's clearly what most people would expect (for a hostname with a public + link-local IP, I think that's true) but in that case it's definitely a breaking change.

We actually don't know what the OP wanted to do and he directly reported that dns.lookup was returning a loopback address, we are not sure he needed that to be used with server.listen

True - I just don't think that DNS returning that address is necessarily wrong. In some use cases it's weird and won't work, but it's not entirely invalid, and the world of computers contains plenty of weird software that might want to actually use these addresses 😄.

The server.listen case however seems to be the underlying motivation, and that case does seem relatively clear cut - if you specify a hostname to listen on, and it has both a public and a link-local address, then I think you do almost certainly want to listen on the public address (@vbraun feel free to chime in with more context if you have thoughts on this).

[puskin94]

@pimterry amazing! thanks for being so thorough with your answers :)

The PR is almost ready, the code is working and new (and old) automated tests are passing, just refining a couple of things and then I will sumbmit it. I will close this one in favor of the other one because the scope (and how it is achieved) is completely different.

Btw, about your

If server.listen on a link-local address does always fail, then I think fixing this is probably not a breaking change.

It looks like it is the case, and this new test confirms it:

[puskin94]

closing in favor of #54264