diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d6a5bf7..01603e53 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 0242b55e..f2a51a36 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,48 @@ 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, since is_announcement. + let has_mp_reach = self.has_attr(AttrType::MP_REACHABLE_NLRI); + if (has_standard_nlri || !has_mp_reach) && !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 +375,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 +423,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, } } } @@ -400,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 b8faa019..84bbd22f 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); @@ -378,25 +385,11 @@ 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] { - validation_warnings.push(BgpValidationWarning::MissingWellKnownAttribute { - attr_type: mandatory_attr, - }); - } - } - - 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 { @@ -527,19 +520,25 @@ 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; 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()); 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 +554,69 @@ 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 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 + } + ) + }); + 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 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()); + + // Attributes with only MP_UNREACH_NLRI - pure withdrawal + let data = Bytes::from(vec![ + 0x80, 0x0F, 0x03, 0x00, 0x01, 0x01, // AFI=1, SAFI=1 + ]); + 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()); + } + #[test] fn test_rfc7606_duplicate_attribute() { // Create two ORIGIN attributes diff --git a/src/parser/bgp/messages.rs b/src/parser/bgp/messages.rs index 3253c23c..1333c6ea 100644 --- a/src/parser/bgp/messages.rs +++ b/src/parser/bgp/messages.rs @@ -468,12 +468,18 @@ 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); + 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 3f64e682..4d201852 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 c2513136..59e5c554 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, diff --git a/tests/test_bgp_update_validation.rs b/tests/test_bgp_update_validation.rs new file mode 100644 index 00000000..d3fbb8c3 --- /dev/null +++ b/tests/test_bgp_update_validation.rs @@ -0,0 +1,182 @@ +use bgpkit_parser::models::*; +use bgpkit_parser::parser::bgp::messages::parse_bgp_update_message; +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), + } + } +}