diff --git a/crates/integration-tests/tests/basic_connection.rs b/crates/integration-tests/tests/basic_connection.rs index dbfa965..e089ef3 100644 --- a/crates/integration-tests/tests/basic_connection.rs +++ b/crates/integration-tests/tests/basic_connection.rs @@ -5,8 +5,9 @@ use bgp_integration_tests::test_peer::TestPeer; use bgp_integration_tests::test_server::TestServer; use bgp_packet::errors::{BgpError, CeaseSubcode, FsmSubcode, MsgHeaderSubcode, UpdateMsgSubcode}; use bgp_packet::message::{ - Message, MpReachNlriPathAttribute, MpUnreachNlriPathAttribute, NlriNextHop, - NotificationMessage, PathAttribute, UpdateMessage, + AsPathAttribute, Message, MpReachNlriPathAttribute, MpUnreachNlriPathAttribute, + NextHopPathAttribute, NlriNextHop, NotificationMessage, OriginPathAttribute, PathAttribute, + UpdateMessage, }; use bgp_primitives::constants::{AddressFamilyId, Afi, Safi}; use bgp_primitives::ip_prefix::IpPrefix; @@ -15,6 +16,27 @@ use eyre::Result; const TIMEOUT: Duration = Duration::from_secs(5); +/// The attributes every announcement of IPv4 routes must carry. An UPDATE +/// without them has its routes withdrawn instead (RFC 7606). +fn mandatory_attributes() -> Vec { + let mut attributes = with_origin_and_as_path(PathAttribute::NextHop(NextHopPathAttribute( + Ipv4Addr::new(192, 0, 2, 1), + ))); + // In order of type code: ORIGIN, AS_PATH, NEXT_HOP. + attributes.rotate_left(1); + attributes +} + +/// `first` followed by the attributes every announcement must carry. +/// Routes in MP_REACH_NLRI have their next hop there. +fn with_origin_and_as_path(first: PathAttribute) -> Vec { + vec![ + first, + PathAttribute::Origin(OriginPathAttribute::IGP), + AsPathAttribute::from_asns(vec![65001]), + ] +} + // --------------------------------------------------------------------------- // Connectivity // --------------------------------------------------------------------------- @@ -195,7 +217,7 @@ async fn test_message_split_across_a_keepalive_tick_is_still_one_message() -> Re let mut update = bytes::BytesMut::new(); Message::Update(UpdateMessage { withdrawn_routes: vec![], - path_attributes: vec![], + path_attributes: mandatory_attributes(), announced_routes: vec![prefix.clone()], }) .to_wire(&peer.ctx, &mut update)?; @@ -322,7 +344,7 @@ async fn test_server_accepts_update() -> Result<()> { let update = Message::Update(UpdateMessage { withdrawn_routes: vec![], - path_attributes: vec![], + path_attributes: mandatory_attributes(), // 10.0.0.0/8 — one byte of prefix for a /8. announced_routes: vec![IpPrefix::new(AddressFamilyId::Ipv4, vec![10], 8)?], }); @@ -345,12 +367,14 @@ async fn test_server_accepts_ipv6_update() -> Result<()> { let prefix = IpPrefix::new(AddressFamilyId::Ipv6, vec![0x20, 0x01, 0x0d, 0xb8], 32)?; let update = Message::Update(UpdateMessage { withdrawn_routes: vec![], - path_attributes: vec![PathAttribute::MpReachNlri(MpReachNlriPathAttribute { - afi: Afi::IPV6, - safi: Safi::UNICAST, - next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()), - prefixes: vec![prefix], - })], + path_attributes: with_origin_and_as_path(PathAttribute::MpReachNlri( + MpReachNlriPathAttribute { + afi: Afi::IPV6, + safi: Safi::UNICAST, + next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()), + prefixes: vec![prefix], + }, + )), announced_routes: vec![], }); peer.send(&update).await?; @@ -377,7 +401,7 @@ async fn test_rib_records_ipv4_announce() -> Result<()> { let prefix = IpPrefix::new(AddressFamilyId::Ipv4, vec![10], 8)?; peer.send(&Message::Update(UpdateMessage { withdrawn_routes: vec![], - path_attributes: vec![], + path_attributes: mandatory_attributes(), announced_routes: vec![prefix.clone()], })) .await?; @@ -411,7 +435,7 @@ async fn test_rib_records_ipv4_withdraw() -> Result<()> { // Announce first. peer.send(&Message::Update(UpdateMessage { withdrawn_routes: vec![], - path_attributes: vec![], + path_attributes: mandatory_attributes(), announced_routes: vec![prefix.clone()], })) .await?; @@ -452,12 +476,14 @@ async fn test_rib_records_ipv6_announce() -> Result<()> { let prefix = IpPrefix::new(AddressFamilyId::Ipv6, vec![0x20, 0x01, 0x0d, 0xb8], 32)?; peer.send(&Message::Update(UpdateMessage { withdrawn_routes: vec![], - path_attributes: vec![PathAttribute::MpReachNlri(MpReachNlriPathAttribute { - afi: Afi::IPV6, - safi: Safi::UNICAST, - next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()), - prefixes: vec![prefix.clone()], - })], + path_attributes: with_origin_and_as_path(PathAttribute::MpReachNlri( + MpReachNlriPathAttribute { + afi: Afi::IPV6, + safi: Safi::UNICAST, + next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()), + prefixes: vec![prefix.clone()], + }, + )), announced_routes: vec![], })) .await?; @@ -491,12 +517,14 @@ async fn test_rib_records_ipv6_withdraw() -> Result<()> { // Announce first. peer.send(&Message::Update(UpdateMessage { withdrawn_routes: vec![], - path_attributes: vec![PathAttribute::MpReachNlri(MpReachNlriPathAttribute { - afi: Afi::IPV6, - safi: Safi::UNICAST, - next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()), - prefixes: vec![prefix.clone()], - })], + path_attributes: with_origin_and_as_path(PathAttribute::MpReachNlri( + MpReachNlriPathAttribute { + afi: Afi::IPV6, + safi: Safi::UNICAST, + next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()), + prefixes: vec![prefix.clone()], + }, + )), announced_routes: vec![], })) .await?; diff --git a/crates/integration-tests/tests/update_errors.rs b/crates/integration-tests/tests/update_errors.rs new file mode 100644 index 0000000..baf7c74 --- /dev/null +++ b/crates/integration-tests/tests/update_errors.rs @@ -0,0 +1,231 @@ +//! End-to-end tests for errors in UPDATE messages (RFC 7606): most of them +//! cost the routes in the UPDATE, not the session. + +use std::net::Ipv4Addr; +use std::time::Duration; + +use bgp_integration_tests::test_peer::TestPeer; +use bgp_integration_tests::test_server::TestServer; +use bgp_packet::errors::{BgpError, UpdateMsgSubcode}; +use bgp_packet::message::{LocalPrefPathAttribute, Message, PathAttribute}; +use bgp_primitives::ip_prefix::IpPrefix; +use bgp_rib::SnapshotEntry; +use bgp_server::metrics::UPDATE_ERRORS; +use eyre::{Result, bail}; + +const LOCAL_ASN: u32 = 65000; +const PEER_ASN: u32 = 65001; +const TIMEOUT: Duration = Duration::from_secs(5); + +const WELL_KNOWN: u8 = 0x40; +const OPTIONAL_TRANSITIVE: u8 = 0xc0; + +fn attr(flags: u8, type_code: u8, value: &[u8]) -> Vec { + let mut wire = vec![flags, type_code, value.len() as u8]; + wire.extend_from_slice(value); + wire +} + +fn origin() -> Vec { + attr(WELL_KNOWN, 1, &[0]) +} + +/// AS_SEQUENCE of the peer's AS, four octets wide. +fn as_path() -> Vec { + attr(WELL_KNOWN, 2, &[2, 1, 0, 0, 0xfd, 0xe9]) +} + +fn next_hop() -> Vec { + attr(WELL_KNOWN, 3, &[192, 0, 2, 1]) +} + +/// A whole UPDATE message announcing `nlri` with `attributes`, as bytes, so +/// that it can be wrong in ways the encoder would not write. +fn update(attributes: &[Vec], nlri: &[u8]) -> Vec { + let attributes = attributes.concat(); + let mut body = vec![0, 0]; + body.extend_from_slice(&(attributes.len() as u16).to_be_bytes()); + body.extend_from_slice(&attributes); + body.extend_from_slice(nlri); + + let mut wire = vec![0xff; 16]; + wire.extend_from_slice(&((19 + body.len()) as u16).to_be_bytes()); + wire.push(2); + wire.extend_from_slice(&body); + wire +} + +/// 10.1.0.0/16 and 10.2.0.0/16. +const ROUTE_A: [u8; 3] = [16, 10, 1]; +const ROUTE_B: [u8; 3] = [16, 10, 2]; + +fn prefix(s: &str) -> IpPrefix { + IpPrefix::try_from(s).unwrap() +} + +async fn established(server: &TestServer, asn: u32) -> Result { + let mut peer = TestPeer::connect(server.addr).await?; + peer.handshake(asn, Ipv4Addr::new(10, 0, 0, 1), 90).await?; + Ok(peer) +} + +/// Waits until the server's Adj-RIB-In holds exactly `prefixes`. +async fn wait_for_routes(server: &TestServer, prefixes: &[&str]) -> Result> { + let mut expected: Vec = prefixes.iter().map(|p| prefix(p)).collect(); + expected.sort_by_key(|p| p.to_string()); + Ok(tokio::time::timeout(TIMEOUT, async { + loop { + let mut routes = server.adj_rib_in.snapshot().await; + routes.sort_by_key(|r| r.prefix.to_string()); + if routes.iter().map(|r| &r.prefix).eq(expected.iter()) { + return routes; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await?) +} + +/// The situation RFC 7606 exists for: a route is in the table, and then an +/// UPDATE for it arrives with a broken attribute. The route goes; the +/// session, and the other routes learned over it, stay. +#[tokio::test] +async fn test_a_malformed_attribute_withdraws_the_route_and_keeps_the_session() -> Result<()> { + let server = TestServer::start(LOCAL_ASN).await?; + let mut peer = established(&server, PEER_ASN).await?; + let handled_before = UPDATE_ERRORS + .with_label_values(&["treat-as-withdraw"]) + .get(); + + let good = [origin(), as_path(), next_hop()]; + peer.send_raw(&update(&good, &[ROUTE_A, ROUTE_B].concat())) + .await?; + wait_for_routes(&server, &["10.1.0.0/16", "10.2.0.0/16"]).await?; + + // ORIGIN with a value that is not defined. + let bad = [attr(WELL_KNOWN, 1, &[7]), as_path(), next_hop()]; + peer.send_raw(&update(&bad, &ROUTE_A)).await?; + wait_for_routes(&server, &["10.2.0.0/16"]).await?; + + // The session goes on: the route can be announced again. + peer.send_raw(&update(&good, &ROUTE_A)).await?; + wait_for_routes(&server, &["10.1.0.0/16", "10.2.0.0/16"]).await?; + assert_eq!(server.registry.list().len(), 1); + assert!( + UPDATE_ERRORS + .with_label_values(&["treat-as-withdraw"]) + .get() + > handled_before + ); + Ok(()) +} + +/// An announcement without a mandatory attribute is not a route we can use. +#[tokio::test] +async fn test_a_route_without_as_path_is_not_accepted() -> Result<()> { + let server = TestServer::start(LOCAL_ASN).await?; + let mut peer = established(&server, PEER_ASN).await?; + + peer.send_raw(&update(&[origin(), next_hop()], &ROUTE_A)) + .await?; + // A well-formed one after it shows that the first was dealt with. + peer.send_raw(&update(&[origin(), as_path(), next_hop()], &ROUTE_B)) + .await?; + + wait_for_routes(&server, &["10.2.0.0/16"]).await?; + assert_eq!(server.registry.list().len(), 1); + Ok(()) +} + +/// A broken attribute that plays no part in choosing a route is dropped, +/// and the route is kept. +#[tokio::test] +async fn test_a_malformed_aggregator_is_dropped_and_the_route_kept() -> Result<()> { + let server = TestServer::start(LOCAL_ASN).await?; + let mut peer = established(&server, PEER_ASN).await?; + + let attributes = [ + origin(), + as_path(), + next_hop(), + // AGGREGATOR is eight octets on this session. + attr(OPTIONAL_TRANSITIVE, 7, &[0, 0, 0xfd, 0xe9, 192]), + ]; + peer.send_raw(&update(&attributes, &ROUTE_A)).await?; + + let routes = wait_for_routes(&server, &["10.1.0.0/16"]).await?; + assert_eq!(routes[0].entry.path_attributes.len(), 3); + assert!( + !routes[0] + .entry + .path_attributes + .iter() + .any(|a| matches!(a, PathAttribute::Aggregator(_))) + ); + Ok(()) +} + +/// LOCAL_PREF is a matter between routers of one AS. From an external peer +/// it is dropped; from an internal one it is kept. +#[tokio::test] +async fn test_local_pref_is_kept_only_from_an_internal_peer() -> Result<()> { + let local_pref = attr(WELL_KNOWN, 5, &[0, 0, 0, 200]); + let has_local_pref = |routes: &[SnapshotEntry]| { + routes[0] + .entry + .path_attributes + .contains(&PathAttribute::LocalPref(LocalPrefPathAttribute(200))) + }; + + let server = TestServer::start(LOCAL_ASN).await?; + let mut external = established(&server, PEER_ASN).await?; + let attributes = [origin(), as_path(), next_hop(), local_pref.clone()]; + external.send_raw(&update(&attributes, &ROUTE_A)).await?; + let routes = wait_for_routes(&server, &["10.1.0.0/16"]).await?; + assert!(!has_local_pref(&routes)); + + let server = TestServer::start(LOCAL_ASN).await?; + let mut internal = established(&server, LOCAL_ASN).await?; + // An internal peer has not added its AS to the path. + let attributes = [origin(), attr(WELL_KNOWN, 2, &[]), next_hop(), local_pref]; + internal.send_raw(&update(&attributes, &ROUTE_A)).await?; + let routes = wait_for_routes(&server, &["10.1.0.0/16"]).await?; + assert!(has_local_pref(&routes)); + Ok(()) +} + +/// When the prefixes themselves cannot be read there is nothing to +/// withdraw, and the session is reset as before. +#[tokio::test] +async fn test_an_unreadable_prefix_still_resets_the_session() -> Result<()> { + let server = TestServer::start(LOCAL_ASN).await?; + let mut peer = established(&server, PEER_ASN).await?; + + // An IPv4 prefix of length 33. + peer.send_raw(&update( + &[origin(), as_path(), next_hop()], + &[33, 10, 1, 2, 3, 4], + )) + .await?; + + loop { + match tokio::time::timeout(TIMEOUT, peer.recv()).await?? { + Message::KeepAlive => {} + Message::Notification(n) => { + assert_eq!( + n.error, + BgpError::UpdateMsg(UpdateMsgSubcode::InvalidNetworkField) + ); + break; + } + other => bail!("expected NOTIFICATION, got {other:?}"), + } + } + tokio::time::timeout(TIMEOUT, async { + while !server.registry.list().is_empty() { + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await?; + Ok(()) +} diff --git a/crates/packet/src/lib.rs b/crates/packet/src/lib.rs index 03b6a7b..1646d90 100644 --- a/crates/packet/src/lib.rs +++ b/crates/packet/src/lib.rs @@ -2,3 +2,4 @@ pub mod community; pub mod errors; pub mod message; pub mod open; +pub mod update_decode; diff --git a/crates/packet/src/message.rs b/crates/packet/src/message.rs index 3083cbb..2bc1387 100644 --- a/crates/packet/src/message.rs +++ b/crates/packet/src/message.rs @@ -14,6 +14,7 @@ use serde::{Deserialize, Serialize}; use serde_repr::{Deserialize_repr, Serialize_repr}; use strum::EnumDiscriminants; +use crate::update_decode::UpdateError; use bgp_primitives::constants::{AddressFamilyId, Afi, AfiSafi, Safi}; use bgp_primitives::ip_prefix::IpPrefix; use bgp_primitives::parser::BgpParserError; @@ -26,7 +27,6 @@ use crate::errors::DecodeError; use crate::errors::InvalidValue; use crate::errors::MsgHeaderSubcode; use crate::errors::OpenMsgSubcode; -use crate::errors::UpdateMsgSubcode; use crate::open::OpenMessage; /// A sentinel AS number used to denote that the actual AS number is provided as @@ -62,7 +62,22 @@ impl Message { /// /// A failure says which NOTIFICATION reports it, following RFC 4271 §6.1 /// for the header and §6.2 and §6.3 for OPEN and UPDATE bodies. + /// + /// An UPDATE comes back as it is to be acted on: errors that RFC 7606 + /// handles without resetting the session have been handled. Use + /// [`Message::decode_with_report`] to learn of them. pub fn decode(ctx: &ParserContext, buf: &[u8]) -> Result { + Self::decode_with_report(ctx, buf).map(|(msg, _)| msg) + } + + /// As [`Message::decode`], and also returns the errors in an UPDATE + /// that were handled without resetting the session. The list is empty + /// for a well-formed UPDATE and for every other message. + pub fn decode_with_report( + ctx: &ParserContext, + buf: &[u8], + ) -> Result<(Self, Vec), DecodeError> { + let mut update_errors = Vec::new(); let header_error = |subcode, data: Vec, detail: String| DecodeError { notification: BgpError::MsgHeader(subcode), data, @@ -106,7 +121,7 @@ impl Message { ) }; - match msg_type { + let message = match msg_type { 1 => OpenMessage::from_wire(ctx, body) .map(|(_, open)| Self::Open(open)) .map_err(|e| { @@ -136,13 +151,11 @@ impl Message { detail: format!("malformed OPEN: {}", describe(&e)), } }), - 2 => UpdateMessage::from_wire(ctx, body) - .map(|(_, update)| Self::Update(update)) - .map_err(|e| DecodeError { - notification: BgpError::UpdateMsg(UpdateMsgSubcode::MalformedAttrs), - data: vec![], - detail: format!("malformed UPDATE: {}", describe(&e)), - }), + 2 => { + let decoded = UpdateMessage::decode(ctx, body)?; + update_errors = decoded.errors; + Ok(Self::Update(decoded.update)) + } 3 => NotificationMessage::from_wire(ctx, body) .map(|(_, notification)| Self::Notification(notification)) .map_err(|e| bad_length(&format!("malformed NOTIFICATION ({})", describe(&e)))), @@ -161,7 +174,8 @@ impl Message { vec![other], format!("unknown BGP message type {other}"), )), - } + }?; + Ok((message, update_errors)) } /// Parses a complete BGP message including the 19-byte header from `buf`. @@ -269,61 +283,18 @@ pub struct UpdateMessage { } impl UpdateMessage { - /// Parses a BGP UPDATE message body per RFC 4271 §4.3. The buffer passed in - /// must contain exactly the UPDATE payload (i.e. without the 19 byte BGP - /// message header). The NLRI section has no explicit length and is inferred - /// as the remainder of the buffer, so passing extra trailing bytes will - /// cause them to be (mis-)parsed as NLRI. + /// Parses a BGP UPDATE message body (RFC 4271 §4.3), without the + /// 19-octet header, in the shape of the other parsers. + /// + /// This is [`UpdateMessage::decode`] with the report of handled errors + /// dropped; use `decode` when it matters what was wrong. pub fn from_wire<'a>( ctx: &ParserContext, buf: &'a [u8], ) -> IResult<&'a [u8], Self, BgpParserError<&'a [u8]>> { - // Withdrawn routes. Per RFC 4271 these are always IPv4 prefixes; other - // address families are carried in the MP_UNREACH_NLRI path attribute. - let (buf, withdrawn_len) = be_u16(buf)?; - let (buf, withdrawn_buf) = nom::bytes::take(withdrawn_len as usize).parse(buf)?; - let (withdrawn_rem, withdrawn_routes) = - nom::multi::many0(|buf| parse_prefix(AddressFamilyId::Ipv4, buf)) - .parse(withdrawn_buf)?; - if !withdrawn_rem.is_empty() { - return Err(Failure(BgpParserError::Malformed(format!( - "Leftover bytes {:x?} after parsing withdrawn routes", - withdrawn_rem - )))); - } - - // Path attributes. - let (buf, total_pa_len) = be_u16(buf)?; - let (buf, pa_buf) = nom::bytes::take(total_pa_len as usize).parse(buf)?; - let (pa_rem, path_attributes) = - nom::multi::many0(|buf| PathAttribute::from_wire(ctx, buf)).parse(pa_buf)?; - if !pa_rem.is_empty() { - return Err(Failure(BgpParserError::Malformed(format!( - "Leftover bytes {:x?} after parsing path attributes", - pa_rem - )))); - } - - // NLRI. As with withdrawn routes, these are IPv4 prefixes per RFC 4271. - // They occupy the remainder of the UPDATE body; if the caller has - // delimited `buf` correctly then `many0` will consume it entirely. - let (buf, announced_routes) = - nom::multi::many0(|buf| parse_prefix(AddressFamilyId::Ipv4, buf)).parse(buf)?; - if !buf.is_empty() { - return Err(Failure(BgpParserError::Malformed(format!( - "Leftover bytes {:x?} after parsing NLRI", - buf - )))); - } - - Ok(( - buf, - Self { - withdrawn_routes, - path_attributes, - announced_routes, - }, - )) + let decoded = Self::decode(ctx, buf) + .map_err(|e| Failure(BgpParserError::Malformed(e.to_string())))?; + Ok((&[], decoded.update)) } pub fn to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> { @@ -383,7 +354,7 @@ fn prefix_list_wire_len(prefixes: &[IpPrefix]) -> Result { } /// A parser failure as text for a log line. -fn describe(e: &nom::Err>) -> String { +pub(crate) fn describe(e: &nom::Err>) -> String { match e { nom::Err::Incomplete(_) => "truncated".into(), nom::Err::Error(inner) | nom::Err::Failure(inner) => match inner { @@ -410,10 +381,10 @@ bitfield! { pub struct PathAttributeFlags(u8); impl new; u8; - optional, set_optional: 7; - transitive, set_transitive: 6; - partial, set_partial: 5; - extended_length, set_extended_length: 4; + pub optional, set_optional: 7; + pub transitive, set_transitive: 6; + pub partial, set_partial: 5; + pub extended_length, set_extended_length: 4; } impl Deref for PathAttributeFlags { @@ -470,6 +441,38 @@ impl From for PathAttributeDiscriminants { } } +/// The Optional and Transitive flags that the attribute with this type code +/// is defined to have, for the attributes whose definition we know. A +/// received attribute with other values is in error (RFC 7606 §3.c). +pub(crate) fn defined_flags(type_code: u8) -> Option<(bool, bool)> { + const WELL_KNOWN: (bool, bool) = (false, true); + const OPTIONAL_TRANSITIVE: (bool, bool) = (true, true); + const OPTIONAL_NON_TRANSITIVE: (bool, bool) = (true, false); + Some(match type_code { + // ORIGIN, AS_PATH, NEXT_HOP, LOCAL_PREF, ATOMIC_AGGREGATE (RFC 4271). + 1 | 2 | 3 | 5 | 6 => WELL_KNOWN, + // MULTI_EXIT_DISC; ORIGINATOR_ID and CLUSTER_LIST (RFC 4456); + // MP_REACH_NLRI and MP_UNREACH_NLRI (RFC 4760). + 4 | 9 | 10 | 14 | 15 => OPTIONAL_NON_TRANSITIVE, + // AGGREGATOR, COMMUNITIES, EXTENDED COMMUNITIES, AS4_PATH, + // AS4_AGGREGATOR, LARGE_COMMUNITY. + 7 | 8 | 16 | 17 | 18 | 32 => OPTIONAL_TRANSITIVE, + _ => return None, + }) +} + +/// A parsed value, provided the parser used all of it. +fn whole((rest, parsed): (&[u8], T)) -> Result>> { + if rest.is_empty() { + Ok(parsed) + } else { + Err(Failure(BgpParserError::Malformed(format!( + "{} octets left over", + rest.len() + )))) + } +} + impl PathAttribute { fn discriminant(&self) -> u8 { match self { @@ -494,41 +497,14 @@ impl PathAttribute { } } - fn optional(&self) -> bool { + /// Whether the Optional and Transitive flags are set on this attribute. + fn optional_and_transitive(&self) -> (bool, bool) { match self { - PathAttribute::Origin(_) => false, - PathAttribute::AsPath(_) => false, - PathAttribute::NextHop(_) => false, - PathAttribute::MultiExitDisc(_) => true, - PathAttribute::LocalPref(_) => false, - PathAttribute::AtomicAggregate(_) => true, - PathAttribute::Aggregator(_) => true, - PathAttribute::Communities(_) => true, - PathAttribute::MpReachNlri(_) => true, - PathAttribute::MpUnreachNlri(_) => true, - PathAttribute::ExtendedCommunities(_) => true, - PathAttribute::As4Path(_) => true, - PathAttribute::LargeCommunities(_) => true, - PathAttribute::UnknownPathAttribute { flags, .. } => flags.optional(), - } - } - - fn transitive(&self) -> bool { - match self { - PathAttribute::Origin(_) => true, - PathAttribute::AsPath(_) => true, - PathAttribute::NextHop(_) => true, - PathAttribute::MultiExitDisc(_) => false, - PathAttribute::LocalPref(_) => true, - PathAttribute::AtomicAggregate(_) => true, - PathAttribute::Aggregator(_) => false, - PathAttribute::Communities(_) => true, - PathAttribute::MpReachNlri(_) => false, - PathAttribute::MpUnreachNlri(_) => false, - PathAttribute::ExtendedCommunities(_) => true, - PathAttribute::As4Path(_) => true, - PathAttribute::LargeCommunities(_) => true, - PathAttribute::UnknownPathAttribute { flags, .. } => flags.transitive(), + PathAttribute::UnknownPathAttribute { flags, .. } => { + (flags.optional(), flags.transitive()) + } + known => defined_flags(known.discriminant()) + .expect("every attribute with a variant has defined flags"), } } @@ -572,61 +548,73 @@ impl PathAttribute { length: u16, ) -> IResult<&'a [u8], Self, BgpParserError<&'a [u8]>> { let (buf, pa_buf) = nom::bytes::take(length).parse(buf)?; + Ok((buf, Self::parse_value(ctx, discriminant, pa_buf)?)) + } + /// Parses the value of an attribute with a variant. `pa_buf` is exactly + /// the value, and all of it has to be used. + pub(crate) fn parse_value<'a>( + ctx: &ParserContext, + discriminant: PathAttributeDiscriminants, + pa_buf: &'a [u8], + ) -> Result>> { let attr: PathAttribute = match discriminant { PathAttributeDiscriminants::Origin => { - PathAttribute::Origin(OriginPathAttribute::from_wire(ctx, pa_buf)?.1) + PathAttribute::Origin(whole(OriginPathAttribute::from_wire(ctx, pa_buf)?)?) } PathAttributeDiscriminants::AsPath => { - PathAttribute::AsPath(AsPathAttribute::from_wire(ctx, pa_buf)?.1) + PathAttribute::AsPath(whole(AsPathAttribute::from_wire(ctx, pa_buf)?)?) } PathAttributeDiscriminants::NextHop => { - PathAttribute::NextHop(NextHopPathAttribute::from_wire(ctx, pa_buf)?.1) - } - PathAttributeDiscriminants::MultiExitDisc => { - PathAttribute::MultiExitDisc(MultiExitDiscPathAttribute::from_wire(ctx, pa_buf)?.1) + PathAttribute::NextHop(whole(NextHopPathAttribute::from_wire(ctx, pa_buf)?)?) } + PathAttributeDiscriminants::MultiExitDisc => PathAttribute::MultiExitDisc(whole( + MultiExitDiscPathAttribute::from_wire(ctx, pa_buf)?, + )?), PathAttributeDiscriminants::LocalPref => { - PathAttribute::LocalPref(LocalPrefPathAttribute::from_wire(ctx, pa_buf)?.1) + PathAttribute::LocalPref(whole(LocalPrefPathAttribute::from_wire(ctx, pa_buf)?)?) } - PathAttributeDiscriminants::AtomicAggregate => PathAttribute::AtomicAggregate( - AtomicAggregatePathAttribute::from_wire(ctx, pa_buf)?.1, - ), + PathAttributeDiscriminants::AtomicAggregate => PathAttribute::AtomicAggregate(whole( + AtomicAggregatePathAttribute::from_wire(ctx, pa_buf)?, + )?), PathAttributeDiscriminants::Aggregator => { - PathAttribute::Aggregator(AggregatorPathAttribute::from_wire(ctx, pa_buf)?.1) - } - PathAttributeDiscriminants::Communities => { - PathAttribute::Communities(CommunitiesPathAttribute::from_wire(ctx, pa_buf)?.1) - } - PathAttributeDiscriminants::MpReachNlri => { - PathAttribute::MpReachNlri(MpReachNlriPathAttribute::from_wire(ctx, pa_buf)?.1) - } - PathAttributeDiscriminants::MpUnreachNlri => { - PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute::from_wire(ctx, pa_buf)?.1) + PathAttribute::Aggregator(whole(AggregatorPathAttribute::from_wire(ctx, pa_buf)?)?) } + PathAttributeDiscriminants::Communities => PathAttribute::Communities(whole( + CommunitiesPathAttribute::from_wire(ctx, pa_buf)?, + )?), + PathAttributeDiscriminants::MpReachNlri => PathAttribute::MpReachNlri(whole( + MpReachNlriPathAttribute::from_wire(ctx, pa_buf)?, + )?), + PathAttributeDiscriminants::MpUnreachNlri => PathAttribute::MpUnreachNlri(whole( + MpUnreachNlriPathAttribute::from_wire(ctx, pa_buf)?, + )?), PathAttributeDiscriminants::ExtendedCommunities => PathAttribute::ExtendedCommunities( - ExtendedCommunitiesPathAttribute::from_wire(ctx, pa_buf)?.1, + whole(ExtendedCommunitiesPathAttribute::from_wire(ctx, pa_buf)?)?, ), PathAttributeDiscriminants::As4Path => { - PathAttribute::As4Path(AsPathAttribute::from_wire_with_width(true, pa_buf)?.1) + PathAttribute::As4Path(whole(AsPathAttribute::from_wire_with_width(true, pa_buf)?)?) + } + PathAttributeDiscriminants::LargeCommunities => PathAttribute::LargeCommunities(whole( + LargeCommunitiesPathAttribute::from_wire(ctx, pa_buf)?, + )?), + PathAttributeDiscriminants::UnknownPathAttribute => { + return Err(Failure(BgpParserError::Malformed( + "an unknown attribute has no value parser".into(), + ))); } - PathAttributeDiscriminants::LargeCommunities => PathAttribute::LargeCommunities( - LargeCommunitiesPathAttribute::from_wire(ctx, pa_buf)?.1, - ), - PathAttributeDiscriminants::UnknownPathAttribute => unreachable!( - "parse_known_path_attribute must never be called with an unknown attribute" - ), }; - Ok((buf, attr)) + Ok(attr) } pub fn to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> { macro_rules! write_path_attribute { ($out: expr, $attribute: expr) => { let wire_len = $attribute.wire_len(ctx)?; + let (optional, transitive) = self.optional_and_transitive(); let attr_flags = PathAttributeFlags::new( - self.optional(), - self.transitive(), + optional, + transitive, /*partial=*/ false, wire_len > 255, ); @@ -865,6 +853,19 @@ impl AsPathAttribute { let parse_segment = |buf| -> IResult<&[u8], AsPathSegment, BgpParserError<&[u8]>> { let (buf, typ) = be_u8(buf)?; let (buf, len) = be_u8(buf)?; + // AS_SET and AS_SEQUENCE. The confederation segment types of + // RFC 5065 are an error from a peer outside a confederation, + // and this speaker is never inside one. + if typ != 1 && typ != 2 { + return Err(Failure(BgpParserError::Malformed(format!( + "unrecognised AS path segment type {typ}" + )))); + } + if len == 0 { + return Err(Failure(BgpParserError::Malformed( + "AS path segment with no AS numbers".into(), + ))); + } let count = usize::from(len); let (buf, path): (_, Vec) = if four_octet { nom::multi::many_m_n(count, count, be_u32).parse(buf)? @@ -882,7 +883,15 @@ impl AsPathAttribute { )) }; - let (buf, segments) = nom::multi::many0(parse_segment).parse(buf)?; + // Not many0: that stops quietly at a segment it cannot read, and a + // truncated segment is an error. + let mut segments = Vec::new(); + let mut buf = buf; + while !buf.is_empty() { + let (rest, segment) = parse_segment(buf)?; + segments.push(segment); + buf = rest; + } Ok((buf, Self { segments })) } @@ -1101,7 +1110,8 @@ impl CommunitiesPathAttribute { _: &ParserContext, buf: &'a [u8], ) -> IResult<&'a [u8], Self, BgpParserError<&'a [u8]>> { - let (buf, values) = nom::multi::many0(|i| (be_u16, be_u16).parse(i)).parse(buf)?; + // At least one: an empty COMMUNITIES is malformed (RFC 7606 §7.8). + let (buf, values) = nom::multi::many1(|i| (be_u16, be_u16).parse(i)).parse(buf)?; Ok((buf, CommunitiesPathAttribute(values))) } @@ -1455,7 +1465,7 @@ impl NlriNextHop { } // parse_prefix is a helper function that implements an NLRI parser for the given AFI. -fn parse_prefix( +pub(crate) fn parse_prefix( afi: AddressFamilyId, buf: &[u8], ) -> IResult<&[u8], IpPrefix, BgpParserError<&[u8]>> { @@ -1729,7 +1739,7 @@ mod tests { fn default_v6_context() -> ParserContext { ParserContext { four_octet_asn: Some(true), - address_family: Some(AddressFamilyId::Ipv6), + ibgp: false, } } @@ -1812,7 +1822,7 @@ mod tests { // Atomic Aggregate Path Attribute. test_path_attribute_roundtrip!( test_atomic_aggregate, - &[0xc0, 0x06, 0x00], + &[0x40, 0x06, 0x00], PathAttribute::AtomicAggregate(AtomicAggregatePathAttribute {}) ); @@ -1820,7 +1830,7 @@ mod tests { test_path_attribute_roundtrip!( test_aggregator, &[ - 0x80, 0x07, 0x08, 0x00, 0x00, 0xfd, 0xe8, 0xc0, 0xa8, 0x01, 0x01 + 0xc0, 0x07, 0x08, 0x00, 0x00, 0xfd, 0xe8, 0xc0, 0xa8, 0x01, 0x01 ], PathAttribute::Aggregator(AggregatorPathAttribute { asn: 65000, @@ -2106,11 +2116,12 @@ mod tests { #[test] fn bits_past_the_prefix_length_are_cleared() { let ctx = default_v6_context(); - // 10.255.0.0/12 as sent, with the low four bits of the second octet set. - let body = [0, 0, 0, 0, 12, 10, 0xff]; + // 10.255.0.0/12 withdrawn, with the low four bits of the second + // octet set. + let body = [0, 3, 12, 10, 0xff, 0, 0]; let (_, update) = UpdateMessage::from_wire(&ctx, &body).unwrap(); - assert_eq!(update.announced_routes, vec![v4_prefix(12, &[10, 0xf0])]); - assert_eq!(update.announced_routes[0].to_string(), "10.240.0.0/12"); + assert_eq!(update.withdrawn_routes, vec![v4_prefix(12, &[10, 0xf0])]); + assert_eq!(update.withdrawn_routes[0].to_string(), "10.240.0.0/12"); } /// An extended community is eight octets whatever its type. The opaque @@ -2154,7 +2165,7 @@ mod tests { fn aggregator_is_six_octets_on_a_two_octet_session() { let two_octet = ParserContext { four_octet_asn: Some(false), - address_family: None, + ibgp: false, }; let wire = [0xfd, 0xe8, 192, 0, 2, 1]; let (rest, parsed) = AggregatorPathAttribute::from_wire(&two_octet, &wire).unwrap(); @@ -2231,11 +2242,11 @@ mod tests { }; let two_octet = ParserContext { four_octet_asn: Some(false), - address_family: None, + ibgp: false, }; let four_octet = ParserContext { four_octet_asn: Some(true), - address_family: None, + ibgp: false, }; let wire = |attribute: &PathAttribute, ctx: &ParserContext| { let mut out = BytesMut::new(); diff --git a/crates/packet/src/open.rs b/crates/packet/src/open.rs index 8147059..229074f 100644 --- a/crates/packet/src/open.rs +++ b/crates/packet/src/open.rs @@ -904,7 +904,7 @@ mod tests { }; use bgp_primitives::{ - constants::{AddressFamilyId, Afi, Safi}, + constants::{Afi, Safi}, parser::ParserContext, }; @@ -928,7 +928,7 @@ mod tests { fn default_v6_context() -> ParserContext { ParserContext { four_octet_asn: Some(true), - address_family: Some(AddressFamilyId::Ipv6), + ibgp: false, } } diff --git a/crates/packet/src/update_decode.rs b/crates/packet/src/update_decode.rs new file mode 100644 index 0000000..15a1230 --- /dev/null +++ b/crates/packet/src/update_decode.rs @@ -0,0 +1,944 @@ +//! Decoding of UPDATE messages with the error handling of RFC 7606. +//! +//! RFC 4271 answered every malformed UPDATE by resetting the session, which +//! withdraws every route learned over it. One bad attribute, often +//! originated by a router several ASes away and passed on faithfully by a +//! peer that did not look at it, could take down a session carrying a full +//! table. RFC 7606 keeps the session where that is safe: +//! +//! - **Treat-as-withdraw.** The routes the UPDATE announces are handled as +//! if it had withdrawn them. Wrong attributes must not be used, and +//! keeping the routes' previous attributes would be wrong too. +//! - **Attribute discard.** The attribute is dropped and the routes are +//! kept, for the few attributes that have no effect on route selection. +//! - **Session reset** remains for errors that leave the prefixes +//! themselves in doubt: if we cannot tell which routes a message is +//! about, we cannot withdraw them. +//! +//! [`UpdateMessage::decode`] applies this. It returns the UPDATE as it is to +//! be acted on, with treat-as-withdraw and attribute discard already carried +//! out, together with a report of what was wrong, for the log. + +use std::collections::HashSet; + +use bgp_primitives::constants::AddressFamilyId; +use bgp_primitives::ip_prefix::IpPrefix; +use bgp_primitives::parser::{BgpParserError, ParserContext}; + +use crate::errors::{BgpError, DecodeError, UpdateMsgSubcode}; +use crate::message::{ + MpUnreachNlriPathAttribute, PathAttribute, PathAttributeDiscriminants, PathAttributeFlags, + UpdateMessage, defined_flags, describe, parse_prefix, +}; + +/// How an error in an UPDATE was handled, short of resetting the session. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Handling { + /// The routes the UPDATE announced were withdrawn instead. + TreatAsWithdraw, + /// The attribute was dropped; the routes were kept. + AttributeDiscard, +} + +impl Handling { + pub fn as_str(self) -> &'static str { + match self { + Self::TreatAsWithdraw => "treat-as-withdraw", + Self::AttributeDiscard => "attribute-discard", + } + } +} + +/// Something wrong in an UPDATE that the session survived. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct UpdateError { + pub handling: Handling, + /// Type code of the attribute at fault, when it is one attribute. + pub attribute: Option, + pub detail: String, +} + +/// An UPDATE as it is to be acted on, and what was wrong with it as sent. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DecodedUpdate { + /// The UPDATE after treat-as-withdraw and attribute discard. + pub update: UpdateMessage, + /// Empty when the UPDATE was well formed. + pub errors: Vec, +} + +const ORIGIN: u8 = 1; +const AS_PATH: u8 = 2; +const NEXT_HOP: u8 = 3; +const LOCAL_PREF: u8 = 5; +const ATOMIC_AGGREGATE: u8 = 6; +const AGGREGATOR: u8 = 7; +const ORIGINATOR_ID: u8 = 9; +const CLUSTER_LIST: u8 = 10; +const MP_REACH_NLRI: u8 = 14; +const MP_UNREACH_NLRI: u8 = 15; +const AS4_PATH: u8 = 17; +const AS4_AGGREGATOR: u8 = 18; + +/// How a malformed value of the attribute with this type code is handled +/// (RFC 7606 §7, and RFC 6793 §6 for the AS4 attributes). `None` means the +/// session is reset: an error inside MP_REACH_NLRI or MP_UNREACH_NLRI leaves +/// it unknown which routes the UPDATE is about. +fn handling_of_malformed(type_code: u8) -> Option { + match type_code { + MP_REACH_NLRI | MP_UNREACH_NLRI => None, + // These do not affect which route is selected. + ATOMIC_AGGREGATE | AGGREGATOR | AS4_PATH | AS4_AGGREGATOR => { + Some(Handling::AttributeDiscard) + } + _ => Some(Handling::TreatAsWithdraw), + } +} + +fn reset(subcode: UpdateMsgSubcode, data: Vec, detail: String) -> DecodeError { + DecodeError { + notification: BgpError::UpdateMsg(subcode), + data, + detail, + } +} + +/// Parses a field that is nothing but prefixes. An error here cannot be +/// recovered from: the prefixes are what treat-as-withdraw would act on. +fn prefixes(field: &str, mut buf: &[u8]) -> Result, DecodeError> { + let mut prefixes = Vec::new(); + while !buf.is_empty() { + let (rest, prefix) = parse_prefix(AddressFamilyId::Ipv4, buf).map_err(|e| { + reset( + UpdateMsgSubcode::InvalidNetworkField, + vec![], + format!("malformed {field}: {}", describe(&e)), + ) + })?; + prefixes.push(prefix); + buf = rest; + } + Ok(prefixes) +} + +impl UpdateMessage { + /// Decodes the body of an UPDATE message (what follows the 19-octet + /// header) with the error handling of RFC 7606. + /// + /// `Err` is an error that resets the session, with the NOTIFICATION to + /// send. Every other error is handled here and listed in the result. + pub fn decode(ctx: &ParserContext, body: &[u8]) -> Result { + // The two length fields frame the message. If they do not fit, there + // is no telling where anything is (RFC 7606 §3.e). + let framing = |detail: &str| { + reset( + UpdateMsgSubcode::MalformedAttrs, + vec![], + format!("malformed UPDATE: {detail}"), + ) + }; + let split_field = |buf: &'_ [u8], what: &str| -> Result<(usize, usize), DecodeError> { + let length = buf + .get(..2) + .map(|l| usize::from(u16::from_be_bytes([l[0], l[1]]))) + .ok_or_else(|| framing(&format!("no room for the {what} length")))?; + if buf.len() - 2 < length { + return Err(framing(&format!( + "{what} length {length} exceeds the message" + ))); + } + Ok((2, 2 + length)) + }; + let (start, end) = split_field(body, "withdrawn routes")?; + let withdrawn_buf = &body[start..end]; + let body = &body[end..]; + let (start, end) = split_field(body, "path attribute")?; + let attributes_buf = &body[start..end]; + let nlri_buf = &body[end..]; + + let withdrawn_routes = prefixes("withdrawn routes", withdrawn_buf)?; + let announced_routes = prefixes("NLRI", nlri_buf)?; + + let mut errors = Vec::new(); + let path_attributes = attributes(ctx, attributes_buf, &mut errors)?; + let mut update = UpdateMessage { + withdrawn_routes, + path_attributes, + announced_routes, + }; + + if !errors + .iter() + .any(|e| e.handling == Handling::TreatAsWithdraw) + && let Some(missing) = missing_mandatory(ctx, &update) + { + errors.push(UpdateError { + handling: Handling::TreatAsWithdraw, + attribute: Some(missing), + detail: format!("mandatory attribute {missing} is missing"), + }); + } + if errors + .iter() + .any(|e| e.handling == Handling::TreatAsWithdraw) + { + update = as_withdrawal(update); + } + Ok(DecodedUpdate { update, errors }) + } +} + +/// Reads the path attributes, leaving out the ones that are discarded. +fn attributes( + ctx: &ParserContext, + mut buf: &[u8], + errors: &mut Vec, +) -> Result, DecodeError> { + let mut attributes = Vec::new(); + let mut seen = HashSet::new(); + let mut error = |handling, attribute: Option, detail: String| { + errors.push(UpdateError { + handling, + attribute, + detail, + }) + }; + + while !buf.is_empty() { + // An attribute that does not fit in what is left of the attributes + // (RFC 7606 §4). Where the next one would start is unknown, so the + // rest is not read. Routes found so far are withdrawn. + let header = match buf { + [flags, type_code, rest @ ..] => { + let flags = PathAttributeFlags(*flags); + match (flags.extended_length(), rest) { + (true, [hi, lo, rest @ ..]) => Some(( + flags, + *type_code, + usize::from(u16::from_be_bytes([*hi, *lo])), + rest, + )), + (false, [length, rest @ ..]) => { + Some((flags, *type_code, usize::from(*length), rest)) + } + _ => None, + } + } + _ => None, + }; + let Some((flags, type_code, length, rest)) = header else { + error( + Handling::TreatAsWithdraw, + None, + format!("{} octets are too few for an attribute", buf.len()), + ); + break; + }; + if rest.len() < length { + error( + Handling::TreatAsWithdraw, + Some(type_code), + format!( + "attribute {type_code} has length {length} but only {} octets remain", + rest.len() + ), + ); + break; + } + let (value, rest) = rest.split_at(length); + let attribute_wire = &buf[..buf.len() - rest.len()]; + buf = rest; + + // RFC 7606 §3.g: a second MP_REACH_NLRI or MP_UNREACH_NLRI makes it + // unclear which routes are meant; a second anything else is ignored. + if !seen.insert(type_code) { + if matches!(type_code, MP_REACH_NLRI | MP_UNREACH_NLRI) { + return Err(reset( + UpdateMsgSubcode::MalformedAttrs, + vec![], + format!("attribute {type_code} appears more than once"), + )); + } + error( + Handling::AttributeDiscard, + Some(type_code), + format!("attribute {type_code} appears more than once; kept the first"), + ); + continue; + } + + let Some((optional, transitive)) = defined_flags(type_code) else { + // An attribute we have no definition for. One marked well-known + // is one every speaker is required to understand (RFC 4271 + // §6.3); an optional one is carried along as it is. + if !flags.optional() { + return Err(reset( + UpdateMsgSubcode::UnrecognizedWellKnownAttr, + attribute_wire.to_vec(), + format!("unrecognised well-known attribute {type_code}"), + )); + } + attributes.push(PathAttribute::UnknownPathAttribute { + flags, + type_code, + payload: value.to_vec(), + }); + continue; + }; + let flags_are_wrong = flags.optional() != optional || flags.transitive() != transitive; + if flags_are_wrong { + // RFC 7606 §3.c. The value is still read below if it is one that + // names routes, since those have to be withdrawn. + error( + Handling::TreatAsWithdraw, + Some(type_code), + format!( + "attribute {type_code} has flags {:#04x} that conflict with its definition", + flags.0 + ), + ); + if !matches!(type_code, MP_REACH_NLRI | MP_UNREACH_NLRI) { + continue; + } + } + + // Attributes that only mean something between internal peers are + // dropped when an external one sends them (RFC 7606 §7.5, §7.9, + // §7.10), well formed or not. + if !ctx.ibgp && matches!(type_code, LOCAL_PREF | ORIGINATOR_ID | CLUSTER_LIST) { + error( + Handling::AttributeDiscard, + Some(type_code), + format!("attribute {type_code} from an external peer"), + ); + continue; + } + // A speaker with four-octet AS numbers has no use for AS4_PATH and + // AS4_AGGREGATOR and must not send them to another (RFC 6793 §6). + if ctx.four_octet_asn == Some(true) && matches!(type_code, AS4_PATH | AS4_AGGREGATOR) { + error( + Handling::AttributeDiscard, + Some(type_code), + format!("attribute {type_code} on a session with four-octet AS numbers"), + ); + continue; + } + + let parsed = match type_code { + // Known by their definition but not modelled: kept as bytes, + // once their length has been checked. + ORIGINATOR_ID | CLUSTER_LIST | AS4_AGGREGATOR => { + let well_formed = match type_code { + ORIGINATOR_ID => value.len() == 4, + CLUSTER_LIST => !value.is_empty() && value.len().is_multiple_of(4), + _ => value.len() == 8, + }; + if well_formed { + Ok(PathAttribute::UnknownPathAttribute { + flags, + type_code, + payload: value.to_vec(), + }) + } else { + Err(format!("a length of {} is not valid", value.len())) + } + } + _ => { + PathAttribute::parse_value(ctx, PathAttributeDiscriminants::from(type_code), value) + .map_err(|e: nom::Err>| describe(&e)) + } + }; + match parsed { + Ok(attribute) => attributes.push(attribute), + Err(detail) => match handling_of_malformed(type_code) { + Some(handling) => error( + handling, + Some(type_code), + format!("malformed attribute {type_code}: {detail}"), + ), + None => { + return Err(reset( + UpdateMsgSubcode::OptionalAttribute, + attribute_wire.to_vec(), + format!("malformed attribute {type_code}: {detail}"), + )); + } + }, + } + } + Ok(attributes) +} + +/// The type code of a mandatory attribute that an UPDATE announcing routes +/// lacks (RFC 4271 §5, RFC 4760 §3, RFC 7606 §3.d). An UPDATE that only +/// withdraws needs no attributes. +fn missing_mandatory(ctx: &ParserContext, update: &UpdateMessage) -> Option { + let has = |type_code: u8| { + update.path_attributes.iter().any(|a| { + matches!( + (type_code, a), + (ORIGIN, PathAttribute::Origin(_)) + | (AS_PATH, PathAttribute::AsPath(_)) + | (NEXT_HOP, PathAttribute::NextHop(_)) + | (LOCAL_PREF, PathAttribute::LocalPref(_)) + ) + }) + }; + let in_body = !update.announced_routes.is_empty(); + let in_mp_reach = update + .path_attributes + .iter() + .any(|a| matches!(a, PathAttribute::MpReachNlri(mp) if !mp.prefixes.is_empty())); + if !in_body && !in_mp_reach { + return None; + } + let mut required = vec![ORIGIN, AS_PATH]; + if in_body { + // Routes in MP_REACH_NLRI carry their next hop there. + required.push(NEXT_HOP); + } + if ctx.ibgp { + required.push(LOCAL_PREF); + } + required.into_iter().find(|type_code| !has(*type_code)) +} + +/// `update` with every route it announces withdrawn instead, and no other +/// attributes: they are not to be trusted, and a withdrawal has none. +fn as_withdrawal(update: UpdateMessage) -> UpdateMessage { + let mut withdrawn_routes = update.withdrawn_routes; + withdrawn_routes.extend(update.announced_routes); + + let mut unreach: Vec = Vec::new(); + let mut withdraw = |afi, safi, prefixes: Vec| match unreach + .iter_mut() + .find(|u| u.afi == afi && u.safi == safi) + { + Some(existing) => existing.prefixes.extend(prefixes), + None => unreach.push(MpUnreachNlriPathAttribute { + afi, + safi, + prefixes, + }), + }; + for attribute in update.path_attributes { + match attribute { + PathAttribute::MpUnreachNlri(mp) => withdraw(mp.afi, mp.safi, mp.prefixes), + PathAttribute::MpReachNlri(mp) => withdraw(mp.afi, mp.safi, mp.prefixes), + _ => {} + } + } + UpdateMessage { + withdrawn_routes, + path_attributes: unreach + .into_iter() + .map(PathAttribute::MpUnreachNlri) + .collect(), + announced_routes: vec![], + } +} + +#[cfg(test)] +mod tests { + use std::net::Ipv4Addr; + + use bgp_primitives::constants::{Afi, AfiSafi, Safi}; + + use super::*; + use crate::message::{ + AsPathAttribute, LocalPrefPathAttribute, MultiExitDiscPathAttribute, NextHopPathAttribute, + OriginPathAttribute, + }; + + const MED: u8 = 4; + const COMMUNITIES: u8 = 8; + const EXTENDED_COMMUNITIES: u8 = 16; + const LARGE_COMMUNITY: u8 = 32; + + const WELL_KNOWN: u8 = 0x40; + const OPTIONAL: u8 = 0x80; + const OPTIONAL_TRANSITIVE: u8 = 0xc0; + + fn ebgp() -> ParserContext { + ParserContext { + four_octet_asn: Some(true), + ibgp: false, + } + } + + fn ibgp() -> ParserContext { + ParserContext { + four_octet_asn: Some(true), + ibgp: true, + } + } + + fn attr(flags: u8, type_code: u8, value: &[u8]) -> Vec { + let mut wire = vec![flags, type_code, value.len() as u8]; + wire.extend_from_slice(value); + wire + } + + fn origin() -> Vec { + attr(WELL_KNOWN, ORIGIN, &[0]) + } + + /// AS_SEQUENCE of AS65001, four octets wide. + fn as_path() -> Vec { + attr(WELL_KNOWN, AS_PATH, &[2, 1, 0, 0, 0xfd, 0xe9]) + } + + fn next_hop() -> Vec { + attr(WELL_KNOWN, NEXT_HOP, &[192, 0, 2, 1]) + } + + fn local_pref() -> Vec { + attr(WELL_KNOWN, LOCAL_PREF, &[0, 0, 0, 100]) + } + + /// The three attributes every IPv4 announcement needs. + fn mandatory() -> Vec> { + vec![origin(), as_path(), next_hop()] + } + + /// 10.1.2.0/24 + const NLRI: [u8; 4] = [24, 10, 1, 2]; + + fn route() -> IpPrefix { + IpPrefix::try_from("10.1.2.0/24").unwrap() + } + + fn body(withdrawn: &[u8], attributes: &[Vec], nlri: &[u8]) -> Vec { + let attributes = attributes.concat(); + let mut body = (withdrawn.len() as u16).to_be_bytes().to_vec(); + body.extend_from_slice(withdrawn); + body.extend_from_slice(&(attributes.len() as u16).to_be_bytes()); + body.extend_from_slice(&attributes); + body.extend_from_slice(nlri); + body + } + + /// An announcement of 10.1.2.0/24 with the mandatory attributes and + /// `extra`. + fn announce_with(ctx: &ParserContext, extra: Vec) -> DecodedUpdate { + let mut attributes = mandatory(); + attributes.push(extra); + UpdateMessage::decode(ctx, &body(&[], &attributes, &NLRI)).unwrap() + } + + fn handled(decoded: &DecodedUpdate) -> Vec<(Handling, Option)> { + decoded + .errors + .iter() + .map(|e| (e.handling, e.attribute)) + .collect() + } + + #[track_caller] + fn assert_withdrawn(decoded: &DecodedUpdate, attribute: Option, case: &str) { + assert_eq!( + handled(decoded), + [(Handling::TreatAsWithdraw, attribute)], + "{case}" + ); + assert_eq!(decoded.update.withdrawn_routes, [route()], "{case}"); + assert!(decoded.update.announced_routes.is_empty(), "{case}"); + assert!(decoded.update.path_attributes.is_empty(), "{case}"); + } + + #[test] + fn a_well_formed_update_is_left_alone() { + let decoded = announce_with(&ebgp(), attr(OPTIONAL, MED, &[0, 0, 0, 50])); + assert!(decoded.errors.is_empty()); + assert_eq!(decoded.update.announced_routes, [route()]); + assert_eq!( + decoded.update.path_attributes, + [ + PathAttribute::Origin(OriginPathAttribute::IGP), + AsPathAttribute::from_asns(vec![65001]), + PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::new(192, 0, 2, 1))), + PathAttribute::MultiExitDisc(MultiExitDiscPathAttribute(50)), + ] + ); + } + + #[test] + fn an_update_that_announces_nothing_needs_no_attributes() { + // End-of-RIB, and a plain withdrawal. + for wire in [body(&[], &[], &[]), body(&NLRI, &[], &[])] { + let decoded = UpdateMessage::decode(&ebgp(), &wire).unwrap(); + assert!(decoded.errors.is_empty()); + } + } + + #[test] + fn a_malformed_attribute_withdraws_the_routes_and_keeps_the_session() { + let cases: Vec<(&str, u8, Vec)> = vec![ + ("MED of three octets", MED, attr(OPTIONAL, MED, &[0, 0, 50])), + ( + "COMMUNITIES with nothing in it", + COMMUNITIES, + attr(OPTIONAL_TRANSITIVE, COMMUNITIES, &[]), + ), + ( + "COMMUNITIES that is not a multiple of four", + COMMUNITIES, + attr(OPTIONAL_TRANSITIVE, COMMUNITIES, &[0xfd, 0xe8, 0, 100, 1]), + ), + ( + "EXTENDED COMMUNITIES of seven octets", + EXTENDED_COMMUNITIES, + attr(OPTIONAL_TRANSITIVE, EXTENDED_COMMUNITIES, &[0; 7]), + ), + ( + "LARGE_COMMUNITY of eleven octets", + LARGE_COMMUNITY, + attr(OPTIONAL_TRANSITIVE, LARGE_COMMUNITY, &[0; 11]), + ), + ( + "MED marked transitive", + MED, + attr(OPTIONAL_TRANSITIVE, MED, &[0, 0, 0, 50]), + ), + ]; + for (case, type_code, extra) in cases { + let decoded = announce_with(&ebgp(), extra); + assert_withdrawn(&decoded, Some(type_code), case); + } + } + + #[test] + fn a_malformed_mandatory_attribute_withdraws_the_routes() { + // Each replaces the good attribute of its type. + let cases: Vec<(&str, u8, Vec)> = vec![ + ("ORIGIN of 5", ORIGIN, attr(WELL_KNOWN, ORIGIN, &[5])), + ( + "ORIGIN of two octets", + ORIGIN, + attr(WELL_KNOWN, ORIGIN, &[0, 0]), + ), + ( + "ORIGIN marked optional", + ORIGIN, + attr(OPTIONAL_TRANSITIVE, ORIGIN, &[0]), + ), + ( + "AS_PATH with a confederation segment", + AS_PATH, + attr(WELL_KNOWN, AS_PATH, &[3, 1, 0, 0, 0xfd, 0xe9]), + ), + ( + "AS_PATH with an empty segment", + AS_PATH, + attr(WELL_KNOWN, AS_PATH, &[2, 0]), + ), + ( + "AS_PATH cut short", + AS_PATH, + attr(WELL_KNOWN, AS_PATH, &[2, 2, 0, 0, 0xfd, 0xe9]), + ), + ( + "NEXT_HOP of three octets", + NEXT_HOP, + attr(WELL_KNOWN, NEXT_HOP, &[192, 0, 2]), + ), + ]; + for (case, type_code, bad) in cases { + let attributes: Vec> = mandatory() + .into_iter() + .map(|good| { + if good[1] == type_code { + bad.clone() + } else { + good + } + }) + .collect(); + let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap(); + assert_withdrawn(&decoded, Some(type_code), case); + } + } + + #[test] + fn a_missing_mandatory_attribute_withdraws_the_routes() { + for missing in [ORIGIN, AS_PATH, NEXT_HOP] { + let attributes: Vec> = mandatory() + .into_iter() + .filter(|a| a[1] != missing) + .collect(); + let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap(); + assert_withdrawn(&decoded, Some(missing), &format!("without {missing}")); + } + } + + #[test] + fn an_attribute_that_overruns_the_attributes_withdraws_what_was_found() { + // A MED that claims 200 octets. + let mut attributes = mandatory(); + attributes.push(vec![OPTIONAL, MED, 200, 0, 0]); + let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap(); + assert_withdrawn(&decoded, Some(MED), "overrun"); + + // Two stray octets after the last attribute: not even a header. + let mut attributes = mandatory(); + attributes.push(vec![OPTIONAL, MED]); + let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap(); + assert_withdrawn(&decoded, None, "stray octets"); + } + + #[test] + fn attributes_that_do_not_affect_selection_are_dropped_and_the_route_kept() { + let cases: Vec<(&str, u8, Vec)> = vec![ + ( + "ATOMIC_AGGREGATE with a value", + ATOMIC_AGGREGATE, + attr(WELL_KNOWN, ATOMIC_AGGREGATE, &[1]), + ), + ( + "AGGREGATOR of five octets", + AGGREGATOR, + attr(OPTIONAL_TRANSITIVE, AGGREGATOR, &[0; 5]), + ), + ("LOCAL_PREF from an external peer", LOCAL_PREF, local_pref()), + ( + "ORIGINATOR_ID from an external peer", + ORIGINATOR_ID, + attr(OPTIONAL, ORIGINATOR_ID, &[10, 0, 0, 1]), + ), + ( + "CLUSTER_LIST from an external peer", + CLUSTER_LIST, + attr(OPTIONAL, CLUSTER_LIST, &[10, 0, 0, 1]), + ), + ( + "AS4_PATH between four-octet speakers", + AS4_PATH, + attr(OPTIONAL_TRANSITIVE, AS4_PATH, &[2, 1, 0, 0, 0xfd, 0xe9]), + ), + ("a second ORIGIN", ORIGIN, origin()), + ]; + for (case, type_code, extra) in cases { + let decoded = announce_with(&ebgp(), extra); + assert_eq!( + handled(&decoded), + [(Handling::AttributeDiscard, Some(type_code))], + "{case}" + ); + assert_eq!(decoded.update.announced_routes, [route()], "{case}"); + assert_eq!(decoded.update.path_attributes.len(), 3, "{case}"); + } + } + + #[test] + fn a_repeated_attribute_keeps_its_first_value() { + let mut attributes = mandatory(); + attributes.push(attr(OPTIONAL, MED, &[0, 0, 0, 50])); + attributes.push(attr(OPTIONAL, MED, &[0, 0, 0, 99])); + let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &NLRI)).unwrap(); + assert_eq!(handled(&decoded), [(Handling::AttributeDiscard, Some(MED))]); + assert!( + decoded + .update + .path_attributes + .contains(&PathAttribute::MultiExitDisc(MultiExitDiscPathAttribute( + 50 + ))) + ); + } + + #[test] + fn internal_peers_must_send_local_pref_and_may_send_reflection_attributes() { + let kept = announce_with(&ibgp(), local_pref()); + assert!(kept.errors.is_empty()); + assert!( + kept.update + .path_attributes + .contains(&PathAttribute::LocalPref(LocalPrefPathAttribute(100))) + ); + + let missing = UpdateMessage::decode(&ibgp(), &body(&[], &mandatory(), &NLRI)).unwrap(); + assert_withdrawn(&missing, Some(LOCAL_PREF), "no LOCAL_PREF"); + + let short = announce_with(&ibgp(), attr(WELL_KNOWN, LOCAL_PREF, &[0, 0, 100])); + assert_withdrawn(&short, Some(LOCAL_PREF), "short LOCAL_PREF"); + + let mut attributes = mandatory(); + attributes.push(local_pref()); + attributes.push(attr(OPTIONAL, ORIGINATOR_ID, &[10, 0, 0, 1])); + attributes.push(attr(OPTIONAL, CLUSTER_LIST, &[10, 0, 0, 2, 10, 0, 0, 3])); + let reflected = UpdateMessage::decode(&ibgp(), &body(&[], &attributes, &NLRI)).unwrap(); + assert!(reflected.errors.is_empty()); + assert_eq!(reflected.update.path_attributes.len(), 6); + + let mut attributes = mandatory(); + attributes.push(local_pref()); + attributes.push(attr(OPTIONAL, ORIGINATOR_ID, &[10, 0, 1])); + let bad = UpdateMessage::decode(&ibgp(), &body(&[], &attributes, &NLRI)).unwrap(); + assert_withdrawn(&bad, Some(ORIGINATOR_ID), "short ORIGINATOR_ID"); + } + + #[test] + fn as4_path_is_read_from_a_two_octet_peer_and_dropped_if_malformed() { + let ctx = ParserContext { + four_octet_asn: Some(false), + ibgp: false, + }; + // On this session AS_PATH is two octets wide. + let attributes = |as4_path: Vec| { + vec![ + origin(), + attr(WELL_KNOWN, AS_PATH, &[2, 1, 0x5b, 0xa0]), + next_hop(), + as4_path, + ] + }; + let good = attr( + OPTIONAL_TRANSITIVE, + AS4_PATH, + &[2, 1, 0xfa, 0x56, 0xea, 0x01], + ); + let decoded = UpdateMessage::decode(&ctx, &body(&[], &attributes(good), &NLRI)).unwrap(); + assert!(decoded.errors.is_empty()); + assert!( + decoded + .update + .path_attributes + .iter() + .any(|a| matches!(a, PathAttribute::As4Path(_))) + ); + + let bad = attr(OPTIONAL_TRANSITIVE, AS4_PATH, &[2, 1, 0xfa, 0x56]); + let decoded = UpdateMessage::decode(&ctx, &body(&[], &attributes(bad), &NLRI)).unwrap(); + assert_eq!( + handled(&decoded), + [(Handling::AttributeDiscard, Some(AS4_PATH))] + ); + assert_eq!(decoded.update.announced_routes, [route()]); + } + + /// 2001:db8:1::/48 via 2001:db8::1. + fn mp_reach_v6() -> Vec { + let mut value = vec![0, 2, 1, 16]; + value.extend_from_slice(&[0x20, 0x01, 0x0d, 0xb8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]); + value.extend_from_slice(&[0, 48, 0x20, 0x01, 0x0d, 0xb8, 0x00, 0x01]); + attr(OPTIONAL, MP_REACH_NLRI, &value) + } + + #[test] + fn routes_in_mp_reach_are_withdrawn_too() { + let v6 = IpPrefix::try_from("2001:db8:1::/48").unwrap(); + let withdrawn = |attributes: Vec>, case: &str| { + let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &attributes, &[])).unwrap(); + assert!( + decoded + .errors + .iter() + .any(|e| e.handling == Handling::TreatAsWithdraw), + "{case}" + ); + assert_eq!( + decoded.update.path_attributes, + [PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute { + afi: Afi::IPV6, + safi: Safi::UNICAST, + prefixes: vec![v6.clone()], + })], + "{case}" + ); + }; + + // A well-formed one first: no NEXT_HOP attribute is needed. + let good = vec![mp_reach_v6(), origin(), as_path()]; + let decoded = UpdateMessage::decode(&ebgp(), &body(&[], &good, &[])).unwrap(); + assert!(decoded.errors.is_empty()); + assert!(matches!( + &decoded.update.path_attributes[0], + PathAttribute::MpReachNlri(mp) if mp.family() == AfiSafi::IPV6_UNICAST + )); + + withdrawn( + vec![mp_reach_v6(), attr(WELL_KNOWN, ORIGIN, &[9]), as_path()], + "bad ORIGIN", + ); + withdrawn(vec![mp_reach_v6(), origin()], "no AS_PATH"); + // Wrong flags on MP_REACH_NLRI itself: its routes are still found. + let mut misflagged = mp_reach_v6(); + misflagged[0] = OPTIONAL_TRANSITIVE; + withdrawn(vec![misflagged, origin(), as_path()], "misflagged"); + } + + #[test] + fn errors_that_leave_the_routes_in_doubt_reset_the_session() { + let subcode = |wire: Vec| match UpdateMessage::decode(&ebgp(), &wire) { + Err(e) => (e.notification, e.data), + Ok(decoded) => panic!("expected a reset, got {decoded:?}"), + }; + let update_error = |subcode| BgpError::UpdateMsg(subcode); + + // The lengths that frame the message do not fit it. + assert_eq!( + subcode(vec![0, 9, 24, 10, 1, 2, 0, 0]).0, + update_error(UpdateMsgSubcode::MalformedAttrs) + ); + assert_eq!( + subcode(vec![0, 0, 0, 9, 0x40, 1, 1, 0]).0, + update_error(UpdateMsgSubcode::MalformedAttrs) + ); + assert_eq!( + subcode(vec![0]).0, + update_error(UpdateMsgSubcode::MalformedAttrs) + ); + + // A prefix that cannot be one. + assert_eq!( + subcode(body(&[], &mandatory(), &[33, 10, 1, 2, 3, 4])).0, + update_error(UpdateMsgSubcode::InvalidNetworkField) + ); + assert_eq!( + subcode(body(&[24, 10, 1], &[], &[])).0, + update_error(UpdateMsgSubcode::InvalidNetworkField) + ); + + // Two MP_REACH_NLRI: which routes are meant? + assert_eq!( + subcode(body( + &[], + &[mp_reach_v6(), mp_reach_v6(), origin(), as_path()], + &[] + )) + .0, + update_error(UpdateMsgSubcode::MalformedAttrs) + ); + + // MP_REACH_NLRI with a next hop of the wrong length. + let bad_mp_reach = attr(OPTIONAL, MP_REACH_NLRI, &[0, 2, 1, 3, 1, 2, 3, 0]); + let (error, data) = subcode(body(&[], &[bad_mp_reach.clone(), origin(), as_path()], &[])); + assert_eq!(error, update_error(UpdateMsgSubcode::OptionalAttribute)); + assert_eq!(data, bad_mp_reach); + + // An attribute nobody has defined, marked as one everybody must know. + let unknown = attr(WELL_KNOWN, 99, &[1, 2]); + let mut attributes = mandatory(); + attributes.push(unknown.clone()); + let (error, data) = subcode(body(&[], &attributes, &NLRI)); + assert_eq!( + error, + update_error(UpdateMsgSubcode::UnrecognizedWellKnownAttr) + ); + assert_eq!(data, unknown); + } + + #[test] + fn an_unknown_optional_attribute_is_carried_as_it_came() { + let decoded = announce_with(&ebgp(), attr(0xe0, 99, &[1, 2, 3])); + assert!(decoded.errors.is_empty()); + assert_eq!( + decoded.update.path_attributes[3], + PathAttribute::UnknownPathAttribute { + flags: PathAttributeFlags(0xe0), + type_code: 99, + payload: vec![1, 2, 3], + } + ); + } +} diff --git a/crates/packet/tests/properties.rs b/crates/packet/tests/properties.rs index 7117a23..e1a014f 100644 --- a/crates/packet/tests/properties.rs +++ b/crates/packet/tests/properties.rs @@ -36,13 +36,29 @@ use bytes::BytesMut; use proptest::collection::vec; use proptest::prelude::*; -fn ctx(four_octet_asn: bool) -> ParserContext { - ParserContext { - four_octet_asn: Some(four_octet_asn), - address_family: None, +/// What a session settled that the codec depends on. +#[derive(Debug, Clone, Copy)] +struct Session { + four_octet_asn: bool, + ibgp: bool, +} + +impl Session { + fn ctx(self) -> ParserContext { + ParserContext { + four_octet_asn: Some(self.four_octet_asn), + ibgp: self.ibgp, + } } } +fn session() -> impl Strategy { + any::<(bool, bool)>().prop_map(|(four_octet_asn, ibgp)| Session { + four_octet_asn, + ibgp, + }) +} + // --------------------------------------------------------------------------- // Generators // --------------------------------------------------------------------------- @@ -76,7 +92,8 @@ fn asn(four_octet_asn: bool) -> BoxedStrategy { } fn as_path(asn: BoxedStrategy) -> impl Strategy { - vec((any::(), vec(asn, 0..6)), 0..4).prop_map(|segments| AsPathAttribute { + // A segment has at least one AS number. + vec((any::(), vec(asn, 1..6)), 0..4).prop_map(|segments| AsPathAttribute { segments: segments .into_iter() .map(|(ordered, path)| AsPathSegment { ordered, path }) @@ -88,7 +105,7 @@ fn as_path(asn: BoxedStrategy) -> impl Strategy { /// they are generated from the eight octets they are on the wire. fn extended_community() -> impl Strategy { any::<[u8; 8]>().prop_map(|bytes| { - ExtendedCommunity::from_wire(&ctx(true), &bytes) + ExtendedCommunity::from_wire(&ParserContext::default(), &bytes) .expect("any eight octets are an extended community") .1 }) @@ -141,68 +158,99 @@ fn mp_unreach() -> impl Strategy { prop_oneof![of(AddressFamilyId::Ipv4), of(AddressFamilyId::Ipv6)] } -/// Type codes that have their own variant; an unknown attribute has none of -/// these. -const KNOWN_ATTRIBUTES: [u8; 13] = [1, 2, 3, 4, 5, 6, 7, 8, 14, 15, 16, 17, 32]; +/// Type codes whose definition the decoder knows; an unknown attribute has +/// none of these. +const DEFINED_ATTRIBUTES: [u8; 16] = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 14, 15, 16, 17, 18, 32]; -fn unknown_attribute() -> impl Strategy { - ( - any::().prop_filter("a type code without a variant", |code| { - !KNOWN_ATTRIBUTES.contains(code) +/// Unknown attributes, each with a type code of its own: a repeated +/// attribute is dropped by the decoder. +fn unknown_attributes() -> impl Strategy> { + proptest::collection::btree_map( + any::().prop_filter("a type code without a definition", |code| { + !DEFINED_ATTRIBUTES.contains(code) }), - any::(), - vec(any::(), 0..300), + (any::(), vec(any::(), 0..300)), + 0..3, ) - .prop_map(|(type_code, flags, payload)| { - // Bit 4 selects the two-octet length, which a payload this - // long needs. - let flags = PathAttributeFlags(if payload.len() > 255 { - flags | 0x10 - } else { - flags - }); - PathAttribute::UnknownPathAttribute { - flags, - type_code, - payload, - } - }) + .prop_map(|attributes| { + attributes + .into_iter() + .map(|(type_code, (flags, payload))| { + // Optional (bit 7): an unknown well-known attribute is an + // error. Bit 4 selects the two-octet length, which a payload + // this long needs. + let mut flags = flags | 0x80; + if payload.len() > 255 { + flags |= 0x10; + } + PathAttribute::UnknownPathAttribute { + flags: PathAttributeFlags(flags), + type_code, + payload, + } + }) + .collect() + }) } -fn path_attribute(four_octet_asn: bool) -> impl Strategy { - prop_oneof![ +/// The attributes of a well-formed UPDATE on `session`: the mandatory ones, +/// and any of the others at most once. +fn path_attributes(session: Session) -> impl Strategy> { + let mandatory = ( prop_oneof![ Just(OriginPathAttribute::IGP), Just(OriginPathAttribute::EGP), Just(OriginPathAttribute::INCOMPLETE) - ] - .prop_map(PathAttribute::Origin), - as_path(asn(four_octet_asn)).prop_map(PathAttribute::AsPath), - any::<[u8; 4]>() - .prop_map(|a| PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::from(a)))), - any::().prop_map(|v| PathAttribute::MultiExitDisc(MultiExitDiscPathAttribute(v))), - any::().prop_map(|v| PathAttribute::LocalPref(LocalPrefPathAttribute(v))), - Just(PathAttribute::AtomicAggregate( - AtomicAggregatePathAttribute {} + ], + as_path(asn(session.four_octet_asn)), + any::<[u8; 4]>(), + any::(), + ) + .prop_map(move |(origin, as_path, next_hop, local_pref)| { + let mut attributes = vec![ + PathAttribute::Origin(origin), + PathAttribute::AsPath(as_path), + PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::from(next_hop))), + ]; + // Mandatory between internal peers, dropped from external ones. + if session.ibgp { + attributes.push(PathAttribute::LocalPref(LocalPrefPathAttribute(local_pref))); + } + attributes + }); + let optional = ( + proptest::option::of( + any::().prop_map(|v| PathAttribute::MultiExitDisc(MultiExitDiscPathAttribute(v))), + ), + proptest::option::of(Just(PathAttribute::AtomicAggregate( + AtomicAggregatePathAttribute {}, + ))), + proptest::option::of((asn(session.four_octet_asn), any::<[u8; 4]>()).prop_map( + |(asn, ip)| { + PathAttribute::Aggregator(AggregatorPathAttribute { + asn, + ip: Ipv4Addr::from(ip), + }) + }, )), - (asn(four_octet_asn), any::<[u8; 4]>()).prop_map(|(asn, ip)| { - PathAttribute::Aggregator(AggregatorPathAttribute { - asn, - ip: Ipv4Addr::from(ip), - }) - }), - vec(any::<(u16, u16)>(), 0..8) - .prop_map(|c| PathAttribute::Communities(CommunitiesPathAttribute(c))), - mp_reach().prop_map(PathAttribute::MpReachNlri), - mp_unreach().prop_map(PathAttribute::MpUnreachNlri), - vec(extended_community(), 1..6).prop_map(|extended_communities| { - PathAttribute::ExtendedCommunities(ExtendedCommunitiesPathAttribute { - extended_communities, - }) - }), - // AS4_PATH is four octets wide on every session. - as_path(any::().boxed()).prop_map(PathAttribute::As4Path), - vec(any::<(u32, u32, u32)>(), 1..6).prop_map(|communities| { + proptest::option::of( + vec(any::<(u16, u16)>(), 1..8) + .prop_map(|c| PathAttribute::Communities(CommunitiesPathAttribute(c))), + ), + proptest::option::of(mp_reach().prop_map(PathAttribute::MpReachNlri)), + proptest::option::of(mp_unreach().prop_map(PathAttribute::MpUnreachNlri)), + proptest::option::of( + vec(extended_community(), 1..6).prop_map(|extended_communities| { + PathAttribute::ExtendedCommunities(ExtendedCommunitiesPathAttribute { + extended_communities, + }) + }), + ), + // AS4_PATH is four octets wide, and only a two-octet speaker sends + // it. + proptest::option::of(as_path(any::().boxed()).prop_map(PathAttribute::As4Path)) + .prop_map(move |as4_path| as4_path.filter(|_| !session.four_octet_asn)), + proptest::option::of(vec(any::<(u32, u32, u32)>(), 1..6).prop_map(|communities| { PathAttribute::LargeCommunities(LargeCommunitiesPathAttribute { communities: communities .into_iter() @@ -213,15 +261,36 @@ fn path_attribute(four_octet_asn: bool) -> impl Strategy }) .collect(), }) - }), - unknown_attribute(), - ] + })), + unknown_attributes(), + ); + (mandatory, optional).prop_map(|(mut attributes, optional)| { + let (med, atomic, aggregator, communities, reach, unreach, extended, as4, large, unknown) = + optional; + attributes.extend( + [ + med, + atomic, + aggregator, + communities, + reach, + unreach, + extended, + as4, + large, + ] + .into_iter() + .flatten(), + ); + attributes.extend(unknown); + attributes + }) } -fn update(four_octet_asn: bool) -> impl Strategy { +fn update(session: Session) -> impl Strategy { ( vec(prefix(AddressFamilyId::Ipv4), 0..5), - vec(path_attribute(four_octet_asn), 0..8), + path_attributes(session), vec(prefix(AddressFamilyId::Ipv4), 0..5), ) .prop_map( @@ -360,10 +429,10 @@ fn notification() -> impl Strategy { ) } -fn message(four_octet_asn: bool) -> impl Strategy { +fn message(session: Session) -> impl Strategy { prop_oneof![ open().prop_map(Message::Open), - update(four_octet_asn).prop_map(Message::Update), + update(session).prop_map(Message::Update), notification().prop_map(Message::Notification), Just(Message::KeepAlive), (afi(), any::(), safi()).prop_map(|(afi, subtype, safi)| { @@ -372,21 +441,25 @@ fn message(four_octet_asn: bool) -> impl Strategy { ] } -/// A session's AS number width, and a message that fits it. -fn session_and_message() -> impl Strategy { - any::().prop_flat_map(|four_octet_asn| (Just(four_octet_asn), message(four_octet_asn))) +/// A session, and a well-formed message that fits it. +fn session_and_message() -> impl Strategy { + session().prop_flat_map(|session| (Just(session), message(session))) } // --------------------------------------------------------------------------- // Properties // --------------------------------------------------------------------------- -/// What must hold for anything the decoder accepts: it can be shown, -/// written, and read back as itself. -fn check_accepted(ctx: &ParserContext, msg: &Message) -> Result<(), TestCaseError> { +/// Decodes `wire` and checks what must hold for anything the decoder +/// accepts: it can be shown and written, and unless the decoder had to +/// repair it, it reads back as itself. +fn check_decoding(ctx: &ParserContext, wire: &[u8]) -> Result<(), TestCaseError> { + let Ok((msg, repairs)) = Message::decode_with_report(ctx, wire) else { + return Ok(()); + }; // Display of what a peer sent must not be able to panic: it is logged. - let _ = format!("{msg:?}"); - if let Message::Update(update) = msg { + let _ = format!("{msg:?} {repairs:?}"); + if let Message::Update(update) = &msg { for prefix in update .withdrawn_routes .iter() @@ -407,13 +480,25 @@ fn check_accepted(ctx: &ParserContext, msg: &Message) -> Result<(), TestCaseErro } } - let mut wire = BytesMut::new(); - msg.to_wire(ctx, &mut wire) - .map_err(|e| TestCaseError::fail(format!("accepted but cannot be written: {e}")))?; - prop_assert_eq!(usize::from(msg.wire_len(ctx).unwrap()), wire.len()); - let again = Message::decode(ctx, &wire) - .map_err(|e| TestCaseError::fail(format!("written but cannot be read: {e}")))?; - prop_assert_eq!(&again, msg); + let mut rewritten = BytesMut::new(); + match msg.to_wire(ctx, &mut rewritten) { + Ok(()) => {} + // Withdrawing what a full-size UPDATE announced can need more room + // than one message has. The result is acted on, never sent. + Err(EncodeError::TooLong { .. }) if !repairs.is_empty() => return Ok(()), + Err(e) => { + return Err(TestCaseError::fail(format!( + "accepted but cannot be written: {e}" + ))); + } + } + prop_assert_eq!(usize::from(msg.wire_len(ctx).unwrap()), rewritten.len()); + if repairs.is_empty() { + let (again, repairs) = Message::decode_with_report(ctx, &rewritten) + .map_err(|e| TestCaseError::fail(format!("written but cannot be read: {e}")))?; + prop_assert!(repairs.is_empty(), "{:?}", repairs); + prop_assert_eq!(again, msg); + } Ok(()) } @@ -503,65 +588,59 @@ proptest! { #![proptest_config(config())] #[test] - fn a_message_that_can_be_written_reads_back_equal((four_octet_asn, msg) in session_and_message()) { - let ctx = ctx(four_octet_asn); + fn a_message_that_can_be_written_reads_back_equal((session, msg) in session_and_message()) { + let ctx = session.ctx(); let Some(wire) = written(&ctx, &msg)? else { return Ok(()); }; prop_assert_eq!(usize::from(msg.wire_len(&ctx).unwrap()), wire.len()); - let decoded = Message::decode(&ctx, &wire) + let (decoded, repairs) = Message::decode_with_report(&ctx, &wire) .map_err(|e| TestCaseError::fail(format!("{e}")))?; + // What we write is well formed: nothing for the decoder to repair. + prop_assert!(repairs.is_empty(), "{:?}", repairs); prop_assert_eq!(&decoded, &msg); let (rest, _) = Message::from_wire(&ctx, &wire).unwrap(); prop_assert!(rest.is_empty()); } #[test] - fn noise_is_never_more_than_an_error(four_octet_asn: bool, bytes in vec(any::(), 0..200)) { - let ctx = ctx(four_octet_asn); - if let Ok(msg) = Message::decode(&ctx, &bytes) { - check_accepted(&ctx, &msg)?; - } + fn noise_is_never_more_than_an_error(session in session(), bytes in vec(any::(), 0..200)) { + check_decoding(&session.ctx(), &bytes)?; } #[test] fn noise_behind_a_valid_header_is_never_more_than_an_error( - four_octet_asn: bool, + session in session(), msg_type in 0u8..7, body in vec(any::(), 0..300), ) { - let ctx = ctx(four_octet_asn); - if let Ok(msg) = Message::decode(&ctx, &framed(msg_type, &body)) { - check_accepted(&ctx, &msg)?; - } + check_decoding(&session.ctx(), &framed(msg_type, &body))?; } #[test] fn a_damaged_message_is_never_more_than_an_error( - (four_octet_asn, msg) in session_and_message(), + (session, msg) in session_and_message(), damage in vec(damage(), 1..4), ) { - let ctx = ctx(four_octet_asn); + let ctx = session.ctx(); let Some(wire) = written(&ctx, &msg)? else { return Ok(()); }; - if let Ok(msg) = Message::decode(&ctx, &damaged(&wire, &damage)) { - check_accepted(&ctx, &msg)?; - } + check_decoding(&ctx, &damaged(&wire, &damage))?; } /// The parsers below the message level are public too. #[test] fn noise_is_never_more_than_an_error_for_the_parts( - four_octet_asn: bool, + session in session(), bytes in vec(any::(), 0..120), ) { - let ctx = ctx(four_octet_asn); + let ctx = session.ctx(); let _ = OpenMessage::from_wire(&ctx, &bytes); let _ = OpenOption::from_wire(&ctx, &bytes); let _ = Capability::from_wire(&ctx, &bytes); - let _ = UpdateMessage::from_wire(&ctx, &bytes); + let _ = UpdateMessage::decode(&ctx, &bytes); let _ = PathAttribute::from_wire(&ctx, &bytes); let _ = NotificationMessage::from_wire(&ctx, &bytes); } diff --git a/crates/primitives/src/parser.rs b/crates/primitives/src/parser.rs index 12d614a..cf1c9f0 100644 --- a/crates/primitives/src/parser.rs +++ b/crates/primitives/src/parser.rs @@ -1,14 +1,16 @@ use nom::error::{ErrorKind, ParseError}; use thiserror::Error; -use crate::constants::AddressFamilyId; - +/// What a parser has to know about the session a message belongs to. #[derive(Debug, Default)] pub struct ParserContext { - /// Whether thi parser is being run with a peer that is RFC6793 compliant. + /// Whether AS numbers are four octets wide on this session (RFC 6793). + /// `None` until the OPENs have been exchanged. pub four_octet_asn: Option, - /// Which address family should be parsed by default with this parser. - pub address_family: Option, + /// Whether the peer is in our own AS. Some attributes mean something + /// only between internal peers and are dropped when an external one + /// sends them (RFC 7606 §7). + pub ibgp: bool, } /// Why a value could not be written to the wire. diff --git a/crates/server/src/advertise.rs b/crates/server/src/advertise.rs index 8d38c2c..25ff71f 100644 --- a/crates/server/src/advertise.rs +++ b/crates/server/src/advertise.rs @@ -290,7 +290,7 @@ mod tests { // Serialise and parse back to make sure the wire encoding is sound. let ctx = ParserContext { four_octet_asn: Some(true), - address_family: None, + ibgp: false, }; let msg = Message::Update(update.clone()); let mut buf = BytesMut::new(); @@ -348,7 +348,7 @@ mod tests { // What the peer reads from the wire. let ctx = ParserContext { four_octet_asn: Some(false), - address_family: None, + ibgp: false, }; let mut buf = BytesMut::new(); Message::Update(update).to_wire(&ctx, &mut buf).unwrap(); diff --git a/crates/server/src/connection.rs b/crates/server/src/connection.rs index 2cce3a0..ba1e974 100644 --- a/crates/server/src/connection.rs +++ b/crates/server/src/connection.rs @@ -14,6 +14,7 @@ use std::sync::Arc; use bgp_packet::errors::{BgpError, DecodeError, MsgHeaderSubcode}; use bgp_packet::message::{BGP_HEADER_LEN, BGP_MAX_MSG_LEN, Message, PathAttribute, UpdateMessage}; +use bgp_packet::update_decode::UpdateError; use bgp_primitives::parser::ParserContext; use bgp_rib::{AdjRibIn, PeerId, ReceivedUpdate}; use bytes::BytesMut; @@ -29,7 +30,7 @@ use crate::error::SessionError; use crate::family::FamilySet; use crate::fsm::{Action, Event, Fsm, FsmConfig, Negotiated}; use crate::metrics::{ - MESSAGES_RECEIVED, MESSAGES_SENT, SESSIONS_ESTABLISHED_TOTAL, message_type_name, + MESSAGES_RECEIVED, MESSAGES_SENT, SESSIONS_ESTABLISHED_TOTAL, UPDATE_ERRORS, message_type_name, }; use crate::session::{SessionGuard, SessionHandle, SessionRegistry}; @@ -157,7 +158,10 @@ impl Session { async fn next_event(&mut self) -> Result { tokio::select! { frame = self.frames.next(&self.ctx) => match frame { - Ok(msg) => Ok(Event::Message(msg)), + Ok((msg, update_errors)) => { + self.report(&update_errors); + Ok(Event::Message(msg)) + } Err(SessionError::Decode(e)) => Ok(Event::DecodeFailed(e)), Err(e) => Err(e), }, @@ -168,6 +172,24 @@ impl Session { } } + /// Records errors in an UPDATE that the session survived. They are the + /// 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]) { + for error in update_errors { + UPDATE_ERRORS + .with_label_values(&[error.handling.as_str()]) + .inc(); + warn!( + peer_addr = %self.peer_addr, + handling = error.handling.as_str(), + detail = %error.detail, + "Error in received UPDATE" + ); + } + } + /// Carries out `actions` in order. Returns the session's result once it /// is over. async fn execute(&mut self, actions: Vec) -> Option> { @@ -197,6 +219,7 @@ impl Session { } Action::Admit(negotiated) => { self.ctx.four_octet_asn = Some(negotiated.four_octet_asn); + self.ctx.ibgp = negotiated.peer_asn == self.config.local_asn; match self.config.collisions.admit( self.config.router_id, negotiated.peer_identifier, @@ -369,17 +392,24 @@ impl FrameReader { } /// The next message from the peer. Cancel-safe. - async fn next(&mut self, ctx: &ParserContext) -> Result { + /// + /// With an UPDATE come the errors in it that were handled without + /// resetting the session (RFC 7606); the UPDATE returned is the one to + /// act on, with their handling already applied. + async fn next( + &mut self, + ctx: &ParserContext, + ) -> Result<(Message, Vec), SessionError> { loop { if let Some(length) = frame_length(&self.buf)? && self.buf.len() >= length { let frame = self.buf.split_to(length); - let msg = Message::decode(ctx, &frame)?; + let (msg, update_errors) = Message::decode_with_report(ctx, &frame)?; MESSAGES_RECEIVED .with_label_values(&[message_type_name(&msg)]) .inc(); - return Ok(msg); + return Ok((msg, update_errors)); } if self.reader.read_buf(&mut self.buf).await? == 0 { return Err(io::Error::new( @@ -456,10 +486,19 @@ mod tests { bytes.extend_from_slice(&KEEPALIVE); } peer.write_all(&bytes[..47]).await.unwrap(); - assert!(matches!(frames.next(&ctx).await, Ok(Message::KeepAlive))); - assert!(matches!(frames.next(&ctx).await, Ok(Message::KeepAlive))); + assert!(matches!( + frames.next(&ctx).await, + Ok((Message::KeepAlive, _)) + )); + assert!(matches!( + frames.next(&ctx).await, + Ok((Message::KeepAlive, _)) + )); peer.write_all(&bytes[47..]).await.unwrap(); - assert!(matches!(frames.next(&ctx).await, Ok(Message::KeepAlive))); + assert!(matches!( + frames.next(&ctx).await, + Ok((Message::KeepAlive, _)) + )); drop(peer); assert!(matches!( @@ -483,7 +522,10 @@ mod tests { assert!(abandoned.is_err(), "half a message is not a message"); peer.write_all(&KEEPALIVE[10..]).await.unwrap(); - assert!(matches!(frames.next(&ctx).await, Ok(Message::KeepAlive))); + assert!(matches!( + frames.next(&ctx).await, + Ok((Message::KeepAlive, _)) + )); } proptest! { @@ -531,7 +573,7 @@ mod tests { }; let reader = async { let mut received = Vec::new(); - while let Ok(msg) = frames.next(&ctx).await { + while let Ok((msg, _)) = frames.next(&ctx).await { received.push(msg); } received diff --git a/crates/server/src/metrics.rs b/crates/server/src/metrics.rs index ebfef25..7fb3182 100644 --- a/crates/server/src/metrics.rs +++ b/crates/server/src/metrics.rs @@ -26,6 +26,15 @@ pub static MESSAGES_RECEIVED: LazyLock = LazyLock::new(|| { .expect("metric registration") }); +pub static UPDATE_ERRORS: LazyLock = LazyLock::new(|| { + register_int_counter_vec!( + "bgp_update_errors_total", + "Errors in received UPDATE messages that did not reset the session, by how they were handled (RFC 7606)", + &["handling"] + ) + .expect("metric registration") +}); + pub static SESSIONS_ESTABLISHED_TOTAL: LazyLock = LazyLock::new(|| { register_int_counter!( "bgp_sessions_established_total", @@ -69,6 +78,7 @@ pub static ADJ_RIB_IN_ROUTES: LazyLock = LazyLock::new(|| { pub fn init() { LazyLock::force(&MESSAGES_SENT); LazyLock::force(&MESSAGES_RECEIVED); + LazyLock::force(&UPDATE_ERRORS); LazyLock::force(&SESSIONS_ESTABLISHED_TOTAL); LazyLock::force(&SESSIONS_ESTABLISHED); LazyLock::force(&COLLISIONS); diff --git a/fuzz/fuzz_targets/message.rs b/fuzz/fuzz_targets/message.rs index 8057605..e5df3a1 100644 --- a/fuzz/fuzz_targets/message.rs +++ b/fuzz/fuzz_targets/message.rs @@ -1,5 +1,5 @@ //! Raw bytes, as they come off a TCP connection. The first byte picks the -//! session's AS number width. +//! session's AS number width and whether the peer is internal. #![no_main] use libfuzzer_sys::fuzz_target; @@ -8,5 +8,5 @@ fuzz_target!(|data: &[u8]| { let Some((selector, wire)) = data.split_first() else { return; }; - bgp_fuzz::decode_and_check(selector & 1 == 1, wire); + bgp_fuzz::decode_and_check(selector & 1 == 1, selector & 0x80 != 0, wire); }); diff --git a/fuzz/fuzz_targets/message_body.rs b/fuzz/fuzz_targets/message_body.rs index 547e5e2..73ab70b 100644 --- a/fuzz/fuzz_targets/message_body.rs +++ b/fuzz/fuzz_targets/message_body.rs @@ -1,7 +1,7 @@ //! A message body behind a correct header, so that the fuzzer spends its //! time in the OPEN, UPDATE and NOTIFICATION parsers and not on guessing -//! the marker and length. The first byte picks the AS number width and the -//! message type. +//! the marker and length. The first byte picks the AS number width, the +//! message type and whether the peer is internal. #![no_main] use libfuzzer_sys::fuzz_target; @@ -20,5 +20,5 @@ fuzz_target!(|data: &[u8]| { wire.extend_from_slice(&((19 + body.len()) as u16).to_be_bytes()); wire.push(msg_type); wire.extend_from_slice(body); - bgp_fuzz::decode_and_check(selector & 1 == 1, &wire); + bgp_fuzz::decode_and_check(selector & 1 == 1, selector & 0x80 != 0, &wire); }); diff --git a/fuzz/src/lib.rs b/fuzz/src/lib.rs index 029c825..d53896c 100644 --- a/fuzz/src/lib.rs +++ b/fuzz/src/lib.rs @@ -1,33 +1,44 @@ //! What the fuzz targets have in common. use bgp_packet::message::Message; -use bgp_primitives::parser::ParserContext; +use bgp_primitives::parser::{EncodeError, ParserContext}; use bytes::BytesMut; /// Decodes `wire` as a peer's message on a session with the given AS number -/// width. Whatever the bytes, this must return: a panic is a way for a peer +/// width and kind of peer. Whatever the bytes, this must return: a panic is a way for a peer /// to take a session down. /// /// A message that is accepted must also be one we can show, write, and read /// back as itself. Anything else means the decoder built a value the rest /// of the program cannot rely on. -pub fn decode_and_check(four_octet_asn: bool, wire: &[u8]) { +pub fn decode_and_check(four_octet_asn: bool, ibgp: bool, wire: &[u8]) { let ctx = ParserContext { four_octet_asn: Some(four_octet_asn), - address_family: None, + ibgp, }; - let Ok(msg) = Message::decode(&ctx, wire) else { + let Ok((msg, repairs)) = Message::decode_with_report(&ctx, wire) else { return; }; - let _ = format!("{msg:?}"); + let _ = format!("{msg:?} {repairs:?}"); let mut rewritten = BytesMut::new(); - msg.to_wire(&ctx, &mut rewritten) - .expect("an accepted message can be written"); + match msg.to_wire(&ctx, &mut rewritten) { + Ok(()) => {} + // Withdrawing what a full-size UPDATE announced can need more room + // than one message has. The result is acted on, never sent. + Err(EncodeError::TooLong { .. }) if !repairs.is_empty() => return, + Err(e) => panic!("an accepted message cannot be written: {e}"), + } assert_eq!( usize::from(msg.wire_len(&ctx).expect("and has a length")), rewritten.len() ); - let again = Message::decode(&ctx, &rewritten).expect("and read back"); - assert_eq!(again, msg); + // An UPDATE the decoder had to repair (RFC 7606) is not what was on the + // wire, and need not read back as itself. + if repairs.is_empty() { + let (again, repairs) = + Message::decode_with_report(&ctx, &rewritten).expect("and read back"); + assert!(repairs.is_empty(), "{repairs:?}"); + assert_eq!(again, msg); + } }