Skip to content

Add a validated OS interface for filesystem and subprocess access - #24782

Draft
NouemanKHAL wants to merge 7 commits into
noueman/os-interface-ddevfrom
noueman/os-interface-base
Draft

Add a validated OS interface for filesystem and subprocess access#24782
NouemanKHAL wants to merge 7 commits into
noueman/os-interface-ddevfrom
noueman/os-interface-base

Conversation

@NouemanKHAL

Copy link
Copy Markdown
Member

What does this PR do?

Stack 2 of 3. Adds the OS abstraction layer in datadog_checks_base that integrations use instead of calling open, os, shutil, glob, and subprocess directly, so path-based validation attaches in one place.

No integration is migrated here; that is stack 3. Behavior is unchanged.

  • Each method is a thin passthrough preceded by a validator hook. Under the default no-op validator, exception types and timing, permission bits, encodings, laziness, and return values match the call being replaced.
  • get_subprocess_output stays its own operation rather than folding into run, preserving its output decoding, empty-output handling, and logging.
  • No new configuration. Validation is gated by the existing integration_ignore_untrusted_file_params setting, and never applies to a trusted provider or an excluded check.
  • Executable validation covers every program a command launches, not just argv[0]: sudo is unwrapped, and under shell=True the shell is validated, since that is what the OS actually runs. Bare names are resolved through PATH.
  • The shared TLS context builder validates the config-derived certificate paths it hands to ssl, via a validate_path operation that validates and returns the value unchanged.
  • A mock_os_interface fixture redirects both bindings at once, so tests need not know which one a check uses.

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 applies that decision to config fields at load time. It does not govern the operation performed later, nor paths derived at runtime.

The main operational risk, for reviewers. 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_params enabled begins enforcing at every migrated call site as soon as stack 3 ships, and there is no dry-run mode. Rollout depends on the excluded-checks setting and on migrating in batches. This is deliberate, and is the thing most worth a second opinion. See rfc-filesystem-abstraction-layer.md.

This is a mediation layer for direct standard-library I/O, not a containment boundary. A Python 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.

Testing

  • 185 unit tests in datadog_checks_base; the OS interface suite is 86 tests, 177 counting the test double. The interface module is at 100% line coverage.
  • A registry test asserts every public method consults the validator, so a new method cannot be added without enforcement coverage. A second asserts the test double covers the full interface surface, after glob was found missing from it.
  • Mutation-tested: dropping a validator hook, dropping shell=True handling, reverting to argv[0]-only validation, or adding an unguarded public method each makes the relevant test fail.
  • A fixture points os.path at ntpath so Windows-only path bugs in the double reproduce on any host.
  • ddev test --lint clean.

Stack: 1. ddev validate os-interface · 2. this PR · 3. integrations migration

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Add qa/required if this PR needs QA validation, or qa/skip-qa if it does not. Exactly one of the two is required.
  • If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

🤖 Generated with Claude Code

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.

Validation adds no configuration. It is gated by the existing
ignore_untrusted_file_params setting, which also governs config-field
validation, so the two cannot be staged separately. That tradeoff is
recorded in the RFC and the developer docs.

Executable validation covers every program a command will launch, not just
argv[0]: sudo is unwrapped, and under shell=True the shell is what gets
validated. Bare command names are resolved through PATH.

Also included: the shared TLS context builder now validates the
config-derived certificate paths it hands to ssl, and a mock_os_interface
fixture that redirects both bindings so tests need not know which a check
uses.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Pipelines  Tests  Code Coverage

⚠️ Warnings

🚦 2 Pipeline jobs failed

PR All | test / jb232c8c / Datadog Checks Base on Linux   View in Datadog   GitHub Actions

🔧 Fix in code. This looks caused by changes in this PR. AttributeError: <class 'datadog_checks.base.checks.base.AgentCheck'> does not have the attribute 'unchecked_os' during setup of test_fixture_redirects_check_property and test_fixture_redirects_module_singleton.

PR All Windows | test / jda96080 / Datadog Checks Base on Windows   View in Datadog   GitHub Actions

🔧 Fix in code. This looks caused by changes in this PR. 2 errors during test setup due to AttributeError: <class 'datadog_checks.base.checks.base.AgentCheck'> does not have the attribute 'unchecked_os'.
📋 Copy prompt for your agent
CI on my pull request is failing. Help me find and fix the root cause of each failing job below — they were flagged as caused by changes in this PR, so focus on the diff. For each job, explain the failure and propose a fix.

