diff --git a/FORK.md b/FORK.md index 637e342625..b460430200 100644 --- a/FORK.md +++ b/FORK.md @@ -111,6 +111,8 @@ Everything on `giantswarm` that is not in the pin (`git log v0.4.0-alpha1..giant | The egress gateway's ephemeral storage is bounded (`atenetEgress.resources` for the `agentgateway` container and `atenetEgress.extProc.resources`, ephemeral-storage requests `16Mi` and limits `256Mi` by default, CPU and memory left to the operator; `atenetEgress.drainSignal.sizeLimit`, `16Mi` on the `drain-signal` emptyDir the ext-proc writes its drain marker to; `null` renders the field as before; `charts/substrate/tests/atenet_egress_ephemeral_storage_test.yaml`; README rows) | the chart rendered both containers without `resources` and the emptyDir without a `sizeLimit`, so a cluster policy that requires bounded ephemeral storage for a container mounting an emptyDir (Kyverno's `require-emptydir-requests-and-limits`) reported the egress gateway on every install ([giantswarm/agent-platform#880](https://github.com/giantswarm/agent-platform/issues/880)) | `9246909a` ([#255](https://github.com/giantswarm/substrate/pull/255), rebase-merged 2026-10-10; first release 1.8.0-rc.4) | to file: upstream's `atenet-egress.yaml` renders the same unbounded shape and no upstream issue covers it; queued in the upstream engagement list | | Require `golang.org/x/net` v0.61.0 in every module that had it below (`go.mod`, `hack/tools/{code-generator,controller-gen,go-licenses,ko}`, `internal/plugins/gcp-secret-manager`, `tools/apitool`; `go get golang.org/x/net@v0.61.0 && go mod tidy` in each, then `go mod vendor` for the root module the CircleCI builds compile with `-mod=vendor`; `x/sync`, `x/sys`, `x/term`, `x/text` and `x/tools` move along, and tidy drops the stale indirect requirements `code-generator`'s module file still listed) | CVE-2026-97032 and CVE-2026-78663, two HTTP/2 vulnerabilities in `x/net` below v0.61.0; the platform's Go repositories move together, their CircleCI nancy step refusing the older module | [#252](https://github.com/giantswarm/substrate/pull/252) (`fix(deps): bump golang.org/x/net to v0.61.0 for two HTTP/2 CVEs`) | not for upstream as such: changes versions only and falls away at the re-pin onto the first upstream commit that requires `x/net` ≥ v0.61.0 (upstream's dependency automation moves it) | | `crashActor` terminates the sandbox on atelet, detaches the actor's volumes and only then releases the worker (a failed step leaves the actor `CRASHED` with its worker assignment, for `DeleteActor` or `RevertActor` to retry); `DeleteActor` refuses with `FailedPrecondition` while the sandbox cannot be stopped or a volume released, keeping the assignment for the retry; `CreateActorTemplate` refuses a DATA `on_pause` or `on_commit` scope unless a container mounts a durable-dir volume (`InvalidArgument` naming the field); the existing-volumes e2e suite covers the delete after a failed suspend | a failed checkpoint crashed the actor and released its worker with the sandbox still running on it, so `DeleteActor` found no worker to reach and removed the record: the sandbox, its existing-volume NFS mounts and the worker's capacity leaked until the worker pod restarted, and the PersistentVolume stayed `Released` on `directory not empty` ([giantswarm/substrate#237](https://github.com/giantswarm/substrate/issues/237); giantswarm/giantswarm#37742 row 162) | [#258](https://github.com/giantswarm/substrate/pull/258) (`fix(ateapi): tear a crashed actor's workload down before it is deleted`) | [agent-substrate/substrate#1953](https://github.com/agent-substrate/substrate/pull/1953) (open: the crash tears the workload down before `CRASHED` and keeps the assignment when it fails, the shape adapted here) and [agent-substrate/substrate#1972](https://github.com/agent-substrate/substrate/pull/1972) (open: refuses the DATA scope without a durable dir, for agent-substrate/substrate#1936); falls away at the re-pin onto the first release that carries both | +| The snapshot takes one rootfs-upper tar per container and one tar per durable volume; the restore creates the layout before it extracts into it, so a snapshot no longer decides which host paths are mounted or deleted; `tarutil` writes and reads the tree through an `os.Root` (`fs.WalkDir` over `root.FS()`, `root.Lstat`, `root.Readlink`), component by component, never by the full host path (`internal/tarutil`, `cmd/ateom-gvisor/durable.go`, `cmd/ateom-microvm/rootfsupper.go`) | a workspace tree deeper than `PATH_MAX` crashed the actor at its checkpoint and lost the session on every harness: the tar walked and stat'ed each entry by its full path, which the kernel refuses with `ENAMETOOLONG`; the same tar named the host paths a restore mounted and deleted (upstream's agent-substrate/substrate#2083; giantswarm/giantswarm#38054; [#37742](https://github.com/giantswarm/giantswarm/issues/37742) row 165). **Snapshots written before it do not resume after it**: the roll needs the snapshot reset, golden and actor snapshots | [#260](https://github.com/giantswarm/substrate/pull/260): `git cherry-pick -x` of upstream's squash commit `893b1edb`, clean | [agent-substrate/substrate#2110](https://github.com/agent-substrate/substrate/pull/2110) (merged 2026-10-06, after the pin). Falls away at the re-pin onto the first release that carries it | +| A snapshot extract creates only `0:0` character devices (overlay whiteouts) and sets only `user.*` xattrs and the path-based overlay attributes (opaque, redirect, impure); `Create` skips other device nodes; on the micro-VM host the merged rootfs overlay and the binds into the virtio-fs share are `nosuid,nodev` (`setNosuidNodev`, recursive) | snapshot tars are extracted as root on the host, and the extract made any device node and set any `trusted.overlay.*` xattr the archive named; a stray device could make a snapshot unrestorable (upstream's agent-substrate/substrate#2090; giantswarm/giantswarm#38054; [#37742](https://github.com/giantswarm/giantswarm/issues/37742) row 166) | [#260](https://github.com/giantswarm/substrate/pull/260): `git cherry-pick -x` of upstream's squash commit `8da76493`. Adapted to the line: it still mounts through `mount(8)`, so `nosuid,nodev` joins the overlay's `-o` options and `BindIntoShare` calls `setNosuidNodev` after the rbind; the hunk's `RemountReadOnly` and `Unmount` tests cover helpers the line does not have and are left out | [agent-substrate/substrate#2111](https://github.com/agent-substrate/substrate/pull/2111) (merged 2026-10-06, after the pin). Falls away at the re-pin onto the first release that carries it | Twenty-three patches change Substrate ahead of upstream — egress for an actor while it resumes, without which no skill-carrying agent of the platform boots, the atelet scheduling knobs, the keep policy on the CRD chart's diff --git a/cmd/ateom-gvisor/durable.go b/cmd/ateom-gvisor/durable.go index ab8bf63ba7..f5fe1e0950 100644 --- a/cmd/ateom-gvisor/durable.go +++ b/cmd/ateom-gvisor/durable.go @@ -18,7 +18,9 @@ package main import ( "context" + "errors" "fmt" + "io/fs" "os" "path/filepath" "strings" @@ -27,10 +29,18 @@ import ( "github.com/agent-substrate/substrate/internal/tarutil" ) -// durableTarFile is the snapshot file holding the tar of the actor's durable-dir -// volumes. Its entries are /... relative to -// ActorDirs.durable_dir_volume_mounts_dir, so extraction restores the same layout. -const durableTarFile = "durable-dir.tar" +// durableTarFile is the snapshot file holding the tar of one durable-dir +// volume: the contents of under +// ActorDirs.durable_dir_volume_mounts_dir, plus a root entry for the volume +// directory's own metadata. The volume directory itself comes from atelet, +// never from the snapshot: runsc bind-mounts it by path, so a symlink planted +// there would expose whatever it points at to the sandbox. +func durableTarFile(volumeName string) (string, error) { + if volumeName == "" || volumeName == "." || strings.Contains(volumeName, "/") || !filepath.IsLocal(volumeName) { + return "", fmt.Errorf("invalid durable-dir volume name %q", volumeName) + } + return "durable-dir-" + volumeName + ".tar", nil +} // hasDurableVolumes reports whether any container mounts a durable-dir volume. func hasDurableVolumes(containers []*ateompb.Container) bool { @@ -42,36 +52,69 @@ func hasDurableVolumes(containers []*ateompb.Container) bool { return false } -// tarDurableVolumes archives the actor's durable-dir volumes (dir) into the -// checkpoint directory. The caller must have paused the guest first. +// tarDurableVolumes archives each durable-dir volume under dir into the +// checkpoint directory, one tar per volume, and returns the file names. The +// caller must have paused the guest first. // // Sockets the workload left behind and gVisor internal files (.gvisor.*) are // skipped rather than archived. -func tarDurableVolumes(ctx context.Context, dir, checkpointDir string) error { +func tarDurableVolumes(ctx context.Context, dir, checkpointDir string, volumes []string) ([]string, error) { skip := func(rel string) bool { base := filepath.Base(rel) return strings.HasPrefix(base, ".gvisor.") } - if err := tarutil.CreateFiltered(ctx, filepath.Join(checkpointDir, durableTarFile), dir, skip); err != nil { - return fmt.Errorf("while archiving durable-dir volumes from %q: %w", dir, err) + var files []string + for _, vol := range volumes { + name, err := durableTarFile(vol) + if err != nil { + return nil, err + } + if err := tarutil.CreateFilteredWithRoot(ctx, filepath.Join(checkpointDir, name), filepath.Join(dir, vol), skip); err != nil { + return nil, fmt.Errorf("while archiving durable-dir volume %q: %w", vol, err) + } + files = append(files, name) } - return nil + return files, nil } -// untarDurableVolumes restores the durable-dir volumes from a snapshot into the -// actor's host directory (dir, which atelet has already created, empty). -func untarDurableVolumes(dir, snapshotDir string) error { - if err := os.MkdirAll(dir, 0o755); err != nil { - return fmt.Errorf("while creating durable-dir volumes dir %q: %w", dir, err) +// untarDurableVolumes restores each durable-dir volume from the snapshot into +// its directory under dir, which atelet has already created, empty. A volume +// with no tar in the snapshot (added to the template since) stays empty. +func untarDurableVolumes(dir, snapshotDir string, volumes []string) error { + for _, vol := range volumes { + name, err := durableTarFile(vol) + if err != nil { + return err + } + tarPath := filepath.Join(snapshotDir, name) + if _, err := os.Stat(tarPath); errors.Is(err, fs.ErrNotExist) { + continue + } + volDir := filepath.Join(dir, vol) + if err := os.MkdirAll(volDir, 0o700); err != nil { + return fmt.Errorf("while creating durable-dir volume dir %q: %w", volDir, err) + } + if err := tarutil.Extract(tarPath, volDir); err != nil { + return fmt.Errorf("while restoring durable-dir volume %q: %w", vol, err) + } + removeGVisorFiles(volDir) } - if err := tarutil.Extract(filepath.Join(snapshotDir, durableTarFile), dir); err != nil { - return fmt.Errorf("while restoring durable-dir volumes into %q: %w", dir, err) + return nil +} + +// removeGVisorFiles deletes gVisor internal files (.gvisor.*) under dir, +// best-effort, through an os.Root so the restored tree's symlinks are never +// followed. +func removeGVisorFiles(dir string) { + root, err := os.OpenRoot(dir) + if err != nil { + return } - _ = filepath.Walk(dir, func(p string, info os.FileInfo, err error) error { - if err == nil && !info.IsDir() && strings.HasPrefix(info.Name(), ".gvisor.") { - _ = os.Remove(p) + defer root.Close() + _ = fs.WalkDir(root.FS(), ".", func(rel string, d fs.DirEntry, err error) error { + if err == nil && !d.IsDir() && strings.HasPrefix(d.Name(), ".gvisor.") { + _ = root.Remove(rel) } return nil }) - return nil } diff --git a/cmd/ateom-gvisor/durable_test.go b/cmd/ateom-gvisor/durable_test.go new file mode 100644 index 0000000000..cf0483542c --- /dev/null +++ b/cmd/ateom-gvisor/durable_test.go @@ -0,0 +1,129 @@ +//go:build linux + +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "os" + "path/filepath" + "slices" + "testing" + + "github.com/agent-substrate/substrate/internal/tarutil" +) + +// mkVolumeDirs creates the per-volume directories atelet prepares. +func mkVolumeDirs(t *testing.T, dir string, vols ...string) { + t.Helper() + for _, v := range vols { + if err := os.MkdirAll(filepath.Join(dir, v), 0o700); err != nil { + t.Fatal(err) + } + } +} + +func TestDurableVolumesRoundTrip(t *testing.T) { + src := t.TempDir() + mkVolumeDirs(t, src, "data", "cache") + for rel, content := range map[string]string{ + "data/notes.txt": "kept", + "data/.gvisor.filestat": "internal", + "cache/c.bin": "cached", + } { + if err := os.WriteFile(filepath.Join(src, rel), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + // The volume root's own mode is the mount point's, so it must survive. + if err := os.Chmod(filepath.Join(src, "data"), 0o750); err != nil { + t.Fatal(err) + } + checkpointDir := t.TempDir() + files, err := tarDurableVolumes(t.Context(), src, checkpointDir, []string{"cache", "data"}) + if err != nil { + t.Fatalf("tarDurableVolumes: %v", err) + } + if want := []string{"durable-dir-cache.tar", "durable-dir-data.tar"}; !slices.Equal(files, want) { + t.Errorf("tarDurableVolumes files = %v, want %v", files, want) + } + + dst := t.TempDir() + mkVolumeDirs(t, dst, "data", "cache") + if err := untarDurableVolumes(dst, checkpointDir, []string{"cache", "data"}); err != nil { + t.Fatalf("untarDurableVolumes: %v", err) + } + for rel, want := range map[string]string{"data/notes.txt": "kept", "cache/c.bin": "cached"} { + if got, err := os.ReadFile(filepath.Join(dst, rel)); err != nil || string(got) != want { + t.Errorf("restored %q = %q, %v; want %q", rel, got, err, want) + } + } + if _, err := os.Lstat(filepath.Join(dst, "data/.gvisor.filestat")); !os.IsNotExist(err) { + t.Errorf(".gvisor.filestat: Lstat = %v; want it skipped", err) + } + if fi, err := os.Stat(filepath.Join(dst, "data")); err != nil || fi.Mode().Perm() != 0o750 { + t.Errorf("restored volume dir: Stat = %v, %v; want mode 0750", fi, err) + } +} + +// runsc bind-mounts / by path, so the snapshot must not be able +// to replace that directory with a symlink out of the actor's tree. +func TestUntarDurableVolumesCannotPlantVolumeDir(t *testing.T) { + victim := t.TempDir() + planted := t.TempDir() + if err := os.Symlink(victim, filepath.Join(planted, "data")); err != nil { + t.Fatal(err) + } + snapshotDir := t.TempDir() + // An entry named after the volume, as the old single-tar layout had. + if err := tarutil.Create(t.Context(), filepath.Join(snapshotDir, "durable-dir-data.tar"), planted); err != nil { + t.Fatal(err) + } + + dst := t.TempDir() + mkVolumeDirs(t, dst, "data") + if err := untarDurableVolumes(dst, snapshotDir, []string{"data"}); err != nil { + t.Fatalf("untarDurableVolumes: %v", err) + } + if fi, err := os.Lstat(filepath.Join(dst, "data")); err != nil || !fi.IsDir() { + t.Errorf("data: Lstat = %v, %v; want a real directory", fi, err) + } + entries, err := os.ReadDir(victim) + if err != nil || len(entries) != 0 { + t.Errorf("victim: ReadDir = %v, %v; want it untouched", entries, err) + } +} + +// A volume added to the template after the snapshot has no tar: it restores +// empty rather than failing. +func TestUntarDurableVolumesMissingTarIsEmpty(t *testing.T) { + dst := t.TempDir() + mkVolumeDirs(t, dst, "new") + if err := untarDurableVolumes(dst, t.TempDir(), []string{"new"}); err != nil { + t.Fatalf("untarDurableVolumes: %v", err) + } + entries, err := os.ReadDir(filepath.Join(dst, "new")) + if err != nil || len(entries) != 0 { + t.Errorf("new: ReadDir = %v, %v; want an empty directory", entries, err) + } +} + +func TestDurableTarFileRejectsBadNames(t *testing.T) { + for _, v := range []string{"", ".", "..", "a/b", "/abs", "../x"} { + if _, err := durableTarFile(v); err == nil { + t.Errorf("durableTarFile(%q) = nil error, want one", v) + } + } +} diff --git a/cmd/ateom-gvisor/main.go b/cmd/ateom-gvisor/main.go index 039298d4f9..69b47d17d5 100644 --- a/cmd/ateom-gvisor/main.go +++ b/cmd/ateom-gvisor/main.go @@ -683,6 +683,8 @@ func (s *AteomService) CheckpointWorkload(ctx context.Context, req *ateompb.Chec return nil, fmt.Errorf("while creating checkpoint directory: %w", err) } + // durableFiles are the durable-dir tars written below: the DATA subset. + var durableFiles []string // Always take durable-dir snapshot if at least one container has a durable-dir volume mount. // TODO(dberkov): this is a temporary workaround until gVisor supports taking durable-dir snapshots in a single request with the process snapshot. switch req.GetScope() { @@ -693,7 +695,8 @@ func (s *AteomService) CheckpointWorkload(ctx context.Context, req *ateompb.Chec if err := rcmd.cmdPause(ctx, ocispec.PauseContainer); err != nil { return nil, fmt.Errorf("while pausing pause container: %w", err) } - tarErr := tarDurableVolumes(ctx, req.GetActorDirs().GetDurableDirVolumeMountsDir(), checkpointPath) + var tarErr error + durableFiles, tarErr = tarDurableVolumes(ctx, req.GetActorDirs().GetDurableDirVolumeMountsDir(), checkpointPath, durableVolumeNames(req.GetSpec())) // Undoing our own pause must not depend on the caller's context: // tarutil does not check ctx, so a deadline expiring mid-tar would // fail the resume instantly and leave the sandbox paused forever. @@ -712,7 +715,9 @@ func (s *AteomService) CheckpointWorkload(ctx context.Context, req *ateompb.Chec return nil, fmt.Errorf("while checkpointing pause: %w", err) } if hasDurableVolumes(req.GetSpec().GetContainers()) { - if err := tarDurableVolumes(ctx, req.GetActorDirs().GetDurableDirVolumeMountsDir(), checkpointPath); err != nil { + var err error + durableFiles, err = tarDurableVolumes(ctx, req.GetActorDirs().GetDurableDirVolumeMountsDir(), checkpointPath, durableVolumeNames(req.GetSpec())) + if err != nil { return nil, fmt.Errorf("while archiving durable-dir volumes: %w", err) } } @@ -739,11 +744,7 @@ func (s *AteomService) CheckpointWorkload(ctx context.Context, req *ateompb.Chec s.actorLogger.EmitLifecycleLog(ctx, "Actor checkpointed", attribution) - resp := &ateompb.CheckpointWorkloadResponse{SnapshotFiles: snapshotFiles} - if slices.Contains(snapshotFiles, durableTarFile) { - resp.DataSnapshotFiles = []string{durableTarFile} - } - return resp, nil + return &ateompb.CheckpointWorkloadResponse{SnapshotFiles: snapshotFiles, DataSnapshotFiles: durableFiles}, nil } // listSnapshotFiles returns the (relative) names of regular files directly under @@ -892,7 +893,7 @@ func (s *AteomService) RestoreWorkload(ctx context.Context, req *ateompb.Restore checkpointDir := req.GetActorDirs().GetRestoreDir() if hasDurableVolumes(req.GetSpec().GetContainers()) { - if err := untarDurableVolumes(req.GetActorDirs().GetDurableDirVolumeMountsDir(), checkpointDir); err != nil { + if err := untarDurableVolumes(req.GetActorDirs().GetDurableDirVolumeMountsDir(), checkpointDir, durableVolumeNames(req.GetSpec())); err != nil { return nil, fmt.Errorf("while restoring durable-dir volumes: %w", err) } } diff --git a/cmd/ateom-microvm/checkpoint.go b/cmd/ateom-microvm/checkpoint.go index f1963d24cd..7c9313ce67 100644 --- a/cmd/ateom-microvm/checkpoint.go +++ b/cmd/ateom-microvm/checkpoint.go @@ -193,7 +193,7 @@ func (s *AteomService) CheckpointWorkload(ctx context.Context, req *ateompb.Chec if scope == ateompb.SnapshotScope_SNAPSHOT_SCOPE_FULL { g.Go(func() error { t := time.Now() - err := tarRootfsUpper(gctx, rootfsUpperDir(actorDirs), checkpointDir) + err := tarRootfsUpper(gctx, rootfsUpperDir(actorDirs), checkpointDir, containerNames(req.GetSpec().GetContainers())) dUpper = time.Since(t) return err }) diff --git a/cmd/ateom-microvm/internal/kata/overlay_linux.go b/cmd/ateom-microvm/internal/kata/overlay_linux.go index 5c6b9c86a8..a7a5f7c614 100644 --- a/cmd/ateom-microvm/internal/kata/overlay_linux.go +++ b/cmd/ateom-microvm/internal/kata/overlay_linux.go @@ -43,6 +43,7 @@ import ( "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/third_party/kata/agentpb" "github.com/agent-substrate/substrate/internal/ocispec" specs "github.com/opencontainers/runtime-spec/specs-go" + "golang.org/x/sys/unix" ) const ( @@ -70,13 +71,30 @@ func VirtiofsdSocketPath(id string) string { return filepath.Join(VMDir(id), "vi // UpperWorkDirs returns the HOST overlay upperdir and workdir for one container // under the actor's rootfs-upper base dir: SIBLING directories, /fs and // /work. Both properties are load-bearing — the kernel requires upperdir -// and workdir on the same filesystem and rejects a nested workdir — and the -// layout is also the snapshot tar's entry layout, so a change here breaks both -// every overlay mount and every existing snapshot. Covered by regression tests. +// and workdir on the same filesystem and rejects a nested workdir. The +// snapshot only carries each upperdir's contents (see +// cmd/ateom-microvm/rootfsupper.go); ateom creates this layout itself. +// Covered by regression tests. func UpperWorkDirs(upperBase, containerID string) (upper, work string) { return filepath.Join(upperBase, containerID, "fs"), filepath.Join(upperBase, containerID, "work") } +// MkdirUpperWorkDirs creates UpperWorkDirs under upperBase, which must exist. +// Only root uses and the workdir, so they are 0700. The upperdir is +// 0755: overlayfs presents its mode as the container's /. +func MkdirUpperWorkDirs(upperBase, containerID string) (upper, work string, err error) { + upper, work = UpperWorkDirs(upperBase, containerID) + for _, d := range []struct { + path string + mode os.FileMode + }{{filepath.Dir(upper), 0o700}, {upper, 0o755}, {work, 0o700}} { + if err := os.Mkdir(d.path, d.mode); err != nil && !errors.Is(err, fs.ErrExist) { + return "", "", fmt.Errorf("creating %q: %w", d.path, err) + } + } + return upper, work, nil +} + // GuestSharedRootfs is the in-guest path the kataShared mount exposes a container's // merged rootfs at. A container with this as Root.Path makes the agent's setup_bundle // bind it to /run/kata-containers//rootfs and run the container there — the @@ -215,7 +233,7 @@ func StageMergedRootfs(ctx context.Context, bundleRootfs, upperBase, restoreID, return fmt.Errorf("StageMergedRootfs: empty container id") } dst := filepath.Join(SharedDir(restoreID), cid, "rootfs") - upper, work := UpperWorkDirs(upperBase, cid) + _, work := UpperWorkDirs(upperBase, cid) // Drop any stale mount first (lazy if busy), then ensure clean mountpoints. if err := reaper.Run(exec.Command("umount", dst)); err != nil { _ = reaper.Run(exec.Command("umount", "-l", dst)) @@ -225,10 +243,12 @@ func StageMergedRootfs(ctx context.Context, bundleRootfs, upperBase, restoreID, if err := os.RemoveAll(work); err != nil { return fmt.Errorf("clearing overlay workdir %q: %w", work, err) } - for _, d := range []string{dst, upper, work} { - if err := os.MkdirAll(d, 0o755); err != nil { - return fmt.Errorf("creating %q: %w", d, err) - } + if err := os.MkdirAll(dst, 0o755); err != nil { + return fmt.Errorf("creating %q: %w", dst, err) + } + upper, work, err := MkdirUpperWorkDirs(upperBase, cid) + if err != nil { + return err } // metacopy=off,index=off: pinned rather than inherited from the host's // overlay module defaults. Both features record file-handle references to @@ -245,8 +265,12 @@ func StageMergedRootfs(ctx context.Context, bundleRootfs, upperBase, restoreID, // snapshot and then deleted — so the durability volatile gives up is // durability we do not use. It refuses to mount over a workdir left dirty by // a previous volatile mount, hence the wipe above. + // nosuid,nodev: the upper is restored from a snapshot, and nothing on the + // host should honor a setuid bit or device node in it. The guest sees the + // tree over virtio-fs under its own mount flags, so this changes nothing + // for it. opts := "lowerdir=" + bundleRootfs + ",upperdir=" + upper + ",workdir=" + work + - ",metacopy=off,index=off,volatile" + ",metacopy=off,index=off,volatile,nosuid,nodev" cmd := exec.CommandContext(ctx, "mount", "-t", "overlay", "overlay", "-o", opts, dst) var stderr strings.Builder cmd.Stderr = &stderr @@ -334,6 +358,20 @@ func BindIntoShare(ctx context.Context, src, id, rel string) error { if err := reaper.Run(cmd); err != nil { return fmt.Errorf("bind-mounting %q into the shared tree at %q: %w (%s)", src, dst, err, strings.TrimSpace(stderr.String())) } + return setNosuidNodev(dst) +} + +// setNosuidNodev adds nosuid and nodev to the mount at dst and every mount +// beneath it, keeping their other flags (a read-only image volume stays +// read-only). Shared trees hold workload-written and snapshot-restored files +// that nothing on the host should honor a setuid bit or device node in; the +// guest sees them over virtio-fs under its own mount flags, so this changes +// nothing for it. +func setNosuidNodev(dst string) error { + attr := &unix.MountAttr{Attr_set: unix.MOUNT_ATTR_NOSUID | unix.MOUNT_ATTR_NODEV} + if err := unix.MountSetattr(unix.AT_FDCWD, dst, unix.AT_RECURSIVE, attr); err != nil { + return fmt.Errorf("setting nosuid,nodev on %q: %w", dst, err) + } return nil } diff --git a/cmd/ateom-microvm/internal/kata/overlay_linux_test.go b/cmd/ateom-microvm/internal/kata/overlay_linux_test.go index b7c435b329..55d005771c 100644 --- a/cmd/ateom-microvm/internal/kata/overlay_linux_test.go +++ b/cmd/ateom-microvm/internal/kata/overlay_linux_test.go @@ -22,6 +22,9 @@ import ( "slices" "strings" "testing" + + "github.com/agent-substrate/substrate/internal/roottest" + "golang.org/x/sys/unix" ) // The container rootfs is untrusted (the image below, the guest's own snapshot @@ -59,9 +62,7 @@ func TestEnsureOCIMountpoints(t *testing.T) { // The kernel requires overlay upperdir and workdir on the same filesystem and // rejects a workdir nested inside (or equal to) upperdir — so they must be -// SIBLINGS under the container's subdirectory of the actor's upper base. The -// layout is also the snapshot tar's entry layout (/fs, /work), so a -// change here breaks every overlay mount AND every existing snapshot. +// SIBLINGS under the container's subdirectory of the actor's upper base. func TestUpperWorkDirsAreSiblings(t *testing.T) { const base = "/var/lib/ateom-gvisor/actors/uid/rootfs-upper" upper, work := UpperWorkDirs(base, "app") @@ -75,10 +76,6 @@ func TestUpperWorkDirsAreSiblings(t *testing.T) { if strings.HasPrefix(work+"/", upper+"/") { t.Errorf("UpperWorkDirs: work %q is nested inside upper %q", work, upper) } - // Tar-layout invariant: entries are /fs and /work. - if upper != filepath.Join(base, "app", "fs") || work != filepath.Join(base, "app", "work") { - t.Errorf("UpperWorkDirs = %q, %q; want the snapshot layout /app/{fs,work}", upper, work) - } } func TestVirtiofsdArgs(t *testing.T) { @@ -100,3 +97,42 @@ func TestVirtiofsdArgs(t *testing.T) { t.Errorf("args %v do not set --migration-on-error guest-error", args) } } + +// setNosuidNodev must add both flags to the bind and the mounts beneath it +// without clearing read-only. +func TestSetNosuidNodev(t *testing.T) { + roottest.Require(t, "mounting requires root") + + src, dst := t.TempDir(), t.TempDir() + if err := os.Mkdir(filepath.Join(src, "sub"), 0o755); err != nil { + t.Fatal(err) + } + if err := unix.Mount("tmpfs", filepath.Join(src, "sub"), "tmpfs", 0, ""); err != nil { + t.Fatalf("mounting nested tmpfs: %v", err) + } + t.Cleanup(func() { _ = unix.Unmount(filepath.Join(src, "sub"), unix.MNT_DETACH) }) + if err := unix.Mount(src, dst, "", unix.MS_BIND|unix.MS_REC, ""); err != nil { + t.Fatalf("bind-mounting: %v", err) + } + t.Cleanup(func() { _ = unix.Unmount(dst, unix.MNT_DETACH) }) + if err := unix.MountSetattr(unix.AT_FDCWD, dst, 0, &unix.MountAttr{Attr_set: unix.MOUNT_ATTR_RDONLY}); err != nil { + t.Fatalf("making bind read-only: %v", err) + } + + if err := setNosuidNodev(dst); err != nil { + t.Fatalf("setNosuidNodev: %v", err) + } + + for path, wantRO := range map[string]bool{dst: true, filepath.Join(dst, "sub"): false} { + var st unix.Statfs_t + if err := unix.Statfs(path, &st); err != nil { + t.Fatalf("statfs %q: %v", path, err) + } + if st.Flags&unix.ST_NOSUID == 0 || st.Flags&unix.ST_NODEV == 0 { + t.Errorf("%q: flags %#x, want nosuid and nodev", path, st.Flags) + } + if gotRO := st.Flags&unix.ST_RDONLY != 0; gotRO != wantRO { + t.Errorf("%q: read-only = %v, want %v", path, gotRO, wantRO) + } + } +} diff --git a/cmd/ateom-microvm/restore.go b/cmd/ateom-microvm/restore.go index 97d6f91e74..9a1b2f214f 100644 --- a/cmd/ateom-microvm/restore.go +++ b/cmd/ateom-microvm/restore.go @@ -201,7 +201,7 @@ func (s *AteomService) RestoreWorkload(ctx context.Context, req *ateompb.Restore // // Each container's rootfs is a host-merged overlay (image lower + host upper). Steps: // rewrite the snapshot config's per-VMDir paths (vsock + serial + fs sockets) to this -// actor's; re-materialize the uppers from rootfs-upper.tar (in the background, +// actor's; re-materialize the uppers from the per-container tars (in the background, // overlapped with bundle preparation) and re-mount the merged trees at the frozen // find-paths paths; start the virtiofsd serving them; rebuild the tap (the snapshot's // virtio-net is fd-backed → fresh net_fds); relaunch CH with --restore (OnDemand), @@ -234,13 +234,10 @@ func (s *AteomService) restoreFullScope(ctx context.Context, p actorBootParams, } tPrep := time.Now() - // Merged-rootfs snapshots carry the upper as rootfs-upper.tar (the tar's - // presence is what says which model the guest expects). Start - // re-materializing the upper contents NOW, in the background: the untar - // scales with the actor's data and is joined right before the host overlay - // mounts need it, so it hides behind the bundle preparation below. Legacy - // guest-tmpfs-upper snapshots have no tar: their upper rides inside the - // restored guest memory and the share presents the bare image instead. + // Full snapshots carry each container's upper as a tar (rootfsUpperTarFile). + // Start re-materializing the upper contents NOW, in the background: the + // untar scales with the actor's data and is joined right before the host + // overlay mounts need it, so it hides behind the bundle preparation below. // // An error return between here and the join MUST drain the goroutine (the // deferred receive below): returning with the untar still writing would let @@ -248,7 +245,7 @@ func (s *AteomService) restoreFullScope(ctx context.Context, p actorBootParams, untarDone := make(chan error, 1) untarJoined := false go func() { - untarDone <- untarRootfsUpper(rootfsUpperDir(p.actorDirs), restoreDir) + untarDone <- untarRootfsUpper(rootfsUpperDir(p.actorDirs), restoreDir, containerNames(p.containers)) }() defer func() { if !untarJoined { diff --git a/cmd/ateom-microvm/rootfsupper.go b/cmd/ateom-microvm/rootfsupper.go index 19c2c5ddf4..82f528a136 100644 --- a/cmd/ateom-microvm/rootfsupper.go +++ b/cmd/ateom-microvm/rootfsupper.go @@ -37,11 +37,13 @@ package main // mounts that use it. // // Snapshots: the upper does not ride in guest memory, so a FULL snapshot -// ships it as a tar (rootfsUpperTarFile), taken while the guest is paused -// (the share is write-through, so a paused guest's completed writes are -// already in the upper). Every FULL snapshot carries one; restoring anything -// older fails at the untar. A DATA snapshot deliberately excludes rootfs state: -// the workload cold-starts on restore. +// ships it as one tar per container (rootfsUpperTarFile), taken while the +// guest is paused (the share is write-through, so a paused guest's completed +// writes are already in the upper). Each tar holds only the contents of that +// container's upperdir: ateom creates the directory layout itself on restore, +// so nothing in the snapshot decides a path ateom later mounts or wipes. A +// DATA snapshot deliberately excludes rootfs state: the workload cold-starts +// on restore. import ( "context" @@ -50,17 +52,32 @@ import ( "path/filepath" "strings" + "github.com/agent-substrate/substrate/cmd/ateom-microvm/internal/kata" "github.com/agent-substrate/substrate/internal/proto/ateompb" "github.com/agent-substrate/substrate/internal/tarutil" ) -// rootfsUpperTarFile is the snapshot file holding the tar of the actor's -// rootfs uppers. Its entries are /fs/... relative to -// rootfsUpperDir — the upperdir half of the layout kata.UpperWorkDirs mounts — -// so extraction restores exactly the tree the merged overlays (and the guest's -// find-paths) expect. The workdir half is deliberately absent (see -// tarRootfsUpper); StageMergedRootfs recreates it at mount. -const rootfsUpperTarFile = "rootfs-upper.tar" +// rootfsUpperTarFile is the snapshot file holding the tar of one container's +// rootfs upperdir (/fs under rootfsUpperDir, see kata.UpperWorkDirs). +// Entries are relative to the upperdir, plus a root entry: overlayfs presents +// the upperdir's own mode, ownership, and xattrs as the container's /. The workdir is never archived: with +// index=off pinned on the mount (see kata.StageMergedRootfs) a restored +// workdir would be inert, and StageMergedRootfs recreates it regardless. +func rootfsUpperTarFile(containerID string) (string, error) { + if containerID == "" || containerID == "." || strings.Contains(containerID, "/") || !filepath.IsLocal(containerID) { + return "", fmt.Errorf("invalid container name %q", containerID) + } + return "rootfs-upper-" + containerID + ".tar", nil +} + +// containerNames returns the names of the actor's containers. +func containerNames(containers []*ateompb.Container) []string { + names := make([]string, len(containers)) + for i, c := range containers { + names[i] = c.GetName() + } + return names +} // resetRootfsUpperDir gives a cold boot a pristine upper directory: a cold // boot must start from the bare image, and atelet's actor-dir reset does not @@ -72,48 +89,56 @@ func resetRootfsUpperDir(actorDirs *ateompb.ActorDirs) error { if err := os.RemoveAll(dir); err != nil { return fmt.Errorf("while clearing rootfs upper dir %q: %w", dir, err) } - if err := os.MkdirAll(dir, 0o755); err != nil { + if err := os.MkdirAll(dir, 0o700); err != nil { return fmt.Errorf("while creating rootfs upper dir %q: %w", dir, err) } return nil } -// tarRootfsUpper archives the actor's rootfs uppers (dir) into the checkpoint -// directory. The caller must have paused the guest first: virtiofsd is -// write-through, so a completed guest write has reached the host overlay's -// upper by then, but a running guest could still add more after the walk. -// -// Each container's overlay WORKDIR (/work) is excluded: with index=off -// pinned on the mount (see kata.StageMergedRootfs) a restored workdir is -// inert — overlayfs wipes and rebuilds it at mount, and StageMergedRootfs -// recreates the directory regardless — so archiving it is dead weight on the -// suspend path. Not always trivial weight, either: a copy-up in flight when -// the guest paused can leave file-sized temp data there. -func tarRootfsUpper(ctx context.Context, dir, checkpointDir string) error { - skipWorkdir := func(rel string) bool { - parts := strings.Split(rel, "/") - return len(parts) == 2 && parts[1] == "work" - } - if err := tarutil.CreateFiltered(ctx, filepath.Join(checkpointDir, rootfsUpperTarFile), dir, skipWorkdir); err != nil { - return fmt.Errorf("while archiving rootfs uppers from %q: %w", dir, err) +// tarRootfsUpper archives each container's rootfs upperdir under dir into the +// checkpoint directory, one tar per container. The caller must have paused the +// guest first: virtiofsd is write-through, so a completed guest write has +// reached the host overlay's upper by then, but a running guest could still +// add more after the walk. +func tarRootfsUpper(ctx context.Context, dir, checkpointDir string, containers []string) error { + for _, cid := range containers { + name, err := rootfsUpperTarFile(cid) + if err != nil { + return err + } + upper, _ := kata.UpperWorkDirs(dir, cid) + if err := tarutil.CreateWithRoot(ctx, filepath.Join(checkpointDir, name), upper); err != nil { + return fmt.Errorf("while archiving rootfs upper %q: %w", upper, err) + } } return nil } -// untarRootfsUpper restores the rootfs uppers from a snapshot into the actor's -// host directory. It must run before the merged overlays are mounted (the -// mounts consume these contents). The directory is recreated from scratch: -// nothing else owns it, and stale contents from a previous activation would -// corrupt the overlay state the guest's find-paths re-opens. -func untarRootfsUpper(dir, snapshotDir string) error { +// untarRootfsUpper restores each container's rootfs upperdir from the snapshot +// into the actor's host directory. It must run before the merged overlays are +// mounted (the mounts consume these contents). The directory is recreated from +// scratch: nothing else owns it, and stale contents from a previous activation +// would corrupt the overlay state the guest's find-paths re-opens. Every +// directory above the upperdirs is created here, never taken from the tar. +func untarRootfsUpper(dir, snapshotDir string, containers []string) error { if err := os.RemoveAll(dir); err != nil { return fmt.Errorf("while clearing rootfs upper dir %q: %w", dir, err) } - if err := os.MkdirAll(dir, 0o755); err != nil { + if err := os.MkdirAll(dir, 0o700); err != nil { return fmt.Errorf("while creating rootfs upper dir %q: %w", dir, err) } - if err := tarutil.Extract(filepath.Join(snapshotDir, rootfsUpperTarFile), dir); err != nil { - return fmt.Errorf("while restoring rootfs uppers into %q: %w", dir, err) + for _, cid := range containers { + name, err := rootfsUpperTarFile(cid) + if err != nil { + return err + } + upper, _, err := kata.MkdirUpperWorkDirs(dir, cid) + if err != nil { + return fmt.Errorf("while creating rootfs upper for %q: %w", cid, err) + } + if err := tarutil.Extract(filepath.Join(snapshotDir, name), upper); err != nil { + return fmt.Errorf("while restoring rootfs upper %q: %w", upper, err) + } } return nil } diff --git a/cmd/ateom-microvm/rootfsupper_test.go b/cmd/ateom-microvm/rootfsupper_test.go index e11334d4cd..79aa2b3f21 100644 --- a/cmd/ateom-microvm/rootfsupper_test.go +++ b/cmd/ateom-microvm/rootfsupper_test.go @@ -21,6 +21,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/agent-substrate/substrate/internal/tarutil" ) // upperDirWith returns a rootfs upper directory laid out the way the host @@ -52,15 +54,20 @@ func TestRootfsUpperRoundTrip(t *testing.T) { "app_ovl/work/#1/tmp.bin": "in-flight copy-up temp", "sidecar_ovl/fs/var/log/s.log": "sidecar write", } + containers := []string{"app_ovl", "sidecar_ovl"} src := upperDirWith(t, files) + // The upperdir's own mode is the container's /, so it must survive too. + if err := os.Chmod(filepath.Join(src, "app_ovl/fs"), 0o750); err != nil { + t.Fatal(err) + } checkpointDir := t.TempDir() - if err := tarRootfsUpper(t.Context(), src, checkpointDir); err != nil { + if err := tarRootfsUpper(t.Context(), src, checkpointDir, containers); err != nil { t.Fatalf("tarRootfsUpper: %v", err) } // Restore: onto a directory holding a stale previous activation's contents, // which must not leak into the restored overlay state. dst := upperDirWith(t, map[string]string{"app_ovl/fs/stale.txt": "stale"}) - if err := untarRootfsUpper(dst, checkpointDir); err != nil { + if err := untarRootfsUpper(dst, checkpointDir, containers); err != nil { t.Fatalf("untarRootfsUpper: %v", err) } for rel, want := range files { @@ -76,12 +83,87 @@ func TestRootfsUpperRoundTrip(t *testing.T) { t.Errorf("restored %q = %q, want %q", rel, got, want) } } - // The workdirs must NOT survive the round trip: they are excluded from the - // archive (dead weight; overlayfs rebuilds them at mount). - if _, err := os.Stat(filepath.Join(dst, "app_ovl/work")); !os.IsNotExist(err) { - t.Errorf("workdir survived the snapshot round trip (stat err = %v), want it excluded", err) + if fi, err := os.Stat(filepath.Join(dst, "app_ovl/fs")); err != nil || fi.Mode().Perm() != 0o750 { + t.Errorf("restored upperdir: Stat = %v, %v; want mode 0750", fi, err) + } + // The workdirs' contents must NOT survive the round trip: they are excluded + // from the archive (dead weight; overlayfs rebuilds them at mount). + if entries, err := os.ReadDir(filepath.Join(dst, "app_ovl/work")); err != nil || len(entries) != 0 { + t.Errorf("workdir after the snapshot round trip: ReadDir = %v, %v; want an empty directory", entries, err) } if _, err := os.Stat(filepath.Join(dst, "app_ovl/fs/stale.txt")); !os.IsNotExist(err) { t.Errorf("stale pre-restore content survived untarRootfsUpper (stat err = %v), want it wiped", err) } } + +// The snapshot is untrusted, so it must not decide any directory above the +// upperdirs: a symlink there would point the overlay mount, or the workdir +// wipe in kata.StageMergedRootfs, outside the actor's directory. +func TestUntarRootfsUpperCannotPlantLayout(t *testing.T) { + victim := t.TempDir() + // A symlink at the tar's root has no name to land on, and one at "fs" + // would only land inside the upperdir as guest content. + planted := upperDirWith(t, nil) + for _, link := range []string{"fs", "work"} { + if err := os.Symlink(victim, filepath.Join(planted, link)); err != nil { + t.Fatal(err) + } + } + snapshotDir := t.TempDir() + if err := tarutil.Create(t.Context(), filepath.Join(snapshotDir, "rootfs-upper-app.tar"), planted); err != nil { + t.Fatal(err) + } + + dst := t.TempDir() + if err := untarRootfsUpper(dst, snapshotDir, []string{"app"}); err != nil { + t.Fatalf("untarRootfsUpper: %v", err) + } + if fi, err := os.Lstat(filepath.Join(dst, "app")); err != nil || !fi.IsDir() { + t.Errorf("app: Lstat = %v, %v; want a real directory", fi, err) + } + if fi, err := os.Lstat(filepath.Join(dst, "app", "fs")); err != nil || !fi.IsDir() { + t.Errorf("app/fs: Lstat = %v, %v; want a real directory", fi, err) + } + if fi, err := os.Lstat(filepath.Join(dst, "app", "work")); err != nil || !fi.IsDir() { + t.Errorf("app/work: Lstat = %v, %v; want a real directory", fi, err) + } +} + +// Only root uses the layout above the upperdir, and the workdir; the upperdir +// is the container's / and must stay traversable. +func TestUntarRootfsUpperModes(t *testing.T) { + src := upperDirWith(t, map[string]string{"f": "x"}) + snapshotDir := t.TempDir() + if err := tarutil.Create(t.Context(), filepath.Join(snapshotDir, "rootfs-upper-app.tar"), src); err != nil { + t.Fatal(err) + } + dst := filepath.Join(t.TempDir(), "upper") + if err := untarRootfsUpper(dst, snapshotDir, []string{"app"}); err != nil { + t.Fatalf("untarRootfsUpper: %v", err) + } + for rel, want := range map[string]os.FileMode{".": 0o700, "app": 0o700, "app/fs": 0o755, "app/work": 0o700} { + fi, err := os.Stat(filepath.Join(dst, rel)) + if err != nil || fi.Mode().Perm() != want { + t.Errorf("%s: Stat = %v, %v; want mode %v", rel, fi, err, want) + } + } +} + +func TestRootfsUpperTarFileRejectsBadNames(t *testing.T) { + for _, cid := range []string{"", ".", "..", "a/b", "/abs", "../x"} { + if _, err := rootfsUpperTarFile(cid); err == nil { + t.Errorf("rootfsUpperTarFile(%q) = nil error, want one", cid) + } + } + if err := untarRootfsUpper(t.TempDir(), t.TempDir(), []string{"../x"}); err == nil { + t.Error("untarRootfsUpper with container ../x = nil error, want one") + } +} + +// A container in the request without a tar in the snapshot fails the restore +// rather than starting it on a silently empty rootfs upper. +func TestUntarRootfsUpperMissingTar(t *testing.T) { + if err := untarRootfsUpper(t.TempDir(), t.TempDir(), []string{"app"}); err == nil { + t.Error("untarRootfsUpper with no tar = nil error, want one") + } +} diff --git a/internal/tarutil/tarutil.go b/internal/tarutil/tarutil.go index 0a89ad746b..0dee7b7acc 100644 --- a/internal/tarutil/tarutil.go +++ b/internal/tarutil/tarutil.go @@ -16,8 +16,8 @@ // Package tarutil archives and restores a directory tree as a tar file, // preserving the metadata a workload's data directory depends on: modes, -// ownership, modification times, symlinks, hardlinks, FIFOs, device nodes, -// and user.* / trusted.overlay.* extended attributes (as PAX SCHILY.xattr +// ownership, modification times, symlinks, hardlinks, FIFOs, overlay +// whiteouts, and user.* / overlay extended attributes (as PAX SCHILY.xattr // records). // // It exists for snapshotting durable-dir volumes and rootfs overlay uppers @@ -30,7 +30,9 @@ // resurrects deleted content after a resume. // // Extraction is confined to the destination with os.Root, so a crafted archive -// cannot write outside it via "..", an absolute path, or a symlink. +// cannot write outside it via "..", an absolute path, or a symlink. It also +// refuses device nodes other than whiteouts and restores only the xattrs +// keepXattr allows. package tarutil import ( @@ -87,10 +89,11 @@ type readerOnly struct{ io.Reader } // relative to srcDir, so extracting into another directory reproduces the tree. // srcDir itself is not an entry. // -// Regular files, directories, symlinks, FIFOs, and device nodes are archived -// with their mode, ownership, modification time, and user.* / trusted.overlay.* -// xattrs. A file with multiple links inside srcDir is archived once and -// referenced as a hardlink thereafter. Sockets are skipped (see writeTree). +// Regular files, directories, symlinks, FIFOs, and overlay whiteouts are +// archived with their mode, ownership, modification time, and the xattrs +// keepXattr allows. A file with multiple links inside srcDir is archived once +// and referenced as a hardlink thereafter. Sockets and other device nodes are +// skipped (see writeTree). func Create(ctx context.Context, tarPath, srcDir string) error { return CreateFiltered(ctx, tarPath, srcDir, nil) } @@ -103,6 +106,22 @@ type SkipFunc func(rel string) bool // CreateFiltered is Create with entries omitted where skip returns true. A nil // skip archives everything. func CreateFiltered(ctx context.Context, tarPath, srcDir string, skip SkipFunc) error { + return create(ctx, tarPath, srcDir, skip, false) +} + +// CreateWithRoot is Create plus a "./" entry carrying srcDir's own mode, +// ownership, modification time, and xattrs, which Extract applies to dstDir. +func CreateWithRoot(ctx context.Context, tarPath, srcDir string) error { + return CreateFilteredWithRoot(ctx, tarPath, srcDir, nil) +} + +// CreateFilteredWithRoot is CreateWithRoot with entries omitted where skip +// returns true. The root entry is never skipped. +func CreateFilteredWithRoot(ctx context.Context, tarPath, srcDir string, skip SkipFunc) error { + return create(ctx, tarPath, srcDir, skip, true) +} + +func create(ctx context.Context, tarPath, srcDir string, skip SkipFunc, includeRoot bool) error { f, err := os.Create(tarPath) if err != nil { return fmt.Errorf("creating tar %q: %w", tarPath, err) @@ -116,7 +135,7 @@ func CreateFiltered(ctx context.Context, tarPath, srcDir string, skip SkipFunc) tarWriterPool.Put(bw) }() tw := tar.NewWriter(bw) - if err := writeTree(ctx, tw, srcDir, skip); err != nil { + if err := writeTree(ctx, tw, srcDir, skip, includeRoot); err != nil { return err } if err := tw.Close(); err != nil { @@ -135,34 +154,42 @@ func CreateFiltered(ctx context.Context, tarPath, srcDir string, skip SkipFunc) return nil } -// writeTree walks srcDir in lexical order (filepath.WalkDir) and writes one -// entry per path, omitting entries (and, for directories, subtrees) that skip -// selects. The deterministic order keeps archives of identical trees -// byte-comparable, which makes snapshot diffs meaningful. -func writeTree(ctx context.Context, tw *tar.Writer, srcDir string, skip SkipFunc) error { +// writeTree walks srcDir in lexical order and writes one entry per path, +// omitting entries (and, for directories, subtrees) that skip selects. srcDir +// itself is written as "./" only if includeRoot is set. The deterministic +// order keeps archives of identical trees byte-comparable, which makes +// snapshot diffs meaningful. +// +// Every read goes through an os.Root rooted at srcDir, so an entry swapped for +// a symlink mid-walk cannot make the archive capture a file outside srcDir. +func writeTree(ctx context.Context, tw *tar.Writer, srcDir string, skip SkipFunc, includeRoot bool) error { + root, err := os.OpenRoot(srcDir) + if err != nil { + return fmt.Errorf("opening %q: %w", srcDir, err) + } + defer root.Close() + // Maps an already-archived multi-link inode to the name it was archived // under, so later links become tar hardlink entries instead of copies. linked := map[inodeKey]string{} - return filepath.WalkDir(srcDir, func(path string, d fs.DirEntry, err error) error { - if err != nil { - return err - } - rel, err := filepath.Rel(srcDir, path) + return fs.WalkDir(root.FS(), ".", func(rel string, d fs.DirEntry, err error) error { if err != nil { return err } - if rel == "." { + if rel == "." && !includeRoot { return nil } - if skip != nil && skip(filepath.ToSlash(rel)) { + if rel != "." && skip != nil && skip(rel) { if d.IsDir() { return filepath.SkipDir } return nil } + path := filepath.Join(srcDir, rel) - info, err := d.Info() + // Not d.Info(): that re-stats by full path, outside the root. + info, err := root.Lstat(rel) if err != nil { return err } @@ -184,7 +211,7 @@ func writeTree(ctx context.Context, tw *tar.Writer, srcDir string, skip SkipFunc link := "" if info.Mode()&os.ModeSymlink != 0 { - if link, err = os.Readlink(path); err != nil { + if link, err = root.Readlink(rel); err != nil { return fmt.Errorf("reading symlink %q: %w", path, err) } } @@ -192,8 +219,8 @@ func writeTree(ctx context.Context, tw *tar.Writer, srcDir string, skip SkipFunc if err != nil { return fmt.Errorf("building tar header for %q: %w", path, err) } - hdr.Name = filepath.ToSlash(rel) - if d.IsDir() { + hdr.Name = rel + if info.IsDir() { hdr.Name += "/" } setOwner(hdr, info) @@ -205,7 +232,7 @@ func writeTree(ctx context.Context, tw *tar.Writer, srcDir string, skip SkipFunc // Symlinks are exempt: Linux refuses user.* xattrs on them, and // overlay metadata never targets them. if info.Mode()&os.ModeSymlink == 0 { - xattrs, err := readOverlayXattrs(path) + xattrs, err := readOverlayXattrs(root, rel) if err != nil { return fmt.Errorf("reading xattrs of %q: %w", path, err) } @@ -230,17 +257,31 @@ func writeTree(ctx context.Context, tw *tar.Writer, srcDir string, skip SkipFunc } linked[key] = hdr.Name } + in, err := openRegular(root, rel) + if err != nil { + return fmt.Errorf("opening %q: %w", path, err) + } + defer in.Close() if err := tw.WriteHeader(hdr); err != nil { return fmt.Errorf("writing tar header for %q: %w", path, err) } - return copyFileInto(tw, path) - - case d.IsDir(), info.Mode()&os.ModeSymlink != 0, info.Mode()&os.ModeNamedPipe != 0, - info.Mode()&os.ModeDevice != 0: - // FileInfoHeader already populated the Typeflag (and, for devices, - // the major/minor) with size 0. Devices are archived rather than - // rejected because an overlay upper legitimately contains them: - // every deleted lower-layer file is a 0:0 char-device whiteout. + if _, err := copyPooled(tw, in); err != nil { + return fmt.Errorf("archiving contents of %q: %w", path, err) + } + return nil + + case info.Mode()&os.ModeDevice != 0: + // Only overlay whiteouts are archived; Extract refuses any other + // device, so archiving one would make the snapshot unrestorable. + if !isWhiteout(hdr) { + slog.WarnContext(ctx, "Skipping device node while archiving directory", + slog.String("path", path), slog.String("root", srcDir)) + return nil + } + return tw.WriteHeader(hdr) + + case info.IsDir(), info.Mode()&os.ModeSymlink != 0, info.Mode()&os.ModeNamedPipe != 0: + // FileInfoHeader already populated the Typeflag with size 0. return tw.WriteHeader(hdr) default: @@ -249,17 +290,24 @@ func writeTree(ctx context.Context, tw *tar.Writer, srcDir string, skip SkipFunc }) } -// copyFileInto streams path's contents into the archive. -func copyFileInto(tw *tar.Writer, path string) error { - in, err := os.Open(path) +// openRegular opens name under root for reading without following a symlink +// at it, and fails unless it is still a regular file. O_NONBLOCK keeps a FIFO +// swapped in since the walk from blocking the open. +func openRegular(root *os.Root, name string) (*os.File, error) { + f, err := root.OpenFile(name, os.O_RDONLY|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0) if err != nil { - return fmt.Errorf("opening %q: %w", path, err) + return nil, err } - defer in.Close() - if _, err := copyPooled(tw, in); err != nil { - return fmt.Errorf("archiving contents of %q: %w", path, err) + fi, err := f.Stat() + if err != nil { + f.Close() + return nil, err } - return nil + if !fi.Mode().IsRegular() { + f.Close() + return nil, fmt.Errorf("no longer a regular file (%v)", fi.Mode().Type()) + } + return f, nil } // Extract unpacks tarPath into dstDir, which must already exist. Modes, @@ -310,6 +358,10 @@ func Extract(tarPath, dstDir string) error { return fmt.Errorf("invalid entry in tar %q: %w", tarPath, err) } if skip { + // A "./" entry (see CreateWithRoot) carries dstDir's own metadata. + if hdr.Name != "" && hdr.Typeflag == tar.TypeDir { + dirs["."] = hdr + } continue } if err := extractEntry(root, tr, hdr, name, dirs); err != nil { @@ -383,6 +435,13 @@ func extractEntry(root *os.Root, tr *tar.Reader, hdr *tar.Header, name string, d return nil case tar.TypeChar, tar.TypeBlock: + // A real device node would hand whoever can reach the extracted tree + // access to host hardware. Overlay whiteouts are the only devices a + // snapshot legitimately carries. + if !isWhiteout(hdr) { + return fmt.Errorf("refusing device node %q (%d:%d): only 0:0 character devices (overlay whiteouts) are allowed", + name, hdr.Devmajor, hdr.Devminor) + } if err := replaceExisting(root, name); err != nil { return err } @@ -396,6 +455,12 @@ func extractEntry(root *os.Root, tr *tar.Reader, hdr *tar.Header, name string, d } } +// isWhiteout reports whether hdr is an overlayfs whiteout: a 0:0 character +// device. +func isWhiteout(hdr *tar.Header) bool { + return hdr.Typeflag == tar.TypeChar && hdr.Devmajor == 0 && hdr.Devminor == 0 +} + // replaceExisting removes whatever currently occupies name so the new entry // does not write through a symlink, truncate a shared inode, or collide with a // directory. @@ -415,13 +480,18 @@ func replaceExisting(root *os.Root, name string) error { // restoreDirMeta applies the archived modes, ownership, and times to extracted // directories, deepest first: a directory's path is always longer than its // parent's, so length-descending order restores children before the parent's -// mode can make them unreachable. +// mode can make them unreachable. The root ("."), if archived, goes last. func restoreDirMeta(root *os.Root, dirs map[string]*tar.Header) error { names := make([]string, 0, len(dirs)) for name := range dirs { names = append(names, name) } - sort.Slice(names, func(i, j int) bool { return len(names[i]) > len(names[j]) }) + sort.Slice(names, func(i, j int) bool { + if names[i] == "." || names[j] == "." { + return names[j] == "." + } + return len(names[i]) > len(names[j]) + }) for _, name := range names { if err := restoreMeta(root, name, dirs[name]); err != nil { return err @@ -462,9 +532,8 @@ func restoreMeta(root *os.Root, name string, hdr *tar.Header) error { return restoreOverlayXattrs(root, name, hdr) } -// restoreOverlayXattrs re-applies the user.* and trusted.overlay.* xattrs -// recorded in the entry's PAX records (writeTree's SCHILY.xattr.* — the -// overlayfs deletion metadata plus workload-owned user attrs). Writing +// restoreOverlayXattrs re-applies the xattrs recorded in the entry's PAX +// records (writeTree's SCHILY.xattr.*) that keepXattr allows. Writing // trusted.* requires CAP_SYS_ADMIN; extraction runs as root in ateom, and the // tests gate on it. // @@ -479,13 +548,14 @@ func restoreMeta(root *os.Root, name string, hdr *tar.Header) error { func restoreOverlayXattrs(root *os.Root, name string, hdr *tar.Header) error { var attrs map[string]string for k, v := range hdr.PAXRecords { - if strings.HasPrefix(k, "SCHILY.xattr.user.") || - strings.HasPrefix(k, "SCHILY.xattr.trusted.overlay.") { - if attrs == nil { - attrs = map[string]string{} - } - attrs[strings.TrimPrefix(k, "SCHILY.xattr.")] = v + attr, ok := strings.CutPrefix(k, "SCHILY.xattr.") + if !ok || !keepXattr(attr) { + continue + } + if attrs == nil { + attrs = map[string]string{} } + attrs[attr] = v } if len(attrs) == 0 { return nil @@ -508,13 +578,44 @@ func restoreOverlayXattrs(root *os.Root, name string, hdr *tar.Header) error { return nil } -// readOverlayXattrs returns path's user.* and trusted.overlay.* extended -// attributes. Filesystems without xattr support report none rather than -// failing: tarutil archives arbitrary workload trees, and only those two -// namespaces carry state the round trip must preserve (trusted.overlay.* is -// the host kernel's overlay deletion metadata; reading it requires root, -// which archiving in ateom always has). -func readOverlayXattrs(path string) (map[string]string, error) { +// overlayXattrs are the trusted.overlay.* attributes the round trip keeps: the +// path-based ones the rootfs overlay mount writes to its upper (see +// kata.StageMergedRootfs). The rest are either file handles that go stale on +// another node (origin, upper) or belong to features the mount turns off +// (metacopy, index), and the kernel trusts all of them when it mounts the +// upper, so a crafted archive must not be able to plant them. +var overlayXattrs = map[string]bool{ + "trusted.overlay.opaque": true, + "trusted.overlay.redirect": true, + "trusted.overlay.impure": true, +} + +// keepXattr reports whether attr is archived and restored: any user.* attribute +// (workload data, which the workload can set itself) or one of overlayXattrs. +// Everything else, notably security.capability, is dropped. +func keepXattr(attr string) bool { + return strings.HasPrefix(attr, "user.") || overlayXattrs[attr] +} + +// readOverlayXattrs returns the extended attributes of name under root that +// keepXattr allows. Filesystems without xattr support report none rather than +// failing: tarutil archives arbitrary workload trees. Reading trusted.* +// requires root, which archiving in ateom always has. +// +// name is addressed through its parent directory opened via root, like +// restoreOverlayXattrs, and the L* calls do not follow a final symlink. +func readOverlayXattrs(root *os.Root, name string) (map[string]string, error) { + dir, base := filepath.Split(name) + if dir == "" { + dir = "." + } + parent, err := root.Open(filepath.Clean(dir)) + if err != nil { + return nil, fmt.Errorf("opening parent directory of %q: %w", name, err) + } + defer parent.Close() + path := fmt.Sprintf("/proc/self/fd/%d/%s", parent.Fd(), base) + sz, err := unix.Llistxattr(path, nil) if err != nil { if errors.Is(err, unix.ENOTSUP) || errors.Is(err, unix.EOPNOTSUPP) { @@ -532,7 +633,7 @@ func readOverlayXattrs(path string) (map[string]string, error) { var attrs map[string]string for _, attr := range strings.Split(string(buf[:sz]), "\x00") { - if !strings.HasPrefix(attr, "user.") && !strings.HasPrefix(attr, "trusted.overlay.") { + if !keepXattr(attr) { continue } vsz, err := unix.Lgetxattr(path, attr, nil) diff --git a/internal/tarutil/tarutil_test.go b/internal/tarutil/tarutil_test.go index f85447ccfe..aefd9640fd 100644 --- a/internal/tarutil/tarutil_test.go +++ b/internal/tarutil/tarutil_test.go @@ -18,6 +18,7 @@ package tarutil import ( "archive/tar" + "context" "errors" "net" "os" @@ -637,20 +638,226 @@ func TestExtractIntoPrecreatedVolumeDir(t *testing.T) { } } -func TestExtractSupportsDeviceEntry(t *testing.T) { +// A crafted archive must not be able to create a usable device node: only the +// 0:0 whiteout is allowed. +func TestExtractRejectsDeviceNodes(t *testing.T) { + for _, hdr := range []tar.Header{ + {Name: "null", Typeflag: tar.TypeChar, Devmajor: 1, Devminor: 3}, + {Name: "mem", Typeflag: tar.TypeChar, Devmajor: 1, Devminor: 1, Mode: 0o666}, + {Name: "sda", Typeflag: tar.TypeBlock, Devmajor: 8, Devminor: 0}, + {Name: "blk0", Typeflag: tar.TypeBlock}, + } { + t.Run(hdr.Name, func(t *testing.T) { + tarPath := filepath.Join(t.TempDir(), "dev.tar") + writeTar(t, tarPath, hdr) + dst := t.TempDir() + if err := Extract(tarPath, dst); err == nil { + t.Fatal("Extract succeeded, want an error") + } + if _, err := os.Lstat(filepath.Join(dst, hdr.Name)); !errors.Is(err, os.ErrNotExist) { + t.Errorf("device node %q exists after a refused extract (err = %v)", hdr.Name, err) + } + }) + } +} + +// Create skips devices Extract would refuse, so a stray one in the tree does +// not make the snapshot unrestorable. +func TestCreateSkipsNonWhiteoutDevices(t *testing.T) { roottest.Require(t, "creating a device node requires root") + src := t.TempDir() + if err := unix.Mknod(filepath.Join(src, "null"), unix.S_IFCHR|0o666, int(unix.Mkdev(1, 3))); err != nil { + t.Fatalf("creating device node: %v", err) + } + if err := unix.Mknod(filepath.Join(src, "whiteout"), unix.S_IFCHR, int(unix.Mkdev(0, 0))); err != nil { + t.Fatalf("creating whiteout: %v", err) + } tarPath := filepath.Join(t.TempDir(), "dev.tar") - writeTar(t, tarPath, tar.Header{Name: "null", Typeflag: tar.TypeChar, Devmajor: 1, Devminor: 3}) + if err := Create(t.Context(), tarPath, src); err != nil { + t.Fatalf("Create: %v", err) + } dst := t.TempDir() if err := Extract(tarPath, dst); err != nil { - t.Fatalf("Extract failed on a device entry: %v", err) + t.Fatalf("Extract: %v", err) + } + if _, err := os.Lstat(filepath.Join(dst, "null")); !errors.Is(err, os.ErrNotExist) { + t.Errorf("non-whiteout device was archived (err = %v)", err) } - st, err := os.Lstat(filepath.Join(dst, "null")) + if _, err := os.Lstat(filepath.Join(dst, "whiteout")); err != nil { + t.Errorf("whiteout was not archived: %v", err) + } +} + +// Extract restores only allowlisted xattrs. security.capability would grant +// file capabilities, and the other trusted.overlay.* attributes are trusted by +// the kernel when the upper is mounted. +func TestExtractDropsDisallowedXattrs(t *testing.T) { + roottest.Require(t, "trusted.* and security.* xattrs require root") + + tarPath := filepath.Join(t.TempDir(), "xattr.tar") + writeTar(t, tarPath, tar.Header{ + Name: "f", Typeflag: tar.TypeReg, Mode: 0o755, Size: 1, + PAXRecords: map[string]string{ + "SCHILY.xattr.security.capability": "\x01\x00\x00\x02\xff\xff\xff\xff\x00\x00\x00\x00\xff\xff\xff\xff\x00\x00\x00\x00", + "SCHILY.xattr.trusted.overlay.origin": "x", + "SCHILY.xattr.trusted.overlay.metacopy": "", + "SCHILY.xattr.trusted.other": "x", + "SCHILY.xattr.trusted.overlay.opaque": "y", + "SCHILY.xattr.user.custom": "val", + }, + }) + dst := t.TempDir() + if err := Extract(tarPath, dst); err != nil { + if errors.Is(err, unix.ENOTSUP) { + t.Skipf("filesystem does not support trusted xattrs: %v", err) + } + t.Fatalf("Extract: %v", err) + } + path := filepath.Join(dst, "f") + for _, attr := range []string{"security.capability", "trusted.overlay.origin", "trusted.overlay.metacopy", "trusted.other"} { + if _, err := unix.Lgetxattr(path, attr, nil); !errors.Is(err, unix.ENODATA) { + t.Errorf("%s: Lgetxattr err = %v, want ENODATA", attr, err) + } + } + for _, attr := range []string{"trusted.overlay.opaque", "user.custom"} { + if _, err := unix.Lgetxattr(path, attr, nil); err != nil { + t.Errorf("%s was not restored: %v", attr, err) + } + } +} + +// CreateWithRoot carries srcDir's own metadata, applied to dstDir; Create +// leaves dstDir's alone. +func TestCreateWithRootRestoresRootMeta(t *testing.T) { + src := t.TempDir() + if err := os.WriteFile(filepath.Join(src, "f"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + if err := unix.Lsetxattr(src, "user.root-attr", []byte("v"), 0); err != nil { + t.Skipf("filesystem does not support user xattrs: %v", err) + } + if err := os.Chmod(src, 0o750|os.ModeSetgid); err != nil { + t.Fatal(err) + } + + for _, tc := range []struct { + name string + create func(ctx context.Context, tarPath, srcDir string) error + wantMode os.FileMode + wantAttr bool + }{ + {"CreateWithRoot", CreateWithRoot, 0o750 | os.ModeSetgid, true}, + {"Create", Create, 0o755, false}, + } { + t.Run(tc.name, func(t *testing.T) { + tarPath := filepath.Join(t.TempDir(), "root.tar") + if err := tc.create(t.Context(), tarPath, src); err != nil { + t.Fatalf("create: %v", err) + } + dst := filepath.Join(t.TempDir(), "dst") + if err := os.Mkdir(dst, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chmod(dst, 0o755); err != nil { + t.Fatal(err) + } + if err := Extract(tarPath, dst); err != nil { + t.Fatalf("Extract: %v", err) + } + fi, err := os.Stat(dst) + if err != nil { + t.Fatal(err) + } + if got := fi.Mode() & (os.ModePerm | os.ModeSetgid); got != tc.wantMode { + t.Errorf("dst mode = %v, want %v", got, tc.wantMode) + } + _, err = unix.Lgetxattr(dst, "user.root-attr", make([]byte, 8)) + if gotAttr := err == nil; gotAttr != tc.wantAttr { + t.Errorf("dst has user.root-attr = %v (err %v), want %v", gotAttr, err, tc.wantAttr) + } + if b, err := os.ReadFile(filepath.Join(dst, "f")); err != nil || string(b) != "x" { + t.Errorf("f = %q, %v; want %q", b, err, "x") + } + }) + } +} + +// Only a directory "./" entry is root metadata: anything else at the root is +// skipped, never created in place of dstDir. +func TestExtractIgnoresNonDirRootEntry(t *testing.T) { + tarPath := filepath.Join(t.TempDir(), "root-symlink.tar") + f, err := os.Create(tarPath) if err != nil { - t.Fatalf("stat extracted device node: %v", err) + t.Fatal(err) } - if st.Mode()&os.ModeCharDevice == 0 { - t.Errorf("extracted node mode = %v, want a character device", st.Mode()) + tw := tar.NewWriter(f) + if err := tw.WriteHeader(&tar.Header{Name: "./", Typeflag: tar.TypeSymlink, Linkname: "/"}); err != nil { + t.Fatal(err) + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + dst := t.TempDir() + if err := Extract(tarPath, dst); err != nil { + t.Fatalf("Extract: %v", err) + } + if fi, err := os.Lstat(dst); err != nil || !fi.IsDir() { + t.Errorf("dst: Lstat = %v, %v; want it still a directory", fi, err) + } +} + +// A directory swapped for a symlink out of srcDir mid-walk must not pull the +// outside directory's contents into the archive. +func TestCreateDoesNotFollowSwappedDir(t *testing.T) { + base := t.TempDir() + outside := filepath.Join(base, "outside") + if err := os.Mkdir(outside, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(outside, "secret"), []byte("host data"), 0o600); err != nil { + t.Fatal(err) + } + src := filepath.Join(base, "src") + if err := os.MkdirAll(filepath.Join(src, "d"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(src, "d", "f"), []byte("ok"), 0o644); err != nil { + t.Fatal(err) + } + + // The skip callback runs after the walk listed "d" as a directory and + // before it is read: swap it for a symlink to outside there. + swap := func(rel string) bool { + if rel == "d" { + if err := os.RemoveAll(filepath.Join(src, "d")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(src, "d")); err != nil { + t.Fatal(err) + } + } + return false + } + tarPath := filepath.Join(t.TempDir(), "swap.tar") + _ = CreateFiltered(t.Context(), tarPath, src, swap) + + f, err := os.Open(tarPath) + if err != nil { + t.Fatal(err) + } + defer f.Close() + tr := tar.NewReader(f) + for { + hdr, err := tr.Next() + if err != nil { + break + } + if strings.HasSuffix(hdr.Name, "secret") { + t.Errorf("archive captured %q from outside srcDir", hdr.Name) + } } }