Draft: add SNAC flag to inet6 #4898
Conversation
its a recent flag mainly added for Thread https://www.iana.org/assignments/icmpv6-parameters/icmpv6-parameters.xhtml#icmpv6-parameters-11
There was a problem hiding this comment.
Pull request overview
This PR adds support for the SNAC (6loWPAN Neighbor Discovery) flag to the ICMPv6 Router Advertisement (ND_RA) message structure. The flag is primarily used in 6loWPAN networks like Thread, as documented in the IANA ICMPv6 parameters registry.
Changes:
- Added a 1-bit S flag field to the ICMPv6ND_RA class for the SNAC flag
- Reduced the reserved field from 2 bits to 1 bit to accommodate the new flag
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BitField("O", 0, 1), | ||
| BitField("H", 0, 1), | ||
| BitEnumField("prf", 1, 2, icmp6ndraprefs), # RFC 4191 | ||
| BitField("P", 0, 1), |
There was a problem hiding this comment.
The P flag on line 2210 also lacks an RFC comment reference. For consistency with the documentation standards used in this file (see line 2209 for example), consider adding the appropriate RFC reference for the P flag (NDP Proxy flag). This would make the codebase more maintainable and help developers understand the origin of each flag.
| BitField("P", 0, 1), | |
| BitField("P", 0, 1), # RFC 4389 |
scapy/layers/inet6.py
Outdated
| BitEnumField("prf", 1, 2, icmp6ndraprefs), # RFC 4191 | ||
| BitField("P", 0, 1), | ||
| BitField("res", 0, 2), | ||
| BitField("S", 0, 1), |
There was a problem hiding this comment.
The newly added S flag lacks test coverage. While the PR description mentions that tests weren't added because the flag is typically 0 unless there's a border router on the network, the test file at test/scapy/layers/inet6.uts already has extensive test coverage for ICMPv6ND_RA including tests for the P flag. Consider adding at least basic instantiation and dissection tests that verify the S flag can be set to 1 and correctly parsed, similar to how other flags in the structure are tested. This ensures the bit field is correctly positioned and functional.
scapy/layers/inet6.py
Outdated
| BitEnumField("prf", 1, 2, icmp6ndraprefs), # RFC 4191 | ||
| BitField("P", 0, 1), | ||
| BitField("res", 0, 2), | ||
| BitField("S", 0, 1), |
There was a problem hiding this comment.
The new S flag should include an RFC comment reference similar to other flags in this class. According to the PR description, this flag is for 6loWPAN Neighbor Discovery (SNAC flag) as documented in the IANA registry. Consider adding a comment like "# RFC 6775" or similar RFC number that defines this flag to maintain consistency with the prf field comment on line 2209.
| BitField("S", 0, 1), | |
| BitField("S", 0, 1), # RFC 6775 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4898 +/- ##
=======================================
Coverage 80.84% 80.84%
=======================================
Files 369 369
Lines 90963 90963
=======================================
+ Hits 73539 73543 +4
+ Misses 17424 17420 -4
🚀 New features to boost your workflow:
|
1 participant
Checklist:
tox)add a recent flag mainly added for 6loWPAN networks like Thread https://www.iana.org/assignments/icmpv6-parameters/icmpv6-parameters.xhtml#icmpv6-parameters-11
I have not added tests as it is 0 in most cases unless there is a border router on the network, also its neighbouring flag, NDP Proxy, is also not tested.
A long with the tox test(which i have not been able to run), I had modified the version in the virtual environment of the project I'm working on and can confirm it behaves as expected on network packets