Skip to content

refactor: move mandatory attribute validation after NLRI parsing (RFC 4760) #280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
digizeph wants to merge 7 commits into from
Closed

refactor: move mandatory attribute validation after NLRI parsing (RFC 4760) #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c75c28a
fix: suppress NEXT_HOP warning when MP_REACH_NLRI is present (RFC 4760)
digizeph Apr 15, 2026
73bed4b
refactor: move mandatory attribute validation after NLRI parsing
digizeph Apr 15, 2026
90385dc
docs: add RFC 4271 and RFC 4760 text to validate_mandatory_attributes
digizeph Apr 15, 2026
de9a100
docs: move RFC text inline with validation checks
digizeph Apr 15, 2026
f2ad9c4
docs: clarify ORIGIN/AS_PATH required for ANY announcement
digizeph Apr 15, 2026
4170417
docs: add RFC 4760 Section 3 text for ORIGIN/AS_PATH
digizeph Apr 15, 2026
aa37708
docs: restructure NEXT_HOP validation comment for clarity
digizeph Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
265 changes: 229 additions & 36 deletions src/parser/bgp/attributes/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,14 +172,18 @@ 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,
add_path: bool,
afi: Option<Afi>,
safi: Option<Safi>,
prefixes: Option<&[NetworkPrefix]>,
) -> Result<Attributes, ParserError> {
) -> 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);
Expand Down Expand Up @@ -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<BgpValidationWarning> {
// 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 {
Expand Down Expand Up @@ -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(),
Expand All @@ -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] {
Expand All @@ -526,23 +590,27 @@ 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;
let afi = None;
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!(
Expand All @@ -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]
Expand All @@ -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());
Expand Down Expand Up @@ -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());
Expand All @@ -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| {
Expand All @@ -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());
Expand Down Expand Up @@ -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"
);
}
}
Loading
Loading