From d876e94823f3f578e59ff88d1e65e7aeee4129e0 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Wed, 2 Sep 2026 22:06:37 -0400 Subject: [PATCH 1/3] fix(didbot-dns): refuse a host more than one label below the wildcard zone WildcardDns::accepts used ends_with, so any depth under the zone was accepted -- a.b.zone as readily as a.zone -- but a deployment answers the zone with a single-label wildcard DNS record, and RFC 1034 wildcards match exactly one label. A deeper host would publish or mint something no request could ever route to. Mirrors the same-shaped fix already made to Provisioner::check_requested_handle in crates/didbot-pds. Mutation-tested: reverting accepts() to the old ends_with check lets the new test's over-deep host publish successfully; restoring it refuses with NotUnderZone. Co-Authored-By: Claude Opus 5 (1M context) Change-Id: Ibecee1d824531823256808ff0383e8a2760667f9 --- crates/didbot-dns/src/lib.rs | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/crates/didbot-dns/src/lib.rs b/crates/didbot-dns/src/lib.rs index 6602fe6a..9983000f 100644 --- a/crates/didbot-dns/src/lib.rs +++ b/crates/didbot-dns/src/lib.rs @@ -897,8 +897,26 @@ impl WildcardDns { } /// Whether `host` is one this provider will accept. + /// + /// A deployment answers this zone with a single-label wildcard DNS + /// record (`*.`), and RFC 1034 wildcards match exactly one label. + /// So `host` must be the zone itself (the apex, for `_acme-challenge` + /// and the like) or exactly one label below it — never two or more. + /// `foo.bar.` would mint or publish something no wildcard record + /// could ever route a request to, unreachable rather than merely + /// unpublished. + /// + /// [`hostname_is_at_or_below`] alone would accept any depth, so this + /// pairs it with a label-count check the same way + /// `Provisioner::check_requested_handle` does for a caller-asserted + /// handle. pub fn accepts(&self, host: &str) -> bool { - host == self.zone || host.ends_with(&format!(".{}", self.zone)) + if !hostname_is_at_or_below(host, &self.zone) { + return false; + } + let zone_labels = self.zone.matches('.').count() + 1; + let host_labels = host.matches('.').count() + 1; + host_labels <= zone_labels + 1 } fn check(&self, host: &str) -> Result<(), DnsError> { @@ -1465,6 +1483,22 @@ mod tests { ); } + #[test] + fn wildcard_refuses_a_host_more_than_one_label_below_the_zone() { + // A deployment answers this zone with a single-label wildcard + // record; RFC 1034 wildcards match exactly one label, so this + // should be refused the same way a host outside the zone is, + // rather than accepted the way `ends_with` would. + let dns = WildcardDns::new("agents.example.com".to_string()); + assert_eq!( + dns.publish("deep.agent.agents.example.com", &RecordTarget::Loopback), + Err(DnsError::NotUnderZone { + host: "deep.agent.agents.example.com".to_string() + }) + ); + assert!(dns.published().is_empty()); + } + #[test] fn wildcard_refuses_a_host_outside_the_zone() { let dns = WildcardDns::new("agents.example.com".to_string()); -- 2.51.2 From e11d4b7b82d222fa0b17c03026b77d7c3edfa344 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Wed, 2 Sep 2026 22:13:37 -0400 Subject: [PATCH 2/3] fix(didbot-dns): key DNS bookkeeping on the case-folded hostname Records, TxtRecords and CaaRecords stored the exact string a caller passed, so "A.example" and "a.example" -- one DNS name per RFC 1035 3.1 -- landed as two separate map entries. A second publish under a different-case spelling of an already-published host succeeded instead of colliding, each caller believing it alone owned the name. Also adds coverage for the zone match itself being case-insensitive, and for a trailing-dot host correctly failing closed rather than matching by accident. Mutation-tested: written before the fix, the new collision test failed with a spurious Ok(()); it passes once every map key runs through the shared normalize_host. Co-Authored-By: Claude Opus 5 (1M context) Change-Id: I28bd755014c43cf406a77a4cbcdc152b4309ca41 --- crates/didbot-dns/src/lib.rs | 86 +++++++++++++++++++++++++++++++----- 1 file changed, 74 insertions(+), 12 deletions(-) diff --git a/crates/didbot-dns/src/lib.rs b/crates/didbot-dns/src/lib.rs index 9983000f..2eb96074 100644 --- a/crates/didbot-dns/src/lib.rs +++ b/crates/didbot-dns/src/lib.rs @@ -396,6 +396,17 @@ impl DnsProvider for Box { #[derive(Debug, Default)] struct Records(Mutex>); +/// Folds `host` to the form every bookkeeping map keys on. +/// +/// RFC 1035 3.1 makes DNS name comparison case-insensitive: `A.example` and +/// `a.example` are one name. Every map in this module is keyed on the +/// lowercased spelling so that two callers racing on different-case +/// spellings of the same host collide the same way two identical spellings +/// do, instead of each believing it alone published a now-ambiguous name. +fn normalize_host(host: &str) -> String { + host.to_ascii_lowercase() +} + impl Records { /// Takes the lock, recovering from a poisoned mutex. /// @@ -410,19 +421,20 @@ impl Records { } fn insert(&self, host: &str, target: &RecordTarget) -> Result<(), DnsError> { + let key = normalize_host(host); let mut map = self.map(); - if map.contains_key(host) { + if map.contains_key(&key) { return Err(DnsError::AlreadyPublished { host: host.to_owned(), }); } - map.insert(host.to_owned(), target.clone()); + map.insert(key, target.clone()); Ok(()) } fn remove(&self, host: &str) -> Result<(), DnsError> { self.map() - .remove(host) + .remove(&normalize_host(host)) .map(|_| ()) .ok_or_else(|| DnsError::NotPublished { host: host.to_owned(), @@ -434,7 +446,7 @@ impl Records { } fn target(&self, host: &str) -> Option { - self.map().get(host).cloned() + self.map().get(&normalize_host(host)).cloned() } } @@ -464,7 +476,7 @@ impl TxtRecords { /// caller can skip a redundant network write on a retried publish. fn insert(&self, host: &str, value: &str) -> bool { self.map() - .entry(host.to_owned()) + .entry(normalize_host(host)) .or_default() .insert(value.to_owned()) } @@ -474,7 +486,8 @@ impl TxtRecords { /// [`Self::values`]. fn remove(&self, host: &str, value: &str) -> Result<(), DnsError> { let mut map = self.map(); - let Some(values) = map.get_mut(host) else { + let key = normalize_host(host); + let Some(values) = map.get_mut(&key) else { return Err(DnsError::NotPublished { host: host.to_owned(), }); @@ -485,14 +498,14 @@ impl TxtRecords { }); } if values.is_empty() { - map.remove(host); + map.remove(&key); } Ok(()) } fn values(&self, host: &str) -> Vec { self.map() - .get(host) + .get(&normalize_host(host)) .map(|values| values.iter().cloned().collect()) .unwrap_or_default() } @@ -524,21 +537,25 @@ impl CaaRecords { let mut sorted = values.to_vec(); sorted.sort(); sorted.dedup(); + let key = normalize_host(host); let mut map = self.map(); if sorted.is_empty() { - return map.remove(host).is_some(); + return map.remove(&key).is_some(); } - match map.get(host) { + match map.get(&key) { Some(existing) if *existing == sorted => false, _ => { - map.insert(host.to_owned(), sorted); + map.insert(key, sorted); true } } } fn values(&self, host: &str) -> Vec { - self.map().get(host).cloned().unwrap_or_default() + self.map() + .get(&normalize_host(host)) + .cloned() + .unwrap_or_default() } } @@ -1299,6 +1316,24 @@ mod tests { assert_eq!(dns.target("a.example"), Some(v4(10, 0, 0, 1))); } + #[test] + fn a_case_different_spelling_of_the_same_host_is_the_same_collision() { + // DNS matching is case-insensitive (RFC 1035 3.1), so + // "A.example" and "a.example" name the same record. Two callers + // racing on different-case spellings of one hostname must collide + // the same way two identical spellings do -- otherwise both + // "publish" successfully, each believing it alone owns the name, + // while only one target can actually be live for it. + let dns = InMemoryDns::new(); + assert!(dns.publish("a.example", &v4(10, 0, 0, 1)).is_ok()); + assert_eq!( + dns.publish("A.example", &v4(10, 0, 0, 2)), + Err(DnsError::AlreadyPublished { + host: "A.example".to_string() + }) + ); + } + #[test] fn withdraw_removes_and_then_reports_absence() { let dns = InMemoryDns::new(); @@ -1525,6 +1560,33 @@ mod tests { )); } + #[test] + fn wildcard_matches_the_zone_case_insensitively() { + // DNS name comparison is case-insensitive (RFC 1035 3.1); a zone + // spelled in mixed case by a deployment's own configuration must + // still accept a host that differs only in case. + let dns = WildcardDns::new("Agents.Example.Com".to_string()); + assert!(dns + .publish("a.agents.example.com", &RecordTarget::Loopback) + .is_ok()); + assert!(dns + .publish("AGENTS.EXAMPLE.COM", &RecordTarget::Loopback) + .is_ok()); + } + + #[test] + fn wildcard_refuses_a_host_with_a_trailing_dot() { + // A trailing dot makes an extra, empty label -- `hostname_is_at_or_ + // below` correctly refuses to align it with the zone's labels, so + // this fails closed (a legitimate FQDN spelling goes unpublished) + // rather than open (an unintended name is accepted). + let dns = WildcardDns::new("agents.example.com".to_string()); + assert!(matches!( + dns.publish("a.agents.example.com.", &RecordTarget::Loopback), + Err(DnsError::NotUnderZone { .. }) + )); + } + #[test] fn wildcard_refuses_to_withdraw_outside_the_zone() { let dns = WildcardDns::new("agents.example.com".to_string()); -- 2.51.2 From ea49fdbd9fb4231d1bcc27893793491a34267666 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Wed, 2 Sep 2026 22:14:13 -0400 Subject: [PATCH 3/3] fix(didbot-dns): apply the same wildcard-depth fix to Route53Dns Route53Dns::accepts had the identical ends_with shape WildcardDns::accepts did before it: any depth under the zone was accepted, not just the one label a wildcard record actually matches. This is the production DNS backend, so the flaw was live where it matters most. Reused the same hostname_is_at_or_below-plus-label-count check. resync's own live-zone test used a TXT fixture two labels below the zone (`_acme-challenge.a.`), a shape didbot_tls::acme never produces -- the real DNS-01 challenge for both the apex and wildcard identifiers always collapses onto `_acme-challenge.`, one label down. Moved the fixture there rather than loosen accepts() to keep a synthetic shape working. Mutation-tested: reverting accepts() to the old ends_with check lets the new over-deep publish test's host through to an unrelated UnsupportedTarget error instead of NotUnderZone; restoring it refuses correctly. Co-Authored-By: Claude Opus 5 (1M context) Change-Id: If1fdfbdfa2c31d40552f8d374a3052fe7d38aed3 --- crates/didbot-dns/src/route53.rs | 42 +++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/crates/didbot-dns/src/route53.rs b/crates/didbot-dns/src/route53.rs index 609c3d5c..8393ad0f 100644 --- a/crates/didbot-dns/src/route53.rs +++ b/crates/didbot-dns/src/route53.rs @@ -91,6 +91,7 @@ //! those keys the same way. use crate::{CaaRecords, CaaValue, DnsError, DnsProvider, RecordTarget, Records, TxtRecords}; +use didbot_identity::hostname_is_at_or_below; use hmac::{Hmac, Mac}; use sha2::Sha256; use std::collections::BTreeSet; @@ -375,8 +376,20 @@ impl Route53Dns { } /// Whether `host` is under this provider's zone. + /// + /// A deployment answers this zone with a single-label wildcard DNS + /// record (`*.`), and RFC 1034 wildcards match exactly one label. + /// So `host` must be the zone itself (the apex, for `_acme-challenge` + /// and the like) or exactly one label below it — never two or more; see + /// [`crate::WildcardDns::accepts`], which mirrors this exactly for the + /// same reason. pub fn accepts(&self, host: &str) -> bool { - host == self.zone || host.ends_with(&format!(".{}", self.zone)) + if !hostname_is_at_or_below(host, &self.zone) { + return false; + } + let zone_labels = self.zone.matches('.').count() + 1; + let host_labels = host.matches('.').count() + 1; + host_labels <= zone_labels + 1 } fn check(&self, host: &str) -> Result<(), DnsError> { @@ -1419,6 +1432,22 @@ mod tests { ); } + #[test] + fn publish_refuses_a_host_more_than_one_label_below_the_zone() { + // Route53Dns answers the zone with a single-label wildcard record; + // a host two labels down would never resolve to a request this + // deployment could serve. No transport call is queued, so this + // would also panic on an unexpected HTTP call if `accepts` failed + // to refuse before `publish` reached the network. + let dns = provider(vec![]); + assert_eq!( + dns.publish("deep.a.agents.example.com", &RecordTarget::Loopback), + Err(DnsError::NotUnderZone { + host: "deep.a.agents.example.com".to_string() + }) + ); + } + #[test] fn publish_refuses_loopback_as_unsupported() { let dns = provider(vec![]); @@ -1708,10 +1737,17 @@ mod tests { #[test] fn resync_hydrates_local_bookkeeping_from_a_live_zone() { + // The TXT fixture sits at `_acme-challenge.` -- one label + // below the zone, exactly where `didbot_tls::acme` always publishes + // the DNS-01 challenge for both the apex and the wildcard + // identifier (see `challenge_record_name`). A per-agent name two + // labels below the zone is not a shape this deployment ever + // produces, and `accepts` now refuses it the same way it refuses a + // handle that deep. let list_body = "\ a.agents.example.com.A300\ 10.0.0.1\ - _acme-challenge.a.agents.example.com.TXT60\ + _acme-challenge.agents.example.com.TXT60\ "tok"\ false"; let transport = Box::new(FakeTransport::new(vec![ok(list_body)])); @@ -1719,7 +1755,7 @@ mod tests { assert!(dns.resync().is_ok()); assert_eq!(dns.published(), vec!["a.agents.example.com"]); assert_eq!( - dns.txt_values("_acme-challenge.a.agents.example.com"), + dns.txt_values("_acme-challenge.agents.example.com"), vec!["tok"] ); // A record now known locally collides on a second real publish. -- 2.51.2