From 054b7a18962c49e87aeb1e776aaf59cc6415c84b Mon Sep 17 00:00:00 2001 From: Trey Aspelund Date: Thu, 4 Jun 2026 16:37:52 -0600 Subject: [PATCH 1/5] falcon-lab: improve failure-time diagnostics collection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a trio test fails, capture more of the state needed to debug it, and fix gaps that left peer diagnostics empty: - ox node: kernel neighbor caches (ndp -a -n, arp -a -n) plus, per BGP router, mgd's NDP-manager state and per-interface discovered peers via the admin API. These surface the link-locals each unnumbered interface learned. - EOS node: disable paging (terminal length 0) and capture each show command separately — a single bundled Cli call stalled on the pager and produced nothing. Also capture the ceos container log and host interface state. - FRR node: trim and drop blank lines when flattening the vtysh script; indented/empty lines became '-c " show ..."' / '-c ""', which vtysh rejects, yielding empty output. Signed-off-by: Trey Aspelund --- falcon-lab/src/eos.rs | 59 ++++++++++++++++++++++++++------------- falcon-lab/src/frr.rs | 2 +- falcon-lab/src/illumos.rs | 2 ++ falcon-lab/src/mgd.rs | 43 ++++++++++++++++++++++++++++ falcon-lab/src/test.rs | 2 ++ 5 files changed, 88 insertions(+), 20 deletions(-) diff --git a/falcon-lab/src/eos.rs b/falcon-lab/src/eos.rs index 2b644a73..fae9a2b2 100644 --- a/falcon-lab/src/eos.rs +++ b/falcon-lab/src/eos.rs @@ -64,29 +64,50 @@ impl EosNode { LinuxNode(self.0) } - /// Capture BGP / BFD / routing state via the Arista CLI. + /// Capture BGP / BFD / routing state via the Arista CLI, plus container + /// and host state. pub async fn collect_diagnostics(&self, d: &Runner, topo: &str) { let name = self.name(d); - // `Cli -c` takes a single newline-separated script. - const SCRIPT: &str = "enable - show running-config - show ip interface brief - show ip bgp summary - show ip bgp - show ipv6 bgp - show ip route - show ipv6 route - show bfd peers - "; - match self.shell(d, SCRIPT).await { - Ok(out) => crate::diagnostics::write_artifact( + for (suffix, cmd) in [ + ("running-config", "show running-config"), + ("ip-interface-brief", "show ip interface brief"), + ("ip-bgp-summary", "show ip bgp summary"), + ("ip-bgp", "show ip bgp"), + ("ipv6-bgp", "show ipv6 bgp"), + ("ip-route", "show ip route"), + ("ipv6-route", "show ipv6 route"), + ("bfd-peers", "show bfd peers"), + ] { + let script = format!("enable\n{cmd}"); + match self.shell(d, &script).await { + Ok(out) => crate::diagnostics::write_artifact( + d, + topo, + &format!("{name}-{suffix}"), + Some(cmd), + &out, + ), + Err(e) => { + slog::warn!(d.log, "diagnostics {name}-{suffix}: {e}") + } + } + } + // Container log plus host-side interface state, so there is something + // to look at even if the CLI is unresponsive. + for (suffix, cmd) in [ + ("ceos-logs", "docker logs --tail 200 ceos"), + ("host-ip-link", "ip -d -s link show"), + ("host-ip-addr", "ip -d -s addr show"), + ("host-ip-neigh", "ip -d -s neigh show"), + ] { + crate::diagnostics::capture( d, + self.0, topo, - &format!("{name}-cli"), - None, - &out, - ), - Err(e) => slog::warn!(d.log, "diagnostics {name}-cli: {e}"), + &format!("{name}-{suffix}"), + cmd, + ) + .await; } } diff --git a/falcon-lab/src/frr.rs b/falcon-lab/src/frr.rs index ed8a12e1..25b2d8bf 100644 --- a/falcon-lab/src/frr.rs +++ b/falcon-lab/src/frr.rs @@ -134,7 +134,7 @@ impl FrrNode { .collect::>() .join(" "); let output = d - .exec(self.0, &format!("vtysh {args}")) + .exec(self.0, &format!("/usr/bin/vtysh {args}")) .await .context("vtysh shell failed")?; Ok(output) diff --git a/falcon-lab/src/illumos.rs b/falcon-lab/src/illumos.rs index 1fa86ac4..19891cf2 100644 --- a/falcon-lab/src/illumos.rs +++ b/falcon-lab/src/illumos.rs @@ -144,6 +144,8 @@ impl IllumosNode { ("ipadm", "ipadm show-addr"), ("dladm", "dladm show-link"), ("netstat", "netstat -nr"), + ("ndp", "ndp -a -n"), + ("arp", "arp -a -n"), ] { crate::diagnostics::capture( d, diff --git a/falcon-lab/src/mgd.rs b/falcon-lab/src/mgd.rs index e8a4a3f2..a540fa28 100644 --- a/falcon-lab/src/mgd.rs +++ b/falcon-lab/src/mgd.rs @@ -62,6 +62,49 @@ impl MgdNode { Err(e) => slog::warn!(d.log, "diagnostics {label}: {e}"), } } + + pub async fn collect_ndp_diagnostics( + &self, + d: &Runner, + client: &Client, + topo: &str, + ) { + let name = d.get_node(self.0).name.clone(); + let routers = match client.read_routers().await { + Ok(r) => r.into_inner(), + Err(e) => { + slog::warn!(d.log, "diagnostics {name}-ndp: read routers: {e}"); + return; + } + }; + for router in routers { + let asn = router.asn; + for (suffix, result) in [ + ( + "ndp-manager", + client + .get_ndp_manager_state(asn) + .await + .map(|r| format!("{:#?}", r.into_inner())), + ), + ( + "ndp-interfaces", + client + .get_ndp_interfaces(asn) + .await + .map(|r| format!("{:#?}", r.into_inner())), + ), + ] { + let label = format!("{name}-{suffix}-asn{asn}"); + match result { + Ok(contents) => crate::diagnostics::write_artifact( + d, topo, &label, None, &contents, + ), + Err(e) => slog::warn!(d.log, "diagnostics {label}: {e}"), + } + } + } + } } pub async fn wait_for_mgd( diff --git a/falcon-lab/src/test.rs b/falcon-lab/src/test.rs index e5184bf2..ebcfe099 100644 --- a/falcon-lab/src/test.rs +++ b/falcon-lab/src/test.rs @@ -97,11 +97,13 @@ where let ox = bt.ox; let cr1 = bt.cr1; let cr2 = bt.cr2; + let mgd = bt.mgd.clone(); let topo_name = bt.topo_name.clone(); let result = body(bt).await; if let Err(e) = &result { warn!(ad.log, "{topo_name} failed: {e:#}"); collect_diagnostics(&ad, ox, cr1, cr2, &topo_name).await; + ox.collect_ndp_diagnostics(&ad, &mgd, &topo_name).await; } result } From be1f61a358cd370442018d1cefc5752f48e92b9c Mon Sep 17 00:00:00 2001 From: Trey Aspelund Date: Thu, 4 Jun 2026 16:37:52 -0600 Subject: [PATCH 2/5] ndp: scope discovery sockets to their interface Each per-interface NDP socket binds to the unspecified address to catch both unicast and multicast solicitations/advertisements. That bind does not scope reception to the interface: illumos only honors the bind sockaddr's scope_id for a link-scoped address, so ::%index leaves the socket's incoming ifindex unset. Multicast reception is still constrained by per-interface group membership, but unicast ICMPv6 is delivered to every NDP socket in the process regardless of arrival link, so a unicast RA from an off-link source can poison an interface's discovered peer and break unnumbered peering. Bind each socket to its interface via socket2's bind_device_by_index_v6 (IPV6_BOUND_IF on illumos, SO_BINDTOIFINDEX on Linux), scoping both unicast and multicast reception to the intended interface. Signed-off-by: Trey Aspelund --- ndp/src/util.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ndp/src/util.rs b/ndp/src/util.rs index d4d52ef0..788372f3 100644 --- a/ndp/src/util.rs +++ b/ndp/src/util.rs @@ -11,6 +11,7 @@ use socket2::{Domain, Protocol, Socket, Type}; use std::{ ffi::c_void, net::{Ipv6Addr, SocketAddrV6}, + num::NonZeroU32, os::fd::AsRawFd, thread::sleep, time::{Duration, Instant}, @@ -72,6 +73,12 @@ pub enum ListeningSocketError { #[error("failed to set ipv6 min hop count: {0}")] SetIpv6MinHopCount(std::io::Error), + + #[error("failed to bind socket to interface: {0}")] + SetBoundIf(std::io::Error), + + #[error("interface index must be non-zero")] + InvalidInterfaceIndex, } pub fn send_ra( @@ -205,6 +212,10 @@ pub fn create_socket(index: u32) -> Result { s.join_multicast_v6(&ALL_ROUTERS_MCAST, index) .map_err(E::JoinAllRoutersMulticast)?; + let ifindex = NonZeroU32::new(index).ok_or(E::InvalidInterfaceIndex)?; + s.bind_device_by_index_v6(Some(ifindex)) + .map_err(E::SetBoundIf)?; + s.bind(&sa).map_err(ListeningSocketError::Bind)?; s.set_read_timeout(Some(READ_TIMEOUT)) From cc5d3cdd5717568ffca6cd8601a51183f4e18c71 Mon Sep 17 00:00:00 2001 From: Trey Aspelund Date: Thu, 4 Jun 2026 20:06:09 -0600 Subject: [PATCH 3/5] =?UTF-8?q?ndp:=20validate=20received=20router=20adver?= =?UTF-8?q?tisements=20per=20RFC=204861=20=C2=A76.1.2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply the RFC 4861 §6.1.2 RA validity checks that aren't otherwise enforced: - Source must be link-local: the kernel does not filter raw-socket delivery by source scope, so a globally- or otherwise-scoped source would be accepted. Reject non-link-local sources in handle_ra. (This does not guard against an off-link link-local source on the wrong interface; interface scoping handles that — see the discovery-socket commit.) - ICMP length >= 16 octets: guard in Icmp6RouterAdvertisement::from_wire. Besides matching the RFC, this prevents a panic: the ispf deserializer indexes the input directly and panics on a buffer shorter than the struct rather than returning an error. - Hop limit 255: already enforced by the kernel via IPV6_MINHOPCOUNT, which was previously set with a hardcoded illumos constant in an ungated unsafe block. Move it behind a cfg-gated helper that works on Linux (libc constant) and illumos (value from ), no-op elsewhere. The checksum check is left to the kernel, which verifies the ICMPv6 checksum before raw delivery. Signed-off-by: Trey Aspelund --- ndp/src/manager.rs | 12 ++++++++++- ndp/src/packet.rs | 8 +++++++ ndp/src/util.rs | 52 +++++++++++++++++++++++++++++++--------------- 3 files changed, 54 insertions(+), 18 deletions(-) diff --git a/ndp/src/manager.rs b/ndp/src/manager.rs index a80b5afe..b1a53d7b 100644 --- a/ndp/src/manager.rs +++ b/ndp/src/manager.rs @@ -11,7 +11,7 @@ use crate::util::{ }; use mg_common::thread::ManagedThread; use mg_common::{lock, read_lock, write_lock}; -use slog::{Logger, error}; +use slog::{Logger, debug, error}; use socket2::Socket; use std::mem::MaybeUninit; use std::net::Ipv6Addr; @@ -340,6 +340,16 @@ impl InterfaceNdpManagerInner { /// is updated as well as the time of reception and the stored advertisement /// containing the reachable time. fn handle_ra(&self, ra: Icmp6RouterAdvertisement, src: Ipv6Addr) { + // Per RFC 4861 Section 6.1.2: a valid RA's source is always link-local + if !src.is_unicast_link_local() { + debug!( + self.log, + "ignoring RA from non-link-local source {src} on {}", + self.ifx.name, + ); + return; + } + let mut guard = lock!(self.neighbor_router); let now = Instant::now(); diff --git a/ndp/src/packet.rs b/ndp/src/packet.rs index 5874f165..c051dc6d 100644 --- a/ndp/src/packet.rs +++ b/ndp/src/packet.rs @@ -39,8 +39,13 @@ impl Icmp6RouterAdvertisement { const TYPE: u8 = 134; const CODE: u8 = 0; const DEFAULT_HOPLIMIT: u8 = 255; + const MIN_PAYLOAD_LEN: usize = 16; pub fn from_wire(buf: &[u8]) -> Result { + // Per RFC 4861 Section 6.1.2: a valid RA has an ICMP payload of >= 16b + if buf.len() < Self::MIN_PAYLOAD_LEN { + return Err(Icmp6RaFromWireError::TooShort(buf.len())); + } let s: Self = ispf::from_bytes_be(buf)?; if s.typ != Self::TYPE { return Err(Icmp6RaFromWireError::WrongType(s.typ)); @@ -86,6 +91,9 @@ pub enum Icmp6RaFromWireError { #[error("deserialization error: {0}")] Ispf(#[from] ispf::Error), + #[error("too short: {0} octets, expected at least 16")] + TooShort(usize), + #[error("wrong type: expected {expected}, got {0}", expected = Icmp6RouterAdvertisement::TYPE)] WrongType(u8), diff --git a/ndp/src/util.rs b/ndp/src/util.rs index 788372f3..dbdc44a5 100644 --- a/ndp/src/util.rs +++ b/ndp/src/util.rs @@ -5,14 +5,11 @@ // Copyright 2026 Oxide Computer Company use crate::packet::{Icmp6RouterAdvertisement, Icmp6RouterSolicitation}; -use libc::{c_int, socklen_t}; use slog::{Logger, error}; use socket2::{Domain, Protocol, Socket, Type}; use std::{ - ffi::c_void, net::{Ipv6Addr, SocketAddrV6}, num::NonZeroU32, - os::fd::AsRawFd, thread::sleep, time::{Duration, Instant}, }; @@ -71,6 +68,7 @@ pub enum ListeningSocketError { #[error("set read timeout error: {0}")] SetReadTimeoutError(std::io::Error), + #[cfg(any(target_os = "linux", target_os = "illumos"))] #[error("failed to set ipv6 min hop count: {0}")] SetIpv6MinHopCount(std::io::Error), @@ -221,23 +219,43 @@ pub fn create_socket(index: u32) -> Result { s.set_read_timeout(Some(READ_TIMEOUT)) .map_err(E::SetReadTimeoutError)?; - unsafe { - // from - const IPV6_MINHOPCOUNT: c_int = 0x2f; - let min_hops: c_int = 255; - let rc = libc::setsockopt( + // Per RFC 4861 Section 6.1.2: a valid RA must have a hop limit of 255 + set_min_hopcount(&s)?; + + Ok(s) +} + +#[cfg(any(target_os = "linux", target_os = "illumos"))] +fn set_min_hopcount(s: &Socket) -> Result<(), ListeningSocketError> { + use std::os::fd::AsRawFd; + + // illumos does not export IPV6_MINHOPCOUNT via libc; value from + // . Linux provides it. + #[cfg(target_os = "illumos")] + const IPV6_MINHOPCOUNT: libc::c_int = 0x2f; + #[cfg(target_os = "linux")] + use libc::IPV6_MINHOPCOUNT; + + let min_hops: libc::c_int = 255; + // SAFETY: setsockopt with a correctly-sized pointer to an integer option. + let rc = unsafe { + libc::setsockopt( s.as_raw_fd(), libc::IPPROTO_IPV6, IPV6_MINHOPCOUNT, - &min_hops as *const _ as *const c_void, - std::mem::size_of::() as socklen_t, - ); - if rc < 0 { - return Err(ListeningSocketError::SetIpv6MinHopCount( - std::io::Error::last_os_error(), - )); - } + &min_hops as *const _ as *const libc::c_void, + std::mem::size_of::() as libc::socklen_t, + ) + }; + if rc < 0 { + return Err(ListeningSocketError::SetIpv6MinHopCount( + std::io::Error::last_os_error(), + )); } + Ok(()) +} - Ok(s) +#[cfg(not(any(target_os = "linux", target_os = "illumos")))] +fn set_min_hopcount(_s: &Socket) -> Result<(), ListeningSocketError> { + Ok(()) } From d808f72477d9f31d1376750da242e7fb2b0df2a1 Mon Sep 17 00:00:00 2001 From: Trey Aspelund Date: Fri, 5 Jun 2026 00:08:54 -0600 Subject: [PATCH 4/5] mg-api: add v11 NDP_NO_ASN, make NDP endpoints ASN-agnostic Signed-off-by: Trey Aspelund --- falcon-lab/src/mgd.rs | 52 ++-- mg-api-types/versions/src/latest.rs | 3 +- mg-api-types/versions/src/lib.rs | 2 + mg-api-types/versions/src/ndp_no_asn/mod.rs | 9 + mg-api-types/versions/src/ndp_no_asn/ndp.rs | 11 + mg-api/src/lib.rs | 44 ++- mgadm/src/ndp.rs | 49 ++-- mgd/src/admin.rs | 24 +- mgd/src/bgp_admin.rs | 204 +------------ mgd/src/main.rs | 1 + mgd/src/ndp_admin.rs | 268 ++++++++++++++++++ .../mg-admin-10.0.0-c462c6.json.gitstub | 1 + ...462c6.json => mg-admin-11.0.0-f87034.json} | 42 +-- openapi/mg-admin/mg-admin-latest.json | 2 +- 14 files changed, 392 insertions(+), 320 deletions(-) create mode 100644 mg-api-types/versions/src/ndp_no_asn/mod.rs create mode 100644 mg-api-types/versions/src/ndp_no_asn/ndp.rs create mode 100644 mgd/src/ndp_admin.rs create mode 100644 openapi/mg-admin/mg-admin-10.0.0-c462c6.json.gitstub rename openapi/mg-admin/{mg-admin-10.0.0-c462c6.json => mg-admin-11.0.0-f87034.json} (99%) diff --git a/falcon-lab/src/mgd.rs b/falcon-lab/src/mgd.rs index a540fa28..b0dbef97 100644 --- a/falcon-lab/src/mgd.rs +++ b/falcon-lab/src/mgd.rs @@ -70,38 +70,28 @@ impl MgdNode { topo: &str, ) { let name = d.get_node(self.0).name.clone(); - let routers = match client.read_routers().await { - Ok(r) => r.into_inner(), - Err(e) => { - slog::warn!(d.log, "diagnostics {name}-ndp: read routers: {e}"); - return; - } - }; - for router in routers { - let asn = router.asn; - for (suffix, result) in [ - ( - "ndp-manager", - client - .get_ndp_manager_state(asn) - .await - .map(|r| format!("{:#?}", r.into_inner())), - ), - ( - "ndp-interfaces", - client - .get_ndp_interfaces(asn) - .await - .map(|r| format!("{:#?}", r.into_inner())), + for (suffix, result) in [ + ( + "ndp-manager", + client + .get_ndp_manager_state() + .await + .map(|r| format!("{:#?}", r.into_inner())), + ), + ( + "ndp-interfaces", + client + .get_ndp_interfaces() + .await + .map(|r| format!("{:#?}", r.into_inner())), + ), + ] { + let label = format!("{name}-{suffix}"); + match result { + Ok(contents) => crate::diagnostics::write_artifact( + d, topo, &label, None, &contents, ), - ] { - let label = format!("{name}-{suffix}-asn{asn}"); - match result { - Ok(contents) => crate::diagnostics::write_artifact( - d, topo, &label, None, &contents, - ), - Err(e) => slog::warn!(d.log, "diagnostics {label}: {e}"), - } + Err(e) => slog::warn!(d.log, "diagnostics {label}: {e}"), } } } diff --git a/mg-api-types/versions/src/latest.rs b/mg-api-types/versions/src/latest.rs index 7ac0125d..efad580b 100644 --- a/mg-api-types/versions/src/latest.rs +++ b/mg-api-types/versions/src/latest.rs @@ -177,11 +177,12 @@ pub mod rdb { pub mod ndp { pub use crate::v5::ndp::NdpInterface; - pub use crate::v5::ndp::NdpInterfaceSelector; pub use crate::v5::ndp::NdpManagerState; pub use crate::v5::ndp::NdpPeer; pub use crate::v5::ndp::NdpPendingInterface; pub use crate::v5::ndp::NdpThreadState; + + pub use crate::v11::ndp::NdpInterfaceSelector; } pub mod rib { diff --git a/mg-api-types/versions/src/lib.rs b/mg-api-types/versions/src/lib.rs index e5b0150c..deaf2d98 100644 --- a/mg-api-types/versions/src/lib.rs +++ b/mg-api-types/versions/src/lib.rs @@ -35,6 +35,8 @@ pub mod latest; pub mod v1; #[path = "v4_over_v6_static_routes/mod.rs"] pub mod v10; +#[path = "ndp_no_asn/mod.rs"] +pub mod v11; #[path = "ipv6_basic/mod.rs"] pub mod v2; #[path = "switch_identifiers/mod.rs"] diff --git a/mg-api-types/versions/src/ndp_no_asn/mod.rs b/mg-api-types/versions/src/ndp_no_asn/mod.rs new file mode 100644 index 00000000..7af63bbb --- /dev/null +++ b/mg-api-types/versions/src/ndp_no_asn/mod.rs @@ -0,0 +1,9 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! v11 (NDP_NO_ASN): the NDP admin endpoints are one-per-mgd-instance, so the +//! BGP ASN was dropped from their selectors. Only `NdpInterfaceSelector` +//! changed shape; the response types are unchanged and remain at v5. + +pub mod ndp; diff --git a/mg-api-types/versions/src/ndp_no_asn/ndp.rs b/mg-api-types/versions/src/ndp_no_asn/ndp.rs new file mode 100644 index 00000000..802c6882 --- /dev/null +++ b/mg-api-types/versions/src/ndp_no_asn/ndp.rs @@ -0,0 +1,11 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Deserialize, Serialize, JsonSchema, Clone)] +pub struct NdpInterfaceSelector { + pub interface_name: String, +} diff --git a/mg-api/src/lib.rs b/mg-api/src/lib.rs index a88963a0..168cd8b9 100644 --- a/mg-api/src/lib.rs +++ b/mg-api/src/lib.rs @@ -24,6 +24,7 @@ api_versions!([ // | example for the next person. // v // (next_int, IDENT), + (11, NDP_NO_ASN), (10, V4_OVER_V6_STATIC_ROUTES), (9, ENDPOINT_RENAME), (8, BGP_SRC_ADDR), @@ -1249,30 +1250,63 @@ pub trait MgAdminApi { #[endpoint { method = GET, path = "/ndp/manager", - versions = VERSION_UNNUMBERED.., + versions = VERSION_NDP_NO_ASN.., }] async fn get_ndp_manager_state( rqctx: RequestContext, - request: Query, ) -> Result, HttpError>; + #[endpoint { + method = GET, + path = "/ndp/manager", + operation_id = "get_ndp_manager_state", + versions = VERSION_UNNUMBERED..VERSION_NDP_NO_ASN, + }] + async fn get_ndp_manager_state_v5( + rqctx: RequestContext, + _request: Query, + ) -> Result, HttpError> { + Self::get_ndp_manager_state(rqctx).await + } + #[endpoint { method = GET, path = "/ndp/interfaces", - versions = VERSION_UNNUMBERED.., + versions = VERSION_NDP_NO_ASN.., }] async fn get_ndp_interfaces( rqctx: RequestContext, - request: Query, + ) -> Result>, HttpError>; + + #[endpoint { + method = GET, + path = "/ndp/interfaces", + operation_id = "get_ndp_interfaces", + versions = VERSION_UNNUMBERED..VERSION_NDP_NO_ASN, + }] + async fn get_ndp_interfaces_v5( + rqctx: RequestContext, + request: Query, ) -> Result>, HttpError>; #[endpoint { method = GET, path = "/ndp/interface", - versions = VERSION_UNNUMBERED.., + versions = VERSION_NDP_NO_ASN.., }] async fn get_ndp_interface_detail( rqctx: RequestContext, request: Query, ) -> Result, HttpError>; + + #[endpoint { + method = GET, + path = "/ndp/interface", + operation_id = "get_ndp_interface_detail", + versions = VERSION_UNNUMBERED..VERSION_NDP_NO_ASN, + }] + async fn get_ndp_interface_detail_v5( + rqctx: RequestContext, + request: Query, + ) -> Result, HttpError>; } diff --git a/mgadm/src/ndp.rs b/mgadm/src/ndp.rs index 208db363..1e1661e4 100644 --- a/mgadm/src/ndp.rs +++ b/mgadm/src/ndp.rs @@ -25,42 +25,32 @@ pub struct StatusArgs { #[derive(Subcommand, Debug)] pub enum StatusCmd { /// Show NDP manager state - Manager { - #[clap(env)] - asn: u32, - }, + Manager, /// List all NDP-managed interfaces - Interfaces { - #[clap(env)] - asn: u32, - }, + Interfaces, /// Show detailed state for a specific interface - Interface { - interface: String, - #[clap(env)] - asn: u32, - }, + Interface { interface: String }, } pub async fn commands(command: Commands, c: Client) -> Result<()> { match command { Commands::Status(args) => match args.command { - StatusCmd::Manager { asn } => ndp_manager_status(asn, c).await?, - StatusCmd::Interfaces { asn } => ndp_interfaces(asn, c).await?, - StatusCmd::Interface { asn, interface } => { - ndp_interface_detail(asn, interface, c).await? + StatusCmd::Manager => ndp_manager_status(c).await?, + StatusCmd::Interfaces => ndp_interfaces(c).await?, + StatusCmd::Interface { interface } => { + ndp_interface_detail(interface, c).await? } }, } Ok(()) } -async fn ndp_manager_status(asn: u32, c: Client) -> Result<()> { - let state = c.get_ndp_manager_state(asn).await?.into_inner(); +async fn ndp_manager_status(c: Client) -> Result<()> { + let state = c.get_ndp_manager_state().await?.into_inner(); - println_nopipe!("NDP Manager State (ASN {})", asn); + println_nopipe!("NDP Manager State"); println_nopipe!("{}", "=".repeat(60)); println_nopipe!(); @@ -104,11 +94,11 @@ async fn ndp_manager_status(asn: u32, c: Client) -> Result<()> { Ok(()) } -async fn ndp_interfaces(asn: u32, c: Client) -> Result<()> { - let interfaces = c.get_ndp_interfaces(asn).await?.into_inner(); +async fn ndp_interfaces(c: Client) -> Result<()> { + let interfaces = c.get_ndp_interfaces().await?.into_inner(); if interfaces.is_empty() { - println_nopipe!("No NDP-managed interfaces found for ASN {}", asn); + println_nopipe!("No NDP-managed interfaces found"); return Ok(()); } @@ -128,7 +118,7 @@ async fn ndp_interfaces(asn: u32, c: Client) -> Result<()> { for iface in interfaces { let (peer_str, reachable_str) = match &iface.discovered_peer { Some(peer) => { - let addr_str = format!("{}%{}", peer.address, iface.interface); + let addr_str = format!("{}", peer.address); let reachable = if peer.expired { "No".red() } else { @@ -173,15 +163,8 @@ async fn ndp_interfaces(asn: u32, c: Client) -> Result<()> { Ok(()) } -async fn ndp_interface_detail( - asn: u32, - interface: String, - c: Client, -) -> Result<()> { - let detail = c - .get_ndp_interface_detail(asn, &interface) - .await? - .into_inner(); +async fn ndp_interface_detail(interface: String, c: Client) -> Result<()> { + let detail = c.get_ndp_interface_detail(&interface).await?.into_inner(); println_nopipe!("NDP State: {}", interface); println_nopipe!("{}", "=".repeat(60)); diff --git a/mgd/src/admin.rs b/mgd/src/admin.rs index e0544a0b..22e88716 100644 --- a/mgd/src/admin.rs +++ b/mgd/src/admin.rs @@ -2,7 +2,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use crate::{bfd_admin, bgp_admin, rib_admin, static_admin}; +use crate::{bfd_admin, bgp_admin, ndp_admin, rib_admin, static_admin}; use bfd_admin::BfdContext; use bgp_admin::BgpContext; use dropshot::{ @@ -651,23 +651,35 @@ impl MgAdminApi for MgAdminApiImpl { async fn get_ndp_manager_state( ctx: RequestContext, - request: Query, ) -> Result, HttpError> { - bgp_admin::get_ndp_manager_state(ctx, request).await + ndp_admin::get_ndp_manager_state(ctx).await } async fn get_ndp_interfaces( ctx: RequestContext, - request: Query, ) -> Result>, HttpError> { - bgp_admin::get_ndp_interfaces(ctx, request).await + ndp_admin::get_ndp_interfaces(ctx).await + } + + async fn get_ndp_interfaces_v5( + ctx: RequestContext, + request: Query, + ) -> Result>, HttpError> { + ndp_admin::get_ndp_interfaces_v5(ctx, request).await } async fn get_ndp_interface_detail( ctx: RequestContext, request: Query, ) -> Result, HttpError> { - bgp_admin::get_ndp_interface_detail(ctx, request).await + ndp_admin::get_ndp_interface_detail(ctx, request).await + } + + async fn get_ndp_interface_detail_v5( + ctx: RequestContext, + request: Query, + ) -> Result, HttpError> { + ndp_admin::get_ndp_interface_detail_v5(ctx, request).await } } diff --git a/mgd/src/bgp_admin.rs b/mgd/src/bgp_admin.rs index 3d9c5903..4fc17b96 100644 --- a/mgd/src/bgp_admin.rs +++ b/mgd/src/bgp_admin.rs @@ -3,9 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. #![allow(clippy::type_complexity)] -use crate::unnumbered_manager::{ - NdpPeerState, NdpThreadStateInternal, UnnumberedManagerNdp, -}; +use crate::unnumbered_manager::UnnumberedManagerNdp; use crate::validation::{ validate_prefixes, validate_prefixes_v4, validate_prefixes_v6, }; @@ -21,7 +19,6 @@ use bgp::{ AdminEvent, ConnectionKind, FsmEvent, SessionInfo, SessionRunner, }, }; -use chrono::{DateTime, SecondsFormat, Utc}; use dropshot::{ ClientErrorStatusCode, HttpError, HttpResponseDeleted, HttpResponseOk, HttpResponseUpdatedNoContent, Path, Query, RequestContext, TypedBody, @@ -43,10 +40,6 @@ use mg_api_types::bgp::session::{ use mg_api_types::bgp::session::{ FsmEventRecord, FsmStateKind, MessageHistory, }; -use mg_api_types::ndp::{ - NdpInterface, NdpInterfaceSelector, NdpManagerState, NdpPeer, - NdpPendingInterface, NdpThreadState, -}; use mg_api_types::rdb::prefix::{Prefix, Prefix4, Prefix6}; use mg_api_types::rdb::rib::AddressFamily; use mg_api_types::rdb::router::BgpRouterInfo; @@ -61,7 +54,6 @@ use std::sync::{ Arc, Mutex, mpsc::{Sender, channel}, }; -use std::time::{Duration, Instant, SystemTime}; const UNIT_BGP: &str = "bgp"; const DEFAULT_BGP_LISTEN: SocketAddr = @@ -483,200 +475,6 @@ pub async fn clear_unnumbered_neighbor( .await?) } -/// Convert an Instant to an ISO 8601 timestamp string -fn instant_to_iso8601(when: Instant) -> String { - let now_instant = Instant::now(); - let now_system = SystemTime::now(); - let elapsed = now_instant.duration_since(when); - let system_time = now_system - elapsed; - DateTime::::from(system_time) - .to_rfc3339_opts(SecondsFormat::Secs, true) -} - -/// Convert NdpPeerState to API type with timestamp formatting -fn convert_ndp_peer_to_api(state: &NdpPeerState) -> NdpPeer { - let elapsed_since_when = Instant::now().duration_since(state.when); - - // Format timestamps: first_seen for when peer was discovered, - // when for when the most recent RA was received - let discovered_at = instant_to_iso8601(state.first_seen); - let last_advertisement = instant_to_iso8601(state.when); - - // Calculate time until expiry - let effective_lifetime = - Duration::from_secs(u64::from(state.router_lifetime)); - let time_until_expiry = if state.expired { - // Calculate time since expiry - let time_since_expiry = elapsed_since_when - .checked_sub(effective_lifetime) - .unwrap_or(Duration::ZERO); - Some(mg_common::format_duration_human(time_since_expiry)) - } else { - // Calculate time until expiry - let time_until = effective_lifetime - .checked_sub(elapsed_since_when) - .unwrap_or(Duration::ZERO); - Some(mg_common::format_duration_human(time_until)) - }; - - NdpPeer { - address: state.address, - discovered_at, - last_advertisement, - router_lifetime: state.router_lifetime, - reachable_time: state.reachable_time, - retrans_timer: state.retrans_timer, - expired: state.expired, - time_until_expiry, - } -} - -/// Convert internal thread state to API type -fn convert_thread_state_to_api( - state: Option<&NdpThreadStateInternal>, -) -> Option { - state.map(|s| NdpThreadState { - tx_running: s.tx_running, - rx_running: s.rx_running, - }) -} - -pub async fn get_ndp_manager_state( - rqctx: RequestContext>, - _request: Query, -) -> Result, HttpError> { - let ctx = rqctx.context(); - - // Get manager state from unnumbered manager - let manager_state = ctx.bgp.unnumbered_manager.get_manager_state(); - - // Convert pending interfaces to API type - let pending_interfaces = manager_state - .pending_interfaces - .into_iter() - .map(|p| NdpPendingInterface { - interface: p.interface, - router_lifetime: p.router_lifetime, - }) - .collect(); - - Ok(HttpResponseOk(NdpManagerState { - monitor_thread_running: manager_state.monitor_thread_running, - pending_interfaces, - active_interfaces: manager_state.active_interfaces, - })) -} - -pub async fn get_ndp_interfaces( - rqctx: RequestContext>, - request: Query, -) -> Result>, HttpError> { - let rq = request.into_inner(); - let ctx = rqctx.context(); - - // Get all unnumbered neighbors for this ASN - let unnumbered_neighbors = ctx - .db - .get_unnumbered_bgp_neighbors() - .map_err(|e| { - HttpError::for_internal_error(format!( - "failed to get unnumbered neighbors: {e}" - )) - })? - .into_iter() - .filter(|n| n.asn == rq.asn) - .collect::>(); - - // Get NDP state for managed interfaces - let ndp_state = ctx.bgp.unnumbered_manager.list_ndp_interfaces(); - - // Build response by matching neighbors to NDP state - let mut result = Vec::new(); - for neighbor in unnumbered_neighbors { - // Find NDP state for this interface - if let Some(ndp) = ndp_state - .iter() - .find(|info| info.interface == neighbor.interface) - { - let discovered_peer = - ndp.peer_state.as_ref().map(convert_ndp_peer_to_api); - let thread_state = - convert_thread_state_to_api(ndp.thread_state.as_ref()); - - result.push(NdpInterface { - interface: neighbor.interface.clone(), - local_address: ndp.local_address, - scope_id: ndp.scope_id, - router_lifetime: neighbor.router_lifetime, - discovered_peer, - thread_state, - }); - } - } - - Ok(HttpResponseOk(result)) -} - -pub async fn get_ndp_interface_detail( - rqctx: RequestContext>, - request: Query, -) -> Result, HttpError> { - let rq = request.into_inner(); - let ctx = rqctx.context(); - - // Verify this interface has an unnumbered neighbor configured for this ASN - let neighbor = ctx - .db - .get_unnumbered_bgp_neighbors() - .map_err(|e| { - HttpError::for_internal_error(format!( - "failed to get unnumbered neighbors: {e}" - )) - })? - .into_iter() - .find(|n| n.asn == rq.asn && n.interface == rq.interface) - .ok_or_else(|| { - HttpError::for_not_found( - None, - format!( - "no unnumbered neighbor for ASN {} on interface {}", - rq.asn, rq.interface - ), - ) - })?; - - // Get detailed NDP state - let unnumbered_manager = &ctx.bgp.unnumbered_manager; - - let ndp_detail = unnumbered_manager - .get_ndp_interface_detail(&rq.interface) - .map_err(|e| { - HttpError::for_internal_error(format!( - "failed to get NDP state: {e}" - )) - })? - .ok_or_else(|| { - HttpError::for_not_found( - None, - format!("interface {} not managed by NDP", rq.interface), - ) - })?; - - let discovered_peer = - ndp_detail.peer_state.as_ref().map(convert_ndp_peer_to_api); - let thread_state = - convert_thread_state_to_api(ndp_detail.thread_state.as_ref()); - - Ok(HttpResponseOk(NdpInterface { - interface: rq.interface, - local_address: ndp_detail.local_address, - scope_id: ndp_detail.scope_id, - router_lifetime: neighbor.router_lifetime, - discovered_peer, - thread_state, - })) -} - // IPv4 origin ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ pub async fn create_origin4( diff --git a/mgd/src/main.rs b/mgd/src/main.rs index 4e7fd276..9e592277 100644 --- a/mgd/src/main.rs +++ b/mgd/src/main.rs @@ -35,6 +35,7 @@ mod bfd_admin; mod bgp_admin; mod error; mod log; +mod ndp_admin; mod oxstats; mod rib_admin; mod signal; diff --git a/mgd/src/ndp_admin.rs b/mgd/src/ndp_admin.rs new file mode 100644 index 00000000..92528d4a --- /dev/null +++ b/mgd/src/ndp_admin.rs @@ -0,0 +1,268 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Admin endpoints for the NDP/unnumbered router-discovery subsystem. + +use crate::admin::HandlerContext; +use crate::unnumbered_manager::{NdpPeerState, NdpThreadStateInternal}; +use chrono::{DateTime, SecondsFormat, Utc}; +use dropshot::{HttpError, HttpResponseOk, Query, RequestContext}; +use mg_api_types::bgp::config::AsnSelector; +use mg_api_types::ndp::{ + NdpInterface, NdpInterfaceSelector, NdpManagerState, NdpPeer, + NdpPendingInterface, NdpThreadState, +}; +use mg_api_types_versions::v5; +use std::sync::Arc; +use std::time::{Duration, Instant, SystemTime}; + +/// Convert an Instant to an ISO 8601 timestamp string +fn instant_to_iso8601(when: Instant) -> String { + let now_instant = Instant::now(); + let now_system = SystemTime::now(); + let elapsed = now_instant.duration_since(when); + let system_time = now_system - elapsed; + DateTime::::from(system_time) + .to_rfc3339_opts(SecondsFormat::Secs, true) +} + +/// Convert NdpPeerState to API type with timestamp formatting +fn convert_ndp_peer_to_api(state: &NdpPeerState) -> NdpPeer { + let elapsed_since_when = Instant::now().duration_since(state.when); + + // Format timestamps: first_seen for when peer was discovered, + // when for when the most recent RA was received + let discovered_at = instant_to_iso8601(state.first_seen); + let last_advertisement = instant_to_iso8601(state.when); + + // Calculate time until expiry + let effective_lifetime = + Duration::from_secs(u64::from(state.router_lifetime)); + let time_until_expiry = if state.expired { + // Calculate time since expiry + let time_since_expiry = elapsed_since_when + .checked_sub(effective_lifetime) + .unwrap_or(Duration::ZERO); + Some(mg_common::format_duration_human(time_since_expiry)) + } else { + // Calculate time until expiry + let time_until = effective_lifetime + .checked_sub(elapsed_since_when) + .unwrap_or(Duration::ZERO); + Some(mg_common::format_duration_human(time_until)) + }; + + NdpPeer { + address: state.address, + discovered_at, + last_advertisement, + router_lifetime: state.router_lifetime, + reachable_time: state.reachable_time, + retrans_timer: state.retrans_timer, + expired: state.expired, + time_until_expiry, + } +} + +/// Convert internal thread state to API type +fn convert_thread_state_to_api( + state: Option<&NdpThreadStateInternal>, +) -> Option { + state.map(|s| NdpThreadState { + tx_running: s.tx_running, + rx_running: s.rx_running, + }) +} + +pub async fn get_ndp_manager_state( + rqctx: RequestContext>, +) -> Result, HttpError> { + let ctx = rqctx.context(); + + // Get manager state from unnumbered manager + let manager_state = ctx.bgp.unnumbered_manager.get_manager_state(); + + // Convert pending interfaces to API type + let pending_interfaces = manager_state + .pending_interfaces + .into_iter() + .map(|p| NdpPendingInterface { + interface: p.interface, + router_lifetime: p.router_lifetime, + }) + .collect(); + + Ok(HttpResponseOk(NdpManagerState { + monitor_thread_running: manager_state.monitor_thread_running, + pending_interfaces, + active_interfaces: manager_state.active_interfaces, + })) +} + +fn build_ndp_interfaces( + ctx: &HandlerContext, + neighbors: Vec, +) -> Vec { + let ndp_state = ctx.bgp.unnumbered_manager.list_ndp_interfaces(); + let mut result = Vec::new(); + for neighbor in neighbors { + if let Some(ndp) = ndp_state + .iter() + .find(|info| info.interface == neighbor.interface) + { + result.push(NdpInterface { + interface: neighbor.interface.clone(), + local_address: ndp.local_address, + scope_id: ndp.scope_id, + router_lifetime: neighbor.router_lifetime, + discovered_peer: ndp + .peer_state + .as_ref() + .map(convert_ndp_peer_to_api), + thread_state: convert_thread_state_to_api( + ndp.thread_state.as_ref(), + ), + }); + } + } + result +} + +pub async fn get_ndp_interfaces( + rqctx: RequestContext>, +) -> Result>, HttpError> { + let ctx = rqctx.context(); + let unnumbered_neighbors = + ctx.db.get_unnumbered_bgp_neighbors().map_err(|e| { + HttpError::for_internal_error(format!( + "failed to get unnumbered neighbors: {e}" + )) + })?; + Ok(HttpResponseOk(build_ndp_interfaces( + ctx, + unnumbered_neighbors, + ))) +} + +pub async fn get_ndp_interfaces_v5( + rqctx: RequestContext>, + request: Query, +) -> Result>, HttpError> { + let rq = request.into_inner(); + let ctx = rqctx.context(); + + // Prior form: scope the interface list to the requested ASN. + let unnumbered_neighbors = ctx + .db + .get_unnumbered_bgp_neighbors() + .map_err(|e| { + HttpError::for_internal_error(format!( + "failed to get unnumbered neighbors: {e}" + )) + })? + .into_iter() + .filter(|n| n.asn == rq.asn) + .collect::>(); + + Ok(HttpResponseOk(build_ndp_interfaces( + ctx, + unnumbered_neighbors, + ))) +} + +fn ndp_interface_detail( + ctx: &HandlerContext, + neighbor: &mg_api_types::rdb::neighbor::BgpUnnumberedNeighborInfo, +) -> Result, HttpError> { + let ndp_detail = ctx + .bgp + .unnumbered_manager + .get_ndp_interface_detail(&neighbor.interface) + .map_err(|e| { + HttpError::for_internal_error(format!( + "failed to get NDP state: {e}" + )) + })? + .ok_or_else(|| { + HttpError::for_not_found( + None, + format!("interface {} not managed by NDP", neighbor.interface), + ) + })?; + + Ok(HttpResponseOk(NdpInterface { + interface: neighbor.interface.clone(), + local_address: ndp_detail.local_address, + scope_id: ndp_detail.scope_id, + router_lifetime: neighbor.router_lifetime, + discovered_peer: ndp_detail + .peer_state + .as_ref() + .map(convert_ndp_peer_to_api), + thread_state: convert_thread_state_to_api( + ndp_detail.thread_state.as_ref(), + ), + })) +} + +pub async fn get_ndp_interface_detail( + rqctx: RequestContext>, + request: Query, +) -> Result, HttpError> { + let rq = request.into_inner(); + let ctx = rqctx.context(); + + let neighbor = ctx + .db + .get_unnumbered_bgp_neighbors() + .map_err(|e| { + HttpError::for_internal_error(format!( + "failed to get unnumbered neighbors: {e}" + )) + })? + .into_iter() + .find(|n| n.interface == rq.interface_name) + .ok_or_else(|| { + HttpError::for_not_found( + None, + format!( + "no unnumbered neighbor on interface {}", + rq.interface_name + ), + ) + })?; + + ndp_interface_detail(ctx, &neighbor) +} + +pub async fn get_ndp_interface_detail_v5( + rqctx: RequestContext>, + request: Query, +) -> Result, HttpError> { + let rq = request.into_inner(); + let ctx = rqctx.context(); + + // Prior form: scope the lookup to the requested ASN. + let neighbor = ctx + .db + .get_unnumbered_bgp_neighbors() + .map_err(|e| { + HttpError::for_internal_error(format!( + "failed to get unnumbered neighbors: {e}" + )) + })? + .into_iter() + .find(|n| n.asn == rq.asn && n.interface == rq.interface) + .ok_or_else(|| { + HttpError::for_not_found( + None, + format!( + "no unnumbered neighbor for ASN {} on interface {}", + rq.asn, rq.interface + ), + ) + })?; + + ndp_interface_detail(ctx, &neighbor) +} diff --git a/openapi/mg-admin/mg-admin-10.0.0-c462c6.json.gitstub b/openapi/mg-admin/mg-admin-10.0.0-c462c6.json.gitstub new file mode 100644 index 00000000..4be1e76f --- /dev/null +++ b/openapi/mg-admin/mg-admin-10.0.0-c462c6.json.gitstub @@ -0,0 +1 @@ +db8fd7089cf58623dae7d8f7fe2fb10dfc2f69b7:openapi/mg-admin/mg-admin-10.0.0-c462c6.json diff --git a/openapi/mg-admin/mg-admin-10.0.0-c462c6.json b/openapi/mg-admin/mg-admin-11.0.0-f87034.json similarity index 99% rename from openapi/mg-admin/mg-admin-10.0.0-c462c6.json rename to openapi/mg-admin/mg-admin-11.0.0-f87034.json index 4890f078..7f5553d7 100644 --- a/openapi/mg-admin/mg-admin-10.0.0-c462c6.json +++ b/openapi/mg-admin/mg-admin-11.0.0-f87034.json @@ -6,7 +6,7 @@ "url": "https://oxide.computer", "email": "api@oxide.computer" }, - "version": "10.0.0" + "version": "11.0.0" }, "paths": { "/bfd/peers": { @@ -1240,19 +1240,7 @@ "parameters": [ { "in": "query", - "name": "asn", - "description": "ASN of the router", - "required": true, - "schema": { - "type": "integer", - "format": "uint32", - "minimum": 0 - } - }, - { - "in": "query", - "name": "interface", - "description": "Interface name", + "name": "interface_name", "required": true, "schema": { "type": "string" @@ -1282,19 +1270,6 @@ "/ndp/interfaces": { "get": { "operationId": "get_ndp_interfaces", - "parameters": [ - { - "in": "query", - "name": "asn", - "description": "ASN of the router to get imported prefixes from.", - "required": true, - "schema": { - "type": "integer", - "format": "uint32", - "minimum": 0 - } - } - ], "responses": { "200": { "description": "successful operation", @@ -1322,19 +1297,6 @@ "/ndp/manager": { "get": { "operationId": "get_ndp_manager_state", - "parameters": [ - { - "in": "query", - "name": "asn", - "description": "ASN of the router to get imported prefixes from.", - "required": true, - "schema": { - "type": "integer", - "format": "uint32", - "minimum": 0 - } - } - ], "responses": { "200": { "description": "successful operation", diff --git a/openapi/mg-admin/mg-admin-latest.json b/openapi/mg-admin/mg-admin-latest.json index 8056856c..e75c8fc1 120000 --- a/openapi/mg-admin/mg-admin-latest.json +++ b/openapi/mg-admin/mg-admin-latest.json @@ -1 +1 @@ -mg-admin-10.0.0-c462c6.json \ No newline at end of file +mg-admin-11.0.0-f87034.json \ No newline at end of file From 28cc72cd5873adf4359355c13a325e524897aa25 Mon Sep 17 00:00:00 2001 From: Trey Aspelund Date: Fri, 5 Jun 2026 15:03:25 -0600 Subject: [PATCH 5/5] bump ispf dependency Bump ispf to its latest main rev so we can benefit from recent ser/des fixes. In particular, we can now rely on ispf to properly parse RAs that arrive with an undersized ICMP payload length rather than checking it ourselves before handing it over to ispf. Signed-off-by: Trey Aspelund --- Cargo.lock | 2 +- ndp/src/packet.rs | 61 ++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 135d7d15..9c250e54 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3294,7 +3294,7 @@ checksum = "a6cb138bb79a146c1bd460005623e142ef0181e3d0219cb493e02f7d08a35695" [[package]] name = "ispf" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/ispf#f78443a98397f7818b1e7a487dbb7d5cad625496" +source = "git+https://github.com/oxidecomputer/ispf#2e04bbfe8547b95e92f0dcdd85e08efb974eda0a" dependencies = [ "serde", ] diff --git a/ndp/src/packet.rs b/ndp/src/packet.rs index c051dc6d..70a80035 100644 --- a/ndp/src/packet.rs +++ b/ndp/src/packet.rs @@ -39,13 +39,8 @@ impl Icmp6RouterAdvertisement { const TYPE: u8 = 134; const CODE: u8 = 0; const DEFAULT_HOPLIMIT: u8 = 255; - const MIN_PAYLOAD_LEN: usize = 16; pub fn from_wire(buf: &[u8]) -> Result { - // Per RFC 4861 Section 6.1.2: a valid RA has an ICMP payload of >= 16b - if buf.len() < Self::MIN_PAYLOAD_LEN { - return Err(Icmp6RaFromWireError::TooShort(buf.len())); - } let s: Self = ispf::from_bytes_be(buf)?; if s.typ != Self::TYPE { return Err(Icmp6RaFromWireError::WrongType(s.typ)); @@ -91,9 +86,6 @@ pub enum Icmp6RaFromWireError { #[error("deserialization error: {0}")] Ispf(#[from] ispf::Error), - #[error("too short: {0} octets, expected at least 16")] - TooShort(usize), - #[error("wrong type: expected {expected}, got {0}", expected = Icmp6RouterAdvertisement::TYPE)] WrongType(u8), @@ -157,3 +149,56 @@ pub enum Icmp6RsFromWireError { #[error("wrong code: expected {expected}, got {0}", expected = Icmp6RouterSolicitation::CODE)] WrongCode(u8), } + +#[cfg(test)] +mod tests { + use super::*; + + // The kernel does not filter received RAs on length, type, or code, so + // from_wire is responsible for rejecting malformed packets. + + fn valid_ra_wire() -> Vec { + ispf::to_bytes_be(&Icmp6RouterAdvertisement::default()).unwrap() + } + + #[test] + fn valid_ra_round_trips() { + let buf = valid_ra_wire(); + let ra = Icmp6RouterAdvertisement::from_wire(&buf) + .expect("a well-formed RA must parse"); + assert_eq!(ra.typ, Icmp6RouterAdvertisement::TYPE); + assert_eq!(ra.code, Icmp6RouterAdvertisement::CODE); + } + + #[test] + fn undersized_ra_is_rejected() { + // Per RFC 4861 Section 6.1.2 a valid RA carries at least 16 octets. + for len in 0..16 { + let buf = vec![Icmp6RouterAdvertisement::TYPE; len]; + assert!( + Icmp6RouterAdvertisement::from_wire(&buf).is_err(), + "RA of {len} octets must be rejected as too short", + ); + } + } + + #[test] + fn wrong_type_ra_is_rejected() { + let mut buf = valid_ra_wire(); + buf[0] = Icmp6RouterAdvertisement::TYPE + 1; + assert!(matches!( + Icmp6RouterAdvertisement::from_wire(&buf), + Err(Icmp6RaFromWireError::WrongType(_)), + )); + } + + #[test] + fn wrong_code_ra_is_rejected() { + let mut buf = valid_ra_wire(); + buf[1] = Icmp6RouterAdvertisement::CODE + 1; + assert!(matches!( + Icmp6RouterAdvertisement::from_wire(&buf), + Err(Icmp6RaFromWireError::WrongCode(_)), + )); + } +}