fix(logs): disable symlink following for process_log-discovered paths - #51746
fix(logs): disable symlink following for process_log-discovered paths#51746vitkyrka wants to merge 4 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
🎯 Code Coverage (details) 🔗 Commit SHA: ce83f4a | Docs | Datadog PR Page | Give us feedback! |
Files inventory check summaryFile checks results against ancestor 83b0223d: Results for datadog-agent_7.83.0~devel.git.492.ce83f4a.pipeline.128204586-1_amd64.deb:No change detected |
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 83b0223 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_idle | memory utilization | +0.45 | [+0.40, +0.50] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | +0.38 | [+0.26, +0.50] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.19 | [-0.06, +0.44] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.13 | [+0.10, +0.17] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | +0.01 | [-0.07, +0.10] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_logs | % cpu utilization | +0.00 | [-1.00, +1.00] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.01 | [-0.04, +0.03] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | -0.25 | [-0.30, -0.19] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 150.64MiB ≤ 154MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 733.92KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 491.22MiB ≤ 512MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.12MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 186.47MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.17MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 386.83 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 413.86MiB ≤ 430MiB | 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.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 72.37MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 29.56 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 300.73MiB ≤ 330MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 62.78 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 280.12MiB ≤ 310MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 20.59 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 282.99MiB ≤ 320MiB | 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".
Replicate Execution Details
We run multiple replicates for each experiment/variant. However, we allow replicates to be automatically retried if there are any failures, up to 8 times, at which point the replicate is marked dead and we are unable to run analysis for the entire experiment. We call each of these attempts at running replicates a replicate execution. This section lists all replicate executions that failed due to the target crashing or being oom killed.
Note: In the below tables we bucket failures by experiment, variant, and failure type. For each of these buckets we list out the replicate indexes that failed with an annotation signifying how many times said replicate failed with the given failure mode. In the below example the baseline variant of the experiment named experiment_with_failures had two replicates that failed by oom kills. Replicate 0, which failed 8 executions, and replicate 1 which failed 6 executions, all with the same failure mode.
| Experiment | Variant | Replicates | Failure | Logs | Debug Dashboard |
|---|---|---|---|---|---|
| experiment_with_failures | baseline | 0 (x8) 1 (x6) | Oom killed | Debug Dashboard |
The debug dashboard links will take you to a debugging dashboard specifically designed to investigate replicate execution failures.
❌ Retried Profiling Replicate Execution Failures (ddprof)
Note: Profiling replicas may still be executing. See the debug dashboard for up to date status.
| Experiment | Variant | Replicates | Failure | Debug Dashboard |
|---|---|---|---|---|
| quality_gate_idle | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle_all_features | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle_all_features | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_logs | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_security_idle | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_security_idle | comparison | 10 | Crashed (exit code: 134) | Debug Dashboard |
| quality_gate_security_no_fs_load | comparison | 10 | Crashed (exit code: 134) | Debug Dashboard |
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_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_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 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 intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_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_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 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 memory_usage: 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_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
This pull request has been automatically marked as stale because it has not had activity in the past 15 days. It will be closed in 30 days if no further activity occurs. If this pull request is still relevant, adding a comment or pushing new commits will keep it open. Also, you can always reopen the pull request if you missed the window. Thank you for your contributions! |
5546c45 to
961c27d
Compare
|
This pull request has been automatically marked as stale because it has not had activity in the past 15 days. It will be closed in 30 days if no further activity occurs. If this pull request is still relevant, adding a comment or pushing new commits will keep it open. Also, you can always reopen the pull request if you missed the window. Thank you for your contributions! |
Co-authored-by: Vincent Whitchurch <vincent.whitchurch@datadoghq.com>
961c27d to
1687931
Compare
…n-linux build - Regenerate BUILD.bazel files with gazelle (dd_agent_go_test macro for pkg/privileged-logs/common, missing testify/require dep for pkg/logs/launchers/file tests). - OpenPathWithoutSymlinks: open directory components with O_PATH instead of O_RDONLY. O_RDONLY|O_DIRECTORY requires read permission on every directory component, which is stricter than the search (execute) permission a plain os.Open(path) needs, and would reject process_log-discovered files sitting under directories that are traversable but not listable (e.g. mode 0711). Found by Codex review. - pkg/privileged-logs/client/open_other.go: fix build break on all non-Linux platforms (windows, darwin, aix) — os.ErrUnsupported does not exist, only errors.ErrUnsupported does. Environment: Datadog workspace Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
checkFileReadable (comp/core/autodiscovery/providers/process_log.go) calls
OpenNoFollow unconditionally, regardless of platform. The non-Linux stubs
previously returned an "unsupported" error unconditionally, which broke
process_log file-readability verification entirely on macOS/Windows (caught by
bazel:test:macos-*:{base,dogstatsd} failing
TestProcessLogProviderFileReadabilityVerification).
Symlink rejection is only meaningful for process_log-discovered paths, and
process_log discovery is based on /proc/<pid>/fd, which only exists on Linux.
So on other platforms this code path is never reached with an untrusted,
attacker-controlled symlink swap, and falling back to a plain open (as the
non-no-follow variants already do) is safe and matches the existing comment
in pkg/logs/internal/util/opener/open_other.go.
Environment: Datadog workspace
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
13 successful checks with minimal change (< 2 KiB)
|
Move the O_NOFOLLOW directory-walk helper out of pkg/privileged-logs/module (the system-probe-side privileged logs module) and into pkg/privileged-logs/common, exporting it as OpenPathWithoutSymlinks. Pure move/rename, no behavior change: this is a preparatory refactor for a follow-up change that needs the same helper on the (unprivileged) agent-side privileged-logs client too. Part of a stack towards DSCVR-475 (disable symlink following for process_log-discovered paths); split out of PR #51746 for easier review. Environment: Datadog workspace Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Vincent Whitchurch <vincent.whitchurch@datadoghq.com>
Add end-to-end support for requesting a symlink-rejecting ("no-follow") open
through the privileged-logs client/module RPC:
- common.OpenFileRequest gets a NoFollow bool field.
- client.Open/OpenPrivileged gain OpenNoFollow/OpenPrivilegedNoFollow
counterparts. On Linux, OpenNoFollow calls common.OpenPathWithoutSymlinks
directly instead of os.Open; OpenPrivilegedNoFollow sets NoFollow on the
wire request. On non-Linux, OpenNoFollow falls back to a plain open (no
error) since symlink rejection is only meaningful for paths discovered via
/proc/<pid>/fd, which is Linux-only.
- module.handler dispatches to a new validateAndOpenNoFollow when the
request's NoFollow flag is set, which skips filepath.EvalSymlinks (the
caller guarantees the path is already canonical) and goes straight to
common.OpenPathWithoutSymlinks.
Also fixes a permission bug in common.OpenPathWithoutSymlinks, found via
Codex review: directory components were opened with O_RDONLY|O_DIRECTORY,
which requires *read* permission on every directory component. That's
stricter than the *search* (execute) permission a plain os.Open(path) needs,
and would incorrectly reject files sitting under directories that are
traversable but not listable (e.g. mode 0711). The existing module-side
caller (root-running system-probe) never hit this, since root bypasses the
extra permission check - it only becomes reachable with this change's new
unprivileged client-side caller. Switched to O_PATH for directory-component
opens, which only requires search permission, matching os.Open's semantics.
Note: a regression test for the permission bug specifically would need a
non-owner UID (root-only, or os/user + a helper process), which isn't added
here - flagging as a possible follow-up rather than skipping silently.
No caller uses OpenNoFollow/OpenPrivilegedNoFollow yet - checkFileReadable
and the file tailer still call the plain Open/OpenLogFile in this PR, so the
symlink-swap protection isn't active end-to-end here. That's intentional:
this PR is scoped to the transport capability itself; the next two PRs in
the stack add the LogsConfig.NoFollow field + tailer wiring, then flip
process_log discovery/tailing over to the no-follow calls.
Part of a stack towards DSCVR-475; split out of PR #51746 for easier review.
Depends on the "extract OpenPathWithoutSymlinks into common" refactor.
Environment: Datadog workspace
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Vincent Whitchurch <vincent.whitchurch@datadoghq.com>
Add a NoFollow bool field to LogsConfig (never parsed from user config -
mapstructure/yaml/json "-") and wire it through every place that opens a log
file for tailing:
- FileOpener interface (pkg/logs/util/opener) gains OpenLogFileNoFollow,
implemented on Linux via privilegedlogsclient.OpenNoFollow and falling back
to a plain open on other platforms (symlink rejection is only meaningful
for /proc/<pid>/fd-derived paths, which are Linux-only).
- The file tailer (pkg/logs/tailers/file/tailer.go) routes every open site
(initial open, rotation re-open) through a new openLogFile() helper that
picks the no-follow variant based on Source.Config().NoFollow.
ReplaceSource refreshes this from the new source.
- The fingerprinter (fingerprint.go) does the same for its own open site.
- pkg/logs/launchers/container/tailerfactory/file.go propagates
source.Config.NoFollow into the docker/k8s FileSource constructors. This is
needed here (not deferred to a later change) because
TestLogsConfigFieldCoverage in that package's test file asserts every
LogsConfig field is either copied or explicitly excluded - it fails the
moment NoFollow exists as a field without being handled.
Nothing sets NoFollow=true yet in this PR - this is generic plumbing,
exercised only by tests that construct LogsConfig{NoFollow: true} explicitly.
The field is deliberately unreachable from real config parsing
(mapstructure/yaml/json "-"). The next (and final) PR in this stack sets
cfg.NoFollow = true in the AD scheduler specifically for names.ProcessLog
sources and switches process_log's file-readability check over to the
no-follow client call - that's what actually activates the protection this
PR builds the mechanism for.
Part of a stack towards DSCVR-475; split out of PR #51746 for easier review.
Depends on the privileged-logs NoFollow transport PR.
Environment: Datadog workspace
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Vincent Whitchurch <vincent.whitchurch@datadoghq.com>
Turn on the no-follow enforcement plumbed through in the previous stack entries, for the one provider it's meant for: - pkg/logs/schedulers/ad/scheduler.go sets cfg.NoFollow = true for sources whose config.Provider is names.ProcessLog. process_log paths come from readlink(/proc/<pid>/fd/<n>); the kernel resolves all symlinks at file-open time, so the string in /proc/fd is already canonical - any symlink appearing later at that path was planted after discovery and indicates an attacker-controlled swap. Other providers (file, kubernetes, ...) are unaffected: their paths are explicitly specified by the user, and it's up to the user to ensure such a path isn't swapped for a symlink by an untrusted party. - comp/core/autodiscovery/providers/process_log.go's checkFileReadable now opens with privilegedlogsclient.OpenNoFollow instead of Open, to match what the tailer does when it actually reads the discovered file. Closes DSCVR-475. Final PR in the stack split out of PR #51746 for easier review; depends on the tailer/fingerprinter NoFollow-plumbing PR. Environment: Datadog workspace Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Vincent Whitchurch <vincent.whitchurch@datadoghq.com>
|
This pull request has been automatically marked as stale because it has not had activity in the past 15 days. It will be closed in 30 days if no further activity occurs. If this pull request is still relevant, adding a comment or pushing new commits will keep it open. Also, you can always reopen the pull request if you missed the window. Thank you for your contributions! |
What does this PR do?
Hardens the log tailer against symlink-following for log paths discovered via the
process_logautodiscovery provider.NoFollow boolfield toLogsConfig(comp/logs/agent/config/integration_config.go), set by the AD scheduler whenever a source's provider isprocess_log(pkg/logs/schedulers/ad/scheduler.go).OpenLogFileNoFollow/OpenNoFollow/OpenPrivilegedNoFollowvariants across the file-opener stack (pkg/logs/internal/util/opener,pkg/logs/util/opener,pkg/privileged-logs/client), all backed by a newcommon.OpenPathWithoutSymlinkshelper (pkg/privileged-logs/common/open_linux.go) that walks each path component withO_NOFOLLOW. This helper is shared by both the privileged-logs client (agent side) and the privileged-logs module (system-probe side), so the no-follow behavior is enforced whether or not privileged log collection is used.pkg/logs/tailers/file/tailer.go) and fingerprinter (pkg/logs/tailers/file/fingerprint.go) now route through anoFollow-aware open on every open site (initial open, rotation re-open, fingerprinting), driven bySource.Config().NoFollow.Tailer.ReplaceSourcerefreshes this flag when a source is replaced.NoFollowis propagated to container/k8s-derived sources inpkg/logs/launchers/container/tailerfactory/file.go. Container/pod log paths (e.g./var/log/pods/…) are intentionally symlinked by the runtime, so those sources continue to follow symlinks as before — onlyprocess_log-originated sources getNoFollow.checkFileReadablein theprocess_logprovider (comp/core/autodiscovery/providers/process_log.go) uses the no-follow open when validating a discovered path is readable.Other log sources (
file,kubernetes, etc.) are unaffected: their paths are explicitly specified by the user, and it's up to the user to ensure such a path isn't swapped for a symlink by an untrusted party.Motivation
Paths discovered via
process_logcome fromreadlink(/proc/<pid>/fd/<n>). The kernel resolves all symlinks at the time the target process opened that fd, so the string returned is already canonical — if a symlink appears at that path later, it was planted after discovery by something other than the process that owns the fd. Since the agent otherwise follows symlinks when tailing, an unprivileged attacker able to write to the resolved path could swap it for a symlink and get the agent to tail an arbitrarydd-agent-readable file (or a root-readable one, if privileged log collection is enabled).Rejecting symlinks for this specific, narrow case (
process_log-discovered paths only) closes that gap without changing behavior for any other log collection method that legitimately relies on symlinks.Tracked internally in DSCVR-475.
Describe how you validated your changes
Added unit tests covering both the policy and the low-level enforcement:
TestNoFollowFlagSetOnConfig(pkg/logs/schedulers/ad/scheduler_test.go): sources fromprocess_loggetNoFollow=true; sources fromfile/kubernetesdo not.TestReplaceSourceRefreshesNoFollow(pkg/logs/tailers/file/tailer_test.go):ReplaceSourceupdates the tailer's no-follow state.TestOpenPathWithoutSymlinks*(pkg/privileged-logs/common/open_linux_test.go): no symlink present, symlink as the final component, symlink in a parent directory, and the swap scenario (open succeeds, file removed and replaced with a symlink, second open is rejected withELOOP).TestLauncherNoFollowSymlink/TestPrivilegedLogsLauncherNoFollowSymlink(pkg/logs/launchers/file/launcher_test.go,launcher_privileged_logs_test.go): end-to-end launcher coverage for both the unprivileged and system-probe-backed (privileged logs) open paths.Additional Notes
names.ProcessLog(the autodiscovery provider name constant) is unchanged — it identifies the provider, not the config flag.