mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-05 19:18:25 +00:00
feat(config): let a peer address name which transport instance it belongs to
The instance-qualified peer address, the candidate comparison it breaks, and the validator it needs. They land as one commit because the syntax on its own ships a known defect: once an address can carry an instance name, `active_peer_matches_candidate` reads a peer's own current path as an alternative one and re-handshakes it on every re-push. The validator has a subject only once the syntax exists. Each original commit message follows in full. --- let a peer address name which transport instance it belongs to --- A node can run several instances of one transport type (`TransportInstances::Named`) — two UDP sockets, say, one pinned to infrastructure Wi-Fi and one to a Wi-Fi Aware data path. The dialer could not tell them apart. `find_udp_transport_for_remote_addr` selects by address family and then takes the lowest `TransportId`, but two wildcard sockets are family-compatible with the same addresses, so every dial deterministically took the same instance and the other socket never carried traffic. `find_transport_for_type` was worse: first match on type name, from a HashMap, so nondeterministic. Qualify the transport field with the instance name the transport was configured under: `"udp"` or `"udp/aware"`. `TransportSpec::parse` splits it, and `matches` accepts any instance for an unqualified spec and only an exact name for a qualified one. A qualified name never falls back. Substituting a different instance is the wrong-lane dial this exists to prevent, so a name that matches nothing fails the address instead. The non-UDP branch refuses an instance-qualified address outright rather than matching any instance of the type, because no resolver there can honour the name yet. Purely additive. Every existing config, caller and peer file produces a bare type, which parses to `instance: None` and behaves exactly as before; a node with a `Single` config has no instance names to match against at all. `/` as the separator: it is already how FIPS qualifies an instance inside an address (`eth0/aa:bb:…`, `hci0/AA:BB:…`) and cannot occur in a type name, so the split is unambiguous. --- compare a candidate address against the instance the peer is on --- `active_peer_matches_candidate` compared a peer address's raw transport field to the transport handle's *type* name. Once an address can name an instance (`"udp/aware"`), that comparison is wrong: the string never equals `"udp"`, so the peer's own current path reads as an alternative one. `api_connect` then sends every re-push of an already-connected peer down `attempt_peer_address_list`, which re-dials and re-handshakes it. Observed on device the moment the Android lanes started qualifying their pushes: link ids for three connected LAN peers climbed continuously — 17, 18, 19, 22, 23, 29, 30 — one fresh Noise handshake per peer per mDNS re-push, for as long as the peers stayed up. Wi-Fi Aware re-pushes on every data-path callback, which is more often still. Parse the candidate instead: match on the type half, and on the instance half only when the address names one. An unqualified address behaves exactly as before. --- reject a peer address naming an unconfigured transport instance --- An address may now qualify its transport with an instance name, "udp/aware", and a qualified name deliberately never falls back: substituting a different instance is the wrong-lane dial the syntax exists to prevent. So an unmatched name makes the address permanently undialable, and the dialer says so at debug level. That is invisible in the case that matters. Where the peer has a second address that does resolve, the dial succeeds and nothing reports that one configured lane is never carrying traffic, which is the same failure the instance names were introduced to fix. A sole bad address already errors, so only the multi-address case is silent. Validate at load instead, where the check is fatal at startup and the message can name the peer, the instance asked for and the instances that exist. A typo, a renamed transport, and a Named config collapsed back to Single all land in the same place. Addresses qualified on a non-UDP transport are refused for the reason the dialer refuses them: no resolver there can honour the name. Break-checked: disabling both refusal arms reds the three rejection tests by name and leaves the two acceptance tests green, so the guard fires on the real defect and does not red a clean config. --- changelog and authorship --- One entry under Added, covering the dialer bug the syntax fixes, the unqualified default that keeps every existing configuration working, the refusal to substitute a different instance, and the validator. There is no entry for the candidate comparison fix: that defect exists only once instance names do, both arrive in this release, and an entry for a bug that never shipped would send a reader looking for a version that had it. The syntax and the comparison fix are Arjen's and the validator is mine, so the commit is authored to him and carries me as a co-author. Co-authored-by: Johnathan Corgan <johnathan@corganlabs.com>
This commit is contained in:
committed by
Johnathan Corgan
co-authored by
Johnathan Corgan
parent
106ca7b00d
commit
9b1d4bb24d
@@ -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
|
||||
|
||||
+155
-1
@@ -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;
|
||||
|
||||
+137
-1
@@ -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: `"<type>"` or `"<type>/<instance>"`, where `<instance>` 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
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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::<SocketAddr>()
|
||||
{
|
||||
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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in New Issue
Block a user