Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sarthurdev wants to merge 1 commit into
base: current
Choose a base branch
from
+34 −0
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions smoketest/scripts/cli/test_firewall.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,20 @@ def test_zone_basic(self):
self.cli_set(['firewall', 'global-options', 'state-policy', 'related', 'action', 'accept'])
self.cli_set(['firewall', 'global-options', 'state-policy', 'invalid', 'action', 'drop'])

# Test error on offload from local zone
self.cli_set(['firewall', 'flowtable', 'smoketest', 'interface', 'eth0'])
self.cli_set(['firewall', 'flowtable', 'smoketest', 'offload', 'software'])
self.cli_set(['firewall', 'ipv4', 'name', 'smoketest', 'rule', '1', 'action', 'offload'])
self.cli_set(['firewall', 'ipv4', 'name', 'smoketest', 'rule', '1', 'offload-target', 'smoketest'])
self.cli_set(['firewall', 'ipv4', 'name', 'smoketest', 'rule', '1', 'state', 'established'])
self.cli_set(['firewall', 'ipv4', 'name', 'smoketest', 'rule', '1', 'state', 'related'])

with self.assertRaises(ConfigSessionError):
self.cli_commit()

self.cli_delete(['firewall', 'flowtable', 'smoketest'])
self.cli_delete(['firewall', 'ipv4', 'name', 'smoketest', 'rule', '1'])

self.cli_commit()

nftables_search = [
Expand Down
20 changes: 20 additions & 0 deletions src/conf_mode/firewall.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,8 @@ def verify_rule(firewall, family, hook, priority, rule_id, rule_conf):

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"')
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"'
)

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@c-po c-po Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if rule_conf['action'] != 'synproxy' and 'synproxy' in rule_conf:
raise ConfigError('"synproxy" option allowed only for action synproxy')
Expand Down Expand Up @@ -532,6 +534,9 @@ def verify(firewall):
if 'url' not in group:
raise ConfigError(f'remote-group {group_name} must have a url configured')

offload_chains_v4 = set()
offload_chains_v6 = set()

for family in ['ipv4', 'ipv6', 'bridge']:
if family in firewall:
for chain in ['name','forward','input','output', 'prerouting']:
Expand All @@ -551,6 +556,12 @@ def verify(firewall):
for rule_id, rule_conf in priority_conf['rule'].items():
verify_rule(firewall, family, chain, priority, rule_id, rule_conf)

if chain == 'name' and rule_conf['action'] == 'offload':
if family == 'ipv4':
offload_chains_v4.add(priority)
elif family == 'ipv6':
offload_chains_v6.add(priority)

local_zone = False
zone_interfaces = []
zone_vrf = []
Expand Down Expand Up @@ -624,6 +635,11 @@ def verify(firewall):
if v6_name and not dict_search_args(firewall, 'ipv6', 'name', v6_name):
raise ConfigError(f'Firewall ipv6-name "{v6_name}" does not exist')

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')
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'
)

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@c-po c-po Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposed improved error message reads reasonable


if 'default_firewall' in zone_conf:
v4_name = dict_search_args(zone_conf, 'default_firewall', 'name')
if v4_name and not dict_search_args(firewall, 'ipv4', 'name', v4_name):
Expand All @@ -636,6 +652,10 @@ def verify(firewall):
if not v4_name and not v6_name:
raise ConfigError('No firewall names specified for default-firewall')

if (v4_name and v4_name in offload_chains_v4) or \
(v6_name and v6_name in offload_chains_v6):
Comment on lines +655 to +656
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
Comment on lines +655 to +656
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)):

Copilot uses AI. Check for mistakes.
raise ConfigError('Cannot use a chain with offloading for zone default-firewall')

return None

def generate(firewall):
Expand Down
Loading