diff --git a/bin/bgp/README.md b/bin/bgp/README.md index 23f367d..2fd70d1 100644 --- a/bin/bgp/README.md +++ b/bin/bgp/README.md @@ -262,10 +262,18 @@ kernel { address in the network of one of the machine's interfaces that is up. For an IPv6 route with a link-local next hop, the interface is the one the peer is a neighbor on. A route whose next hop is further away (a route - from a peer in our own AS that did not set itself as next hop, say) is not - installed, and neither is whatever it replaced: looking up the way to such - a next hop through other routes is not done yet. The status page counts - these, by reason. + from a peer in our own AS that did not set itself as next hop, say) + cannot be installed: looking up the way to such a next hop through other + routes is not done yet. +- **Routes that cannot be installed are not chosen.** With `kernel {}`, a + route whose next hop cannot be reached is not taken into the table, so + that it cannot be the best route of its prefix while another peer has + one that works (RFC 4271 §9.1.2.1). It is held aside, counted as refused + for "next hop unreachable" and shown as held on the status page, and + taken as soon as the machine's interfaces change so that it can be + reached; a route in the table whose next hop goes away is held in the + same way. Without `kernel {}` the daemon does not know what can be + reached, and takes every route. - **Nexthop objects.** Where the kernel has them (Linux 5.3 and later) a gateway is put there once, as a nexthop object, and routes name it; `ip route` shows such routes with `nhid`, and `ip nexthop` the objects. The @@ -359,7 +367,8 @@ Routinator 0.15.2. What it covers: | `restart` | The daemon stopped in good order (kernel emptied, peers told) and killed (routes kept, stale one cleared by the next start) | | `scale` | A table from BIRD (`SCALE_ROUTES`, a million for the Internet's size) into the table and the kernel; the daemon killed and restarted under it; the table withdrawn | | `limits` | A peer whose role does not fit gets no session; a peer over its prefix limit gets Cease / Maximum Number of Prefixes and is held down | -| `hijacks` | FRR announces our own prefix, a route whose next hop is BIRD, and the exchange LAN's own prefix: the first two are refused, the third stays out of the kernel | +| `hijacks` | FRR announces our own prefix, a route whose next hop is BIRD, and the exchange LAN's own prefix: none is taken | +| `nexthops` | A better route from FRR whose next hop cannot be reached is held and BIRD's is chosen and installed; they swap when the next hop becomes a neighbor, and swap back when it stops being one | | `v6only` | The exchange LAN without IPv4 addresses: one IPv6 session with each router for both families, IPv4 routes with IPv6 next hops (RFC 8950) in BGP and in the kernel, IPv4 pings across | | `linklocal` | The exchange LAN with link-local addresses only, and the same ones on a second interface: sessions with FRR (by interface, with radvd beside the daemon) and BIRD at `fe80::…%ix0`, both families in BGP and in the kernel, pings across, the interface taken down and up | diff --git a/crates/fib/src/kernel.rs b/crates/fib/src/kernel.rs index 73e9c55..977ca10 100644 --- a/crates/fib/src/kernel.rs +++ b/crates/fib/src/kernel.rs @@ -977,6 +977,7 @@ impl Task { UNRESOLVED.set(self.plan.unresolved() as i64); REFUSED.set(self.plan.refused() as i64); let interfaces = interfaces_changed.then(|| { + self.fib.publish_interfaces(&self.interfaces); let mut interfaces: Vec = self .interfaces .links() diff --git a/crates/fib/src/lib.rs b/crates/fib/src/lib.rs index 68dfca9..c28d957 100644 --- a/crates/fib/src/lib.rs +++ b/crates/fib/src/lib.rs @@ -135,9 +135,14 @@ pub struct Status { pub interfaces: Vec, } +/// The machine's interfaces as the kernel last described them; `None` +/// until it has. Whoever holds one is told when they change. +pub type InterfacesWatch = watch::Receiver>>; + struct Shared { config: FibConfig, status: Mutex, + interfaces: watch::Sender>>, stop: watch::Sender, /// Whether the task has ended. ended: watch::Sender, @@ -165,6 +170,7 @@ impl Fib { shared: Arc::new(Shared { config, status: Mutex::default(), + interfaces: watch::channel(None).0, stop, ended: watch::channel(false).0, }), @@ -177,6 +183,19 @@ impl Fib { self.shared.config } + /// The machine's interfaces, for those who want to know whether a + /// next hop can be reached before the kernel is asked to use it. + pub fn interfaces(&self) -> InterfacesWatch { + self.shared.interfaces.subscribe() + } + + #[cfg_attr(not(target_os = "linux"), allow(dead_code))] + fn publish_interfaces(&self, interfaces: &interfaces::Interfaces) { + self.shared + .interfaces + .send_replace(Some(Arc::new(interfaces.clone()))); + } + pub fn status(&self) -> Status { self.shared .status diff --git a/crates/fib/src/plan.rs b/crates/fib/src/plan.rs index 8d859d1..66a383a 100644 --- a/crates/fib/src/plan.rs +++ b/crates/fib/src/plan.rs @@ -97,6 +97,19 @@ pub fn resolve( if interfaces.has_network_around(prefix) { return Err(Unresolved::OwnNetwork); } + resolve_next_hop(interfaces, prefix.address_family(), next_hop, peer) +} + +/// The part of [`resolve`] that is the same for every route of IP version +/// `routes` with one next hop from one peer: where the next hop is. For +/// those who ask it of many such routes, and ask of each only whether it +/// is for one of the machine's own networks. +pub fn resolve_next_hop( + interfaces: &Interfaces, + routes: IpVersion, + next_hop: Option<&NlriNextHop>, + peer: Option, +) -> Result { let next_hop = next_hop.ok_or(Unresolved::Missing)?; let version = match next_hop { NlriNextHop::Ipv4(_) => IpVersion::Ipv4, @@ -105,7 +118,7 @@ pub fn resolve( // An IPv4 route may have an IPv6 next hop: that is how routes go over // a link with only IPv6 addresses (RFC 8950), and the kernel takes // such a route. The other way about means nothing. - if version != prefix.address_family() && version == IpVersion::Ipv4 { + if version != routes && version == IpVersion::Ipv4 { return Err(Unresolved::OtherFamily); } let direct = |gateway: IpAddr| { diff --git a/crates/integration-tests/tests/kernel.rs b/crates/integration-tests/tests/kernel.rs index 933b54c..80f994d 100644 --- a/crates/integration-tests/tests/kernel.rs +++ b/crates/integration-tests/tests/kernel.rs @@ -142,9 +142,20 @@ async fn test_routes_from_a_session_reach_the_kernel() -> Result<()> { assert_eq!(kernel["enabled"], true); assert_eq!(kernel["table"], TABLE); assert_eq!(kernel["installed"], 2); + // The route by way of a router that is no neighbor is not in the + // table for the kernel to be asked for: it is held, in case the + // router becomes one. + assert_eq!(kernel["unresolved"], serde_json::json!({})); + let session = server.registry.list().pop().expect("the session"); + assert_eq!(session.counters.held(), 1); + assert_eq!(server.rib.len(), 2); + let refused: Value = reqwest::get(format!("{base}/api/routes/rejected")) + .await? + .json() + .await?; assert_eq!( - kernel["unresolved"], - serde_json::json!({"next hop not directly connected": 1}) + refused[0]["by_reason"], + serde_json::json!({"next hop unreachable": 1}) ); let shown = kernel["interfaces"] .as_array() diff --git a/crates/server/src/connection.rs b/crates/server/src/connection.rs index 716608e..665bb47 100644 --- a/crates/server/src/connection.rs +++ b/crates/server/src/connection.rs @@ -13,6 +13,7 @@ use std::pin::Pin; use std::sync::Arc; use std::time::Duration; +use bgp_fib::InterfacesWatch; use bgp_packet::errors::{BgpError, DecodeError, MsgHeaderSubcode}; use bgp_packet::message::RouteRefreshMessage; use bgp_packet::message::{ @@ -45,6 +46,7 @@ use crate::metrics::{ }; use crate::panic::contained; use crate::policy::{self, HeldDown, HoldDowns, ImportPolicy, PeerRole}; +use crate::reach::{Reach, neighbor_at}; use crate::refusals::{PeerRefusals, RefusalLog}; use crate::routes::{self, Held}; use crate::rpki::{Checks, Subject, Validator}; @@ -82,6 +84,8 @@ pub struct ServerContext { pub refusals: RefusalLog, /// The data for route origin validation, where that is configured. pub rpki: Option, + /// The machine's interfaces, where routes are installed in the kernel. + pub interfaces: Option, } /// What is configured for the peer at the other end of a connection. @@ -787,14 +791,20 @@ impl Session { negotiated.peer_asn, negotiated.ibgp, ); - let validation = self + // And where routes go into the kernel, a route has to have a next + // hop the kernel can use. + let reach = self .server - .rpki + .interfaces .clone() - .zip(subject) - .map(|(rpki, subject)| Validation { - validator: Validator::new(rpki, subject), - changes: handle.validate(subject), + .map(|interfaces| Reach::new(interfaces, neighbor_at(self.peer_addr))); + let subject = subject.filter(|_| self.server.rpki.is_some()); + let validation = + Validator::new(self.server.rpki.clone().zip(subject), reach).map(|validator| { + Validation { + validator, + changes: handle.validate(subject), + } }); self.established = Some(Established { since: Instant::now(), @@ -1162,6 +1172,7 @@ mod tests { registry: SessionRegistry::new(), collisions: CollisionTable::new(false), hold_downs: HoldDowns::default(), + interfaces: None, refusals: RefusalLog::default(), rpki: None, }); @@ -1210,6 +1221,7 @@ mod tests { registry: SessionRegistry::new(), collisions: CollisionTable::new(false), hold_downs: HoldDowns::default(), + interfaces: None, refusals: RefusalLog::default(), rpki: None, }); diff --git a/crates/server/src/lib.rs b/crates/server/src/lib.rs index 675eb63..bd434ec 100644 --- a/crates/server/src/lib.rs +++ b/crates/server/src/lib.rs @@ -10,6 +10,7 @@ mod fsm; pub mod metrics; mod panic; pub mod policy; +mod reach; pub mod refusals; mod routes; pub mod rpki; diff --git a/crates/server/src/policy.rs b/crates/server/src/policy.rs index 1817958..4cd5c04 100644 --- a/crates/server/src/policy.rs +++ b/crates/server/src/policy.rs @@ -185,10 +185,23 @@ pub enum Rejection { NextHopNotPeer, /// The prefix is one of ours, or inside one. OwnPrefix, + /// The next hop cannot be reached from this machine as its interfaces + /// are now, so the kernel could not be given the route. + NextHopUnreachable, } impl Rejection { - pub const ALL: [Self; 16] = [ + /// Whether the reason may pass by itself, with the route unchanged: + /// the RPKI's data changes, and so do the machine's interfaces. A + /// route refused for such a reason is held, to be taken when it does. + pub(crate) fn may_pass(self) -> bool { + matches!( + self, + Self::RpkiInvalid | Self::AspaInvalid | Self::NextHopUnreachable + ) + } + + pub const ALL: [Self; 17] = [ Self::BogonPrefix, Self::TooShort, Self::TooSpecific, @@ -205,6 +218,7 @@ impl Rejection { Self::NextHopUnusable, Self::NextHopNotPeer, Self::OwnPrefix, + Self::NextHopUnreachable, ]; /// Where the reason is in [`Rejection::ALL`]. @@ -230,6 +244,7 @@ impl Rejection { Self::NextHopUnusable => "next hop unusable", Self::NextHopNotPeer => "next hop is not the peer", Self::OwnPrefix => "prefix is our own", + Self::NextHopUnreachable => "next hop unreachable", } } } diff --git a/crates/server/src/reach.rs b/crates/server/src/reach.rs new file mode 100644 index 0000000..3ce757b --- /dev/null +++ b/crates/server/src/reach.rs @@ -0,0 +1,438 @@ +//! Whether a route's next hop can be reached at all. +//! +//! A route with a next hop that packets cannot be sent to is not to be +//! chosen over one that works (RFC 4271 §9.1.2.1). What can be reached is +//! known where the daemon installs routes in the kernel, which is where +//! it matters: it is what the kernel says of the machine's interfaces. +//! There, a route the kernel could not be given is not taken into the +//! table, so that it cannot be the best route of its prefix; it is held +//! aside like a route the RPKI finds invalid, and looked at again when +//! the interfaces change (see `crate::routes::Held`). +//! +//! Only next hops that are neighbors on one of the machine's interfaces +//! count as reachable: the way to any other is not looked up. Routes from +//! a peer in our own AS that did not set itself as next hop are therefore +//! held, where they used to be chosen and then not installed. + +use std::cell::RefCell; +use std::collections::HashMap; +use std::net::SocketAddr; +use std::sync::Arc; +use std::time::Duration; + +use bgp_fib::InterfacesWatch; +use bgp_fib::interfaces::Interfaces; +use bgp_fib::plan::{Neighbor, resolve, resolve_next_hop}; +use bgp_packet::message::NlriNextHop; +use bgp_primitives::constants::IpVersion; +use bgp_primitives::ip_prefix::{IpPrefix, Nlri}; +use bgp_rib::table::{PeerIndex, RibTable}; +use tokio::task::JoinHandle; +use tracing::{info, warn}; + +use crate::address::PeerAddress; +use crate::session::SessionRegistry; + +/// How long after the interfaces change the table is looked through: +/// such changes come in bursts, and a session in the middle of an UPDATE +/// judged it by the interfaces as they were. +const SETTLE: Duration = Duration::from_millis(500); + +/// The interfaces to judge one peer's next hops by. +#[derive(Clone, Copy)] +pub(crate) struct NextHops<'a> { + pub(crate) interfaces: &'a Interfaces, + /// The peer: where a link-local next hop is. + pub(crate) neighbor: Neighbor, +} + +impl NextHops<'_> { + /// Whether the kernel can be given a route for `prefix` by way of + /// `next_hop`. + pub(crate) fn reaches(&self, prefix: &IpPrefix, next_hop: Option<&NlriNextHop>) -> bool { + resolve(self.interfaces, prefix, next_hop, Some(self.neighbor)).is_ok() + } + + /// The two halves of [`NextHops::reaches`], for an UPDATE's worth of + /// routes with one next hop: whether the next hop is somewhere, which + /// is asked once, and whether a prefix is one the machine reaches by + /// itself, which is asked of each. + pub(crate) fn next_hop_is_near(&self, routes: IpVersion, next_hop: &NlriNextHop) -> bool { + resolve_next_hop(self.interfaces, routes, Some(next_hop), Some(self.neighbor)).is_ok() + } + + pub(crate) fn is_own_network(&self, prefix: &IpPrefix) -> bool { + self.interfaces.has_network_around(prefix) + } +} + +/// A session's view of the interfaces: those in use, replaced when the +/// kernel says they have changed. Asking whether they have costs the load +/// of one number, which is done for every UPDATE. +pub(crate) struct Reach { + watch: InterfacesWatch, + neighbor: Neighbor, + interfaces: Option>, +} + +impl Reach { + pub(crate) fn new(watch: InterfacesWatch, neighbor: Neighbor) -> Self { + let interfaces = watch.borrow().clone(); + Self { + watch, + neighbor, + interfaces, + } + } + + /// What to judge next hops by now; nothing until the kernel has said + /// what interfaces there are. + pub(crate) fn next_hops(&mut self) -> Option> { + if self.watch.has_changed().unwrap_or(false) { + self.interfaces = self.watch.borrow_and_update().clone(); + } + self.interfaces.as_deref().map(|interfaces| NextHops { + interfaces, + neighbor: self.neighbor, + }) + } +} + +/// Starts the task that, whenever the machine's interfaces change, finds +/// the routes in the table whose next hop can no longer be reached and has +/// their sessions take them out, and has every session look again at the +/// routes it holds. +pub(crate) fn spawn_revalidator( + mut watch: InterfacesWatch, + rib: Arc, + registry: SessionRegistry, +) -> JoinHandle<()> { + tokio::spawn(async move { + // Ends only if the task that publishes the interfaces has gone, + // which the server notices for itself. + while watch.changed().await.is_ok() { + tokio::time::sleep(SETTLE).await; + let Some(interfaces) = watch.borrow_and_update().clone() else { + continue; + }; + let sessions = registry.list(); + // By peer, for looking up as often as there are routes. + let mut neighbors: Vec> = Vec::new(); + for session in &sessions { + let at = session.rib_peer.as_usize(); + if neighbors.len() <= at { + neighbors.resize(at + 1, None); + } + neighbors[at] = Some(neighbor_at(session.peer.addr)); + } + let table = Arc::clone(&rib); + // Looks at every route: not for a thread that sessions run on. + let found = tokio::task::spawn_blocking(move || { + now_unreachable(&table, &interfaces, &neighbors) + }) + .await; + let mut suspects = match found { + Ok(suspects) => suspects, + Err(e) => { + warn!(err = %e, "Looking for routes whose next hop is gone failed"); + HashMap::new() + } + }; + info!( + now_unreachable = suspects.values().map(Vec::len).sum::(), + sessions = sessions.len(), + "The machine's interfaces changed; looking at next hops again" + ); + // Every session, also those with nothing found: each has its + // held routes to look at again. + for session in sessions { + session.revalidate(suspects.remove(&session.rib_peer).unwrap_or_default()); + } + } + }) +} + +/// The peer at `address`, as far as its next hops are concerned. +pub(crate) fn neighbor_at(address: SocketAddr) -> Neighbor { + let address = PeerAddress::from(address); + Neighbor { + address: address.ip, + interface: address.interface, + } +} + +/// The routes in the table that the kernel could not be given, as the +/// interfaces are now, by peer. `neighbors` says, for each peer by its +/// number in the table, where it is. +fn now_unreachable( + rib: &RibTable, + interfaces: &Interfaces, + neighbors: &[Option], +) -> HashMap> { + // There are as many next hops as neighbors, and a table's worth of + // routes share them: each is worked out once. + type Key = (Option, IpVersion, PeerIndex); + let resolved: RefCell> = RefCell::default(); + let routes = rib.routes_where(usize::MAX, |route| { + let Some(Some(neighbor)) = neighbors.get(route.peer.as_usize()) else { + return false; + }; + if interfaces.has_network_around(&route.prefix) { + return true; + } + let next_hop = route.attrs.next_hop(); + let key = (next_hop, route.prefix.address_family(), route.peer); + !*resolved.borrow_mut().entry(key).or_insert_with(|| { + resolve( + interfaces, + &route.prefix, + next_hop.as_ref(), + Some(*neighbor), + ) + .is_ok() + }) + }); + let mut by_peer: HashMap> = HashMap::new(); + for route in routes { + by_peer.entry(route.peer).or_default().push(Nlri { + prefix: route.prefix, + path_id: route.path_id, + }); + } + by_peer +} + +#[cfg(test)] +mod tests { + use std::net::{IpAddr, Ipv4Addr}; + + use bgp_fib::netlink::{Address, Link, Message}; + use bgp_packet::message::{ + AsPathAttribute, NextHopPathAttribute, OriginPathAttribute, PathAttribute, UpdateMessage, + }; + use bgp_rib::table::PeerInfo; + + use super::*; + use crate::policy::{ImportPolicy, Rejection, RouteSource}; + use crate::refusals::RefusalLog; + use crate::routes::{self, Held, Import}; + use crate::rpki::Checks; + + fn prefix(s: &str) -> IpPrefix { + IpPrefix::try_from(s).unwrap() + } + + /// A machine with eth2 (192.0.2.1/24), and eth3 (198.51.100.1/24) if + /// `both`. + fn interfaces(both: bool) -> Interfaces { + let mut interfaces = Interfaces::default(); + let all = [ + (2, "192.0.2.1", "192.0.2.0/24"), + (3, "198.51.100.1", "198.51.100.0/24"), + ]; + for (index, local, network) in all.into_iter().take(if both { 2 } else { 1 }) { + interfaces.apply(&Message::Link { + present: true, + link: Link { + index, + name: format!("eth{index}"), + up: true, + }, + }); + interfaces.apply(&Message::Address { + present: true, + address: Address { + interface: index, + local: local.parse().unwrap(), + network: prefix(network), + }, + }); + } + interfaces + } + + struct Peer { + index: PeerIndex, + address: Ipv4Addr, + asn: u32, + held: Held, + } + + struct Bed { + table: RibTable, + policy: ImportPolicy, + refusals: RefusalLog, + } + + impl Bed { + fn new() -> Self { + Self { + table: RibTable::new(4), + policy: ImportPolicy::permissive(), + refusals: RefusalLog::default(), + } + } + + fn peer(&self, address: &str, asn: u32) -> Peer { + let address: Ipv4Addr = address.parse().unwrap(); + Peer { + index: self + .table + .add_peer(PeerInfo { + asn, + router_id: address, + address: address.into(), + interface: 0, + ibgp: false, + }) + .unwrap(), + address, + asn, + held: Held::default(), + } + } + + /// What `work` is given: the peer's routes as a session judges + /// them, with `interfaces` to go by. + fn with_import( + &self, + peer: &mut Peer, + interfaces: &Interfaces, + work: impl FnOnce(&RibTable, &Import<'_>, &mut Held) -> T, + ) -> T { + let address = IpAddr::V4(peer.address); + let import = Import { + peer: peer.index, + from: RouteSource { + peer_asn: peer.asn, + local_asn: 65000, + ibgp: false, + four_octet_asn: true, + role: None, + peer_address: address, + local_address: Ipv4Addr::new(192, 0, 2, 1).into(), + extended_next_hop: false, + }, + policy: &self.policy, + checks: Checks { + next_hops: Some(NextHops { + interfaces, + neighbor: address.into(), + }), + ..Checks::NONE + }, + refusals: &self.refusals.of_peer(address.into(), "test"), + }; + work(&self.table, &import, &mut peer.held) + } + + /// The peer announces `route` with a path of `hops` AS numbers and + /// itself as next hop. + fn announce(&self, peer: &mut Peer, interfaces: &Interfaces, route: &str, hops: usize) { + let update = UpdateMessage { + withdrawn_routes: vec![], + path_attributes: vec![ + PathAttribute::Origin(OriginPathAttribute::IGP), + AsPathAttribute::from_asns(vec![peer.asn; hops]), + PathAttribute::NextHop(NextHopPathAttribute(peer.address)), + ], + announced_routes: vec![prefix(route).into()], + }; + self.with_import(peer, interfaces, |table, import, held| { + routes::apply(table, import, held, &update, None).unwrap() + }); + } + + fn revalidate(&self, peer: &mut Peer, interfaces: &Interfaces, suspects: &[Nlri]) { + self.with_import(peer, interfaces, |table, import, held| { + routes::revalidate(table, import, held, suspects) + }); + } + + fn best(&self, route: &str) -> Option { + self.table.best(&prefix(route)).map(|(peer, _)| peer) + } + } + + /// RFC 4271 §9.1.2.1: a route whose next hop cannot be reached is not + /// chosen, however good it looks; and it is when it can be. + #[test] + fn a_route_is_in_the_running_only_while_its_next_hop_can_be_reached() { + let bed = Bed::new(); + let one_link = interfaces(false); + let two_links = interfaces(true); + // The better route is from the peer on the link that is not there. + let mut near = bed.peer("192.0.2.2", 65001); + let mut far = bed.peer("198.51.100.2", 65002); + let route = "203.0.113.0/24"; + bed.announce(&mut near, &one_link, route, 3); + bed.announce(&mut far, &one_link, route, 1); + assert_eq!(bed.best(route), Some(near.index)); + assert_eq!(bed.table.len(), 1, "the other is not in the table at all"); + assert_eq!((near.held.len(), far.held.len()), (0, 1)); + let refused = bed.refusals.peers(); + assert_eq!(refused.len(), 1); + assert_eq!( + refused[0].1.by_reason(), + [(Rejection::NextHopUnreachable, 1)] + ); + + // The link comes: nothing in the table is wrong, and what was + // held is taken, and is the better route. + let neighbors = |peers: &[&Peer]| { + let mut by_index = vec![None; 8]; + for peer in peers { + by_index[peer.index.as_usize()] = Some(Neighbor::from(IpAddr::V4(peer.address))); + } + by_index + }; + let both = neighbors(&[&near, &far]); + assert!(now_unreachable(&bed.table, &two_links, &both).is_empty()); + bed.revalidate(&mut near, &two_links, &[]); + bed.revalidate(&mut far, &two_links, &[]); + assert_eq!(bed.best(route), Some(far.index)); + assert_eq!((bed.table.len(), far.held.len()), (2, 0)); + + // The link goes: the route by way of it is found in the table, + // taken out and held, and the other is the best again. + let mut suspects = now_unreachable(&bed.table, &one_link, &both); + assert_eq!( + suspects.keys().copied().collect::>(), + [far.index], + "only the routes of the peer that is cut off" + ); + let suspects = suspects.remove(&far.index).unwrap(); + assert_eq!(suspects, [Nlri::from(prefix(route))]); + bed.revalidate(&mut far, &one_link, &suspects); + bed.revalidate(&mut near, &one_link, &[]); + assert_eq!(bed.best(route), Some(near.index)); + assert_eq!((bed.table.len(), far.held.len()), (1, 1)); + + // A peer that withdraws a held route does not get it back. + let withdrawal = UpdateMessage { + withdrawn_routes: vec![prefix(route).into()], + path_attributes: vec![], + announced_routes: vec![], + }; + bed.with_import(&mut far, &one_link, |table, import, held| { + routes::apply(table, import, held, &withdrawal, None).unwrap() + }); + bed.revalidate(&mut far, &two_links, &[]); + assert_eq!(bed.best(route), Some(near.index)); + assert_eq!(far.held.len(), 0); + } + + /// A route for a network the machine is on is not one the kernel is + /// given either, and so not one to choose. + #[test] + fn a_route_for_one_of_our_own_networks_is_held() { + let bed = Bed::new(); + let interfaces = interfaces(true); + let mut peer = bed.peer("192.0.2.2", 65001); + bed.announce(&mut peer, &interfaces, "198.51.100.128/25", 1); + bed.announce(&mut peer, &interfaces, "203.0.113.0/24", 1); + assert_eq!(bed.table.len(), 1); + assert_eq!(peer.held.len(), 1); + assert_eq!(bed.best("198.51.100.128/25"), None); + } +} diff --git a/crates/server/src/routes.rs b/crates/server/src/routes.rs index c83e04f..86b7bb7 100644 --- a/crates/server/src/routes.rs +++ b/crates/server/src/routes.rs @@ -216,18 +216,25 @@ impl Applying<'_> { // Whatever becomes of them, these replace what was held of the // same routes. self.held.forget(nlri); - let origin = match self.judge_together(first.prefix.address_family(), next_hop) { - Ok(origin) => origin, + let family = first.prefix.address_family(); + let (origin, used_next_hop) = match self.judge_together(family, next_hop) { + Ok(judged) => judged, Err(rejection) => { self.refuse(nlri, rejection); return Ok(()); } }; + // Asked once for all of them: they have the one next hop. + let near = self + .import + .checks + .next_hops + .is_none_or(|next_hops| next_hops.next_hop_is_near(family, &used_next_hop)); // Nearly always every route is taken, and then nothing is copied. let mut accepted = Cow::Borrowed(nlri); let mut invalid = Vec::new(); for (at, route) in nlri.iter().enumerate() { - match self.judge(route, origin) { + match self.judge(route, origin, near) { Ok(()) => { if let Cow::Owned(accepted) = &mut accepted { accepted.push(*route); @@ -237,7 +244,7 @@ impl Applying<'_> { if let Cow::Borrowed(_) = accepted { accepted = Cow::Owned(nlri[..at].to_vec()); } - if matches!(rejection, Rejection::RpkiInvalid | Rejection::AspaInvalid) { + if rejection.may_pass() { invalid.push(*route); } self.refuse(std::slice::from_ref(route), rejection); @@ -259,12 +266,12 @@ impl Applying<'_> { /// Judges what routes announced together have in common: the path, /// and the next hop. `routes` is their IP version. Returns the AS they - /// originate from, if the path says. + /// originate from, if the path says, and their next hop. fn judge_together( &self, routes: IpVersion, next_hop: Option<&NlriNextHop>, - ) -> Result, Rejection> { + ) -> Result<(Option, NlriNextHop), Rejection> { let origin = self.path_verdict?; let in_attributes = || { self.update @@ -283,11 +290,13 @@ impl Applying<'_> { self.import .policy .check_next_hop(&next_hop, routes, &self.import.from)?; - Ok(origin) + Ok((origin, next_hop)) } - /// Judges one route, whose origin is `origin`. - fn judge(&self, route: &Nlri, origin: Option) -> Result<(), Rejection> { + /// Judges one route, whose origin is `origin`; `near` says whether + /// its next hop can be reached. What is wrong with the route for good + /// comes before what may pass. + fn judge(&self, route: &Nlri, origin: Option, near: bool) -> Result<(), Rejection> { let import = self.import; import.policy.check_prefix(&route.prefix)?; let origin_invalid = import @@ -303,6 +312,14 @@ impl Applying<'_> { { return Err(Rejection::AspaInvalid); } + let reachable = near + && !import + .checks + .next_hops + .is_some_and(|next_hops| next_hops.is_own_network(&route.prefix)); + if !reachable { + return Err(Rejection::NextHopUnreachable); + } Ok(()) } @@ -382,7 +399,7 @@ pub(crate) fn revalidate( ) -> Applied { let mut applied = Applied::default(); let checks = &import.checks; - if checks.vrps.is_none() && checks.aspas.is_none() { + if checks.is_none() { return applied; } let mut path = Vec::new(); diff --git a/crates/server/src/rpki.rs b/crates/server/src/rpki.rs index 33472f2..57e4e1a 100644 --- a/crates/server/src/rpki.rs +++ b/crates/server/src/rpki.rs @@ -29,6 +29,7 @@ use tracing::{info, warn}; use crate::metrics::{RPKI_ASPAS, RPKI_UPDATES, RPKI_VRPS}; use crate::policy::{ImportPolicy, PeerRole, Rejection}; +use crate::reach::{NextHops, Reach}; use crate::session::{SessionHandle, SessionRegistry}; /// How long after new data is published the table is looked through. A @@ -117,6 +118,9 @@ impl Subject { pub(crate) struct Checks<'a> { pub(crate) vrps: Option<&'a VrpTable>, pub(crate) aspas: Option<(&'a AspaTable, PathCheck)>, + /// The machine's interfaces, where routes are installed in the kernel + /// and a next hop therefore has to be one that can be reached. + pub(crate) next_hops: Option>, } impl<'a> Checks<'a> { @@ -124,13 +128,12 @@ impl<'a> Checks<'a> { pub(crate) const NONE: Self = Self { vrps: None, aspas: None, + next_hops: None, }; - fn new(subject: Subject, data: &'a Update) -> Self { - Self { - vrps: subject.origin.then_some(&*data.vrps), - aspas: subject.path.map(|check| (&*data.aspas, check)), - } + /// Whether there is nothing to check. + pub(crate) fn is_none(&self) -> bool { + self.vrps.is_none() && self.aspas.is_none() && self.next_hops.is_none() } /// Why a route for `prefix` with the attributes `attrs` is invalid, @@ -155,35 +158,49 @@ impl<'a> Checks<'a> { return Some(Rejection::AspaInvalid); } } + if let Some(next_hops) = self.next_hops + && !next_hops.reaches(prefix, attrs.next_hop().as_ref()) + { + return Some(Rejection::NextHopUnreachable); + } None } } -/// A session's view of the validation data: the data in use, replaced when -/// newer is published. Asking whether there is newer costs the load of one -/// number, which is done for every UPDATE. +/// A session's view of what its peer's routes are checked against and +/// that changes while the session lasts: the RPKI's data and the +/// machine's interfaces, each as in use, replaced when there is newer. +/// Asking whether there is costs the load of a number, which is done for +/// every UPDATE. pub(crate) struct Validator { - rpki: Rpki, - subject: Subject, - data: Update, + rpki: Option<(Rpki, Subject, Update)>, + reach: Option, } impl Validator { - pub(crate) fn new(rpki: Rpki, subject: Subject) -> Self { - let data = rpki.current(); - Self { - rpki, - subject, - data, - } + /// `None` if the peer's routes are subject to none of the checks. + pub(crate) fn new(rpki: Option<(Rpki, Subject)>, reach: Option) -> Option { + let rpki = rpki.map(|(rpki, subject)| { + let data = rpki.current(); + (rpki, subject, data) + }); + (rpki.is_some() || reach.is_some()).then_some(Self { rpki, reach }) } /// What to validate against now. pub(crate) fn checks(&mut self) -> Checks<'_> { - if self.rpki.version() != self.data.version { - self.data = self.rpki.current(); + let mut checks = Checks::NONE; + if let Some((rpki, subject, data)) = &mut self.rpki { + if rpki.version() != data.version { + *data = rpki.current(); + } + checks.vrps = subject.origin.then_some(&*data.vrps); + checks.aspas = subject.path.map(|check| (&*data.aspas, check)); } - Checks::new(self.subject, &self.data) + if let Some(reach) = &mut self.reach { + checks.next_hops = reach.next_hops(); + } + checks } } diff --git a/crates/server/src/server.rs b/crates/server/src/server.rs index e3d2b51..a00c875 100644 --- a/crates/server/src/server.rs +++ b/crates/server/src/server.rs @@ -28,6 +28,7 @@ use crate::error::ServerError; use crate::metrics::{CONNECTIONS_REFUSED, PEER_MAX_PREFIXES}; use crate::panic; use crate::policy::HoldDowns; +use crate::reach; use crate::refusals::RefusalLog; use crate::rpki::spawn_revalidator; use crate::session::{ConfiguredPeer, SessionRegistry}; @@ -192,6 +193,7 @@ impl BgpServer { hold_downs: HoldDowns::default(), refusals: self.refusals.clone(), rpki: self.rpki.clone(), + interfaces: self.kernel.as_ref().map(Fib::interfaces), }); // The tasks the server cannot do without. None of them ends while @@ -214,6 +216,12 @@ impl BgpServer { spawn_revalidator(rpki, Arc::clone(&self.rib), self.registry.clone()), )); } + if let Some(interfaces) = context.interfaces.clone() { + tasks.push(named( + "next hop revalidator", + reach::spawn_revalidator(interfaces, Arc::clone(&self.rib), self.registry.clone()), + )); + } for dialer in spawn_dialers(&self.config.peers, &context, self.config.connect_retry) { tasks.push(named("dialer", dialer)); } diff --git a/crates/server/src/session.rs b/crates/server/src/session.rs index d04b694..8ec2573 100644 --- a/crates/server/src/session.rs +++ b/crates/server/src/session.rs @@ -148,7 +148,7 @@ pub struct SessionHandle { outbound: mpsc::Sender, /// Which of the RPKI's checks the peer's routes are subject to, if /// any, and where the session is told to look at its routes again. - validation: Option<(Subject, mpsc::UnboundedSender>)>, + validation: Option<(Option, mpsc::UnboundedSender>)>, } #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] @@ -189,7 +189,10 @@ impl SessionHandle { /// Marks the session's routes as subject to validation with the /// RPKI's data. What is sent with [`SessionHandle::revalidate`] /// arrives at the receiver returned. - pub(crate) fn validate(&mut self, subject: Subject) -> mpsc::UnboundedReceiver> { + pub(crate) fn validate( + &mut self, + subject: Option, + ) -> mpsc::UnboundedReceiver> { let (tx, rx) = mpsc::unbounded_channel(); self.validation = Some((subject, tx)); rx @@ -198,7 +201,7 @@ impl SessionHandle { /// Which of the RPKI's checks the session's routes are subject to, if /// any. pub fn validated(&self) -> Option { - self.validation.as_ref().map(|(subject, _)| *subject) + self.validation.as_ref().and_then(|(subject, _)| *subject) } /// Tells the session that the validation data has changed. `suspects` diff --git a/docs/RFCs.md b/docs/RFCs.md index 52a68c4..84ab788 100644 --- a/docs/RFCs.md +++ b/docs/RFCs.md @@ -33,7 +33,8 @@ The base protocol specification. | §6.2 | Bad BGP Identifier check (peer using our identifier, or 0.0.0.0) | ✅ | | §9 | Decision process / best-path selection | ✅ | | §9.1.2 | Best routes installed for forwarding (Linux kernel, over netlink), where the next hop is directly connected | ✅ | -| §9.1.2.1 | Routes with an unresolvable next hop excluded from selection; next hops resolved through other routes | ❌ (such a route can be chosen, and is then not installed) | +| §9.1.2.1 | Routes with an unresolvable next hop excluded from selection | ✅ where routes are installed in the kernel: such a route is held out of the table until its next hop can be reached | +| §9.1.2.1 | Next hops resolved through other routes | ❌ (only next hops that are neighbors count as reachable) | | §9 | Adj-RIB-In (per-peer table of received routes) | ✅ | | §9 | Locally originated routes, advertised to all established peers | ✅ | | §9 | Loc-RIB / Adj-RIB-Out for routes learned from peers (re-advertisement) | ❌ | diff --git a/scripts/interop/lab.sh b/scripts/interop/lab.sh index 524785f..3609e17 100755 --- a/scripts/interop/lab.sh +++ b/scripts/interop/lab.sh @@ -29,7 +29,7 @@ cd "$(dirname "$0")/../.." bgp="${CARGO_TARGET_DIR:-target}/release/bgp" work=/tmp/ferrix-lab pathspace=ferrixlab -all_phases="basics roles rpki refresh flaps restart scale limits hijacks v6only linklocal" +all_phases="basics roles rpki refresh flaps restart scale limits hijacks nexthops v6only linklocal" phases="${PHASES:-$all_phases}" scale_routes="${SCALE_ROUTES:-200000}" @@ -129,9 +129,9 @@ END # --- the daemon under test ------------------------------------------------------ -# dut_config [frr-role] [frr-max-prefixes] +# dut_config [frr-role] [frr-max-prefixes] [more of frr4's import policy] dut_config() { - local frr_role=${1:-PEER} frr_limit=${2:-2000000} + local frr_role=${1:-PEER} frr_limit=${2:-2000000} frr_import=${3:-} cat > "$work/dut.textpb" <= 1' >/dev/null \ - || fail "the kernel's status does not say why: $(api /api/kernel | jq -c .unresolved)" in_ns dut ping -q -c 1 -W 2 10.9.0.3 >/dev/null || fail "the LAN is not reachable any more" - ok "a route for the exchange LAN itself stays out of the kernel" + ok "a route for the exchange LAN itself is not taken, and stays out of the kernel" frr_configure \ "router bgp 64602" "address-family ipv4 unicast" \ "no network 10.1.0.0/24" "no network 10.9.0.0/24" "no network 10.2.7.0/24" \ "no neighbor 10.9.0.1 route-map TO-DUT out" - wait_for "the LAN's prefix to be withdrawn" dut_lacks 10.9.0.0/24 + converged + ok "back to normal" +} + +# FRR and BIRD both have a route for one prefix, and FRR's is the better. +# But its next hop is nowhere the machine can reach: then BIRD's is the +# one chosen and installed (RFC 4271 §9.1.2.1), until the next hop is +# somewhere, and again when it no longer is. +best_is() { # best_is prefix peer-name + api "/api/routes/received?prefix=$1&best=true" | jq -e --arg peer "$2" '.routes | length == 1 and .[0].peer_name == $peer' +} +phase_nexthops() { + dut_down TERM + dut_config PEER 2000000 "accept_third_party_next_hop: true" + dut_up + converged + # A prefix nobody has said anything about in the RPKI, from both. + echo "route 10.77.0.0/24 blackhole;" > "$work/bird-both.conf" + bird_config "$work/bird-both.conf" + bird_ctl configure >/dev/null + wait_for "BIRD's route for it" dut_has 10.77.0.0/24 bird4 + frr_configure \ + "ip prefix-list FAR seq 5 permit 10.77.0.0/24" \ + "route-map TO-DUT permit 10" "match ip address prefix-list FAR" "set ip next-hop 10.250.0.1" \ + "route-map TO-DUT permit 20" \ + "router bgp 64602" "address-family ipv4 unicast" \ + "neighbor 10.9.0.1 route-map TO-DUT out" "network 10.77.0.0/24" + + wait_for "FRR's route to be held" refused_for "next hop unreachable" + peer_is frr4 '.held_routes >= 1' >/dev/null || fail "the route is not held: $(peer_is frr4 .)" + best_is 10.77.0.0/24 bird4 >/dev/null || fail "BIRD's route is not the one chosen" + dut_kernel -4 "10.77.0.0/24 via 10.9.0.3 dev ix0 metric 20" || fail "BIRD's route is not in the kernel" + ok "a better route whose next hop is nowhere is held; the one that works is chosen and installed" + + # The next hop becomes a neighbor. + in_ns dut ip addr add 10.250.0.9/24 dev lan + wait_for "FRR's route to be taken" best_is 10.77.0.0/24 frr4 + wait_for "it in the kernel" dut_kernel -4 "10.77.0.0/24 via 10.250.0.1 dev lan metric 20" + ok "when its next hop can be reached the better route is taken, chosen and installed" + + # And stops being one. + in_ns dut ip addr del 10.250.0.9/24 dev lan + wait_for "BIRD's route to be chosen again" best_is 10.77.0.0/24 bird4 + wait_for "it in the kernel" dut_kernel -4 "10.77.0.0/24 via 10.9.0.3 dev ix0 metric 20" + peer_is frr4 '.held_routes >= 1' >/dev/null || fail "FRR's route is not held again" + ok "and when it no longer can, the other route is back" + + frr_configure \ + "router bgp 64602" "address-family ipv4 unicast" \ + "no network 10.77.0.0/24" "no neighbor 10.9.0.1 route-map TO-DUT out" + bird_config + bird_ctl configure >/dev/null + dut_down TERM + dut_config + dut_up converged ok "back to normal" }