Repository navigation
pfSense: translate a missing <protocol> and source ports exactly (#80) - #367
Merged
Merged
Conversation
pfSense's own source answers #80: it saves "any" by unsetting <protocol>, so `ip` is correct for protocol-less rules. Two cases were not, and both widened silently: no <protocol> + a port pfSense enforces tcp, any port was: ip tcp/udp + source port pfSense enforces the port was: dropped Both are now written exactly (`tcp`, and `eq N` after the source). Refusing them would skip pass rules, which narrows the model and can turn a real policy violation into a clean result (#302). A source port that is not one valid number refuses, as destination ports already do. Verified in Batfish: every flow tested now matches pfSense; the old conversion denied UDP a block rule never covered, and permitted a source port a rule never allowed.
ARSH871-bot
changed the base branch from
fix/78-rule-level-resilience
to
main
September 28, 2026 01:53
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.
Closes #80. Stacked on #302, so it merges after it; retarget to
mainbefore #302 lands.@patelankeet2, this also answers your 5 September question on #80 about Option C. There's no silent default left to disclose: every absent-
<protocol>case is now pfSense's own documented behaviour, cited in the code.#80's question, answered from pfSense's source
Senaka hadn't replied in 25 days (Discussion #294), and the answer turned out to be in pfSense itself. From
github.com/pfsense/pfsenseat480a1c0626:firewall_rules_edit.php:1195<protocol>. Absence is "any".firewall_rules_edit.php:619filter.inc:4497<protocol>but a port set givesproto tcp.filter.inc:3930So
ipis right for the client's four protocol-less rules, and refusing them (Option B) would have been wrong. Two cases next to it were silently widened, and they're what this PR fixes:<protocol>+ a port (hand-edited XML only)iptcpeq Nafter the sourceany+ portipip(unchanged, now pinned)icmpicmp(unchanged, now pinned)Why translate rather than refuse
I first wrote this as refusals and threw it away. Refusing skips the rule, and @SamikaPerera's finding on #302 applies: skipping a
passrule narrows the model, so a "must be denied" policy can come back clean while the real firewall permits the traffic. Translating exactly is the only option that is wrong in neither direction. A source port that isn't one valid number (a range or an alias) still refuses, exactly as destination ports already do.Verified in Batfish, not only in unit tests
The same synthetic file, converted by #302's head and by this branch, checked with
testFilters:The first row was the dangerous direction: the old model reported traffic as blocked that the real firewall lets through.
Tests
tests/test_pfsense_convert.py.test_port_is_not_emitted_for_protocol_anyis untouched: pfSense's source agrees with it.anylike an absent protocol, keeping the port on the inferred-TCP line, skipping source-port validation, and writing a source port for icmp.anymutation was caught only by a test added for it. Before that test, nothing distinguished the two cases.CLAUDE.md §7 gets one dated update; the earlier entries saying #80 was untouched are left as history.
Not in this PR