Repository navigation
Conversation
dd-octo-sts
Bot
added
internal
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI review by Codex (OpenAI) - workflow run
The patch is correct. I found no actionable regressions in the installer changes, systemd dependencies, socket migration, or fallback behavior. The new E2E tests appropriately exercise deployed permissions and service lifecycle boundaries. Tests were not run in the read-only environment.
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
19 successful checks with minimal change (< 2 KiB)
|
astuyve
changed the title
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: d6711c7 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_metrics_logs | memory utilization | +0.48 | [+0.26, +0.70] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_cpu | % cpu utilization | +0.20 | [-0.06, +0.46] | 1 | Logs |
| ➖ | python_openmetrics | % cpu utilization | +0.19 | [-0.47, +0.84] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.06 | [-0.02, +0.13] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.03 | [-0.01, +0.06] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | -0.04 | [-0.10, +0.02] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | -0.22 | [-0.26, -0.18] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.22 | [-0.35, -0.09] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | -0.25 | [-0.28, -0.22] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_memory | memory utilization | -0.35 | [-0.56, -0.14] | 1 | Logs |
| ➖ | dsd_uds_client_drop_detector_cpu | % cpu utilization | -1.22 | [-1.75, -0.68] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | -2.83 | [-3.73, -1.92] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | python_openmetrics | checks_execution_time | 10/10 | 79.59 ≤ 100 | bounds checks dashboard |
| ✅ | python_openmetrics | cpu_usage | 10/10 | 1360.06 ≤ 1500 | bounds checks dashboard |
| ✅ | python_openmetrics | memory_usage | 10/10 | 4.31GiB ≤ 4.75GiB | bounds checks dashboard |
| ✅ | quality_gate_idle | intake_connections | 10/10 | 4 ≤ 5 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 179.42MiB ≤ 181MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 781.40KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 2 ≤ 5 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 473.53MiB ≤ 542MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.16MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 16 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 216.63MiB ≤ 228MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.33MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 371.90 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 18 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 433.82MiB ≤ 455MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.95GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 75.12MiB ≤ 77MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 29.65 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 330.12MiB ≤ 357MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 42.70 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 308.24MiB ≤ 337MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 23.09 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 334.29MiB ≤ 348MiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
Go Package Import DifferencesBaseline: d6711c7
|
astuyve
added
the
qa/done
There was a problem hiding this comment.
AI review by Codex (OpenAI) - workflow run
The patch is correct. I found no actionable defects in the runtime gate, socket lifecycle changes, or fallback behavior. The new E2E tests appropriately exercise deployed packaging and systemd boundaries. Validation was limited to static review; tests were not run.
matt-dz
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
🤖 Review by Matthew's agent (Dogpark), on their behalf:
Review of the dd-agent-mcp files only: pkg/privateactionrunner/bundles/remoteaction/rshell/{BUILD.bazel,privileged_support_linux.go,privileged_support_other.go,run_command.go,run_command_test.go}. I didn't review the installer or systemd parts.
Verdict: looks good, a few nits. The runtime gate in Run only ever moves in the safe direction. When the helper is off, unsupported or has no socket, EscalationAllowed tasks run in the ordinary interpreter, and sudo is still rejected (the new test checks the 126). Once PAR has dialed, a failure is returned and the task is not retried locally. I don't see a way to escalate when the setting is disabled. The package tests pass locally (dda inv test --targets=./pkg/privateactionrunner/bundles/remoteaction/rshell).
Nits, most useful first:
- The fallback is easy to miss. The warning is logged once per handler for the life of the process, and the task output never says it ran without elevation. Here is how that bites. On an enabled host where the helper keeps crashing (for example a bad
policy.json), the socket reaches its trigger limit, andRemoveOnStop=yesdeletes the file. From then on every elevated task runs unprivileged andsudoexits 126. The operator gets nothing beyond a warning that may have been logged hours earlier. I'd suggest adding aSandboxWarningsentry on every fallback, so the backend and UI can show why elevation didn't happen. - The Landlock ABI is duplicated.
privilegedHelperMinLandlockABI = 3copiesrequireKernelSupportin rshell'sinternal/sandbox/landlock. rshell doesn't export it, so if the helper's minimum changes (or it is ever built withlandlocktsync, which needs ABI 8), PAR will dial helpers that accept the connection and then fail. Either export the constant from rshell, or add a comment that ties the two together. - The no-fallback test is weaker. It now only matches
"privileged rshell helper". Counting accepted connections in the test listener and asserting the count is at least 1 would show the task actually reached the helper and wasn't retried.
| // warnPrivilegedFallback logs the first fallback only, since every elevated | ||
| // task would otherwise repeat it. | ||
| func (h *RunCommandHandler) warnPrivilegedFallback(format string, args ...interface{}) { | ||
| if h.privilegedFallbackWarned.CompareAndSwap(false, true) { |
There was a problem hiding this comment.
🤖 Matthew's agent (Dogpark):
Nit: this logs once per handler for the life of the process. A host can go from "socket present" to "socket removed", for example when the helper fails repeatedly, the socket reaches its trigger limit and RemoveOnStop=yes deletes it. If an earlier fallback already used up the warning, that later change is never logged. Could each fallback also add an entry to the task's SandboxWarnings, such as "ran without elevation: "? Then the operator can see why sudo returned 126.
| h.warnPrivilegedFallback("this host cannot run the rshell privileged helper (%v); running without elevation", err) | ||
| return false | ||
| } | ||
| if _, err := os.Stat(h.privilegedSocket); errors.Is(err, os.ErrNotExist) { |
There was a problem hiding this comment.
🤖 Matthew's agent (Dogpark):
This is fine, and it fails closed. Any Stat error other than ErrNotExist, or a stale file nothing listens on, still dials and returns the error. It might be worth saying in the comment that a socket unit that failed at its trigger limit also lands here, because RemoveOnStop deletes the file, and the task then falls back silently instead of erroring.
|
|
||
| // privilegedHelperMinLandlockABI is the Landlock ABI the privileged helper | ||
| // sandbox requires. | ||
| const privilegedHelperMinLandlockABI = 3 |
There was a problem hiding this comment.
🤖 Matthew's agent (Dogpark):
This copies the version < 3 check in rshell's internal/sandbox/landlock.requireKernelSupport, and nothing keeps the two in sync. If rshell raises its minimum, or the helper is built with landlocktsync (which needs ABI 8), PAR will dial a helper that accepts the connection and then fails, and the task is not retried. Could rshell export the constant from privilegedhelper, or at least have a comment here pointing at the rshell code?
| out, err := action.handler.Run(context.Background(), task, nil) | ||
|
|
||
| require.ErrorContains(t, err, "privileged rshell helper: connect to privileged helper") | ||
| require.ErrorContains(t, err, "privileged rshell helper") |
There was a problem hiding this comment.
🤖 Matthew's agent (Dogpark):
Nit: this assertion is looser than before. Counting Accepts in the listener goroutine and asserting the count is at least 1 would prove the task reached the helper, which is the point of the test. It would also catch a future change that returns a different privileged rshell helper error before dialing.
astuyve
changed the title
|
Moving this back to draft. We're going to move socket creation into the rshell privileged helper itself, to clean up the implementation: That needs an rshell change first. Until it's released, this branch pins rshell to the PR's commit for testing. We'll update this PR when the rshell PR lands. 🤖 Generated with Claude Code |
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>
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>
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>
641df17 ("root-own the files root executes on Linux installs") makes /opt/datadog-agent root:root so dd-agent cannot replace binaries root runs, but the installer e2e assertions still expected dd-agent. They failed in TestPackageAgentFapolicyd and would in every TestPackages/agent_*/TestInstall. processes.d stays dd-agent. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Move the testing pin to DataDog/rshell#700's b6318365 (v0.0.31-0.20261027191057-b63183659b81): the helper now holds a lock on privileged.sock.lock for its lifetime so only one serves the socket, removes a socket whose chown failed, and requires dd-agent to be able to reach it. The lock file lives in the unit's RuntimeDirectory, which systemd removes on stop. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I completed package-level QA on Ubuntu 24.04 x86_64 and arm64, including direct DEB downgrade/upgrade and Fleet experiment rollback/promotion. I found two lifecycle issues:
The unit uses PartOf=datadog-agent.service. After repeatedly killing the Agent until datadog-agent.service exhausted its restart limit and entered failed, datadog-agent-rshell-privileged.service remained active and /run/datadog-agent-rshell/privileged.sock remained reachable. I Using BindsTo=datadog-agent.service instead stopped the helper and removed its RuntimeDirectory when the Agent failed. systemd-analyze verify reported no ordering cycle.
I started from official Agent 7.84.6 with the legacy socket active, installed this PR’s package as an experiment, tested rollback, then reinstalled and promoted it. Promotion correctly removed the old socket and enablement links and started the new helper, but removeLegacyPrivilegedRshellSocket removes unit files only from unitsPath(ctx). During OCI promotion that is /etc/systemd/system, so it does not remove a unit installed by the previous DEB under /lib/systemd/system (which resolves to /usr/lib/systemd/system on Ubuntu). Could it Everything else I exercised passed: x86_64 and arm64 package installation, config gating, environment overrides, permissions, helper UIDs, concurrent startup, stale/live socket handling, reboot/systemd behavior, direct DEB downgrade/upgrade, experiment rollback, and experiment |
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>
|
End-to-end verification on the staging Action Platform (org 2 / datad0g.com), on a VM running this branch's build with rshell
The VM, the execution policy, and the keys were torn down after the run. Now pinned to the 🤖 Generated with Claude Code |
[P1] Do not fall back to the ordinary interpreter when the enabled helper socket is absent
I reproduced this with a valid signed
This can happen during helper startup/restart/cleanup races or after an unexpected helper failure. It does not require a forged task; it only requires a valid task whose backend policy is broader than the host’s local helper policy. Please fail the task explicitly when privileged execution is enabled and an ReproductionPrerequisites:
|
…r after the Agent Addresses review findings on #58809: - [P1] When privileged mode is enabled, an EscalationAllowed task whose helper socket is unreachable now fails instead of falling back to the ordinary interpreter. The local helper policy gates elevation and the ordinary path does not apply it, so falling back let a backend grant broader than the local policy through (e.g. a :rw task under a :ro local policy) during a socket race or helper failure. Removed privilegedHelperAvailable; enabled => always route to the helper. - The helper is now BindsTo= (not PartOf=) its Agent, so it stops and its RuntimeDirectory is removed when the Agent fails (restart limit exhausted), not only on a clean stop. - Fix the ordering direction: the Agent no longer orders itself After the helper (an inverted leftover from socket activation); the helper orders After the Agent it is bound to. Pairs correctly with BindsTo. - removeLegacyPrivilegedRshellSocket now removes the legacy socket unit from every unit install path, so a DEB-to-OCI promotion no longer leaves the old unit under /usr/lib/systemd/system. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @julesmcrt — thorough QA, all three fixed in [P1] Missing socket now fails closed. Helper now BindsTo= the Agent. Swapped Ordering fixed (caught in parallel). The Agent was ordered DEB-to-OCI promotion. 🤖 Generated with Claude Code |
The rshell v0.0.31 bump pulls in a new subpackage, github.com/DataDog/rshell/builtins/internal/boundedreplace, which lint_licenses flagged as missing. Regenerated with `dda inv generate-licenses`. Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cy socket cleanup Addresses review feedback on the privileged rshell helper units: - Unit wiring parity: the stable and experiment helper units now mirror datadog-agent-action's wiring. Each BindsTo its matching core Agent, orders After both core Agents, and Conflicts with the opposite core Agent and helper, so stable and experiment never run the helper at the same time against the shared socket. - Consolidate permissions: the privateactionrunner launcher and the rshell helper binary are now root:root entries in agentPackagePermissions, replacing the separate permissions table and the bespoke ensure step. systemd runs both as root, so dd-agent must not be able to replace them. - Finish legacy socket retirement: removeLegacyPrivilegedRshellSocket now stops and disables the old socket units (guarded by systemd.IsRunning) before deleting their files, mirroring retireLegacyProcmgrUnits, so a stale still-loaded socket on an upgraded host is cleared rather than left active in systemd. - e2e: cover the failure-containment transition. A helper killed until it exhausts its restart limit enters failed, drops its socket, and leaves the core Agent running (BindsTo binds the helper to the Agent, not the reverse); restarting the Agent clears it and brings it back. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks — addressed all four in fc2c77d:
gofmt/linter clean on both modules, installer + template unit tests green. |
…g it in e2e The failure-containment case in TestPrivilegedRshellHelperLifecycle drives the helper to `failed` by exhausting its systemd start limit, then expected a plain `systemctl restart datadog-agent` to bring it back. It does not: a unit that hit its start limit stays failed, and the Agent's Wants= does not reset that counter. On Ubuntu 24.04 (the only matrix distro with Landlock support, so the only one that runs this block) requireRshellHelperSupported then timed out with "the enabled helper neither started nor declined the kernel". Clear the helper's start-limit counter with `systemctl reset-failed` before restarting the Agent, and fix the comment that wrongly claimed Wants= resets it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Follow-ups:
Have the previously reproduced Agent-failure and DEB→OCI cleanup cases been rerun against the final build? If so, please link the results. |
|
Potential security regression in the permission consolidation: the removed If its target is in a dd-agent-writable directory, making the target root-owned does not prevent dd-agent from replacing it afterward. The root launcher then executes whatever that helper path resolves to. Please preserve the regular-file validation for rshell and restore the deleted symlink-rejection test. The root-executed privateactionrunner launcher should receive equivalent validation. This can coexist with the consolidated permission list; no need to restore the separate permission table. I confirmed the changed acceptance behavior with a focused Linux test against |
The fixes note said restricted shell tasks "run without elevation instead of failing" when the helper is unavailable, which reads as a blanket fallback. With privileged mode enabled, EscalationAllowed tasks fail closed if the helper is unreachable (to preserve its host-local policy); only ordinary, non-elevated commands keep running. Spell both out, plus the privileged-mode-disabled case. Reported by Jules in review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ar file The permission consolidation removed ensurePrivilegedRshellPermissions, whose Lstat+IsRegular check rejected a symlinked rshell. agentPackagePermissions.Ensure instead resolves symlinks with filepath.EvalSymlinks before Lchown, so it would follow a symlinked binary and chown its target to root rather than reject it. If that target sat in a dd-agent-writable directory, dd-agent could replace it afterward and the root launcher would exec whatever the path resolved to. Restore the validation in two places, keeping the consolidated ownership list: - The privateactionrunner launcher, which runs as root, now refuses to exec a helper that is not a regular file, is not root-owned, or is writable by group or other, before execve. This is the runtime gate and catches a tampered binary regardless of how it got there. - The installer rejects a symlinked (or non-regular) rshell and privateactionrunner before applying ownership, restoring the removed install-time check for both root-executed binaries. Tests: restored the installer symlink-rejection test over both binaries, added launcher unit tests (symlink, non-root owner, group/other-writable), and an e2e assertion that the service refuses to start a symlinked helper binary. Also clarify the release note: with privileged mode enabled, EscalationAllowed tasks fail closed when the helper is unreachable (only ordinary commands keep running), rather than the blanket "run without elevation instead of failing". Both reported by Jules in review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1. The root launcher inherits a dd-agent-writable environmentThe new helper unit imports /etc/datadog-agent/environment before starting privateactionrunner as root. The installer makes /etc/datadog-agent writable by dd-agent, so that account can create or replace the environment file. I checked the Omnibus package from this PR’s earlier commit a5dcb66, pipeline 147972088: privateactionrunner is dynamically linked, and the relevant build recipes are unchanged at the current head. On that packaging path, an LD_PRELOAD entry can load code as root when the service Could we remove the shared EnvironmentFile= input from this root unit, or preserve the required overrides through a root-controlled or narrowly filtered mechanism? Changing only the environment file’s ownership would not suffice while its parent directory remains writable by 2. The shared Lchown change can break symlinked configuration updatesThis is distinct from the removed regular-file check for executables. The Fleet config writer in pkg/fleet/installer/config/config_nix.go calls file.Chown directly, without going through Permission.Ensure. It still chmods a symlink’s target, but changing file.Chown to use Lchown For example, given a relative link datadog.yaml -> real-datadog.yaml whose target is root:root 0644, a Fleet config patch changes the target’s mode to 0640 while leaving its ownership as root:root. Without an additional ACL, the experiment Agent running as dd-agent can no longer Could we preserve target-ownership updates in the config writer, preferably through the confined opened file, while keeping installer permission walks non-following? A regression test should check the target’s ownership and readability, not just its mode. Validation was source/caller review plus inspection of the earlier PR package. I have not run a live exploit or reproduced the config-update failure end to end. |
…helper The privileged helper unit runs privateactionrunner as root and sourced EnvironmentFile=-/etc/datadog-agent/environment. That directory is dd-agent:dd-agent, so dd-agent can replace the environment file (it owns the directory even though the file itself is root-owned) and inject variables such as LD_PRELOAD into the root launcher. The dynamic loader acts before main(), so this runs attacker code as root before the config, Landlock, and helper-binary checks run - a dd-agent to root escalation, reproduced on a staging VM. Remove EnvironmentFile= from the helper unit. The launcher needs nothing from it; the only variable it receives, DD_FLEET_POLICIES_DIR, stays a root-controlled literal baked into the unit. Invert the template test to assert the unit has no EnvironmentFile directive. Reported by Jules in review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| } | ||
| } | ||
| if p.Mode != 0 { | ||
| // Skip symlinks: there is no portable lchmod and os.Chmod follows them. |
There was a problem hiding this comment.
based on this feedback from jules #58809 (comment)
we're trying to ensure the binary can't be swapped by a rogue dd-agent process and then started
|
|
||
| // Chown changes the ownership of a file to the specified owner and group. | ||
| // Chown changes the ownership of a file to the specified owner and group. It | ||
| // does not follow symlinks. |
There was a problem hiding this comment.
Why ? Not sure how safe this is, there could be symlinks somewhere we use this function no ?
| // enablement symlinks left by older versions. It only runs when installing or | ||
| // promoting the stable package: during an experiment, the previous stable | ||
| // version may still rely on them to start its helper after a rollback. | ||
| func removeLegacyPrivilegedRshellSocket(ctx HookContext) { |
There was a problem hiding this comment.
Do we need that migration if it was never shipped ?
There was a problem hiding this comment.
yeah it did ship in 7.84 although it likely has very very very few installs
…ged helper dd-agent is a trusted, elevated user that can chown the install directory and binaries at any time, so hardening the installer and launcher against a hostile dd-agent adds complexity without a real security boundary. Back it out and keep only creating the items: - Launcher: remove the pre-exec regular-file / root-owner / not-writable check; it execs the configured helper directly again. - Installer: remove ensureRootExecutablesAreRegularFiles, and drop the root-ownership pinning of embedded/bin/rshell and embedded/bin/privateactionrunner. - Unit: restore EnvironmentFile=-<etc>/environment on the helper unit. - Tests: drop the launcher trust tests, the installer symlink-rejection tests, and the e2e symlinked-helper assertion. Kept intact: the legacy socket cleanup and the general root-trust-boundary (VULN-4673), which are unrelated to this helper's trust model. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What does this PR do?
Reworks how the Linux installer ships the privileged rshell helper:
datadog-agent-rshell-privileged.serviceis always written.datadog-agent.servicepulls it in withWants=, and it stops with the Agent throughPartOf=. There's no.socketunit, no enablement check unit, and no[Install]section, so nothing gets enabled and no symlinks have to carry across upgrades. The installer no longer parsesdatadog.yaml.privateactionrunner privileged-helperas root, which loads the Agent config without the secret backend. Ifprivate_action_runner.restricted_shell.privileged.enabledis false, or the kernel lacks Landlock ABI ≥ 3, it exits 0 without creating anything. Otherwise itexecsrshell privileged-helper --socket=/run/datadog-agent-rshell/privileged.sock --idle-timeout=0. The helper creates the socket itself (dd-agent,0600, in a root-ownedRuntimeDirectory) once its policy and account are loaded, removes it when it exits, and keeps the service's PID, so systemd supervises it directly.--socket.go.modpins rshell to that PR's commit for testing. It moves to the release once [tagger] cleanup pkg/tagger public api #700 lands, and this PR stays in draft until then.privateactionrunnerbecomes root-owned, likershell, since systemd runs it as root. A stable install or promote removes the legacy socket unit files and enablement symlinks that older versions left.EscalationAllowedtasks run in the ordinary interpreter withsudorejected. A helper that accepts the connection and then fails is still not retried locally.Motivation
The installer decided at install time and PAR decided at runtime, and the two could disagree:
Separately, the socket was bound to
datadog-agent-action.service, so it stopped whenever the monolithic runner exited (split mode, or PAR disabled). It also kept the implicitBefore=sockets.targetwhile being ordered after the Agent, which created a boot ordering cycle.This replaces closed #58241 and the systemd changes in #58769.
Tradeoffs:
dd-agentas its effective user, keeping root only as its real user ID./etc/datadog-agent-rshell/policy.jsonwhen it starts, so a change takes effect at the next helper or Agent restart.Note for the author of #57345: an enabled helper whose socket doesn't exist now falls back instead of erroring.
TestEnabledPrivilegedHelperFailureDoesNotFallBacknow tests a socket that accepts the connection and then fails. Please confirm that's acceptable.Describe how you validated your changes
exec.TestPackages/agent_*:TestPrivilegedRshellHelperLifecycle: the helper exits while disabled; enabling and restarting starts it asrshell(real uid 0, effective uiddd-agent) with its socket; it survivesdatadog-agent-actionstopping and stops with the Agent.TestPrivilegedRshellSurvivesReboot(Ubuntu 24.04): no ordering cycle, and the helper is serving after a reboot.TestPrivilegedRshellEndToEndruns signed elevated tasks through the long-running helper.a5dcb66, pipeline 147972088, with rshell pinned to feat(privileged-helper): create and serve its own socket with --socket rshell#700 atb6318365, after both reviews and the runtime QA fixes.systemd-analyze verifyreported the boot cycle, and after a reboot systemd deleted the socket's start job..socketunit and its symlinks were removed. The package root,embedded/bin,privateactionrunner,rshellandinstallerareroot:root 755.rshell privileged-helper --socket=… --idle-timeout=0(real uid 0, effective uiddd-agent), with no shell in between, and it survives PAR exiting. The helper's directory checks accept/run/datadog-agent-rshell. The socket isdd-agent:dd-agent 600;dd-agentcan connect andnobodyis denied. The helper holds an exclusiveflockonprivileged.sock.lock(root-owned,0600) for its lifetime. A second helper refuses to start ("another helper is serving"), and the original keeps serving. Stopping the helper makes systemd remove the socket, the lock file and the directory.🤖 Generated with Claude Code