From 653c3ac6048b3dda630561133df0847bcaf69498 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 22 Sep 2026 15:30:03 +0300 Subject: [PATCH 1/3] Expose strict VirtioFS ownership preparation --- README.md | 12 ++ docs/ARCHITECTURE.md | 14 +- docs/MACOS.md | 70 ++++++++ docs/RELEASE_NOTES.md | 11 ++ docs/SECURITY.md | 37 +++-- internal/xattr/walk.go | 60 +------ internal/xattr/walk_test.go | 13 +- microvm.go | 66 ++++---- microvm_test.go | 6 +- microvm_virtiofs_test.go | 179 ++++++++++++++++++++ options.go | 23 ++- virtiofs/doc.go | 8 + virtiofs/example_test.go | 19 +++ virtiofs/noattr_darwin.go | 14 ++ virtiofs/noattr_linux.go | 14 ++ virtiofs/prepare_linux_test.go | 52 ++++++ virtiofs/prepare_other.go | 17 ++ virtiofs/prepare_unix.go | 251 ++++++++++++++++++++++++++++ virtiofs/prepare_unix_test.go | 290 +++++++++++++++++++++++++++++++++ 19 files changed, 1041 insertions(+), 115 deletions(-) create mode 100644 docs/RELEASE_NOTES.md create mode 100644 microvm_virtiofs_test.go create mode 100644 virtiofs/doc.go create mode 100644 virtiofs/example_test.go create mode 100644 virtiofs/noattr_darwin.go create mode 100644 virtiofs/noattr_linux.go create mode 100644 virtiofs/prepare_linux_test.go create mode 100644 virtiofs/prepare_other.go create mode 100644 virtiofs/prepare_unix.go create mode 100644 virtiofs/prepare_unix_test.go diff --git a/README.md b/README.md index ef80c94..64e95c5 100644 --- a/README.md +++ b/README.md @@ -16,6 +16,7 @@ Quick Start · Architecture · Docs · + Release Notes · Contributing · License

