Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/aicr/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe change applies the shared Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
566bf81 to
ffa53bc
Compare
ffa53bc to
fea8500
Compare
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
Coverage Report ✅
Coverage BadgeCoverage unchanged by this PR. |
60502d3 to
3d94b35
Compare
install-upstream-helm.sh.tmpl and install-local-helm.sh.tmpl interpolated
{{ .Name }} and {{ .Namespace }} bare into the helm upgrade command line.
Component names are validated only as path components, which rejects
separators but not shell metacharacters, and the namespace is not
validated on this path at all, so a recipe could put arbitrary shell into
a script the operator then runs.
Apply the shellSingleQuote helper the sibling apply-crds.sh template
already uses, hoisted into a shared shellFuncs map.
Regression runs the rendered script with a payload embedding both a space
and a command substitution, and asserts helm received each value as one
argv element and that no canary file was created. Reverting the quoting
makes the payload execute.
Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
3d94b35 to
43be8f0
Compare
2 participants
Summary
Quote
{{ .Name }}and{{ .Namespace }}where the localformat install templates interpolate them into thehelm upgradecommand line, using theshellSingleQuotehelper the siblingapply-crds.shtemplate already uses.Motivation / Context
install-upstream-helm.sh.tmplandinstall-local-helm.sh.tmplbuilt theirhelm upgradeinvocation with both values bare:Neither value is constrained to shell-safe characters on the way in. Component names are validated only as path components, and
deployer.IsSafePathComponentrejects separators rather than shell metacharacters; the namespace is not validated on this path at all. The result is a generated script that an operator then runs, so a malformed or hostile recipe turns into command execution on the operator's workstation with their cluster credentials loaded.This is defense in depth rather than a remotely reachable exploit: the values come from
recipes/registry.yamland recipe overlays, both of which are reviewed. But relying on a distant validator that was written for a different purpose, and that does not actually cover this character class, is the fragile part worth removing.Fixes: #2844
Related: N/A
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
No new escaping mechanism.
shellSingleQuotealready existed inupstream_helm.goand was already registered asshqon theapply-crds.shtemplate; these two templates simply never adopted it. Single quotes rather than double are what matters here, because$(), backticks, and$VARall still expand inside double quotes.The
template.FuncMap{"shq": shellSingleQuote}literal would have appeared three times, whichgoconstflags, so it is hoisted into oneshellFuncsvar carrying the rationale in its doc comment.16 golden
install.shfixtures andstock_render_golden.yamlare regenerated; those are the only other changes.Testing
All pass; linter reports 0 issues.
The regression is execution-based, and mutation-verified. A
strings.Containsassertion on the rendered script would prove nothing about quoting, since it passes whether or not the value is safe to hand to bash. Instead each test renders a bundle whose name and namespace embed both a space and a$(touch <canary>)substitution, runs the script under a stubhelmthat records its argv, and asserts the hostile value arrived as a single argv element and that no canary file exists.Reverting just the
| shqfilters and re-running shows the payload genuinely executing, not merely word-splitting:Coverage:
pkg/bundler/deployer/localformat82.2%. No exported API added.Risk Assessment
Two template lines plus a FuncMap hoist. Every currently-shipped component name and namespace is a plain DNS-1123 label, for which single-quoting is a no-op in effect, so the rendered behavior is unchanged for real recipes. The regenerated goldens show exactly that: quotes appear, nothing else moves.
Rollout notes: N/A.
Checklist
make testwith-race)make lint)git commit -S)