refactor: move mandatory attribute validation after NLRI parsing (RFC 4760)#280
Closed
refactor: move mandatory attribute validation after NLRI parsing (RFC 4760)#280
Conversation
Per RFC 4760 Section 3, UPDATE messages carrying no NLRI other than what's encoded in MP_REACH_NLRI should not carry a separate NEXT_HOP attribute, as the next hop is embedded within MP_REACH_NLRI itself. This eliminates false positive warnings for valid MP-BGP messages. Caveat: may produce false negatives for rare edge cases where an UPDATE contains both regular NLRI and MP_REACH_NLRI but lacks NEXT_HOP.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #280 +/- ##
==========================================
+ Coverage 90.53% 90.55% +0.01%
==========================================
Files 84 84
Lines 15835 15847 +12
==========================================
+ Hits 14336 14350 +14
+ Misses 1499 1497 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Complete refactoring of BGP mandatory attribute validation to handle RFC 4760 (MP-BGP) semantics correctly: **Breaking change:** - parse_attributes() now returns (Attributes, [bool; 256]) tuple to track seen attribute types for post-parsing validation **New validation logic:** - ORIGIN and AS_PATH: required for any announcement (IPv4 or MP_REACH_NLRI) - NEXT_HOP: required only when IPv4 NLRI is present (RFC 4271) - No mandatory attributes for withdrawal-only messages (RFC 4760 Section 4) - MP_REACH_NLRI carries embedded next hop, no separate NEXT_HOP needed **Validation moved from parse_attributes() to parse_bgp_update_message()** after NLRI parsing, enabling proper context-aware checks. **Files changed:** - src/parser/bgp/attributes/mod.rs: return tuple, add validate_mandatory_attributes() - src/parser/bgp/messages.rs: call validation after NLRI parsing - src/parser/mrt/messages/table_dump*.rs: handle tuple return - CHANGELOG.md: document breaking change and bug fix
Include verbatim RFC excerpts to help readers understand the actual validation rules and their RFC provenance: - RFC 4271 Section 6.3: ORIGIN, AS_PATH, NEXT_HOP as well-known mandatory - RFC 4760 Section 3: MP_REACH_NLRI requirements, NEXT_HOP omission - RFC 4760 Section 4: MP_UNREACH_NLRI requires no other attributes
Place RFC 4271 and RFC 4760 excerpts directly next to each check inside validate_mandatory_attributes() for better readability: - RFC 4760 Section 4 comment at withdrawals check - RFC 4271/4760 Section 3 comments at ORIGIN check - RFC 4271/4760 Section 3 comments at AS_PATH check - RFC 4271 Section 6.3 + RFC 4760 Section 3 comments at NEXT_HOP check
Update RFC comments to clarify that: - RFC 4271 Section 6.3 is the primary source (well-known mandatory) - RFC 4760 Section 3 clarifies this applies to MP_REACH_NLRI too - Therefore the check applies to ANY announcement (IPv4 or MP)
Explain why ORIGIN and AS_PATH are required for MP_REACH_NLRI using the actual RFC 4760 text: 'An UPDATE message that carries the MP_REACH_NLRI MUST also carry the ORIGIN and the AS_PATH attributes...' This clarifies that RFC 4271's mandatory requirement extends to multiprotocol announcements as well.
Follow RFC 4760 Section 3 logic flow more naturally: 1. First quote RFC 4760 on when NEXT_HOP should NOT be present (when only MP_REACH_NLRI carries the NLRI) 2. Explain in plain English what this means for validation 3. Place RFC 4271 quote inside the check block, showing this is where we enforce the mandatory requirement for IPv4 NLRI This avoids the double-negative confusion and makes the RFC-to-code mapping clearer.
Member
Author
|
prefer #281 implementation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Complete refactoring of BGP mandatory attribute validation to properly handle RFC 4760 (MP-BGP) "conditionally mandatory" semantics. The validation now occurs after NLRI parsing, enabling context-aware checks that eliminate false positives without introducing false negatives.
Breaking Change
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 context is known.RFC Compliance
RFC 4271 (BGP-4)
ORIGINandAS_PATH: required for any announcementNEXT_HOP: required when IPv4 NLRI is presentRFC 4760 (Multiprotocol Extensions)
MP_REACH_NLRIcarries embedded next hop; no separateNEXT_HOPattribute needed when only MP prefixes are announcedMP_UNREACH_NLRIonly) require no attributesChanges
Core Changes
parse_attributes(): Now returns(Attributes, seen_attributes)tuple; removed inline mandatory validationvalidate_mandatory_attributes(): New public function that performs context-aware validation after NLRI parsingparse_bgp_update_message(): Calls validation after NLRI parsing with full context (has_ipv4_nlri, has_mp_reach_nlri, has_mp_unreach_nlri)Test Updates
test_rfc7606_missing_mandatory_attributetotest_mandatory_attributes_validationwith comprehensive NLRI context scenariosMRT Parsers
Validation Logic
Testing
All existing tests pass. Validation now correctly handles:
Files Changed
src/parser/bgp/attributes/mod.rs- Core validation logicsrc/parser/bgp/messages.rs- UPDATE parsing with post-NLRI validationsrc/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs- Handle tuple returnsrc/parser/mrt/messages/table_dump.rs- Handle tuple returnCHANGELOG.md- Document breaking change and fix