fix(capture): make HostPath a subpath under a fixed base dir by SRodi · Pull Request #2335 · microsoft/retina

SRodi · 2026-05-13T17:55:26Z

Problem

Capture.spec.outputConfiguration.hostPath was passed unchecked into corev1.HostPathVolumeSource.Path. Any user able to create a Capture CR could mount an arbitrary host directory RW into a privileged pod and write artifacts anywhere on the node.

Fix

HostPath is now a relative subpath name. The operator mounts ${baseDir}/${hostPath}, where baseDir is set by the cluster operator via Helm value capture.hostPathBaseDir (default /var/log/retina/captures). CR authors cannot influence baseDir.

Validation rejects (loudly, no silent rewrite):

empty / .
absolute paths (POSIX /, Windows C:\, leading \)
any .. segment, before or after filepath.Clean
defense-in-depth: cleaned join must stay under baseDir

The resolved path is used for HostPathVolumeSource.Path, the container VolumeMount.MountPath, and the CAPTURE_OUTPUT_LOCATION_HOST_PATH env var.

Key changes

New pkg/capture/hostpath_validation.go + tests (validateHostPath, sentinel errors, DefaultHostPathBaseDir).
pkg/capture/crd_to_job.go: validateCapture rejects early; initJobTemplate and obtainCaptureOutputEnv use the resolved path.
pkg/config/capture.go + operator/config/config.go: new CaptureHostPathBaseDir (defaulted, cleaned, must be absolute).
Helm: capture.hostPathBaseDir in values.yaml and operator-configmap.yaml.
CLI: --host-path default is now retina; new --host-path-base-dir flag; auto-allow hack removed.
CRD godoc + manifest regenerated; samples and docs updated.

Breaking change

Capture CRs with an absolute hostPath (e.g. /tmp/retina) are rejected. Migration: use a bare name (retina); the mount becomes /var/log/retina/captures/retina. Operators needing a different root set capture.hostPathBaseDir.

Test

go test ./pkg/capture/ ./pkg/config/ ./operator/config/ ./cli/... — pass
go vet ./pkg/capture/ ./pkg/config/ ./operator/config/ ./cli/... — clean
Unit cases: empty, ., POSIX/Windows absolute, pre-clean .., post-clean foo/../bar, escape, accepted nested subpaths.
Manual kind run: absolute / .. / foo/../bar rejected with no Job created and a single CaptureError condition; bare subpath mounts at ${baseDir}/${name}; overriding capture.hostPathBaseDir relocates the mount without changing the CR.

Related Issue

If this pull request is related to any issue, please mention it here. Additionally, make sure that the issue is assigned to you before submitting this pull request.

Checklist

I have read the contributing documentation.
I signed and signed-off the commits (git commit -S -s ...). See this documentation on signing commits.
I have correctly attributed the author(s) of the code.
I have tested the changes locally.
I have followed the project's style guidelines.
I have updated the documentation, if necessary.
I have added tests, if applicable.

Screenshots (if applicable) or Testing Completed

Negative — absolute HostPath is rejected loudly

Negative — parent segment is rejected

Negative — post-clean traversal is rejected

Positive — bare subpath is accepted

Override the base directory

Additional Notes

Add any additional notes or context about the pull request here.

Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.

mereta

LGTM

alexcastilio

I sent some comments on the current approach. I'd like to suggest a different approach:

Instead of allowing the user to set any folder in the host, use the value provided by the user in outputConfiguration.hostPath, to create that as a subpath inside a default folder. E.g. defaultOutputDir = /captures and user sets outputConfiguration.hostPath=my_capture123, then it will be saved to /captures/my_capture123

Keep the validation against using .. to go outside of that folder, and we don't need to create a breaking change which is to create a new list of allowedPrefixes.

SRodi · 2026-05-18T16:12:44Z

I sent some comments on the current approach. I'd like to suggest a different approach:

