fix(packaging): the wired LAN ports get a bridge of their own, scoped to the admin surfaces

[felixfelix-bot]

fix(packaging): the wired LAN ports get a bridge of their own, scoped to the admin surfaces

Implements the bridge half of
docs/architecture/lan-port-management-bridge-decision.md
(#599, D1-D8): the wired LAN ports move off the captive bridge onto br-mgmt, so
a cabled operator reaches the administration surfaces before paying anything, and
the port stops sharing a layer-2 domain with the strangers on the open guest
SSID.

The requirement this was written for has two halves, and this PR delivers one
of them. The operator asked for a br-mgmt "where the connected devices can
access luci and the luci alternative dashboard, but they still need to pay". The
first half is here. The second half — a paywalled wired bridge — is not
delivered, and cannot be with the shipped stack: #599 measured that nodogsplash
5.0.2 gates exactly one interface and that a second instance is the dangerous
wrong answer (fixed nds* chain names in one namespace, teardown by name, one
control socket), so br-mgmt has no captive gate at all. A bridge this module
cannot gate must not take money it cannot deliver, so it is built as a
management network: admin surfaces reachable, no internet, and the payment
surfaces unreachable. D8 in the decision record says exactly this; F1/F2 is
the follow-up if a wired customer network is still wanted.

The three writers

1. setup_mgmt_bridge in packaging/files/etc/uci-defaults/99-tollgate-setup
(+206) — MOVES the port list the base image writes on the captive bridge onto
br-mgmt:

• it never names a port (eth1 on the MT3000, lan1…lan5 elsewhere — it moves
  what it finds on network.lan.device's device section, located by NAME because
  the device sections are anonymous in a shipped config);
• a router with no port list on the captive bridge and none on br-mgmt is
  refused: ERROR, return 1, and no uci set at all — a portless br-mgmt
  would report a management bridge the operator has no way into;
• an existing network.mgmt.ipaddr is kept (a reinstall must not move an address
  the operator chose); otherwise it takes the first free /24 next to the LAN's,
  skipping the private network's and any upstream collision, reusing the existing
  prefix24_of/octet_of/subnet_conflicts helpers;
• dhcp.mgmt so the laptop has a lease (and therefore so its MAC still resolves,
  which is what keeps a future F1 possible without redesigning identity);
• its own firewall.mgmt_zone with forward 'REJECT' and no config forwarding — deliberately not firewall.private_zone, whose forward 'ACCEPT' plus private → wan forwarding would hand every wired client free
  internet, which is the hole this change exists not to create.

The writer is re-asserted on the same-version (reinstall/upgrade) path as well
as full setup, because the port list is image-owned: sysupgrade -n or a factory
reset puts the wired port back on the captive bridge, and nothing else in this
repository would notice. Each of the three configs it touches is committed only
when its export actually differs, so a converged router is a no-op.

2. packaging/files/etc/nftables.d/33-mgmt-bridge-scope.nft (new, +86) — an
input fragment, same shape as its five siblings so fw4 reload loads it and a
nodogsplash restart does not remove it:

• allow: DHCP/DNS (53/67/68), SSH (22), and the four administration surfaces
  443, 8080, 8090, 8443;
• IPv6 dropped outright (the bridge has ip6assign '0' and no RA, the same
  posture setup_disable_ipv6_lan takes);
• a catch-all counter drop for everything else, which is what makes
  "this bridge cannot transact" structural rather than incidental: :2050,
  :2051 and :2121 are not in the list, and :2121 is what marks a network
  that sells.

3. Both packaging recipes — packaging/Makefile and
packaging/local-build-ipk.sh install the new fragment (an nftables file under
packaging/files/ that no recipe installs is not a control).

The scope is pinned in BOTH directions

31-admin-board-not-guest-reachable.nft, 32-luci-not-guest-reachable.nft,
20-nds-enforce.nft and 30-backend-firewall.nft are unchanged and still match
br-lan by name, which is the whole mechanism: they are about a stranger on an
open SSID, and br-mgmt holds no stranger (reaching it means walking to the
router and plugging in a cable). The tests refuse both drift directions —
extending a guard to br-mgmt, or extending the new fragment to br-lan — and
refuse an allow list that re-opens a payment surface.

Evidence

Offline — every assertion #599 lists for this tier (1-8), all green on the
merged tip (1c22bfac):

#	assertion (decision record)	where it is pinned	result
1	the move, and the loud refusal	tests/uci-defaults-mgmt-bridge_test.sh — full-setup writer: the captive bridge's port list is cleared / br-mgmt lists port eth1 exactly once / returns non-zero when there is nothing to move / LIB_ONLY refusal: nothing was written	57 passed, 0 failed
2	idempotent, no duplicates	same suite (…listed exactly once, two-run fixture)	—
3	one nodogsplash, gatewayinterface br-lan, nothing else	same suite — exactly one nodogsplash section, pinned to the captive bridge (4 assertions)	—
4	both guards stay on br-lan, no fragment drops them on br-mgmt	tests/packaging/mgmt-bridge-scope_test.sh — 31-*.nft is pinned…, 32-*.nft is pinned…, 30-backend-firewall.nft still drops :2121 from every network but br-lan/loopback	37 passed, 0 failed
5	nft -c -f inside the table inet fw4 {} wrapper, with a negative control; allow list and the covered drops	same suite — nft -c accepts every fragment inside one table inet fw4 + negative control: nft -c rejects a malformed rule + the two allow-set assertions	—
6	no forwarding to wan, no reuse of private_zone	uci-defaults-mgmt-bridge_test.sh — the mgmt zone is forward REJECT (no way out to the wan), no forwarding section names the mgmt zone as a source, firewall.private_zone still owns only the private network (not reused)	—
7	the same-version path reaches the writer	same suite — RED: the bridge is written on the reinstall path	—
8	make go-battery unaffected	no Go file is touched: git diff --name-only upstream/main...HEAD lists 10 files, none .go	n/a

Both suites also carry negative controls that must go red when the line they
pin is removed (four in the uci-defaults suite, three in the packaging suite,
including a RED run that proves the catch-all drop is what makes the allow list
real and a second that reddens when :2121 is added to it) — so they are tests,
not transcripts.

The whole offline tier, re-run on the merged tip — 18 suites, 0 failures:
uci-defaults-{admin-tls-identity,band,luci-ports-not-pre-auth,mgmt-bridge,nodogsplash-convergence,private-subnet,same-version-allowlist,setup-marker-order,trusted-entry-80},
packaging/{admin-board-not-guest-reachable,admin-board-requires-credential,guest-client-isolation,luci-not-guest-reachable,mgmt-bridge-scope,package-nodogsplash-dependency},
ngit-{ci-trigger,release-pipeline}, verify_publication — 0 failed in every one.

The two suites are now wired into CI. They existed but nothing invoked them:
both lanes enumerate the offline suites by name, and a suite that is not listed is
a suite that never runs. contract-lint in .github/workflows/test.yml and in
the .ngit/act copy now run both, next to the other uci-defaults suites (second
commit, 68a0e6fa).

The decision record's review findings ask the implementing PR for two harnesses
and both are added here. The findings in flight on the record (PR #600) note
that an nft -c -f wrapper harness does not exist in the tree
(grep -rn "nft -c" matches nothing) and that the existing offline uci shim
answers show with a silent no-op, so a writer enumerating sections that way
would see no ports and take the fail-loudly branch on every fixture. Assertion 5
is met by the generated table inet fw4 {} wrapper plus its negative control,
and assertion 1 by a suite whose shim answers get on network.@device[N] over a
fixture seeded with flat keys — and which records delete/del_list, so a
removal assertion cannot pass vacuously. This writer addresses the section by
name via uci -q get network.@device[N].name/.ports rather than by a
uci show … | awk pipeline, which is the valid addressing for an anonymous
device section (/bin/config_generate:109-119 emits it unnamed).

Not measured, and not claimed: the bench tier. Assertions 9-17 of the
decision record (the port really moved, :8090/:8080/:443 answer from the
cable pre-auth, ping 9.9.9.9 fails, :2121/:2050/:2051 are refused from the
cable, the guest path is unchanged end to end, one gate, the mgmt lease, the
money path untouched, and recovery via the private SSID) need a router and have
not been run: this PR is an unmeasured build, and per this repo's promotion order
nothing here should reach a feed bump before those rows are recorded on the
artifact that carries it. Two of those rows must be read as the decision record's
review findings word them rather than as a guarantee this fragment makes
reachable: :8443 answers only when the opt-in listener exists (the feed's
92-tollgate-admin-setup writes it, 99-tollgate-setup only clears it), and
:22 answers only if dropbear listens on the mgmt network — nothing in
99-tollgate-setup configures SSH. The fragment allows both; reachability is
the bench row's question.

Docs

docs/rc-tester-guide.md §7 is corrected in the same change: it told a tester
that tcp 8090/8443 are dropped for "a wired-LAN or guest-SSID client", which
after this change is true of a guest-SSID/br-lan client and false of the wired
one. It now says which bridge each client is on, what a cable can reach
(including that the wired bridge has its own address — uci get network.mgmt.ipaddr — so http://<router>.lan/ is not the URL to use), and that
a cable into a LAN port is an administration port, not a customer one.

CHANGELOG.md: one entry under Changed / Internal, re-based on the #599 entry
that is now on main (the merge kept both, main's first, and the bullet sets were
compared before/after to prove nothing was dropped).

Merge state

Merged forward with main (5296eff7, so #594, #598 and #599 are included) by
git merge --no-ff, never a rebase. git diff --stat upstream/main...HEAD
touches 10 files: 2 CI workflow files, CHANGELOG.md, docs/rc-tester-guide.md,
packaging/Makefile, the new fragment, 99-tollgate-setup,
packaging/local-build-ipk.sh and the two test suites. No Go file, no portal
file. Commits: 29ae09e1 (the bridge and its scope), 68a0e6fa (CI wiring),
1c22bfac (merge-forward), 9e904b81 (the CHANGELOG link pointed at the number
the entry was written for, not the number this PR opened as). Landing it is the
maintainer's call.

[Amperstrand]

Technical review (evidence pass) — PR head 9e904b81e4faa4f6bb1a40b7b70f3c2631b972b0 (base 2 commits behind main; both post-date this branch's merge-forward point 5296eff7, GitHub reports MERGEABLE, no rebase needed).

This PR implements the bridge half of the #599 ADR exactly as the record specifies it, including the parts #600's review findings added to the record after this branch had already merged forward — which is the cross-context I checked hardest, details below. I re-ran the evidence myself rather than trusting the body's tables: both new suites pass as claimed (tests/uci-defaults-mgmt-bridge_test.sh 57/0, tests/packaging/mgmt-bridge-scope_test.sh 37/0 — and on this host sudo -n is available, so the nft -c -f wrapper half genuinely executed, wrapper acceptance plus the malformed-rule negative control both green), and I re-ran the rest of the offline uci-defaults/packaging tier the writer touches: setup-marker-order 156/0, same-version-allowlist 51/0, nodogsplash-convergence 29/0, private-subnet 12/0, luci-ports-not-pre-auth 37/0, plus the five other packaging suites — 0 failures anywhere. git diff --name-only origin/main...HEAD confirms the body's claim: 10 files, no .go, so make go-battery is untouched by construction.

The writer is the part worth trusting least on paper and most in practice. setup_mgmt_bridge (99-tollgate-setup#L1795-L1918) checks every precondition before the first uci set (portless router → ERROR, return 1, zero writes — pinned including through the LIB_ONLY seam), locates the anonymous device section by name in a bounded scan rather than trusting an index, moves rather than copies (the delete at L1892 is what makes it a move, and RED control (a) proves the assertion bites), reuses the existing prefix24_of/subnet_conflicts/choose_private_subnet collision machinery for its address, and is re-asserted on the same-version path (L2039-L2070) with per-config commit-only-on-diff — which is the correct answer to the port list being image-owned across sysupgrade -n. The refusal on a portless router returning 1 but the driver continuing (no set -e, and the same-version suite pins exit 0 with the ERROR logged) is the right shape: loud, non-fatal to the rest of setup.

ADR alignment, including against main's newer #600. This branch merged main at 5296eff7, i.e. before 25f8a348 (#600) folded the review findings into the record — but the PR already answers both things #600 made requirements of the implementing PR: assertion 1's amendment (shim must answer get on network.@device[N] or the fixture must seed flat keys, with the negative proven in the same change — the suite does both, and implements del_list/delete so removal assertions can't pass vacuously) and assertion 5's amendment (the nft -c -f table inet fw4 wrapper harness "does not exist in the tree today — the implementing PR must add it": added at mgmt-bridge-scope_test.sh#L272-L292 with its negative control). One deliberate deviation, correctly called out in the PR body: amended D1's parenthetical suggests the uci show network | awk idiom, while the writer uses uci -q get network.@device[N].name in a bounded loop. That is valid UCI addressing for an anonymous section (the ADR's complaint is @device[br-lan] name-indexing, not positional @device[N]), and it is precisely what makes the move provable against the offline shim. No change wanted here — but since the ADR text on main still prescribes the awk pipeline, a one-line ADR touch-up recording the implemented idiom would keep the record and the tree telling the same story (follow-up, not a request).

One design question I'd put to the maintainer, non-blocking. The zone is written input ACCEPT (L1916) per D2, on the stated grounds that "fw4 needs the zone permissive for DHCP/DNS/SSH/the admin ports to work at all". That justification is doubtful: the fragment is its own base chain at input priority −1, and an accept verdict there is final — fw4's zone chain at filter priority never sees those packets, so input REJECT would behave identically while the fragment is loaded and would fail closed in exactly the case the fragment's own CAVEAT names (33-mgmt-bridge-scope.nft#L54-L56: a renamed bridge means the rules stop matching and "the ports fall back to the zone's input policy" — which is currently ACCEPT-everything). D2 records input ACCEPT as the decision, so I'm not asking this PR to relitigate it; I'm noting that the fail-safe direction looks free on bench row 10/11, and if the semantics hold as I read them, flipping the zone to input REJECT (fragment unchanged) would make the catch-all drop redundant armor instead of the only armor. Worth one bench measurement before F1/F2 builds anything on top.

Scope pinning and security. The both-directions story is real, not prose: the four guest guards are asserted free of any br-mgmt mention, the new fragment asserted free of any br-lan rule, the :2121 ACL in 30-backend-firewall.nft asserted still br-lan/lo-only, and the two RED controls that matter most for the money half — catch-all drop stripped, and :2121 widened into the allow list — both go red as designed (mgmt-bridge-scope_test.sh#L296-L323). The one-gate invariant (single config nodogsplash, gatewayinterface br-lan, no stray writers) is pinned in the uci-defaults suite around L506-L530. No new dependency surface, no new shell-outs, no portal/Go changes; the money path is untouched and the only payment-relevant property is the negative one (a cable cannot reach :2050/:2051/:2121), which is structural via the catch-all drop rather than incidental. Reaching :8090 (cleartext, root-capable) from a physical cable is the ADR's trust-boundary argument, and I accept it as recorded.

Hygiene, docs, and sequencing. Four commits, each accurately scoped (implementation, CI wiring for both lanes, merge-forward, changelog-link fix), no assistant attribution anywhere in the bodies. The CHANGELOG merge resolution dropped nothing from main (additions-only, verified against 5296eff7) and the new entry sits under Changed / Internal with the #601 link in house style. docs/rc-tester-guide.md §7 is corrected in the same change as the ADR mandates, with the :8443-is-opt-in caveat carried correctly. One sequencing note for criterion-watchers: open PR #607 (LAN port role: mgmt | private | guest) builds directly on this writer — it needs this to land first, and no other open PR or issue overlaps (the lockout was a bench report, not a tracked issue, so the ADR cross-reference is the correct one in place of Closes #N).

Disposition: land-with-followups.

• Record ADR bench rows 9–17 on the artifact that carries this change before any feed bump / release cut — the PR commits to this and the ADR's rollout makes it the gate; rows 11 and 12 are the difference between "operator can administer" and "stranger has free internet", and rows 10/11 should also answer the input-policy question above.
• Optional ADR touch-up: record the implemented @device[N] enumeration idiom in D1 (one clause).
• Optional, bench-informed: firewall.mgmt_zone input REJECT as the fail-closed fallback.

The offline evidence is as strong as this tier gets — real RED controls on both halves, recipe-coverage proven by building an actual .ipk, and the CI lanes now invoke what they pin. What's missing is exactly what the PR says is missing: the router.

[felixfelix-bot]

Cold cross-family review: OpenTollGate/tollgate-module-basic-go #601

reviewer_model: kimi-k3
head reviewed: 82ee5df
verdict: APPROVED

Findings

• [info/Q1] 31-admin-board-not-guest-reachable.nft / 32-luci-not-guest-reachable.nft — rules still match iifname "br-lan" literally; moving wired ports to br-mgmt does not widen guest-SSID exposure.
• [info/Q2] firewall.mgmt_zone — forward REJECT plus no config forwarding section using mgmt as source means no internet from br-mgmt; output ACCEPT is router-originated.
• [info/Q3] 33-mgmt-bridge-scope.nft — accepts admin ports only on br-mgmt; tail drop catches 2050/2051/2121; IPv6 family dropped outright.
• [info/Q4] setup_mgmt_bridge — add-then-delete ordering carries the port from br-lan to br-mgmt; normal uci operation prevents duplication, and tests pin this.
• [info/Q5] setup_mgmt_bridge — the "no ports" guard requires both captive and mgmt port lists empty; on reinstall mgmt_ports is non-empty so the writer proceeds correctly.
• [info] Honest scope: PR body does not claim payment/internet on br-mgmt; bench rows 9-17 explicitly not measured.

Reviewer working notes

Generated via oneshot-cross-family-review prompts against the live router/local proxy.
The author family was deepseek/glm-backed; this review was produced by the kimi family to satisfy D-115.

[Amperstrand]

Technical review (evidence pass) — delta re-review, PR head 82ee5df8cdd11752a8a6a9df1c42db34c61a2371 (base 1 commit behind main; MERGEABLE). My prior review stood at 9e904b81; this pass covers only the two commits since — the merge-forward of main (96990a19) and the src/go.sum repair (82ee5df8) — plus main's movement. The author's round-6 thread documents the go.sum defect and fix; I verified it independently rather than taking the verdict on faith.

The go.sum fix is exactly what the thread claims, proven at the blob level. src/go.sum is blob eaeefca8 on both parents of the merge (9e904b81 and main's 84f9cb6f), on the current main tip, and on the fixed head; only the merge commit carried the bloated 7cf897ef (+148 entries, none removed). The fix restores the shared blob with numstat 0 additions / 148 deletions and touches nothing else. On the head tree I ran the checks the thread names: go mod tidy -diff exits 0 with an empty diff and go build ./... exits 0. Net effect on the PR surface: the diff is back to the same 10 files, no .go, no go.sum — make go-battery remains untouched by construction.

The merge-forward is clean in both directions, and I re-tested the semantic half. Against the main parent, the merge differs only in this PR's own files — nothing from main was dropped; the CHANGELOG resolution is additions-only; both CI files (.github/workflows/test.yml and the .ngit/act mirror) carry both sides' suite invocations — this PR's two mgmt-bridge suites and main's new device-code/gatewayname suites. Textual cleanliness isn't the risk here: main's #605 heavily edited the same writer, 99-tollgate-setup, so I re-ran the offline tier at the new head: uci-defaults-mgmt-bridge 57/0, packaging/mgmt-bridge-scope 37/0 (RED controls included), setup-marker-order 159/0 (main grew it 156→159), device-code 72/0, same-version-allowlist 51/0. The two writers coexist and the assertions still bite.

Interaction with main since the merge. Main's only new commit is #614 (54e8c368, router-test workflow), which overlaps this PR in CHANGELOG.md alone — no conflict — and its physical+lab router lanes are the natural vehicle for the bench-rows followup below. #615 (admin LAN subnet collision) rode in via this merge as an explicit proposal: no requirement on this PR, though if it ever ships it touches the same prefix24_of/collision machinery br-mgmt's address derives from — the cross-reference will belong there, not here.

Disposition: land-with-followups — unchanged. The prior gate stands: record ADR bench rows 9–17 on an artifact carrying this change before any feed bump or release cut (rows 10/11 also answering the input-policy question), plus the two optional touch-ups (the D1 idiom clause, bench-informed input REJECT). Nothing in the delta moves any of those. The author's round-6 residual notes (24-section scan cap failing loud, forced /24 netmask, the unexercised uci export branch) match my read of the code; none is a blocker.

[Amperstrand]

Technical review (evidence pass) — PR head fa1ab55db48bac6a358e46c01a6f07932beb0eee (2 commits behind main; mergeable as-is). Author: the drafting account.

This implements D1–D8 of the wired-LAN bridge record: setup_mgmt_bridge moves the image-owned port list off the captive bridge onto br-mgmt, scoped by a new 33-mgmt-bridge-scope.nft allow list (DHCP/DNS, SSH, :443/:8080/:8090/:8443, IPv6 dropped outright, catch-all drop last), with its own mgmt_zone (forward REJECT, no wan forwarding, no private_zone reuse) and dhcp.mgmt. The implementation matches the body claim-for-claim — I walked every one: the name-free @device[N]-by-name port move, preconditions-before-writes with the fail-loudly return 1 (the driver is plain #!/bin/sh, no set -e, so the semantics are exactly "log, change nothing, continue"), network.mgmt.ipaddr preservation, first-free-/24 derivation reusing prefix24_of/octet_of/subnet_conflicts/upstream_networks/choose_private_subnet (all verified present on main), ip6assign='0' + ra/dhcpv6 disabled, and the same-version re-assert block with per-config export-diff commits. The money-path property is doubly enforced and I verified both layers: :2121 is absent from the allow list, and 30-backend-firewall.nft independently drops :2121 from everything but br-lan/loopback — the bridge cannot transact even if the fragment were lost. The two offline suites are the best in this repo: mutant RED controls for every load-bearing line (the port-list delete, the reinstall-path call, the forward REJECT), an exact allow-set == guards'-drop-set equality check, a non-vacuous nft -c negative control, an ipk built from the recipe's own install lines through assert-artifact-contents.sh, and a gateway-interface containment check that pins "one gate, ever". The bench tier is honestly declared not measured, which matches the record's promotion order (nothing reaches a feed bump before assertions 9–17 are recorded on the artifact).

Two things stand between this and landing, and both are decisions rather than defects:

1. #607 contains this PR. #607 (retargeted to main, no longer stacked here) carries setup_mgmt_bridge, the same 33-mgmt-bridge-scope.nft, both of this PR's test suites, the same Makefile/local-build/CI wiring — plus the tollgate.lan_ports.role generalization (mgmt|private|guest), /etc/config/tollgate, keep.d, operator-guide and its own edits to the bridge record. Merging both as-is is impossible; they are the same lines. The choice is: land this PR (narrower, record-faithful D1–D8, deeply tested) and rebase #607 down to its role layer, or close this in favor of #607's superset. What tips it for me: #607 is younger, unreviewed at this depth, also 2 behind main, and edits the bridge record on a base that predates #623's amendment — whoever lands second owns a meaty ADR/CHANGELOG reconciliation. If the role field is wanted for v0.6 anyway, landing this first and shrinking #607 is the lower-risk order — but that is the maintainer's call to make before either merges.

2. The record this implements was narrowed while this PR was open. #623 (merged as fa414c73, not in this branch) amends the ADR this PR cites as its motivation: the operator's requirement — cable reaches admin surfaces and still pays — is now recorded as satisfiable by re-keying the guards alone (AM-1, card t_8590499a), and br-mgmt stays Proposed on the L2-separation merit alone (AM-4). This PR's body and CHANGELOG bullet still present "a cabled operator reaches the administration surfaces before paying" as the delivered requirement-half; post-amendment that is AM-1's claim, and D1's independent merit is who may see whom. Nothing here contradicts the amendment technically — the two mechanisms compose (the guards stay br-lan-scoped either way, and D7's never-extend-to-br-mgmt is pinned by your own test) — but landing D1 is now a deliberate choice between two lockout fixes, and the PR should say so. Concretely: merge main forward (fa414c73 + aba19479; expect CHANGELOG adjacency), cite the amendment in the body and the CHANGELOG bullet, and reframe the rc-tester-guide note's motivation the same way.

Two notes below the blocker line. Deployment impact worth saying out loud in the PR body: on every already-flashed router, a wired client that today can pay (it sits on br-lan) loses that ability entirely at the next package upgrade until F1/F2 exists — the CHANGELOG discloses it ("a wired client therefore has no internet at all"), but it is a behavior change for any operator actually selling over the cable. And one nano-nit while I was in the function: private_ip is missing from setup_mgmt_bridge's local list (it works because the value is read from uci, but it writes a shell global in ash — one word fixes it).

Security posture verified: allow-list-with-catch-all, IPv6 refused, no route off the box, payment surfaces structurally unreachable, cable-as-trust-boundary argument is the record's own D2/D4, and the guard-scope invariants are test-pinned in both directions. No Go, config-schema or wire surface; both CI workflows (GitHub and ngit) wired for the uci suite; commits clean, no attribution footers; bench debt is explicit and correctly gated at the feed bump, not at main.

Disposition: request-changes — (1) the maintainer decides this PR vs #607 before either lands (my lean: this one first, #607 rebased to the role layer); (2) merge main forward through fa414c73 and re-anchor the body/CHANGELOG framing to the amended record. The implementation itself is merge-quality.

[felixfelix-bot]

tags: code-review
reviewer_model: glm-5.3
review_tier: tier/review-glm

Review of head fa1ab55db48bac6a358e46c01a6f07932beb0eee (fetched from the fork; rev-parse verified). Gate-2.5 cold audit (worker-reviewer-ds) incorporated.

VERDICT: CHANGES-REQUESTED — 1 BLOCK, 1 RISK, 2 NIT, 2 INFO.

[BLOCK] 99-tollgate-setup:2058 — this PR is written against a main that no longer exists. Its premise ("the wired LAN ports are members of the captive bridge") was true at your merge base 54e8c36; main is now 74cd6f99 and carries #625 (343f183, "the physical LAN port moves onto br-private"), which moves the same port list the same way onto br-private, and #623 (fa414c7), which narrowed the br-mgmt ADR and kept br-mgmt "Proposed" on its own merit. Merge probe of your head into current main: conflicts in .github/workflows/test.yml, .ngit/act/workflows/test.yml, and 99-tollgate-setup itself. On a merged tree, setup_lan_ports_private runs first on both paths and empties the captive port list, so setup_mgmt_bridge takes its ERROR branch every run — dead code; and any interleaving where both write paths run has each writer's "clear the source" aimed at the other's target, with the operator's cable as the hostage. #625's commit message says "br-mgmt lands after it" — this needs to land after it, re-based, and state the relationship: supersede #625 (say why mgmt-class beats owner-class for the cable), narrow to what survives it (the scoped allow list, the mgmt zone, F1/F2 groundwork), or close in its favour.

[RISK] 99-tollgate-setup:2152-2155 — uci add_list network.mgmt_bridge.ports="$p" ignores the exit status, then uci -q delete "$sec.ports" runs unconditionally with no read-back. A failed or silently no-op'd add_list clears the port off br-lan without landing it on br-mgmt: the wired port sits on NO bridge and the operator's only cable access is dead — the exact worst case your own comment block names, reached from the write path. #625 hit this and fixed it in its second commit: re-read the target list after the add loop, require every port present matched on whole words, ERROR + return 1 with the source untouched otherwise. Its suite carries negative controls for a failing AND a silently no-op'd add_list; your RED controls cover only the port-clear, the reinstall call, and forward REJECT. Adopt the verify-before-clear and the no-op control.

[NIT] 99-tollgate-setup:2103 — private_ip is assigned inside setup_mgmt_bridge but missing from its local lines (2059-2060). No current impact (nothing after the call reads it; the writer does not clobber setup_private_network's own private_ip, which is local at :1891), but the F1/F2 follow-up will touch exactly this function.

[NIT] docs/rc-tester-guide.md:475 — the guide says ping 9.9.9.9 fails but not that the fragment drops ICMP outright (33-mgmt-bridge-scope.nft:51), so a cabled tester pinging the router's own mgmt address to check the cable will conclude the bridge is dead. One line: ping is not the liveness check here; the admin surface or :22 is.

[INFO] CI evidence at this head: ngit has no runs for fa1ab55/29ae09e/68a0e6f/1c22bfac (only go-test on the intermediate merges 96990a1/82ee5df), and GH reports no checks for the fork branch. The offline evidence is what I re-ran myself at your head: uci-defaults-mgmt-bridge 57/0 and packaging/mgmt-bridge-scope 37/0 (nft -c wrapper + negative control, .ipk build, both RED controls biting), and the surrounding offline tier green — 17/18 in my sandbox, the one red (ngit-ci-trigger) failing identically on origin/main, i.e. environmental. Bench tier remains unmeasured as you state; after #625 the bench rows must be re-derived for whichever design survives.

[INFO] 99-tollgate-setup:2091-2093 keeps an existing network.mgmt.ipaddr unconditionally while setup_private_network recomputes from network.lan.ipaddr every FULL pass; a LAN re-homed into the mgmt /24 leaves br-mgmt overlapping with no warning. Contrived, recoverable, preserve-by-default is right — but F1/F2 should validate a preserved address against the live LAN. And :2070's device-section scan bound of 24 should inherit #625's 64 (+ its beyond-index-23 test) if this survives.

What it gets right, kept short: the no-transact design call is correct and well-argued (one nodogsplash gates one bridge; a bridge you cannot gate must not take money you cannot deliver); the scope is pinned in both directions and the tests are real, not transcripts; the no-way-out half is structural (forward REJECT, no forwarding, no private_zone reuse; 30-backend-firewall.nft's :2121 drop covers br-mgmt too); both recipes install the fragment; go.sum is byte-identical to main at the final merge; gitleaks clean on all 10 files.

secret-scan: clean (gitleaks)
FULL_REVIEW: /home/c03rad0r/reports/reviews/tollgate-module-basic-go-t_810e2cfe-glm-5.3.md
#601

[felixfelix-bot]

CHANGES-REQUESTED at head fa1ab55.

1 BLOCK: main has moved past this PR's merge base — #625 (ports onto br-private) is landed and collides head-on with setup_mgmt_bridge in the same file (merge probe conflicts in 3 files; the two writers fight over one port list).
1 RISK: the writer clears the captive port list without verifying the add_list landed (99-tollgate-setup:2152-2155) — the dead-cable case, fixed in #625's verify-before-clear and untested here.
2 NIT (private_ip not local; guide omits that ICMP to the router is dropped), 2 INFO (no ngit/GH CI at head; preserved mgmt address never re-validated vs live LAN).

Full findings, evidence and re-run test counts: see the review comment thread.

[Amperstrand]

Scope reframed after #625 (and labelled post-0.6.0). #625 shipped the minimal release path for this PR's core ask — the physical LAN ports now move onto br-private, making a cabled client an owner-class client — and #623 narrowed the ADR's verdict to line-rate NAT. What remains uniquely valuable in this branch is therefore the fuller scope: the dedicated management bridge (br-mgmt) on its own merit per the ADR, the ADR bench rows 9–17 gate that #610's review kept attached to this line of work, and the D1 idiom / bench-informed input REJECT touch-ups.

Plan: this stays open, labelled release:post-0.6.0; when it revives it should rebase onto the post-#625 world (expect real conflicts in 99-tollgate-setup and the guard rules — the two-bridge analysis is now scoped against a tree that already carries one new bridge) and re-derive the bench rows against then-current main. The ADR itself is merged and current; only the implementation work remains.

[Amperstrand]

Superseded by #625 (merged: the physical LAN ports moved onto br-private via the minimal release path). Suggest closing — the wider bridge-of-its-own design discussion can re-open post-0.6 if still wanted.

[c03rad0r]

Dragons be here, this is whats currently holding up our release, lets think it through more thoroughly after the release.

[Amperstrand]

Agreed on the dragons — and to keep the release picture precise: nothing in 0.6.0-rc1 depends on this PR. #625 already shipped the minimal path (wired LAN ports onto br-private, admin guards scoped to it); #601 is the bigger "a bridge of its own for the admin surfaces" design. It is not what the tag is waiting on — that is only the operator call (plus the #648 freeze question). Post-release I would fold this into a design issue that carries the br-mgmt ADR context and cross-links the Rust line, where the enforcement-surface architecture is being rebuilt anyway — deciding it there avoids paying the migration twice.

[Amperstrand]

Rebased in place onto current main (ec77401) — one commit (3 real commits squashed; the 3 merge commits and the 82ee5df go.sum repair dropped: it reverted evil-merge bloat that does not exist on a clean rebase; 9e904b8's link fix incorporated — the entry points at #601).

Semantic resolutions made during the rebase — maintainer, please confirm:

1. This PR replaces feat(packaging): the physical LAN port moves onto br-private (minimal release path, supersedes #607 for this release) #625's minimal path: setup_lan_ports_private (ports → br-private) is now setup_mgmt_bridge (ports → br-mgmt). The superseded test tests/uci-defaults-lan-private-wired_test.sh (658 lines, asserted ports-on-br-private) was deleted and its CI step swapped for this PR's two mgmt-bridge steps. br-private itself is untouched (still the private SSID's network).
2. One stale pin in tests/packaging/mgmt-bridge-scope_test.sh updated: it demanded :2121 restricted to {br-lan, lo}, but main has since deliberately exempted br-private (fix(firewall): answer :2121 on br-private, where the admin board lives #638 admin-board data path). The check now pins {br-lan, br-private, lo} exactly — br-mgmt remains excluded, so this PR's the-wired-bridge-cannot-transact invariant is unchanged.
3. CHANGELOG entry under [Unreleased].

Verification (offline suites): mgmt-bridge 57/0, mgmt-bridge-scope 37/0, backend-api-owner-network 18/0, device-code 73/0, gatewayname-banner 21/0, nodogsplash-convergence 29/0, run-order 6/0; bash -n clean. No Go files touched, so the go battery is not applicable.

@felixfelix-bot this one is done — skip it (see coordination note on #676).

[Amperstrand]

Acknowledged per @c03rad0r's call: this is deliberately parked until after the release (and felix's latest cold review is right that its premise has aged — the writer it replaces has moved under it again). When it comes back it needs a fresh pass against then-current main, not a merge of this head.

[Amperstrand]

Skipped per PR-REVIEW.md Step 1 (draft). Long-lived packaging branch; needs a fresh rebase + the entry-port interplay re-check against #709/#745 before any full review.

[felixfelix-bot]

Re-review — PR #601 (ours, draft, conflicting)

fix(packaging): the wired LAN ports get a bridge of their own, scoped to the admin surfaces
head 6b3d8dae25 · base main 22f111ab · 1 commit, 38 behind · +1346/-844
artifact: ~/reports/reviews/tollgate-module-basic-go-601-rereview.md

Reviewer: tier/manager-min (zai) — single-family advisory review.
The cross-family lane was unavailable at review time: the flat router returned
503 all providers exhausted (dispatch_gate: can_dispatch=false — all lanes priced out (+inf)),
which killed the 3 delegated review lanes. This verdict is therefore not the D-115
cross-family review; treat it as advisory and re-run a different family before any merge.
CI: this repo has zero checks configured — ci=no_checks_configured. Nothing here is a CI verdict.
Scope: gh pr diff <N> (head vs base). NOT the compare/<old>...<new> endpoint, which reports
97/50 commits of base churn for these branches and is not the PR's delta.

What is new since our last review — the lineage changed, so there is no incremental delta

Our previous review (felixfelix-bot, COMMENTED) is anchored at fa1ab55db4, 2026-10-01.
That SHA is not in the current branch history: the branch was rebased onto the
667d0e47 era and now carries exactly one commit, 6b3d8da, with a changed title
("…, scoped to the admin surfaces"). So this is not "a few new commits on top of what we
reviewed" — it is a rebuilt revision of the same work and needs a fresh verdict end to end.

Verdict: COMMENT — design is sound and unusually well-guarded; two conditions before merge

Verified from the diff (not from the title)

• packaging/files/etc/nftables.d/33-mgmt-bridge-scope.nft (new, 86 lines) is an allow list with a
  catch-all drop: udp 53/67/68, tcp 53, tcp 22, tcp {443,8080,8090,8443} accept, then
  meta nfproto ipv4 iifname "br-mgmt" counter drop; IPv6 dropped outright.
• The payment surfaces :2050/:2051/:2121 are deliberately absent. Rationale in the file: one
  nodogsplash gates exactly one bridge, so br-mgmt has no captive gate; a client there can be quoted
  and can pay, and the grant then dies in ndsctl as Client <mac> not found. — money taken, nothing
  delivered. Excluding them is what makes "this bridge cannot transact" structural rather than incidental.
• 99-tollgate-setup (+181/-149) creates a separate firewall.mgmt_zone — forward 'REJECT',
  no config forwarding — explicitly not reusing firewall.private_zone (whose forward ACCEPT
  plus private → wan would hand the cable free internet). dhcp.mgmt is created
  (start 100, limit 150, leasetime 12h, ra/dhcpv6 disabled) so the laptop gets a lease.
• The port move is precondition-checked before the first uci set: a lan_dev with no ports and no
  existing br-mgmt ports logs ERROR: skipping … nothing was changed. and returns without writing.
  Fail-safe, not fail-open.
• Both directions are pinned by tests: tests/packaging/mgmt-bridge-scope_test.sh (700, includes a
  negative case that re-runs the checks against a fragment with the catch-all drop removed) and
  tests/uci-defaults-mgmt-bridge_test.sh (1704). A guest guard mentioning br-mgmt fails a test.
• Happy-path (b) is documented, which was my main concern: docs/rc-tester-guide.md now states that
  from a cable the wired ports do not share the captive bridge's address, that
  http://<router>.lan/ is not the URL to use there, and that the address is found with
  uci get network.mgmt.ipaddr. Without that line I would have filed discoverability as a blocker.

Conditions before merge

1. Router happy-path run. This edits the install path (99-tollgate-setup) and where the
   administration surfaces answer. The in-repo tests are shell/unit-level; the guest-side drop is
   described as "measured on the bench". Run install → panel/password → cabled board login at
   :8090 → guest SSID → portal, on a real router, and attach the transcript.
2. Merge-forward, not rebase. It conflicts on CHANGELOG.md, docs/rc-tester-guide.md and
   tests/uci-defaults-lan-private-wired_test.sh. Note the second one conflicts in the very section
   this PR adds — keep both sides deliberately rather than accepting either wholesale. All three are
   cheap text conflicts; nothing structural (src//packaging logic) conflicts.
3. Un-draft is the operator's call, and so is the release bucket (below).

Tag recommendation

tag	verdict	evidence
installer relevant	YES	packaging/Makefile, packaging/local-build-ipk.sh, and 99-tollgate-setup (+181/-149) are in the diff
security	YES	management-plane isolation: br-mgmt gets forward REJECT + no WAN forwarding, and the guard "refuses an allow list that re-opens a payment" path; the previously shared L2 domain between the operator's cable and the open guest SSID is split
unlikely to break the happy path	NO	it is the install path and it relocates the admin surfaces a cabled operator uses — happy-path items (a) and (b)
before upcoming release	needs your decision	it is currently labelled release:post-0.6.0, but it changes install topology and admin reachability
after upcoming release	currently labelled, but re-check	see below

FLAG — the release bucket for this one is probably wrong

It carries release:post-0.6.0 + needs:human-review, yet it edits the installer and moves the admin
address. That is exactly happy-path (a) and (b). Either this fixes a tester-facing blocker — in which
case it must land before v0.6.0 (and then it is before upcoming release + installer relevant) —
or it does not, in which case post-0.6.0 stands but a router happy-path run is still required before it
lands at all. I would not leave this defaulted.

[c03rad0r]

🏷️ Next action — on-hold

On hold — draft PR. Not in the active merge queue.

[Amperstrand]

Closing in favor of #856 — identical change rebased onto current main. This PR's head branch on the fork was deleted, which froze it at the old commit; the recreated branch cannot re-attach to this PR (GitHub limitation). Continuing in #856.