fix(AGENT-001): anti-retarget re-resolution for inline customer-confirmed wipe
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
|
||||
|
||||
@@ -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).
|
||||
|
||||
@@ -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) },
|
||||
)
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user