Skip to content

Commit 024ab7f

Browse files
matheus23matheus23
authored
fix(iroh): Parse DNS answer with multiple records into a proper NodeAddr (#3104)
## Description <!-- A summary of what this pull request achieves and a rough list of changes. --> 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 <!-- Optional, if there are any breaking changes document them, including how to migrate older code. --> None. ## Notes & open questions <!-- Any notes, remarks or open questions you have to make about the PR. --> 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](https://en.wikipedia.org/wiki/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 - [x] Self-review. - [x] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [x] Tests if relevant. - [x] All breaking changes documented.
1 parent f08d560 commit 024ab7f

File tree

1 file changed

+134
-25
lines changed

1 file changed

+134
-25
lines changed

‎iroh/src/dns/node_info.rs

+134-25
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,10 @@ use std::{
4040
str::FromStr,
4141
};
4242

43-
use anyhow::{anyhow, ensure, Result};
43+
use anyhow::{anyhow, Result};
4444
use hickory_resolver::{proto::ProtoError, Name, TokioResolver};
4545
use iroh_base::{NodeAddr, NodeId, SecretKey};
46+
use tracing::warn;
4647
use url::Url;
4748

4849
/// The DNS name for the iroh TXT record.
@@ -162,9 +163,9 @@ impl NodeInfo {
162163
self.into()
163164
}
164165

165-
/// Parses a [`NodeInfo`] from a set of DNS records.
166-
pub fn from_hickory_records(records: &[hickory_resolver::proto::rr::Record]) -> Result<Self> {
167-
let attrs = TxtAttrs::from_hickory_records(records)?;
166+
/// Parses a [`NodeInfo`] from a TXT records lookup.
167+
pub fn from_hickory_lookup(lookup: hickory_resolver::lookup::TxtLookup) -> Result<Self> {
168+
let attrs = TxtAttrs::from_hickory_lookup(lookup)?;
168169
Ok(attrs.into())
169170
}
170171

@@ -258,7 +259,7 @@ impl<T: FromStr + Display + Hash + Ord> TxtAttrs<T> {
258259
async fn lookup(resolver: &TokioResolver, name: Name) -> Result<Self> {
259260
let name = ensure_iroh_txt_label(name)?;
260261
let lookup = resolver.txt_lookup(name).await?;
261-
let attrs = Self::from_hickory_records(lookup.as_lookup().records())?;
262+
let attrs = Self::from_hickory_lookup(lookup)?;
262263
Ok(attrs)
263264
}
264265

@@ -311,25 +312,45 @@ impl<T: FromStr + Display + Hash + Ord> TxtAttrs<T> {
311312
Self::from_strings(node_id, txt_strs)
312313
}
313314

314-
/// Parses a set of DNS resource records.
315-
pub fn from_hickory_records(records: &[hickory_resolver::proto::rr::Record]) -> Result<Self> {
316-
use hickory_resolver::proto::rr;
317-
let mut records = records.iter().filter_map(|rr| match rr.data() {
318-
rr::RData::TXT(txt) => {
319-
node_id_from_hickory_name(rr.name()).map(|node_id| (node_id, txt))
315+
/// Parses a TXT records lookup.
316+
pub fn from_hickory_lookup(lookup: hickory_resolver::lookup::TxtLookup) -> Result<Self> {
317+
let queried_node_id = node_id_from_hickory_name(lookup.query().name())
318+
.ok_or_else(|| anyhow!("invalid DNS answer: not a query for _iroh.z32encodedpubkey"))?;
319+
320+
let strings = lookup.as_lookup().record_iter().filter_map(|record| {
321+
match node_id_from_hickory_name(record.name()) {
322+
// Filter out only TXT record answers that match the node_id we searched for.
323+
Some(n) if n == queried_node_id => match record.data().as_txt() {
324+
Some(txt) => Some(txt.to_string()),
325+
None => {
326+
warn!(
327+
?queried_node_id,
328+
data = ?record.data(),
329+
"unexpected record type for DNS discovery query"
330+
);
331+
None
332+
}
333+
},
334+
Some(answered_node_id) => {
335+
warn!(
336+
?queried_node_id,
337+
?answered_node_id,
338+
"unexpected node ID answered for DNS query"
339+
);
340+
None
341+
}
342+
None => {
343+
warn!(
344+
?queried_node_id,
345+
name = ?record.name(),
346+
"unexpected answer record name for DNS query"
347+
);
348+
None
349+
}
320350
}
321-
_ => None,
322351
});
323-
let (node_id, first) = records.next().ok_or_else(|| {
324-
anyhow!("invalid DNS answer: no TXT record with name _iroh.z32encodedpubkey found")
325-
})?;
326-
ensure!(
327-
&records.all(|(n, _)| n == node_id),
328-
"invalid DNS answer: all _iroh txt records must belong to the same node domain"
329-
);
330-
let records = records.map(|(_, txt)| txt).chain(Some(first));
331-
let strings = records.map(ToString::to_string);
332-
Self::from_strings(node_id, strings)
352+
353+
Self::from_strings(queried_node_id, strings)
333354
}
334355

335356
fn to_txt_strings(&self) -> impl Iterator<Item = String> + '_ {
@@ -405,11 +426,24 @@ fn node_domain(node_id: &NodeId, origin: &str) -> Result<Name> {
405426

406427
#[cfg(test)]
407428
mod tests {
408-
use std::str::FromStr;
409-
410-
use iroh_base::SecretKey;
429+
use std::{collections::BTreeSet, str::FromStr, sync::Arc};
430+
431+
use hickory_resolver::{
432+
lookup::Lookup,
433+
proto::{
434+
op::Query,
435+
rr::{
436+
rdata::{A, TXT},
437+
RData, Record, RecordType,
438+
},
439+
},
440+
Name,
441+
};
442+
use iroh_base::{NodeId, SecretKey};
443+
use testresult::TestResult;
411444

412445
use super::NodeInfo;
446+
use crate::dns::node_info::to_z32;
413447

414448
#[test]
415449
fn txt_attr_roundtrip() {
@@ -438,4 +472,79 @@ mod tests {
438472
let actual = NodeInfo::from_pkarr_signed_packet(&packet).unwrap();
439473
assert_eq!(expected, actual);
440474
}
475+
476+
/// There used to be a bug where uploading a NodeAddr with more than only exactly
477+
/// one relay URL or one publicly reachable IP addr would prevent connection
478+
/// establishment.
479+
///
480+
/// The reason was that only the first address was parsed (e.g. 192.168.96.145 in
481+
/// this example), which could be a local, unreachable address.
482+
#[test]
483+
fn test_from_hickory_lookup() -> TestResult {
484+
let name = Name::from_utf8(
485+
"_iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link.",
486+
)?;
487+
let query = Query::query(name.clone(), RecordType::TXT);
488+
let records = [
489+
Record::from_rdata(
490+
name.clone(),
491+
30,
492+
RData::TXT(TXT::new(vec!["addr=192.168.96.145:60165".to_string()])),
493+
),
494+
Record::from_rdata(
495+
name.clone(),
496+
30,
497+
RData::TXT(TXT::new(vec!["addr=213.208.157.87:60165".to_string()])),
498+
),
499+
// Test a record with mismatching record type (A instead of TXT). It should be filtered out.
500+
Record::from_rdata(name.clone(), 30, RData::A(A::new(127, 0, 0, 1))),
501+
// Test a record with a mismatching name
502+
Record::from_rdata(
503+
Name::from_utf8(format!(
504+
"_iroh.{}.dns.iroh.link.",
505+
to_z32(&NodeId::from_str(
506+
// Another NodeId
507+
"a55f26132e5e43de834d534332f66a20d480c3e50a13a312a071adea6569981e"
508+
)?)
509+
))?,
510+
30,
511+
RData::TXT(TXT::new(vec![
512+
"relay=https://euw1-1.relay.iroh.network./".to_string()
513+
])),
514+
),
515+
// Test a record with a completely different name
516+
Record::from_rdata(
517+
Name::from_utf8("dns.iroh.link.")?,
518+
30,
519+
RData::TXT(TXT::new(vec![
520+
"relay=https://euw1-1.relay.iroh.network./".to_string()
521+
])),
522+
),
523+
Record::from_rdata(
524+
name.clone(),
525+
30,
526+
RData::TXT(TXT::new(vec![
527+
"relay=https://euw1-1.relay.iroh.network./".to_string()
528+
])),
529+
),
530+
];
531+
let lookup = Lookup::new_with_max_ttl(query, Arc::new(records));
532+
533+
let node_info = NodeInfo::from_hickory_lookup(lookup.into())?;
534+
assert_eq!(
535+
node_info,
536+
NodeInfo {
537+
node_id: NodeId::from_str(
538+
"1992d53c02cdc04566e5c0edb1ce83305cd550297953a047a445ea3264b54b18"
539+
)?,
540+
relay_url: Some("https://euw1-1.relay.iroh.network./".parse()?),
541+
direct_addresses: BTreeSet::from([
542+
"192.168.96.145:60165".parse()?,
543+
"213.208.157.87:60165".parse()?,
544+
])
545+
}
546+
);
547+
548+
Ok(())
549+
}
441550
}

0 commit comments

Comments
 (0)