diff --git a/CHANGELOG.md b/CHANGELOG.md index 992596b..322e4b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,40 @@ +## v0.113.0 — E-2a: the guarded backup-target wrapper + POST /backup/target (2026-07-29) + +**The agent cannot do this itself, and that is the point.** Creating a PVE storage needs +`Datastore.Allocate` at `/storage`; the ACL grant needs `Permissions.Modify`. The agent holds +**neither** — its token is scoped per storage path for blast-radius containment, and +`Permissions.Modify` would let it rewrite its own authority. Widening the role to make the move +possible would trade the whole containment model for one feature. So the privileged half lives in a +new fenced shim, `configs/felhom-backup-target-apply`, behind a literal `FELHOM_BACKUPTARGET` +sudoers alias — the felhom-mkfs-guarded / felhom-pbs-apply pattern. + +The wrapper enforces the two laws E-1 paid for on live hardware, so they cannot be forgotten by a +caller: + +- **F-1** the path must BE the drive's own mountpoint (`mountpoint -q`), or the target reports + `disconnected` forever and its durable id degrades off the filesystem UUID; +- **F-2** `--is_mountpoint 1` is HARDCODED, not a caller flag. Without it an unplugged drive leaves a + bare directory on the root fs and vzdump writes onto the system drive while PVE reports `active`. + +It also refuses a target backed by the root device, and carries **no storage-removal path of any +kind** (the felhom-pbs-apply no-delete law; grep-assertable). `create` is idempotent for the same +path and REFUSES to repoint an existing id at a different one — silently moving a live backup target +is the failure this arc closes. + +`POST /backup/target` drives it in a fixed order — **create → grant → config**. Reversed, a config +pointing at an ungranted storage 403s every backup on first run, which is exactly E-1's finding F-3. +A failed grant therefore leaves the config untouched (tested). + +**It deliberately does NOT restart the agent.** The tiers are built once at daemon start, so the move +needs a restart — but restarting with a backup in flight cancels the wait and records a SPURIOUS +tier failure for a backup that actually succeeded, which E-1 did to a real `felhom-pbs` run. The +handler returns `restart_required: true` and the caller restarts behind its own immediate in-flight +check. Tests assert the handler never issues a restart itself. + +Config rewrite preserves unknown keys verbatim (`map[string]json.RawMessage`, the +`pbsdr.seedEscrowStorageID` discipline) and writes in place, because /etc/felhom-agent is root-owned +while agent.json is agent-owned 0600 — a rename is impossible for the non-root agent. + ## v0.112.0 — E-2: GET /disks flags the backup-target drive (2026-07-29) Additive field `backup_target` on each `/disks` entry, true for the drive backing the PRIMARY tier. diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index dccb354..7d0baea 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -1396,7 +1396,12 @@ func buildLocalAPIServer(cfg config.Config, px *proxmox.Client, store *backup.St GuestAttach: guestBinder, // slice 10 P2: bind enrolled data drives into the guest Memory: px, // v0.90.0 R-24: guest RAM resize (SetConfig live cgroup apply) // Network storage (NAS) — Part A1: the privileged host network-mount surface (NFS/SMB automount). - NetStorage: hostOps, + NetStorage: hostOps, + // E-2a: the fenced root shim for the backup-target move. Same runner mode as every other + // privileged call; the sudoers vector is what actually bounds it. + Privileged: &proxmox.ExecRunner{Mode: gaMode, SudoPath: cfg.Privileged.SudoPath}, + ConfigPath: cfg.SourcePath, + StateDir: cfg.WGTunnel.WithDefaults().StateDir, SmbCredsDir: cfg.Privileged.SmbCredsDir, ControllerSwap: guestBinder, // Phase 1: agentic controller update — in-guest image swap // F2-b: recover a guest left with a stale vzdump lock by a reboot-during-backup. Reads + start diff --git a/configs/felhom-agent.sudoers b/configs/felhom-agent.sudoers index 65b6e1b..07bc3d6 100644 --- a/configs/felhom-agent.sudoers +++ b/configs/felhom-agent.sudoers @@ -245,6 +245,15 @@ Cmnd_Alias FELHOM_SSHD = \ # blind to an `applied`-but-401 tier. It is NOT a general file-read: the wrapper pins the directory # and prefix-asserts the resolved path, and the id grammar admits no slash. The secret goes to # STDOUT, never argv — sudo logs argv. +# E-2a: the backup-target storage shim. Creating a PVE storage needs Datastore.Allocate at /storage +# and the grant needs Permissions.Modify -- the agent holds NEITHER by design (blast-radius +# containment; Permissions.Modify would let it rewrite its own authority). Both live behind this +# fixed-vocabulary root shim instead, exactly like the mkfs and pbs-apply wrappers. The wrapper has +# NO storage-removal path, enforces is_mountpoint 1, and refuses a target on the root device. +Cmnd_Alias FELHOM_BACKUPTARGET = \ + /usr/local/sbin/felhom-backup-target-apply create *, \ + /usr/local/sbin/felhom-backup-target-apply grant * + Cmnd_Alias FELHOM_PBSDR = \ /usr/local/sbin/felhom-pbs-apply create *, \ /usr/local/sbin/felhom-pbs-apply reconcile *, \ @@ -296,4 +305,4 @@ Cmnd_Alias FELHOM_GUESTNET = \ /usr/sbin/pct exec [0-9]* -- pgrep -x dhclient, \ /usr/sbin/pct exec [0-9]* -- dhclient -pf /run/dhclient.eth0.pid -lf /var/lib/dhcp/dhclient.eth0.leases eth0 -felhom-agent ALL=(root) NOPASSWD: FELHOM_MOUNT, FELHOM_DISK, FELHOM_PROVISION, FELHOM_FORMAT, FELHOM_DNSMASQ, FELHOM_GUESTHOOK, FELHOM_INTERMEDIARY, FELHOM_CONTROLLERSWAP, FELHOM_STALELOCK, FELHOM_NETMOUNT, FELHOM_WG, FELHOM_SELFUPDATE, FELHOM_SSHD, FELHOM_OOB, FELHOM_PBSDR, FELHOM_SELFHEAL, FELHOM_ESCROW, FELHOM_GUESTNET, FELHOM_SCRATCH_TEARDOWN +felhom-agent ALL=(root) NOPASSWD: FELHOM_MOUNT, FELHOM_DISK, FELHOM_PROVISION, FELHOM_FORMAT, FELHOM_DNSMASQ, FELHOM_GUESTHOOK, FELHOM_INTERMEDIARY, FELHOM_CONTROLLERSWAP, FELHOM_STALELOCK, FELHOM_NETMOUNT, FELHOM_WG, FELHOM_SELFUPDATE, FELHOM_SSHD, FELHOM_OOB, FELHOM_PBSDR, FELHOM_BACKUPTARGET, FELHOM_SELFHEAL, FELHOM_ESCROW, FELHOM_GUESTNET, FELHOM_SCRATCH_TEARDOWN diff --git a/configs/felhom-backup-target-apply b/configs/felhom-backup-target-apply new file mode 100755 index 0000000..827d97e --- /dev/null +++ b/configs/felhom-backup-target-apply @@ -0,0 +1,111 @@ +#!/bin/bash +#=============================================================================== +# felhom-backup-target-apply — the ONLY path the felhom-agent sudoers permits for creating the +# whole-guest backup TARGET storage and granting the agent access to it (E-2a). +# +# WHY A WRAPPER AT ALL. Creating a PVE storage needs `Datastore.Allocate` at `/storage`, and the ACL +# grant needs `Permissions.Modify`. The agent holds NEITHER by design — its token is scoped per +# storage path for blast-radius containment, and `Permissions.Modify` would let it rewrite its own +# authority. Widening the PVE role to make the move possible would trade the entire containment model +# for one feature. So the privileged half lives here: a minimal, auditable root shim with a fixed +# vocabulary, exactly like felhom-mkfs-guarded and felhom-pbs-apply. +# +# THE NO-DELETE LAW (inherited from felhom-pbs-apply, same reasoning class). This wrapper contains NO +# storage-removal path of any kind. `pvesm remove` on a dir storage does not delete the archives, but +# it DOES silently orphan a configured backup tier, and a "cleanup" verb here would be reachable by +# any bug in the agent. Retiring a target is a deliberate operator op, not this tool. Grep-assertable; +# do not add one. +# +# THE TWO LAWS E-1 PAID FOR ON LIVE HARDWARE, both enforced here rather than trusted to the caller: +# +# F-1 the storage path must BE the drive's own mountpoint. A subdirectory fails the agent's +# exactMount check, so the target reports `disconnected` FOREVER and its durable id degrades +# off the filesystem UUID. Enforced: `mountpoint -q` must pass on the exact path given. +# +# F-2 --is_mountpoint 1 is not optional. Without it, an unplugged or late-mounting drive leaves a +# bare directory on the ROOT filesystem and vzdump writes the whole-guest backup onto the +# system drive — the exact device the whole change exists to escape — while PVE reports the +# storage `active` and advertises the root filesystem's free space. Proven live: the unguarded +# form had already created dump/ on pve-root. Hardcoded below; not a caller-supplied flag. +# +# Ops (all non-secret; nothing here touches a credential, so nothing arrives on stdin): +# create +# Create a `dir` storage with content=backup at , is_mountpoint 1. +# IDEMPOTENT: an existing entry with the SAME path is accepted (re-run safe, and the +# installer re-run path depends on it). An existing entry with a DIFFERENT path is REFUSED +# — silently repointing a live backup target is the failure this whole arc closes. +# grant +# The dual grant: FelhomAgentStore on /storage/ to the agent user AND token (privsep +# intersection — a token's rights are the intersection, so granting one is granting neither). +# Without it every backup 403s on first run (E-1 finding F-3, found by the first real backup). +#=============================================================================== +set -euo pipefail + +die() { echo "felhom-backup-target-apply: REFUSED: $*" >&2; exit 1; } + +op="${1:-}"; id="${2:-}" +[[ -n "$op" && -n "$id" ]] || die "usage: felhom-backup-target-apply [mountpoint]" + +# Storage id: PVE grammar, conservative. Also the ACL path component — no slashes possible. +[[ "$id" =~ ^[A-Za-z][A-Za-z0-9_.-]{0,27}$ ]] || die "bad storage id ($id)" + +STORECFG=/etc/pve/storage.cfg + +# current_path_of — the configured `path` of dir storage , or "" when absent/not-a-dir. +current_path_of() { + awk -v want="dir: $1" ' + $0 == want { found=1; next } + found && /^[a-z]+: / { exit } + found && $1 == "path" { print $2; exit } + ' "$STORECFG" 2>/dev/null || true +} + +case "$op" in +create) + [[ $# -eq 3 ]] || die "create takes " + mp="$3" + # Absolute, normalized, no traversal, no shell metacharacters. The value reaches pvesm and the + # filesystem, so it is validated here rather than assumed well-formed. + [[ "$mp" = /* ]] || die "mountpoint must be absolute ($mp)" + [[ "$mp" != *".."* ]] || die "mountpoint must not contain .. ($mp)" + [[ "$mp" =~ ^[A-Za-z0-9/_.-]+$ ]] || die "mountpoint has unexpected characters ($mp)" + [[ "$mp" != "/" ]] || die "refusing / as a backup target" + + # F-1 + F-2, checked as one: the path must BE a mountpoint right now. A bare directory here is + # precisely the silent-retarget shape, and is_mountpoint would make PVE refuse it later anyway — + # better to refuse now, with a reason, than to create a storage that can never activate. + mountpoint -q "$mp" || die "$mp is not a mountpoint — the backup target must be the drive's OWN mountpoint (F-1), and an unmounted path would silently retarget onto the system drive (F-2)" + + # Never the system disk: a target on the root filesystem is not drive-loss protection, it is the + # thing we are escaping. The root device and the candidate's device are compared, not their paths. + root_dev="$(findmnt -no SOURCE / 2>/dev/null || true)" + mp_dev="$(findmnt -no SOURCE "$mp" 2>/dev/null || true)" + [[ -n "$mp_dev" ]] || die "could not resolve the backing device of $mp" + [[ "$mp_dev" != "$root_dev" ]] || die "$mp is backed by the ROOT device ($root_dev) — a backup target there protects against corruption only, never drive loss" + + existing="$(current_path_of "$id")" + if [[ -n "$existing" ]]; then + if [[ "$existing" == "$mp" ]]; then + echo "felhom-backup-target-apply: storage $id already exists at $mp — nothing to do (idempotent)" >&2 + exit 0 + fi + die "storage $id already exists at $existing — refusing to repoint it at $mp (a live backup target is never silently moved)" + fi + + # is_mountpoint 1 is HARDCODED (F-2). content=backup only: this storage exists for vzdump archives + # and must never become a place guests are allocated on. + pvesm add dir "$id" --path "$mp" --content backup --is_mountpoint 1 >&2 + echo "felhom-backup-target-apply: created dir storage $id at $mp (content=backup, is_mountpoint 1)" >&2 + ;; +grant) + [[ $# -eq 2 ]] || die "grant takes only " + # BOTH, always. A privsep token's rights are the intersection of the user's and the token's ACLs, + # so granting one of the two grants nothing usable. + pveum acl modify "/storage/$id" --users felhom-agent@pve --roles FelhomAgentStore >&2 + pveum acl modify "/storage/$id" --tokens 'felhom-agent@pve!agent' --roles FelhomAgentStore >&2 + echo "felhom-backup-target-apply: granted FelhomAgentStore on /storage/$id (user + token)" >&2 + ;; +*) + die "unknown op ($op)" + ;; +esac diff --git a/internal/localapi/backup_target.go b/internal/localapi/backup_target.go new file mode 100644 index 0000000..1812a3f --- /dev/null +++ b/internal/localapi/backup_target.go @@ -0,0 +1,212 @@ +package localapi + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "os" + "path/filepath" + "strings" +) + +// BackupTargetWrapperPath is the pinned sudoers vector (configs/felhom-backup-target-apply). The +// agent cannot create a PVE storage or grant an ACL itself — Datastore.Allocate at /storage and +// Permissions.Modify are deliberately outside its role — so the privileged half runs here. +const BackupTargetWrapperPath = "/usr/local/sbin/felhom-backup-target-apply" + +// backupTargetRequest is POST /backup/target: move the PRIMARY whole-guest backup tier onto the +// drive mounted at Where, creating the storage if needed. +type backupTargetRequest struct { + VMID int `json:"vmid"` + Where string `json:"where"` // the drive's OWN host mountpoint (F-1) + ID string `json:"id,omitempty"` // storage id; default backupTargetStorageID +} + +// backupTargetStorageID is the conventional id, matching what E-1 created by hand on both demo boxes. +// Keeping the name identical is what makes this endpoint IDEMPOTENT on an already-migrated box: the +// wrapper accepts an existing entry with the same path and changes nothing. +const backupTargetStorageID = "felhom-backup" + +// handleSetBackupTarget performs the whole move as one ordered operation: create the storage, grant +// the agent access, repoint the primary tier in agent.json, and hand back what the caller must do to +// make it take effect. +// +// THE ORDER IS THE DESIGN, and each step is a precondition for the next: +// +// create → grant → config +// +// Reversed, a config pointing at a storage that does not exist would make the tier DEFER (harmless +// but silent), and a config pointing at an ungranted storage would make every backup 403 on its +// first run — which is exactly what E-1 hit when the grant was forgotten (finding F-3). Creating and +// granting BEFORE the config means the worst interruption leaves an unused storage, never a broken +// tier. +// +// IT DOES NOT RESTART THE AGENT. That is deliberate and it is the E-1 lesson encoded: the backup +// tiers are built once at daemon start, so the move needs a restart to take effect — but restarting +// while a backup or restore-test is in flight cancels the wait and records a SPURIOUS tier failure +// for a backup that actually succeeded (E-1 did exactly this to a felhom-pbs run). A restart that +// this handler fires itself could never be re-checked against in-flight work by the caller, so the +// response reports `restart_required` and the caller performs it behind its own immediate +// in-flight check. +func (s *Server) handleSetBackupTarget(w http.ResponseWriter, r *http.Request, vmid int) { + if s.privileged == nil { + writeErr(w, http.StatusServiceUnavailable, "privileged runner not configured on this host") + return + } + var req backupTargetRequest + if !decodeBody(w, r, &req) { + return + } + if !s.scopedFromBody(w, req.VMID, vmid, r.URL.Path) { + return + } + where := strings.TrimSpace(req.Where) + if where == "" { + writeErr(w, http.StatusBadRequest, "where (the drive's own mountpoint) is required") + return + } + id := strings.TrimSpace(req.ID) + if id == "" { + id = backupTargetStorageID + } + + // AGENT-SIDE VALIDATION FIRST, from the agent's own storage view — never the caller's claim. + // The wrapper re-checks everything as root (it is the security boundary), but refusing here gives + // the customer a reason instead of a shell error, and keeps a bad request from reaching sudo at all. + if err := s.validateBackupTargetMount(r.Context(), where); err != nil { + s.logger.Warn("local-api: backup-target move refused", "vmid", vmid, "where", where, "err", err) + writeErr(w, http.StatusBadRequest, err.Error()) + return + } + + if _, errOut, err := s.privileged.Run(r.Context(), BackupTargetWrapperPath, "create", id, where); err != nil { + s.logger.Error("local-api: backup-target create failed", "id", id, "where", where, "err", err, "stderr", string(errOut)) + writeErr(w, http.StatusBadGateway, "could not create the backup storage: "+wrapperReason(errOut, err)) + return + } + if _, errOut, err := s.privileged.Run(r.Context(), BackupTargetWrapperPath, "grant", id); err != nil { + // The storage exists but the agent cannot write to it. Say so precisely: this is the exact + // state that produced E-1's "403 permission denied at /storage/felhom-backup" on first backup. + s.logger.Error("local-api: backup-target grant failed", "id", id, "err", err, "stderr", string(errOut)) + writeErr(w, http.StatusBadGateway, "storage created but the access grant failed — backups would 403: "+wrapperReason(errOut, err)) + return + } + if err := s.setConfiguredBackupTarget(id); err != nil { + s.logger.Error("local-api: backup-target config write failed", "id", id, "err", err) + writeErr(w, http.StatusInternalServerError, "storage is ready but the config could not be updated: "+err.Error()) + return + } + + s.logger.Info("local-api: backup target moved — RESTART REQUIRED for it to take effect", + "vmid", vmid, "target", id, "where", where) + writeOK(w, map[string]any{ + "vmid": vmid, "target": id, "where": where, + // The caller must restart the agent BEHIND ITS OWN in-flight check — see the doc comment. + "restart_required": true, + }) +} + +// validateBackupTargetMount refuses a mount that cannot be a real backup target, from the agent's own +// storage view + mount table. Mirrors the wrapper's laws so the customer gets a reason, not a shell error. +func (s *Server) validateBackupTargetMount(ctx context.Context, where string) error { + if s.storage == nil { + return fmt.Errorf("storage view unavailable") + } + // It must currently BE a mountpoint (F-1/F-2). Resolved from the mount table, which is the same + // source the wrapper's `mountpoint -q` consults. + mounts, err := s.hostReader().Mounts() + if err != nil { + return fmt.Errorf("could not read the mount table") + } + var dev string + for _, m := range mounts { + if m.MountPoint == where { + dev = m.Device + break + } + } + if dev == "" { + return fmt.Errorf("%s is not a mountpoint — the backup target must be the drive's own mountpoint", where) + } + // Never the system disk: a target there protects against corruption only, never drive loss. + for _, m := range mounts { + if m.MountPoint == "/" && m.Device == dev { + return fmt.Errorf("%s is on the system disk — a backup target there cannot survive a drive failure", where) + } + } + return nil +} + +// setConfiguredBackupTarget rewrites backup.local_backup_target in agent.json. +// +// Read-modify-write over map[string]json.RawMessage so UNKNOWN KEYS ARE PRESERVED VERBATIM — the +// same discipline as pbsdr.seedEscrowStorageID, and the property that made E-1's hand edit safe to +// begin with. A typed round-trip would silently drop any key this build does not know about. +// +// Written IN PLACE (O_TRUNC), not tmp+rename: /etc/felhom-agent is root-owned while agent.json is +// agent-owned 0600, so the non-root agent cannot rename into that directory. A recovery copy is +// parked first, so a torn write is recoverable. +func (s *Server) setConfiguredBackupTarget(id string) error { + path := s.configPath + if path == "" { + return fmt.Errorf("no config path known to this agent (env-only config)") + } + raw, err := os.ReadFile(path) + if err != nil { + return err + } + var doc map[string]json.RawMessage + if err := json.Unmarshal(raw, &doc); err != nil { + return fmt.Errorf("parse %s: %w", path, err) + } + var bk map[string]json.RawMessage + if cur, ok := doc["backup"]; ok { + if err := json.Unmarshal(cur, &bk); err != nil { + return fmt.Errorf("parse backup section: %w", err) + } + } else { + bk = map[string]json.RawMessage{} + } + idJSON, _ := json.Marshal(id) + bk["local_backup_target"] = idJSON + bkJSON, err := json.Marshal(bk) + if err != nil { + return err + } + doc["backup"] = bkJSON + out, err := json.MarshalIndent(doc, "", " ") + if err != nil { + return err + } + st, err := os.Stat(path) + if err != nil { + return err + } + if s.stateDir != "" { + if err := os.MkdirAll(s.stateDir, 0o700); err == nil { + _ = os.WriteFile(filepath.Join(s.stateDir, "agent.json.pre-backup-target"), raw, 0o600) + } + } + f, err := os.OpenFile(path, os.O_WRONLY|os.O_TRUNC, st.Mode().Perm()) + if err != nil { + return err + } + if _, err := f.Write(out); err != nil { + f.Close() + return err + } + return f.Close() +} + +// wrapperReason surfaces the wrapper's own REFUSED line when it produced one — it explains WHY in +// terms the customer can act on ("not a mountpoint", "already exists at …") — falling back to the +// exec error only when stderr said nothing useful. +func wrapperReason(errOut []byte, err error) string { + for _, line := range strings.Split(string(errOut), "\n") { + if strings.Contains(line, "REFUSED:") { + return strings.TrimSpace(line) + } + } + return err.Error() +} diff --git a/internal/localapi/backup_target_move_test.go b/internal/localapi/backup_target_move_test.go new file mode 100644 index 0000000..6085e15 --- /dev/null +++ b/internal/localapi/backup_target_move_test.go @@ -0,0 +1,174 @@ +package localapi + +import ( + "context" + "encoding/json" + "io" + "log/slog" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/storage" +) + +// wrapperRunner captures the wrapper vector without ever running sudo. The wrapper IS the security +// boundary, so tests substitute it rather than bypassing it — what is asserted here is the ORDER and +// the ARGUMENTS the agent sends, which is the agent's half of the contract. +type wrapperRunner struct { + calls [][]string + failOn string // verb to fail, "" = all succeed +} + +func (r *wrapperRunner) Run(_ context.Context, name string, args ...string) ([]byte, []byte, error) { + r.calls = append(r.calls, append([]string{name}, args...)) + if len(args) > 0 && args[0] == r.failOn { + return nil, []byte("felhom-backup-target-apply: REFUSED: synthetic " + r.failOn + " failure\n"), + io.ErrUnexpectedEOF + } + return nil, nil, nil +} + +func (r *wrapperRunner) verbs() []string { + var out []string + for _, c := range r.calls { + if len(c) > 1 { + out = append(out, c[1]) + } + } + return out +} + +// moveServer builds a server with /mnt/data mounted on its own device and / on another, plus a +// throwaway agent.json the move can rewrite. +func moveServer(t *testing.T, run *wrapperRunner) (http.Handler, string) { + t.Helper() + dir := t.TempDir() + cfgPath := filepath.Join(dir, "agent.json") + // An UNKNOWN key is deliberately present: the rewrite must preserve it verbatim. + seed := `{"backup":{"local_backup_target":"local","local_backup_retention":3},"some_future_key":{"keep":"me"}}` + if err := os.WriteFile(cfgPath, []byte(seed), 0o600); err != nil { + t.Fatalf("seed config: %v", err) + } + hr := fakeHostReader{mounts: []storage.Mount{ + {Device: "/dev/sda1", MountPoint: "/"}, + {Device: "/dev/sdb1", MountPoint: "/mnt/data"}, + }} + srv, err := NewServer(Options{ + ListenAddr: "127.0.0.1:0", + Guests: &fakeGuests{}, Backups: &fakeBackups{}, Store: &fakeStore{}, Storage: fakeStorage{}, + Tokens: staticTokens{"A": 8200}, HostReader: hr, + Privileged: run, ConfigPath: cfgPath, StateDir: dir, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }) + if err != nil { + t.Fatalf("new server: %v", err) + } + srv.baseCtx = context.Background() + return srv.Handler(), cfgPath +} + +// THE ORDER IS THE CONTRACT: create → grant → config. Reversed, a config pointing at an ungranted +// storage makes every backup 403 on first run, which is precisely what E-1 hit (finding F-3). +func TestBackupTargetMoveOrdersCreateThenGrantThenConfig(t *testing.T) { + run := &wrapperRunner{} + h, cfgPath := moveServer(t, run) + + rr := do(t, h, http.MethodPost, "/backup/target", "A", `{"vmid":8200,"where":"/mnt/data"}`) + if rr.Code != http.StatusOK { + t.Fatalf("move = %d, body %s", rr.Code, rr.Body.String()) + } + got := strings.Join(run.verbs(), ",") + if got != "create,grant" { + t.Fatalf("wrapper verbs = %q, want create,grant (in that order)", got) + } + // The config must have been written only AFTER both wrapper calls succeeded. + raw, _ := os.ReadFile(cfgPath) + var doc map[string]json.RawMessage + if err := json.Unmarshal(raw, &doc); err != nil { + t.Fatalf("config unreadable after move: %v", err) + } + var bk map[string]any + _ = json.Unmarshal(doc["backup"], &bk) + if bk["local_backup_target"] != "felhom-backup" { + t.Errorf("local_backup_target = %v, want felhom-backup", bk["local_backup_target"]) + } + // Unknown keys preserved verbatim — the property that made E-1's hand edit safe. + if _, ok := doc["some_future_key"]; !ok { + t.Error("the rewrite DROPPED an unknown top-level key — a typed round-trip would do this " + + "and silently discard config this build does not know about") + } + // Sibling keys inside `backup` survive too. + if bk["local_backup_retention"] == nil { + t.Error("the rewrite dropped local_backup_retention from the backup section") + } +} + +// A FAILED GRANT MUST NOT LEAVE THE CONFIG POINTING AT THE NEW STORAGE. That state is exactly E-1's +// 403-on-every-backup: the tier looks configured and cannot write. +func TestBackupTargetMoveDoesNotRepointWhenTheGrantFails(t *testing.T) { + run := &wrapperRunner{failOn: "grant"} + h, cfgPath := moveServer(t, run) + + rr := do(t, h, http.MethodPost, "/backup/target", "A", `{"vmid":8200,"where":"/mnt/data"}`) + if rr.Code == http.StatusOK { + t.Fatalf("move SUCCEEDED despite a failed grant (%d)", rr.Code) + } + if !strings.Contains(rr.Body.String(), "403") { + t.Errorf("the error should name the consequence (backups would 403); got %s", rr.Body.String()) + } + raw, _ := os.ReadFile(cfgPath) + if strings.Contains(string(raw), "felhom-backup") { + t.Fatal("the config was repointed at a storage the agent cannot write to — every backup " + + "would 403 while the tier reported as configured") + } +} + +// It must NOT restart the agent itself. Restarting with a backup in flight cancels the wait and +// records a spurious tier failure for a backup that actually succeeded — E-1 did exactly that to a +// felhom-pbs run. Only the caller can re-check in-flight work immediately before restarting. +func TestBackupTargetMoveReportsRestartRequiredRatherThanRestarting(t *testing.T) { + run := &wrapperRunner{} + h, _ := moveServer(t, run) + rr := do(t, h, http.MethodPost, "/backup/target", "A", `{"vmid":8200,"where":"/mnt/data"}`) + if !strings.Contains(rr.Body.String(), `"restart_required":true`) { + t.Errorf("response must tell the caller a restart is required; got %s", rr.Body.String()) + } + for _, c := range run.calls { + joined := strings.Join(c, " ") + if strings.Contains(joined, "systemctl") || strings.Contains(joined, "restart") { + t.Fatalf("the handler restarted the agent itself: %q — the caller must do it behind its "+ + "own in-flight check", joined) + } + } +} + +// A path that is not a mountpoint is refused BEFORE sudo is reached (F-1): a subdirectory target +// reports disconnected forever, and an unmounted path silently retargets onto the system drive. +func TestBackupTargetMoveRefusesANonMountpoint(t *testing.T) { + run := &wrapperRunner{} + h, _ := moveServer(t, run) + rr := do(t, h, http.MethodPost, "/backup/target", "A", `{"vmid":8200,"where":"/mnt/data/sub"}`) + if rr.Code == http.StatusOK { + t.Fatal("a non-mountpoint was accepted as the backup target") + } + if len(run.calls) != 0 { + t.Errorf("a refused request still reached the privileged wrapper: %v", run.calls) + } +} + +// The system disk is refused: a target there protects against corruption only, never drive loss — +// which is the entire point of the move. +func TestBackupTargetMoveRefusesTheSystemDisk(t *testing.T) { + run := &wrapperRunner{} + h, _ := moveServer(t, run) + rr := do(t, h, http.MethodPost, "/backup/target", "A", `{"vmid":8200,"where":"/"}`) + if rr.Code == http.StatusOK { + t.Fatal("the system disk was accepted as the backup target") + } + if len(run.calls) != 0 { + t.Errorf("a refused request still reached the privileged wrapper: %v", run.calls) + } +} diff --git a/internal/localapi/server.go b/internal/localapi/server.go index db524f3..84898ae 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -34,6 +34,12 @@ type GuestAPI interface { WaitTask(ctx context.Context, upid string, opts proxmox.WaitOptions) (proxmox.TaskStatus, error) } +// PrivilegedRunner runs a fenced root wrapper. The seam exists so the backup-target move is testable +// without sudo: the wrapper IS the security boundary, so tests substitute it, never bypass it. +type PrivilegedRunner interface { + Run(ctx context.Context, name string, args ...string) (stdout, stderr []byte, err error) +} + // BackupService enqueues a vzdump/PBS backup of a guest. Satisfied by *backup.BackupRunner. // BackupWithSnapshotHook (8B.2) invokes onSnapshot once mid-backup when the storage snapshot is // taken (snapshot mode only) so the controller can resume its app early; in stop mode it is never @@ -146,6 +152,14 @@ type Options struct { // NetStorage is the privileged network-mount (NAS) surface (Part A1). OPTIONAL — when nil, the // /netstorage endpoints report "not configured". Satisfied by *storage.SudoHostOps. NetStorage NetworkStorageOps + // Privileged runs the fenced root wrappers (E-2a: felhom-backup-target-apply). OPTIONAL — when + // nil, POST /backup/target reports "not configured". Satisfied by *proxmox.ExecRunner. + Privileged PrivilegedRunner + // ConfigPath is agent.json, so the backup-target move can repoint the primary tier. "" (env-only + // config) → the move reports it cannot persist rather than pretending it did. + ConfigPath string + // StateDir is where a pre-write recovery copy of agent.json is parked. "" → no copy is parked. + StateDir string // SmbCredsDir is where the agent writes the 0600 SMB credentials files (out-of-band). "" → // /var/lib/felhom-agent/smb-creds. SmbCredsDir string @@ -340,6 +354,9 @@ type Server struct { escrowDone <-chan struct{} // closes when the detached job finishes (tests wait on it) // ceremonyRun executes the fixed-argv sudo self-invocation (tests inject canned JSON). ceremonyRun ceremonyRunner + privileged PrivilegedRunner + configPath string + stateDir string // escrowSudoCheck is the preflight's list-mode grant probe (`sudo -n -l -- `). escrowSudoCheck func(ctx context.Context) error // escrowLookPath resolves a binary on PATH for preflight (tests inject). @@ -384,6 +401,9 @@ func NewServer(o Options) (*Server, error) { guestAttach: o.GuestAttach, mem: o.Memory, netStorage: o.NetStorage, + privileged: o.Privileged, + configPath: o.ConfigPath, + stateDir: o.StateDir, netMountRoot: storage.NetworkMountRoot, smbCredsDir: o.SmbCredsDir, escrowStagePath: o.EscrowStagePath, @@ -442,6 +462,7 @@ func (s *Server) Handler() http.Handler { mux.HandleFunc("POST /backup", s.withGuest(s.handleBackup)) mux.HandleFunc("GET /backup/due", s.withGuest(s.handleBackupDue)) mux.HandleFunc("GET /backup/tiers", s.withGuest(s.handleBackupTiers)) + mux.HandleFunc("POST /backup/target", s.withGuest(s.handleSetBackupTarget)) mux.HandleFunc("GET /backup/status", s.withGuest(s.handleBackupStatus)) mux.HandleFunc("GET /restore-test/status", s.withGuest(s.handleRestoreTestStatus)) // Host metrics (slice 9): host-wide health + per-storage capacity for the customer's monitoring