From 3844df7c23b879b726aa14117b7072833020dd94 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 29 Jun 2026 22:45:41 +0200 Subject: [PATCH] =?UTF-8?q?controllerswap:=20F1=20verify=20hardening=20?= =?UTF-8?q?=E2=80=94=20reject=20crash-looping=20no-healthcheck=20image=20v?= =?UTF-8?q?0.47.0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit controllerHealthy reads RestartCount (running&&rc>0 -> not ok) + signals needsDwell for no-healthcheck; verify requires verifyDwell(=3) consecutive ok polls for a no-healthcheck image (real healthcheck trusted immediately). Closes the F1 hole (alpine crash-loop passed the point-in-time check). Red-proof + dwell + real-image tests. No sudoers/orchestration change. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Pg8ANF97SEeKYSN5Jxw3qJ --- CHANGELOG.md | 20 +++++ cmd/felhom-agent/main.go | 2 +- internal/localapi/controllerswap.go | 65 +++++++++++----- internal/localapi/controllerswap_test.go | 94 +++++++++++++++++++++++- internal/localapi/disks_test.go | 32 +++++--- internal/localapi/server.go | 36 ++++----- internal/localapi/server_test.go | 12 ++- internal/localapi/wipe_reresolve_test.go | 14 ++-- 8 files changed, 218 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a25994..eef77ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,23 @@ +## v0.47.0 — controller-swap verify hardening: reject a crash-looping no-healthcheck image (F1) (2026-06-29) + +Closes F1 from the no-mercy testrun: a controller image with **no HEALTHCHECK that crash-loops** could +land a single "Running" inspect poll → the swap marked it healthy → **no rollback** (alpine tagged as +the controller passed in ~4 s, then `Restarting (0)`). The real controller image has a healthcheck so +the live severity is low, but the rollback safety net had a hole. + +- **`internal/localapi/controllerswap.go`:** `controllerHealthy` now also reads `{{.RestartCount}}` (a + 4th `docker inspect -f` field) — `running && RestartCount>0` → not-ok (a process that has already + crash-restarted isn't stably up, regardless of healthcheck). It also signals `needsDwell` for the + no-healthcheck (`none`) case. `verify` adds a **stability dwell**: a no-healthcheck image must report + ok on `verifyDwell` (=3) **consecutive** polls before it's accepted; a real `healthy` result is + trusted immediately (Docker already gated it). Any not-ok resets the dwell. Timeout → existing + rollback path runs. No change to writeImage, the sudoers grants (the `*` in `docker inspect -f *` + spans the extended template — confirmed live), or the state-file/rollback orchestration. +- Tests: F1 **red-proof** (`RestartCount>0` → verify false; companion: rc=0+dwell=1 verifies → the rc + check is what blocks it); the **dwell** (single ok then crash → verify false; companion dwell=1 + accepts it); a real `healthy` image verifies promptly (no false rollback). Existing + `RollbackOnUnhealthy` / `HealthyWithNoHealthcheck` stay green. Version `0.46.0 → 0.47.0`. + ## v0.46.0 — leaf lifecycle: signal + loud-log a regenerated leaf (prevention, Part B.1) (2026-06-29) Makes an accidental local-API leaf **regeneration** (the 2026-06-28 root→non-root migration class — diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index c752fb8..fb02b9d 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -45,7 +45,7 @@ import ( // version is the agent version. Overridable at build time with // -ldflags "-X main.version="; defaults to the in-repo CHANGELOG version. -var version = "0.46.0" +var version = "0.47.0" // runGuestHook is the PVE pre-start hook body (`felhom-agent guest-hook `). On the // pre-start phase it creates placeholder dirs for any absent bind-mount source so the guest always boots diff --git a/internal/localapi/controllerswap.go b/internal/localapi/controllerswap.go index d764c60..b801b79 100644 --- a/internal/localapi/controllerswap.go +++ b/internal/localapi/controllerswap.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "regexp" + "strconv" "strings" "time" ) @@ -65,6 +66,8 @@ type ControllerSwapper struct { logger *slog.Logger verifyTimeout time.Duration verifyInterval time.Duration + verifyDwell int // F1: consecutive ok polls required to accept a NO-healthcheck image (a real + // healthcheck passes without dwell). Catches a slow crash-loop that hasn't bumped RestartCount yet. } // NewControllerSwapper builds a swapper. stateDir holds controller-swap-.json (default @@ -82,6 +85,7 @@ func NewControllerSwapper(exec GuestExecutor, stateDir string, logger *slog.Logg logger: logger, verifyTimeout: 90 * time.Second, verifyInterval: 3 * time.Second, + verifyDwell: 3, } } @@ -152,40 +156,67 @@ func (c *ControllerSwapper) restartBootstrap(ctx context.Context, vmid int) erro return err } -// controllerHealthy reports running + (healthy or no healthcheck) + the running image == want. -// Returns (ok, stillStarting): stillStarting=true means keep polling. -func (c *ControllerSwapper) controllerHealthy(ctx context.Context, vmid int, want string) (ok, starting bool) { +// controllerHealthy is a pure point-in-time predicate: running + RestartCount==0 + (healthy OR no +// healthcheck) + the running image == want. Returns (ok, stillStarting, needsDwell): +// - stillStarting=true → keep polling (container absent, not-yet-running, wrong image, unhealthy, or +// already-restarted). +// - needsDwell=true → ok BUT the image has NO healthcheck, so the caller must confirm it stays ok for +// verifyDwell consecutive polls before trusting it (F1: a no-healthcheck crash-loop can flicker +// Running for one instant). A real `healthy` result is trusted immediately (Docker already gated it). +// +// RestartCount>0 means the process has already crashed+restarted → not stably up, regardless of +// healthcheck presence (the F1 hole: alpine flickered Running between restarts and passed). +func (c *ControllerSwapper) controllerHealthy(ctx context.Context, vmid int, want string) (ok, starting, needsDwell bool) { out, err := c.exec.GuestExec(ctx, vmid, "docker", "inspect", "-f", - "{{.State.Running}}|{{if .State.Health}}{{.State.Health.Status}}{{else}}none{{end}}|{{.Config.Image}}", controllerContainer) + "{{.State.Running}}|{{if .State.Health}}{{.State.Health.Status}}{{else}}none{{end}}|{{.Config.Image}}|{{.RestartCount}}", controllerContainer) if err != nil { - return false, true // container not there yet (rm -f window) → keep polling + return false, true, false // container not there yet (rm -f window) → keep polling } - f := strings.SplitN(strings.TrimSpace(out), "|", 3) - if len(f) != 3 { - return false, true + f := strings.SplitN(strings.TrimSpace(out), "|", 4) + if len(f) != 4 { + return false, true, false } - running, health, image := f[0] == "true", f[1], f[2] + running, health, image, restartStr := f[0] == "true", f[1], f[2], f[3] if !running { - return false, true + return false, true, false } if image != want { - return false, true // bootstrap may not have re-run yet + return false, true, false // bootstrap may not have re-run yet + } + if rc, err := strconv.Atoi(strings.TrimSpace(restartStr)); err == nil && rc > 0 { + return false, true, false // already crash-restarted → not stably up } switch health { - case "healthy", "none": - return true, false + case "healthy": + return true, false, false // Docker gated it → trust immediately + case "none": + return true, false, true // no healthcheck → ok, but require the dwell case "starting": - return false, true + return false, true, false default: // unhealthy - return false, true + return false, true, false } } func (c *ControllerSwapper) verify(ctx context.Context, vmid int, want string) bool { deadline := time.Now().Add(c.verifyTimeout) + consecutiveOK := 0 + dwell := c.verifyDwell + if dwell < 1 { + dwell = 1 + } for { - if ok, _ := c.controllerHealthy(ctx, vmid, want); ok { - return true + ok, _, needsDwell := c.controllerHealthy(ctx, vmid, want) + switch { + case ok && !needsDwell: + return true // real healthcheck passed → done + case ok: // no-healthcheck image: require verifyDwell consecutive ok polls + consecutiveOK++ + if consecutiveOK >= dwell { + return true + } + default: + consecutiveOK = 0 // any not-ok resets the dwell (a crash between polls) } if time.Now().After(deadline) || ctx.Err() != nil { return false diff --git a/internal/localapi/controllerswap_test.go b/internal/localapi/controllerswap_test.go index 144d6d0..284833f 100644 --- a/internal/localapi/controllerswap_test.go +++ b/internal/localapi/controllerswap_test.go @@ -26,6 +26,11 @@ type fakeGuestExec struct { teeStdin []string // raw bytes piped into each `tee` write (the swap's write vector) failRestart bool noHealthBlock bool // if set, .State.Health is absent ("none") + restartCount int // F1: .RestartCount reported by docker inspect (a crash-looper has >0) + // runningSeq, when non-nil, drives .State.Running per docker-inspect poll (last value repeats) — + // e.g. {true, false} models a container that flickers Running then crashes. nil → always running. + runningSeq []bool + inspectN int } func (f *fakeGuestExec) GuestExec(_ context.Context, _ int, args ...string) (string, error) { @@ -50,13 +55,22 @@ func (f *fakeGuestExec) GuestExec(_ context.Context, _ int, args ...string) (str if f.containerImg == "" { return "", fmt.Errorf("no such container") } + running := true + if f.runningSeq != nil { + i := f.inspectN + if i >= len(f.runningSeq) { + i = len(f.runningSeq) - 1 + } + running = f.runningSeq[i] + f.inspectN++ + } health := "healthy" if f.noHealthBlock { health = "none" } else if !f.good[f.containerImg] { health = "unhealthy" } - return fmt.Sprintf("true|%s|%s", health, f.containerImg), nil + return fmt.Sprintf("%t|%s|%s|%d", running, health, f.containerImg, f.restartCount), nil } return "", fmt.Errorf("fake: unexpected exec %v", args) } @@ -267,3 +281,81 @@ func TestControllerSwapHandler_SingleFlight(t *testing.T) { t.Fatalf("status = %d, want 409 (swap already in progress)", rr.Code) } } + +// inspectScript is a minimal GuestExecutor for verify()-level F1 tests: it returns scripted +// `docker inspect` outputs (one per poll; the last repeats) and no-ops everything else. +type inspectScript struct { + outs []string + i int +} + +func (s *inspectScript) GuestExec(_ context.Context, _ int, args ...string) (string, error) { + if len(args) >= 2 && args[0] == "docker" && args[1] == "inspect" { + j := s.i + if j >= len(s.outs) { + j = len(s.outs) - 1 + } + s.i++ + return s.outs[j], nil + } + return "", nil +} +func (s *inspectScript) GuestExecStdin(_ context.Context, _ int, _ io.Reader, _ ...string) (string, error) { + return "", nil +} + +func fastSwapper(exec GuestExecutor) *ControllerSwapper { + s := NewControllerSwapper(exec, "", discardLogger()) + s.verifyTimeout = 100 * time.Millisecond + s.verifyInterval = 4 * time.Millisecond + return s +} + +// F1 RED-PROOF: a no-healthcheck image with RestartCount>0 (a crash-looper) must NOT verify ok → +// verify() returns false → the swap's existing rollback path runs. Companion: the same image with +// RestartCount=0 and dwell=1 DOES verify — proving the RestartCount check is what blocks the crasher +// (the old `running + none → ok` shape had no such guard, the F1 hole found live). +func TestF1_Verify_CrashLoopRestartCountBlocks(t *testing.T) { + s := fastSwapper(&inspectScript{outs: []string{"true|none|" + newImg + "|2"}}) + if s.verify(context.Background(), 9201, newImg) { + t.Fatal("RestartCount>0 (crash-looping no-healthcheck) must NOT verify ok") + } + // controllerHealthy is a pure predicate: rc>0 → (false, starting). + ok, starting, _ := s.controllerHealthy(context.Background(), 9201, newImg) + if ok || !starting { + t.Fatalf("controllerHealthy(rc=2) = (ok=%v,starting=%v), want (false,true)", ok, starting) + } + // Companion: rc=0 + dwell=1 → verifies (shows the rc check, not something else, blocks the crasher). + s2 := fastSwapper(&inspectScript{outs: []string{"true|none|" + newImg + "|0"}}) + s2.verifyDwell = 1 + if !s2.verify(context.Background(), 9201, newImg) { + t.Fatal("companion: no-healthcheck rc=0 dwell=1 should verify") + } +} + +// F1 dwell: a single ok poll then a crash (Running=false) must NOT verify with the dwell. Companion: +// dwell=1 accepts the single ok (the false-positive the dwell removes). +func TestF1_Verify_DwellSingleOkThenCrash(t *testing.T) { + seq := []string{"true|none|" + newImg + "|0", "false|none|" + newImg + "|0"} + s := fastSwapper(&inspectScript{outs: seq}) // verifyDwell=3 (default) + if s.verify(context.Background(), 9201, newImg) { + t.Fatal("a single ok then crash must NOT verify with the dwell") + } + s2 := fastSwapper(&inspectScript{outs: seq}) + s2.verifyDwell = 1 + if !s2.verify(context.Background(), 9201, newImg) { + t.Fatal("companion: dwell=1 accepts the single ok") + } +} + +// A real healthy image verifies promptly (no dwell) — the real controller path is unaffected. +func TestF1_Verify_RealHealthcheckPassesPromptly(t *testing.T) { + s := fastSwapper(&inspectScript{outs: []string{"true|healthy|" + newImg + "|0"}}) + if !s.verify(context.Background(), 9201, newImg) { + t.Fatal("a real healthy image must verify promptly") + } + ok, _, needsDwell := s.controllerHealthy(context.Background(), 9201, newImg) + if !ok || needsDwell { + t.Fatalf("controllerHealthy(healthy) = (ok=%v,needsDwell=%v), want (true,false)", ok, needsDwell) + } +} diff --git a/internal/localapi/disks_test.go b/internal/localapi/disks_test.go index d36a715..1fb2d37 100644 --- a/internal/localapi/disks_test.go +++ b/internal/localapi/disks_test.go @@ -51,7 +51,11 @@ func (f *fakeDiskOps) Unmount(_ context.Context, where string) error { f.mu.Unlock() return nil } -func (f *fakeDiskOps) formatted() []string { f.mu.Lock(); defer f.mu.Unlock(); return append([]string(nil), f.formatCalls...) } +func (f *fakeDiskOps) formatted() []string { + f.mu.Lock() + defer f.mu.Unlock() + return append([]string(nil), f.formatCalls...) +} type fakeGate struct { mu sync.Mutex @@ -80,11 +84,11 @@ func (f fakeGuestList) ListLXC(context.Context) ([]proxmox.Guest, error) { retur // a different whole-disk (e.g. /dev/sdb1) classifies as user-data without touching the real host. type fakeHostReader struct{ mounts []storage.Mount } -func (f fakeHostReader) Mounts() ([]storage.Mount, error) { return f.mounts, nil } -func (f fakeHostReader) ResolveUUID(string) (string, bool) { return "", false } -func (f fakeHostReader) DeviceExists(string) bool { return true } -func (f fakeHostReader) Rotational(string) (bool, bool) { return false, false } -func (f fakeHostReader) Removable(string) (bool, bool) { return false, false } +func (f fakeHostReader) Mounts() ([]storage.Mount, error) { return f.mounts, nil } +func (f fakeHostReader) ResolveUUID(string) (string, bool) { return "", false } +func (f fakeHostReader) DeviceExists(string) bool { return true } +func (f fakeHostReader) Rotational(string) (bool, bool) { return false, false } +func (f fakeHostReader) Removable(string) (bool, bool) { return false, false } // sysOnSDA is the default system-disk fixture (root on /dev/sda) used by the disk-server test helpers. func sysOnSDA() fakeHostReader { @@ -345,7 +349,7 @@ func TestEject_RoleGated(t *testing.T) { sv := fakeStorage{targets: []hub.StorageTarget{ {Name: "bulk", Type: hub.StorageTypeUSB, BackingDevice: "/dev/sdb1", MountPath: "/mnt/bulk"}, // user-data {Name: "local", Type: "local", MountPath: "/var/lib/vz"}, // system (builtin dir) - {Name: "felhom-pbs", Type: hub.StorageTypePBS, MountPath: "/mnt/pbs"}, // backup (PBS) + {Name: "felhom-pbs", Type: hub.StorageTypePBS, MountPath: "/mnt/pbs"}, // backup (PBS) }} // system mount → refused, no Unmount. @@ -438,7 +442,7 @@ func (f *fakeGuestAttacher) AttachDrive(_ context.Context, _ int, where string) return StablePathForRaw(where), nil } func (f *fakeGuestAttacher) GuestSeesMount(_ context.Context, _ int, _ string) bool { return true } -func (f *fakeGuestAttacher) GuestBootID(_ context.Context, _ int) string { return "boot-1" } +func (f *fakeGuestAttacher) GuestBootID(_ context.Context, _ int) string { return "boot-1" } func (f *fakeGuestAttacher) attachDriveCount() int { f.mu.Lock() defer f.mu.Unlock() @@ -480,7 +484,11 @@ func (f *fakeGuestAttacher) RebootGuest(_ context.Context, vmid int) error { f.reboots = append(f.reboots, vmid) return nil } -func (f *fakeGuestAttacher) rebootCount() int { f.mu.Lock(); defer f.mu.Unlock(); return len(f.reboots) } +func (f *fakeGuestAttacher) rebootCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return len(f.reboots) +} func newAttachServer(t *testing.T, ga GuestAttacher, mounts map[int]map[string]string) http.Handler { t.Helper() @@ -646,8 +654,10 @@ func (f *fakeGuestsCfg) GuestConfig(_ context.Context, vmid int) (proxmox.GuestC } return proxmox.GuestConfig{Extra: extra}, nil } -func (f *fakeGuestsCfg) Snapshot(context.Context, int, string, string) (string, error) { return "", nil } -func (f *fakeGuestsCfg) Rollback(context.Context, int, string) (string, error) { return "", nil } +func (f *fakeGuestsCfg) Snapshot(context.Context, int, string, string) (string, error) { + return "", nil +} +func (f *fakeGuestsCfg) Rollback(context.Context, int, string) (string, error) { return "", nil } func (f *fakeGuestsCfg) WaitTask(context.Context, string, proxmox.WaitOptions) (proxmox.TaskStatus, error) { return proxmox.TaskStatus{ExitStatus: "OK"}, nil } diff --git a/internal/localapi/server.go b/internal/localapi/server.go index 8b835b0..4965e8b 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -207,24 +207,24 @@ func NewServer(o Options) (*Server, error) { cadence = defaultBackupCadence } s := &Server{ - addr: o.ListenAddr, - cert: o.Cert, - guests: o.Guests, - backups: o.Backups, - store: o.Store, - storage: o.Storage, - tokens: o.Tokens, - cadence: cadence, - logger: o.Logger, - now: func() time.Time { return time.Now().UTC() }, - disks: o.Disks, - diskGate: o.DiskGate, - guestList: o.Guests2, - guestAttach: o.GuestAttach, - intent: o.Intent, - guestBinds: o.GuestBinds, - formatJobs: o.FormatJobs, - host: o.HostReader, + addr: o.ListenAddr, + cert: o.Cert, + guests: o.Guests, + backups: o.Backups, + store: o.Store, + storage: o.Storage, + tokens: o.Tokens, + cadence: cadence, + logger: o.Logger, + now: func() time.Time { return time.Now().UTC() }, + disks: o.Disks, + diskGate: o.DiskGate, + guestList: o.Guests2, + guestAttach: o.GuestAttach, + intent: o.Intent, + guestBinds: o.GuestBinds, + formatJobs: o.FormatJobs, + host: o.HostReader, hostMetrics: o.HostMetrics, hostID: o.HostID, jobs: map[int]*backupJob{}, diff --git a/internal/localapi/server_test.go b/internal/localapi/server_test.go index be2fa65..2a77c28 100644 --- a/internal/localapi/server_test.go +++ b/internal/localapi/server_test.go @@ -89,7 +89,11 @@ func (f *fakeBackups) BackupWithSnapshotHook(_ context.Context, vmid int, onSnap } return hub.Backup{VMID: vmid, Success: true, Archive: "local:backup/vzdump-x", StartedAt: "2026-06-10T00:00:00Z"}, nil } -func (f *fakeBackups) called() []int { f.mu.Lock(); defer f.mu.Unlock(); return append([]int(nil), f.vmids...) } +func (f *fakeBackups) called() []int { + f.mu.Lock() + defer f.mu.Unlock() + return append([]int(nil), f.vmids...) +} type fakeStore struct { mu sync.Mutex @@ -97,7 +101,11 @@ type fakeStore struct { tests []hub.RestoreTest } -func (s *fakeStore) RecordBackup(b hub.Backup) { s.mu.Lock(); s.backups = append(s.backups, b); s.mu.Unlock() } +func (s *fakeStore) RecordBackup(b hub.Backup) { + s.mu.Lock() + s.backups = append(s.backups, b) + s.mu.Unlock() +} func (s *fakeStore) Backups(context.Context) []hub.Backup { s.mu.Lock() defer s.mu.Unlock() diff --git a/internal/localapi/wipe_reresolve_test.go b/internal/localapi/wipe_reresolve_test.go index 2003b40..d58a01c 100644 --- a/internal/localapi/wipe_reresolve_test.go +++ b/internal/localapi/wipe_reresolve_test.go @@ -22,13 +22,13 @@ func TestAntiRetargetResolve(t *testing.T) { okInspect := func(string) (storage.DeviceProbe, error) { return dataBearing, nil } cases := []struct { - name string - durable string - resolve func(string) (string, error) - derive func(string) (string, error) - inspect func(string) (storage.DeviceProbe, error) - wantDev string - wantErr string // substring; "" = expect success + name string + durable string + resolve func(string) (string, error) + derive func(string) (string, error) + inspect func(string) (storage.DeviceProbe, error) + wantDev string + wantErr string // substring; "" = expect success }{ { name: "happy-path-matches",