Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(iroh): Parse DNS answer with multiple records into a proper NodeAddr #3104

Merged
merged 7 commits into from
Jan 8, 2025

Conversation

matheus23
Copy link
Contributor

@matheus23 matheus23 commented Jan 8, 2025

Description

Running iroh-doctor this morning with flub I wasn't able to connect. Flub was on iroh 0.30, I was on iroh main (commit c650ea8).
I noticed this in the logs:

2025-01-08T08:09:59.939435Z DEBUG connect{me=cabd2a9e0b alpn="n0/drderp/1"}:discovery{me=cabd2a9e0b node=1992d53c02}: hickory_proto::error: response: ; header 65474:RESPONSE:RD,RA:NoError:QUERY:3/0/0
; query
;; _iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link. IN TXT
; answers 3
_iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link. 30 IN TXT addr=192.168.96.145:60165
_iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link. 30 IN TXT addr=213.208.157.87:60165
_iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link. 30 IN TXT relay=https://euw1-1.relay.iroh.network./
; nameservers 0
; additionals 0

2025-01-08T08:09:59.939617Z DEBUG connect{me=cabd2a9e0b alpn="n0/drderp/1"}:discovery{me=cabd2a9e0b node=1992d53c02}: iroh::discovery: discovery: new address found provenance=dns addr=NodeAddr { node_id: PublicKey(1992d53c02cdc04566e5c0edb1ce83305cd550297953a047a445ea3264b54b18), relay_url: None, direct_addresses: {192.168.96.145:60165} }
[...]
2025-01-08T08:10:09.545100Z DEBUG ep{me=cabd2a9e0b}:magicsock:actor:stayin_alive{node=1992d53c02}: iroh::magicsock::node_map::node_state: can not send call-me-maybe, no relay URL

"no relay URL"? Even though the DNS record clearly contains one.

From the logs it showed that the DiscoveryItem didn't contain all information from the DNS answer.

This PR adds a test that initially failed - it only parsed the first TXT record out of the DNS answer, because of a call to records.all, which drained the iterator of all remaining items.

It also fixes this bug by avoiding a call to .all.

Breaking Changes

None.

Notes & open questions

I tried to avoid functional changes besides the bugfix. But, all of this parsing code makes me think of Postel's Law / the robustness principle: Instead of erroring out when we see conflicting information - should we filter out non-matching answer records?
There seems to be some difference between from_pkarr_signed_packet and from_hickory_records: The former is very lenient in what it parses, the latter is very strict and opts to error out when something's amiss.

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

@matheus23 matheus23 self-assigned this Jan 8, 2025
Copy link

github-actions bot commented Jan 8, 2025

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3104/docs/iroh/

Last updated: 2025-01-08T15:58:04Z

Copy link

github-actions bot commented Jan 8, 2025

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 695a197

@matheus23 matheus23 marked this pull request as ready for review January 8, 2025 10:27
@dignifiedquire
Copy link
Contributor

The former is very lenient in what it parses, the latter is very strict and opts to error out when something's amiss.

the reasons is likely, that we "know" what we expect to publish here, but I agree we probably want to be more relaxed here, and just log invalid entries

Comment on lines -326 to -329
ensure!(
&records.all(|(n, _)| n == node_id),
"invalid DNS answer: all _iroh txt records must belong to the same node domain"
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This records.all call was the culprit: It takes &mut self, and records is an impl Iterator, so it actually drains all remaining items.

Then below, the empty iterator is .chained with the first item.

@matheus23
Copy link
Contributor Author

I verified that this fixes the issue connecting between me and flub using iroh-doctor again.

@matheus23 matheus23 added this to the v0.31.0 milestone Jan 8, 2025
@matheus23 matheus23 enabled auto-merge January 8, 2025 13:22
@matheus23 matheus23 added this pull request to the merge queue Jan 8, 2025
Merged via the queue into main with commit 024ab7f Jan 8, 2025
25 of 26 checks passed
@dignifiedquire dignifiedquire deleted the matheus23/records-parse-fix branch January 8, 2025 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants