commands/sbom: emit an SPDX 3.0 SBOM of the installed packages

[hiagofranco]

commands/sbom: emit an SPDX 3.0 SBOM of the installed packages

Generate SPDX 3.0 SBOM json output from the installed RPM packages.

The package list is read from each installed sysroot's RPM database, so
it is the transitive closure the device actually holds rather than the
packages avocado.yaml declares. Generate one document with one root per
scope rather than one document per scope.

Co-Authored-By: Claude Opus 5 noreply@anthropic.com

[yoctopidg3]

This looks good. One thing maybe for the future is a --validate using https://tools.spdx.org/app/validate/

[hiagofranco]

This looks good. One thing maybe for the future is a --validate using https://tools.spdx.org/app/validate/

I played with their API, it works, however it keeps the SBOM json-ld file public for 10 days in their servers. So instead of doing that without the user consent, I just documented this and added as a 'note' output to suggest the user to upload himself, knowing it will be there for 10 days.

[hiagofranco]

Rebased, checks are now passing. Fixed an issues where 'install -f' would fail after the second run trying to generate the sbom output. This is now ready for reviews.

[mobileoverlord]

Two things — one standalone, one an interaction with another open PR.

The document isn't byte-stable

namespace_digest is careful about this and says so:

Derived from the content rather than randomly so the document stays byte-stable: the same installed set must produce the same bytes twice running, or a consumer diffing two SBOMs sees churn that is not there.

But build_document then sets

let created = chrono::Utc::now().format("%Y-%m-%dT%H:%M:%SZ").to_string();

so two runs over an unchanged sysroot still produce different bytes — exactly the churn the comment is guarding against. The namespace work does its job and created undoes it a few lines later.

Honoring SOURCE_DATE_EPOCH when set, falling back to now(), would close it. That also makes the document referenceable by content hash from anything that pins or signs it, which is worth having before consumers start depending on the current behavior.

Interaction with #193

#193 changes the rpmdb seed source from the rootfs to a dependency's sysroot. That breaks the assumption behind base_names here: find(|d| d.scope == "rootfs") stops being the extension's actual base, so a dependency's transactions contain packages the rootfs lacks, fail the all_in_base test, and read as extension content. It compounds up the chain — #193 documents the composition as rootfs ∪ base ∪ mid.

Concretely, #193 measures kiosk-a at 3.5 MB because weston isn't in the image, while this would emit a document saying kiosk-a contains weston — over-reporting by exactly the content #193 deduplicates. The comment here already names that as the direction to avoid: "the opposite error hands every extension the whole base system."

