From d96e5bddd0ebc2a4430071df0f945d439601159e Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sat, 13 Jun 2026 19:26:27 +0200 Subject: [PATCH] fix(AGENT-001): anti-retarget re-resolution for inline customer-confirmed wipe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleDiskFormat's customer-confirmed branch formatted the mutable req.Device path; the durable id only bound the confirmation, never the mkfs target. A /dev reassignment between inspect and mkfs could wipe the wrong physical disk. Now mirrors signedjobs.WipeExecutor: resolve confirmed durable id -> current device, re-derive + require exact match, re-inspect (still data-bearing), then format THAT device. Any refusal -> 409, no mkfs. New antiRetargetResolve helper (injected deps, unit-tested: mismatch/gone/blank/empty all refuse). Injectable reresolveWipe seam on Server (defaults to real storage funcs). BRANCH ONLY — pending supervised review/deploy (see AGENT-001-FIX-NOTES.md). Co-Authored-By: Claude Opus 4.8 (1M context) --- AGENT-001-FIX-NOTES.md | 52 +++++++++++ internal/localapi/disks.go | 25 ++++-- internal/localapi/disks_test.go | 5 ++ internal/localapi/server.go | 12 ++- internal/localapi/wipe_reresolve.go | 71 +++++++++++++++ internal/localapi/wipe_reresolve_test.go | 108 +++++++++++++++++++++++ 6 files changed, 266 insertions(+), 7 deletions(-) create mode 100644 AGENT-001-FIX-NOTES.md create mode 100644 internal/localapi/wipe_reresolve.go create mode 100644 internal/localapi/wipe_reresolve_test.go diff --git a/AGENT-001-FIX-NOTES.md b/AGENT-001-FIX-NOTES.md new file mode 100644 index 0000000..285300a --- /dev/null +++ b/AGENT-001-FIX-NOTES.md @@ -0,0 +1,52 @@ +# AGENT-001 fix — inline customer-confirmed wipe: anti-retarget re-resolution + +**Branch:** `fix/agent-001-wipe-durable-reresolve` (off `main` @ d17b5ab) — **PENDING REVIEW, NOT deployed.** +**Status:** branch-only. No agent build, no deploy, no golden rebake, no re-provision. That is reserved for a supervised session (this fix touches the destructive storage-wipe path). + +## The bug + +`internal/localapi/disks.go` `handleDiskFormat`, the data-bearing **customer-confirmed** branch +(`dec.Allowed`), inspected and gate-bound the device by its durable id but then called +`s.disks.Format(ctx, req.Device, …)` on the **caller-supplied mutable `/dev` path**. The durable id +only bound the *confirmation*; it was never re-resolved to the device just before `mkfs`. If a USB +re-enumeration reassigned `/dev/sdbN` to a **different physical disk** between inspection and `mkfs` +(the classify→mkfs TOCTOU), the wipe hit the wrong drive — data loss on an unintended disk. + +The signed-jobs path (`internal/signedjobs/wipe.go` `WipeExecutor.Execute`) already does this +correctly: resolve durable→device, re-derive the device's durable id and require an exact match, +re-inspect, then format the *re-resolved* device. The inline path did not. + +## What changed + +- **New `internal/localapi/wipe_reresolve.go`:** + - `antiRetargetResolve(durableID, resolve, derive, inspect)` — pure, injected-deps helper mirroring + `WipeExecutor`: refuse empty durable id; resolve→current device; re-derive and require exact + match (a `/dev` node now pointing at a different disk derives a different id → refuse); re-inspect + and require still-`DataBearing()`. Returns the **re-resolved device** to format. + - `(*Server).reresolveDurableForWipe` wires it with the real `storage.ResolveDurableDevice` / + `storage.DeviceDurableID` / `s.disks.InspectDevice`. +- **`internal/localapi/disks.go`:** the `dec.Allowed` branch now calls `s.reresolveWipe(ctx, deviceDurable)` + and formats the **returned** device (never `req.Device`). On any refusal it returns `409 Conflict` + with the reason and does **not** call `mkfs`. The response/log now reference the re-resolved device. +- **`internal/localapi/server.go`:** new injectable field `reresolveWipe`, defaulted in `NewServer` to + `s.reresolveDurableForWipe`. Production behaviour unchanged (same real functions); the seam exists so + the inline path is unit-testable without real `/dev/disk/by-*`. +- **Tests:** `wipe_reresolve_test.go` covers `antiRetargetResolve` directly — happy path, empty durable, + no-longer-resolves, **durable-id mismatch (the core retarget case)**, no-longer-data-bearing, + re-inspect error. The existing integration test `TestFormat_DataBearing_UserDataConfirmed_Formats` + stubs `reresolveWipe` (the test helper sets it). Full `go build/vet/test ./...` green at the branch HEAD. + +## Why it can't break legitimate wipes + +`storage.DeviceDurableID` produces `byid:`/`byuuid:` ids and `storage.ResolveDurableDevice` accepts +exactly those — they round-trip. A normal user-data drive resolves back to itself, re-derives the same +id, and inspects data-bearing → format proceeds. Only three new refusals occur, all correct: +durable id gone, durable-id mismatch (retarget), or no-longer-data-bearing. + +## Review checklist for the supervised merge+deploy+golden-rebake session + +1. Confirm `antiRetargetResolve` refuses on mismatch/gone/blank/empty (run `go test ./internal/localapi/ -run AntiRetarget -v`). +2. Sanity-check `DeviceDurableID`↔`ResolveDurableDevice` scheme round-trip on the real demo USB (`felhom-usb`) so a genuine customer wipe still resolves+matches (read-only `DeviceDurableID` on the live device; do NOT wipe). +3. Confirm the `409 Conflict` refusal surfaces sensibly in the controller UI (controller `agentapi` maps the format response — verify it doesn't treat 409 as a hard error that hides the reason). +4. Bump agent version + CHANGELOG, build, golden-rebake, re-provision per the agent deploy runbook. +5. Live-validate one happy customer-confirmed wipe on a demo scratch drive (re-resolve matches → mkfs) and confirm the audit log line shows the re-resolved device == the confirmed durable id. diff --git a/internal/localapi/disks.go b/internal/localapi/disks.go index 0275cfb..0a63842 100644 --- a/internal/localapi/disks.go +++ b/internal/localapi/disks.go @@ -457,15 +457,30 @@ func (s *Server) handleDiskFormat(w http.ResponseWriter, r *http.Request, vmid i }) if dec.Allowed { - // USER-DATA, customer-confirmed (durable-id-bound). The gate already AUDITED it. Wipe. - if err := s.disks.Format(r.Context(), req.Device, req.FSType); err != nil { - s.logger.Error("local-api: customer-confirmed format", "vmid", vmid, "device", req.Device, "err", err) + // USER-DATA, customer-confirmed (durable-id-bound). The gate already AUDITED it. + // [AGENT-001] anti-retarget: re-resolve the confirmed durable id to the CURRENT + // device, require the re-derived id to match, and confirm it is still + // data-bearing — then format THAT device, never the mutable req.Device path. + // This closes the classify→mkfs TOCTOU (a /dev reassignment in the window could + // otherwise wipe a different physical disk). Mirrors signedjobs.WipeExecutor. + device, rerr := s.reresolveWipe(r.Context(), deviceDurable) + if rerr != nil { + s.logger.Warn("local-api: customer-confirmed wipe REFUSED at anti-retarget re-resolve", + "vmid", vmid, "req_device", req.Device, "durable_id", deviceDurable, "err", rerr) + writeStatus(w, http.StatusConflict, false, + FormatResponse{VMID: vmid, Device: req.Device, Formatted: false, DataBearing: true, + Role: string(role), DurableID: deviceDurable, Reason: probe.Reason()}, + "wipe refused (device may have changed since confirmation): "+rerr.Error()) + return + } + if err := s.disks.Format(r.Context(), device, req.FSType); err != nil { + s.logger.Error("local-api: customer-confirmed format", "vmid", vmid, "device", device, "err", err) writeErr(w, http.StatusBadGateway, "format failed: "+err.Error()) return } s.logger.Warn("local-api: USER-DATA data-bearing format — CUSTOMER CONFIRMED (no operator signature)", - "vmid", vmid, "device", req.Device, "durable_id", deviceDurable, "fstype", req.FSType, "why", probe.Reason()) - writeOK(w, FormatResponse{VMID: vmid, Device: req.Device, Formatted: true, DataBearing: true, + "vmid", vmid, "device", device, "durable_id", deviceDurable, "fstype", req.FSType, "why", probe.Reason()) + writeOK(w, FormatResponse{VMID: vmid, Device: device, Formatted: true, DataBearing: true, Role: string(role), DurableID: deviceDurable, Reason: "customer-confirmed wipe (" + probe.Reason() + ")"}) return } diff --git a/internal/localapi/disks_test.go b/internal/localapi/disks_test.go index dd3be38..9e942bb 100644 --- a/internal/localapi/disks_test.go +++ b/internal/localapi/disks_test.go @@ -113,6 +113,11 @@ func newDiskServer(t *testing.T, d *fakeDiskOps, g *fakeGate, sv StorageView, gl t.Fatalf("new server: %v", err) } srv.baseCtx = context.Background() + // [AGENT-001] The real anti-retarget re-resolution touches /dev/disk/by-*, + // which doesn't exist in unit tests. Stub it to a successful re-resolve of the + // device the format tests use; antiRetargetResolve itself is covered directly + // in wipe_reresolve_test.go. + srv.reresolveWipe = func(_ context.Context, _ string) (string, error) { return "/dev/sdb", nil } return srv.Handler() } diff --git a/internal/localapi/server.go b/internal/localapi/server.go index 1c4f23b..dea0ba5 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -148,6 +148,12 @@ type Server struct { hostMetrics HostMetricsProvider // slice 9 (optional) hostID string // slice 10B: for the data-bearing-format pending-op hint + // reresolveWipe performs the [AGENT-001] anti-retarget re-resolution before an + // inline customer-confirmed wipe (durable id → current device, re-derive+match, + // re-inspect). Defaults to s.reresolveDurableForWipe (real storage funcs); tests + // override it to avoid touching real /dev. + reresolveWipe func(ctx context.Context, durableID string) (string, error) + jobsMu sync.Mutex jobs map[int]*backupJob // per-guest backup job state (slice 8B) @@ -169,7 +175,7 @@ func NewServer(o Options) (*Server, error) { if cadence <= 0 { cadence = defaultBackupCadence } - return &Server{ + s := &Server{ addr: o.ListenAddr, cert: o.Cert, guests: o.Guests, @@ -189,7 +195,9 @@ func NewServer(o Options) (*Server, error) { hostMetrics: o.HostMetrics, hostID: o.HostID, jobs: map[int]*backupJob{}, - }, nil + } + s.reresolveWipe = s.reresolveDurableForWipe + return s, nil } // Handler builds the routed mux (exposed for tests via httptest). diff --git a/internal/localapi/wipe_reresolve.go b/internal/localapi/wipe_reresolve.go new file mode 100644 index 0000000..8127ae4 --- /dev/null +++ b/internal/localapi/wipe_reresolve.go @@ -0,0 +1,71 @@ +package localapi + +import ( + "context" + "fmt" + + "gitea.dooplex.hu/admin/felhom-agent/internal/storage" +) + +// antiRetargetResolve mirrors signedjobs.WipeExecutor.Execute's anti-retarget +// sequence for the inline customer-confirmed wipe path [AGENT-001]. +// +// The bug: handleDiskFormat inspected and gate-bound the device by its durable +// id but then formatted the mutable caller-supplied /dev path (req.Device). If a +// USB re-enumeration reassigned that /dev node to a DIFFERENT physical disk +// between inspection and mkfs, the wipe hit the wrong drive — the classic +// classify→mkfs TOCTOU the signed-jobs path already guards against. +// +// The fix: resolve the confirmed durable id to the CURRENT device, re-derive the +// device's durable id and require an exact match (a path now pointing at a +// different disk derives a different id → refuse), then re-inspect and require it +// to STILL be data-bearing. Returns the re-resolved device to format — never the +// caller-supplied path. resolve/derive/inspect are injected so this is unit +// testable without touching real /dev (the Server wires the real storage funcs). +func antiRetargetResolve( + durableID string, + resolve func(string) (string, error), + derive func(string) (string, error), + inspect func(string) (storage.DeviceProbe, error), +) (string, error) { + if durableID == "" { + // A path-only binding is exactly what the durable id exists to prevent. + return "", fmt.Errorf("no durable_id — refusing a path-only wipe binding") + } + device, err := resolve(durableID) + if err != nil { + return "", fmt.Errorf("durable id %q no longer resolves (device removed/replaced?) — refusing: %w", durableID, err) + } + got, err := derive(device) + if err != nil { + return "", fmt.Errorf("cannot re-derive durable id for %s — refusing: %w", device, err) + } + if got != durableID { + return "", fmt.Errorf("durable-id mismatch — %s now has id %q, confirmed id was %q — refusing", device, got, durableID) + } + probe, err := inspect(device) + if err != nil { + return "", fmt.Errorf("re-inspect %s failed — refusing: %w", device, err) + } + if !probe.Probed { + return "", fmt.Errorf("%s did not probe cleanly at execution — refusing", device) + } + if !probe.DataBearing() { + // The customer confirmed wiping a DATA-BEARING device; if it is now blank, + // the target changed since confirmation — refuse rather than wipe blindly. + return "", fmt.Errorf("%s is no longer data-bearing (target changed since confirmation) — refusing", device) + } + return device, nil +} + +// reresolveDurableForWipe wires antiRetargetResolve with the real storage funcs +// and the server's disk inspector. Returns the device to format, or an error to +// refuse on. +func (s *Server) reresolveDurableForWipe(ctx context.Context, durableID string) (string, error) { + return antiRetargetResolve( + durableID, + storage.ResolveDurableDevice, + storage.DeviceDurableID, + func(dev string) (storage.DeviceProbe, error) { return s.disks.InspectDevice(ctx, dev) }, + ) +} diff --git a/internal/localapi/wipe_reresolve_test.go b/internal/localapi/wipe_reresolve_test.go new file mode 100644 index 0000000..2003b40 --- /dev/null +++ b/internal/localapi/wipe_reresolve_test.go @@ -0,0 +1,108 @@ +package localapi + +import ( + "errors" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/storage" +) + +// Test for [AGENT-001]: the inline customer-confirmed wipe must re-resolve the +// confirmed durable id to the current device and refuse if anything changed in +// the window, rather than formatting a mutable /dev path that may now point at a +// different physical disk. +func TestAntiRetargetResolve(t *testing.T) { + const durable = "byid:wwn-0xCONFIRMED" + const dev = "/dev/sdb1" + + // probe helpers + dataBearing := storage.DeviceProbe{Probed: true, HasFilesystem: true, FSType: "ext4"} // DataBearing() true + blank := storage.DeviceProbe{Probed: true} // Probed, no signatures → not data-bearing + 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: "happy-path-matches", + durable: durable, + resolve: func(string) (string, error) { return dev, nil }, + derive: func(string) (string, error) { return durable, nil }, + inspect: okInspect, + wantDev: dev, + }, + { + name: "empty-durable-refused", + durable: "", + resolve: func(string) (string, error) { return dev, nil }, + derive: func(string) (string, error) { return durable, nil }, + inspect: okInspect, + wantErr: "path-only", + }, + { + name: "durable-no-longer-resolves", + durable: durable, + resolve: func(string) (string, error) { return "", errors.New("gone") }, + derive: func(string) (string, error) { return durable, nil }, + inspect: okInspect, + wantErr: "no longer resolves", + }, + { + // THE core AGENT-001 case: the /dev node now points at a DIFFERENT disk, + // so re-deriving its durable id yields a different value → refuse. + name: "retarget-mismatch-refused", + durable: durable, + resolve: func(string) (string, error) { return dev, nil }, + derive: func(string) (string, error) { return "byid:wwn-0xDIFFERENT", nil }, + inspect: okInspect, + wantErr: "durable-id mismatch", + }, + { + name: "no-longer-data-bearing-refused", + durable: durable, + resolve: func(string) (string, error) { return dev, nil }, + derive: func(string) (string, error) { return durable, nil }, + inspect: func(string) (storage.DeviceProbe, error) { return blank, nil }, + wantErr: "no longer data-bearing", + }, + { + name: "reinspect-error-refused", + durable: durable, + resolve: func(string) (string, error) { return dev, nil }, + derive: func(string) (string, error) { return durable, nil }, + inspect: func(string) (storage.DeviceProbe, error) { return storage.DeviceProbe{}, errors.New("io") }, + wantErr: "re-inspect", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := antiRetargetResolve(tc.durable, tc.resolve, tc.derive, tc.inspect) + if tc.wantErr == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tc.wantDev { + t.Fatalf("device = %q, want %q", got, tc.wantDev) + } + return + } + if err == nil { + t.Fatalf("expected refusal containing %q, got device %q and nil error", tc.wantErr, got) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error %q does not contain %q", err.Error(), tc.wantErr) + } + if got != "" { + t.Fatalf("on refusal the device must be empty, got %q", got) + } + }) + } +}