firewall: T8247: Allow both protocol names and numbers consistently#4978
firewall: T8247: Allow both protocol names and numbers consistently#4978Nyandorew wants to merge 1 commit intovyos:currentfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
👍 |
|
I have read the CLA Document and I hereby sign the CLA |
- Add PROTOCOL_NUMBER_TO_NAME mapping for IANA protocol numbers
- Normalize protocol numbers to names early in verify_rule()
- Fix inconsistent behavior when protocol numbers used with various options
- Ensures protocol numbers (e.g., 6 for TCP) work consistently across:
* Port specifications
* TCP flags
* GRE settings
* synproxy action
* ICMP protocol validation
2239417 to
ad9154c
Compare
|
CI integration ❌ failed! Details
|
Nyandorew
left a comment
There was a problem hiding this comment.
I've tested this code and it works fine.
| # Protocol number to name mapping (IANA Protocol Numbers) | ||
| # This ensures consistent handling of numeric protocol specifications | ||
| PROTOCOL_NUMBER_TO_NAME = { | ||
| '1': 'icmp', | ||
| '2': 'igmp', | ||
| '6': 'tcp', | ||
| '17': 'udp', | ||
| '47': 'gre', | ||
| '50': 'esp', | ||
| '51': 'ah', | ||
| '58': 'ipv6-icmp', | ||
| '89': 'ospf', | ||
| '132': 'sctp', | ||
| } | ||
|
|
There was a problem hiding this comment.
I do not believe this should be hardcoded. Instead, we could use nftables’ built-in interface to retrieve the list of supported protocols (nft describe inet_proto), or at minimum, reference /etc/protocols, which, if I recall correctly, is already used for validation in certain parts of the CLI.
There was a problem hiding this comment.
Acknowledged—thanks for the suggestion.
I’ll update this to avoid hardcoding and switch to using nftables’ built-in interface (nft describe inet_proto) or, alternatively, referencing /etc/protocols.
This will require a bit of investigation and implementation work, so could you please allow me a few days to post an update?
|
@Nyandorew I see, you requested the review earlier, but there were no changes after the previous one - maybe this was done accidentally? Could you confirm that you are still working on the PR? |
Related Task(s)
https://vyos.dev/T8247
Change summary
Fixes inconsistent handling of protocol specification in firewall configuration. When users specify protocol by IANA number (e.g.,
6for TCP), certain options like port specifications, TCP flags, and GRE settings would fail validation because the checks compare against string names.This PR normalizes protocol numbers to their string names early in
verify_rule()so all subsequent validations work consistently.Example of the bug:
Types of changes
Related PR(s)
Component(s) name
src/conf_mode/firewall.pyHow to test
Test result
test_firewall_protocol_result.txt
test_firewall_protocol.sh
Checklist:
Requested by: @Nyandorew