diff --git a/CHANGELOG.md b/CHANGELOG.md index d08a2786..fe06b2db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -433,6 +433,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 #### Transports & config +- The UDP transport's DNS cache is now bounded and actually evicts. The map + held one entry per distinct hostname string ever dialed, and the TTL was + applied only on the read, so a stale entry was overwritten on the next dial + of the same name and otherwise stayed for the life of the process. Under a + rendezvous policy that accepts advertised endpoints the keys are strings a + remote party chose, which made the growth theirs to drive. A store now + sweeps entries past their TTL and, if the map is still full, drops the + oldest, holding it to 256 hostnames. Refreshing a name already cached + evicts nothing. Eviction is by insertion time rather than last use, so a + rarely dialed name in a very large peer list may re-resolve more often; the + cost of a wrong eviction is one DNS lookup, not a failed dial. + - A failed private-key write no longer leaves a node silently running an ephemeral identity. Six write results in the identity path were discarded, and the sharpest was in `persistent` mode: a failed write to `fips.key` fell diff --git a/src/transport/udp/mod.rs b/src/transport/udp/mod.rs index 83de32df..81c5b4eb 100644 --- a/src/transport/udp/mod.rs +++ b/src/transport/udp/mod.rs @@ -29,6 +29,18 @@ use tracing::{debug, info, trace, warn}; /// DNS cache TTL for hostname resolution (60 seconds). const DNS_CACHE_TTL: Duration = Duration::from_secs(60); +/// Upper bound on the number of hostnames the DNS cache holds at once. +/// +/// The cache is keyed by the address string a dial was asked for, and under a +/// rendezvous policy that accepts advertised endpoints those strings come from +/// remote parties, so without a bound the map grows for the life of the +/// process. 256 sits about two orders of magnitude above the number of +/// distinct hostnames a configured peer list produces, so no ordinary +/// deployment reaches it. Lowering it starts to be reachable by a large peer +/// list, and the only cost of an eviction is one extra DNS lookup on the next +/// dial of that name; raising it buys nothing but resident memory. +const DNS_CACHE_MAX_ENTRIES: usize = 256; + /// UDP transport for FIPS. /// /// Provides connectionless, unreliable packet delivery over UDP/IP. @@ -145,10 +157,8 @@ impl UdpTransport { // Check cache { let cache = self.dns_cache.lock().unwrap_or_else(|e| e.into_inner()); - if let Some((resolved, cached_at)) = cache.get(addr) - && cached_at.elapsed() < DNS_CACHE_TTL - { - return Ok(*resolved); + if let Some(resolved) = cache_lookup(&cache, addr, Instant::now()) { + return Ok(resolved); } } @@ -158,7 +168,13 @@ impl UdpTransport { // Store in cache { let mut cache = self.dns_cache.lock().unwrap_or_else(|e| e.into_inner()); - cache.insert(addr.clone(), (resolved, Instant::now())); + cache_store( + &mut cache, + addr.clone(), + resolved, + Instant::now(), + DNS_CACHE_MAX_ENTRIES, + ); } Ok(resolved) @@ -601,6 +617,56 @@ async fn udp_receive_loop( } } +/// A cached resolution for `key`, if one is present and still inside +/// `DNS_CACHE_TTL` at `now`. +fn cache_lookup( + cache: &HashMap, + key: &TransportAddr, + now: Instant, +) -> Option { + cache + .get(key) + .filter(|(_, cached_at)| now.duration_since(*cached_at) < DNS_CACHE_TTL) + .map(|(resolved, _)| *resolved) +} + +/// Record a resolution, keeping the cache at or below `cap` entries. +/// +/// Refreshing a name already present never evicts anything. Otherwise every +/// entry past its TTL is dropped first, and only if that leaves the map full +/// is the oldest remaining entry evicted. Eviction is by insertion time rather +/// than by last use: the timestamp is already there as the TTL clock, and +/// tracking last use would mean writing to the map on the read path of every +/// dial. The sweep is linear in `cap` and runs only on a resolution miss, so +/// at most once per TTL per name. +fn cache_store( + cache: &mut HashMap, + key: TransportAddr, + resolved: SocketAddr, + now: Instant, + cap: usize, +) { + if let Some(entry) = cache.get_mut(&key) { + *entry = (resolved, now); + return; + } + + cache.retain(|_, (_, cached_at)| now.duration_since(*cached_at) < DNS_CACHE_TTL); + + while cache.len() >= cap { + let Some(oldest) = cache + .iter() + .min_by_key(|(_, (_, cached_at))| *cached_at) + .map(|(key, _)| key.clone()) + else { + break; + }; + cache.remove(&oldest); + } + + cache.insert(key, (resolved, now)); +} + // ============================================================================ // Tests // ============================================================================ @@ -611,6 +677,112 @@ mod tests { use crate::transport::packet_channel; use tokio::time::{Duration, timeout}; + /// A distinct hostname key, so each store is a fresh entry. + fn dns_key(n: usize) -> TransportAddr { + TransportAddr::from(format!("host{n}.example:2121")) + } + + fn dns_value() -> SocketAddr { + "198.51.100.1:2121".parse().unwrap() + } + + /// The cache is keyed by strings a remote party can choose, so its size + /// has to be bounded no matter how many distinct names are dialed. + #[test] + fn dns_cache_store_refuses_to_exceed_the_cap() { + const CAP: usize = 8; + let now = Instant::now(); + let mut cache = HashMap::new(); + + for n in 0..CAP + 5 { + cache_store(&mut cache, dns_key(n), dns_value(), now, CAP); + assert!( + cache.len() <= CAP, + "cache grew to {} entries past a cap of {CAP}", + cache.len() + ); + } + } + + /// A stale entry used to be overwritten on the next dial of the same name + /// and otherwise never removed, so a name dialed once sat there forever. + #[test] + fn dns_cache_store_evicts_entries_past_their_ttl() { + let now = Instant::now(); + let expired_at = now.checked_sub(DNS_CACHE_TTL * 2).expect("monotonic clock"); + let mut cache = HashMap::new(); + cache.insert(dns_key(0), (dns_value(), expired_at)); + + cache_store( + &mut cache, + dns_key(1), + dns_value(), + now, + DNS_CACHE_MAX_ENTRIES, + ); + + assert!( + !cache.contains_key(&dns_key(0)), + "an entry past its TTL should be swept, not left to accumulate" + ); + assert!(cache_lookup(&cache, &dns_key(0), now).is_none()); + assert!(cache_lookup(&cache, &dns_key(1), now).is_some()); + } + + /// With nothing expired, the cap is enforced by dropping the oldest entry. + /// The ages here are all well inside the TTL, so the expiry sweep cannot + /// be what makes room and the eviction branch is the one under test. + #[test] + fn dns_cache_store_evicts_the_oldest_entry_when_every_entry_is_fresh() { + const CAP: usize = 4; + let now = Instant::now(); + let mut cache = HashMap::new(); + for n in 0..CAP { + let age = Duration::from_secs((CAP - n) as u64); + assert!(age < DNS_CACHE_TTL, "fixture must stay inside the TTL"); + let cached_at = now.checked_sub(age).expect("monotonic clock"); + cache.insert(dns_key(n), (dns_value(), cached_at)); + } + assert_eq!(cache.len(), CAP, "no entry should be expired going in"); + + cache_store(&mut cache, dns_key(CAP), dns_value(), now, CAP); + + assert_eq!(cache.len(), CAP); + assert!( + !cache.contains_key(&dns_key(0)), + "the oldest entry should be the one evicted" + ); + for n in 1..=CAP { + assert!( + cache.contains_key(&dns_key(n)), + "entry {n} should have survived" + ); + } + } + + /// Re-resolving a name already cached is the common case on a live node. + /// It must not cost another entry its place. + #[test] + fn dns_cache_store_refreshing_an_existing_key_evicts_nothing() { + const CAP: usize = 4; + let now = Instant::now(); + let mut cache = HashMap::new(); + for n in 0..CAP { + let cached_at = now + .checked_sub(Duration::from_secs((CAP - n) as u64)) + .expect("monotonic clock"); + cache.insert(dns_key(n), (dns_value(), cached_at)); + } + + cache_store(&mut cache, dns_key(0), dns_value(), now, CAP); + + assert_eq!(cache.len(), CAP); + for n in 0..CAP { + assert!(cache.contains_key(&dns_key(n)), "entry {n} should remain"); + } + assert_eq!(cache_lookup(&cache, &dns_key(0), now), Some(dns_value())); + } + fn make_config(port: u16) -> UdpConfig { UdpConfig { bind_addr: Some(format!("127.0.0.1:{}", port)),