Branch: noueman/os-interface-base

PR All | test / jb232c8c / Datadog Checks Base on Linux
Commit: e22f6a2085a78f0678dbf18d8372466061cf5af9
Error (code / test):
AttributeError: <class 'datadog_checks.base.checks.base.AgentCheck'> does not have the attribute 'unchecked_os' during setup of test_fixture_redirects_check_property and test_fixture_redirects_module_singleton.
CI job: https://github.com/DataDog/integrations-core/actions/runs/31197320791/job/92928666775

PR All Windows | test / jda96080 / Datadog Checks Base on Windows
Commit: e22f6a2085a78f0678dbf18d8372466061cf5af9
Error (code / test):
2 errors during test setup due to AttributeError: <class 'datadog_checks.base.checks.base.AgentCheck'> does not have the attribute 'unchecked_os'.
CI job: https://github.com/DataDog/integrations-core/actions/runs/31197322275/job/92928657543

🧪 2 Tests failed in 1 job

PR All Windows | run   GitHub Actions

test_fixture_redirects_check_property from test_os_wrapper_fixture.py   View in Datadog
&lt;class &#39;datadog_checks.base.checks.base.AgentCheck&#39;&gt; does not have the attribute &#39;unchecked_os&#39;
test_fixture_redirects_module_singleton from test_os_wrapper_fixture.py   View in Datadog
&lt;class &#39;datadog_checks.base.checks.base.AgentCheck&#39;&gt; does not have the attribute &#39;unchecked_os&#39;

ℹ️ Info

No other issues found (see more)

❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 88.91% (+0.42%)

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 012f595 | Docs | Datadog PR Page | Give us feedback!

NouemanKHAL and others added 6 commits August 5, 2026 15:27
SafeOS, the `safe_os` singleton, and `AgentCheck.safe_os`. The test double
becomes MockSafeOS and its fixture `mock_safe_os`.

Also revert the shared TLS context builder. Validating there would mean
either threading an interface argument through TlsContextWrapper or
duplicating its config remapping in AgentCheck; neither is worth it. The
certificate paths it hands to ssl are recorded as an uncovered gap in the
docs, alongside vsphere.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
create_ssl_context makes exactly one call that touches the filesystem,
os.path.isdir on the configured tls_ca_cert. Route it through safe_os.

Everything else it does with a certificate path is handed to ssl, which
opens the file itself, so the interface cannot mediate it; the docs note is
narrowed to say so rather than implying more is missing than is.

Also drop a leftover safe_os= keyword that get_tls_context still passed to
TlsContextWrapper after the wrapper's parameter was reverted. The -m unit
subset skips test_tls.py, which is why this was not caught earlier.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
safe_os overclaimed: the layer is explicitly not a containment boundary,
and with validation off it is a plain passthrough. Worse, the module-level
singleton shared that name while being bound to the no-op validator, so the
most dangerous object in the design read as the most reassuring.

Follow the self.http precedent and name the check-bound accessor for its
domain, self.os, backed by OSWrapper. Name the singleton unchecked_os, so
the hazard is visible at every call site rather than only in the docs. The
test double becomes MockOSWrapper behind a mock_os fixture.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dd-octo-sts

dd-octo-sts Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Validation Report

All 21 validations passed.

Show details
Validation Description Status
agent-reqs Verify check versions match the Agent requirements file
ci Validate CI configuration and code coverage settings
codeowners Validate every integration has a CODEOWNERS entry
config Validate default configuration files against spec.yaml
dep Verify dependency pins are consistent and Agent-compatible
http Validate integrations use the HTTP wrapper correctly
imports Validate check imports do not use deprecated modules
integration-style Validate check code style conventions
jmx-metrics Validate JMX metrics definition files and config
labeler Validate PR labeler config matches integration directories
legacy-signature Validate no integration uses the legacy Agent check signature
license-headers Validate Python files have proper license headers
licenses Validate third-party license attribution list
metadata Validate metadata.csv metric definitions
models Validate configuration data models match spec.yaml
openmetrics Validate OpenMetrics integrations disable the metric limit
package Validate Python package metadata and naming
qa-label Validate the pull request declares whether it needs QA for the next Agent release
readmes Validate README files have required sections
saved-views Validate saved view JSON file structure and fields
version Validate version consistency between package and changelog

View full run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

base_package dev_package documentation qa/required QA is required for this PR and will generate a QA card

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant