fix!: the SNMP agent takes the caller's values and opens only to its sources (0.2.0)

fix_snmp enabled os-net-snmp with community "public", contact
netork@localhost and location "Managed by netOrk", and added a
floating pass rule for UDP/161 from any source on every interface: a
firewall managed over WAN answered its SNMP tree to anyone there (#13).

The driver implements napalm-device-types 4.13's configure_snmp_agent
steps instead:
- install: os-net-snmp only when missing (a reinstall resets it);
- configure: community, contact, location and listen address from the
  caller; a DNS name is not a listen address; a refused save fails;
- firewall: one floating pass rule per allowed source, the old
  any-source rule and rules of an earlier run replaced, operators'
  rules untouched; no sources, no change;
- start: restart net-snmp.
The hand-built UDP probe goes; the caller asks the agent itself.

BREAKING CHANGE: run_device_action is removed. Requires
napalm-device-types 4.13.

Closes #13

napalm_opnsense/opnsense.py

@@ -36,6 +36,7 @@ per-module through their respective API controllers.
 from __future__ import annotations
 
 import difflib
+import ipaddress
 import json
 import logging
 import re
@@ -53,8 +54,10 @@ _REFRESH_INTERVAL = 3
 import requests
 from requests.exceptions import RequestException
 
-from napalm_device_types import ConfigSyncMixin, FingerprintRule, FirewallDriver
+from napalm_device_types import ConfigSyncMixin, FingerprintRule, FirewallDriver, SnmpAgentMixin
 from napalm_device_types.models import (
+    FixStepOutcomeDict,
+    SnmpAgentSettingsDict,
     ConfigSyncDict,
     DhcpHaDict,
     ListeningSocketsDict,
@@ -86,7 +89,7 @@ def _carp_master(entry: dict[str, Any]) -> bool:
     return str(entry.get("status") or "").strip().upper() == "MASTER"
 
 
-class OPNsenseDriver(OPNsensePingMixin, ConfigSyncMixin, FirewallDriver):
+class OPNsenseDriver(OPNsensePingMixin, ConfigSyncMixin, SnmpAgentMixin, FirewallDriver):
     """NAPALM driver for OPNsense (read-only, REST API)."""
 
     VENDOR = "OPNsense"
@@ -2391,157 +2394,87 @@ class OPNsenseDriver(OPNsensePingMixin, ConfigSyncMixin, FirewallDriver):
             }
         return {"success": True, "output": f"Magic packet sent to {mac_address} via {interface}"}
 
-    def run_device_action(self, action: str) -> dict[str, Any]:
-        """Execute a named action on the firewall."""
-        if action == "fix_snmp":
-            return self._action_fix_snmp()
-        raise NotImplementedError(f"Unknown action: {action!r}")
+    # ------------------------------------------------------------------
+    # SNMP agent (napalm-device-types SnmpAgentMixin): the caller's values
+    # ------------------------------------------------------------------
 
-    def _action_fix_snmp(self) -> dict[str, Any]:
-        """Install os-net-snmp plugin, configure community 'public', open firewall.
+    #: What marks a pass rule this driver added for an allowed SNMP source; the
+    #: old any-source rule is recognised by its older description.
+    _SNMP_RULE_PREFIX = "napalm-opnsense: SNMP from "
+    _SNMP_RULE_OLD = "[netork] Allow SNMP"
 
-        Steps:
-        1. Install os-net-snmp via firmware API (idempotent)
-        2. Configure via POST /api/netsnmp/general/set
-        3. Start/restart the service
-        4. Add a floating firewall rule allowing UDP/161 from any source
-           (OPNsense default-drops traffic arriving on non-LAN interfaces
-           such as WireGuard; without this rule SNMP is unreachable from
-           management networks even though the daemon is running)
-        5. Apply firewall and verify
-        """
-        lines: list = []
-
-        # 1. Install os-net-snmp plugin ONLY if not already present.
-        # Calling firmware/install on an already-installed plugin triggers an
-        # async reinstall that overwrites the config with factory defaults a few
-        # minutes later — causing SNMP to stop working again after the fix.
-        plugin_installed = False
+    def _install_snmp_agent(self) -> FixStepOutcomeDict:
+        # Installing over an installed os-net-snmp starts an async reinstall that
+        # resets its configuration minutes later, so only when it is missing.
         try:
             chk = self._get("/api/netsnmp/general/get")
-            plugin_installed = isinstance(chk, dict) and "general" in chk
-        except Exception:
-            pass
+        except Exception:  # noqa: BLE001 - the plugin's API is not there
+            chk = {}
+        if isinstance(chk, dict) and "general" in chk:
+            return {"ok": True, "output": "os-net-snmp already installed"}
+        result = self._post("/api/core/firmware/install/os-net-snmp")
+        time.sleep(5)  # the install settles before its API answers
+        return {"ok": result.get("status") == "ok", "output": f"os-net-snmp install: {result}"}
 
-        if not plugin_installed:
-            try:
-                result = self._post("/api/core/firmware/install/os-net-snmp")
-                lines.append(f"[install] {result}")
-                import time as _time
-                _time.sleep(5)  # wait for install to settle
-            except Exception as exc:
-                logger.debug("os-net-snmp install failed: %s", exc)
-                lines.append(f"[install] failed: {exc}")
-        else:
-            lines.append("[install] os-net-snmp already installed — skipping reinstall")
-
-        # 2. Configure SNMP
-        # The listen field must contain the management IP so that snmpd binds
-        # to the correct interface. Without it snmpd may not respond on any
-        # non-loopback address.
-        # The API expects {"<ip>": {"selected": 1}} — we use self.hostname
-        # which is always the device's management IP in netOrk.
-        listen_val: dict = {self.hostname: {"selected": 1}}
+    def _write_snmp_agent_config(self, settings: SnmpAgentSettingsDict) -> FixStepOutcomeDict:
+        community = settings["community"]
+        if not community or any(c.isspace() or c in "\"#" for c in community):
+            raise ValueError("the community must be one word without quotes or #")
+        listen = settings.get("listen") or self.hostname
         try:
-            self._post("/api/netsnmp/general/set", {
-                "general": {
+            listen_val = {str(ipaddress.ip_address(listen)): {"selected": 1}}
+        except ValueError:
+            listen_val = {}  # a DNS name binds nothing; snmpd then listens everywhere
+        result = self._post("/api/netsnmp/general/set", {
+            "general": {
+                "enabled": "1",
+                "community": community,
+                "contact": settings.get("contact", ""),
+                "location": settings.get("location", ""),
+                "sysobjid": "",
+                "bindip": "",
+                "listen": listen_val,
+            }
+        })
+        if result.get("result") != "saved":
+            return {"ok": False, "output": f"configuration refused: {result}"}
+        return {"ok": True, "output": f"community set, listen {list(listen_val) or 'all addresses'}"}
+
+    def _allow_snmp_sources(self, settings: SnmpAgentSettingsDict) -> FixStepOutcomeDict:
+        sources = [str(ipaddress.ip_network(s.strip(), strict=False)) for s in settings.get("allowed_sources", [])]
+        if not sources:
+            return {"ok": True, "output": "no sources given; firewall unchanged"}
+        found = self._post("/api/firewall/filter/searchRule", {"current": 1, "rowCount": -1, "searchPhrase": "SNMP"})
+        for row in found.get("rows") or []:
+            descr = str(row.get("description") or row.get("descr") or "")
+            if descr == self._SNMP_RULE_OLD or descr.startswith(self._SNMP_RULE_PREFIX):
+                self._post(f"/api/firewall/filter/delRule/{row['uuid']}")
+        for source in sources:
+            self._post("/api/firewall/filter/addRule", {
+                "rule": {
                     "enabled": "1",
-                    "community": "public",
-                    "contact": "netork@localhost",
-                    "location": "Managed by netOrk",
-                    "sysobjid": "",
-                    "bindip": "",
-                    "listen": listen_val,
+                    "action": "pass",
+                    "quick": "1",
+                    "interface": "",  # floating: wherever the source comes in
+                    "direction": "in",
+                    "ipprotocol": "inet6" if ":" in source else "inet",
+                    "protocol": "udp",
+                    "source_net": source,
+                    "source_not": "0",
+                    "destination_net": "(self)",
+                    "destination_not": "0",
+                    "destination_port": "161",
+                    "log": "0",
+                    "floating": "yes",
+                    "descr": f"{self._SNMP_RULE_PREFIX}{source}",
                 }
             })
-            lines.append(f"[config] SNMP enabled with community 'public', listen={self.hostname}.")
-        except Exception as exc:
-            logger.debug("SNMP config failed: %s", exc)
-            lines.append(f"[config] error: {exc}")
+        self._post("/api/firewall/filter/apply", {})
+        return {"ok": True, "output": f"udp/161 open for {', '.join(sources)} only"}
 
-        # 3. Start / restart the SNMP service
-        try:
-            self._post("/api/netsnmp/service/restart")
-            lines.append("[service] net-snmp restarted.")
-        except Exception as exc:
-            logger.debug("SNMP service restart failed, trying start: %s", exc)
-            try:
-                self._post("/api/netsnmp/service/start")
-                lines.append("[service] net-snmp started.")
-            except Exception as exc2:
-                lines.append(f"[service] start failed: {exc2}")
-
-        # 4. Add floating firewall rule for SNMP (UDP/161 → self)
-        # OPNsense blocks traffic arriving on non-LAN interfaces by default.
-        # A floating pass rule makes SNMP reachable from all management nets.
-        try:
-            existing = self._post("/api/firewall/filter/searchRule",
-                                  {"current": 1, "rowCount": -1, "searchPhrase": "[netork] Allow SNMP"})
-            if not (existing.get("rows") or []):
-                self._post("/api/firewall/filter/addRule", {
-                    "rule": {
-                        "enabled": "1",
-                        "sequence": "1",
-                        "action": "pass",
-                        "quick": "1",
-                        "interface": "",        # floating — all interfaces
-                        "direction": "in",
-                        "ipprotocol": "inet",
-                        "protocol": "udp",
-                        "source_net": "any",
-                        "source_not": "0",
-                        "destination_net": "(self)",
-                        "destination_not": "0",
-                        "destination_port": "161",
-                        "log": "0",
-                        "floating": "yes",
-                        "descr": "[netork] Allow SNMP",
-                    }
-                })
-                lines.append("[firewall] Floating pass rule added for UDP/161.")
-            else:
-                lines.append("[firewall] Pass rule already present.")
-            self._post("/api/firewall/filter/apply", {})
-            lines.append("[firewall] Rules applied.")
-        except Exception as exc:
-            logger.debug("Firewall rule for SNMP failed: %s", exc)
-            lines.append(f"[firewall] error: {exc}")
-
-        # 5. Verify — try actual UDP/161 probe first, fall back to API config check
-        success = False
-        try:
-            import socket as _socket
-            sock = _socket.socket(_socket.AF_INET, _socket.SOCK_DGRAM)
-            sock.settimeout(3)
-            community = b"public"
-            # Minimal SNMPv2c GetRequest for sysDescr
-            pdu = (b"\x30\x26\x02\x01\x01\x04"
-                   + bytes([len(community)]) + community
-                   + b"\xa0\x19\x02\x04\x00\x00\x00\x01"
-                   + b"\x02\x01\x00\x02\x01\x00"
-                   + b"\x30\x0b\x30\x09\x06\x05\x2b\x06\x01\x02\x01\x05\x00")
-            sock.sendto(pdu, (self.hostname, 161))
-            try:
-                data, _ = sock.recvfrom(1024)
-                success = len(data) > 0
-                lines.append("[ok] SNMP UDP probe successful.")
-            except _socket.timeout:
-                lines.append("[warn] SNMP UDP probe timed out — service may be starting.")
-            finally:
-                sock.close()
-        except Exception as exc:
-            logger.debug("SNMP UDP probe failed: %s", exc)
-            # Fall back to API config check
-            try:
-                cfg = self._get("/api/netsnmp/general/get")
-                general = cfg.get("general", cfg)
-                success = str(general.get("enabled", "0")) == "1" and bool(general.get("community"))
-                lines.append("[ok] SNMP config active (UDP probe unavailable)." if success
-                             else "[warn] SNMP config check failed.")
-            except Exception:
-                lines.append("[warn] Could not verify SNMP state.")
-
-        return {"success": success, "output": "\n".join(lines)}
+    def _start_snmp_agent(self) -> FixStepOutcomeDict:
+        result = self._post("/api/netsnmp/service/restart")
+        return {"ok": result.get("status") == "ok", "output": f"net-snmp restart: {result}"}
 
     def get_port_forwards(self) -> list[dict[str, Any]]:
         """Destination NAT on the WAN interfaces, as port forwards.