fix(sbom): stop duplicating components and links across merged SBOMs - #329
reyreavman wants to merge 42 commits into
Conversation
…ct SBOM The container assembler took components from the namespaced per-image BOMs but dependencies from MergeBOMs, which had already rewritten every ref to a purl+package-id form and mutated the shared slices in place. No dependency subject or target matched a component in the result, so the product graph was unusable while the ISPRAS checker stayed silent. Copy the per-image dependency graphs before the merge and use them as the product graph, so every ref resolves to a namespaced component. Cover it with a unit test on the assembler and assert graph integrity in the multi-image lifecycle e2e, whose fixture now includes a package with real dependencies. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
GOST upsert and validation walked only metadata.component and the top-level components list, and the container value in the product SBOM was aggregated from the top level alone. A base or imported SBOM with nested components kept its old values, passed validation with missing properties, and could hold a higher value than the container above it, which the ISPRAS checker rejects. Recurse through nested components in Upsert, Validate and aggregateGOST so the whole tree gets the configured values, is checked, and the container reflects the maximum over all descendants. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…nt too Upsert and Validate descended into bom.Components but not into metadata.component.components, so a BOM whose root component nests its parts — the shape werf itself emits for a container-format product — left those parts untouched and unvalidated. Walk them with the same recursion. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Verification
Review focus
Follow-up
|
…n SBOM Deduplication compared array items byte for byte and ran once, after every bom-ref had been rewritten to be unique, so components without a purl duplicated on every merge, duplicate components were dropped together with their data, and every dependsOn, provides and affects that pointed at a dropped copy was left dangling. GOST properties and root-level properties were never deduplicated; nested components not at all. Replace it with a canonicalization pass that defines the identity of every CycloneDX entity, merges duplicates into the surviving entry (external references, properties, hashes and licenses unioned, GOST values collapsed to the strongest one, at most one vcs link) and rewrites every reference to the survivor. Inputs of a merge are deep-copied so that base and import SBOMs reused across images are not rewritten in place; a bom-ref derived from a purl no longer carries two package-id qualifiers. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
The external reference enricher copied the union of all resolved links into the root externalReferences in map order, so every vcs url appeared at the component and again at the root, and the artifact differed byte for byte between builds of the same input. Links now stay on their components only, and the merged SBOM is canonicalized after all patchers have run. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
The container assembler discarded the merged components and re-nested the original per-image ones, so no dependency ref matched a component. Wrap each image's components into its container before merging and keep the namespaced refs through the merge; the edges of the image root component move to the container that replaces it. Namespacing also covers the metadata component, provides, affects, compositions and annotations, so nothing is left dangling under the shared ref rewriter. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
An image referenced by more than one import directive was resolved and its SBOM pulled once per directive, so the same BOM entered the merge repeatedly. Components carrying a purl collapsed during deduplication, but components without one kept a distinct bom-ref per copy and survived, landing in the resulting SBOM as many times as the image was imported. Each extra copy also cost a registry round trip. Resolve the import sources first and keep a single entry per repository and digest, so every imported image contributes its SBOM exactly once. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
2c2d847 to
98e0e61
Compare
…ly bom-ref A component without a bom-ref that absorbed a duplicate carrying one left every reference to the duplicate pointing at nothing. The survivor now takes over that ref, and the dependency edges follow it. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Cached SBOM artifacts are keyed by the generator version; without a bump an image whose SBOM was already published keeps serving the pre-merge document with its duplicates and dangling refs until the image itself changes. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
A file component has no purl, and two files with the same name are not the same file; identify files by their hashes and never merge a file without them. When a merged duplicate carries an SPDX expression, keep only the expressions: CycloneDX forbids a license list mixing expressions with individual licenses. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Validation against real product SBOMsNot a full diff review — a comparison run of this PR head ( Result: the PR does what it claims
The key confirmation: the number of components without a purl in the merged oss document drops from 216 to 1. Those 216 were byte-identical copies of Duplicate vcs links: 3 → 0 warnings ( Major — the vcs link selection policy is currently correct by accident
In 2 of 3 pairs the dropped URL is dead, so "first wins" currently saves us from unreachable resolver output. But that is an artifact of merge order, not a policy: as soon as a pair shows up where the cataloger's URL is the broken one, Minor — outside this PR's scope, but visible in its output
Different purls because the two catalogers normalize the version differently ( What stays red after this PR (context, not requests for this PR)
|
… resolved one When a component carries several vcs links only one may stay, and which one used to fall out of slice order: the package manager's link happened to come first, the resolver's second. Mark the resolver's link as resolved from the purl and prefer an unmarked one explicitly, so a build that merges links in a different order does not silently switch to the guessed url. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
|
Thanks — the comparison run is exactly the evidence this PR was missing. Purl-less duplicates on already published SBOMs — added to What as its own claim with your numbers (216 → 1, E=223 → 8), marked Major — vcs selection by slice order — fixed in Minor — What stays red — all three are producer-side ( |
Fral738
left a comment
Fral738
left a comment
There was a problem hiding this comment.
Changes needed
The canonical merge still loses SBOM information in four cases detailed inline:
- Nested component merges produce transitive ref mappings, but rewriting follows only one hop. Vulnerability references can name a removed component and dependency edges are filtered out.
- Container output collapses distinct image wrappers when their metadata PURLs match, losing per-image package ownership.
- Replacing an image root redirects dependency subjects only, leaving references to that root dangling. This is an incomplete part of the advertised reference-preservation fix, not a claim that every affected reference type worked on main.
- Import deduplication ignores the image-name selector even though that selector distinguishes SBOM artifacts at the same repository/digest.
Description
The statements that every reference is rewritten to the survivor and that container copies stay separate need the corrections above. The additional claim that undeclared affects entries are dropped is not implemented: canonicalizeVulnerabilities deduplicates/unions them without checking known refs. Either implement that policy or narrow the description.
The license-producer problem, cataloger version disagreement, and newer checker schema requirements already listed in the author's follow-up are excluded from these findings.
Test gaps
- Input-isolation tests: replacing
boms[i] = clonewith_ = cloneleaves all three tests passing. Add two components sharing a PURL with refslib/lib-dup, plus a vulnerability affectinglib-dup, to the before/after serialization fixture. That strengthened fixture passes with the original implementation and fails under the same mutation because the input reference changes fromlib-duptolib. - Enricher tests: clearing the generated resolver
Commentleaves all 25 Enricher specs passing. AssertComment == cyclonedxutil.ExternalReferenceCommentResolvedon the actual enrichment output and pass it through canonicalization before an unmarked package-source VCS link. The preference test currently supplies the marker manually, so it does not protect its production.
Verification
Reviewed head 5885fd43088409bef0741fb22f2573f305bf3946 against main (9fc5085a917292eb7b61847ef7f1f0757c7176a2); acceptance criteria inferred from the description and author comments. task build and the full task test:unit passed. Temporary probes reproduced the nested-ref and container/root-reference cases. Import artifact selection was verified by code tracing, not a registry end-to-end reproduction. Mutations exercised ref remapping, input isolation, same-kind enrichment, GOST traversal/precedence, and container root/namespacing behavior. Original files were restored and scoped tests passed afterward.
E2E/integration and exhaustive mutation coverage were not run. Remaining mutation checks include disabling package-id normalization, merging unlike file hashes, retaining mixed license choices, and independently removing composition/annotation rewriting. The local Ginkgo CLI also warns about a version mismatch (2.20.1 vs library 2.28.1).
|
|
||
| bom.Components = canonicalizeComponents(bom.Components, refMap) | ||
| bom.Services = canonicalizeServices(bom.Services, refMap) | ||
|
|
There was a problem hiding this comment.
Recursive merging can leave a chain in refMap, while RewriteRefs resolves only one hop. For duplicate parents containing children [child-a] and [child-b, child-c] with the same child identity, the map becomes child-c -> child-b -> child-a. A probe with a vulnerability affecting child-c returns child-b, although only child-a survives; a dependency on the intermediate ref is then removed by known-ref filtering.
Keep the map flattened whenever a duplicate is redirected, including mappings produced by recursive merges. For example, update existing aliases before recording the new mapping in the component merge path:
for ref, target := range refMap {
if target == comp.BOMRef {
refMap[ref] = survivor.BOMRef
}
}
refMap[comp.BOMRef] = survivor.BOMRefApply the same invariant to other identity-merging paths and add the two-parent/three-child regression case for both dependsOn and affects.
| } else { | ||
| result.Components = nil | ||
|
|
||
| result, err := cyclonedxutil.MergeBOMs(nil, cyclonedxutil.MergeOpts{ |
There was a problem hiding this comment.
Passing the wrapper containers through generic component canonicalization merges distinct logical images whenever their metadata PURLs match. The wrapper inherits Metadata.Component.PackageURL, and componentKey prefers that PURL over its distinct image name/ref; PreserveBOMRefs does not disable identity merging. Giving imageBOM("a") and imageBOM("b") the same metadata PURL produces one container instead of two. Their package trees and ownership collapse together.
Keep the per-image wrappers outside cross-image identity merging, while canonicalizing each image's contents and retaining its original metadata PURL. Extend the existing container-isolation fixture with the colliding identity:
images[0].BOM.Metadata.Component.PackageURL = "pkg:oci/shared@sha256:aaa"
images[1].BOM.Metadata.Component.PackageURL = "pkg:oci/shared@sha256:aaa"The existing HaveLen(2) assertion must still hold, with the two separately namespaced package trees.
| imgBOM := *img.BOM | ||
| imgBOM.Components = &[]cdx.Component{container} | ||
| if img.BOM.Metadata != nil && img.BOM.Metadata.Component != nil && img.BOM.Metadata.Component.BOMRef != "" { | ||
| imgBOM.Dependencies = rootDependenciesAs(img.BOM.Dependencies, img.BOM.Metadata.Component.BOMRef, img.Name) |
There was a problem hiding this comment.
The root replacement changes only dependencies[].ref. A reference targeting the old root in dependsOn, provides, affects, compositions or annotations is not redirected to the replacement container. A probe with affects: [{ref: "a/root"}] retains a/root after assembly even though the surviving container ref is a; dependency targets naming that old root are instead pruned.
Replace all uses of the old root ref on a deep copy of the image BOM, rather than only changing dependency subjects:
cyclonedxutil.RewriteRefs(&imgBOM, map[string]string{
img.BOM.Metadata.Component.BOMRef: img.Name,
})The copy must be deep before this call because the current imgBOM := *img.BOM shares reference-bearing slices with the input. Cover root targets as well as dependency subjects in the assembler regression test.
| } | ||
|
|
||
| if digest := importImageInfo.GetDigest(); digest != "" { | ||
| key := importImageInfo.Repository + "@" + digest |
There was a problem hiding this comment.
Repository/digest is not the complete identity of an imported SBOM. Two internal images can share that image digest but have differently named SBOM artifacts; GetImageBOM forwards lookupName to PullSBOM, and matchesReaderFilter explicitly selects by WerfImageNameAnnotation. This key drops the second internal image before its name-specific artifact is read, losing its SBOM contribution. It also collapses an unnamed external lookup with a named internal lookup.
Compute importLookupName before deduplication and include it in a structured key, retaining empty-name alias deduplication for external imports:
type importSBOMKey struct {
repository string
digest string
lookupName string
}
seenImages := make(map[importSBOMKey]struct{})Use importSBOMKey{importImageInfo.Repository, digest, importLookupName} for the lookup and add a test with two distinct internal lookup names at the same repository/digest.
Merging duplicate parents merges their children too, so a child ref could be mapped onto a ref that a later merge removed in its turn. Rewriting resolved a single hop and left the intermediate ref behind: a vulnerability then named a component that no longer existed, and the dependency edge through it was dropped as unknown. Follow the mapping to its end instead, stopping on a cycle. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
The container wrapping an image inherited the purl of the image metadata component, and component identity prefers the purl over anything else, so two images built from the same contents collapsed into one container and one of the package trees was lost. Canonicalize each image on its own and keep the wrappers out of cross-image identity merging. Replacing the image root with the container also rewrote dependency subjects only, leaving every other reference to the root dangling; rewrite all of them, on a deep copy so the caller's BOM stays untouched. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…laimed Neither spec failed when the behaviour it describes was removed: the isolation fixture had nothing to canonicalize, so mutating the input clone away changed nothing, and no spec asserted the resolver marker that the vcs preference reads, only a hand-written copy of it. Give the fixture a duplicate pair and assert the marker on real enrichment output. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Two internal images can share a repository and digest while their SBOM artifacts are told apart by the werf image name they carry, so deduplicating by repository and digest alone dropped the second image before its artifact was read. Include the lookup name in the key. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
|
Reproduced all four findings with probes before fixing anything — all four hold. Thanks: two of them broke exactly the property this PR exists for. 1. Ref chains in 2. Containers collapsing on the metadata purl — 3. Image root replacement — same commit. 4. Import deduplication key — On Test gaps —
Mutation check on all six new/changed places (after first confirming the focus actually selects the spec — my initial run silently executed the whole suite):
Local: Re ginkgo 2.20.1 vs 2.28.1: that is a local CLI, not pinned in the repo; CI has no such mismatch. |
Fral738
left a comment
Fral738
left a comment
There was a problem hiding this comment.
Remaining changes
Two regressions introduced by the follow-up fixes are reproduced on this head:
- Transitive
remapRefchanges simultaneous namespace renames into alias resolution. With original refslibandimg/lib, namespacing byimgsilently moves a vulnerability from the first component to the second (img/img/lib). IsolateComponentsalso skips cross-image service canonicalization. Two images carrying the same service produce two declarations with the samebom-ref, leaving the product BOM with ambiguous service identity.
The inline comments describe the distinct contracts that need preserving. The description's reference-preservation claim is not satisfied by merely keeping references resolvable: the first reproduction points to an existing but wrong component.
Recheck scope and verification
Follow-up diff 5885fd4308..fda2c919e9 (10 files), with main at 9fc5085a917292eb7b61847ef7f1f0757c7176a2 as the PR base. Acceptance criteria inferred from the updated description and author response. The earlier reproduction cases are addressed; the revised decision to retain undeclared affects is documented and is not repeated as a finding.
task build and the full task test:unit passed on the unmodified head. Two temporary regression probes failed with img/img/lib instead of img/lib, and service refs [svc svc]. Single-hop remapping makes the namespace probe pass but fails the new nested-merge-chain test. Restoring full result canonicalization makes the service probe pass but fails equal-PURL container isolation. Therefore neither regression can be fixed by simply reverting its previous fix wholesale.
Addressed-test mutation checks: bypassing the clone fails input isolation; clearing the resolver marker fails both new marker/precedence assertions; removing lookupName fails the internal-import test; disabling root rewriting fails the root-affects assertion. Original files were restored after each mutation, and the corresponding restored tests passed.
No persistent source changes. E2E/integration and a live-registry import scenario were not run in this recheck.
| } | ||
|
|
||
| seen[newRef] = struct{}{} | ||
| ref = newRef |
There was a problem hiding this comment.
Following every mapping transitively corrupts simultaneous renames made by NamespaceBOMRefs. For two distinct original component refs lib and img/lib, prefix img creates mappings lib -> img/lib and img/lib -> img/img/lib. The first component is declared as img/lib, but its affects reference now becomes img/img/lib, assigning the vulnerability to the second component. The namespace probe fails with Expected img/img/lib to equal img/lib; temporarily restoring one-hop remapping makes it pass, while the nested-merge-chain regression test then fails.
Resolve transitive merge aliases inside canonicalization, before calling RewriteRefs, and preserve one-hop semantics for ordinary namespace/ref regeneration:
func remapRef(ref string, refMap map[string]string) string {
if mapped, ok := refMap[ref]; ok {
return mapped
}
return ref
}Keep the cycle-guarded chain resolution as a separate canonicalization-only operation that flattens its alias map. Add the overlapping lib/img/lib case alongside the existing nested-merge test so both contracts remain protected.
| result.Declarations = mergeDeclarations(boms) | ||
|
|
||
| ensureUniqueBOMRefs(result) | ||
| if opts.IsolateComponents { |
There was a problem hiding this comment.
This branch isolates services as well as wrapper components, but service refs have not been made unique across images: NamespaceBOMRefs only namespaces component declarations, and the container assembler sets PreserveBOMRefs: true. Giving both existing image fixtures Services: &[]cdx.Service{{BOMRef: "svc", Name: "api"}} produces service refs [svc svc]. The uniqueness probe fails; restoring full Canonicalize(result) makes it pass but reintroduces the equal-PURL wrapper collapse.
Keep wrapper isolation without dropping service identity merging. For example, canonicalize services and rewrite their aliases before document-only canonicalization in this branch:
refMap := map[string]string{}
result.Services = canonicalizeServices(result.Services, refMap)
RewriteRefs(result, refMap)
CanonicalizeDocument(result)Add the two-image service fixture to the assembler tests while retaining the equal-metadata-PURL test. If services must also remain per-image, namespace their declarations and references consistently instead of retaining colliding refs.
Following a ref mapping to its end broke namespacing: with components "lib" and "img/lib", prefixing by "img" maps the first onto the ref the second already has, and everything pointing at the first ended up pointing at the second. A rename maps distinct entities at once, so it must be applied exactly once; only the aliases left by merging duplicates describe entities that are gone. Flatten those inside canonicalization instead, before rewriting. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Isolating the components of each image also stopped services from merging, and service refs are not namespaced per image, so two images declaring the same service left the product BOM with two services sharing one ref. Canonicalize services across the merged BOMs and rewrite their refs before the document pass. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
|
Both regressions reproduced before touching anything — both are mine, and both are real. Thanks for catching that the two contracts pull in opposite directions; that is exactly why neither could be fixed by reverting the other. 1. Transitive remapping corrupted simultaneous renames —
So 2. Mutation check — each contract is now protected by its own test, and the other test passes on its own merits:
Local: On the description: agreed that "references resolve" is not the claim worth making — a reference that resolves to the wrong component is worse than a dangling one. The reference-preservation wording is what the rename regression violated, and the new test pins the stronger property (a reference keeps naming the entity it named). |
… stop handing them to the container Image SBOMs published before this change list the vcs link of every package at the document level. The container component inherited that list and the single-vcs rule then kept one arbitrary package repository as the source of the image, while the product document lost the links of every image but one. The rule now applies to components only. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…e SBOM Service refs are not namespaced, but every unknown ref in the dependency graph was, so an edge pointing at a service stopped matching the service and the product merge dropped it as dangling. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…onent They were neither merged nor known to the dangling-ref check, so every dependency edge pointing at one of them was dropped. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…ping most of it Only external references and properties were carried over; every other field and the nested services of the duplicate were lost, and refs to those nested services were dropped as dangling. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…n entity that survives A BOM may reuse one bom-ref for several entities. When a duplicate carrying such a ref merged away, every reference to the ref was rewritten onto the survivor of the merge and the edge to the entity that kept the ref was lost. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…lacks Author, publisher, copyright, pedigree and a dozen more fields of the duplicate were silently dropped. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
… document The container shared its property slice with the image document, so the GOST properties set on the container were appended to the document properties of the image and merged into the product document. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
… of each image AssertDependencyGraphResolves cannot fail on a dropped edge, since canonicalization drops every dangling edge by construction. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
|
Independent review round (two reviewers plus a manual pass over the sections earlier rounds never examined). Every finding below was reproduced with a probe before the fix and each fix carries a regression test that was mutation-checked: the fix was reverted and the test seen failing. Data loss and wrong-entity references
Tests
Description
Considered, not changed
|
Fral738
left a comment
Fral738
left a comment
There was a problem hiding this comment.
The service-reference fix still assumes that refs from different source BOMs share a global namespace. Two individually valid inputs declaring different services as svc produce two output declarations with that ref and ambiguous incoming edges. Namespace service declarations before combining documents, then merge equal service identities and rewrite their aliases.
The new metadata-child handling is incomplete across the product pipeline: standalone canonicalization retains the child, but namespacing does not rename its declaration and container assembly overwrites it when top-level components exist. The claim that these entities and their edges are preserved needs coverage through assembly, not only through Canonicalize.
The new edge-preservation assertion also rejects a correct OSS graph when an endpoint has no PURL, because OSS regenerates that component's ref while the caller passes an identity mapping. The serialized source/output refs must be matched by entity identity before comparing edges. This is a demonstrated helper false positive, not a claim that the current lifecycle fixture necessarily exercises it.
Finally, the new missing-field preservation pass still omits Version. The metadata pipeline and missing-version cases already existed before this follow-up; they remain gaps in the newly expanded fixes and their description, rather than regressions introduced by the latest commits.
| if _, known := refMap[ref]; known || ref == "" { | ||
| return | ||
| } | ||
| if _, service := serviceRefs[ref]; service { |
There was a problem hiding this comment.
Leaving service refs unchanged preserves collisions between different source documents. Source A can declare {bom-ref: "svc", name: "api"} and source B {bom-ref: "svc", name: "db"}; both BOMs are individually valid. After namespacing and container assembly the result still has two services named by svc, and both images' edges target that ambiguous ref. canonicalizeServices does not merge them because their names differ. The two-image probe fails the distinct-ref assertion with Should not be: "svc"; the existing test covers only equal service identities.
Namespace service declarations recursively together with their references before combining documents; retain cross-image identity canonicalization afterward so genuinely shared services still collapse. The service traversal can populate the same simultaneous-rename map as components:
for index := range services {
service := &services[index]
if service.BOMRef != "" {
oldRef := service.BOMRef
service.BOMRef = namespacedRef(oldRef, prefix)
refMap[oldRef] = service.BOMRef
}
namespaceServiceBOMRefs(lo.FromPtr(service.Services), prefix, refMap)
}Cover both equal identities and different identities that reuse the same source-local ref, asserting the target of each image's edge.
| refMap := map[string]string{} | ||
|
|
||
| if bom.Metadata != nil && bom.Metadata.Component != nil { | ||
| namespaceComponentBOMRef(bom.Metadata.Component, prefix, refMap) |
There was a problem hiding this comment.
The metadata root is renamed here, but its nested components are not. With metadata.component.components = [nested] and a top-level os -> nested edge, namespacing changes the edge target to a/nested while the declaration stays nested. Container assembly then overwrites those root children with the top-level components. The Canonicalize -> NamespaceBOMRefs -> ContainerAssembler.Assemble probe ends with refs {a, a/os, a/lib}: a/nested and its incoming edge are gone. The new standalone metadata test does not exercise this path.
Recurse below the metadata root when building the rename map:
namespaceComponentBOMRefs(lo.FromPtr(bom.Metadata.Component.Components), prefix, refMap)Also retain its children when adding top-level components in ContainerAssembler, instead of replacing them:
container.Components = lo.ToPtr(append(
slices.Clone(lo.FromPtr(container.Components)),
lo.FromPtr(imgBOM.Components)...,
))Add an assembler test with both layouts present and assert the retained declaration and exact dependency target; both corrections are needed.
| sbomtest.AssertDependencyGraphResolves(merged) | ||
| for name, imageBOM := range imageBOMs { | ||
| sbomtest.AssertKeepsDependencyEdges(merged, imageBOM, func(ref string) string { | ||
| return lo.Ternary(isprasFormat == "container", name+"/"+ref, ref) |
There was a problem hiding this comment.
The identity mapping in OSS mode is wrong for components without a PURL. OSSAssembler calls MergeBOMs with ref regeneration enabled, so an OS component receives a new serial-derived hash. AssertKeepsDependencyEdges only strips package-id from PURLs and cannot relate the two hashes. A probe that first verifies the output OS-to-library edge against the actual output component refs still fails this helper with <old OS hash> -> pkg:golang/lib@1.0 reported missing. No edge was lost.
Build a source-ref-to-output-ref map using the merge's entity identity rules (including type/group/name/version for purl-less non-file components), then supply that mapping to the helper instead of returning the input ref:
sbomtest.AssertKeepsDependencyEdges(merged, imageBOM, func(ref string) string {
return outputRefBySourceRef[ref]
})Use a purl-less OS-to-library fixture to prove a correct OSS result passes, and delete that output edge to prove the same check fails. Preserve the image namespace when constructing the mapping for container mode.
|
|
||
| takeString(&survivor.MIMEType, dup.MIMEType) | ||
| takeString(&survivor.Group, dup.Group) | ||
| takeString(&survivor.Author, dup.Author) |
There was a problem hiding this comment.
The expanded missing-field merge still drops Version. Two components may have the same PURL while only the second supplies the optional version field. They merge by PURL, but Version is not copied, so the populated value disappears when the first component survives. A probe with PURL pkg:generic/lib@1, survivor version empty and duplicate version 1 returns an empty version. The new whole-struct test misses this because both input versions are already 1.
Carry over the missing version alongside the other strings:
takeString(&survivor.Version, dup.Version)Remove Version: "1" from the survivor in the whole-struct fixture while keeping it on the duplicate and in the expected result.
… cannot share one ref Two images may declare different services under one source-local bom-ref; the product then carried two services with that ref and edges pointing at an ambiguous target. Service declarations are namespaced with their references now, and equal service identities still collapse when the image documents are merged. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…roduct assembly Namespacing renamed the image root but not its nested components, so edges to them became dangling, and the container assembler replaced the root's children with the image's top-level components. Both are preserved now. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…survivor lacks Components merge by PURL, so a duplicate may be the only one carrying the optional name or version; those values disappeared when the survivor had none. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
… by component identity A component without a PURL gets a ref derived from the merge, so the identity mapping the oss lifecycle entries pass could not relate it and the edge assertion reported a loss that never happened. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
The images were pulled in map iteration order, and the merged product depends on the order of the documents it merges: the same input mapping produced different component order, and in oss format different surviving duplicates, on every run. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…te the refs that pointed at it Merging two entries of one vulnerability kept only the list fields of the duplicate, so its description, dates, credits, tools and analysis were lost when the survivor lacked them, and the bom-ref of the dropped entry stayed in compositions and annotations pointing at nothing. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
… well The oss merge regenerated the refs of top-level entities only, so nested components of two imports that shared a ref stayed duplicated and the edges pointing at them became ambiguous. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…es per image Only the metadata component, the components and the services were namespaced, so two images declaring a tool, a formula or a vulnerability under the same ref produced duplicate refs in the container product. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…hing Dependencies were already filtered against the entities the BOM declares, while compositions and annotations kept every ref, including those of entities that were merged away or never existed. A BOM-Link addresses another document and is left alone. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…r instead of copying them The properties were copied onto the container and also left on the image document, so the merge unioned the document properties of every image into the product document, where the GOST names of different images collapsed into one value. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…n empty ref as declared AssertKeepsDependencyEdges walked the top-level components only, so the edges from the image root to its packages went unchecked; the lifecycle test maps the root onto the container and skips it in oss format, where the merge drops it by design. AssertDependencyGraphResolves registered the empty ref of ref-less components, letting a dependency with an empty ref pass. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
An annotation may take the serial number of its BOM as subject; the dangling-ref filter would have dropped it. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
The container took over the image root but its properties were then replaced by the document properties, so the GOST values configured for the image were lost and re-derived from the strongest value among its packages, which can turn a configured no into yes. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
A composition with an aggregate and no refs is a legal statement about the entire BOM; the dangling-ref filter dropped it along with the ones whose refs were all gone. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…ve BOM-Links alone Compositions and annotations of two images declaring the same ref collided in the container product, and a dependency on an entity of another document got the image prefix, which turned the BOM-Link into a plain ref pointing at nothing. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
The dangling-ref filter on dependencies dropped BOM-Links, which address an entity this document cannot check, while compositions and annotations kept them. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
|
Addressed the 2026-09-21 review, then ran two more independent review passes over the branch and fixed what they found. 16 commits since Review findings
Found by the additional passes
Not changed
Description updated: the "service refs are left alone" claim was wrong and is replaced; the doc-properties, ordering and namespacing scope claims are new. |
2 participants
Summary
Image and product SBOMs no longer carry duplicate components, duplicate links or dependency edges that point at nothing. Merging a base image, imports and an os-pm scan into one SBOM, enriching it and assembling a product SBOM now goes through a single canonicalization pass that defines what makes two CycloneDX entities the same and merges them, rewriting every reference to the survivor. Reproduces on any stapel project with
packages: os-pmwhose base image shares packages with the image itself, merged withwerf sbom merge --ispras-format container.Supersedes #321 and #324 (both closed) and carries #321's nested-component GOST handling unchanged — its three commits are part of this branch by the same hashes.
What
Image SBOM (
werf build)package-id) or, without a purl, the same type, group, name and version become one component; its list-valued fields (external references, properties, hashes, licenses, authors, tags, data, omniborId, swhid) are the union of both, and every scalar field the survivor lacks (CPE, description, scope, supplier, manufacturer, author, publisher, copyright, pedigree, evidence, SWID, release notes and the rest) is taken from the duplicate.bom-refs were rewritten to index-derived hashes at publish time, and coordinates are the only identity that survived that.werf sbom mergeover existing images drops them without a rebuild. VERIFIED: on a 6-image product set,operating-system/deckhouse/1.0goes from 216 copies to 1; the pinned ISPRAS checker reports 8 errors instead of 223 on the oss product.vcslinks keeps the one the package manager reported (os-pm'sOriginalRepo); a link the PURL resolver looked up is kept only when there is no other, and is marked with the commentresolved from purlso the preference does not depend on the order links were added. The single-vcs rule applies to components only: the document and a service may legitimately list the sources of many components, so their lists are only deduplicated.dependsOn,provides,vulnerabilities[].affects, composition and annotation that named a merged duplicate names the survivor, including when a chain of merges moved it more than once.dependsOnandprovidesentries no entity declares are dropped, and adependsOnnever repeats an entry or names its own subject.affectsentries are deduplicated and unioned but never dropped: a BOM may carry vulnerability data for components it does not itself list, and discarding it would lose security information.assemblies/dependencies/vulnerabilitiesand annotationsubjectsnaming abom-refno entity declares are dropped too; a composition or annotation left with no refs is removed, while a composition that never had any (aggregateonly) stays, as does an annotation whose subject is the document's ownserialNumber.GOST:attack_surface/GOST:security_functionproperty; when duplicates disagree the stronger GOST value wins (yesoverindirectoverno).componentsare canonicalized the same way; previously they were never deduplicated.filecomponent is merged only with a file carrying the same hashes; a file without hashes is never merged. Two files with the same name but different content stay separate.bom-refhands its ref to the survivor, so edges that named the duplicate stay attached.dependsOnorprovidesentry naming abom-refthat no entity in the document declares is dropped without a log line. Neither syft nor os-pm produce such entries. A BOM-Link (urn:cdx:…) addressing an entity of another document is kept in dependencies, compositions and annotations alike, since this document cannot check it.affectsis deliberately exempt, so a vulnerability may still name a ref the document does not declare.externalReferencesis no longer populated with a copy of every component's vcs link. BREAKING for a consumer that read the image-level list as "all sources of the image": readcomponents[].externalReferencesinstead.vcsorsource-distribution, the two the ISPRAS schema accepts on a component; any other kind (website,distribution, …) fails enrichment withexternal reference kind %q is not allowedand the build with it. BREAKING for a resolver deployment that answers with another kind: previously every CycloneDX reference type was accepted and the SBOM then failed ISPRAS validation.metadata.componentare canonicalized and count as declared entities; previously an edge pointing at one of them was dropped as dangling.package-idreplaces the qualifier instead of appending a second one (?package-id=aaa&package-id=…no longer occurs).clone BOM for merge: …instead of silently falling back to the shared object.affects,ratings,advisories,cwes,referencesandproperties, and every scalar the survivor lacks (description, detail, recommendation, workaround, dates, proof of concept, credits, tools, analysis) is taken from the duplicate; every composition and annotation that named the dropped entry'sbom-refnames the survivor. Previously only the list fields were unioned and the droppedbom-refstayed referenced.Product SBOM (
werf sbom merge)--ispras-format containereverydependencies[].refanddependsOnentry resolves to a component in the container tree; the edges of an image's root component belong to the container that replaces it, so an OS component keeps its incoming edge.bom-refs as well; previously only top-level entities did, so nested entities of two images sharing a ref stayed duplicated.bom-refan image declares — the metadata component with its nested components,metadata.tools, components, services (nested included), formulation, vulnerabilities, compositions and annotations — and every reference to them (dependencies,provides,affects, compositions, annotations). Two images declaring different entities under one source-local ref stay two entities in the product. BREAKING for a consumer that looked services up by their sourcebom-ref: in container format a service of imageadeclared assvcis nowa/svc; services of equal identity across images still collapse into one, under the ref of the first image.metadata.component.components) survives the container assembly together with its edges; previously the container's children were replaced by the image's top-level components.propertiesof the image; the product document carries no image-level properties. Previously the root's properties were replaced by the document's, so a GOST value configured for the image was re-derived from the strongest value among its packages (nocould becomeyes), and the document properties of every image were unioned into the product document, where the GOST names of different images collapsed into one value.werf sbom mergeprocesses the images of the input mapping in name order, so the same mapping produces the same product document on every run; previously the order followed Go map iteration and the component order — and in oss format the surviving copy of a duplicate — differed between runs.3p-ispras-sbom-checker:master) reportsфайл корректныйfor oss and container output assembled from inputs with duplicated OS components, duplicated purls and duplicated vcs links.test/e2e/sbom) were not run locally — fixture images are amd64 only; CI covers them.Why
There was no notion of entity identity:
DedupBOMremoved byte-identical array items once, in the middle of the pipeline, afterensureUniqueBOMRefshad already given every component a fresh bom-ref, so the component pass was dead code and the purl pass dropped duplicates without redirecting anything that referred to them. The enricher then appended to already enriched components, the assembler discarded the merged components but kept the merged graph, and none of it ran after the patchers. Each of #321 and #324 fixes one symptom in its own place; the graph they left behind would still show a transitive dependency as a root, which any in-degree based GOST derivation would misclassify.Alternative considered: keep
DedupBOMand add ref remapping to it (#324). Rejected because it leaves purl-less components, nested components, properties and post-patcher duplicates unaddressed and keeps the dedup at a point where the identity information is already lost.Kaiten: 70686804. Cards 70002558 and 70002611 are closed as taken a different way: their point fixes are replaced by the canonicalization here.