tarfs: keep mode bits and ownership from tar headers

[smoser]

Follow-up to a finding mattmoor made while reviewing #2418:

Found while probing, pre-existing and format-neutral (separate-issue material): tarfs WriteHeader(TypeDir) drops setgid/sticky and uid/gid from tar dir headers — both the tar and erofs paths serialize the degraded node state equally.

Confirmed, and it turned out to be broader than directories.

What was wrong

pkg/tarfs/fs.go never carried tar-header metadata onto its nodes:

	before
dir mode	MkdirAll(name, hdr.FileInfo().Mode().Perm()) — .Perm() masks to 0o777, so setuid/setgid/sticky were dropped
dir uid/gid	never applied, so every package-owned directory landed 0:0
file and symlink uid/gid	never applied either

The loss is format-neutral, as Matt said, because both writers read the same node: pkg/build/tarball.go builds output headers with tar.FileInfoHeader, which takes special bits from FileInfo.Mode() and Uid/Gid from FileInfo.Sys(), and pkg/build/erofs.go reads that same Sys() header.

Impact, measured

An apko build of a wolfi postfix image, diffed against what the apk actually declares:

• 15 directories under /var/spool/postfix and /var/lib/postfix are uid 100 in the apk (four of them also gid 100 or 101). The built image had all 15 as 0:0.
• /usr/bin/postdrop and /usr/bin/postqueue are mode 0o2755 gid 101 — setgid to the unprivileged postdrop group. apko emitted mode 0o2755 gid 0, i.e. setgid to root. That is a privilege change rather than cosmetics, which is why this PR does not stop at the directory half.

After the fix all 17 entries match the apk, and nothing else in the image moves. apko erofs ls on an --format erofs build of the same config agrees.

The change

Two commits, one per half:

1. tarfs: keep dir mode bits and ownership from tar headers — MkdirAll as before for the permission bits, then a separate Chmod for setuid/setgid/sticky, plus a Chown.
2. tarfs: keep file and symlink ownership from tar headers — set uid/gid from the header when writeHeader builds the node.

One deliberate subtlety in the first commit: the Chmod and Chown apply only to directories that header actually created.

• InitDB creates /tmp as 1777 before any package installs, and packages ship a tmp directory header with no sticky bit — wolfi-baselayout runs chmod 1777, but the published apk records plain 0777 (checked at the raw tar-header bytes). Applying dir modes unconditionally would therefore take the sticky bit off /tmp in every wolfi-based image.
• Ancestors MkdirAll has to invent are not described by the header, so they must not inherit its ownership.

Both cases are covered by tests. Each of the four new assertions was mutation-checked — back out either half of the fix, or the newly-created-only condition, and the tests fail.

Golden fixtures

The second commit moves the golden images, and the reason is worth stating: the apks under internal/cli/testdata/packages declare their files as uid 501 gid 20, the account on the machine that built them, and apko now reproduces that faithfully. /etc/os-release is the only entry whose metadata changes in either golden image (verified by extracting and diffing the layer entries).

testdata/golden was rebuilt with the same options TestBuild passes, after first confirming that command reproduces the committed golden byte-identically when built against the unfixed code. testdata/top_image was rebuilt with the existing internal/cli/testdata/regenerate_golden_top_image.sh.

go test ./... and make lint are clean.

Deliberately not in this PR

• The streaming install path has the same gap, filed as apk: streaming install path drops directory mode bits and all uid/gid #2456. installAPKFiles in pkg/apk/apk/install.go has the identical .Perm() mask and no chown at all, for files as well as directories. In-tree the consumer is apko build-cpio, which runs BuildLayer against apkfs.DirFS; melange's package builds are not affected, since those use tarfs.New() and are covered by this PR.
• melange drops sticky/setgid on directory headers, filed as retrieveWorkspace drops setuid/setgid/sticky from directory headers melange#2642. wolfi-baselayout asks for 1777 on /tmp and /var/tmp; the published r29 apk records 0777 for both. /tmp gets 1777 in images today only because apko's baseDirectories sets it, which is why /var/tmp ships as 0777.
• A latent bug in InitDB, also in apk: streaming install path drops directory mode bits and all uid/gid #2456: stat.Mode().Perm() != e.perms compares a masked Perm() against 0o777|fs.ModeSticky, so it can never be equal for /tmp. If /tmp ever pre-exists at InitDB time, that errors with "incorrect permissions". I did not establish a reachable caller, so I left it alone.

