diff --git a/CHANGELOG.md b/CHANGELOG.md index db74e321..e4a1f360 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -168,6 +168,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 transport that adopts a socket handed in by the traversal bootstrap does not fire the seam. Unix only, since the Windows UDP backend has no descriptor. +- A peer address may name which *instance* of a transport it belongs to, as + `transport: "udp/aware"` rather than `"udp"`, where the part after the slash + is the key the transport was configured under. A node running several + instances of one type could not be told them apart by a dialer: both bind + wildcard sockets, so the address-family test matches either, and selection + fell through to the lowest transport id. One socket carried every dial and + the other never carried traffic. A bare type is unqualified and matches any + instance, which is what every existing configuration and caller produces, so + nothing changes for a node that does not use the syntax. A qualified name is + never substituted with a different instance: that is the wrong-lane dial the + syntax exists to prevent, so an unmatched name fails the address instead, and + the same name is what an embedder binds by and what the dialer routes on. The + slash is already how FIPS qualifies an instance inside an address + (`eth0/aa:bb:...`) and cannot occur in a type name. Only UDP resolves an + instance name today; an address that qualifies any other transport type is + refused rather than matched loosely. **Because a qualified name never falls + back, the configuration validator rejects one that no configured transport + answers to**, naming the peer, the instance asked for and the instances that + exist. Otherwise the address would simply be skipped at every dial, which is + invisible for a peer that has a second address that works: the lane would + never carry traffic and nothing above debug logging would say so. + ### Changed - Node health is determined at start completion instead of unconditionally diff --git a/src/config/mod.rs b/src/config/mod.rs index f136d218..7c172abd 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -41,7 +41,7 @@ pub use node::{ NodeConfig, NostrRendezvousConfig, NostrRendezvousPolicy, RateLimitConfig, RekeyConfig, RendezvousConfig, RetryConfig, SessionConfig, SessionMmpConfig, TreeConfig, }; -pub use peer::{ConnectPolicy, PeerAddress, PeerConfig}; +pub use peer::{ConnectPolicy, PeerAddress, PeerConfig, TransportSpec}; pub use transport::{ BleConfig, DirectoryServiceConfig, EthernetConfig, NymConfig, TcpConfig, TorConfig, TransportInstances, TransportsConfig, UdpConfig, @@ -1127,6 +1127,52 @@ impl Config { } } + // Reject a peer address naming a transport instance that no + // configured transport answers to. A qualified name deliberately never + // falls back: substituting a different instance is the wrong-lane dial + // the syntax exists to prevent, so the dialer refuses the address and + // says so at debug. Where the peer has a second address that does + // resolve, that refusal is invisible — the lane is simply never used, + // which is the failure the instance names were introduced to fix. A + // typo, a renamed transport, or a `Named` config collapsed back to + // `Single` all land here, and all of them are cheaper to find at + // startup than in a packet capture. + for peer in &self.peers { + for addr in &peer.addresses { + let spec = addr.spec(); + let Some(want) = spec.instance else { + continue; + }; + if spec.kind != "udp" { + return Err(ConfigError::Validation(format!( + "peer `{}` has address `{}` on transport `{}`, but only `udp` resolves an instance name; \ + for any other type the dialer would have to pick an arbitrary instance, which is the wrong-lane dial the syntax exists to prevent. \ + Drop the `/{want}` qualifier to match any instance of `{}`.", + peer.npub, addr.addr, addr.transport, spec.kind + ))); + } + let configured: Vec<&str> = self + .transports + .udp + .iter() + .filter_map(|(name, _)| name) + .collect(); + if !configured.contains(&want) { + let known = if configured.is_empty() { + "no named udp instances are configured (the udp transport is a single unnamed instance)".to_string() + } else { + format!("configured udp instances are: {}", configured.join(", ")) + }; + return Err(ConfigError::Validation(format!( + "peer `{}` has address `{}` on transport `{}`, but no udp transport is configured under the instance name `{want}`; \ + a qualified name is never substituted, so this address would be skipped at every dial and the peer reached only over its other addresses, if it has any. \ + {known}.", + peer.npub, addr.addr, addr.transport + ))); + } + } + } + // Reject rekey triggers that fire immediately and forever. Both // arms are checked regardless of `node.rekey.enabled` so that // turning rekey on later cannot surface a config error at a @@ -2338,6 +2384,114 @@ node: assert!(!is_loopback_addr_str("example.com:443")); } + #[test] + fn test_a_peer_address_naming_a_configured_udp_instance_passes_validation() { + let mut config = Config { + peers: vec![PeerConfig { + npub: "npub1peer".to_string(), + addresses: vec![PeerAddress::new("udp/aware", "203.0.113.1:2121")], + ..Default::default() + }], + ..Default::default() + }; + config.transports.udp = TransportInstances::Named(HashMap::from([ + ("aware".to_string(), UdpConfig::default()), + ("infra".to_string(), UdpConfig::default()), + ])); + + config + .validate() + .expect("an instance name that matches a configured transport must validate"); + } + + #[test] + fn test_a_peer_address_naming_an_unconfigured_udp_instance_is_rejected() { + let mut config = Config { + peers: vec![PeerConfig { + npub: "npub1peer".to_string(), + addresses: vec![PeerAddress::new("udp/awre", "203.0.113.1:2121")], + ..Default::default() + }], + ..Default::default() + }; + config.transports.udp = TransportInstances::Named(HashMap::from([ + ("aware".to_string(), UdpConfig::default()), + ("infra".to_string(), UdpConfig::default()), + ])); + + let err = config + .validate() + .expect_err("a typo in an instance name must not validate"); + let text = err.to_string(); + assert!( + text.contains("awre"), + "the error must name the instance asked for: {text}" + ); + assert!( + text.contains("aware") && text.contains("infra"), + "the error must list the instances that do exist: {text}" + ); + } + + #[test] + fn test_an_unqualified_peer_address_still_validates_against_named_instances() { + let mut config = Config { + peers: vec![PeerConfig { + npub: "npub1peer".to_string(), + addresses: vec![PeerAddress::new("udp", "203.0.113.1:2121")], + ..Default::default() + }], + ..Default::default() + }; + config.transports.udp = + TransportInstances::Named(HashMap::from([("aware".to_string(), UdpConfig::default())])); + + config + .validate() + .expect("a bare type matches any instance and must stay valid"); + } + + #[test] + fn test_a_qualified_peer_address_is_rejected_when_the_udp_transport_is_unnamed() { + let mut config = Config { + peers: vec![PeerConfig { + npub: "npub1peer".to_string(), + addresses: vec![PeerAddress::new("udp/aware", "203.0.113.1:2121")], + ..Default::default() + }], + ..Default::default() + }; + config.transports.udp = TransportInstances::Single(UdpConfig::default()); + + let err = config + .validate() + .expect_err("a Single config has no instance name to match and must not validate"); + assert!( + err.to_string().contains("no named udp instances"), + "the error must say why nothing matched: {err}" + ); + } + + #[test] + fn test_a_qualified_peer_address_on_a_non_udp_transport_is_rejected() { + let config = Config { + peers: vec![PeerConfig { + npub: "npub1peer".to_string(), + addresses: vec![PeerAddress::new("ethernet/eth0", "eth0/aa:bb:cc:dd:ee:ff")], + ..Default::default() + }], + ..Default::default() + }; + + let err = config + .validate() + .expect_err("only udp resolves an instance name, so any other type must be refused"); + assert!( + err.to_string().contains("only `udp` resolves"), + "the error must say which transport types support the syntax: {err}" + ); + } + #[test] fn test_validate_loopback_bind_with_external_peer_rejected() { use crate::config::PeerAddress; diff --git a/src/config/peer.rs b/src/config/peer.rs index 32477370..8fc32bfd 100644 --- a/src/config/peer.rs +++ b/src/config/peer.rs @@ -22,6 +22,74 @@ pub enum ConnectPolicy { Manual, } +/// The separator between a transport type and an instance name in a +/// [`PeerAddress::transport`] field (`"udp/aware"`). +/// +/// `/` rather than `:` or `.`: it is already how FIPS qualifies an instance +/// inside an *address* (`"eth0/aa:bb:cc:dd:ee:ff"` for Ethernet, +/// `"hci0/AA:BB:…"` for BLE), and it cannot occur in a transport type name, +/// so the split is unambiguous. +const INSTANCE_SEPARATOR: char = '/'; + +/// A [`PeerAddress::transport`] field, split into a transport *type* and an +/// optional *instance name*. +/// +/// A node can run several instances of one transport type +/// ([`TransportInstances::Named`](crate::config::TransportInstances::Named)) — +/// two UDP sockets, say, one pinned to infrastructure Wi-Fi and one to a Wi-Fi +/// Aware data path. Both bind wildcard sockets, so the dialer's address-family +/// test cannot tell them apart: every dial would deterministically take the +/// same instance and the other socket would never carry traffic. Qualifying +/// the transport field with the configured instance name says which one an +/// address belongs to. +/// +/// Syntax: `""` or `"/"`, where `` is the key +/// the transport was configured under. A bare type is *unqualified* and +/// matches any instance of that type, which is what every existing config and +/// caller produces — so this is purely additive. +/// +/// ``` +/// use fips::config::TransportSpec; +/// +/// assert_eq!(TransportSpec::parse("udp").instance, None); +/// assert_eq!(TransportSpec::parse("udp/aware").kind, "udp"); +/// assert_eq!(TransportSpec::parse("udp/aware").instance, Some("aware")); +/// ``` +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct TransportSpec<'a> { + /// The transport type name, as reported by `TransportType::name`. + pub kind: &'a str, + /// The required instance name, or `None` to match any instance. + pub instance: Option<&'a str>, +} + +impl<'a> TransportSpec<'a> { + /// Split a transport field into type and optional instance name. + /// + /// A field with no separator, an empty type, or an empty instance is + /// treated as an unqualified type — malformed input degrades to the + /// pre-existing behaviour rather than becoming an unmatchable name. + pub fn parse(field: &'a str) -> Self { + match field.split_once(INSTANCE_SEPARATOR) { + Some((kind, instance)) if !kind.is_empty() && !instance.is_empty() => Self { + kind, + instance: Some(instance), + }, + _ => Self { + kind: field, + instance: None, + }, + } + } + + /// Whether a transport of type `kind` configured under `name` satisfies + /// this spec. An unqualified spec accepts any instance; a qualified one + /// accepts only an exact name match, and never falls back. + pub fn matches(&self, kind: &str, name: Option<&str>) -> bool { + self.kind == kind && self.instance.is_none_or(|want| name == Some(want)) + } +} + /// A transport-specific address for reaching a peer. /// /// Each peer can have multiple addresses across different transports, @@ -29,7 +97,8 @@ pub enum ConnectPolicy { #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct PeerAddress { - /// Transport type (e.g., "udp", "tor", "ethernet"). + /// Transport type (e.g., "udp", "tor", "ethernet"), optionally qualified + /// with a named instance (`"udp/aware"`) — see [`TransportSpec`]. pub transport: String, /// Transport-specific address string. @@ -106,6 +175,12 @@ impl PeerAddress { self.seen_at_ms = Some(seen_at_ms); self } + + /// The [`transport`](Self::transport) field split into type and optional + /// instance name. + pub fn spec(&self) -> TransportSpec<'_> { + TransportSpec::parse(&self.transport) + } } /// Configuration for a known peer. @@ -202,3 +277,64 @@ impl PeerConfig { matches!(self.connect_policy, ConnectPolicy::AutoConnect) } } + +#[cfg(test)] +mod transport_spec_tests { + use super::*; + + #[test] + fn a_bare_type_is_unqualified_and_matches_any_instance() { + let spec = TransportSpec::parse("udp"); + assert_eq!(spec.kind, "udp"); + assert_eq!(spec.instance, None); + assert!(spec.matches("udp", None), "an unnamed Single instance"); + assert!(spec.matches("udp", Some("aware")), "a named instance"); + assert!(!spec.matches("tcp", None), "a different transport type"); + } + + #[test] + fn a_qualified_type_matches_only_that_instance() { + let spec = TransportSpec::parse("udp/aware"); + assert_eq!(spec.kind, "udp"); + assert_eq!(spec.instance, Some("aware")); + assert!(spec.matches("udp", Some("aware"))); + assert!( + !spec.matches("udp", Some("lan")), + "a different instance must not be substituted" + ); + assert!( + !spec.matches("udp", None), + "an unnamed instance cannot satisfy a named request" + ); + assert!(!spec.matches("tcp", Some("aware"))); + } + + #[test] + fn malformed_fields_degrade_to_an_unqualified_type() { + // Neither half may be empty; anything else keeps the whole string as + // the type, so a typo fails the type test rather than silently + // matching some instance. + for field in ["udp/", "/aware", "/"] { + let spec = TransportSpec::parse(field); + assert_eq!(spec.kind, field, "{field}"); + assert_eq!(spec.instance, None, "{field}"); + } + } + + #[test] + fn only_the_first_separator_splits() { + let spec = TransportSpec::parse("udp/a/b"); + assert_eq!(spec.kind, "udp"); + assert_eq!(spec.instance, Some("a/b")); + } + + #[test] + fn peer_address_exposes_its_spec() { + let addr = PeerAddress::new("udp/aware", "[fe80::1%7]:4872"); + assert_eq!(addr.spec().instance, Some("aware")); + assert_eq!( + PeerAddress::new("udp", "1.2.3.4:2121").spec().instance, + None + ); + } +} diff --git a/src/node/lifecycle/mod.rs b/src/node/lifecycle/mod.rs index 8c54a351..1588f136 100644 --- a/src/node/lifecycle/mod.rs +++ b/src/node/lifecycle/mod.rs @@ -406,14 +406,24 @@ impl Node { /// service. A wildcard IPv4 socket cannot send to an IPv6 link-local /// target, and vice versa, so callers must choose by socket family rather /// than by transport type alone. + /// + /// `instance` names a specific configured UDP instance + /// ([`TransportSpec`](crate::config::TransportSpec)) and is the only way to + /// discriminate two wildcard sockets: they are family-compatible with the + /// same addresses, so without a name the lowest `TransportId` always wins + /// and one socket carries everything. When it is `Some`, a non-matching + /// instance is never substituted — the caller gets `None` and can say so, + /// rather than dialing down the wrong lane. fn find_udp_transport_for_remote_addr( &self, remote_addr: SocketAddr, + instance: Option<&str>, ) -> Option<(TransportId, SocketAddr)> { self.transports .iter() .filter(|(id, handle)| { handle.transport_type().name == "udp" + && instance.is_none_or(|want| handle.name() == Some(want)) && handle.is_operational() && !self.supervisor.nostr_rendezvous.is_bootstrap_transport(id) }) @@ -1148,7 +1158,7 @@ impl Node { for event in events { let crate::mdns::LanEvent::Discovered(peer) = event; let Some((transport_id, _local_addr)) = - self.find_udp_transport_for_remote_addr(peer.addr) + self.find_udp_transport_for_remote_addr(peer.addr, None) else { debug!( addr = %peer.addr, @@ -2413,7 +2423,12 @@ impl Node { if attempted >= max_attempts { break; } - if addr.transport == "udp" && addr.addr.eq_ignore_ascii_case("nat") { + // The transport field may name a specific instance + // (`"udp/aware"`); everything below dispatches on the type half + // and hands the instance half to whichever resolver can honour it. + let spec = addr.spec(); + + if spec.kind == "udp" && addr.addr.eq_ignore_ascii_case("nat") { if !allow_bootstrap_nat { continue; } @@ -2463,10 +2478,11 @@ impl Node { continue; } } else { - let tid = if addr.transport == "udp" + let tid = if spec.kind == "udp" && let Ok(remote_socket_addr) = addr.addr.parse::() { - match self.find_udp_transport_for_remote_addr(remote_socket_addr) { + match self.find_udp_transport_for_remote_addr(remote_socket_addr, spec.instance) + { Some((id, _)) => id, None => { debug!( @@ -2477,8 +2493,20 @@ impl Node { continue; } } + } else if spec.instance.is_some() { + // Only the UDP resolver above can honour an instance name. + // Matching any instance of the type here would be the + // silent wrong-lane substitution this whole mechanism + // exists to prevent, so refuse instead. + debug!( + transport = %addr.transport, + addr = %addr.addr, + "Instance-qualified address for a transport type that \ + does not support instance selection" + ); + continue; } else { - match self.find_transport_for_type(&addr.transport) { + match self.find_transport_for_type(spec.kind) { Some(id) => id, None => { debug!( @@ -3167,11 +3195,21 @@ impl Node { let current_transport = peer .transport_id() .and_then(|id| self.transports.get(&id)) - .map(|transport| transport.transport_type().name); + .map(|transport| (transport.transport_type().name, transport.name())); + // Compare against the candidate's *parsed* transport: a peer address + // may name an instance (`"udp/aware"`), while a handle reports its type + // and its instance name separately. Comparing the raw field to the type + // name would call every instance-qualified address an alternative path, + // so a platform lane that re-pushes its peers — Wi-Fi Aware does, on + // every NDP callback — would re-dial a peer it is already connected to, + // forever. + let spec = candidate.spec(); candidate.addr == current_addr && current_transport - .map(|transport| transport == candidate.transport) + .map(|(kind, instance)| { + kind == spec.kind && spec.instance.is_none_or(|want| instance == Some(want)) + }) .unwrap_or(true) } diff --git a/src/node/tests/unit.rs b/src/node/tests/unit.rs index e35061a2..8c340994 100644 --- a/src/node/tests/unit.rs +++ b/src/node/tests/unit.rs @@ -1253,6 +1253,69 @@ fn active_peer_same_path_discovery_refreshes_stale_peer() { )); } +/// An instance-qualified candidate is the peer's *current* path only when it +/// names the instance the peer is actually on. Without this, every qualified +/// address looked like a different path from the `"udp"` a transport reports as +/// its type, so a platform lane that re-pushes its peers — Wi-Fi Aware does, on +/// every data-path callback — would re-dial a peer it is already connected to, +/// for as long as it stayed connected. +#[tokio::test] +async fn an_instance_qualified_candidate_matches_only_its_own_instance() { + let mut listeners = std::collections::HashMap::new(); + for name in ["main", "backup"] { + listeners.insert( + name.to_string(), + crate::config::UdpConfig { + bind_addr: Some("127.0.0.1:0".to_string()), + ..Default::default() + }, + ); + } + let mut config = crate::Config::new(); + config.transports.udp = crate::config::TransportInstances::Named(listeners); + config.dns.enabled = false; + + let mut node = make_node_with(config); + node.start().await.unwrap(); + + let main_id = *node + .transports + .iter() + .find(|(_, handle)| handle.name() == Some("main")) + .expect("the `main` listener came up") + .0; + + let peer_full = Identity::generate(); + let peer_identity = PeerIdentity::from_pubkey_full(peer_full.pubkey_full()); + let peer_node_addr = *peer_identity.node_addr(); + let mut active_peer = ActivePeer::new(peer_identity, LinkId::new(7), Node::now_ms()); + active_peer.set_current_addr(main_id, TransportAddr::from_string("127.0.0.1:9")); + node.peers.insert(peer_node_addr, active_peer); + + let matches = |transport: &str| { + let candidate = crate::config::PeerAddress::new(transport, "127.0.0.1:9"); + node.active_peer_candidate_is_fresh_enough_to_skip( + &peer_node_addr, + std::slice::from_ref(&candidate), + ) + }; + + assert!( + matches("udp"), + "an unqualified candidate still matches, as it always did", + ); + assert!( + matches("udp/main"), + "the instance the peer is on is the same path, not an alternative", + ); + assert!( + !matches("udp/backup"), + "a different instance is a genuinely different path", + ); + + node.stop().await.unwrap(); +} + #[tokio::test] async fn node_context_mirrors_config_and_immutable_facades() { let mut node = make_node();