DNS lookup should try to avoid IPv6 link-local addresses

[vbraun]

What is the problem this feature will solve?

Naive calls to createServer happily resolve host names to link-local addresses, which then fail to listen:

$ node -e "net.createServer(c=>console.log).listen(5555,'zen')"
node:events:492
      throw er; // Unhandled 'error' event
      ^

Error: listen EINVAL: invalid argument fe80::2d8:xxxx:xxxx:xxxx:5555
    at Server.setupListenHandle [as _listen2] (node:net:1855:21)
    at listenInCluster (node:net:1920:12)
    at GetAddrInfoReqWrap.doListen [as callback] (node:net:2069:7)
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:109:8)
Emitted 'error' event on Server instance at:
    at emitErrorNT (node:net:1899:8)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  code: 'EINVAL',
  errno: -22,
  syscall: 'listen',
  address: 'fe80::2d8:xxxx:xxxx:xxxx',
  port: 5555
}

Node.js v20.10.0

What is the feature you are proposing to solve the problem?

DNS lookup randomly picks the first available entry

> dns.lookup('zen', { verbatim: true }, console.info)
GetAddrInfoReqWrap {
  callback: [Function: info],
  family: 0,
  hostname: 'zen',
  oncomplete: [Function: onlookup]
}
> null fe80::2d8:xxxx:xxxx:xxxx 6

if there are multiple addresses returned by getaddrinfo

> dns.lookup('zen', { verbatim: true, all: true }, console.info)
GetAddrInfoReqWrap {
  callback: [Function: info],
  family: 0,
  hostname: 'zen',
  oncomplete: [Function: onlookupall]
}
> null [
  { address: 'fe80::2d8:xxxx:xxxx:xxxx', family: 6 },
  { address: '2a02:8109:xxxx:xxxx:2d8:xxxx:xxxx:xxxx', family: 6 },
  { address: '192.168.178.3', family: 4 },
  { address: '192.168.122.1', family: 4 }

Instead, it should pick the first non-link-local address. It is extremely unlikely that you want to listen to a link-local address (starting with fe80::). And in the unlikely case that you do, you certainly would call with options.all to get all resolutions.

What alternatives have you considered?

We could alter all client code to always add options.all and filter, but it seems like node is the correct way to fix this once and for all.

Related issues

• https://github.com/webpack/webpack-dev-server/issues/4630

[ryker-uptycs]

I wish to take this up

[marco-ippolito]

I guess it can be fixed with something like this:

diff --git a/lib/dns.js b/lib/dns.js
index 681f0aa3e5..de5bb73d39 100644
--- a/lib/dns.js
+++ b/lib/dns.js
@@ -24,6 +24,7 @@
 const {
   ObjectDefineProperties,
   ObjectDefineProperty,
+  StringPrototypeStartsWith,
   Symbol,
 } = primordials;
 
@@ -127,6 +128,14 @@ function onlookupall(err, addresses) {
     };
   }
 
+  if (addresses.length > 1) {
+    const firstAddr = addresses[0];
+    // Skip ipv6 link local address
+    if (firstAddr.family === 6 && StringPrototypeStartsWith(firstAddr.address, 'fe80::')) {
+      addresses.push(addresses.shift());
+    }
+  }
+
   this.callback(null, addresses);
   if (this[kPerfHooksDnsLookupContext] && hasObserver('dns')) {
     stopPerf(this, kPerfHooksDnsLookupContext, { detail: { addresses } });

[wathika-eng]

Guess this is my error

[puskin94]

I am willing to work on this, but I am a little confused about one aspect:

should I remove ALL the local IPV6 links from addresses in onlookupall or just the first one, if present?

[vbraun]

You often have more than one ipv6 address, this is pretty normal. So all link-local ones should be skipped, not just the first one.

[puskin94]

You often have more than one ipv6 address, this is pretty normal. So all link-local ones should be skipped, not just the first one.

that's what I thought as well, but I got confused by the examples 😄
Thanks! working on it then

[puskin94]

I am struggling with automated tests:

the function is working fine, but I noticed that the DNS resolution is actually real and not mocked anywhere (at least I could not find it anywhere in this huge codebase).
Due to this, I would have to create an entry in the runner's /etc/hosts with a little something like this:

fe80::1         zen

and, of course, this is not doeable or even a good idea :)

the test looks something like this:

TEST(function test_lookup_ip_all_promise(done) {
  const req = util.promisify(dns.lookup)('zen', { all: true, family: 6 })
    .then(function(ips) {
      assert.ok(Array.isArray(ips));
      assert.ok(ips.length === 1);
      assert.strictEqual(ips[0], 'fe80::1');

      done();
    });

  checkWrap(req);
});

the best solution would be to mock the call to const req = new GetAddrInfoReqWrap(); but not seeing something similar anywhere else I am wondering if there isn't a better solution?

Thanks in advance!