Instead of allowing the user to set any folder in the host, use the value provided by the user in outputConfiguration.hostPath, to create that as a subpath inside a default folder. E.g. defaultOutputDir = /captures and user sets outputConfiguration.hostPath=my_capture123, then it will be saved to /captures/my_capture123

Keep the validation against using .. to go outside of that folder, and we don't need to create a breaking change which is to create a new list of allowedPrefixes.

Thanks @alexcastillo Agreed. I implemented exactly that (user value is treated as a subpath under an operator-configured base dir, default /var/log/retina/captures); pushed in the last commit. The only remaining breaking change is that absolute hostPath values are now rejected loudly instead of being silently relocated, to avoid surprising on-disk moves on upgrade.

mereta

LGTM

alexcastilio · 2026-05-20T09:27:58Z

@SRodi have you reviewed the download capture flow? Will they download from right location with your change? I noticed that there's no change in cli/cmd/capture/{download,download_test}.go files.

SRodi · 2026-05-20T10:20:58Z

@SRodi have you reviewed the download capture flow? Will they download from right location with your change? I noticed that there's no change in cli/cmd/capture/{download,download_test}.go files.

good catch! thanks @alexcastilio! We need to automate capture and download tests to avoid manual testing for each capture change.

Here is a manual test for now

Copilot

Pull request overview

This PR hardens the capture “HostPath” output option by treating Capture.spec.outputConfiguration.hostPath as a relative subpath and resolving it under an operator-controlled absolute base directory, preventing Capture CR authors from mounting arbitrary host directories into privileged pods.

Changes:

Added HostPath validation + resolution helper (with tests) to block absolute paths and traversal, and to ensure the resolved path stays under an operator-configured base directory.
Updated capture job rendering to use the resolved HostPath consistently across volume HostPath.Path, VolumeMount.MountPath, annotations, and the CAPTURE_OUTPUT_LOCATION_HOST_PATH env var.
Introduced/configured captureHostPathBaseDir via operator config + Helm values, updated CLI flags/defaults, and refreshed docs/samples/CRD comments.

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
samples/capture/podblobupload.yaml	Updates sample HostPath to a relative subpath.
samples/capture/nodeblobupload.yaml	Updates sample HostPath to a relative subpath.
samples/capture/node-s3upload-minio.yaml	Updates sample HostPath to a relative subpath.
samples/capture/node-s3upload-aws.yaml	Updates sample HostPath to a relative subpath.
samples/capture/capture-specific-pod-on-a-node.yaml	Updates sample HostPath to a relative subpath.
pkg/controllers/operator/capture/controller.go	Removes redundant error logging when job translation fails.
pkg/config/capture.go	Adds `CaptureHostPathBaseDir` to operator capture config.
pkg/capture/utils/annotations.go	Adds resolved-host-path plumbing for pod annotations.
pkg/capture/utils/annotations_test.go	Adds unit tests for new annotation behavior.
pkg/capture/hostpath_validation.go	Introduces HostPath validation + resolution under a base dir.
pkg/capture/hostpath_validation_test.go	Adds unit tests for HostPath validation and containment.
pkg/capture/crd_to_job.go	Validates HostPath early and uses resolved paths for volumes/mounts/env/annotations.
pkg/capture/crd_to_job_test.go	Updates tests for relative HostPath semantics and resolved path usage.
operator/config/config.go	Defaults/cleans/validates the configured HostPath base dir.
docs/05-Concepts/CRDs/Capture.md	Documents HostPath as relative subpath under a base dir.
docs/04-Captures/04-managed-storage-account.md	Updates examples to use relative HostPath.
docs/04-Captures/03-crd.md	Updates CRD examples to use relative HostPath.
docs/02-Installation/03-Config.md	Documents new Helm/operator config value `capture.hostPathBaseDir`.
deploy/standard/manifests/controller/helm/retina/values.yaml	Adds Helm value `capture.hostPathBaseDir`.
deploy/standard/manifests/controller/helm/retina/templates/operator-configmap.yaml	Wires Helm value into operator configmap.
crd/api/v1alpha1/capture_types.go	Updates CRD godoc for HostPath behavior/validation.
cli/cmd/capture/download_test.go	Fixes test isolation by resetting flag-bound globals between subtests.
cli/cmd/capture/create.go	Changes CLI HostPath defaults, adds `--host-path-base-dir`, and passes base dir into translator config.
cli/cmd/capture/create_test.go	Updates/extends CLI tests for new HostPath semantics and config propagation.
cli/cmd/capture/capture.go	Extends CLI options struct with `hostPathBaseDir`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

