NAPALM/napalm-openwrt
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(snmp): resolve the real firewall zone instead of guessing "lan"

Browse Source 
fix_snmp reported success on APs where the rule never reached nftables.
Five defects stacked up:

1. Zone detection required ".src=" and "ssh" in the same `uci show` line.
   UCI prints one option per line, so anonymous rules never matched and
   every device fell through to the hardcoded "lan" fallback.
2. That fallback was never checked against the zones that actually exist.
   On an AP whose zone section has no `option name`, fw4 skips the section,
   so `src='lan'` referenced a zone that was not there and the rule was
   dropped with it.
3. The "already present" guard was a substring test, so a rule written by
   an earlier broken run was skipped forever instead of repaired.
4. Stale-rule deletion never committed — the only `uci commit firewall`
   sat in the add branch that the guard had just skipped.
5. `fw4 reload` errors were swallowed by `|| true`, and with no local
   snmpget the action hardcoded success = True.

Now: the management address comes from $SSH_CONNECTION and is mapped to
its network section (via ipaddr, or via `ip -o -4 addr` -> device when the
interface is DHCP-addressed) and from there to the owning zone. A zone
section without a name aborts the action with the repair command rather
than writing a dead rule — naming it is left to the operator, since an
inert zone becoming active changes what the AP filters. Rules are written
in full every run, stale ones are deleted highest anonymous index first
(uci renumbers @rule[n] on delete) and committed, reload output is no
longer truncated or ignored, and success is verified on the device via
`ss -lun` and a udp/161 lookup in the live ruleset.
This commit is contained in:
Christian Manivong
2026-08-18 17:54:22 +07:00
parent c686fac55e
commit 597a59fa39
2 changed files with 614 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/unit/test_driver.py
+339
View File
@@ -1395,3 +1395,342 @@ class TestPushMacAcl:
driver.push_mac_acl("CorpWiFi", "whitelist", ["AA:BB:CC:DD:EE:01"])
assert any("uci commit wireless" in c for c in issued)
assert any(c.strip() == "wifi reload" for c in issued)
# ---------------------------------------------------------------------------
# fix_snmp — firewall zone handling
# ---------------------------------------------------------------------------
UCI_FIREWALL_HEALTHY = """\
firewall.@defaults[0]=defaults
firewall.@defaults[0].input='REJECT'
firewall.@zone[0]=zone
firewall.@zone[0].name='lan'
firewall.@zone[0].network='lan'
firewall.@zone[0].input='ACCEPT'
firewall.@zone[1]=zone
firewall.@zone[1].name='wan'
firewall.@zone[1].network='wan' 'wan6'
firewall.@zone[1].input='REJECT'
firewall.@rule[0]=rule
firewall.@rule[0].name='Allow-DHCP-Renew'
firewall.@rule[0].src='wan'
firewall.@rule[0].dest_port='68'
"""
# The zone that owns the management network lost its 'name' — fw4 skips the
# whole section and every rule pointing at it.
UCI_FIREWALL_NAMELESS_ZONE = """\
firewall.@defaults[0]=defaults
firewall.@defaults[0].input='REJECT'
firewall.@zone[0]=zone
firewall.@zone[0].network='lan'
firewall.@zone[0].input='ACCEPT'
firewall.@rule[0]=rule
firewall.@rule[0].name='Allow-DHCP-Renew'
firewall.@rule[0].src='wan'
"""
# A dedicated management zone — the AP layout the action is meant to handle.
UCI_FIREWALL_MGMT_ZONE = """\
firewall.@zone[0]=zone
firewall.@zone[0].name='lan'
firewall.@zone[0].network='lan'
firewall.@zone[1]=zone
firewall.@zone[1].name='mgmt'
firewall.@zone[1].network='mgmt'
firewall.@zone[1].input='REJECT'
firewall.@rule[0]=rule
firewall.@rule[0].name='Allow-SSH'
firewall.@rule[0].src='mgmt'
firewall.@rule[0].dest_port='22'
"""
UCI_NETWORK_STATIC = """\
network.loopback=interface
network.loopback.device='lo'
network.lan=interface
network.lan.device='br-lan'
network.lan.proto='static'
network.lan.ipaddr='192.168.1.1'
network.mgmt=interface
network.mgmt.device='br-lan.9'
network.mgmt.proto='static'
network.mgmt.ipaddr='10.10.0.5'
"""
UCI_NETWORK_DHCP = """\
network.lan=interface
network.lan.device='br-lan'
network.lan.proto='dhcp'
"""
IP_ADDR_BRLAN = """\
1: lo inet 127.0.0.1/8 scope host lo\\ valid_lft forever preferred_lft forever
7: br-lan inet 10.10.0.5/24 brd 10.10.0.255 scope global br-lan\\ valid_lft forever
"""
class _FakeShell:
"""Collects issued commands and answers them from a canned config."""
def __init__(self, firewall="", network="", ip_addr="", ssh_connection="",
reload_out="", nft_hits="1", listen_hits="1", snmpd="running"):
self.firewall = firewall
self.network = network
self.ip_addr = ip_addr
self.ssh_connection = ssh_connection
self.reload_out = reload_out
self.nft_hits = nft_hits
self.listen_hits = listen_hits
self.snmpd = snmpd
self.issued: list[str] = []
def __call__(self, cmd, **kw):
self.issued.append(cmd)
if cmd.startswith("uci show firewall"):
return self.firewall
if cmd.startswith("uci show network"):
return self.network
if "$SSH_CONNECTION" in cmd:
return self.ssh_connection
if "ip -o -4 addr" in cmd:
return self.ip_addr
if "fw4 reload" in cmd or "firewall reload" in cmd:
return self.reload_out
if "dport 161" in cmd:
return self.nft_hits
if ":161" in cmd:
return self.listen_hits
if "snmpd status" in cmd:
return self.snmpd
return ""
class TestFixSnmpZoneDetection:
"""_action_fix_snmp() must resolve the real zone that owns the mgmt address."""
def test_static_mgmt_address_selects_owning_zone(self, driver):
"""10.10.0.5 lives on network 'mgmt' → zone 'mgmt', not the 'lan' fallback."""
shell = _FakeShell(
firewall=UCI_FIREWALL_MGMT_ZONE,
network=UCI_NETWORK_STATIC,
ssh_connection="10.10.0.1 51234 10.10.0.5 22",
)
driver._send_command = shell
result = driver._action_fix_snmp()
assert "Management zone: 'mgmt'" in result["output"]
assert any("firewall.allow_snmp_from_mgmt.src='mgmt'" in c for c in shell.issued)
def test_anonymous_ssh_rule_does_not_decide_the_zone(self, driver):
"""The old per-line 'src= and ssh' heuristic never matched anonymous rules."""
shell = _FakeShell(
firewall=UCI_FIREWALL_HEALTHY,
network=UCI_NETWORK_STATIC,
ssh_connection="192.168.1.50 5000 192.168.1.1 22",
)
driver._send_command = shell
result = driver._action_fix_snmp()
assert "Management zone: 'lan'" in result["output"]
def test_dhcp_mgmt_address_resolved_via_l3_device(self, driver):
"""No ipaddr in UCI → resolve address → device → network section → zone."""
shell = _FakeShell(
firewall=UCI_FIREWALL_HEALTHY,
network=UCI_NETWORK_DHCP,
ip_addr=IP_ADDR_BRLAN,
ssh_connection="10.10.0.1 51234 10.10.0.5 22",
)
driver._send_command = shell
result = driver._action_fix_snmp()
assert "Management zone: 'lan'" in result["output"]
def test_nameless_zone_is_reported_and_action_fails(self, driver):
"""A zone without 'name' is skipped by fw4 — say so instead of writing a dead rule."""
shell = _FakeShell(
firewall=UCI_FIREWALL_NAMELESS_ZONE,
network=UCI_NETWORK_STATIC,
ssh_connection="192.168.1.50 5000 192.168.1.1 22",
)
driver._send_command = shell
result = driver._action_fix_snmp()
assert result["success"] is False
assert "@zone[0]" in result["output"]
assert "name" in result["output"]
def test_nameless_zone_does_not_get_a_rule_written(self, driver):
"""No SNMP rule may be committed while the owning zone is invalid."""
shell = _FakeShell(
firewall=UCI_FIREWALL_NAMELESS_ZONE,
network=UCI_NETWORK_STATIC,
ssh_connection="192.168.1.50 5000 192.168.1.1 22",
)
driver._send_command = shell
driver._action_fix_snmp()
assert not any("allow_snmp" in c for c in shell.issued)
class TestFixSnmpRuleWriting:
"""The rule must be (re)written idempotently, not skipped when present."""
def test_existing_rule_is_repaired_not_skipped(self, driver):
"""A rule that exists but lacks src must be rewritten, not left broken."""
broken = UCI_FIREWALL_HEALTHY + (
"firewall.allow_snmp_from_lan=rule\n"
"firewall.allow_snmp_from_lan.name='Allow-SNMP-from-lan'\n"
"firewall.allow_snmp_from_lan.dest_port='161'\n"
)
shell = _FakeShell(
firewall=broken,
network=UCI_NETWORK_STATIC,
ssh_connection="192.168.1.50 5000 192.168.1.1 22",
)
driver._send_command = shell
driver._action_fix_snmp()
assert any("firewall.allow_snmp_from_lan.src='lan'" in c for c in shell.issued)
def test_stale_rules_are_deleted_highest_index_first(self, driver):
"""Anonymous sections shift on delete — descending order keeps the keys valid."""
stale = UCI_FIREWALL_HEALTHY + (
"firewall.@rule[1]=rule\n"
"firewall.@rule[1].name='Allow-SNMP'\n"
"firewall.@rule[1].dest_port='161'\n"
"firewall.@rule[2]=rule\n"
"firewall.@rule[2].name='Allow-SNMP-netOrk'\n"
"firewall.@rule[2].dest_port='161'\n"
)
shell = _FakeShell(
firewall=stale,
network=UCI_NETWORK_STATIC,
ssh_connection="192.168.1.50 5000 192.168.1.1 22",
)
driver._send_command = shell
driver._action_fix_snmp()
deletes = [c for c in shell.issued if "delete" in c and "@rule" in c]
joined = " ".join(deletes)
assert joined.index("@rule[2]") < joined.index("@rule[1]")
def test_deletion_is_committed(self, driver):
"""Old code staged deletes in /tmp/.uci and never committed them."""
stale = UCI_FIREWALL_HEALTHY + (
"firewall.snmp_netork=rule\n"
"firewall.snmp_netork.name='Allow-SNMP-from-mgmt'\n"
"firewall.snmp_netork.dest_port='161'\n"
)
shell = _FakeShell(
firewall=stale,
network=UCI_NETWORK_STATIC,
ssh_connection="192.168.1.50 5000 192.168.1.1 22",
)
driver._send_command = shell
driver._action_fix_snmp()
delete_idx = next(i for i, c in enumerate(shell.issued) if "snmp_netork" in c and "delete" in c)
assert any("uci commit firewall" in c for c in shell.issued[delete_idx:])
def test_legacy_snmp_netork_rule_is_recognised_as_stale(self, driver):
"""The pre-0.x rule was named 'Allow-SNMP-from-mgmt' — hyphens, not underscores."""
stale = UCI_FIREWALL_HEALTHY + (
"firewall.snmp_netork=rule\n"
"firewall.snmp_netork.name='Allow-SNMP-from-mgmt'\n"
"firewall.snmp_netork.src='*'\n"
"firewall.snmp_netork.dest_port='161'\n"
)
shell = _FakeShell(
firewall=stale,
network=UCI_NETWORK_STATIC,
ssh_connection="192.168.1.50 5000 192.168.1.1 22",
)
driver._send_command = shell
driver._action_fix_snmp()
assert any("snmp_netork" in c and "delete" in c for c in shell.issued)
class TestFixSnmpReloadVerification:
"""A failing fw4 reload must fail the action, not be swallowed."""
FW4_ZONE_ERROR = (
"Section @zone[0] option 'name' is mandatory but not set\n"
"Section @zone[0] skipped due to invalid options\n"
"Section @rule[0] references unknown zone 'lan'\n"
"Section @rule[0] skipped due to invalid options"
)
def test_reload_error_fails_the_action(self, driver):
shell = _FakeShell(
firewall=UCI_FIREWALL_HEALTHY,
network=UCI_NETWORK_STATIC,
ssh_connection="192.168.1.50 5000 192.168.1.1 22",
reload_out=self.FW4_ZONE_ERROR,
)
driver._send_command = shell
result = driver._action_fix_snmp()
assert result["success"] is False
def test_reload_output_is_not_truncated(self, driver):
"""The old 120-char cap hid the 'references unknown zone' line."""
shell = _FakeShell(
firewall=UCI_FIREWALL_HEALTHY,
network=UCI_NETWORK_STATIC,
ssh_connection="192.168.1.50 5000 192.168.1.1 22",
reload_out=self.FW4_ZONE_ERROR,
)
driver._send_command = shell
result = driver._action_fix_snmp()
assert "references unknown zone" in result["output"]
def test_missing_live_rule_fails_the_action(self, driver):
"""snmpd up + clean reload, but no udp/161 accept in the packet filter."""
shell = _FakeShell(
firewall=UCI_FIREWALL_HEALTHY,
network=UCI_NETWORK_STATIC,
ssh_connection="192.168.1.50 5000 192.168.1.1 22",
nft_hits="0",
)
driver._send_command = shell
result = driver._action_fix_snmp()
assert result["success"] is False
def test_snmpd_not_listening_fails_the_action(self, driver):
shell = _FakeShell(
firewall=UCI_FIREWALL_HEALTHY,
network=UCI_NETWORK_STATIC,
ssh_connection="192.168.1.50 5000 192.168.1.1 22",
listen_hits="0",
)
driver._send_command = shell
result = driver._action_fix_snmp()
assert result["success"] is False
def test_fully_healthy_run_succeeds(self, driver):
shell = _FakeShell(
firewall=UCI_FIREWALL_HEALTHY,
network=UCI_NETWORK_STATIC,
ssh_connection="192.168.1.50 5000 192.168.1.1 22",
)
driver._send_command = shell
result = driver._action_fix_snmp()
assert result["success"] is True
class TestUciSectionParser:
"""_parse_uci_sections() underpins all of the above."""
def test_section_type_captured(self, driver):
parsed = driver._parse_uci_sections(UCI_FIREWALL_HEALTHY)
assert parsed["@zone[0]"]["_type"] == "zone"
def test_option_value_unquoted_on_read(self, driver):
parsed = driver._parse_uci_sections(UCI_FIREWALL_HEALTHY)
assert driver._uci_scalar(parsed["@zone[0]"]["name"]) == "lan"
def test_list_values_split_into_tokens(self, driver):
parsed = driver._parse_uci_sections(UCI_FIREWALL_HEALTHY)
assert driver._uci_tokens(parsed["@zone[1]"]["network"]) == ["wan", "wan6"]
def test_value_containing_equals_is_kept_whole(self, driver):
parsed = driver._parse_uci_sections("firewall.x=rule\nfirewall.x.name='a=b'\n")
assert driver._uci_scalar(parsed["x"]["name"]) == "a=b"
def test_blank_and_malformed_lines_ignored(self, driver):
parsed = driver._parse_uci_sections("\n\nnot a uci line\nfirewall.x=rule\n")
assert list(parsed) == ["x"]