🤖 Generated with Claude Code

[stevebeattie]

Thanks for this. Reviewed against your branch rebased onto current main (rebases clean, suite green), so this accounts for #2458 landing this morning. The golden delta re-derives exactly as described: all four regenerated layers differ from base by one line, etc/os-release 0/0 → 501/20, nothing else, and no directory entry changed anywhere.

Worth adding to the PR description

I chased that 501/20 and it's worth writing down, because the next person to look will wonder. It isn't one stray file — every file created by the melange build pipeline in the internal/cli fixtures is 501/20 (etc/os-release in both baselayout packages, both .crt files in custom-ca-certs-1), while everything melange emits itself (.PKGINFO, .melange.yaml, .SIGN.*, the SBOMs) and every directory is root/root. 501/20 is a macOS host user — uid 501, gid 20 (staff). The split is the tell: pretend-baselayout's pipeline does mkdir -p .../etc then cat > .../etc/os-release one line apart, so melange must be normalizing directory entries while taking regular files' headers from disk, where Docker Desktop's shared filesystem reports the host user. (The declared build user is uid 1000, so it isn't the container account either.)

Nothing wrong with this PR — you're faithfully propagating what the apk records, and the apk records something odd. But it does mean a melange-on-macOS local build now produces images carrying the developer's uid on files, where apko previously normalized them to root. Real packages are unaffected — I pulled wolfi-baselayout-20230201-r27.apk and all 54 entries are root/root — but it's a behaviour change for local dev workflows that's worth one line so it isn't a surprise later.

Highest priority: the created gate makes correctness depend on install order — and it's now in two places

Gating both Chmod and Chown on "we just created this" means the fix only applies when the owning package's header happens to touch the path first. Three orderings that occur in any real multi-package install lose metadata silently — no error, no log line:

WriteHeader(dir "srv/data",              0o755,  0:0)      # package A
WriteHeader(dir "srv/data",              0o2770, 100:100)  # package B, owns it
→ drwxr-xr-x 0:0        setgid bit AND ownership discarded

WriteHeader(dir "var/spool/postfix/pid", 0o755,  0:0)
WriteHeader(dir "var/spool/postfix",     0o2755, 100:101)  # its own header
→ drwxr-xr-x 0:0

WriteHeader(dir "var/log/app",           0o755,  0:0)
WriteHeader(dir "var/log/app",           0o755,  100:101)  # no special bits at all
→ drwxr-xr-x 0:0        the Chown gate alone

There's also a filesystem-vs-database divergence. lazilyInstallAPKFiles appends every header to files regardless of WriteHeader's return (install.go:341 — installed is only consulted for installedFiles), and AddInstalledPackage emits M:uid:gid:perm straight from the header (installed.go:76-113). So in the srv/data case the image ships drwxr-xr-x 0:0 while /lib/apk/db/installed claims M:100:100:2770, and runtime apk fix/verify sees a mismatch.

On the rationale in the comment — it's real, and I verified it against the actual package rather than assuming. wolfi-baselayout ships:

drwxrwxrwx root/root   tmp
drwxrwxrwx root/root   var/tmp

0777, no sticky bit. So that header genuinely would strip what InitDB set. But note the owner: root/root. The /tmp story justifies gating the Chmod; it says nothing about the Chown. InitDB never calls Chown anywhere — grep across pkg/apk/apk/implementation.go is empty, and Mkdir's node literal leaves uid/gid at 0 — so in tarfs, which starts empty, every pre-existing directory is 0:0 and applying the header's owner is either a no-op or a correction.

