From b54bd3284abf924c9998ed20d23cf730a8cea742 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 3 Mar 2026 13:15:39 +0200 Subject: [PATCH] Make rootfs hook chown best-effort for macOS support On macOS, non-root users cannot os.Chown to a different UID, causing InjectAuthorizedKeys to fail with EPERM. This makes VM startup impossible on macOS without root. Two-layer fix: - Host side: InjectAuthorizedKeys now uses an injected ChownFunc (defaulting to BestEffortLchown) that swallows EPERM. Uses Lchown instead of Chown to avoid following symlinks in the rootfs. - Guest side: New fixHomeOwnership step in boot.Run recursively chowns /home/sandbox and enforces SSH strict permissions (0700 for .ssh/, 0600 for files). Runs as PID 1 before cap drop, so chown always succeeds. Symlinks are skipped for safety. Co-Authored-By: Claude Opus 4.6 --- guest/boot/boot.go | 25 +++++--- guest/boot/fixhome.go | 69 ++++++++++++++++++++++ guest/boot/fixhome_test.go | 118 +++++++++++++++++++++++++++++++++++++ hooks/hooks.go | 41 ++++++++++--- hooks/hooks_test.go | 79 +++++++++++++++++-------- 5 files changed, 292 insertions(+), 40 deletions(-) create mode 100644 guest/boot/fixhome.go create mode 100644 guest/boot/fixhome_test.go diff --git a/guest/boot/boot.go b/guest/boot/boot.go index 4e916c3..4a05003 100644 --- a/guest/boot/boot.go +++ b/guest/boot/boot.go @@ -31,10 +31,11 @@ import ( // 3. Workspace mount (non-fatal if fails) // 4. Kernel sysctl hardening // 5. Lock down /root (if enabled) -// 6. Load environment file -// 7. Parse SSH authorized keys -// 8. Drop bounding capabilities + set no_new_privs -// 9. Start SSH server +// 6. Fix home directory ownership (rootfs hooks may not chown on macOS) +// 7. Load environment file +// 8. Parse SSH authorized keys +// 9. Drop bounding capabilities + set no_new_privs +// 10. Start SSH server func Run(logger *slog.Logger, opts ...Option) (shutdown func(), err error) { cfg := defaultConfig() for _, o := range opts { @@ -74,19 +75,25 @@ func Run(logger *slog.Logger, opts ...Option) (shutdown func(), err error) { lockdownRoot(logger) } - // 6. Load environment file. + // 6. Fix home directory ownership. Rootfs hooks run on the host + // before boot and may not be able to chown to the sandbox UID (e.g. + // macOS non-root users). Fix ownership now that we're running as + // root inside the guest. + fixHomeOwnership(logger, cfg.userHome, int(cfg.userUID), int(cfg.userGID)) + + // 7. Load environment file. envVars, err := env.Load(cfg.envFilePath) if err != nil { return nil, fmt.Errorf("loading environment: %w", err) } - // 7. Parse authorized keys. + // 8. Parse authorized keys. authorizedKeys, err := ParseAuthorizedKeys(cfg.sshKeysPath) if err != nil { return nil, fmt.Errorf("parsing authorized keys: %w", err) } - // 7b. Load injected host key (if present). The key is deleted from + // 8b. Load injected host key (if present). The key is deleted from // disk after loading into memory so it cannot be read by the sandbox // user. If the file does not exist, hostKeySigner remains nil and the // SSH server will generate an ephemeral key. @@ -103,7 +110,7 @@ func Run(logger *slog.Logger, opts ...Option) (shutdown func(), err error) { _ = os.Remove(cfg.sshHostKeyPath) } - // 8. Drop unneeded capabilities from the bounding set. + // 9. Drop unneeded capabilities from the bounding set. logger.Info("dropping unnecessary capabilities") if err := harden.DropBoundingCaps( harden.CapSetUID, @@ -118,7 +125,7 @@ func Run(logger *slog.Logger, opts ...Option) (shutdown func(), err error) { return nil, fmt.Errorf("setting no_new_privs: %w", err) } - // 9. Start SSH server. + // 10. Start SSH server. sshdCfg := sshd.Config{ Port: cfg.sshPort, AuthorizedKeys: authorizedKeys, diff --git a/guest/boot/fixhome.go b/guest/boot/fixhome.go new file mode 100644 index 0000000..cadd442 --- /dev/null +++ b/guest/boot/fixhome.go @@ -0,0 +1,69 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package boot + +import ( + "io/fs" + "log/slog" + "os" + "path/filepath" + "strings" +) + +// fixHomeOwnership recursively chowns the user's home directory so that +// files injected by rootfs hooks (which may have been written by a non-root +// host user) are owned by the sandbox user. It also enforces strict SSH +// directory permissions (0700 for .ssh/, 0600 for files inside .ssh/). +// +// This runs as PID 1 (root) inside the guest, so chown always succeeds. +func fixHomeOwnership(logger *slog.Logger, home string, uid, gid int) { + logger.Info("fixing home directory ownership", "home", home, "uid", uid, "gid", gid) + + err := filepath.WalkDir(home, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + + // Skip symlinks — Lchown on a symlink itself is harmless but + // Chmod would follow the symlink and modify the target. + if d.Type()&fs.ModeSymlink != 0 { + return nil + } + + if chownErr := os.Lchown(path, uid, gid); chownErr != nil { + logger.Warn("chown failed", "path", path, "error", chownErr) + } + + // Enforce strict SSH permissions. + // Rel cannot fail for paths from WalkDir(home, ...). + rel, _ := filepath.Rel(home, path) + if isSSHPath(rel) { + enforcePerm := sshPermission(d.IsDir()) + if chmodErr := os.Chmod(path, enforcePerm); chmodErr != nil { + logger.Warn("chmod failed", "path", path, "perm", enforcePerm, "error", chmodErr) + } + } + + return nil + }) + if err != nil { + logger.Warn("home ownership fixup incomplete", "home", home, "error", err) + } +} + +// isSSHPath returns true if the relative path is inside the .ssh directory. +func isSSHPath(rel string) bool { + return rel == ".ssh" || strings.HasPrefix(rel, ".ssh"+string(filepath.Separator)) +} + +// sshPermission returns the required permission for SSH paths: +// 0700 for directories, 0600 for files. +func sshPermission(isDir bool) os.FileMode { + if isDir { + return 0o700 + } + return 0o600 +} diff --git a/guest/boot/fixhome_test.go b/guest/boot/fixhome_test.go new file mode 100644 index 0000000..0e917a9 --- /dev/null +++ b/guest/boot/fixhome_test.go @@ -0,0 +1,118 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//go:build linux + +package boot + +import ( + "log/slog" + "os" + "os/user" + "path/filepath" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFixHomeOwnership_FixesPermissions(t *testing.T) { + t.Parallel() + + u, err := user.Current() + require.NoError(t, err) + uid, err := strconv.Atoi(u.Uid) + require.NoError(t, err) + gid, err := strconv.Atoi(u.Gid) + require.NoError(t, err) + + home := t.TempDir() + + // Create .ssh dir with wrong permissions (0755 instead of 0700). + sshDir := filepath.Join(home, ".ssh") + require.NoError(t, os.MkdirAll(sshDir, 0o755)) + + // Create authorized_keys with wrong permissions (0644 instead of 0600). + akPath := filepath.Join(sshDir, "authorized_keys") + require.NoError(t, os.WriteFile(akPath, []byte("ssh-ed25519 AAAA test"), 0o644)) + + // Create a non-SSH file that should not get SSH permission enforcement. + require.NoError(t, os.WriteFile(filepath.Join(home, ".gitconfig"), []byte("[user]"), 0o644)) + + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + + fixHomeOwnership(logger, home, uid, gid) + + // Verify .ssh directory permissions are 0700. + info, err := os.Stat(sshDir) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o700), info.Mode().Perm(), ".ssh dir should be 0700") + + // Verify authorized_keys permissions are 0600. + info, err = os.Stat(akPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(), "authorized_keys should be 0600") + + // Verify non-SSH file permissions are unchanged. + info, err = os.Stat(filepath.Join(home, ".gitconfig")) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o644), info.Mode().Perm(), ".gitconfig should be unchanged") +} + +func TestFixHomeOwnership_HandlesNestedSSHFiles(t *testing.T) { + t.Parallel() + + u, err := user.Current() + require.NoError(t, err) + uid, err := strconv.Atoi(u.Uid) + require.NoError(t, err) + gid, err := strconv.Atoi(u.Gid) + require.NoError(t, err) + + home := t.TempDir() + + // Create .ssh with a known_hosts file. + sshDir := filepath.Join(home, ".ssh") + require.NoError(t, os.MkdirAll(sshDir, 0o755)) + khPath := filepath.Join(sshDir, "known_hosts") + require.NoError(t, os.WriteFile(khPath, []byte("github.com ..."), 0o644)) + + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + + fixHomeOwnership(logger, home, uid, gid) + + info, err := os.Stat(khPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(), "known_hosts should be 0600") +} + +func TestFixHomeOwnership_MissingHome(t *testing.T) { + t.Parallel() + + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + + // Should not panic on missing directory — just logs a warning. + fixHomeOwnership(logger, "/nonexistent/home/dir", 1000, 1000) +} + +func TestIsSSHPath(t *testing.T) { + t.Parallel() + + tests := []struct { + rel string + want bool + }{ + {".ssh", true}, + {".ssh/authorized_keys", true}, + {".ssh/known_hosts", true}, + {".gitconfig", false}, + {".config/opencode", false}, + {"", false}, + {".sshconfig", false}, + } + + for _, tt := range tests { + assert.Equal(t, tt.want, isSSHPath(tt.rel), "isSSHPath(%q)", tt.rel) + } +} diff --git a/hooks/hooks.go b/hooks/hooks.go index 27c9cbd..d5944f2 100644 --- a/hooks/hooks.go +++ b/hooks/hooks.go @@ -5,6 +5,7 @@ package hooks import ( "fmt" + "log/slog" "os" "path/filepath" "regexp" @@ -27,14 +28,19 @@ type keyOptionFunc func(*keyConfig) func (f keyOptionFunc) apply(c *keyConfig) { f(c) } +// ChownFunc abstracts file ownership changes for testability. +// Production code uses BestEffortLchown; tests can pass a recording mock. +type ChownFunc func(path string, uid, gid int) error + type keyConfig struct { - home string - uid int - gid int + home string + uid int + gid int + chown ChownFunc } func defaultKeyConfig() *keyConfig { - return &keyConfig{home: "/home/sandbox", uid: 1000, gid: 1000} + return &keyConfig{home: "/home/sandbox", uid: 1000, gid: 1000, chown: BestEffortLchown} } // WithKeyUser overrides the default user home, UID, and GID for SSH key injection. @@ -42,6 +48,12 @@ func WithKeyUser(home string, uid, gid int) KeyOption { return keyOptionFunc(func(c *keyConfig) { c.home = home; c.uid = uid; c.gid = gid }) } +// WithChown overrides the chown function used by InjectAuthorizedKeys. +// Useful for testing or environments where chown must be handled differently. +func WithChown(fn ChownFunc) KeyOption { + return keyOptionFunc(func(c *keyConfig) { c.chown = fn }) +} + // InjectAuthorizedKeys returns a RootFSHook that writes the given public key // to {home}/.ssh/authorized_keys inside the rootfs. The .ssh directory is // created with 0700 permissions and the authorized_keys file with 0600. @@ -61,7 +73,7 @@ func InjectAuthorizedKeys(pubKey string, opts ...KeyOption) func(string, *image. if err := os.MkdirAll(sshDir, 0o700); err != nil { return fmt.Errorf("create .ssh dir: %w", err) } - if err := os.Chown(sshDir, cfg.uid, cfg.gid); err != nil { + if err := cfg.chown(sshDir, cfg.uid, cfg.gid); err != nil { return fmt.Errorf("chown .ssh dir: %w", err) } @@ -73,7 +85,7 @@ func InjectAuthorizedKeys(pubKey string, opts ...KeyOption) func(string, *image. if err := os.WriteFile(akPath, []byte(pubKey+"\n"), 0o600); err != nil { return fmt.Errorf("write authorized_keys: %w", err) } - if err := os.Chown(akPath, cfg.uid, cfg.gid); err != nil { + if err := cfg.chown(akPath, cfg.uid, cfg.gid); err != nil { return fmt.Errorf("chown authorized_keys: %w", err) } @@ -152,5 +164,20 @@ func InjectEnvFile(guestPath string, envMap map[string]string) func(string, *ima // shellEscape wraps a value in single quotes for safe shell sourcing. // Internal single quotes are escaped with the '\” idiom. func shellEscape(s string) string { - return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'" + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + +// BestEffortLchown attempts os.Lchown and silently ignores permission errors, +// returning nil. On macOS non-root users cannot chown to a different UID; +// the guest init will fix ownership at boot time. Non-permission errors are +// logged at warn level and also swallowed. Callers that need strict chown +// should call os.Lchown directly instead. +// Lchown is used instead of Chown to avoid following symlinks in the rootfs. +func BestEffortLchown(path string, uid, gid int) error { + if err := os.Lchown(path, uid, gid); err != nil { + if !os.IsPermission(err) { + slog.Warn("lchown failed", "path", path, "uid", uid, "gid", gid, "err", err) + } + } + return nil } diff --git a/hooks/hooks_test.go b/hooks/hooks_test.go index 20dfb78..baae58a 100644 --- a/hooks/hooks_test.go +++ b/hooks/hooks_test.go @@ -5,15 +5,41 @@ package hooks import ( "os" - "os/user" "path/filepath" - "strconv" + "sync" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// chownCall records a single chown invocation. +type chownCall struct { + Path string + UID int + GID int +} + +// recordingChown returns a ChownFunc that records calls and a getter +// for the recorded calls. Thread-safe. +func recordingChown() (ChownFunc, func() []chownCall) { + var mu sync.Mutex + var calls []chownCall + + fn := func(path string, uid, gid int) error { + mu.Lock() + defer mu.Unlock() + calls = append(calls, chownCall{Path: path, UID: uid, GID: gid}) + return nil + } + get := func() []chownCall { + mu.Lock() + defer mu.Unlock() + return append([]chownCall{}, calls...) + } + return fn, get +} + func TestInjectFile_WritesContent(t *testing.T) { t.Parallel() @@ -139,61 +165,58 @@ func TestInjectEnvFile_EmptyMap(t *testing.T) { func TestInjectAuthorizedKeys_DefaultPaths(t *testing.T) { t.Parallel() - // Use current user's UID/GID so chown succeeds without root. - u, err := user.Current() - require.NoError(t, err) - uid, err := strconv.Atoi(u.Uid) - require.NoError(t, err) - gid, err := strconv.Atoi(u.Gid) - require.NoError(t, err) + chown, getCalls := recordingChown() rootfs := t.TempDir() - // Pre-create the default home directory. require.NoError(t, os.MkdirAll(filepath.Join(rootfs, "home", "sandbox"), 0o755)) pubKey := "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAATEST test@example.com" - hook := InjectAuthorizedKeys(pubKey, WithKeyUser("/home/sandbox", uid, gid)) + hook := InjectAuthorizedKeys(pubKey, WithChown(chown)) - err = hook(rootfs, nil) + err := hook(rootfs, nil) require.NoError(t, err) - // Verify .ssh directory exists. + // Verify .ssh directory exists with correct permissions. sshDir := filepath.Join(rootfs, "home", "sandbox", ".ssh") info, err := os.Stat(sshDir) require.NoError(t, err) assert.True(t, info.IsDir()) assert.Equal(t, os.FileMode(0o700), info.Mode().Perm()) - // Verify authorized_keys content. + // Verify authorized_keys content and permissions. akPath := filepath.Join(sshDir, "authorized_keys") got, err := os.ReadFile(akPath) require.NoError(t, err) assert.Equal(t, pubKey+"\n", string(got)) - // Verify authorized_keys permissions. info, err = os.Stat(akPath) require.NoError(t, err) assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + + // Verify chown was called with default sandbox UID/GID. + calls := getCalls() + require.Len(t, calls, 2) + for _, c := range calls { + assert.Equal(t, 1000, c.UID) + assert.Equal(t, 1000, c.GID) + } } func TestInjectAuthorizedKeys_CustomUser(t *testing.T) { t.Parallel() - // Use current user's UID/GID to avoid permission errors in tests. - u, err := user.Current() - require.NoError(t, err) - uid, err := strconv.Atoi(u.Uid) - require.NoError(t, err) - gid, err := strconv.Atoi(u.Gid) - require.NoError(t, err) + chown, getCalls := recordingChown() rootfs := t.TempDir() require.NoError(t, os.MkdirAll(filepath.Join(rootfs, "customhome"), 0o755)) pubKey := "ssh-rsa AAAAB3NzaC1yc2EAAAADTEST custom@host" - hook := InjectAuthorizedKeys(pubKey, WithKeyUser("/customhome", uid, gid)) + hook := InjectAuthorizedKeys(pubKey, + WithKeyUser("/customhome", 2000, 2000), + WithChown(chown), + ) - err = hook(rootfs, nil) + err := hook(rootfs, nil) require.NoError(t, err) // Verify custom path. @@ -206,6 +229,14 @@ func TestInjectAuthorizedKeys_CustomUser(t *testing.T) { info, err := os.Stat(filepath.Join(rootfs, "customhome", ".ssh")) require.NoError(t, err) assert.Equal(t, os.FileMode(0o700), info.Mode().Perm()) + + // Verify chown was called with the custom UID/GID. + calls := getCalls() + require.Len(t, calls, 2) + for _, c := range calls { + assert.Equal(t, 2000, c.UID) + assert.Equal(t, 2000, c.GID) + } } func TestInjectFile_RejectsPathTraversal(t *testing.T) {