fix(sbom): stop sbom validation failing on duplicate component links - #326
reyreavman wants to merge 6 commits into
Conversation
The ISPRAS checker can verify that a source-distribution url exists and points to an archive, and can restrict vcs checks to leaf components, but neither switch was reachable: only --check-vcs was passed to the container. A dead source distribution link therefore passed validation silently. Expose both as --check-source-distribution and --check-vcs-leaf-only. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
A component that already carries an external reference of the resolved kind got a second one appended: os-pm supplies a vcs link of its own, and every downstream image re-enriches an already enriched BOM. Two links of the same type make the ISPRAS checker report the component, which werf treats as a validation failure. Keep the reference already present and skip the resolved one. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
…ccept The resolver response was accepted for any CycloneDX reference type, while the ISPRAS schema demands a vcs or a source-distribution link per component. A website or issue-tracker answer therefore produced a green build and an SBOM that fails validation later, away from the change that caused it. Fail the build on any other kind instead. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Verification
Review focus
Follow-up
|
…ut a digest The PURL resolver reports a STREEBOG (GOST R 34.11-2012) digest of the archive next to a source-distribution url, but the resolve response was decoded into a struct without a hashes field, so the digest was dropped and the external reference entered the SBOM with a url alone. The ISPRAS oss schema requires a leaf component whose only link is a source distribution to carry that digest, so every such component made the published SBOM unvalidatable while the build stayed green. Hashes now travel from the resolver into the external reference, and enrichment rejects a source distribution it could not be validated with: no digest at all, an algorithm other than STREEBOG-256 or STREEBOG-512, or content that is not 64 respectively 128 hexadecimal characters. A vcs reference stays free of hashes, as the schema expects. A source distribution left without a digest by an earlier enrichment — the merged BOM of a base image built before this change — is replaced by the resolved one instead of being kept: that base image cannot be fixed from the downstream build, and keeping its link would carry the unvalidatable reference into this SBOM too. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
Fral738
left a comment
Fral738
left a comment
There was a problem hiding this comment.
Two interactions in the checker image make the newly exposed checks behave differently from their CLI contract:
- Combining
--check-source-distributionwith--check-vcs-leaf-onlysilently skips source archives on non-leaf components. A dead parent archive changes from failure to success just by enabling the VCS modifier. --check-source-distributionalone also enables VCS validation, despite--check-vcs=falseand the log reporting only a source distribution check.
The inline comments contain reproductions and concrete corrections. They were reproduced through the built CLI against checker image digest sha256:d9a95347a28b4bbbd6163b0c0dc5215e0e5b5a60acc2d519a55cc5bca86e6c49.
There is also a description/test claim to correct: "a component carrying only that reference is not an error" is not true of the source-distribution reference produced by the enricher. The ISPRAS schema requires hashes, including a STREEBOG hash, when a leaf has no vcs reference. The new acceptance test only checks the reference type; the resolver outcome supplies no hashes. A minimal otherwise-valid OSS BOM with that reference fails werf sbom validate --ispras-format oss --path <bom> with ERROR: 'hashes' is a required property, exit 1, even without URL checks. Missing hashes predate this PR; please qualify the description and the test's claimed coverage rather than presenting source-only output as proven valid. If schema-valid source-only output is intended in this PR, supply the required hash and test the serialized BOM against the checker.
Verification: task build and the full task test:unit passed. Mutations disabling duplicate suppression, changing the allow-list, and omitting the new Docker arguments were detected by the corresponding tests; restored runs passed. Full image-build e2e/integration suites were not run.
| } | ||
|
|
||
| if opts.CheckVCSLeafOnly { | ||
| args = append(args, "--check-vcs-leaf-only") |
There was a problem hiding this comment.
Forwarding --check-vcs-leaf-only together with --check-source-distribution skips source-distribution URLs on non-leaf components too. The checker executes if args.check_vcs_leaf_only and components_value: continue before collecting either reference type, so this VCS modifier suppresses an independently requested archive check.
For an otherwise-valid OSS BOM with a dead http://127.0.0.1:1/source.tar.gz on a parent component and a child marked GOST:provided_by, werf sbom validate --ispras-format oss --path <bom> --check-source-distribution returns the archive warning and exit 1; adding --check-vcs-leaf-only returns Result: 1 passed, 0 failed, exit 0.
Until the checker image is fixed to apply the leaf predicate only while collecting VCS references, reject this flag combination before running Docker (users can run source validation separately). Add a runtime regression covering a non-leaf source archive; the current argv-only test accepts the combination without testing what it checks.
There was a problem hiding this comment.
Confirmed against sbom-checker.py:178 — the leaf filter is a continue before externalReferences is read. Fixed in b952906: RunOptions.Validate rejects the combination up front (--check-vcs-leaf-only cannot be combined with --check-source-distribution … use --check-vcs instead), called from the CLI flag validation and from checker.Run. Running two passes is not an option because of your second point — the source-distribution pass would still do the full VCS check. Image fix + digest pin tracked in Follow-up.
| } | ||
|
|
||
| if opts.CheckSourceDistribution { | ||
| args = append(args, "--check-source-distribution") |
There was a problem hiding this comment.
--check-source-distribution also activates VCS URL validation in this checker image. Its shared URL-checking block is entered for any of the three flags and collects vcs references unconditionally, so --check-vcs=false does not prevent repository probes and failures. The header nevertheless reports only with source distribution check.
An otherwise-valid OSS BOM containing only vcs: http://127.0.0.1:1/repo.git passes without checks (exit 0). Adding only --check-source-distribution makes the same CLI invocation fail with the VCS warning and exit 1, although there are no source-distribution references to validate.
Fix the checker image so VCS references are collected only when args.check_vcs or args.check_vcs_leaf_only, then use that corrected image and add a runtime regression for the source-only flag. If that image change cannot ship here, require explicit VCS opt-in and document the coupling instead of exposing this as an independent source-only check.
There was a problem hiding this comment.
Confirmed: vcs collection at sbom-checker.py:183 is unconditional inside the check_vcs or check_vcs_leaf_only or check_source_distribution block. Fixed in b952906: the flag help and the command docs state that --check-source-distribution also validates VCS URLs of every component, and enabledChecks now reports what the image really runs — --check-source-distribution alone logs with VCS, source distribution check. Description updated accordingly; the "component carrying only source-distribution is not an error" line is replaced by the hashes claims from e9e9c68 plus an explicit UNVERIFIED for the checker run.
…bution checks The ISPRAS checker image applies --check-vcs-leaf-only before it reads a component's external references, so combined with --check-source-distribution the archives of every non-leaf component went unchecked while the run reported success. Reject the combination up front with an error naming --check-vcs as the way out. The same image also turns on VCS URL validation whenever --check-source-distribution is passed, so a run with that flag alone can fail on an unreachable repository. Say so in the flag help and in the "with ... check" header, which now lists the checks the image really runs instead of the flags that were passed. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
576f9b0 to
b952906
Compare
Fral738
left a comment
Fral738
left a comment
There was a problem hiding this comment.
The previous flag findings are addressed by the early combination guard and the updated help/logging contract. The remaining findings are in the new source-distribution repair path:
- An inherited
hashes: []is treated as an existing digest, so enrichment succeeds without repairing the hash-less reference despite a valid resolver result. - Replacing one component's hash-less reference deletes the same URL/type from the BOM-wide reference set even when another component retains that reference with a valid digest.
Both cases were reproduced with focused Go tests on this head; the inline comments include the scenarios and corrections. The first also qualifies the description's claim that an inherited source distribution without a digest is replaced: it currently only handles a nil hashes pointer.
Verification: task build and the full task test:unit passed. Mutations of hash validation/copying/replacement and checker guard/logging were detected; restored runs passed and the worktree is clean. The built CLI rejects the incompatible flags before file access or container startup. The source-only serialized BOM has not been revalidated against the checker in this pass: the local Docker socket is absent and SSH to the available Linux host timed out. Full image-build e2e/integration suites were not run.
| // rather than kept: the base image it came from cannot be fixed from here, | ||
| // and keeping it would carry the unvalidatable link into this SBOM too. | ||
| if existing, idx, ok := findRefType(*comp.ExternalReferences, outcome.ref.Type); ok { | ||
| if existing.Hashes != nil || outcome.ref.Hashes == nil { |
There was a problem hiding this comment.
An inherited source reference with "hashes": [] has a non-nil Hashes pointer and is kept here, although it contains no digest. The valid replacement returned by the resolver is discarded, leaving a source-only leaf without the required hash while Enrich returns nil.
Changing the existing replaces a source distribution an earlier enrichment left without a digest fixture to Hashes: &[]cdx.Hash{} makes task test:unit paths="./pkg/sbom/externalref/..." -- --focus='replaces a source distribution' fail: the result still has https://example.com/old.tgz rather than the resolved URL. Restoring the nil-hashes fixture passes.
Treat an empty slice as missing too, and cover both nil and empty hashes in the replacement test:
| if existing.Hashes != nil || outcome.ref.Hashes == nil { | |
| if (existing.Hashes != nil && len(*existing.Hashes) > 0) || outcome.ref.Hashes == nil { |
There was a problem hiding this comment.
Confirmed and fixed in 32c896c: hasHashes treats a nil pointer and an empty slice alike. The replacement spec is now a table over hashes absent / hashes: []; the nil-only check fails the empty case. Description claim qualified.
| if existing.Hashes != nil || outcome.ref.Hashes == nil { | ||
| continue | ||
| } | ||
| delete(seen, refKey(existing)) |
There was a problem hiding this comment.
seen is shared by all components, so deleting the old URL/type while repairing one component also removes a reference still used by another component. The other component's keep-existing branch does not reinsert it, and the serialized BOM-wide externalReferences loses that source link.
A focused test with two different PURLs sharing old.tgz reproduces this: component A has a valid STREEBOG digest and keeps old.tgz; component B has no digest and resolves to new.tgz. Enrich succeeds, A still contains old.tgz, but bom.ExternalReferences contains only new.tgz; ContainElement(existing) fails.
Build the aggregate from the final component references after all replacements instead of deleting a shared key during one component's update. Before converting seen to BOM references, rebuild it from the resulting components:
seen = make(map[string]cdx.ExternalReference)
for _, comp := range components {
if comp.ExternalReferences == nil {
continue
}
for _, ref := range *comp.ExternalReferences {
seen[refKey(ref)] = ref
}
}Remove the now-redundant incremental aggregation and add the shared-URL regression with both component orders.
There was a problem hiding this comment.
Confirmed and fixed in 32c896c as you suggest: incremental seen bookkeeping removed, the BOM-wide list is rebuilt from the final component references after the loop. Added keeps a shared source distribution in the BOM list when another component's copy is replaced in both component orders; restoring the delete-on-replace behaviour fails both.
…d keep shared links An inherited source-distribution reference with "hashes": [] passed the "already has a digest" check because only a nil pointer counted as missing, so the valid replacement from the resolver was discarded and the SBOM kept a source-only leaf without the digest the ISPRAS schema requires. Treat an empty hash list as missing too. Replacing one component's hash-less reference also deleted its URL/type from the BOM-wide externalReferences list while another component still carried that link with a valid digest. Derive the BOM-wide list from the final component references after all replacements instead of maintaining it incrementally. Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
2 participants
Summary
An image SBOM could carry two external references of the same type for one component, which the ISPRAS checker reports and werf turns into a validation failure. The enrichment step also accepted any CycloneDX reference type from the PURL resolver and dropped the archive digest the resolver reports for a source distribution, so a green build could produce an SBOM that fails validation afterwards.
werf sbom validatenow reaches the checker's source-distribution and leaf-only VCS checks, and refuses the one combination the checker does not honor.What
Enrichment
vcsorsource-distribution, withenrich: external reference kind "<kind>" is not allowed, expected "vcs" or "source-distribution". Such a kind previously produced an SBOM that failed ISPRAS validation later; a resolver returning e.g.websitenow stops the build instead.source-distributionreference carries the STREEBOG digest the resolver reports inhashes; avcsreference carries none.source-distributionwithout a usable digest — nohashes, an algorithm other thanSTREEBOG-256/STREEBOG-512, or content that is not 64/128 hex characters — withenrich: source distribution has no hashes …or the matching algorithm/content error. Previously such a component entered the SBOM as a leaf with a bare archive URL, which the ISPRAS oss schema rejects.hashes: [{"alg":"STREEBOG-256",…}]forpkg:npm/commondir@1.0.1andhashes: []for vcs results.hashesabsent orhashes: []— is replaced by the freshly resolved one instead of being kept; one that already carries a digest is kept.externalReferenceslist is derived from the final component references: a link replaced on one component stays listed while another component still carries it with a digest.werf sbom validate--check-source-distributionmakes the checker verify that each source-distribution URL exists and points to an archive; default false. It also turns on VCS URL validation of every component, exactly as--check-vcsdoes — that is how the checker image behaves, and the flag help says so.--check-vcs-leaf-onlyrestricts VCS URL checking to leaf components; default false.--check-vcs-leaf-onlytogether with--check-source-distributionis rejected before the checker runs, with--check-vcs-leaf-only cannot be combined with --check-source-distribution: the checker would skip source distributions of non-leaf components; use --check-vcs instead. The checker applies the leaf filter before reading any reference, so the combination silently left non-leaf archives unchecked.--check-source-distributionalone yieldswith VCS, source distribution check;--check-vcs --check-vcs-leaf-onlyyieldswith leaf-only VCS check.--check-source-distributiona dead tarball URL yieldsWARNING: … не указывает на архив или не существуетandResult: 0 passed, 1 failed; without the flag the same file passes that check silently.source-distributionreference with hashes has not been run through the ISPRAS checker image; the schema (component_spec→externalReferenceswith STREEBOGhashes) was read, not executed. A fixture run against the pinned checker image would settle it.Why
Two independent producers write external references — os-pm for system packages and the PURL resolver — and the resolver appended unconditionally, so a component resolved to a kind it already had ended up with two links. Enrichment also runs on the merged BOM of every downstream image, and it runs after
DedupBOM, so each merge appended another copy. The ISPRAS checker warns on a component with two distinct vcs links and werf treats any warning as a failure, which makes the published SBOM unvalidatable.Deduplicating by URL and type would not have been enough: the checker collapses identical links and only reports distinct ones, which is exactly the os-pm plus resolver case.
The enrichment allow-list was the full CycloneDX enum while the ISPRAS schema requires
vcsorsource-distributionper component, so the two disagreed and the disagreement surfaced at validation time rather than at the build that caused it. The same schema requires a leaf whose only link is a source distribution to carry a STREEBOG digest of the archive; the resolver already computes it, but the response struct had no field for it.The two validate flags leak the checker image's internals: its
--check-vcs-leaf-onlyis acontinueahead of any reference parsing, and its VCS collection is not gated by--check-vcs. Fixing the image would be the right long-term answer, but the image is consumed by a floating tag, so a werf-side guard is the only thing that makes the CLI contract hold for whatever image is pulled. Running the checker twice — leaf-only VCS in one pass, source distribution in another — is not an alternative: the second pass would still run the full VCS check.