From 4a70e30506bc04ac85d3ac6059206ace91b8dcd7 Mon Sep 17 00:00:00 2001 From: Ties de Kock Date: Wed, 15 Apr 2026 19:33:57 +0200 Subject: [PATCH 1/6] fix: implement conditional mandatory BGP attribute validation Update attribute verification to follow RFC 4760 (MP-BGP) rules where mandatory attributes depend on the message payload. - pure withdrawal (withdrawn routes or MP_UNREACH_NLRI) -> none. - ORIGIN + AS_PATH for announcements - NEXT_HOP: conditional. IPv4 NLRI present -> required, IPv4 NLRI not - present -> should be omitted. Optimisation: the [bool; 256] tracking array is now a [u64; 4] bitmask. --- src/parser/bgp/attributes/mod.rs | 108 ++++++++++++++++++++++++++----- 1 file changed, 92 insertions(+), 16 deletions(-) diff --git a/src/parser/bgp/attributes/mod.rs b/src/parser/bgp/attributes/mod.rs index b8faa01..7affea7 100644 --- a/src/parser/bgp/attributes/mod.rs +++ b/src/parser/bgp/attributes/mod.rs @@ -185,8 +185,15 @@ pub fn parse_attributes( let estimated_attrs = (data.remaining() / 3).min(256); let mut attributes: Vec = Vec::with_capacity(estimated_attrs.max(8)); let mut validation_warnings: Vec = Vec::new(); - // boolean flags for seen attributes - small dataset in hot loop. - let mut seen_attributes: [bool; 256] = [false; 256]; + + // Bitmask for seen attributes (256 bits total, using 4x u64). Fits in registers/cache easily. + let mut seen_attributes: [u64; 4] = [0; 4]; + let has_attr = |seen: &[u64; 4], attr: u8| -> bool { + (seen[(attr / 64) as usize] & (1u64 << (attr % 64))) != 0 + }; + let set_attr = |seen: &mut [u64; 4], attr: u8| { + seen[(attr / 64) as usize] |= 1u64 << (attr % 64); + }; while data.remaining() >= 3 { // each attribute is at least 3 bytes: flag(1) + type(1) + length(1) @@ -223,13 +230,13 @@ pub fn parse_attributes( let parsed_attr_type = AttrType::from(attr_type); // RFC 7606: Check for duplicate attributes - if seen_attributes[attr_type as usize] { + if has_attr(&seen_attributes, attr_type) { validation_warnings.push(BgpValidationWarning::DuplicateAttribute { attr_type: parsed_attr_type, }); // Continue processing - don't skip duplicate for now } - seen_attributes[attr_type as usize] = true; + set_attr(&mut seen_attributes, attr_type); // Validate attribute flags and length validate_attribute_flags(parsed_attr_type, flag, &mut validation_warnings); @@ -379,17 +386,31 @@ pub fn parse_attributes( } // Check for missing well-known mandatory attributes - let mandatory_attributes = [ - AttrType::ORIGIN, - AttrType::AS_PATH, - AttrType::NEXT_HOP, - // LOCAL_PREFERENCE is only mandatory for IBGP, so we don't check it here - ]; - - for &mandatory_attr in &mandatory_attributes { - if !seen_attributes[u8::from(mandatory_attr) as usize] { + let has_mp_reach = has_attr(&seen_attributes, u8::from(AttrType::MP_REACHABLE_NLRI)); + let has_mp_unreach = has_attr(&seen_attributes, u8::from(AttrType::MP_UNREACHABLE_NLRI)); + + // Pure withdrawals (no attributes or just MP_UNREACH_NLRI) require no mandatory path attributes. + // Announcements (not pure withdrawals) require ORIGIN and AS_PATH. + // NEXT_HOP is only required if it's an announcement and NO MP_REACH_NLRI is present. + let is_pure_withdrawal = attributes.is_empty() || (attributes.len() == 1 && has_mp_unreach); + + if !is_pure_withdrawal { + // ORIGIN and AS_PATH are universally mandatory for announcements + if !has_attr(&seen_attributes, u8::from(AttrType::ORIGIN)) { + validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { + attr_type: AttrType::ORIGIN, + }); + } + if !has_attr(&seen_attributes, u8::from(AttrType::AS_PATH)) { + validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { + attr_type: AttrType::AS_PATH, + }); + } + + // NEXT_HOP is required if there is no MP_REACH_NLRI (standard IPv4 announcement) + if !has_mp_reach && !has_attr(&seen_attributes, u8::from(AttrType::NEXT_HOP)) { validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { - attr_type: mandatory_attr, + attr_type: AttrType::NEXT_HOP, }); } } @@ -527,8 +548,10 @@ mod tests { #[test] fn test_rfc7606_missing_mandatory_attribute() { - // Empty attributes - should have warnings for missing mandatory attributes - let data = Bytes::from(vec![]); + // Attributes with only LOCAL_PREF (missing ORIGIN, AS_PATH, NEXT_HOP) + let data = Bytes::from(vec![ + 0x40, 0x05, 0x04, 0x00, 0x00, 0x00, 0x64, // LOCAL_PREF = 100 + ]); let asn_len = AsnLength::Bits16; let add_path = false; let afi = None; @@ -540,6 +563,7 @@ mod tests { // Should have warnings for missing mandatory attributes assert!(attributes.has_validation_warnings()); let warnings = attributes.validation_warnings(); + // LOCAL_PREF is not a withdrawal, so ORIGIN, AS_PATH, NEXT_HOP are required assert_eq!(warnings.len(), 3); // ORIGIN, AS_PATH, NEXT_HOP for warning in warnings { @@ -555,6 +579,58 @@ mod tests { } } + #[test] + fn test_mp_reach_no_next_hop() { + // Attributes with MP_REACH_NLRI (missing ORIGIN, AS_PATH) + // MP_REACH_NLRI is type 14 (0x0E). + // We just need a dummy MP_REACH_NLRI. + let data = Bytes::from(vec![ + 0x80, 0x0E, 0x06, + 0x00, 0x01, 0x01, // AFI=1, SAFI=1 + 0x00, // Next Hop Len = 0 (invalid for parsing, but enough to trigger logic) + 0x00, // Reserved + 0x00, // NLRI + ]); + let asn_len = AsnLength::Bits16; + let add_path = false; + let afi = None; + let safi = None; + let prefixes = None; + + let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + + // Should NOT have NEXT_HOP warning because MP_REACH_NLRI is present + let warnings = attributes.validation_warnings(); + let has_next_hop_warning = warnings.iter().any(|w| { + matches!(w, BgpValidationWarning::MissingWellKnownAttribute { attr_type: AttrType::NEXT_HOP }) + }); + assert!(!has_next_hop_warning); + } + + #[test] + fn test_pure_withdrawal_no_warnings() { + // Empty attributes - pure withdrawal + let data = Bytes::from(vec![]); + let asn_len = AsnLength::Bits16; + let add_path = false; + let afi = None; + let safi = None; + let prefixes = None; + + let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + + // Should have NO warnings + assert!(!attributes.has_validation_warnings()); + + // Attributes with only MP_UNREACH_NLRI - pure withdrawal + let data = Bytes::from(vec![ + 0x80, 0x0F, 0x03, + 0x00, 0x01, 0x01, // AFI=1, SAFI=1 + ]); + let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + assert!(!attributes.has_validation_warnings()); + } + #[test] fn test_rfc7606_duplicate_attribute() { // Create two ORIGIN attributes From 947c2f9e8fb200276c83ebe7cb56f06e0cb110fb Mon Sep 17 00:00:00 2001 From: Ties de Kock Date: Wed, 15 Apr 2026 19:49:38 +0200 Subject: [PATCH 2/6] explain inferring it's an announcement --- src/parser/bgp/attributes/mod.rs | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/parser/bgp/attributes/mod.rs b/src/parser/bgp/attributes/mod.rs index 7affea7..425e9d5 100644 --- a/src/parser/bgp/attributes/mod.rs +++ b/src/parser/bgp/attributes/mod.rs @@ -385,17 +385,33 @@ pub fn parse_attributes( }; } - // Check for missing well-known mandatory attributes + // Check for missing well-known mandatory attributes. + // + // RFC 4271 (BGP-4) and RFC 4760 (MP-BGP) define which attributes are mandatory. + // The rules shift from "universally mandatory" to "conditionally mandatory" in MP-BGP: + // + // 1. Pure Withdrawals: If an UPDATE only withdraws routes (standard or MP), + // NO path attributes are required. + // 2. Announcements: If an UPDATE announces any reachable prefix, + // ORIGIN and AS_PATH are strictly required. + // 3. NEXT_HOP (Attribute 3): + // - Required if the base IPv4 NLRI field is populated. + // - NOT required (and should be omitted) if the base IPv4 NLRI field is empty. + // - MP_REACH_NLRI (IPv6, VPNv4, etc.) is self-sufficient as it bundles its + // own next-hop inside the attribute. + // + // Inference: Since we parse attributes before reaching the trailing IPv4 NLRI field, + // we infer the requirement: if it's an announcement and NO MP_REACH_NLRI is present, + // it MUST be a standard IPv4 announcement, thus requiring Attribute 3. let has_mp_reach = has_attr(&seen_attributes, u8::from(AttrType::MP_REACHABLE_NLRI)); let has_mp_unreach = has_attr(&seen_attributes, u8::from(AttrType::MP_UNREACHABLE_NLRI)); - // Pure withdrawals (no attributes or just MP_UNREACH_NLRI) require no mandatory path attributes. - // Announcements (not pure withdrawals) require ORIGIN and AS_PATH. - // NEXT_HOP is only required if it's an announcement and NO MP_REACH_NLRI is present. + // A "pure withdrawal" is defined as having no attributes (standard IPv4 withdrawal) + // or exactly one attribute that is MP_UNREACH_NLRI. let is_pure_withdrawal = attributes.is_empty() || (attributes.len() == 1 && has_mp_unreach); if !is_pure_withdrawal { - // ORIGIN and AS_PATH are universally mandatory for announcements + // ORIGIN and AS_PATH are universally mandatory for all announcements. if !has_attr(&seen_attributes, u8::from(AttrType::ORIGIN)) { validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { attr_type: AttrType::ORIGIN, @@ -407,7 +423,7 @@ pub fn parse_attributes( }); } - // NEXT_HOP is required if there is no MP_REACH_NLRI (standard IPv4 announcement) + // NEXT_HOP is required if this is an IPv4 announcement (no MP_REACH_NLRI seen). if !has_mp_reach && !has_attr(&seen_attributes, u8::from(AttrType::NEXT_HOP)) { validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { attr_type: AttrType::NEXT_HOP, From 711e7414f78a66e03f007d8170a4c3e60baa5adb Mon Sep 17 00:00:00 2001 From: Ties de Kock Date: Wed, 15 Apr 2026 20:34:11 +0200 Subject: [PATCH 3/6] Move up attribute verification --- src/models/bgp/attributes/mod.rs | 78 +++++++++++++++++-- src/parser/bgp/attributes/mod.rs | 71 ++++------------- src/parser/bgp/messages.rs | 11 ++- src/parser/mrt/messages/table_dump.rs | 5 +- .../messages/table_dump_v2/rib_afi_entries.rs | 5 +- 5 files changed, 108 insertions(+), 62 deletions(-) diff --git a/src/models/bgp/attributes/mod.rs b/src/models/bgp/attributes/mod.rs index 0242b55..f471368 100644 --- a/src/models/bgp/attributes/mod.rs +++ b/src/models/bgp/attributes/mod.rs @@ -134,18 +134,30 @@ pub fn get_deprecated_attr_type(attr_type: u8) -> Option<&'static str> { } /// Convenience wrapper for a list of attributes -#[derive(Debug, PartialEq, Clone, Default, Eq)] +#[derive(PartialEq, Clone, Default, Eq)] pub struct Attributes { // Black box type to allow for later changes/optimizations. The most common attributes could be // added as fields to allow for easier lookup. pub(crate) inner: Vec, /// RFC 7606 validation warnings collected during parsing pub(crate) validation_warnings: Vec, + /// Bitmask of seen attributes to allow O(1) checks. Fits in 4 u64s. + pub(crate) attr_mask: [u64; 4], +} + +impl std::fmt::Debug for Attributes { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Attributes") + .field("inner", &self.inner) + .field("validation_warnings", &self.validation_warnings) + .finish() + } } impl Attributes { pub fn has_attr(&self, ty: AttrType) -> bool { - self.inner.iter().any(|x| x.value.attr_type() == ty) + let attr = u8::from(ty); + (self.attr_mask[(attr / 64) as usize] & (1u64 << (attr % 64))) != 0 } pub fn get_attr(&self, ty: AttrType) -> Option { @@ -156,9 +168,46 @@ impl Attributes { } pub fn add_attr(&mut self, attr: Attribute) { + let ty = u8::from(attr.value.attr_type()); + self.attr_mask[(ty / 64) as usize] |= 1u64 << (ty % 64); self.inner.push(attr); } + /// Check for missing well-known mandatory attributes. + /// + /// RFC 4271 (BGP-4) and RFC 4760 (MP-BGP) define which attributes are mandatory. + /// - Pure Withdrawals: NO path attributes are required. + /// - Announcements: ORIGIN and AS_PATH are strictly required. + /// - NEXT_HOP is required if standard IPv4 NLRI is present or if no MP_REACH_NLRI is present. + pub fn check_mandatory_attributes(&mut self, is_announcement: bool, has_standard_nlri: bool) { + if !is_announcement { + return; + } + + // ORIGIN and AS_PATH are universally mandatory for all announcements. + if !self.has_attr(AttrType::ORIGIN) { + self.validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { + attr_type: AttrType::ORIGIN, + }); + } + if !self.has_attr(AttrType::AS_PATH) { + self.validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { + attr_type: AttrType::AS_PATH, + }); + } + + // NEXT_HOP is required if this is an IPv4 announcement (has standard NLRI) + // or if we haven't seen MP_REACH_NLRI (which implies standard NLRI is expected). + let has_mp_reach = self.has_attr(AttrType::MP_REACHABLE_NLRI); + if has_standard_nlri || !has_mp_reach { + if !self.has_attr(AttrType::NEXT_HOP) { + self.validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { + attr_type: AttrType::NEXT_HOP, + }); + } + } + } + /// Add a validation warning to the attributes pub fn add_validation_warning(&mut self, warning: BgpValidationWarning) { self.validation_warnings.push(warning); @@ -324,27 +373,43 @@ impl Iterator for MetaCommunitiesIter<'_> { } } +fn compute_mask(inner: &[Attribute]) -> [u64; 4] { + let mut attr_mask = [0; 4]; + for attr in inner { + let ty = u8::from(attr.value.attr_type()); + attr_mask[(ty / 64) as usize] |= 1u64 << (ty % 64); + } + attr_mask +} + impl FromIterator for Attributes { fn from_iter>(iter: T) -> Self { + let inner: Vec = iter.into_iter().collect(); + let attr_mask = compute_mask(&inner); Attributes { - inner: iter.into_iter().collect(), + inner, validation_warnings: Vec::new(), + attr_mask, } } } impl From> for Attributes { fn from(value: Vec) -> Self { + let attr_mask = compute_mask(&value); Attributes { inner: value, validation_warnings: Vec::new(), + attr_mask, } } } impl Extend for Attributes { fn extend>(&mut self, iter: T) { - self.inner.extend(iter) + for attr in iter { + self.add_attr(attr); + } } } @@ -356,9 +421,12 @@ impl Extend for Attributes { impl FromIterator for Attributes { fn from_iter>(iter: T) -> Self { + let inner: Vec = iter.into_iter().map(Attribute::from).collect(); + let attr_mask = compute_mask(&inner); Attributes { - inner: iter.into_iter().map(Attribute::from).collect(), + inner, validation_warnings: Vec::new(), + attr_mask, } } } diff --git a/src/parser/bgp/attributes/mod.rs b/src/parser/bgp/attributes/mod.rs index 425e9d5..e6a3680 100644 --- a/src/parser/bgp/attributes/mod.rs +++ b/src/parser/bgp/attributes/mod.rs @@ -385,55 +385,11 @@ pub fn parse_attributes( }; } - // Check for missing well-known mandatory attributes. - // - // RFC 4271 (BGP-4) and RFC 4760 (MP-BGP) define which attributes are mandatory. - // The rules shift from "universally mandatory" to "conditionally mandatory" in MP-BGP: - // - // 1. Pure Withdrawals: If an UPDATE only withdraws routes (standard or MP), - // NO path attributes are required. - // 2. Announcements: If an UPDATE announces any reachable prefix, - // ORIGIN and AS_PATH are strictly required. - // 3. NEXT_HOP (Attribute 3): - // - Required if the base IPv4 NLRI field is populated. - // - NOT required (and should be omitted) if the base IPv4 NLRI field is empty. - // - MP_REACH_NLRI (IPv6, VPNv4, etc.) is self-sufficient as it bundles its - // own next-hop inside the attribute. - // - // Inference: Since we parse attributes before reaching the trailing IPv4 NLRI field, - // we infer the requirement: if it's an announcement and NO MP_REACH_NLRI is present, - // it MUST be a standard IPv4 announcement, thus requiring Attribute 3. - let has_mp_reach = has_attr(&seen_attributes, u8::from(AttrType::MP_REACHABLE_NLRI)); - let has_mp_unreach = has_attr(&seen_attributes, u8::from(AttrType::MP_UNREACHABLE_NLRI)); - - // A "pure withdrawal" is defined as having no attributes (standard IPv4 withdrawal) - // or exactly one attribute that is MP_UNREACH_NLRI. - let is_pure_withdrawal = attributes.is_empty() || (attributes.len() == 1 && has_mp_unreach); - - if !is_pure_withdrawal { - // ORIGIN and AS_PATH are universally mandatory for all announcements. - if !has_attr(&seen_attributes, u8::from(AttrType::ORIGIN)) { - validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { - attr_type: AttrType::ORIGIN, - }); - } - if !has_attr(&seen_attributes, u8::from(AttrType::AS_PATH)) { - validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { - attr_type: AttrType::AS_PATH, - }); - } - - // NEXT_HOP is required if this is an IPv4 announcement (no MP_REACH_NLRI seen). - if !has_mp_reach && !has_attr(&seen_attributes, u8::from(AttrType::NEXT_HOP)) { - validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { - attr_type: AttrType::NEXT_HOP, - }); - } - } - - let mut result = Attributes::from(attributes); - result.validation_warnings = validation_warnings; - Ok(result) + Ok(Attributes { + inner: attributes, + validation_warnings, + attr_mask: seen_attributes, + }) } impl Attribute { @@ -574,7 +530,9 @@ mod tests { let safi = None; let prefixes = None; - let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let mut attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + // Manually trigger mandatory check as an announcement with standard NLRI + attributes.check_mandatory_attributes(true, true); // Should have warnings for missing mandatory attributes assert!(attributes.has_validation_warnings()); @@ -613,9 +571,11 @@ mod tests { let safi = None; let prefixes = None; - let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let mut attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + // Manually trigger mandatory check as an announcement but NO standard NLRI (MP only) + attributes.check_mandatory_attributes(true, false); - // Should NOT have NEXT_HOP warning because MP_REACH_NLRI is present + // Should NOT have NEXT_HOP warning because MP_REACH_NLRI is present and has_standard_nlri is false let warnings = attributes.validation_warnings(); let has_next_hop_warning = warnings.iter().any(|w| { matches!(w, BgpValidationWarning::MissingWellKnownAttribute { attr_type: AttrType::NEXT_HOP }) @@ -633,7 +593,9 @@ mod tests { let safi = None; let prefixes = None; - let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let mut attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + // Manually trigger mandatory check as a withdrawal + attributes.check_mandatory_attributes(false, false); // Should have NO warnings assert!(!attributes.has_validation_warnings()); @@ -643,7 +605,8 @@ mod tests { 0x80, 0x0F, 0x03, 0x00, 0x01, 0x01, // AFI=1, SAFI=1 ]); - let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let mut attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + attributes.check_mandatory_attributes(false, false); assert!(!attributes.has_validation_warnings()); } diff --git a/src/parser/bgp/messages.rs b/src/parser/bgp/messages.rs index 3253c23..b7a4cf7 100644 --- a/src/parser/bgp/messages.rs +++ b/src/parser/bgp/messages.rs @@ -468,12 +468,21 @@ pub fn parse_bgp_update_message( input.has_n_remaining(attribute_length)?; let attr_data_slice = input.split_to(attribute_length); - let attributes = parse_attributes(attr_data_slice, asn_len, add_path, None, None, None)?; + let mut attributes = parse_attributes(attr_data_slice, asn_len, add_path, None, None, None)?; // parse announced prefixes nlri. // the remaining bytes are announced prefixes. let announced_prefixes = read_nlri(input, &afi, add_path)?; + // validate mandatory attributes + let is_announcement = !announced_prefixes.is_empty() + || attributes.has_attr(AttrType::MP_REACHABLE_NLRI) + || (!attributes.inner.is_empty() + && !(attributes.inner.len() == 1 + && attributes.has_attr(AttrType::MP_UNREACHABLE_NLRI))); + let has_standard_nlri = !announced_prefixes.is_empty(); + attributes.check_mandatory_attributes(is_announcement, has_standard_nlri); + Ok(BgpUpdateMessage { withdrawn_prefixes, attributes, diff --git a/src/parser/mrt/messages/table_dump.rs b/src/parser/mrt/messages/table_dump.rs index 3f64e68..4d20185 100644 --- a/src/parser/mrt/messages/table_dump.rs +++ b/src/parser/mrt/messages/table_dump.rs @@ -99,9 +99,12 @@ pub fn parse_table_dump_message( let attr_data_slice = data.split_to(attribute_length); // for TABLE_DUMP type, the AS number length is always 2-byte. - let attributes = + let mut attributes = parse_attributes(attr_data_slice, &AsnLength::Bits16, false, None, None, None)?; + // validate mandatory attributes (TABLE_DUMP is always an announcement) + attributes.check_mandatory_attributes(true, afi == Afi::Ipv4); + Ok(TableDumpMessage { view_number, sequence_number, diff --git a/src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs b/src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs index c251313..59e5c55 100644 --- a/src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs +++ b/src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs @@ -140,7 +140,7 @@ pub fn parse_rib_entry( input.has_n_remaining(attribute_length)?; let attr_data_slice = input.split_to(attribute_length); - let attributes = parse_attributes( + let mut attributes = parse_attributes( attr_data_slice, &AsnLength::Bits32, is_add_path, @@ -149,6 +149,9 @@ pub fn parse_rib_entry( Some(&[prefix]), )?; + // validate mandatory attributes (RIB entry is always an announcement) + attributes.check_mandatory_attributes(true, *afi == Afi::Ipv4); + Ok(RibEntry { peer_index, originated_time, From fee64c548a347f725ca4cf7ada047d10aff89de9 Mon Sep 17 00:00:00 2001 From: Ties de Kock Date: Wed, 15 Apr 2026 20:41:28 +0200 Subject: [PATCH 4/6] Comprehensive validation of attribute presence --- src/parser/bgp/messages.rs | 7 +-- tests/test_bgp_update_validation.rs | 80 +++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 5 deletions(-) create mode 100644 tests/test_bgp_update_validation.rs diff --git a/src/parser/bgp/messages.rs b/src/parser/bgp/messages.rs index b7a4cf7..1333c6e 100644 --- a/src/parser/bgp/messages.rs +++ b/src/parser/bgp/messages.rs @@ -475,11 +475,8 @@ pub fn parse_bgp_update_message( let announced_prefixes = read_nlri(input, &afi, add_path)?; // validate mandatory attributes - let is_announcement = !announced_prefixes.is_empty() - || attributes.has_attr(AttrType::MP_REACHABLE_NLRI) - || (!attributes.inner.is_empty() - && !(attributes.inner.len() == 1 - && attributes.has_attr(AttrType::MP_UNREACHABLE_NLRI))); + let is_announcement = + !announced_prefixes.is_empty() || attributes.has_attr(AttrType::MP_REACHABLE_NLRI); let has_standard_nlri = !announced_prefixes.is_empty(); attributes.check_mandatory_attributes(is_announcement, has_standard_nlri); diff --git a/tests/test_bgp_update_validation.rs b/tests/test_bgp_update_validation.rs new file mode 100644 index 0000000..82fbc44 --- /dev/null +++ b/tests/test_bgp_update_validation.rs @@ -0,0 +1,80 @@ +use bgpkit_parser::parser::bgp::messages::parse_bgp_update_message; +use bgpkit_parser::models::*; +use bytes::Bytes; + +#[test] +fn test_validation_combinations() { + let asn_len = AsnLength::Bits32; + + // Helper to build an attribute block + fn build_attrs(has_origin: bool, has_as_path: bool, has_next_hop: bool, mp_reach: Option>, mp_unreach: Option>) -> Vec { + let mut attrs = Vec::new(); + if has_origin { + attrs.extend_from_slice(&[0x40, 0x01, 0x01, 0x00]); // ORIGIN = IGP + } + if has_as_path { + attrs.extend_from_slice(&[0x40, 0x02, 0x00]); // AS_PATH = empty + } + if has_next_hop { + attrs.extend_from_slice(&[0x40, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04]); // NEXT_HOP = 1.2.3.4 + } + if let Some(data) = mp_reach { + let len = data.len(); + attrs.extend_from_slice(&[0x80, 0x0E, len as u8]); + attrs.extend_from_slice(&data); + } + if let Some(data) = mp_unreach { + let len = data.len(); + attrs.extend_from_slice(&[0x80, 0x0F, len as u8]); + attrs.extend_from_slice(&data); + } + attrs + } + + // Helper to build a full UPDATE message + fn build_update(withdrawn: Vec, attrs: Vec, nlri: Vec) -> Bytes { + let mut msg = Vec::new(); + msg.extend_from_slice(&(withdrawn.len() as u16).to_be_bytes()); + msg.extend_from_slice(&withdrawn); + msg.extend_from_slice(&(attrs.len() as u16).to_be_bytes()); + msg.extend_from_slice(&attrs); + msg.extend_from_slice(&nlri); + Bytes::from(msg) + } + + let dummy_mp_reach = vec![0x00, 0x01, 0x01, 0x04, 0x01, 0x01, 0x01, 0x01, 0x00]; // IPv4 Unicast, NH=1.1.1.1, no prefixes + let dummy_mp_unreach = vec![0x00, 0x01, 0x01]; // IPv4 Unicast + + let scenarios = vec![ + // --- Standard IPv4 Scenarios --- + ("IPv4 Announce + NH", build_update(vec![], build_attrs(true, true, true, None, None), vec![0x18, 0x0A, 0x00, 0x00]), 0), + ("IPv4 Announce - NH", build_update(vec![], build_attrs(true, true, false, None, None), vec![0x18, 0x0A, 0x00, 0x00]), 1), + ("IPv4 Withdraw Only", build_update(vec![0x18, 0x0A, 0x00, 0x00], vec![], vec![]), 0), + ("IPv4 Withdraw + NH", build_update(vec![0x18, 0x0A, 0x00, 0x00], build_attrs(false, false, true, None, None), vec![]), 0), + + // --- MP-BGP Scenarios --- + ("MP_REACH - NH", build_update(vec![], build_attrs(true, true, false, Some(dummy_mp_reach.clone()), None), vec![]), 0), + ("MP_REACH + NH", build_update(vec![], build_attrs(true, true, true, Some(dummy_mp_reach.clone()), None), vec![]), 0), + ("MP_UNREACH Only", build_update(vec![], build_attrs(false, false, false, None, Some(dummy_mp_unreach.clone())), vec![]), 0), + + // --- Mixed Scenarios (IPv4 Withdraw + MP_REACH Announcement) --- + ("IPv4 Withdraw + MP_REACH (AS+ORIGIN)", build_update(vec![0x18, 0x0A, 0x00, 0x00], build_attrs(true, true, false, Some(dummy_mp_reach.clone()), None), vec![]), 0), + ("IPv4 Withdraw + MP_REACH (ORIGIN only)", build_update(vec![0x18, 0x0A, 0x00, 0x00], build_attrs(true, false, false, Some(dummy_mp_reach.clone()), None), vec![]), 1), + ("IPv4 Withdraw + MP_REACH (AS+ORIGIN+NH)", build_update(vec![0x18, 0x0A, 0x00, 0x00], build_attrs(true, true, true, Some(dummy_mp_reach.clone()), None), vec![]), 0), + + // --- Mixed Scenarios (IPv4 Announce + MP_REACH Announcement) --- + ("IPv4 Announce + MP_REACH + NH", build_update(vec![], build_attrs(true, true, true, Some(dummy_mp_reach.clone()), None), vec![0x18, 0x0A, 0x00, 0x00]), 0), + ("IPv4 Announce + MP_REACH - NH", build_update(vec![], build_attrs(true, true, false, Some(dummy_mp_reach.clone()), None), vec![0x18, 0x0A, 0x00, 0x00]), 1), + ]; + + for (name, msg, expected_warnings) in scenarios { + let res = parse_bgp_update_message(msg, false, &asn_len); + match res { + Ok(update) => { + let warnings = update.attributes.validation_warnings(); + assert_eq!(warnings.len(), expected_warnings, "Scenario '{}' failed. Warnings: {:?}", name, warnings); + } + Err(e) => panic!("Scenario '{}' failed to parse: {}", name, e), + } + } +} From 3e17f555165989026804ffae6b610975214fb84e Mon Sep 17 00:00:00 2001 From: Ties de Kock Date: Wed, 15 Apr 2026 20:45:00 +0200 Subject: [PATCH 5/6] elaborate on implication --- src/models/bgp/attributes/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/models/bgp/attributes/mod.rs b/src/models/bgp/attributes/mod.rs index f471368..f9a7008 100644 --- a/src/models/bgp/attributes/mod.rs +++ b/src/models/bgp/attributes/mod.rs @@ -197,7 +197,8 @@ impl Attributes { } // NEXT_HOP is required if this is an IPv4 announcement (has standard NLRI) - // or if we haven't seen MP_REACH_NLRI (which implies standard NLRI is expected). + // or if we haven't seen MP_REACH_NLRI, + // which implies standard NLRI is expected, since is_announcement. let has_mp_reach = self.has_attr(AttrType::MP_REACHABLE_NLRI); if has_standard_nlri || !has_mp_reach { if !self.has_attr(AttrType::NEXT_HOP) { From 6679abf18234b2e3c11ba988e08d46a111287124 Mon Sep 17 00:00:00 2001 From: Mingwei Zhang Date: Wed, 15 Apr 2026 14:25:33 -0700 Subject: [PATCH 6/6] fix: add missing attr_mask in Deserialize impl and address clippy warning Fixes build failure caused by missing attr_mask field in the serde Deserialize implementation for Attributes. The mask is now computed from the deserialized inner attribute vector. Also collapses nested if statements to address clippy warning. Changes: - src/models/bgp/attributes/mod.rs: Fix Deserialize impl and clippy - CHANGELOG.md: Document RFC 4760 validation changes --- CHANGELOG.md | 21 +++++ src/models/bgp/attributes/mod.rs | 26 +++--- src/parser/bgp/attributes/mod.rs | 27 ++++-- tests/test_bgp_update_validation.rs | 140 ++++++++++++++++++++++++---- 4 files changed, 174 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d6a5bf..01603e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,27 @@ All notable changes to this project will be documented in this file. ## Unreleased +### Fixed + +* **RFC 4760 mandatory attribute validation**: Fixed validation to properly handle multiprotocol BGP (MP-BGP) conditional mandatory attributes: + - Pure withdrawals (no announcements): no mandatory attributes required + - MP_REACH_NLRI only: ORIGIN and AS_PATH required, NEXT_HOP not required (embedded in MP_REACH_NLRI) + - IPv4 NLRI: ORIGIN, AS_PATH, and NEXT_HOP all required + - Mixed (IPv4 NLRI + MP_REACH_NLRI): ORIGIN, AS_PATH required; NEXT_HOP required because IPv4 NLRI is present + +### Added + +* **`Attributes::check_mandatory_attributes()`**: New method to validate mandatory attributes with proper NLRI context awareness. Must be called after NLRI parsing to correctly determine requirements. +* **`Attributes::attr_mask`**: Internal `[u64; 4]` bitmask field for O(1) attribute presence tracking (32 bytes vs 256 bytes for `[bool; 256]`) +* **Integration tests**: Added `tests/test_bgp_update_validation.rs` with comprehensive scenario coverage for all validation cases + +### Changed + +* **`parse_attributes()`**: Removed inline mandatory attribute validation; validation now performed by callers via `Attributes::check_mandatory_attributes()` after NLRI context is known +* **`Attributes::has_attr()`**: Now O(1) via bitmask lookup instead of O(n) vector scan +* **`Attributes` Debug impl**: Changed from derived to manual implementation (excludes internal `attr_mask` field) +* **All `Attributes` constructors**: Updated to compute and populate `attr_mask` from `inner` vector + ## v0.16.0 - 2026-04-07 ### Breaking changes diff --git a/src/models/bgp/attributes/mod.rs b/src/models/bgp/attributes/mod.rs index f9a7008..f2a51a3 100644 --- a/src/models/bgp/attributes/mod.rs +++ b/src/models/bgp/attributes/mod.rs @@ -186,26 +186,27 @@ impl Attributes { // ORIGIN and AS_PATH are universally mandatory for all announcements. if !self.has_attr(AttrType::ORIGIN) { - self.validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { - attr_type: AttrType::ORIGIN, - }); + self.validation_warnings + .push(BgpValidationWarning::MissingWellKnownAttribute { + attr_type: AttrType::ORIGIN, + }); } if !self.has_attr(AttrType::AS_PATH) { - self.validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { - attr_type: AttrType::AS_PATH, - }); + self.validation_warnings + .push(BgpValidationWarning::MissingWellKnownAttribute { + attr_type: AttrType::AS_PATH, + }); } // NEXT_HOP is required if this is an IPv4 announcement (has standard NLRI) // or if we haven't seen MP_REACH_NLRI, // which implies standard NLRI is expected, since is_announcement. let has_mp_reach = self.has_attr(AttrType::MP_REACHABLE_NLRI); - if has_standard_nlri || !has_mp_reach { - if !self.has_attr(AttrType::NEXT_HOP) { - self.validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { + if (has_standard_nlri || !has_mp_reach) && !self.has_attr(AttrType::NEXT_HOP) { + self.validation_warnings + .push(BgpValidationWarning::MissingWellKnownAttribute { attr_type: AttrType::NEXT_HOP, }); - } } } @@ -469,9 +470,12 @@ mod serde_impl { where D: Deserializer<'de>, { + let inner = >::deserialize(deserializer)?; + let attr_mask = compute_mask(&inner); Ok(Attributes { - inner: >::deserialize(deserializer)?, + inner, validation_warnings: Vec::new(), + attr_mask, }) } } diff --git a/src/parser/bgp/attributes/mod.rs b/src/parser/bgp/attributes/mod.rs index e6a3680..84bbd22 100644 --- a/src/parser/bgp/attributes/mod.rs +++ b/src/parser/bgp/attributes/mod.rs @@ -530,7 +530,8 @@ mod tests { let safi = None; let prefixes = None; - let mut attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let mut attributes = + parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); // Manually trigger mandatory check as an announcement with standard NLRI attributes.check_mandatory_attributes(true, true); @@ -556,11 +557,10 @@ mod tests { #[test] fn test_mp_reach_no_next_hop() { // Attributes with MP_REACH_NLRI (missing ORIGIN, AS_PATH) - // MP_REACH_NLRI is type 14 (0x0E). + // MP_REACH_NLRI is type 14 (0x0E). // We just need a dummy MP_REACH_NLRI. let data = Bytes::from(vec![ - 0x80, 0x0E, 0x06, - 0x00, 0x01, 0x01, // AFI=1, SAFI=1 + 0x80, 0x0E, 0x06, 0x00, 0x01, 0x01, // AFI=1, SAFI=1 0x00, // Next Hop Len = 0 (invalid for parsing, but enough to trigger logic) 0x00, // Reserved 0x00, // NLRI @@ -571,14 +571,20 @@ mod tests { let safi = None; let prefixes = None; - let mut attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let mut attributes = + parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); // Manually trigger mandatory check as an announcement but NO standard NLRI (MP only) attributes.check_mandatory_attributes(true, false); // Should NOT have NEXT_HOP warning because MP_REACH_NLRI is present and has_standard_nlri is false let warnings = attributes.validation_warnings(); let has_next_hop_warning = warnings.iter().any(|w| { - matches!(w, BgpValidationWarning::MissingWellKnownAttribute { attr_type: AttrType::NEXT_HOP }) + matches!( + w, + BgpValidationWarning::MissingWellKnownAttribute { + attr_type: AttrType::NEXT_HOP + } + ) }); assert!(!has_next_hop_warning); } @@ -593,7 +599,8 @@ mod tests { let safi = None; let prefixes = None; - let mut attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let mut attributes = + parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); // Manually trigger mandatory check as a withdrawal attributes.check_mandatory_attributes(false, false); @@ -602,10 +609,10 @@ mod tests { // Attributes with only MP_UNREACH_NLRI - pure withdrawal let data = Bytes::from(vec![ - 0x80, 0x0F, 0x03, - 0x00, 0x01, 0x01, // AFI=1, SAFI=1 + 0x80, 0x0F, 0x03, 0x00, 0x01, 0x01, // AFI=1, SAFI=1 ]); - let mut attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let mut attributes = + parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); attributes.check_mandatory_attributes(false, false); assert!(!attributes.has_validation_warnings()); } diff --git a/tests/test_bgp_update_validation.rs b/tests/test_bgp_update_validation.rs index 82fbc44..d3fbb8c 100644 --- a/tests/test_bgp_update_validation.rs +++ b/tests/test_bgp_update_validation.rs @@ -1,5 +1,5 @@ -use bgpkit_parser::parser::bgp::messages::parse_bgp_update_message; use bgpkit_parser::models::*; +use bgpkit_parser::parser::bgp::messages::parse_bgp_update_message; use bytes::Bytes; #[test] @@ -7,7 +7,13 @@ fn test_validation_combinations() { let asn_len = AsnLength::Bits32; // Helper to build an attribute block - fn build_attrs(has_origin: bool, has_as_path: bool, has_next_hop: bool, mp_reach: Option>, mp_unreach: Option>) -> Vec { + fn build_attrs( + has_origin: bool, + has_as_path: bool, + has_next_hop: bool, + mp_reach: Option>, + mp_unreach: Option>, + ) -> Vec { let mut attrs = Vec::new(); if has_origin { attrs.extend_from_slice(&[0x40, 0x01, 0x01, 0x00]); // ORIGIN = IGP @@ -16,7 +22,8 @@ fn test_validation_combinations() { attrs.extend_from_slice(&[0x40, 0x02, 0x00]); // AS_PATH = empty } if has_next_hop { - attrs.extend_from_slice(&[0x40, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04]); // NEXT_HOP = 1.2.3.4 + attrs.extend_from_slice(&[0x40, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04]); + // NEXT_HOP = 1.2.3.4 } if let Some(data) = mp_reach { let len = data.len(); @@ -47,24 +54,113 @@ fn test_validation_combinations() { let scenarios = vec![ // --- Standard IPv4 Scenarios --- - ("IPv4 Announce + NH", build_update(vec![], build_attrs(true, true, true, None, None), vec![0x18, 0x0A, 0x00, 0x00]), 0), - ("IPv4 Announce - NH", build_update(vec![], build_attrs(true, true, false, None, None), vec![0x18, 0x0A, 0x00, 0x00]), 1), - ("IPv4 Withdraw Only", build_update(vec![0x18, 0x0A, 0x00, 0x00], vec![], vec![]), 0), - ("IPv4 Withdraw + NH", build_update(vec![0x18, 0x0A, 0x00, 0x00], build_attrs(false, false, true, None, None), vec![]), 0), - + ( + "IPv4 Announce + NH", + build_update( + vec![], + build_attrs(true, true, true, None, None), + vec![0x18, 0x0A, 0x00, 0x00], + ), + 0, + ), + ( + "IPv4 Announce - NH", + build_update( + vec![], + build_attrs(true, true, false, None, None), + vec![0x18, 0x0A, 0x00, 0x00], + ), + 1, + ), + ( + "IPv4 Withdraw Only", + build_update(vec![0x18, 0x0A, 0x00, 0x00], vec![], vec![]), + 0, + ), + ( + "IPv4 Withdraw + NH", + build_update( + vec![0x18, 0x0A, 0x00, 0x00], + build_attrs(false, false, true, None, None), + vec![], + ), + 0, + ), // --- MP-BGP Scenarios --- - ("MP_REACH - NH", build_update(vec![], build_attrs(true, true, false, Some(dummy_mp_reach.clone()), None), vec![]), 0), - ("MP_REACH + NH", build_update(vec![], build_attrs(true, true, true, Some(dummy_mp_reach.clone()), None), vec![]), 0), - ("MP_UNREACH Only", build_update(vec![], build_attrs(false, false, false, None, Some(dummy_mp_unreach.clone())), vec![]), 0), - + ( + "MP_REACH - NH", + build_update( + vec![], + build_attrs(true, true, false, Some(dummy_mp_reach.clone()), None), + vec![], + ), + 0, + ), + ( + "MP_REACH + NH", + build_update( + vec![], + build_attrs(true, true, true, Some(dummy_mp_reach.clone()), None), + vec![], + ), + 0, + ), + ( + "MP_UNREACH Only", + build_update( + vec![], + build_attrs(false, false, false, None, Some(dummy_mp_unreach.clone())), + vec![], + ), + 0, + ), // --- Mixed Scenarios (IPv4 Withdraw + MP_REACH Announcement) --- - ("IPv4 Withdraw + MP_REACH (AS+ORIGIN)", build_update(vec![0x18, 0x0A, 0x00, 0x00], build_attrs(true, true, false, Some(dummy_mp_reach.clone()), None), vec![]), 0), - ("IPv4 Withdraw + MP_REACH (ORIGIN only)", build_update(vec![0x18, 0x0A, 0x00, 0x00], build_attrs(true, false, false, Some(dummy_mp_reach.clone()), None), vec![]), 1), - ("IPv4 Withdraw + MP_REACH (AS+ORIGIN+NH)", build_update(vec![0x18, 0x0A, 0x00, 0x00], build_attrs(true, true, true, Some(dummy_mp_reach.clone()), None), vec![]), 0), - + ( + "IPv4 Withdraw + MP_REACH (AS+ORIGIN)", + build_update( + vec![0x18, 0x0A, 0x00, 0x00], + build_attrs(true, true, false, Some(dummy_mp_reach.clone()), None), + vec![], + ), + 0, + ), + ( + "IPv4 Withdraw + MP_REACH (ORIGIN only)", + build_update( + vec![0x18, 0x0A, 0x00, 0x00], + build_attrs(true, false, false, Some(dummy_mp_reach.clone()), None), + vec![], + ), + 1, + ), + ( + "IPv4 Withdraw + MP_REACH (AS+ORIGIN+NH)", + build_update( + vec![0x18, 0x0A, 0x00, 0x00], + build_attrs(true, true, true, Some(dummy_mp_reach.clone()), None), + vec![], + ), + 0, + ), // --- Mixed Scenarios (IPv4 Announce + MP_REACH Announcement) --- - ("IPv4 Announce + MP_REACH + NH", build_update(vec![], build_attrs(true, true, true, Some(dummy_mp_reach.clone()), None), vec![0x18, 0x0A, 0x00, 0x00]), 0), - ("IPv4 Announce + MP_REACH - NH", build_update(vec![], build_attrs(true, true, false, Some(dummy_mp_reach.clone()), None), vec![0x18, 0x0A, 0x00, 0x00]), 1), + ( + "IPv4 Announce + MP_REACH + NH", + build_update( + vec![], + build_attrs(true, true, true, Some(dummy_mp_reach.clone()), None), + vec![0x18, 0x0A, 0x00, 0x00], + ), + 0, + ), + ( + "IPv4 Announce + MP_REACH - NH", + build_update( + vec![], + build_attrs(true, true, false, Some(dummy_mp_reach.clone()), None), + vec![0x18, 0x0A, 0x00, 0x00], + ), + 1, + ), ]; for (name, msg, expected_warnings) in scenarios { @@ -72,7 +168,13 @@ fn test_validation_combinations() { match res { Ok(update) => { let warnings = update.attributes.validation_warnings(); - assert_eq!(warnings.len(), expected_warnings, "Scenario '{}' failed. Warnings: {:?}", name, warnings); + assert_eq!( + warnings.len(), + expected_warnings, + "Scenario '{}' failed. Warnings: {:?}", + name, + warnings + ); } Err(e) => panic!("Scenario '{}' failed to parse: {}", name, e), }