Skip to content

fix(sbom): keep the dependency graph intact when merged SBOMs share a package - #324

Closed
reyreavman wants to merge 3 commits into
mainfrom
fix/sbom/dedup-dependency-refs
Closed

reyreavman wants to merge 3 commits into
mainfrom
fix/sbom/dedup-dependency-refs

Conversation

@reyreavman

@reyreavman reyreavman commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

When two images being merged into one SBOM contain the same package, deduplication kept one component and dropped the other, but every reference to the dropped bom-refdependsOn, provides, vulnerabilities[].affects — was left in place. The resulting document referenced components that were not in it. Reproduces whenever a shared package has an incoming edge — a library both images' binaries link against — and affects werf build (base + imports + os-pm merge) and both --ispras-format outputs of werf sbom merge.

What

  • Every bom-ref reference to a deduplicated component — in dependencies, vulnerabilities, compositions, annotations, declarations — now names the component that survived deduplication.
  • A dependency entry whose subject was deduplicated is kept under the surviving bom-ref; previously the entry and all its outgoing edges were dropped.
  • Dependency entries that collapse onto the same subject are merged, dependsOn and provides unioned, first occurrence ordered first; an edge that becomes self-referential is dropped.
  • Nothing is merged or redirected when no component was deduplicated — a document without duplicates is unchanged.
  • A duplicate whose surviving component has no bom-ref is removed without redirection; references to the removed one stay as they are, since there is no ref to point them at.
  • Container product (--ispras-format container): on today's main its dependency graph comes from the merged BOM, so it receives this fix; once fix(sbom): keep the dependency graph intact in container-format product SBOM #321 lands the graph is copied from each image before the merge and this code no longer touches it.
  • VERIFIED: on a two-image product where the shared package is a graph root, the oss document is byte-identical before and after. This is a no-regression check only; that stand has no dangling target, so it cannot distinguish the fix from the old code — the unit specs do.
  • Nested components are not deduplicated, here or before this change; only top-level components are.

Why

dedupComponentsByPURL recorded removed components as a set of refs, which carries no way back to the component that replaced them. dropDependenciesByRefs could therefore only delete entries whose subject was removed, and had nothing to rewrite targets with — every incoming edge was left dangling, and the outgoing edges of the removed subject were lost rather than transferred. Recording removed → survivor makes both directions expressible, and the package already has rewriteAllRefs for exactly that map shape — the same rewriter ensureUniqueBOMRefs uses — so the fix reuses it rather than remapping dependencies alone.

The ISPRAS checker does not validate that refs resolve, so this passes validation and surfaces only when a consumer walks the graph — for example to decide which components are reachable from a root.

Deduplication dropped a component but left every dependsOn entry pointing at its bom-ref, so a package shared by two images produced a graph with dangling targets. Removed refs are now redirected to the surviving component, collapsed entries merged.

Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Reuse the package's existing ref rewriter so vulnerabilities, compositions, annotations and declarations follow the surviving component too, instead of a dependencies-only remap. Merging entries that collapse onto one subject stays as a small step after the rewrite.

Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
@reyreavman

Copy link
Copy Markdown
Collaborator Author

Verification

  • Mutation loop on pkg/sbom/cyclonedxutil/dedup.go at HEAD, run with task -y test:unit paths="./pkg/sbom/cyclonedxutil/...":
    • rewriteAllRefs call removed → 7 specs fail
    • seen[key] overwritten on every collision (would chain c→b→a) → 8 specs fail
    • survivor != "" guard removed → "leaves refs untouched when the surviving duplicate has no bom-ref"
    • collapsed entries not merged (merging forced false) → "merges dependency entries…"
    • self-ref guard removed → "drops self-references…"
    • len(replacedRefs) > 0 gate on the merge step removed → survives. Reaching it needs a document with two entries for one ref and no duplicate components, which is invalid CycloneDX; left uncovered on purpose.
  • Hand-run against a real two-image product in a local registry: werf sbom merge --ispras-format oss output compared before/after — 10 components, 3 dependency entries, 0 dangling refs both times. No-regression evidence only (see VERIFIED: in the description).

Review focus

  • unionRefs returns nil when every ref in a non-empty list was dropped as a self-reference, so a one-element dependsOn becomes an entry with no dependsOn key. In this repo nothing distinguishes absent from empty (ispras/bomref.go itself only writes dependencies when non-empty), and CycloneDX treats both as a leaf.
  • Entry order: the merged entry keeps the position of the first occurrence; the surviving component is the first by document order.

Follow-up

@reyreavman

reyreavman commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #329 — closing.

The approach changed. Here dangling dependsOn entries were repaired after a duplicate was removed; in #329 duplicates are no longer removed — components with matching identity are merged, and the bom-ref of the removed copy is redirected across all sections at once. Dangling references cannot occur by construction, and the data of the removed copy (references, properties, hashes, licenses) is no longer lost.

#329 additionally covers a case this PR did not: when the only bom-ref belonged to the copy being removed, the whole graph detached (4423b5d502).

Kaiten card 70002611 is closed and the work continues in a new one, 70686804.

@reyreavman reyreavman closed this Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant