feat(privileged-helper): create and serve its own socket with --socket by astuyve · Pull Request #700 · DataDog/rshell

astuyve · 2026-10-27T15:47:45Z

What does this PR do?

Adds --socket=PATH to rshell privileged-helper. With it, the helper creates its socket itself instead of requiring systemd socket activation. It does this after loading its policy and account, and before dropping privileges:

Ownership and mode: owned by --user and its primary group, mode 0600, created under a 0177 umask so nobody can connect before ownership is handed over.
Directory check: every directory above PATH must be owned by root and not writable by group or others. Directories further up may instead be sticky, like /tmp; the socket's own directory may not, because --user owns the socket. This stops anyone else from swapping in their own socket.
Existing files: a socket at PATH is replaced only if nothing accepts connections on it (ECONNREFUSED). A socket that's still being served, or any other file, makes the helper refuse to start.
Cleanup: the helper doesn't remove the socket on exit, because by then it runs as --user and can't write the directory. Removing it is the supervisor's job, for example with systemd RuntimeDirectory=.
Idle timeout: --idle-timeout defaults to 0 with --socket, and a non-zero value is rejected, since nothing would restart the helper on demand.

Without --socket, the systemd-activated path is unchanged.

Motivation

The Datadog Agent now starts the helper from a single systemd service, and only when the Agent configuration enables it. Since the helper only accepts an inherited socket (fd 3, LISTEN_PID, LISTEN_FDS=1), the Agent currently creates the socket and fakes socket activation through a /bin/sh trampoline that moves the descriptor and sets LISTEN_PID. With --socket, the Agent's launcher only checks its configuration and then execs the helper directly. It also means the socket exists only while a ready helper serves it.

Agent PR: DataDog/datadog-agent#58809. It's pinned to this branch for testing and will move to the release that includes this.

Testing

Unit tests:
- socket creation (mode, owner, connecting), replacing a stale socket, refusing a socket that's still served;
- refusing a relative path, an existing non-socket file (left intact), a writable or sticky socket directory, a writable ancestor, and a symlinked directory;
- --socket flag handling, including that no socket exists before the helper is ready.
Root integration test TestPrivilegedHelperOwnSocketRootIntegration: runs the real binary as root with --socket --user=nobody and checks:
- the socket's owner and mode;
- real UID 0 and effective nobody;
- that a second instance refuses the live socket;
- that the socket remains after exit, and a restart replaces it.
It and the existing root test pass under sudo with the CI setup.
go test ./cmd/rshell/ ./privilegedhelper/, go vet and gofmt are clean.

Checklist

Tests added/updated
Documentation updated (if applicable)

🤖 Generated with Claude Code

`rshell privileged-helper` only accepted a systemd-activated socket (fd 3 with LISTEN_PID and LISTEN_FDS=1). An embedding application that starts the helper itself had to fake socket activation, moving the descriptor to fd 3 and setting LISTEN_PID through a shell trampoline. With --socket=PATH the helper creates the socket itself, after loading its policy and account and before dropping privileges, so the socket exists only while a ready helper serves it: - owned by --user and its primary group, mode 0600, created under a 0177 umask so nobody can connect before it is handed over; - its directory must be owned by the helper's effective UID (root) and not writable by group or others, since anyone able to write it could replace the socket with their own listener; - an existing socket at PATH is replaced, any other file is not; - it is removed when the helper exits. Without --socket, the systemd-activated path is unchanged. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Pin github.com/DataDog/rshell to the DataDog/rshell#700 branch commit (v0.0.31-0.20261027154721-1f4107c16f20) for testing; it adds `privileged-helper --socket=PATH`, which creates and serves the socket itself before dropping privileges. Move to the release once it lands. The privileged-helper launcher now only checks the configuration and Landlock, then execs the helper with --socket. That removes the /bin/sh trampoline, the descriptor handoff and the launcher's socket creation. The socket now appears only once the helper has loaded its policy and account, and the helper removes it when it exits. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

