Skip to content

Commit 017e7c4

Browse files
maxholmanclaude
andcommitted
feat(routes): warn when manual route targets a CIDR not advertised by the peer
When a user adds a route pointing at a peer that advertises auto-managed routes (from its handshake), but none of those routes cover the requested CIDR, the route is silently accepted yet traffic will never reach its destination. This commit makes the gap visible across all interfaces: - IPC layer logs a tracing::warn and returns the warning in OkResponse.warning - REPL/CLI prints the warning to stderr after "OK" - MCP tool appends it to the success text - REST API includes it in the SuccessResponse.message field If the peer has no auto-managed routes (e.g. explicit-mode peers that don't advertise routes), no warning is issued. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 1ebb4af commit 017e7c4

9 files changed

Lines changed: 217 additions & 16 deletions

File tree

crates/api/src/handlers.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -392,11 +392,15 @@ pub async fn add_route(
392392

393393
match resp {
394394
Ok(r) => match r.response {
395-
Some(management_response::Response::Ok(_)) => (
395+
Some(management_response::Response::Ok(ok)) => (
396396
StatusCode::CREATED,
397397
Json(SuccessResponse {
398398
success: true,
399-
message: None,
399+
message: if ok.warning.is_empty() {
400+
None
401+
} else {
402+
Some(ok.warning)
403+
},
400404
}),
401405
),
402406
Some(management_response::Response::Error(e)) => (

crates/cli/src/output.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,11 @@ pub fn print_response(resp: &ManagementResponse) -> Result<(), CtlError> {
157157
println!("Fingerprint: {}", l.fingerprint);
158158
}
159159
}
160-
Some(management_response::Response::Ok(_)) => {
160+
Some(management_response::Response::Ok(ok)) => {
161161
println!("OK");
162+
if !ok.warning.is_empty() {
163+
eprintln!("warning: {}", ok.warning);
164+
}
162165
}
163166
Some(management_response::Response::Error(e)) => {
164167
return Err(CtlError::Daemon(e.message.clone()));

crates/core/src/control/handler.rs

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -430,15 +430,32 @@ impl crate::node_api::NodeApi for Handler {
430430
))
431431
}
432432

433-
fn add_route(&self, cidr: crate::Cidr, peer: String) -> crate::node_api::Result<()> {
433+
fn add_route(
434+
&self,
435+
cidr: crate::Cidr,
436+
peer: String,
437+
) -> crate::node_api::Result<Option<String>> {
434438
// Resolve peer name by prefix (will error if not found or ambiguous)
435439
let peer_info = self.peers.find_by_prefix(&peer)?;
436440

437-
let (_, new_entry) = self.routes.add(cidr, peer_info.name);
441+
let (_, new_entry) = self.routes.add(cidr, peer_info.name.clone());
438442
let _ = self
439443
.route_updates
440444
.send(super::routes::RouteUpdate::Add(new_entry));
441-
Ok(())
445+
446+
// Warn if the peer advertises routes but none of them cover the new CIDR.
447+
// If the peer has no auto-routes at all, it may not advertise routes at all
448+
// (e.g. an explicit-mode peer), so silence the warning in that case.
449+
let auto_routes = self.routes.auto_routes_for_peer(&peer_info.name);
450+
if !auto_routes.is_empty() && !auto_routes.iter().any(|r| r.cidr.contains(&cidr)) {
451+
let warning = format!(
452+
"peer {} does not advertise a route covering {cidr}; traffic may not reach the destination",
453+
peer_info.name,
454+
);
455+
return Ok(Some(warning));
456+
}
457+
458+
Ok(None)
442459
}
443460

444461
fn route_del(&self, cidr: &crate::Cidr) -> crate::node_api::Result<()> {

crates/core/src/control/routes.rs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,21 @@ impl RouteTable {
144144
removed
145145
}
146146

147+
/// List all auto-managed routes for a specific peer.
148+
///
149+
/// Returns routes that were installed from the peer's handshake
150+
/// advertisement. Used to check whether a manually added route is
151+
/// covered by what the peer actually advertises.
152+
#[must_use]
153+
pub fn auto_routes_for_peer(&self, peer: &str) -> Vec<RouteEntry> {
154+
self.routes
155+
.load()
156+
.values()
157+
.filter(|entry| entry.peer == peer && entry.auto_managed)
158+
.cloned()
159+
.collect()
160+
}
161+
147162
/// Look up a route by CIDR.
148163
#[must_use]
149164
pub fn get(&self, cidr: &Cidr) -> Option<RouteEntry> {
@@ -258,4 +273,31 @@ mod tests {
258273
let removed = table.remove_by_peer("no-such-peer");
259274
assert!(removed.is_empty());
260275
}
276+
277+
#[test]
278+
fn test_auto_routes_for_peer() {
279+
let table = RouteTable::new();
280+
let cidr_auto: Cidr = "10.99.1.0/24".parse().unwrap();
281+
let cidr_manual: Cidr = "10.99.3.0/24".parse().unwrap();
282+
let cidr_other: Cidr = "10.99.2.0/24".parse().unwrap();
283+
284+
table.add_auto(cidr_auto, "peer-1".into());
285+
table.add(cidr_manual, "peer-1".into());
286+
table.add_auto(cidr_other, "peer-2".into());
287+
288+
let peer1_auto = table.auto_routes_for_peer("peer-1");
289+
assert_eq!(
290+
peer1_auto.len(),
291+
1,
292+
"only auto routes for peer-1 should be returned"
293+
);
294+
assert_eq!(peer1_auto[0].cidr, cidr_auto);
295+
296+
let peer2_auto = table.auto_routes_for_peer("peer-2");
297+
assert_eq!(peer2_auto.len(), 1);
298+
assert_eq!(peer2_auto[0].cidr, cidr_other);
299+
300+
let no_auto = table.auto_routes_for_peer("unknown-peer");
301+
assert!(no_auto.is_empty());
302+
}
261303
}

crates/core/src/ipc.rs

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,14 @@ fn dispatch_request(request: &ManagementRequest, api: &dyn NodeApi) -> Managemen
356356

357357
Some(management_request::Request::RouteAdd(req)) => match req.cidr.parse() {
358358
Ok(cidr) => match api.add_route(cidr, req.peer.clone()) {
359-
Ok(()) => management_response::Response::Ok(OkResponse {}),
359+
Ok(warning) => {
360+
if let Some(ref msg) = warning {
361+
tracing::warn!("{msg}");
362+
}
363+
management_response::Response::Ok(OkResponse {
364+
warning: warning.unwrap_or_default(),
365+
})
366+
}
360367
Err(e) => error_response(&e),
361368
},
362369
Err(_) => management_response::Response::Error(ErrorResponse {
@@ -367,7 +374,9 @@ fn dispatch_request(request: &ManagementRequest, api: &dyn NodeApi) -> Managemen
367374

368375
Some(management_request::Request::RouteDel(req)) => match req.cidr.parse() {
369376
Ok(cidr) => match api.route_del(&cidr) {
370-
Ok(()) => management_response::Response::Ok(OkResponse {}),
377+
Ok(()) => management_response::Response::Ok(OkResponse {
378+
warning: String::new(),
379+
}),
371380
Err(e) => error_response(&e),
372381
},
373382
Err(_) => management_response::Response::Error(ErrorResponse {
@@ -383,7 +392,9 @@ fn dispatch_request(request: &ManagementRequest, api: &dyn NodeApi) -> Managemen
383392
api.peer_disconnect(req.peer.clone())
384393
};
385394
match result {
386-
Ok(()) => management_response::Response::Ok(OkResponse {}),
395+
Ok(()) => management_response::Response::Ok(OkResponse {
396+
warning: String::new(),
397+
}),
387398
Err(ref e) => error_response(e),
388399
}
389400
}
@@ -412,15 +423,19 @@ fn dispatch_request(request: &ManagementRequest, api: &dyn NodeApi) -> Managemen
412423
},
413424

414425
Some(management_request::Request::Disconnect(_)) => match api.disconnect() {
415-
Ok(()) => management_response::Response::Ok(OkResponse {}),
426+
Ok(()) => management_response::Response::Ok(OkResponse {
427+
warning: String::new(),
428+
}),
416429
Err(e) => error_response(&e),
417430
},
418431

419432
Some(management_request::Request::Shutdown(_)) => {
420433
// Shutdown is handled by the caller via DaemonHandle, not NodeApi.
421434
// Return Ok here — the daemon layer should intercept ShutdownRequest
422435
// before it reaches dispatch, or handle it after dispatch returns.
423-
management_response::Response::Ok(OkResponse {})
436+
management_response::Response::Ok(OkResponse {
437+
warning: String::new(),
438+
})
424439
}
425440

426441
Some(management_request::Request::HintSet(req)) => {
@@ -431,13 +446,17 @@ fn dispatch_request(request: &ManagementRequest, api: &dyn NodeApi) -> Managemen
431446
target: target.into(),
432447
};
433448
match api.hint_set(hint) {
434-
Ok(()) => management_response::Response::Ok(OkResponse {}),
449+
Ok(()) => management_response::Response::Ok(OkResponse {
450+
warning: String::new(),
451+
}),
435452
Err(e) => error_response(&e),
436453
}
437454
}
438455

439456
Some(management_request::Request::HintSetAuto(_)) => match api.hint_set_auto() {
440-
Ok(()) => management_response::Response::Ok(OkResponse {}),
457+
Ok(()) => management_response::Response::Ok(OkResponse {
458+
warning: String::new(),
459+
}),
441460
Err(e) => error_response(&e),
442461
},
443462

crates/core/src/node_api.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,11 @@ pub trait NodeApi: Send + Sync {
195195
///
196196
/// Only supported on entry nodes. Returns error for exit/relay nodes.
197197
/// Peer must be directly connected.
198-
fn add_route(&self, cidr: Cidr, peer: String) -> Result<()>;
198+
///
199+
/// Returns `Ok(Some(warning))` when the route was added but the peer's
200+
/// advertised routes do not cover the requested CIDR, meaning traffic
201+
/// may be silently dropped. Returns `Ok(None)` on clean success.
202+
fn add_route(&self, cidr: Cidr, peer: String) -> Result<Option<String>>;
199203

200204
/// Delete a route by CIDR.
201205
///

crates/core/src/types.rs

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,44 @@ impl Cidr {
8787
pub fn prefix_len(&self) -> u8 {
8888
self.prefix_len
8989
}
90+
91+
/// Returns `true` if `other` is a subnet of (or equal to) `self`.
92+
///
93+
/// Requires the same IP family. A CIDR contains another when the
94+
/// other's prefix length is at least as long (i.e. it is more specific
95+
/// or equal), and the other's network address falls within this network.
96+
#[must_use]
97+
pub fn contains(&self, other: &Cidr) -> bool {
98+
match (self.addr, other.addr) {
99+
(IpAddr::V4(self_v4), IpAddr::V4(other_v4)) => {
100+
if other.prefix_len < self.prefix_len {
101+
return false;
102+
}
103+
let self_bits = u32::from(self_v4);
104+
let other_bits = u32::from(other_v4);
105+
if self.prefix_len == 0 {
106+
true
107+
} else {
108+
let shift = 32u32.saturating_sub(u32::from(self.prefix_len));
109+
(self_bits >> shift) == (other_bits >> shift)
110+
}
111+
}
112+
(IpAddr::V6(self_v6), IpAddr::V6(other_v6)) => {
113+
if other.prefix_len < self.prefix_len {
114+
return false;
115+
}
116+
let self_bits = u128::from(self_v6);
117+
let other_bits = u128::from(other_v6);
118+
if self.prefix_len == 0 {
119+
true
120+
} else {
121+
let shift = 128u32.saturating_sub(u32::from(self.prefix_len));
122+
(self_bits >> shift) == (other_bits >> shift)
123+
}
124+
}
125+
_ => false, // mixed IP families never contain each other
126+
}
127+
}
90128
}
91129

92130
impl FromStr for Cidr {
@@ -246,6 +284,72 @@ mod tests {
246284
assert_eq!(NodeRole::Exit.to_string(), "exit");
247285
}
248286

287+
#[test]
288+
fn test_cidr_contains_ipv4() {
289+
let wide: Cidr = "10.0.0.0/8".parse().unwrap();
290+
let narrow: Cidr = "10.99.3.0/24".parse().unwrap();
291+
let equal: Cidr = "10.0.0.0/8".parse().unwrap();
292+
let unrelated: Cidr = "192.168.0.0/16".parse().unwrap();
293+
let broader: Cidr = "10.0.0.0/4".parse().unwrap();
294+
let host: Cidr = "10.99.3.42/32".parse().unwrap();
295+
296+
assert!(
297+
wide.contains(&narrow),
298+
"10.0.0.0/8 should contain 10.99.3.0/24"
299+
);
300+
assert!(wide.contains(&equal), "a CIDR should contain itself");
301+
assert!(
302+
!wide.contains(&unrelated),
303+
"10.0.0.0/8 should not contain 192.168.0.0/16"
304+
);
305+
assert!(
306+
!wide.contains(&broader),
307+
"10.0.0.0/8 should not contain 10.0.0.0/4"
308+
);
309+
assert!(
310+
wide.contains(&host),
311+
"10.0.0.0/8 should contain a host address within it"
312+
);
313+
assert!(
314+
!narrow.contains(&wide),
315+
"subnet should not contain supernet"
316+
);
317+
}
318+
319+
#[test]
320+
fn test_cidr_contains_default_route() {
321+
let default_route: Cidr = "0.0.0.0/0".parse().unwrap();
322+
let any: Cidr = "192.168.100.0/24".parse().unwrap();
323+
assert!(
324+
default_route.contains(&any),
325+
"0.0.0.0/0 should contain any IPv4 CIDR"
326+
);
327+
}
328+
329+
#[test]
330+
fn test_cidr_contains_ipv6() {
331+
let wide: Cidr = "fd00::/8".parse().unwrap();
332+
let narrow: Cidr = "fd00:1234::/32".parse().unwrap();
333+
let unrelated: Cidr = "2001:db8::/32".parse().unwrap();
334+
335+
assert!(
336+
wide.contains(&narrow),
337+
"fd00::/8 should contain fd00:1234::/32"
338+
);
339+
assert!(
340+
!wide.contains(&unrelated),
341+
"fd00::/8 should not contain 2001:db8::/32"
342+
);
343+
}
344+
345+
#[test]
346+
fn test_cidr_contains_mixed_families() {
347+
let v4: Cidr = "10.0.0.0/8".parse().unwrap();
348+
let v6: Cidr = "fd00::/8".parse().unwrap();
349+
assert!(!v4.contains(&v6), "IPv4 CIDR should not contain IPv6 CIDR");
350+
assert!(!v6.contains(&v4), "IPv6 CIDR should not contain IPv4 CIDR");
351+
}
352+
249353
#[test]
250354
fn test_cidr_equality_and_hash() {
251355
use std::collections::HashSet;

crates/mcp/src/convert.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,13 @@ pub fn format_response(resp: &ManagementResponse) -> Result<String, String> {
8989
}
9090
Ok(out)
9191
}
92-
Some(management_response::Response::Ok(_)) => Ok("OK".to_string()),
92+
Some(management_response::Response::Ok(ok)) => {
93+
if ok.warning.is_empty() {
94+
Ok("OK".to_string())
95+
} else {
96+
Ok(format!("OK\nwarning: {}", ok.warning))
97+
}
98+
}
9399
Some(management_response::Response::Error(e)) => Err(e.message.clone()),
94100
None => Err("empty response from daemon".to_string()),
95101
}

crates/wire/proto/management.proto

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,9 @@ enum ErrorCode {
215215
ERROR_CODE_PEER_AMBIGUOUS = 9;
216216
}
217217

218-
message OkResponse {}
218+
message OkResponse {
219+
string warning = 1; // non-fatal advisory (e.g. route reachability warning); empty when none
220+
}
219221

220222
message ConnectResponse {
221223
string peer_addr = 1; // resolved peer address

0 commit comments

Comments
 (0)