A Capture CR's OutputConfiguration.HostPath was passed unchecked into corev1.HostPathVolumeSource, letting a CRD author mount any host directory RW into the privileged capture pod and write pcap output anywhere on the node filesystem. Add validateHostPath which requires an absolute, traversal-free path that, after cleaning, equals or is nested under one of the configured allowed prefixes (with a trailing-separator check to prevent prefix-confusion such as /foo vs /foo-evil). Wire it into both validateCapture (early reject) and initJobTemplate (uses the cleaned path for the Volume and VolumeMount). Add CaptureConfig.CaptureHostPathAllowedPrefixes, default it to [/var/log/retina/captures] in the operator config loader, and expose it via the Helm value capture.hostPathAllowedPrefixes. The CLI auto- allows the user-supplied --host-path so interactive UX is preserved; the threat model is CRD-author abuse via the operator. Tests: new table-driven coverage for traversal, prefix-confusion, defaulting, and rejection; existing crd_to_job tests updated to configure /tmp/capture as an allowed prefix.

OutputConfiguration.HostPath was passed unchecked into HostPathVolumeSource.Path, letting any Capture CR author mount an arbitrary host directory RW into the privileged capture pod. Reinterpret the field as a relative subpath name that the operator joins under a single, operator-configured base directory (capture.hostPathBaseDir, default /var/log/retina/captures). Reject absolute paths, Windows drive-letter paths, and any '..' segment loudly during reconciliation; do not silently rewrite. The joined, cleaned path is used for both the host volume and the workload's mount path / output env var so the capture writes where the volume is actually mounted. The CLI follows the same rule via a new --host-path-base-dir flag; the previous CLI auto-allow hack is removed. Samples, CRD field godoc, generated CRD manifest, and user docs are updated. BREAKING CHANGE: Capture CRs supplying an absolute HostPath are now rejected. Migrate to a bare subpath name; the mount becomes ${capture.hostPathBaseDir}/${value}

mereta

LGTM! 👌🏼

mereta

Re-approve!

SRodi self-assigned this May 13, 2026

SRodi requested a review from a team as a code owner May 13, 2026 17:55

SRodi requested review from camrynl and orangejuice May 13, 2026 17:55

mereta previously approved these changes May 14, 2026

View reviewed changes

alexcastilio reviewed May 14, 2026

View reviewed changes

Comment thread cli/cmd/capture/create.go

Comment thread operator/config/config.go Outdated

mereta self-requested a review May 14, 2026 17:20

SRodi dismissed mereta’s stale review via cf829e8 May 18, 2026 16:11

SRodi force-pushed the srodi/capture-hostpath-restrict branch from cf829e8 to 05a0423 Compare May 18, 2026 16:13

SRodi changed the title ~~fix(capture): restrict HostPath volume mounts~~ fix(capture): make HostPath a subpath under a fixed base dir May 18, 2026

SRodi requested a review from alexcastilio May 19, 2026 09:06

mereta previously approved these changes May 19, 2026

View reviewed changes

SRodi dismissed mereta’s stale review via 0dc68fb May 20, 2026 10:17

SRodi force-pushed the srodi/capture-hostpath-restrict branch from 0dc68fb to a743a5a Compare May 20, 2026 10:18

