diff --git a/Cargo.lock b/Cargo.lock index a101496..d4e49b4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -195,6 +195,7 @@ dependencies = [ "bytes", "foldhash", "netlink-sys", + "nix", "prometheus", "proptest", "thiserror 2.0.12", diff --git a/bin/bgp/README.md b/bin/bgp/README.md index fcfc188..8441e01 100644 --- a/bin/bgp/README.md +++ b/bin/bgp/README.md @@ -57,13 +57,30 @@ table (`import` in a peer's configuration; see `config.proto`): - Refused for any peer unless turned off: prefixes that are not routed on the Internet (private, documentation, multicast, reserved), prefixes longer than /24 or /48, a default route, paths with a private or reserved AS - number, and paths that do not begin with the peer's own AS. -- Always refused: a path with our own AS number in it. + number, paths that do not begin with the peer's own AS, and paths with an + AS_SET in them (RFC 9774). +- The next hop has to be the peer itself (`accept_third_party_next_hop` + turns that off; a peer whose role is `ROUTE_SERVER` is exempt). A next + hop is where our traffic will be sent: a peer may draw it to itself, not + point it at another member of an exchange or at a machine on some other + network of ours. +- The prefixes this router originates, and anything inside them, are not + taken from another network, unless the peer's `prefixes` list names them + (a customer with part of our space). +- Always refused: a path with our own AS number in it, and a next hop that + is no address to send to (unspecified, loopback, multicast, our own). - If configured: a list of the prefixes the peer may announce and of the AS numbers its routes may originate from. - `max_prefixes`: one route more ends the session with Cease / Maximum Number of Prefixes Reached and keeps the peer down for ten minutes. +A configured peer is one router: a second connection from its address +under another BGP Identifier is refused, so the limits above are not +multiplied by opening more sessions. And a peer whose sessions keep coming +up and failing within a minute is kept down, from the third time for a +second and for twice as long each time after, up to five minutes: every +such session costs a look through the whole table. + A refused route is dropped, along with the route it would have replaced. `bgp_routes_rejected_total` counts them by reason, and the status page shows how many routes each peer has and how many were refused. @@ -254,6 +271,12 @@ kernel { its own: it does not use anyone else's, puts its own back if one is removed, and removes them when it stops. `without_nexthop_objects: true` has every route carry its own gateway instead, as on older kernels. +- **What is the machine's own.** A route for a network the machine is + itself on, or for part of one, is never installed, whatever the policy + let into the table: the kernel reaches those directly. A peer on a shared + LAN that announces part of the LAN would otherwise take the traffic to + its neighbors, and our sessions with them. The status page counts such + routes as "inside a network of this machine". - **What is ours.** Routes are marked `proto bgp`. Every route with that mark in the configured table with the configured metric is taken to be this daemon's, and no other route is ever changed or removed. So do not @@ -266,8 +289,13 @@ kernel { SIGTERM the daemon closes its sessions and then removes its routes. After a crash they stay until the next start. - **Interfaces.** The daemon learns the machine's interfaces and addresses - from the kernel and is told when they change; a second later it works all + from the kernel and is told when they change; a second after the last + change, and five seconds after the first at the latest, it works all routes out again, since any next hop may resolve differently. +- **When the kernel does not answer.** The kernel says which requests it + refused. If it had more to say than there was room for, or says nothing + for ten seconds, the daemon asks it what it has of our routes and puts + right what is not as it should be. - **Privileges.** Changing routes needs the `CAP_NET_ADMIN` capability, and listening on port 179 needs `CAP_NET_BIND_SERVICE`; neither needs root. Without the first the daemon stops as soon as the kernel refuses it. @@ -327,6 +355,7 @@ 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 | | `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/Cargo.toml b/crates/fib/Cargo.toml index 8af5352..c41b8ea 100644 --- a/crates/fib/Cargo.toml +++ b/crates/fib/Cargo.toml @@ -22,6 +22,8 @@ tracing.workspace = true # crate builds, and is tested, anywhere. [target.'cfg(target_os = "linux")'.dependencies] netlink-sys = { version = "0.8", features = ["tokio_socket"] } +# For a socket option netlink-sys has no name for. +nix = { workspace = true, features = ["socket"] } [dev-dependencies] proptest.workspace = true diff --git a/crates/fib/src/interfaces.rs b/crates/fib/src/interfaces.rs index f74f2ad..51386d4 100644 --- a/crates/fib/src/interfaces.rs +++ b/crates/fib/src/interfaces.rs @@ -85,6 +85,15 @@ impl Interfaces { self.addresses.iter().any(|ours| ours.local == address) } + /// Whether `prefix` is the network of one of this machine's addresses, + /// or inside one. Whether the interface is up does not matter: the + /// network is ours either way. + pub fn has_network_around(&self, prefix: &IpPrefix) -> bool { + self.addresses + .iter() + .any(|address| address.network.contains(prefix)) + } + /// Where `neighbor` can be reached without a gateway: the interface /// that is up and has an address in a network `neighbor` is in, the /// most specific such network if there are several. `None` if there is diff --git a/crates/fib/src/kernel.rs b/crates/fib/src/kernel.rs index d54bb4f..cad6b41 100644 --- a/crates/fib/src/kernel.rs +++ b/crates/fib/src/kernel.rs @@ -57,11 +57,27 @@ const READ_BUFFER: usize = 256 * 1024; /// are worked out again. Such changes come in bursts. const SETTLE: Duration = Duration::from_secs(1); +/// How long the routes are worked out again after at the latest, however +/// long the changes keep coming: on a machine whose interfaces come and go +/// all the time there is never a quiet second. +const SETTLE_AT_MOST: Duration = Duration::from_secs(5); + +/// How much the kernel may have waiting for us on a socket. It answers +/// every request it refuses, and a write is `WRITE_BATCH` requests that +/// it deals with before any answer is read. The usual allowance holds +/// fewer than three hundred such answers. +const RECEIVE_BUFFER: usize = 8 * 1024 * 1024; + +/// How long the kernel is waited for. It answers within microseconds, or +/// not at all. +const ANSWER_TIMEOUT: Duration = Duration::from_secs(10); + /// The errors of the kernel that have a meaning here. mod errno { pub const EPERM: i32 = 1; pub const ENOENT: i32 = 2; pub const ESRCH: i32 = 3; + pub const ENOBUFS: i32 = 105; } static ROUTES: LazyLock = LazyLock::new(|| { @@ -114,6 +130,20 @@ impl Socket { fn open(groups: u32) -> io::Result { let mut socket = TokioSocket::new(NETLINK_ROUTE)?; socket.socket_mut().bind(&SocketAddr::new(0, groups))?; + // Room for the answers to a whole write, and answers that do not + // repeat the request. Both are for the better and neither is + // needed: what is lost for want of room is noticed and made good. + let raw = socket.socket_ref(); + if nix::sys::socket::setsockopt( + raw, + nix::sys::socket::sockopt::RcvBufForce, + &RECEIVE_BUFFER, + ) + .is_err() + { + let _ = raw.set_rx_buf_sz(RECEIVE_BUFFER); + } + let _ = raw.set_cap_ack(true); Ok(Self { socket, sequence: 0, @@ -134,6 +164,19 @@ impl Socket { Ok(&self.buf) } + /// Waits for the kernel's answer to something it was asked. + async fn answer(&mut self) -> io::Result<&[u8]> { + self.buf.clear(); + self.buf.reserve(READ_BUFFER); + match tokio::time::timeout(ANSWER_TIMEOUT, self.socket.recv(&mut self.buf)).await { + Ok(received) => received.map(|_| &self.buf[..]), + Err(_) => Err(io::Error::new( + io::ErrorKind::TimedOut, + "the kernel did not answer", + )), + } + } + /// Sends one request and gives each message of the answer to `each`, /// up to the one that says what became of the request, whose error /// number (zero for none) is returned. A dump ends without one, and @@ -148,7 +191,7 @@ impl Socket { request(&mut wire, sequence); self.socket.send(&wire).await?; loop { - let mut rest = self.read().await?; + let mut rest = self.answer().await?; while !rest.is_empty() { let (header, message, used) = decode(rest)?; rest = &rest[used..]; @@ -190,11 +233,13 @@ impl Socket { /// Asks the kernel for `requests`, each a change to a route, and /// returns which of them it refused: the place in `requests` and the /// error's number. - async fn change( - &mut self, - requests: &[(Change, Route)], - ) -> Result, FibError> { + /// + /// The kernel may have refused more than there was room to say + /// (`ENOBUFS`), or may not have answered. Then what became of some + /// requests is not known, and `Changed::lost` says so. + async fn change(&mut self, requests: &[(Change, Route)]) -> Result { let mut refused = Vec::new(); + let mut lost = false; for batch_start in (0..requests.len()).step_by(WRITE_BATCH) { let batch = &requests[batch_start..requests.len().min(batch_start + WRITE_BATCH)]; // One write for all of them. The kernel answers a request only @@ -210,7 +255,17 @@ impl Socket { self.socket.send(&wire).await?; let mut done = false; while !done { - let mut rest = self.read().await?; + let mut rest = match self.answer().await { + Ok(rest) => rest, + Err(e) + if e.raw_os_error() == Some(errno::ENOBUFS) + || e.kind() == io::ErrorKind::TimedOut => + { + lost = true; + break; + } + Err(e) => return Err(e.into()), + }; while !rest.is_empty() { let (header, message, used) = decode(rest)?; rest = &rest[used..]; @@ -228,10 +283,19 @@ impl Socket { } } } - Ok(refused) + Ok(Changed { refused, lost }) } } +/// What became of changes the kernel was asked for. +struct Changed { + /// Those it refused: the place among the requests, and the error's + /// number. + refused: Vec<(usize, i32)>, + /// Whether some of its answers were lost. + lost: bool, +} + /// The sockets the task uses. pub(crate) struct Sockets { /// For asking questions, and for the few changes that are made one at @@ -262,6 +326,37 @@ impl Sockets { } } +/// When the routes are next to be worked out again, if they are. +/// +/// Reasons for it come in bursts, so it waits for a quiet moment; but not +/// for ever, on a machine that has none. +#[derive(Default)] +struct Again { + /// When the first reason came, and when the latest. + since: Option<(Instant, Instant)>, +} + +impl Again { + /// There is a reason to work the routes out again. + fn soon(&mut self) { + let now = Instant::now(); + self.since = Some((self.since.map_or(now, |(first, _)| first), now)); + } + + fn due(&self) -> Option { + self.since + .map(|(first, latest)| (latest + SETTLE).min(first + SETTLE_AT_MOST)) + } +} + +/// Waits until `when`; for ever if there is no such time. +async fn at(when: Option) { + match when { + Some(when) => sleep_until(when).await, + None => std::future::pending().await, + } +} + /// Changes to routes for the kernel, in the order they are to be made. struct Batch { ops: Vec, @@ -278,6 +373,9 @@ enum Report { Refused { op: Op, errno: i32 }, /// The batch with this mark, and all before it, are done. Done(u64), + /// Some of the kernel's answers were lost: what it has of ours is no + /// longer known, and has to be asked for. + Lost, } /// Sends the batches to the kernel as they come, and reports what became @@ -290,13 +388,17 @@ async fn write( reports: mpsc::UnboundedSender, ) -> Result<(), FibError> { while let Some(batch) = batches.recv().await { - for (at, errno) in socket.change(&batch.requests).await? { + let changed = socket.change(&batch.requests).await?; + for (at, errno) in changed.refused { // Nobody to tell only when everything is being torn down. let _ = reports.send(Report::Refused { op: batch.ops[at], errno, }); } + if changed.lost { + let _ = reports.send(Report::Lost); + } if let Some(mark) = batch.mark { let _ = reports.send(Report::Done(mark)); } @@ -697,10 +799,18 @@ impl Task { interface: peer.interface, }); let interfaces = &self.interfaces; - let target = *self - .resolved - .entry((next_hop, prefix.address_family(), peer)) - .or_insert_with(|| resolve(interfaces, &prefix, next_hop.as_ref(), peer)); + // Asked of every route, and not remembered with the + // next hop: it is about the prefix. + let target = if interfaces.has_network_around(&prefix) { + Err(Unresolved::OwnNetwork) + } else { + *self + .resolved + .entry((next_hop, prefix.address_family(), peer)) + .or_insert_with(|| { + resolve(interfaces, &prefix, next_hop.as_ref(), peer) + }) + }; ops.extend(self.plan.best(prefix, target)); } News::Withdrawn(prefix) => ops.extend(self.plan.withdrawn(prefix)), @@ -725,6 +835,13 @@ impl Task { } Ok(false) } + Report::Lost => { + ERRORS.inc(); + warn!( + "Some of the kernel's answers were lost; asking it what it has of our routes" + ); + Ok(true) + } Report::Refused { op, errno } => { let instead = self.refused(&op, errno)?; self.send(instead.into_iter().collect(), false).await?; @@ -743,6 +860,43 @@ impl Task { } } + /// Takes in what the kernel says unasked, or the failure to read it. + /// Returns whether the routes should be worked out again. + async fn on_news(&mut self, read: io::Result<&[u8]>) -> Result { + let mut rest = match read { + Ok(rest) => rest, + // The kernel had more to say than the socket had room for: + // what was missed is asked for. + Err(e) if e.raw_os_error() == Some(errno::ENOBUFS) => { + warn!("Missed news from the kernel; asking for the interfaces again"); + self.learn_interfaces().await?; + return Ok(true); + } + Err(e) => return Err(e.into()), + }; + let mut changed = false; + while !rest.is_empty() { + let (_, message, used) = decode(rest)?; + rest = &rest[used..]; + if self.interfaces.apply(&message) { + self.resolved.clear(); + changed = true; + } + // A nexthop object of ours has been removed by someone, and + // the routes that named it with it. + if let Message::Nexthop { + present: false, + nexthop, + } = &message + && self.owned.contains(&nexthop.id) + { + warn!(id = nexthop.id, "A nexthop object of ours was removed"); + changed = true; + } + } + Ok(changed) + } + /// Finds out anew what the kernel has of ours. When interfaces or /// nexthop objects go, the kernel removes the routes that used them /// without a word, and what was thought to be there no longer is. @@ -756,7 +910,9 @@ impl Task { self.synced = false; self.plan = Plan::default(); let nexthops = self.kernel_nexthops().await?; - self.adopt_routes(&nexthops, false).await?; + // While routes of an earlier run are being kept for the peers to + // announce again, they are kept through this too. + self.adopt_routes(&nexthops, self.keeping).await?; self.use_nexthops(&nexthops); Ok(()) } @@ -924,7 +1080,7 @@ pub(crate) async fn run( // A reader is first given the whole table, which is what brings the // kernel in step; starting a new one does it over. let mut reader = task.rib.reader(); - let mut again_at: Option = None; + let mut again = Again::default(); loop { tokio::select! { () = reader.changed() => { @@ -932,60 +1088,26 @@ pub(crate) async fn run( task.publish(false); } Some(report) = heard.recv() => { - if task.hear(report).await? && again_at.is_none() { - again_at = Some(Instant::now() + SETTLE); + if task.hear(report).await? { + again.soon(); } task.publish(false); } read = news.read() => { - let changed = match read { - Ok(mut rest) => { - let mut changed = false; - while !rest.is_empty() { - let (_, message, used) = decode(rest)?; - rest = &rest[used..]; - if task.interfaces.apply(&message) { - task.resolved.clear(); - changed = true; - } - // A nexthop object of ours has been removed - // by someone, and the routes that named it - // with it. - if let Message::Nexthop { present: false, nexthop } = &message - && task.owned.contains(&nexthop.id) - { - warn!(id = nexthop.id, "A nexthop object of ours was removed"); - changed = true; - } - } - changed - } - // The kernel had more to say than the socket had room - // for (ENOBUFS): what was missed is asked for. - Err(e) => { - warn!(err = %e, "Missed news from the kernel; asking for the interfaces again"); - task.learn_interfaces().await?; - true - } - }; - if changed { + if task.on_news(read).await? { // What a next hop resolves to may be different now, // for any route. - again_at = Some(Instant::now() + SETTLE); + again.soon(); task.publish(true); } } - () = async { sleep_until(again_at.expect("checked by the guard")).await }, - if again_at.is_some() => - { - again_at = None; + () = at(again.due()), if again.due().is_some() => { + again = Again::default(); info!("Working the kernel's routes out again"); task.relearn(&mut heard).await?; reader = task.rib.reader(); } - () = async { sleep_until(release_at.expect("checked by the guard")).await }, - if release_at.is_some() => - { + () = at(release_at), if release_at.is_some() => { release_at = None; task.keeping = false; let stale = task.plan.release(); diff --git a/crates/fib/src/plan.rs b/crates/fib/src/plan.rs index 2fa31b7..4a2a88d 100644 --- a/crates/fib/src/plan.rs +++ b/crates/fib/src/plan.rs @@ -36,14 +36,20 @@ pub enum Unresolved { /// Finding the way to such a next hop through other routes is not /// done. NotConnected, + /// The route is for a network this machine is on itself, or for part + /// of one. The kernel reaches that directly; a route from a peer that + /// said otherwise would send the neighbors' traffic, and the sessions + /// with them, wherever the peer liked. + OwnNetwork, } impl Unresolved { - pub const ALL: [Self; 4] = [ + pub const ALL: [Self; 5] = [ Self::Missing, Self::OtherFamily, Self::Ourselves, Self::NotConnected, + Self::OwnNetwork, ]; pub fn as_str(self) -> &'static str { @@ -52,6 +58,7 @@ impl Unresolved { Self::OtherFamily => "IPv4 next hop for an IPv6 route", Self::Ourselves => "next hop is this machine", Self::NotConnected => "next hop not directly connected", + Self::OwnNetwork => "inside a network of this machine", } } } @@ -91,6 +98,9 @@ pub fn resolve( next_hop: Option<&NlriNextHop>, peer: Option, ) -> Result { + if interfaces.has_network_around(prefix) { + return Err(Unresolved::OwnNetwork); + } let next_hop = next_hop.ok_or(Unresolved::Missing)?; let version = match next_hop { NlriNextHop::Ipv4(_) => IpVersion::Ipv4, @@ -208,7 +218,7 @@ pub struct Plan { unresolved: HashMap, /// How many of them for each reason, in the order of /// [`Unresolved::ALL`]. - unresolved_counts: [usize; 4], + unresolved_counts: [usize; Unresolved::ALL.len()], /// Prefixes whose route the kernel refused. refused: HashSet, } @@ -308,7 +318,7 @@ impl Plan { entry.confirmed = false; } self.unresolved.clear(); - self.unresolved_counts = [0; 4]; + self.unresolved_counts = [0; Unresolved::ALL.len()]; self.refused.clear(); } @@ -394,7 +404,7 @@ impl Plan { /// Everything goes: the changes that empty the kernel of our routes. pub fn clear(&mut self) -> Vec { self.unresolved.clear(); - self.unresolved_counts = [0; 4]; + self.unresolved_counts = [0; Unresolved::ALL.len()]; self.refused.clear(); self.installed .drain() @@ -485,6 +495,34 @@ mod tests { resolve(&interfaces, &v6, Some(&v4_hop("192.0.2.2")), peer), Err(Unresolved::OtherFamily) ); + // A route for part of a network we are on is not for the kernel, + // however good its next hop; one for more than the network is. + for ours in [ + "192.0.2.0/24", + "192.0.2.128/25", + "192.0.2.7/32", + "2001:db8::/64", + ] { + let ours = prefix(ours); + let hop = match ours.address_family() { + IpVersion::Ipv4 => v4_hop("192.0.2.2"), + IpVersion::Ipv6 => v6_hop("2001:db8::2"), + }; + assert_eq!( + resolve(&interfaces, &ours, Some(&hop), peer), + Err(Unresolved::OwnNetwork), + "{ours}" + ); + } + assert_eq!( + resolve( + &interfaces, + &prefix("192.0.0.0/22"), + Some(&v4_hop("192.0.2.2")), + peer + ), + Ok(target("192.0.2.2", 2)) + ); // An IPv4 route by way of an IPv6 neighbor: a link with only IPv6 // addresses (RFC 8950). assert_eq!( diff --git a/crates/fib/tests/kernel.rs b/crates/fib/tests/kernel.rs index ede5a20..49611d7 100644 --- a/crates/fib/tests/kernel.rs +++ b/crates/fib/tests/kernel.rs @@ -460,6 +460,59 @@ async fn many_routes_are_installed_and_removed() { ); } +/// Someone empties the table behind our back (`ip route flush`), and then +/// the peers withdraw everything: the kernel refuses every removal, a +/// table's worth of refusals at once. That is more than the socket holds +/// answers for, and must be no more than a reason to look again. +#[tokio::test(flavor = "multi_thread")] +#[ignore = "changes the machine's routes; run scripts/kernel-tests.sh"] +async fn a_flood_of_refusals_does_not_stop_the_daemon() { + const ROUTES: u32 = 20_000; + let mut machine = Machine::new(11); + machine.nexthop_objects = false; + let peer = machine.peer(2); + let via = machine.via(machine.neighbor(2).into(), 0); + let prefixes: Vec = (0..ROUTES) + .map(|n| { + let [_, a, b, c] = n.to_be_bytes(); + IpPrefix::new( + bgp_primitives::constants::IpVersion::Ipv4, + [100 + a, b, c, 0], + 24, + ) + .unwrap() + }) + .collect(); + machine.rib.announce(peer, &via, &prefixes); + let (fib, task) = machine.start(); + status_becomes(&fib, "all installed", |status| { + status.synced && status.installed == ROUTES as usize + }); + + ip(&format!("route flush table {}", machine.table())); + assert!(machine.kernel_routes().is_empty()); + let routes: Vec = + prefixes.iter().map(|prefix| (*prefix).into()).collect(); + machine.rib.withdraw(peer, &routes); + status_becomes(&fib, "nothing installed", |status| { + status.synced && status.installed == 0 + }); + assert!( + !task.is_finished(), + "the task that talks to the kernel ended" + ); + + // And it goes on working. + machine.rib.announce(peer, &via, &prefixes[..3]); + status_becomes(&fib, "three installed", |status| { + status.synced && status.installed == 3 + }); + assert_eq!(machine.kernel_routes().len(), 3); + assert!(!task.is_finished()); + fib.stop().await; + task.await.unwrap().unwrap(); +} + /// Routes name a nexthop object for their gateway where the kernel has /// such objects. The object is ours: made when first needed, put back if /// someone removes it, and removed when we stop. Other objects are not diff --git a/crates/integration-tests/src/lib.rs b/crates/integration-tests/src/lib.rs index 694548d..cc5ae02 100644 --- a/crates/integration-tests/src/lib.rs +++ b/crates/integration-tests/src/lib.rs @@ -1,3 +1,13 @@ pub mod test_cache; pub mod test_peer; pub mod test_server; + +/// The policy a peer has with nothing configured, but for one thing: the +/// peers of these tests are all at 127.0.0.1, which no route can have as +/// its next hop, so their routes name another. +pub fn default_policy() -> bgp_server::policy::ImportPolicy { + bgp_server::policy::ImportPolicy { + accept_third_party_next_hop: true, + ..Default::default() + } +} diff --git a/crates/integration-tests/tests/aspa.rs b/crates/integration-tests/tests/aspa.rs index 77ec20a..44b21f6 100644 --- a/crates/integration-tests/tests/aspa.rs +++ b/crates/integration-tests/tests/aspa.rs @@ -6,6 +6,7 @@ use std::collections::BTreeSet; use std::net::Ipv4Addr; use std::time::Duration; +use bgp_integration_tests::default_policy; use bgp_integration_tests::test_cache::TestCache; use bgp_integration_tests::test_peer::TestPeer; use bgp_integration_tests::test_server::TestServer; @@ -154,8 +155,7 @@ fn aspas() -> Vec<(u32, Vec)> { #[tokio::test] async fn test_a_leaked_route_is_refused_and_the_rest_are_taken() -> Result<()> { let cache = TestCache::start_with(vec![], aspas()).await; - let Setup { server, .. } = - server_with(&cache, Some(PeerRole::Peer), ImportPolicy::default()).await?; + let Setup { server, .. } = server_with(&cache, Some(PeerRole::Peer), default_policy()).await?; let (base, _http) = server.serve_http().await?; let mut peer = established(&server).await?; @@ -211,7 +211,7 @@ async fn test_a_route_that_has_crossed_a_peering_is_taken_from_a_provider_only() (None, true), ] { let cache = TestCache::start_with(vec![], aspas()).await; - let Setup { server, .. } = server_with(&cache, role, ImportPolicy::default()).await?; + let Setup { server, .. } = server_with(&cache, role, default_policy()).await?; let mut peer = established(&server).await?; announce(&mut peer, &["8.8.8.0/24"], &[PEER_ASN, 3356, 15169]).await?; let expected = if taken { @@ -232,7 +232,7 @@ async fn test_paths_from_a_route_server_do_not_begin_with_its_as() -> Result<()> let cache = TestCache::start_with(vec![], aspas()).await; let import = ImportPolicy { accept_any_first_as: true, - ..ImportPolicy::default() + ..default_policy() }; let Setup { server, .. } = server_with(&cache, Some(PeerRole::RouteServer), import).await?; let mut peer = established(&server).await?; @@ -250,8 +250,7 @@ async fn test_paths_from_a_route_server_do_not_begin_with_its_as() -> Result<()> #[tokio::test] async fn test_routes_follow_the_aspas() -> Result<()> { let cache = TestCache::start_with(vec![], aspas()).await; - let Setup { server, .. } = - server_with(&cache, Some(PeerRole::Peer), ImportPolicy::default()).await?; + let Setup { server, .. } = server_with(&cache, Some(PeerRole::Peer), default_policy()).await?; let mut peer = established(&server).await?; announce(&mut peer, &["194.0.0.0/24"], &[PEER_ASN, 2121]).await?; @@ -311,7 +310,7 @@ async fn test_a_peer_can_be_exempt_from_path_verification() -> Result<()> { let cache = TestCache::start_with(vec![], aspas()).await; let import = ImportPolicy { accept_aspa_invalid: true, - ..ImportPolicy::default() + ..default_policy() }; let Setup { server, .. } = server_with(&cache, Some(PeerRole::Peer), import).await?; let (base, _http) = server.serve_http().await?; @@ -337,8 +336,7 @@ async fn test_a_route_is_held_until_origin_and_path_are_both_in_order() -> Resul asn, }; let cache = TestCache::start_with(vec![google(13335)], aspas()).await; - let Setup { server, .. } = - server_with(&cache, Some(PeerRole::Peer), ImportPolicy::default()).await?; + let Setup { server, .. } = server_with(&cache, Some(PeerRole::Peer), default_policy()).await?; let mut peer = established(&server).await?; announce(&mut peer, &["8.8.8.0/24"], &[PEER_ASN, 3356, 15169]).await?; table_becomes(&server, &[], 1).await?; diff --git a/crates/integration-tests/tests/debugging.rs b/crates/integration-tests/tests/debugging.rs index 007cdf2..8ef82a5 100644 --- a/crates/integration-tests/tests/debugging.rs +++ b/crates/integration-tests/tests/debugging.rs @@ -4,6 +4,7 @@ use std::net::Ipv4Addr; use std::time::Duration; +use bgp_integration_tests::default_policy; use bgp_integration_tests::test_peer::TestPeer; use bgp_integration_tests::test_server::TestServer; use bgp_packet::message::{ @@ -175,7 +176,7 @@ async fn test_refused_routes_can_be_seen_and_counted() -> Result<()> { add_path: None, import: ImportPolicy { max_prefixes: Some(100), - ..ImportPolicy::default() + ..default_policy() }, role: None, require_role: false, diff --git a/crates/integration-tests/tests/import_policy.rs b/crates/integration-tests/tests/import_policy.rs index 57483df..69a1dd8 100644 --- a/crates/integration-tests/tests/import_policy.rs +++ b/crates/integration-tests/tests/import_policy.rs @@ -5,6 +5,7 @@ use std::collections::BTreeSet; use std::net::Ipv4Addr; use std::time::Duration; +use bgp_integration_tests::default_policy; use bgp_integration_tests::test_peer::TestPeer; use bgp_integration_tests::test_server::TestServer; use bgp_packet::errors::{BgpError, CeaseSubcode}; @@ -118,7 +119,7 @@ fn set(prefixes: &[&str]) -> BTreeSet { /// through us. Everything a peer sent used to be taken. #[tokio::test] async fn test_routes_that_no_peer_should_announce_are_refused() -> Result<()> { - let (server, _listener) = server_with(ImportPolicy::default()).await?; + let (server, _listener) = server_with(default_policy()).await?; let mut peer = established(&server).await?; // One UPDATE with good and bad prefixes: the good ones are taken. @@ -156,7 +157,7 @@ async fn test_routes_that_no_peer_should_announce_are_refused() -> Result<()> { /// and the new one is not acceptable. #[tokio::test] async fn test_a_refused_route_replaces_the_one_that_was_taken() -> Result<()> { - let (server, _listener) = server_with(ImportPolicy::default()).await?; + let (server, _listener) = server_with(default_policy()).await?; let mut peer = established(&server).await?; announce(&mut peer, &["193.0.0.0/21", "193.0.16.0/24"], &[PEER_ASN]).await?; @@ -176,7 +177,7 @@ async fn test_a_peer_is_held_to_its_prefix_list_and_origins() -> Result<()> { let (server, _listener) = server_with(ImportPolicy { prefixes: Some(prefixes), origin_asns: Some([PEER_ASN, 2121].into()), - ..ImportPolicy::default() + ..default_policy() }) .await?; let mut peer = established(&server).await?; @@ -206,7 +207,7 @@ async fn test_a_peer_over_its_limit_is_cut_off_and_kept_down() -> Result<()> { let (server, listener) = server_with(ImportPolicy { max_prefixes: Some(3), hold_down: Duration::from_millis(1500), - ..ImportPolicy::default() + ..default_policy() }) .await?; // The server dials the peer as well; that connection is left unanswered diff --git a/crates/integration-tests/tests/roles.rs b/crates/integration-tests/tests/roles.rs index 29d5f5e..d66e085 100644 --- a/crates/integration-tests/tests/roles.rs +++ b/crates/integration-tests/tests/roles.rs @@ -5,6 +5,7 @@ use std::net::{Ipv4Addr, Ipv6Addr}; use std::time::Duration; +use bgp_integration_tests::default_policy; use bgp_integration_tests::test_peer::TestPeer; use bgp_integration_tests::test_server::TestServer; use bgp_packet::errors::{BgpError, OpenMsgSubcode}; @@ -18,7 +19,7 @@ use bgp_primitives::ip_prefix::IpPrefix; use bgp_rib::LocalRoute; use bgp_server::config::PeerConfig; use bgp_server::family::FamilySet; -use bgp_server::policy::{ImportPolicy, PeerRole}; +use bgp_server::policy::PeerRole; use eyre::{Result, bail}; use serde_json::Value; use tokio::net::TcpListener; @@ -54,7 +55,7 @@ async fn server_with(role: Option, require_role: bool) -> Result Res #[tokio::test] async fn test_an_invalid_route_is_refused_and_the_rest_are_taken() -> Result<()> { let cache = TestCache::start(vec![vrp("193.0.0.0/21", 24, PEER_ASN)]).await; - let Setup { server, .. } = server_with(&cache, None, ImportPolicy::default()).await?; + let Setup { server, .. } = server_with(&cache, None, default_policy()).await?; let (base, _http) = server.serve_http().await?; let mut peer = established(&server).await?; @@ -222,7 +224,7 @@ async fn test_an_invalid_route_is_refused_and_the_rest_are_taken() -> Result<()> async fn test_routes_follow_the_validation_data() -> Result<()> { let ripe = vrp("193.0.0.0/21", 24, PEER_ASN); let cache = TestCache::start(vec![ripe]).await; - let Setup { server, .. } = server_with(&cache, None, ImportPolicy::default()).await?; + let Setup { server, .. } = server_with(&cache, None, default_policy()).await?; let mut peer = established(&server).await?; // Nothing is known about 8.8.8.0/24, so it is taken. 193.0.4.0/24 is @@ -265,7 +267,7 @@ async fn test_routes_follow_the_validation_data() -> Result<()> { async fn test_a_held_route_the_peer_withdrew_does_not_come_back() -> Result<()> { let ripe = vrp("193.0.0.0/21", 24, PEER_ASN); let cache = TestCache::start(vec![ripe]).await; - let Setup { server, .. } = server_with(&cache, None, ImportPolicy::default()).await?; + let Setup { server, .. } = server_with(&cache, None, default_policy()).await?; let mut peer = established(&server).await?; announce(&mut peer, &["193.0.4.0/24", "193.0.5.0/24"], 15169).await?; @@ -302,7 +304,7 @@ async fn test_a_peer_can_be_exempt_from_validation() -> Result<()> { let cache = TestCache::start(vec![vrp("193.0.0.0/21", 24, PEER_ASN)]).await; let import = ImportPolicy { accept_rpki_invalid: true, - ..ImportPolicy::default() + ..default_policy() }; let Setup { server, .. } = server_with(&cache, None, import).await?; let (base, _http) = server.serve_http().await?; @@ -325,12 +327,8 @@ async fn test_a_peer_can_be_exempt_from_validation() -> Result<()> { #[tokio::test] async fn test_held_routes_are_taken_when_the_validation_data_expires() -> Result<()> { let cache = TestCache::start(vec![vrp("193.0.0.0/21", 24, PEER_ASN)]).await; - let Setup { server, .. } = server_with( - &cache, - Some(Duration::from_millis(1500)), - ImportPolicy::default(), - ) - .await?; + let Setup { server, .. } = + server_with(&cache, Some(Duration::from_millis(1500)), default_policy()).await?; let mut peer = established(&server).await?; announce(&mut peer, &["193.0.4.0/24"], 15169).await?; table_becomes(&server, &[], 1).await?; @@ -340,3 +338,64 @@ async fn test_held_routes_are_taken_when_the_validation_data_expires() -> Result assert!(server.rpki.as_ref().unwrap().current().vrps.is_empty()); Ok(()) } + +/// A hijack that was possible: on a session with two-octet AS numbers a +/// route has two paths, AS_PATH and AS4_PATH, and a peer can make them +/// say different things. Here AS_PATH is the peer alone, and an AS4_PATH +/// too long to be used ends at the AS that holds the addresses. The route +/// is the peer's and invalid, when it arrives and when it is looked at +/// again; nothing may take the unused path's word for where it comes from. +#[tokio::test] +async fn test_an_unused_as4_path_does_not_make_a_route_valid() -> Result<()> { + use bgp_packet::message::AsPathSegment; + const HOLDER: u32 = 15169; + let cache = TestCache::start(vec![vrp("193.0.0.0/21", 24, HOLDER)]).await; + let Setup { server, .. } = server_with(&cache, None, default_policy()).await?; + + // No capabilities: AS numbers are two octets wide on this session. + let mut peer = TestPeer::connect(server.addr).await?; + peer.handshake_with(PEER_ASN, Ipv4Addr::new(10, 0, 0, 2), 90, vec![]) + .await?; + tokio::time::timeout(TIMEOUT, async { + while server.registry.list().is_empty() { + tokio::time::sleep(Duration::from_millis(5)).await; + } + }) + .await?; + assert!(!server.registry.list()[0].four_octet_asn); + + peer.send(&Message::Update(UpdateMessage { + withdrawn_routes: vec![], + path_attributes: vec![ + PathAttribute::Origin(OriginPathAttribute::IGP), + AsPathAttribute::from_asns(vec![PEER_ASN]), + PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::new(192, 0, 2, 1))), + PathAttribute::As4Path(AsPathAttribute { + segments: vec![AsPathSegment { + ordered: true, + path: vec![3356, HOLDER], + }], + }), + ], + announced_routes: vec![prefix("193.0.4.0/24").into()], + })) + .await?; + table_becomes(&server, &[], 1).await?; + + // The data changes, and every held route is looked at again. + cache.set(vec![ + vrp("193.0.0.0/21", 24, HOLDER), + vrp("198.51.100.0/24", 24, 64500), + ]); + let rpki = server.rpki.clone().unwrap(); + tokio::time::timeout(TIMEOUT, async { + while rpki.current().vrps.len() != 2 { + tokio::time::sleep(Duration::from_millis(5)).await; + } + }) + .await?; + // Time enough for the look, and then some. + tokio::time::sleep(Duration::from_millis(600)).await; + table_becomes(&server, &[], 1).await?; + Ok(()) +} diff --git a/crates/packet/src/message.rs b/crates/packet/src/message.rs index b5d3935..8047899 100644 --- a/crates/packet/src/message.rs +++ b/crates/packet/src/message.rs @@ -767,6 +767,53 @@ impl AsPathAttribute { }) } + /// How long the path is, as RFC 4271 §9.1.2.2 counts: one for each AS + /// of a sequence, one for a whole set. + pub fn hops(&self) -> usize { + self.segments + .iter() + .map(|segment| { + if segment.ordered { + segment.path.len() + } else { + 1 + } + }) + .sum() + } + + /// The path as it really is, given what a two-octet session carried: + /// this, with AS_TRANS wherever a number did not fit, and `wide`, the + /// AS4_PATH that has the numbers themselves for the path's tail + /// (RFC 6793 §4.2.3). What `wide` does not cover is kept from the + /// front of this. + /// + /// `None` if `wide` is longer than the path, which is not to be used: + /// some speaker on the way that knew nothing of AS4_PATH changed the + /// one and not the other. + pub fn with_as4_path(&self, wide: &AsPathAttribute) -> Option { + let mut keep = self.hops().checked_sub(wide.hops())?; + let mut segments = Vec::with_capacity(self.segments.len() + wide.segments.len()); + for segment in &self.segments { + if keep == 0 { + break; + } + let (hops, taken) = if segment.ordered { + let taken = segment.path.len().min(keep); + (taken, &segment.path[..taken]) + } else { + (1, &segment.path[..]) + }; + keep -= hops; + segments.push(AsPathSegment { + ordered: segment.ordered, + path: taken.to_vec(), + }); + } + segments.extend(wide.segments.iter().cloned()); + Some(AsPathAttribute { segments }) + } + /// Parses an AS_PATH with AS numbers as wide as the session negotiated /// (RFC 6793). pub fn from_wire<'a>(ctx: &ParserContext, buf: &'a [u8]) -> Parsed<'a, Self> { diff --git a/crates/packet/src/update_decode.rs b/crates/packet/src/update_decode.rs index 2282702..4ccf50f 100644 --- a/crates/packet/src/update_decode.rs +++ b/crates/packet/src/update_decode.rs @@ -185,6 +185,35 @@ impl UpdateMessage { } } +/// Makes one path of the two a session with two-octet AS numbers brings: +/// AS_PATH becomes the path as it really is, and AS4_PATH is gone, used or +/// not (RFC 6793 §4.2.3). +/// +/// For whoever receives an UPDATE to do once, before anything looks at +/// the path: the checks on it, the choice between routes, validation +/// against the RPKI now and again later. Were each to read the two for +/// itself, a peer could have them disagree about where a route comes +/// from. +pub fn merge_as4_path(attributes: &mut Vec) { + let Some(at) = attributes + .iter() + .position(|attribute| matches!(attribute, PathAttribute::As4Path(_))) + else { + return; + }; + let PathAttribute::As4Path(wide) = attributes.remove(at) else { + return; + }; + for attribute in attributes { + if let PathAttribute::AsPath(path) = attribute { + if let Some(merged) = path.with_as4_path(&wide) { + *path = merged; + } + return; + } + } +} + /// What to do with an attribute, once it is clear. enum Verdict { /// Use it. `route_property` is false for one that was read only to find @@ -908,6 +937,18 @@ mod tests { .iter() .any(|a| matches!(a, PathAttribute::As4Path(_))) ); + // The two made one: AS 4200000001 where AS_PATH had AS_TRANS, and + // no AS4_PATH. + let mut one_path = decoded.update.path_attributes; + merge_as4_path(&mut one_path); + assert_eq!( + one_path, + [ + PathAttribute::Origin(OriginPathAttribute::IGP), + AsPathAttribute::from_asns(vec![4_200_000_001]), + PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::new(192, 0, 2, 1))), + ] + ); let bad = attr(OPTIONAL_TRANSITIVE, AS4_PATH, &[2, 1, 0xfa, 0x56]); let decoded = UpdateMessage::decode(&ctx, &body(&[], &attributes(bad), &NLRI)).unwrap(); @@ -918,6 +959,69 @@ mod tests { assert_eq!(decoded.update.announced_routes, [route()]); } + /// RFC 6793 §4.2.3. AS4_PATH is the tail of the path; the front is + /// kept from AS_PATH. One longer than the path is not used, and is + /// gone all the same: nothing after the decoder is to see two paths, + /// or a peer could have them disagree about a route's origin. + #[test] + fn as4_path_is_merged_into_the_path() { + use crate::message::AsPathSegment; + let sequence = |path: &[u32]| AsPathSegment { + ordered: true, + path: path.to_vec(), + }; + let set = |path: &[u32]| AsPathSegment { + ordered: false, + path: path.to_vec(), + }; + let merged = |narrow: Vec, wide: Vec| { + let mut attributes = vec![ + PathAttribute::Origin(OriginPathAttribute::IGP), + PathAttribute::As4Path(AsPathAttribute { segments: wide }), + PathAttribute::AsPath(AsPathAttribute { segments: narrow }), + ]; + merge_as4_path(&mut attributes); + assert_eq!(attributes.len(), 2, "AS4_PATH is gone"); + match attributes.pop() { + Some(PathAttribute::AsPath(path)) => path.segments, + other => panic!("{other:?}"), + } + }; + assert_eq!( + merged( + vec![sequence(&[3333, 23456, 23456])], + vec![sequence(&[210_036, 4_200_000_001])] + ), + [sequence(&[3333]), sequence(&[210_036, 4_200_000_001])] + ); + // As long as the path: all of it. + assert_eq!( + merged(vec![sequence(&[23456])], vec![sequence(&[210_036])]), + [sequence(&[210_036])] + ); + // Longer than the path: not used. The hijack this would allow is + // a path [3333] that says it ends at someone else's AS. + assert_eq!( + merged(vec![sequence(&[3333])], vec![sequence(&[64500, 1299])]), + [sequence(&[3333])] + ); + // A set counts as one, whole. + assert_eq!( + merged( + vec![sequence(&[3333]), set(&[1, 2]), sequence(&[23456])], + vec![sequence(&[210_036])] + ), + [sequence(&[3333]), set(&[1, 2]), sequence(&[210_036])] + ); + assert_eq!( + merged( + vec![sequence(&[3333, 23456])], + vec![set(&[210_036, 210_037])] + ), + [sequence(&[3333]), set(&[210_036, 210_037])] + ); + } + /// 2001:db8:1::/48 via 2001:db8::1. fn mp_reach_v6() -> Vec { let mut value = vec![0, 2, 1, 16]; diff --git a/crates/rib/src/table/attrs.rs b/crates/rib/src/table/attrs.rs index 63f70cf..faceea0 100644 --- a/crates/rib/src/table/attrs.rs +++ b/crates/rib/src/table/attrs.rs @@ -59,10 +59,6 @@ fn scan(mut wire: &[u8]) -> Option { origin: 2, origin_as: 0, }; - // The origin as AS4_PATH has it, if there is one: on a session with - // two-octet AS numbers the path's own last AS may be a stand-in - // (RFC 6793). - let mut wide_origin = None; let number = |value: &[u8]| Some(u32::from_be_bytes(<[u8; 4]>::try_from(value).ok()?)); while !wire.is_empty() { let (attribute, rest) = split_attribute(wire).ok()?; @@ -76,15 +72,11 @@ fn scan(mut wire: &[u8]) -> Option { key.neighbor_as = path.first; key.origin_as = path.last; } - attr_type::AS4_PATH => wide_origin = Some(scan_as_path(value)?.last), attr_type::MULTI_EXIT_DISC => key.med = Some(number(value)?), attr_type::LOCAL_PREF => key.local_pref = Some(number(value)?), _ => {} } } - if let Some(origin) = wide_origin { - key.origin_as = origin; - } Some(key) } @@ -198,35 +190,20 @@ impl AttrSet { /// origin's last, sets and all. Returns whether the path has a set in /// it. /// - /// This is the path as it really is: where AS4_PATH has the numbers - /// that a two-octet session could not carry, they are in their places + /// This is the whole path: what a session with two-octet AS numbers + /// brought in AS4_PATH was put in its place when the UPDATE was read /// (RFC 6793 §4.2.3). It reads the stored bytes and builds nothing /// else, for those who ask it of every route. pub fn as_path(&self, out: &mut Vec) -> bool { out.clear(); - let (mut path, mut wide): (&[u8], &[u8]) = (&[], &[]); let mut wire = &*self.wire; while let Ok((attribute, rest)) = split_attribute(wire) { - match attribute.type_code { - attr_type::AS_PATH => path = attribute.value, - attr_type::AS4_PATH => wide = attribute.value, - _ => {} + if attribute.type_code == attr_type::AS_PATH { + return read_as_path(attribute.value, out); } wire = rest; } - let mut has_set = read_as_path(path, out); - let narrow = out.len(); - has_set |= read_as_path(wide, out); - let wide = out.len() - narrow; - if wide <= narrow { - // AS4_PATH is the path's tail; what comes before it is - // AS_PATH's. - out.drain(narrow - wide..narrow); - } else { - // An AS4_PATH longer than the path is not used. - out.truncate(narrow); - } - has_set + false } /// The attributes, parsed, and the next hop if it came in @@ -569,29 +546,11 @@ mod tests { assert!(!set.as_path(&mut path)); assert_eq!(path, [3333, 3333, 1299]); - // A two-octet path whose tail AS4_PATH has in full. + // A set is said to be there. let sequence = |path: Vec| AsPathSegment { ordered: true, path, }; - let mut narrow = attributes(vec![3333, 23456, 23456]); - narrow.push(PathAttribute::As4Path(AsPathAttribute { - segments: vec![sequence(vec![210036, 4_200_000_001])], - })); - let set = store.intern(&narrow, None).unwrap(); - assert!(!set.as_path(&mut path)); - assert_eq!(path, [3333, 210036, 4_200_000_001]); - - // An AS4_PATH longer than the path is not used. - let mut too_wide = attributes(vec![3333]); - too_wide.push(PathAttribute::As4Path(AsPathAttribute { - segments: vec![sequence(vec![1, 2])], - })); - let set = store.intern(&too_wide, None).unwrap(); - assert!(!set.as_path(&mut path)); - assert_eq!(path, [3333]); - - // A set is said to be there. let mut with_set = attributes(vec![]); with_set[1] = PathAttribute::AsPath(AsPathAttribute { segments: vec![ diff --git a/crates/rib/src/table/mod.rs b/crates/rib/src/table/mod.rs index 57394f2..68c1efc 100644 --- a/crates/rib/src/table/mod.rs +++ b/crates/rib/src/table/mod.rs @@ -625,12 +625,19 @@ fn write(shard: &RwLock) -> RwLockWriteGuard<'_, Shard> { /// /// That table takes a key's slot from the low bits of the hash and, from /// the top seven, the tag it compares before it looks at the key itself -/// (hashbrown). The bucket is taken from bits in between: taken from -/// either end, the keys of one bucket would all share those bits, and the -/// table inside would lose them. +/// (hashbrown). Taken from either end, the keys of one bucket would all +/// share those bits, and the table inside would lose them. Nor is it +/// enough to take the bits in between: the hash is one multiplication, +/// which for keys that are numbers in a row (prefixes are) can leave +/// neighbouring bits alike. So the hash is stirred once more, and the +/// bucket taken from the top of that, which every bit of the hash has a +/// part in. fn spread(hash: u64, count: usize) -> usize { - let middle = (hash << 7) >> 32; - ((middle * count as u64) >> 32) as usize + // 2^64 divided by the golden ratio: the multiplier of Fibonacci + // hashing. + const STIR: u64 = 0x9E37_79B9_7F4A_7C15; + let stirred = hash.wrapping_mul(STIR) >> 32; + ((stirred * count as u64) >> 32) as usize } /// Adds to a vector without the doubling that `push` does: a row of a few diff --git a/crates/rib/src/table/tests.rs b/crates/rib/src/table/tests.rs index 5fa3fb5..e4b612e 100644 --- a/crates/rib/src/table/tests.rs +++ b/crates/rib/src/table/tests.rs @@ -699,12 +699,35 @@ proptest! { /// differ where that table looks. #[test] fn keys_of_one_shard_differ_in_the_bits_their_table_uses() { - let hasher = RandomState::default(); + // With keys of the hash that are the same every time: a test that + // fails one run in a thousand tells nobody anything. + for seed in 0..32u64 { + keys_of_one_shard_differ(foldhash::fast::FixedState::with_seed(seed)); + } + // Always one of the buckets there are. + for hash in [0, 1, u64::MAX, u64::MAX / 2, 1 << 63] { + for count in [1, 3, 64, 1000] { + assert!(spread(hash, count) < count, "{hash} of {count}"); + } + } +} + +fn keys_of_one_shard_differ(hasher: impl std::hash::BuildHasher) { let shards = 64; let mut counts = vec![0usize; shards]; let mut tags = vec![std::collections::HashSet::new(); shards]; + // Keys as the table has them, and as regular as they come: every /24 + // of a /8. for n in 0..64_000u32 { - let hash = hasher.hash_one(n); + let [_, a, b, c] = n.to_be_bytes(); + let key: Nlri = IpPrefix::new( + bgp_primitives::constants::IpVersion::Ipv4, + [100 + a, b, c, 0], + 24, + ) + .unwrap() + .into(); + let hash = hasher.hash_one(key); let shard = spread(hash, shards); counts[shard] += 1; // The seven bits hashbrown keeps as a tag. @@ -724,8 +747,6 @@ fn keys_of_one_shard_differ_in_the_bits_their_table_uses() { tags[shard].len() ); } - assert_eq!(spread(u64::MAX, 3), 2); - assert_eq!(spread(0, 3), 0); } /// A peer that sends several routes for a prefix can send more than fit a diff --git a/crates/rpki/src/client.rs b/crates/rpki/src/client.rs index ff23d06..ae6d48a 100644 --- a/crates/rpki/src/client.rs +++ b/crates/rpki/src/client.rs @@ -42,6 +42,15 @@ const CONNECT_TIMEOUT: Duration = Duration::from_secs(10); /// How long a cache may take to send the next part of an answer. const RESPONSE_TIMEOUT: Duration = Duration::from_secs(60); +/// How long a whole answer may take. Everything a validator has takes it +/// seconds to send. +const ANSWER_TIMEOUT: Duration = Duration::from_secs(600); + +/// The most records taken in one answer: several times what the whole +/// RPKI holds. A cache that sends more is not sending the RPKI, and +/// would otherwise be listened to until there is no memory left. +const MAX_RECORDS: usize = 8_000_000; + /// How long to wait before trying the next cache after one has failed. const NEXT_CACHE_DELAY: Duration = Duration::from_secs(1); @@ -380,10 +389,17 @@ impl Client { .unwrap_or(Duration::from_secs(self.timers.retry.clamp(1, 7200).into())) } + /// Never sooner than a refresh that failed and was tried once more + /// would take, whatever the cache or the configuration says: data that + /// ran out between two refreshes would leave nothing validated for a + /// part of every round (RFC 8210 §6 wants it longer than either). fn expire(&self) -> Duration { - self.config.expire.unwrap_or(Duration::from_secs( - self.timers.expire.clamp(600, 172_800).into(), - )) + self.config + .expire + .unwrap_or(Duration::from_secs( + self.timers.expire.clamp(600, 172_800).into(), + )) + .max(self.refresh() + self.retry()) } async fn run(mut self) { @@ -560,9 +576,15 @@ impl Client { connection: &mut Connection, since: Option, ) -> Result { - let mut next = async || match timeout(RESPONSE_TIMEOUT, connection.next()).await { - Ok(pdu) => pdu, - Err(_) => Err(SessionError::Timeout), + // Each PDU in good time, and all of them too: a cache that sends + // one a minute would keep the old data in use for ever. + let deadline = Instant::now() + ANSWER_TIMEOUT; + let mut next = async || { + let wait = RESPONSE_TIMEOUT.min(deadline.saturating_duration_since(Instant::now())); + match timeout(wait, connection.next()).await { + Ok(pdu) => pdu, + Err(_) => Err(SessionError::Timeout), + } }; // The answer begins with a Cache Response, or is a Cache Reset. @@ -611,6 +633,9 @@ impl Client { if version != self.protocol_version && !matches!(pdu, Pdu::ErrorReport { .. }) { return Err(SessionError::Inconsistent(OTHER_VERSION)); } + if records.len() >= MAX_RECORDS { + return Err(SessionError::Unexpected("more data than any cache has")); + } match pdu { Pdu::Prefix { announce, vrp } => records.push(Record::Vrp { announce, vrp }), Pdu::Aspa { diff --git a/crates/rpki/tests/client.rs b/crates/rpki/tests/client.rs index 0b48bea..21f5062 100644 --- a/crates/rpki/tests/client.rs +++ b/crates/rpki/tests/client.rs @@ -656,9 +656,10 @@ async fn data_that_is_not_confirmed_in_time_is_dropped() { let cache = Cache::start().await; let (rpki, _task) = Rpki::spawn(RpkiConfig { caches: vec![cache.address()], + // Data never expires sooner than a refresh and a retry take. + refresh: Some(Duration::from_millis(200)), retry: Some(Duration::from_millis(50)), expire: Some(Duration::from_millis(300)), - ..RpkiConfig::default() }); let mut updates = rpki.subscribe(); let mut connection = cache.accept().await; diff --git a/crates/server/proto/config.proto b/crates/server/proto/config.proto index 21cff60..f845843 100644 --- a/crates/server/proto/config.proto +++ b/crates/server/proto/config.proto @@ -71,7 +71,7 @@ message Kernel { // The metric of the routes. Defaults to 20. Of several routes for a // prefix the kernel uses the one with the lowest metric. - uint32 metric = 2; + optional uint32 metric = 2; // By default, where the kernel has nexthop objects (Linux 5.3 and // later), a gateway is put there once as such an object and routes name @@ -86,7 +86,8 @@ message Kernel { // have sent theirs; removing the old routes at once would stop the // machine forwarding for as long as that takes. After this time, what // was not announced again is removed. - uint32 stale_seconds = 4; + // Zero removes them at once. + optional uint32 stale_seconds = 4; } // Where to get the validated ROA payloads that say which AS may originate @@ -160,8 +161,9 @@ message Peer { uint32 remote_asn = 3; // Hold time to advertise in the OPEN message, in seconds (RFC 4271 §4.2). - // Must be 0 or >= 3. Defaults to 90 when unset. - uint32 hold_time_seconds = 4; + // At least 3, or 0 for a session without keepalives that is never timed + // out. Defaults to 90 when unset. + optional uint32 hold_time_seconds = 4; // Source IP address for connections we open to this peer. It is also the // next hop of the routes we send it. Required when the peer identifies us @@ -282,6 +284,18 @@ message ImportPolicy { // a route whose AS path they find invalid is refused. True takes it all // the same. bool accept_aspa_invalid = 12; + + // By default a route whose AS path has an AS_SET in it is refused: sets + // are deprecated (RFC 9774), and with one nobody can say where a route + // comes from. True takes such routes. + bool accept_as_sets = 13; + + // By default the next hop of a route from another network has to be the + // peer itself, so that a peer can only draw traffic to itself and not + // send it to a third party. True takes any next hop packets can be sent + // to. Not needed for a peer whose role is ROUTE_SERVER, whose routes + // have its clients as next hops. + bool accept_third_party_next_hop = 14; } // A prefix, and optionally the more specific prefixes inside it. diff --git a/crates/server/src/advertise.rs b/crates/server/src/advertise.rs index 0be473e..248424a 100644 --- a/crates/server/src/advertise.rs +++ b/crates/server/src/advertise.rs @@ -89,7 +89,7 @@ impl Advertiser { } Ok(SessionEvent::RefreshRequested(session)) => { if let Some(exporter) = self.exporters.get(&session.peer) { - info!(peer = %session.peer, "Route refresh requested, re-sending local routes"); + debug!(peer = %session.peer, "Route refresh requested, re-sending local routes"); exporter.resend.notify_one(); } } diff --git a/crates/server/src/collision.rs b/crates/server/src/collision.rs index a15d0d8..cecb29c 100644 --- a/crates/server/src/collision.rs +++ b/crates/server/src/collision.rs @@ -103,7 +103,9 @@ impl CollisionTable { } } - /// Call when a connection receives the peer's OPEN. `None` means this + /// Call when a connection receives the peer's OPEN. `one_identifier` + /// says that the address is that of one router, which holds for a + /// configured peer. `None` means this /// connection lost and must be closed with Cease / Connection Collision /// Resolution. Otherwise any rival has been told to close and this /// connection holds the slot until the guard drops. @@ -113,9 +115,21 @@ impl CollisionTable { remote_addr: PeerAddress, remote_id: Ipv4Addr, direction: Direction, + one_identifier: bool, ) -> Option { let peer = (remote_addr, remote_id); let mut entries = self.entries.locked(); + // A configured peer is one router, with one identifier. A second + // connection from its address under another would be a second + // session, with a second allowance of routes. + if one_identifier + && entries + .keys() + .any(|(address, id)| *address == remote_addr && *id != remote_id) + { + COLLISIONS.with_label_values(&["new"]).inc(); + return None; + } if let Some(existing) = entries.get_mut(&peer) { if new_loses( local_id, @@ -266,24 +280,32 @@ mod tests { let table = CollisionTable::new(false); let addr = PeerAddress::from(std::net::IpAddr::from(Ipv4Addr::new(192, 0, 2, 1))); // We are LOW: outbound first, then the peer's inbound wins. - let mut out = table.admit(LOW, addr, HIGH, Outbound).unwrap(); - let inb = table.admit(LOW, addr, HIGH, Inbound).unwrap(); + let mut out = table.admit(LOW, addr, HIGH, Outbound, false).unwrap(); + let inb = table.admit(LOW, addr, HIGH, Inbound, false).unwrap(); assert!(out.killed.try_recv().is_ok()); // The loser's guard dropping must not free the winner's slot. drop(out); inb.set_established(); - assert!(table.admit(LOW, addr, HIGH, Outbound).is_none()); + assert!(table.admit(LOW, addr, HIGH, Outbound, false).is_none()); // A different identifier never collides. assert!( table - .admit(LOW, addr, Ipv4Addr::new(192, 0, 2, 2), Inbound) + .admit(LOW, addr, Ipv4Addr::new(192, 0, 2, 2), Inbound, false) .is_some() ); + // Unless the address is one router's, which has one identifier: a + // second would be a second session, with its own limit of routes. + assert!( + table + .admit(LOW, addr, Ipv4Addr::new(192, 0, 2, 3), Inbound, true) + .is_none() + ); + // Once the survivor goes away the identifier is free again. drop(inb); - assert!(table.admit(LOW, addr, HIGH, Outbound).is_some()); + assert!(table.admit(LOW, addr, HIGH, Outbound, false).is_some()); } /// A neighbour reached over IPv4 and over IPv6 announces the same @@ -296,9 +318,9 @@ mod tests { let v4 = PeerAddress::from(std::net::IpAddr::from(Ipv4Addr::new(192, 0, 2, 1))); let v6 = PeerAddress::from("2001:db8::1".parse::().unwrap()); - let mut over_v4 = table.admit(LOW, v4, HIGH, Outbound).unwrap(); + let mut over_v4 = table.admit(LOW, v4, HIGH, Outbound, false).unwrap(); over_v4.set_established(); - let over_v6 = table.admit(LOW, v6, HIGH, Inbound); + let over_v6 = table.admit(LOW, v6, HIGH, Inbound, false); assert!(over_v6.is_some()); assert!(over_v4.killed.try_recv().is_err(), "the first is untouched"); } diff --git a/crates/server/src/config.rs b/crates/server/src/config.rs index 7477c06..bc734d4 100644 --- a/crates/server/src/config.rs +++ b/crates/server/src/config.rs @@ -1,5 +1,6 @@ use std::net::{IpAddr, Ipv4Addr, SocketAddr}; use std::str::FromStr; +use std::sync::Arc; use std::time::Duration; use bgp_fib::FibConfig; @@ -150,6 +151,17 @@ impl Config { } originate.push(route); } + // What we originate is not taken from other networks. + let mut own_prefixes = PrefixSet::default(); + for route in &originate { + own_prefixes.allow(route.prefix, route.prefix.address_family().bits()); + } + let own_prefixes = Arc::new(own_prefixes); + for peer in &mut peers { + if peer.remote_asn != proto.local_asn { + peer.import.own_prefixes = Arc::clone(&own_prefixes); + } + } let http_endpoint = match proto.http_endpoint.into_option() { Some(ep) if ep.port == 0 => return Err(ConfigError::HttpPort), @@ -179,24 +191,28 @@ impl Config { accept_unconfigured_peers: proto.accept_unconfigured_peers, originate, http_endpoint, - kernel: proto.kernel.as_ref().map(|kernel| { - let defaults = FibConfig::default(); - let or = |value: u32, default: u32| if value == 0 { default } else { value }; - FibConfig { - table: or(kernel.table, defaults.table), - metric: or(kernel.metric, defaults.metric), - nexthop_objects: !kernel.without_nexthop_objects, - stale_time: match kernel.stale_seconds { - 0 => defaults.stale_time, - seconds => Duration::from_secs(seconds.into()), - }, - } - }), + kernel: proto.kernel.as_ref().map(parse_kernel), rpki, }) } } +fn parse_kernel(kernel: &proto::Kernel) -> FibConfig { + let defaults = FibConfig::default(); + FibConfig { + // The kernel has no table 0 to put routes in. + table: match kernel.table { + 0 => defaults.table, + table => table, + }, + metric: kernel.metric.unwrap_or(defaults.metric), + nexthop_objects: !kernel.without_nexthop_objects, + stale_time: kernel.stale_seconds.map_or(defaults.stale_time, |seconds| { + Duration::from_secs(seconds.into()) + }), + } +} + fn parse_peer(peer: Peer) -> Result { let ep = peer.endpoint.as_ref().ok_or(PeerProblem::MissingEndpoint)?; let endpoint = parse_port(ep.port) @@ -208,10 +224,10 @@ fn parse_peer(peer: Peer) -> Result { } let hold_time_secs = match peer.hold_time_seconds { - 0 => DEFAULT_HOLD_TIME_SECS, + None => DEFAULT_HOLD_TIME_SECS, // RFC 4271 §4.2: zero, or at least three seconds. - 1 | 2 => return Err(PeerProblem::HoldTime), - secs => u16::try_from(secs).map_err(|_| PeerProblem::HoldTime)?, + Some(1 | 2) => return Err(PeerProblem::HoldTime), + Some(secs) => u16::try_from(secs).map_err(|_| PeerProblem::HoldTime)?, }; let local_address = if peer.local_address.is_empty() { @@ -367,6 +383,9 @@ fn parse_import(import: &proto::ImportPolicy) -> Result defaults.max_as_path_length, length => u16::try_from(length).map_err(|_| ImportProblem::PathLength(length))?, diff --git a/crates/server/src/connection.rs b/crates/server/src/connection.rs index abda53a..ca5f259 100644 --- a/crates/server/src/connection.rs +++ b/crates/server/src/connection.rs @@ -19,7 +19,7 @@ use bgp_packet::message::{ BGP_HEADER_LEN, BGP_MAX_MSG_LEN, Decoded, Message, PathAttribute, UpdateMessage, }; use bgp_packet::open::AddPathSendRecv; -use bgp_packet::update_decode::UpdateError; +use bgp_packet::update_decode::{UpdateError, merge_as4_path}; use bgp_primitives::ip_prefix::Nlri; use bgp_primitives::parser::ParserContext; use bgp_rib::table::{Outcome, PeerIndex, PeerInfo, RibTable}; @@ -44,11 +44,12 @@ use crate::metrics::{ SESSIONS_ESTABLISHED_TOTAL, UPDATE_ERRORS, message_type_name, }; use crate::panic::contained; -use crate::policy::{self, HoldDowns, ImportPolicy, PeerRole}; +use crate::policy::{self, HeldDown, HoldDowns, ImportPolicy, PeerRole}; use crate::refusals::{PeerRefusals, RefusalLog}; use crate::routes::{self, Held}; use crate::rpki::{Checks, Subject, Validator}; use crate::session::{PeerId, SessionCounters, SessionGuard, SessionHandle, SessionRegistry}; +use crate::throttle::Throttle; /// Number of outbound messages that may be queued for a session before /// senders are back-pressured. @@ -172,17 +173,32 @@ pub async fn run_session( Err(panic) => Err(session.close_after_panic(panic).await), }; let established = session.established.is_some(); + let hold_downs = &session.server.hold_downs; if matches!(result, Err(SessionError::PrefixLimitExceeded { .. })) { // A peer that sends far more than it should will do so again on // the next session; it is left alone for a while. - session - .server - .hold_downs - .hold(peer_addr.into(), session.import.hold_down); + hold_downs.hold( + peer_addr.into(), + session.import.hold_down, + HeldDown::OverLimit, + ); PREFIX_LIMIT_EXCEEDED .with_label_values(&[session.refusals.label()]) .inc(); } + // A configured peer whose sessions keep coming up and failing is + // slowed down. Not one that is nobody in particular: such peers are + // only taken on a test network, where many share an address. + if let (Some(up), Some(_)) = (&session.established, &session.peer_name) + && let Some(hold) = + hold_downs.session_ended(peer_addr.into(), up.since.elapsed(), result.is_err()) + { + warn!( + peer_addr = %socket_text(peer_addr), + hold_down = ?hold, + "The peer's sessions keep failing soon after they come up; keeping it down for a while" + ); + } // Whatever ended the session, routes learned over it are gone // (RFC 4271 §8.2.2). Do this before the registration drops so that the @@ -213,6 +229,8 @@ pub async fn run_session( /// What exists only once a session is Established. struct Established { + /// When it was. + since: Instant, peer_id: PeerId, /// The peer's number in the routing table. rib_peer: PeerIndex, @@ -285,13 +303,21 @@ impl PeerRoutes { /// Removes the routes and says what that did. async fn remove(mut self) -> Outcome { - self.removed = true; let (table, peer) = (Arc::clone(&self.table), self.peer); // Looks at every prefix, which for a full table is a tenth of a // second: too long to hold up the other sessions on this thread. - tokio::task::spawn_blocking(move || Self::remove_now(&table, peer)) - .await - .unwrap_or_default() + match tokio::task::spawn_blocking(move || Self::remove_now(&table, peer)).await { + Ok(outcome) => { + self.removed = true; + outcome + } + // Not removed, then, or not all of them: dropping this tries + // once more. + Err(e) => { + error!(err = %e, "Removing a peer's routes failed; trying again"); + Outcome::default() + } + } } fn remove_now(table: &RibTable, peer: PeerIndex) -> Outcome { @@ -344,7 +370,28 @@ enum Wake { Revalidate(Vec), } +/// What a session says in the log only once in a while, because the peer +/// decides how often it happens. +struct Said { + update_errors: Throttle, + routes_ignored: Throttle, + refreshes: Throttle, +} + +impl Said { + const INTERVAL: Duration = Duration::from_secs(60); + + fn new() -> Self { + Self { + update_errors: Throttle::every(Self::INTERVAL), + routes_ignored: Throttle::every(Self::INTERVAL), + refreshes: Throttle::every(Self::INTERVAL), + } + } +} + struct Session { + said: Said, fsm: Fsm, frames: FrameReader, writer: OwnedWriteHalf, @@ -432,6 +479,7 @@ impl Session { peer_name: peer.name, import: peer.import, role: peer.role, + said: Said::new(), }; Ok((session, first_actions)) } @@ -487,8 +535,15 @@ impl Session { async fn next_event(&mut self) -> Result { let event = tokio::select! { frame = self.frames.next(&self.ctx) => match frame { - Ok(decoded) => { + Ok(mut decoded) => { self.report(&decoded.update_errors); + // From here on a route has one path, whatever the + // session brought it as. + if !self.ctx.four_octet_asn + && let Message::Update(update) = &mut decoded.message + { + merge_as4_path(&mut update.path_attributes); + } Event::Message(Received { message: decoded.message, attributes_wire: decoded.attributes_wire, @@ -509,17 +564,23 @@ impl Session { /// peer's fault, or that of a router behind it, and worth an operator's /// attention: routes were withdrawn or attributes dropped because of /// them. - fn report(&self, update_errors: &[UpdateError]) { + /// + /// Counted every time, said once in a while: one bad attribute is on + /// every route that came the same way, a table's worth of them. + fn report(&mut self, update_errors: &[UpdateError]) { for error in update_errors { UPDATE_ERRORS .with_label_values(&[error.handling.as_str()]) .inc(); - warn!( - peer_addr = %socket_text(self.peer_addr), - handling = error.handling.as_str(), - detail = %error.detail, - "Error in received UPDATE" - ); + if let Some(unsaid) = self.said.update_errors.pass() { + warn!( + peer_addr = %socket_text(self.peer_addr), + handling = error.handling.as_str(), + detail = %error.detail, + others_since_last_report = unsaid, + "Error in received UPDATE" + ); + } } } @@ -577,11 +638,14 @@ impl Session { } } Action::RoutesIgnored(count) => { - warn!( - peer_addr = %socket_text(self.peer_addr), - routes = count, - "Ignored routes of an address family that was not negotiated" - ); + if let Some(unsaid) = self.said.routes_ignored.pass() { + warn!( + peer_addr = %socket_text(self.peer_addr), + routes = count, + others_since_last_report = unsaid, + "Ignored routes of an address family that was not negotiated" + ); + } } Action::DeliverUpdate { update, @@ -606,7 +670,13 @@ impl Session { } Action::RouteRefreshRequested => { if let Some(established) = &self.established { - info!(peer = %established.peer_id, "Received ROUTE-REFRESH, re-sending our routes"); + if let Some(unsaid) = self.said.refreshes.pass() { + info!( + peer = %established.peer_id, + others_since_last_report = unsaid, + "Received ROUTE-REFRESH, re-sending our routes" + ); + } self.server.registry.request_refresh(&established.peer_id); } } @@ -636,6 +706,7 @@ impl Session { self.peer_addr.into(), negotiated.peer_identifier, self.direction, + self.peer_name.is_some(), ); if self.collision.is_none() { info!(peer_addr = %socket_text(self.peer_addr), "Closing connection: lost collision resolution"); @@ -706,6 +777,7 @@ impl Session { changes: handle.validate(subject), }); self.established = Some(Established { + since: Instant::now(), validation, held: Held::default(), refreshable: negotiated @@ -721,6 +793,9 @@ impl Session { ibgp: negotiated.ibgp, four_octet_asn: negotiated.four_octet_asn, role: self.role, + peer_address: self.peer_addr.ip(), + local_address: self.local_addr.ip(), + extended_next_hop: negotiated.extended_next_hop, }, counters: Arc::clone(&handle.counters), routes_metric: PEER_ROUTES.with_label_values(&[self.refusals.label()]), @@ -1188,7 +1263,9 @@ mod tests { .unwrap(); let update = UpdateMessage { withdrawn_routes: vec![], - path_attributes: vec![], + path_attributes: vec![PathAttribute::NextHop( + bgp_packet::message::NextHopPathAttribute(Ipv4Addr::new(192, 0, 2, 1)), + )], announced_routes: vec![ bgp_primitives::ip_prefix::IpPrefix::try_from("10.0.0.0/8") .unwrap() @@ -1203,6 +1280,9 @@ mod tests { ibgp: false, four_octet_asn: true, role: None, + peer_address: Ipv4Addr::new(192, 0, 2, 1).into(), + local_address: Ipv4Addr::new(192, 0, 2, 9).into(), + extended_next_hop: false, }, policy: &ImportPolicy::permissive(), checks: Checks::NONE, diff --git a/crates/server/src/dialer.rs b/crates/server/src/dialer.rs index f11b2b5..47605b2 100644 --- a/crates/server/src/dialer.rs +++ b/crates/server/src/dialer.rs @@ -8,6 +8,7 @@ //! connection is the one closed, the next pass finds the surviving session //! and goes idle. +use std::hash::{BuildHasher, Hasher, RandomState}; use std::sync::Arc; use std::time::{Duration, SystemTime}; @@ -55,6 +56,14 @@ async fn connect(peer: &PeerConfig) -> std::io::Result { socket.connect(endpoint).await } +/// `wait`, less up to a quarter of it, by chance (RFC 4271 §10). +fn jittered(wait: Duration) -> Duration { + // The standard library's source of chance, which is what keys its + // hash tables: good enough to tell dialers apart. + let chance = RandomState::new().build_hasher().finish(); + wait.mul_f64(0.75 + 0.25 * (chance as f64 / u64::MAX as f64)) +} + async fn dial_loop(peer: PeerConfig, server: Arc, connect_retry: Duration) { let registry = &server.registry; let mut backoff = connect_retry; @@ -72,12 +81,14 @@ async fn dial_loop(peer: PeerConfig, server: Arc, connect_retry: continue; } - if let Some(remaining) = address.and_then(|address| server.hold_downs.remaining(address)) { + if let Some((remaining, why)) = + address.and_then(|address| server.hold_downs.remaining(address)) + { registry.set_dial_state( &peer.name, DialState::Backoff { until: SystemTime::now() + remaining, - last_error: "held down after going over its limit of routes".into(), + last_error: why.as_str().into(), }, ); tokio::time::sleep(remaining).await; @@ -124,14 +135,16 @@ async fn dial_loop(peer: PeerConfig, server: Arc, connect_retry: } }; + // Not all at the same moment, when many peers went away together. + let wait = jittered(backoff); registry.set_dial_state( &peer.name, DialState::Backoff { - until: SystemTime::now() + backoff, + until: SystemTime::now() + wait, last_error: reason, }, ); - tokio::time::sleep(backoff).await; + tokio::time::sleep(wait).await; backoff = (backoff * 2).min(MAX_BACKOFF); } } diff --git a/crates/server/src/lib.rs b/crates/server/src/lib.rs index 4782bc4..58703ad 100644 --- a/crates/server/src/lib.rs +++ b/crates/server/src/lib.rs @@ -16,6 +16,7 @@ pub mod rpki; pub mod server; pub mod session; mod sync; +mod throttle; pub mod tracing; pub mod proto { diff --git a/crates/server/src/policy.rs b/crates/server/src/policy.rs index 3787f8a..6203944 100644 --- a/crates/server/src/policy.rs +++ b/crates/server/src/policy.rs @@ -25,10 +25,11 @@ //! replaced it, and the replacement is not acceptable. use std::collections::{HashMap, HashSet}; -use std::sync::{LazyLock, Mutex}; +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; +use std::sync::{Arc, LazyLock, Mutex}; use std::time::{Duration, Instant}; -use bgp_packet::message::{AS_TRANS, AsPathAttribute, PathAttribute}; +use bgp_packet::message::{AS_TRANS, AsPathAttribute, NlriNextHop, PathAttribute}; use bgp_packet::open::BgpRole; use bgp_primitives::constants::IpVersion; use bgp_primitives::ip_prefix::IpPrefix; @@ -81,6 +82,20 @@ pub struct ImportPolicy { pub accept_rpki_invalid: bool, /// Take routes whose path the ASPAs find invalid. pub accept_aspa_invalid: bool, + /// Take routes whose path has an AS_SET in it. They are deprecated + /// (RFC 9774): with one, nobody can say where a route comes from. + pub accept_as_sets: bool, + /// Take routes whose next hop is not the peer itself. Without this a + /// peer can only draw our traffic to itself, not send it to someone + /// who never asked for it. A route server's routes always have its + /// clients as next hops, and are taken either way. + pub accept_third_party_next_hop: bool, + /// The prefixes this router originates. Those, and what is inside + /// them, are ours and not taken from another network, unless + /// `prefixes` says in so many words that this peer may announce them: + /// a customer with a part of our space. Empty for a peer in our own + /// AS. + pub own_prefixes: Arc, } impl Default for ImportPolicy { @@ -100,6 +115,9 @@ impl Default for ImportPolicy { origin_asns: None, accept_rpki_invalid: false, accept_aspa_invalid: false, + accept_as_sets: false, + accept_third_party_next_hop: false, + own_prefixes: Arc::default(), } } } @@ -118,6 +136,8 @@ impl ImportPolicy { max_as_path_length: u16::MAX, accept_rpki_invalid: true, accept_aspa_invalid: true, + accept_as_sets: true, + accept_third_party_next_hop: true, ..Self::default() } } @@ -155,10 +175,20 @@ pub enum Rejection { /// customers, and it did not come from above: someone has leaked it /// (RFC 9234). Leak, + /// The path has an AS_SET in it (RFC 9774). + AsSet, + /// The next hop is no address packets could be sent to, or is our + /// own, or is an IPv6 address for an IPv4 route on a session that did + /// not agree to that (RFC 8950). + NextHopUnusable, + /// The next hop is not the peer. + NextHopNotPeer, + /// The prefix is one of ours, or inside one. + OwnPrefix, } impl Rejection { - pub const ALL: [Self; 12] = [ + pub const ALL: [Self; 16] = [ Self::BogonPrefix, Self::TooShort, Self::TooSpecific, @@ -171,6 +201,10 @@ impl Rejection { Self::RpkiInvalid, Self::AspaInvalid, Self::Leak, + Self::AsSet, + Self::NextHopUnusable, + Self::NextHopNotPeer, + Self::OwnPrefix, ]; /// Where the reason is in [`Rejection::ALL`]. @@ -192,6 +226,10 @@ impl Rejection { Self::RpkiInvalid => "RPKI invalid", Self::AspaInvalid => "ASPA invalid", Self::Leak => "route leak (Only to Customer)", + Self::AsSet => "AS_SET in path", + Self::NextHopUnusable => "next hop unusable", + Self::NextHopNotPeer => "next hop is not the peer", + Self::OwnPrefix => "prefix is our own", } } } @@ -291,6 +329,11 @@ pub struct From { pub four_octet_asn: bool, /// What the peer is to us, if that is known. pub role: Option, + /// The two ends of the session. + pub peer_address: IpAddr, + pub local_address: IpAddr, + /// Whether IPv4 routes may come with IPv6 next hops (RFC 8950). + pub extended_next_hop: bool, } impl ImportPolicy { @@ -306,15 +349,10 @@ impl ImportPolicy { PathAttribute::AsPath(path) => Some(path), _ => None, }); - let as4_path = attributes.iter().find_map(|attribute| match attribute { - PathAttribute::As4Path(path) => Some(path), - _ => None, - }); - let facts = PathFacts::of(path, as4_path, from); + let facts = PathFacts::of(path, from); // The origin is the last AS of the path. If the path ends in a // set, there is no telling which of its members that is. - let ends_in_sequence = as4_path - .or(path) + let ends_in_sequence = path .and_then(|path| path.segments.last()) .is_some_and(|segment| segment.ordered); let origin = facts.last.filter(|_| ends_in_sequence); @@ -357,6 +395,9 @@ impl ImportPolicy { if facts.length > usize::from(self.max_as_path_length) { return Err(Rejection::PathTooLong); } + if !self.accept_as_sets && facts.has_set { + return Err(Rejection::AsSet); + } if !self.accept_bogons && facts.has_bogon_asn { return Err(Rejection::BogonAs); } @@ -389,10 +430,71 @@ impl ImportPolicy { return Err(Rejection::BogonPrefix); } } - if let Some(allowed) = &self.prefixes - && !allowed.allows(prefix) - { - return Err(Rejection::PrefixNotAllowed); + match &self.prefixes { + Some(allowed) if !allowed.allows(prefix) => Err(Rejection::PrefixNotAllowed), + // Said to be this peer's to announce, ours or not. + Some(_) => Ok(()), + None if self.own_prefixes.allows(prefix) => Err(Rejection::OwnPrefix), + None => Ok(()), + } + } + + /// Judges the next hop of routes of IP version `routes`. + /// + /// It has to be an address packets can be sent to and not our own + /// (RFC 4271 §5.1.3), and of a kind the session agreed on. From + /// another network it also has to be the peer itself, unless the + /// policy says otherwise: a next hop is where our traffic will go, + /// and a peer is not to send it to a third party, whether another + /// member of an exchange or a machine on some other network of ours. + pub fn check_next_hop( + &self, + next_hop: &NlriNextHop, + routes: IpVersion, + from: &From, + ) -> Result<(), Rejection> { + let (first, link_local): (IpAddr, Option) = match *next_hop { + NlriNextHop::Ipv4(address) => (address.into(), None), + NlriNextHop::Ipv6(address) => (address.into(), None), + NlriNextHop::Ipv6WithLl { global, link_local } => (global.into(), Some(link_local)), + }; + match (routes, first) { + (IpVersion::Ipv4, IpAddr::V4(_)) | (IpVersion::Ipv6, IpAddr::V6(_)) => {} + (IpVersion::Ipv4, IpAddr::V6(_)) if from.extended_next_hop => {} + _ => return Err(Rejection::NextHopUnusable), + } + // With a link-local address after it, the first may be nothing at + // all, or the same once more, from a peer that has no global one. + let filler = link_local + .is_some_and(|link_local| first.is_unspecified() || first == IpAddr::V6(link_local)); + let named = (!filler) + .then_some(first) + .into_iter() + .chain(link_local.map(IpAddr::V6)); + let mut is_peer = false; + for address in named { + let usable = !address.is_unspecified() + && !address.is_loopback() + && !address.is_multicast() + && address != IpAddr::V4(Ipv4Addr::BROADCAST) + && address != from.local_address; + if !usable { + return Err(Rejection::NextHopUnusable); + } + is_peer |= address == from.peer_address; + } + if link_local.is_some_and(|link_local| !link_local.is_unicast_link_local()) { + return Err(Rejection::NextHopUnusable); + } + // Whether the next hop is the peer can be told only if both are + // addresses of the same kind: IPv6 routes over an IPv4 session + // have a next hop the session knows nothing of. + let comparable = first.is_ipv4() == from.peer_address.is_ipv4(); + let third_party_allowed = from.ibgp + || self.accept_third_party_next_hop + || from.role == Some(PeerRole::RouteServer); + if comparable && !is_peer && !third_party_allowed { + return Err(Rejection::NextHopNotPeer); } Ok(()) } @@ -405,30 +507,24 @@ struct PathFacts { length: usize, has_local_asn: bool, has_bogon_asn: bool, + has_set: bool, last: Option, } impl PathFacts { - /// Reads the path as it really is. On a session with two-octet AS - /// numbers the path's tail is in AS4_PATH with the numbers that did - /// not fit (RFC 6793 §4.2.3), and that is what counts. - fn of(path: Option<&AsPathAttribute>, as4_path: Option<&AsPathAttribute>, from: &From) -> Self { - let length = asns(path).count(); - let wide = asns(as4_path).count(); - // A longer AS4_PATH than AS_PATH is not to be used (RFC 6793 - // §4.2.3). - let (kept, tail) = if wide <= length { - (length - wide, as4_path) - } else { - (length, None) - }; + /// Reads the path. It is the whole path: what a session with + /// two-octet AS numbers brought in AS4_PATH the decoder has put in its + /// place (RFC 6793 §4.2.3). + fn of(path: Option<&AsPathAttribute>, from: &From) -> Self { let mut facts = Self { - length, + length: 0, has_local_asn: false, has_bogon_asn: false, + has_set: path.is_some_and(|path| path.segments.iter().any(|segment| !segment.ordered)), last: None, }; - for asn in asns(path).take(kept).chain(asns(tail)) { + for asn in asns(path) { + facts.length += 1; facts.has_local_asn |= asn == from.local_asn; facts.has_bogon_asn |= is_bogon_asn(asn, from.four_octet_asn); facts.last = Some(asn); @@ -442,25 +538,13 @@ impl PathFacts { /// first, the origin's last, sets and all. Returns whether the path has a /// set in it. pub(crate) fn effective_path(attributes: &[PathAttribute], out: &mut Vec) -> bool { - let find = |wide: bool| { - attributes.iter().find_map(|attribute| match attribute { - PathAttribute::AsPath(path) if !wide => Some(path), - PathAttribute::As4Path(path) if wide => Some(path), - _ => None, - }) - }; - let (path, as4_path) = (find(false), find(true)); - let length = asns(path).count(); - let wide = asns(as4_path).count(); - let (kept, tail) = if wide <= length { - (length - wide, as4_path) - } else { - (length, None) - }; + let path = attributes.iter().find_map(|attribute| match attribute { + PathAttribute::AsPath(path) => Some(path), + _ => None, + }); out.clear(); - out.extend(asns(path).take(kept).chain(asns(tail))); + out.extend(asns(path)); path.into_iter() - .chain(tail) .flat_map(|path| &path.segments) .any(|segment| !segment.ordered) } @@ -508,6 +592,9 @@ static BOGONS: LazyLock> = LazyLock::new(|| { "240.0.0.0/4", // IPv6 inside the global unicast space. "2001::/32", + "2001:2::/48", + "2001:10::/28", + "2001:20::/28", "2001:db8::/32", "2002::/16", "3ffe::/16", @@ -609,28 +696,101 @@ impl PrefixSet { } } +/// Why a peer is being kept down. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum HeldDown { + /// It went over its limit of routes. + OverLimit, + /// Its sessions keep coming up and failing. + Flapping, +} + +impl HeldDown { + pub fn as_str(self) -> &'static str { + match self { + Self::OverLimit => "held down after going over its limit of routes", + Self::Flapping => "held down because its sessions keep failing", + } + } +} + +/// A session that fails sooner than this after coming up counts as a +/// flap. +const FLAP_WITHIN: Duration = Duration::from_secs(60); +/// This many flaps in a row are let pass: a router being restarted, a +/// cable put back. +const FLAPS_ALLOWED: u32 = 2; +/// How long a peer is kept down after the first flap too many; twice as +/// long after each one more, up to `FLAP_HOLD_MAX`. +const FLAP_HOLD: Duration = Duration::from_secs(1); +const FLAP_HOLD_MAX: Duration = Duration::from_secs(300); + /// Peers that are being kept down, and until when: those that went over -/// their limit. Shared by the sessions, which put peers here, and by the -/// dialer and the listeners, which leave them alone meanwhile. +/// their limit, and those whose sessions keep failing. Shared by the +/// sessions, which put peers here, and by the dialer and the listeners, +/// which leave them alone meanwhile. +/// +/// Every session that comes up and goes costs a look at every route in +/// the table, and tells every other peer of the changes. A peer that +/// does so over and over, by fault or by design, is slowed down. #[derive(Debug, Default)] -pub struct HoldDowns(Mutex>); +pub struct HoldDowns { + held: Mutex>, + /// How many times in a row each peer's session has failed soon after + /// coming up. + flaps: Mutex>, +} impl HoldDowns { /// Keeps the peer at `address` down for `duration` from now. - pub fn hold(&self, address: PeerAddress, duration: Duration) { - self.0.locked().insert(address, Instant::now() + duration); + pub fn hold(&self, address: PeerAddress, duration: Duration, why: HeldDown) { + let until = Instant::now() + duration; + let mut held = self.held.locked(); + // Not for less long than it already is. + if held + .get(&address) + .is_none_or(|(already, _)| *already < until) + { + held.insert(address, (until, why)); + } } - /// How much longer the peer at `address` is kept down, if it is. - pub fn remaining(&self, address: PeerAddress) -> Option { - let mut held = self.0.locked(); - let until = *held.get(&address)?; + /// How much longer the peer at `address` is kept down, if it is, and + /// why. + pub fn remaining(&self, address: PeerAddress) -> Option<(Duration, HeldDown)> { + let mut held = self.held.locked(); + let (until, why) = *held.get(&address)?; let remaining = until.saturating_duration_since(Instant::now()); if remaining.is_zero() { held.remove(&address); return None; } - Some(remaining) + Some((remaining, why)) + } + + /// A session with the peer at `address` that had come up has ended, + /// `lasted` after it did; `failed` if not at our own wish. Returns for + /// how long the peer is kept down for it, if it is. + pub fn session_ended( + &self, + address: PeerAddress, + lasted: Duration, + failed: bool, + ) -> Option { + let mut flaps = self.flaps.locked(); + if !failed || lasted >= FLAP_WITHIN { + flaps.remove(&address); + return None; + } + let count = flaps.entry(address).or_insert(0); + *count += 1; + let over = count.checked_sub(FLAPS_ALLOWED + 1)?; + let hold = FLAP_HOLD + .checked_mul(1u32.checked_shl(over).unwrap_or(u32::MAX)) + .map_or(FLAP_HOLD_MAX, |hold| hold.min(FLAP_HOLD_MAX)); + drop(flaps); + self.hold(address, hold, HeldDown::Flapping); + Some(hold) } } @@ -651,6 +811,9 @@ mod tests { ibgp: false, four_octet_asn: true, role: None, + peer_address: IpAddr::V4(Ipv4Addr::new(192, 0, 2, 2)), + local_address: IpAddr::V4(Ipv4Addr::new(192, 0, 2, 1)), + extended_next_hop: false, }; fn path(asns: &[u32]) -> Vec { @@ -853,41 +1016,6 @@ mod tests { } } - /// On a session with two-octet AS numbers a wider number is AS_TRANS in - /// the path and itself in AS4_PATH. The path is judged as it really is. - #[test] - fn a_path_from_a_two_octet_peer_is_judged_with_its_as4_path() { - let two_octet = From { - four_octet_asn: false, - ..EXTERNAL - }; - let policy = ImportPolicy { - origin_asns: Some([210036].into()), - ..ImportPolicy::default() - }; - let mut attributes = path(&[3333, 23456]); - attributes.push(PathAttribute::As4Path(AsPathAttribute { - segments: vec![AsPathSegment { - ordered: true, - path: vec![210036], - }], - })); - assert_eq!(policy.check_path(&attributes, &two_octet).map(drop), Ok(())); - - // What AS4_PATH reveals is judged too. - let mut attributes = path(&[3333, 23456]); - attributes.push(PathAttribute::As4Path(AsPathAttribute { - segments: vec![AsPathSegment { - ordered: true, - path: vec![4_200_000_001], - }], - })); - assert_eq!( - ImportPolicy::default().check_path(&attributes, &two_octet), - Err(Rejection::BogonAs) - ); - } - /// RFC 9234 §5, the ingress procedure. #[test] fn a_route_only_for_customers_is_a_leak_from_below_or_from_a_second_peer() { @@ -983,7 +1111,7 @@ mod tests { } #[test] - fn the_path_is_read_with_its_as4_path_and_its_sets() { + fn the_path_is_read_with_its_sets() { let mut out = vec![99]; assert!(!effective_path(&path(&[3333, 1299]), &mut out)); assert_eq!(out, [3333, 1299]); @@ -992,21 +1120,6 @@ mod tests { ordered: true, path, }; - let mut attributes = path(&[3333, 23456, 23456]); - attributes.push(PathAttribute::As4Path(AsPathAttribute { - segments: vec![sequence(vec![210036, 4_200_000_001])], - })); - assert!(!effective_path(&attributes, &mut out)); - assert_eq!(out, [3333, 210036, 4_200_000_001]); - - // An AS4_PATH longer than the path is not used. - let mut attributes = path(&[3333]); - attributes.push(PathAttribute::As4Path(AsPathAttribute { - segments: vec![sequence(vec![1, 2])], - })); - assert!(!effective_path(&attributes, &mut out)); - assert_eq!(out, [3333]); - let with_set = vec![PathAttribute::AsPath(AsPathAttribute { segments: vec![ sequence(vec![3333]), @@ -1023,6 +1136,236 @@ mod tests { assert!(out.is_empty()); } + /// The counts of refusals are kept by a reason's place in the list. + #[test] + fn every_reason_is_at_its_own_place_in_the_list() { + for (at, rejection) in Rejection::ALL.iter().enumerate() { + assert_eq!(rejection.index(), at, "{rejection:?}"); + } + let names: HashSet<_> = Rejection::ALL.iter().map(|r| r.as_str()).collect(); + assert_eq!(names.len(), Rejection::ALL.len()); + } + + /// RFC 9774: nobody can say where a route with a set in its path + /// comes from. + #[test] + fn a_path_with_a_set_is_refused() { + let with_set = vec![PathAttribute::AsPath(AsPathAttribute { + segments: vec![ + AsPathSegment { + ordered: true, + path: vec![3333, 6939], + }, + AsPathSegment { + ordered: false, + path: vec![2121, 2122], + }, + ], + })]; + assert_eq!( + ImportPolicy::default().check_path(&with_set, &EXTERNAL), + Err(Rejection::AsSet) + ); + let lenient = ImportPolicy { + accept_as_sets: true, + ..ImportPolicy::default() + }; + assert_eq!(lenient.check_path(&with_set, &EXTERNAL), Ok(None)); + } + + /// What we originate is ours: a more specific route for part of it + /// from another network would draw our own traffic away. + #[test] + fn our_own_prefixes_are_not_taken_from_others() { + let mut own = PrefixSet::default(); + own.allow(prefix("193.0.0.0/21"), 32); + let policy = ImportPolicy { + own_prefixes: Arc::new(own), + ..ImportPolicy::default() + }; + for ours in ["193.0.0.0/21", "193.0.4.0/22", "193.0.7.0/24"] { + assert_eq!( + policy.check_prefix(&prefix(ours)), + Err(Rejection::OwnPrefix), + "{ours}" + ); + } + // What covers it, or is beside it, is someone else's. + for theirs in ["193.0.0.0/20", "193.0.8.0/24"] { + assert_eq!(policy.check_prefix(&prefix(theirs)), Ok(()), "{theirs}"); + } + // A customer that has a part of it, and is listed as having it. + let mut listed = PrefixSet::default(); + listed.allow(prefix("193.0.4.0/24"), 24); + let customer = ImportPolicy { + prefixes: Some(listed), + ..policy + }; + assert_eq!(customer.check_prefix(&prefix("193.0.4.0/24")), Ok(())); + assert_eq!( + customer.check_prefix(&prefix("193.0.5.0/24")), + Err(Rejection::PrefixNotAllowed) + ); + } + + /// RFC 4271 §5.1.3 and more: the next hop is where our traffic goes. + #[test] + fn the_next_hop_has_to_be_the_peer() { + let policy = ImportPolicy::default(); + let v4 = |s: &str| NlriNextHop::Ipv4(s.parse().unwrap()); + let check = |policy: &ImportPolicy, next_hop: &NlriNextHop, from: &From| { + policy.check_next_hop(next_hop, IpVersion::Ipv4, from) + }; + assert_eq!(check(&policy, &v4("192.0.2.2"), &EXTERNAL), Ok(())); + // Another machine on the same network, or anywhere. + assert_eq!( + check(&policy, &v4("192.0.2.3"), &EXTERNAL), + Err(Rejection::NextHopNotPeer) + ); + // Nowhere at all, whatever the policy. + let lenient = ImportPolicy::permissive(); + for nowhere in [ + "0.0.0.0", + "127.0.0.1", + "224.0.0.5", + "255.255.255.255", + "192.0.2.1", + ] { + for policy in [&policy, &lenient] { + assert_eq!( + check(policy, &v4(nowhere), &EXTERNAL), + Err(Rejection::NextHopUnusable), + "{nowhere}" + ); + } + } + // Where it may be someone else: said so, a route server, our own AS. + assert_eq!(check(&lenient, &v4("192.0.2.3"), &EXTERNAL), Ok(())); + for from in [ + From { + role: Some(PeerRole::RouteServer), + ..EXTERNAL + }, + From { + ibgp: true, + ..EXTERNAL + }, + ] { + assert_eq!(check(&policy, &v4("192.0.2.3"), &from), Ok(())); + } + // A provider is not a route server. + let provider = From { + role: Some(PeerRole::Provider), + ..EXTERNAL + }; + assert_eq!( + check(&policy, &v4("192.0.2.3"), &provider), + Err(Rejection::NextHopNotPeer) + ); + } + + #[test] + fn an_ipv6_next_hop_is_judged_in_each_of_its_forms() { + let policy = ImportPolicy::default(); + let peer: Ipv6Addr = "2001:db8::2".parse().unwrap(); + let peer_ll: Ipv6Addr = "fe80::2".parse().unwrap(); + let over_v6 = From { + peer_address: peer.into(), + local_address: "2001:db8::1".parse().unwrap(), + ..EXTERNAL + }; + let v6 = |policy: &ImportPolicy, next_hop: NlriNextHop, from: &From| { + policy.check_next_hop(&next_hop, IpVersion::Ipv6, from) + }; + assert_eq!(v6(&policy, NlriNextHop::Ipv6(peer), &over_v6), Ok(())); + let both = + |global: Ipv6Addr, link_local: Ipv6Addr| NlriNextHop::Ipv6WithLl { global, link_local }; + assert_eq!(v6(&policy, both(peer, peer_ll), &over_v6), Ok(())); + assert_eq!( + v6( + &policy, + both("2001:db8::3".parse().unwrap(), peer_ll), + &over_v6 + ), + Err(Rejection::NextHopNotPeer) + ); + // What goes with a global address has to be a link-local one. + assert_eq!( + v6( + &policy, + both(peer, "2001:db8::9".parse().unwrap()), + &over_v6 + ), + Err(Rejection::NextHopUnusable) + ); + assert_eq!( + v6(&policy, both(peer, "::1".parse().unwrap()), &over_v6), + Err(Rejection::NextHopUnusable) + ); + + // A session between link-local addresses: the peer's own, alone, + // twice, or after nothing. + let link_local = From { + peer_address: peer_ll.into(), + local_address: "fe80::1".parse().unwrap(), + ..EXTERNAL + }; + for next_hop in [ + NlriNextHop::Ipv6(peer_ll), + both(peer_ll, peer_ll), + both(Ipv6Addr::UNSPECIFIED, peer_ll), + ] { + assert_eq!(v6(&policy, next_hop, &link_local), Ok(()), "{next_hop:?}"); + } + assert_eq!( + v6( + &policy, + both(Ipv6Addr::UNSPECIFIED, "fe80::3".parse().unwrap()), + &link_local + ), + Err(Rejection::NextHopNotPeer) + ); + assert_eq!( + v6( + &policy, + NlriNextHop::Ipv6(Ipv6Addr::UNSPECIFIED), + &link_local + ), + Err(Rejection::NextHopUnusable) + ); + assert_eq!( + v6( + &policy, + NlriNextHop::Ipv6("fe80::1".parse().unwrap()), + &link_local + ), + Err(Rejection::NextHopUnusable), + "our own address" + ); + + // An IPv4 route with an IPv6 next hop only where that was agreed + // on (RFC 8950). + let v4_route = + |from: &From| policy.check_next_hop(&NlriNextHop::Ipv6(peer), IpVersion::Ipv4, from); + assert_eq!(v4_route(&over_v6), Err(Rejection::NextHopUnusable)); + let agreed = From { + extended_next_hop: true, + ..over_v6 + }; + assert_eq!(v4_route(&agreed), Ok(())); + // IPv6 routes over an IPv4 session: the session's addresses say + // nothing about the next hop. + assert_eq!(v6(&policy, NlriNextHop::Ipv6(peer), &EXTERNAL), Ok(())); + assert_eq!( + policy.check_next_hop( + &NlriNextHop::Ipv4("192.0.2.2".parse().unwrap()), + IpVersion::Ipv6, + &EXTERNAL + ), + Err(Rejection::NextHopUnusable) + ); + } + #[test] fn an_absurdly_long_path_is_refused() { let policy = ImportPolicy::default(); @@ -1055,6 +1398,10 @@ mod tests { Err(Rejection::OriginNotAllowed) ); // A path that ends in a set has no one origin. + let policy = ImportPolicy { + accept_as_sets: true, + ..policy + }; let ends_in_set = vec![PathAttribute::AsPath(AsPathAttribute { segments: vec![ AsPathSegment { @@ -1077,20 +1424,55 @@ mod tests { fn a_peer_is_held_down_for_as_long_as_was_said() { let held = HoldDowns::default(); let address = PeerAddress::from("192.0.2.1".parse::().unwrap()); - assert_eq!(held.remaining(address), None); + let other = PeerAddress::from("192.0.2.2".parse::().unwrap()); - held.hold(address, Duration::from_secs(600)); - let remaining = held.remaining(address).expect("held"); - assert!(remaining > Duration::from_secs(590), "{remaining:?}"); + // Two failures soon after coming up are let pass; from the third + // the peer is kept down, twice as long each time, up to a limit. + let flaps = HoldDowns::default(); + let soon = Duration::from_secs(5); + assert_eq!(flaps.session_ended(address, soon, true), None); + assert_eq!(flaps.session_ended(address, soon, true), None); + assert_eq!(flaps.session_ended(address, soon, true), Some(FLAP_HOLD)); assert_eq!( - held.remaining("192.0.2.2".parse::().unwrap().into()), - None + flaps.session_ended(address, soon, true), + Some(2 * FLAP_HOLD) ); - - held.hold(address, Duration::ZERO); + assert_eq!( + flaps.remaining(address).map(|(_, why)| why), + Some(HeldDown::Flapping) + ); + assert_eq!(flaps.remaining(other), None, "each peer for itself"); + for _ in 0..40 { + flaps.session_ended(address, soon, true); + } + assert_eq!( + flaps.session_ended(address, soon, true), + Some(FLAP_HOLD_MAX) + ); + // A session that lasted, or that we ended ourselves, starts the + // count over. + assert_eq!(flaps.session_ended(address, FLAP_WITHIN, true), None); + assert_eq!(flaps.session_ended(address, soon, true), None); + assert_eq!(flaps.session_ended(address, soon, false), None); + assert_eq!(flaps.session_ended(address, soon, true), None); assert_eq!(held.remaining(address), None); - assert!( - held.0.locked().is_empty(), + + held.hold(address, Duration::from_secs(600), HeldDown::OverLimit); + let (remaining, why) = held.remaining(address).expect("held"); + assert!(remaining > Duration::from_secs(590), "{remaining:?}"); + assert_eq!(why, HeldDown::OverLimit); + assert_eq!(held.remaining(other), None); + // Not made shorter by a lesser reason. + held.hold(address, Duration::from_secs(1), HeldDown::Flapping); + let (remaining, why) = held.remaining(address).expect("held"); + assert!(remaining > Duration::from_secs(590), "{remaining:?}"); + assert_eq!(why, HeldDown::OverLimit); + + held.hold(other, Duration::ZERO, HeldDown::OverLimit); + assert_eq!(held.remaining(other), None); + assert_eq!( + held.held.locked().len(), + 1, "a hold that is over is forgotten" ); } diff --git a/crates/server/src/routes.rs b/crates/server/src/routes.rs index ad08dfd..618f10d 100644 --- a/crates/server/src/routes.rs +++ b/crates/server/src/routes.rs @@ -7,6 +7,7 @@ use std::collections::HashMap; use std::sync::Arc; use bgp_packet::message::{NlriNextHop, PathAttribute, UpdateMessage, only_to_customer_wire}; +use bgp_primitives::constants::IpVersion; use bgp_primitives::ip_prefix::Nlri; use bgp_primitives::parser::EncodeError; use bgp_rib::table::{AttrSet, Outcome, PeerIndex, RibTable}; @@ -113,36 +114,42 @@ pub(crate) fn apply( update: &UpdateMessage, attributes_wire: Option<&[u8]>, ) -> Result { - let peer = import.peer; - let mut applied = Applied::default(); - - if !update.withdrawn_routes.is_empty() { - applied.outcome += table.withdraw(peer, &update.withdrawn_routes); - held.forget(&update.withdrawn_routes); - } + let mut applying = Applying { + table, + import, + held, + update, + attributes_wire, + applied: Applied::default(), + // What the routes have in common is judged once. + path_verdict: import + .policy + .check_path(&update.path_attributes, &import.from), + path_invalid: OnceCell::new(), + as_path: OnceCell::new(), + mark: only_to_customer_mark(import, update), + }; + applying.withdraw(); + // Routes in the body have their next hop in the NEXT_HOP attribute, + // which is one of the set's attributes; routes in MP_REACH_NLRI bring + // theirs along. + applying.announce(&update.announced_routes, None)?; for attribute in &update.path_attributes { - if let PathAttribute::MpUnreachNlri(mp) = attribute { - applied.outcome += table.withdraw(peer, &mp.prefixes); - held.forget(&mp.prefixes); + if let PathAttribute::MpReachNlri(mp) = attribute { + applying.announce(&mp.prefixes, Some(&mp.next_hop))?; } } + Ok(applying.applied) +} - // The path the routes came with, for the record of what is refused: - // made only if something is. - let as_path = OnceCell::new(); - // What the routes have in common is judged once. - let path_verdict = import - .policy - .check_path(&update.path_attributes, &import.from); - // Whether the ASPAs find the path invalid: asked only if a route gets - // as far as that mattering. - let path_invalid = OnceCell::new(); - - // A route from above or beside us may from here on only go down to - // customers. If nobody has marked it so yet, we do (RFC 9234 §5, - // ingress, 3): the mark is part of the route as it is kept, and goes - // wherever the route goes. - let mark = import +/// The AS to mark the routes of `update` with as ones that may from here +/// on only go down to customers, if they are to be marked. +/// +/// A route from above or beside us is such a route. If nobody has marked +/// it so yet, we do (RFC 9234 §5, ingress, 3): the mark is part of the +/// route as it is kept, and goes wherever the route goes. +fn only_to_customer_mark(import: &Import<'_>, update: &UpdateMessage) -> Option { + import .from .role .filter(|_| !import.from.ibgp) @@ -152,90 +159,185 @@ pub(crate) fn apply( .path_attributes .iter() .any(|attribute| matches!(attribute, PathAttribute::OnlyToCustomer(_))) - }); - let attributes = |next_hop: Option<&NlriNextHop>| -> Result, EncodeError> { - let Some(marked_by) = mark else { - if let Some(set) = - attributes_wire.and_then(|wire| table.attrs().intern_received(wire, next_hop)) - { - return Ok(set); + }) +} + +/// One UPDATE on its way into the table. +struct Applying<'a> { + table: &'a RibTable, + import: &'a Import<'a>, + held: &'a mut Held, + update: &'a UpdateMessage, + attributes_wire: Option<&'a [u8]>, + applied: Applied, + /// What the policy makes of the path, and the AS the routes originate + /// from if it passes. + path_verdict: Result, Rejection>, + /// Whether the ASPAs find the path invalid: asked only if a route + /// gets as far as that mattering. + path_invalid: OnceCell, + /// The path the routes came with, for the record of what is refused: + /// made only if something is. + as_path: OnceCell>, + /// See [`only_to_customer_mark`]. + mark: Option, +} + +impl Applying<'_> { + /// Takes out what the UPDATE withdraws. + fn withdraw(&mut self) { + let peer = self.import.peer; + let unreachable = + self.update + .path_attributes + .iter() + .filter_map(|attribute| match attribute { + PathAttribute::MpUnreachNlri(mp) => Some(&mp.prefixes), + _ => None, + }); + for routes in std::iter::once(&self.update.withdrawn_routes).chain(unreachable) { + if !routes.is_empty() { + self.applied.outcome += self.table.withdraw(peer, routes); + self.held.forget(routes); } - return table.attrs().intern(&update.path_attributes, next_hop); - }; - let added = only_to_customer_wire(marked_by); - if let Some(set) = attributes_wire - .and_then(|wire| table.attrs().intern_received_with(wire, &added, next_hop)) - { - return Ok(set); } - let mut marked = update.path_attributes.clone(); - marked.push(PathAttribute::OnlyToCustomer(marked_by)); - table.attrs().intern(&marked, next_hop) - }; - let mut announce = |nlri: &[Nlri], next_hop| -> Result<(), EncodeError> { - if nlri.is_empty() { + } + + /// Puts in the routes of `nlri` that the policy takes. `next_hop` is + /// theirs if they came in MP_REACH_NLRI. + fn announce( + &mut self, + nlri: &[Nlri], + next_hop: Option<&NlriNextHop>, + ) -> Result<(), EncodeError> { + let Some(first) = nlri.first() else { return Ok(()); - } + }; // Whatever becomes of them, these replace what was held of the // same routes. - held.forget(nlri); - let mut refuse = |refused: &[Nlri], rejection: Rejection| { - import - .refusals - .note(refused, rejection, as_path.get_or_init(|| path_of(update))); - applied.rejected += refused.len(); - applied.outcome += table.withdraw(peer, refused); - }; - let origin = match path_verdict { + self.held.forget(nlri); + let origin = match self.judge_together(first.prefix.address_family(), next_hop) { Ok(origin) => origin, Err(rejection) => { - refuse(nlri, rejection); + self.refuse(nlri, rejection); return Ok(()); } }; + // Nearly always every route is taken, and then nothing is copied. + let mut accepted = Cow::Borrowed(nlri); let mut invalid = Vec::new(); - let accepted = sift( - nlri, - |route| { - import.policy.check_prefix(&route.prefix)?; - let origin_invalid = import - .checks - .vrps - .is_some_and(|vrps| vrps.validate(&route.prefix, origin) == Validity::Invalid); - let rejection = if origin_invalid { - Rejection::RpkiInvalid - } else if *path_invalid.get_or_init(|| path_is_invalid(import, update)) { - Rejection::AspaInvalid - } else { - return Ok(()); - }; - invalid.push(*route); - Err(rejection) - }, - &mut refuse, - ); + for (at, route) in nlri.iter().enumerate() { + match self.judge(route, origin) { + Ok(()) => { + if let Cow::Owned(accepted) = &mut accepted { + accepted.push(*route); + } + } + Err(rejection) => { + if let Cow::Borrowed(_) = accepted { + accepted = Cow::Owned(nlri[..at].to_vec()); + } + if matches!(rejection, Rejection::RpkiInvalid | Rejection::AspaInvalid) { + invalid.push(*route); + } + self.refuse(std::slice::from_ref(route), rejection); + } + } + } if accepted.is_empty() && invalid.is_empty() { return Ok(()); } - let attrs = attributes(next_hop)?; + let attrs = self.attributes(next_hop)?; for route in invalid { - held.hold(route, &attrs); + self.held.hold(route, &attrs); } if !accepted.is_empty() { - applied.outcome += table.announce(peer, &attrs, &accepted); + self.applied.outcome += self.table.announce(self.import.peer, &attrs, &accepted); } Ok(()) - }; - // Routes in the body have their next hop in the NEXT_HOP attribute, - // which is one of the set's attributes; routes in MP_REACH_NLRI bring - // theirs along. - announce(&update.announced_routes, None)?; - for attribute in &update.path_attributes { - if let PathAttribute::MpReachNlri(mp) = attribute { - announce(&mp.prefixes, Some(&mp.next_hop))?; - } } - Ok(applied) + + /// 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. + fn judge_together( + &self, + routes: IpVersion, + next_hop: Option<&NlriNextHop>, + ) -> Result, Rejection> { + let origin = self.path_verdict?; + let in_attributes = || { + self.update + .path_attributes + .iter() + .find_map(|attribute| match attribute { + PathAttribute::NextHop(next_hop) => Some(NlriNextHop::Ipv4(next_hop.0)), + _ => None, + }) + }; + // The decoder lets no route through without one. + let next_hop = next_hop + .copied() + .or_else(in_attributes) + .ok_or(Rejection::NextHopUnusable)?; + self.import + .policy + .check_next_hop(&next_hop, routes, &self.import.from)?; + Ok(origin) + } + + /// Judges one route, whose origin is `origin`. + fn judge(&self, route: &Nlri, origin: Option) -> Result<(), Rejection> { + let import = self.import; + import.policy.check_prefix(&route.prefix)?; + let origin_invalid = import + .checks + .vrps + .is_some_and(|vrps| vrps.validate(&route.prefix, origin) == Validity::Invalid); + if origin_invalid { + return Err(Rejection::RpkiInvalid); + } + if *self + .path_invalid + .get_or_init(|| path_is_invalid(import, self.update)) + { + return Err(Rejection::AspaInvalid); + } + Ok(()) + } + + /// Notes that `refused` were not taken, and takes out what the peer + /// had announced for them before. + fn refuse(&mut self, refused: &[Nlri], rejection: Rejection) { + let as_path = self.as_path.get_or_init(|| path_of(self.update)); + self.import.refusals.note(refused, rejection, as_path); + self.applied.rejected += refused.len(); + self.applied.outcome += self.table.withdraw(self.import.peer, refused); + } + + /// The attribute set the UPDATE's routes go into the table with. + fn attributes(&self, next_hop: Option<&NlriNextHop>) -> Result, EncodeError> { + let store = self.table.attrs(); + let Some(marked_by) = self.mark else { + if let Some(set) = self + .attributes_wire + .and_then(|wire| store.intern_received(wire, next_hop)) + { + return Ok(set); + } + return store.intern(&self.update.path_attributes, next_hop); + }; + let added = only_to_customer_wire(marked_by); + if let Some(set) = self + .attributes_wire + .and_then(|wire| store.intern_received_with(wire, &added, next_hop)) + { + return Ok(set); + } + let mut marked = self.update.path_attributes.clone(); + marked.push(PathAttribute::OnlyToCustomer(marked_by)); + store.intern(&marked, next_hop) + } } /// Whether the ASPAs find the path of `update`'s routes invalid. @@ -266,34 +368,6 @@ fn path_of(update: &UpdateMessage) -> Arc<[u32]> { policy::asns(path).collect() } -/// The routes of `nlri` that `verdict` passes; the others are given to -/// `refuse`. Nearly always that is all of them, and nothing is copied. -fn sift<'a>( - nlri: &'a [Nlri], - mut verdict: impl FnMut(&Nlri) -> Result<(), Rejection>, - refuse: &mut impl FnMut(&[Nlri], Rejection), -) -> Cow<'a, [Nlri]> { - let mut first_refused = None; - for (at, route) in nlri.iter().enumerate() { - if let Err(rejection) = verdict(route) { - first_refused = Some((at, rejection)); - break; - } - } - let Some((at, rejection)) = first_refused else { - return Cow::Borrowed(nlri); - }; - refuse(std::slice::from_ref(&nlri[at]), rejection); - let mut accepted = nlri[..at].to_vec(); - for route in &nlri[at + 1..] { - match verdict(route) { - Ok(()) => accepted.push(*route), - Err(rejection) => refuse(std::slice::from_ref(route), rejection), - } - } - Cow::Owned(accepted) -} - /// Looks again at a peer's routes after the validation data has changed. /// /// `suspects` are routes of the peer's in the table that may now be diff --git a/crates/server/src/server.rs b/crates/server/src/server.rs index aa8d56d..e3d2b51 100644 --- a/crates/server/src/server.rs +++ b/crates/server/src/server.rs @@ -32,6 +32,7 @@ use crate::refusals::RefusalLog; use crate::rpki::spawn_revalidator; use crate::session::{ConfiguredPeer, SessionRegistry}; use crate::sync::IgnorePoison; +use crate::throttle::Throttle; /// How long sessions get to send their Cease when the server stops. const SHUTDOWN_GRACE: Duration = Duration::from_secs(2); @@ -50,6 +51,11 @@ const ACCEPT_RETRY: Duration = Duration::from_millis(200); /// table whose routes keep changing attributes collects them. const SWEEP_INTERVAL: Duration = Duration::from_secs(60); +/// How often it is looked at whether they have piled up, and how many +/// more than twice what the last sweep left count as that. +const SWEEP_CHECK_INTERVAL: Duration = Duration::from_secs(1); +const SWEEP_SLACK: usize = 100_000; + /// How many connections may wait to be accepted on a listener. const LISTEN_BACKLOG: u32 = 1024; @@ -385,11 +391,16 @@ impl Refusal { } /// Counts refused connections, and says so in the log now and then. -#[derive(Default)] struct Refusals { - logged: Option, - /// How many were refused since the log last said so. - unlogged: u64, + log: Throttle, +} + +impl Default for Refusals { + fn default() -> Self { + Self { + log: Throttle::every(REFUSAL_LOG_INTERVAL), + } + } } impl Refusals { @@ -397,21 +408,14 @@ impl Refusals { CONNECTIONS_REFUSED .with_label_values(&[refusal.as_str()]) .inc(); - if self - .logged - .is_some_and(|logged| logged.elapsed() < REFUSAL_LOG_INTERVAL) - { - self.unlogged += 1; - return; + if let Some(unsaid) = self.log.pass() { + warn!( + peer_addr = %socket_text(peer_addr), + reason = refusal.as_str(), + others_since_last_report = unsaid, + "Refused a connection" + ); } - warn!( - peer_addr = %socket_text(peer_addr), - reason = refusal.as_str(), - others_since_last_report = self.unlogged, - "Refused a connection" - ); - self.logged = Some(Instant::now()); - self.unlogged = 0; } } @@ -453,16 +457,28 @@ impl Drop for ConnectionSlot { } } -/// Drops, every so often, the attribute sets no route uses any more. +/// Drops, every so often, the attribute sets no route uses any more; and +/// sooner if they pile up. A peer that announces one prefix over and over +/// with different attributes each time has one route, well within any +/// limit, and leaves a set behind with every UPDATE. fn spawn_sweeper(rib: Arc) -> JoinHandle<()> { tokio::spawn(async move { - let mut tick = tokio::time::interval(SWEEP_INTERVAL); + let mut look = tokio::time::interval(SWEEP_CHECK_INTERVAL); + let mut swept_at = Instant::now(); + // How many sets there were after the last sweep. + let mut kept = rib.attrs().len(); loop { - tick.tick().await; - let rib = Arc::clone(&rib); + look.tick().await; + let piled_up = rib.attrs().len() > 2 * kept + SWEEP_SLACK; + if !piled_up && swept_at.elapsed() < SWEEP_INTERVAL { + continue; + } + let table = Arc::clone(&rib); // Takes each of the store's locks in turn and looks at every // set: not for a thread that sessions run on. - let _ = tokio::task::spawn_blocking(move || rib.attrs().sweep()).await; + let _ = tokio::task::spawn_blocking(move || table.attrs().sweep()).await; + swept_at = Instant::now(); + kept = rib.attrs().len(); } }) } diff --git a/crates/server/src/throttle.rs b/crates/server/src/throttle.rs new file mode 100644 index 0000000..eb66ac8 --- /dev/null +++ b/crates/server/src/throttle.rs @@ -0,0 +1,63 @@ +//! Saying a thing in the log once in a while instead of every time. +//! +//! What a peer can make us log, it can make us log a million times: a +//! malformed attribute on every route of a full table, a connection +//! attempt every millisecond. Such things are counted every time, in a +//! metric, and logged at most once per interval with how many went unsaid. + +use std::time::{Duration, Instant}; + +#[derive(Debug)] +pub(crate) struct Throttle { + interval: Duration, + said: Option, + unsaid: u64, +} + +impl Throttle { + pub(crate) const fn every(interval: Duration) -> Self { + Self { + interval, + said: None, + unsaid: 0, + } + } + + /// Whether the thing may be said now. If so, returns how many times it + /// went unsaid since it last was. + pub(crate) fn pass(&mut self) -> Option { + if self.said.is_some_and(|said| said.elapsed() < self.interval) { + self.unsaid += 1; + return None; + } + self.said = Some(Instant::now()); + Some(std::mem::take(&mut self.unsaid)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_thing_is_said_once_per_interval_with_what_went_unsaid() { + let mut throttle = Throttle::every(Duration::from_secs(3600)); + assert_eq!(throttle.pass(), Some(0)); + assert_eq!(throttle.pass(), None); + assert_eq!(throttle.pass(), None); + + let mut throttle = Throttle::every(Duration::ZERO); + assert_eq!(throttle.pass(), Some(0)); + assert_eq!(throttle.pass(), Some(0)); + } + + #[test] + fn what_went_unsaid_is_reported_once() { + let mut throttle = Throttle::every(Duration::from_millis(20)); + assert_eq!(throttle.pass(), Some(0)); + assert_eq!(throttle.pass(), None); + assert_eq!(throttle.pass(), None); + std::thread::sleep(Duration::from_millis(30)); + assert_eq!(throttle.pass(), Some(2)); + } +} diff --git a/crates/web/static/statusz.html b/crates/web/static/statusz.html index 6bad946..a5571cc 100644 --- a/crates/web/static/statusz.html +++ b/crates/web/static/statusz.html @@ -120,7 +120,7 @@ from }; const $ = id => document.getElementById(id); - const esc = s => String(s).replace(/[&<>"]/g, c => ({'&':'&','<':'<','>':'>','"':'"'}[c])); + const esc = s => String(s).replace(/[&<>"']/g, c => ({'&':'&','<':'<','>':'>','"':'"',"'":'''}[c])); const state = { status: null, peers: [], local: [], received: [], receivedPage: {}, refused: [], rpki: null, kernel: null }; const DASH = '-'; diff --git a/docs/RFCs.md b/docs/RFCs.md index 33b61d5..c770cf9 100644 --- a/docs/RFCs.md +++ b/docs/RFCs.md @@ -52,6 +52,15 @@ https://datatracker.ietf.org/doc/html/rfc6793 | `FourByteAsn` capability advertisement and parsing | ✅ | | 4-byte AS_PATH segment encoding/decoding via `ParserContext` | ✅ | | 4-byte ASN negotiation in FSM | ✅ | +| AS4_PATH from a two-octet peer merged into AS_PATH on receipt (§4.2.3); one longer than AS_PATH ignored | ✅ | + +### RFC 9774 — Deprecation of AS_SET and AS_CONFED_SET in BGP +https://datatracker.ietf.org/doc/html/rfc9774 + +| Feature | Status | +|---------|--------| +| Routes with an AS_SET in their path refused on import (`accept_as_sets` takes them) | ✅ | +| AS_SET never sent | ✅ | ### RFC 9072 — Extended Optional Parameters Length for BGP OPEN Message https://datatracker.ietf.org/doc/html/rfc9072 diff --git a/scripts/interop/lab.sh b/scripts/interop/lab.sh index 7621bbe..524785f 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 v6only linklocal" +all_phases="basics roles rpki refresh flaps restart scale limits hijacks v6only linklocal" phases="${PHASES:-$all_phases}" scale_routes="${SCALE_ROUTES:-200000}" @@ -726,6 +726,51 @@ phase_limits() { ok "back to normal" } +# FRR tries what a peer must not get away with: announcing our own +# prefix, announcing the exchange LAN itself (which would take the +# sessions with everyone on it), and sending our traffic to someone else. +refused_for() { # refused_for reason: how many of frr4's routes were refused for it + api "/api/routes/rejected?peer=frr4" | jq -e --arg why "$1" '.[0].by_reason[$why] // 0 | . >= 1' +} +phase_hijacks() { + frr_configure \ + "ip prefix-list ELSEWHERE seq 5 permit 10.2.7.0/24" \ + "route-map TO-DUT permit 10" "match ip address prefix-list ELSEWHERE" "set ip next-hop 10.9.0.3" \ + "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.1.0.0/24" "network 10.9.0.0/24" "network 10.2.7.0/24" + + # Our own prefix, from someone else. + wait_for "our own prefix to be refused" refused_for "prefix is our own" + dut_lacks 10.1.0.0/24 || fail "our own prefix was taken from FRR" + ok "our own prefix is not taken from a peer" + + # A route whose next hop is BIRD, from FRR. + wait_for "the third-party next hop to be refused" refused_for "next hop is not the peer" + dut_lacks 10.2.7.0/24 || fail "a route by way of a third party was taken" + ok "a route whose next hop is someone else is not taken" + + # The LAN's own prefix. The lab takes bogons, so the route is taken + # into the table; it must not get into the kernel, where it would + # compete with the connected route. + wait_for "the LAN's prefix at the daemon" dut_has 10.9.0.0/24 frr4 + sleep 1 + in_ns dut ip -4 route show proto bgp | grep -q "^10.9.0.0/24 " && fail "the LAN's own prefix was installed by way of a peer" + api /api/kernel | jq -e '.unresolved["inside a network of this machine"] >= 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" + + 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" +} + # The exchange LAN loses its IPv4 addresses. One session over IPv6 with # each router carries both families, and IPv4 routes have IPv6 next hops # (RFC 8950), in BGP and in the kernel. Run after the rest: the lab stays