Skip to content

Commit 8a4b41e

Browse files
authored
logs: teach the file tailer/fingerprinter to honor NoFollow
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>
1 parent 6812f6d commit 8a4b41e

16 files changed

Lines changed: 317 additions & 19 deletions

File tree

comp/logs/agent/config/integration_config.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,12 @@ type LogsConfig struct {
147147
IntegrationSource string `mapstructure:"integration_source" json:"integration_source" yaml:"integration_source"`
148148
// IntegrationFileIndex is the index of the integration file that contains this source.
149149
IntegrationSourceIndex int `mapstructure:"integration_source_index" json:"integration_source_index" yaml:"integration_source_index"`
150+
151+
// NoFollow is true when this file source must not follow symlinks. Set by the AD
152+
// scheduler for sources discovered by the process_log provider, whose paths come from
153+
// /proc/<pid>/fd and are canonical at discovery time — any symlink found later
154+
// indicates an attacker-controlled swap. Never parsed from config.
155+
NoFollow bool `json:"-" yaml:"-" mapstructure:"-"`
150156
}
151157

152158
// SourceAutoMultiLineOptions defines per-source auto multi-line detection overrides.

pkg/logs/internal/util/opener/open_linux.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,12 @@ func OpenLogFile(path string) (*os.File, error) {
1717
return privilegedlogsclient.Open(path)
1818
}
1919

20+
// OpenLogFileNoFollow opens a file with the privileged logs client without
21+
// following symbolic links in any path component.
22+
func OpenLogFileNoFollow(path string) (*os.File, error) {
23+
return privilegedlogsclient.OpenNoFollow(path)
24+
}
25+
2026
// StatLogFile stats a log file with the privileged logs client
2127
func StatLogFile(path string) (os.FileInfo, error) {
2228
return privilegedlogsclient.Stat(path)

pkg/logs/internal/util/opener/open_other.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,22 @@ import (
1414
"github.com/DataDog/datadog-agent/pkg/util/filesystem"
1515
)
1616

17-
// OpenLogFile opens a file with filesystem.OpenShared
17+
// OpenLogFile opens a file with filesystem.OpenShared.
18+
// On non-Linux platforms we don't need to support symlink rejection since it's
19+
// only needed for process_log-discovered paths which are currently only
20+
// supported on Linux.
1821
func OpenLogFile(path string) (*os.File, error) {
1922
return filesystem.OpenShared(path)
2023
}
2124

25+
// OpenLogFileNoFollow falls back to a regular open on non-Linux platforms:
26+
// symlink rejection is only needed for process_log-discovered paths, and
27+
// process_log discovery (based on /proc/<pid>/fd) is Linux-only, so this path
28+
// is not reachable with an untrusted, attacker-controlled symlink swap here.
29+
func OpenLogFileNoFollow(path string) (*os.File, error) {
30+
return filesystem.OpenShared(path)
31+
}
32+
2233
// StatLogFile stats a log file
2334
func StatLogFile(path string) (os.FileInfo, error) {
2435
return os.Stat(path)

pkg/logs/launchers/container/tailerfactory/file.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,9 @@ func (tf *factory) makeDockerFileSource(source *sources.LogSource) (*sources.Log
122122
}
123123

124124
// check access to the file; if it is not readable, then returning an error will
125-
// try to fall back to reading from a socket.
125+
// try to fall back to reading from a socket. Container log paths (e.g.
126+
// /var/log/pods/…) are symlinks created by the container runtime intentionally,
127+
// so symlink-following is correct here.
126128
f, err := opener.OpenLogFile(path)
127129
if err != nil {
128130
// (this error already has the form 'open <path>: ..' so needs no further embellishment)
@@ -155,6 +157,7 @@ func (tf *factory) makeDockerFileSource(source *sources.LogSource) (*sources.Log
155157
AutoMultiLineSamples: source.Config.AutoMultiLineSamples,
156158
ExperimentalAdaptiveSampling: source.Config.ExperimentalAdaptiveSampling,
157159
ExperimentalNoisyLogDetection: source.Config.ExperimentalNoisyLogDetection,
160+
NoFollow: source.Config.NoFollow,
158161
})
159162

160163
// inform the file launcher that it should expect docker-formatted content
@@ -276,6 +279,7 @@ func (tf *factory) makeK8sFileSource(source *sources.LogSource) (*sources.LogSou
276279
AutoMultiLineSamples: source.Config.AutoMultiLineSamples,
277280
ExperimentalAdaptiveSampling: source.Config.ExperimentalAdaptiveSampling,
278281
ExperimentalNoisyLogDetection: source.Config.ExperimentalNoisyLogDetection,
282+
NoFollow: source.Config.NoFollow,
279283
})
280284

281285
switch source.Config.Type {

pkg/logs/launchers/file/BUILD.bazel

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ dd_agent_go_test(
6262
"//pkg/logs/tailers",
6363
"//pkg/logs/util/opener",
6464
"//pkg/logs/util/testutils",
65+
"@com_github_stretchr_testify//require",
6566
"@com_github_stretchr_testify//suite",
6667
],
6768
"@rules_go//go/platform:android": [
@@ -98,6 +99,7 @@ dd_agent_go_test(
9899
"//pkg/logs/tailers",
99100
"//pkg/logs/util/opener",
100101
"//pkg/logs/util/testutils",
102+
"@com_github_stretchr_testify//require",
101103
"@com_github_stretchr_testify//suite",
102104
],
103105
"@rules_go//go/platform:dragonfly": [
@@ -115,6 +117,7 @@ dd_agent_go_test(
115117
"//pkg/logs/tailers",
116118
"//pkg/logs/util/opener",
117119
"//pkg/logs/util/testutils",
120+
"@com_github_stretchr_testify//require",
118121
"@com_github_stretchr_testify//suite",
119122
],
120123
"@rules_go//go/platform:freebsd": [
@@ -132,6 +135,7 @@ dd_agent_go_test(
132135
"//pkg/logs/tailers",
133136
"//pkg/logs/util/opener",
134137
"//pkg/logs/util/testutils",
138+
"@com_github_stretchr_testify//require",
135139
"@com_github_stretchr_testify//suite",
136140
],
137141
"@rules_go//go/platform:illumos": [
@@ -149,6 +153,7 @@ dd_agent_go_test(
149153
"//pkg/logs/tailers",
150154
"//pkg/logs/util/opener",
151155
"//pkg/logs/util/testutils",
156+
"@com_github_stretchr_testify//require",
152157
"@com_github_stretchr_testify//suite",
153158
],
154159
"@rules_go//go/platform:ios": [
@@ -166,6 +171,7 @@ dd_agent_go_test(
166171
"//pkg/logs/tailers",
167172
"//pkg/logs/util/opener",
168173
"//pkg/logs/util/testutils",
174+
"@com_github_stretchr_testify//require",
169175
"@com_github_stretchr_testify//suite",
170176
],
171177
"@rules_go//go/platform:js": [
@@ -183,6 +189,7 @@ dd_agent_go_test(
183189
"//pkg/logs/tailers",
184190
"//pkg/logs/util/opener",
185191
"//pkg/logs/util/testutils",
192+
"@com_github_stretchr_testify//require",
186193
"@com_github_stretchr_testify//suite",
187194
],
188195
"@rules_go//go/platform:linux": [
@@ -219,6 +226,7 @@ dd_agent_go_test(
219226
"//pkg/logs/tailers",
220227
"//pkg/logs/util/opener",
221228
"//pkg/logs/util/testutils",
229+
"@com_github_stretchr_testify//require",
222230
"@com_github_stretchr_testify//suite",
223231
],
224232
"@rules_go//go/platform:openbsd": [
@@ -236,6 +244,7 @@ dd_agent_go_test(
236244
"//pkg/logs/tailers",
237245
"//pkg/logs/util/opener",
238246
"//pkg/logs/util/testutils",
247+
"@com_github_stretchr_testify//require",
239248
"@com_github_stretchr_testify//suite",
240249
],
241250
"@rules_go//go/platform:osx": [
@@ -253,6 +262,7 @@ dd_agent_go_test(
253262
"//pkg/logs/tailers",
254263
"//pkg/logs/util/opener",
255264
"//pkg/logs/util/testutils",
265+
"@com_github_stretchr_testify//require",
256266
"@com_github_stretchr_testify//suite",
257267
],
258268
"@rules_go//go/platform:plan9": [
@@ -270,6 +280,7 @@ dd_agent_go_test(
270280
"//pkg/logs/tailers",
271281
"//pkg/logs/util/opener",
272282
"//pkg/logs/util/testutils",
283+
"@com_github_stretchr_testify//require",
273284
"@com_github_stretchr_testify//suite",
274285
],
275286
"@rules_go//go/platform:qnx": [
@@ -287,6 +298,7 @@ dd_agent_go_test(
287298
"//pkg/logs/tailers",
288299
"//pkg/logs/util/opener",
289300
"//pkg/logs/util/testutils",
301+
"@com_github_stretchr_testify//require",
290302
"@com_github_stretchr_testify//suite",
291303
],
292304
"@rules_go//go/platform:solaris": [
@@ -304,6 +316,7 @@ dd_agent_go_test(
304316
"//pkg/logs/tailers",
305317
"//pkg/logs/util/opener",
306318
"//pkg/logs/util/testutils",
319+
"@com_github_stretchr_testify//require",
307320
"@com_github_stretchr_testify//suite",
308321
],
309322
"//conditions:default": [],

pkg/logs/launchers/file/launcher_privileged_logs_test.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,10 @@ func (s *PrivilegedLogsTestSetupStrategy) Setup(t *testing.T) TestSetupResult {
7878
return privilegedlogstest.WithParentPermFixup(t, name, func() error {
7979
return os.Remove(name)
8080
})
81+
}, symlink: func(oldname, newname string) error {
82+
return privilegedlogstest.WithParentPermFixup(t, newname, func() error {
83+
return os.Symlink(oldname, newname)
84+
})
8185
}}}
8286
}
8387

@@ -99,6 +103,17 @@ func TestPrivilegedLogsLauncherTestSuiteWithConfigID(t *testing.T) {
99103
suite.Run(t, s)
100104
}
101105

106+
// TestPrivilegedLogsLauncherNoFollowSymlink runs the NoFollow symlink policy
107+
// test (defined in launcher_test.go) against the privileged-logs path. The files
108+
// live in unsearchable directories, so the unprivileged open fails and the launcher
109+
// falls back to system-probe; the swapped symlink must be rejected there with
110+
// O_NOFOLLOW rather than followed.
111+
func TestPrivilegedLogsLauncherNoFollowSymlink(t *testing.T) {
112+
strategy := &PrivilegedLogsTestSetupStrategy{}
113+
res := strategy.Setup(t)
114+
runLauncherNoFollowSymlinkTest(t, res.TestOps, res.TestDirs[0])
115+
}
116+
102117
func TestPrivilegedLogsLauncherScanStartNewTailer(t *testing.T) {
103118
setup := setupPrivilegedLogsTest(t)
104119
runLauncherScanStartNewTailerTest(t, setup.tempDirs[:])

0 commit comments

Comments
 (0)