Repository navigation
feat(sbom): track Go and Rust dependencies in shipped binaries - #466
Conversation
Record digest-bound binary evidence from verified production layers and preserve it through component merges and imports. Keep dependencies when provenance, classification, inherited evidence or hardlink reconstruction is incomplete. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Persist validated stage ancestry and layer boundaries for SBOM-enabled Stapel builds, including exact JSON transport through Docker commits. Regenerate binary evidence without rebuilding cached stages and fail open for older images or invalid ancestry. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Cover selective import from a two-binary donor, a second import and a manifest-only image. Assert dependency verdicts and occurrences remain stable after SBOM cache hits and regeneration from cached stages, while inherited unknown evidence remains conservative. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Verification
Review focus
Follow-up
|
reyreavman
left a comment
There was a problem hiding this comment.
Code Review Report — static
Base: main (ac29d1bb, #464 merged), head 4687f022
Diff: 30 files, +2013/-53 — produced-set recovery from cached stage ancestry (sbom_provenance.go), per-layer binary attribution (produced.go), digest-bound binary evidence in CycloneDX (binary_occurrences.go), werf.io/sbom-stage-v1 label through build/commit/image-spec, format 13 → 14, e2e.
Context: cards D→E→F closing the Go/Rust side; concept §1.1 (per-stage produced set), §3.3, §4 pass 1a.
Mode: static, as requested — read every changed file, no build/test/mutation. Line refs are from the head blobs.
Verdict
- Technical: The design is the concept's: produced set =
diff(install) ∪ diff(beforeSetup) ∪ diff(setup)by verified layer boundaries, neverpackages/dependencies*/base; evidence is digest-bound and survives import by content; every gap (no label, broken ancestry, mismatched DiffIDs, hardlink uncertainty) resolves to unknown → keep. Reading it, I did not find a path to a false drop. I did find one false-positive-prone fail-open that will neuter the feature on common images (Major #1), and one place where recovery cost is unbounded (Major #2). Everything else is Minor. - Product: This is the increment where Go modules first get
werf:shipped=falsein real SBOMs. The eligibility gates are so conservative that on a stock glibc base most images will saywerf:binaries=unknownfor every module and mark nothing — correct, but the body should say so, or the first measurement will read as "the feature does nothing". - Risk: Medium, operational. A new label on every SBOM-enabled stapel stage changes stage content → image-build cache invalidates once for every such project. The body says the opposite.
DoD Criteria
From cards D/E/F "Готово, когда", concept §1.1/§3.3/§4, and the body.
| Criteria | Inferred? | Met? | Evidence |
|---|---|---|---|
produced set per stage: install ∪ beforeSetup ∪ setup; packages/dependencies*/base excluded |
no | ✅ | sbom_provenance.go:115-117 |
| stage boundaries verified against DiffIDs (parent is prefix) | yes | ✅ | :110-114; collectShippedScope re-checks saved image DiffIDs |
| test binary contributes nothing | no | ✅ | produced.go:188, binary_occurrences.go:77 |
| 1a: no exclusivity, any binary naming the module holds it | no | ✅ | binary_occurrences.go:86-92 |
unknown verdict holds everything of that ecosystem |
no | ✅ | :93, :99-100; binaryDecision :197-204 |
produced set unknown → no-data (never "empty") |
no | ✅ | binaryDecision :206-207; RecordProducedBinaryOccurrences :72-74 |
| evidence survives import, relocation, re-import by content | no | ✅ | digest-keyed occurrences; NormalizeInheritedBinaryOccurrences before merge |
| cached SBOM invalidated | no | ✅ | format "14" + provenance in checksum |
| "without invalidating image-build caches" (body) | no | ❌ | build_phase.go:1832-1843 — Major #3 |
| e2e: selective import, second import, manifest-only, cache hit | no | produced_binaries_test.go present; not run here |
|
task format/build/lint/test:unit |
no | — | not run (static) |
Issues
Major
-
FileIndex.Incompleteis set whenever any hardlink falls to the flattened fallback —file_index.go:132-134.remainingafterresolveHardlinksWithinLayersholds every link whose own layer did not answer. #433's own specs show legitimate cases that land there and resolve fine in the fallback: a link chain, a link whose target sits in a layer below. Any such link →Incomplete = true→CollectProducedFilesreturns{}(produced.go:50) →werf:binaries=unknownon every module → nothing is ever marked for Go/Rust in that image. busybox/coreutils/glibc bases are full of hardlinks; one chain is enough. The intent ("a hardlink resolved against the final filesystem may hide bytes") is right, but the signal is too coarse: setIncompleteonly inresolveHardlinksfor the unresolved count (:363, already there), not for everything that reached the fallback. Then write a spec: image with a hardlink chain →Incomplete == false. -
Boundary reconstruction re-flattens and re-hashes the image once per distinct boundary —
produced.go:70-83. Three eligible stages → up to fiveIndexImageFilescalls over growing prefixes of the same layer list, each a fullmutate.Extract+ sha256 of every file. On thenode:20-class images #453 measured at minutes for one pass this is 3–5×, on every SBOM regeneration, with no cap and no log line. Two cheap improvements: (a) index boundaries incrementally — one top-down walk of the layer tars with the same whiteout logic, snapshotting the map at each boundary, is one pass; (b) at minimum, log the count and time atDefaultlevel so the cost is visible. -
The new label invalidates every SBOM-enabled stapel stage once —
build_phase.go:1832-1843.serviceLabelsbecome part of the committed image config; a new key with a new value is new stage content. The body claims "without invalidating image-build caches" — it will, exactly once per stage, on the first build after upgrade. That is acceptable (same as any stage-config change) but must be stated, and the sentence in the body removed.
Minor
-
classifyProducedLayerspools and sha256s every executable in every layer, not only candidates —produced.go:105-113, :143-162.candidatesis known before the loop; the digest is only available after reading the whole file, so the spool is unavoidable, but the scan can skip layers outside every range (a base-image layer cannot hold a produced file unless a produced file is byte-identical to a base one, whichcandidateswould then match anyway — but then it is also in a produced layer). Restrict toimgLayers[r.Start:r.End]over the ranges. -
Three
Decision/Reasonpaths for binary-backed components now exist —match.go:goVerdict,binary_occurrences.go:binaryDecision, andMarkNotShippedbypassingMatchfor them (mark.go:55-63).Matchstill acceptsProducedBinariesandEcosystemGo/Rustcomponents, but nothing passes them any more;MarkNotShippedhandsProducedBinaries{}and routes binary-backed components around it. Either delete the dead path inMatch(and its specs) or makeMarkNotShippedthe only caller and say so in theMatchgodoc. Two rules for one question is how they drift. -
werf:binariesproperty semantics are undocumented at the type —binary_occurrences.go:14-20.none/unknown/ absent, withunknownaccumulated (never removed once added,:58, :73, :95) andnoneremoved once a positive occurrence lands (:118-121). That lattice is correct and subtle; it needs a paragraph onPropertyBinariesthe wayPropertyShippedhas one inmark.go:13-20. -
sbom_provenance.go:54-59hardcodes the stage order.stage.AllStagesor whatever the conveyor uses would keep this from drifting when a stage is added; if no such list exists, a spec asserting this slice equals the conveyor's order is the next best thing. -
ExactValues || k == image.WerfSBOMStageLabel—legacy_stage_image_container_options.go:218, :260. Special-casing one key in the escape path is a smell; the JSON record needs exact quoting, so either allwerf.io/*labels go exact (they are all machine-written) or the record is base64'd and the escape path stays uniform. Covered bylegacy_sbom_label_test.go, so it works — this is about the next label.
Risks
| № | Risk | Type | Likelihood | Severity | Location | Circumstances | Consequences | Recommendation |
|---|---|---|---|---|---|---|---|---|
| 1 | Go/Rust marking silently inert on hardlink-rich bases | UX/Product | Likely | Medium | file_index.go:132 |
Any base with a hardlink chain or cross-layer link | binaries=unknown everywhere; F produces no data for H |
Major #1 |
| 2 | SBOM regeneration time × boundaries | Operational | Likely | Medium | produced.go:70-83 |
Three eligible stages on a large image | Minutes → tens of minutes per image | Major #2 |
| 3 | One-time rebuild of every SBOM-enabled stage | Operational | Certain | Low | build_phase.go:1832 |
First build after upgrade | Expected, undocumented | Major #3: fix the body |
| 4 | Provenance read fails on a registry hiccup → unknown for the whole build, cached under a checksum that encodes Known=false |
Operational | Possible | Low | sbom_step.go:98-105, sbomProvenanceChecksum |
Transient GetRepoImageConfigFile error |
SBOM cached without binary evidence until stage digest moves — the #453 trap, one level up | Fold a failed provenance read into a cache miss (do not encode Known=false into the checksum; recompute next time) |
Not verified
- Nothing was built, linted or run — static pass by request. Specifically unverified: that
produced_test.go/produced_overlay_test.gofail under the mutations I would try (Incomplete set only on unresolved; ranges restricted;nonenot removed on positive evidence), and that the e2e fixture builds on linux/amd64. #464(the base) was not reviewed by me;CloneBOMand the import-evidence preservation it added are taken as given.
| index.Incomplete = true | ||
| } | ||
| resolveHardlinks(ctx, index, remaining) |
There was a problem hiding this comment.
Major. remaining here is every hardlink whose own layer did not answer — that includes links #433 resolves correctly in the flattened fallback (a chain, a target in a lower layer; both have specs). Setting Incomplete on all of them makes CollectProducedFiles return {} (produced.go:50) → werf:binaries=unknown on every module → Go/Rust marking never fires for that image. On a busybox/coreutils/glibc base one chain is enough.
The guard you want is already at :363: set Incomplete only when resolveHardlinks fails to resolve a link (the unresolved counter). Drop this block, and add a spec: image with a→b→origin → Incomplete == false.
There was a problem hiding this comment.
Addressed in da08270, but not by removing the safety guard. Same-layer chains now resolve through owning-layer maps, propagate captured bodies and remain complete even when the terminal path is later replaced or removed. Regression tests cover regular content, captured historical bytes, symlink targets and produced binaries surviving only through the chain.
The proposed blanket removal is unsafe: layer 0 writes z=old binary, layer 1 creates a -> z, and layer 2 replaces z. Flattened fallback can credit a with the replacement bytes, hiding the surviving old binary. Cross-layer/unverified fallback therefore remains incomplete. The mutation removing that guard failed the overlay assertions; the PR now explicitly describes this remaining limitation.
| for _, boundary := range []int{r.Start, r.End} { | ||
| if _, ok := indexes[boundary]; ok { | ||
| continue | ||
| } | ||
| prefix, err := mutate.AppendLayers(empty.Image, imgLayers[:boundary]...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("read stage filesystem boundary %d: %w", boundary, err) | ||
| } | ||
| boundaryIndex, err := IndexImageFiles(ctx, prefix) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("index stage filesystem boundary %d: %w", boundary, err) | ||
| } | ||
| indexes[boundary] = boundaryIndex | ||
| } |
There was a problem hiding this comment.
Major (cost). Each distinct boundary that is not 0 or N triggers mutate.AppendLayers + IndexImageFiles over a prefix of the layer list — a full flatten + sha256 of every file up to that point. With install/beforeSetup/setup eligible and each adding layers, that is up to 5 full passes over growing prefixes of the same image, on top of the one CollectFilesets already did. #453 measured one pass at 65 s on node:20-slim; this is a multiple of that on every SBOM regeneration, and nothing logs it.
One top-down walk with the whiteout bookkeeping from mutate.go:295-397, snapshotting the path→digest map at each boundary, does all boundaries in a single pass. If that is out of scope for this PR, at least log boundary count and elapsed at Default so the number is visible in CI logs.
There was a problem hiding this comment.
Addressed with the bounded alternative in 6f49d04: default-level output reports actual filesystem boundary passes, scanned layers and elapsed time, and identifies hardlink fallback when it prevents attribution. Classification also skips non-eligible layers.
Prefix reconstruction itself remains multi-pass, explicitly stated in the PR description; this change does not claim a new overlay engine or an unmeasured 3–5x speedup. The new scan-cost regression counts real layer openings: two base-prefix reads are retained, while redundant base/final-layer classification reads are eliminated. Reintroducing excluded-layer scans or removing the diagnostic causes assertion failures.
| if phase.Conveyor.EnableSbom() && img.StapelImageConfig != nil { | ||
| if _, ok := serviceLabels[imagePkg.WerfParentStageID]; !ok { | ||
| serviceLabels[imagePkg.WerfParentStageID] = "" | ||
| } | ||
| record, err := json.Marshal(sbomStageRecord{ | ||
| Stage: stg.Name(), StageDigest: stg.GetDigest(), ParentStageID: serviceLabels[imagePkg.WerfParentStageID], | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("encode SBOM stage record: %w", err) | ||
| } | ||
| serviceLabels[imagePkg.WerfSBOMStageLabel] = string(record) | ||
| } |
There was a problem hiding this comment.
Major (claim). A new service label with a per-stage value is new committed config → new stage content → every SBOM-enabled stapel stage rebuilds once after upgrade. The PR body says "without invalidating image-build caches". That sentence is false and should go; the one-time rebuild itself is fine and worth a line in the release notes.
There was a problem hiding this comment.
This cache-invalidation claim does not match the actual lookup order. BuildPhase.onImageStage finds a suitable cached stage and returns before calling prepareStageInstructions; the new service label is written only after a miss. A newly committed image config has different content, but that does not change the recipe/anchor key used to select an existing stage.
b6a9ee3 adds regressions for both image-anchor reuse and an individual cached stage with SBOM enabled and no provenance label. Both pass, leave the old label absent and avoid rebuilding. A mutation rejecting such a cached stage fails the regression. The description now states the precise consequence: old image caches remain reusable, but their binary provenance remains unknown until a newly built chain supplies it. No release-note claim of an unavoidable rebuild was added.
| for _, layer := range imgLayers { | ||
| files, err := classifyProducedLayer(ctx, layer, candidates) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, file := range files { | ||
| found[file.Digest] = file | ||
| } | ||
| } |
There was a problem hiding this comment.
Minor (cost). Every layer of the image is scanned and every executable spooled + hashed, but a file can only be a candidate if it was written in a produced range (:88-97). A byte-identical copy in a base layer would match the same digest, but then the produced-range copy exists too and is found there. Restrict to the layers inside layers.Ranges; on a 1 GB base with a 10 MB app layer that is the difference between reading 1 GB and 10 MB.
There was a problem hiding this comment.
Addressed in 6f49d04: classification visits only layers in the eligible ranges, deduplicated by layer index. The regression counts actual Uncompressed openings and fails if scanning all layers is restored.
This optimization depends on keeping the completeness guard: admitted hardlink content must come from the owning layer. Same-layer chain resolution preserves that invariant. If precise cross-layer resolution is added later, classification must also track source layers or use an appropriate fallback; that dependency is documented in the collector godoc.
| if isBinaryBacked(key.ecosystem) { | ||
| decision := binaryDecision(comp, filesets.Index.Incomplete, byDigest) | ||
| if !decision.Shipped { | ||
| setProperty(comp, PropertyShipped, propertyShippedFalse) | ||
| count++ | ||
| logboek.Context(ctx).Debug().LogF("not shipped %s %s@%s (%s)\n", key.ecosystem, key.name, key.version, decision.Reason) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Minor. Binary-backed components now bypass Match entirely and go through binaryDecision; Match still has goVerdict/rustVerdict and takes ProducedBinaries, but this is its only caller and passes ProducedBinaries{} with no Go/Rust components. Two implementations of pass 1a is how they drift. Remove the dead path from Match (with its specs) or document that Match is file-backed only.
There was a problem hiding this comment.
Clarified in 328ea41 without removing the public low-level API or its tests. Match accepts a normalized evidence set; it cannot represent the completeness of multiple origins in a persisted merged BOM. MarkNotShipped handles that persisted binary evidence and delegates file-backed components to Match.
The Match godoc now directs persisted-BOM callers to MarkNotShipped and explains this boundary. Calling Match “file-backed only” would be inaccurate: its public Go/Rust behavior remains supported and tested.
| const PropertyBinaries = "werf:binaries" | ||
|
|
||
| const ( | ||
| binaryEvidencePrefix = "werf:binary:v1:" | ||
| propertyBinariesNone = "none" | ||
| propertyBinariesUnknown = "unknown" | ||
| ) |
There was a problem hiding this comment.
Minor. The werf:binaries lattice — none means "checked, nothing produced names it", unknown means "some origin could not say", absent means "a positive occurrence exists"; unknown accumulates across merges and is never removed, none is removed on the first positive — is correct and subtle. PropertyShipped has that paragraph in mark.go:13-20; this one needs the same, since a downstream reader (and the next PR) will have to reconstruct it from binaryOccurrences :160-188.
There was a problem hiding this comment.
Documented on PropertyBinaries in 328ea41: checked-empty none, accumulated origin-level unknown, replacement of none by positive or digest-bound-unknown occurrences, and preservation of inherited uncertainty.
One important qualification: absence of the property alone does not mean positive evidence exists. With neither a recognized completeness property nor a valid binary occurrence, the component is unknown. The godoc states that explicitly.
| var provenance sbomStageProvenance | ||
| if isStapel { | ||
| var err error | ||
| provenance, err = resolveSBOMStageProvenance(ctx, stageDesc, docker_registry.API().GetRepoImageConfigFile) | ||
| if err != nil { | ||
| logboek.Context(ctx).Debug().LogF("image %s: produced binaries are unknown: %s\n", werfImgName, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Risk, non-blocking. A transient failure in GetRepoImageConfigFile → provenance.Layers.Known == false → that is encoded into the checksum (sbomProvenanceChecksum) → the SBOM is pushed and cached under a key that says "unknown". The next build recomputes the same key and hits the cache — the degraded SBOM stays until the stage digest moves. Same shape as the trap #453 closed by moving the read local. Here the read is to the registry by nature; the fix is to not let a failed read into the checksum: on error, skip the cache lookup for this build (force a miss) rather than encoding Known=false. A deterministically unknown provenance (old image, no label) is fine to cache.
There was a problem hiding this comment.
The proposed sticky-cache failure does not occur with the current ordering: ConvergeWithMerge resolves provenance before calculating the checksum and before querying the SBOM cache on every invocation. Once the registry read recovers, Known, DiffIDs and ranges change the key, so the degraded artifact is not reused under the recovered key.
b6a9ee3 adds a transient-failure → recovery → stable-success regression using the real resolver and full SBOM checksum. Removing provenance from that checksum makes it fail. The existing missing-ancestor checksum regression remains. No forced cache-miss policy was added; the PR description now states the recovery behavior.
Resolve chains and captured content using only their owning layer. Keep flattened fallback incomplete so overwritten or cross-layer targets cannot turn surviving binary content into a false absence. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Limit classification to eligible layers while retaining the same-layer completeness guard. Report filesystem boundary passes, scanned layers, elapsed time and hardlink fallback at the default log level; keep prefix reconstruction unchanged. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Feed all sixteen mapper-generated stages through provenance recovery, including imports and signing. Cover registry recovery changing the SBOM cache key and image-cache hits without the new provenance label. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
|
Remaining general-review points:
All seven inline threads have individual responses with evidence. The review-response delta has a fresh independent APPROVE. On the Linux VM, the full unit suite passed (91 suites), and the produced-binaries + pip e2e selection passed (5 specs). The PR remains draft; the new CI result is not yet established. |
reyreavman
left a comment
There was a problem hiding this comment.
Code Review — follow-up on da082709…328ea41d
Delta since 4687f022: 4 commits, 17 files, +381/-16.
Verdict
All findings closed, and one of my Majors was wrong — I retract it below. Static review plus, this round, build + targeted tests + one mutation.
| # | Finding | Status |
|---|---|---|
| Major 1 | Incomplete set for every flattened-fallback hardlink |
✅ fixed — chains resolve within the owning layer; only genuine fallback stays Incomplete. file_index_chains_test.go pins all three cases (same-layer chain → Incomplete=false + captured bytes; intermediate target only in flattened view → Incomplete=true; produced binary surviving through a chain → classified). My mutation (revert chain pre-capture to single-hop) fails the capture spec. |
| Major 2 | boundary reconstruction cost unbounded/invisible | ✅ acceptably — classification restricted to eligible layers (produced.go:124-131), and boundary passes / scanned layers / elapsed / hardlink-fallback now logged at Default. The multi-pass prefix reconstruction is kept, which I flagged as the acceptable fallback; the cost is now visible in CI. |
| Major 3 | new label invalidates image-build caches | ❌ retracted — I was wrong. introspect_anchor_test.go proves the opposite: an SBOM-enabled image reuses a stage cached before the provenance label existed (AnchorReused=true, Rebuilt=false, reused stage has no WerfSBOMStageLabel, processedStages empty). It is a service label like WerfParentStageID, excluded from the content digest. The body claim is correct and now test-pinned. |
| Minor 4 | every layer scanned, not just produced ranges | ✅ eligibleLayers gate |
| Minor 5 | two 1a implementations drift | ✅ Match godoc now routes persisted BOMs to MarkNotShipped and declares itself file-backed-only |
| Minor 6 | werf:binaries lattice undocumented |
✅ godoc on PropertyBinaries |
| Minor 7 | hardcoded stage order | ✅ sbom_stage_order_test.go drives all 16 mapper-generated stages through recovery |
| Risk 4 | transient provenance → degraded SBOM cached, not retried | ✅ provenance is in the checksum, so a recovered read computes a different key than the failed one (recovers from a transient registry error before selecting the SBOM cache key): the degraded SBOM self-heals on the next successful build instead of sticking. Cleaner than the cache-miss I suggested. |
Verified this round
task buildOK, lint 0 issues,shippedscope246/246, the newpkg/buildanchor/provenance/stage-order/produced specs pass.- Mutation on
resolveHardlinksWithinLayerschain pre-capture →file_index_chains_test.gokeeps captured historical bytes after the terminal target is replacedfails as expected.
Not verified
- e2e
produced_binaries— compiled, not run (linux/amd64 + kind); the body says it was exercised on a linux host. #464(base) not reviewed by me.
Nothing outstanding on this PR from my side — I would approve. This closes the Go/Rust side of the concept: #433 → #435 → #442 → #452 → #453 → #459 → #464 → #466.
🤖 I have created a release *beep* *boop* --- ## [3.12.0-dk.2](v3.12.0-dk.1...v3.12.0-dk.2) (2026-10-22) ### Features * **build, stapel:** sign files inside the image with a key the build container never sees ([#438](#438)) ([09f22e7](09f22e7)) * **sbom:** track Go and Rust dependencies in shipped binaries ([#466](#466)) ([359006b](359006b)) ### Bug Fixes * **sbom:** preserve shipped-scope evidence across image imports ([#464](#464)) ([ac29d1b](ac29d1b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
SBOM-enabled Stapel builds now distinguish Go modules and Rust crates backed by shipped, build-produced binaries from dependencies present only in manifests. Binary evidence survives selective image imports and SBOM regeneration from cached build stages.
What
Evidence and imports
install,beforeSetupandsetuponly when stage ancestry and layer boundaries are verified; package-installation, base-image and import stages are not local production.werf:shipped=falseonly when complete evidence establishes that its binaries are absent from the final filesystem.Caches and diagnostics
werf.io/sbom-stage-v1label for newly built SBOM-enabled Stapel stages, preserving it through label removal and Docker commit quoting. Existing cached image stages remain reusable without that label; their provenance stays unknown until a new build supplies a verifiable chain.MatchAPI are unchanged.Why
A manifest says which dependencies were resolved, not which ones were compiled into the binaries that an image ships. Inspecting every executable would also attribute base-image and toolchain binaries to the current build, while keeping evidence only in memory would lose it on cached builds and downstream imports. Verified stage boundaries identify local production; persisted content-bound evidence lets each descendant judge its own filesystem without discarding uncertainty from other origins.