firewall: T8446: Prevent chain with offload rule on local zone by sarthurdev · Pull Request #5100 · vyos/vyos-1x

sarthurdev · 2026-03-31T21:18:47Z

Change summary

Prevent chain with offload rule being used to/from zone-policy local-zone
Add smoketest coverage
Add warning when defining offload-target without setting action to offload

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code style update (formatting, renaming)
Refactoring (no functional changes)
Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
Other (please describe):

Related Task(s)

Related PR(s)

How to test / Smoketest result

DEBUG - Running Testcase (1/1): /usr/libexec/vyos/tests/smoke/cli/test_firewall.py
DEBUG - test_bridge_firewall (__main__.TestFirewall.test_bridge_firewall) ... ok
DEBUG - test_cyclic_jump_validation (__main__.TestFirewall.test_cyclic_jump_validation) ... ok
DEBUG - test_disable_conntrack_per_chain (__main__.TestFirewall.test_disable_conntrack_per_chain) ... ok
DEBUG - test_flow_offload (__main__.TestFirewall.test_flow_offload) ... ok
DEBUG - test_geoip (__main__.TestFirewall.test_geoip) ... ok
DEBUG - test_gre_match (__main__.TestFirewall.test_gre_match) ... ok
DEBUG - test_groups (__main__.TestFirewall.test_groups) ... ok
DEBUG - test_ipsec_metadata_match (__main__.TestFirewall.test_ipsec_metadata_match) ... ok
DEBUG - test_ipv4_advanced (__main__.TestFirewall.test_ipv4_advanced) ... ok
DEBUG - test_ipv4_basic_rules (__main__.TestFirewall.test_ipv4_basic_rules) ... ok
DEBUG - test_ipv4_dynamic_groups (__main__.TestFirewall.test_ipv4_dynamic_groups) ... ok
DEBUG - test_ipv4_global_state (__main__.TestFirewall.test_ipv4_global_state) ... ok
DEBUG - test_ipv4_mask (__main__.TestFirewall.test_ipv4_mask) ... ok
DEBUG - test_ipv4_remote_group (__main__.TestFirewall.test_ipv4_remote_group) ... ok
DEBUG - test_ipv4_state_and_status_rules (__main__.TestFirewall.test_ipv4_state_and_status_rules) ... ok
DEBUG - test_ipv4_synproxy (__main__.TestFirewall.test_ipv4_synproxy) ... ok
DEBUG - test_ipv6_advanced (__main__.TestFirewall.test_ipv6_advanced) ... ok
DEBUG - test_ipv6_basic_rules (__main__.TestFirewall.test_ipv6_basic_rules) ... ok
DEBUG - test_ipv6_dynamic_groups (__main__.TestFirewall.test_ipv6_dynamic_groups) ... ok
DEBUG - test_ipv6_mask (__main__.TestFirewall.test_ipv6_mask) ... ok
DEBUG - test_ipv6_remote_group (__main__.TestFirewall.test_ipv6_remote_group) ... ok
DEBUG - test_nested_groups (__main__.TestFirewall.test_nested_groups) ... ok
DEBUG - test_remote_group (__main__.TestFirewall.test_remote_group) ... ok
DEBUG - test_source_validation (__main__.TestFirewall.test_source_validation) ... ok
DEBUG - test_sysfs (__main__.TestFirewall.test_sysfs) ... ok
DEBUG - test_timeout_sysctl (__main__.TestFirewall.test_timeout_sysctl) ... ok
DEBUG - test_wildcard_interfaces (__main__.TestFirewall.test_wildcard_interfaces) ... ok
DEBUG - test_zone_basic (__main__.TestFirewall.test_zone_basic) ... ok
DEBUG - test_zone_flow_offload (__main__.TestFirewall.test_zone_flow_offload) ... ok
DEBUG - test_zone_with_default_firewall (__main__.TestFirewall.test_zone_with_default_firewall) ... ok
DEBUG - test_zone_with_vrf (__main__.TestFirewall.test_zone_with_vrf) ... ok
DEBUG - test_zone_without_member (__main__.TestFirewall.test_zone_without_member) ... ok
DEBUG - 
DEBUG - ----------------------------------------------------------------------
DEBUG - Ran 32 tests in 136.576s
DEBUG - 
DEBUG - OK

Checklist:

I have read the CONTRIBUTING document
I have linked this PR to one or more Phabricator Task(s)
I have run the components SMOKETESTS if applicable
My commit headlines contain a valid Task id
My change requires a change to the documentation
I have updated the documentation accordingly

