Repository navigation
Conversation
docker-credential-acr-env matches registry hosts with an unanchored regular expression, so a host such as evil.azurecr.io.attacker.com is treated as Azure Container Registry and receives an AAD refresh token exchanged from the environment's service principal. Upstream has no fix. Port the helper into pkg/creds with an anchored expression. It still uses the module's token and registry packages for the exchange, so behavior for real ACR and MCR hosts is unchanged.
Fixes GO-2026-4858, GO-2026-4859 and GO-2026-6255 (buildkit), GO-2026-6253 (moby/go-archive) and removes docker/docker, which carries GO-2026-4883 and GO-2026-4887 with no fixed release. - pkg/archive: use github.com/moby/go-archive v0.3.3 and its compression package. Constants and detection are identical to docker v27.3.1. - pkg/system xattr helpers: port Lgetxattr and Lsetxattr from docker v27.3.1 onto golang.org/x/sys/unix, with a !linux stub. - builder/dockerfile BuildArgs: port unchanged, with its tests, into pkg/dockerfile/internal/buildargs. This also removes the docker daemon packages from the build. - api/types/container.HealthConfig: buildkit now exposes the moby/docker-image-spec HealthcheckConfig type directly. buildkit v0.31.2 raises the minimum versions of go-containerregistry (v0.21.6), the AWS and Azure storage SDKs, cobra and golang.org/x/*.
- Parse with linter.New instead of a zero-value or nil linter. buildkit now applies "# check=" comments to the linter for each instruction, which panicked on Dockerfiles that parsed with v0.16. - Reject COPY/ADD --exclude, COPY --parents, ADD --unpack, RUN --security=insecure and RUN --device. buildkit v0.16 failed to parse them; v0.31 accepts them, and kaniko does not implement them, so builds would otherwise silently differ (COPY --exclude would copy the excluded files).
The standalone docker-credential-acr-env in the kaniko images was still installed from github.com/chrismellard/docker-credential-acr-env, so it kept GO-2026-6225, and scanners flagged the module in the executor. - Move the fixed helper to pkg/creds/acr and port the module's token and registry packages, dropping the module from go.mod. - Tighten the hostname check to DNS labels followed by an ACR domain and accept a trailing dot. - Build cmd/docker-credential-acr-env into the images from this repo. - Bump golang-jwt/jwt/v4 to v4.5.2 (GO-2024-3250, GO-2025-3553), used by the Azure token exchange. - Build images with golang:1.25 (go.mod requires 1.25.9) and pin CI to Go 1.25. - Use debian:bookworm-slim for the certs stage; bullseye security packages now return 404 and fail the image build.
go-containerregistry v0.21 holds a pull limiter slot for every open remote blob reader. GetFSFromLayers deferred each layer's Close until the function returned, so images with more layers than the limit (4) deadlocked during extraction. Also bump go-containerregistry to v0.21.7, which closes layer readers in tarball.Write. v0.21.6 hangs the same way when saving a stage or an external COPY --from image with more than four layers as a tarball.
sudo resets PATH, so root ran the runner's older Go, which downloaded the go 1.25.9 toolchain from go.mod. That switched toolchain fails with 'no such tool "covdata"' for packages without tests under -cover.
debian:bullseye-20220328 apt sources and the ubi7 yum repos no longer resolve, so the docker reference build fails before kaniko runs. Use debian:bookworm and ubi8 (which keeps the /lib -> usr/lib symlinks issue 1039 covers), and drop the el7 package version pins.
BobbyHo
left a comment
There was a problem hiding this comment.
One follow-up from #39, which I closed in favour of this PR.
The ported helper still asks Azure AD for a token with the Azure Resource Manager audience and hands that to the registry's exchange endpoint. The maintained fork, osscontainertools/docker-credential-acr v0.9.1, asks for a token scoped to https://containerregistry.azure.net instead and uses the same exchange call, so the registry accepts it. With the anchored host check the token now only reaches real ACR hosts, so this is no longer a leak. A registry-scoped token is still worth much less than a management-plane one if that check ever fails or an ACR endpoint is compromised, so it seems worth carrying over.
Two one-line changes below. The second is needed because the client-credentials branch uses clientCredentialsConfig.Resource, which also defaults to the ARM endpoint, so the resource argument is ignored on that path today.
Lisa-Fiander
left a comment
There was a problem hiding this comment.
Can you confirm kaniko is only used as a library through envbuilder, and the kaniko executor image built from deploy/Dockerfile is never published or used?
|
/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, no findings, COMMENT. Review Finding inventoryFindings
Law analysis
Contested and acknowledgedRound logRound 1Netero + Law. Law: Split, Mandatory, so the panel did not run. Netero: 1 P3, 3 P4, 1 Nit, 2 Notes, 2 out-of-scope; all dropped from posting because the requester (IC_kwDOJPpmn88AAAABapuxow) limited the review to P0-P2. Panel planned for the next round (full panel: ging-go, kurapika, ryosuke, takumi, killua, melody, pariston, mafuuu, bisky, gon, leorio, mafu-san, wildcard chopper). Reviewed against b20ff58..a81f425. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
This PR removes docker/docker and the chrismellard ACR helper and bumps buildkit, go-containerregistry and Go. The first pass found no P0, P1 or P2 issues; lower-severity items are not posted because the review was limited to P0 to P2. The full review panel has not run.
The panel review is blocked until the PR is split. The ACR hostname check (GO-2026-6225) decides which hosts receive tokens derived from the Azure service principal, and here it sits in a diff where most lines are buildkit, go-containerregistry and docker/docker migrations. Applying only the pkg/creds, cmd and tools/tools.go changes to b20ff58 builds and passes go test ./pkg/creds/... with buildkit v0.16.0 and Go 1.24.6, so it does not need the rest of the PR.
Proposed split, each PR with its own tests:
- ACR helper port with the anchored hostname check:
pkg/creds/acr,cmd/docker-credential-acr-env, thego installline indeploy/Dockerfile, README. No dependencies. - EOL base images: the
debian:bookworm-slimcerts stage and the issue 1039 and 2049 fixtures. No dependencies. - go-containerregistry v0.21.7 with the
GetFSFromLayersclose fix and both pull-limit tests, plus the Go 1.25 toolchain (go.mod, builder image, workflows,sudo env "PATH=$PATH"). - docker/docker removal: go-archive, the xattr port, the buildargs copy, healthcheck types. After 3, because go-archive v0.3.3 requires Go 1.25.
- buildkit v0.31.2 parser changes: linter,
ParseCommands, unsupported-flag rejection. After 3, because buildkit v0.31.2 requires go-containerregistry v0.21.6 or later, which hangs on images with more than 4 layers without the close fix.
coder/envbuilder#535 can pin the top of the stack. If the PR is instead merged without squashing, commits ac3ca5a, b36b6fe and 8e293b6 pin go-containerregistry v0.21.6 without the close fix, so git bisect across them hangs on images with more than 4 layers.
🤖 This review was automatically generated with Coder Agents.
- Go 1.25 reached end of life on Aug 19 and gets no further security fixes - go.mod, builder image, and both workflows move to 1.26 - Importers such as envbuilder must build with Go 1.26.9 or newer
Removes
github.com/docker/dockerandgithub.com/chrismellard/docker-credential-acr-envfrom the module graph and bumps buildkit to v0.31.2. These are the kaniko-side fixes for the remaining envbuilder advisories, consumed by coder/envbuilder#535.Advisories fixed
moby/buildkitdocker/dockerchrismellard/docker-credential-acr-envmoby/go-archivegovulncheck ./...(reachable, non-stdlib): 46 onmain, 27 on this branch, none introduced. The 27 remaining are in go-git, containerd, grpc, x/crypto and x/text. They are out of scope here, and envbuilder already overrides them with newer versions; its source scan is 0.Changes
pkg/archive→github.com/moby/go-archive(+compression).pkg/systemxattr helpers →pkg/util/xattr_linux.go, with a non-Linux stub.builder/dockerfile.BuildArgs→pkg/dockerfile/internal/buildargs, copied verbatim with its tests.moby/docker-image-spec.# check=directives.ParseCommandsrejects stage instructions.COPY/ADD --exclude,COPY --parents,ADD --unpack,RUN --security=insecure,RUN --device) now fail with a clear "not supported by kaniko" error. Previously they failed at parse time as unknown flags, so builds that worked before still work.pkg/creds/acrports chrismellard's helper, with attribution.^(?:[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?\.)+azurecr\.(?:io|cn|de|us)$. The original regex was unanchored, so a host likeevil.azurecr.io.attacker.comreceived an AAD token exchanged from the environment's service principal.cmd/docker-credential-acr-envbuilds the binary indeploy/Dockerfile.GetFSFromLayersdeferred every layer'sCloseuntil return, so images with more than 4 layers deadlocked. Each layer is now extracted in a helper that closes its reader.tarball.Writenever closes layer readers, which hung multi-stage builds (FROM <stage>) andCOPY --from=<image>. v0.21.7 fixes it.go 1.26.9. Buildkit requires at least 1.25.9, and Go 1.25 reached end of life on Aug 19, so the module moves to the current release. Builder imagegolang:1.26, workflowsgo-version: 1.26. The certs stage moves todebian:bookworm-slim, becausebullseye-securitynow returns 404 and breaks the image build onmaintoo.sudo env "PATH=$PATH" make test. Plainsudoused the runner's older Go, which auto-downloaded go1.25.9 and then failed withno such tool "covdata"(reproduced locally).Behavior changes for users
RUNheredoc bodies, as it did before.*.localregistries no longer get an automatic plain-HTTP fallback. Onlylocalhost,*.localhostand loopback IPs do. Use--insecure/--insecure-registryfor an in-cluster HTTP registry.Validation
hack/boilerplate.sh,hack/gofmt.sh,go vet(non-integration packages) and the full unit suite as root all pass.deploy/Dockerfilebuilds. The resulting binaries contain nodocker/dockerorchrismellardmodules. The ACR helper rejects a lookalike host.stirby/kaniko-moby(pins this branch):maindevcontainer (64-layer base image) to INIT. The previous pin deadlocked at layer 5.FROM <stage>andCOPY --from=golang:1.25-bookwormbuilds. The previous pin deadlocked in "Storing source image".# check=Dockerfiles build.COPY --excludefails with the new error, matching 1.3.0, which also fails.Dockerfile_test_issue_2049moves fromdebian:bullseye-20220328todebian:bookworm, andDockerfile_test_issue_1039from ubi7 to ubi8 with the el7 version pins dropped. Both old bases' package repos are gone, so the plaindocker buildreference step failed before kaniko ran;mainfails the same way. ubi8 keeps the/lib -> usr/libsymlinks that 1039 covers.Decision log
docker/dockerandchrismellardwere ported in-tree rather than upgraded. Neither module has a fixed version: the moby fixes exist only ingithub.com/moby/moby/v2betas, and chrismellard is unmaintained. The ported code is small and copied verbatim except for the hostname check.x/cryptonot bumped: out of scope here. The Go 1.26 bump removes the earlier blocker (v0.56+ requires Go 1.26), so it can follow separately..localchange documented, not shimmed: it is an upstream security hardening, and restoring the plain-HTTP fallback would undo it.credsStore "acr"naming, and no NOTICE file for the ported Apache-2.0 code (attribution is in the file headers).