@@ -165,6 +166,16 @@ layer extraction, rootfs caching, networking setup, subprocess spawn, and post-boot hooks. It returns a `*VM` handle that you use to query status, stop, or remove the VM. +Virtio-fs ownership overrides preserve host ownership and mode. Writable mounts +with `OverrideUID` are prepared strictly before startup; read-only mounts can be +prepared explicitly when the host OS permits the required xattr operation. +Preparation reports path-specific errors, confines traversal beneath the selected +root, and supports targeted updates after startup. An unprivileged caller cannot +normally annotate an unannotated `0400` file; the permission error is actionable +and does not weaken the backing file. See +[macOS support](docs/MACOS.md#virtio-fs-shared-directory-ownership) and +[release notes](docs/RELEASE_NOTES.md). + ## Advanced Usage For appliance-style deployments, go-microvm exposes hooks and overrides at every @@ -329,6 +340,7 @@ func main() { | `ssh` | No | ECDSA key generation and SSH client for guest communication | | `state` | No | flock-based state persistence with atomic JSON writes | | `rootfs` | No | Rootfs cloning with reflink (copy-on-write) support | +| `virtiofs` | No | Strict, symlink-confined `override_stat` ownership preparation for shared host trees | | `internal/pathutil` | No | Path traversal validation for safe file operations | | `internal/xattr` | No | Extended attribute helpers for `override_stat` ownership mapping | diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 3d64f9f..c77c17c 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -76,7 +76,15 @@ in `microvm.Run()`: config (Entrypoint, Cmd, Env, WorkingDir), applying `WithInitOverride` if set. Writes the JSON file to `/.krun_config.json` in the rootfs. -5. **Start networking** -- Networking follows one of two paths: +5. **Prepare virtio-fs ownership** -- For writable mounts opting into + `OverrideUID`, strictly prepares `override_stat` metadata through the public + `virtiofs.PrepareOwnership` implementation. New or changed metadata needs + OS xattr-write permission, so an unannotated `0400` backing file can fail + preparation for an unprivileged caller. Validation and preparation complete + before any custom network provider starts. Read-only mounts are not prepared + automatically. + +6. **Start networking** -- Networking follows one of two paths: - **Default (no `WithNetProvider`)**: Port forwards are passed to the runner via `runner.Config`. The runner creates an in-process VirtualNetwork (gvisor-tap-vsock) connected via a socketpair. @@ -90,14 +98,14 @@ in `microvm.Run()`: runs the VirtualNetwork in the caller's process and supports HTTP services on the gateway IP. -6. **Start VM via backend** -- The `hypervisor.Backend` handles rootfs +7. **Start VM via backend** -- The `hypervisor.Backend` handles rootfs preparation and VM launch. The default libkrun backend serializes `runner.Config` as JSON and spawns `go-microvm-runner` as a detached subprocess (`setsid` for new session). The runner is located by searching: explicit path, system PATH, then next to the calling executable. Custom backends can be provided via `WithBackend()`. -7. **Post-boot hooks** -- Runs caller-provided `PostBootHook` functions. If +8. **Post-boot hooks** -- Runs caller-provided `PostBootHook` functions. If any hook fails, the VM is stopped and the error is returned. ### Runner Side (go-microvm-runner) diff --git a/docs/MACOS.md b/docs/MACOS.md index 8563bc1..c5a28e2 100644 --- a/docs/MACOS.md +++ b/docs/MACOS.md @@ -86,6 +86,76 @@ uid/gid/mode to the guest. This is the same mechanism used by podman on macOS. The xattr is set automatically during OCI layer extraction and rootfs cloning -- no user action is needed. +### virtio-fs shared directory ownership + +A writable `microvm.VirtioFSMount` with `OverrideUID > 0` is strictly prepared +before networking starts. `OverrideGID` defaults to the UID. Startup fails on +inaccessible entries, malformed metadata, unsupported special files, or xattr +errors instead of continuing with partly incorrect guest ownership. This is a +deliberate compatibility change from the previous best-effort behavior. There is +no best-effort toggle for opted-in writable mounts. + +```go +microvm.WithVirtioFS(microvm.VirtioFSMount{ + Tag: "shared", + HostPath: "/srv/vm-share", + OverrideUID: 65532, + OverrideGID: 65532, +}) +``` + +Read-only mounts are not prepared automatically, and explicit preparation does +not change their export flags. Prepare the backing tree first when the host OS +permits its xattr operation, then request a read-only export: + +```go +if err := virtiofs.PrepareOwnership(ctx, "/srv/vm-share", ".", 65532, 65532); err != nil { + return err +} +vm, err := microvm.Run(ctx, image, + microvm.WithVirtioFS(microvm.VirtioFSMount{ + Tag: "shared", HostPath: "/srv/vm-share", ReadOnly: true, + }), +) +``` + +The same API can prepare one replaced file or subtree while a VM is running, +without rescanning siblings: + +```go +if err := virtiofs.PrepareOwnership(ctx, "/srv/vm-share", "results/job-42", 65532, 65532); err != nil { + return err +} +``` + +The root must be a real directory, and the selected target must be `.` or a +relative path. Trusted ancestor symlinks such as macOS `/var` are allowed, but +the final root and every explicit relative component are opened without +following symlinks. Descendant symlinks are skipped. Only directories and +regular files are supported. Keep the export root stable for the VM lifetime: +libkrun pins that host mount, so replacing the root pathname does not retarget a +running guest. + +Host ownership and mode are unchanged. New metadata derives the guest mode from +the host inode. Existing metadata retains its permission, set-ID, and sticky +bits (including guest `chmod` changes), while preparation corrects the file type +and applies the requested uid/gid. Matching metadata is not rewritten. + +Creating or changing metadata requires the host OS permission to write xattrs. +An ordinary unprivileged user therefore cannot normally annotate an unannotated +`0400` file. `PrepareOwnership` returns a path-specific permission error and +leaves its host mode and IDs intact. A read-only virtio-fs export does not grant +xattr-write permission on its backing inodes. + +Preparation is nontransactional. Callers must synchronize it with host rename, +creation, and replacement and with guest access or `chmod`. There is no atomic +visibility or cache-invalidation guarantee; a descriptor held across replacement +continues to refer to the old inode. A hard link in the authorized tree +authorizes changing the xattr on that inode, including names outside the tree. +The caller must also trust and protect the root's parent while the root descriptor +is acquired; subsequent traversal is descriptor-relative and confined beneath +the acquired root. + ## Guest Networking On macOS, libkrun's Hypervisor.framework backend pre-configures the guest diff --git a/docs/RELEASE_NOTES.md b/docs/RELEASE_NOTES.md new file mode 100644 index 0000000..9b91760 --- /dev/null +++ b/docs/RELEASE_NOTES.md @@ -0,0 +1,11 @@ +# Release notes + +## Unreleased + +### Virtio-fs ownership preparation + +- Added public `virtiofs.PrepareOwnership(ctx, root, relativePath, uid, gid)` for strict full-tree or targeted `user.containers.override_stat` preparation on macOS and Linux. +- Writable mounts with `OverrideUID` now use the same strict preparation before networking starts. This deliberately replaces best-effort startup behavior; failures are path-specific and abort startup. +- Read-only mounts are not prepared automatically. Explicit preparation leaves export flags and host ownership and mode unchanged. New or changed metadata still requires host xattr-write permission, so an unprivileged caller normally receives a permission error for an unannotated `0400` file. +- Existing override mode bits and `OverrideGID` defaulting are preserved. Non-opted-in and read-only mounts remain unmodified at startup, and Linux user-namespace behavior is unchanged. +- Traversal is descriptor-relative and confined beneath the authorized root: explicit symlinks fail, descendant symlinks are skipped, and malformed metadata, unsupported types, and traversal or xattr errors are fatal. diff --git a/docs/SECURITY.md b/docs/SECURITY.md index e550ca5..e4a1f8d 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -464,17 +464,32 @@ capsh --addamb=cap_chown -- -c '/path/to/your-binary' ### override_stat xattr (macOS and Linux) -go-microvm also sets the `user.containers.override_stat` extended attribute on -extracted files so that libkrun's virtiofs server reports correct ownership to -the guest. This is the same mechanism that podman uses on macOS. - -On Linux, the xattr is set on regular files and directories. The kernel -restricts `user.*` xattrs on symlinks and special files, so those are silently -skipped. Once libkrun's Linux virtiofs passthrough adds support for reading -these xattrs (the same support already exists on macOS), file ownership in the -guest will be correct without requiring `CAP_CHOWN`. - -See the `internal/xattr` package for details. +go-microvm sets `user.containers.override_stat` on extracted files so libkrun's +virtiofs server reports intended guest ownership without changing host uid, gid, +or mode. This is the mechanism podman uses on macOS. Image extraction and hooks +retain their single-entry best-effort behavior for compatibility. + +For shared virtio-fs trees, writable mounts with `OverrideUID > 0` use the strict +public `virtiofs.PrepareOwnership` implementation before networking starts; +read-only mounts are not modified automatically. Matching existing metadata is +read without a rewrite, but new or changed metadata requires OS permission to +write xattrs. Thus an unprivileged caller can receive a path-specific permission +error for an unannotated `0400` backing file, while host mode and IDs remain +unchanged. New metadata derives guest mode from the host inode; later preparation +preserves guest mode bits recorded in `override_stat`. It uses descriptor-relative, +`O_NOFOLLOW` traversal after opening the authorized root. Explicit root/target +symlinks fail, descendant symlinks are skipped, and regular files/directories +are the only supported types. This prevents mutable intermediate symlinks from +redirecting the walk, but does not make preparation transactional. The caller +must trust the root's parent during root descriptor acquisition and synchronize +rename, creation, replacement, and guest chmod. Hard links authorize their +shared inode, including names outside the tree. There are no cache invalidation +or atomic guest-visibility guarantees. + +Malformed existing xattrs and read, write, or traversal failures are fatal and +path-specific. Valid matching metadata is not rewritten. On Linux, this does +not alter user-namespace behavior; it only prepares metadata for libkrun +versions that consume `override_stat`. ## File Permissions diff --git a/internal/xattr/walk.go b/internal/xattr/walk.go index b2e57e6..cdf39fc 100644 --- a/internal/xattr/walk.go +++ b/internal/xattr/walk.go @@ -6,60 +6,18 @@ package xattr import ( + "context" "fmt" - "io/fs" - "os" - "path/filepath" - "strings" + "math" + + "github.com/stacklok/go-microvm/virtiofs" ) -// SetOverrideStatTree walks root and sets user.containers.override_stat -// on every file and directory. Each entry's real mode (from Lstat) is -// preserved in the xattr value. Symlinks are skipped — they cannot carry -// user.* xattrs on Linux, and skipping them prevents setting xattrs -// outside the mount boundary via symlink traversal. -// -// The root path is resolved via [filepath.EvalSymlinks] before walking, -// and every visited entry is verified to remain under the resolved root. -// -// Errors on individual entries are logged at debug level and skipped. -// Returns an error only if the root itself cannot be accessed. -// -// On platforms other than macOS and Linux a no-op stub is provided. +// SetOverrideStatTree strictly prepares the entire root for virtio-fs ownership +// mapping. New code should call [virtiofs.PrepareOwnership] directly. func SetOverrideStatTree(root string, uid, gid int) error { - if _, err := os.Lstat(root); err != nil { - return fmt.Errorf("access root %s: %w", root, err) - } - - realRoot, err := filepath.EvalSymlinks(root) - if err != nil { - return fmt.Errorf("resolve root: %w", err) + if uid < 0 || gid < 0 || uint64(uid) > math.MaxUint32 || uint64(gid) > math.MaxUint32 { + return fmt.Errorf("override_stat uid/gid out of uint32 range: %d:%d", uid, gid) } - realRoot = filepath.Clean(realRoot) - rootPrefix := realRoot + string(filepath.Separator) - - return filepath.WalkDir(realRoot, func(path string, d fs.DirEntry, err error) error { - if err != nil { - return nil // best-effort, skip inaccessible entries - } - // Skip symlinks: prevents setting xattrs outside mount boundary, - // and Linux rejects user.* xattrs on symlinks anyway. - if d.Type()&fs.ModeSymlink != 0 { - return nil - } - // Boundary check: verify path stays under resolved root. - cleanPath := filepath.Clean(path) - if cleanPath != realRoot && !strings.HasPrefix(cleanPath, rootPrefix) { - if d.IsDir() { - return fs.SkipDir - } - return nil - } - info, err := d.Info() - if err != nil { - return nil - } - SetOverrideStat(path, uid, gid, info.Mode()) - return nil - }) + return virtiofs.PrepareOwnership(context.Background(), root, ".", uint32(uid), uint32(gid)) } diff --git a/internal/xattr/walk_test.go b/internal/xattr/walk_test.go index abb5bd9..9ddf794 100644 --- a/internal/xattr/walk_test.go +++ b/internal/xattr/walk_test.go @@ -102,20 +102,11 @@ func TestSetOverrideStatTree_RootIsSymlink(t *testing.T) { t.Parallel() real := t.TempDir() - sub := filepath.Join(real, "child") - require.NoError(t, os.Mkdir(sub, 0o755)) - - // Create a symlink that points to real. The walk should resolve it - // and set xattrs on the real directory tree. link := filepath.Join(t.TempDir(), "link") require.NoError(t, os.Symlink(real, link)) - require.NoError(t, SetOverrideStatTree(link, 1000, 1000)) - - val := readXattrOpt(t, real) - assert.Contains(t, val, "1000:1000:", "resolved root should have override xattr") - val = readXattrOpt(t, sub) - assert.Contains(t, val, "1000:1000:", "child dir should have override xattr") + err := SetOverrideStatTree(link, 1000, 1000) + assert.ErrorContains(t, err, "open authorized root") } func TestSetOverrideStatTree_DifferentUIDGID(t *testing.T) { diff --git a/microvm.go b/microvm.go index 65f8b0a..33391f6 100644 --- a/microvm.go +++ b/microvm.go @@ -37,11 +37,11 @@ import ( "github.com/stacklok/go-microvm/hypervisor" "github.com/stacklok/go-microvm/hypervisor/libkrun" "github.com/stacklok/go-microvm/image" - "github.com/stacklok/go-microvm/internal/xattr" "github.com/stacklok/go-microvm/net/firewall" "github.com/stacklok/go-microvm/net/hosted" rootfspkg "github.com/stacklok/go-microvm/rootfs" "github.com/stacklok/go-microvm/state" + "github.com/stacklok/go-microvm/virtiofs" ) // Run pulls an OCI image and boots it as a microVM. It is the primary entry @@ -209,7 +209,39 @@ func Run(ctx context.Context, imageRef string, opts ...Option) (*VM, error) { span.End() } - // 5. Start networking. + // 5. Validate and prepare ownership overrides before starting networking. + // Read-only backing directories are deliberately not modified at startup; + // callers can prepare them explicitly with virtiofs.PrepareOwnership. + for _, m := range cfg.virtioFS { + if m.OverrideUID < 0 || m.OverrideGID < 0 { + return nil, fmt.Errorf("virtiofs mount %q: OverrideUID/OverrideGID must be non-negative", m.Tag) + } + if uint64(m.OverrideUID) > uint64(^uint32(0)) || uint64(m.OverrideGID) > uint64(^uint32(0)) { + return nil, fmt.Errorf("virtiofs mount %q: OverrideUID/OverrideGID must fit in uint32", m.Tag) + } + if m.OverrideUID == 0 && m.OverrideGID > 0 { + return nil, fmt.Errorf("virtiofs mount %q: OverrideGID set without OverrideUID", m.Tag) + } + } + + _, xattrSpan := tracer.Start(ctx, "microvm.VirtioFSOverrideStat") + for _, m := range cfg.virtioFS { + if m.OverrideUID > 0 && !m.ReadOnly { + gid := m.OverrideGID + if gid == 0 { + gid = m.OverrideUID + } + if err := virtiofs.PrepareOwnership(ctx, m.HostPath, ".", uint32(m.OverrideUID), uint32(gid)); err != nil { + xattrSpan.RecordError(err) + xattrSpan.SetStatus(codes.Error, err.Error()) + xattrSpan.End() + return nil, fmt.Errorf("prepare virtiofs mount %q ownership: %w", m.Tag, err) + } + } + } + xattrSpan.End() + + // 6. Start networking. // // Default path: port forwards are passed to the runner, which creates // an in-process VirtualNetwork (gvisor-tap-vsock) alongside the VM. @@ -232,33 +264,7 @@ func Run(ctx context.Context, imageRef string, opts ...Option) (*VM, error) { span.End() } - // 5b. Validate and set override_stat xattrs on virtiofs mount entries so - // the guest sees correct ownership (macOS + Linux; no-op on other platforms). - for _, m := range cfg.virtioFS { - if m.OverrideUID < 0 || m.OverrideGID < 0 { - return nil, fmt.Errorf("virtiofs mount %q: OverrideUID/OverrideGID must be non-negative", m.Tag) - } - if m.OverrideUID == 0 && m.OverrideGID > 0 { - return nil, fmt.Errorf("virtiofs mount %q: OverrideGID set without OverrideUID", m.Tag) - } - } - _, xattrSpan := tracer.Start(ctx, "microvm.VirtioFSOverrideStat") - for _, m := range cfg.virtioFS { - if m.OverrideUID > 0 && !m.ReadOnly { - gid := m.OverrideGID - if gid <= 0 { - gid = m.OverrideUID - } - if err := xattr.SetOverrideStatTree(m.HostPath, m.OverrideUID, gid); err != nil { - xattrSpan.RecordError(err) - slog.Warn("failed to set override_stat on virtiofs mount", - "tag", m.Tag, "path", m.HostPath, "error", err) - } - } - } - xattrSpan.End() - - // 6. Start VM via backend. + // 7. Start VM via backend. _, vmSpawnSpan := tracer.Start(ctx, "microvm.VMSpawn") slog.Debug("starting VM") var netEndpoint hypervisor.NetEndpoint @@ -325,7 +331,7 @@ func Run(ctx context.Context, imageRef string, opts ...Option) (*VM, error) { ls.Release() } - // 7. Post-boot hooks (no-op on happy path). + // 8. Post-boot hooks (no-op on happy path). { _, span := tracer.Start(ctx, "microvm.PostBoot") for _, hook := range cfg.postBootHooks { diff --git a/microvm_test.go b/microvm_test.go index 4dc682e..711349f 100644 --- a/microvm_test.go +++ b/microvm_test.go @@ -175,6 +175,8 @@ type mockBackend struct { preparePath string // if set, returned instead of rootfsPath startHandle hypervisor.VMHandle startErr error + startCalls int + lastConfig hypervisor.VMConfig } func (m *mockBackend) Name() string { return "mock" } @@ -189,7 +191,9 @@ func (m *mockBackend) PrepareRootFS(_ context.Context, rootfsPath string, _ hype return rootfsPath, nil } -func (m *mockBackend) Start(_ context.Context, _ hypervisor.VMConfig) (hypervisor.VMHandle, error) { +func (m *mockBackend) Start(_ context.Context, cfg hypervisor.VMConfig) (hypervisor.VMHandle, error) { + m.startCalls++ + m.lastConfig = cfg return m.startHandle, m.startErr } diff --git a/microvm_virtiofs_test.go b/microvm_virtiofs_test.go new file mode 100644 index 0000000..7ddfcd0 --- /dev/null +++ b/microvm_virtiofs_test.go @@ -0,0 +1,179 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build darwin || linux + +package microvm + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" + + "github.com/stacklok/go-microvm/guest/vmconfig" + "github.com/stacklok/go-microvm/preflight" + "github.com/stacklok/go-microvm/virtiofs" +) + +func TestRunPreparesWritableVirtioFSOwnership(t *testing.T) { + dataDir := t.TempDir() + rootfs := filepath.Join(dataDir, "rootfs") + share := filepath.Join(dataDir, "share") + require.NoError(t, os.Mkdir(rootfs, 0o755)) + require.NoError(t, os.Mkdir(share, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(share, "data"), []byte("x"), 0o640)) + require.NoError(t, unix.Lsetxattr(filepath.Join(share, "data"), "user.containers.override_stat", []byte("12:13:0100600"), 0)) + + backend := &mockBackend{startHandle: &mockVMHandle{id: "42", alive: true}} + vm, err := Run(context.Background(), "unused", + WithDataDir(dataDir), WithPreflightChecker(preflight.NewEmpty()), + WithRootFSPath(rootfs), WithBackend(backend), + WithVirtioFS(VirtioFSMount{Tag: "share", HostPath: share, OverrideUID: 65532}), + ) + require.NoError(t, err) + t.Cleanup(func() { _ = vm.Stop(context.Background()) }) + assert.Equal(t, "65532:65532:0100600", readOverrideForRunTest(t, filepath.Join(share, "data"))) +} + +func TestRunOwnershipFailurePreventsNetworkAndBackendStart(t *testing.T) { + dataDir := t.TempDir() + rootfs := filepath.Join(dataDir, "rootfs") + share := filepath.Join(dataDir, "share") + require.NoError(t, os.Mkdir(rootfs, 0o755)) + require.NoError(t, os.Mkdir(share, 0o755)) + require.NoError(t, unix.Lsetxattr(share, "user.containers.override_stat", []byte("malformed"), 0)) + + backend := &mockBackend{startHandle: &mockVMHandle{id: "42", alive: true}} + provider := &mockNetProvider{} + _, err := Run(context.Background(), "unused", + WithDataDir(dataDir), WithPreflightChecker(preflight.NewEmpty()), + WithRootFSPath(rootfs), WithBackend(backend), WithNetProvider(provider), + WithVirtioFS(VirtioFSMount{Tag: "share", HostPath: share, OverrideUID: 65532}), + ) + require.ErrorContains(t, err, "prepare virtiofs mount \"share\" ownership") + assert.Zero(t, provider.startCalls) + assert.Zero(t, backend.startCalls) +} + +func TestExplicitlyPreparedReadOnlyMountRemainsReadOnly(t *testing.T) { + dataDir := t.TempDir() + rootfs := filepath.Join(dataDir, "rootfs") + share := filepath.Join(dataDir, "share") + require.NoError(t, os.Mkdir(rootfs, 0o755)) + require.NoError(t, os.Mkdir(share, 0o700)) + data := filepath.Join(share, "data") + // This is a wiring-only preannotated snapshot: setup stamps guest 0400 + // while the file is writable, then makes the host backing inode 0400. + require.NoError(t, os.WriteFile(data, []byte("readonly"), 0o600)) + require.NoError(t, unix.Lsetxattr(data, "user.containers.override_stat", []byte("65532:65532:0100400"), 0)) + require.NoError(t, virtiofs.PrepareOwnership(context.Background(), share, ".", 65532, 65532)) + require.NoError(t, os.Chmod(data, 0o400)) + beforeDir, err := os.Stat(share) + require.NoError(t, err) + beforeData, err := os.Stat(data) + require.NoError(t, err) + + backend := &mockBackend{startHandle: &mockVMHandle{id: "42", alive: true}} + vm, err := Run(context.Background(), "unused", + WithDataDir(dataDir), WithPreflightChecker(preflight.NewEmpty()), + WithRootFSPath(rootfs), WithBackend(backend), + WithVirtioFS(VirtioFSMount{Tag: "share", HostPath: share, ReadOnly: true, OverrideUID: 65532}), + ) + require.NoError(t, err) + t.Cleanup(func() { _ = vm.Stop(context.Background()) }) + require.Len(t, backend.lastConfig.FilesystemMounts, 1) + assert.True(t, backend.lastConfig.FilesystemMounts[0].ReadOnly) + + afterDir, err := os.Stat(share) + require.NoError(t, err) + afterData, err := os.Stat(data) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o700), afterDir.Mode().Perm()) + assert.Equal(t, os.FileMode(0o400), afterData.Mode().Perm()) + assert.Equal(t, "65532:65532:0100400", readOverrideForRunTest(t, data)) + assert.Equal(t, beforeDir.Sys().(*syscall.Stat_t).Uid, afterDir.Sys().(*syscall.Stat_t).Uid) + assert.Equal(t, beforeDir.Sys().(*syscall.Stat_t).Gid, afterDir.Sys().(*syscall.Stat_t).Gid) + assert.Equal(t, beforeData.Sys().(*syscall.Stat_t).Uid, afterData.Sys().(*syscall.Stat_t).Uid) + assert.Equal(t, beforeData.Sys().(*syscall.Stat_t).Gid, afterData.Sys().(*syscall.Stat_t).Gid) + + configData, err := os.ReadFile(filepath.Join(rootfs, vmconfig.GuestPath)) + require.NoError(t, err) + var guestConfig vmconfig.Config + require.NoError(t, json.Unmarshal(configData, &guestConfig)) + require.Equal(t, []vmconfig.VirtioFSMountInfo{{Tag: "share", ReadOnly: true}}, guestConfig.VirtioFSMounts) +} + +func TestRunDoesNotTraverseNonOptInOrReadOnlyMounts(t *testing.T) { + for _, tt := range []struct { + name string + mount VirtioFSMount + }{ + {name: "non-opt-in", mount: VirtioFSMount{Tag: "share"}}, + {name: "read-only", mount: VirtioFSMount{Tag: "share", ReadOnly: true, OverrideUID: 65532}}, + } { + t.Run(tt.name, func(t *testing.T) { + dataDir := t.TempDir() + rootfs := filepath.Join(dataDir, "rootfs") + share := filepath.Join(dataDir, "share") + require.NoError(t, os.Mkdir(rootfs, 0o755)) + require.NoError(t, os.Mkdir(share, 0o755)) + require.NoError(t, unix.Lsetxattr(share, "user.containers.override_stat", []byte("malformed"), 0)) + tt.mount.HostPath = share + backend := &mockBackend{startHandle: &mockVMHandle{id: "42", alive: true}} + vm, err := Run(context.Background(), "unused", + WithDataDir(dataDir), WithPreflightChecker(preflight.NewEmpty()), + WithRootFSPath(rootfs), WithBackend(backend), WithVirtioFS(tt.mount), + ) + require.NoError(t, err) + t.Cleanup(func() { _ = vm.Stop(context.Background()) }) + assert.Equal(t, "malformed", readOverrideForRunTest(t, share)) + assert.Equal(t, 1, backend.startCalls) + }) + } +} + +func TestPrepareOwnershipAfterRunTargetsReplacementWithoutRestart(t *testing.T) { + dataDir := t.TempDir() + rootfs := filepath.Join(dataDir, "rootfs") + share := filepath.Join(dataDir, "share") + require.NoError(t, os.Mkdir(rootfs, 0o755)) + require.NoError(t, os.Mkdir(share, 0o755)) + unrelated := filepath.Join(share, "unrelated") + require.NoError(t, os.WriteFile(unrelated, nil, 0o644)) + require.NoError(t, unix.Lsetxattr(unrelated, "user.containers.override_stat", []byte("65532:65532:0100600"), 0)) + target := filepath.Join(share, "target") + require.NoError(t, os.WriteFile(target, []byte("old"), 0o600)) + + backend := &mockBackend{startHandle: &mockVMHandle{id: "42", alive: true}} + vm, err := Run(context.Background(), "unused", + WithDataDir(dataDir), WithPreflightChecker(preflight.NewEmpty()), + WithRootFSPath(rootfs), WithBackend(backend), + WithVirtioFS(VirtioFSMount{Tag: "share", HostPath: share}), + ) + require.NoError(t, err) + t.Cleanup(func() { _ = vm.Stop(context.Background()) }) + + staged := filepath.Join(share, "staged") + require.NoError(t, os.WriteFile(staged, []byte("new"), 0o640)) + require.NoError(t, os.Rename(staged, target)) + require.NoError(t, virtiofs.PrepareOwnership(context.Background(), share, "target", 65532, 65532)) + + assert.Equal(t, "65532:65532:0100640", readOverrideForRunTest(t, target)) + assert.Equal(t, "65532:65532:0100600", readOverrideForRunTest(t, unrelated)) + assert.Equal(t, 1, backend.startCalls) +} + +func readOverrideForRunTest(t *testing.T, path string) string { + t.Helper() + buf := make([]byte, 256) + n, err := unix.Lgetxattr(path, "user.containers.override_stat", buf) + require.NoError(t, err) + return string(buf[:n]) +} diff --git a/options.go b/options.go index bc27c95..477762f 100644 --- a/options.go +++ b/options.go @@ -62,14 +62,21 @@ type VirtioFSMount struct { // krun_add_virtiofs3 API; the runner fails before VM start if it is unavailable // or rejects the configuration. ReadOnly bool - // OverrideUID, when > 0, causes go-microvm to set the - // user.containers.override_stat xattr on every file and directory under - // HostPath before the VM starts. This makes libkrun's virtiofs FUSE - // server report the given UID/GID to the guest instead of the real - // host values. Symlinks are skipped for safety. - // A zero value means "no override." Since 0 is the zero value for int, - // overriding to UID 0 (root) is not supported through this field. - // Ignored for ReadOnly mounts. + // OverrideUID, when > 0 on a writable mount, strictly prepares every + // regular file and directory under HostPath with libkrun's + // user.containers.override_stat xattr before networking or VM startup. The + // guest sees OverrideUID/OverrideGID while host ownership and mode remain + // unchanged. Symlinks below HostPath are skipped; unsupported file types, + // inaccessible entries, malformed xattrs, and xattr failures abort startup. + // New or changed xattrs require the OS permission to write xattrs; an + // unannotated 0400 backing file can therefore fail preparation with EACCES. + // A new xattr derives its guest mode from the host inode, and an existing + // xattr preserves its guest mode bits. + // + // A zero value means "no startup override." Since 0 is the zero value for + // int, overriding to UID 0 is not supported through this field. Read-only + // mounts are not modified automatically; prepare their backing directory + // explicitly with virtiofs.PrepareOwnership before calling Run. OverrideUID int // OverrideGID sets the group ID for the override_stat xattr. // When 0 and OverrideUID > 0, defaults to OverrideUID. diff --git a/virtiofs/doc.go b/virtiofs/doc.go new file mode 100644 index 0000000..9e33c47 --- /dev/null +++ b/virtiofs/doc.go @@ -0,0 +1,8 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +// Package virtiofs prepares host files for libkrun virtio-fs ownership mapping. +// +// PrepareOwnership changes only the user.containers.override_stat extended +// attribute. It does not change host ownership or permissions. +package virtiofs diff --git a/virtiofs/example_test.go b/virtiofs/example_test.go new file mode 100644 index 0000000..b5946a0 --- /dev/null +++ b/virtiofs/example_test.go @@ -0,0 +1,19 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package virtiofs_test + +import ( + "context" + "log" + + "github.com/stacklok/go-microvm/virtiofs" +) + +func ExamplePrepareOwnership() { + // Prepare a newly created subtree of an existing virtio-fs backing + // directory for the fixed guest service account. + if err := virtiofs.PrepareOwnership(context.Background(), "/srv/vm-share", "results", 65532, 65532); err != nil { + log.Fatal(err) + } +} diff --git a/virtiofs/noattr_darwin.go b/virtiofs/noattr_darwin.go new file mode 100644 index 0000000..2ccb6cb --- /dev/null +++ b/virtiofs/noattr_darwin.go @@ -0,0 +1,14 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build darwin + +package virtiofs + +import ( + "errors" + + "golang.org/x/sys/unix" +) + +func isNoAttribute(err error) bool { return errors.Is(err, unix.ENOATTR) } diff --git a/virtiofs/noattr_linux.go b/virtiofs/noattr_linux.go new file mode 100644 index 0000000..c6cccf1 --- /dev/null +++ b/virtiofs/noattr_linux.go @@ -0,0 +1,14 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package virtiofs + +import ( + "errors" + + "golang.org/x/sys/unix" +) + +func isNoAttribute(err error) bool { return errors.Is(err, unix.ENODATA) } diff --git a/virtiofs/prepare_linux_test.go b/virtiofs/prepare_linux_test.go new file mode 100644 index 0000000..5d21b91 --- /dev/null +++ b/virtiofs/prepare_linux_test.go @@ -0,0 +1,52 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package virtiofs + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" +) + +func TestPrepareOwnershipMatchingMetadataIsNotRewritten(t *testing.T) { + root := t.TempDir() + file := filepath.Join(root, "file") + require.NoError(t, os.WriteFile(file, []byte("data"), 0o640)) + require.NoError(t, PrepareOwnership(context.Background(), root, "file", 65532, 65532)) + // libkrun interprets the mode as octal with or without a leading zero. + // Preserve an equivalent non-canonical representation rather than rewriting it. + require.NoError(t, unix.Lsetxattr(file, overrideKey, []byte("65532:65532:100640"), 0)) + + var before unix.Stat_t + require.NoError(t, unix.Lstat(file, &before)) + time.Sleep(10 * time.Millisecond) + require.NoError(t, PrepareOwnership(context.Background(), root, "file", 65532, 65532)) + var after unix.Stat_t + require.NoError(t, unix.Lstat(file, &after)) + + assert.Equal(t, before.Ctim, after.Ctim, "semantically matching xattr should not be rewritten") + assert.Equal(t, "65532:65532:100640", getOverride(t, file)) +} + +func TestPrepareOwnershipLaterSubtreeDoesNotRescanSiblings(t *testing.T) { + root := t.TempDir() + unrelated := filepath.Join(root, "unrelated") + require.NoError(t, os.WriteFile(unrelated, []byte("data"), 0o600)) + require.NoError(t, unix.Lsetxattr(unrelated, overrideKey, []byte("malformed"), 0)) + + created := filepath.Join(root, "created", "nested") + require.NoError(t, os.MkdirAll(created, 0o755)) + require.NoError(t, PrepareOwnership(context.Background(), root, "created", 65532, 65532)) + + assert.Contains(t, getOverride(t, created), "65532:65532:") + assert.Equal(t, "malformed", getOverride(t, unrelated)) +} diff --git a/virtiofs/prepare_other.go b/virtiofs/prepare_other.go new file mode 100644 index 0000000..ae9bc88 --- /dev/null +++ b/virtiofs/prepare_other.go @@ -0,0 +1,17 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !darwin && !linux + +package virtiofs + +import ( + "context" + "fmt" + "runtime" +) + +// PrepareOwnership reports that override_stat preparation is unavailable. +func PrepareOwnership(_ context.Context, _, _ string, _, _ uint32) error { + return fmt.Errorf("virtiofs ownership preparation is unsupported on %s", runtime.GOOS) +} diff --git a/virtiofs/prepare_unix.go b/virtiofs/prepare_unix.go new file mode 100644 index 0000000..6d2d589 --- /dev/null +++ b/virtiofs/prepare_unix.go @@ -0,0 +1,251 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build darwin || linux + +package virtiofs + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + + "golang.org/x/sys/unix" +) + +const overrideKey = "user.containers.override_stat" + +// PrepareOwnership sets libkrun's override_stat ownership metadata on an +// authorized host entry or tree. relativePath must be "." for the whole root, +// or a non-empty relative path naming one entry and, if it is a directory, its +// subtree. +// +// The caller needs the OS permission required to read existing xattrs and to +// write new or changed metadata. A matching existing xattr is read but not +// rewritten. In particular, an unannotated host file with mode 0400 commonly +// rejects a new xattr from an unprivileged owner with EACCES; this function +// returns that error without changing host mode or ownership. It does not +// provide a way to choose an initial guest mode: a new xattr derives it from +// the host inode, while an existing xattr preserves its guest mode bits. +// +// The final component of root and every component of relativePath are opened +// without following symlinks. Symlinks below the selected target are skipped. +// Only directories and regular files are supported; encountering another file +// type fails the operation. The real host uid, gid, and mode are not changed. +// Existing mode permission, set-ID, and sticky bits are preserved in the +// override, while the requested uid and gid are reported to the guest. +// +// The caller authorizes the directory reached when root is opened and must +// trust and protect root's parent path while that acquisition occurs. All +// subsequent traversal is descriptor-relative, so mutable intermediate +// symlinks cannot escape that directory. Hard links inside root authorize the +// linked inode, even when that inode also has names outside root. +// +// Preparation is non-transactional. An error may leave earlier entries +// prepared. Callers must synchronize concurrent rename, creation, replacement, +// and guest chmod operations. The function provides neither cache invalidation +// nor atomic visibility to a running guest. +func PrepareOwnership(ctx context.Context, root, relativePath string, uid, gid uint32) error { + if root == "" { + return errors.New("virtiofs ownership root must not be empty") + } + parts, err := validateTarget(relativePath) + if err != nil { + return err + } + if err := ctx.Err(); err != nil { + return fmt.Errorf("prepare ownership %q: %w", relativePath, err) + } + + fd, err := acquireTarget(root, relativePath, parts) + if err != nil { + return err + } + return prepareTree(ctx, fd, relativePath, uid, gid) +} + +// acquireTarget pins the selected inode. Callers own the returned descriptor. +func acquireTarget(root, relativePath string, parts []string) (int, error) { + // Cleaning removes a trailing slash or /. that would otherwise make + // O_NOFOLLOW ineffective for a symlink in the final root component. + root = filepath.Clean(root) + fd, err := unix.Open(root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err != nil { + return -1, fmt.Errorf("open authorized root %q without following symlinks: %w", root, err) + } + + if len(parts) == 0 { + return fd, nil + } + + current := fd + for i, part := range parts { + flags := unix.O_RDONLY | unix.O_NOFOLLOW | unix.O_CLOEXEC | unix.O_NONBLOCK + if i < len(parts)-1 { + flags |= unix.O_DIRECTORY + } + next, openErr := unix.Openat(current, part, flags, 0) + if closeErr := unix.Close(current); closeErr != nil && openErr == nil { + if next >= 0 { + _ = unix.Close(next) + } + return -1, fmt.Errorf("close target parent %q: %w", filepath.Join(parts[:i]...), closeErr) + } + if openErr != nil { + return -1, fmt.Errorf("open target %q without following symlinks: %w", relativePath, openErr) + } + current = next + } + return current, nil +} + +func validateTarget(relativePath string) ([]string, error) { + if relativePath == "" { + return nil, errors.New("virtiofs ownership target must not be empty") + } + if filepath.IsAbs(relativePath) { + return nil, fmt.Errorf("virtiofs ownership target %q must be relative", relativePath) + } + if relativePath == "." { + return nil, nil + } + parts := strings.Split(relativePath, string(filepath.Separator)) + for _, part := range parts { + if part == "" || part == "." || part == ".." { + return nil, fmt.Errorf("virtiofs ownership target %q contains an invalid path component", relativePath) + } + } + return parts, nil +} + +func prepareTree(ctx context.Context, fd int, displayPath string, uid, gid uint32) error { + return prepareTreeWith(ctx, fd, displayPath, uid, gid, unix.Openat, func(file *os.File) ([]os.DirEntry, error) { + return file.ReadDir(-1) + }) +} + +func prepareTreeWith( + ctx context.Context, + fd int, + displayPath string, + uid, gid uint32, + openat func(int, string, int, uint32) (int, error), + readDir func(*os.File) ([]os.DirEntry, error), +) (retErr error) { + file := os.NewFile(uintptr(fd), displayPath) + if file == nil { + _ = unix.Close(fd) + return fmt.Errorf("open target %q: invalid file descriptor", displayPath) + } + defer func() { + if err := file.Close(); err != nil && retErr == nil { + retErr = fmt.Errorf("close %q: %w", displayPath, err) + } + }() + + if err := ctx.Err(); err != nil { + return fmt.Errorf("prepare ownership %q: %w", displayPath, err) + } + + var stat unix.Stat_t + if err := unix.Fstat(fd, &stat); err != nil { + return fmt.Errorf("stat %q: %w", displayPath, err) + } + typeBits := uint32(stat.Mode) & unix.S_IFMT + if typeBits != unix.S_IFDIR && typeBits != unix.S_IFREG { + return fmt.Errorf("prepare ownership %q: unsupported file type (mode %#o)", displayPath, stat.Mode) + } + if err := prepareEntry(fd, displayPath, uid, gid, uint32(stat.Mode)); err != nil { + return err + } + if typeBits != unix.S_IFDIR { + return nil + } + + entries, err := readDir(file) + if err != nil { + return fmt.Errorf("read directory %q: %w", displayPath, err) + } + for _, entry := range entries { + if err := ctx.Err(); err != nil { + return fmt.Errorf("prepare ownership %q: %w", filepath.Join(displayPath, entry.Name()), err) + } + childPath := filepath.Join(displayPath, entry.Name()) + child, err := openat(fd, entry.Name(), unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_CLOEXEC|unix.O_NONBLOCK, 0) + if err != nil { + if errors.Is(err, unix.ELOOP) { + continue + } + return fmt.Errorf("open descendant %q without following symlinks: %w", childPath, err) + } + if err := prepareTreeWith(ctx, child, childPath, uid, gid, openat, readDir); err != nil { + return err + } + } + return nil +} + +func prepareEntry(fd int, path string, uid, gid, hostMode uint32) error { + return prepareEntryWith(fd, path, uid, gid, hostMode, unix.Fgetxattr, unix.Fsetxattr) +} + +func prepareEntryWith( + fd int, + path string, + uid, gid, hostMode uint32, + getxattr func(int, string, []byte) (int, error), + setxattr func(int, string, []byte, int) error, +) error { + mode := hostMode + buf := make([]byte, 256) + n, err := getxattr(fd, overrideKey, buf) + if err == nil { + currentUID, currentGID, currentMode, parseErr := parseOverride(string(buf[:n])) + if parseErr != nil { + return fmt.Errorf("read existing override_stat on %q: %w", path, parseErr) + } + // Guest chmod changes only override_stat. Keep those permission, set-ID, + // and sticky bits, but always take the type from the current inode. + mode = hostMode&unix.S_IFMT | currentMode&0o7777 + if currentUID == uid && currentGID == gid && currentMode == mode { + return nil + } + } else if !isNoAttribute(err) { + return fmt.Errorf("read override_stat on %q: %w", path, err) + } + + desired := fmt.Sprintf("%d:%d:0%o", uid, gid, mode) + if err := setxattr(fd, overrideKey, []byte(desired), 0); err != nil { + return fmt.Errorf("write override_stat on %q: %w", path, err) + } + return nil +} + +func parseOverride(value string) (uint32, uint32, uint32, error) { + parts := strings.Split(value, ":") + if len(parts) != 3 { + return 0, 0, 0, fmt.Errorf("malformed value %q", value) + } + uid, err := strconv.ParseUint(parts[0], 10, 32) + if err != nil { + return 0, 0, 0, fmt.Errorf("malformed uid in value %q: %w", value, err) + } + gid, err := strconv.ParseUint(parts[1], 10, 32) + if err != nil { + return 0, 0, 0, fmt.Errorf("malformed gid in value %q: %w", value, err) + } + // libkrun's override_stat mode field is octal, including when it has no + // leading zero. POSIX st_mode has no representable bits above 0177777. + mode, err := strconv.ParseUint(parts[2], 8, 32) + if err != nil || mode > 0o177777 { + if err == nil { + err = errors.New("mode exceeds POSIX st_mode bits") + } + return 0, 0, 0, fmt.Errorf("malformed mode in value %q: %w", value, err) + } + return uint32(uid), uint32(gid), uint32(mode), nil +} diff --git a/virtiofs/prepare_unix_test.go b/virtiofs/prepare_unix_test.go new file mode 100644 index 0000000..1fad570 --- /dev/null +++ b/virtiofs/prepare_unix_test.go @@ -0,0 +1,290 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build darwin || linux + +package virtiofs + +import ( + "context" + "errors" + "os" + "path/filepath" + "runtime" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" +) + +func TestPrepareOwnershipTargeted(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "target") + unrelated := filepath.Join(root, "unrelated") + require.NoError(t, os.Mkdir(target, 0o750)) + require.NoError(t, os.Chmod(target, os.ModeSticky|0o750)) + require.NoError(t, os.WriteFile(filepath.Join(target, "file"), []byte("data"), 0o640)) + require.NoError(t, os.WriteFile(unrelated, []byte("other"), 0o600)) + + before, err := os.Lstat(filepath.Join(target, "file")) + require.NoError(t, err) + beforeStat := before.Sys().(*syscall.Stat_t) + + require.NoError(t, PrepareOwnership(context.Background(), root, "target", 65532, 65532)) + assert.Equal(t, "65532:65532:041750", getOverride(t, target)) + assert.Equal(t, "65532:65532:0100640", getOverride(t, filepath.Join(target, "file"))) + assert.Empty(t, getOverride(t, root)) + assert.Empty(t, getOverride(t, unrelated)) + + after, err := os.Lstat(filepath.Join(target, "file")) + require.NoError(t, err) + afterStat := after.Sys().(*syscall.Stat_t) + assert.Equal(t, before.Mode(), after.Mode()) + assert.Equal(t, beforeStat.Uid, afterStat.Uid) + assert.Equal(t, beforeStat.Gid, afterStat.Gid) +} + +func TestPrepareOwnershipRejectsTargetsAndSymlinks(t *testing.T) { + root := t.TempDir() + external := t.TempDir() + require.NoError(t, os.Symlink(external, filepath.Join(root, "link"))) + rootLink := filepath.Join(t.TempDir(), "root-link") + require.NoError(t, os.Symlink(root, rootLink)) + + for _, target := range []string{"", "/absolute", "../escape", "a/../escape", "a//b"} { + err := PrepareOwnership(context.Background(), root, target, 1, 1) + assert.Error(t, err, target) + } + for _, rootPath := range []string{rootLink, rootLink + string(os.PathSeparator), rootLink + string(os.PathSeparator) + "."} { + assert.ErrorContains(t, PrepareOwnership(context.Background(), rootPath, ".", 1, 1), "authorized root") + } + trustedParent := t.TempDir() + realRoot := filepath.Join(trustedParent, "root") + require.NoError(t, os.Mkdir(realRoot, 0o700)) + ancestorLink := filepath.Join(t.TempDir(), "trusted-ancestor") + require.NoError(t, os.Symlink(trustedParent, ancestorLink)) + require.NoError(t, PrepareOwnership(context.Background(), filepath.Join(ancestorLink, "root"), ".", 1, 1)) + assert.ErrorContains(t, PrepareOwnership(context.Background(), root, "link", 1, 1), "open target") +} + +func TestPrepareOwnershipSkipsDescendantSymlink(t *testing.T) { + root := t.TempDir() + external := t.TempDir() + externalFile := filepath.Join(external, "secret") + require.NoError(t, os.WriteFile(externalFile, []byte("secret"), 0o600)) + require.NoError(t, os.Symlink(external, filepath.Join(root, "escape"))) + + require.NoError(t, PrepareOwnership(context.Background(), root, ".", 42, 43)) + assert.Empty(t, getOverride(t, externalFile)) +} + +func TestPrepareOwnershipSkipsDescendantReplacedWithSymlink(t *testing.T) { + root := t.TempDir() + victim := filepath.Join(root, "victim") + replaced := filepath.Join(root, "replaced-victim") + external := t.TempDir() + externalFile := filepath.Join(external, "secret") + require.NoError(t, os.Mkdir(victim, 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(victim, "inside"), nil, 0o600)) + require.NoError(t, os.WriteFile(externalFile, nil, 0o600)) + + fd, err := unix.Open(root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + require.NoError(t, err) + replacedOnce := false + err = prepareTreeWith(context.Background(), fd, ".", 42, 43, + func(parent int, name string, flags int, mode uint32) (int, error) { + if !replacedOnce && name == "victim" { + replacedOnce = true + require.NoError(t, os.Rename(victim, replaced)) + require.NoError(t, os.Symlink(external, victim)) + } + return unix.Openat(parent, name, flags, mode) + }, + func(file *os.File) ([]os.DirEntry, error) { return file.ReadDir(-1) }, + ) + require.NoError(t, err) + assert.Empty(t, getOverride(t, external)) + assert.Empty(t, getOverride(t, externalFile)) +} + +func TestPrepareOwnershipUnannotatedReadOnlyFileFailsWithoutXattrWritePermission(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("requires a non-root user to verify normal xattr permission enforcement") + } + + // t.TempDir creates a caller-owned directory without inherited ACL assumptions. + root := t.TempDir() + path := filepath.Join(root, "readonly") + require.NoError(t, os.WriteFile(path, []byte("data"), 0o600)) + require.NoError(t, os.Chmod(path, 0o400)) + before, err := os.Lstat(path) + require.NoError(t, err) + beforeStat := before.Sys().(*syscall.Stat_t) + + err = PrepareOwnership(context.Background(), root, "readonly", 42, 43) + require.Error(t, err) + assert.True(t, errors.Is(err, unix.EACCES) || errors.Is(err, unix.EPERM), "expected a Unix permission error, got %v", err) + assert.Empty(t, getOverride(t, path)) + + after, err := os.Lstat(path) + require.NoError(t, err) + afterStat := after.Sys().(*syscall.Stat_t) + assert.Equal(t, os.FileMode(0o400), after.Mode().Perm()) + assert.Equal(t, beforeStat.Uid, afterStat.Uid) + assert.Equal(t, beforeStat.Gid, afterStat.Gid) +} + +func TestPrepareOwnershipStrictErrors(t *testing.T) { + t.Run("malformed existing value", func(t *testing.T) { + root := t.TempDir() + require.NoError(t, unix.Lsetxattr(root, overrideKey, []byte("broken"), 0)) + assert.ErrorContains(t, PrepareOwnership(context.Background(), root, ".", 1, 1), "malformed") + }) + + t.Run("special file", func(t *testing.T) { + if runtime.GOOS == "darwin" { + t.Skip("mkfifo test is covered on Linux") + } + root := t.TempDir() + require.NoError(t, unix.Mkfifo(filepath.Join(root, "pipe"), 0o600)) + assert.ErrorContains(t, PrepareOwnership(context.Background(), root, ".", 1, 1), "unsupported file type") + }) + + t.Run("cancelled", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + assert.ErrorIs(t, PrepareOwnership(ctx, t.TempDir(), ".", 1, 1), context.Canceled) + }) +} + +func TestPrepareOwnershipPreservesGuestMode(t *testing.T) { + tests := []struct { + name string + dir bool + hostMode os.FileMode + existing string + uid uint32 + gid uint32 + want string + }{ + {name: "host 0644 guest 0600", hostMode: 0o644, existing: "7:8:0100600", uid: 7, gid: 8, want: "7:8:0100600"}, + {name: "changed owners", hostMode: 0o644, existing: "7:8:0100600", uid: 9, gid: 10, want: "9:10:0100600"}, + {name: "file type corrected", hostMode: 0o644, existing: "7:8:041750", uid: 7, gid: 8, want: "7:8:0101750"}, + {name: "directory sticky and set-ID preserved", dir: true, hostMode: 0o700, existing: "7:8:0106751", uid: 7, gid: 8, want: "7:8:046751"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + root := t.TempDir() + path := filepath.Join(root, "entry") + if tt.dir { + require.NoError(t, os.Mkdir(path, tt.hostMode)) + } else { + require.NoError(t, os.WriteFile(path, nil, tt.hostMode)) + } + require.NoError(t, unix.Lsetxattr(path, overrideKey, []byte(tt.existing), 0)) + require.NoError(t, PrepareOwnership(context.Background(), root, "entry", tt.uid, tt.gid)) + assert.Equal(t, tt.want, getOverride(t, path)) + info, err := os.Lstat(path) + require.NoError(t, err) + assert.Equal(t, tt.hostMode.Perm(), info.Mode().Perm()) + }) + } +} + +func TestPrepareOwnershipMalformedMetadataIsNotClobbered(t *testing.T) { + for _, value := range []string{ + "broken", "x:2:0100644", "1:x:0100644", "4294967296:2:0100644", + "1:4294967296:0100644", "1:2:0100999", "1:2:0200000", + } { + t.Run(value, func(t *testing.T) { + root := t.TempDir() + require.NoError(t, unix.Lsetxattr(root, overrideKey, []byte(value), 0)) + assert.ErrorContains(t, PrepareOwnership(context.Background(), root, ".", 1, 2), "malformed") + assert.Equal(t, value, getOverride(t, root)) + }) + } +} + +func TestPrepareOwnershipPinnedTargetCannotEscapeAfterReplacement(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "target") + require.NoError(t, os.Mkdir(target, 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(target, "inside"), nil, 0o600)) + external := t.TempDir() + externalFile := filepath.Join(external, "outside") + require.NoError(t, os.WriteFile(externalFile, nil, 0o600)) + + parts, err := validateTarget("target") + require.NoError(t, err) + fd, err := acquireTarget(root, "target", parts) + require.NoError(t, err) + oldTarget := filepath.Join(root, "old-target") + require.NoError(t, os.Rename(target, oldTarget)) + require.NoError(t, os.Symlink(external, target)) + + require.NoError(t, prepareTree(context.Background(), fd, "target", 42, 43)) + assert.Contains(t, getOverride(t, filepath.Join(oldTarget, "inside")), "42:43:") + assert.Empty(t, getOverride(t, externalFile)) +} + +func TestPrepareOwnershipStrictInjectedErrors(t *testing.T) { + t.Run("xattr read oversized", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "file") + require.NoError(t, os.WriteFile(path, nil, 0o600)) + require.NoError(t, unix.Lsetxattr(path, overrideKey, make([]byte, 300), 0)) + file, err := os.Open(path) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, file.Close()) }) + assert.ErrorContains(t, prepareEntry(int(file.Fd()), path, 1, 1, unix.S_IFREG|0o600), "read override_stat") + }) + + t.Run("xattr write", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "file") + require.NoError(t, os.WriteFile(path, nil, 0o600)) + file, err := os.Open(path) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, file.Close()) }) + err = prepareEntryWith(int(file.Fd()), path, 1, 1, unix.S_IFREG|0o600, unix.Fgetxattr, + func(int, string, []byte, int) error { return unix.EROFS }) + assert.ErrorContains(t, err, "write override_stat") + }) + + t.Run("directory enumeration", func(t *testing.T) { + fd, err := unix.Open(t.TempDir(), unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + require.NoError(t, err) + err = prepareTreeWith(context.Background(), fd, ".", 1, 1, unix.Openat, + func(*os.File) ([]os.DirEntry, error) { return nil, unix.EACCES }) + assert.ErrorContains(t, err, "read directory") + }) + + t.Run("descendant open", func(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "child"), nil, 0o600)) + fd, err := unix.Open(root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + require.NoError(t, err) + err = prepareTreeWith(context.Background(), fd, ".", 1, 1, + func(int, string, int, uint32) (int, error) { return -1, unix.EACCES }, + func(file *os.File) ([]os.DirEntry, error) { return file.ReadDir(-1) }) + assert.ErrorContains(t, err, "open descendant") + }) +} + +func TestPrepareOwnershipMissingPaths(t *testing.T) { + root := t.TempDir() + assert.ErrorContains(t, PrepareOwnership(context.Background(), "", ".", 1, 1), "root must not be empty") + assert.ErrorContains(t, PrepareOwnership(context.Background(), filepath.Join(root, "missing"), ".", 1, 1), "authorized root") + assert.ErrorContains(t, PrepareOwnership(context.Background(), root, "missing", 1, 1), "open target") +} + +func getOverride(t *testing.T, path string) string { + t.Helper() + buf := make([]byte, 256) + n, err := unix.Lgetxattr(path, overrideKey, buf) + if isNoAttribute(err) { + return "" + } + require.NoError(t, err) + return string(buf[:n]) +} From 40a13d41e664a247dfa2f82b368b60b9226afcdb Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 22 Sep 2026 16:05:54 +0300 Subject: [PATCH 2/3] Preserve best-effort ownership startup policy --- README.md | 17 ++- docs/ARCHITECTURE.md | 15 +- docs/MACOS.md | 76 +++++++--- docs/RELEASE_NOTES.md | 8 +- docs/SECURITY.md | 33 +++-- internal/xattr/noattr_darwin.go | 14 ++ internal/xattr/noattr_linux.go | 14 ++ internal/xattr/preparation.go | 40 ++++++ internal/xattr/walk.go | 214 +++++++++++++++++++++++++++- internal/xattr/walk_other.go | 10 ++ internal/xattr/walk_test.go | 152 ++++++++++++++++++++ microvm.go | 35 +++-- microvm_virtiofs_test.go | 133 ++++++++++++----- options.go | 29 ++-- options_example_test.go | 22 +++ options_test.go | 3 +- virtiofs/lifecycle_test.go | 92 ++++++++++++ virtiofs/prepare_unix.go | 243 +++----------------------------- virtiofs/prepare_unix_test.go | 95 +------------ 19 files changed, 807 insertions(+), 438 deletions(-) create mode 100644 internal/xattr/noattr_darwin.go create mode 100644 internal/xattr/noattr_linux.go create mode 100644 internal/xattr/preparation.go create mode 100644 options_example_test.go create mode 100644 virtiofs/lifecycle_test.go diff --git a/README.md b/README.md index 64e95c5..b457877 100644 --- a/README.md +++ b/README.md @@ -166,13 +166,16 @@ layer extraction, rootfs caching, networking setup, subprocess spawn, and post-boot hooks. It returns a `*VM` handle that you use to query status, stop, or remove the VM. -Virtio-fs ownership overrides preserve host ownership and mode. Writable mounts -with `OverrideUID` are prepared strictly before startup; read-only mounts can be -prepared explicitly when the host OS permits the required xattr operation. -Preparation reports path-specific errors, confines traversal beneath the selected -root, and supports targeted updates after startup. An unprivileged caller cannot -normally annotate an unannotated `0400` file; the permission error is actionable -and does not weaken the backing file. See +Virtio-fs ownership overrides preserve host ownership and mode. Mounts with +`OverrideUID` are prepared before startup, including read-only exports. Startup +uses bounded best-effort reporting by default: recoverable entry failures produce +one warning for the incomplete mount while safe descendants, siblings, later +mounts, and VM startup continue. Set `StrictOwnershipPreparation: true` on a +mount to fail startup before networking. The public +`virtiofs.PrepareOwnership` API is always strict and supports targeted updates +after startup. Both policies use the same descriptor-relative, symlink-confined +filesystem operations. New or changed xattrs still require host permission; no +path implicitly changes mode or ownership. See [macOS support](docs/MACOS.md#virtio-fs-shared-directory-ownership) and [release notes](docs/RELEASE_NOTES.md). diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index c77c17c..11c903b 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -76,13 +76,14 @@ in `microvm.Run()`: config (Entrypoint, Cmd, Env, WorkingDir), applying `WithInitOverride` if set. Writes the JSON file to `/.krun_config.json` in the rootfs. -5. **Prepare virtio-fs ownership** -- For writable mounts opting into - `OverrideUID`, strictly prepares `override_stat` metadata through the public - `virtiofs.PrepareOwnership` implementation. New or changed metadata needs - OS xattr-write permission, so an unannotated `0400` backing file can fail - preparation for an unprivileged caller. Validation and preparation complete - before any custom network provider starts. Read-only mounts are not prepared - automatically. +5. **Prepare virtio-fs ownership** -- For every mount opting into + `OverrideUID`, including read-only exports, prepares `override_stat` metadata + with descriptor-relative, symlink-confined traversal. Startup is best-effort + by default: recoverable entry failures are bounded in one incomplete-mount + warning while safe descendants, siblings, and subsequent mounts continue. + `StrictOwnershipPreparation` instead fails before networking. The public + `virtiofs.PrepareOwnership` API remains strictly fail-fast. All mount + configuration is validated before any metadata is stamped. 6. **Start networking** -- Networking follows one of two paths: - **Default (no `WithNetProvider`)**: Port forwards are passed to the diff --git a/docs/MACOS.md b/docs/MACOS.md index c5a28e2..0e4be80 100644 --- a/docs/MACOS.md +++ b/docs/MACOS.md @@ -88,46 +88,63 @@ The xattr is set automatically during OCI layer extraction and rootfs cloning ### virtio-fs shared directory ownership -A writable `microvm.VirtioFSMount` with `OverrideUID > 0` is strictly prepared -before networking starts. `OverrideGID` defaults to the UID. Startup fails on -inaccessible entries, malformed metadata, unsupported special files, or xattr -errors instead of continuing with partly incorrect guest ownership. This is a -deliberate compatibility change from the previous best-effort behavior. There is -no best-effort toggle for opted-in writable mounts. +A `microvm.VirtioFSMount` with `OverrideUID > 0` is prepared before +networking starts, whether its export is writable or read-only. `OverrideGID` +defaults to the UID. Startup is best-effort by default: inaccessible entries, +malformed metadata, unsupported special files, and xattr errors are retained in +a bounded report and emitted as one warning for the incomplete mount. Traversal +continues through safe accessible descendants, siblings, and subsequent mounts. +Set `StrictOwnershipPreparation: true` to fail startup on the first such error. +Root/target acquisition failures and cancellation always abort startup. ```go -microvm.WithVirtioFS(microvm.VirtioFSMount{ - Tag: "shared", - HostPath: "/srv/vm-share", - OverrideUID: 65532, - OverrideGID: 65532, -}) +microvm.WithVirtioFS( + // Backward-compatible default: report incomplete preparation and continue. + microvm.VirtioFSMount{ + Tag: "shared", HostPath: "/srv/vm-share", OverrideUID: 65532, + }, + // Mecatl data must be complete before networking or VM startup. + microvm.VirtioFSMount{ + Tag: "mecatl", HostPath: "/srv/mecatl", OverrideUID: 65532, + StrictOwnershipPreparation: true, + }, +) ``` -Read-only mounts are not prepared automatically, and explicit preparation does -not change their export flags. Prepare the backing tree first when the host OS -permits its xattr operation, then request a read-only export: +`ReadOnly` remains enforced independently by libkrun and the guest mount. It does +not skip ownership preparation or alter the backing inode's host mode: ```go -if err := virtiofs.PrepareOwnership(ctx, "/srv/vm-share", ".", 65532, 65532); err != nil { - return err -} vm, err := microvm.Run(ctx, image, microvm.WithVirtioFS(microvm.VirtioFSMount{ Tag: "shared", HostPath: "/srv/vm-share", ReadOnly: true, + OverrideUID: 65532, }), ) ``` -The same API can prepare one replaced file or subtree while a VM is running, -without rescanning siblings: +The same strict public API can prepare a newly created worktree within an +already exported stable root before it is registered for consumer-level guest +use, or one replaced file/subtree after a merge, without rescanning siblings or +restarting the VM: ```go +// The caller holds its normal guest/worktree synchronization here. +if err := virtiofs.PrepareOwnership(ctx, "/srv/vm-share", "worktrees/job-42", 65532, 65532); err != nil { + return err +} +registerWorktreeWithGuest("worktrees/job-42") + +// After a synchronized host create or replacement: if err := virtiofs.PrepareOwnership(ctx, "/srv/vm-share", "results/job-42", 65532, 65532); err != nil { return err } ``` +This changes host metadata only; no dynamic mount-add API or VM restart is +implied. The running guest or virtio-fs implementation may cache attributes, so +there is no immediate cache-invalidation or visibility guarantee. + The root must be a real directory, and the selected target must be `.` or a relative path. Trusted ancestor symlinks such as macOS `/var` are allowed, but the final root and every explicit relative component are opened without @@ -139,7 +156,24 @@ running guest. Host ownership and mode are unchanged. New metadata derives the guest mode from the host inode. Existing metadata retains its permission, set-ID, and sticky bits (including guest `chmod` changes), while preparation corrects the file type -and applies the requested uid/gid. Matching metadata is not rewritten. +and applies the requested uid/gid. Matching metadata is not rewritten. For a +sealed snapshot, guest `0600` deliberately preserves guest-owner readability +while host `0400` narrows the backing inode; prepare while it is `0600`, then +explicitly narrow it: + +```go +if err := virtiofs.PrepareOwnership(ctx, root, "snapshot", 65532, 65532); err != nil { + return err +} +if err := os.Chmod(filepath.Join(root, "snapshot"), 0o400); err != nil { + return err +} +``` + +A later matching preparation only reads the metadata, so it does not need to +rewrite the xattr. This is an explicit caller operation; preparation never calls +`chmod`, `chown`, or widens permissions. Host mode `0400` is not itself a +workaround for guest writes—use a read-only export for enforcement. Creating or changing metadata requires the host OS permission to write xattrs. An ordinary unprivileged user therefore cannot normally annotate an unannotated diff --git a/docs/RELEASE_NOTES.md b/docs/RELEASE_NOTES.md index 9b91760..b62503e 100644 --- a/docs/RELEASE_NOTES.md +++ b/docs/RELEASE_NOTES.md @@ -5,7 +5,7 @@ ### Virtio-fs ownership preparation - Added public `virtiofs.PrepareOwnership(ctx, root, relativePath, uid, gid)` for strict full-tree or targeted `user.containers.override_stat` preparation on macOS and Linux. -- Writable mounts with `OverrideUID` now use the same strict preparation before networking starts. This deliberately replaces best-effort startup behavior; failures are path-specific and abort startup. -- Read-only mounts are not prepared automatically. Explicit preparation leaves export flags and host ownership and mode unchanged. New or changed metadata still requires host xattr-write permission, so an unprivileged caller normally receives a permission error for an unannotated `0400` file. -- Existing override mode bits and `OverrideGID` defaulting are preserved. Non-opted-in and read-only mounts remain unmodified at startup, and Linux user-namespace behavior is unchanged. -- Traversal is descriptor-relative and confined beneath the authorized root: explicit symlinks fail, descendant symlinks are skipped, and malformed metadata, unsupported types, and traversal or xattr errors are fatal. +- Mounts with `OverrideUID`, including read-only exports, prepare ownership before networking. Startup is best-effort by default: recoverable entry failures produce one bounded incomplete-mount warning while safe descendants, siblings, later mounts, and startup continue. +- Added per-mount `StrictOwnershipPreparation` to abort before networking and VM startup on the first preparation failure. The public API remains strict, and cancellation or mount root/target acquisition failures always abort. +- Both policies share descriptor-relative, `O_NOFOLLOW` traversal. Explicit symlinks fail, descendant symlinks are skipped, and host ownership, mode, and export flags are unchanged. +- Existing override mode bits and `OverrideGID` defaulting are preserved. Mounts without `OverrideUID` remain unmodified, and Linux user-namespace behavior is unchanged. New or changed metadata still requires host xattr-write permission; matching metadata is not rewritten. diff --git a/docs/SECURITY.md b/docs/SECURITY.md index e4a1f8d..e551a55 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -469,14 +469,22 @@ virtiofs server reports intended guest ownership without changing host uid, gid, or mode. This is the mechanism podman uses on macOS. Image extraction and hooks retain their single-entry best-effort behavior for compatibility. -For shared virtio-fs trees, writable mounts with `OverrideUID > 0` use the strict -public `virtiofs.PrepareOwnership` implementation before networking starts; -read-only mounts are not modified automatically. Matching existing metadata is -read without a rewrite, but new or changed metadata requires OS permission to -write xattrs. Thus an unprivileged caller can receive a path-specific permission -error for an unannotated `0400` backing file, while host mode and IDs remain -unchanged. New metadata derives guest mode from the host inode; later preparation -preserves guest mode bits recorded in `override_stat`. It uses descriptor-relative, +For shared virtio-fs trees, mounts with `OverrideUID > 0`, including read-only +exports, use the same descriptor-relative ownership walker before networking. +Startup is best-effort by default: recoverable per-entry failures are retained in +a bounded report, emitted as one warning for the incomplete mount, and do not +stop safe descendants, siblings, later mounts, or VM startup. Setting +`StrictOwnershipPreparation` on a mount makes the first failure fatal. Root or +target acquisition failures and cancellation are always fatal. The public +`virtiofs.PrepareOwnership` API is always strict. + +Matching existing metadata is read without a rewrite, but new or changed +metadata requires OS permission to write xattrs. Thus an unprivileged caller can +receive a path-specific permission error for an unannotated `0400` backing file, +while host mode and IDs remain unchanged. New metadata derives guest mode from +the host inode; later preparation preserves guest mode bits recorded in +`override_stat`. Read-only export enforcement is independent and does not skip +preparation or change the backing inode. The walker uses descriptor-relative, `O_NOFOLLOW` traversal after opening the authorized root. Explicit root/target symlinks fail, descendant symlinks are skipped, and regular files/directories are the only supported types. This prevents mutable intermediate symlinks from @@ -486,10 +494,11 @@ rename, creation, replacement, and guest chmod. Hard links authorize their shared inode, including names outside the tree. There are no cache invalidation or atomic guest-visibility guarantees. -Malformed existing xattrs and read, write, or traversal failures are fatal and -path-specific. Valid matching metadata is not rewritten. On Linux, this does -not alter user-namespace behavior; it only prepares metadata for libkrun -versions that consume `override_stat`. +Public strict errors and startup incomplete reports are path-specific. Valid +matching metadata is not rewritten. Neither policy changes host ownership or +mode, invokes `chmod`/`chown`, widens permissions, watches the tree, or claims +atomicity. On Linux, this does not alter user-namespace behavior; it only +prepares metadata for libkrun versions that consume `override_stat`. ## File Permissions diff --git a/internal/xattr/noattr_darwin.go b/internal/xattr/noattr_darwin.go new file mode 100644 index 0000000..f5e8807 --- /dev/null +++ b/internal/xattr/noattr_darwin.go @@ -0,0 +1,14 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build darwin + +package xattr + +import ( + "errors" + + "golang.org/x/sys/unix" +) + +func isNoAttribute(err error) bool { return errors.Is(err, unix.ENOATTR) } diff --git a/internal/xattr/noattr_linux.go b/internal/xattr/noattr_linux.go new file mode 100644 index 0000000..66a61ef --- /dev/null +++ b/internal/xattr/noattr_linux.go @@ -0,0 +1,14 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package xattr + +import ( + "errors" + + "golang.org/x/sys/unix" +) + +func isNoAttribute(err error) bool { return errors.Is(err, unix.ENODATA) } diff --git a/internal/xattr/preparation.go b/internal/xattr/preparation.go new file mode 100644 index 0000000..477621c --- /dev/null +++ b/internal/xattr/preparation.go @@ -0,0 +1,40 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package xattr + +import ( + "fmt" + "strings" +) + +const maxPreparationErrors = 16 + +// PreparationReport describes recoverable entries that could not be prepared. +// Error details are bounded so a large tree cannot produce an unbounded report. +type PreparationReport struct { + Errors []error + Omitted int +} + +// Complete reports whether every selected entry was prepared. +func (r PreparationReport) Complete() bool { return len(r.Errors) == 0 && r.Omitted == 0 } + +func (r PreparationReport) Error() string { + parts := make([]string, 0, len(r.Errors)+1) + for _, err := range r.Errors { + parts = append(parts, err.Error()) + } + if r.Omitted > 0 { + parts = append(parts, fmt.Sprintf("%d additional errors omitted", r.Omitted)) + } + return strings.Join(parts, "; ") +} + +func (r *PreparationReport) add(err error) { + if len(r.Errors) < maxPreparationErrors { + r.Errors = append(r.Errors, err) + } else { + r.Omitted++ + } +} diff --git a/internal/xattr/walk.go b/internal/xattr/walk.go index cdf39fc..45807fa 100644 --- a/internal/xattr/walk.go +++ b/internal/xattr/walk.go @@ -7,17 +7,223 @@ package xattr import ( "context" + "errors" "fmt" "math" + "os" + "path/filepath" + "strconv" + "strings" - "github.com/stacklok/go-microvm/virtiofs" + "golang.org/x/sys/unix" ) -// SetOverrideStatTree strictly prepares the entire root for virtio-fs ownership -// mapping. New code should call [virtiofs.PrepareOwnership] directly. +// PrepareOwnership prepares a selected tree using descriptor-relative, +// symlink-confined traversal. In strict mode the first entry failure is +// returned. Otherwise recoverable entry failures are collected and traversal +// continues where it is safe; target acquisition and cancellation always fail. +func PrepareOwnership(ctx context.Context, root, relativePath string, uid, gid uint32, strict bool) (PreparationReport, error) { + var report PreparationReport + if root == "" { + return report, errors.New("virtiofs ownership root must not be empty") + } + parts, err := validateTarget(relativePath) + if err != nil { + return report, err + } + if err := ctx.Err(); err != nil { + return report, fmt.Errorf("prepare ownership %q: %w", relativePath, err) + } + fd, err := acquireTarget(root, relativePath, parts) + if err != nil { + return report, err + } + err = prepareTree(ctx, fd, relativePath, uid, gid, strict, &report) + return report, err +} + +// SetOverrideStatTree strictly prepares the entire root for virtio-fs ownership mapping. func SetOverrideStatTree(root string, uid, gid int) error { if uid < 0 || gid < 0 || uint64(uid) > math.MaxUint32 || uint64(gid) > math.MaxUint32 { return fmt.Errorf("override_stat uid/gid out of uint32 range: %d:%d", uid, gid) } - return virtiofs.PrepareOwnership(context.Background(), root, ".", uint32(uid), uint32(gid)) + _, err := PrepareOwnership(context.Background(), root, ".", uint32(uid), uint32(gid), true) + return err +} + +func acquireTarget(root, relativePath string, parts []string) (int, error) { + root = filepath.Clean(root) + fd, err := unix.Open(root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err != nil { + return -1, fmt.Errorf("open authorized root %q without following symlinks: %w", root, err) + } + if len(parts) == 0 { + return fd, nil + } + current := fd + for i, part := range parts { + flags := unix.O_RDONLY | unix.O_NOFOLLOW | unix.O_CLOEXEC | unix.O_NONBLOCK + if i < len(parts)-1 { + flags |= unix.O_DIRECTORY + } + next, openErr := unix.Openat(current, part, flags, 0) + if closeErr := unix.Close(current); closeErr != nil && openErr == nil { + if next >= 0 { + _ = unix.Close(next) + } + return -1, fmt.Errorf("close target parent %q: %w", filepath.Join(parts[:i]...), closeErr) + } + if openErr != nil { + return -1, fmt.Errorf("open target %q without following symlinks: %w", relativePath, openErr) + } + current = next + } + return current, nil +} + +func validateTarget(relativePath string) ([]string, error) { + if relativePath == "" { + return nil, errors.New("virtiofs ownership target must not be empty") + } + if filepath.IsAbs(relativePath) { + return nil, fmt.Errorf("virtiofs ownership target %q must be relative", relativePath) + } + if relativePath == "." { + return nil, nil + } + parts := strings.Split(relativePath, string(filepath.Separator)) + for _, part := range parts { + if part == "" || part == "." || part == ".." { + return nil, fmt.Errorf("virtiofs ownership target %q contains an invalid path component", relativePath) + } + } + return parts, nil +} + +func prepareTree(ctx context.Context, fd int, displayPath string, uid, gid uint32, strict bool, report *PreparationReport) error { + return prepareTreeWith(ctx, fd, displayPath, uid, gid, strict, report, unix.Openat, func(file *os.File) ([]os.DirEntry, error) { + return file.ReadDir(-1) + }) +} + +func prepareTreeWith(ctx context.Context, fd int, displayPath string, uid, gid uint32, strict bool, report *PreparationReport, openat func(int, string, int, uint32) (int, error), readDir func(*os.File) ([]os.DirEntry, error)) (retErr error) { + file := os.NewFile(uintptr(fd), displayPath) + if file == nil { + _ = unix.Close(fd) + return fmt.Errorf("open target %q: invalid file descriptor", displayPath) + } + defer func() { + if err := file.Close(); err != nil { + closeErr := fmt.Errorf("close %q: %w", displayPath, err) + if strict && retErr == nil { + retErr = closeErr + } else if !strict { + report.add(closeErr) + } + } + }() + if err := ctx.Err(); err != nil { + return fmt.Errorf("prepare ownership %q: %w", displayPath, err) + } + var stat unix.Stat_t + if err := unix.Fstat(fd, &stat); err != nil { + return handleEntryError(fmt.Errorf("stat %q: %w", displayPath, err), strict, report) + } + typeBits := uint32(stat.Mode) & unix.S_IFMT + if typeBits != unix.S_IFDIR && typeBits != unix.S_IFREG { + return handleEntryError(fmt.Errorf("prepare ownership %q: unsupported file type (mode %#o)", displayPath, stat.Mode), strict, report) + } + if err := prepareEntry(fd, displayPath, uid, gid, uint32(stat.Mode)); err != nil { + if strict { + return err + } + report.add(err) + } + if typeBits != unix.S_IFDIR { + return nil + } + entries, err := readDir(file) + if err != nil { + return handleEntryError(fmt.Errorf("read directory %q: %w", displayPath, err), strict, report) + } + for _, entry := range entries { + childPath := filepath.Join(displayPath, entry.Name()) + if err := ctx.Err(); err != nil { + return fmt.Errorf("prepare ownership %q: %w", childPath, err) + } + child, err := openat(fd, entry.Name(), unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_CLOEXEC|unix.O_NONBLOCK, 0) + if err != nil { + if errors.Is(err, unix.ELOOP) { + continue + } + err = fmt.Errorf("open descendant %q without following symlinks: %w", childPath, err) + if strict { + return err + } + report.add(err) + continue + } + if err := prepareTreeWith(ctx, child, childPath, uid, gid, strict, report, openat, readDir); err != nil { + return err + } + } + return nil +} + +func handleEntryError(err error, strict bool, report *PreparationReport) error { + if strict { + return err + } + report.add(err) + return nil +} + +func prepareEntry(fd int, path string, uid, gid, hostMode uint32) error { + return prepareEntryWith(fd, path, uid, gid, hostMode, unix.Fgetxattr, unix.Fsetxattr) +} + +func prepareEntryWith(fd int, path string, uid, gid, hostMode uint32, getxattr func(int, string, []byte) (int, error), setxattr func(int, string, []byte, int) error) error { + mode := hostMode + buf := make([]byte, 256) + n, err := getxattr(fd, overrideKey, buf) + if err == nil { + currentUID, currentGID, currentMode, parseErr := parseOverride(string(buf[:n])) + if parseErr != nil { + return fmt.Errorf("read existing override_stat on %q: %w", path, parseErr) + } + mode = hostMode&unix.S_IFMT | currentMode&0o7777 + if currentUID == uid && currentGID == gid && currentMode == mode { + return nil + } + } else if !isNoAttribute(err) { + return fmt.Errorf("read override_stat on %q: %w", path, err) + } + desired := fmt.Sprintf("%d:%d:0%o", uid, gid, mode) + if err := setxattr(fd, overrideKey, []byte(desired), 0); err != nil { + return fmt.Errorf("write override_stat on %q: %w", path, err) + } + return nil +} + +func parseOverride(value string) (uint32, uint32, uint32, error) { + parts := strings.Split(value, ":") + if len(parts) != 3 { + return 0, 0, 0, fmt.Errorf("malformed value %q", value) + } + uid, err := strconv.ParseUint(parts[0], 10, 32) + if err != nil { + return 0, 0, 0, fmt.Errorf("malformed uid in value %q: %w", value, err) + } + gid, err := strconv.ParseUint(parts[1], 10, 32) + if err != nil { + return 0, 0, 0, fmt.Errorf("malformed gid in value %q: %w", value, err) + } + mode, err := strconv.ParseUint(parts[2], 8, 32) + if err != nil || mode > 0o177777 { + if err == nil { + err = errors.New("mode exceeds POSIX st_mode bits") + } + return 0, 0, 0, fmt.Errorf("malformed mode in value %q: %w", value, err) + } + return uint32(uid), uint32(gid), uint32(mode), nil } diff --git a/internal/xattr/walk_other.go b/internal/xattr/walk_other.go index edd6e6c..e478060 100644 --- a/internal/xattr/walk_other.go +++ b/internal/xattr/walk_other.go @@ -5,5 +5,15 @@ package xattr +import ( + "context" + "errors" +) + +// PrepareOwnership reports that override_stat preparation is unavailable. +func PrepareOwnership(_ context.Context, _, _ string, _, _ uint32, _ bool) (PreparationReport, error) { + return PreparationReport{}, errors.New("virtiofs ownership preparation is unsupported on this platform") +} + // SetOverrideStatTree is a no-op on platforms without xattr support. func SetOverrideStatTree(_ string, _, _ int) error { return nil } diff --git a/internal/xattr/walk_test.go b/internal/xattr/walk_test.go index 9ddf794..3397d85 100644 --- a/internal/xattr/walk_test.go +++ b/internal/xattr/walk_test.go @@ -6,6 +6,8 @@ package xattr import ( + "context" + "fmt" "os" "path/filepath" "testing" @@ -126,6 +128,156 @@ func TestSetOverrideStatTree_DifferentUIDGID(t *testing.T) { assert.Contains(t, val, "1000:2000:", "file should have uid=1000 gid=2000") } +func TestPrepareOwnershipBestEffortContinuesAfterEntryFailure(t *testing.T) { + root := t.TempDir() + dir := filepath.Join(root, "broken-dir") + require.NoError(t, os.Mkdir(dir, 0o700)) + require.NoError(t, unix.Lsetxattr(dir, overrideKey, []byte("malformed"), 0)) + descendant := filepath.Join(dir, "descendant") + sibling := filepath.Join(root, "sibling") + require.NoError(t, os.WriteFile(descendant, nil, 0o600)) + require.NoError(t, os.WriteFile(sibling, nil, 0o640)) + + report, err := PrepareOwnership(context.Background(), root, ".", 42, 43, false) + require.NoError(t, err) + require.False(t, report.Complete()) + assert.Contains(t, report.Error(), "malformed") + assert.Equal(t, "42:43:0100600", readXattrOpt(t, descendant)) + assert.Equal(t, "42:43:0100640", readXattrOpt(t, sibling)) +} + +func TestPrepareOwnershipBestEffortBoundsFailures(t *testing.T) { + root := t.TempDir() + for i := range maxPreparationErrors + 3 { + path := filepath.Join(root, fmt.Sprintf("broken-%02d", i)) + require.NoError(t, os.WriteFile(path, nil, 0o600)) + require.NoError(t, unix.Lsetxattr(path, overrideKey, []byte("malformed"), 0)) + } + report, err := PrepareOwnership(context.Background(), root, ".", 42, 43, false) + require.NoError(t, err) + assert.Len(t, report.Errors, maxPreparationErrors) + assert.Equal(t, 3, report.Omitted) + assert.Contains(t, report.Error(), "3 additional errors omitted") +} + +func TestPrepareOwnershipBestEffortKeepsSymlinkConfined(t *testing.T) { + root := t.TempDir() + external := filepath.Join(t.TempDir(), "external") + require.NoError(t, os.WriteFile(external, nil, 0o600)) + require.NoError(t, os.Symlink(external, filepath.Join(root, "link"))) + + report, err := PrepareOwnership(context.Background(), root, ".", 42, 43, false) + require.NoError(t, err) + assert.True(t, report.Complete()) + assert.Empty(t, readXattrOpt(t, external)) +} + +func TestPrepareOwnershipBestEffortDoesNotSwallowCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _, err := PrepareOwnership(ctx, t.TempDir(), ".", 42, 43, false) + assert.ErrorIs(t, err, context.Canceled) +} + +func TestPrepareOwnershipInjectedFailures(t *testing.T) { + t.Run("xattr read oversized", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "file") + require.NoError(t, os.WriteFile(path, nil, 0o600)) + require.NoError(t, unix.Lsetxattr(path, overrideKey, make([]byte, 300), 0)) + file, err := os.Open(path) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, file.Close()) }) + assert.ErrorContains(t, prepareEntry(int(file.Fd()), path, 1, 1, unix.S_IFREG|0o600), "read override_stat") + }) + + t.Run("xattr write strict", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "file") + require.NoError(t, os.WriteFile(path, nil, 0o600)) + file, err := os.Open(path) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, file.Close()) }) + err = prepareEntryWith(int(file.Fd()), path, 1, 1, unix.S_IFREG|0o600, unix.Fgetxattr, + func(int, string, []byte, int) error { return unix.EROFS }) + assert.ErrorContains(t, err, "write override_stat") + }) + + t.Run("directory enumeration strict", func(t *testing.T) { + fd, err := unix.Open(t.TempDir(), unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + require.NoError(t, err) + report := PreparationReport{} + err = prepareTreeWith(context.Background(), fd, ".", 1, 1, true, &report, unix.Openat, + func(*os.File) ([]os.DirEntry, error) { return nil, unix.EACCES }) + assert.ErrorContains(t, err, "read directory") + }) + + t.Run("descendant open best effort continues sibling", func(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "blocked"), nil, 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(root, "safe"), nil, 0o640)) + fd, err := unix.Open(root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + require.NoError(t, err) + report := PreparationReport{} + err = prepareTreeWith(context.Background(), fd, ".", 1, 1, false, &report, + func(parent int, name string, flags int, mode uint32) (int, error) { + if name == "blocked" { + return -1, unix.EACCES + } + return unix.Openat(parent, name, flags, mode) + }, func(file *os.File) ([]os.DirEntry, error) { return file.ReadDir(-1) }) + require.NoError(t, err) + assert.Contains(t, report.Error(), "blocked") + assert.Equal(t, "1:1:0100640", readXattrOpt(t, filepath.Join(root, "safe"))) + }) +} + +func TestPrepareOwnershipPinnedTargetCannotEscapeAfterReplacement(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "target") + require.NoError(t, os.Mkdir(target, 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(target, "inside"), nil, 0o600)) + external := t.TempDir() + externalFile := filepath.Join(external, "outside") + require.NoError(t, os.WriteFile(externalFile, nil, 0o600)) + + parts, err := validateTarget("target") + require.NoError(t, err) + fd, err := acquireTarget(root, "target", parts) + require.NoError(t, err) + oldTarget := filepath.Join(root, "old-target") + require.NoError(t, os.Rename(target, oldTarget)) + require.NoError(t, os.Symlink(external, target)) + report := PreparationReport{} + require.NoError(t, prepareTree(context.Background(), fd, "target", 42, 43, true, &report)) + assert.Contains(t, readXattrOpt(t, filepath.Join(oldTarget, "inside")), "42:43:") + assert.Empty(t, readXattrOpt(t, externalFile)) +} + +func TestPrepareOwnershipDescendantReplacementCannotEscape(t *testing.T) { + root := t.TempDir() + victim := filepath.Join(root, "victim") + replaced := filepath.Join(root, "replaced-victim") + external := t.TempDir() + externalFile := filepath.Join(external, "secret") + require.NoError(t, os.Mkdir(victim, 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(victim, "inside"), nil, 0o600)) + require.NoError(t, os.WriteFile(externalFile, nil, 0o600)) + fd, err := unix.Open(root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + require.NoError(t, err) + replacedOnce := false + report := PreparationReport{} + err = prepareTreeWith(context.Background(), fd, ".", 42, 43, false, &report, + func(parent int, name string, flags int, mode uint32) (int, error) { + if !replacedOnce && name == "victim" { + replacedOnce = true + require.NoError(t, os.Rename(victim, replaced)) + require.NoError(t, os.Symlink(external, victim)) + } + return unix.Openat(parent, name, flags, mode) + }, func(file *os.File) ([]os.DirEntry, error) { return file.ReadDir(-1) }) + require.NoError(t, err) + assert.Empty(t, readXattrOpt(t, externalFile)) +} + // readXattrOpt reads the override_stat xattr and returns its value, or // empty string if the xattr is not set. func readXattrOpt(t *testing.T, path string) string { diff --git a/microvm.go b/microvm.go index 33391f6..12923f6 100644 --- a/microvm.go +++ b/microvm.go @@ -37,11 +37,11 @@ import ( "github.com/stacklok/go-microvm/hypervisor" "github.com/stacklok/go-microvm/hypervisor/libkrun" "github.com/stacklok/go-microvm/image" + "github.com/stacklok/go-microvm/internal/xattr" "github.com/stacklok/go-microvm/net/firewall" "github.com/stacklok/go-microvm/net/hosted" rootfspkg "github.com/stacklok/go-microvm/rootfs" "github.com/stacklok/go-microvm/state" - "github.com/stacklok/go-microvm/virtiofs" ) // Run pulls an OCI image and boots it as a microVM. It is the primary entry @@ -210,8 +210,7 @@ func Run(ctx context.Context, imageRef string, opts ...Option) (*VM, error) { } // 5. Validate and prepare ownership overrides before starting networking. - // Read-only backing directories are deliberately not modified at startup; - // callers can prepare them explicitly with virtiofs.PrepareOwnership. + // Validation is completed for every mount before any host metadata is changed. for _, m := range cfg.virtioFS { if m.OverrideUID < 0 || m.OverrideGID < 0 { return nil, fmt.Errorf("virtiofs mount %q: OverrideUID/OverrideGID must be non-negative", m.Tag) @@ -226,17 +225,25 @@ func Run(ctx context.Context, imageRef string, opts ...Option) (*VM, error) { _, xattrSpan := tracer.Start(ctx, "microvm.VirtioFSOverrideStat") for _, m := range cfg.virtioFS { - if m.OverrideUID > 0 && !m.ReadOnly { - gid := m.OverrideGID - if gid == 0 { - gid = m.OverrideUID - } - if err := virtiofs.PrepareOwnership(ctx, m.HostPath, ".", uint32(m.OverrideUID), uint32(gid)); err != nil { - xattrSpan.RecordError(err) - xattrSpan.SetStatus(codes.Error, err.Error()) - xattrSpan.End() - return nil, fmt.Errorf("prepare virtiofs mount %q ownership: %w", m.Tag, err) - } + if m.OverrideUID == 0 { + continue + } + gid := m.OverrideGID + if gid == 0 { + gid = m.OverrideUID + } + report, err := xattr.PrepareOwnership(ctx, m.HostPath, ".", uint32(m.OverrideUID), uint32(gid), m.StrictOwnershipPreparation) + if err != nil { + xattrSpan.RecordError(err) + xattrSpan.SetStatus(codes.Error, err.Error()) + xattrSpan.End() + return nil, fmt.Errorf("prepare virtiofs mount %q ownership: %w", m.Tag, err) + } + if !report.Complete() { + xattrSpan.RecordError(report) + xattrSpan.SetStatus(codes.Error, "ownership preparation incomplete") + slog.Warn("virtiofs ownership preparation incomplete", + "tag", m.Tag, "path", m.HostPath, "errors", report.Error()) } } xattrSpan.End() diff --git a/microvm_virtiofs_test.go b/microvm_virtiofs_test.go index 7ddfcd0..979a49b 100644 --- a/microvm_virtiofs_test.go +++ b/microvm_virtiofs_test.go @@ -55,26 +55,21 @@ func TestRunOwnershipFailurePreventsNetworkAndBackendStart(t *testing.T) { _, err := Run(context.Background(), "unused", WithDataDir(dataDir), WithPreflightChecker(preflight.NewEmpty()), WithRootFSPath(rootfs), WithBackend(backend), WithNetProvider(provider), - WithVirtioFS(VirtioFSMount{Tag: "share", HostPath: share, OverrideUID: 65532}), + WithVirtioFS(VirtioFSMount{Tag: "share", HostPath: share, OverrideUID: 65532, StrictOwnershipPreparation: true}), ) require.ErrorContains(t, err, "prepare virtiofs mount \"share\" ownership") assert.Zero(t, provider.startCalls) assert.Zero(t, backend.startCalls) } -func TestExplicitlyPreparedReadOnlyMountRemainsReadOnly(t *testing.T) { +func TestRunPreparesReadOnlyMountAndPreservesReadOnlyFlags(t *testing.T) { dataDir := t.TempDir() rootfs := filepath.Join(dataDir, "rootfs") share := filepath.Join(dataDir, "share") require.NoError(t, os.Mkdir(rootfs, 0o755)) require.NoError(t, os.Mkdir(share, 0o700)) data := filepath.Join(share, "data") - // This is a wiring-only preannotated snapshot: setup stamps guest 0400 - // while the file is writable, then makes the host backing inode 0400. - require.NoError(t, os.WriteFile(data, []byte("readonly"), 0o600)) - require.NoError(t, unix.Lsetxattr(data, "user.containers.override_stat", []byte("65532:65532:0100400"), 0)) - require.NoError(t, virtiofs.PrepareOwnership(context.Background(), share, ".", 65532, 65532)) - require.NoError(t, os.Chmod(data, 0o400)) + require.NoError(t, os.WriteFile(data, []byte("readonly"), 0o640)) beforeDir, err := os.Stat(share) require.NoError(t, err) beforeData, err := os.Stat(data) @@ -96,8 +91,8 @@ func TestExplicitlyPreparedReadOnlyMountRemainsReadOnly(t *testing.T) { afterData, err := os.Stat(data) require.NoError(t, err) assert.Equal(t, os.FileMode(0o700), afterDir.Mode().Perm()) - assert.Equal(t, os.FileMode(0o400), afterData.Mode().Perm()) - assert.Equal(t, "65532:65532:0100400", readOverrideForRunTest(t, data)) + assert.Equal(t, os.FileMode(0o640), afterData.Mode().Perm()) + assert.Equal(t, "65532:65532:0100640", readOverrideForRunTest(t, data)) assert.Equal(t, beforeDir.Sys().(*syscall.Stat_t).Uid, afterDir.Sys().(*syscall.Stat_t).Uid) assert.Equal(t, beforeDir.Sys().(*syscall.Stat_t).Gid, afterDir.Sys().(*syscall.Stat_t).Gid) assert.Equal(t, beforeData.Sys().(*syscall.Stat_t).Uid, afterData.Sys().(*syscall.Stat_t).Uid) @@ -110,33 +105,97 @@ func TestExplicitlyPreparedReadOnlyMountRemainsReadOnly(t *testing.T) { require.Equal(t, []vmconfig.VirtioFSMountInfo{{Tag: "share", ReadOnly: true}}, guestConfig.VirtioFSMounts) } -func TestRunDoesNotTraverseNonOptInOrReadOnlyMounts(t *testing.T) { - for _, tt := range []struct { - name string - mount VirtioFSMount - }{ - {name: "non-opt-in", mount: VirtioFSMount{Tag: "share"}}, - {name: "read-only", mount: VirtioFSMount{Tag: "share", ReadOnly: true, OverrideUID: 65532}}, - } { - t.Run(tt.name, func(t *testing.T) { - dataDir := t.TempDir() - rootfs := filepath.Join(dataDir, "rootfs") - share := filepath.Join(dataDir, "share") - require.NoError(t, os.Mkdir(rootfs, 0o755)) - require.NoError(t, os.Mkdir(share, 0o755)) - require.NoError(t, unix.Lsetxattr(share, "user.containers.override_stat", []byte("malformed"), 0)) - tt.mount.HostPath = share - backend := &mockBackend{startHandle: &mockVMHandle{id: "42", alive: true}} - vm, err := Run(context.Background(), "unused", - WithDataDir(dataDir), WithPreflightChecker(preflight.NewEmpty()), - WithRootFSPath(rootfs), WithBackend(backend), WithVirtioFS(tt.mount), - ) - require.NoError(t, err) - t.Cleanup(func() { _ = vm.Stop(context.Background()) }) - assert.Equal(t, "malformed", readOverrideForRunTest(t, share)) - assert.Equal(t, 1, backend.startCalls) - }) - } +func TestRunDoesNotTraverseMountWithoutOverride(t *testing.T) { + dataDir := t.TempDir() + rootfs := filepath.Join(dataDir, "rootfs") + share := filepath.Join(dataDir, "share") + require.NoError(t, os.Mkdir(rootfs, 0o755)) + require.NoError(t, os.Mkdir(share, 0o755)) + require.NoError(t, unix.Lsetxattr(share, "user.containers.override_stat", []byte("malformed"), 0)) + backend := &mockBackend{startHandle: &mockVMHandle{id: "42", alive: true}} + vm, err := Run(context.Background(), "unused", + WithDataDir(dataDir), WithPreflightChecker(preflight.NewEmpty()), + WithRootFSPath(rootfs), WithBackend(backend), + WithVirtioFS(VirtioFSMount{Tag: "share", HostPath: share, ReadOnly: true, StrictOwnershipPreparation: true}), + ) + require.NoError(t, err) + t.Cleanup(func() { _ = vm.Stop(context.Background()) }) + assert.Equal(t, "malformed", readOverrideForRunTest(t, share)) + assert.Equal(t, 1, backend.startCalls) +} + +func TestRunBestEffortOwnershipDoesNotSwallowCancellation(t *testing.T) { + dataDir := t.TempDir() + rootfs := filepath.Join(dataDir, "rootfs") + share := filepath.Join(dataDir, "share") + require.NoError(t, os.Mkdir(rootfs, 0o755)) + require.NoError(t, os.Mkdir(share, 0o755)) + backend := &mockBackend{startHandle: &mockVMHandle{id: "42", alive: true}} + provider := &mockNetProvider{} + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := Run(ctx, "unused", + WithDataDir(dataDir), WithPreflightChecker(preflight.NewEmpty()), + WithRootFSPath(rootfs), WithBackend(backend), WithNetProvider(provider), + WithVirtioFS(VirtioFSMount{Tag: "share", HostPath: share, OverrideUID: 65532}), + ) + require.ErrorIs(t, err, context.Canceled) + assert.Zero(t, provider.startCalls) + assert.Zero(t, backend.startCalls) +} + +func TestRunValidatesAllOwnershipMountsBeforeStamping(t *testing.T) { + dataDir := t.TempDir() + rootfs := filepath.Join(dataDir, "rootfs") + first := filepath.Join(dataDir, "first") + require.NoError(t, os.Mkdir(rootfs, 0o755)) + require.NoError(t, os.Mkdir(first, 0o755)) + backend := &mockBackend{startHandle: &mockVMHandle{id: "42", alive: true}} + + _, err := Run(context.Background(), "unused", + WithDataDir(dataDir), WithPreflightChecker(preflight.NewEmpty()), + WithRootFSPath(rootfs), WithBackend(backend), + WithVirtioFS( + VirtioFSMount{Tag: "first", HostPath: first, OverrideUID: 65532}, + VirtioFSMount{Tag: "invalid", HostPath: first, OverrideGID: 1}, + ), + ) + require.ErrorContains(t, err, "OverrideGID set without OverrideUID") + _, xattrErr := unix.Lgetxattr(first, "user.containers.override_stat", make([]byte, 256)) + assert.Error(t, xattrErr) + assert.Zero(t, backend.startCalls) +} + +func TestRunBestEffortOwnershipContinuesMountAndSubsequentMounts(t *testing.T) { + dataDir := t.TempDir() + rootfs := filepath.Join(dataDir, "rootfs") + first := filepath.Join(dataDir, "first") + second := filepath.Join(dataDir, "second") + require.NoError(t, os.Mkdir(rootfs, 0o755)) + require.NoError(t, os.Mkdir(first, 0o755)) + require.NoError(t, os.Mkdir(second, 0o755)) + require.NoError(t, unix.Lsetxattr(first, "user.containers.override_stat", []byte("malformed"), 0)) + firstChild := filepath.Join(first, "child") + secondChild := filepath.Join(second, "child") + require.NoError(t, os.WriteFile(firstChild, nil, 0o600)) + require.NoError(t, os.WriteFile(secondChild, nil, 0o640)) + + backend := &mockBackend{startHandle: &mockVMHandle{id: "42", alive: true}} + vm, err := Run(context.Background(), "unused", + WithDataDir(dataDir), WithPreflightChecker(preflight.NewEmpty()), + WithRootFSPath(rootfs), WithBackend(backend), + WithVirtioFS( + VirtioFSMount{Tag: "first", HostPath: first, OverrideUID: 65532}, + VirtioFSMount{Tag: "second", HostPath: second, OverrideUID: 65532}, + ), + ) + require.NoError(t, err) + t.Cleanup(func() { _ = vm.Stop(context.Background()) }) + assert.Equal(t, "malformed", readOverrideForRunTest(t, first)) + assert.Equal(t, "65532:65532:0100600", readOverrideForRunTest(t, firstChild)) + assert.Equal(t, "65532:65532:0100640", readOverrideForRunTest(t, secondChild)) + assert.Equal(t, 1, backend.startCalls) } func TestPrepareOwnershipAfterRunTargetsReplacementWithoutRestart(t *testing.T) { diff --git a/options.go b/options.go index 477762f..6a47d77 100644 --- a/options.go +++ b/options.go @@ -62,25 +62,28 @@ type VirtioFSMount struct { // krun_add_virtiofs3 API; the runner fails before VM start if it is unavailable // or rejects the configuration. ReadOnly bool - // OverrideUID, when > 0 on a writable mount, strictly prepares every - // regular file and directory under HostPath with libkrun's - // user.containers.override_stat xattr before networking or VM startup. The - // guest sees OverrideUID/OverrideGID while host ownership and mode remain - // unchanged. Symlinks below HostPath are skipped; unsupported file types, - // inaccessible entries, malformed xattrs, and xattr failures abort startup. - // New or changed xattrs require the OS permission to write xattrs; an - // unannotated 0400 backing file can therefore fail preparation with EACCES. - // A new xattr derives its guest mode from the host inode, and an existing - // xattr preserves its guest mode bits. + // OverrideUID, when > 0, prepares every regular file and directory under + // HostPath with libkrun's user.containers.override_stat xattr before + // networking or VM startup. This applies to both writable and read-only + // exports; ReadOnly flags are not changed. The guest sees OverrideUID and + // OverrideGID while host ownership and mode remain unchanged. Symlinks below + // HostPath are skipped. By default, recoverable per-entry failures are logged + // as one incomplete-mount warning and preparation continues through safe, + // accessible descendants, sibling entries, and subsequent mounts. // // A zero value means "no startup override." Since 0 is the zero value for - // int, overriding to UID 0 is not supported through this field. Read-only - // mounts are not modified automatically; prepare their backing directory - // explicitly with virtiofs.PrepareOwnership before calling Run. + // int, overriding to UID 0 is not supported through this field. A new xattr + // derives its guest mode from the host inode; an existing xattr preserves its + // guest mode bits. OverrideUID int // OverrideGID sets the group ID for the override_stat xattr. // When 0 and OverrideUID > 0, defaults to OverrideUID. OverrideGID int + // StrictOwnershipPreparation makes any ownership preparation failure abort + // startup before networking and VM start. It applies only when OverrideUID is + // greater than zero; otherwise it is a no-op. Both policies use the same + // descriptor-relative, no-symlink traversal and filesystem permission rules. + StrictOwnershipPreparation bool } // EgressPolicy restricts outbound VM traffic to specific DNS hostnames. diff --git a/options_example_test.go b/options_example_test.go new file mode 100644 index 0000000..9c681c7 --- /dev/null +++ b/options_example_test.go @@ -0,0 +1,22 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package microvm_test + +import ( + microvm "github.com/stacklok/go-microvm" +) + +func ExampleVirtioFSMount_ownershipPreparationPolicy() { + _ = microvm.WithVirtioFS( + // The default reports incomplete ownership preparation and continues. + microvm.VirtioFSMount{ + Tag: "workspace", HostPath: "/srv/workspace", OverrideUID: 65532, + }, + // Mecatl requires complete ownership metadata before startup. + microvm.VirtioFSMount{ + Tag: "mecatl", HostPath: "/srv/mecatl", OverrideUID: 65532, + StrictOwnershipPreparation: true, + }, + ) +} diff --git a/options_test.go b/options_test.go index 7c5b7e3..74a1875 100644 --- a/options_test.go +++ b/options_test.go @@ -139,13 +139,14 @@ func TestWithVirtioFS(t *testing.T) { cfg := defaultConfig() WithVirtioFS( - VirtioFSMount{Tag: "workspace", HostPath: "/home/user/src"}, + VirtioFSMount{Tag: "workspace", HostPath: "/home/user/src", StrictOwnershipPreparation: true}, ).apply(cfg) require.Len(t, cfg.virtioFS, 1) assert.Equal(t, "workspace", cfg.virtioFS[0].Tag) assert.Equal(t, "/home/user/src", cfg.virtioFS[0].HostPath) assert.False(t, cfg.virtioFS[0].ReadOnly) + assert.True(t, cfg.virtioFS[0].StrictOwnershipPreparation) } func TestWithVirtioFS_ReadOnly(t *testing.T) { diff --git a/virtiofs/lifecycle_test.go b/virtiofs/lifecycle_test.go new file mode 100644 index 0000000..ebc3e3c --- /dev/null +++ b/virtiofs/lifecycle_test.go @@ -0,0 +1,92 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build darwin || linux + +package virtiofs_test + +import ( + "context" + "os" + "path/filepath" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" + + "github.com/stacklok/go-microvm/virtiofs" +) + +const overrideStatKey = "user.containers.override_stat" + +func TestPrepareOwnershipStrictPublicAPIReportsEntry(t *testing.T) { + root := t.TempDir() + broken := filepath.Join(root, "broken") + require.NoError(t, os.WriteFile(broken, nil, 0o600)) + require.NoError(t, unix.Lsetxattr(broken, overrideStatKey, []byte("malformed"), 0)) + + err := virtiofs.PrepareOwnership(context.Background(), root, ".", 65532, 65532) + require.Error(t, err) + assert.ErrorContains(t, err, "strict ownership preparation") + assert.ErrorContains(t, err, "broken") + assert.ErrorContains(t, err, "malformed") +} + +func TestSnapshotPreparedBeforeHostReadOnlySeal(t *testing.T) { + root := t.TempDir() + snapshot := filepath.Join(root, "snapshot") + require.NoError(t, os.WriteFile(snapshot, []byte("data"), 0o600)) + + require.NoError(t, virtiofs.PrepareOwnership(context.Background(), root, "snapshot", 65532, 65532)) + require.NoError(t, os.Chmod(snapshot, 0o400)) // Explicit caller-owned sealing step. + // Matching metadata requires no rewrite, so preparation still succeeds after sealing. + require.NoError(t, virtiofs.PrepareOwnership(context.Background(), root, "snapshot", 65532, 65532)) + + info, err := os.Stat(snapshot) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o400), info.Mode().Perm()) + assert.Equal(t, "65532:65532:0100600", readPublicOverride(t, snapshot)) +} + +func TestNewWorktreePreparedBeforeGuestRegistration(t *testing.T) { + root := t.TempDir() + worktree := filepath.Join(root, "worktrees", "job-42") + require.NoError(t, os.MkdirAll(worktree, 0o700)) + file := filepath.Join(worktree, "checkout") + require.NoError(t, os.WriteFile(file, nil, 0o600)) + + registerGuestWorktree := func(path string) { + assert.Equal(t, "65532:65532:0100600", readPublicOverride(t, filepath.Join(path, "checkout"))) + } + require.NoError(t, virtiofs.PrepareOwnership(context.Background(), root, "worktrees/job-42", 65532, 65532)) + registerGuestWorktree(worktree) +} + +func TestPostMergeReplacementPreparedUnderCallerSynchronization(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "result") + require.NoError(t, os.WriteFile(target, []byte("old"), 0o600)) + require.NoError(t, virtiofs.PrepareOwnership(context.Background(), root, ".", 65532, 65532)) + + var guestAccess sync.Mutex + guestAccess.Lock() + staged := filepath.Join(root, "merged") + require.NoError(t, os.WriteFile(staged, []byte("new"), 0o640)) + require.NoError(t, os.Rename(staged, target)) + require.NoError(t, virtiofs.PrepareOwnership(context.Background(), root, "result", 65532, 65532)) + guestAccess.Unlock() + + guestAccess.Lock() + defer guestAccess.Unlock() + assert.Equal(t, "65532:65532:0100640", readPublicOverride(t, target)) +} + +func readPublicOverride(t *testing.T, path string) string { + t.Helper() + buf := make([]byte, 256) + n, err := unix.Lgetxattr(path, overrideStatKey, buf) + require.NoError(t, err) + return string(buf[:n]) +} diff --git a/virtiofs/prepare_unix.go b/virtiofs/prepare_unix.go index 6d2d589..70f99b7 100644 --- a/virtiofs/prepare_unix.go +++ b/virtiofs/prepare_unix.go @@ -7,245 +7,38 @@ package virtiofs import ( "context" - "errors" "fmt" - "os" - "path/filepath" - "strconv" - "strings" - "golang.org/x/sys/unix" + "github.com/stacklok/go-microvm/internal/xattr" ) -const overrideKey = "user.containers.override_stat" - // PrepareOwnership sets libkrun's override_stat ownership metadata on an // authorized host entry or tree. relativePath must be "." for the whole root, // or a non-empty relative path naming one entry and, if it is a directory, its // subtree. // -// The caller needs the OS permission required to read existing xattrs and to -// write new or changed metadata. A matching existing xattr is read but not -// rewritten. In particular, an unannotated host file with mode 0400 commonly -// rejects a new xattr from an unprivileged owner with EACCES; this function -// returns that error without changing host mode or ownership. It does not -// provide a way to choose an initial guest mode: a new xattr derives it from -// the host inode, while an existing xattr preserves its guest mode bits. +// The operation is strict: the first inaccessible entry, malformed xattr, +// unsupported file type, or xattr failure is returned. The caller needs host +// permission to read existing xattrs and write new or changed metadata; an +// unannotated 0400 file commonly rejects a write by an unprivileged owner. A +// matching xattr is not rewritten. A new xattr derives its guest mode from the +// host inode; an existing xattr preserves its guest permission, set-ID, and +// sticky bits. // // The final component of root and every component of relativePath are opened -// without following symlinks. Symlinks below the selected target are skipped. -// Only directories and regular files are supported; encountering another file -// type fails the operation. The real host uid, gid, and mode are not changed. -// Existing mode permission, set-ID, and sticky bits are preserved in the -// override, while the requested uid and gid are reported to the guest. -// -// The caller authorizes the directory reached when root is opened and must -// trust and protect root's parent path while that acquisition occurs. All -// subsequent traversal is descriptor-relative, so mutable intermediate -// symlinks cannot escape that directory. Hard links inside root authorize the -// linked inode, even when that inode also has names outside root. +// without following symlinks. Symlinks below the target are skipped. Traversal +// after root acquisition is descriptor-relative. The caller must trust and +// protect root's parent during acquisition; hard links in root authorize their +// inode even when it also has names outside root. // -// Preparation is non-transactional. An error may leave earlier entries -// prepared. Callers must synchronize concurrent rename, creation, replacement, -// and guest chmod operations. The function provides neither cache invalidation -// nor atomic visibility to a running guest. +// Host ownership and mode are never changed. Preparation is non-transactional: +// an error may leave earlier entries prepared. Callers must synchronize rename, +// creation, replacement, and guest chmod operations. This function provides no +// cache invalidation or atomic visibility to a running guest. func PrepareOwnership(ctx context.Context, root, relativePath string, uid, gid uint32) error { - if root == "" { - return errors.New("virtiofs ownership root must not be empty") - } - parts, err := validateTarget(relativePath) - if err != nil { - return err - } - if err := ctx.Err(); err != nil { - return fmt.Errorf("prepare ownership %q: %w", relativePath, err) - } - - fd, err := acquireTarget(root, relativePath, parts) + _, err := xattr.PrepareOwnership(ctx, root, relativePath, uid, gid, true) if err != nil { - return err - } - return prepareTree(ctx, fd, relativePath, uid, gid) -} - -// acquireTarget pins the selected inode. Callers own the returned descriptor. -func acquireTarget(root, relativePath string, parts []string) (int, error) { - // Cleaning removes a trailing slash or /. that would otherwise make - // O_NOFOLLOW ineffective for a symlink in the final root component. - root = filepath.Clean(root) - fd, err := unix.Open(root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) - if err != nil { - return -1, fmt.Errorf("open authorized root %q without following symlinks: %w", root, err) - } - - if len(parts) == 0 { - return fd, nil - } - - current := fd - for i, part := range parts { - flags := unix.O_RDONLY | unix.O_NOFOLLOW | unix.O_CLOEXEC | unix.O_NONBLOCK - if i < len(parts)-1 { - flags |= unix.O_DIRECTORY - } - next, openErr := unix.Openat(current, part, flags, 0) - if closeErr := unix.Close(current); closeErr != nil && openErr == nil { - if next >= 0 { - _ = unix.Close(next) - } - return -1, fmt.Errorf("close target parent %q: %w", filepath.Join(parts[:i]...), closeErr) - } - if openErr != nil { - return -1, fmt.Errorf("open target %q without following symlinks: %w", relativePath, openErr) - } - current = next - } - return current, nil -} - -func validateTarget(relativePath string) ([]string, error) { - if relativePath == "" { - return nil, errors.New("virtiofs ownership target must not be empty") - } - if filepath.IsAbs(relativePath) { - return nil, fmt.Errorf("virtiofs ownership target %q must be relative", relativePath) - } - if relativePath == "." { - return nil, nil - } - parts := strings.Split(relativePath, string(filepath.Separator)) - for _, part := range parts { - if part == "" || part == "." || part == ".." { - return nil, fmt.Errorf("virtiofs ownership target %q contains an invalid path component", relativePath) - } - } - return parts, nil -} - -func prepareTree(ctx context.Context, fd int, displayPath string, uid, gid uint32) error { - return prepareTreeWith(ctx, fd, displayPath, uid, gid, unix.Openat, func(file *os.File) ([]os.DirEntry, error) { - return file.ReadDir(-1) - }) -} - -func prepareTreeWith( - ctx context.Context, - fd int, - displayPath string, - uid, gid uint32, - openat func(int, string, int, uint32) (int, error), - readDir func(*os.File) ([]os.DirEntry, error), -) (retErr error) { - file := os.NewFile(uintptr(fd), displayPath) - if file == nil { - _ = unix.Close(fd) - return fmt.Errorf("open target %q: invalid file descriptor", displayPath) - } - defer func() { - if err := file.Close(); err != nil && retErr == nil { - retErr = fmt.Errorf("close %q: %w", displayPath, err) - } - }() - - if err := ctx.Err(); err != nil { - return fmt.Errorf("prepare ownership %q: %w", displayPath, err) - } - - var stat unix.Stat_t - if err := unix.Fstat(fd, &stat); err != nil { - return fmt.Errorf("stat %q: %w", displayPath, err) - } - typeBits := uint32(stat.Mode) & unix.S_IFMT - if typeBits != unix.S_IFDIR && typeBits != unix.S_IFREG { - return fmt.Errorf("prepare ownership %q: unsupported file type (mode %#o)", displayPath, stat.Mode) - } - if err := prepareEntry(fd, displayPath, uid, gid, uint32(stat.Mode)); err != nil { - return err - } - if typeBits != unix.S_IFDIR { - return nil - } - - entries, err := readDir(file) - if err != nil { - return fmt.Errorf("read directory %q: %w", displayPath, err) - } - for _, entry := range entries { - if err := ctx.Err(); err != nil { - return fmt.Errorf("prepare ownership %q: %w", filepath.Join(displayPath, entry.Name()), err) - } - childPath := filepath.Join(displayPath, entry.Name()) - child, err := openat(fd, entry.Name(), unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_CLOEXEC|unix.O_NONBLOCK, 0) - if err != nil { - if errors.Is(err, unix.ELOOP) { - continue - } - return fmt.Errorf("open descendant %q without following symlinks: %w", childPath, err) - } - if err := prepareTreeWith(ctx, child, childPath, uid, gid, openat, readDir); err != nil { - return err - } - } - return nil -} - -func prepareEntry(fd int, path string, uid, gid, hostMode uint32) error { - return prepareEntryWith(fd, path, uid, gid, hostMode, unix.Fgetxattr, unix.Fsetxattr) -} - -func prepareEntryWith( - fd int, - path string, - uid, gid, hostMode uint32, - getxattr func(int, string, []byte) (int, error), - setxattr func(int, string, []byte, int) error, -) error { - mode := hostMode - buf := make([]byte, 256) - n, err := getxattr(fd, overrideKey, buf) - if err == nil { - currentUID, currentGID, currentMode, parseErr := parseOverride(string(buf[:n])) - if parseErr != nil { - return fmt.Errorf("read existing override_stat on %q: %w", path, parseErr) - } - // Guest chmod changes only override_stat. Keep those permission, set-ID, - // and sticky bits, but always take the type from the current inode. - mode = hostMode&unix.S_IFMT | currentMode&0o7777 - if currentUID == uid && currentGID == gid && currentMode == mode { - return nil - } - } else if !isNoAttribute(err) { - return fmt.Errorf("read override_stat on %q: %w", path, err) - } - - desired := fmt.Sprintf("%d:%d:0%o", uid, gid, mode) - if err := setxattr(fd, overrideKey, []byte(desired), 0); err != nil { - return fmt.Errorf("write override_stat on %q: %w", path, err) + return fmt.Errorf("strict ownership preparation: %w", err) } return nil } - -func parseOverride(value string) (uint32, uint32, uint32, error) { - parts := strings.Split(value, ":") - if len(parts) != 3 { - return 0, 0, 0, fmt.Errorf("malformed value %q", value) - } - uid, err := strconv.ParseUint(parts[0], 10, 32) - if err != nil { - return 0, 0, 0, fmt.Errorf("malformed uid in value %q: %w", value, err) - } - gid, err := strconv.ParseUint(parts[1], 10, 32) - if err != nil { - return 0, 0, 0, fmt.Errorf("malformed gid in value %q: %w", value, err) - } - // libkrun's override_stat mode field is octal, including when it has no - // leading zero. POSIX st_mode has no representable bits above 0177777. - mode, err := strconv.ParseUint(parts[2], 8, 32) - if err != nil || mode > 0o177777 { - if err == nil { - err = errors.New("mode exceeds POSIX st_mode bits") - } - return 0, 0, 0, fmt.Errorf("malformed mode in value %q: %w", value, err) - } - return uint32(uid), uint32(gid), uint32(mode), nil -} diff --git a/virtiofs/prepare_unix_test.go b/virtiofs/prepare_unix_test.go index 1fad570..7f0e287 100644 --- a/virtiofs/prepare_unix_test.go +++ b/virtiofs/prepare_unix_test.go @@ -19,6 +19,8 @@ import ( "golang.org/x/sys/unix" ) +const overrideKey = "user.containers.override_stat" + func TestPrepareOwnershipTargeted(t *testing.T) { root := t.TempDir() target := filepath.Join(root, "target") @@ -80,35 +82,6 @@ func TestPrepareOwnershipSkipsDescendantSymlink(t *testing.T) { assert.Empty(t, getOverride(t, externalFile)) } -func TestPrepareOwnershipSkipsDescendantReplacedWithSymlink(t *testing.T) { - root := t.TempDir() - victim := filepath.Join(root, "victim") - replaced := filepath.Join(root, "replaced-victim") - external := t.TempDir() - externalFile := filepath.Join(external, "secret") - require.NoError(t, os.Mkdir(victim, 0o700)) - require.NoError(t, os.WriteFile(filepath.Join(victim, "inside"), nil, 0o600)) - require.NoError(t, os.WriteFile(externalFile, nil, 0o600)) - - fd, err := unix.Open(root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) - require.NoError(t, err) - replacedOnce := false - err = prepareTreeWith(context.Background(), fd, ".", 42, 43, - func(parent int, name string, flags int, mode uint32) (int, error) { - if !replacedOnce && name == "victim" { - replacedOnce = true - require.NoError(t, os.Rename(victim, replaced)) - require.NoError(t, os.Symlink(external, victim)) - } - return unix.Openat(parent, name, flags, mode) - }, - func(file *os.File) ([]os.DirEntry, error) { return file.ReadDir(-1) }, - ) - require.NoError(t, err) - assert.Empty(t, getOverride(t, external)) - assert.Empty(t, getOverride(t, externalFile)) -} - func TestPrepareOwnershipUnannotatedReadOnlyFileFailsWithoutXattrWritePermission(t *testing.T) { if os.Geteuid() == 0 { t.Skip("requires a non-root user to verify normal xattr permission enforcement") @@ -207,70 +180,6 @@ func TestPrepareOwnershipMalformedMetadataIsNotClobbered(t *testing.T) { } } -func TestPrepareOwnershipPinnedTargetCannotEscapeAfterReplacement(t *testing.T) { - root := t.TempDir() - target := filepath.Join(root, "target") - require.NoError(t, os.Mkdir(target, 0o700)) - require.NoError(t, os.WriteFile(filepath.Join(target, "inside"), nil, 0o600)) - external := t.TempDir() - externalFile := filepath.Join(external, "outside") - require.NoError(t, os.WriteFile(externalFile, nil, 0o600)) - - parts, err := validateTarget("target") - require.NoError(t, err) - fd, err := acquireTarget(root, "target", parts) - require.NoError(t, err) - oldTarget := filepath.Join(root, "old-target") - require.NoError(t, os.Rename(target, oldTarget)) - require.NoError(t, os.Symlink(external, target)) - - require.NoError(t, prepareTree(context.Background(), fd, "target", 42, 43)) - assert.Contains(t, getOverride(t, filepath.Join(oldTarget, "inside")), "42:43:") - assert.Empty(t, getOverride(t, externalFile)) -} - -func TestPrepareOwnershipStrictInjectedErrors(t *testing.T) { - t.Run("xattr read oversized", func(t *testing.T) { - path := filepath.Join(t.TempDir(), "file") - require.NoError(t, os.WriteFile(path, nil, 0o600)) - require.NoError(t, unix.Lsetxattr(path, overrideKey, make([]byte, 300), 0)) - file, err := os.Open(path) - require.NoError(t, err) - t.Cleanup(func() { require.NoError(t, file.Close()) }) - assert.ErrorContains(t, prepareEntry(int(file.Fd()), path, 1, 1, unix.S_IFREG|0o600), "read override_stat") - }) - - t.Run("xattr write", func(t *testing.T) { - path := filepath.Join(t.TempDir(), "file") - require.NoError(t, os.WriteFile(path, nil, 0o600)) - file, err := os.Open(path) - require.NoError(t, err) - t.Cleanup(func() { require.NoError(t, file.Close()) }) - err = prepareEntryWith(int(file.Fd()), path, 1, 1, unix.S_IFREG|0o600, unix.Fgetxattr, - func(int, string, []byte, int) error { return unix.EROFS }) - assert.ErrorContains(t, err, "write override_stat") - }) - - t.Run("directory enumeration", func(t *testing.T) { - fd, err := unix.Open(t.TempDir(), unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) - require.NoError(t, err) - err = prepareTreeWith(context.Background(), fd, ".", 1, 1, unix.Openat, - func(*os.File) ([]os.DirEntry, error) { return nil, unix.EACCES }) - assert.ErrorContains(t, err, "read directory") - }) - - t.Run("descendant open", func(t *testing.T) { - root := t.TempDir() - require.NoError(t, os.WriteFile(filepath.Join(root, "child"), nil, 0o600)) - fd, err := unix.Open(root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) - require.NoError(t, err) - err = prepareTreeWith(context.Background(), fd, ".", 1, 1, - func(int, string, int, uint32) (int, error) { return -1, unix.EACCES }, - func(file *os.File) ([]os.DirEntry, error) { return file.ReadDir(-1) }) - assert.ErrorContains(t, err, "open descendant") - }) -} - func TestPrepareOwnershipMissingPaths(t *testing.T) { root := t.TempDir() assert.ErrorContains(t, PrepareOwnership(context.Background(), "", ".", 1, 1), "root must not be empty") From 63f847c6c803d5dd4c01085dfb754f313c14aa07 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 22 Sep 2026 16:16:24 +0300 Subject: [PATCH 3/3] Prepare v0.0.41 release notes --- docs/RELEASE_NOTES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/RELEASE_NOTES.md b/docs/RELEASE_NOTES.md index b62503e..18798be 100644 --- a/docs/RELEASE_NOTES.md +++ b/docs/RELEASE_NOTES.md @@ -1,6 +1,6 @@ # Release notes -## Unreleased +## v0.0.41 ### Virtio-fs ownership preparation