Repository navigation
fix(bundler): quote recipe-supplied name and namespace in install.sh - #2853
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 BadgeMerging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
3d94b35 to
43be8f0
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>
d17c667 to
8eba4e8
Compare
|
Heads up @mchmarny: I force-pushed over your No content of yours was lost. I checked before saying so: the only commits unique to The end state matches what the repo asks for — CLAUDE.md requires rebasing rather than a merge commit or the "Update branch" button, since squash-merge composes the commit from the PR title. But that is a justification for the result, not for how I got there: my sync script read the remote head and leased against that value, which only proves nothing changed between the read and the push. It could not tell that what it read was your commit rather than mine. I have since changed it to refuse the push outright when the remote carries any commit the local branch does not contain. Sorry for the churn. If you were mid-something on this branch, tell me and I will restore |
These two fixtures arrived on main after this branch was cut, so the rebase brought in install.sh goldens that predate the quoting change. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
The two golden fixtures collided on derived values: every conflict hunk was a sha256 of bundle-info.yaml or checksums.txt. Neither side's digest describes the merged tree, so both goldens were regenerated with make update-goldens rather than resolved by hand. Verified the result carries both sides: it is byte-different from each pure side, our slinky-slurm and topograph values digests are present, and main's #2853 quoting shows in the rendered install.sh fixtures as 'gpu-operator'. catalog_parity_golden.yaml auto-merged and regeneration left it unchanged, which is the check that the automatic resolution was correct. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
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)