This is now a cross-path question, which is why I'd rather settle it than block on it. #2458 shipped the identical gate to main for installAPKFiles (if statErr != nil { Chown; Chmod }, MkdirAll(name, mode.Perm())), so the design is the convention in two places and it'd be odd to block the second instance of a pattern that just merged as the first. Worth settling before a third path copies it. And the gate has a genuinely stronger justification there than here: installAPKFiles can run against a DirFS over a pre-existing tree (apk.New's fallback is DirFS(ctx, "/")), where an existing directory may legitimately have a non-root owner — your own test pins tmp at 5:5 for that reason. tarfs has no such case.

Suggestion for tarfs: apply the header's mode and ownership unconditionally, but never clear a special bit already present.

existing, statErr := m.getNode(hdr.Name)
var present fs.FileMode
if statErr == nil {
    present = existing.mode
} else if !errors.Is(statErr, fs.ErrNotExist) {
    return false, fmt.Errorf("error checking directory %s: %w", hdr.Name, statErr)
}

mode := hdr.FileInfo().Mode()
if err := m.MkdirAll(hdr.Name, mode.Perm()); err != nil { ... }

if err := m.Chown(hdr.Name, hdr.Uid, hdr.Gid); err != nil { ... }
keep := present & (os.ModeSetuid | os.ModeSetgid | os.ModeSticky)
if err := m.Chmod(hdr.Name, (mode&^os.ModeType)|keep); err != nil { ... }

I ran this. It fixes all three orderings above and the cross-package ancestor case below, subsumes the errors.Is issue, and leaves every golden digest untouched — internal/cli passes unchanged, so no regeneration. The only thing in the suite that changes is TestWriteHeaderDirExisting:291-292: /tmp keeps its sticky bit and 0777, but its uid follows the header, 0 → 100. I think that assertion encodes the gate's behaviour rather than the desired behaviour, and the real package above makes it doubly synthetic — nothing ships a non-root tmp header, so the real-world result is unchanged either way. Worth deciding deliberately rather than inheriting.

Two notes on that shape. The tradeoff is that a package can no longer remove a special bit from an existing directory — the same tradeoff the current gate makes, just far narrower, and worth stating in the comment. And I've put Chown before Chmod to match what you landed in #2458; in tarfs the order is immaterial (Chown touches only uid/gid, and Chmod re-applies the node's type bits at fs.go:654), but your own comment there argues the paths shouldn't disagree, and right now they do.

Tests. The created == false branch is only reached via tfs.Mkdir — the InitDB shape — never through WriteHeader, which is the branch's own gating function and the path real installs take. All three losing orderings pass the current suite untouched. The same gap exists in #2458, where TestInstallAPKFilesModesAndOwnership orders its fixture "dirs first, so they exist before the files under them" and its only pre-existing case is again Mkdir-created.

While you're in here: a directory header now walks the tree five times (getNode probe, Chmod, Chown, Chtimes, on top of MkdirAll), each redoing the same O(depth) walk with a mutex per component, where it used to walk twice. If MkdirAll returned (*node, created bool) you'd drop three walks and the probe.

Should fix before merge

created := statErr != nil is too broad (fs.go:122). getNode also returns "maximum symlink depth exceeded" (fs.go:238) and errors bubbled from resolving intermediate symlink targets; for those, created becomes true although nothing was created. Latent today because MkdirAll fails on the same inputs first, but the coupling is invisible and unpinned. errors.Is(statErr, fs.ErrNotExist) — the sketch above does this. Same one-word issue now exists at install.go.

hdr.Uid/hdr.Gid reach the image unchecked, and the two output formats disagree. The pinned go-erofs does e.ino.uid = uint32(uid) (mkfs.go:320), so uid -1 becomes 4294967295 and 1<<33 becomes 0; archive/tar instead promotes both to PAX records, and an extractor reading uid=-1 calls chown(-1) — "leave as the extracting user". Both reproduce. Pre-PR this couldn't happen because nodes were always 0:0, so it's a new input path. A 0 <= uid <= math.MaxUint32 check at the copy point keeps the formats in agreement. Out-of-range values also flip go-erofs's compact-inode selection (layout.go:54), so it isn't purely cosmetic.

Directory coverage has no real-apk fixture, and setuid-on-a-directory is asserted nowhere despite being in the PR description — the dir test covers 0o2755|0o1000 only. Every directory in all four fixture apks is root/root 0755, which is why the regenerated goldens exercise only the file/symlink half; that's also why the hand-built tar.Header literals never hit the ordering case, since a real apk lists parents before children. wolfi-baselayout would make a much better fixture for this — it ships tmp/var/tmp at 0777 and root at 0700, which is precisely the shape the dir branch needs to get right.

Adjacent and pre-existing — your call whether to fold in

writeHeader trusts S_IFMT bits in the header's Mode field (fs.go:476, 540). Not introduced here, but this PR adds uid:/gid: on those exact lines, and headerFileInfo.Mode() ORs Go mode-type bits in from Mode. A TypeReg header with Mode=0o120777 and a Linkname installs as a symlink to the attacker's target, bypassing the default: arm at fs.go:198. All six S_IFMT values are accepted, and the tar and erofs writers disagree on which are fatal:

