p2p: Use absolute FQDN for DNS seed domains #23268

Fixes #23193 by using absolute FQDN for domains used by DNS seeds.

It improves security and should not break anything based on my research and testing. Few things about absolute FQDN are shared in https://superuser.com/questions/1467958/why-does-putting-a-dot-after-the-url-remove-login-information

Master branch:

DNS seed x9.dnsseed.bitcoin.dashjr.org. responded with IP 127.0.0.1

PR branch:

DNS seed x9.dnsseed.bitcoin.dashjr.org. responded with IP 159.89.108.149

Reviewers can follow the steps mentioned in Issue to test: #23193 (comment)

@practicalswift did all the research and helped me with testing. There was weird behavior with Fedora which is explained in #23193 (comment)

I just created this PR and added dots in the domains used by DNS seeds.

utACK

You might consider rewriting this by appending the final period in ThreadDNSAddressSeed, rather than expecting the chainparams to have it for each entry. That seems safe and more mistake-resistant (eg, you forgot the testnet DNS seeds).

utACK 04ef501ae030df490d419e3cf97b2dd34f2ec655

Concept ACK on fixing this issue @prayank23, you'll have to address all instances of git grep -E 'vSeeds.emplace_back\("[^"]+[a-z]"\);' if you want to fix this by taking the "add dots to vSeeds" route :)

I see three different approaches to fixing this:

• Specify seed node strings as FQDNs by appending . to the seed hostname strings in src/chainparams.cpp's vSeed.
• Change from strprintf("x%x.%s", requiredServiceBits, seed) to strprintf("x%x.%s.", requiredServiceBits, seed) when building the seed hostname in CConnman::ThreadDNSAddressSeed().
• Add a bool allow_non_fqdn argument to Lookup* functions, and automatically add a trailing . before passing the hostname string to getaddrinfo if !allow_non_fqdn. This approach would make the choice between FQDN vs "potentially non-FQDN" explicit.

Concept ACK. No strong opinion on where to add the dots. But I think there's something to be said in keeping the domain lookup functionality generic and adding the dots at the source in chainparams (in case people want to specify non-FQDN domains in the configuration for their own local use), as it is now.

• Made changes in 4 domains used by testnet DNS seeds (L233-L236)
• Added @practicalswift as co-author

@prayank23

Still not complete I'm afraid: the signet DNS seed node is not changed :)

This is the command I used locally to check the changes:

$ sed -i 's/vSeeds.emplace_back("\(.*[a-z]\)");/vSeeds.emplace_back("\1.");/g' src/chainparams.cpp

When doing the touch up you may drop me as co-author: I'm just an enthusiastic reviewer, and to not introduce confusion I think "co-author" tagging should be reserved to situations where the actual commit is written by multiple parties (such as when editing a cherry-picked commit from another author) :)

@prayank23

Looks good now. Please squash into one commit and I'll do the final review :)

@practicalswift I did something wrong with squashing. Will fix it. Signet domain wasn't there because PR branch wasn't synced with master when I created pull request.

I had added you as co-author because its a small change and was suggested by you. Will remove it if you aren't okay with it.

cr ACK ca2c313aa291ae44adc1b7148ed49125bdc77bf4

Reviewed by comparing diff with sed -i 's/vSeeds.emplace_back("\(.*[a-z]\)");/vSeeds.emplace_back("\1.");/g' src/chainparams.cpp on local tree.

code review ACK ca2c313aa291ae44adc1b7148ed49125bdc77bf4

Code review ACK ca2c313aa291ae44adc1b7148ed49125bdc77bf4.