feat(allowedpaths): prepend /host to symlink targets in containers#170
feat(allowedpaths): prepend /host to symlink targets in containers#170
Conversation
In containerized environments (DOCKER_DD_AGENT set), host symlinks use host-absolute paths (e.g. /var/log/pods/...) that don't include the /host mount prefix. When resolving cross-root symlinks, prepend /host to the target path so it matches the container's allowed roots. Refs #166 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ontainers In containerized environments (DOCKER_DD_AGENT set), host symlinks use host-absolute paths (e.g. /var/log/pods/...) that don't include the /host mount prefix. When resolving cross-root symlinks, prepend the host prefix so the path matches the container's allowed roots. The host prefix is stored as a field on Sandbox (defaulting to /host) so tests can override it with temp dirs. Refs #166 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…iner symlinks - Add SetHostPrefix on Sandbox and public HostPrefix RunnerOption - Add containerized field to scenario YAML that sets DOCKER_DD_AGENT and host prefix to testdir/host automatically - Groups with containerized scenarios skip t.Parallel (t.Setenv) - Add container_host_prefix_symlink.yaml end-to-end scenario test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8 scenarios covering: - cat through container symlink (host/var/log as single allowed root) - ls directory listing through container symlink - grep through container symlink - multiple sequential reads - cat/head/tail/wc/grep all through same container symlink - blocked: symlink to /etc/secret outside allowed root - blocked: symlink to /opt/data outside allowed root - blocked: symlink using parent traversal to escape Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abd098f351
ℹ️ 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".
Remove the conditional check that skipped prepending when the path already started with the host prefix. In containerized environments, the host prefix is always prepended blindly to symlink targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The container host prefix feature is Linux-only (host mounts under /host). Skip containerized scenarios on Windows where symlinks work differently and the /host prefix concept doesn't apply. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Clean trailing slashes and other path artifacts via filepath.Clean to prevent malformed prefix comparisons. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Explain that the Private Action Runner in the Datadog agent mounts host volumes exclusively under /host, so the blind prepend is correct for our deployment. Note the HostPrefix option for future changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review. note that we always prepend the host prefix when encountering a symlink in a containerized environment, that is the whole reason for this PR so you should not consider that a concern. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05e1891151
ℹ️ 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".
Store the host prefix on the runner and apply it to the sandbox in New() after all options are processed. HostPrefix can now be applied before or after AllowedPaths without silently dropping the value. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add tests for: - HostPrefix after AllowedPaths - HostPrefix before AllowedPaths - HostPrefix without AllowedPaths (silently ignored) - Default prefix when HostPrefix is not called Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review. note that we always prepend the host prefix when encountering a symlink in a containerized environment, that is the whole reason for this PR so you should not consider that a concern. |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…enarios - Rename GetHostPrefix() to HostPrefix() per Effective Go naming (no Get prefix on getters) - Load scenarios once per group and reuse, avoiding double loadScenario calls in the containerized check loop - Fix skip message: containerized tests are unsupported on Windows, not Linux-only Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review. note that we always prepend the host prefix when encountering a symlink in a containerized environment, that is the whole reason for this PR so you should not consider that a concern. |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Container host prefix is not supported on Windows. Skip the three HostPrefix ordering tests that fail due to filepath.Clean converting forward slashes to backslashes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
matt-dz
requested review from
AlexandreYang,
astuyve,
julesmcrt,
thieman and
val06
as code owners
Remove auto-detection via DOCKER_DD_AGENT env var. Container symlink
resolution is now enabled by calling HostPrefix — a non-empty prefix
activates the feature. This removes:
- containerized bool from Sandbox
- os.Getenv("DOCKER_DD_AGENT") from New()
- defaultHostMountPrefix const
- t.Setenv and parallel restrictions from test harness
The scenario harness sets HostPrefix directly via the RunnerOption.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…er prefix Relative symlinks (e.g. ../pods/app.log) resolve to paths that already include the host prefix. Skip prepending to avoid double-prefixing. Added unit test and scenario test for relative symlinks in container mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Summary
In containerized Kubernetes environments, host symlinks use host-absolute paths (e.g.
/var/log/pods/...) that don't include the/hostmount prefix. The Private Action Runner in the Datadog agent mounts host volumes exclusively under/host, so these symlinks fail to resolve.This PR adds container-aware symlink resolution:
HostPrefixpublicRunnerOptionenables the feature and sets the mount prefix (defaults to/host)containerizedfield in scenario YAML for end-to-end testingSetHostPrefix/HostPrefixgetter/setter onSandboxfor direct accessCloses #166
Test plan
🤖 Generated with Claude Code