header Mode	node becomes	tar writer	erofs writer
0o120777 + Linkname	symlink	Typeflag 2, target from Linkname	symlink
0o020644	char device	Typeflag 3	Mknod S_IFCHR
0o060644	block device	Typeflag 4	Readnod fails → build dies
0o010644	fifo	Typeflag 6	Mknod S_IFIFO
0o040755	dir, node.dir == false	build dies, "unknown file mode"	—
0o140644	socket	build dies, "sockets not supported"	Mknod S_IFSOCK

Four silently produce the wrong node type while the database records a regular file; two are build-killing. It needs a package you already chose to install, so it isn't urgent — and the PR only amplifies it (a forged node now also carries attacker uid/gid rather than always 0:0). The streaming path is unaffected: it dispatches on Typeflag and its new Chmod correctly masks with &^ fs.ModeType.

The obvious fix doesn't work — copying &^ os.ModeType across from your dir branch strips ModeSymlink from real symlinks and TestTarFS fails with file is not a link, because writeHeader serves both TypeReg and TypeSymlink. The type bits have to come from Typeflag:

func nodeModeFromHeader(header *tar.Header) fs.FileMode {
	mode := header.FileInfo().Mode() &^ os.ModeType
	if header.Typeflag == tar.TypeSymlink {
		mode |= os.ModeSymlink
	}
	return mode
}

Verified: closes all six and keeps the suite green.

Same-checksum short circuit keeps the first package's metadata (fs.go:512, and :157 for symlinks). etc/conf shipped 0644 1:1 by one package and 0600 100:101 by another with identical content stays -rw-r--r-- 1:1 while the database records a:100:101:0600 — the same divergence, on the file half, newly observable now that ownership is load-bearing. This needs a decision rather than a patch: does the later package's metadata win, or is first-wins intentional? Either way, a comment and a test.

Follow-ups I'd file rather than ask you to fix here

• The symlink-to-dir early return above your new code is dead (fs.go:112). getNode resolves symlinks for every component including the last, so Stat returns the target's FileInfo and ModeSymlink is never set — Lstat too. A TypeDir header naming usr/lib64 -> lib falls through, returns installed=true instead of false, and Chtimes at :140 retimes the target usr/lib (reproduced: the stamp lands on usr/lib). #2458 restructured the equivalent at install.go:219 but kept the same Stat-based test, so it's still dead there. Flagging it here only because your new created probe sits directly underneath and shares the same symlink-resolving getNode.
• Invented ancestors that no header ever describes keep the leaf's mode. A lone home/alice/.ssh 0700 header produces home and home/alice as drwx------; a 0777 leaf produces 0777. This matches os.MkdirAll, and the cross-package variant is fixed by the change above, so it's the narrow residue — but for an image builder, 0755 for invented intermediates is probably more defensible than inheriting the leaf's.
• Hardlink members take the target's uid/gid, not their own header's. usr/bin/a at 5:6 then TypeLink usr/bin/b -> a declaring 7:8 gives both 5:6, while the database records 7:8 for b. Arguably correct for a genuine hardlink — one inode, one owner — but the assumption that the headers agree is now load-bearing and pinned nowhere.
• memFileInfo embeds *node, so tarfs's fs.FileInfo is a live view rather than a stat snapshot (fs.go:997-1009). Every os-backed implementation is a snapshot, so a before/after check written against this API compares the mutated node with itself and reports "unchanged". This cost me a false negative on the retiming above on first pass. Worth a comment on the type at minimum; pkg/apk/fs/memfs.go looks the same.
• pkg/cpio/layer.go never sets Record.UID/GID, declared out of scope in #2458, so apko build-cpio output still lands 0:0 regardless of either change.

Nits

• The four new tests differ only in the header fed to WriteHeader and the expected uid/gid/mode — one table with named rows would make the missing ordering cases a one-line addition each. Also mixes t.Fatal(err) with require.NoError, and require.Zero at :291-292 carries no identifying message where require.Zerof at :259-260 does.
• dirReader (fs_test.go:178) re-declares what apkfs.ReaderFS already has — both ReadDir and Readlink are on it, and internal/cli/build.go:269 passes tarfs.New() straight in where a FullFS is wanted.

---

