p2p: Use absolute FQDN for DNS seed domains #23268

ghost · 2021-10-12T15:35:24Z

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)

ghost · 2021-10-12T15:35:52Z

@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.

luke-jr

utACK

luke-jr · 2021-10-12T19:58:26Z

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).

kristapsk

utACK 04ef501

practicalswift · 2021-10-13T08:08:56Z

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.

laanwj · 2021-10-13T10:59:26Z

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.

ghost · 2021-10-13T16:37:07Z

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

practicalswift · 2021-10-14T09:37:05Z

@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) :)

practicalswift · 2021-10-14T11:09:15Z

@prayank23

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

ghost · 2021-10-14T11:59:42Z

@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.

practicalswift · 2021-10-14T13:04:24Z

cr ACK ca2c313

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

laanwj · 2021-10-15T12:28:20Z

code review ACK ca2c313

promag

Code review ACK ca2c313.

ca2c313 Use absolute FQDN for DNS seed domains (Prayank) Pull request description: Fixes bitcoin#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: bitcoin#23193 (comment) ACKs for top commit: practicalswift: cr ACK ca2c313 laanwj: code review ACK ca2c313 promag: Code review ACK ca2c313. Tree-SHA512: 9818227332282a78c45b4470c2fc80bf899ed78aed76644ebf014e0fff1b139402ea023acdea162363a478b6f6613dbf1da57e214d2240ea0f04310473f57cca

…itcoin#23268, bitcoin#23287, bitcoin#23314, bitcoin#23320, bitcoin#23323, bitcoin#23341 ea107ac Merge bitcoin#23320: rpc: Add RPC help for getblock verbosity level 3 (W. J. van der Laan) 6df2fae Merge bitcoin#23341: RPC: Better safety with newkeypool command and wallet backups (Andrew Chow) 910f4c0 Merge bitcoin#23093: Add ability to flush keypool and always flush when upgrading non-HD to HD (W. J. van der Laan) cf6e969 Merge bitcoin#22539: Re-include RBF replacement txs in fee estimation (W. J. van der Laan) 5c226da Merge bitcoin#23323: doc: Add note on deleting past-EOL release branches (MarcoFalke) bf28e85 Merge bitcoin#23287: test: get and decode tx with a single `gettransaction` RPC call (MarcoFalke) 6f46f58 Merge bitcoin#22918: rpc: Add level 3 verbosity to getblock RPC call (bitcoin#21245 modified) (W. J. van der Laan) 4e8f6af Merge bitcoin#23268: p2p: Use absolute FQDN for DNS seed domains (fanquake) 135d650 Merge bitcoin#23314: build: explicitly disable libsecp256k1 openssl based tests (W. J. van der Laan) Pull request description: ## What was done? Just regular backports from Bitcoin Core v23. ## How Has This Been Tested? Run unit / functional tests ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK ea107ac Tree-SHA512: df36fc867d533366c7f46903e018048fc9f64ef01960e1d2338195f00203fe6a9c5ac9bd13120b949015846d10cf4969853990b6c6023fbf6f4c19e716747889

DrahtBot added P2P Validation labels Oct 12, 2021

luke-jr approved these changes Oct 12, 2021

View reviewed changes

luke-jr mentioned this pull request Oct 12, 2021

p2p: Use absolute FQDN for DNS seeds bitcoinknots/bitcoin#40

Closed

kristapsk approved these changes Oct 12, 2021

View reviewed changes

maflcko removed the Validation label Oct 13, 2021

ghost closed this Oct 14, 2021

Use absolute FQDN for DNS seed domains

ca2c313

ghost reopened this Oct 14, 2021

promag reviewed Oct 15, 2021

View reviewed changes

fanquake merged commit feedb9c into bitcoin:master Oct 16, 2021

tylerchambers mentioned this pull request Jan 15, 2022

chaincfg: Use FQDN for DNS seed domain names btcsuite/btcd#1789

Closed

ghost mentioned this pull request Jan 17, 2022

[22.x] Backports for 22.x #23276

Merged

delta1 added a commit to delta1/elements that referenced this pull request May 11, 2023

Merge feedb9c into merged_master (Bitcoin PR bitcoin/bitcoin#23268)

a5f6936

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

p2p: Use absolute FQDN for DNS seed domains #23268

p2p: Use absolute FQDN for DNS seed domains #23268

Uh oh!

ghost commented Oct 12, 2021

Uh oh!

ghost commented Oct 12, 2021

Uh oh!

luke-jr left a comment

Uh oh!

luke-jr commented Oct 12, 2021

Uh oh!

kristapsk left a comment

Uh oh!

practicalswift commented Oct 13, 2021

Uh oh!

laanwj commented Oct 13, 2021 •

edited

Loading

Uh oh!

ghost commented Oct 13, 2021

Uh oh!

practicalswift commented Oct 14, 2021 •

edited

Loading

Uh oh!

practicalswift commented Oct 14, 2021

Uh oh!

ghost commented Oct 14, 2021

Uh oh!

practicalswift commented Oct 14, 2021 •

edited

Loading

Uh oh!

laanwj commented Oct 15, 2021

Uh oh!

promag left a comment

Uh oh!

Uh oh!

p2p: Use absolute FQDN for DNS seed domains #23268

p2p: Use absolute FQDN for DNS seed domains #23268

Uh oh!

Conversation

ghost commented Oct 12, 2021

Uh oh!

ghost commented Oct 12, 2021

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Oct 12, 2021

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Oct 13, 2021

Uh oh!

laanwj commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 13, 2021

Uh oh!

practicalswift commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Oct 14, 2021

Uh oh!

ghost commented Oct 14, 2021

Uh oh!

practicalswift commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 15, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented Oct 13, 2021 •

edited

Loading

practicalswift commented Oct 14, 2021 •

edited

Loading

practicalswift commented Oct 14, 2021 •

edited

Loading