Skip to content

fix(security): address CodeQL path-injection and log-injection alerts - #928

Merged
furykerry merged 2 commits into
masterfrom
chore/code-scanning-tier3
Sep 3, 2026
Merged

furykerry merged 2 commits into
masterfrom
chore/code-scanning-tier3

Conversation

@furykerry

Copy link
Copy Markdown
Member

Summary

Address 43 CodeQL security alerts that do not require Kubernetes version upgrades:

  • 31 go/path-injection alerts (CWE-22, CWE-23, CWE-36, CWE-73, CWE-99)
  • 12 go/log-injection alerts (CWE-117)

This is Tier 3 of the code-scanning hardening effort, following:

Changes

Path injection prevention

Add validateSafePath barriers that reject empty paths and .. segments after filepath.Clean. Applied before all filesystem operations:

  • pkg/agent-runtime/storage-cli/link/symlink.go — validate both target and link paths in CreateSymlink
  • pkg/utils/webhookutils/writer/atomic/atomic_writer.go — validate targetDir in constructor and oldTsDir from symlink read
  • pkg/utils/webhookutils/writer/fs.go — validate cert writer path in FSCertWriterOptions.validate()
  • pkg/agent-runtime/storage-cli/mountfinder/root_mount_helper.go — validate resolved mount path before existence check

Log injection prevention

Add sanitizeLogValue helper that strips \n and \r characters. Applied to all log statements using user-controlled or filesystem-derived values:

  • cmd/sandbox-gateway-cert-init/main.go — sanitize error in fatal log
  • pkg/agent-runtime/storage-cli/main.go — sanitize origin directory, mount target path, and error messages
  • pkg/agent-runtime/storage-cli/mountfinder/root_mount_helper.go — sanitize mount path in debug log
  • pkg/utils/webhookutils/writer/atomic/atomic_writer.go — sanitize target directory and data directory paths in 5 log statements
  • pkg/utils/webhookutils/writer/fs.go — sanitize file paths and error messages in cert directory operations

Testing

  • All existing tests pass
  • Updated symlink_test.go to match new error message format from validateSafePath
  • Verified with go test on all affected packages

CodeQL alert resolution

This PR resolves all 43 non-Kubernetes-version-dependent CodeQL alerts identified in the Tier 3 hardening plan. The remaining 3 alerts require cel-go version bumps tied to Kubernetes 1.34+ and are out of scope for this PR.

Test plan

  • go test ./pkg/agent-runtime/storage-cli/...
  • go test ./pkg/utils/webhookutils/writer/...
  • go test ./cmd/sandbox-gateway-cert-init/...
  • Verify CodeQL scan no longer reports these 43 alerts after merge

Add path validation barriers and log sanitization to resolve 43 CodeQL
security alerts:
- 31 go/path-injection alerts (CWE-22/23/36/73/99)
- 12 go/log-injection alerts (CWE-117)

Path validation:
- Add validateSafePath helper that rejects empty paths and ".." segments
- Apply validation before filesystem operations in symlink creation,
  atomic writer, cert writer, and mount finder
- Validate symlink targets and data-directory symlink reads

Log sanitization:
- Add sanitizeLogValue helper to strip newlines and carriage returns
- Apply to all log statements using user-controlled or filesystem paths
- Prevents log injection through crafted file names or error messages

Affected packages:
- cmd/sandbox-gateway-cert-init
- pkg/agent-runtime/storage-cli
- pkg/agent-runtime/storage-cli/link
- pkg/agent-runtime/storage-cli/mountfinder
- pkg/utils/webhookutils/writer
- pkg/utils/webhookutils/writer/atomic

Signed-off-by: 守辰 <shouchen@users.noreply.github.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>
@kruise-bot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from furykerry by writing /assign @furykerry in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov

codecov Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 47.36842% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.05%. Comparing base (b59aee6) to head (6bd4a3d).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
.../utils/webhookutils/writer/atomic/atomic_writer.go 0.00% 10 Missing ⚠️
pkg/utils/webhookutils/writer/fs.go 33.33% 4 Missing ⚠️
...ntime/storage-cli/mountfinder/root_mount_helper.go 0.00% 2 Missing and 1 partial ⚠️
pkg/agent-runtime/storage-cli/link/symlink.go 50.00% 1 Missing and 1 partial ⚠️
pkg/agent-runtime/storage-cli/main.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #928      +/-   ##
==========================================
- Coverage   83.09%   83.05%   -0.05%     
==========================================
  Files         257      259       +2     
  Lines       22536    22561      +25     
