[WIP] Add an OS abstraction layer for filesystem and subprocess access - #24772
[WIP] Add an OS abstraction layer for filesystem and subprocess access#24772NouemanKHAL wants to merge 20 commits into
Conversation
Integrations reach open/os/shutil/glob/subprocess at scattered call sites with paths that can come from configuration, so there is no central place to apply the trusted-provider decision at the moment a path is used. Add an OSInterface that mediates those operations. Each method is a thin passthrough preceded by a validator hook, so with the default no-op validator behavior is byte-identical: same exception types and timing, permission bits, encodings and laziness. Enforcement is gated by its own path_enforcement_mode setting rather than the existing field-validation flag. Reusing that flag would mean any operator who already enabled it starts enforcing at every migrated call site the moment this ships, with no gradual path. The log mode reports what would be denied without blocking, so a fleet can be assessed first. Executable validation covers every program a command will launch, not just argv[0]: several checks wrap a config-derived binary in sudo, and under shell=True the program that launches is the shell. Bare command names are resolved through PATH, since that is what the OS runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erface Replace direct open/os/shutil/glob/subprocess calls with the mediated interface so config-derived paths are validated at the point of use. Where a module-level helper performs the access, it now takes the interface as a required parameter and is called with self.os_interface. The module-level singleton is bound to the no-op validator, so passing it there would preserve parity and pass every test while enforcing nothing. Paths handed to libraries that open them themselves (ssl in esxi, fdb in foundationdb, duckdb) are validated and resolved at the handoff, which is the last point the check controls. Test changes are limited to mock-target updates where a test patched a module-level import that moved. No assertions changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two regressions are cheap to introduce and invisible in tests: a raw open()/subprocess call reintroduces unmediated I/O, and reaching for the module-level singleton inside a check module enforces nothing while looking like coverage. Detect both with an AST-based check, so a method named open and mentions inside docstrings are not misreported. Narrow legitimate cases are waived with an inline SKIP_OS_INTERFACE_VALIDATION comment, which forces the decision to be written down rather than made by accident. php_fpm is excluded because it ships vendored third-party sources. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The developer guide covers which of the two bindings to use, since choosing the wrong one silently disables enforcement, plus the enforcement modes, the test fixture, and the explicit limitations. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Choosing the non-enforcing binding is the failure mode most likely to recur, and it is invisible in tests, so the rule belongs where every contributor and agent reads it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
METHOD_NAMES did not list glob, so the mock_os_interface fixture left it pointing at the real filesystem. A test using the fixture would have read the actual disk without any sign that it had. Back glob with the in-memory filesystem and assert METHOD_NAMES equals the real interface surface, so a method added to OSInterface cannot silently escape the fixture again. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tls_ca_cert, tls_cert and tls_private_key are config-derived paths handed to ssl, which opens them itself. This is the path most integrations reach TLS through, so validating here covers many at once. Also covers the remaining direct handoffs in http_check, esxi, foundationdb and duckdb. Add validate_path for this purpose. resolve_path was the wrong tool: it normalizes, so a relative path such as 'foo' reached ssl as an absolute one, changing observable behavior and breaking the parity requirement. validate_path validates and returns the value unchanged. Existing tls tests asserting the exact argument caught this. refresh_tls_context rebuilt the context without the interface, so a refresh silently skipped validation. It now threads it through. vsphere has the same exposure but VSphereAPI has no check reference and its constructor is used across many tests, so it is left for a follow-up rather than threaded here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Checks run on a schedule and perform many file operations per run, so a single misconfigured mode or one disallowed path produced a log line every time. That turns one configuration mistake into a flood and makes the dry-run mode unusable for assessing a fleet. Report each unknown mode and each distinct violation once per validator, which is once per check. The diagnostic value is knowing which paths would be denied, not how often they are touched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The validator lives as long as its check, so remembering every distinct disallowed path would grow without bound in a long-running Agent. Cap the set and log once when the cap is reached, rather than going quiet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
.ddev/config.toml pins an explicit list of validations for `validate all`, so registering the command in the orchestrator was not enough: CI would never have run it, and the erosion guard would have existed without guarding anything. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🛑 Gate Violations
ℹ️ InfoNo other issues found (see more)🧪 All tests passed 🎯 Code Coverage (details) Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: a89a1f7 | Docs | Datadog PR Page | Give us feedback! |
Three tests assumed POSIX semantics and failed on the Windows CI job: - os_open asserted POSIX mode bits round-trip. Windows implements only the read-only flag, so the value cannot survive. Assert the bits on POSIX and keep the write/read round-trip everywhere. - The PATH-resolution tests created a suffix-less executable, which Windows cannot find because bare names resolve through PATHEXT. Create a .bat there so the test exercises resolution on both platforms rather than being skipped on one. - sudo wrapping is a POSIX concept; that test is now skipped on Windows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The off-mode early return in check_exec was the only uncovered line in the interface, and it is the state every exec site ships in. Assert it directly rather than inferring it from the path-side equivalent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
directory exists to scan a user-supplied path, which makes it the clearest path-traversal case in the repository, yet it was the only migrated integration without an end-to-end enforcement test. Assert that a root outside the allowlist is never actually read. Verified by mutation: reverting the check to unmediated stdlib access makes this fail. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`from os import open` was caught but `os.open(...)` was not, because the call sat in the from-import table and not the dotted one. The interface exposes os_open precisely for this call, so both forms must be flagged. No integration uses it today; this closes the hole before one does. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two Windows failures in the in-memory filesystem, both from letting the platform separator into a key space that callers populate with forward slashes: - walk re-joined child paths with os.path.join, yielding '/root\sub' on Windows while the registered key was '/root/sub'. Recurse on the registered child keys instead of rebuilding them. - glob measured depth with os.sep, so on Windows `*` would have crossed separators. Translate the pattern per segment to a regex over '/'. The glob rewrite also fixes a semantic error: `**` now matches zero or more segments, so `/a/**/*.conf` matches `/a/x.conf` as real glob does. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both separator defects in the test double were invisible on POSIX and only surfaced in the Windows CI job, which is a slow and coarse feedback loop for something reproducible in-process. Point os.path at ntpath for the duration of a test so the same failure appears on any host. Verified by mutation: reintroducing the os.path.join in walk fails with '/root\\sub' on macOS. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…config Drop the path_enforcement_mode setting and its off/log/enforce modes. Point-of-use validation is now governed by the existing ignore_untrusted_file_params switch, the same one that already controls config-field validation, so this adds no operator-facing configuration. The tradeoff is recorded in the RFC and the developer docs rather than left implicit: the two can no longer be staged separately, so an operator already using field validation begins enforcing at every migrated call site as soon as this ships, with no dry-run. Rollout depends on the excluded-checks setting and batched migration instead. Removing the modes also removed the logging plumbing and the violation dedup/cap, which only existed to serve the log mode. Fixes a test isolation defect the change exposed: the datadog_agent stub is a module-level singleton that only resets for tests requesting the fixture, so enabling enforcement leaked into unrelated tests. The default mode of 'off' had been masking it. Fixtures now reset on teardown. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Validation ReportAll 22 validations passed. Show details
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a89a1f78c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| definition_file = _resolve_definition_file(definition_file) | ||
|
|
||
| with open(definition_file) as f: | ||
| with os_interface.open(definition_file) as f: |
There was a problem hiding this comment.
Pass the check-bound interface when reading SNMP profiles
definition_file is taken from SNMP init_config['profiles'], but this helper now opens the resolved file through the module-level os_interface singleton. When OS-interface enforcement is enabled for a provider outside the trusted list, a configured profile path or an extends entry can still be read with the no-op validator, so the new point-of-use protection is bypassed; thread the check-bound self.os_interface into the profile-loading helpers instead.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
|
Superseded by a stack of three PRs, split by package:
The guard is deliberately not selected in Closing this one. |
What does this PR do?
Adds an OS abstraction layer in
datadog_checks_basethat integrations use instead of callingopen,os,shutil,glob, andsubprocessdirectly, so path-based security validation can attach in one place. Migrates the integrations that perform direct I/O, adds addev validate os-interfaceguard, and includes the design RFC.Behavior is unchanged by default. Point-of-use enforcement is off unless an operator opts in.
Core interface (
datadog_checks_base/datadog_checks/base/utils/os_interface.py)get_subprocess_outputstays its own operation rather than being folded intorun, so its output decoding, empty-output handling, and logging are preserved exactly.integration_ignore_untrusted_file_paramssetting, the same switch that already governs config-field validation, and never applies to a trusted provider or an excluded check.argv[0]:sudois unwrapped, and undershell=Truethe shell is what gets validated since that is what the OS actually runs. Bare command names are resolved throughPATH.Migration
Integrations now use
self.os_interface. Module-level helpers take the interface as a required parameter, because the module-level singleton is bound to the no-op validator and passing it there would preserve parity and pass every test while enforcing nothing.Paths handed to libraries that open them themselves are validated at the handoff, via a
validate_pathoperation that validates and returns the value unchanged. This covers the shared TLS context builder indatadog_checks_base(tls_ca_cert,tls_cert,tls_private_key), which is how most integrations reach TLS, plushttp_check,esxi,foundationdb, andduckdb.vspherehas the same exposure butVSphereAPIhas no check reference and its constructor is used across many tests, so it is called out as follow-up rather than half-threaded here.Erosion guard
ddev validate os-interfaceflags raw stdlib I/O and use of the non-enforcing singleton in check modules. Detection is AST-based, so a method namedopenor a mention inside a docstring is not misreported, andfrom os import scandirstyle imports are resolved back to the call they bind. Narrow legitimate cases use an inline# SKIP_OS_INTERFACE_VALIDATIONcomment. The repository is currently clean: 410 passed, 0 errors.Adding import tracking immediately paid for itself: it surfaced
directory, whose entire purpose is scanning a user-configured path and which was reaching the filesystem completely unmediated viafrom os import scandir/from os.path import exists. That is the clearest path-traversal case in the repository, and dotted-name matching alone had missed it. It is migrated here.Note for reviewers: the main operational risk. Because validation reuses the existing setting rather than adding one, it cannot be staged separately from field validation. An operator who already has
integration_ignore_untrusted_file_paramsenabled will begin enforcing at every migrated call site as soon as this ships, and there is no dry-run mode in which violations are reported without being blocked. Rollout therefore depends on the excluded-checks setting and on migrating in batches. This was a deliberate choice to avoid adding operator-facing configuration; it is called out in the RFC and the developer docs, and is the thing most worth a second opinion.Motivation
Integrations accept inputs that are paths: a file to read, a
bin_dir, a path to a binary the check executes. The trusted-provider mechanism already decides whether such an input is acceptable, but it applies that decision to config fields at load time. It does not govern the operation performed later, nor paths derived at runtime.The exec case is the sharper one and is not hypothetical.
slurmexecutes binaries resolved from a configurableslurm_binaries_dirand per-command*_pathoptions,cephruns a configurableceph_cmd,glusterfsruns agstatus_path, andgunicornruns a configurablegunicornbinary. Several wrap the configured binary insudo, so the attacker-chosen program is not even the first element of the command.This is a mediation layer for direct standard-library I/O, not a containment boundary. A Python-level wrapper cannot intercept a path opened inside a third-party library, anything a subprocess does once launched, or what a shell string executes. Those limits are documented rather than implied. See
rfc-filesystem-abstraction-layer.md.Testing
datadog_checks_base: 185 unit tests pass; the OS interface suite is 86 tests, 177 counting the test double. The interface module is at 100% line coverage, re-measured after the final change rather than claimed once.slurm,gunicorn,esxi,foundationdb, anddirectoryhave tests asserting that a disallowed binary, certificate path, or traversal root is never launched, handed to the library, or read, exercising the real check and its real config parsing.shell=Truehandling, reverting toargv[0]-only validation, reverting an integration to the non-enforcing singleton, revertingdirectoryto unmediated stdlib access, adding an unguarded public method) makes the relevant test fail. Source was restored after each.ddev: the validate suite passes, including 15 tests for the new validation.ddev test --lintclean across all 43 changed packages.cactineedingrrd.h,btrfsbeing Linux-only,foundationdbneeding the nativelibfdb_c); CI has since run all of them onubuntu-22.04and they pass.ddev docs build --checkpasses.Several defects in this work were caught by its own tooling or by CI rather than by review, and are worth flagging since each was an invisible failure mode:
glob, so a fixture-based test would have silently read the real filesystem. Fixed, and there is now a drift guard asserting the double covers the full interface surface..ddev/config.tomlpins an explicit validation list, so the erosion guard would never have run in CI despite being registered.resolve_pathnormalized paths at library handoffs, turning a relative path absolute and breaking the parity requirement. The pre-existingtest_tls.pycaught it; hencevalidate_path.refresh_tls_contextrebuilt the TLS context without the validator, skipping validation on refresh.PATHEXT) and failed the Windows job; they now exercise both platforms rather than being skipped.walkre-joined children withos.path.join(/root\subvs the registered/root/sub) andglobmeasured depth withos.sep. Rewritingglobalso fixed a platform-independent semantic bug, sincefnmatchcannot express**matching zero or more segments. A double that under-matches makes tests pass for the wrong reason.Those last defects were only reachable through the Windows CI job, which is a slow loop for something reproducible in-process, so there is now a fixture that points
os.pathatntpathand reproduces them on any host.Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged🤖 Generated with Claude Code