Repository navigation
fix: clear scanner findings (docker/docker, buildkit, containerd, Go 1.26.10 and more) - #536
coder[bot] wants to merge 14 commits into
Conversation
Pins coder/kaniko to 8e293b69, which removes github.com/docker/docker, bumps buildkit to v0.31.2 and go-archive to v0.3.3, and vendors a fixed ACR credential helper. Fixes GO-2026-4858, GO-2026-4859, GO-2026-6255, GO-2026-6253, GO-2026-6225, and removes docker/docker (GO-2026-4883, GO-2026-4887) from the envbuilder binary. Parse Dockerfiles with an initialized linter: buildkit v0.31 applies "# check=" comments to the linter and panicked with a nil one.
…v0.21.7 go-containerregistry v0.21 holds a pull limiter slot per open remote blob reader. The previous kaniko pin deadlocked extracting base images with more than four layers, and v0.21.6 tarball.Write hangs the same way. Also close the devcontainer feature layer reader.
…/moby/api Drops github.com/docker/docker and the github.com/moby/moby monolith from the module graph. The monolith was only selected because docker/cli (+incompatible, no go.mod) imports moby/moby/api/types/network; requiring the split moby/moby/api module resolves that import instead.
runc is only reached through a coder/coder test dependency (ory/dockertest), but its checksum is recorded in go.sum, which module scanners report.
trivy is only reached through coder/coder test packages (coderdtest via coder/preview), but its checksum is recorded in go.sum, which module scanners report. v0.72.0 is the lowest release that fixes the reported advisories; MVS also raises aws-sdk-go-v2/service/ecr, go-redis, zap and gopsutil in the binary.
containerd v1 is only reached through kaniko's pkg/executor tests, but its checksum is recorded in go.sum. v1.7.36 clears the advisories that have a v1.7 fix; the remaining CRI advisories affect every v1 release.
Re-pins the kaniko fork to coder/kaniko@76b64cc0, which imports platforms from github.com/containerd/platforms instead of containerd v1. containerd v1 is no longer in the module graph, so the v1.7.36 requirement is dropped. The fork now declares go 1.26.9, which raises the go directive here and makes the separate toolchain line redundant.
…envbuilder-zero-vulns # Conflicts: # go.mod # go.sum
|
/coder-agents-review Please limit the review to P0, P1, and P2 issues only. |
|
Chat: Review posted | View chat Review history
deep-review v0.13.0 | Round 1 | Last posted: Round 1, 3 findings (3 P2), COMMENT. Review Finding inventoryFindings
Contested and acknowledgedLaw analysisNot run. Effective additions 86, below the 1000 threshold. Round logRound 1Full panel (round 1). Netero first pass: 1 P2, 2 Notes; panel proceeded. Panel: ging-go, ryosuke, pariston, mafuuu, bisky, zoro, gon, leorio, mafu-san, kurapika, wildcard razor (11; multi-domain because the change is a supply-chain security fix). Requester comment IC_kwDOJPSNMM8AAAABcH4Vzw asked to limit the review to P0-P2, so P3 and below were not posted. Orchestrator verified CRF-1 (git ls-remote), CRF-4 (kaniko diff 61a5b7da..76b64cc0 pkg/creds/acr/token.go and docs.azure.cn ACR scope), CRF-5 (go list -m all at head). Gon comment findings moved out of scope: comment lines unchanged by the diff. 3 P2 posted. Reviewed against 77452e6..37667dd. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR re-pins the kaniko fork, migrates the integration tests to moby/moby/client, and raises dependency floors to clear scanner findings. 3 P2 findings. As requested on this PR, findings below P2 are not posted.
Out of scope (needs a ticket or explicit acceptance by a human):
coder/kaniko pkg/creds/acr/token.go:39: the hard-codedhttps://containerregistry.azure.netaudience is the root cause of the Azure China finding below; sovereign-cloud ACR authentication fails until kaniko picks the audience per cloud.coder/kaniko go.mod:3: the fork'sgo 1.26.9directive forces every module that imports envbuilder to require Go 1.26.9, and thereplacedoes not apply to importers, so a library consumer keeps its own kaniko pin and containerd v1 until it re-pins..github/workflows/ci.yaml: no job runs govulncheck, trivy, or grype, so a later dependency bump can bring scanner findings back without any check failing..github/workflows/ci.yaml:40: CI installs Go~1.22and relies onGOTOOLCHAIN=autoto download 1.26.9; a runner withGOTOOLCHAIN=localfails the build.
🤖 This review was automatically generated with Coder Agents.
| // There are a few options we need added to Kaniko! | ||
| // See: https://github.com/GoogleContainerTools/kaniko/compare/main...coder:kaniko:main | ||
| replace github.com/GoogleContainerTools/kaniko => github.com/coder/kaniko v0.0.0-20261008191411-61a5b7da7ea3 | ||
| replace github.com/GoogleContainerTools/kaniko => github.com/coder/kaniko v0.0.0-20261011010809-76b64cc01bec |
There was a problem hiding this comment.
P2 [CRF-1] The kaniko replace pins coder/kaniko@76b64cc01bec, which exists only on the unmerged branch stirby/kaniko-containerd-platforms (refs/pull/40/head), not on kaniko main (a51d65b5). (Netero)
If coder/kaniko#40 is squash-merged or rebased and the branch is deleted, the commit stays fetchable only from proxy.golang.org's cache, so GOPROXY=direct builds fail and envbuilder releases ship kaniko code that is not on the fork's main. Keep this PR unmerged until the replace points at a commit on kaniko main.
🤖
| // There are a few options we need added to Kaniko! | ||
| // See: https://github.com/GoogleContainerTools/kaniko/compare/main...coder:kaniko:main | ||
| replace github.com/GoogleContainerTools/kaniko => github.com/coder/kaniko v0.0.0-20261008191411-61a5b7da7ea3 | ||
| replace github.com/GoogleContainerTools/kaniko => github.com/coder/kaniko v0.0.0-20261011010809-76b64cc01bec |
There was a problem hiding this comment.
P2 [CRF-4] The kaniko re-pin brings in a change that hard-codes the ACR token audience to https://containerregistry.azure.net, so ACR authentication in Azure China likely stops working. (Kurapika)
Between 61a5b7da and 76b64cc0, pkg/creds/acr/token.go:39 changed from settings.Environment.ResourceManagerEndpoint to that constant for the client secret, federated token, and managed identity paths, with a comment saying it is "One value for every Azure cloud." Azure China documents the ACR scope as https://containerregistry.chinacloudapi.cn/.default (docs.azure.cn, "Configure Registry Acceptance of Microsoft Entra Authentication Scopes"), while the token is still requested from that cloud's ActiveDirectoryEndpoint, so private base images, features, and cache repos on *.azurecr.cn that creds.GetKeychain() authenticates (envbuilder.go:500, devcontainer/devcontainer.go:487, devcontainer/features/features.go:29) would fail where the ARM audience worked. This is inferred from the docs and was not run against a China tenant. Fix in kaniko by choosing the audience from settings.Environment and keeping ResourceManagerEndpoint for clouds with no known ACR audience, then re-pin.
🤖
| // There are a few options we need added to Kaniko! | ||
| // See: https://github.com/GoogleContainerTools/kaniko/compare/main...coder:kaniko:main | ||
| replace github.com/GoogleContainerTools/kaniko => github.com/coder/kaniko v0.0.0-20261008191411-61a5b7da7ea3 | ||
| replace github.com/GoogleContainerTools/kaniko => github.com/coder/kaniko v0.0.0-20261011010809-76b64cc01bec |
There was a problem hiding this comment.
P2 [CRF-5] The description and commit f403dc1 say containerd v1, docker/docker, and the moby/moby monolith were removed from the module graph, but go list -m all at head still lists github.com/containerd/containerd v1.4.13 and github.com/docker/docker v28.5.2+incompatible. (Mafu-san, Razor, Pariston)
go mod graph shows gvisor requiring containerd v1.4.13, older than the v1.7.29 this PR started from, and trivy@v0.72.0 requiring docker/docker v28.5.2. Neither is in go.sum or the binary, so the six scanners in the table are unaffected, but a scanner or auditor that reads the build list reports both and contradicts the PR's stated result. Change "removed from module graph" to "absent from go.mod, go.sum, and the binary", and state that the build list still names both versions.
🤖
stirby
changed the title
Clears the reachable advisories and the package/container scanner findings in envbuilder by upgrading or removing the flagged dependencies. Supersedes #535, whose commits are all included here.
Before merge: the kaniko
replacepoints at76b64cc0(coder/kaniko#40), which is not on kanikomainyet. coder/kaniko#41 lands it there; this PR gets re-pinned to #41's merge commit once it merges.Scan results (
9420f6f)maingovulncheck ./...(reachable)trivy fs,trivy image,grype dir:.grypeon the imageThe one finding left is GO-2026-5932 (
golang.org/x/crypto/openpgp). The advisory covers every x/crypto version and has no fix. envbuilder never imports openpgp (go list -depson every package, including tests). x/crypto master has deleted openpgp, so it clears with the next x/crypto release once vuln.go.dev marks it fixed.Changes
docker/docker, the chrismellard ACR helper and containerd v1. buildkit v0.16.0 → v0.31.2, go-containerregistry → v0.21.7, go-archive → v0.3.3 (fix: drop docker/docker and the chrismellard ACR helper, bump buildkit to v0.31.2 kaniko#38, feat: Make cache TTL configurable #40).devcontainer.UserFromDockerfile: passes a buildkit linter toParseInstructionWithLinter. buildkit v0.31 panics without one on# check=Dockerfiles. Test:TestUserFromDockerfile_BuildArgs/LintCheckComment.features.Extract: closes the feature layer reader. go-containerregistry v0.21 holds a pull slot until the reader closes.moby/moby/clientandmoby/moby/api, which removesdocker/dockerand themoby/mobymonolith fromgo.mod.Behavior changes
*.localregistries no longer fall back to plain HTTP (go-containerregistry v0.21). Plain-HTTP in-cluster registries needENVBUILDER_INSECURE=true.COPY/ADD --exclude,COPY --parents,ADD --unpack,RUN --security=insecure,RUN --device) fail with "not supported by kaniko". 1.3.0 also failed on them.Validation
make test(unit and integration, local registry) passes at9420f6f.make lint/go,make fmtandmake genproduce no diff.scripts/build.sh --arch=amd64and scanned with govulncheck (source and binary), trivy and grype, as above.COPY --from=<image>builds work, and# check=Dockerfiles build.TestLogsfailure ("container never exited") on a cold workspace; it passed in the next full run and 5 isolated runs. It doesn't touch the changed modules.Decision log
moby/moby/v2betas, and chrismellard is unmaintained. The ACR helper was ported with an anchored hostname regex.tarball.Writeleaks pull slots and hangs multi-stage builds..localHTTP fallback documented, not shimmed: it's an upstream security hardening.coder/trivy: the fork lacks the fixes and would only hide findings behind a different module path.