diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d6a5bf..fc0b6d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,20 @@ All notable changes to this project will be documented in this file. ## Unreleased +### Breaking changes + +* **`parse_attributes()` signature change**: The function now returns a tuple `(Attributes, [bool; 256])` where the second element tracks which attribute types were seen during parsing. This enables proper mandatory attribute validation after NLRI parsing. Callers need to update to handle the tuple return. + +### Bug fixes + +* **RFC 4760 mandatory attribute validation**: Moved mandatory attribute validation from `parse_attributes()` to after NLRI parsing in `parse_bgp_update_message()`. This allows proper context-aware validation per RFC 4271 and RFC 4760: + - `ORIGIN` and `AS_PATH`: required for any announcement (IPv4 NLRI or `MP_REACH_NLRI`) + - `NEXT_HOP`: required only when IPv4 NLRI is present (RFC 4271) + - No mandatory attributes required for withdrawal-only messages (RFC 4760 Section 4) + - `MP_REACH_NLRI` carries its own next hop, so no separate `NEXT_HOP` attribute needed (RFC 4760 Section 3) + + This eliminates false positive warnings for valid MP-BGP messages and properly handles the "conditionally mandatory" nature of BGP attributes. + ## v0.16.0 - 2026-04-07 ### Breaking changes diff --git a/src/parser/bgp/attributes/mod.rs b/src/parser/bgp/attributes/mod.rs index b8faa01..da96565 100644 --- a/src/parser/bgp/attributes/mod.rs +++ b/src/parser/bgp/attributes/mod.rs @@ -172,6 +172,10 @@ fn validate_attribute_length( /// /// The `data: &[u8]` contains the entirety of the attributes bytes, therefore the size of /// the slice is the total byte length of the attributes section of the message. +/// +/// Returns the parsed attributes and a boolean array tracking which attribute types were seen. +/// Mandatory attribute validation should be performed after NLRI parsing using +/// `validate_mandatory_attributes()` with proper context. pub fn parse_attributes( mut data: Bytes, asn_len: &AsnLength, @@ -179,7 +183,7 @@ pub fn parse_attributes( afi: Option, safi: Option, prefixes: Option<&[NetworkPrefix]>, -) -> Result { +) -> Result<(Attributes, [bool; 256]), ParserError> { // Estimate capacity from data size: each attribute is at least 3 bytes // (flag + type + length). Cap at 256 to avoid over-allocation for corrupted data. let estimated_attrs = (data.remaining() / 3).min(256); @@ -378,25 +382,84 @@ 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((result, seen_attributes)) +} + +/// Validate mandatory attributes based on NLRI context. +/// +/// This should be called after NLRI parsing to properly determine which attributes +/// are mandatory based on the message content per RFC 4271 and RFC 4760. +pub fn validate_mandatory_attributes( + seen_attributes: &[bool; 256], + has_ipv4_nlri: bool, + has_mp_reach_nlri: bool, + _has_mp_unreach_nlri: bool, +) -> Vec { + // RFC 4760 Section 4: + // "An UPDATE message that contains the MP_UNREACH_NLRI is not required + // to carry any other path attributes." + // + // RFC 4760 Section 3: + // "An UPDATE message that carries the MP_REACH_NLRI MUST also carry the + // ORIGIN and the AS_PATH attributes..." + // + // Therefore: if no announcements (withdrawals only), no mandatory attributes required. + let has_announcements = has_ipv4_nlri || has_mp_reach_nlri; + if !has_announcements { + return vec![]; + } + + let mut warnings = Vec::new(); + + // RFC 4271 Section 6.3: + // "ORIGIN (Type Code 1): + // [...] well-known mandatory attribute [...]" + // + // RFC 4760 Section 3: + // "An UPDATE message that carries the MP_REACH_NLRI MUST also carry the + // ORIGIN and the AS_PATH attributes..." + // + // This confirms ORIGIN is required for MP_REACH_NLRI announcements as well. + if !seen_attributes[u8::from(AttrType::ORIGIN) as usize] { + warnings.push(BgpValidationWarning::MissingWellKnownAttribute { + attr_type: AttrType::ORIGIN, + }); + } + + // RFC 4271 Section 6.3: + // "AS_PATH (Type Code 2): + // [...] well-known mandatory attribute [...]" + // + // RFC 4760 Section 3: + // "An UPDATE message that carries the MP_REACH_NLRI MUST also carry the + // ORIGIN and the AS_PATH attributes..." + // + // This confirms AS_PATH is required for MP_REACH_NLRI announcements as well. + if !seen_attributes[u8::from(AttrType::AS_PATH) as usize] { + warnings.push(BgpValidationWarning::MissingWellKnownAttribute { + attr_type: AttrType::AS_PATH, + }); + } + + // RFC 4760 Section 3: + // "An UPDATE message that carries no NLRI, other than the one encoded in + // the MP_REACH_NLRI attribute, SHOULD NOT carry the NEXT_HOP attribute." + // + // This means NEXT_HOP is only needed when there is regular IPv4 NLRI. + // MP_REACH_NLRI carries its own next-hop internally, so no separate NEXT_HOP + // attribute is required for multiprotocol routes. + if has_ipv4_nlri && !seen_attributes[u8::from(AttrType::NEXT_HOP) as usize] { + // RFC 4271 Section 6.3: + // "NEXT_HOP (Type Code 3): + // [...] well-known mandatory attribute [...]" + warnings.push(BgpValidationWarning::MissingWellKnownAttribute { + attr_type: AttrType::NEXT_HOP, + }); + } + + warnings } impl Attribute { @@ -489,9 +552,9 @@ mod tests { let afi = None; let safi = None; let prefixes = None; - let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes); - assert!(attributes.is_ok()); - let attributes = attributes.unwrap(); + let result = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes); + assert!(result.is_ok()); + let (attributes, _seen_attributes) = result.unwrap(); assert_eq!(attributes.inner.len(), 1); assert_eq!( attributes.inner[0].value.attr_type(), @@ -509,12 +572,13 @@ mod tests { let safi = None; let prefixes = None; - let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let (attributes, _seen_attributes) = + parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); // Should have validation warning for incorrect flags assert!(attributes.has_validation_warnings()); let warnings = attributes.validation_warnings(); - // Will have attribute flags error + missing mandatory attributes + // Will have attribute flags error (mandatory warnings moved to post-NLRI validation) assert!(!warnings.is_empty()); match &warnings[0] { @@ -526,8 +590,9 @@ mod tests { } #[test] - fn test_rfc7606_missing_mandatory_attribute() { - // Empty attributes - should have warnings for missing mandatory attributes + fn test_mandatory_attributes_validation() { + // Test that validate_mandatory_attributes correctly requires + // ORIGIN, AS_PATH, and NEXT_HOP based on NLRI context let data = Bytes::from(vec![]); let asn_len = AsnLength::Bits16; let add_path = false; @@ -535,14 +600,17 @@ mod tests { let safi = None; let prefixes = None; - let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let (_attributes, seen_attributes) = + parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); - // Should have warnings for missing mandatory attributes - assert!(attributes.has_validation_warnings()); - let warnings = attributes.validation_warnings(); - assert_eq!(warnings.len(), 3); // ORIGIN, AS_PATH, NEXT_HOP + // Empty attributes with no announcements should have no mandatory warnings + let warnings = validate_mandatory_attributes(&seen_attributes, false, false, false); + assert!(warnings.is_empty()); - for warning in warnings { + // Empty attributes with IPv4 NLRI should warn about all three + let warnings = validate_mandatory_attributes(&seen_attributes, true, false, false); + assert_eq!(warnings.len(), 3); // ORIGIN, AS_PATH, NEXT_HOP + for warning in &warnings { match warning { BgpValidationWarning::MissingWellKnownAttribute { attr_type } => { assert!(matches!( @@ -553,6 +621,21 @@ mod tests { _ => panic!("Expected MissingWellKnownAttribute warning"), } } + + // Empty attributes with MP_REACH_NLRI should only warn about ORIGIN and AS_PATH + let warnings = validate_mandatory_attributes(&seen_attributes, false, true, false); + assert_eq!(warnings.len(), 2); // ORIGIN, AS_PATH (no NEXT_HOP per RFC 4760) + for warning in &warnings { + match warning { + BgpValidationWarning::MissingWellKnownAttribute { attr_type } => { + assert!( + matches!(attr_type, AttrType::ORIGIN | AttrType::AS_PATH), + "Should only warn about ORIGIN or AS_PATH for MP_REACH_NLRI only" + ); + } + _ => panic!("Expected MissingWellKnownAttribute warning"), + } + } } #[test] @@ -568,7 +651,8 @@ mod tests { let safi = None; let prefixes = None; - let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let (attributes, _seen_attributes) = + parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); // Should have warning for duplicate attribute assert!(attributes.has_validation_warnings()); @@ -601,7 +685,8 @@ mod tests { data.extend_from_slice(&[0x40, 0xFF, 0x01, 0x00]); // development let data = Bytes::from(data); - let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let (attributes, _seen_attributes) = + parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); assert!(attributes.has_attr(AttrType::DEVELOPMENT)); assert!(!attributes.has_validation_warnings()); @@ -611,7 +696,8 @@ mod tests { data.extend_from_slice(&[0x40, 0x00, 0x01, 0x01]); // reserved let data = Bytes::from(data); - let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let (attributes, _seen_attributes) = + parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); // There is a validation warning about the reserved attribute assert!(attributes.validation_warnings.iter().any(|vw| { @@ -629,7 +715,8 @@ mod tests { let safi = None; let prefixes = None; - let attributes = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + let (attributes, _seen_attributes) = + parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); // Should have warning for incorrect attribute length assert!(attributes.has_validation_warnings()); @@ -659,11 +746,117 @@ mod tests { let result = parse_attributes(data, &asn_len, add_path, afi, safi, prefixes); assert!(result.is_ok()); - let attributes = result.unwrap(); + let (attributes, _seen_attributes) = result.unwrap(); assert!(attributes.has_validation_warnings()); // Should have multiple warnings but parsing should continue let warnings = attributes.validation_warnings(); assert!(!warnings.is_empty()); } + + #[test] + fn test_rfc4760_mp_reach_nlri_no_next_hop_validation() { + // RFC 4760 Section 3: When MP_REACH_NLRI is present, the NEXT_HOP attribute + // should not be present (the next hop is carried within MP_REACH_NLRI itself). + // Test validate_mandatory_attributes with MP_REACH_NLRI context. + let data = Bytes::from(vec![ + // ORIGIN attribute (type 1) - required + 0x40, 0x01, 0x01, 0x00, // flags (transitive), type 1, length 1, value IGP + // AS_PATH attribute (type 2) - required + 0x40, 0x02, 0x00, // flags (transitive), type 2, length 0 (empty AS_PATH) + // MP_REACH_NLRI attribute (type 14) with IPv4 unicast + 0x80, 0x0E, // flags (optional), type 14 + 0x0D, // length: 13 bytes (AFI(2)+SAFI(1)+NH-len(1)+NH(4)+Reserved(1)+Prefix-len(1)+Prefix(3)) + 0x00, 0x01, // AFI: IPv4 + 0x01, // SAFI: Unicast + 0x04, // Next hop length: 4 bytes + 0xC0, 0x00, 0x02, 0x01, // Next hop: 192.0.2.1 + 0x00, // Reserved + 0x18, // Prefix length: 24 bits + 0xC0, 0x00, + 0x02, // NLRI: 192.0.2.0/24 + // Note: No NEXT_HOP attribute (type 3) + ]); + let asn_len = AsnLength::Bits16; + let add_path = false; + let afi = None; + let safi = None; + let prefixes = None; + + let (attributes, seen_attributes) = + parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + + // MP_REACH_NLRI should be present + assert!(attributes.has_attr(AttrType::MP_REACHABLE_NLRI)); + // NEXT_HOP should not be present + assert!(!attributes.has_attr(AttrType::NEXT_HOP)); + + // Test with MP_REACH_NLRI only (no IPv4 NLRI) - should not warn about NEXT_HOP + let warnings = validate_mandatory_attributes(&seen_attributes, false, true, false); + assert!( + !warnings.iter().any(|w| matches!( + w, + BgpValidationWarning::MissingWellKnownAttribute { attr_type } if *attr_type == AttrType::NEXT_HOP + )), + "Should not warn about missing NEXT_HOP when only MP_REACH_NLRI is present (RFC 4760)" + ); + + // Test with both IPv4 NLRI and MP_REACH_NLRI - should warn about missing NEXT_HOP + let warnings = validate_mandatory_attributes(&seen_attributes, true, true, false); + assert!( + warnings.iter().any(|w| matches!( + w, + BgpValidationWarning::MissingWellKnownAttribute { attr_type } if *attr_type == AttrType::NEXT_HOP + )), + "Should warn about missing NEXT_HOP when IPv4 NLRI is present" + ); + } + + #[test] + fn test_rfc4760_mp_reach_nlri_with_next_hop_present() { + // RFC 4760 Section 3: If NEXT_HOP is present when MP_REACH_NLRI is present, + // it should be ignored (no warning should be generated for having it) + let data = Bytes::from(vec![ + // ORIGIN attribute (type 1) - required + 0x40, 0x01, 0x01, 0x00, // flags (transitive), type 1, length 1, value IGP + // AS_PATH attribute (type 2) - required + 0x40, 0x02, 0x00, // flags (transitive), type 2, length 0 (empty AS_PATH) + // NEXT_HOP attribute (type 3) - present + 0x40, 0x03, 0x04, // flags (transitive), type 3, length 4 + 0xC0, 0x00, 0x02, 0x01, // Next hop: 192.0.2.1 + // MP_REACH_NLRI attribute (type 14) with IPv6 unicast + 0x80, 0x0E, // flags (optional), type 14 + 0x1E, // length: 30 bytes (AFI(2)+SAFI(1)+NH-len(1)+NH(16)+Reserved(1)+Prefix-len(1)+Prefix(8)) + 0x00, 0x02, // AFI: IPv6 + 0x01, // SAFI: Unicast + 0x10, // Next hop length: 16 bytes + // IPv6 next hop: 2001:db8::1 + 0x20, 0x01, 0x0d, 0xb8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x01, 0x00, // Reserved + 0x40, // Prefix length: 64 bits + 0x20, 0x01, 0x0d, 0xb8, 0x00, 0x00, 0x00, 0x00, // NLRI: 2001:db8::/64 + ]); + let asn_len = AsnLength::Bits16; + let add_path = false; + let afi = None; + let safi = None; + let prefixes = None; + + let (attributes, seen_attributes) = + parse_attributes(data, &asn_len, add_path, afi, safi, prefixes).unwrap(); + + // Both NEXT_HOP and MP_REACH_NLRI should be present + assert!(attributes.has_attr(AttrType::NEXT_HOP)); + assert!(attributes.has_attr(AttrType::MP_REACHABLE_NLRI)); + + // Validate with no IPv4 NLRI - should have no warnings (NEXT_HOP is optional here) + let warnings = validate_mandatory_attributes(&seen_attributes, false, true, false); + assert!( + !warnings.iter().any(|w| matches!( + w, + BgpValidationWarning::MissingWellKnownAttribute { attr_type } if *attr_type == AttrType::NEXT_HOP + )), + "Should not warn about NEXT_HOP when MP_REACH_NLRI is present" + ); + } } diff --git a/src/parser/bgp/messages.rs b/src/parser/bgp/messages.rs index 3253c23..4e86173 100644 --- a/src/parser/bgp/messages.rs +++ b/src/parser/bgp/messages.rs @@ -10,7 +10,7 @@ use crate::models::capabilities::{ MultiprotocolExtensionsCapability, RouteRefreshCapability, }; use crate::models::error::BgpError; -use crate::parser::bgp::attributes::parse_attributes; +use crate::parser::bgp::attributes::{parse_attributes, validate_mandatory_attributes}; use crate::parser::{encode_nlri_prefixes, parse_nlri_list, ReadUtils}; use log::warn; use zerocopy::big_endian::{U16, U32}; @@ -468,12 +468,31 @@ 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, seen_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)?; + // Determine NLRI context for mandatory attribute validation + let has_ipv4_nlri = !announced_prefixes.is_empty(); + let has_mp_reach_nlri = seen_attributes[u8::from(AttrType::MP_REACHABLE_NLRI) as usize]; + let has_mp_unreach_nlri = seen_attributes[u8::from(AttrType::MP_UNREACHABLE_NLRI) as usize]; + + // Validate mandatory attributes based on NLRI context (RFC 4271, RFC 4760) + let mandatory_warnings = validate_mandatory_attributes( + &seen_attributes, + has_ipv4_nlri, + has_mp_reach_nlri, + has_mp_unreach_nlri, + ); + + // Add mandatory validation warnings to attributes + for warning in mandatory_warnings { + attributes.add_validation_warning(warning); + } + 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..adac402 100644 --- a/src/parser/mrt/messages/table_dump.rs +++ b/src/parser/mrt/messages/table_dump.rs @@ -99,7 +99,7 @@ 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 (attributes, _seen_attributes) = parse_attributes(attr_data_slice, &AsnLength::Bits16, false, None, None, None)?; Ok(TableDumpMessage { 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..1149b88 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 (attributes, _seen_attributes) = parse_attributes( attr_data_slice, &AsnLength::Bits32, is_add_path,