_{Reviewed with Claude Code assistance. Every behavioural claim above was reproduced against a45a6a4 rebased onto c7be9d8f with a table-driven probe harness; happy to share it as a starting point for the tests.}

[stevebeattie]

Two additions since the review above: a class of metadata I didn't cover, and some evidence from your own melange change that bears on the gate question.

xattrs on symlink headers

I only looked at mode and ownership. xattrs travel the same PAX path, and the case tar.TypeReg, tar.TypeSymlink: arm applies them for both types — but SetXattr resolves through getNode, which follows the final component. So a symlink header's xattrs land on whatever it points at. Same root cause as the dead early return in my last comment; two consequences, and the second is the one I'd act on.

A symlink's own SELinux label is lost and misapplied. Symlinks legitimately carry security.selinux, distinct from the target's — unlike user.* (EPERM on a symlink) and ACLs (EOPNOTSUPP, since a symlink's mode is meaningless). Today:

usr/bin/sl -> target, SCHILY.xattr.security.selinux = system_u:object_r:bin_t:s0
→ usr/bin/target  gains that label
→ usr/bin/sl      has none

Then both writers exclude symlinks from xattr emission (tarball.go:163, erofs.go:322), so the label is lost on the way out as well as misapplied on the way in. Anyone labelling images with apko can't get symlink labels right.

And the redirection launders metadata past the file-conflict check. A capability xattr is only honoured on a regular file, so that's where it has to land — and an attacking package can't ship the victim's path directly:

direct   — attacker ships usr/bin/target carrying the xattr
         → packages map[attacker:attacker victim:victim] has conflicting file: "usr/bin/target"    blocked

indirect — attacker ships usr/bin/harmless -> target carrying the xattr
         → no error; victim's usr/bin/target now carries it                                        allowed

writeHeader's conflict detection is keyed on the path in the header, so the symlink route never trips it, and getNode delivers the xattr to the victim's inode regardless. Needs a package you already chose to install, same as the S_IFMT item — so not urgent, but it's a privilege-bearing attribute arriving on a file its own package didn't consent to.

Suggested fix, and #2458 already set the precedent. From that PR's body:

Symlinks and hardlinks are left alone: FullFS has no Lchown, and Chown would follow the link and retarget its target's ownership.

Exactly right for ownership, and the xattr path wants the same treatment: skip the xattr loop for TypeSymlink headers, with the same comment explaining why. That closes the laundering immediately and loses nothing apko can currently represent, since neither writer emits symlink xattrs anyway.

The general shape is that apkfs.FullFS has one l-variant in the entire interface — Lstat, which is itself broken because it resolves the final component and so never reports ModeSymlink. No Lchown, no Lsetxattr, no Lchmod. Symlink-own metadata is structurally unrepresentable, and every attempt to set it silently redirects to the target. Making it representable is a much bigger change and probably wants its own issue; skipping symlinks is the honest interim, and it's what the streaming path already does.

Two smaller notes while I'm here. The created gate protects a pre-existing directory's mode and owner but not its xattrs — a package's tmp header can't touch /tmp's mode, but it can set security.selinux and system.posix_acl_access on it. Whichever way the gate question lands, it should cover all three kinds of metadata rather than two. And nothing records xattrs in the installed database (no mentions in installed.go), so apk fix/verify can't see that a label or capability was set — not this PR's problem, just worth knowing it's the floor.

melange#2646 makes the gate case better than the comment here does

From that PR body:

wolfi-baselayout explicitly chmod 1777s both /tmp and /var/tmp, and the published apk records both as 0777. /tmp is masked downstream by luck (apko's InitDB creates it as 1777 first, and apko won't let a package header downgrade an existing directory); /var/tmp has no such backstop and ships world-writable with no sticky bit.

That's a better justification than the comment at fs.go:129-132, and it argues for changing the gate rather than keeping it: it protected /tmp by accident and did nothing for /var/tmp. Preserving already-set special bits instead of skipping the whole update gets /tmp right on purpose and /var/tmp right for the first time, once melange#2646 propagates and the apk starts recording 1777. Worth quoting that reasoning into the comment here — it's the real story, and right now it lives in another repo.

---

_{Probes for all of the above are in the same harness as the earlier findings; happy to share. The one thing I could not verify locally is the kernel's rule for security.capability on a symlink — that needs CAP_SETFCAP — so I've framed the bypass around where the attribute has to land to be honoured, which doesn't depend on it.}