chore: fix TAVA vulnerabilities - #38
Closed
emilyzhangbg wants to merge 6 commits into
Closed
Conversation
The generated client buffers every response whole before parsing, so an oversized or endless body was bounded only by available memory. Guard the response body at the transport seam, which also covers the SDK's own unmarshal calls and the signing key fetch without touching generated code. Addresses NSPECT-ZJGA-VOED threat 3 (resource exhaustion during response deserialization): byte ceiling via a bounded reader, JSON nesting ceiling, and typed errors raised outside the retry loop so an oversized body cannot drive a retry loop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Emily Zhang <emizhang@nvidia.com>
Path parameters are percent-escaped by the generated client, but dot
segments survive escaping and are resolved away when the operation path is
joined to the base URL: a node UUID of ".." turned GET /v1/nodes/{id} into
GET /v1/, and an empty one into a request against the collection. Validate
identifiers through one shared nvfleetint.ValidateResourceID, called from
both the SDK and the CLI, so caller-supplied input cannot change which
endpoint is called.
Also bound page and page size in the SDK. Those limits previously existed
only in the CLI flag layer, leaving them unenforced for programs that use
the SDK directly.
Addresses NSPECT-ZJGA-VOED threat 5, requirement 1. Requirements 2 and 3 of
that threat are server-side controls and are out of scope for this repo. The
threat as written describes corrupting backend records through request
bodies; no command here writes, so this covers the reachable surface, which
is the path and query parameters of read requests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Emily Zhang <emizhang@nvidia.com>
Go leaves MaxConnsPerHost unlimited, so a concurrent SDK embedder opened one socket per in-flight call and could turn a single program into a load spike on a shared backend. Cap connections per host in the shared hardened transport, and raise MaxIdleConnsPerHost to match so the cap does not cost the backend extra TLS handshakes through connection churn. A caller's stricter settings are preserved, and WithHTTPClient remains the escape hatch for a different pool. Addresses NSPECT-ZJGA-VOED threat 6, requirement 3. The other two parts of that requirement were already in place: per-request timeouts through Client.requestContext, and exponential backoff with jitter and Retry-After in retryingDoer. Both now have tests asserting the property rather than relying on it incidentally. Requirements 1 and 2 of this threat are server-side rate limiting and are out of scope for this repo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Emily Zhang <emizhang@nvidia.com>
Neither installer bounded its downloads: a dropped or throttled connection left curl or Invoke-WebRequest waiting on defaults, and a single failed request aborted the install with no second attempt and no alternative source. A degraded GitHub Releases could therefore hang a provisioning pipeline instead of failing it. Both scripts now fetch through one retry helper with explicit connect and request timeouts, a bounded attempt count, and exponential backoff capped at a maximum delay. Retries are limited to transient failures: transport errors and 408/425/429/5xx are retried, while a 404 fails immediately rather than delaying a certain failure. Exhaustion logs the reason and the attempt count and exits non-zero. Adds optional fallback sources: NVFLEETINT_BASE_URL overrides the download root, NVFLEETINT_FALLBACK_BASE_URL is tried after the primary is exhausted, and NVFLEETINT_CACHE_DIR is read before the network. The cache is populated only after checksum verification, so a later run never reuses an artifact the current one could not vouch for. Mirror URLs must be https, with plain http allowed only for loopback, matching the rule the SDK already applies to its own base URL, so adding a mirror cannot downgrade the transport. Addresses NSPECT-ZJGA-VOED threat 9, all three requirements. install.sh was verified against a mock release server covering retry with backoff, non-retryable 404, attempt exhaustion, request timeout, mirror fallback, cache read and populate, and input rejection. install.ps1 mirrors it but is unverified by execution: no PowerShell is available on the development machine and CI has no Windows job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Emily Zhang <emizhang@nvidia.com>
The generated client decodes into strong Go types, so a value of the wrong JSON kind already fails to unmarshal. What that did not catch was a well-formed payload carrying values the contract forbids: an IP field that is not an address, a hostname carrying terminal escape sequences, an alert severity outside the enum. Those reached the operator as rendered fleet state, which is what a tampered or compromised backend would rely on. Validate every JSON response before it is mapped into domain types, at all 16 SDK entry points that decode one. Control characters are refused in any string, most importantly ESC, which starts a sequence that could rewrite an operator's terminal; tab, newline, and return stay allowed because alert messages legitimately wrap and the table renderer already collapses them. Fields the contract constrains are checked by name wherever they appear: publicIP and privateIP must parse as addresses, hostname must be within the DNS length and character set, and severity and state must fall in the generated enums, so regenerating from an updated spec widens them automatically. The walk is streaming rather than decoding into an interface tree, so validating a large response does not add a second copy of it to memory and undo the bounds added for threat 3. Bodies that are not JSON — the CSV and ZIP report payloads — are skipped. Addresses NSPECT-ZJGA-VOED threat 2, requirement 3, and requirement 2 in part. This validates the constraints openapi.yaml actually declares for the named fields; it is not a general JSON Schema engine bound to each operation's response schema, which would need a schema validator dependency and the spec embedded in the binary. Verified against the live dev backend: overview, node list, node describe, node health, alert list, alert timeline, event list, tag list, computezone list, nodegroup list, report inventory, and report error all pass unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Emily Zhang <emizhang@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe SDK adds shared request and response validation, bounded response handling, and transport connection limits. The shell and PowerShell installers add configurable sources, retries, caching, checksum verification, and deterministic failure handling. ChangesSDK hardening
Installer resilience
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
install.sh (1)
290-295: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrite cache entries atomically.
cpwrites into the final cache path in place. If a second install runs concurrently with the sameNVFLEETINT_CACHE_DIR,obtain_filecan read a half-written file. That run then fails checksum verification instead of installing.Copy into a temporary name in the same directory, then rename.
♻️ Proposed fix for atomic cache writes
cache_store() { local name=$1 [[ -n "$cache_dir" ]] || return 0 mkdir -p "${cache_dir}/${tag}" - cp "${work_dir}/${name}" "${cache_dir}/${tag}/${name}" + local tmp="${cache_dir}/${tag}/.${name}.$$" + cp "${work_dir}/${name}" "$tmp" && mv -f "$tmp" "${cache_dir}/${tag}/${name}" || rm -f "$tmp" }🤖 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 `@install.sh` around lines 290 - 295, Update cache_store to write each cache entry to a temporary file within the target ${cache_dir}/${tag} directory, then atomically rename it to the final ${name} path after the copy completes. Preserve the existing cache_dir guard and source/destination naming, and ensure temporary files are cleaned up if the copy fails.nvfleetint/client.go (1)
284-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClamp
MaxIdleConnsPerHostto the effectiveMaxConnsPerHost.The comment on Line 291 states that idle reuse should track
MaxConnsPerHost. The code does not do that when a caller sets a stricterMaxConnsPerHostand leavesMaxIdleConnsPerHostat zero. Example: a base transport withMaxConnsPerHost: 4andMaxIdleConnsPerHost: 0produces an idle cap of 16 against an active cap of 4. The extra idle slots are unusable, so this is only a documentation-versus-code mismatch, not a leak.
TestHardenedTransportPreservesStricterConnectionLimitssets both fields, so this combination is untested.♻️ Proposed clamp against the effective per-host limit
if cloned.MaxConnsPerHost <= 0 || cloned.MaxConnsPerHost > maxConnsPerHost { cloned.MaxConnsPerHost = maxConnsPerHost } // Zero here means net/http's default of 2, which is stricter than the cap // but only in the sense of holding fewer idle sockets; raise it so reuse // tracks MaxConnsPerHost. A caller who deliberately set a higher number // keeps it only up to the cap, since more idle connections than the // per-host limit cannot be used anyway. - if cloned.MaxIdleConnsPerHost <= 0 || cloned.MaxIdleConnsPerHost > maxIdleConnsPerHost { - cloned.MaxIdleConnsPerHost = maxIdleConnsPerHost - } + idleCap := min(maxIdleConnsPerHost, cloned.MaxConnsPerHost) + if cloned.MaxIdleConnsPerHost <= 0 || cloned.MaxIdleConnsPerHost > idleCap { + cloned.MaxIdleConnsPerHost = idleCap + }🤖 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 `@nvfleetint/client.go` around lines 284 - 296, Update the MaxIdleConnsPerHost clamping logic in the transport-hardening flow to use the effective cloned.MaxConnsPerHost as its upper bound, including when the caller leaves MaxIdleConnsPerHost at zero. Preserve stricter caller limits while ensuring the resulting idle limit never exceeds the effective per-host connection limit.nvfleetint/client_test.go (1)
611-639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert growth on observed delays, not on the locally computed base.
The test recomputes the production formula in
baseat Line 613. Lines 627-632 and Lines 637-638 then assert onbasealone. Those three checks cannot fail, becausebaseis derived in the test and is already clamped tomaximumRetryDelayat Lines 614-616. A regression indefaultRetryDelaygrowth or capping would still pass.The jitter-window check at Lines 620-625 is the only assertion that exercises the production function. Track the observed minimum per attempt and assert that it increases until the cap.
💚 Proposed assertion on observed delays
var previousBase time.Duration + var previousObservedMin time.Duration for attempt := 1; attempt <= 8; attempt++ { base := initialRetryDelay << (attempt - 1) if base > maximumRetryDelay { base = maximumRetryDelay } // Jitter spreads each delay over 50%-150% of the base. low, high := base/2, base*3/2 + observedMin := time.Duration(math.MaxInt64) for range samples { delay := defaultRetryDelay(attempt, nil) if delay < low || delay > high { t.Fatalf("attempt %d delay %v outside [%v, %v]", attempt, delay, low, high) } + if delay < observedMin { + observedMin = delay + } } - if attempt > 1 && base < previousBase { - t.Fatalf("attempt %d base %v shrank from %v", attempt, base, previousBase) + // Growth is asserted on measured delays, and only while the base is + // still below the cap. + if attempt > 1 && previousBase < maximumRetryDelay && observedMin <= previousObservedMin { + t.Fatalf("attempt %d observed minimum %v did not grow from %v", + attempt, observedMin, previousObservedMin) } - if base > maximumRetryDelay { - t.Fatalf("attempt %d base %v exceeded the cap %v", attempt, base, maximumRetryDelay) - } previousBase = base + previousObservedMin = observedMin } - - // The growth has to actually happen, not just stay within bounds. - if initialRetryDelay<<3 <= initialRetryDelay { - t.Fatal("retry delay does not grow between attempts") - } }The diff adds a
mathimport.🤖 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 `@nvfleetint/client_test.go` around lines 611 - 639, Update the retry-delay test around defaultRetryDelay to track the minimum observed delay from each attempt’s samples, using math.MinInt64 or the appropriate existing numeric sentinel. Remove the assertions based solely on the locally computed base and instead assert that observed minimum delays increase between attempts until the maximumRetryDelay cap is reached, while preserving the existing jitter-window validation.nvfleetint/responseguard.go (1)
206-208: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueClamp
depthat zero on a closing brace or bracket.An unmatched
}or]drivesdepthbelow zero. Later nesting is then under-counted againstmax. Example: a body that starts with four stray]characters and then opens 68 objects reports a depth of 64 and passes a limit of 64.Such a body is malformed, so
encoding/jsonfails it anyway. The clamp keeps the counter monotonic for the guard and costs one comparison.🛡️ Proposed clamp
case '}', ']': - s.depth-- + if s.depth > 0 { + s.depth-- + } }🤖 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 `@nvfleetint/responseguard.go` around lines 206 - 208, Update the closing-brace/bracket handling in the depth-scanning logic to decrement depth without allowing it to become negative; clamp depth at zero after processing `}` or `]`. Preserve the existing maximum-depth enforcement and handling for valid nesting.
🤖 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 `@install.sh`:
- Around line 270-274: Update obtain_file and the checksum-fetch flow so
checksums.txt is always retrieved from the network and never served from or
stored in the cache; remove the cache_store call for the checksum file. Keep
cache usage limited to the requested artifact, and when creating
NVFLEETINT_CACHE_DIR, apply restrictive directory permissions.
---
Nitpick comments:
In `@install.sh`:
- Around line 290-295: Update cache_store to write each cache entry to a
temporary file within the target ${cache_dir}/${tag} directory, then atomically
rename it to the final ${name} path after the copy completes. Preserve the
existing cache_dir guard and source/destination naming, and ensure temporary
files are cleaned up if the copy fails.
In `@nvfleetint/client_test.go`:
- Around line 611-639: Update the retry-delay test around defaultRetryDelay to
track the minimum observed delay from each attempt’s samples, using
math.MinInt64 or the appropriate existing numeric sentinel. Remove the
assertions based solely on the locally computed base and instead assert that
observed minimum delays increase between attempts until the maximumRetryDelay
cap is reached, while preserving the existing jitter-window validation.
In `@nvfleetint/client.go`:
- Around line 284-296: Update the MaxIdleConnsPerHost clamping logic in the
transport-hardening flow to use the effective cloned.MaxConnsPerHost as its
upper bound, including when the caller leaves MaxIdleConnsPerHost at zero.
Preserve stricter caller limits while ensuring the resulting idle limit never
exceeds the effective per-host connection limit.
In `@nvfleetint/responseguard.go`:
- Around line 206-208: Update the closing-brace/bracket handling in the
depth-scanning logic to decrement depth without allowing it to become negative;
clamp depth at zero after processing `}` or `]`. Preserve the existing
maximum-depth enforcement and handling for valid nesting.
🪄 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: 75409de7-fa46-4806-a02b-c9332c9702f5
📒 Files selected for processing (26)
cmd/nvfleetint/alert.gocmd/nvfleetint/node.gocmd/nvfleetint/node_health.gocmd/nvfleetint/node_test.goinstall.ps1install.shinternal/clihelpers/pagination.gonvfleetint/alert.gonvfleetint/auth.gonvfleetint/client.gonvfleetint/client_test.gonvfleetint/computezone.gonvfleetint/event.gonvfleetint/node.gonvfleetint/node_health.gonvfleetint/nodegroup.gonvfleetint/overview.gonvfleetint/params.gonvfleetint/params_test.gonvfleetint/report.gonvfleetint/responseguard.gonvfleetint/responseguard_test.gonvfleetint/responsevalidate.gonvfleetint/responsevalidate_test.gonvfleetint/tag.gonvfleetint/verify_test.go
Comment on lines
+270
to
+274
| if [[ -n "$cache_dir" && -f "${cache_dir}/${tag}/${name}" ]]; then | ||
| echo "Using cached ${name} from ${cache_dir}/${tag}" | ||
| cp "${cache_dir}/${tag}/${name}" "$dest" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Cache hits bypass the integrity anchor.
obtain_file returns a cached artifact without any check at Line 272. cache_store also caches checksums.txt at Line 294. When both files are cached, verification compares one cache entry against another cache entry. No trusted source is consulted.
Any writer to NVFLEETINT_CACHE_DIR can then install an arbitrary binary. Shared cache directories on CI runners make this reachable.
Fetch checksums.txt from the network on every run, and use the cache only for the artifact. Also restrict the cache directory permissions when the installer creates it.
🔒 Proposed change: never trust a cached checksum list
obtain_file() {
local name=$1
local dest="${work_dir}/${name}"
local root
- if [[ -n "$cache_dir" && -f "${cache_dir}/${tag}/${name}" ]]; then
+ if [[ -n "$cache_dir" && "$name" != "$checksum_file" && -f "${cache_dir}/${tag}/${name}" ]]; then
echo "Using cached ${name} from ${cache_dir}/${tag}"
cp "${cache_dir}/${tag}/${name}" "$dest"
return 0
fi cache_store() {
local name=$1
[[ -n "$cache_dir" ]] || return 0
- mkdir -p "${cache_dir}/${tag}"
+ mkdir -p -m 0700 "${cache_dir}/${tag}"
cp "${work_dir}/${name}" "${cache_dir}/${tag}/${name}"
}Then drop the cache_store "$checksum_file" call at Line 323.
Also applies to: 290-295
🤖 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 `@install.sh` around lines 270 - 274, Update obtain_file and the checksum-fetch
flow so checksums.txt is always retrieved from the network and never served from
or stored in the cache; remove the cache_store call for the checksum file. Keep
cache usage limited to the requested artifact, and when creating
NVFLEETINT_CACHE_DIR, apply restrictive directory permissions.
4 tasks
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
1 participant
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.
Description
Checklist
git commit -s) per the Contributing Guidelines.Summary by CodeRabbit
New Features
Bug Fixes