Skip to content

vendor: github.com/moby/go-archive main / v0.3.0-dev#7111

Draft
thaJeztah wants to merge 4 commits into
docker:masterfrom
thaJeztah:bump_go_archive
Draft

vendor: github.com/moby/go-archive main / v0.3.0-dev#7111
thaJeztah wants to merge 4 commits into
docker:masterfrom
thaJeztah:bump_go_archive

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

vendor: github.com/moby/go-archive main / v0.3.0-dev

full diff: moby/go-archive@v0.2.0...91b7e54

vendor: github.com/moby/go-archive with hardening

full diff: ctalledo/go-archive@91b7e54...df2ce62

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

full diff: moby/go-archive@v0.2.0...91b7e54

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
full diff: ctalledo/go-archive@91b7e54...df2ce62

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟡 NEEDS ATTENTION

This vendor update of github.com/moby/go-archive introduces significant security hardening — os.Root-based filesystem operations with openat(2) semantics, filepath.IsLocal path validation, and improved AUFS whiteout handling. The overall direction is a clear improvement.

Two security findings in the new code warrant attention before merging:

  1. TOCTOU in opaque-whiteout walk (diff.go:151): os.RemoveAll(p) uses an absolute path outside the os.Root containment boundary, unlike every other removal in the same function.
  2. Empty tar entry name (diff.go:62): A tar entry with an empty name normalizes to "." (the extraction root), passes filepath.IsLocal, and can overwrite root directory metadata.

Comment thread vendor/github.com/moby/go-archive/diff.go Outdated
Comment thread vendor/github.com/moby/go-archive/diff.go Outdated
@thaJeztah
thaJeztah requested a review from docker-agent July 17, 2026 11:46

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

This vendor update introduces -based containment for tar extraction (using semantics), replaces manual path-traversal checks with , hardens device-number overflow detection, and updates the implementation to use a parent-directory file descriptor. The security model is sound and the hardening changes are a clear improvement over the previous string-prefix checks.

Comment thread vendor/github.com/moby/go-archive/archive_unix.go Outdated

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

This PR vendors github.com/moby/go-archive from v0.2.0 to a pre-release dev version (v0.2.1-0.20260716170833 via a replace directive pointing to ctalledo's fork). The core changes are security hardening of the tar extraction path: Unpack and createTarFile now use Go's os.Root API (openat-backed bounded filesystem operations) instead of raw os.* calls with manual prefix checks, and path traversal validation is strengthened with filepath.IsLocal after path.Clean.

The hardening changes are well-structured. The os.Root API provides the primary containment guarantee for all path operations. All drafter findings were low-severity and, on deeper analysis, confirmed to be either correct behavior or no-bug outcomes. No high or medium severity issues were identified.

Note: vendor.mod contains a // FIXME comment indicating this uses a personal fork (ctalledo/go-archive) for testing moby/go-archive#45. This is explicitly called out in the PR description and is expected for a dev-version vendor bump.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🔴 CRITICAL

This PR vendors an updated github.com/moby/go-archive (v0.3.0-dev) with security hardening, but the vendor.mod includes a replace directive that substitutes the canonical module with a personal testing fork (ctalledo/go-archive). This was intentionally marked FIXME and must be removed before merging.

Lower-confidence findings (not posted inline)

None

Comment thread vendor.mod Outdated
)

// FIXME(thaJeztah): testing https://github.com/moby/go-archive/pull/45
replace github.com/moby/go-archive => github.com/ctalledo/go-archive v0.2.1-0.20260717121834-2bc39549db4e

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[high] replace directive points to a personal testing fork instead of the canonical moby/go-archive

The PR adds a replace directive in vendor.mod that redirects github.com/moby/go-archive to github.com/ctalledo/go-archive v0.2.1-0.20260717121834-2bc39549db4e. The adjacent FIXME(thaJeztah) comment explicitly marks this as a temporary testing state while waiting for moby/go-archive#45 to land.

Impact if merged: Every build of docker/cli at this commit uses code from the unreviewed personal fork (ctalledo/go-archive) rather than the canonical moby/go-archive — a supply-chain risk that defeats the stated goal of the PR. The require line correctly points to the official pre-release (v0.2.1-0.20260716170833-91b7e546daf2), but the replace directive overrides it at build time.

Resolution: Remove the replace directive (and the accompanying FIXME comment) before merging, once PR #45 in moby/go-archive is merged and the canonical module is published.

Confidence Score
🟢 strong 100/100

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is temporary; upstream changes have not been merged yet

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood — I agree this should remain until moby/go-archive#45 is merged upstream. My finding was to flag this as blocking merge, not to suggest removing it from the branch during testing.

Once the upstream PR lands and the canonical module is published, this replace directive should be removed before merging this PR into the main branch. Until then, the temporary fork is the correct approach for validation.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
thaJeztah requested a review from docker-agent July 20, 2026 12:47

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

This PR vendors github.com/moby/go-archive at a dev pre-release (v0.3.0-dev / main) and applies a security-hardening layer from ctalledo/go-archive. The changes are well-structured and represent a clear improvement over the previous extraction logic.

Hardening changes reviewed:

  • Unpack/UnpackLayer now use os.Root (openat(2)-based) for all extraction operations, preventing symlink-escape attacks at the OS level.
  • Path validation replaces the old filepath.Clean + filepath.Rel breakout check with filepath.IsLocal + path.Clean, which is more robust.
  • chmodNoSymlink uses fchmodat(2) with AT_SYMLINK_NOFOLLOW (with documented fallback for older kernels that matches or improves on previous os.Chmod behavior).
  • lchtimes is now fd-based (unix.UtimesNanoAt via parent directory FD), correctly scoped within the extraction root.
  • fsRootPath/walkLinks provide correct symlink bounding for operations os.Root cannot handle (mknod, xattrs) — absolute symlink targets are correctly re-bounded to root on the subsequent iteration.
  • The unpackedPaths key change (absolute → POSIX-relative) is consistent: path.Clean removes trailing slashes before storage, and the opaque-whiteout lookup uses matching normalization.
  • Error-handling cleanups (_ = writer.Close(), combined defers) are all correct.

No bugs were found in the introduced + lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants