fix(nvsnap): confine agent path construction to its own directories - #519
Merged
Conversation
Code scanning flagged 39 go/path-injection alerts across the agent. They
are not 39 defects: every one is a filesystem call downstream of one of
two identifiers the agent joins onto a host directory without checking.
checkpointDir := filepath.Join(a.config.CheckpointDir, req.CheckpointID)
req.CheckpointID is decoded straight from an HTTP request body, and the
relative paths driving cascade fetch come from a manifest another agent
serves over HTTP. The agent runs privileged with hostPath mounts covering
/var/lib and the containerd root, so a "../" in either is a read or write
anywhere on the node as root, not a contained bug.
Closed at the two entry points rather than the 39 sinks:
- validPathSegment rejects an identifier that is not a single, benign path
component. Applied to Restore, TriggerRestore, EnsureLocal and the
gpuRestore handler, and to every {id}/{hash}/{pod-uid} route through
pathVarGuard router middleware -- a per-handler check is one forgotten
line away from reopening the hole on the next route added.
- joinWithinRoot is the write-side counterpart to the existing
resolveWithinRoot: it confines a peer-supplied relative path to the
destination directory without requiring the file to exist yet. It also
closes the two-step variant a lexical check alone misses, where the peer
sends a symlink out of the tree and then a file underneath it.
The shape check is deliberately looser than what buildCheckpointID emits so
checkpoints written by older agents stay readable; the property being
enforced is "cannot leave the parent directory", not "matches today's
generator".
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe agent adds path-segment validation, symlink-aware destination confinement, authentication configuration and middleware wiring, early checkpoint ID validation, and no-follow temporary-file creation across restore, GPU restore, routing, and cascade download paths. ChangesAgent security hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/compute-plane-services/nvsnap/internal/agent/restore.go (1)
396-403: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidation runs before the span exists, unlike
EnsureLocal's pattern.
EnsureLocalincascade_fetch.gostarts its span first, then validates and records the failure on that span (span.SetStatus(codes.Error, err.Error())). Here,validPathSegmentruns beforetracing.Tracer().Start(ctx, "restore.full"), so a rejectedCheckpointIDon the primary restore entrypoint produces no span at all — no error status, nothing observable for tracing/alerting on attempted path-injection against this path. Moving the span creation above the validation check (and recording the error on it before returning) would align this with the sibling pattern and keep rejected attempts visible.As per path instructions, "If request handlers or cross-service paths are affected, add OpenTelemetry spans for inbound requests and outbound calls... set error=true + otel status on failures."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compute-plane-services/nvsnap/internal/agent/restore.go` around lines 396 - 403, Start the "restore.full" tracing span at the beginning of Agent.Restore before validating req.CheckpointID; when validPathSegment rejects the ID, record the error on that span with error status before returning. Preserve the existing validation and restore flow for valid requests.Source: Path instructions
src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go (1)
121-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
span.RecordErroron the new validation-failure branch.Every other failure branch in
EnsureLocalcalls bothRecordErrorandSetStatus(e.g. lines 244-246, 279-281, 294-295), but this new validation check only callsspan.SetStatus(codes.Error, err.Error()). That loses the structured error detail (exception type/message as a span event) that the sibling branches, and most OTel-aware dashboards, rely on.As per coding guidelines, spans should "set error attributes on failures" — recording the error alongside the status keeps this consistent with the rest of the function.
🛠️ Suggested fix
if err := validPathSegment("checkpoint id", checkpointID); err != nil { + span.RecordError(err) span.SetStatus(codes.Error, err.Error()) return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go` around lines 121 - 126, Add span.RecordError(err) to the checkpointID validation-failure branch in EnsureLocal before setting the error status, matching the error-recording behavior of the other failure branches while preserving the existing return flow.Source: Coding guidelines
src/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.go (1)
107-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape-loop assertion can never fail.
targetis built asfilepath.Join(root, cleaned)wherecleanedis already anchored under"/", so whenevererr == nil,gotis guaranteed to have therootprefix by construction — the../escape,../../etc/cron.d/x,/etc/passwdcases can never tript.Errorfhere. This loop doesn't actually assert that lexical escapes are rejected/contained; only the symlink test below (117-123) exercises a real failure path.♻️ Suggested fix: assert on the resolved relationship instead
for _, rel := range []string{"../escape", "../../etc/cron.d/x", "/etc/passwd"} { got, err := joinWithinRoot(root, rel) - if err == nil && !strings.HasPrefix(got, root+string(os.PathSeparator)) { - t.Errorf("joinWithinRoot(%q) = %q, escaped root", rel, got) + if err != nil { + t.Errorf("joinWithinRoot(%q) unexpected error: %v", rel, err) + continue + } + if !strings.HasPrefix(got, root+string(os.PathSeparator)) { + t.Errorf("joinWithinRoot(%q) = %q, escaped root", rel, got) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.go` around lines 107 - 113, Replace the ineffective prefix-based assertion in the escape-case loop with an assertion on the actual resolved relationship returned by joinWithinRoot. Ensure ../escape, ../../etc/cron.d/x, and /etc/passwd are verified as rejected or contained according to the function’s contract, while preserving the existing symlink test separately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go`:
- Around line 121-126: Add span.RecordError(err) to the checkpointID
validation-failure branch in EnsureLocal before setting the error status,
matching the error-recording behavior of the other failure branches while
preserving the existing return flow.
In `@src/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.go`:
- Around line 107-113: Replace the ineffective prefix-based assertion in the
escape-case loop with an assertion on the actual resolved relationship returned
by joinWithinRoot. Ensure ../escape, ../../etc/cron.d/x, and /etc/passwd are
verified as rejected or contained according to the function’s contract, while
preserving the existing symlink test separately.
In `@src/compute-plane-services/nvsnap/internal/agent/restore.go`:
- Around line 396-403: Start the "restore.full" tracing span at the beginning of
Agent.Restore before validating req.CheckpointID; when validPathSegment rejects
the ID, record the error on that span with error status before returning.
Preserve the existing validation and restore flow for valid requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 568625e5-4b76-4dcf-b96d-de2c2f30e02e
📒 Files selected for processing (5)
src/compute-plane-services/nvsnap/internal/agent/agent.gosrc/compute-plane-services/nvsnap/internal/agent/cascade_fetch.gosrc/compute-plane-services/nvsnap/internal/agent/pathsafe.gosrc/compute-plane-services/nvsnap/internal/agent/pathsafe_write_test.gosrc/compute-plane-services/nvsnap/internal/agent/restore.go
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/compute-plane-services/nvsnap/internal/agent/agent.go (1)
448-451: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winInstrument path-guard rejections.
The new 400 branch has no structured security log, span error attribute, or RED error metric. Record a bounded rejection event without logging the supplied identifier or request body.
As per coding guidelines, “Add OpenTelemetry spans for inbound requests” and “Request-handling services must expose RED metrics”; as per path instructions, “structured logs on every request path … then tracing … then metrics.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compute-plane-services/nvsnap/internal/agent/agent.go` around lines 448 - 451, Update the pathVarGuard middleware used by router.Use to instrument rejected path-variable requests with a bounded structured security log, a span error attribute, and the existing RED error metric. Record only safe rejection context—never the supplied identifier or request body—and preserve the current 400 response behavior.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/compute-plane-services/nvsnap/internal/agent/agent.go`:
- Around line 451-458: Register the middleware returned by tokenGuard before
pathVarGuard in the router setup, so authentication executes first in required
mode even for malformed requests. Preserve the existing conditional installation
behavior when no guard is configured.
In `@src/compute-plane-services/nvsnap/internal/agent/auth.go`:
- Around line 96-99: Update tokenGuard so AuthRequired with an empty token never
returns nil or permits requests: reject the invalid configuration during
startup, or return a deny-all middleware guard. Preserve the existing
unauthenticated behavior only for AuthDisabled, and use the Agent.Run startup
path or tokenGuard as the implementation point.
- Around line 100-127: Instrument the middleware returned by the auth handler so
every request, including unauthenticatedPaths and successful authOK decisions,
emits a structured audit log with safe fields only. Add an inbound OpenTelemetry
span with stable service.operation naming and mark rejected requests as errors.
Add pre-initialized, bounded RED metrics for request rate, duration, and errors,
while preserving AuthPermissive behavior and existing rejection handling.
- Around line 145-148: Update the authorization header parsing around
strings.CutPrefix to split the header into scheme and token, then compare the
scheme with strings.EqualFold so Bearer is accepted case-insensitively. Preserve
authInvalid for malformed headers or non-Bearer schemes, and continue returning
the extracted token for valid headers.
In `@src/compute-plane-services/nvsnap/internal/agent/pathsafe_test.go`:
- Around line 162-174: Update the path-safety test around the guarded handler to
table-drive the malicious values across every guarded URL variable: id, hash,
and pod-uid. For each variable and value, set that key in mux.SetURLVars while
keeping the other required variables valid, then assert the handler is not
reached and the response status is http.StatusBadRequest.
---
Outside diff comments:
In `@src/compute-plane-services/nvsnap/internal/agent/agent.go`:
- Around line 448-451: Update the pathVarGuard middleware used by router.Use to
instrument rejected path-variable requests with a bounded structured security
log, a span error attribute, and the existing RED error metric. Record only safe
rejection context—never the supplied identifier or request body—and preserve the
current 400 response behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2aaea338-c1c7-49f7-9055-f84d42339358
📒 Files selected for processing (3)
src/compute-plane-services/nvsnap/internal/agent/agent.gosrc/compute-plane-services/nvsnap/internal/agent/auth.gosrc/compute-plane-services/nvsnap/internal/agent/pathsafe_test.go
CI's BUILD-file check (#491) failed: pathsafe_write_test.go was a new file absent from internal/agent/BUILD.bazel srcs. Adding it to BUILD would work, but the package already pairs one test file per source file and pathsafe_test.go was the obvious home. Same tests, no BUILD churn, and one fewer place to look for coverage of pathsafe.go. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
balajinvda
force-pushed
the
nvsnap-path-hardening
branch
from
July 29, 2026 16:16
d2d49c7 to
7a59a4d
Compare
This was referenced Jul 29, 2026
Two findings from CodeRabbit on #519. Cover every guarded variable, not just "id". The middleware validates id, hash and pod-uid, but the test only exercised id, so deleting either of the other two would have gone unnoticed. Now table-driven across all three keys with the same malicious values, plus a legitimate value per key so a guard that rejected everything could not masquerade as a pass. Confirmed the coverage is real by narrowing the guard to id alone and watching the test fail. Refuse to write through a final-component symlink. joinWithinRoot resolves the deepest existing ancestor, but that check and the subsequent open are separate syscalls, leaving a TOCTOU window. O_NOFOLLOW closes the case an attacker is most likely to win. A swapped parent directory remains theoretically possible. Closing that needs openat2 with RESOLVE_BENEATH, and it is not worth the complexity here: nothing in the fetch path creates a symlink -- every manifest entry lands as a regular file through this exact call -- so an attacker would already need the ability to create symlinks inside the checkpoint directory, which means root on the node, at which point the write path is not the weakest link. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
sbaum1994
approved these changes
Aug 3, 2026
balajinvda
added a commit
that referenced
this pull request
Aug 4, 2026
#519 landed, so this branch retargeted from nvsnap-path-hardening to main. #561 was separately merged into this branch, so it now carries both the agent authentication work and the pod-networking support. Two conflicts, both #519 artifacts: this branch forked before I hardened that PR in review, so it carried the earlier shape of code that has since improved on main. internal/agent/agent.go -- took this branch. Purely additive: main has router.Use(pathVarGuard) from #519, and this branch inserts the RED metrics, outbound-token and auth-guard registrations above it. Verified after resolving that the order is still metrics, then auth, then pathVarGuard. That ordering is load-bearing -- auth must precede path validation, or a malformed {id} is answered with 400 before the caller is authenticated, which tells an unauthenticated client which routes exist. internal/agent/pathsafe_test.go -- took MAIN. This branch has the original that only exercises the "id" route variable; main has the table-driven version from review that runs the same malicious values across id, hash and pod-uid with a positive case per key. Keeping this branch's copy would have silently undone that coverage. Also confirmed #561's contribution survived the merge: AdvertiseIP is still present in agent.go and cascade_fetch.go. Verified: build clean; 13 internal packages pass, 0 fail; helm lint clean; the chart renders 7 wiring points with agent.auth.enabled=true and agent.hostNetwork=false, exercising both features together. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Code scanning raised 39
go/path-injectionalerts across the agent. They are not39 defects: every one is a filesystem call downstream of one of two identifiers
the agent joins onto a host directory without checking.
req.CheckpointIDis decoded straight from an HTTP request body and reachesos.Stat/ReadFile/WriteFile/MkdirAll/RemoveAll/Renameacross
restore.go,cascade_fetch.go,range_fetch.go,fsstore.goandcriu/manager.go.serves over HTTP, joined onto the local destination with no containment check.
The agent runs privileged with hostPath mounts covering
/var/liband thecontainerd root, so a
../in either is a read or write anywhere on the node asroot rather than a contained bug.
The read side was already hardened (
resolveWithinRoot, #92). The write side andthe identifiers themselves were not.
What changed
Closed at the two entry points rather than the 39 sinks.
validPathSegmentrejects an identifier that is not a single, benign pathcomponent. Applied to
Restore,TriggerRestore,EnsureLocaland thegpu-restore handler, and to every
{id}/{hash}/{pod-uid}route throughpathVarGuardrouter middleware. Middleware rather than per-handler checks:the agent has a dozen such routes today, and a per-call-site check is one
forgotten line away from reopening the hole on the next route added.
joinWithinRootis the write-side counterpart toresolveWithinRoot. Itconfines a peer-supplied relative path to the destination without requiring
the target to exist, and closes the two-step variant a lexical check alone
misses: the peer sends a symlink out of the tree, then a file underneath it.
The shape check is deliberately looser than what
buildCheckpointIDemits socheckpoints written by older agents stay readable. The property enforced is
"cannot leave the parent directory", not "matches today's generator".
Customer Release Notes
Not customer visible.
Plan Summary
Not applicable.
Usage
Not applicable.
Testing
internal/agent/pathsafe_write_test.gocovers the validator (accepts the<shorthash>__<timestamp>and pod-UID shapes, rejects.., embeddedseparators, absolute paths, leading dots, NUL, over-length), the middleware
(rejects a bad route var, admits a good one), and
joinWithinRootincluding thesymlink-escape case.
go build ./...andgo test ./internal/...pass. CodeQL re-ran against thischange and moved all 39 alerts to
fixed.The middleware test drives
pathVarGuardwith vars already bound rather thanthrough a URL, deliberately: gorilla/mux 301-redirects
..in a request pathbefore any middleware runs, so a URL-driven test would only prove mux's own path
normalization works, not our guard.
Notes
Split out of #472 so it can be reviewed on its own merits and land without
waiting on that PR's GPU e2e gate. No functional overlap with the criu-v2
migration.
Follow-ups already filed, both marked as production-launch blockers: #486 (the
agent API has no authentication) and #490 (it is bound to every node's IP via
hostNetwork + hostPort). Neither is externally reachable in current deployments.
References
Closes #516
Related Merge Requests/Pull Requests
#472
Dependencies
None.
Summary by CodeRabbit
Security Improvements
Tests