* Add warning when defining `offload-target` without setting action to `offload`

github-actions · 2026-03-31T21:18:59Z

👍
No issues in PR Title / Commit Title

github-actions · 2026-04-01T09:06:06Z

CI integration ❌ failed!

Details

CI logs

CLI Smoketests 👍 passed
CLI Smoketests (interfaces only) ❌ failed
Config tests 👍 passed
RAID1 tests 👍 passed
CLI Smoketests VPP 👍 passed
Config tests VPP 👍 passed
TPM tests 👍 passed

Copilot

Pull request overview

This PR updates the firewall configuration validation to prevent nftables flow-offload rules from being used in contexts where zone-policy traffic involves the local-zone (which would result in invalid/unsafe chain usage), and adds smoketest coverage for the new validation behavior.

Changes:

Add validation to block use of named chains containing action offload when applied to/from a local-zone zone-policy.
Add a warning when offload-target is configured but the rule action is not offload.
Extend firewall smoketests to cover the local-zone + offload invalid configuration.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`src/conf_mode/firewall.py`	Adds warning for mismatched `offload-target`, tracks offload-containing named chains, and validates zone/local-zone references against them.
`smoketest/scripts/cli/test_firewall.py`	Adds a smoketest asserting commit failure when an offload rule is used with local-zone zone-policy.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T13:35:29Z

src/conf_mode/firewall.py

+                if (v4_name and v4_name in offload_chains_v4) or \
+                    (v6_name and v6_name in offload_chains_v6):


The default-firewall validation rejects any zone whose referenced chain contains an offload rule, even when no local-zone is configured. Per data/templates/firewall/nftables-zone.j2 (VZONE_ chain is only used from the forward hook when there is no local-zone), an offload rule in a NAME chain should still be valid for pure forwarding. Suggest conditioning this error on the presence of a local-zone (or only when the default-firewall can be applied to/from local-zone) to avoid unnecessarily breaking existing zone default-firewall + offload setups.

Suggested change

if (v4_name and v4_name in offload_chains_v4) or \

(v6_name and v6_name in offload_chains_v6):

if local_zone and ((v4_name and v4_name in offload_chains_v4) or

(v6_name and v6_name in offload_chains_v6)):

Copilot · 2026-04-02T13:35:29Z

src/conf_mode/firewall.py

        if not dict_search_args(firewall, 'flowtable', offload_target):
            raise ConfigError(f'Invalid offload-target. Flowtable "{offload_target}" does not exist on the system')
+    elif 'offload_target' in rule_conf:
+        Warning('offload-target is specified but action is not set to "offload"')


This warning is emitted without any context about which rule triggered it (family/hook/chain name, priority, rule-id). In real configs with many rules, that makes it hard to find and fix the offending stanza. Consider including the rule location (e.g., firewall family/chain + rule-id) in the warning text.

Suggested change

Warning('offload-target is specified but action is not set to "offload"')

Warning(

f'Firewall rule {family} {hook} (priority {priority}, rule {rule_id}): '

'offload-target is specified but action is not set to "offload"'

)

Should we probably add this context aware warning? It might make sense indeed.

Copilot · 2026-04-02T13:35:30Z

src/conf_mode/firewall.py

+                    if 'local_zone' in zone_conf or 'local_zone' in firewall['zone'][from_zone]:
+                        if (v4_name and v4_name in offload_chains_v4) or \
+                            (v6_name and v6_name in offload_chains_v6):
+                            raise ConfigError('Cannot use a firewall chain with offloading on local zone')


The ConfigError here doesn’t identify which zone pair and/or which firewall chain name contains the offload rule. Including the offending zone(s) and chain name in the message would make troubleshooting much easier (especially when multiple zones reference different chains).

Suggested change

raise ConfigError('Cannot use a firewall chain with offloading on local zone')

offending_chains = []

if v4_name and v4_name in offload_chains_v4:

offending_chains.append(f'ipv4 "{v4_name}"')

if v6_name and v6_name in offload_chains_v6:

offending_chains.append(f'ipv6 "{v6_name}"')

chains_str = ', '.join(offending_chains) if offending_chains else 'with offloading enabled'

raise ConfigError(

f'Cannot use firewall chain(s) {chains_str} between zones "{from_zone}" and "{zone}" when either zone is configured as a local zone'

)

Proposed improved error message reads reasonable

Copilot · 2026-04-02T13:35:30Z

src/conf_mode/firewall.py

+                if (v4_name and v4_name in offload_chains_v4) or \
+                    (v6_name and v6_name in offload_chains_v6):


