Respond to TXT record queries with peer addresses#12
Respond to TXT record queries with peer addresses#12arya2 wants to merge 2 commits intoZcashFoundation:mainfrom
Conversation
…ts and spawning background tasks to update/prune them from server logic.
…cords Combines redundant match statements
|
Thanks @arya2 . Looks like we're on the right track, and agree that we should really be using TXT instead of A. (automated) Review (opus 4.6)This PR does two things:
The refactoring is clean — moved code is essentially identical. However, the TXT feature has a critical correctness bug and a few other issues. Blocking Issue: TXT responses use A/AAAA RData instead of TXT RDataThe core feature of this PR does not work correctly. When a TXT query arrives, the handler chains IPv4 and IPv6 addresses together but then creates A/AAAA // In handle_request_inner — the TXT arm produces PeerSocketAddr values...
RecordType::TXT => {
let AddressRecords { ipv4, ipv6 } = self.latest_addresses.borrow().clone();
Some(("TXT", ipv4.into_iter().chain(ipv6.into_iter()).collect()))
}
// ...which are then turned into A/AAAA RData:
let rdata = match addr.ip() {
std::net::IpAddr::V4(ipv4) => RData::A(...),
std::net::IpAddr::V6(ipv6) => RData::AAAA(...),
};Most resolvers will silently drop records whose rdata type doesn't match the query type. The fix is to produce RecordType::TXT => {
// Collect all addresses, serialize as TXT rdata
let AddressRecords { ipv4, ipv6 } = self.latest_addresses.borrow().clone();
for addr in ipv4.into_iter().chain(ipv6) {
let txt = hickory_proto::rr::rdata::TXT::new(vec![addr.ip().to_string()]);
records.push(Record::from_rdata(name.clone().into(), self.dns_ttl, RData::TXT(txt)));
}
// record metric + send response (needs its own path, not the shared one below)
}This needs a separate code path from A/AAAA since the rdata construction is different. Consider whether each address should be a separate TXT record or all addresses should be packed into a single TXT record with multiple character-strings — the DNS spec allows both, but one TXT record with multiple character-strings is more compact and conventional for multi-value responses. Non-blocking issues0. No test for TXT queries The PR adds the headline feature (TXT support) but has no test for it. There are existing tests for A and AAAA queries ( 1. Spurious log message in pub fn spawn(config: SeederConfig) -> (Option<Arc<RateLimiter>>, JoinHandle<()>) {
tracing::info!("Initializing zebra-network..."); // ← copy-paste errorThis function initializes the rate limiter, not zebra-network. Remove or fix this log line. 2. The function only reads 3. TXT response has no peer count bound A/AAAA responses are already bounded by 4. Test in The relocated 5. } else {
// For NS, SOA, etc, we might want to return something else or Refused.
// Returning empty NOERROR or NXDOMAIN?
// Let's return NOERROR empty for now.
}The block is empty — either remove the Refactoring (commit 1) looks goodThe extraction of |
Zebra can use these fns to get the records:
It's implicitly bound, perhaps should be documented. All the non-blocking issues are good callouts. |
|
SRV records, if they work, might be more appropriate than TXT records. |
Based on #11
Motivation
We want to eventually replace use TXT records instead of A or AAAA records so that browsers pointed at the domain won't send requests to random IPs.