matt-dz

🤖 Reviewed by Matthew's coding agent:

Verdict: changes needed. Nothing here is a sandbox escape, and the creation path itself is careful: absolute path, 0177 umask before bind, Lchown before serving, and the non-socket file is left alone. But the cleanup half of the feature doesn't work as written, and the PR description and docs rely on it ("removed when the helper exits", "the socket exists only while a ready helper serves it").

The socket is never removed in production (main issue). SetUnlinkOnClose(true) unlinks during the deferred listener.Close(), and by then setEffectiveUID has dropped the effective UID to dd-agent. Unlinking from the root-owned, non-group/other-writable directory that requireTrustedDirectory requires then fails with EACCES, and Go discards that error silently. I checked this as root: createListener(path, 65534, 65534) → setEffectiveUID(65534) → Close() leaves the socket in place. Separately, main.go passes context.Background() and installs no signal handler, so a systemctl stop (SIGTERM) kills the process without running defers anyway. The unit test passes only because it never drops privileges.
The default --idle-timeout=30s is a trap with --socket. After 30s idle the helper exits 0 and leaves a dead socket (see 1) that nothing will re-activate. The Agent passes --idle-timeout=0, but the flag is generic, and right now only the docs prevent this.
The trust check covers only the immediate parent. Anyone who can write an ancestor directory can rename the checked directory away and put their own in its place, either before the check or between the check and bind. That is exactly the "replace the socket with one they listen on" threat the comment describes.
A live socket is replaced without checking. Any socket at PATH counts as stale, including one another helper instance is serving.
Tests. Nothing exercises the real sequence (chown to another UID → privilege drop → close) or the --socket wiring in runPrivilegedHelper. A root-gated test would have caught 1.

go test -race ./cmd/rshell/ ./privilegedhelper/, go vet, and gofmt are clean on 1f4107c.

matt-dz · 2026-10-27T16:44:16Z

+		listener.Close()
+		return nil, fmt.Errorf("chown %s: %w", path, err)
+	}
+	listener.SetUnlinkOnClose(true)


🤖 Matthew's coding agent:

This unlink can't succeed in production. net.UnixListener.Close unlinks the path when it runs, and that happens in the deferred listener.Close() (line 92), after setEffectiveUID(credentials.uid). By then the process's fsuid is dd-agent, and requireTrustedDirectory has just guaranteed that dd-agent can't write the parent directory, so unlink returns EACCES. Go ignores that error.

I checked this as root with a throwaway test: createListener(path, 65534, 65534), then setEffectiveUID(65534), then l.Close(); afterwards os.Lstat(path) still returns nil. TestCreateListener passes only because it never drops privileges.

The directory-trust requirement and unlink-after-drop can't both hold. I'd suggest SetUnlinkOnClose(false) (a stale socket is already handled on the next start), and dropping the "removed when the helper exits" / "exists only while a ready helper serves it" claims, or putting cleanup on the supervisor (RuntimeDirectory=). As written, the documented invariant is false.

Agreed, and thanks for checking it as root. Fixed in 6e02cbc: the listener no longer unlinks on close (SetUnlinkOnClose(false)), the docs now say cleanup is the supervisor's job (the Agent's unit uses RuntimeDirectory=, which removes it on stop and between restarts), and a stale socket is replaced on the next start. The new root integration test asserts the socket is left behind after the helper exits.

matt-dz · 2026-10-27T16:44:16Z

+			return 1
+		}
 	}
 	defer listener.Close()


🤖 Matthew's coding agent:

Even with an unlink that works, this defer doesn't run on the normal stop path. main.go calls runPrivilegedHelper(context.Background(), ...) and installs no signal handler, so when systemd sends SIGTERM on systemctl stop, Go's default handler terminates the process and no defers run. If removing the socket on exit is meant to be a guarantee, the helper needs signal.NotifyContext(ctx, SIGTERM, SIGINT) so Serve returns context.Canceled and the defers run. That still hits the EACCES problem above, though.

