diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 71813fdee..2cc9ec0af 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -66,12 +66,14 @@ jobs: | tee -a "$RUNNER_TEMP/root-tests.log" sudo env RSHELL_ROOT_INTEGRATION=1 RSHELL_BINARY="$helper_dir/rshell" \ "$RUNNER_TEMP/rshell.test" -test.v -test.timeout=2m \ - -test.run='^TestPrivilegedHelperRootIntegration$' \ + -test.run='^TestPrivilegedHelper(OwnSocket|ConcurrentSocketStarts)?RootIntegration$' \ | tee -a "$RUNNER_TEMP/root-tests.log" for test in TestRestrictEnforcesReadAndWritePolicy \ TestRestrictUsesValidatedDescriptorAfterPathReplacement \ TestDeniedSyscallRemainsDeniedAfterSelectiveElevation \ - TestPrivilegedHelperRootIntegration; do + TestPrivilegedHelperRootIntegration \ + TestPrivilegedHelperOwnSocketRootIntegration \ + TestPrivilegedHelperConcurrentSocketStartsRootIntegration; do if ! grep -Eq "^--- PASS: $test \\(" "$RUNNER_TEMP/root-tests.log"; then echo "Required root test did not pass: $test" >&2 exit 1 diff --git a/cmd/rshell/privileged_helper_linux.go b/cmd/rshell/privileged_helper_linux.go index 94429e0eb..761706fe6 100644 --- a/cmd/rshell/privileged_helper_linux.go +++ b/cmd/rshell/privileged_helper_linux.go @@ -19,6 +19,7 @@ import ( "os" "os/exec" "os/user" + "path/filepath" "runtime" "slices" "sort" @@ -45,6 +46,7 @@ func runPrivilegedHelper(ctx context.Context, args []string, stderr io.Writer) i flags.StringVar(&policyPath, "credential", policyPath, "deprecated alias for --policy") idleTimeout := flags.Duration("idle-timeout", 30*time.Second, "exit after this long without a connection") userName := flags.String("user", "dd-agent", "unprivileged effective user") + socketPath := flags.String("socket", "", "create and serve this Unix socket, owned by --user with mode 0600, instead of a systemd-activated one; the path must be clean and must not go through symlinks (such as /var/run), and every directory above it must be owned by root and not writable by others unless sticky; implies --idle-timeout=0") if err := flags.Parse(args); err != nil { return 2 } @@ -52,6 +54,18 @@ func runPrivilegedHelper(ctx context.Context, args []string, stderr io.Writer) i fmt.Fprintln(stderr, "privileged-helper does not accept positional arguments") return 2 } + if *socketPath != "" { + // Nothing restarts a helper that owns its socket on the next + // connection, as systemd socket activation does, so it must not exit + // when idle. + idleTimeoutSet := false + flags.Visit(func(f *flag.Flag) { idleTimeoutSet = idleTimeoutSet || f.Name == "idle-timeout" }) + if idleTimeoutSet && *idleTimeout != 0 { + fmt.Fprintln(stderr, "privileged-helper --socket requires --idle-timeout=0") + return 2 + } + *idleTimeout = 0 + } if os.Getuid() != 0 { fmt.Fprintln(stderr, "privileged-helper requires real uid 0") return 1 @@ -71,10 +85,25 @@ func runPrivilegedHelper(ctx context.Context, args []string, stderr io.Writer) i fmt.Fprintf(stderr, "resolving credentials for %s: %v\n", *userName, err) return 1 } - listener, err := inheritedListener() - if err != nil { - fmt.Fprintf(stderr, "loading systemd socket: %v\n", err) - return 1 + var listener net.Listener + if *socketPath != "" { + // Created last, once the helper is ready to serve, so that the socket + // existing means a working helper is behind it. + var lock *os.File + groups := append([]int{credentials.primaryGID}, credentials.supplementaryGIDs...) + listener, lock, err = createListener(*socketPath, credentials.uid, groups) + if err != nil { + fmt.Fprintf(stderr, "creating socket: %v\n", err) + return 1 + } + // Held until the helper exits, so no other helper starts on this path. + defer lock.Close() + } else { + listener, err = inheritedListener() + if err != nil { + fmt.Fprintf(stderr, "loading systemd socket: %v\n", err) + return 1 + } } defer listener.Close() if err := setProcessGroups(credentials.primaryGID, credentials.supplementaryGIDs); err != nil { @@ -252,6 +281,194 @@ func inheritedListener() (net.Listener, error) { return listener, nil } +// createListener creates the helper socket at path, owned by uid and the +// first of groups with mode 0600, while the helper still runs as root, and +// returns it with the lock that keeps other helpers off path. The caller holds +// the lock for as long as it serves. +// +// The socket is the only access control in front of the helper, so every +// directory above it must be one that only root (the helper's effective UID) +// can change: anyone else able to rename an entry on the way could swap in a +// socket they listen on. Every one of them must also let uid, or one of +// groups, through, so the helper's client can reach the socket. A stale socket +// is replaced; a socket that is still being served, or any other file, is not. +// +// The helper does not remove the socket when it exits, so that the +// socket-facing process, which serves as --user, never regains root for it. +// Removing it, and the lock file, is the supervisor's job, for example with +// systemd's RuntimeDirectory=. +func createListener(path string, uid int, groups []int) (*net.UnixListener, *os.File, error) { + if !filepath.IsAbs(path) { + return nil, nil, fmt.Errorf("socket path %q is not absolute", path) + } + // The checks below must examine the directories the kernel resolves. A + // path that is not clean could differ: the kernel resolves ".." after + // following a symlink, which filepath.Clean cannot see. + if path != filepath.Clean(path) { + return nil, nil, fmt.Errorf("socket path %q is not clean", path) + } + dir := filepath.Dir(path) + if err := requireTrustedAncestors(dir); err != nil { + return nil, nil, err + } + if err := requireTraversable(dir, uid, groups); err != nil { + return nil, nil, err + } + // Serializes the stale check, the bind and the chown across helpers: + // otherwise two of them can both find the old socket stale, and the + // second unlinks the socket the first just bound. + lock, err := lockSocketPath(path) + if err != nil { + return nil, nil, err + } + if err := removeStaleSocket(path); err != nil { + lock.Close() + return nil, nil, err + } + // The socket is created owner-only, so nobody can connect before it is + // handed to uid. + oldUmask := syscall.Umask(0o177) + listener, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) + syscall.Umask(oldUmask) + if err != nil { + lock.Close() + return nil, nil, fmt.Errorf("listen on %s: %w", path, err) + } + if err := os.Lchown(path, uid, groups[0]); err != nil { + // Still root here, so closing the listener unlinks the socket. + listener.Close() + lock.Close() + return nil, nil, fmt.Errorf("chown %s: %w", path, err) + } + listener.SetUnlinkOnClose(false) + return listener, lock, nil +} + +// lockSocketPath takes an exclusive lock on path + ".lock", a root-owned +// file next to the socket, and fails at once if another helper holds it. The +// kernel releases the lock when its holder exits, however it exits. +func lockSocketPath(path string) (*os.File, error) { + lockPath := path + ".lock" + lock, err := os.OpenFile(lockPath, os.O_RDWR|os.O_CREATE|syscall.O_NOFOLLOW|syscall.O_CLOEXEC, 0o600) + if err != nil { + return nil, fmt.Errorf("open %s: %w", lockPath, err) + } + info, err := lock.Stat() + if err != nil { + lock.Close() + return nil, fmt.Errorf("inspect %s: %w", lockPath, err) + } + if stat, ok := info.Sys().(*syscall.Stat_t); !ok || !info.Mode().IsRegular() || int(stat.Uid) != os.Geteuid() { + lock.Close() + return nil, fmt.Errorf("%s is not a regular file owned by uid %d", lockPath, os.Geteuid()) + } + if err := syscall.Flock(int(lock.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + lock.Close() + if errors.Is(err, syscall.EWOULDBLOCK) { + return nil, fmt.Errorf("another helper is serving %s", path) + } + return nil, fmt.Errorf("lock %s: %w", lockPath, err) + } + return lock, nil +} + +// requireTraversable reports an error unless uid, or one of groups, may +// search dir and every directory above it, as the kernel decides for a +// process with those credentials. +func requireTraversable(dir string, uid int, groups []int) error { + for { + info, err := os.Stat(dir) + if err != nil { + return fmt.Errorf("inspect socket directory %s: %w", dir, err) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("socket directory %s has no ownership information", dir) + } + perm := info.Mode().Perm() + var searchable bool + switch { + case uid == 0: + searchable = perm&0o111 != 0 + case int(stat.Uid) == uid: + searchable = perm&0o100 != 0 + case slices.Contains(groups, int(stat.Gid)): + searchable = perm&0o010 != 0 + default: + searchable = perm&0o001 != 0 + } + if !searchable { + return fmt.Errorf("socket directory %s cannot be searched by uid %d (mode %#o)", dir, uid, perm) + } + parent := filepath.Dir(dir) + if parent == dir { + return nil + } + dir = parent + } +} + +// removeStaleSocket removes a socket at path that nothing serves anymore. It +// refuses to replace a socket that accepts connections, or anything that is +// not a socket. +func removeStaleSocket(path string) error { + info, err := os.Lstat(path) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("inspect %s: %w", path, err) + } + if info.Mode()&os.ModeSocket == 0 { + return fmt.Errorf("%s exists and is not a socket", path) + } + conn, err := net.DialTimeout("unix", path, time.Second) + if err == nil { + conn.Close() + return fmt.Errorf("%s is already being served", path) + } + if !errors.Is(err, syscall.ECONNREFUSED) { + return fmt.Errorf("check whether %s is being served: %w", path, err) + } + if err := os.Remove(path); err != nil { + return fmt.Errorf("remove stale socket: %w", err) + } + return nil +} + +// requireTrustedAncestors reports an error unless dir and every directory +// above it is owned by root or the current effective UID and not writable by +// group or others. Directories above dir may instead be sticky, like /tmp: a +// sticky directory only lets owners rename or remove their own entries, and +// the directory below it is itself checked. dir cannot be sticky: the socket +// is owned by --user, who could then remove and replace it. +func requireTrustedAncestors(dir string) error { + for socketDir := true; ; socketDir = false { + info, err := os.Lstat(dir) + if err != nil { + return fmt.Errorf("inspect socket directory %s: %w", dir, err) + } + if !info.IsDir() { + return fmt.Errorf("socket directory %s is not a directory", dir) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("socket directory %s has no ownership information", dir) + } + if stat.Uid != 0 && int(stat.Uid) != os.Geteuid() { + return fmt.Errorf("socket directory %s is owned by uid %d, not root", dir, stat.Uid) + } + if info.Mode().Perm()&0o022 != 0 && (socketDir || info.Mode()&os.ModeSticky == 0) { + return fmt.Errorf("socket directory %s is writable by group or others (mode %#o)", dir, info.Mode().Perm()) + } + parent := filepath.Dir(dir) + if parent == dir { + return nil + } + dir = parent + } +} + type workerCommandFactory func(context.Context) (*exec.Cmd, error) type helperExecutor struct { diff --git a/cmd/rshell/privileged_helper_linux_test.go b/cmd/rshell/privileged_helper_linux_test.go index d9f50768f..2a538ad75 100644 --- a/cmd/rshell/privileged_helper_linux_test.go +++ b/cmd/rshell/privileged_helper_linux_test.go @@ -8,8 +8,12 @@ package main import ( + "bytes" + "context" + "net" "os" "path/filepath" + "syscall" "testing" "github.com/stretchr/testify/require" @@ -29,3 +33,185 @@ func TestLoadOptionalPolicyEmpty(t *testing.T) { require.NoError(t, err) require.Nil(t, credential) } + +// shortTempDir returns an owner-only temporary directory with a path short +// enough for a Unix socket (about 100 bytes). +func shortTempDir(t *testing.T) string { + t.Helper() + dir, err := os.MkdirTemp("", "rsh") + require.NoError(t, err) + t.Cleanup(func() { os.RemoveAll(dir) }) + require.NoError(t, os.Chmod(dir, 0o755)) + return dir +} + +func TestCreateListener(t *testing.T) { + path := filepath.Join(shortTempDir(t), "h.sock") + + listener, lock, err := createListener(path, os.Getuid(), []int{os.Getgid()}) + require.NoError(t, err) + t.Cleanup(func() { lock.Close() }) + + var stat syscall.Stat_t + require.NoError(t, syscall.Lstat(path, &stat)) + require.Equal(t, uint32(syscall.S_IFSOCK), stat.Mode&syscall.S_IFMT) + require.Equal(t, uint32(0o600), stat.Mode&0o777, "only the owner may connect") + require.Equal(t, uint32(os.Getuid()), stat.Uid) + + conn, err := net.Dial("unix", path) + require.NoError(t, err) + require.NoError(t, conn.Close()) + + // After the privilege drop the helper cannot unlink the socket, so it + // does not try: the supervisor removes it. + require.NoError(t, listener.Close()) + _, err = os.Lstat(path) + require.NoError(t, err) +} + +func TestCreateListenerReplacesStaleSocket(t *testing.T) { + path := filepath.Join(shortTempDir(t), "h.sock") + stale, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) + require.NoError(t, err) + stale.SetUnlinkOnClose(false) + require.NoError(t, stale.Close()) + + listener, lock, err := createListener(path, os.Getuid(), []int{os.Getgid()}) + require.NoError(t, err) + t.Cleanup(func() { lock.Close() }) + require.NoError(t, listener.Close()) +} + +func TestCreateListenerRefusesServedSocket(t *testing.T) { + path := filepath.Join(shortTempDir(t), "h.sock") + served, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) + require.NoError(t, err) + t.Cleanup(func() { served.Close() }) + + _, _, err = createListener(path, os.Getuid(), []int{os.Getgid()}) + require.ErrorContains(t, err, "already being served") + conn, err := net.Dial("unix", path) + require.NoError(t, err, "the served socket must stay reachable") + require.NoError(t, conn.Close()) +} + +func TestCreateListenerRejectsUnsafeTargets(t *testing.T) { + t.Run("relative path", func(t *testing.T) { + _, _, err := createListener("h.sock", os.Getuid(), []int{os.Getgid()}) + require.ErrorContains(t, err, "not absolute") + }) + t.Run("existing non-socket file", func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "h.sock") + require.NoError(t, os.WriteFile(path, []byte("keep"), 0o600)) + _, _, err := createListener(path, os.Getuid(), []int{os.Getgid()}) + require.ErrorContains(t, err, "is not a socket") + content, err := os.ReadFile(path) + require.NoError(t, err) + require.Equal(t, "keep", string(content), "a non-socket file must not be removed") + }) + t.Run("socket directory writable by others", func(t *testing.T) { + dir := shortTempDir(t) + require.NoError(t, os.Chmod(dir, 0o777)) + _, _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), []int{os.Getgid()}) + require.ErrorContains(t, err, "writable by group or others") + }) + t.Run("sticky socket directory", func(t *testing.T) { + // --user owns the socket and could replace it in a sticky directory. + dir := shortTempDir(t) + require.NoError(t, os.Chmod(dir, 0o777|os.ModeSticky)) + _, _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), []int{os.Getgid()}) + require.ErrorContains(t, err, "writable by group or others") + }) + t.Run("ancestor writable by others", func(t *testing.T) { + ancestor := shortTempDir(t) + dir := filepath.Join(ancestor, "d") + require.NoError(t, os.Mkdir(dir, 0o755)) + require.NoError(t, os.Chmod(ancestor, 0o777)) + _, _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), []int{os.Getgid()}) + require.ErrorContains(t, err, ancestor+" is writable by group or others") + }) + t.Run("path is not clean", func(t *testing.T) { + // The kernel resolves ".." after following link, so a check on the + // cleaned path would examine the wrong directory. + parent := shortTempDir(t) + link := filepath.Join(parent, "link") + require.NoError(t, os.Symlink(filepath.Join(shortTempDir(t), "sub"), link)) + _, _, err := createListener(link+"/../h.sock", os.Getuid(), []int{os.Getgid()}) + require.ErrorContains(t, err, "is not clean") + }) + t.Run("directory is a symlink", func(t *testing.T) { + parent := shortTempDir(t) + link := filepath.Join(parent, "link") + require.NoError(t, os.Symlink(shortTempDir(t), link)) + _, _, err := createListener(filepath.Join(link, "h.sock"), os.Getuid(), []int{os.Getgid()}) + require.ErrorContains(t, err, "is not a directory") + }) +} + +func TestCreateListenerLocksThePath(t *testing.T) { + path := filepath.Join(shortTempDir(t), "h.sock") + listener, lock, err := createListener(path, os.Getuid(), []int{os.Getgid()}) + require.NoError(t, err) + t.Cleanup(func() { listener.Close() }) + + _, _, err = createListener(path, os.Getuid(), []int{os.Getgid()}) + require.ErrorContains(t, err, "another helper is serving") + + // The lock outlives the socket: a helper whose socket was removed by + // someone else still keeps others off the path until it exits. + require.NoError(t, os.Remove(path)) + _, _, err = createListener(path, os.Getuid(), []int{os.Getgid()}) + require.ErrorContains(t, err, "another helper is serving") + + require.NoError(t, lock.Close()) + listener2, lock2, err := createListener(path, os.Getuid(), []int{os.Getgid()}) + require.NoError(t, err, "the lock is released when its holder closes it") + require.NoError(t, lock2.Close()) + require.NoError(t, listener2.Close()) +} + +func TestCreateListenerCleansUpFailedChown(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("root can chown to any account") + } + path := filepath.Join(shortTempDir(t), "h.sock") + _, _, err := createListener(path, 65534, []int{65534}) + require.ErrorContains(t, err, "chown") + _, err = os.Lstat(path) + require.ErrorIs(t, err, os.ErrNotExist, "a socket that was never handed over must not be left behind") + + listener, lock, err := createListener(path, os.Getuid(), []int{os.Getgid()}) + require.NoError(t, err, "a failed start must release the lock") + require.NoError(t, lock.Close()) + require.NoError(t, listener.Close()) +} + +func TestCreateListenerRequiresTraversableDirectories(t *testing.T) { + dir := shortTempDir(t) + require.NoError(t, os.Chmod(dir, 0o700)) + _, _, err := createListener(filepath.Join(dir, "h.sock"), 65534, []int{65534}) + require.ErrorContains(t, err, "cannot be searched by uid 65534") + + require.NoError(t, os.Chmod(dir, 0o711)) + require.NoError(t, requireTraversable(dir, 65534, []int{65534})) +} + +func TestRunPrivilegedHelperSocketFlags(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("checks the flag handling that runs before the root requirement") + } + socket := filepath.Join(shortTempDir(t), "h.sock") + var stderr bytes.Buffer + code := runPrivilegedHelper(context.Background(), []string{"--socket=" + socket, "--idle-timeout=5s"}, &stderr) + require.Equal(t, 2, code) + require.Contains(t, stderr.String(), "--socket requires --idle-timeout=0") + + for _, args := range [][]string{{"--socket=" + socket}, {"--socket=" + socket, "--idle-timeout=0"}} { + stderr.Reset() + code = runPrivilegedHelper(context.Background(), args, &stderr) + require.Equal(t, 1, code, "%v: the flags are accepted", args) + require.Contains(t, stderr.String(), "requires real uid 0") + } + _, err := os.Lstat(socket) + require.ErrorIs(t, err, os.ErrNotExist, "no socket is created before the helper is ready") +} diff --git a/cmd/rshell/privileged_helper_root_integration_test.go b/cmd/rshell/privileged_helper_root_integration_test.go index 91b36c77f..b9497c3e9 100644 --- a/cmd/rshell/privileged_helper_root_integration_test.go +++ b/cmd/rshell/privileged_helper_root_integration_test.go @@ -8,6 +8,7 @@ package main import ( + "bytes" "context" "crypto/ed25519" "crypto/rand" @@ -19,7 +20,11 @@ import ( "net" "os" "os/exec" + "os/user" "path/filepath" + "strconv" + "sync" + "syscall" "testing" "time" @@ -194,3 +199,161 @@ func signedIntegrationRequestForAction(t *testing.T, privateKey ed25519.PrivateK }}, }} } + +// TestPrivilegedHelperOwnSocketRootIntegration runs the helper with --socket +// as root, so the socket is created for another user and the helper really +// drops privileges, which unit tests cannot do. +func TestPrivilegedHelperOwnSocketRootIntegration(t *testing.T) { + if os.Getenv("RSHELL_ROOT_INTEGRATION") != "1" { + t.Skip("set RSHELL_ROOT_INTEGRATION=1 to run") + } + if os.Getuid() != 0 { + t.Fatal("root integration test must run with real uid 0") + } + binary := os.Getenv("RSHELL_BINARY") + if binary == "" { + t.Fatal("RSHELL_BINARY is required") + } + nobody, err := user.Lookup("nobody") + require.NoError(t, err) + + dir, err := os.MkdirTemp("", "rsh") + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, os.RemoveAll(dir)) }) + require.NoError(t, os.Chmod(dir, 0o755)) + socket := filepath.Join(dir, "h.sock") + + start := func() (*exec.Cmd, *bytes.Buffer) { + var stderr bytes.Buffer + command := exec.Command(binary, "privileged-helper", "--user=nobody", "--socket="+socket) + command.Stderr = &stderr + return command, &stderr + } + waitServing := func(command *exec.Cmd, stderr *bytes.Buffer) { + require.Eventually(t, func() bool { + conn, err := net.Dial("unix", socket) + if err != nil { + return false + } + conn.Close() + return true + }, 10*time.Second, 50*time.Millisecond, "helper did not serve its socket: %s", stderr) + } + stop := func(command *exec.Cmd) { + _ = command.Process.Kill() + _ = command.Wait() + } + + first, firstStderr := start() + require.NoError(t, first.Start()) + t.Cleanup(func() { stop(first) }) + waitServing(first, firstStderr) + + var stat syscall.Stat_t + require.NoError(t, syscall.Lstat(socket, &stat)) + require.Equal(t, uint32(syscall.S_IFSOCK), stat.Mode&syscall.S_IFMT) + require.Equal(t, uint32(0o600), stat.Mode&0o777) + require.Equal(t, nobody.Uid, strconv.Itoa(int(stat.Uid)), "the socket belongs to --user") + status, err := os.ReadFile(fmt.Sprintf("/proc/%d/status", first.Process.Pid)) + require.NoError(t, err) + require.Regexp(t, `(?m)^Uid:\s+0\s+`+nobody.Uid+`\s`, string(status), "the helper keeps real uid 0 and serves as --user") + + // A second helper must not take over a socket that is still served. + second, secondStderr := start() + err = second.Run() + var exitErr *exec.ExitError + require.ErrorAs(t, err, &exitErr) + require.Regexp(t, "another helper is serving|already being served", secondStderr.String()) + + // The helper cannot unlink the socket after dropping privileges, so it is + // left for the supervisor; the next start replaces it. + stop(first) + _, err = os.Lstat(socket) + require.NoError(t, err) + third, thirdStderr := start() + require.NoError(t, third.Start()) + t.Cleanup(func() { stop(third) }) + waitServing(third, thirdStderr) +} + +// TestPrivilegedHelperConcurrentSocketStartsRootIntegration starts many +// helpers on the same --socket path at once, over a stale socket. Exactly one +// may serve: without the startup lock, two can both find the stale socket +// stale, and the later one unlinks the socket the earlier one just bound. +func TestPrivilegedHelperConcurrentSocketStartsRootIntegration(t *testing.T) { + if os.Getenv("RSHELL_ROOT_INTEGRATION") != "1" { + t.Skip("set RSHELL_ROOT_INTEGRATION=1 to run") + } + if os.Getuid() != 0 { + t.Fatal("root integration test must run with real uid 0") + } + binary := os.Getenv("RSHELL_BINARY") + if binary == "" { + t.Fatal("RSHELL_BINARY is required") + } + const starters = 20 + for iteration := 0; iteration < 60; iteration++ { + dir, err := os.MkdirTemp("", "rsh") + require.NoError(t, err) + require.NoError(t, os.Chmod(dir, 0o755)) + socket := filepath.Join(dir, "h.sock") + // Start from a stale socket, as after a crash: every starter then + // finds it stale, which is what makes them race to replace it. + stale, err := net.ListenUnix("unix", &net.UnixAddr{Name: socket, Net: "unix"}) + require.NoError(t, err) + stale.SetUnlinkOnClose(false) + require.NoError(t, stale.Close()) + + var ( + mu sync.Mutex + processes []*os.Process + ) + exited := make(chan struct{}, starters) + release := make(chan struct{}) + var ready sync.WaitGroup + for range starters { + command := exec.Command(binary, "privileged-helper", "--user=nobody", "--socket="+socket) + ready.Add(1) + go func() { + ready.Done() + <-release + if err := command.Start(); err != nil { + exited <- struct{}{} + return + } + mu.Lock() + processes = append(processes, command.Process) + mu.Unlock() + _ = command.Wait() + exited <- struct{}{} + }() + } + ready.Wait() + close(release) + + // Every starter but one exits; the survivor keeps serving. + for i := 0; i < starters-1; i++ { + select { + case <-exited: + case <-time.After(20 * time.Second): + t.Fatalf("iteration %d: only %d of %d starters exited", iteration, i, starters-1) + } + } + select { + case <-exited: + t.Fatalf("iteration %d: every starter exited, none serves", iteration) + case <-time.After(500 * time.Millisecond): + } + conn, err := net.Dial("unix", socket) + require.NoError(t, err, "iteration %d: the surviving helper's socket must be reachable", iteration) + conn.Close() + + mu.Lock() + for _, process := range processes { + _ = process.Kill() + } + mu.Unlock() + <-exited + require.NoError(t, os.RemoveAll(dir)) + } +} diff --git a/docs/PRIVILEGED_HELPER.md b/docs/PRIVILEGED_HELPER.md index 0731a67f4..09e13c562 100644 --- a/docs/PRIVILEGED_HELPER.md +++ b/docs/PRIVILEGED_HELPER.md @@ -1,8 +1,39 @@ # Privileged helper -The `rshell privileged-helper` mode is the Linux-only, socket-activated -execution boundary for selectively elevated Private Action Runner tasks. It is -not enabled by the normal rshell CLI. +The `rshell privileged-helper` mode is the Linux-only execution boundary for +selectively elevated Private Action Runner tasks. It is not enabled by the +normal rshell CLI. + +The helper serves one Unix socket, which only the `dd-agent` account can +connect to. It either inherits it from systemd socket activation (fd 3 with +`LISTEN_PID` and `LISTEN_FDS=1`), or, with `--socket=PATH`, creates it itself +once its policy and account are loaded, before dropping privileges: owned by +`--user` and its primary group, with mode `0600`. + +With `--socket`: + +- `PATH` must be absolute and clean (no `.`, `..` or repeated slashes), and + must not go through symlinks: under `/var/run`, which is usually a symlink to + `/run`, use `/run` directly. +- Every directory above `PATH` must be owned by root and not writable by group + or others, so nobody else can swap in a socket of their own, and must let + `--user` or one of its groups search it, so the client can reach the socket. Directories + further up may instead be sticky, like `/tmp`; the socket's own directory may + not, since `--user` owns the socket. +- The helper holds an exclusive lock on `PATH.lock`, a root-owned file next to + the socket, for as long as it runs, so only one helper serves `PATH`; a + second one refuses to start. The kernel releases the lock however the helper + exits. +- An existing socket at `PATH` is replaced only if nothing accepts connections + on it. A socket that is still being served, or any other file, makes the + helper refuse to start. +- The helper does not remove the socket when it exits, so that the + socket-facing process, which serves as `--user`, never regains root for it. + Removing it, and the lock file, is the supervisor's job, for example by + placing them in a systemd `RuntimeDirectory=`. A socket that fails to start + is removed before the helper exits. +- `--idle-timeout` defaults to `0`, and a non-zero value is rejected: nothing + would start the helper again on the next connection. The helper must start with real UID 0. It loads its trust policy, resolves the `dd-agent` UID and complete group set, permanently switches its real, effective,