Short version of the fix: have the seed step record its transaction ids at cp -rf time — right after the copy, before anything is installed, every row in the destination db is seed by definition — and drop exactly those here. That replaces base_names and the transaction grouping entirely, and it also closes the two failure modes documented above it (NVRA subtraction dropping 57 of 113 packages; --force replacing the rootfs's ids) plus the acknowledged blind spot where a transaction installs only packages the rootfs already carries by name. All of those are the same root cause: cp -rf of an rpmdb leaves no record of what arrived in the copy, so everything downstream has to infer it.

Whichever of these two lands second should carry the change.

[hiagofranco]

Two things — one standalone, one an interaction with another open PR.

The document isn't byte-stable

namespace_digest is careful about this and says so:

Derived from the content rather than randomly so the document stays byte-stable: the same installed set must produce the same bytes twice running, or a consumer diffing two SBOMs sees churn that is not there.

But build_document then sets

let created = chrono::Utc::now().format("%Y-%m-%dT%H:%M:%SZ").to_string();

so two runs over an unchanged sysroot still produce different bytes — exactly the churn the comment is guarding against. The namespace work does its job and created undoes it a few lines later.

Honoring SOURCE_DATE_EPOCH when set, falling back to now(), would close it. That also makes the document referenceable by content hash from anything that pins or signs it, which is worth having before consumers start depending on the current behavior.

Interaction with #193

#193 changes the rpmdb seed source from the rootfs to a dependency's sysroot. That breaks the assumption behind base_names here: find(|d| d.scope == "rootfs") stops being the extension's actual base, so a dependency's transactions contain packages the rootfs lacks, fail the all_in_base test, and read as extension content. It compounds up the chain — #193 documents the composition as rootfs ∪ base ∪ mid.

Concretely, #193 measures kiosk-a at 3.5 MB because weston isn't in the image, while this would emit a document saying kiosk-a contains weston — over-reporting by exactly the content #193 deduplicates. The comment here already names that as the direction to avoid: "the opposite error hands every extension the whole base system."

Short version of the fix: have the seed step record its transaction ids at cp -rf time — right after the copy, before anything is installed, every row in the destination db is seed by definition — and drop exactly those here. That replaces base_names and the transaction grouping entirely, and it also closes the two failure modes documented above it (NVRA subtraction dropping 57 of 113 packages; --force replacing the rootfs's ids) plus the acknowledged blind spot where a transaction installs only packages the rootfs already carries by name. All of those are the same root cause: cp -rf of an rpmdb leaves no record of what arrived in the copy, so everything downstream has to infer it.

Whichever of these two lands second should carry the change.

Thanks, fixed the bug and left the second one open, as you mentioned, whatever PR is merged first should fix it. Please take a look.

[jetm]

Reviewed at 1faa682. The correctness gate I most wanted to check passes: enumeration
is rpm -qa --root="$root" per sysroot, so this is the full installed closure and not
avocado.yaml's declared deps_map. Reading the declared list would have produced ~15
packages instead of ~400 and reported clean, which is the failure mode that makes an
SBOM worse than none. avocado.yaml is used only to find which extension dirs are
scannable. cargo fmt, clippy -D warnings and the 136 integration tests are clean.

The SPDX 3.0 shapes look right to me: software_Package with software_packageVersion
/ software_packageUrl / software_attributionText, Hash under verifiedUsing,
simplelicensing_LicenseExpression referenced by id, and Relationship with
from/relationshipType/to. Sharing license and supplier elements by id rather than
repeating them per package is the right call, and slug_id's injectivity argument
(generate.rs:262) is the kind of thing that is easy to get wrong and expensive to debug
later. The hasDeclaredLicense relationship id derives from the package id's last
segment, which I checked for collisions: emitted is keyed on
(name, epoch, version, release, arch) so each package is emitted once globally, and
slug_id is injective, so distinct packages cannot share a relationship id.

Requested change: test_output_path_is_optional is flaky, and the mechanism is not quite what it looks like

It fails in roughly two of three full cargo test runs and passes 10/10 when the sbom
tests run alone. tests/integration.rs:5 declares mod interpolation;, so
tests/interpolation.rs and tests/commands/avocado/sbom.rs compile into one binary and
share one process. tests/interpolation.rs sets AVOCADO_TARGET=riscv64-unknown-linux-gnu
process-globally, resolve_target_required (src/utils/target.rs:82) takes
CLI arg then AVOCADO_TARGET then config, so target resolution succeeds and the assertion
on "No target architecture specified" fails. The observed failure names
riscv64-unknown-linux-gnu exactly.

Two details change what the fix has to be, so they are worth stating.

First, interpolation.rs does clean up after itself: it calls
env::remove_var("AVOCADO_TARGET") nine times and every test that sets the variable is
#[serial]. But #[serial] only serialises against other #[serial] tests, and none of
the sbom tests carry it. So the sbom tests run on the parallel pool alongside a serial
test that has the variable live, and they observe the window between set and remove.
That is why this is intermittent rather than deterministic.

Second, tests/common/mod.rs does not filter the variable out, it forwards it. The loop
maps AVOCADO_TARGET to TEST_AVOCADO_TARGET in the child env, and the bash wrapper
above it then does unset AVOCADO_TARGET followed by
export AVOCADO_TARGET="$TEST_AVOCADO_TARGET" when that is non-empty. The comment reads
"so the script can decide whether to use it", and the script's decision is to use it. So
spawning through that mechanism is what carries the leaked value into the CLI, and
"spawn with AVOCADO_TARGET removed" via that path will not do it.

Marking the sbom tests #[serial] is the smallest fix and matches what every other test
touching this variable already does. A run_cli variant that declines to forward it
would also work if you would rather not serialise.

Requested change: the seeded-scope tripwire cannot fire in the case it exists for

The subtraction at generate.rs:598-699 is well argued and I am not asking you to change
it. Grouping by INSTALLTID and treating a transaction as seed exactly when every package
in it is in the rootfs by name and arch survives both failure modes the comment
documents, and the writeup of why NVRA subtraction and per-row tid matching were wrong is
genuinely useful.

The problem is the interaction between two guards. The subtraction runs only
if seeded && !base_names.is_empty() (generate.rs:671), and base_names is built from
the rootfs dump's readable rows. The tripwire that catches a silent subtraction failure
runs only when rootfs.packages.len() > 1 (generate.rs:507).

So when the rootfs scan fails, or when every rootfs row is unreadable because a tag
carried a tab, base_names is empty, no subtraction happens, and every seeded scope
keeps its full copy of the base. That is the direction the comment calls out as the
harmful one, "hands every extension the whole base system". And because the rootfs
contributed zero packages, rootfs.packages.len() > 1 is false and the tripwire that was
written for exactly this is disabled by the same condition. The command does not bail,
because the extensions have packages, so it emits a plausible-looking document.

A failed-scope warning does fire, so this is not literally silent, but it says the rootfs
is missing rather than that every extension's contents are now unsubtracted. Making the
empty-base_names case warn in its own right, or refusing to emit seeded scopes at all
when the rootfs contributed nothing, would close it.

PURL namespace mismatch with the Yocto-side SBOM

This emits pkg:rpm/avocado/{name}@{version}-{release}?arch={arch} (generate.rs:98). The
Yocto build's SPDX emits pkg:yocto/{layer}/{recipe}@{version}. Different purl type and
different granularity, binary package against recipe, so a correlation service fed both
cannot join them on the purl.

You already capture SOURCERPM and derive the recipe from it (generate.rs:119), which is
the bridge. My only suggestion is that it currently lands in
software_attributionText as the free-text string "recipe: glibc", so joining on it
means parsing prose. SPDX 3.0's ExternalIdentifier with
externalIdentifierType: packageUrl is repeatable, so the document could carry the
yocto-form purl alongside the rpm one and make the join structural. Worth at least
naming the mapping explicitly somewhere a consumer will find it.

Not a finding: the inventory/assessment boundary is drawn in the right place

The document carries no CPE identifiers and no VEX assessments, and that is correct and
deliberate rather than a gap. This command emits inventory; assessment is a separate
surface, tracked in ENG-2325 and ENG-2369. Emitting CPEs here would actively invite the
confusion it might look like it prevents, because a document carrying vulnerability
identifiers reads as though it carried an assessment.

test_the_command_offers_no_way_to_upload_the_document pins the neighbouring concern,
and pins it harder than I would have thought to ask for: --validate,
--validator-url and --upload are all asserted absent from help, on the stated
grounds that the service running the reference SPDX tools keeps uploads for about ten
days and serves them back unauthenticated, while this document names every package and
version on the target. That is the right instinct and worth saying so.

One point sits outside this PR rather than in it: a clean avocado sbom output must
never be read as a clean vulnerability assessment. That belongs in the M5 docs
limitation note, not in this diff, so it is not a request here.

Nothing above blocks the shape of the work, which is solid. The two requested changes are
the flaky test and the tripwire gap.

[hiagofranco]

1 and 2 are fixed, "Flaky test_output_path_is_optional" and "Tripwire gap".

3 was not done, since adding ExternalIdentifier with yocto-form purl means we will guess the layer name, which SOURCERPM does not carry (pkg:yocto/{layer}/{recipe} needs the layer). Emitting a purl with a wrong or invented namespace is worse than the current text recipe: glibc.

[jetm]

Re-reviewed the increment since 1faa682 — 2 blocking findings inline, both mutation-proven rather than argued.

The refactor itself is right: is_seeded_scope is behaviour-identical at both call sites, the base_count > 1 re-gate is a pure hoist of a loop-invariant term with no lost coverage across the old-vs-new truth table, and the SPDX document is provably unaffected since both branches only eprintln! and warnings go to stderr. The #[serial] fix addresses a real leak — interpolation.rs is mod-included into the integration binary and all four of its AVOCADO_TARGET setters are already serial. cargo fmt, clippy -D warnings, cargo test --lib sbom (34) and the sbom integration tests (10) all pass at 48c3cd7.

Five advisory notes withheld to keep the blocking ones readable — happy to post them if useful.

[jetm]

Re-reviewed the amend since 48c3cd7 (same base, so this is the 581-line increment only, not the whole PR). Three blocking findings inline, all on the stdout/stderr routing change rather than the SBOM generation itself.

Five advisory notes were withheld rather than appended here, so the blocking ones stay readable.

[jetm]

Re-reviewed the amend since 4569921, checking my three earlier findings rather than re-reading the whole change.

Two are closed. The stdout-on-auto-start one is genuinely fixed - I re-enumerated the emitters rather than trusting my old line numbers, and there is no println!, print! or stdout handle left anywhere under src/utils/vm. The print_notice_above bypass is fixed too, so the renderer-collision risk is gone.

The third is partial: the false "cannot interleave" claim is gone and the new prose is accurate, but the mitigation it promises never fires. That plus one gap in the new guard are inline, two findings.

The new test is not vacuous - reverting lifecycle.rs turns it red and it names all five sites. Six advisory notes were withheld rather than appended here.

[hiagofranco]

Both applied, please take a look.

[jetm]

Re-reviewed the increment since be54d62.

The needle-set fix is confirmed fixed, and confirmed the way it was found: injecting print_warning_above("x"), print!("y") and a raw stdout().write_all(b"z") separately into a scratch copy of state.rs turns the guard red on each, naming the needle it matched. A writeln!(stdout().lock(), ...) injection goes red too, and eprint!/eprintln! both stay green - so the blanking order does not false-positive, which was the trap I flagged when raising it. The earlier round's fixes have not regressed: no println!, print! or stdout handle remains reachable under src/utils/vm/.

The json-guard finding is partial. The dead is_json_output_active() branch and the false NDJSON-event claim are both gone, and the new predicate genuinely is true pre-dispatch. But the replacement introduces a new wrong call-graph claim in the same doc block, and the argv scan itself gives two demonstrably wrong answers. Three findings inline; all three are one-file fixes and only the argv one changes shipped behaviour.

Seven advisory notes were withheld rather than appended here. One is worth naming since it sits directly on this design: NEEDLES does not cover emit_json_event/emit_json_object, which write via stdout().lock() and match none of the seven spellings - and those are precisely the alternative output.rs:158-164 points a future author toward.

[hiagofranco]

All three applied, please take a look

[jetm]

Re-reviewed the increment since f7ceaab. One of the four is fully closed; three are partial, and four findings are inline.

Closed: the guard now catches emit_json_event and emit_json_object. Demonstrated by injecting both into route.rs in a scratch clone and watching the test go red naming each line, not by reading the needle list.

Partial, and worth reading in this order:

• The argv scan answers both inputs I gave last round correctly now, plus every extra spelling I probed - attached --output=json, mixed last-wins orderings, a repeated --, a valueless trailing --output, and -- as the flag's own value. What remains is the same class the original finding named: trailing_var_arg does not require a -- at all.
• The dropped-notice fix works for vm update and only vm update. lifecycle::start has four callers; the pre-dispatch one installs no guard, so every needs_vm_routing command still drops all five notices - which is exactly the failure this increment's own comment describes.
• The doc block is on its third consecutive corrected-and-still-wrong call-graph claim.

The severity bug is new in this increment rather than pre-existing, so it is worth catching before it ships.

Nine advisory notes were withheld. One is a project-rule matter rather than a preference and I will name it here: neither behaviour change has a test. Nothing calls print_warning_stderr/print_info_stderr/print_stderr_notice from the suite and nothing constructs UpdateCommand, so deleting the is_json_output_active check - or update.rs:64-67 outright - restores the original defect with the suite green.

Build is clean at 857aa52: 1445 + 1454 unit tests, all integration suites, clippy silent, fmt clean.

[jetm]

Re-reviewed the increment since 857aa52. One of the four is fixed; three findings inline.

Fixed: the severity bug is gone, by elimination rather than by carrying severity - print_stderr_notice lost its emit branch entirely, so an INFO can no longer surface as event: "warning". Both severities still format and print correctly on the human path.

The argv fix works for the cases I named and introduces a new one. Both forms from last round now return false, and all four declared pass-throughs behave. But PASS_THROUGH is matched against every argv token rather than a subcommand slot, so avocado install dnf --output json - which clap resolves to Install(packages=["dnf"], output=Json) - now reads as human output. That is a regression against 857aa52, where the same line was correctly suppressed. Measured both ways in one run, scan result against Cli::try_parse_from.

The dropped-notice finding is not fixed, and the increment now documents that as the intent. avocado vm update --output json with a failed supervisor bind still reports a clean update over a VM whose SSH proxy is down; the scenario reproduces unchanged. I have anchored the disagreement to the doc rather than re-filing the bug, because the trade the comment describes was priced against a caller that could not be hurt - sbom never reaches a notice emitter - and paid by the only caller that was.

Worth saying plainly, since it is now the pattern rather than the incident: this is the fourth consecutive round in which a doc claim about the call graph is one the code does not support. The code has improved every round. The prose is the part that keeps failing review, and it is failing in a way that matters - two of the three findings here are arguments the comments make, not defects in the logic.

Four advisory notes were withheld. Two more things I could not anchor: vm update's bare println! at update.rs:152/223/260/280 already corrupts its own --output json stdout and the new guard test covers only src/utils/vm, not src/commands/vm - both pre-existing and untouched here; and the CHANGELOG carries no entry for the notice-routing behaviour change at all.

Head is clean: cargo fmt --check, clippy --all-targets --all-features -D warnings, and the full suite all pass at 165351b.

[hiagofranco]

Hi, pushed all three fixes, please take a look