There’s smoketest coverage for the local-zone from direction, but no test exercising the new default-firewall + offload validation (and the intended scoping to local-zone). Adding a smoketest that (a) fails when a local-zone exists and another zone’s default-firewall points to an offload chain, and (b) succeeds when no local-zone is configured, would prevent regressions.

Suggested change

if (v4_name and v4_name in offload_chains_v4) or \

(v6_name and v6_name in offload_chains_v6):

if local_zone and ((v4_name and v4_name in offload_chains_v4) or \

(v6_name and v6_name in offload_chains_v6)):

AnDroed09 · 2026-04-03T10:55:51Z

Tested integration build.

Test A — offload chain from LOCAL to LAN

configure
set firewall flowtable FT interface eth2
set firewall flowtable FT offload software
set firewall ipv4 name OFFTEST rule 1 action offload
set firewall ipv4 name OFFTEST rule 1 offload-target FT
set firewall ipv4 name OFFTEST rule 1 state established
set firewall ipv4 name OFFTEST rule 1 state related
set firewall zone LAN member interface eth2
set firewall zone LAN from LOCAL firewall name OFFTEST
set firewall zone LOCAL local-zone
commit
[ firewall ]
Cannot use a firewall chain with offloading on local zone

Test B — reverse direction: LOCAL from LAN

configure
set firewall flowtable FT interface eth2
set firewall flowtable FT offload software
set firewall ipv4 name OFFTEST rule 1 action offload
set firewall ipv4 name OFFTEST rule 1 offload-target FT
set firewall ipv4 name OFFTEST rule 1 state established
set firewall ipv4 name OFFTEST rule 1 state related
set firewall zone LAN member interface eth2
set firewall zone LOCAL local-zone
set firewall zone LOCAL from LAN firewall name OFFTEST
commit
[ firewall ]
Cannot use a firewall chain with offloading on local zone

Test С — offload-target without action offload (warning)

set firewall ipv4 forward filter rule 10 action accept
set firewall ipv4 forward filter rule 10 offload-target FT
commit

WARNING: offload-target is specified but action is not set to "offload"

sever-sever

Prevent the use of the local ZBF zone for the offload action

firewall: T8446: Prevent chain with offload rule on local zone

5d51856

* Add warning when defining `offload-target` without setting action to `offload`

sarthurdev requested review from c-po, dmbaturin, jestabro and sever-sever March 31, 2026 21:18

sarthurdev added the bp/circinus Create automatic backport for circinus label Mar 31, 2026

github-actions bot added the current label Mar 31, 2026

github-actions bot assigned sarthurdev Mar 31, 2026

sever-sever requested a review from Copilot April 2, 2026 13:29

Copilot started reviewing on behalf of sever-sever April 2, 2026 13:29 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

sever-sever approved these changes Apr 3, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

firewall: T8446: Prevent chain with offload rule on local zone#5100

firewall: T8446: Prevent chain with offload rule on local zone#5100
sarthurdev wants to merge 1 commit intovyos:currentfrom
sarthurdev:T8446

sarthurdev commented Mar 31, 2026

Uh oh!

github-actions bot commented Mar 31, 2026

Uh oh!

github-actions bot commented Apr 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

c-po Apr 6, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

c-po Apr 6, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

AnDroed09 commented Apr 3, 2026

Uh oh!

sever-sever left a comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

		if (v4_name and v4_name in offload_chains_v4) or \
		(v6_name and v6_name in offload_chains_v6):

-                            raise ConfigError('Cannot use a firewall chain with offloading on local zone')
+                            offending_chains = []
+                            if v4_name and v4_name in offload_chains_v4:
+                                offending_chains.append(f'ipv4 "{v4_name}"')
+                            if v6_name and v6_name in offload_chains_v6:
+                                offending_chains.append(f'ipv6 "{v6_name}"')
+                            chains_str = ', '.join(offending_chains) if offending_chains else 'with offloading enabled'
+                            raise ConfigError(
+                                f'Cannot use firewall chain(s) {chains_str} between zones "{from_zone}" and "{zone}" when either zone is configured as a local zone'
+                            )

Conversation

sarthurdev commented Mar 31, 2026

Change summary

Types of changes

Related Task(s)

Related PR(s)

How to test / Smoketest result

Checklist:

Uh oh!

github-actions bot commented Mar 31, 2026

Uh oh!

github-actions bot commented Apr 1, 2026

Details

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

c-po Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

c-po Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

AnDroed09 commented Apr 3, 2026

Uh oh!

sever-sever left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants