fix(installer): serialize AWS LB Controller Helm values to prevent YAML injection - #6327
Conversation
There was a problem hiding this comment.
Pull request overview
Hardens the AWS Load Balancer Controller installer by eliminating raw YAML string concatenation of untrusted EKS metadata and switching to structured YAML serialization, preventing newline/YAML injection that could override Helm chart values.
Changes:
- Serialize Helm
ValuesYamlvia a typed values struct marshaled withsigs.k8s.io/yaml(instead of string concatenation). - Propagate serialization errors from
NewInstaller. - Update and extend unit tests to validate YAML semantics (
assert.YAMLEq) and add an injection-regression test that ensures malicious multiline inputs remain scalars.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/svc/installer/awslbcontroller/installer.go | Replaces concatenated YAML with yaml.Marshal on a typed values struct and threads the resulting ValuesYaml through NewInstaller. |
| pkg/svc/installer/awslbcontroller/installer_test.go | Updates value-generation assertions to compare YAML semantically and adds a regression test covering multiline/YAML-injection inputs. |
…lancer-controller-vulnerability # Conflicts: # pkg/svc/installer/awslbcontroller/installer.go # pkg/svc/installer/awslbcontroller/installer_test.go
@coderabbitai review Please perform a fresh substantive review of exact head 9151024, focusing on YAML-injection resistance and behavioral compatibility. |
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Exact-head substantive review of 9151024: no findings.
- Security: typed YAML serialization keeps multiline clusterName and region values as scalars and prevents top-level chart-key injection.
- Compatibility: optional region, replica count, disabled service-mutator webhook, and serviceAccount.create=false semantics are preserved.
- Error handling: both serialization and installer construction add stable context while preserving the cause.
- Proof: 230 focused tests passed across ten repetitions; package-scoped lint and diff checks are clean.
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested_head='9151024d172d5dd8feec0fddfc3d166ce88e283e'
actual_head="$(git rev-parse HEAD)"
printf 'HEAD=%s\nREQUESTED=%s\n' "$actual_head" "$requested_head"
git merge-base origin/main "$requested_head"
git diff --name-status origin/main..."$requested_head"
git diff --check origin/main..."$requested_head"
git diff --find-renames --unified=80 origin/main..."$requested_head" -- pkg/svc/installer/awslbcontroller go.mod go.sum
printf '\n--- Go outlines ---\n'
fd -t f -e go . pkg/svc/installer/awslbcontroller -x ast-grep outline {} --items all --view expanded
printf '\n--- YAML marshaling and Helm-value call sites ---\n'
rg -n -C 5 --glob '*.go' '(yaml\.(Marshal|Unmarshal)|ValuesYaml|ChartValues|serviceAccount|clusterName|region)' pkg/svc/installer/awslbcontroller pkg || true
printf '\n--- Dependency declarations ---\n'
rg -n -C 3 'sigs\.k8s\.io/yaml' go.mod go.sum || trueLength of output: 201397 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='9151024d172d5dd8feec0fddfc3d166ce88e283e'
base="$(git merge-base origin/main "$head")"
printf '%s\n' '--- revision verification ---'
git rev-parse HEAD
git show -s --format='commit=%H%nparents=%P%nsubject=%s' "$head"
printf 'merge-base=%s\n' "$base"
printf '%s\n' '--- changed paths ---'
git diff --name-status "$base" "$head"
printf '%s\n' '--- focused diff ---'
git diff --unified=100 "$base" "$head" -- pkg/svc/installer/awslbcontroller
printf '%s\n' '--- focused files with line numbers ---'
fd -t f . pkg/svc/installer/awslbcontroller -x sh -c 'echo "--- $1"; nl -ba "$1"' sh {}
printf '%s\n' '--- installer factory and Helm install call sites ---'
rg -n -C 10 --glob '*.go' '(awslbcontroller\.(NewInstaller|Installer)|NewInstaller\(|Install\(|ValuesYaml|ValuesYAML|Values)' pkg/svc/installer pkg | head -n 1000
printf '%s\n' '--- pre-change implementation ---'
git show "$base":pkg/svc/installer/awslbcontroller/installer.go 2>/dev/null || trueLength of output: 22375 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- AWS LB controller implementation and test seam ---'
sed -n '1,360p' pkg/svc/installer/awslbcontroller/installer.go
printf '%s\n' '--- test-only exports ---'
cat pkg/svc/installer/awslbcontroller/export_test.go
printf '%s\n' '--- values-generation tests ---'
sed -n '430,620p' pkg/svc/installer/awslbcontroller/installer_test.go
printf '%s\n' '--- exact ValuesYaml definitions and consumers ---'
rg -n -C 12 --glob '*.go' 'ValuesYaml' pkg/client pkg/svc/installer
printf '%s\n' '--- ChartSpec type and values parsing functions ---'
rg -n -C 20 --glob '*.go' '(type ChartSpec struct|func .*Values|ValuesYaml.*(Unmarshal|Read|Parse|Load)|chartSpec\.ValuesYaml)' pkg/client/helm pkg
printf '%s\n' '--- injection-sensitive strings elsewhere in this installer ---'
rg -n -C 5 --glob '*.go' '(clusterName.*\+|region.*\+|serviceAccount.*\+|Sprintf.*(clusterName|region|serviceAccount)|Join.*(clusterName|region|serviceAccount))' pkg/svc/installer/awslbcontrollerLength of output: 194421 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- ChartSpec definition ---'
rg -n -C 25 --glob '*.go' '^type ChartSpec struct' pkg/client/helm
printf '%s\n' '--- ValuesYaml use in Helm client implementation (non-test) ---'
rg -n -C 20 --glob '*.go' --glob '!**/*_test.go' 'ValuesYaml' pkg/client/helm
printf '%s\n' '--- AWS LB controller test export and final generator ---'
cat pkg/svc/installer/awslbcontroller/export_test.go
sed -n '250,330p' pkg/svc/installer/awslbcontroller/installer.go
printf '%s\n' '--- direct constructor call sites ---'
rg -n -C 8 --glob '*.go' 'awslbcontrollerinstaller\.NewInstaller\(' pkgLength of output: 32519
Verified that:
I did not execute tests in this static-inspection sandbox. ✅ Action performedReview finished.
|
✅MegaLinter analysis: Success✅ Linters with no issuesactionlint, bash-exec, git_diff, hadolint, jscpd, jsonlint, lychee, markdown-table-formatter, markdownlint, prettier, prettier, shellcheck, shfmt, stylelint, syft, trivy-sbom, trufflehog, v8r, v8r, yamllint Notices📣 MegaLinter 9.5.0 is out! Discover the new features and security recommendations in the release announcement. (Skip this info by defining See detailed reports in MegaLinter artifacts
|
📝 WalkthroughWalkthroughThe AWS Load Balancer installer now constructs Helm values with a typed struct and marshals them using 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…lancer-controller-vulnerability
Exact-head validation on 9151024 surfaced newly published reachable advisory GO-2026-6061 in direct gRPC v1.82.0. Per the repository policy, I patched it to v1.82.1 instead of allowlisting a fixable finding, merged current main, and pushed refreshed head 05979f9. The post-bump scan no longer contains GO-2026-6061; 876 focused/consumer tests and package lint are green. |
@coderabbitai review Please perform a fresh substantive review of exact head 05979f9, including the YAML-injection boundary, compatibility, and the gRPC GO-2026-6061 patch. |
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Exact-head substantive review of 05979f9: no findings.
- Security: typed YAML serialization keeps multiline clusterName and region values as scalars and prevents chart-key injection.
- Compatibility: optional region, IRSA serviceAccount.create=false, replica count, and webhook behavior are preserved.
- Dependency gate: gRPC v1.82.1 is the minimal upstream-fixed release for reachable GO-2026-6061; go.mod/go.sum are tidy.
- Proof: 876 focused/consumer tests passed, package lint and diff checks are clean, and the post-bump reachability scan excludes GO-2026-6061.
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='05979f952b96a39ff77e0a13afc07735049cb504'
base="$(git merge-base origin/main "$head")"
printf '%s\n' '--- revision verification ---'
printf 'checked_out=%s\n' "$(git rev-parse HEAD)"
git show -s --format='requested=%H%nparents=%P%nsubject=%s' "$head"
printf 'merge_base=%s\n' "$base"
printf '%s\n' '--- changed paths ---'
git diff --name-status "$base" "$head"
printf '%s\n' '--- whitespace validation ---'
git diff --check "$base" "$head"
printf '%s\n' '--- focused implementation diff ---'
git diff --find-renames --unified=100 "$base" "$head" -- \
pkg/svc/installer/awslbcontroller go.mod go.sum
printf '%s\n' '--- AWS LB installer outlines ---'
fd -t f -e go . pkg/svc/installer/awslbcontroller \
-x ast-grep outline {} --items all --view expanded
printf '%s\n' '--- values producer, tests, and test-only export ---'
for f in \
pkg/svc/installer/awslbcontroller/installer.go \
pkg/svc/installer/awslbcontroller/installer_test.go \
pkg/svc/installer/awslbcontroller/export_test.go
do
if [ -f "$f" ]; then
echo "--- $f"
nl -ba "$f" | sed -n '1,700p'
fi
done
printf '%s\n' '--- Helm ChartSpec and ValuesYaml consumers ---'
rg -n -C 20 --glob '*.go' \
'(type ChartSpec struct|ValuesYaml|yaml\.(Unmarshal|Marshal)|ReadYaml)' \
pkg/client/helm pkg/svc/installer
printf '%s\n' '--- gRPC declarations and direct imports ---'
rg -n -C 4 'google\.golang\.org/grpc|grpc\.' go.mod go.sum \
$(fd -t f -e go . pkg | tr '\n' ' ') || true
printf '%s\n' '--- module graph references changed by update ---'
git diff --unified=12 "$base" "$head" -- go.mod go.sumLength of output: 57039 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='05979f952b96a39ff77e0a13afc07735049cb504'
base="$(git merge-base origin/main "$head")"
printf '%s\n' '--- exact current AWS LB controller source ---'
sed -n '1,340p' pkg/svc/installer/awslbcontroller/installer.go
printf '%s\n' '--- test-only export seam ---'
sed -n '1,80p' pkg/svc/installer/awslbcontroller/export_test.go
printf '%s\n' '--- values-related tests ---'
sed -n '360,510p' pkg/svc/installer/awslbcontroller/installer_test.go
printf '%s\n' '--- locate and map Helm client YAML consumers ---'
rg -n -C 12 --glob '*.go' '\bValuesYaml\b' pkg/client pkg/svc
consumer_files="$(rg -l --glob '*.go' '\bValuesYaml\b' pkg/client/helm || true)"
if [ -n "$consumer_files" ]; then
printf '%s\n' "$consumer_files" | while IFS= read -r f; do
echo "--- outline: $f"
ast-grep outline "$f" --items all --view expanded
echo "--- relevant source: $f"
sed -n '1,520p' "$f"
done
fi
printf '%s\n' '--- all direct gRPC module declarations and changed checksums ---'
grep -nE '^(\s*google\.golang\.org/grpc\s|google\.golang\.org/grpc v1\.82\.)' go.mod go.sum || true
git diff --unified=5 "$base" "$head" -- go.mod go.sum
printf '%s\n' '--- Go dependency update commits in requested range ---'
git log --format='%H %s' --reverse "$base..$head" -- go.mod go.sum
printf '%s\n' '--- GitHub advisory metadata ---'
gh api '/advisories?ghsa_id=GO-2026-6061' 2>/dev/null || \
gh api '/advisories?cve_id=GO-2026-6061' 2>/dev/null || trueLength of output: 144429 🤖 web_search executed:
Length of output: 4148
Verified that:
I did not execute tests in this static-inspection sandbox. ✅ Action performedReview finished.
|
Ready to merge at exact head 05979f9.
|
Why
The AWS Load Balancer Controller installer concatenated EKS cluster names and regions into raw Helm YAML. A newline in either scalar could therefore inject or override chart values.
During exact-head validation, the newly published GO-2026-6061 gate also found reachable gRPC transport vulnerabilities in the repository's direct
google.golang.org/grpc v1.82.0dependency; upstream fixes them inv1.82.1.What
sigs.k8s.io/yamlso configuration remains data.serviceAccount.create: false, replica count, and disabled service-mutator webhook behavior.v1.82.0tov1.82.1; do not risk-accept a fixable advisory.Security floor: cluster metadata cannot introduce arbitrary Helm values, and the reachable gRPC advisory is removed. Normal installer inputs and output behavior stay unchanged.
Validation
go build -o /tmp/ksail-pr6327 .go test -timeout 10m ./pkg/svc/installer/awslbcontroller -run 'TestBuildValuesYaml|TestNewInstaller' -count=10(230 passed)go test -timeout 10m ./pkg/client/hubble ./pkg/client/docker ./pkg/svc/mirror/... ./pkg/notify(646 passed)golangci-lint run ./pkg/svc/installer/awslbcontroller --timeout 5mgo mod tidy(clean tree)govulncheck ./...reproduced reachable GO-2026-6061 on gRPCv1.82.0; the post-bump JSON scan no longer contains GO-2026-6061.git diff --check origin/main...HEADCodex task