feat(nvsnap): shared-token authentication for the agent API - #555
Open
balajinvda wants to merge 12 commits into
Open
feat(nvsnap): shared-token authentication for the agent API#555balajinvda wants to merge 12 commits into
balajinvda wants to merge 12 commits into
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>
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>
The agent API had no authentication. Any caller reaching the port got the full
control surface of a privileged process: POST /v1/restore, DELETE
/v1/checkpoints/{id}, GET /v1/checkpoints/{id}/file, and /debug/pprof/*, on a
process running privileged with /var/lib and the containerd root bind-mounted.
That is wider than it looks, because the DaemonSet binds it to each node's IP
(hostNetwork + hostPort 8081) rather than a cluster-internal Service, and
NetworkPolicy cannot fence it -- a hostNetwork pod carries node identity, so a
podSelector ingress rule does not match it. Access control has to live in the
request path.
A shared bearer token rather than mTLS. This same router serves the peer
fan-out endpoints that move multi-GB checkpoints, which is the path range
chunking and parallel multi-source fetch exist to speed up. TLS handshakes
amortize with connection reuse; per-byte encryption on the bulk stream does
not. A header comparison costs nothing on either control or transfer requests.
Three modes, because agents and their callers cannot be updated in the same
instant:
disabled (default) no check, so an upgrade without a token behaves
exactly as before -- but the agent logs a warning saying so
permissive check, count and log failures, serve anyway. Operators run here
until nvsnap_agent_auth_total{result="missing|invalid"} reaches
zero, which proves every caller sends a token
required 401
Details worth noting:
- The token is env-only (NVSNAP_AGENT_TOKEN), never a flag: flag values appear
in the pod spec and in `ps`, and this is a credential.
- Comparison is constant-time, so the token cannot be recovered byte by byte
from response timing.
- /health and /metrics are exempt so probes and scraping keep working without
distributing the token to kubelet and Prometheus. /debug/pprof/* is NOT
exempt: profiles expose memory contents and goroutine state.
- An unrecognized --auth-mode fails startup rather than falling back to
disabled. An operator who typos the flag should hear about it immediately,
not discover months later that the API was open.
- nvsnap_agent_auth_total is pre-initialized to zero for all three results so
the series exist on the first scrape and absent() alerts do not misfire.
This is the server half. Callers (nvsnap-server, restore-entrypoint, webhook,
peer agents) do not send the header yet, which is why the default is disabled
and why permissive exists. Chart wiring and the client half follow.
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
Completes the auth work: callers now present the token and the chart
distributes it, so the feature is usable rather than just implemented.
Outbound, agent to peer: authTransport wraps peerHTTPClient's tuned transport
instead of editing call sites. Every cascade and capture-fanout request is
signed, and a peer endpoint added later is authenticated without anyone
remembering to do it -- the same reasoning as pathVarGuard inbound. The token
lives in an atomic package var because peerHTTPClient is built at import time,
long before flags are parsed. An unset token sends no header, so a cluster
running with auth off is byte-for-byte unchanged on the wire.
Outbound, init container to agent: nvsnap-mount-prep reads NVSNAP_AGENT_TOKEN
and attaches it to both the POST and the status poll. The webhook injects that
env from the Secret with optional: true, so a pod admitted before the operator
enables auth still starts.
Chart: agent.auth.{enabled,mode,token}, default off. When enabled the chart
renders a Secret, sets NVSNAP_AGENT_TOKEN on the DaemonSet, and passes
--auth-mode. Two details that matter more than they look:
- The generated token is preserved across upgrades via a lookup of the
existing Secret. `helm upgrade` re-renders every template, so a fresh
randAlphaNum on each upgrade would rotate the credential out from under
running callers and cause a self-inflicted outage mid-rollout.
- helm.sh/resource-policy: keep, so a delete/reinstall cycle does not silently
rotate it either.
Verified by rendering the chart both ways: default produces zero occurrences of
the Secret, the env, or the flag; enabled produces all four wiring points, a
48-character generated token, and honors an explicitly set one. helm lint
passes.
Tests cover the transport signing (and not mutating the caller's request), the
no-token case sending nothing, and a round trip asserting that a request signed
by authTransport is accepted by tokenGuard -- testing the two halves separately
would not catch a format mismatch between them.
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe agent now supports configurable bearer-token authentication, authenticated peer and mount-prep requests, advertised peer addresses, and host or pod network deployment. Helm templates create and distribute tokens and configure local routing and network policies. ChangesAgent security and networking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MountPrep
participant AgentAPI
participant AgentAuth
participant AgentMetrics
MountPrep->>AgentAPI: Request with bearer token
AgentAPI->>AgentAuth: Validate bearer token
AgentAuth->>AgentMetrics: Record authentication result
AgentAuth-->>AgentAPI: Accept or reject request
AgentAPI-->>MountPrep: HTTP response
Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Four findings from CodeRabbit.
Fail closed when required has no token. tokenGuard returned nil for
AuthRequired with an empty token, so Agent.Run skipped the middleware and
served the privileged API unauthenticated. Startup already rejected that
combination, but a security primitive that silently becomes a no-op when
misconfigured is the wrong shape: any future caller building a guard without
going through main() would open the API and nothing would say so. Now returns a
deny-all guard, logs why, and keeps /health and /metrics working so the
operator sees the misconfiguration rather than a crashloop.
Authenticate before validating. gorilla/mux runs middleware in registration
order, and pathVarGuard was registered first, so a malformed {id} got a 400
before the caller was authenticated -- telling an unauthenticated client which
routes exist and how their variables are shaped. Swapped.
Accept the Bearer scheme case-insensitively. RFC 7235 makes the scheme
case-insensitive, so "bearer <token>" is a valid credential a conforming client
may send and we were rejecting it. Compared with EqualFold; the token itself
stays a byte-exact constant-time compare.
RED metrics on the agent API. metrics.InstrumentRoute already existed and was
wired only to nvsnap-server, so the agent's API had no rate, error or duration
series at all. Now registered outermost on the agent router, so it also
observes requests the auth guard rejects -- a spike of 401s is exactly what the
permissive-to-required rollout needs to watch. Keyed on the route template, not
the concrete path, so checkpoint IDs never become label values.
The shared rate/duration pair is now registered through its own sync.Once,
since both RegisterAgent and RegisterServer reference it and
prometheus.MustRegister panics on a duplicate. A test pins that calling both,
twice, does not panic.
Not adopting the per-request OpenTelemetry span from the same comment. This
router serves the peer file-transfer endpoints, where a large fetch is
hundreds of parallel range requests; a span each would add real overhead and
cardinality to the exact path we chose a header check over mTLS to keep fast.
The route-level RED metrics give the rate, error and duration signal, and the
existing operation spans still cover checkpoint and restore. Happy to add
inbound spans scoped to the control endpoints if reviewers want them.
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
sbaum1994
approved these changes
Aug 3, 2026
Co-authored-by: balaji <balaji7@gmail.com>
#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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go (1)
202-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for the new agent configuration.
The existing webhook test does not cover
NVSNAP_AGENT_TOKEN,AgentBaseURL, trailing-slash normalization, or the Secret reference. Add these tests, add ago_testtarget fornvsnap-mount-prep, and record the native Bazel test result.🤖 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/cmd/nvsnap-mount-prep/main.go` around lines 202 - 263, Add focused tests covering NVSNAP_AGENT_TOKEN handling in setAgentAuth/getStatus, AgentBaseURL trailing-slash normalization, and Secret reference propagation across src/compute-plane-services/nvsnap/internal/agent/webhook_integration.go:90-95, src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go:93-132, and src/compute-plane-services/nvsnap/internal/webhook/mutate.go:354-361. Add a go_test target for nvsnap-mount-prep, run the native Bazel test, and record its result; the main.go site requires no direct production change.Sources: Coding guidelines, Path instructions
src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go (1)
125-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the validation error on the span.
Line 128 sets failure status but does not call
span.RecordError(err). Record the error before setting status so trace exporters retain the failure event and error attributes.Proposed change
if err := validPathSegment("checkpoint id", checkpointID); err != nil { + span.RecordError(err) span.SetStatus(codes.Error, err.Error()) return err }As per path instructions, set error attributes 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/cascade_fetch.go` around lines 125 - 129, Update the checkpointID validation failure branch in cascade fetch to call span.RecordError(err) before span.SetStatus(codes.Error, err.Error()), preserving the existing return behavior and ensuring the validation error is retained with its attributes.Source: Path instructions
src/compute-plane-services/nvsnap/internal/agent/pathsafe_test.go (1)
195-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert rejection for unsafe manifest paths.
Lines 196-201 pass when
joinWithinRootnormalizes a traversal or absolute path underroot. The production call sites rely on an error to reject invalid manifest entries. Asserterr != nilfor every unsafe path.Proposed change
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 := joinWithinRoot(root, rel); err == nil { + t.Errorf("joinWithinRoot(%q) accepted unsafe path", rel) } }🤖 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_test.go` around lines 195 - 201, Update the unsafe-path loop in the joinWithinRoot test to require err != nil for every traversal and absolute manifest path, rather than accepting normalized results under root. Keep the existing unsafe path cases and ensure each rejected input fails through the error contract used by production callers.
🤖 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/cmd/nvsnap-mount-prep/main.go`:
- Around line 252-263: When NVSNAP_AGENT_TOKEN is configured, enforce
authenticated TLS transport and fail closed for any http:// agent URL; update
setAgentAuth in src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go
to require HTTPS with certificate validation, preferably mTLS, and apply the
same URL/security requirement to the Helm Service URL in
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml
(lines 91-96) and the default URL in
src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go (lines
93-132).
In
`@src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yaml`:
- Around line 20-27: Update the token selection logic in the agent-token Secret
template to avoid generating a random token when lookup cannot access the
cluster and returns empty. Require an explicit managed token for offline
rendering, or integrate an external Secret workflow that preserves the existing
value; retain lookup reuse when cluster access is available and explicit token
handling unchanged.
- Around line 29-44: Distribute the token Secret to every configured restore
namespace, preserving the same token and resource-policy behavior from
agent-token-secret.yaml. In mount_prep_init.go, update the
authentication-enabled SecretKeyRef to use the restore pod’s namespace-local
token Secret instead of the release-namespace Secret, while preserving optional
authentication behavior when auth is disabled.
In `@src/compute-plane-services/nvsnap/internal/agent/agent.go`:
- Around line 466-481: Configure the agent listener and peer URL advertisement
in src/compute-plane-services/nvsnap/internal/agent/agent.go:466-481 to use TLS
and https:// consistently, while preserving tokenGuard authentication ordering
and SetOutboundToken behavior. In
src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go:98-110 and
578-594, validate redirect destinations and reject plaintext or otherwise unsafe
URLs before attaching the Authorization header or sending requests.
In `@src/compute-plane-services/nvsnap/internal/agent/auth_test.go`:
- Around line 80-85: Update TestTokenGuardPermissiveServesButCounts to capture
the missing-result authentication metric before calling served, then read it
afterward and assert it increased by exactly one while preserving the existing
response assertions.
In `@src/compute-plane-services/nvsnap/internal/agent/auth.go`:
- Around line 191-197: Update authTransport.RoundTrip to add the bearer token
only when r.URL uses HTTPS; leave non-HTTPS requests unauthenticated while
preserving the existing clone-before-modify behavior and header checks.
- Around line 191-202: Update authTransport.RoundTrip to prevent bearer-token
injection on untrusted cross-origin redirects: validate the request destination
against the trusted origin before setting authHeader, or disable redirect
following in peerHTTPClient. Preserve authorization for trusted requests while
ensuring redirected requests to other origins are not given the bearer token.
In `@src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go`:
- Around line 579-594: Update selfAgentURL to construct the authority with
net.JoinHostPort(ip, port), preserving the existing IP and port selection
behavior and producing valid IPv6 URLs. Add an IPv6 assertion to
TestSelfAgentURL_DerivesPort covering the bracketed host-port result.
In `@src/compute-plane-services/nvsnap/internal/agent/restore.go`:
- Around line 218-220: Update the checkpoint ID validation handling in both
restoreHandler and triggerRestoreHandler so errors returned by
validPathSegment("checkpointId", ...) produce HTTP 400 via http.StatusBadRequest
instead of HTTP 500. Preserve existing handling for other error types and normal
requests.
---
Nitpick comments:
In `@src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go`:
- Around line 202-263: Add focused tests covering NVSNAP_AGENT_TOKEN handling in
setAgentAuth/getStatus, AgentBaseURL trailing-slash normalization, and Secret
reference propagation across
src/compute-plane-services/nvsnap/internal/agent/webhook_integration.go:90-95,
src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go:93-132,
and src/compute-plane-services/nvsnap/internal/webhook/mutate.go:354-361. Add a
go_test target for nvsnap-mount-prep, run the native Bazel test, and record its
result; the main.go site requires no direct production change.
In `@src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go`:
- Around line 125-129: Update the checkpointID validation failure branch in
cascade fetch to call span.RecordError(err) before span.SetStatus(codes.Error,
err.Error()), preserving the existing return behavior and ensuring the
validation error is retained with its attributes.
In `@src/compute-plane-services/nvsnap/internal/agent/pathsafe_test.go`:
- Around line 195-201: Update the unsafe-path loop in the joinWithinRoot test to
require err != nil for every traversal and absolute manifest path, rather than
accepting normalized results under root. Keep the existing unsafe path cases and
ensure each rejected input fails through the error contract used by production
callers.
🪄 Autofix
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: e4b70cbc-c3c7-4a1f-939a-0ce3cc332f26
📒 Files selected for processing (21)
src/compute-plane-services/nvsnap/cmd/agent/main.gosrc/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.gosrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yamlsrc/compute-plane-services/nvsnap/internal/agent/BUILD.bazelsrc/compute-plane-services/nvsnap/internal/agent/advertise_test.gosrc/compute-plane-services/nvsnap/internal/agent/agent.gosrc/compute-plane-services/nvsnap/internal/agent/auth.gosrc/compute-plane-services/nvsnap/internal/agent/auth_test.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_test.gosrc/compute-plane-services/nvsnap/internal/agent/restore.gosrc/compute-plane-services/nvsnap/internal/agent/webhook_integration.gosrc/compute-plane-services/nvsnap/internal/metrics/BUILD.bazelsrc/compute-plane-services/nvsnap/internal/metrics/metrics.gosrc/compute-plane-services/nvsnap/internal/metrics/register_test.gosrc/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.gosrc/compute-plane-services/nvsnap/internal/webhook/mutate.go
Comment on lines
+252
to
+263
|
|
||
| // setAgentAuth attaches the agent API bearer token when one is configured. | ||
| // Empty is the normal state until the operator turns auth on, and sending no | ||
| // header is exactly what a disabled or permissive agent expects. See GH #486. | ||
| func setAgentAuth(r *http.Request) { | ||
| if r == nil { | ||
| return | ||
| } | ||
| if tok := os.Getenv("NVSNAP_AGENT_TOKEN"); tok != "" { | ||
| r.Header.Set("Authorization", "Bearer "+tok) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
for f in \
src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go \
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml \
src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go
do
echo "--- $f ---"
sed -n '1,340p' "$f" | grep -n -E -C 8 \
'setAgentAuth|NVSNAP_AGENT_TOKEN|NVSNAP_AGENT_URL|AgentBaseURL|webhook-agent-base-url|Authorization|http://|https://'
done
printf '%s\n' '--- related agent auth and transport references ---'
rg -n -S -g '*.go' -g '*.yaml' -g '*.yml' -g '*.tpl' \
'NVSNAP_AGENT_TOKEN|AgentTokenSecret|Authorization|Bearer|agent.*(TLS|HTTPS|http://)|webhook-agent-base-url|internalTrafficPolicy|hostNetwork|hostPort' \
src/compute-plane-services/nvsnap
printf '%s\n' '--- relevant manifests and tests ---'
fd -i 'agent|mount_prep|nvsnap-mount-prep|webhook' src/compute-plane-services/nvsnap | head -120Repository: NVIDIA/nvcf
Length of output: 27143
🏁 Script executed:
#!/bin/bash
set -eu
for f in \
src/compute-plane-services/nvsnap/internal/agent/auth.go \
src/compute-plane-services/nvsnap/internal/agent/agent.go \
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml \
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml \
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yaml \
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yaml
do
echo "--- $f ---"
wc -l "$f"
done
printf '%s\n' '--- auth middleware and outbound calls ---'
sed -n '1,250p' src/compute-plane-services/nvsnap/internal/agent/auth.go
sed -n '450,500p' src/compute-plane-services/nvsnap/internal/agent/agent.go
sed -n '1,180p' src/compute-plane-services/nvsnap/internal/agent/webhook_integration.go
printf '%s\n' '--- restore-pod network policy ---'
sed -n '80,155p' src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml
printf '%s\n' '--- agent service and token secret ---'
sed -n '420,465p' src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml
sed -n '1,100p' src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yaml
printf '%s\n' '--- auth and URL values ---'
sed -n '170,220p' src/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yaml
sed -n '470,510p' src/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yaml
rg -n -S 'auth:|mode:|enabled:|agent-token|NVSNAP_WEBHOOK_AGENT_BASE_URL|AgentBaseURL|webhook-agent-base-url' \
src/compute-plane-services/nvsnap/deploy/helm/nvsnapRepository: NVIDIA/nvcf
Length of output: 34614
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal
Protect NVSNAP_AGENT_TOKEN with authenticated transport.
When NVSNAP_AGENT_TOKEN is set, fail closed for http:// agent URLs. Use HTTPS with certificate validation, preferably mTLS, for setAgentAuth, the Helm Service URL, and the default URL in mount_prep_init.go.
📍 Affects 3 files
src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go#L252-L263(this comment)src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml#L91-L96src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go#L93-L132
🤖 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/cmd/nvsnap-mount-prep/main.go` around lines
252 - 263, When NVSNAP_AGENT_TOKEN is configured, enforce authenticated TLS
transport and fail closed for any http:// agent URL; update setAgentAuth in
src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go to require HTTPS
with certificate validation, preferably mTLS, and apply the same URL/security
requirement to the Helm Service URL in
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml
(lines 91-96) and the default URL in
src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go (lines
93-132).
Comment on lines
+20
to
+27
| {{- $existing := lookup "v1" "Secret" $ns $name }} | ||
| {{- $token := "" }} | ||
| {{- if .Values.agent.auth.token }} | ||
| {{- $token = .Values.agent.auth.token | b64enc }} | ||
| {{- else if and $existing $existing.data $existing.data.token }} | ||
| {{- $token = $existing.data.token }} | ||
| {{- else }} | ||
| {{- $token = randAlphaNum 48 | b64enc }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not generate a token when lookup has no cluster access.
Client-side helm template renders lookup as empty. randAlphaNum then creates a new token on every render. Applying the rendered manifests updates the Secret, while running agents retain the old environment value and new mount-prep pods use the new value. Required authentication then fails until the agent rolls.
Require an explicit managed token for offline rendering, or use an external Secret workflow that preserves the value outside this template.
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 25-25: too many spaces after hyphen
(hyphens)
[warning] 27-27: too many spaces after hyphen
(hyphens)
🤖 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/deploy/helm/nvsnap/templates/agent-token-secret.yaml`
around lines 20 - 27, Update the token selection logic in the agent-token Secret
template to avoid generating a random token when lookup cannot access the
cluster and returns empty. Require an explicit managed token for offline
rendering, or integrate an external Secret workflow that preserves the existing
value; retain lookup reuse when cluster access is available and explicit token
handling unchanged.
Comment on lines
+29
to
+44
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: {{ $name }} | ||
| namespace: {{ $ns }} | ||
| labels: | ||
| app.kubernetes.io/name: nvsnap | ||
| app.kubernetes.io/part-of: nvsnap | ||
| annotations: | ||
| # helm.sh/resource-policy keeps the Secret if the release is removed with | ||
| # --keep-history style workflows; without it a delete/reinstall cycle | ||
| # silently rotates the token. | ||
| helm.sh/resource-policy: keep | ||
| type: Opaque | ||
| data: | ||
| token: {{ $token }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Distribute the token to every restore namespace.
Kubernetes resolves SecretKeyRef only in the mutated pod’s namespace. The chart creates nvsnap-agent-token only in the release namespace. If agent.auth.enabled=true, agent.auth.mode=required, and a restore pod runs in another configured restore namespace, the optional reference omits NVSNAP_AGENT_TOKEN. Mount-prep then sends no header and the agent returns 401.
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yaml#L29-L44: create or reference the same token Secret in each restore namespace.src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go#L123-L132: reference the namespace-local token Secret when authentication is enabled.
📍 Affects 2 files
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yaml#L29-L44(this comment)src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go#L123-L132
🤖 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/deploy/helm/nvsnap/templates/agent-token-secret.yaml`
around lines 29 - 44, Distribute the token Secret to every configured restore
namespace, preserving the same token and resource-policy behavior from
agent-token-secret.yaml. In mount_prep_init.go, update the
authentication-enabled SecretKeyRef to use the restore pod’s namespace-local
token Secret instead of the release-namespace Secret, while preserving optional
authentication behavior when auth is disabled.
Comment on lines
+466
to
+481
| // Present the token on our own peer calls too. Set unconditionally: an | ||
| // agent in permissive mode still has peers that may already require it. | ||
| SetOutboundToken(a.config.AuthToken) | ||
|
|
||
| // Order matters: gorilla/mux runs middleware in registration order, so | ||
| // auth is registered FIRST. Otherwise pathVarGuard answers a malformed | ||
| // {id} with 400 before the caller is authenticated, telling an | ||
| // unauthenticated client which routes exist and how their variables are | ||
| // shaped. Authenticate, then validate. | ||
| if guard := tokenGuard(a.config.AuthMode, a.config.AuthToken, a.log); guard != nil { | ||
| router.Use(guard) | ||
| a.log.WithField("mode", a.config.AuthMode).Info("Agent API authentication enabled") | ||
| } else { | ||
| a.log.Warn("Agent API is UNAUTHENTICATED: set NVSNAP_AGENT_TOKEN and " + | ||
| "--auth-mode to require a bearer token (GH #486)") | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
files=(
"src/compute-plane-services/nvsnap/internal/agent/agent.go"
"src/compute-plane-services/nvsnap/internal/agent/auth.go"
"src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go"
"src/compute-plane-services/nvsnap/cmd/agent/main.go"
)
for f in "${files[@]}"; do
if [ -f "$f" ]; then
echo "===== $f ====="
wc -l "$f"
else
echo "MISSING $f"
fi
done
echo "===== agent.go server startup and listener calls ====="
rg -n -C 8 'ListenAndServe|ServeTLS|TLSConfig|http\.Server|Listen\(' \
src/compute-plane-services/nvsnap/internal/agent/agent.go \
src/compute-plane-services/nvsnap/cmd/agent/main.go
echo "===== auth.go ====="
cat -n src/compute-plane-services/nvsnap/internal/agent/auth.go
echo "===== cascade_fetch.go transport, client, redirects, URL construction ====="
rg -n -C 12 'authTransport|Authorization|Bearer|http\.Client|CheckRedirect|selfAgentURL|CatalogURL|BlobStoreURL|url\.Parse|http://' \
src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.goRepository: NVIDIA/nvcf
Length of output: 25215
🏁 Script executed:
#!/bin/bash
set -eu
echo "===== agent startup and configuration ====="
sed -n '600,645p' src/compute-plane-services/nvsnap/internal/agent/agent.go
rg -n -C 8 'AuthToken|AuthMode|ListenAddr|TLS|ServeTLS|https?://' \
src/compute-plane-services/nvsnap/cmd/agent/main.go \
src/compute-plane-services/nvsnap/internal/agent \
--glob '*.go'
echo "===== deployment and chart configuration ====="
rg -n -C 6 'NVSNAP_AGENT_TOKEN|auth-mode|hostNetwork|hostPort|8081|tls|TLS|ListenAddr|advertise' \
--glob '*.yaml' --glob '*.yml' --glob '*.tpl' --glob '*.json' --glob '*.md' \
. | head -n 800
echo "===== all peer client behavior ====="
rg -n -C 15 'peerHTTPClient\.Do|downloadToFile|CheckRedirect|Redirect|selfAgentURL|AgentURL|BlobURI' \
src/compute-plane-services/nvsnap/internal/agent --glob '*.go'
echo "===== focused source assertions ====="
python3 - <<'PY'
from pathlib import Path
agent = Path("src/compute-plane-services/nvsnap/internal/agent/agent.go").read_text()
auth = Path("src/compute-plane-services/nvsnap/internal/agent/auth.go").read_text()
cascade = Path("src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go").read_text()
checks = {
"listener uses ListenAndServe": "a.server.ListenAndServe()" in agent,
"listener does not use ServeTLS": "ServeTLS(" not in agent,
"peer URL is HTTP": 'fmt.Sprintf("http://%s:%s", ip, port)' in cascade,
"auth transport adds bearer header": 'r.Header.Set(authHeader, "Bearer "+*tok)' in auth,
"no redirect policy on peer client": "CheckRedirect" not in cascade,
}
for name, ok in checks.items():
print(f"{name}: {ok}")
raise SystemExit(0 if all(checks.values()) else 1)
PYRepository: NVIDIA/nvcf
Length of output: 50370
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Exploitability: Moderate
Reachability path
● Entry
src/compute-plane-services/nvsnap/internal/agent/auth_test.go:44
TestTokenGuardRequired: A prefix of the real token must not pass: constant-time compare
│
▼
● Sink
src/compute-plane-services/nvsnap/internal/agent/agent.go
Use authenticated encryption for every bearer-token hop.
The agent uses ListenAndServe, advertises http:// peer URLs, and adds the bearer token to outbound requests. Configure TLS for the listener, advertise matching https:// URLs, and reject plaintext or unsafe redirect destinations before sending Authorization.
📍 Affects 2 files
src/compute-plane-services/nvsnap/internal/agent/agent.go#L466-L481(this comment)src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go#L98-L110src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go#L578-L594
🤖 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 466 -
481, Configure the agent listener and peer URL advertisement in
src/compute-plane-services/nvsnap/internal/agent/agent.go:466-481 to use TLS and
https:// consistently, while preserving tokenGuard authentication ordering and
SetOutboundToken behavior. In
src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go:98-110 and
578-594, validate redirect destinations and reject plaintext or otherwise unsafe
URLs before attaching the Authorization header or sending requests.
Comment on lines
+80
to
+85
| func TestTokenGuardPermissiveServesButCounts(t *testing.T) { | ||
| ran, code := served(t, AuthPermissive, "tok", "", "/v1/restore") | ||
| if !ran || code != http.StatusOK { | ||
| t.Errorf("permissive rejected an unauthenticated request: ran=%v code=%d", ran, code) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the authentication metric increment.
This test verifies only that permissive mode serves the request. It does not verify the missing result counter.
Record the counter before and after the request. Assert that the value increases by one.
🤖 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/auth_test.go` around lines
80 - 85, Update TestTokenGuardPermissiveServesButCounts to capture the
missing-result authentication metric before calling served, then read it
afterward and assert it increased by exactly one while preserving the existing
response assertions.
Comment on lines
+191
to
+197
| func (t *authTransport) RoundTrip(r *http.Request) (*http.Response, error) { | ||
| tok := outboundToken.Load() | ||
| if tok != nil && *tok != "" && r.Header.Get(authHeader) == "" { | ||
| // RoundTrip must not modify the request it is given. | ||
| r = r.Clone(r.Context()) | ||
| r.Header.Set(authHeader, "Bearer "+*tok) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="src/compute-plane-services/nvsnap/internal/agent/auth.go"
printf '%s\n' '--- auth.go relevant sections ---'
sed -n '1,230p' "$file"
printf '%s\n' '--- authTransport and HTTP client call sites ---'
rg -n -C 3 'authTransport|http\.Client|SetOutboundToken|selfAgentURL|http://|https://' \
src/compute-plane-services/nvsnap/internal/agent \
src/compute-plane-services/nvsnap/cmd/agent
printf '%s\n' '--- auth tests around RoundTrip ---'
sed -n '160,240p' src/compute-plane-services/nvsnap/internal/agent/auth_test.goRepository: NVIDIA/nvcf
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- peer URL construction and fetches ---'
rg -n -C 5 'peerAgentURL|selfAgentURL|http\.NewRequest|client\.Do|http\.Get|http://%s|http://<peer' \
src/compute-plane-services/nvsnap/internal/agent/capture_cascade.go \
src/compute-plane-services/nvsnap/internal/agent/peer_fanout.go \
src/compute-plane-services/nvsnap/internal/agent/agent.go \
src/compute-plane-services/nvsnap/internal/agent/advertise.go 2>/dev/null || true
printf '%s\n' '--- scheme and transport validation ---'
rg -n -C 3 'Scheme|url\.Parse|https|TLSClientConfig|Transport:' \
src/compute-plane-services/nvsnap/internal/agent \
-g '*.go' | rg -v 'LICENSE|https://www.apache.org'Repository: NVIDIA/nvcf
Length of output: 8543
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal
Reachability path
● Entry
src/compute-plane-services/nvsnap/internal/agent/advertise_test.go:14
TestSelfAgentURL: Pod networking: peers must dial the pod IP; the node IP would
│
▼
● Sink
src/compute-plane-services/nvsnap/internal/agent/auth.go
Require HTTPS before adding NVSNAP_AGENT_TOKEN.
Peer URLs use http://<peerInternalIP>:<port>, so authTransport can expose the bearer token to a network observer. Reject non-HTTPS URLs before setting Authorization, or configure authenticated TLS.
🤖 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/auth.go` around lines 191 -
197, Update authTransport.RoundTrip to add the bearer token only when r.URL uses
HTTPS; leave non-HTTPS requests unauthenticated while preserving the existing
clone-before-modify behavior and header checks.
Comment on lines
+191
to
+202
| func (t *authTransport) RoundTrip(r *http.Request) (*http.Response, error) { | ||
| tok := outboundToken.Load() | ||
| if tok != nil && *tok != "" && r.Header.Get(authHeader) == "" { | ||
| // RoundTrip must not modify the request it is given. | ||
| r = r.Clone(r.Context()) | ||
| r.Header.Set(authHeader, "Bearer "+*tok) | ||
| } | ||
| base := t.base | ||
| if base == nil { | ||
| base = http.DefaultTransport | ||
| } | ||
| return base.RoundTrip(r) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the client and inspect its redirect policy and destination construction.
ast-grep outline src/compute-plane-services/nvsnap/internal/agent --items all \
--type function,variable --match 'peerHTTPClient|authTransport'
rg -n -C5 --type=go \
'\bauthTransport\b|\bpeerHTTPClient\b|\bCheckRedirect\b|\bLocation\b' \
src/compute-plane-services/nvsnapRepository: NVIDIA/nvcf
Length of output: 21850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the shared client configuration, every URL source passed to it,
# and the peer discovery/configuration path that determines whether a
# cross-origin redirect is possible.
sed -n '80,125p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go
sed -n '300,575p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go
rg -n -C6 --type=go \
'\b(peerURL|CatalogURL|blobBaseURL|peerHTTPClient|selfAgentURL|Peers|peer)\b' \
src/compute-plane-services/nvsnap/internal/agent
rg -n -C8 --type=go \
'http\.Client\{|CheckRedirect|Redirect|Location|Do\(req\)' \
src/compute-plane-services/nvsnapRepository: NVIDIA/nvcf
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '80,125p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go
sed -n '300,575p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go
rg -n -C6 --type=go \
'\b(peerURL|CatalogURL|blobBaseURL|peerHTTPClient|selfAgentURL|Peers|peer)\b' \
src/compute-plane-services/nvsnap/internal/agent
rg -n -C8 --type=go \
'http\.Client\{|CheckRedirect|Redirect|Location|Do\(req\)' \
src/compute-plane-services/nvsnapRepository: NVIDIA/nvcf
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- shared client ---'
sed -n '97,116p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go
printf '%s\n' '--- peer URL construction ---'
sed -n '575,600p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go
sed -n '404,424p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go
sed -n '446,535p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go
printf '%s\n' '--- redirect policy in the nvsnap tree ---'
rg -n -C3 --type=go 'CheckRedirect|ErrUseLastResponse|http\.Client\s*\{' \
src/compute-plane-services/nvsnap || true
printf '%s\n' '--- peer/catalog URL validation ---'
rg -n -C5 --type=go \
'peer-add|agent_url|BlobURI|blob_uri|AgentURL' \
src/compute-plane-services/nvsnap | head -n 500
printf '%s\n' '--- Go redirect header behavior source ---'
if command -v go >/dev/null 2>&1; then
goroot="$(go env GOROOT)"
rg -n -C8 \
'shouldCopyHeaderOnRedirect|Authorization|sensitiveHeaders' \
"$goroot/src/net/http/client.go" "$goroot/src/net/http/transport.go" | head -n 300
else
printf '%s\n' 'go command unavailable'
fiRepository: NVIDIA/nvcf
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '97,116p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go
sed -n '575,600p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go
rg -n -C3 --type=go 'CheckRedirect|ErrUseLastResponse|http\.Client\s*\{' \
src/compute-plane-services/nvsnap || true
rg -n -C5 --type=go \
'peer-add|agent_url|BlobURI|blob_uri|AgentURL' \
src/compute-plane-services/nvsnap | head -n 500Repository: NVIDIA/nvcf
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- peer registration validation ---'
rg -n --type=go 'type peerRegisterRequest|func \(s \*Server\) peerAddCheckpoint|func \(s \*Server\) getCheckpointSources|func \(s \*Server\) blobUploaded' \
src/compute-plane-services/nvsnap/internal/server
sed -n '1,140p' src/compute-plane-services/nvsnap/internal/server/sources.go 2>/dev/null || true
printf '%s\n' '--- standard-library redirect handling ---'
if command -v go >/dev/null 2>&1; then
goroot="$(go env GOROOT)"
printf 'GOROOT=%s\n' "$goroot"
rg -n -C10 \
'shouldCopyHeaderOnRedirect|Authorization|sensitiveHeaders' \
"$goroot/src/net/http/client.go" | head -n 160
else
printf '%s\n' 'go command unavailable'
fiRepository: NVIDIA/nvcf
Length of output: 10110
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: Internal · Exploitability: Moderate
Reachability path
● Entry
src/compute-plane-services/nvsnap/internal/agent/advertise_test.go:14
TestSelfAgentURL: Pod networking: peers must dial the pod IP; the node IP would
│
▼
● Sink
src/compute-plane-services/nvsnap/internal/agent/auth.go
Restrict authTransport redirects to trusted origins. peerHTTPClient follows redirects, and authTransport.RoundTrip restores Authorization after net/http removes it for a cross-origin redirect. Disable redirects or validate the destination before adding the bearer token.
🤖 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/auth.go` around lines 191 -
202, Update authTransport.RoundTrip to prevent bearer-token injection on
untrusted cross-origin redirects: validate the request destination against the
trusted origin before setting authHeader, or disable redirect following in
peerHTTPClient. Preserve authorization for trusted requests while ensuring
redirected requests to other origins are not given the bearer token.
Comment on lines
+579
to
+594
| // AdvertiseIP first: under pod networking peers must dial the pod IP, | ||
| // since the node IP only resolves to us via hostPort (GH #490). Falls | ||
| // back to NodeIP so a deployment that sets neither, or only the older | ||
| // value, keeps working. | ||
| ip := a.config.AdvertiseIP | ||
| if ip == "" { | ||
| ip = a.config.NodeIP | ||
| } | ||
| if ip == "" { | ||
| return "" | ||
| } | ||
| port := "8081" | ||
| if addr := a.config.ListenAddr; len(addr) > 1 && addr[0] == ':' { | ||
| port = addr[1:] | ||
| } | ||
| return fmt.Sprintf("http://%s:%s", a.config.NodeIP, port) | ||
| return fmt.Sprintf("http://%s:%s", ip, port) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -C 12 'TestSelfAgentURL|SelfAgentURL|AdvertiseIP|ListenAddr|NodeIP' src/compute-plane-services/nvsnap/internal/agentRepository: NVIDIA/nvcf
Length of output: 41846
🏁 Script executed:
sed -n '1,45p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go
sed -n '520,610p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go
sed -n '415,445p' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch_test.go
python3 - <<'PY'
from urllib.parse import urlsplit
cases = [
("10.0.0.1", "8081"),
("2001:db8::1", "8081"),
]
for host, port in cases:
current = f"http://{host}:{port}"
fixed = f"http://[{host}]:{port}"
print(f"host={host!r}")
for label, value in [("current", current), ("joined", fixed)]:
parsed = urlsplit(value)
try:
hostname = parsed.hostname
except ValueError as exc:
hostname = f"ERROR: {exc}"
print(label, value, "hostname=", hostname, "port=", end=" ")
try:
print(parsed.port)
except ValueError as exc:
print(f"ERROR: {exc}")
PYRepository: NVIDIA/nvcf
Length of output: 5769
🏁 Script executed:
rg -n -C 8 'peerURL|alternateURLs|AgentURL|agent_url|http\.NewRequest|peerHTTPClient' src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go src/compute-plane-services/nvsnap/internal/agent/*_test.go
python3 - <<'PY'
from urllib.parse import urlsplit
def join_host_port(host, port):
# net.JoinHostPort's relevant output for IP literals:
if ":" in host and not (host.startswith("[") and host.endswith("]")):
host = f"[{host}]"
return f"{host}:{port}"
for host, port in [("10.0.0.1", "8081"), ("2001:db8::1", "8081")]:
current = f"http://{host}:{port}"
joined = f"http://{join_host_port(host, port)}"
print(f"host={host!r}")
for label, value in [("current", current), ("joined", joined)]:
print(label, value)
try:
parsed = urlsplit(value)
print(" hostname=", parsed.hostname, "port=", parsed.port)
except ValueError as exc:
print(" parse error:", exc)
PYRepository: NVIDIA/nvcf
Length of output: 50367
🏁 Script executed:
python3 - <<'PY'
from urllib.parse import urlsplit
def go_join_host_port(host, port):
# For the IP literals under review, this matches net.JoinHostPort:
if ":" in host:
host = "[" + host + "]"
return host + ":" + port
for host, port in [("10.0.0.1", "8081"), ("2001:db8::1", "8081")]:
current = "http://" + host + ":" + port
joined = "http://" + go_join_host_port(host, port)
print("host:", host)
for label, value in (("current", current), ("joined", joined)):
print(label + ":", value)
try:
parsed = urlsplit(value)
print(" hostname:", parsed.hostname)
try:
print(" port:", parsed.port)
except ValueError as exc:
print(" port error:", exc)
except ValueError as exc:
print(" parse error:", exc)
PYRepository: NVIDIA/nvcf
Length of output: 501
Use net.JoinHostPort to build the peer authority.
selfAgentURL returns http://2001:db8::1:8081 for an IPv6 address. Return "http://" + net.JoinHostPort(ip, port) and add an IPv6 case to TestSelfAgentURL_DerivesPort.
🤖 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 579 - 594, Update selfAgentURL to construct the authority with
net.JoinHostPort(ip, port), preserving the existing IP and port selection
behavior and producing valid IPv6 URLs. Add an IPv6 assertion to
TestSelfAgentURL_DerivesPort covering the bracketed host-port result.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
🧹 Nitpick comments (3)
src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go (1)
202-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for the new agent configuration.
The existing webhook test does not cover
NVSNAP_AGENT_TOKEN,AgentBaseURL, trailing-slash normalization, or the Secret reference. Add these tests, add ago_testtarget fornvsnap-mount-prep, and record the native Bazel test result.🤖 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/cmd/nvsnap-mount-prep/main.go` around lines 202 - 263, Add focused tests covering NVSNAP_AGENT_TOKEN handling in setAgentAuth/getStatus, AgentBaseURL trailing-slash normalization, and Secret reference propagation across src/compute-plane-services/nvsnap/internal/agent/webhook_integration.go:90-95, src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go:93-132, and src/compute-plane-services/nvsnap/internal/webhook/mutate.go:354-361. Add a go_test target for nvsnap-mount-prep, run the native Bazel test, and record its result; the main.go site requires no direct production change.Sources: Coding guidelines, Path instructions
src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go (1)
125-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the validation error on the span.
Line 128 sets failure status but does not call
span.RecordError(err). Record the error before setting status so trace exporters retain the failure event and error attributes.Proposed change
if err := validPathSegment("checkpoint id", checkpointID); err != nil { + span.RecordError(err) span.SetStatus(codes.Error, err.Error()) return err }As per path instructions, set error attributes 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/cascade_fetch.go` around lines 125 - 129, Update the checkpointID validation failure branch in cascade fetch to call span.RecordError(err) before span.SetStatus(codes.Error, err.Error()), preserving the existing return behavior and ensuring the validation error is retained with its attributes.Source: Path instructions
src/compute-plane-services/nvsnap/internal/agent/pathsafe_test.go (1)
195-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert rejection for unsafe manifest paths.
Lines 196-201 pass when
joinWithinRootnormalizes a traversal or absolute path underroot. The production call sites rely on an error to reject invalid manifest entries. Asserterr != nilfor every unsafe path.Proposed change
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 := joinWithinRoot(root, rel); err == nil { + t.Errorf("joinWithinRoot(%q) accepted unsafe path", rel) } }🤖 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_test.go` around lines 195 - 201, Update the unsafe-path loop in the joinWithinRoot test to require err != nil for every traversal and absolute manifest path, rather than accepting normalized results under root. Keep the existing unsafe path cases and ensure each rejected input fails through the error contract used by production callers.
🤖 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/cmd/nvsnap-mount-prep/main.go`:
- Around line 252-263: When NVSNAP_AGENT_TOKEN is configured, enforce
authenticated TLS transport and fail closed for any http:// agent URL; update
setAgentAuth in src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go
to require HTTPS with certificate validation, preferably mTLS, and apply the
same URL/security requirement to the Helm Service URL in
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml
(lines 91-96) and the default URL in
src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go (lines
93-132).
In
`@src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yaml`:
- Around line 20-27: Update the token selection logic in the agent-token Secret
template to avoid generating a random token when lookup cannot access the
cluster and returns empty. Require an explicit managed token for offline
rendering, or integrate an external Secret workflow that preserves the existing
value; retain lookup reuse when cluster access is available and explicit token
handling unchanged.
- Around line 29-44: Distribute the token Secret to every configured restore
namespace, preserving the same token and resource-policy behavior from
agent-token-secret.yaml. In mount_prep_init.go, update the
authentication-enabled SecretKeyRef to use the restore pod’s namespace-local
token Secret instead of the release-namespace Secret, while preserving optional
authentication behavior when auth is disabled.
In `@src/compute-plane-services/nvsnap/internal/agent/agent.go`:
- Around line 466-481: Configure the agent listener and peer URL advertisement
in src/compute-plane-services/nvsnap/internal/agent/agent.go:466-481 to use TLS
and https:// consistently, while preserving tokenGuard authentication ordering
and SetOutboundToken behavior. In
src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go:98-110 and
578-594, validate redirect destinations and reject plaintext or otherwise unsafe
URLs before attaching the Authorization header or sending requests.
In `@src/compute-plane-services/nvsnap/internal/agent/auth_test.go`:
- Around line 80-85: Update TestTokenGuardPermissiveServesButCounts to capture
the missing-result authentication metric before calling served, then read it
afterward and assert it increased by exactly one while preserving the existing
response assertions.
In `@src/compute-plane-services/nvsnap/internal/agent/auth.go`:
- Around line 191-197: Update authTransport.RoundTrip to add the bearer token
only when r.URL uses HTTPS; leave non-HTTPS requests unauthenticated while
preserving the existing clone-before-modify behavior and header checks.
- Around line 191-202: Update authTransport.RoundTrip to prevent bearer-token
injection on untrusted cross-origin redirects: validate the request destination
against the trusted origin before setting authHeader, or disable redirect
following in peerHTTPClient. Preserve authorization for trusted requests while
ensuring redirected requests to other origins are not given the bearer token.
In `@src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go`:
- Around line 579-594: Update selfAgentURL to construct the authority with
net.JoinHostPort(ip, port), preserving the existing IP and port selection
behavior and producing valid IPv6 URLs. Add an IPv6 assertion to
TestSelfAgentURL_DerivesPort covering the bracketed host-port result.
In `@src/compute-plane-services/nvsnap/internal/agent/restore.go`:
- Around line 218-220: Update the checkpoint ID validation handling in both
restoreHandler and triggerRestoreHandler so errors returned by
validPathSegment("checkpointId", ...) produce HTTP 400 via http.StatusBadRequest
instead of HTTP 500. Preserve existing handling for other error types and normal
requests.
---
Nitpick comments:
In `@src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go`:
- Around line 202-263: Add focused tests covering NVSNAP_AGENT_TOKEN handling in
setAgentAuth/getStatus, AgentBaseURL trailing-slash normalization, and Secret
reference propagation across
src/compute-plane-services/nvsnap/internal/agent/webhook_integration.go:90-95,
src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go:93-132,
and src/compute-plane-services/nvsnap/internal/webhook/mutate.go:354-361. Add a
go_test target for nvsnap-mount-prep, run the native Bazel test, and record its
result; the main.go site requires no direct production change.
In `@src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go`:
- Around line 125-129: Update the checkpointID validation failure branch in
cascade fetch to call span.RecordError(err) before span.SetStatus(codes.Error,
err.Error()), preserving the existing return behavior and ensuring the
validation error is retained with its attributes.
In `@src/compute-plane-services/nvsnap/internal/agent/pathsafe_test.go`:
- Around line 195-201: Update the unsafe-path loop in the joinWithinRoot test to
require err != nil for every traversal and absolute manifest path, rather than
accepting normalized results under root. Keep the existing unsafe path cases and
ensure each rejected input fails through the error contract used by production
callers.
🪄 Autofix
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: e4b70cbc-c3c7-4a1f-939a-0ce3cc332f26
📒 Files selected for processing (21)
src/compute-plane-services/nvsnap/cmd/agent/main.gosrc/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.gosrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yamlsrc/compute-plane-services/nvsnap/internal/agent/BUILD.bazelsrc/compute-plane-services/nvsnap/internal/agent/advertise_test.gosrc/compute-plane-services/nvsnap/internal/agent/agent.gosrc/compute-plane-services/nvsnap/internal/agent/auth.gosrc/compute-plane-services/nvsnap/internal/agent/auth_test.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_test.gosrc/compute-plane-services/nvsnap/internal/agent/restore.gosrc/compute-plane-services/nvsnap/internal/agent/webhook_integration.gosrc/compute-plane-services/nvsnap/internal/metrics/BUILD.bazelsrc/compute-plane-services/nvsnap/internal/metrics/metrics.gosrc/compute-plane-services/nvsnap/internal/metrics/register_test.gosrc/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.gosrc/compute-plane-services/nvsnap/internal/webhook/mutate.go
🛑 Comments failed to post (1)
src/compute-plane-services/nvsnap/internal/agent/restore.go (1)
218-220: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail ast-grep outline src/compute-plane-services/nvsnap/internal/agent/agent.go --items all rg -n -C 12 \ 'func \(a \*Agent\) (restoreHandler|triggerRestoreHandler)|a\.(Restore|TriggerRestore)\(' \ src/compute-plane-services/nvsnap/internal/agent/agent.go rg -n -C 6 \ 'restoreHandler|triggerRestoreHandler|StatusBadRequest|checkpointId' \ src/compute-plane-services/nvsnap/internal/agentRepository: NVIDIA/nvcf
Length of output: 50367
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '717,755p;839,855p' src/compute-plane-services/nvsnap/internal/agent/agent.go printf '\n--- restore validation ---\n' sed -n '390,410p' src/compute-plane-services/nvsnap/internal/agent/restore.go printf '\n--- status mapping verifier ---\n' python3 - <<'PY' from pathlib import Path agent = Path("src/compute-plane-services/nvsnap/internal/agent/agent.go").read_text() restore = Path("src/compute-plane-services/nvsnap/internal/agent/restore.go").read_text() for name, text in [ ("restoreHandler", agent), ("triggerRestoreHandler", agent), ]: start = text.index(f"func (a *Agent) {name}") end = text.find("\nfunc (a *Agent) ", start + 1) body = text[start:] if end == -1 else text[start:end] print(name) print(" calls validation-bearing operation:", ("a.Restore(" in body if name == "restoreHandler" else "a.TriggerRestore(" in body)) print(" maps operation error to 400:", "http.StatusBadRequest" in body[body.find("if err != nil"):]) print(" maps operation error to 500:", "http.StatusInternalServerError" in body[body.find("if err != nil"):]) for marker in [ 'if err := validPathSegment("checkpointId", req.CheckpointID); err != nil', ]: print("restore.go validation occurrence:", restore.count(marker)) PYRepository: NVIDIA/nvcf
Length of output: 3289
Return HTTP 400 for invalid checkpoint IDs
When
validPathSegmentrejectscheckpointId, bothrestoreHandlerandtriggerRestoreHandlercurrently return HTTP 500. Map these validation errors tohttp.StatusBadRequest.🤖 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 218 - 220, Update the checkpoint ID validation handling in both restoreHandler and triggerRestoreHandler so errors returned by validPathSegment("checkpointId", ...) produce HTTP 400 via http.StatusBadRequest instead of HTTP 500. Preserve existing handling for other error types and normal requests.
CI's bazel build failed on internal/webhook with "missing strict dependencies:
import of k8s.io/utils/ptr". My doing: I reached for ptr.To(true) when adding
the optional Secret reference for the agent token, and never declared the
dependency in the BUILD file. `go test` resolves it from the module graph, so
it passed locally and only bazel's strict-deps enforcement caught it.
Inlined an addressable bool instead of declaring the dep. One call site, one
file, and nothing else in nvsnap imports it -- AGENTS.md says not to add a
library for something that can be safely expressed in existing code, and under
bazel that import is a new external node in the build graph for a line the
language already has.
Also picks up gazelle's reordering of auth_test.go and advertise_test.go in the
agent BUILD after the main merge. Confirmed both stayed in go_test rather than
drifting into go_library, along with pathsafe_test.go.
Unrelated BUILD churn gazelle produced under src/libraries/{java,rust} was
reverted; it is outside this PR and outside check-gazelle's Go-root scope.
Verified: build clean; 13 internal packages pass, 0 fail; gofmt clean; helm
lint clean; check-gazelle reports BUILD files up to date.
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go (1)
579-594: 🎯 Functional Correctness | 🟡 MinorUse
net.JoinHostPortfor IPv6 addresses.
fmt.Sprintf("http://%s:%s", ip, port)produces an invalid authority for an IPv6 address, such ashttp://2001:db8::1:8081. Peer registration or fetching can fail in IPv6 environments.Return
"http://" + net.JoinHostPort(ip, port)and add an IPv6 case toTestSelfAgentURL.🤖 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 579 - 594, The self-agent URL builder must correctly format IPv6 addresses. In the function containing the AdvertiseIP/NodeIP fallback, import and use net.JoinHostPort(ip, port) when constructing the HTTP URL, preserving the existing empty-IP and port-selection behavior; extend TestSelfAgentURL with an IPv6 case.
🤖 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/cascade_fetch.go`:
- Around line 98-110: Update peerHTTPClient’s redirect policy to reject
redirects that change the request host or scheme before authTransport injects
the bearer token, while preserving same-origin redirects. Add tests covering
cross-origin redirects and HTTPS-to-HTTP redirects.
In `@src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go`:
- Around line 105-107: Update the agent API setup around agentURL and the
nvsnap-mount-prep HTTP client to use HTTPS with certificate verification
whenever NVSNAP_AGENT_TOKEN is configured, including for the default
NVSNAP_AGENT_URL path. Configure the transport to prevent cross-origin redirects
before attaching or sending the bearer token, while preserving the existing
custom AgentBaseURL behavior.
---
Duplicate comments:
In `@src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go`:
- Around line 579-594: The self-agent URL builder must correctly format IPv6
addresses. In the function containing the AdvertiseIP/NodeIP fallback, import
and use net.JoinHostPort(ip, port) when constructing the HTTP URL, preserving
the existing empty-IP and port-selection behavior; extend TestSelfAgentURL with
an IPv6 case.
🪄 Autofix
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: 6f71d51a-077b-4359-8348-70978020cc0b
📒 Files selected for processing (18)
src/compute-plane-services/nvsnap/cmd/agent/main.gosrc/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.gosrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-token-secret.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yamlsrc/compute-plane-services/nvsnap/internal/agent/BUILD.bazelsrc/compute-plane-services/nvsnap/internal/agent/advertise_test.gosrc/compute-plane-services/nvsnap/internal/agent/agent.gosrc/compute-plane-services/nvsnap/internal/agent/auth.gosrc/compute-plane-services/nvsnap/internal/agent/auth_test.gosrc/compute-plane-services/nvsnap/internal/agent/cascade_fetch.gosrc/compute-plane-services/nvsnap/internal/agent/webhook_integration.gosrc/compute-plane-services/nvsnap/internal/metrics/BUILD.bazelsrc/compute-plane-services/nvsnap/internal/metrics/metrics.gosrc/compute-plane-services/nvsnap/internal/metrics/register_test.gosrc/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.gosrc/compute-plane-services/nvsnap/internal/webhook/mutate.go
🚧 Files skipped from review as they are similar to previous changes (15)
- src/compute-plane-services/nvsnap/internal/agent/BUILD.bazel
- src/compute-plane-services/nvsnap/internal/webhook/mutate.go
- src/compute-plane-services/nvsnap/internal/metrics/BUILD.bazel
- src/compute-plane-services/nvsnap/internal/agent/advertise_test.go
- src/compute-plane-services/nvsnap/internal/metrics/register_test.go
- src/compute-plane-services/nvsnap/internal/agent/webhook_integration.go
- src/compute-plane-services/nvsnap/cmd/agent/main.go
- src/compute-plane-services/nvsnap/deploy/helm/nvsnap/values.yaml
- src/compute-plane-services/nvsnap/internal/agent/agent.go
- src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml
- src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go
- src/compute-plane-services/nvsnap/internal/metrics/metrics.go
- src/compute-plane-services/nvsnap/internal/agent/auth.go
- src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml
- src/compute-plane-services/nvsnap/internal/agent/auth_test.go
Comment on lines
+98
to
+110
| // authTransport wraps the tuned transport rather than replacing it: every | ||
| // agent-to-agent request carries the bearer token (a no-op until one is | ||
| // configured) without any cascade call site knowing about auth. See | ||
| // auth.go and GH #486. | ||
| Transport: &authTransport{base: &http.Transport{ | ||
| MaxIdleConns: peerFetchConcurrency * 2, | ||
| MaxIdleConnsPerHost: peerFetchConcurrency * 2, | ||
| IdleConnTimeout: 90 * time.Second, | ||
| // Disable HTTP/2 forced upgrade; we want plain HTTP/1.1 so we | ||
| // can reason about TCP stream count for the Cilium-multi-stream | ||
| // hypothesis. Re-enable explicitly if/when we switch to h2c. | ||
| ForceAttemptHTTP2: false, | ||
| }, | ||
| }}, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant client and transport definitions ---'
rg -n -A35 -B15 'peerHTTPClient|CheckRedirect|authTransport|http\.Client|NewRequest|Do\(' src/compute-plane-services/nvsnap/internal/agent --glob '*.go'
printf '%s\n' '--- standalone Go redirect probe ---'
cat >/tmp/redirect_probe.go <<'GO'
package main
import (
"fmt"
"net/http"
"net/http/httptest"
"sync"
)
type authTransport struct {
base http.RoundTripper
}
func (t *authTransport) RoundTrip(r *http.Request) (*http.Response, error) {
r = r.Clone(r.Context())
r.Header.Set("Authorization", "Bearer secret")
fmt.Printf("roundtrip method=%s url=%s authorization=%q\n",
r.Method, r.URL.String(), r.Header.Get("Authorization"))
return t.base.RoundTrip(r)
}
func main() {
var mu sync.Mutex
var requests []string
record := func(prefix string) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
mu.Lock()
requests = append(requests, fmt.Sprintf("%s %s authorization=%q", prefix, r.URL.String(), r.Header.Get("Authorization")))
mu.Unlock()
if prefix == "source" {
http.Redirect(w, r, targetURL, http.StatusFound)
return
}
w.WriteHeader(http.StatusNoContent)
})
}
var targetURL string
target := httptest.NewServer(record("target"))
defer target.Close()
targetURL = target.URL + "/redirected"
source := httptest.NewServer(record("source"))
defer source.Close()
client := &http.Client{Transport: &authTransport{base: http.DefaultTransport}}
resp, err := client.Get(source.URL + "/start")
if err != nil {
panic(err)
}
resp.Body.Close()
mu.Lock()
defer mu.Unlock()
for _, request := range requests {
fmt.Println(request)
}
}
GO
go run /tmp/redirect_probe.go
rm -f /tmp/redirect_probe.goRepository: NVIDIA/nvcf
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- client construction and redirect configuration ---'
rg -n -A25 -B20 'http\.Client|CheckRedirect|Transport:|authTransport|Do\(' \
src/compute-plane-services/nvsnap/internal/agent/cascade_fetch.go \
src/compute-plane-services/nvsnap/internal/agent/auth.go || true
printf '%s\n' '--- standalone Go redirect probe ---'
cat >/tmp/redirect_probe.go <<'GO'
package main
import (
"fmt"
"net/http"
"net/http/httptest"
)
type authTransport struct{ base http.RoundTripper }
func (t *authTransport) RoundTrip(r *http.Request) (*http.Response, error) {
r = r.Clone(r.Context())
r.Header.Set("Authorization", "Bearer secret")
fmt.Printf("transport: %s authorization=%q\n", r.URL, r.Header.Get("Authorization"))
return t.base.RoundTrip(r)
}
func main() {
target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
fmt.Printf("target: %s authorization=%q\n", r.URL, r.Header.Get("Authorization"))
w.WriteHeader(http.StatusNoContent)
}))
defer target.Close()
source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
fmt.Printf("source: %s authorization=%q\n", r.URL, r.Header.Get("Authorization"))
http.Redirect(w, r, target.URL+"/redirected", http.StatusFound)
}))
defer source.Close()
client := &http.Client{Transport: &authTransport{base: http.DefaultTransport}}
resp, err := client.Get(source.URL + "/start")
if err != nil {
panic(err)
}
_ = resp.Body.Close()
}
GO
go run /tmp/redirect_probe.go
rm -f /tmp/redirect_probe.goRepository: NVIDIA/nvcf
Length of output: 34150
🏁 Script executed:
#!/bin/bash
set -eu
cat >/tmp/redirect_probe.go <<'GO'
package main
import (
"fmt"
"net/http"
"net/http/httptest"
)
type authTransport struct{ base http.RoundTripper }
func (t *authTransport) RoundTrip(r *http.Request) (*http.Response, error) {
r = r.Clone(r.Context())
r.Header.Set("Authorization", "Bearer secret")
fmt.Printf("transport: %s authorization=%q\n", r.URL, r.Header.Get("Authorization"))
return t.base.RoundTrip(r)
}
func main() {
target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
fmt.Printf("target: %s authorization=%q\n", r.URL, r.Header.Get("Authorization"))
w.WriteHeader(http.StatusNoContent)
}))
defer target.Close()
source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
fmt.Printf("source: %s authorization=%q\n", r.URL, r.Header.Get("Authorization"))
http.Redirect(w, r, target.URL+"/redirected", http.StatusFound)
}))
defer source.Close()
client := &http.Client{Transport: &authTransport{base: http.DefaultTransport}}
resp, err := client.Get(source.URL + "/start")
if err != nil {
panic(err)
}
_ = resp.Body.Close()
}
GO
CGO_ENABLED=0 go run /tmp/redirect_probe.go
rm -f /tmp/redirect_probe.goRepository: NVIDIA/nvcf
Length of output: 400
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reject cross-origin redirects before injecting the bearer token.
peerHTTPClient uses the default redirect policy, while authTransport.RoundTrip adds the token to every request. A redirect can therefore send the bearer token to a different host or scheme. Configure CheckRedirect to reject host and scheme changes, or restrict token injection to approved origins. Add cross-origin and HTTPS-to-HTTP redirect tests.
🤖 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 98 - 110, Update peerHTTPClient’s redirect policy to reject redirects that
change the request host or scheme before authTransport injects the bearer token,
while preserving same-origin redirects. Add tests covering cross-origin
redirects and HTTPS-to-HTTP redirects.
Comment on lines
105
to
+107
| agentURL := fmt.Sprintf("http://$(NVSNAP_HOST_IP):%d", agentPort) | ||
| if m.AgentBaseURL != "" { | ||
| agentURL = strings.TrimRight(m.AgentBaseURL, "/") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- guidance files ---'
find .. -name AGENTS.md -print
printf '%s\n' '--- target file ---'
sed -n '1,220p' src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go
printf '%s\n' '--- mount-prep URL and token consumers ---'
rg -n -C 4 'NVSNAP_AGENT_URL|NVSNAP_AGENT_TOKEN|AgentBaseURL|mount.?prep|MountPrep|agentURL' src/compute-plane-services/nvsnapRepository: NVIDIA/nvcf
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable guidance ---'
cat AGENTS.md
printf '%s\n' '--- mount-prep source files ---'
fd -t f . src/compute-plane-services/nvsnap | rg '(^|/)(cmd|.*mount.*prep|.*restore.*prep|.*agent.*client)'
printf '%s\n' '--- exact environment and HTTP symbols ---'
rg -n -C 5 'NVSNAP_AGENT_URL|NVSNAP_AGENT_TOKEN|Authorization|Bearer|http\\.Client|CheckRedirect|Do\\(|Post\\(|restore/prep|AgentBaseURL' src/compute-plane-services/nvsnap/cmd src/compute-plane-services/nvsnap/internal --glob '*.go'
printf '%s\n' '--- chart configuration references ---'
rg -n -C 5 'AgentBaseURL|agent.*base.*url|internalTrafficPolicy|nvsnap-agent-token|tls|https://' . --glob '*.yaml' --glob '*.yml' --glob '*.tpl' --glob '*.go' --glob '*.md'Repository: NVIDIA/nvcf
Length of output: 24248
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- mount-prep client ---'
sed -n '1,260p' src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go
printf '%s\n' '--- restore-prep HTTP handler and auth ---'
sed -n '1,280p' src/compute-plane-services/nvsnap/internal/agent/restore_prep_http.go
printf '%s\n' '--- exact client/config references ---'
rg -n -F -C 4 'NVSNAP_AGENT_URL' src/compute-plane-services/nvsnap --glob '*.go'
rg -n -F -C 4 'NVSNAP_AGENT_TOKEN' src/compute-plane-services/nvsnap --glob '*.go'
rg -n -F -C 4 'CheckRedirect' src/compute-plane-services/nvsnap --glob '*.go'
rg -n -F -C 4 'Authorization' src/compute-plane-services/nvsnap --glob '*.go'
printf '%s\n' '--- chart references ---'
rg -n -C 5 'AgentBaseURL|agentBaseURL|internalTrafficPolicy|nvsnap-agent-token|https://' src/compute-plane-services/nvsnap deploy --glob '*.yaml' --glob '*.yml' --glob '*.tpl' --glob '*.go' --glob '*.md' || trueRepository: NVIDIA/nvcf
Length of output: 24052
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- remaining client code ---'
sed -n '245,275p' src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go
printf '%s\n' '--- agent listener and auth setup ---'
sed -n '430,500p' src/compute-plane-services/nvsnap/internal/agent/agent.go
sed -n '200,235p' src/compute-plane-services/nvsnap/cmd/agent/main.go
printf '%s\n' '--- AgentBaseURL and TLS configuration ---'
rg -n -C 4 -F 'AgentBaseURL' . --glob '*.go' --glob '*.yaml' --glob '*.yml' --glob '*.tpl' --glob '*.md' || true
rg -n -C 4 -E 'internalTrafficPolicy|nvsnap-agent-token|tls|https://' src/compute-plane-services/nvsnap deploy --glob '*.yaml' --glob '*.yml' --glob '*.tpl' --glob '*.go' --glob '*.md' || true
printf '%s\n' '--- read-only source invariant check ---'
python3 - <<'PY'
from pathlib import Path
webhook = Path("src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go").read_text()
client = Path("src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go").read_text()
checks = {
"default URL is plaintext HTTP": 'fmt.Sprintf("http://$(NVSNAP_HOST_IP):%d", agentPort)' in webhook,
"token is injected": 'Name: "NVSNAP_AGENT_TOKEN"' in webhook,
"token becomes bearer authorization": 'r.Header.Set("Authorization", "Bearer "+tok)' in client,
"POST uses default client": 'http.DefaultClient.Do(httpReq)' in client,
"no custom redirect policy": 'CheckRedirect' not in client,
"POST endpoint is mount prep": 'agentURL+"/v1/restore/prep"' in client,
"GET endpoint is mount prep status": 'agentURL+"/v1/restore/prep/"+podUID' in client,
}
for name, result in checks.items():
print(f"{name}: {'PASS' if result else 'FAIL'}")
if not all(checks.values()):
raise SystemExit(1)
PYRepository: NVIDIA/nvcf
Length of output: 14946
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal · Exploitability: Moderate
Use authenticated transport for the agent API.
When NVSNAP_AGENT_TOKEN is configured, nvsnap-mount-prep sends it as a bearer token through http.DefaultClient, while the default NVSNAP_AGENT_URL uses http://. An observer on the node-local path can capture and replay the token. Use HTTPS with certificate verification, and disable cross-origin redirects before sending the token.
🤖 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/webhook/mount_prep_init.go` around
lines 105 - 107, Update the agent API setup around agentURL and the
nvsnap-mount-prep HTTP client to use HTTPS with certificate verification
whenever NVSNAP_AGENT_TOKEN is configured, including for the default
NVSNAP_AGENT_URL path. Configure the transport to prevent cross-origin redirects
before attaching or sending the bearer token, while preserving the existing
custom AgentBaseURL behavior.
One conflict, in the restore-pod egress NetworkPolicy, and it needed a real hand-merge rather than picking a side. Both changes are wanted: this branch: (or (not .Values.agent.hostNetwork) .Values.webhook.agentHostCIDR) ... -}} main (#472): .Values.webhook.agentHostCIDR ... }} This branch carries #561's condition, which lets the policy render under pod networking without an operator-supplied node CIDR. Main carries #472's chart repair, which drops the trailing dash -- `-}}` chomps the following newline and glues the next line onto the preceding comment. Taking --ours would have silently reverted the chomping fix; taking --theirs would have made the init-container strategy unavailable under pod networking again. Kept both: this branch's condition with main's non-chomping close. Verified by rendering rather than by reading: - pod networking, no agentHostCIDR -> policy renders (the #561 behaviour) - hostNetwork, no agentHostCIDR -> policy absent (unchanged, correct) - rendered output starts at the Source comment and apiVersion with nothing glued to it (the #472 fix intact) - helm lint clean Also: build clean, 14 internal packages pass / 0 fail, check-gazelle up to date. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
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/deploy/helm/nvsnap/templates/agent-daemonset.yaml (1)
95-95: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External · Exploitability: Moderate
Use authenticated TLS for token-bearing mount-prep requests.
When authentication is enabled,
nvsnap-mount-prepsendsNVSNAP_AGENT_TOKENover HTTP to/v1/restore/prepand its status endpoint. The host-network fallback also uses HTTP. An attacker who observes node or cluster traffic can replay the captured token against the privileged agent API. Use HTTPS and reject redirects or prevent credential forwarding across origins.🤖 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/deploy/helm/nvsnap/templates/agent-daemonset.yaml` at line 95, The webhook agent base URL in the agent DaemonSet must use HTTPS so token-bearing mount-prep and status requests are encrypted; update the `--webhook-agent-base-url` argument and ensure the host-network fallback also uses HTTPS, with redirect handling that does not forward credentials across origins.
🧹 Nitpick comments (2)
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml (1)
91-96: 📐 Maintainability & Code Quality | 🔵 TrivialDocument both agent networking modes.
Pod-networked agents now use
nvsnap-agent-localwithinternalTrafficPolicy: Local. Host-networked agents use node addressing. Update the relevant ASCII or Mermaid architecture or sequence diagram to show both mount-prep paths.As per coding guidelines, runtime behavior, data flow, and component interaction changes require an architecture or sequence diagram assessment.
🤖 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/deploy/helm/nvsnap/templates/agent-daemonset.yaml` around lines 91 - 96, Update the relevant architecture or sequence diagram to document both mount-prep networking paths: pod-networked agents reaching nvsnap-agent-local through the internalTrafficPolicy: Local Service, and host-networked agents reaching the agent via node addressing. Keep the diagram’s existing runtime flow intact while clearly distinguishing these two modes.Source: Coding guidelines
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml (1)
133-151: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd Helm render tests for both network modes. The DaemonSet,
nvsnap-agent-localService, and pod-networkNetworkPolicyuse the samenvsnap.agent.selectorLabels. Assert theinternalTrafficPolicy: Localpath and the host-network CIDR path.🤖 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/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml` around lines 133 - 151, Add Helm render tests covering both hostNetwork and pod-network configurations, verifying the DaemonSet, nvsnap-agent-local Service, and pod-network NetworkPolicy consistently use nvsnap.agent.selectorLabels. Assert internalTrafficPolicy: Local for the local Service path and the configured webhook.agentHostCIDR ipBlock for the host-network NetworkPolicy path.Source: Coding guidelines
🤖 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/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml`:
- Around line 111-112: Update the comment immediately preceding the pod
networking render condition to document that the policy is rendered when agent
host networking is disabled or agentHostCIDR is configured, alongside the
existing webhook, init-container strategy, and restore namespace requirements.
Keep the condition itself unchanged.
---
Outside diff comments:
In
`@src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml`:
- Line 95: The webhook agent base URL in the agent DaemonSet must use HTTPS so
token-bearing mount-prep and status requests are encrypted; update the
`--webhook-agent-base-url` argument and ensure the host-network fallback also
uses HTTPS, with redirect handling that does not forward credentials across
origins.
---
Nitpick comments:
In
`@src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml`:
- Around line 91-96: Update the relevant architecture or sequence diagram to
document both mount-prep networking paths: pod-networked agents reaching
nvsnap-agent-local through the internalTrafficPolicy: Local Service, and
host-networked agents reaching the agent via node addressing. Keep the diagram’s
existing runtime flow intact while clearly distinguishing these two modes.
In
`@src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml`:
- Around line 133-151: Add Helm render tests covering both hostNetwork and
pod-network configurations, verifying the DaemonSet, nvsnap-agent-local Service,
and pod-network NetworkPolicy consistently use nvsnap.agent.selectorLabels.
Assert internalTrafficPolicy: Local for the local Service path and the
configured webhook.agentHostCIDR ipBlock for the host-network NetworkPolicy
path.
🪄 Autofix
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: 93a6fd49-cadd-46de-b805-a9ea47d00942
📒 Files selected for processing (2)
src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yamlsrc/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml
Comment on lines
+111
to
112
| {{- if and .Values.webhook.enabled (eq (.Values.webhook.restorePrepStrategy | default "inline") "init-container") (or (not .Values.agent.hostNetwork) .Values.webhook.agentHostCIDR) .Values.agent.l2.restoreNamespaces }} | ||
| {{- range $ns := .Values.agent.l2.restoreNamespaces }} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the render-condition comment for pod networking.
Line 111 renders this policy when .Values.agent.hostNetwork is false, even when agentHostCIDR is empty. The preceding comment still says that the policy is rendered only when agentHostCIDR is set. This can cause incorrect chart configuration.
Proposed comment update
-Only rendered when the init-container strategy is selected AND
-agentHostCIDR is set. The default inline strategy does the mount inside
-the webhook and needs no pod->agent egress.
+Rendered when the init-container strategy is selected.
+Host-networked agents require agentHostCIDR and use an ipBlock.
+Pod-networked agents use namespace and pod selectors and do not require
+agentHostCIDR. The default inline strategy needs no pod->agent egress.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {{- if and .Values.webhook.enabled (eq (.Values.webhook.restorePrepStrategy | default "inline") "init-container") (or (not .Values.agent.hostNetwork) .Values.webhook.agentHostCIDR) .Values.agent.l2.restoreNamespaces }} | |
| {{- range $ns := .Values.agent.l2.restoreNamespaces }} | |
| {{/* | |
| Rendered when the init-container strategy is selected. | |
| Host-networked agents require agentHostCIDR and use an ipBlock. | |
| Pod-networked agents use namespace and pod selectors and do not require | |
| agentHostCIDR. The default inline strategy needs no pod->agent egress. | |
| */}} | |
| {{- if and .Values.webhook.enabled (eq (.Values.webhook.restorePrepStrategy | default "inline") "init-container") (or (not .Values.agent.hostNetwork) .Values.webhook.agentHostCIDR) .Values.agent.l2.restoreNamespaces }} | |
| {{- range $ns := .Values.agent.l2.restoreNamespaces }} |
🤖 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/deploy/helm/nvsnap/templates/network-policy-restore-pods.yaml`
around lines 111 - 112, Update the comment immediately preceding the pod
networking render condition to document that the policy is rendered when agent
host networking is disabled or agentHostCIDR is configured, alongside the
existing webhook, init-container strategy, and restore namespace requirements.
Keep the condition itself unchanged.
Contributor
Author
|
Cluster validation on nvcf-dgxc-k8s-aws-usw2-dev2 (2 GPU nodes, RTX PRO 6000 Blackwell, driver r580, EKS 1.34). Built the branch as What passesChart and enforcement behave as designed. Token preservation across Enforcement,
Both agents 0 restarts under What fails:
|
With --auth-mode=required the agent rejects unauthenticated API calls, but checkpoint.sh sent no Authorization header, so every checkpoint failed with an opaque "unauthorized". Caught running test-e2e.sh vllm-small against a required-mode agent on a live cluster: Error: Failed to get checkpoint ID from response Response: unauthorized Read the token from the nvsnap-agent-token Secret and attach it. A missing Secret is deliberately not an error: the chart only creates it when agent.auth.enabled is set, and the same script has to work against both an authenticating and a non-authenticating agent. All agent API calls in this script funnel through call_agent_api, so the header is added in exactly one place. Also move the port-forward readiness probe from /v1/checkpoints to /health. /health is unauthenticated in every auth mode, so the probe tests what it is meant to test -- that the port-forward is up -- rather than incidentally depending on the credential. The old probe only "worked" under required mode because curl -s exits 0 on a 401. The other scripts that talk to port 8081 are still token-unaware and need the same treatment before required mode is recommended. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/scripts/checkpoint.sh`:
- Around line 159-174: Update agent_auth_header and call_agent_api to accept the
selected namespace as an argument, using it for both Secret retrieval and
port-forward targeting instead of the global NAMESPACE. Update every
call_agent_api caller, including cmd_create and cmd_list, to pass its namespace
through consistently.
- Around line 172-174: Update the authentication handling near agent_auth_header
so the bearer token is not passed through curl arguments or exposed by shell
tracing. Supply the header through a restrictive temporary file or another
secret-safe curl input, disable tracing while handling it, and securely remove
any temporary data after the request.
- Around line 203-208: Update call_agent_api to make curl fail on HTTP
authentication errors by enabling HTTP status failure handling, capture the curl
exit status before stopping the port-forward, and return that status after
cleanup. Apply this consistently to both request branches so cmd_list reports
failure for 401/403 responses.
🪄 Autofix
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: e6cb057b-df57-44db-83e6-8f46eef13aa5
📒 Files selected for processing (1)
src/compute-plane-services/nvsnap/scripts/checkpoint.sh
Comment on lines
+159
to
+174
| agent_auth_header() { | ||
| local token | ||
| token=$(kubectl get secret nvsnap-agent-token -n "$NAMESPACE" \ | ||
| -o jsonpath='{.data.token}' 2>/dev/null | base64 -d 2>/dev/null) || true | ||
| [ -n "$token" ] && printf 'Authorization: Bearer %s' "$token" | ||
| } | ||
|
|
||
| call_agent_api() { | ||
| local agent_pod="$1" | ||
| local method="$2" | ||
| local endpoint="$3" | ||
| local data="${4:-}" | ||
|
|
||
|
|
||
| local auth=() hdr | ||
| hdr=$(agent_auth_header) | ||
| [ -n "$hdr" ] && auth=(-H "$hdr") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the selected namespace through the agent API helpers.
cmd_create and cmd_list can target a namespace that differs from $NAMESPACE. agent_auth_header reads the Secret from $NAMESPACE, and the port-forward also uses $NAMESPACE. A non-default namespace can therefore use the wrong token and the wrong port-forward target.
Pass the selected namespace to agent_auth_header and call_agent_api. Use it for both kubectl get secret and kubectl port-forward. Update every caller.
Proposed fix
-agent_auth_header() {
+agent_auth_header() {
+ local namespace="$1"
local token
- token=$(kubectl get secret nvsnap-agent-token -n "$NAMESPACE" \
+ token=$(kubectl get secret nvsnap-agent-token -n "$namespace" \
-o jsonpath='{.data.token}' 2>/dev/null | base64 -d 2>/dev/null) || true
[ -n "$token" ] && printf 'Authorization: Bearer %s' "$token"
}
call_agent_api() {
local agent_pod="$1"
- local method="$2"
- local endpoint="$3"
- local data="${4:-}"
+ local namespace="$2"
+ local method="$3"
+ local endpoint="$4"
+ local data="${5:-}"
- hdr=$(agent_auth_header)
+ hdr=$(agent_auth_header "$namespace")Also use "$namespace" in the port-forward and pass it from every call_agent_api caller.
🤖 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/scripts/checkpoint.sh` around lines 159 -
174, Update agent_auth_header and call_agent_api to accept the selected
namespace as an argument, using it for both Secret retrieval and port-forward
targeting instead of the global NAMESPACE. Update every call_agent_api caller,
including cmd_create and cmd_list, to pass its namespace through consistently.
Comment on lines
+203
to
+208
| "${auth[@]}" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d "$data" | ||
| else | ||
| curl -s --max-time "$max_time" -X "$method" "http://localhost:${AGENT_PORT}${endpoint}" | ||
| curl -s --max-time "$max_time" -X "$method" "http://localhost:${AGENT_PORT}${endpoint}" \ | ||
| "${auth[@]}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- guidance files ---'
find .. -name AGENTS.md -print
printf '%s\n' '--- target file context ---'
sed -n '1,280p' src/compute-plane-services/nvsnap/scripts/checkpoint.sh
printf '%s\n' '--- relevant symbols and call sites ---'
rg -n -C 4 'call_agent_api|cmd_list|auth|pf_pid|AGENT_PORT' src/compute-plane-services/nvsnap/scripts/checkpoint.shRepository: NVIDIA/nvcf
Length of output: 17248
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository guidance ---'
cat AGENTS.md
printf '%s\n' '--- remaining command flow ---'
sed -n '280,380p' src/compute-plane-services/nvsnap/scripts/checkpoint.sh
printf '%s\n' '--- isolated status-propagation probe ---'
bash <<'BASH'
set -u
# Model curl: /health succeeds; the API returns HTTP 401.
curl() {
local previous=""
for arg in "$@"; do
if [[ "$previous" == "http://localhost:8081/health" ]]; then
return 0
fi
previous="$arg"
done
printf '%s\n' '{"error":"unauthorized"}'
return 0
}
kubectl() {
if [[ "${1:-}" == "port-forward" ]]; then
sleep 30
else
return 0
fi
}
agent_auth_header() { return 0; }
original_call() {
local auth=()
kubectl port-forward agent 8081:8081 >/dev/null 2>&1 &
local pf_pid=$!
local ready=false
for i in {1..15}; do
if timeout 5 curl -sf http://localhost:8081/health >/dev/null 2>&1; then
ready=true
break
fi
done
[[ "$ready" == true ]] || return 1
curl -s --max-time 600 -X GET http://localhost:8081/v1/checkpoints "${auth[@]}"
kill "$pf_pid" 2>/dev/null || true
}
corrected_call() {
local auth=() curl_status=0
kubectl port-forward agent 8081:8081 >/dev/null 2>&1 &
local pf_pid=$!
local ready=false
for i in {1..15}; do
if timeout 5 curl -sf http://localhost:8081/health >/dev/null 2>&1; then
ready=true
break
fi
done
[[ "$ready" == true ]] || return 1
curl -sS -f --max-time 600 -X GET http://localhost:8081/v1/checkpoints "${auth[@]}" ||
curl_status=$?
kill "$pf_pid" 2>/dev/null || true
return "$curl_status"
}
set +e
original_call >/tmp/original-body
original_status=$?
corrected_call >/tmp/corrected-body
corrected_status=$?
set -e
printf 'original_status=%s body=%s\n' "$original_status" "$(tr -d '\n' </tmp/original-body)"
printf 'corrected_status=%s body=%s\n' "$corrected_status" "$(tr -d '\n' </tmp/corrected-body)"
rm -f /tmp/original-body /tmp/corrected-body
BASHRepository: NVIDIA/nvcf
Length of output: 24415
🏁 Script executed:
#!/bin/bash
set -eu
bash <<'BASH'
set -u
timeout() {
shift
"$@"
}
curl() {
local has_fail=false
local url=""
for arg in "$@"; do
case "$arg" in
-f) has_fail=true ;;
http://*) url="$arg" ;;
esac
done
if [[ "$url" == "http://localhost:8081/health" ]]; then
return 0
fi
printf '%s\n' '{"error":"unauthorized"}'
[[ "$has_fail" == true ]] && return 22
return 0
}
kubectl() {
if [[ "${1:-}" == "port-forward" ]]; then
sleep 30
else
return 0
fi
}
original_call() {
local auth=()
kubectl port-forward agent 8081:8081 >/dev/null 2>&1 &
local pf_pid=$!
local ready=false
for i in {1..15}; do
if timeout 5 curl -sf http://localhost:8081/health >/dev/null 2>&1; then
ready=true
break
fi
done
[[ "$ready" == true ]] || return 1
curl -s --max-time 600 -X GET http://localhost:8081/v1/checkpoints "${auth[@]}"
kill "$pf_pid" 2>/dev/null || true
}
corrected_call() {
local auth=() curl_status=0
kubectl port-forward agent 8081:8081 >/dev/null 2>&1 &
local pf_pid=$!
local ready=false
for i in {1..15}; do
if timeout 5 curl -sf http://localhost:8081/health >/dev/null 2>&1; then
ready=true
break
fi
done
[[ "$ready" == true ]] || return 1
curl -sS -f --max-time 600 -X GET http://localhost:8081/v1/checkpoints "${auth[@]}" ||
curl_status=$?
kill "$pf_pid" 2>/dev/null || true
return "$curl_status"
}
set +e
original_call >/tmp/original-body
original_status=$?
corrected_call >/tmp/corrected-body
corrected_status=$?
set -e
printf 'original_status=%s body=%s\n' "$original_status" "$(tr -d '\n' </tmp/original-body)"
printf 'corrected_status=%s body=%s\n' "$corrected_status" "$(tr -d '\n' </tmp/corrected-body)"
rm -f /tmp/original-body /tmp/corrected-body
BASHRepository: NVIDIA/nvcf
Length of output: 247
Return a failure status for authentication errors.
curl -s returns status 0 for HTTP 401 and 403 responses. Preserve the curl status before stopping the port-forward, then return it from call_agent_api; otherwise cmd_list can report success with an invalid or missing token.
🤖 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/scripts/checkpoint.sh` around lines 203 -
208, Update call_agent_api to make curl fail on HTTP authentication errors by
enabling HTTP status failure handling, capture the curl exit status before
stopping the port-forward, and return that status after cleanup. Apply this
consistently to both request branches so cmd_list reports failure for 401/403
responses.
With --auth-mode=required the agent rejects unauthenticated calls, so
scripts that omit the header fail with a bare "unauthorized" that does
not say which credential is missing. Running test-e2e.sh vllm-small
against a required-mode agent failed at checkpoint, then -- once
checkpoint.sh was fixed -- at restore, with the agent logging:
Rejected unauthenticated request path=/v1/restore result=missing
Of the 16 scripts that mention port 8081, only three make a real
authenticated call; the rest hit /health, print instructions, or
delegate to checkpoint.sh. Those three plus checkpoint.sh are converted
here.
Add scripts/lib/agent-auth.sh with a single token lookup and replace the
inline helper checkpoint.sh carried. An absent Secret yields an empty
header list rather than an error, so the same script works against a
cluster installed without agent.auth.enabled.
Two call shapes need different handling:
- Local curl, or kubectl exec whose arguments the local shell expands
(checkpoint.sh, test-e2e.sh): read the Secret and pass the header as
an array, which keeps it intact through word-splitting.
- curl inside a quoted sh -c run in the agent container
(test-agent-driven-e2e.sh, bench-cross-node-restore.sh): use the
NVSNAP_AGENT_TOKEN the chart already injects, via --oauth2-bearer.
The -H form would word-split, since the quotes inside ${VAR:+...}
are literal rather than grouping, and the header value has spaces.
--oauth2-bearer takes the token as one argument, so it survives.
pprof is included deliberately: it sits on the guarded router and is not
in unauthenticatedPaths, because profiles leak memory contents.
Verified on nvcf-dgxc-k8s-aws-usw2-dev2 with --auth-mode=required:
helper returns 2 args with the Secret present and 0 without (under
set -u, no error); --oauth2-bearer against /debug/pprof/goroutine in the
agent container returns 200; all five files pass bash -n.
Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
Contributor
Author
|
Verified. Full e2e now passes with 33G checkpoint, agent Fixing it took two rounds, and the intermediate failure is the useful part: with only That is a good argument for the warning-level log on rejection -- it turned an opaque
Added
The second one has a trap worth recording: pprof is included on purpose: it is on the guarded router and not in Helper behaviour checked both ways: 2 args with the Secret present, 0 without under Remaining before |
Contributor
Author
|
Blocker found while tearing down the test cluster. My earlier audit missed a caller: nvsnap-server itself talks to the agent API, and it is token-unaware. What happensWith The agent side shows why:
The server also polls Why this one is worse than #734It fails silently and destroys the recovery path. The API reports success, the catalog row (the only record pointing at that dump) is deleted, and 33G is orphaned on the node with nothing left to reference it. No retry, no periodic sweep, and At one checkpoint per capture this fills a node disk with no operator-visible signal beyond a warning line in the agent log. AssessmentThis is a merge blocker for But Suggested fixGive nvsnap-server the same treatment the scripts got in Separately, and regardless of auth: Test cluster has been reverted to the main build (agent v0.2.32, auth off), checkpoints cleared, token secret removed. |
nvsnap-server is an agent client, and it was the one caller the #734 audit missed. Under --auth-mode=required every server-to-agent call 401s, and the delete path fails in the worst possible way. Observed on nvcf-dgxc-k8s-aws-usw2-dev2: DELETE -> 204 catalog: 0 rows disk: 33G /var/lib/nvsnap/checkpoints (unchanged) agent: Rejected unauthenticated request path=/v1/checkpoints/247eb2e0... remote="10.1.255.118:35534" result=missing 10.1.255.118 is the nvsnap-server pod. cascadeDeleteCheckpoint calls the agent, gets 401, and the row is dropped anyway -- so the API claims success while orphaning the dump with nothing left pointing at it. The row is the only handle, so checkpoint.sh cleanup cannot see it and DELETE by id 404s. Recovery is a manual rm on the node. The server also polls /v1/checkpoints on every agent, so its view of agent state was broken for as long as required was on. Add agentAuthTransport and wrap every agent-bound client: s.httpClient, the short-timeout client in cascadeDeleteCheckpoint, the agent proxy, the listAgentCheckpoints poller, and checkAgentHealth. Health is unauthenticated today and does not need it, but wrapping uniformly means nobody has to track which agent routes are exempt. demo.go's client is left alone -- it targets the workload pod, not an agent. Deliberately simpler than the agent's own outbound wrapper, which needs an atomic pointer because its peer client is built at import time before the token is known. The server builds its client in New() with config already parsed, so a plain field suffices. The token arrives via NVSNAP_AGENT_TOKEN rather than a flag, keeping the credential out of the process argv. The chart projects it from the nvsnap-agent-token Secret, and the server env block now renders when either jaeger or auth is enabled rather than jaeger alone. An empty token installs no wrapper at all, so a deployment without agent.auth.enabled is byte-identical to before. Verified: go build, go vet and bazel build clean across the nvsnap tree; new table tests cover bearer injection, the empty-token no-op, and not overwriting a caller-supplied Authorization; chart renders the env var for auth-on with jaeger both on and off, and omits it entirely when auth is off. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
Contributor
Author
|
Server fix validated on nvcf-dgxc-k8s-aws-usw2-dev2. Agent The regression now passesSame delete that previously returned 204 and leaked the dump:
The cascade reaches the agent, the L1 dump is removed, and nothing is orphaned. e2e also passes under requiredSo all three token gaps found today are now closed and verified: Still open, and worth deciding before this merges
Note on how this was foundEvery one of the 8 auth unit tests on this branch passes, and always did. They test a single process, so none could catch a second binary calling the API without a token. The e2e gate also passed throughout, because auth defaults off, so it exercised none of this code. Worth considering as a rule: a default-off feature should have the e2e run once in its enabled configuration before merge. One run with Build note
|
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
The agent API has no authentication. Any caller that can reach the port gets
the full control surface of a privileged process:
POST /v1/restore,DELETE /v1/checkpoints/{id},GET /v1/checkpoints/{id}/file?path=..., and/debug/pprof/*, on a process running privileged with/var/liband thecontainerd root bind-mounted.
That reach is wider than it looks. The DaemonSet binds the API to each node's
IP (
hostNetwork+hostPort: 8081, both defaults) rather than acluster-internal Service, and NetworkPolicy cannot fence it: a hostNetwork pod
carries node identity, so a
podSelectoringress rule does not match it. Thecomment at
values.yaml:454already records this. Access control has to livein the request path.
Not externally reachable in current deployments, so this is hardening rather
than an active exposure -- but the API should not still be open at GA.
What changed
Bearer token rather than mTLS. This same router serves the peer fan-out
endpoints that move multi-GB checkpoints, which is the path range chunking and
parallel multi-source fetch exist to speed up. TLS handshakes amortize with
connection reuse; per-byte encryption on the bulk stream does not. A header
comparison costs nothing on either control or transfer requests.
Inbound (
internal/agent/auth.go):tokenGuardrouter middleware, installednext to
pathVarGuard. Three modes:disabled(default)permissiverequiredpermissiveexists because agents and callers cannot be updated in the sameinstant. Operators run there until
nvsnap_agent_auth_total{result="missing|invalid"}reaches zero -- whichproves every caller now sends a token -- then switch to
required.Outbound:
authTransportwrapspeerHTTPClient's tuned transport rather thanediting call sites, so every cascade and capture-fanout request is signed and a
peer endpoint added later is authenticated by construction.
nvsnap-mount-prepreadsNVSNAP_AGENT_TOKENand attaches it to both its POSTand its status poll; the webhook injects that env from the Secret with
optional: true.Chart:
agent.auth.{enabled,mode,token}, default off. When enabled, renders aSecret, sets the env on the DaemonSet, and passes
--auth-mode.Details worth a reviewer's attention
NVSNAP_AGENT_TOKEN), never a flag: flag valuesappear in the pod spec and in
ps, and this is a credential.from response timing.
/healthand/metricsare exempt so probes and scraping keep workingwithout distributing the token to kubelet and Prometheus.
/debug/pprof/*isdeliberately NOT exempt -- profiles expose memory contents and goroutine
state.
--auth-modefails startup rather than falling back todisabled. An operator who typos the flag should hear about it immediately,
not discover months later that the API was open.
lookupof theexisting Secret.
helm upgradere-renders every template, so a freshrandAlphaNumper upgrade would rotate the credential out from under runningcallers and cause a self-inflicted outage mid-rollout.
helm.sh/resource-policy: keepcovers the delete/reinstall case.nvsnap_agent_auth_totalis pre-initialized to zero for all three results sothe series exist on the first scrape and
absent()alerts do not misfire.Customer Release Notes
The nvsnap agent API can now require a shared bearer token. Off by default;
enable with
agent.auth.enabled=trueand roll out viaagent.auth.mode=permissivebefore switching torequired.Plan Summary
Adds one Secret (
nvsnap-agent-token) whenagent.auth.enabled=true. Noresources are created at the default settings.
Usage
Testing
Unit tests cover the guard (valid, missing, wrong token, missing
Bearerprefix, empty bearer, token prefix, wrong scheme), permissive serving while
counting, disabled installing no middleware, exempt vs gated paths, mode
parsing including typo rejection, outbound signing, the no-token case sending
nothing, non-mutation of the caller's request, and a round trip asserting a
request signed by
authTransportis accepted bytokenGuard. Testing the twohalves separately would not catch a format mismatch between them.
Chart verified by rendering both ways: default yields zero occurrences of the
Secret, env, or flag; enabled yields all four wiring points, a 48-character
generated token, and honors an explicitly set one.
helm lintpasses.Not yet exercised on a cluster. The e2e gate needs GPU nodes that are currently
held for QA.
Notes
Stacked on #519 (path hardening) -- both add router middleware at the same
line, so basing this on
mainwould conflict. Retarget tomainonce #519merges.
Companion: #490 (the API is bound to every node's IP) is the other half of the
exposure and is tracked separately, since narrowing the listener is a different
change from authenticating it.
References
Closes #486
Related Merge Requests/Pull Requests
#519
Dependencies
None.
Summary by CodeRabbit