Add an upsert overlay op so fixtures survive base-image re-pins

[stevebeattie]

Unblocks #173. That PR is red and the cron will hit the same wall tomorrow, so
this wants to land before or with it.

What broke

The offline matrix failed on #173, the daily base-image re-pin. Every
detect_openssl fixture errored before any scan ran:

overlay apply: adding "etc/ssl/openssl.cnf": path already exists

The newer wolfi-base ships /etc/ssl/openssl.cnf; the previously pinned image
did not. Confirmed by inspecting both:

base	etc/ssl/openssl.cnf
7e62cecd (main)	0 occurrences
103eb3f4 (#173)	1 occurrence

Nothing about the datastream or the checks was wrong — End-to-end scan, the
mirror check, the lint gate and the sidecar guard all passed on that PR.

The defect is an API gap, not a bad fixture

No overlay op could express "this file holds this content":

op	precondition
AddFile	path must not exist
ReplaceFile	path must exist
AppendFile	path must exist

So every fixture was implicitly betting on whether the base image ships a path,
and the bet expired silently whenever the base was re-pinned. openssl.cnf just
went first — fipsmodule.cnf, ssh_config, sshd_config and the FIPS drop-ins
were all equally exposed.

Swapping to ReplaceFile would only invert the bet: it would break the moment a
base dropped a path.

Change

PutFile replaces an existing entry's content in place — keeping its header and
position in the tar — or creates one if absent. mode/uid/gid apply only on
creation, since a fixture replacing content is not usually restating permissions.

All 29 AddFile calls in the fixture matrix move onto it. Each means "this file
holds this content"; none relied on the absence precondition for its meaning. The
fixtures that do test absence express it by omitting the file
(fail_missing_ssh_fips_conf leaves out the drop-ins, fail_java_missing_stamp
adds a truststore and no stamp), so that behaviour is unchanged. AddFile stays
for cases where a base gaining a path should fail loudly, and remains in use by
CopyFile and PutFile.

Verified causally, not just green

• Against chore(oscap): re-pin fixture base-image digests #173's base, the converted fixtures pass — and reverting them to
  AddFile reproduces the exact path already exists error.
• Against main's base they also pass, so this is not a swap of one bet for the
  other.
• Full matrix green: 47 rows, 11 of them detect_openssl, 0 failures.
  gofmt/vet clean, mirror check unaffected.

TestPutFile covers both directions plus the property that motivated it: the
same op applies cleanly whether or not the base ships the path.

Idempotency

TestPutFileIsDeterministic asserts the same ops over the same base give
byte-identical output, and that applying PutFile twice equals applying it once.
It runs over both branches, which matter separately: replace rewrites an entry
in place, while create appends to the added list, so a repeat that appended twice
would emit the path twice rather than once.

This matters because the matrix shares one base across every parallel subtest: a
PutFile whose second application differed from its first would make a fixture's
content depend on what else ran.

An earlier draft of this PR also asserted that Apply leaves the base bytes
untouched, and billed that as the load-bearing guarantee. That was wrong, and it
is worth stating rather than quietly dropping: Apply takes an io.Reader and
only ever copies out of it, so it cannot write to a caller's slice through that
signature at all. The assertion could not fail whatever Apply did internally —
it really tested buildTar's determinism. TestApplyReusesABaseSafely asserts
the properties that genuinely can fail instead (no per-run state shared between
calls; ops not retaining and then mutating the caller's content slice) and
records that the remaining guard, cp := *hdr, is structural and not observable
from outside a single Apply call.

One stale premise found along the way

detect_openssl/fail_clean_no_fips has no ops — it is the "an unmodified image
fails this rule" control. Its assertion is still correct, but its comment claimed
the vanilla base lacks the OpenSSL FIPS module config, and the base now ships
openssl.cnf. What is actually true of the new base:

etc/ssl/openssl.cnf	present (new)
etc/ssl/fipsmodule.cnf	absent
FIPS apk packages	0 installed

So it still fails, on the module config and the packages; openssl.cnf's absence
is simply no longer part of why. Comment corrected, and the fixture deliberately
left op-free: it degrades loudly, not silently — if the base ever gained the
FIPS pieces, that row would flip to pass and fail the suite rather than quietly
assert less. Worth noting AddFile never protected that fixture either, since it
calls no ops, so this exposure was independent of the change.

Added in review

b9e6db2 — a duplicate tar member on remove-then-re-add. RemoveFile dropped
a path from the entry map but left its name in the base ordering, so a later op
re-adding it made Apply write the path twice: once from the base-order loop,
again from the added loop. The result is a fixture tar with a duplicated member
that no op reports as an error. Pre-existing and not reachable today — the sole
RemoveFile call site never re-adds — but PutFile's "whether or not it already
exists" contract is what makes the pairing something an author would reach for, so
it is fixed here rather than documented as a footgun.

Same commit pins three PutFile edges: a non-regular target fails with
ErrNotRegular (a real risk for these paths, not a theoretical one — the base
ships etc/ssl/cert.pem and etc/ssl/certs/ca-bundle.crt as symlinks to
ca-certificates.crt); idempotency covers the create branch; and safePath is
checked in PutFile itself, making a guarantee that was emergent from two other
functions local. doc.go's op list had drifted to four of seven ops.

25c9bfb — base composition is now asserted directly. Converting all 29 calls
to PutFile removes the base-drift alarm from the whole matrix at once: the
ErrExists that fired when the base gained openssl.cnf was crude, but it was
the only thing that noticed, and afterwards the failure mode inverts from "CI
breaks confusingly" to "a fixture quietly asserts less than its name claims".

TestBaseImageComposition covers that surface where the cause is, rather than as
a downstream verdict flip: paths an existence-precondition op needs; paths whose
absence gives a fixture its discriminating power (the FIPS ssh drop-ins,
fipsmodule.cnf, the Java truststore pair behind CertificateAudit tst:5's
none_exist); PutFile targets being absent or regular files; and no FIPS apk
packages installed. Each row carries why it matters, so a failure explains itself.

etc/ssl/openssl.cnf is asserted only as "absent or regular" — it is the path
that actually drifted, so pinning it either way would re-create the bet PutFile
removed. Re-pinned to #173's digest the check stays green; planting a wrong row in
any of the three path lists turns the matching subtest red naming the path, its
tar type and the dependent fixture. It needs only a registry pull, so it runs in
more environments than the matrix it protects.

This also gives the fail_clean_no_fips premise above a real home: both halves of
that control — no fipsmodule.cnf, no FIPS packages — are now checked explicitly
rather than resting on a comment.

🤖 Generated with Claude Code

[0xDom-S]

🤖: Verdict: APPROVE. PR #174 adds overlay.PutFile (tests/oscap-offline/internal/overlay/overlay.go:141-175) as an upsert op — it replaces the target's content in place when the base tar already holds the path, otherwise delegates to AddFile. All 29 AddFile call sites in fixtures_test.go move onto it, unblocking a red CI matrix caused by base re-pins that add paths fixtures used to assert as absent.

Both safety invariants that matter were verified by execution, not just inspection: path safety (every p.byName key reaching PutFile was already validated by safePath at ingest/AddFile time) and type safety (ReplaceFile → requireReg still returns ErrNotRegular for a non-regular target). overlay has no production importer (only test files import it), so there is no security surface in shipped binaries — all 29 call sites also pass string literals / package-level []byte constants, so no injection surface either.

No Critical Issues were found. The Suggestions below are non-blocking engineering follow-ups; two are worth prioritizing (class-wide alarm loss, and the stale doc.go op list). The following could not be anchored to a changed line in this diff and are recorded here instead:

• tests/oscap-offline/internal/overlay/doc.go:5 — Package doc comment still enumerates the exported op set as "(AppendFile, AddFile, CopyFile, Chown)" and omits the new PutFile (also pre-existing: ReplaceFile/RemoveFile were already missing). Adding an exported op is the moment to fix this.
• tests/oscap-offline/internal/overlay/doc.go:16-20 — The error-contract paragraph documents which ops return ErrNotFound/ErrExists/ErrNotRegular but doesn't mention PutFile's contract (it can surface ErrNotRegular via its ReplaceFile branch, and ErrUnsafePath/ErrExists via its AddFile branch).
• tests/oscap-offline/internal/overlay/overlay.go:111 and :279-288 (pre-existing, outside this diff's changed-line range) — RemoveFile(p) followed by PutFile(p, …) emits two members named p: RemoveFile deletes the entry from byName but leaves the name in p.order, so a subsequent PutFile/AddFile re-add writes the path in both the base-order loop and the added loop. Reproduced empirically. Not introduced by this PR and no fixture currently pairs RemoveFile with a re-add of the same path (the sole RemoveFile call is fixtures_test.go:620, with no re-add), but PutFile's doc comment ("gives path the given content whether or not it already exists") actively invites that pairing going forward — worth a one-line caveat in the doc comment.
• .github/workflows/offline-tests.yaml:32-37 (unchanged by this PR) — OSCAP_OFFLINE_REQUIRE: "1" only fails the job on a prerequisite gap (no docker / no datastream / no registry), per its own comment. It does not and cannot detect a fixture that has been silently converted to assert less (see the PutFile-class-wide-alarm-loss suggestion below) — it is not the safety net one might assume it to be for that failure mode.
• gpos/xml/scap/ssg/content/ssg-chainguard-xccdf/OvalDefinitions/CertificateAuditTest.xml (unchanged by this PR) — re: the mode/uid/gid-drop concern on the PutFile replace branch: this file's OVAL states carry no mode/uid/gid predicate (six <ind:state> refs at :55,62,72,79,83,90 plus a <states> block at :144-163, none checking permissions), so today's fixture conversions are verdict-neutral here. Separately, :64-66 uses check_existence="none_exist" for the Java truststore, meaning this fixture family's discriminating power also tracks base drift via existence rather than permissions — same class of concern as the alarm-loss suggestion below.
• gpos/xml/scap/ssg/content/ssg-chainguard-xccdf/OvalDefinitions/LibraryPermissionsTest.xml:58-60 and .../VarLogPermissionsTest.xml:37-39 (unchanged) — the only OVAL states in this repo that do read ownership/permissions target /usr/lib and /var/log, which are disjoint from the etc/ssl/**/etc/ssh/** paths converted to PutFile in this PR. So the mode/uid/gid-drop trap (see suggestion below) has no live consequence today, but is a real latent trap for the next permissions-checking STIG rule added over etc/ssh/sshd_config or similar.
• gpos/xml/scap/ssg/content/ssg-chainguard-xccdf/OvalDefinitions/DetectOpenSslTest.xml:268-272 (unchanged) — same check: its only state (environmentvariable58_state) does not read file permissions, so it is unaffected by the mode/uid/gid-drop concern today.

[0xDom-S]

🤖: Suggestion — the base-drift alarm is now gone from the entire fixture matrix; keep one canary. This doc comment advises "Use AddFile instead only where a fixture's point is that the path was absent, so that the base gaining it should be a loud failure" — but after this PR, zero fixtures do that. fixtures_test.go now has 29 overlay.PutFile calls and 0 overlay.AddFile calls (e.g. lines 464-465, 479-482, 630-631, 641-642, 653, 854-855, 867-868, 879-882, 893-894, 905-910, 919-920, 932-933). The precedent this PR fixes for etc/ssl/openssl.cnf is removed class-wide for fipsmodule.cnf, ssh_config, sshd_config and the drop-in fixtures simultaneously. This is a defensible, explicitly-argued trade — absence-testing fixtures still express absence by omission and will fail loudly if the base gains the path — but there is no CI control that separately detects reduced discriminating power of a fixture (verified: OSCAP_OFFLINE_REQUIRE=1 in offline-tests.yaml only guards prerequisite gaps, not a degraded assertion). Cheapest mitigation: retain a single AddFile op on one path the base is asserted not to ship (e.g. etc/ssl/fipsmodule.cnf in one fixture), so a re-pin that adds FIPS pieces still trips something.

[stevebeattie]

Confirmed the count: 29 PutFile, 0 AddFile in fixtures_test.go. AddFile survives only in example_test.go and as PutFile/CopyFile's internal callee, so the diagnosis is exactly right — the alarm is gone class-wide, and OSCAP_OFFLINE_REQUIRE=1 does only guard prerequisite gaps.

I went a different way on the remedy. Keeping one AddFile canary re-arms the fragility inside a fixture whose job is a scan verdict, so the next re-pin turns the matrix red for a reason unrelated to the rule under test — and it only watches whichever single path it names, leaving the other seven converted paths uncovered.

25c9bfb asserts base composition directly instead, in TestBaseImageComposition. Four premise families, each row carrying why it matters so a failure explains itself rather than needing archaeology:

• paths an existence-precondition op needs (AppendFile on etc/shadow, CopyFile's source, Chown on var/log)
• paths whose absence gives a fixture its discriminating power — the FIPS ssh drop-ins, etc/ssl/fipsmodule.cnf, and the Java truststore pair behind CertificateAudit tst:5's none_exist. This is the silent class you identified: gaining one of these leaves the fixture green while it tests less.
• PutFile targets are absent or regular files (see the ErrNotRegular thread)
• no FIPS apk packages installed — the other half of what makes detect_openssl/fail_clean_no_fips a control

etc/ssl/openssl.cnf is deliberately in the third list only. It is the path that actually drifted, so pinning it present or absent would just re-create the bet PutFile removed; what must hold is that it stays a regular file.

Verified both directions: re-pinned to #173's digest (Dockerfile and the pinned const, which the update-ca-cert workflow updates together via TEST_PIN_FILE), the check stays green — so it is not a new bet. Planting a wrong row in each of the three path lists turns the matching subtest red naming the path, its tar type and the dependent fixture. It needs only a registry pull, not docker or the datastream, so it runs in more environments than the matrix it protects.

[0xDom-S]

🤖: Suggestion — mode/uid/gid are silently dropped on the PutFile replace branch, leaving this fixture base-dependent in attribute space. PutFile(javaTrustStorePath, javaTrustStore, 0o444, 0, 0) will silently start asserting base-derived permissions instead of 0o444/uid 0/gid 0 the day the base ships this path (verified empirically against overlay.go: on the replace branch, mode/uid/gid args are ignored and the base header's own values are kept). This is verdict-neutral today for the OVAL rules that read this repo's etc/ssl/**/etc/ssh/** fixtures, but sshd_config permissions are a classic STIG rule, so this is a live trap for the next check added over one of the 29 converted paths. Consider applying mode/uid/gid unconditionally on the replace branch (more consistent with "the fixture, not the base, decides"), or pairing PutFile with a Chown/mode op at the sites that specifically care about attributes, and calling this out in overlay.go's doc comment beyond the current "not usually trying to restate permissions".

[stevebeattie]

Confirmed the behavior — the replace branch keeps the base header's mode/uid/gid and ignores the arguments — and confirmed verdict-neutrality more tightly than "for the rules that read this repo's fixtures": the only OVAL states in the tree that read ownership are LibraryPermissionsTest.xml:59-60 and VarLogPermissionsTest.xml:38-39, targeting /usr/lib and /var/log. All 29 converted paths are under etc/ssl/** or etc/ssh/**, so the sets are disjoint. (Worth noting the grep for this needs user_id/group_id — my first pass searched uid|gid and came back empty, which is misleading.)

I did not take the "apply mode/uid/gid unconditionally" option. It would override ReplaceFile's documented "leaves its header intact" contract, and it would make all 29 sites assert 0o644/0/0 over base files whose real modes may legitimately differ — turning an incidental argument into a silent assertion at every call site. That trades a latent trap for an active one.

Your alternative is the right one and is now what the docs say: a fixture that means to pin an existing entry's attributes says so with Chown. b9e6db2 spells that out in PutFile's doc comment, along with the corollary — no OVAL check should read permissions on a path a fixture only ever PutFiles.

On "live trap for the next check added over one of the 29 paths": agreed, and that is the part a doc comment alone does not catch. TestBaseImageComposition in 25c9bfb is where it now surfaces, since a permissions rule landing on one of these paths would need a base-composition premise to be meaningful, and that file is where such premises are written down.

[0xDom-S]

🤖: Suggestion — the re-pin fragility class is narrowed, not closed, and this gap is untested. If a future base ships one of the 29 converted paths as a symlink or directory instead of a regular file, PutFile → ReplaceFile → requireReg will hard-fail with ErrNotRegular — which is exactly the re-pin breakage shape this PR sets out to eliminate (reproduced). TestPutFile here never exercises a non-regular target for the replace branch. Worth one sentence in the PutFile doc comment plus one subtest covering a non-regular base entry at the target path.

[stevebeattie]

Implemented, and the finding is stronger than stated. You framed it as "if a future base ships one of the 29 converted paths as a symlink or directory" — but the base already ships symlinks inside exactly that directory family:

lrwxrwxrwx etc/ssl/cert.pem            -> certs/ca-certificates.crt
lrwxrwxrwx etc/ssl/certs/ca-bundle.crt -> ca-certificates.crt
lrwxrwxrwx etc/pki/tls/certs/ca-bundle.crt -> ../../../ssl/certs/ca-certificates.crt

So the realistic trigger is not a re-pin at all — it is the next CA-bundle fixture written against an alias, which is a natural thing to reach for given SSL_CERT_FILE already points into that directory. None of the 8 distinct converted paths is currently non-regular in either the current or #173's base, so nothing is broken today.

b9e6db2 adds the doc sentence plus a table-driven rejects a non-regular target subtest covering both a directory and a symlink target. Confirmed it is not vacuous: replacing PutFile's replace branch with a direct e.data = bytes.Clone(content) that skips requireReg turns both subtests red with

Apply error = writing content "etc/ssl/openssl.cnf": archive/tar: write too long, want errors.Is ErrNotRegular

which is precisely the deferred tar-write failure requireReg exists to pre-empt.

The class is also now guarded at the base rather than only at the op: TestBaseImageComposition in 25c9bfb asserts every PutFile target is absent or a regular file, so a base that starts shipping one as a link fails there — naming the path and its type — instead of surfacing as ErrNotRegular from whichever fixture ran first.

[0xDom-S]

🤖: Suggestion — two small gaps in otherwise strong new tests. (1) TestPutFileIsDeterministic only exercises idempotency on the replace path; the create-then-replace direction (path absent in base) is uncovered. Verified it is byte-identical there too, so the assertion is free to add. (2) This "Apply leaves the base bytes untouched" assertion cannot actually fail through Apply: apply() hands Apply a bytes.NewReader(base) and Apply only reads from it (io.ReadAll copies into a new byte slice) — in practice this just re-asserts buildTar determinism. Harmless and worth keeping, but overselling it as the load-bearing immutability guarantee is misleading; the properties that actually hold are the fresh plan allocated per Apply call, cp := *hdr when copying base headers, and bytes.Clone when copying content in ReplaceFile/AddFile.

[stevebeattie]

Both taken.

(1) create-branch idempotency. Confirmed byte-identical, so the assertion was free as you said. TestPutFileIsDeterministic is now table-driven over both branches. Worth recording why it is not merely symmetry: the two carry different bookkeeping — replace rewrites an entry in place, while create appends to p.added — so a repeat that appended a second time would emit the path twice rather than once. That is the same failure shape as the RemoveFile bug in the summary thread, and it is now covered from both directions.

(2) The immutability assertion. You are right that it cannot fail through Apply. Verified: Apply takes an io.Reader, io.ReadAll allocates fresh, and a probe confirms the read data does not alias the base backing array — so there is no path by which a caller's slice could be written through that signature. As written it really tested buildTar's determinism.

I dropped it rather than keeping it, because a green assertion that cannot go red is exactly the thing this PR should not be adding. TestApplyReusesABaseSafely asserts the two properties that genuinely can fail instead — no per-run state shared between calls, and ops not retaining and then mutating the caller's content slice — and its comment records that the third guard you named (cp := *hdr) is structural and not observable from outside a single Apply call, so it is documented rather than fake-asserted.

The PR description had billed the base-untouched assertion as the load-bearing guarantee, which was the same overstatement. I have corrected that section rather than leaving the claim standing in the description and only walking it back down here.

[0xDom-S]

🤖: Optional hardening — hoist the path-safety check to the top of PutFile. This line reads p.byName[path] before any explicit validation in PutFile itself. It is safe today only because of an unstated closed-set invariant: every key that can be in p.byName was already gated through safePath at base-ingest time (overlay.go:243) or at AddFile's own explicit check (overlay.go:119-122) — PutFile's replace branch never adds a new, unvalidated key. Adding an explicit if !safePath(path) at the top of PutFile (line 164) would make this guarantee local and self-evident instead of emergent from two other functions, at zero runtime cost. (Note: a prior automated reviewer claimed safePath is called "before either branch runs" inside PutFile — that's not accurate as written; the guarantee currently holds only via the invariant described above, which is exactly why making it explicit here is worthwhile.)

[stevebeattie]

Implemented in b9e6db2 — PutFile now checks safePath before touching p.byName, so the guarantee is local instead of emergent from base ingest (overlay.go:243) plus AddFile (:119-122).

Being explicit about what this is and is not: it is not a bug fix. I mutation-tested it by removing the hoist again, and the suite stays green — the create branch is still covered by AddFile's own check, and the replace branch cannot see an unvalidated key because nothing can put one in byName. So the closed-set invariant you describe does hold, and the check is defense-in-depth that makes it self-evident at zero runtime cost, exactly as you framed it. I have not added a test that pretends otherwise, since there is no reachable input that distinguishes the two versions.

Also confirming your correction of the earlier automated reviewer: safePath was not called before either branch ran. The replace branch indexed byName directly with no validation in PutFile itself. That reading was wrong and yours matches the code as written.

[0xDom-S]

Approved

[stevebeattie]

Pushed b9e6db2 and 25c9bfb addressing the review. Per-thread replies cover the five inline suggestions; this is for the notes in the review body that had no anchor line.

RemoveFile + PutFile emits the path twice — upgraded from doc caveat to a fix

Reproduced independently:

member order = [etc/keep etc/stamp etc/stamp]; "etc/stamp" appears 2 time(s)

Your analysis is exactly right: RemoveFile deleted from byName but left the name in p.order, so a re-add wrote the path from both the base-order loop and the added loop. Also confirmed it is not live — the sole RemoveFile call site (certificate_audit/fail_missing_stamp, fixtures_test.go:620) never re-adds.

I fixed it rather than documenting it. The output is a fixture tar with a duplicated member that no op reports as an error, so the failure is silent and produces a malformed artifact; and as you note, PutFile's "whether or not it already exists" contract actively invites the pairing, so the doc-caveat version leaves a footgun whose warning sits in a different function than the one an author would be calling. RemoveFile now drops the name from the ordering too, so a re-add emits one member positioned with the added entries. TestPutFileAfterRemoveFile pins it and fails with the order printed if the fix is reverted.

Pre-existing rather than introduced here, agreed — but PutFile is what makes the pairing reachable in practice, so it belongs in this PR.

doc.go

Both points correct and fixed in b9e6db2. The op list named four of seven exported ops (ReplaceFile and RemoveFile were already missing before PutFile joined them), and the error-contract paragraph now covers PutFile reaching ErrNotRegular via its replace branch and ErrUnsafePath/ErrExists via its create branch.

The OVAL and workflow notes

Verified each and they all hold:

• LibraryPermissionsTest.xml:59-60 and VarLogPermissionsTest.xml:38-39 are the only ownership-reading states in the tree, targeting /usr/lib and /var/log — disjoint from the converted etc/ssl/**/etc/ssh/** paths. Discussed in the mode/uid/gid thread.
• CertificateAuditTest.xml carries no mode/uid/gid predicate, and its tst:5 none_exist on the Java truststore does make that fixture family's discriminating power track base composition. That premise is now asserted explicitly in TestBaseImageComposition.
• DetectOpenSslTest.xml tst:1/tst:2 being at_least_one_exists is what made detect_openssl/fail_clean_no_fips's premise go stale in the first place. Both halves of that control — no fipsmodule.cnf, no FIPS apk packages — are now asserted directly.
• OSCAP_OFFLINE_REQUIRE guarding only prerequisite gaps: correct, and it is worth having stated plainly. It is not a net for reduced discriminating power, which is what motivated the base-composition test rather than a canary fixture.

Verification

gofmt/vet clean; full suite green under OSCAP_OFFLINE_REQUIRE=1; matrix still 47 rows / 11 detect_openssl / 0 failures, so no fixture was dropped. Re-pinned to #173's digest (Dockerfile and the pinned const together) the whole suite including the new composition check stays green, so #173 is unblocked. Every new assertion was mutation-tested — details in each thread.