vacuum-wall/HARDEN.md

Hardening Plan — Firewall Zone Handling (post DHCP-incident)

Status: ready to implement (line refs re-verified against code 2026-08-28; review corrections applied 2026-08-28; full cross-file re-verification with minor clarifications applied 2026-08-28). Background: LAN clients stopped receiving DHCP leases because the internal zone lost its eth1 assignment (in both live firewalld and config/firewall/config.json — incident-time state; the appliance was recovered before this plan was written, so the repo's config.json now shows internal: eth1 again, per Sequencing item 4). With no zone covering eth1, all inbound traffic hit the default reject policy — DHCP (and everything else) from the LAN was dropped before reaching a healthy dnsmasq. Nothing flagged the uncovered-interface condition; every apply silently reinforced it. Follow-ups below close that blind spot and the related sharp edges. /etc/sudoers.d/wall stays as-is (dev privs, user-acknowledged).

Decisions (confirmed):

• Target drift: Option A — absent target = unmanaged (not diffed, not touched by apply). The absent key is the single canonical form for "unmanaged". A zone whose target normalizes to default (e.g. the legacy explicit "DEFAULT") is treated as unmanaged in the pending diff as well — it mirrors _config_apply, which only ever sets ACCEPT/DROP/REJECT. No backward compatibility is needed.
• Coverage guard: ConflictError + force in _config_apply; UI confirm in the interfaces picker when an interface's last zone is dropped (zone deletion is out of scope — delete_zone stays unguarded).
• Guard scope: explicit lo/wg* filter — guarded ifaces = network-config keys minus lo and wg* prefixes (regardless of config contents); the coverage union also counts live interfaces of zones absent from config (apply never touches them).
• Config cleanup is applied via POST /api/firewall/config (full replace) + apply, not a raw JSON edit (re-stamps _last_applied_hash/_last_applied_config so cancel-all baselines stay consistent). PATCH cannot be used: deep_merge (lib/common.py:244-256) has no key-removal path, so the current patch_config endpoint cannot delete the target keys (a null value would be written instead).
• Execution: parallel streams — WI-1 (A/B/C) + WI-3 are file-disjoint and run concurrently; WI-2 → WI-4 → WI-5 stay serial (shared files: _config_apply, _compute_pending_changes, tests/test_firewall.py, docs/config.md).
• Pin "target": "ACCEPT" for internal in config (declared intent = trusted LAN).

Extra latent bugs found during research (folded in):

• create_zone (daemon/handlers/firewall.py:655-663) never creates the zone at all: the --new-zone step is missing and it runs firewall-cmd --set-target=<target> on a nonexistent zone. For the default target it would run --set-target=default, which the codebase's defensive guards treat as un-settable (the new-zone branch of _config_apply, lines 188-205, already guards this; create_zone does not). The current firewalld man page (verified via live docs 2026-08-28) lists default as an accepted --set-target value for zones; the planned defensive skip of default targets is still correct under Option A. An appliance-side check of actual behavior is optional during live verification, not required.
• _compute_pending_changes (lib/firewall.py:380-381) skips entire zones with no interfaces key — no field drift is ever reported for such zones.
• The explicit "target": "DEFAULT" entries in config/firewall/config.json (public, vpn-full, work) are a one-shot install-time import artifact: system_import.import_firewall (lib/system_import.py:921-932) emits a target key for every imported zone with interfaces, and _live_target_to_config maps live default to the config token "DEFAULT" (lib/firewall.py:353-361). No other code path writes target into config. Hand/UI-edited zones (e.g. internal) omit the key — two notations for the same meaning. Fixed by WI-2.

---

WI-1 — Interface-coverage invariant + apply guard (core fix)

Goal: an interface managed by the network subsystem that ends up in no zone becomes a loud, always-visible condition, and bulk-apply cannot silently produce it.

1. daemon/handlers/firewall.py, _config_apply (lines 138-403):
   • Interfaces step (lines 252-279) — new semantics:
     • key absent → hands off: skip the remove-then-add for that zone (matches the existing "None = don't change" masquerade semantic at line 281).
     • explicit [] → intentional unassign-all (the UI picker legitimately sends this).
     • Pre-mutation coverage guard: compute post-apply coverage = union of each zone's desired interfaces (explicit list if key present, else its current live set for absent-key zones) plus the live interfaces of zones absent from config (apply never touches those; without this the guard false-positives when a live-only zone still holds an interface). Guarded interfaces = keys of lib.network.get_config()["interfaces"] with lo and wg* prefixes explicitly filtered (vpn zones are managed by WgToFirewallSync; lo is normally zoneless — without the filter a networkd-managed lo would make every apply raise). If any guarded interface would be uncovered → ConflictError naming the interfaces and the consequence (clients lose connectivity/DHCP), overridable with body.force = true (same pattern as the https/ssh lockout at lines 154-169; the config_apply handler at 608-631 already receives body). Implementation note: the guard needs current live interfaces for all zones before any mutation — take them from one firewall-cmd --get-active-zones call before the zone loop (same pattern as set_zone_interfaces, daemon/handlers/firewall.py:731-732); do not rely on the per-zone --list-all reads that currently happen inside the mutation loop. Place the guard alongside the https/ssh lockout check (lines 154-169), i.e. before the pre-apply backup write — the existing lockout test (test_config_apply_blocks_lockout_before_backup, tests/test_firewall.py:670-677) pins mock_backup.assert_not_called(), and the new guard must respect the same no-side-effect-on-conflict invariant. Update the _config_apply and config_apply docstrings (both currently document only the lockout guard + force).
2. set_zone_interfaces (lines 706-791): when the new selection leaves an interface in no zone, emit a prominent logger.warning. No block — deliberate UI action.
3. lib/firewall.py, _compute_pending_changes (lines 364-476): remove the blanket if not zone_cfg.get("interfaces"): continue (line 380-381) — only gate the interfaces diff on key presence; report services/target/masquerade/rules/fwd-ports drift for such zones as today. Implementation detail: the interfaces diff itself must be gated — cfg_ifaces = set(zone_cfg.get("interfaces", [])) (line 383) would otherwise diff set() against live for absent-key zones and emit a spurious entry; compute it only when "interfaces" in zone_cfg. Behavior change: zones with interfaces: [] (live: vpn, vpn-full, work) will now report field drift on every poll, and config zones absent from live entirely will diff against an empty zone — the pending list may be non-empty immediately after merge.
4. lib/state.py, _collect_firewall (line 449+): compute uncovered_interfaces (network-config ifaces not in any live zone) on every poll, so it is visible even with zero pending changes. Apply the same lo/wg* filter as the WI-1.1 guard. A network-config iface that is absent from live state entirely (down/renamed) counts as uncovered too — not just zoneless-on-live. Add uncovered_interfaces: list[str] to schema.FirewallState (lib/schema.py:87); the TypedDict is shape-checked against the collector return by tests/test_schema_types.py::test_firewall_state (lines 21-54), so the collector must always include the key. Update the FirewallState block in docs/state-model.md (lines 51-78 — the authoritative Markdown reference per lib/schema.py:3-5). Test note: the network-config read is a file read, not run() — patch lib.network.get_config in test_firewall_state (which mocks only lib.state.run) so the value is deterministic. This is the detection that would have caught the incident within 30s.
5. Surface it:
   • daemon/handlers/status.py (lines 62-105): advisory coverage warnings in the firewall section of /api/status/pending (not counted in needs_apply).
   • webui/static/pages/zones.js: warning banner from state.firewall.data.uncovered_interfaces.
   • docs/api.md (line 1965): document the new advisory field in the /api/status/pending firewall section (not counted in needs_apply/total_changes).
   • docs/api.md: note that POST /api/status/apply-all runs the firewall apply with force=false — a coverage ConflictError surfaces in the response errors dict under "Firewall" while the other subsystems proceed (the desired no-silent-apply behavior).
6. UI guard: the interfaces MultiSelectModal in zones.js gets a confirm hook (same pattern as the services lockout at lines 89-99): if the selection would drop an interface's last zone, warn that clients on that segment lose connectivity and DHCP.

Tests: tests/test_firewall.py — absent-key zone keeps live interfaces on apply; explicit [] unassigns; conflict raised when a network iface goes uncovered; force bypasses; guard ignores lo/wg* even when present in network config; live-only-zone interfaces count as covered; pending diff now reports services drift on interface-less zones (absent-key zones emit no spurious interfaces entry). tests/test_api.py / tests/test_status_pending.py for the new advisory field. tests/test_schema_types.py for the new FirewallState key. Test setup note: the guard reads lib.network.get_config() — a file read, not run() — so every non-force _config_apply test must patch lib.network.get_config (e.g. return {"interfaces": {"eth0": {}}} matching the mocked live state). Without it, the repo's real config/network/config.json (carries eth0+eth1) combined with the mocked run (one return string for all calls, so --get-active-zones does not cover eth1) raises a spurious ConflictError; existing tests affected include test_applies_existing_zone (~560) and test_stamps_applied_baseline (~750). The patch value must be consistent with the mocked live state per test: {"interfaces": {"eth0": {}}} only works where the config driving the guard (the lib.firewall.get_config mock) carries an explicit interfaces list (test_stamps_applied_baseline, _STAMP_TEST_CFG with "interfaces": ["eth0"]) — the explicit list covers eth0 in the post-apply union. In test_applies_existing_zone that mock also carries "interfaces": ["eth0"] (the {"public": {}} mock is _get_config, used only for the end-of-apply stamp at firewall.py:396), so {"interfaces": {"eth0": {}}} works there too — but the single-string run mock makes --get-active-zones parse to garbage covering neither eth0 nor eth1, so the simplest patch is {"interfaces": {}} (or upgrade the run mock to a side_effect answering --get-active-zones with eth0 covered). Guard ordering: the https/ssh lockout check (lines 154-169) must run before the coverage guard — test_config_apply_blocks_lockout_before_backup (~670) asserts the "https and ssh" message and mocks neither run nor the network config, so a coverage guard evaluated first would hit the un-mocked run/file read and break that test.

WI-2 — Target-drift semantics (Option A: omit = unmanaged)

Goal: stop the trap where config omits target (→ implicit "default"), live says ACCEPT, the pending diff flags it, and apply can never clear it (firewalld cannot set "default" back) → permanent fake "pending" + dead apply button.

• lib/firewall.py, _compute_pending_changes: skip the target diff when the zone config has no explicit target key or the value normalizes to default (precedent: the public-masquerade skip at lines 419-424). Defensive — it covers legacy configs still carrying explicit "DEFAULT". _config_apply already leaves default targets alone (lines 207-219). Explicit ACCEPT/DROP/REJECT remains fully managed.
• Fix the source — lib/system_import.py, import_firewall (lines 921-932): emit the "target" key only when the imported zone's live target normalizes to something other than default. Today the importer emits a faithful snapshot with a target key for every zone, and _live_target_to_config maps live default → "DEFAULT" (this is the sole author of the explicit "target": "DEFAULT" entries; no other code path writes target into config). Update tests/test_system_import.py (assertions at lines 538-574). Keep _live_target_to_config itself (lib/firewall.py:353-361; still asserted at tests/test_firewall.py:148-149).
• create_zone (daemon/handlers/firewall.py:632-670): rewrite the body to mirror the full _config_apply new-zone branch (lines 188-205): run --new-zone first (currently missing entirely — the endpoint would create no zone at all), then --set-target only when the target normalizes to something other than default, then _reload().
• Config cleanup (on the appliance): remove the legacy "target": "DEFAULT" entries from the public, vpn-full, and work zones (making key-absence the one canonical "unmanaged" notation), and add "target": "ACCEPT" to the internal zone — declares the trusted-LAN intent and makes future apply enforce it and flag any drift. Apply via POST /api/firewall/config (full replace) + apply, not a raw JSON edit and not PATCH (which cannot delete keys — see the Decisions note above): GET the current config, drop the three target entries, add "target": "ACCEPT" to internal, POST, then apply. Apply re-stamps _last_applied_hash/_last_applied_config so cancel-all baselines stay consistent.
• docs/config.md: document "target omitted (or normalizes to default) → live value is preserved, not diffed, and never re-set by apply".

Tests: pending-diff cases (absent target ⇒ no target entry; explicit "DEFAULT" ⇒ no target entry; explicit ACCEPT vs live default ⇒ entry); create_zone paths (--new-zone always called; --set-target only for non-default targets); import_firewall omits target for default-target zones while keeping it for ACCEPT/DROP/REJECT.

WI-3 — Make the sync bus non-destructive (stale DHCP ranges)

Goal: a zone interface change must never delete user data. FirewallToDhcpSync currently hard-deletes ranges the moment an interface loses zone coverage (lib/sync.py:806-822) — exactly what ate the eth1 pool during the incident's mis-click.

• lib/sync.py, FirewallToDhcpSync.on_firewall_config_saved (lines 804-822):
  • Keep the range in the dnsmasq config.
  • Log a warning and emit a SyncResult.changes entry: "DHCP range on '' has no firewall zone coverage — inactive until a zone covers it".
  • Report dnsmasq in affected_subsystems only when the gateway auto-fill step (lines 824-855) actually mutated config — the return at sync.py:866 becomes ["dnsmasq"] if changed else [].
  • Update the class docstring (lines 742-748) and method docstring accordingly.
• Rationale: a range is inert only while the firewall drops the traffic; keeping it makes zone re-assignment self-heal with zero follow-up.

Tests: tests/test_sync.py TestFirewallToDhcpSync (lines 763-920): test_removes_stale_ranges becomes test_flags_uncovered_range_without_deleting (assert dnsmasq config untouched + warning present). test_keeps_global_ranges also asserts the stale eth2 range is removed (len(saved_ranges) == 1, dnsmasq affected, mock_dm_save.call_args read unconditionally) and must be rewritten for the non-destructive semantics (both ranges kept, no save, no affected subsystems, warning present).

WI-4 — Make the firewall "backup" real

Goal: data/firewall/rules.json stores an empty skeleton before and after apply (daemon/handlers/firewall.py:171-179, 385-393), so the disaster-recovery artifact promised by docs/architecture.md:134 contains nothing.

• _config_apply: save a pre-apply snapshot only, before any mutation: {timestamp, default_zone, zones: _parse_all_zones_output(firewall-cmd --list-all-zones --permanent), config: <config.json contents>} → data/firewall/rules.json via _save_backup. The permanent view is what is reproducible for manual recovery. Note: _parse_all_zones_output must be added to the handler's lib.firewall import (daemon/handlers/firewall.py:38-43) — it is not currently imported there.
• Remove the misleading post-apply skeleton write (lines 385-393); the apply response's backup path field is unchanged. load_backup has no live consumers — no API changes.
• Docs: update architecture.md:134/298, config.md:485, overview.md:77 to describe the shape.

Tests: the _save_backup patch sites in tests/test_firewall.py (~lines 566, 701, 751) carry return_value="/tmp/rules.json" and stay valid as-is — no existing test asserts on its arguments or call count. Optionally add one assertion that the single pre-apply call receives the snapshot payload (default_zone/zones/config keys).

WI-5 (optional, low) — Daemon shutdown noise

"Task was destroyed but it is pending" + logging-error tracebacks on daemon SIGTERM (7 occurrences since the Aug 22 restart, still happening on current code).

• daemon/server.py shutdown path: stop accepting new connections, give in-flight request tasks a bounded grace period (await server.wait_closed() with timeout) before runner.cleanup(); suppress the asyncio default exception handler during the teardown window.

Cosmetic. Do last, or defer.

---

Sequencing & verification

1. One branch; one commit per WI: 1 → 3 → 2 → 4 → 5. WI-1 + WI-3 together fix the incident class; WI-2/WI-4 are hygiene; WI-5 optional. Execution: Phase 1 in parallel (file-disjoint streams — A: WI-1 backend, daemon/handlers/firewall.py + lib/firewall.py pending-diff; B: WI-1 state surface, lib/state.py + lib/schema.py + daemon/handlers/status.py + schema/status tests + docs/state-model.md + docs/api.md; C: WI-1 frontend, webui/static/pages/zones.js; D: WI-3, lib/sync.py + tests/test_sync.py); Phase 2 serial — WI-2 → WI-4 → WI-5 (shared files: _config_apply, _compute_pending_changes, tests/test_firewall.py, docs/config.md).
2. Per commit:
   • .venv/bin/ruff check lib/ webui/ daemon/ tests/
   • .venv/bin/ruff format lib/ webui/ daemon/ tests/
   • .venv/bin/python -m pytest tests/ -v
   • node tests/test-*.js for touched hoover components (zones.js itself has no node test file — verify by loading the page in the running UI).
3. Live verification on the appliance after merge:
   • Confirm uncovered_interfaces is empty in firewall state.
   • Check the firewall pending list for the expected post-WI-1.3/WI-2 drift entries (interfaces: [] zones now report field drift; internal target pinned to ACCEPT) and confirm nothing unexpected appears.
   • Optional drill: create a throwaway zone and move eth0 onto it via the API with force omitted (expect ConflictError) and added (expect success + warning), then restore. Skip if undesired — mocked tests cover the logic.
4. No live-system changes during implementation; DHCP/zone state stays as the operator left it (internal=eth1, public=eth0, leases confirmed 01:29).