From feda00cec6716e0dc3feda2cc39dd77ee28fe81d Mon Sep 17 00:00:00 2001 From: Andrew SG Date: Thu, 13 Aug 2026 10:20:03 +0100 Subject: [PATCH 1/2] docs: add v1.0 milestone scope to CLAUDE.md Defines v1.0 as safe-for-closed-LAN-use rather than fully hardened, scoping in the remaining diagnosed correctness bugs and the severity-regardless-of-network security items, and explicitly deferring the larger structural refactors and transport-layer hardening to post-1.0. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 153fbbc2..fea575bf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -153,6 +153,25 @@ Phased plan derived from a full codebase review. Detailed items live in the TODO **Not scale-dependent, do sooner than phase 4**: the 2026-07-27 security review (see the Security section in TODO) found items that are risks today regardless of lab size — two hardcoded fleet-wide Samba passwords already committed to the repo, and an unauthenticated-ZMQ path to full RCE-as-root via `update_saviour` that doesn't require ever touching the web UI's login. The web UI *does* already have a real (if limited) auth system, contrary to this roadmap's original "auth on... web UI" framing — what it actually needs is TLS, rate-limiting, and closing the unauthenticated endpoints, not auth from scratch. +## v1.0 Milestone (proposed 2026-08-13, currently v0.6) + +Scope for calling this v1.0: **safe to run unattended on a closed lab LAN without silent data loss or easy compromise** — not "fully hardened for internet exposure." Deliberately excludes the big structural refactors (they're real debt, but don't block correctness/safety): `web.py` blueprint split, the `examples/`→`variants/` manifest rename, Samba→rsync, and the module god-object composition refactor all stay post-1.0. + +- [ ] **Correctness gate** — the small, already-diagnosed bugs still open from the 2026-07-31/08-03 reviews: + - [ ] `modules.py` heartbeat-timeout offline path bypasses `on_status_change()` (mid-recording dropout alert doesn't fire for this path) + - [ ] `controller/config.py` `set_all()`/`_recursive_update()` has no lock (racing reads see half-merged config) + - [ ] `controller/config.py` `set_all()` raises `KeyError` on a genuinely new config key (crashes controller-settings save) + - [ ] `health.py` `get_ptp_sync()` treats a perfect `0` offset as missing data + - [ ] `health.py` `get_health_summary()` crashes on any online module with a `None` metric + - [ ] `recording.py` `create_session()` busy-module overlap check is unlocked (double-submit/scheduler race can double-start a session) +- [ ] **Security floor** — the items that are real regardless of LAN-vs-WAN exposure, per the 2026-07-27 review: + - [ ] `update_saviour` / `deploy_update`: add package signature or checksum verification before rsync-over-install (currently only checks `zipfile.is_zipfile()`) + - [ ] Session recording downloads (`/api/sessions//download...`) have no auth check at all — add `_require_auth`, matching every other handler + - [ ] Gate the other unauthenticated data-leak endpoints: `/facade/list_modules`, `/facade/module_health`, `/facade/exported_recordings`, `/update/package`, `get_bug_report` (and fix `bug_report_ready` broadcasting to every socket instead of `room=request.sid`) + - [ ] ZMQ identity-hijack: either disable `ROUTER_HANDOVER` or require a shared pre-shared secret on `"hello"` so a spoofed `module_id` can't silently steal another module's command stream +- [ ] **Explicitly deferred, documented as known limitation for v1.0**: ZMQ CURVE auth/encryption, web UI TLS, Samba SMB3 sealing, fleet SSH-key/password rotation on clone — the roadmap already frames these as "closed-LAN only" caveats; v1.0 ships with that caveat stated plainly rather than blocked on the multi-day transport-layer work. +- [ ] **Process**: wire the existing `python-app.yml` CI job up as a *required* status check on `main`/`staging` before tagging v1.0 (see GitHub repo settings section below) — a tagged release should not be possible to cut from a red build. + ## TODO Known issues and planned improvements, grouped by priority. Check these off (`- [x]`) as they are completed. From ce19878f936569346b9d8cdf9934c9bf99c14cfb Mon Sep 17 00:00:00 2001 From: Andrew SG Date: Thu, 13 Aug 2026 10:37:36 +0100 Subject: [PATCH 2/2] fix(saviour-config): stop progress() from blanking the SSH terminal progress() previously drew a whiptail --infobox per step throughout run_configuration()/run_clone_fix(). On terminals with smcup/rmcup support (virtually every SSH client), each infobox flashes onto the alternate screen and back off the instant it exits, since infoboxes don't wait for input. Between calls -- especially during multi-minute silent steps like npm install/apt-get install -- the primary screen just shows whatever was there before the flow started (blank, since configure_device() clears it beforehand), which reads as the whole TUI having vanished for the length of the run, over SSH specifically. It also meant apply_from_config() (the boot-time, fully headless path via saviour-provision.service) called whiptail with no controlling terminal at all, which fails immediately and, under run_configuration's set -e, would abort the entire boot-time provisioning run on the very first progress() call. progress() now just prints plain status lines -- unconditionally safe whether or not there's a tty, and nothing to flash since it never touches terminal modes. Also fixes a latent bug found while touching this: run_configuration() never called `set +e` at the end (unlike run_clone_fix(), which already does), so set -e -- a global shell option, not function-scoped -- stayed enabled for the rest of the script after a successful run. Co-Authored-By: Claude Sonnet 5 --- saviour-config | 44 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/saviour-config b/saviour-config index 531e5f91..8c3780e1 100755 --- a/saviour-config +++ b/saviour-config @@ -50,8 +50,32 @@ OCTET_A=""; OCTET_B=""; OCTET_C="" # 3>&1 1>&2 2>&3 swaps fd1 and fd2 so $() captures the selection. wt() { whiptail "$@" 3>&1 1>&2 2>&3; } +# ── Progress reporting ─────────────────────────────────────────────────────── +# Plain status lines, not a whiptail dialog. This used to be +# `whiptail --title "Configuring…" --infobox "\n $1" 7 $W` -- an +# eye-catching box, but each call is its own short-lived newt session: on +# terminals that support smcup/rmcup (virtually every SSH client -- PuTTY, +# xterm-256color, etc.) newt switches to the alternate screen to draw the +# box and switches straight back to the primary screen the instant it +# exits, which for a fire-and-forget infobox is near instantaneous. Between +# calls -- especially during multi-minute steps like `npm install`/ +# `apt-get install`, which print nothing to the terminal at all (see +# run_logged) -- the primary screen shows whatever was there before the +# flow started (blank, since configure_device() clears it right before this +# phase begins), which looks like the whole TUI has vanished until the next +# call flashes it back into view. Over SSH specifically this reads as the +# session going completely black for the whole run, with no way to tell +# progress from a hang. +# +# Plain echo has none of that: nothing here touches terminal modes, so nothing +# to flash or restore. It's also unconditionally safe with no controlling +# terminal at all -- e.g. saviour-provision.service invoking +# `saviour-config --apply` at boot, fully headless -- where whiptail would +# fail outright with no tty and, under run_configuration()'s `set -e`, abort +# the entire boot-time provisioning run on its very first progress() call. progress() { - whiptail --title "Configuring…" --infobox "\n $1" 7 $W + echo + echo "==> $1" } log() { @@ -584,10 +608,10 @@ configure_device() { return fi - # run_configuration()'s progress() infoboxes don't block or get - # dismissed -- the last one shown just stays on screen (whiptail/newt - # never clears between dialogs) until something bigger happens to paint - # over the same region. Explicit clear so it can't linger. + # run_configuration() prints plain progress lines (see progress() above) + # rather than whiptail dialogs, so there's a scrollback's worth of that + # sitting above wherever the cursor is now -- explicit clear so the + # credentials/reboot dialogs below get a clean screen. clear # Step 6b: surface first-time login credentials while the operator is @@ -1471,6 +1495,12 @@ run_configuration() { systemctl restart saviour.service >> "$LOG" 2>&1 || true log "Configuration complete" + + # Matches run_clone_fix()'s own set -e/set +e bracketing above -- set -e + # is a global shell option, not function-scoped, so without this a + # successful run left errexit switched on for the rest of the script + # (the main_menu loop, any later reconfiguration, etc.). + set +e } # ── Non-interactive apply (boot-time provisioning) ───────────────────────────── @@ -1540,6 +1570,6 @@ fi main_menu # whiptail/newt dialogs paint their own box without a full-screen clear, and # never clear on the way out either -- whatever was last drawn (the main -# menu, or a lingering progress/infobox from a prior step) would otherwise -# still be sitting on screen once control returns to the shell prompt. +# menu) would otherwise still be sitting on screen once control returns to +# the shell prompt. clear