amadan

carlos / platform Public

instancefs: symlink-refusing opens and atomic writes for instance directories (plan 1, task 2)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YZ1ZvWj7naf73GjTX8MBv5
Paul Campbell pushed by paul@keymail.dev 380acab5e695753736436b3455f4d9794260fa52 parent e41f808
4 files changed, +393 −0
  • internal/instancefs/instancefs.go +56 −0
  • internal/instancefs/instancefs_linux.go +174 −0
  • internal/instancefs/instancefs_linux_test.go +137 −0
  • internal/instancefs/instancefs_other.go +26 −0
diff --git a/internal/instancefs/instancefs.go b/internal/instancefs/instancefs.go
new file mode 100644
index 000000000..de8ec6824
--- /dev/null
+++ b/internal/instancefs/instancefs.go
@@ -0,0 +1,56 @@
+// Package instancefs is how the agent opens anything inside an instance's
+// own directory. Every exec-backed instance runs as the agent's uid, and
+// its directory is writable by it, so any path the agent opens there is a
+// request the instance makes on the agent's behalf: a symlink at
+// <host>.env pointing at /data/litestream.env would hand the instance the
+// bucket credentials as its own environment (isolated-exec design §1.3).
+//
+// The kernel closes that for every reader at once with the nosymfollow
+// mount option on the instance trees (design §3.2). This package is the
+// belt to that brace: it opens with openat2(RESOLVE_BENEATH |
+// RESOLVE_NO_SYMLINKS | RESOLVE_NO_MAGICLINKS), so a symlink is refused
+// with a named error even on a box whose mount is missing the option, and
+// it writes through O_EXCL temp files and rename, so a planted symlink at
+// a destination is replaced as a directory entry rather than written
+// through.
+//
+// Nothing here is Linux-specific in its contract; the implementations
+// are, and the !linux build returns errors.ErrUnsupported so the CLI still
+// builds on a Mac (AGENTS.md's cross-compile gate).
+package instancefs
+
+import (
+ "errors"
+ "io"
+ "os"
+ "path/filepath"
+ "strings"
+)
+
+// ErrSymlink is returned when any component of the path is a symlink.
+var ErrSymlink = errors.New("instancefs: path contains a symlink")
+
+// ErrEscapes is returned for a name that is absolute or reaches above dir.
+var ErrEscapes = errors.New("instancefs: path escapes its directory")
+
+// cleanRel validates name as a relative path that stays inside dir.
+func cleanRel(name string) (string, error) {
+ if name == "" || filepath.IsAbs(name) {
+ return "", ErrEscapes
+ }
+ c := filepath.Clean(name)
+ if c == ".." || strings.HasPrefix(c, "../") {
+ return "", ErrEscapes
+ }
+ return c, nil
+}
+
+// ReadBeneath is OpenBeneath followed by reading the whole file.
+func ReadBeneath(dir, name string) ([]byte, error) {
+ f, err := OpenBeneath(dir, name, os.O_RDONLY, 0)
+ if err != nil {
+ return nil, err
+ }
+ defer f.Close()
+ return io.ReadAll(f)
+}
diff --git a/internal/instancefs/instancefs_linux.go b/internal/instancefs/instancefs_linux.go
new file mode 100644
index 000000000..4c6ca657a
--- /dev/null
+++ b/internal/instancefs/instancefs_linux.go
@@ -0,0 +1,174 @@
+//go:build linux
+
+package instancefs
+
+import (
+ "bufio"
+ "crypto/rand"
+ "encoding/hex"
+ "errors"
+ "fmt"
+ "os"
+ "path/filepath"
+ "strings"
+
+ "golang.org/x/sys/unix"
+)
+
+const resolveFlags = unix.RESOLVE_BENEATH | unix.RESOLVE_NO_SYMLINKS | unix.RESOLVE_NO_MAGICLINKS
+
+// openDir opens dir itself as a directory fd, also refusing symlinks in
+// dir's own components: the instance directory's parent is the agent's,
+// but the instance directory itself could have been swapped for a
+// symlink by a same-uid process before the mount option landed.
+func openDir(dir string) (int, error) {
+ fd, err := unix.Open(dir, unix.O_PATH|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
+ if err != nil {
+ if errors.Is(err, unix.ELOOP) {
+ return -1, fmt.Errorf("%w: %s", ErrSymlink, dir)
+ }
+ return -1, &os.PathError{Op: "open", Path: dir, Err: err}
+ }
+ return fd, nil
+}
+
+// OpenBeneath opens name under dir with openat2, following no symlink at
+// any component and never leaving dir.
+func OpenBeneath(dir, name string, flag int, perm os.FileMode) (*os.File, error) {
+ rel, err := cleanRel(name)
+ if err != nil {
+ return nil, err
+ }
+ dfd, err := openDir(dir)
+ if err != nil {
+ return nil, err
+ }
+ defer unix.Close(dfd)
+ how := unix.OpenHow{
+ Flags: uint64(flag) | unix.O_CLOEXEC,
+ Mode: uint64(perm.Perm()),
+ Resolve: resolveFlags,
+ }
+ fd, err := unix.Openat2(dfd, rel, &how)
+ if err != nil {
+ switch {
+ case errors.Is(err, unix.ELOOP):
+ return nil, fmt.Errorf("%w: %s", ErrSymlink, filepath.Join(dir, rel))
+ case errors.Is(err, unix.EXDEV):
+ return nil, fmt.Errorf("%w: %s", ErrEscapes, filepath.Join(dir, rel))
+ }
+ return nil, &os.PathError{Op: "openat2", Path: filepath.Join(dir, rel), Err: err}
+ }
+ return os.NewFile(uintptr(fd), filepath.Join(dir, rel)), nil
+}
+
+// WriteAtomic writes data to a fresh O_EXCL temp file beside name and
+// renames it over name. rename(2) replaces the directory entry and never
+// follows a symlink at the destination, so a planted link is displaced,
+// not written through. The temp name is random, so no same-uid process
+// can pre-plant it (the O_EXCL refuses it if one does).
+func WriteAtomic(dir, name string, data []byte, perm os.FileMode) error {
+ rel, err := cleanRel(name)
+ if err != nil {
+ return err
+ }
+ if strings.Contains(rel, "/") {
+ return fmt.Errorf("%w: WriteAtomic takes a bare file name, got %q", ErrEscapes, name)
+ }
+ dfd, err := openDir(dir)
+ if err != nil {
+ return err
+ }
+ defer unix.Close(dfd)
+ var rnd [8]byte
+ if _, err := rand.Read(rnd[:]); err != nil {
+ return err
+ }
+ tmp := "." + rel + ".tmp-" + hex.EncodeToString(rnd[:])
+ how := unix.OpenHow{
+ Flags: unix.O_WRONLY | unix.O_CREAT | unix.O_EXCL | unix.O_CLOEXEC,
+ Mode: uint64(perm.Perm()),
+ Resolve: resolveFlags,
+ }
+ fd, err := unix.Openat2(dfd, tmp, &how)
+ if err != nil {
+ return &os.PathError{Op: "openat2", Path: filepath.Join(dir, tmp), Err: err}
+ }
+ f := os.NewFile(uintptr(fd), filepath.Join(dir, tmp))
+ cleanup := func() { f.Close(); _ = unix.Unlinkat(dfd, tmp, 0) }
+ if _, err := f.Write(data); err != nil {
+ cleanup()
+ return err
+ }
+ if err := f.Sync(); err != nil {
+ cleanup()
+ return err
+ }
+ if err := f.Close(); err != nil {
+ _ = unix.Unlinkat(dfd, tmp, 0)
+ return err
+ }
+ if err := unix.Renameat(dfd, tmp, dfd, rel); err != nil {
+ _ = unix.Unlinkat(dfd, tmp, 0)
+ return &os.PathError{Op: "renameat", Path: filepath.Join(dir, rel), Err: err}
+ }
+ return nil
+}
+
+// MountNoSymfollow reports whether mountpoint is a mountpoint whose
+// options include nosymfollow, by reading /proc/self/mountinfo. A path
+// that is not a mountpoint is (false, nil).
+func MountNoSymfollow(mountpoint string) (bool, error) {
+ f, err := os.Open("/proc/self/mountinfo")
+ if err != nil {
+ return false, err
+ }
+ defer f.Close()
+ want := filepath.Clean(mountpoint)
+ sc := bufio.NewScanner(f)
+ for sc.Scan() {
+ if mountinfoLineHas(sc.Text(), want, "nosymfollow") {
+ return true, nil
+ }
+ }
+ return false, sc.Err()
+}
+
+// mountinfoLineHas parses one mountinfo line: field 5 is the mountpoint
+// (octal-escaped), field 6 the per-mount options.
+func mountinfoLineHas(line, mountpoint, option string) bool {
+ fields := strings.Fields(line)
+ if len(fields) < 6 {
+ return false
+ }
+ if unescapeMountinfo(fields[4]) != mountpoint {
+ return false
+ }
+ for _, o := range strings.Split(fields[5], ",") {
+ if o == option {
+ return true
+ }
+ }
+ return false
+}
+
+// unescapeMountinfo undoes the \040-style escapes mountinfo applies to
+// spaces, tabs, newlines and backslashes in paths.
+func unescapeMountinfo(s string) string {
+ if !strings.Contains(s, `\`) {
+ return s
+ }
+ var b strings.Builder
+ for i := 0; i < len(s); i++ {
+ if s[i] == '\\' && i+3 < len(s) {
+ var v int
+ if _, err := fmt.Sscanf(s[i+1:i+4], "%o", &v); err == nil {
+ b.WriteByte(byte(v))
+ i += 3
+ continue
+ }
+ }
+ b.WriteByte(s[i])
+ }
+ return b.String()
+}
diff --git a/internal/instancefs/instancefs_linux_test.go b/internal/instancefs/instancefs_linux_test.go
new file mode 100644
index 000000000..aad349adf
--- /dev/null
+++ b/internal/instancefs/instancefs_linux_test.go
@@ -0,0 +1,137 @@
+//go:build linux
+
+package instancefs
+
+import (
+ "errors"
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+)
+
+func TestOpenBeneathReadsAPlainFile(t *testing.T) {
+ dir := t.TempDir()
+ if err := os.WriteFile(filepath.Join(dir, "h.env"), []byte("A=1\n"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ got, err := ReadBeneath(dir, "h.env")
+ if err != nil {
+ t.Fatalf("ReadBeneath: %v", err)
+ }
+ if string(got) != "A=1\n" {
+ t.Errorf("got %q", got)
+ }
+}
+
+func TestOpenBeneathRefusesASymlinkAtTheLeaf(t *testing.T) {
+ dir := t.TempDir()
+ secret := filepath.Join(t.TempDir(), "secret")
+ if err := os.WriteFile(secret, []byte("token"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Symlink(secret, filepath.Join(dir, "h.env")); err != nil {
+ t.Fatal(err)
+ }
+ _, err := ReadBeneath(dir, "h.env")
+ if !errors.Is(err, ErrSymlink) {
+ t.Fatalf("ReadBeneath through a symlink: err = %v, want ErrSymlink", err)
+ }
+}
+
+func TestOpenBeneathRefusesASymlinkedDirectory(t *testing.T) {
+ dir := t.TempDir()
+ other := t.TempDir()
+ if err := os.WriteFile(filepath.Join(other, "x"), []byte("x"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Symlink(other, filepath.Join(dir, "sub")); err != nil {
+ t.Fatal(err)
+ }
+ _, err := ReadBeneath(dir, "sub/x")
+ if !errors.Is(err, ErrSymlink) {
+ t.Fatalf("err = %v, want ErrSymlink", err)
+ }
+}
+
+func TestOpenBeneathRefusesDotDotAndAbsolute(t *testing.T) {
+ dir := t.TempDir()
+ for _, name := range []string{"../x", "a/../../x", "/etc/passwd"} {
+ _, err := ReadBeneath(dir, name)
+ if !errors.Is(err, ErrEscapes) {
+ t.Errorf("ReadBeneath(%q): err = %v, want ErrEscapes", name, err)
+ }
+ }
+}
+
+func TestWriteAtomicReplacesAndNeverFollows(t *testing.T) {
+ dir := t.TempDir()
+ if err := WriteAtomic(dir, "h.config.env", []byte("one\n"), 0o600); err != nil {
+ t.Fatalf("WriteAtomic: %v", err)
+ }
+ if err := WriteAtomic(dir, "h.config.env", []byte("two\n"), 0o600); err != nil {
+ t.Fatalf("WriteAtomic (replace): %v", err)
+ }
+ got, err := os.ReadFile(filepath.Join(dir, "h.config.env"))
+ if err != nil || string(got) != "two\n" {
+ t.Fatalf("got %q, %v", got, err)
+ }
+ fi, err := os.Stat(filepath.Join(dir, "h.config.env"))
+ if err != nil || fi.Mode().Perm() != 0o600 {
+ t.Errorf("mode = %v, %v", fi.Mode(), err)
+ }
+ // A symlink planted at the destination is REPLACED as a directory
+ // entry; its target is untouched.
+ victim := filepath.Join(t.TempDir(), "victim")
+ if err := os.WriteFile(victim, []byte("keep"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Remove(filepath.Join(dir, "h.config.env")); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Symlink(victim, filepath.Join(dir, "h.config.env")); err != nil {
+ t.Fatal(err)
+ }
+ if err := WriteAtomic(dir, "h.config.env", []byte("three\n"), 0o600); err != nil {
+ t.Fatalf("WriteAtomic over a symlink: %v", err)
+ }
+ if b, _ := os.ReadFile(victim); string(b) != "keep" {
+ t.Errorf("symlink target was written through: %q", b)
+ }
+ if fi, err := os.Lstat(filepath.Join(dir, "h.config.env")); err != nil || fi.Mode()&os.ModeSymlink != 0 {
+ t.Errorf("destination is still a symlink: %v %v", fi, err)
+ }
+ // No temp file is left behind.
+ entries, _ := os.ReadDir(dir)
+ for _, e := range entries {
+ if strings.Contains(e.Name(), ".tmp-") {
+ t.Errorf("temp file left behind: %s", e.Name())
+ }
+ }
+}
+
+func TestMountNoSymfollowReadsMountinfo(t *testing.T) {
+ // / is a mount and does not carry nosymfollow on any ordinary box; a
+ // path that is not a mountpoint is reported as not a nosymfollow mount
+ // with a nil error (the caller's message names the path).
+ got, err := MountNoSymfollow("/")
+ if err != nil {
+ t.Fatalf("MountNoSymfollow(/): %v", err)
+ }
+ if got {
+ t.Error("/ reported as nosymfollow")
+ }
+ got, err = MountNoSymfollow(t.TempDir())
+ if err != nil || got {
+ t.Errorf("temp dir: %v %v", got, err)
+ }
+ // The parser, against a fixture line, since a real nosymfollow mount
+ // needs root to create.
+ line := "36 35 98:0 /.instances /data/instances rw,nosuid,nodev,noexec,nosymfollow,relatime - ext4 /dev/root rw"
+ if !mountinfoLineHas(line, "/data/instances", "nosymfollow") {
+ t.Error("parser missed nosymfollow on a fixture line")
+ }
+ if mountinfoLineHas(line, "/data", "nosymfollow") {
+ t.Error("parser matched the wrong mountpoint")
+ }
+}
diff --git a/internal/instancefs/instancefs_other.go b/internal/instancefs/instancefs_other.go
new file mode 100644
index 000000000..8c8e31d1b
--- /dev/null
+++ b/internal/instancefs/instancefs_other.go
@@ -0,0 +1,26 @@
+//go:build !linux
+
+package instancefs
+
+import (
+ "errors"
+ "os"
+)
+
+func OpenBeneath(dir, name string, flag int, perm os.FileMode) (*os.File, error) {
+ if _, err := cleanRel(name); err != nil {
+ return nil, err
+ }
+ return nil, errors.ErrUnsupported
+}
+
+func WriteAtomic(dir, name string, data []byte, perm os.FileMode) error {
+ if _, err := cleanRel(name); err != nil {
+ return err
+ }
+ return errors.ErrUnsupported
+}
+
+func MountNoSymfollow(mountpoint string) (bool, error) { return false, errors.ErrUnsupported }
+
+func mountinfoLineHas(line, mountpoint, option string) bool { return false }