waku-org / nwaku

Waku node and protocol.
Other
188 stars 49 forks source link

Get rid of the Hardcoded DNS server addresses #1490

Open jakubgs opened 1 year ago

jakubgs commented 1 year ago

Problem

 > ag 1.0.0.1 | grep ValidIpAddress     
apps/wakubridge/config.nim:162:      defaultValue: @[ValidIpAddress.init("1.1.1.1"), ValidIpAddress.init("1.0.0.1")]
apps/wakunode2/config.nim:102:      defaultValue: @[ValidIpAddress.init("1.1.1.1"), ValidIpAddress.init("1.0.0.1")]
apps/wakunode2/config.nim:370:      defaultValue: @[ValidIpAddress.init("1.1.1.1"), ValidIpAddress.init("1.0.0.1")]
apps/chat2/config_chat2.nim:200:      defaultValue: @[ValidIpAddress.init("1.1.1.1"), ValidIpAddress.init("1.0.0.1")]
tools/wakucanary/wakucanary.nim:96:      initTAddress(ValidIpAddress.init("1.0.0.1"), Port(53))]
tools/networkmonitor/networkmonitor.nim:200:    let dnsNameServers = @[ValidIpAddress.init("1.1.1.1"), ValidIpAddress.init("1.0.0.1")]

1668621310380258

Impact

People configure DNS servers on their machines and networks for a reason. Entirely avoiding local DNS settings is a bad practice and should be avoided.

alrevuelta commented 1 year ago

Not sure I undestand the problem here. Some tools like wakucanary and networkmonitor use hardcoded dns values, so agree that a new flag for changing it would be nice to have.

But regarding wakunode2 don't see the problem. There are some default values, yes, but can be overiden by dnsDiscoveryNameServers and dnsAddrsNameServers. So no action here I guess? Or what do you mean by "Entirely avoiding local DNS settings ...". How can we use these local DNS settings besides using the mentioned flags?

not sure about the meme/image though. wouldn't like this to create precedent and end up having the repo full of them.

jakubgs commented 1 year ago

In general whatever the default on the host is should be used:

admin@node-01.do-ams3.wakuv2.prod:~ % grep -v '^#' /etc/resolv.conf
nameserver 127.0.0.53
options edns0 trust-ad
search .

Providing an explicit nameserver option or cli flag is not something that's a common practice, but is fine as long as the default is to use what the system uses.

jakubgs commented 7 months ago

Any news on this?

Ivansete-status commented 7 months ago

I suggest looking at this issue once we move from ValidIpAddress (type deprecated currently) to IpAdress to avoid conflicts.

Thereafter, we would need to apply something like the following, for Linux environments (need to check for Windows and MacOS)

import os

proc getDNSServers(): seq[string] =
  var dnsServers: seq[string] = @[]
  let resolvConf = "/etc/resolv.conf"

  if existsFile(resolvConf):
    let resolvContent = readFile(resolvConf)
    for line in resolvContent.splitLines():
      if line.strip.startswith("nameserver"):
        let parts = line.strip.split(' ')
        if parts.len >= 2:
          dnsServers.add(parts[1])

  return dnsServers

( cc @jakubgs )

jakubgs commented 7 months ago

Do we really need special logic for this? Don't calls like getaddrinfo or gethostbyname(deprecated) already use system DNS by default by reading /etc/resolv.conf? Seems like overcomplicating it to me.