With cleanup moved to the supervisor, the helper no longer needs its defers to run on SIGTERM, so I left signal handling as is. The supervisor removes the socket regardless of how the helper exits.

matt-dz · 2026-10-27T16:44:16Z

+	if *socketPath != "" {
+		// Created last, once the helper is ready to serve, so that the socket
+		// existing means a working helper is behind it.
+		listener, err = createListener(*socketPath, credentials.uid, credentials.primaryGID)


🤖 Matthew's coding agent:

With --socket, the 30s default --idle-timeout is a trap. With systemd socket activation an idle exit is harmless because systemd re-activates the helper on the next connection. Here the helper returns 0 after 30s with no traffic, so Restart=on-failure won't restart it, and it leaves a dead socket behind (see the unlink comment). The docs warn about this, but the code doesn't enforce it. Could --socket default the idle timeout to 0, or reject a non-zero value unless it was passed explicitly (flags.Visit tells you)? The Agent passes --idle-timeout=0, but any other caller of this flag gets a helper that quietly disappears.

Fixed in 6e02cbc: with --socket, --idle-timeout defaults to 0, and an explicit non-zero value is rejected with exit 2 (detected through flags.Visit). Covered by TestRunPrivilegedHelperSocketFlags.

matt-dz · 2026-10-27T16:44:16Z

+	if !filepath.IsAbs(path) {
+		return nil, fmt.Errorf("socket path %q is not absolute", path)
+	}
+	if err := requireTrustedDirectory(filepath.Dir(path)); err != nil {


🤖 Matthew's coding agent:

This trusts only filepath.Dir(path). The threat model in the doc comment is "anyone who could write it could replace the socket with one they listen on", and that applies to every ancestor too. Anyone who can write the grandparent can rename the checked directory away and put in one they control, either before the check or between this check and ListenUnix, which re-resolves the path. Clients would then connect to an attacker's socket and send it signed requests. Two ways to close this: walk the ancestors up to / with the same owner/mode check (as OpenSSH StrictModes does), or open the directory once with O_PATH|O_DIRECTORY|O_NOFOLLOW, run the checks on that fd with fstat, and bind and fchownat through /proc/self/fd/N/<base> so the check and the bind refer to the same inode. The /run/datadog-agent-rshell deployment is safe, but the function is generic and its comment claims a stronger guarantee than it gives.

Fixed in 6e02cbc with the StrictModes approach: requireTrustedAncestors walks up to / and requires each directory to be owned by root (the effective UID) and not group/other writable. Directories above the socket's own directory may instead be sticky, like /tmp. The socket's own directory may not, because --user owns the socket and could replace it there. Since nobody else can modify any ancestor, the check and the bind resolve to the same directory. Tests cover a writable ancestor and a sticky socket directory.

matt-dz · 2026-10-27T16:44:16Z

+	switch {
+	case err == nil && info.Mode()&os.ModeSocket == 0:
+		return nil, fmt.Errorf("%s exists and is not a socket", path)
+	case err == nil:


🤖 Matthew's coding agent:

Every socket at path is treated as stale, including one a running helper (or an old socket-activated unit) is still listening on. That helper keeps its fd but becomes unreachable, and this one takes over without logging anything. A cheap guard is to net.DialTimeout("unix", path, …) first: a successful connect means something is serving, so refuse to start. Only ECONNREFUSED means the socket is really stale.

Fixed in 6e02cbc: removeStaleSocket dials first. A successful connect refuses to start (already being served), only ECONNREFUSED removes the socket, and any other error refuses. Covered by TestCreateListenerRefusesServedSocket and the root integration test, which starts a second helper against a live one.

matt-dz · 2026-10-27T16:44:16Z

+`--user` with mode `0600`, and removed when the helper exits. The directory
+holding `PATH` must be owned by root and not writable by group or others, and
+an existing file at `PATH` is only replaced if it is a socket. With `--socket`,
+the socket exists only while a ready helper serves it. Pass


🤖 Matthew's coding agent:

Given the unlink and SIGTERM issues above, "removed when the helper exits" (line 11) and "the socket exists only while a ready helper serves it" are not true today. After any exit, a dead socket stays at PATH until the next start replaces it, so clients get ECONNREFUSED rather than ENOENT. Please either make the code meet these claims or reword them, and say whether cleanup is the supervisor's job (e.g. RuntimeDirectory=).

Reworded in 6e02cbc: the docs no longer claim removal on exit or that the socket exists only while a helper serves it. They describe the ancestor rule, live-socket refusal, supervisor cleanup (RuntimeDirectory=), and the forced idle timeout.

matt-dz · 2026-10-27T16:44:16Z

+	return dir
+}
+
+func TestCreateListener(t *testing.T) {


🤖 Matthew's coding agent:

This test runs as the invoking user, chowns to that same user, and never drops privileges, so it can't catch the two production-only behaviours: chown to a different UID, and unlink after the euid drop. A test gated on os.Getuid() == 0 (skip otherwise) that creates the socket for a different UID such as 65534, checks the owner, drops with setEffectiveUID, then closes and asserts what happens to the path would have found the cleanup bug. Nothing tests the --socket wiring in runPrivilegedHelper either (that the socket is created before the privilege drop, and the flag error path).

Added TestPrivilegedHelperOwnSocketRootIntegration in 6e02cbc. It runs the real binary as root with --socket --user=nobody, checks the socket is owned by nobody with mode 0600 and that the helper's Uid line shows real 0 and effective nobody, checks a second instance refuses the live socket, checks the socket remains after the helper is killed, and checks a restart replaces it. TestRunPrivilegedHelperSocketFlags covers the flag wiring and checks no socket exists before the helper is ready. Both root tests pass locally under sudo with the CI setup.

- Do not unlink the socket on close: by then the helper's effective UID is --user, which cannot write the root-owned directory, so the unlink failed silently and the docs promised cleanup that never happened. Removing the socket is the supervisor's job (e.g. RuntimeDirectory=). - With --socket, default --idle-timeout to 0 and reject a non-zero value: nothing restarts a helper that owns its socket on demand. - Check every ancestor directory, not only the parent: each must be owned by root (the effective UID) and not group/other writable, or sticky above the socket's own directory. Otherwise anyone able to rename an entry on the way could swap in their own socket. - Refuse to replace a socket that still accepts connections; only an ECONNREFUSED socket is stale. - Add a root integration test that runs the helper with --socket and --user=nobody: socket owner and mode, the effective UID after the drop, refusal of a served socket, and replacement of a stale one. Add flag-handling and ancestor tests, and correct the docs. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Move the testing pin to DataDog/rshell#700's 6e02cbc8 (v0.0.31-0.20261027164856-6e02cbc8081f), which addresses review: the helper no longer tries to unlink its socket after dropping privileges (the unit's RuntimeDirectory= removes it), checks every ancestor directory, refuses a socket that is still served, and forces --idle-timeout=0 with --socket. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

matt-dz

🤖 Reviewed by Matthew's coding agent:

Re-review of 6e02cbc against the five points from the earlier review.

Verdict: nearly there. Points 1–4 are fixed. Point 5 is only half done, because the new root test never runs in CI. I also found one new, small gap in the ancestor check. Both are quick fixes.

Fixed

Socket cleanup. SetUnlinkOnClose(false) is now set right after ListenUnix, and the code comment and docs say cleanup is the supervisor's job (RuntimeDirectory=). The false claims are gone. With cleanup moved off the helper, not handling SIGTERM is fine, and a socket left behind is handled by removeStaleSocket on the next start. Minor wording, not blocking: "it runs as --user, which cannot write the directory" is not strictly true, because setEffectiveUID changes only the effective UID, so real and saved UID stay 0 and the helper could switch back to unlink. "Does not" fits better than "cannot". The decision itself is right: the socket-facing process shouldn't regain root for this.
Idle timeout. With --socket it defaults to 0, and an explicit non-zero value exits 2 (detected with flags.Visit). Covered by TestRunPrivilegedHelperSocketFlags.
Ancestor check. requireTrustedAncestors walks up to /. Each directory must be root-owned and not writable by group or others; directories above the socket's own may be sticky, and the socket's own may not. That closes the rename-a-parent attack for normal paths. One gap remains, covered inline: the check runs on filepath.Clean(path), but the bind uses the raw path.
Live socket. removeStaleSocket dials first and removes the socket only on ECONNREFUSED. Any other error makes the helper refuse to start, which is correctly fail-closed.

Still open
5. The root test exists but CI never runs it. In .github/workflows/test.yml, the root job filters with -test.run='^TestPrivilegedHelperRootIntegration$' and only requires that test to pass. In this PR's CI run, TestPrivilegedHelperOwnSocketRootIntegration is --- SKIP in the race job and absent from the root job. I ran both root tests under sudo with the CI setup and they pass, so this only needs wiring up: widen the filter to '^TestPrivilegedHelper(OwnSocket)?RootIntegration$' and add the test to the required-pass list.

New

A symlink followed by .. in --socket gets past the ancestor check (inline). I reproduced it as root, details in the inline comment. The impact is limited because the path comes from the root supervisor, not a script, but the check is meant to fail closed on exactly this.

go test -race ./..., go vet ./... and gofmt -l . are clean on 6e02cbc. Both root integration tests pass under sudo.

matt-dz · 2026-10-27T17:01:31Z

+	if !filepath.IsAbs(path) {
+		return nil, fmt.Errorf("socket path %q is not absolute", path)
+	}
+	if err := requireTrustedAncestors(filepath.Dir(filepath.Clean(path))); err != nil {


🤖 Matthew's coding agent:

The ancestor check runs on filepath.Clean(path), but removeStaleSocket(path) and net.ListenUnix (line 304) use the raw path. Clean removes .. as text, while the kernel resolves .. after following a symlink, so the two can name different directories.

I reproduced this as root with a throwaway test. Setup: /tmp/rshA is root-owned with mode 0755, /tmp/rshA/link is a symlink to /tmp/evil/sub, and /tmp/evil is mode 0777 and owned by nobody. Then createListener("/tmp/rshA/link/../h.sock", 65534, 65534):

returns nil;

checks only /tmp/rshA, /tmp and /;

binds the socket at /tmp/evil/h.sock, in a directory anyone can write;

creates nothing at /tmp/rshA/h.sock.

The path is chosen by the root supervisor, so this needs a misconfiguration, not an attack from a script. Still, the function's comment promises that every directory above the socket is root-controlled, and this breaks that promise silently. Simplest fix: reject the path unless path == filepath.Clean(path), or use the cleaned path for every later step. Either way, the walk's Lstat already rejects symlinked components, so every component it checks is then the one the kernel resolves. A link/../h.sock subtest next to "directory is a symlink" would pin it.

Side note, not blocking: because every component is checked with Lstat, paths under /var/run (a symlink to /run on most distros) fail with "is not a directory", which is hard to diagnose. Mentioning in the flag help or docs that the path must not go through symlinks would save someone a debugging session.

Good catch, thanks for reproducing it. Fixed in d57cc35: createListener now rejects a --socket path unless it equals filepath.Clean(path), so the walk's Lstat checks exactly the components the kernel resolves. Added a "path is not clean" subtest using link/../h.sock. The flag help and docs now say the path must not go through symlinks, with /var/run as the example.

matt-dz · 2026-10-27T17:01:31Z

+// 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) {


🤖 Matthew's coding agent:

This test covers the right sequence: socket chowned to nobody, mode 0600, Uid: line showing real 0 and effective nobody, a live socket refused, the socket left behind after exit, and a restart replacing it. It passes for me under sudo with the CI setup. But it never runs in CI. The root job in .github/workflows/test.yml runs -test.run='^TestPrivilegedHelperRootIntegration$' and only requires that test in its pass loop. In this PR's run, this test shows --- SKIP in "Run tests with race detector" and doesn't appear in "Root sandbox enforcement (Linux)" at all. Could you widen the filter to '^TestPrivilegedHelper(OwnSocket)?RootIntegration$' and add TestPrivilegedHelperOwnSocketRootIntegration to the required-pass list? Otherwise a regression in the drop-privileges path ships green.

Wired up in d57cc35: the root job now runs -test.run='^TestPrivilegedHelper(OwnSocket)?RootIntegration$' and requires TestPrivilegedHelperOwnSocketRootIntegration in its pass loop. Both pass locally under sudo with that exact filter and the CI directory setup.

matt-dz · 2026-10-27T17:01:31Z

+  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: by then it runs as
+  `--user`, which cannot write the directory. Removing it is the supervisor's


🤖 Matthew's coding agent:

Nit, not blocking: setEffectiveUID changes only the effective UID (setresuid(unchanged, uid, unchanged)), so real and saved UID stay 0. The helper could switch back to unlink, it just shouldn't. Something like "does not remove the socket when it exits, so the socket-facing process never regains root for it" would describe the reason accurately. Same wording in the comment above createListener.

Agreed. Reworded in d57cc35 in both the docs and the createListener comment: the helper does not remove the socket, so the socket-facing process, which serves as --user, never regains root for it.

…ot test in CI - Reject a --socket path that is not already clean. The ancestor check examined filepath.Clean(path) while the bind used the raw path, and the kernel resolves ".." after following a symlink, so link/../h.sock could bind in a directory the check never saw. - Run TestPrivilegedHelperOwnSocketRootIntegration in the root CI job and require it to pass; it was skipped everywhere in CI. - Say the helper does not, rather than cannot, remove its socket: only its effective UID drops, and it deliberately never regains root for it. Document that the path must not go through symlinks such as /var/run. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Move the testing pin to DataDog/rshell#700's d57cc35c (v0.0.31-0.20261027170324-d57cc35c5040), which also rejects --socket paths that are not clean. The Agent's path, /run/datadog-agent-rshell/privileged.sock, is clean and has no symlinked components. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

astuyve · 2026-10-27T17:45:12Z

Manual QA of d57cc35c through DataDog/datadog-agent#58809 (pinned to this commit). I upgraded an Ubuntu 24.04 VM (kernel 7.0, systemd 255) in place from Agent 7.85.0-rc.8:

Startup: the Agent's unit runs rshell privileged-helper --socket=/run/datadog-agent-rshell/privileged.sock --idle-timeout=0 directly as root. The ancestor check accepts the systemd RuntimeDirectory. The helper serves with real uid 0 and effective uid dd-agent, and the socket is dd-agent:dd-agent 0600.
Access: dd-agent can connect; nobody is denied.
Live socket: a second helper started against the live socket exits 1 with "already being served", and the original keeps serving.
Cleanup: stopping the helper makes systemd remove the socket along with the RuntimeDirectory, so cleanup by the supervisor works as documented.
Restarts and reboot: the helper comes back on Agent restart and after a reboot.

🤖 Generated with Claude Code

julesmcrt

Additional runtime QA on Ubuntu 24.04 and RHEL 9 found the three socket-startup issues below. The concurrent-start finding is the one I consider blocking.

julesmcrt · 2026-10-27T19:01:32Z

+	if err := requireTrustedAncestors(filepath.Dir(path)); err != nil {
+		return nil, err
+	}
+	if err := removeStaleSocket(path); err != nil {


removeStaleSocket, ListenUnix, and Lchown need process-shared serialization. Two helpers can both classify the original socket as stale; after the first binds, the second can unlink that newly live socket and bind its own. Both processes then stay alive, but only the final socket remains reachable, and either process can chown the final pathname.

I reproduced this on real VMs with 20 synchronized starters:

Ubuntu 24.04: 2 live helpers by iteration 4.

RHEL 9: 20 live helpers by iteration 142.

The systemd units normally reduce the likelihood, but createListener itself does not uphold its live-socket refusal guarantee. Could startup take a root-owned flock in the trusted socket directory and hold it through stale inspection, bind, and Lchown? A synchronized multi-process root test would cover the observed interleaving.

Thanks for the VM reproduction. Fixed in b631836: createListener takes an exclusive, non-blocking flock on a root-owned PATH.lock next to the socket before the stale check, holds it through bind and Lchown, and the helper keeps it until it exits. A second helper fails at once with "another helper is serving", and the kernel releases the lock however the holder exits. Added TestPrivilegedHelperConcurrentSocketStartsRootIntegration: 20 synchronized starters over a stale socket, 60 iterations, exactly one survivor whose socket is reachable. With the lock disabled it reproduces your result here, two live helpers by iteration 23. It's in the root CI job's filter and required-pass list.

Verified independently at c6e6e1a on Ubuntu 24.04. The new root integration test passed, and a separate synchronized 20-starter harness completed 1,000 stale-socket rounds with exactly one reachable survivor each time. The lock is root:root 0600 and is held for the helper lifetime. This fixes the race I reported—thanks.

julesmcrt · 2026-10-27T19:01:32Z

+	if err != nil {
+		return nil, fmt.Errorf("listen on %s: %w", path, err)
+	}
+	listener.SetUnlinkOnClose(false)


SetUnlinkOnClose(false) is set before Lchown. If Lchown fails, startup returns an error but leaves a root-owned 0600 socket behind. I reproduced this by dropping CAP_CHOWN.

Moving SetUnlinkOnClose(false) until after successful Lchown lets the default close behavior clean up startup failures while preserving supervisor-owned cleanup after readiness.

Fixed in b631836: SetUnlinkOnClose(false) now runs only after Lchown succeeds, so a failed chown, still running as root, removes the socket when the listener closes, and the lock is released. TestCreateListenerCleansUpFailedChown covers it by chowning to 65534 as a non-root user, then checks the socket is gone and a retry succeeds.

Verified independently at c6e6e1a by dropping CAP_CHOWN. Startup failed at Lchown, the socket was removed, and the lock was no longer held. This fixes the cleanup issue I reported—thanks.

julesmcrt · 2026-10-27T19:01:32Z

+	if path != filepath.Clean(path) {
+		return nil, fmt.Errorf("socket path %q is not clean", path)
+	}
+	if err := requireTrustedAncestors(filepath.Dir(path)); err != nil {


The trust check accepts a root-owned 0700 socket directory even when --user cannot traverse it. The helper then stays healthy with a correctly owned socket that its intended client cannot connect to (EACCES).

Production's RuntimeDirectoryMode=0755 avoids this, but the generic flag should either validate traversal for the resolved account or document it as a required precondition.

Fixed in b631836: requireTraversable checks that --user, or its primary or a supplementary group, can search the socket's directory and every directory above it, using the same owner/group/other rule the kernel applies, and the helper refuses to start otherwise. TestCreateListenerRequiresTraversableDirectories covers a 0700 directory, which is rejected, and 0711, which is accepted. The docs list it as a requirement.

Verified independently at c6e6e1a: a root-owned 0700 directory is now rejected before socket creation with the expected traversal error, while the production 0755 RuntimeDirectory path remains accepted. This fixes the reachability issue I reported—thanks.

… chown, check traversal Runtime QA findings on Ubuntu 24.04 and RHEL 9: - Concurrent starts could leave several live helpers: two could both find a stale socket stale, and the later one unlinked the socket the earlier one had just bound. The helper now holds an exclusive, non-blocking flock on a root-owned PATH.lock for its lifetime, taken before the stale check and held through bind and chown; a second helper refuses to start. The kernel releases it however the helper exits. - A failed Lchown left a root-owned socket behind, because unlink on close was disabled before the chown. It is now disabled only once the chown succeeds, so a failed start, still running as root, removes it. - The socket's directories must now be searchable by --user or one of its groups, so the helper cannot run healthy behind a socket its client gets EACCES on. A root integration test now starts 20 helpers at once over a stale socket, 60 times, and requires exactly one to serve; with the lock disabled it fails within 25 iterations. It runs in the root CI job. Unit tests cover the lock, the failed-chown cleanup and the traversal check. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

astuyve · 2026-10-27T19:51:15Z

Manual QA of b6318365 through DataDog/datadog-agent#58809 (pinned to this commit), upgrading an Ubuntu 24.04 VM in place from Agent 7.85.0-rc.8:

Lock: the helper starts with --socket and holds an exclusive flock on privileged.sock.lock (root-owned, 0600; /proc/locks shows FLOCK WRITE for the helper's PID). The socket is dd-agent:dd-agent 0600.
Traversal check: it accepts the systemd RuntimeDirectory (root:dd-agent 0755). dd-agent can connect; nobody is denied.
Second helper: a second helper on the same path exits 1 with "another helper is serving …", and the original keeps serving.
Cleanup: stopping the helper makes systemd remove the socket, the lock file and the directory together, and a restart recreates them cleanly.
Restarts and reboot: the helper comes back on Agent restart and after a reboot (lock re-taken, no ordering cycle).

🤖 Generated with Claude Code

… test The root CI job builds the tests with -race, which flagged the cleanup loop reading each exec.Cmd's Process while the starter goroutines were still writing it in Start. Record started processes under a mutex. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

DataDog/rshell#700 merged as 6d758c7e; pin to it (v0.0.31-0.20261028124019-6d758c7edecf). The helper code is identical to the reviewed tip. Re-pin to the v0.0.31 tag once it is cut. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

DataDog/rshell#700 shipped in v0.0.31 (tag on 6d758c7e, the commit this branch was pinned to by pseudo-version). Switch to the release tag. The privileged helper code is unchanged; verified end to end against the staging Action Platform on this build. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

astuyve mentioned this pull request Oct 27, 2026

fix(rshell): run the privileged helper as one Agent-owned service gated on config DataDog/datadog-agent#58809

Open

astuyve marked this pull request as ready for review October 27, 2026 16:39

astuyve requested review from a team as code owners October 27, 2026 16:39

astuyve requested a review from san-jos October 27, 2026 16:39

matt-dz reviewed Oct 27, 2026

View reviewed changes

julesmcrt reviewed Oct 27, 2026

View reviewed changes

julesmcrt mentioned this pull request Oct 27, 2026

privileged helper: uname fails on RHEL 9 when /proc/sys/kernel/arch is absent #702

Closed

matt-dz mentioned this pull request Oct 27, 2026

fix(uname): tolerate missing /proc/sys/kernel/arch on pre-6.1 kernels (Fixes #702) #703

Merged

julesmcrt approved these changes Oct 28, 2026

View reviewed changes

astuyve added this pull request to the merge queue Oct 28, 2026

Merged via the queue into main with commit 6d758c7 Oct 28, 2026
49 checks passed

astuyve deleted the ajstuyvenberg/privileged-helper-socket branch October 28, 2026 12:41

julesmcrt mentioned this pull request Oct 28, 2026

fix(interp): bound script nesting depth to prevent stack-overflow crash (VULN-4562, VULN-1673, VULN-4535) #696

Merged

Conversation

astuyve commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Testing

Checklist

Uh oh!

matt-dz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matt-dz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astuyve commented Oct 27, 2026

Uh oh!

julesmcrt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astuyve commented Oct 27, 2026

Uh oh!

Uh oh!

astuyve commented Oct 27, 2026 •

edited

Loading