==========================================
+ Hits        18726    18737      +11     
- Misses       3095     3106      +11     
- Partials      715      718       +3     
Flag Coverage Δ
unittests 83.05% <47.36%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Share the duplicated validation and log-value sanitization helpers so the
security barriers have one tested implementation to maintain.

Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>
@furykerry
furykerry merged commit 871ff6e into master Sep 3, 2026
32 of 35 checks passed
furykerry added a commit that referenced this pull request Sep 11, 2026
* fix(security): address CodeQL path-injection and log-injection alerts (#928)

* fix(security): address CodeQL path-injection and log-injection alerts

Add path validation barriers and log sanitization to resolve 43 CodeQL
security alerts:
- 31 go/path-injection alerts (CWE-22/23/36/73/99)
- 12 go/log-injection alerts (CWE-117)

Path validation:
- Add validateSafePath helper that rejects empty paths and ".." segments
- Apply validation before filesystem operations in symlink creation,
  atomic writer, cert writer, and mount finder
- Validate symlink targets and data-directory symlink reads

Log sanitization:
- Add sanitizeLogValue helper to strip newlines and carriage returns
- Apply to all log statements using user-controlled or filesystem paths
- Prevents log injection through crafted file names or error messages

Affected packages:
- cmd/sandbox-gateway-cert-init
- pkg/agent-runtime/storage-cli
- pkg/agent-runtime/storage-cli/link
- pkg/agent-runtime/storage-cli/mountfinder
- pkg/utils/webhookutils/writer
- pkg/utils/webhookutils/writer/atomic

Signed-off-by: 守辰 <shouchen@users.noreply.github.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

* refactor(security): centralize path and log sanitizers

Share the duplicated validation and log-value sanitization helpers so the
security barriers have one tested implementation to maintain.

Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

---------

Signed-off-by: 守辰 <shouchen@users.noreply.github.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>
(cherry picked from commit 871ff6e)

* docs: add v0.6.0-alpha1 release notes (#932)

Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>
(cherry picked from commit 27c7201)

* fix(sandbox): propagate startup failures to wait-ready and ScalingLimited (#936)

(cherry picked from commit fd02f31)

* feat(poolautoscaler): enable feature gate by default (#938)

Signed-off-by: 少师 <zengyuwei.zyw@alibaba-inc.com>
Co-authored-by: 少师 <zengyuwei.zyw@alibaba-inc.com>
(cherry picked from commit 0ba722a)

* refactor(controller): generalize Pod status synchronization (#939)

(cherry picked from commit 1781b7c)

* feat(sandbox-manager): load secret config at startup and add startup hook (#857)

(cherry picked from commit 0e46dfa)

* fix(sandbox): classify unschedulable startup failures (#942)

* fix(sandbox): classify unschedulable startup failures

Signed-off-by: 少师 <zengyuwei.zyw@alibaba-inc.com>

* fix(sandbox): report startup failure reason before pod IP check in claim diagnostics

Unschedulable sandboxes have no node assignment and therefore no pod IP,
so the pod IP check masked the startup failure reason in wait timeout
diagnostics. Check startup failure reasons first so the scheduler message
is reported, and add a test covering an empty pod IP with an Unschedulable
ready condition.

Signed-off-by: 少师 <zengyuwei.zyw@alibaba-inc.com>

---------

Signed-off-by: 少师 <zengyuwei.zyw@alibaba-inc.com>
Co-authored-by: 少师 <zengyuwei.zyw@alibaba-inc.com>
(cherry picked from commit 18d90c1)

* fix: unbreak go vet and make build on master (#940)

The csi spec bump in #876 (v1.9.0 to v1.13.0) gave
csi.NodePublishVolumeRequest a protoimpl.MessageState, which embeds a
sync.Mutex. The storage-cli passes that struct by value throughout, so
go vet started reporting 18 copylocks findings and exiting 1.

Makefile declares vet as a prerequisite of both build and test-e2e, so
neither ran. Take the request by pointer instead, which is the normal
way to handle a protobuf message and removes a real struct copy from the
mount path.

make build still produced no binary after that, for a second and
unrelated reason: the target compiled a single file rather than the
package, so executeCABindings in ca_binding.go was invisible to it.
Build the package, matching what the okactl target two lines below
already does.

go vet ./... is now clean and make build produces
bin/agent-sandbox-controller.

Fixes #905

Signed-off-by: Om <omlahore47@gmail.com>
(cherry picked from commit cb37dce)

* feat(sandbox-manager): add network interface binding and reliable peer discovery (#920)

(cherry picked from commit 8765d81)

* fix(network): format IPv6 upstream addresses (#901)

(cherry picked from commit 6971312)

* build(deps): bump github/codeql-action/upload-sarif (#949)

Bumps [github/codeql-action/upload-sarif](https://github.com/github/codeql-action) from 4.37.8 to 4.37.9.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@db488dd...cdf488f)

---
updated-dependencies:
- dependency-name: github/codeql-action/upload-sarif
  dependency-version: 4.37.9
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
(cherry picked from commit 5735cf9)

* chore(skill): drop Deployment from sync-charts manifests drift check (#944)

* chore(skill): drop Deployment from sync-charts manifests drift check

Deployment manifests are no longer synchronized by the manifests
drift checker. The comparison now covers only Service, ConfigMap,
Ingress, and Secret across the controller, manager, and gateway
components. This removes Deployment-only comparison functions,
simplifies MANIFEST_SPEC to seven non-Deployment entries, and
updates the test suite and SKILL.md accordingly.

Signed-off-by: 守辰 <shouchen@example.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

* docs(skill): codify sync policy for manifests drift fixes

Document the three-rule sync policy applied when splicing DRIFT
findings into chart templates: never modify resource metadata,
append only to spec (and to data/stringData on ConfigMap/Secret)
without changing existing field values, and never replace a
Helm {{ ... }} template expression with a concrete source value.
Tests assert the three rules appear in SKILL.md.

Signed-off-by: 守辰 <shouchen@example.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

* docs(skill): drop append-only constraint on spec sync policy

The spec append-only rule is removed from the manifests sync
policy. The remaining rules are: do not modify resource metadata,
and never replace a Helm {{ ... }} template expression with a
concrete source value on spec, data, or stringData fields.

Signed-off-by: 守辰 <shouchen@example.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

* feat(skill): classify templated-field drift and index-based port compare

The manifests drift checker now reads the raw chart template file and
reclassifies a value-difference DRIFT as TEMPLATED when the affected
field is rendered through a {{ ... }} expression or a {{- range }}
block, guiding the fix toward updating the values.yaml default instead
of hardcoding a literal. Service ports are compared by index so
source order matters and chart-only ports are reported by position.
The sync policy is restated as three rules: preserve chart-managed
metadata while adding source-defined labels/annotations, keep
templates and fix values-driven drift via values.yaml defaults, and
match source order for indexed lists. Verification now requires a
manual diff review for template regressions, because a hardcoded
value that matches the source renders identically and cannot be
detected by the checker.

Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>

---------

Signed-off-by: 守辰 <shouchen@example.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>
(cherry picked from commit 51149a4)

* Tracing user operation (#950)

* feat(tracing): emit trace ID as the first JSON log field

Install a trace-first JSON zap encoder for the controller so each log line
carries the trace ID as its leading field, keeping log collectors that index
the first field stable.

Co-authored-by: WWKKAA <1938897817@qq.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>

* feat(tracing): record Sandbox conditions on controller spans

Attach each Sandbox condition as a "Status:Reason" span attribute on both the
Reconcile span (before) and the updateSandboxStatus span (after), so a single
trace shows how a Reconcile iteration drove the condition transition.

Co-authored-by: WWKKAA <1938897817@qq.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>

* feat(tracing): propagate user operations across components

Carry the user-facing operation (create/pause/resume/kill) as OTel baggage from
the API entry point through to controller Reconcile via a CR annotation, and
surface it as the traceOperation log field so cross-component logs can be
filtered by operation.

Co-authored-by: WWKKAA <1938897817@qq.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>

* fix(tracing): emit stdout spans as single-line JSON

Drop pretty-print from the std-mode stdout exporter so newline-based log
collectors treat each span as one entry instead of splitting it into fragments.
File mode keeps pretty-print since it is read directly, not line-collected.

Co-authored-by: WWKKAA <1938897817@qq.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>

* fix(tracing): 在 connect 路由入口统一标记 resume

Signed-off-by: 马赫 <mahe@bupt.edu.cn>

* fix(e2e): 按 JSON 字段校验 tracing 日志

Co-authored-by: WWKKAA <1938897817@qq.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>

* fix(tracing): address encoder and baggage propagation edge cases

Document overridden zap encoder and time flags, handle empty JSON objects, and clear stale baggage before trace injection. Add regression coverage for encoder output and baggage replacement.

Co-authored-by: WWKKAA <1938897817@qq.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>

---------

Signed-off-by: 马赫 <mahe@bupt.edu.cn>
Co-authored-by: WWKKAA <1938897817@qq.com>
(cherry picked from commit 3036e82)

---------

Signed-off-by: 守辰 <shouchen@users.noreply.github.com>
Signed-off-by: 守辰 <shouchen.zz@alibaba-inc.com>
Signed-off-by: 少师 <zengyuwei.zyw@alibaba-inc.com>
Signed-off-by: Om <omlahore47@gmail.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: 守辰 <shouchen@example.com>
Signed-off-by: 马赫 <mahe@bupt.edu.cn>
Co-authored-by: ywExcellent <yuweizeng97@163.com>
Co-authored-by: 少师 <zengyuwei.zyw@alibaba-inc.com>
Co-authored-by: Ai Ranthem <zhongtianyun.zty@alibaba-inc.com>
Co-authored-by: Om Lahore <omlahore47@gmail.com>
Co-authored-by: Shirui Cheng <34178628+cyrilcsr@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: He Ma <71874811+Liquorice-Ma@users.noreply.github.com>
Co-authored-by: WWKKAA <1938897817@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants