From 22171c44ab0b0330010c79d089d4656d4cbfa31e Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Sun, 2 Aug 2026 18:58:34 +1000 Subject: [PATCH] fix: close the storage-exhaustion recovery gaps (durable terminate, per-service memory caps, local volume leak) (#653) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(vm): durably record terminate even when local state persistence fails finalizeTerminated bailed out of WriteTerminatedInstance whenever the transition to terminated hit a persistence-only failure (e.g. ENOSPC writing local state), even though the in-memory status had already reached terminated. That left the instance owned by nothing: StuckTerminateReaper skips it (no longer shutting-down) and TerminatedTeardownReaper never sees it (never reached the terminated KV bucket). Only an operator daemon restart recovered it. WriteTerminatedInstance is a JetStream KV write, independent of local disk space, so keep going into it whenever the in-memory status did reach terminated, distinguishing that case from a genuine invalid/raced transition. Once the disk has space again, teardown now completes on its own via the existing idempotent TerminatedTeardownReaper. * fix(systemd): cap per-service memory under spinifex-system.slice Every service under spinifex.slice ran with MemoryMax=infinity / MemoryHigh=infinity, so one runaway service could exhaust the whole slice budget and the slice-level OOM would then reap an arbitrary victim instead of the offender. Add per-unit MemoryHigh/MemoryMax as percentages of physical RAM (so the same units scale from a dev box to bare metal). spinifex-daemon.service still hosts every guest QEMU process in its own cgroup (no per-instance slice yet), so it gets by far the largest allowance, just under the slice's own 80%/90%, to avoid starving VM launches. predastore is the legitimately memory-hungry service and gets generous headroom above its idle footprint. The remaining infra services are small, single-purpose Go processes and get a shared conservative bound. * fix(viperblockd): remove local volume directory on ebs.delete DeleteVolume deleted the S3 data and notified viperblockd, but the ebs.delete handler only stopped the volume's goroutines, killed nbdkit, and removed the NBD socket — it never removed the local WAL/checkpoint cache directory under BaseDir. The only path that removed it (VB.Close's RemoveLocalFiles) only ran from the unmount handler's seal step, which -efi volumes never reach (they carry no durable data to seal) and which a main volume also skips whenever a prior seal failed (e.g. disk full, deliberately keeping local files for recovery). Since terminate always unmounts before deleting, and delete is permanent, retention had no remaining purpose by the time ebs.delete ran. Unconditionally remove BaseDir/ in the ebs.delete handler regardless of mount-tracking state, closing the leak for both the root volume and its -efi companion. * fix(viperblockd): validate volume names before removing local state ebs.delete unmarshalled the volume name straight off the wire and handed it to os.RemoveAll: an empty name collapsed to BaseDir itself, and ".." components could escape it entirely. localVolumeDir now rejects an empty/invalid name or one that resolves outside BaseDir before any removal, and ebs.delete returns Success: false with an error instead of silently skipping. Applied the same gate to ebs.mount and ebs.config, the other two handlers that turn a wire-supplied volume name into a BaseDir-relative path. * chore: pin viperblock to the merged concurrent-open corruption fix The ansible build role builds with GOWORK=off, so the deployed binary links whatever go.mod pins rather than the go.work overlay. Move the pin to the revision carrying the per-volume flock and the WAL/state write fixes. --- build/systemd/spinifex-awsgw.service | 4 + build/systemd/spinifex-daemon.service | 4 + build/systemd/spinifex-nats.service | 4 + build/systemd/spinifex-northstar.service | 4 + build/systemd/spinifex-predastore.service | 5 + build/systemd/spinifex-qmp-collector.service | 4 + build/systemd/spinifex-ui.service | 4 + build/systemd/spinifex-viperblock.service | 4 + build/systemd/spinifex-vpcd.service | 3 + go.mod | 2 +- go.sum | 4 +- spinifex/services/viperblockd/viperblockd.go | 88 +++++++++- .../viperblockd/viperblockd_handlers_test.go | 160 ++++++++++++++++++ .../services/viperblockd/viperblockd_test.go | 60 +++++++ spinifex/vm/shutdown.go | 22 ++- spinifex/vm/shutdown_test.go | 49 ++++++ 16 files changed, 414 insertions(+), 7 deletions(-) diff --git a/build/systemd/spinifex-awsgw.service b/build/systemd/spinifex-awsgw.service index cb1e5ef21..5efc21605 100644 --- a/build/systemd/spinifex-awsgw.service +++ b/build/systemd/spinifex-awsgw.service @@ -19,6 +19,10 @@ RestartSec=5 TimeoutStopSec=10 OOMScoreAdjust=-500 +# Per-service ceiling bounds a leak in this service alone rather than +# tripping the slice-wide cap, which would pick an arbitrary victim. +MemoryHigh=10% +MemoryMax=20% Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml Environment=SPINIFEX_BASE_DIR=/var/lib/spinifex/awsgw/ Environment=SPINIFEX_AWSGW_TLS_CERT=/etc/spinifex/server.pem diff --git a/build/systemd/spinifex-daemon.service b/build/systemd/spinifex-daemon.service index 54767f701..ea16f9b75 100644 --- a/build/systemd/spinifex-daemon.service +++ b/build/systemd/spinifex-daemon.service @@ -19,6 +19,10 @@ KillMode=process TimeoutStopSec=15 # RG-4: infra tier — reaped only after every guest OOMScoreAdjust=-500 +# Guest QEMU still runs inside this cgroup, so the ceiling tracks the slice +# budget (just under 80%/90%), not just daemon overhead. +MemoryHigh=75% +MemoryMax=85% Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml Environment=SPINIFEX_BASE_DIR=/var/lib/spinifex/spinifex/ Environment=SPINIFEX_WAL_DIR=/var/lib/spinifex/spinifex/ diff --git a/build/systemd/spinifex-nats.service b/build/systemd/spinifex-nats.service index 8c9762563..583ed3d11 100644 --- a/build/systemd/spinifex-nats.service +++ b/build/systemd/spinifex-nats.service @@ -17,6 +17,10 @@ RestartSec=5 # stops before nats does, so size this for nats's own graceful-stop needs. TimeoutStopSec=20 OOMScoreAdjust=-500 +# Per-service ceiling bounds a leak in this service alone rather than +# tripping the slice-wide cap, which would pick an arbitrary victim. +MemoryHigh=10% +MemoryMax=20% Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/nats/nats.conf # Filesystem access diff --git a/build/systemd/spinifex-northstar.service b/build/systemd/spinifex-northstar.service index fa199b98b..f65bb2507 100644 --- a/build/systemd/spinifex-northstar.service +++ b/build/systemd/spinifex-northstar.service @@ -17,6 +17,10 @@ RestartSec=5 # NATS unsubscribe/close and process exit that follow it. TimeoutStopSec=10 OOMScoreAdjust=-500 +# Per-service ceiling bounds a leak in this service alone rather than +# tripping the slice-wide cap, which would pick an arbitrary victim. +MemoryHigh=10% +MemoryMax=20% Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml Environment=SPINIFEX_BASE_DIR=/var/lib/spinifex/northstar/ diff --git a/build/systemd/spinifex-predastore.service b/build/systemd/spinifex-predastore.service index cd6d211ce..307db3194 100644 --- a/build/systemd/spinifex-predastore.service +++ b/build/systemd/spinifex-predastore.service @@ -22,6 +22,11 @@ RestartSec=5 # backstop if that ordering is ever bypassed (direct stop, future edit). TimeoutStopSec=30 OOMScoreAdjust=-500 +# Legitimately the most memory-hungry infra service (S3 buffering/caching); +# High/Max stay well above idle so normal load isn't throttled, while still +# bounding a leak short of the slice-wide cap. +MemoryHigh=40% +MemoryMax=55% Environment=SPINIFEX_PREDASTORE_CONFIG_PATH=/etc/spinifex/predastore/predastore.toml Environment=SPINIFEX_PREDASTORE_BASE_PATH=/var/lib/spinifex/predastore/ Environment=SPINIFEX_PREDASTORE_TLS_CERT=/etc/spinifex/server.pem diff --git a/build/systemd/spinifex-qmp-collector.service b/build/systemd/spinifex-qmp-collector.service index 707eb2be4..0cdb59842 100644 --- a/build/systemd/spinifex-qmp-collector.service +++ b/build/systemd/spinifex-qmp-collector.service @@ -19,6 +19,10 @@ RestartSec=5 # Poll loop selects on ctx.Done() directly and closes NATS locally; shutdown # is near-instant, so this only bounds an unexpectedly wedged process. TimeoutStopSec=10 +# Per-service ceiling bounds a leak in this service alone rather than +# tripping the slice-wide cap, which would pick an arbitrary victim. +MemoryHigh=10% +MemoryMax=20% Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml Environment=SPINIFEX_BASE_DIR=/var/lib/spinifex/spinifex/ # Same runtime dir as the daemon: telemetry sockets + metadata live there diff --git a/build/systemd/spinifex-ui.service b/build/systemd/spinifex-ui.service index ee6d306e1..600f5641a 100644 --- a/build/systemd/spinifex-ui.service +++ b/build/systemd/spinifex-ui.service @@ -16,6 +16,10 @@ RestartSec=5 # long streaming proxy responses, e.g. console passthrough); add margin above it. TimeoutStopSec=40 OOMScoreAdjust=-500 +# Per-service ceiling bounds a leak in this service alone rather than +# tripping the slice-wide cap, which would pick an arbitrary victim. +MemoryHigh=10% +MemoryMax=20% Environment=SPINIFEX_UI_TLS_CERT=/etc/spinifex/server.pem Environment=SPINIFEX_UI_TLS_KEY=/etc/spinifex/server.key Environment=SPINIFEX_UI_BASE_DIR=/var/lib/spinifex/spinifex-ui/ diff --git a/build/systemd/spinifex-viperblock.service b/build/systemd/spinifex-viperblock.service index 3c83933f4..71f83c62f 100644 --- a/build/systemd/spinifex-viperblock.service +++ b/build/systemd/spinifex-viperblock.service @@ -15,6 +15,10 @@ ExecStart=/usr/local/bin/spx service viperblock start Restart=on-failure RestartSec=5 OOMScoreAdjust=-500 +# Per-service ceiling bounds a leak in this service alone rather than +# tripping the slice-wide cap, which would pick an arbitrary victim. +MemoryHigh=15% +MemoryMax=25% # KillMode=process: nbdkit still serving a guest survives a restart here, the # way guest QEMU survives a daemon restart. Tearing one out mid-write corrupts # that guest's filesystem; idle nbdkit is reaped explicitly on shutdown. diff --git a/build/systemd/spinifex-vpcd.service b/build/systemd/spinifex-vpcd.service index f4d37325e..3ef8606ed 100644 --- a/build/systemd/spinifex-vpcd.service +++ b/build/systemd/spinifex-vpcd.service @@ -18,6 +18,9 @@ RestartSec=5 # no sub-timeout bounds that call today, so this covers a normal pass. TimeoutStopSec=20 OOMScoreAdjust=-500 +# Per-service ceiling: bounds a leak here instead of the slice-wide cap. +MemoryHigh=10% +MemoryMax=20% Environment=SPINIFEX_CONFIG_PATH=/etc/spinifex/spinifex.toml Environment=SPINIFEX_BASE_DIR=/var/lib/spinifex/vpcd/ diff --git a/go.mod b/go.mod index 82890692e..74f7fdf73 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( github.com/miekg/dns v1.1.72 github.com/mulgadc/northstar v1.14.0 github.com/mulgadc/predastore v1.14.1-0.20260730221229-608939ef2bb4 - github.com/mulgadc/viperblock v1.14.1-0.20260801223811-8a4787ade83a + github.com/mulgadc/viperblock v1.14.1-0.20260802084514-ed52f6a0e4c0 github.com/nats-io/nats-server/v2 v2.14.3 github.com/nats-io/nats.go v1.52.0 github.com/opencontainers/runtime-spec v1.3.0 diff --git a/go.sum b/go.sum index d6ef7700e..837017fdb 100644 --- a/go.sum +++ b/go.sum @@ -1321,8 +1321,8 @@ github.com/mulgadc/northstar v1.14.0 h1:8JzDcS4k3SN5PF3jBdB8x9FOWC24eITCClmOdwpo github.com/mulgadc/northstar v1.14.0/go.mod h1:6EArbWIQUrCFQ+XZ5ZJrGS/ep/u/rF6l+dKZh5vVIRs= github.com/mulgadc/predastore v1.14.1-0.20260730221229-608939ef2bb4 h1:Q4/NEryYTMiALkFwbPPOrKpNgLir+d7T2hbyRpAFnWc= github.com/mulgadc/predastore v1.14.1-0.20260730221229-608939ef2bb4/go.mod h1:na0aNXABZF03/HfFWCrAPHrId/2FIEIUQfB82vlu1xg= -github.com/mulgadc/viperblock v1.14.1-0.20260801223811-8a4787ade83a h1:7HZ8K0hpA0tuExkWgcNmUuK1ZH7ZvjNpVsFvKLvOMrE= -github.com/mulgadc/viperblock v1.14.1-0.20260801223811-8a4787ade83a/go.mod h1:5ENQb0OrjTplIRiLShRW/oNWOzbFvSCFgo+PkNH4858= +github.com/mulgadc/viperblock v1.14.1-0.20260802084514-ed52f6a0e4c0 h1:etjtOa6Sq21CtX+8RU4vYb5JBz+dExPLLym6xpXD4Mw= +github.com/mulgadc/viperblock v1.14.1-0.20260802084514-ed52f6a0e4c0/go.mod h1:5ENQb0OrjTplIRiLShRW/oNWOzbFvSCFgo+PkNH4858= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= diff --git a/spinifex/services/viperblockd/viperblockd.go b/spinifex/services/viperblockd/viperblockd.go index 8a4b31895..d5c1a06f8 100644 --- a/spinifex/services/viperblockd/viperblockd.go +++ b/spinifex/services/viperblockd/viperblockd.go @@ -253,12 +253,51 @@ func isAuxVolume(volumeName string) bool { return strings.HasSuffix(volumeName, "-efi") } +// validVolumeName reports whether volume is safe to use as a single path +// component under BaseDir: non-empty, no separator, and not "." or "..". +func validVolumeName(volume string) bool { + return volume != "" && volume != "." && volume != ".." && filepath.Base(volume) == volume +} + +// localVolumeDir validates volume and baseDir, then returns baseDir/volume. +// Every handler that turns a wire-supplied volume name into a filesystem path +// (mount, unmount, delete, config) must go through this: the name arrives +// unmarshalled straight from a NATS message, so an empty or ".."-laden value +// must never reach a filesystem call. The character checks in validVolumeName +// already rule out an escape, but the path is still resolved and checked +// against baseDir as a second, independent guard. +func localVolumeDir(baseDir, volume string) (string, error) { + if baseDir == "" { + return "", fmt.Errorf("empty base directory") + } + if !validVolumeName(volume) { + return "", fmt.Errorf("invalid volume name %q", volume) + } + + absBase, err := filepath.Abs(baseDir) + if err != nil { + return "", fmt.Errorf("resolve base directory: %w", err) + } + dir := filepath.Join(absBase, volume) + + rel, err := filepath.Rel(absBase, dir) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("volume name %q escapes base directory", volume) + } + return dir, nil +} + // volumeNeedsSeal reports whether an unmounted volume has local viperblock // state under baseDir/ to flush. A node that never held the local // WAL has nothing to seal. Callers handle auxiliary volumes separately (see // isAuxVolume). func volumeNeedsSeal(volumeName, baseDir string) bool { - _, err := os.Stat(filepath.Join(baseDir, volumeName)) + dir, err := localVolumeDir(baseDir, volumeName) + if err != nil { + slog.Warn("volumeNeedsSeal: rejecting invalid volume name", "volume", volumeName, "err", err) + return false + } + _, err = os.Stat(dir) return err == nil } @@ -469,6 +508,17 @@ func launchService(cfg *Config) (err error) { return } + // Reject before touching anything: an empty or path-traversing name + // must never reach the RemoveAll below, which is otherwise wide + // enough to wipe BaseDir itself or escape it entirely. + localPath, err := localVolumeDir(cfg.BaseDir, ebsRequest.Volume) + if err != nil { + slog.ErrorContext(ctx, "ebs.delete: refusing invalid volume name", "volume", ebsRequest.Volume, "err", err) + utils.MarkSpanError(span, err) + respondJSON(msg, types.EBSDeleteResponse{Volume: ebsRequest.Volume, Error: fmt.Sprintf("invalid volume name: %v", err)}) + return + } + response := types.EBSDeleteResponse{Volume: ebsRequest.Volume, Success: true} // Find and clean up the mounted volume if it exists @@ -515,6 +565,20 @@ func launchService(cfg *Config) (err error) { slog.InfoContext(ctx, "ebs.delete: volume not mounted (expected for available volumes)", "volume", ebsRequest.Volume) } + // Delete is permanent: remove the on-disk WAL/checkpoint cache + // regardless of mount-tracking state. -efi volumes never go through + // the unmount seal (isAuxVolume skips it, they carry no durable + // data), and a main volume's seal can have been skipped after a + // failed flush (e.g. disk full), so this is the only guaranteed + // cleanup point for both. localPath was validated above. + if err := os.RemoveAll(localPath); err != nil { + slog.ErrorContext(ctx, "ebs.delete: failed to remove local volume directory", + "volume", ebsRequest.Volume, "path", localPath, "err", err) + } else { + slog.InfoContext(ctx, "ebs.delete: removed local volume directory", + "volume", ebsRequest.Volume, "path", localPath) + } + respondJSON(msg, response) }); err != nil { return fmt.Errorf("failed to subscribe to ebs.delete: %w", err) @@ -718,6 +782,16 @@ func launchService(cfg *Config) (err error) { return } + // Reject before the not-live branch below can hand this name to + // openLoadedVolumeVB, which derives a BaseDir/ path. + if !validVolumeName(req.Volume) { + err := fmt.Errorf("invalid volume name %q", req.Volume) + slog.ErrorContext(ctx, "ebs.config: refusing invalid volume name", "volume", req.Volume) + utils.MarkSpanError(span, err) + respondJSON(msg, types.EBSConfigUpdateResponse{Volume: req.Volume, Error: err.Error()}) + return + } + cfg.mu.Lock() var live *viperblock.VB for _, volume := range cfg.MountedVolumes { @@ -793,6 +867,18 @@ func launchService(cfg *Config) (err error) { slog.InfoContext(ctx, "ebs.mount", "request", ebsRequest) + // Reject before any local path is derived from the name: this is the + // first point an unvalidated wire name would otherwise reach the + // filesystem (clearStaleSealReceipt) and, further down, viperblock's + // own BaseDir/ layout. + if !validVolumeName(ebsRequest.Name) { + err := fmt.Errorf("invalid volume name %q", ebsRequest.Name) + slog.ErrorContext(ctx, "ebs.mount: refusing invalid volume name", "volume", ebsRequest.Name) + utils.MarkSpanError(span, err) + respondJSON(msg, types.EBSMountResponse{Error: err.Error()}) + return + } + // Clear any receipt left by a previous mount before anything else can // return early, so a stale receipt can never survive into this mount. clearStaleSealReceipt(cfg.BaseDir, ebsRequest.Name) diff --git a/spinifex/services/viperblockd/viperblockd_handlers_test.go b/spinifex/services/viperblockd/viperblockd_handlers_test.go index 15d4fc153..ad72827d0 100644 --- a/spinifex/services/viperblockd/viperblockd_handlers_test.go +++ b/spinifex/services/viperblockd/viperblockd_handlers_test.go @@ -125,6 +125,166 @@ func TestIntegration_EBSDeleteUnmountedVolume(t *testing.T) { assert.True(t, resp.Success) } +// TestIntegration_EBSDeleteRemovesLocalVolumeDirectory covers the residual +// leak: terminateCleanup always unmounts before DeleteVolumes sends +// ebs.delete, so the common case is an already-unmounted volume. Before the +// fix, ebs.delete only cleaned up a still-mounted volume's nbdkit +// process/socket and never touched cfg.BaseDir/, so the +// WAL/checkpoint directory a prior mount (or a failed unmount seal) left +// behind survived every DeleteVolume call — including for -efi companion +// volumes, which never go through the unmount seal at all (isAuxVolume skips +// it), so they leaked unconditionally rather than only on a failed seal. +func TestIntegration_EBSDeleteRemovesLocalVolumeDirectory(t *testing.T) { + t.Parallel() + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + ns, natsURL := setupEmbeddedNATS(t) + defer ns.Shutdown() + + cfg := setupTestConfig(t, natsURL) + // No mounted volumes: matches the terminate path where Unmount already + // ran and left local state behind (a failed seal for the main volume, or + // unconditionally for the -efi companion) before ebs.delete fires. + createMockVolumeState(t, cfg.BaseDir, "vol-residual-test") + createMockVolumeState(t, cfg.BaseDir, "vol-residual-test-efi") + + go func() { launchService(cfg) }() + time.Sleep(500 * time.Millisecond) + + nc, err := nats.Connect(natsURL) + require.NoError(t, err) + defer nc.Close() + + for _, volume := range []string{"vol-residual-test", "vol-residual-test-efi"} { + reqData, _ := json.Marshal(types.EBSDeleteRequest{Volume: volume}) + msg, err := nc.Request("ebs.delete", reqData, 3*time.Second) + require.NoError(t, err) + + var resp types.EBSDeleteResponse + require.NoError(t, json.Unmarshal(msg.Data, &resp)) + assert.True(t, resp.Success) + + assert.False(t, fileExistsCheck(filepath.Join(cfg.BaseDir, volume)), + "ebs.delete must remove the local volume directory for %s", volume) + } +} + +// TestIntegration_EBSDeleteEmptyVolumeNameDoesNotWipeBaseDir is the +// regression test for the data-destruction hazard: before validation, +// filepath.Join(cfg.BaseDir, "") collapsed to cfg.BaseDir itself, so an empty +// Volume made ebs.delete os.RemoveAll the node's entire local WAL/checkpoint +// cache. This must be rejected, and BaseDir must survive. +func TestIntegration_EBSDeleteEmptyVolumeNameDoesNotWipeBaseDir(t *testing.T) { + t.Parallel() + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + ns, natsURL := setupEmbeddedNATS(t) + defer ns.Shutdown() + + cfg := setupTestConfig(t, natsURL) + createMockVolumeState(t, cfg.BaseDir, "vol-survivor") + + go func() { launchService(cfg) }() + time.Sleep(500 * time.Millisecond) + + nc, err := nats.Connect(natsURL) + require.NoError(t, err) + defer nc.Close() + + reqData, _ := json.Marshal(types.EBSDeleteRequest{Volume: ""}) + msg, err := nc.Request("ebs.delete", reqData, 3*time.Second) + require.NoError(t, err) + + var resp types.EBSDeleteResponse + require.NoError(t, json.Unmarshal(msg.Data, &resp)) + assert.False(t, resp.Success, "an empty volume name must not report success") + assert.NotEmpty(t, resp.Error) + + assert.True(t, fileExistsCheck(cfg.BaseDir), "BaseDir itself must survive an empty volume name") + assert.True(t, fileExistsCheck(filepath.Join(cfg.BaseDir, "vol-survivor")), + "an unrelated volume directory must survive an empty volume name") +} + +// TestIntegration_EBSDeleteRejectsPathTraversal covers names that would +// otherwise let ebs.delete escape BaseDir via filepath.Join's ".." cleaning. +// Both cases must be refused before any RemoveAll runs, and Success must not +// come back true for a request that was refused. +func TestIntegration_EBSDeleteRejectsPathTraversal(t *testing.T) { + t.Parallel() + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + ns, natsURL := setupEmbeddedNATS(t) + defer ns.Shutdown() + + cfg := setupTestConfig(t, natsURL) + createMockVolumeState(t, cfg.BaseDir, "vol-survivor") + + go func() { launchService(cfg) }() + time.Sleep(500 * time.Millisecond) + + nc, err := nats.Connect(natsURL) + require.NoError(t, err) + defer nc.Close() + + for _, name := range []string{"../..", "a/b"} { + reqData, _ := json.Marshal(types.EBSDeleteRequest{Volume: name}) + msg, err := nc.Request("ebs.delete", reqData, 3*time.Second) + require.NoError(t, err) + + var resp types.EBSDeleteResponse + require.NoError(t, json.Unmarshal(msg.Data, &resp)) + assert.False(t, resp.Success, "volume name %q must not report success", name) + assert.NotEmpty(t, resp.Error, "volume name %q must return an error", name) + } + + assert.True(t, fileExistsCheck(filepath.Join(cfg.BaseDir, "vol-survivor")), + "an unrelated volume directory must survive a path-traversal attempt") +} + +// TestIntegration_EBSDeleteValidNameLeavesSiblingsUntouched is the essential +// non-regression check for the new validation: a legitimate delete must still +// remove exactly its own directory, and nothing else under BaseDir. +func TestIntegration_EBSDeleteValidNameLeavesSiblingsUntouched(t *testing.T) { + t.Parallel() + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + ns, natsURL := setupEmbeddedNATS(t) + defer ns.Shutdown() + + cfg := setupTestConfig(t, natsURL) + createMockVolumeState(t, cfg.BaseDir, "vol-target") + createMockVolumeState(t, cfg.BaseDir, "vol-sibling") + + go func() { launchService(cfg) }() + time.Sleep(500 * time.Millisecond) + + nc, err := nats.Connect(natsURL) + require.NoError(t, err) + defer nc.Close() + + reqData, _ := json.Marshal(types.EBSDeleteRequest{Volume: "vol-target"}) + msg, err := nc.Request("ebs.delete", reqData, 3*time.Second) + require.NoError(t, err) + + var resp types.EBSDeleteResponse + require.NoError(t, json.Unmarshal(msg.Data, &resp)) + assert.True(t, resp.Success) + assert.Empty(t, resp.Error) + + assert.False(t, fileExistsCheck(filepath.Join(cfg.BaseDir, "vol-target")), + "the targeted volume directory must be removed") + assert.True(t, fileExistsCheck(filepath.Join(cfg.BaseDir, "vol-sibling")), + "a sibling volume directory must survive deleting a different volume") +} + func TestIntegration_EBSDeleteInvalidJSON(t *testing.T) { t.Parallel() if testing.Short() { diff --git a/spinifex/services/viperblockd/viperblockd_test.go b/spinifex/services/viperblockd/viperblockd_test.go index a64ee70c6..9fd10c18d 100644 --- a/spinifex/services/viperblockd/viperblockd_test.go +++ b/spinifex/services/viperblockd/viperblockd_test.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "sync" "testing" "time" @@ -512,3 +513,62 @@ func TestRetryLoadState(t *testing.T) { assert.Equal(t, 225*time.Millisecond, lastDelay) }) } + +// TestValidVolumeName covers the character-level gate every handler that +// turns a wire volume name into a filesystem path must pass first. +func TestValidVolumeName(t *testing.T) { + valid := []string{"vol-0123456789abcdef0", "vol-0123456789abcdef0-efi", "a"} + for _, name := range valid { + assert.True(t, validVolumeName(name), "expected %q to be valid", name) + } + + invalid := []string{"", ".", "..", "../..", "a/b", "/etc/passwd", "vol/../.."} + for _, name := range invalid { + assert.False(t, validVolumeName(name), "expected %q to be rejected", name) + } +} + +// TestLocalVolumeDir is the regression test for the ebs.delete data-destruction +// hazard: an unvalidated volume name reaching os.RemoveAll could wipe BaseDir +// itself (empty name) or escape it entirely ("../.."). Every case here must be +// rejected before a caller ever gets a path back to remove. +func TestLocalVolumeDir(t *testing.T) { + baseDir := t.TempDir() + + t.Run("empty_volume_rejected", func(t *testing.T) { + _, err := localVolumeDir(baseDir, "") + require.Error(t, err, "an empty volume must never resolve to BaseDir itself") + }) + + t.Run("dot_rejected", func(t *testing.T) { + _, err := localVolumeDir(baseDir, ".") + require.Error(t, err) + }) + + t.Run("parent_traversal_rejected", func(t *testing.T) { + for _, name := range []string{"..", "../..", "../../etc"} { + _, err := localVolumeDir(baseDir, name) + require.Error(t, err, "volume %q must not escape BaseDir", name) + } + }) + + t.Run("embedded_separator_rejected", func(t *testing.T) { + for _, name := range []string{"a/b", "a/../../b", "/abs/path"} { + _, err := localVolumeDir(baseDir, name) + require.Error(t, err, "volume %q must not contain a path separator", name) + } + }) + + t.Run("empty_base_dir_rejected", func(t *testing.T) { + _, err := localVolumeDir("", "vol-abc123") + require.Error(t, err) + }) + + t.Run("valid_name_resolves_under_base_dir", func(t *testing.T) { + dir, err := localVolumeDir(baseDir, "vol-abc123") + require.NoError(t, err) + absBase, err := filepath.Abs(baseDir) + require.NoError(t, err) + assert.Equal(t, filepath.Join(absBase, "vol-abc123"), dir) + }) +} diff --git a/spinifex/vm/shutdown.go b/spinifex/vm/shutdown.go index 66b02d177..e93745811 100644 --- a/spinifex/vm/shutdown.go +++ b/spinifex/vm/shutdown.go @@ -240,8 +240,21 @@ func (m *Manager) finalizeTerminated(instance *VM) error { // instance that was never inserted into the local map. m.Inspect(instance, func(v *VM) { v.LastNode = m.deps.NodeID }) - if err := m.transitionWithPrecheck(instance, StateTerminated); err != nil { - return fmt.Errorf("transition to terminated: %w", err) + transitionErr := m.transitionWithPrecheck(instance, StateTerminated) + if transitionErr != nil && errors.Is(transitionErr, ErrInvalidTransition) { + // A genuine invalid/raced transition: in-memory status never reached + // terminated, so there is nothing durable to record yet. + return fmt.Errorf("transition to terminated: %w", transitionErr) + } + if transitionErr != nil { + // In-memory status reached terminated even though local persistence + // failed (e.g. ENOSPC writing the local state file). Keep going: + // WriteTerminatedInstance below is a JetStream KV write independent + // of local disk space, so the durable record can still land and + // TerminatedTeardownReaper picks it up on its next sweep without + // requiring an operator restart. + slog.Warn("Local state persistence failed on terminate, continuing to durable KV write", + "instanceId", instance.ID, "err", transitionErr) } // Stamp the termination time so the GC backstop can preserve a @@ -254,6 +267,9 @@ func (m *Manager) finalizeTerminated(instance *VM) error { if err := m.deps.StateStore.WriteTerminatedInstance(instance.ID, instance); err != nil { slog.Error("Failed to write terminated instance to KV, keeping in local state for retry", "instanceId", instance.ID, "err", err) + if transitionErr != nil { + return transitionErr + } return err } } @@ -276,7 +292,7 @@ func (m *Manager) finalizeTerminated(instance *VM) error { } slog.Info("Released instance ownership to KV", "instanceId", instance.ID, "state", string(StateTerminated), "lastNode", m.deps.NodeID) - return nil + return transitionErr } // reconcileVanishedQEMU finalizes a shutting-down instance whose QEMU process diff --git a/spinifex/vm/shutdown_test.go b/spinifex/vm/shutdown_test.go index f1c9c1163..c7141215f 100644 --- a/spinifex/vm/shutdown_test.go +++ b/spinifex/vm/shutdown_test.go @@ -1018,6 +1018,55 @@ func TestTerminate_WriteTerminatedFailure_PropagatesAndKeepsLocal(t *testing.T) assert.True(t, stillInMap, "instance must remain in local map for retry on the next daemon restart") } +// TestTerminate_LocalPersistenceFailure_StillDurablyRecordsTerminated covers +// the ENOSPC scenario: TransitionState fails to persist locally (e.g. the +// local state file write hits a full disk) but the in-memory status still +// reaches StateTerminated. Before the fix, finalizeTerminated bailed out on +// that error before ever calling WriteTerminatedInstance, so the durable +// terminated record TerminatedTeardownReaper scans was never written and +// nothing retried teardown without an operator restart. The durable KV write +// (a JetStream call independent of local disk space) must still happen. +func TestTerminate_LocalPersistenceFailure_StillDurablyRecordsTerminated(t *testing.T) { + t.Setenv("XDG_RUNTIME_DIR", t.TempDir()) + persistErr := errors.New("no space left on device") + store := newFakeStateStore() + cleaner := &recordingInstanceCleaner{} + down := &atomic.Int64{} + m := NewManager() + m.SetDeps(Deps{ + NodeID: "test-node", + StateStore: store, + VolumeMounter: &fakeVolumeMounter{}, + InstanceCleaner: cleaner, + TransitionState: func(v *VM, target InstanceState) error { + // Mirrors daemon.TransitionState: memory mutation always + // succeeds; only the local file persistence fails, matching a + // full disk on the terminated transition specifically. + m.Inspect(v, func(vv *VM) { vv.Status = target }) + if target == StateTerminated { + return persistErr + } + return nil + }, + ShutdownSignal: func() bool { return false }, + Hooks: ManagerHooks{ + OnInstanceDown: func(id string) { down.Add(1) }, + }, + }) + + v := &VM{ID: "i-local-persist-fail", Status: StateRunning, InstanceType: "t3.micro", Instance: &ec2.Instance{}} + m.Insert(v) + + err := m.Terminate(v.ID) + + require.ErrorIs(t, err, persistErr, "the local persistence failure must still surface to the caller") + require.NotNil(t, store.terminated[v.ID], + "WriteTerminatedInstance must still run so TerminatedTeardownReaper can find and finish this instance") + assert.Equal(t, int64(1), down.Load(), "OnInstanceDown must still fire once the durable record lands") + _, stillInMap := m.Get(v.ID) + assert.False(t, stillInMap, "instance must leave the local running map once terminated is durable") +} + // TestTerminate_DeleteIfReclaimed_NoHook covers the slot-reclaim branch // in finalizeTerminated (shutdown.go:197-201): a concurrent handler // inserts a fresh VM under the same id between WriteTerminatedInstance