From 33914867d152e549187914824784e4d625df0e37 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Fri, 28 Aug 2026 20:34:34 +0200 Subject: [PATCH] The advert names its address, and the client stops rolling dice on the rest A field client streamed over the host's ZeroTier address while both machines shared a LAN. Neither end chose that: the host registers exactly one address (its routed primary), but the OS's own mDNS responder also answers A queries for .local. per interface with that interface's address, so the resolved set a client folds is a union polluted by every overlay network whose multicast reaches it - and both clients picked with HashSet::iter().next(): arbitrary, and re-rolled on every re-announce. punktfunk_core::discovery now makes the pick deterministic, shared by the desktop and Android clients. Ranking, best wins: longest common prefix with any of this machine's unicast addresses (on-link beats routed, which settles LAN-vs-overlay in BOTH directions - a client that can only reach the host through the overlay has no interface for the host's LAN address to match), then the address the host itself declared, then prefix against our default-route source, then the numerically lowest. The host declares which address its advert is FOR in a new advisory TXT key (addr); it never overrides reachability, only settles a multi-NIC host's tie, and old hosts without it still resolve deterministically. --- clients/android/native/src/discovery.rs | 11 +- crates/pf-client-core/src/discovery.rs | 16 ++- crates/punktfunk-core/src/discovery.rs | 171 ++++++++++++++++++++++++ crates/punktfunk-core/src/lib.rs | 1 + crates/punktfunk-host/src/discovery.rs | 8 ++ 5 files changed, 199 insertions(+), 8 deletions(-) create mode 100644 crates/punktfunk-core/src/discovery.rs diff --git a/clients/android/native/src/discovery.rs b/clients/android/native/src/discovery.rs index 5c49d5bc5..9e236bd4d 100644 --- a/clients/android/native/src/discovery.rs +++ b/clients/android/native/src/discovery.rs @@ -178,11 +178,12 @@ fn resolve(info: &ResolvedService) -> Option { if !proto.is_empty() && proto != PROTO { return None; // some other DNS-SD service sharing the type — ignore } - let addr = info - .get_addresses_v4() - .iter() - .next() - .map(|a| a.to_string())?; + // Deterministic pick from the union of per-interface answers (the host OS's responder + // contributes VPN/overlay addresses; `iter().next()` on the HashSet dialed an arbitrary + // one) — same policy as the desktop client, shared in `punktfunk_core::discovery`. + let candidates: Vec = info.get_addresses_v4().into_iter().collect(); + let addr = punktfunk_core::discovery::pick_host_addr(&candidates, val("addr").parse().ok())? + .to_string(); let id = val("id"); let fullname = info.get_fullname(); Some(Host { diff --git a/crates/pf-client-core/src/discovery.rs b/crates/pf-client-core/src/discovery.rs index 4432a6cb4..546a67cf3 100644 --- a/crates/pf-client-core/src/discovery.rs +++ b/crates/pf-client-core/src/discovery.rs @@ -141,9 +141,19 @@ pub fn browse() -> (async_channel::Receiver, Rescan) { // responder often answers AAAA for its hostname) would render a host card // that fails on every click. A v6-only advert is dropped — the honest // "not found" — until the stack actually speaks IPv6. - let Some(addr) = - info.get_addresses_v4().iter().next().map(|a| a.to_string()) - else { + // + // Among the v4 addresses, pick deterministically: the set is a union of + // per-interface answers from EVERY responder (a host on ZeroTier/… + // contributes its overlay address via the OS responder), and taking + // `iter().next()` of the HashSet dialed an arbitrary one — a field + // client streamed over the host's VPN while both machines shared a LAN. + let candidates: Vec = + info.get_addresses_v4().into_iter().collect(); + let Some(addr) = punktfunk_core::discovery::pick_host_addr( + &candidates, + val("addr").parse().ok(), + ) + .map(|a| a.to_string()) else { continue; }; let id = val("id"); diff --git a/crates/punktfunk-core/src/discovery.rs b/crates/punktfunk-core/src/discovery.rs new file mode 100644 index 000000000..5687a37bc --- /dev/null +++ b/crates/punktfunk-core/src/discovery.rs @@ -0,0 +1,171 @@ +//! Shared discovery-address selection: which A record to dial when an mDNS advert resolves to +//! several. +//! +//! The resolved set is a UNION of answers from every responder on every interface. The host's +//! own advert registers exactly one address (its routed primary — see the host crate's +//! `discovery.rs`), but the host OS's built-in mDNS responder also answers A queries for the +//! same `.local.` label per interface, with that interface's address — so a host running +//! an overlay network (ZeroTier, Tailscale, …) whose multicast reaches this client contributes +//! its overlay address to the set. Field case: a client dialed the host's ZeroTier address +//! while both machines shared a LAN, because the pick was `HashSet::iter().next()` — arbitrary, +//! and re-rolled on every re-announce. +//! +//! [`rank_host_addr`] is the pure policy (testable); [`pick_host_addr`] applies it with this +//! machine's live context. + +use std::net::{IpAddr, Ipv4Addr, UdpSocket}; + +/// Common leading bits of two addresses — the "how on-link is this" proxy the ranking runs on. +/// No netmasks: a longer shared prefix with one of our own addresses is monotonically "more +/// likely on this segment", which is all a RANKING needs. +fn prefix_bits(a: Ipv4Addr, b: Ipv4Addr) -> u32 { + (u32::from(a) ^ u32::from(b)).leading_zeros() +} + +/// The address to dial, chosen deterministically. Score, best wins, in order: +/// +/// 1. longest common prefix with ANY of this machine's unicast addresses — an address on one of +/// our own subnets beats one we would have to route. This alone settles the overlay case in +/// both directions: on a shared LAN the host's LAN address out-prefixes its overlay address, +/// and a client that can ONLY reach the host through the overlay has no LAN interface for +/// the host's LAN address to match, so the overlay address wins instead; +/// 2. the address the host itself declared (mDNS TXT `addr`, its routed primary) — settles a +/// multi-NIC host's tie without ever overriding reachability, because a declared address we +/// cannot see on-link already lost rung 1; +/// 3. longest common prefix with OUR routed (default-route) source address — a host that +/// predates the `addr` TXT still resolves the common ties here; +/// 4. the numerically lowest address — pure determinism, so a re-announce cannot flap the pick. +pub fn rank_host_addr( + candidates: &[Ipv4Addr], + host_declared: Option, + local_ips: &[Ipv4Addr], + routed_local: Option, +) -> Option { + candidates.iter().copied().max_by_key(|&c| { + ( + local_ips + .iter() + .map(|&l| prefix_bits(c, l)) + .max() + .unwrap_or(0), + host_declared == Some(c), + routed_local.map_or(0, |r| prefix_bits(c, r)), + std::cmp::Reverse(u32::from(c)), + ) + }) +} + +/// [`rank_host_addr`] with this machine's live context: every non-loopback unicast IPv4, plus +/// the source address the OS routes toward the internet. Gathered per call — discovery events +/// are rare, and interfaces change (VPN up/down) between them. +pub fn pick_host_addr( + candidates: &[Ipv4Addr], + host_declared: Option, +) -> Option { + rank_host_addr( + candidates, + host_declared, + &local_ipv4s(), + routed_local_ipv4(), + ) +} + +fn local_ipv4s() -> Vec { + if_addrs::get_if_addrs() + .map(|ifs| { + ifs.into_iter() + .filter_map(|i| match i.ip() { + IpAddr::V4(v) if !v.is_loopback() => Some(v), + _ => None, + }) + .collect() + }) + .unwrap_or_default() +} + +/// Same trick as the host's `primary_local_ip`: a UDP `connect()` performs the route lookup +/// without sending a packet, and `local_addr` is the source address the OS chose. +fn routed_local_ipv4() -> Option { + let sock = UdpSocket::bind("0.0.0.0:0").ok()?; + sock.connect("8.8.8.8:80").ok()?; + match sock.local_addr().ok()?.ip() { + IpAddr::V4(v) if !v.is_loopback() => Some(v), + _ => None, + } +} + +#[cfg(test)] +mod tests { + use super::rank_host_addr; + use std::net::Ipv4Addr; + + fn ip(s: &str) -> Ipv4Addr { + s.parse().unwrap() + } + + // The 2026-08-28 field case: host advertises from its LAN address, the OS responder adds + // the ZeroTier address over the overlay's multicast, and both machines are on both + // networks. The LAN address must win — with or without the host's TXT declaration. + #[test] + fn shared_lan_beats_shared_overlay() { + let candidates = [ip("192.168.196.206"), ip("192.168.1.170")]; + let locals = [ip("192.168.1.150"), ip("192.168.196.57")]; + for declared in [None, Some(ip("192.168.1.170"))] { + assert_eq!( + rank_host_addr(&candidates, declared, &locals, Some(ip("192.168.1.150"))), + Some(ip("192.168.1.170")) + ); + } + } + + // A client that can ONLY reach the host through the overlay (different site): the host's + // declared LAN address is not on any of our subnets, so it must NOT win — the overlay + // address is the reachable one. + #[test] + fn overlay_only_client_ignores_the_declared_lan_address() { + let candidates = [ip("192.168.1.170"), ip("192.168.196.206")]; + let locals = [ip("10.1.2.3"), ip("192.168.196.57")]; + assert_eq!( + rank_host_addr( + &candidates, + Some(ip("192.168.1.170")), + &locals, + Some(ip("10.1.2.3")) + ), + Some(ip("192.168.196.206")) + ); + } + + // A multi-NIC host (Ethernet + Wi-Fi on the same LAN) ties on every reachability rung; + // its own declaration settles which of ITS addresses we dial. Without the declaration + // (older host) the pick is still deterministic. + #[test] + fn declared_addr_settles_a_multi_nic_tie() { + let candidates = [ip("192.168.1.170"), ip("192.168.1.171")]; + let locals = [ip("192.168.1.150")]; + assert_eq!( + rank_host_addr( + &candidates, + Some(ip("192.168.1.171")), + &locals, + Some(ip("192.168.1.150")) + ), + Some(ip("192.168.1.171")) + ); + assert_eq!( + rank_host_addr(&candidates, None, &locals, Some(ip("192.168.1.150"))), + Some(ip("192.168.1.170")), + "no declaration: lowest address, never a hash-order roll" + ); + } + + #[test] + fn no_context_is_still_deterministic() { + let candidates = [ip("10.0.0.9"), ip("10.0.0.5")]; + assert_eq!( + rank_host_addr(&candidates, None, &[], None), + Some(ip("10.0.0.5")) + ); + assert_eq!(rank_host_addr(&[], None, &[], None), None); + } +} diff --git a/crates/punktfunk-core/src/lib.rs b/crates/punktfunk-core/src/lib.rs index 08f74f5c4..a7b9f2255 100644 --- a/crates/punktfunk-core/src/lib.rs +++ b/crates/punktfunk-core/src/lib.rs @@ -61,6 +61,7 @@ pub mod client; pub mod clipboard; pub mod config; pub mod crypto; +pub mod discovery; pub mod error; pub mod fec; pub mod input; diff --git a/crates/punktfunk-host/src/discovery.rs b/crates/punktfunk-host/src/discovery.rs index bfdb808f1..19405e3ff 100644 --- a/crates/punktfunk-host/src/discovery.rs +++ b/crates/punktfunk-host/src/discovery.rs @@ -21,6 +21,11 @@ //! - `os` — the host's OS identity chain (`windows` | `macos` | `linux[/][/]`, e.g. //! `linux/fedora/bazzite` — see [`crate::osinfo`]), so a client can show an OS icon on the host //! card. Advisory/unauthenticated like `mac`: a wrong value only draws a wrong icon. +//! - `addr` — the IPv4 this advert was registered for (the host's routed primary). The A-record +//! set a client resolves is a union polluted by OTHER responders answering per-interface (the +//! host OS's own mDNS stack answers for `.local.` on a VPN/overlay interface with that +//! interface's address); the client's picker (`punktfunk_core::discovery`) uses this declared +//! value to break ties among addresses it resolved anyway. Advisory like the rest. use anyhow::{Context, Result}; use mdns_sd::{ServiceDaemon, ServiceInfo}; @@ -220,6 +225,9 @@ pub fn advertise_native( if !macs.is_empty() { props.insert("mac".into(), macs.join(",")); } + // `addr` — which of the host's addresses this advert is FOR (see module doc): the + // client-side picker's tie-breaker against per-interface answers from other responders. + props.insert("addr".into(), ip.to_string()); // Detect & warn (never modifies) if the routed NIC isn't armed to wake — the usual reason // WoL silently fails. Re-checked on an address change because the routed NIC may be a // different one now. -- 2.54.0