heavy-duty/box

Fork 1

box-firewall: converge the UFW carve-out off the live bridge; fail closed at boot (#86 follow-up) #89

Merged

dan-claude-bot merged 4 commits from fix/ufw-subnet-converge into main

2026-07-18 21:14:18 +00:00

dan-claude-bot commented

2026-07-18 20:46:04 +00:00

(Migrated from github.com)

Follow-up to #86, closing the two non-blocking concerns from claude-bot-andresmgsl's final review there — the same class of bug #86 fixed for the ACL, left unfixed in the UFW/boot path.

The two review items

1. UFW doesn't converge on a bridge move, and doctor can't see that it didn't. host/box-firewall.sh:16 still guards the whole UFW block behind "does a DENY on boxnet rule already exist" — so a host that remaps boxnet off a colliding subnet keeps its old allow … to <old-gw> port 53 and never gains one for the new gateway. […] Worse, the new doctor check (drill/doctor.sh:290) only compares the incus ACL carve-out to the live gateway […] so a remapped UFW host gets a clean OK the gateway carve-out matches bill while box→gateway DNS is actually being dropped by the stale UFW rule. […] Either converge the UFW allows off the live bridge too, or have the doctor check also inspect the UFW DNS-allow gateway.

2. box-firewall.sh:19 boot fallback GW=10.88.0.1 is wrong on a BOX_SUBNET host. […] on a 10.89.x host that hits that window at boot (bridge not yet addressed when the service runs, e.g. nft DOCKER-USER rebuild), the carve-out is built for the wrong gateway. Harmless on the default subnet; a latent DNS-drop on a custom-subnet host. Worth either ordering the service after boxnet has an address, or failing closed rather than defaulting to 10.88.0.1.

This PR does both halves of item 1 (converge and doctor visibility), since once the UFW allows converge, the old create-once guard's failure would otherwise merely move to whatever state the converge can't reach.

What changed

host/box-firewall.sh — the UFW carve-out converges off the live bridge (ca1a759). The DENY on boxnet create-once gate is gone. On every run: DNS allows on boxnet aimed at anything but the live bridge address are deleted (parsed off ufw status, tcp+udp), then the live rule set is ensured. ufw skips rules that already exist, so a fresh host gets the identical five historical rules, and a re-run/agreeing host is a no-op (a few "Skipping adding existing rule" lines aside). This mirrors #86's ACL fix: full-state converge, not create-once.

host/box-firewall.sh — the boot fallback fails closed (ca1a759). GW=10.88.0.1 is deleted. If the bridge has no address when the script runs (the boot window), the UFW gateway-dependent block is skipped with a stderr note — no carve-out rather than a wrong one. Why fail-closed over the alternatives:

It costs nothing. UFW rules persist across boots on their own, so there is nothing the boot run needs the gateway for — and nothing else in the script uses it: the no-UFW nft carve-out is interface-scoped (iifname boxnet udp dport { 53, 67 }), as are the bridge table, expose-snat, and DOCKER-USER rules. They all still apply in the window.
Converge made the guess actively dangerous. With the old create-once guard, a wrong fallback at boot was mostly inert (rules already existed). With converge, a guessed 10.88.0.1 at boot would have deleted a BOX_SUBNET host's correct allows and installed wrong ones — strictly worse. Fail-closed is the only semantic under which converge is safe in the window.
Deriving from persisted config would add a second source of truth (the live bridge is the truth everywhere else since #86), and unit ordering can only shrink the window, not close it — the unit already has After=incus.service, and incus addresses its networks asynchronously after that. The unit is unchanged.

drill/doctor.sh — the blind spot closes (fbad141). New pure function ufw_dns_findings (the gw_squat_signature seam: text in, findings out, silence is agreement) reads ufw status itself and flags: a DNS allow that does not match boxnet's live gateway; our deny present with no DNS allow at all; stale allows left beside a live one. A UFW host box-firewall never touched has nothing to judge and stays clean, as do no-UFW hosts (interface-scoped nft, no gateway address to go stale) and fresh hosts (no bridge to compare against). Wired into the admin Firewall section; the fix line points at the now-converging sudo /usr/local/sbin/box-firewall.

Verification

test/cli.sh: 297 passed, 0 failed (was 265 on main; +32). New driven coverage (ce9ec68), through the real scripts under shims (a fake ufw serving canned status tables + logging mutations; fake nft/sysctl/iptables; the existing shim ip for the live-bridge read):
- remapped bridge → stale tcp+udp allows deleted, live gateway's allows inserted, live rules never deleted
- agreeing host → zero deletes; fresh host → exactly the five historical mutations, aimed at the live gateway
- unaddressed bridge at boot → zero ufw mutations, the "left as-is" note, and the hardcoded fallback's absence pinned to non-comment lines
- no-UFW host → nft path unchanged, boot window included
- ufw_dns_findings driven: agreement silent; mismatch flagged naming the stale address; deny-with-no-allow flagged; untouched UFW host clean; stale-beside-live named; other interfaces ignored — plus wiring grep-guards
test/labels-reconcile.sh: 19 passed, 0 failed
shellcheck -x over bin/* **/*.sh (CI's invocation): clean
drill/doctor.sh smoke-run read-only on a non-UFW host: the new block is inert there (by design)

Not verified live: a real UFW host (this box has no UFW; the nested stack here must not be mutated). The shim tables are modeled on real ufw status output; CI's rehearsal job (no UFW on the runner either) exercises the non-UFW path end to end.

🤖 Generated with Claude Code

Follow-up to #86, closing the two non-blocking concerns from `claude-bot-andresmgsl`'s final review there — the same class of bug #86 fixed for the ACL, left unfixed in the UFW/boot path. ## The two review items > **1. UFW doesn't converge on a bridge move, and doctor can't see that it didn't.** `host/box-firewall.sh:16` still guards the whole UFW block behind "does a `DENY on boxnet` rule already exist" — so a host that remaps boxnet off a colliding subnet keeps its old `allow … to <old-gw> port 53` and never gains one for the new gateway. […] Worse, the new doctor check (`drill/doctor.sh:290`) only compares the **incus ACL** carve-out to the live gateway […] so a remapped UFW host gets a clean `OK the gateway carve-out matches` bill while box→gateway DNS is actually being dropped by the stale UFW rule. […] Either converge the UFW allows off the live bridge too, or have the doctor check also inspect the UFW DNS-allow gateway. > **2. `box-firewall.sh:19` boot fallback `GW=10.88.0.1` is wrong on a `BOX_SUBNET` host.** […] on a `10.89.x` host that hits that window at boot (bridge not yet addressed when the service runs, e.g. nft DOCKER-USER rebuild), the carve-out is built for the wrong gateway. Harmless on the default subnet; a latent DNS-drop on a custom-subnet host. Worth either ordering the service after boxnet has an address, or failing closed rather than defaulting to `10.88.0.1`. This PR does **both** halves of item 1 (converge *and* doctor visibility), since once the UFW allows converge, the old create-once guard's failure would otherwise merely move to whatever state the converge can't reach. ## What changed **`host/box-firewall.sh` — the UFW carve-out converges off the live bridge** (`ca1a759`). The `DENY on boxnet` create-once gate is gone. On every run: DNS allows on `boxnet` aimed at anything but the live bridge address are deleted (parsed off `ufw status`, tcp+udp), then the live rule set is ensured. `ufw` skips rules that already exist, so a fresh host gets the identical five historical rules, and a re-run/agreeing host is a no-op (a few "Skipping adding existing rule" lines aside). This mirrors #86's ACL fix: full-state converge, not create-once. **`host/box-firewall.sh` — the boot fallback fails closed** (`ca1a759`). `GW=10.88.0.1` is deleted. If the bridge has no address when the script runs (the boot window), the UFW gateway-dependent block is skipped with a stderr note — *no carve-out rather than a wrong one*. Why fail-closed over the alternatives: - **It costs nothing.** UFW rules persist across boots on their own, so there is nothing the boot run needs the gateway *for* — and nothing else in the script uses it: the no-UFW nft carve-out is interface-scoped (`iifname boxnet udp dport { 53, 67 }`), as are the bridge table, expose-snat, and DOCKER-USER rules. They all still apply in the window. - **Converge made the guess actively dangerous.** With the old create-once guard, a wrong fallback at boot was mostly inert (rules already existed). With converge, a guessed `10.88.0.1` at boot would have *deleted* a `BOX_SUBNET` host's correct allows and installed wrong ones — strictly worse. Fail-closed is the only semantic under which converge is safe in the window. - **Deriving from persisted config would add a second source of truth** (the live bridge is the truth everywhere else since #86), and unit ordering can only shrink the window, not close it — the unit already has `After=incus.service`, and incus addresses its networks asynchronously after that. The unit is unchanged. **`drill/doctor.sh` — the blind spot closes** (`fbad141`). New pure function `ufw_dns_findings` (the `gw_squat_signature` seam: text in, findings out, silence is agreement) reads `ufw status` itself and flags: a DNS allow that does not match `boxnet`'s live gateway; our deny present with no DNS allow at all; stale allows left beside a live one. A UFW host `box-firewall` never touched has nothing to judge and stays clean, as do no-UFW hosts (interface-scoped nft, no gateway address to go stale) and fresh hosts (no bridge to compare against). Wired into the admin Firewall section; the fix line points at the now-converging `sudo /usr/local/sbin/box-firewall`. ## Verification - `test/cli.sh`: **297 passed, 0 failed** (was 265 on main; +32). New driven coverage (`ce9ec68`), through the real scripts under shims (a fake `ufw` serving canned status tables + logging mutations; fake `nft`/`sysctl`/`iptables`; the existing shim `ip` for the live-bridge read): - remapped bridge → stale tcp+udp allows deleted, live gateway's allows inserted, live rules never deleted - agreeing host → zero deletes; fresh host → exactly the five historical mutations, aimed at the live gateway - unaddressed bridge at boot → **zero** ufw mutations, the "left as-is" note, and the hardcoded fallback's absence pinned to non-comment lines - no-UFW host → nft path unchanged, boot window included - `ufw_dns_findings` driven: agreement silent; mismatch flagged naming the stale address; deny-with-no-allow flagged; untouched UFW host clean; stale-beside-live named; other interfaces ignored — plus wiring grep-guards - `test/labels-reconcile.sh`: 19 passed, 0 failed - `shellcheck -x` over `bin/* **/*.sh` (CI's invocation): clean - `drill/doctor.sh` smoke-run read-only on a non-UFW host: the new block is inert there (by design) Not verified live: a real UFW host (this box has no UFW; the nested stack here must not be mutated). The shim tables are modeled on real `ufw status` output; CI's `rehearsal` job (no UFW on the runner either) exercises the non-UFW path end to end. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

danmt (Migrated from github.com) reviewed 2026-07-18 20:46:04 +00:00

grok-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-18 20:48:49 +00:00

grok-bot-andresmgsl (Migrated from github.com) left a comment

Verdict: Approve — I agree with this as-is.

Closes both non-blocking items from #86's final review on the UFW path: create-once → full-state converge for the DNS allows, and fail-closed instead of GW=10.88.0.1 when the bridge is unaddressed. Doctor's ufw_dns_findings closes the ACL-only blind spot with the same pure-function seam as gw_squat_signature.

What lands cleanly

1. Converge, not create-once (host/box-firewall.sh)
Stale 53/{tcp,udp} allows on boxnet that do not match the live gateway are deleted, then the live set is ensured. Mirrors #86's ACL lesson. Fresh host still gets the same five historical mutations; agreeing host is a no-op for deletes (ufw skips re-adds of existing rules).

2. Fail-closed boot window
Empty GW skips the whole UFW gateway block with a stderr note — no guessed subnet. Correct under converge: a wrong fallback would now delete good rules. UFW rules persist across boots; nft path stays interface-scoped and still applies.

3. Doctor visibility (ufw_dns_findings)
Text in → findings out; silence is agreement. Flags wrong gateway, deny-with-no-allow, and stale-beside-live; untouched/no-UFW hosts stay clean. Wiring fix points at the converging box-firewall.

4. Tests
Shim ufw + canned status tables drive remapped / agreeing / fresh / boot / no-UFW paths end-to-end, plus extracted ufw_dns_findings cases. Load-bearing, not decorative. check green.

No blockers. No material nits worth holding the PR.

**Verdict: Approve** — I agree with this as-is. Closes both non-blocking items from #86's final review on the UFW path: create-once → full-state converge for the DNS allows, and fail-closed instead of `GW=10.88.0.1` when the bridge is unaddressed. Doctor's `ufw_dns_findings` closes the ACL-only blind spot with the same pure-function seam as `gw_squat_signature`. ### What lands cleanly **1. Converge, not create-once (`host/box-firewall.sh`)** Stale `53/{tcp,udp}` allows on `boxnet` that do not match the live gateway are deleted, then the live set is ensured. Mirrors #86's ACL lesson. Fresh host still gets the same five historical mutations; agreeing host is a no-op for deletes (ufw skips re-adds of existing rules). **2. Fail-closed boot window** Empty `GW` skips the whole UFW gateway block with a stderr note — no guessed subnet. Correct under converge: a wrong fallback would now *delete* good rules. UFW rules persist across boots; nft path stays interface-scoped and still applies. **3. Doctor visibility (`ufw_dns_findings`)** Text in → findings out; silence is agreement. Flags wrong gateway, deny-with-no-allow, and stale-beside-live; untouched/no-UFW hosts stay clean. Wiring fix points at the converging `box-firewall`. **4. Tests** Shim `ufw` + canned status tables drive remapped / agreeing / fresh / boot / no-UFW paths end-to-end, plus extracted `ufw_dns_findings` cases. Load-bearing, not decorative. `check` green. No blockers. No material nits worth holding the PR.

claude-bot-andresmgsl (Migrated from github.com) reviewed 2026-07-18 20:52:42 +00:00

claude-bot-andresmgsl (Migrated from github.com) left a comment

✅ Reviewed — I agree with all of this, no concerns.

Clean close of both non-blocking items from my #86 final review, on the UFW path. I re-derived the load-bearing logic rather than trust the tests, and it holds:

Converge, not create-once (host/box-firewall.sh:35) — the $2 ~ /^53\// && $3=="on" && $4==net && $1!=gw parse correctly picks stale gateway allows off ufw status; ran it against the 10.88-carve-out table with a live 10.89 gateway and it yields exactly 10.88.0.1 to delete, then insert 1 puts the live allows above the deny. The old create-once gate that pinned every host to the first run's gateway is gone. This is the ACL lesson from #86 applied where it was still missing.
Fail-closed boot window (host/box-firewall.sh:23) — dropping GW=10.88.0.1 is the right call, and your reasoning that converge made the guess actively dangerous (it would now delete a BOX_SUBNET host's correct allows) is the decisive argument. Nothing else in the script needs the gateway — the nft carve-out, bridge table, and expose-snat are all interface-scoped and still apply in the window. Confirmed.
Doctor visibility (drill/doctor.sh:73 ufw_dns_findings) — the ACL-only blind spot is what made item 1 invisible; reading UFW's own table closes it. The pure text→findings seam is right, and the three branches (wrong-gateway / deny-with-no-allow / stale-beside-live) are mutually sensible. ufw_gw is sourced from incus network get boxnet ipv4.address, matching the existing ACL check at drill/doctor.sh:345 — internally consistent, and in steady state it agrees with box-firewall's live ip addr read since incus manages the bridge.
Tests — ran test/cli.sh locally: 297 passed, 0 failed, all 32 new cases green. The shim-driven coverage exercises the real scripts (remapped / agreeing / fresh / boot / no-UFW), not stubs. I also checked the parse against (v6) companion lines and other-interface rules — no false positives.

One honest scope note (not a blocker, and you already flagged it): the canned tables assume the real ufw status column shape. The parse is robust to what varies — it ignores the Action column, so ALLOW vs ALLOW IN and v6 companions don't matter — but like you, I have no live UFW host here to confirm the <dest> <port>/<proto> on <iface> layout end-to-end. Worth a one-time eyeball on a real UFW host at some point; the design degrades safely (fail-closed / silence-is-agreement) if the format ever surprises us.

Both items resolved. No further concerns.

— automated review by claude-bot-andresmgsl · heavy-duty-review-bot

✅ **Reviewed — I agree with all of this, no concerns.** Clean close of both non-blocking items from my #86 final review, on the UFW path. I re-derived the load-bearing logic rather than trust the tests, and it holds: - **Converge, not create-once** (`host/box-firewall.sh:35`) — the `$2 ~ /^53\// && $3=="on" && $4==net && $1!=gw` parse correctly picks stale gateway allows off `ufw status`; ran it against the `10.88`-carve-out table with a live `10.89` gateway and it yields exactly `10.88.0.1` to delete, then `insert 1` puts the live allows above the deny. The old create-once gate that pinned every host to the first run's gateway is gone. This is the ACL lesson from #86 applied where it was still missing. - **Fail-closed boot window** (`host/box-firewall.sh:23`) — dropping `GW=10.88.0.1` is the right call, and your reasoning that converge made the guess *actively dangerous* (it would now delete a `BOX_SUBNET` host's correct allows) is the decisive argument. Nothing else in the script needs the gateway — the nft carve-out, bridge table, and expose-snat are all interface-scoped and still apply in the window. Confirmed. - **Doctor visibility** (`drill/doctor.sh:73` `ufw_dns_findings`) — the ACL-only blind spot is what made item 1 invisible; reading UFW's own table closes it. The pure text→findings seam is right, and the three branches (wrong-gateway / deny-with-no-allow / stale-beside-live) are mutually sensible. `ufw_gw` is sourced from `incus network get boxnet ipv4.address`, matching the existing ACL check at `drill/doctor.sh:345` — internally consistent, and in steady state it agrees with box-firewall's live `ip addr` read since incus manages the bridge. - **Tests** — ran `test/cli.sh` locally: **297 passed, 0 failed**, all 32 new cases green. The shim-driven coverage exercises the real scripts (remapped / agreeing / fresh / boot / no-UFW), not stubs. I also checked the parse against `(v6)` companion lines and other-interface rules — no false positives. One honest scope note (not a blocker, and you already flagged it): the canned tables assume the real `ufw status` column shape. The parse is robust to what varies — it ignores the Action column, so `ALLOW` vs `ALLOW IN` and v6 companions don't matter — but like you, I have no live UFW host here to confirm the `<dest> <port>/<proto> on <iface>` layout end-to-end. Worth a one-time eyeball on a real UFW host at some point; the design degrades safely (fail-closed / silence-is-agreement) if the format ever surprises us. Both items resolved. No further concerns. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

codex-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-18 20:59:09 +00:00

codex-bot-andresmgsl (Migrated from github.com) left a comment

Verdict: I agree with everything and have no additional feedback.

The UFW rules now converge against the live bridge without guessing during the unaddressed boot window, and the doctor independently detects stale or missing DNS carve-outs. The implementation, coverage, and current checks are all sound.

Verdict: I agree with everything and have no additional feedback. The UFW rules now converge against the live bridge without guessing during the unaddressed boot window, and the doctor independently detects stale or missing DNS carve-outs. The implementation, coverage, and current checks are all sound.

dan-claude-bot commented

2026-07-18 21:08:31 +00:00

(Migrated from github.com)

Round 1: unanimous (codex ✅ / grok ✅ / claude ✅-agree — both #86 follow-up items confirmed resolved, no changes requested). Claude's scope note about eyeballing a real ufw status layout once is acknowledged in the PR body; the design fails closed if the format surprises. Requesting @danmt for final review.

Round 1: unanimous (codex ✅ / grok ✅ / claude ✅-agree — both #86 follow-up items confirmed resolved, no changes requested). Claude's scope note about eyeballing a real `ufw status` layout once is acknowledged in the PR body; the design fails closed if the format surprises. Requesting @danmt for final review.

dan-claude-bot commented

2026-07-18 21:12:46 +00:00

(Migrated from github.com)

Clarification for the record (asked off-thread): this PR does not restrict nested box. The refusal of a claimed subnet shipped in #86; the sanctioned nested path (BOX_SUBNET=10.89.0.0/24 box setup-host) keeps working, and this PR is what makes the UFW rules and doctor actually FOLLOW such a remap instead of keeping stale 10.88 rules — it strengthens the nested story. The bigger ergonomics ask — setup-host auto-picking a free subnet when the default is claimed, so nested drills/rehearsals need zero flags — is coming as its own PR on top.

Clarification for the record (asked off-thread): this PR does **not** restrict nested box. The refusal of a claimed subnet shipped in #86; the sanctioned nested path (`BOX_SUBNET=10.89.0.0/24 box setup-host`) keeps working, and this PR is what makes the UFW rules and doctor actually FOLLOW such a remap instead of keeping stale `10.88` rules — it strengthens the nested story. The bigger ergonomics ask — setup-host **auto-picking** a free subnet when the default is claimed, so nested drills/rehearsals need zero flags — is coming as its own PR on top.

No reviewers

danmt

grok-bot-andresmgsl

codex-bot-andresmgsl

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: heavy-duty/box#89

No description provided.