mteehan/vacuum-wall
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

firewall: interface-coverage apply guard, target drift, non-destructive DHCP sync

Browse Source 
Post-DHCP-incident hardening per HARDEN.md.

- apply guard: refuse (ConflictError, `force` overrides) when a
  network-managed interface would end up in no zone; absent
  `interfaces` key = hands-off, explicit `[]` = unassign-all
- surface `uncovered_interfaces` in firewall state (lo/wg* filtered)
  + advisory in /api/status/pending; zones.js banner + interfaces-picker
  last-zone confirm
- target drift (Option A): absent or default-normalizing target is
  unmanaged: not diffed, never re-set by apply; create_zone runs
  --new-zone first and sets non-default targets only; importer omits
  the target key for default zones
- FirewallToDhcpSync keeps stale DHCP ranges and flags them instead of
  deleting; `dnsmasq` affected only on a real gateway mutation
- real pre-apply recovery snapshot in data/firewall/rules.json
  ({timestamp, default_zone, zones, config}); drop the empty post-apply
  skeleton
- daemon shutdown: bounded grace for in-flight tasks + suppressed
  teardown exception noise on SIGTERM
- also carries the firewall service-descriptions feature
  (get_service_descriptions + service_descriptions state field + UI)
- tests + docs across firewall/status/state/sync/schema; ruff clean,
  867 passing
This commit is contained in:
Mike Teehan
2026-08-28 23:38:21 +00:00
parent 55309cfd86
commit ac52918df5
25 changed files with 1677 additions and 168 deletions
Download Patch File Download Diff File Expand all files Collapse all files
HARDEN.md
+289
View File
@@ -0,0 +1,289 @@
# 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 '<iface>' 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).