alexcastilio reviewed May 20, 2026

View reviewed changes

Comment thread cli/cmd/capture/create_test.go

alexcastilio reviewed May 20, 2026

View reviewed changes

Comment thread cli/cmd/capture/create_test.go

alexcastilio reviewed May 20, 2026

View reviewed changes

Comment thread pkg/capture/utils/annotations.go

SRodi requested review from alexcastilio and mereta May 20, 2026 11:08

alexcastilio requested review from Copilot and removed request for alexcastilio and mereta May 20, 2026 11:28

Copilot started reviewing on behalf of alexcastilio May 20, 2026 11:32 View session

Copilot AI reviewed May 20, 2026

View reviewed changes

Comment thread pkg/capture/hostpath_validation.go

Comment thread pkg/capture/hostpath_validation.go Outdated

Comment thread pkg/capture/crd_to_job.go Outdated

Comment thread operator/config/config.go

carlotaarvela reviewed May 20, 2026

View reviewed changes

Comment thread pkg/controllers/operator/capture/controller.go

mereta reviewed May 20, 2026

View reviewed changes

Comment thread pkg/capture/utils/annotations.go Outdated

mereta reviewed May 20, 2026

View reviewed changes

Comment thread pkg/capture/crd_to_job.go Outdated

SRodi added 10 commits May 21, 2026 11:27

test(cli): add unit test for getCLICaptureConfig

e0b6e96

fix linting

9d2ca36

fix linting (part 2)

3fb6b55

fix failing download test

eaca210

fix failing node selector test

92a5785

fix download flow

53fa175

address comments

41ee793

address comments

e01f8d0

SRodi dismissed mereta’s stale review via e01f8d0 May 21, 2026 10:28

SRodi force-pushed the srodi/capture-hostpath-restrict branch from 65af10e to e01f8d0 Compare May 21, 2026 10:28

SRodi requested review from alexcastilio, carlotaarvela and mereta May 21, 2026 10:28

mereta previously approved these changes May 21, 2026

View reviewed changes

SRodi dismissed mereta’s stale review via bd91f65 May 21, 2026 10:39

SRodi requested a review from mereta May 21, 2026 10:40

carlotaarvela previously approved these changes May 21, 2026

View reviewed changes

fix linting

e920a64

SRodi dismissed carlotaarvela’s stale review via e920a64 May 21, 2026 10:47

SRodi force-pushed the srodi/capture-hostpath-restrict branch from bd91f65 to e920a64 Compare May 21, 2026 10:47

mereta approved these changes May 21, 2026

View reviewed changes

SRodi requested a review from carlotaarvela May 21, 2026 11:05

mereta added this pull request to the merge queue May 21, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 21, 2026

mereta added this pull request to the merge queue May 21, 2026

Merged via the queue into microsoft:main with commit a481e45 May 21, 2026
34 checks passed

mereta linked an issue May 22, 2026 that may be closed by this pull request

vuln(retina-capture): cmdline Injection - Validate tcpdump args created via CR #2298

Closed

Conversation

SRodi commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Key changes

Breaking change

Test

Related Issue

Checklist

Screenshots (if applicable) or Testing Completed

Negative — absolute HostPath is rejected loudly

Negative — parent segment is rejected

Negative — post-clean traversal is rejected

Positive — bare subpath is accepted

Override the base directory

Additional Notes

Uh oh!

mereta left a comment

Choose a reason for hiding this comment

Uh oh!

alexcastilio left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SRodi commented May 18, 2026

Uh oh!

mereta left a comment

Choose a reason for hiding this comment

Uh oh!

alexcastilio commented May 20, 2026

Uh oh!

SRodi commented May 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mereta left a comment

Choose a reason for hiding this comment

Uh oh!

mereta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SRodi commented May 13, 2026 •

edited

Loading

alexcastilio left a comment •

edited

Loading