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

[jetm]

Re-reviewed after the rebase onto 1f88d1b. No code changed - I diffed 165351b against c17a16b restricted to the files this commit owns and it is empty, and git range-diff across the two bases shows only CHANGELOG context pulled in from #192 and #199. So this is a clean rebase, not a revision.

I am re-posting the three findings from the previous round because the rebase orphaned every inline comment on this PR. All six of mine now report outdated: true with a null line, including the three posted hours before the push. That happens because the commit each was bound to is no longer in the PR's history - even though the lines themselves are byte-identical.

Two consequences worth knowing, since neither is visible from the PR page:

• The comments are collapsed as outdated, so the findings read as addressed when nothing about them changed.
• open_threads drops to zero, so the merge gate stops counting them. Nothing now blocks on three unfixed blocking findings.

These are not new findings. They are the same three, re-anchored to the current head so they are countable again: the PASS_THROUGH token match that regressed avocado install dnf --output json, the unpinned suppression policy that survives two mutations with the suite green, and the sbom splice the doc claims as past-tense fact that the call graph does not permit.

Nothing else to review here - the code is unchanged since my last pass.

[jetm]

Correction to my review a few minutes ago (4961542468): its central claim is false and I have retracted all three findings it re-posted.

I said the push from 165351b to c17a16b was "a clean rebase, not a revision" and that no code changed. It changed by 111 insertions and 31 deletions across output.rs and output_format.rs, and those changes address all three findings from my previous round:

• PASS_THROUGH became (parent, child) pairs, which fixes the avocado install dnf --output json regression.
• print_stderr_notice returns Option<&str> so the decision is observable from a test, which was the gap behind the unpinned-policy finding.
• The doc's sbom claim was corrected, and more precisely than I asked: prepended rather than spliced mid-document, and Docker-Desktop-only.

What went wrong on my end, since it is worth knowing if you see me do it again: I built the file list with a command substitution and passed it unquoted as a git pathspec. It did not word-split in that shell, so git received one multi-line string, matched nothing, and exited 0 with empty output. I read empty-and-successful as "no changes" instead of checking that the pathspec had expanded. The rebase onto 1f88d1b made the mistake plausible - a plain two-dot diff really does show ~3500 lines of other people's merged work - which is why I did not question the empty result.

Sorry for the noise on your threads. The three findings are addressed as far as I can tell; I will re-review the actual increment properly rather than trusting this pass.

[jetm]

Re-reviewed the increment properly this time. My previous review on this head claimed no code had changed; that was wrong and I retracted it in #198 (comment). This one supersedes it.

Scope I actually read: 165351b3 (my last real review) to c17a16b8, restricted to what this PR owns. The set difference between the two heads and the three main commits the rebase pulled in leaves exactly src/utils/output.rs, src/utils/output_format.rs and this PR's CHANGELOG hunk - 154 insertions, 50 deletions.

All three findings are fixed, and both new tests hold up under mutation. Swapping the print_stderr_notice callsite to is_json_output_active() fails an_environment_notice_reads_argv_not_the_process_flag with exactly left: None, right: Some("[WARNING] x"), which is what its comment predicts. Reverting PASS_THROUGH to bare-token matching fails a_pass_through_name_used_as_a_value_is_not_a_pass_through on the install dnf --output json case. The four pairs match the four trailing_var_arg sites one for one (main.rs:1519 sdk run, 1578 sdk dnf, 1787 runtime dnf, 4449 ext dnf), and holding --output's consumed value out of previous is right. The CHANGELOG move fixes a real problem: ### Changed and ### Breaking had been splitting the ### Added list, and Unreleased now reads Added / Changed / Breaking / Fixed in order.

Locally at c17a16b8: cargo fmt --check clean, cargo clippy --all-targets -- -D warnings clean, full suite green (1502 lib + 1511 bin + integration). Repo defines no required contexts, so that is behavioral evidence rather than a green required check.

Two things worth a look, one of them small.

The doc comment on print_stderr_notice now says SbomCommand::execute calls nothing in this module. It calls four - src/commands/sbom/generate.rs:21 imports print_info and print_success from crate::utils::output, and print_summary reaches them at 1115, 1126, 1133 and 1148 from execute's line 613. The conclusion still holds and the very next clause is what carries it: the record originates in ensure_routed_for_process, which runs before dispatch, so it lands ahead of the document rather than inside it. That timing argument is sufficient on its own, and the "calls nothing in this module" clause is both wrong and redundant next to it. Worth noting because the thing keeping sbom's own prose off stdout is narrower than "it never calls in here" - it is the output_path.is_some() arm at 613 plus tui_is_active().

The bigger one is the tradeoff itself. The note names vm update as the caller that lost most, on the grounds that it is a stream and could have carried the record. avocado vm update --output json does not currently have a clean stream to protect: src/commands/vm/update.rs has four ungated println! on the apply path (152, 223, 260, 280), json_mode at 167 is computed after the first of them and only gates the progress bars, and vm update never enables a JsonOutputGuard, so tui_is_active() is false and nothing else suppresses them. Line 152 in particular emits a bare prose line and returns, so a --output json consumer gets no JSON at all. --check is properly gated; it is the apply path that is not. So the record looks dropped to protect a property the named beneficiary does not have. tests/no_stdout_on_the_vm_path.rs reasons that every emitter reachable from the path has to be stderr and not just the outermost one, which is the right argument - it just stops at src/utils/vm, and the same failure is live one directory over. Not asking for update.rs in this PR; it does change whether blanket silence is the right call, and pointing that guard at src/commands/vm too would catch it.

Small one: the PASS_THROUGH bounds paragraph reads as exhaustive about what the pair cannot separate, and there is a second direction. --runs-on, --nfs-port and --sdk-arch are global = true (main.rs:102-111), so a value of one sitting between the parent and the child makes previous the value rather than the parent - avocado sdk --runs-on user@host dnf install foo misses ("sdk", "dnf") and the cutoff never fires, after which the wrapped tool's args get scanned. It fails in the safe direction and it is the same class as the --dnf-arg --output json bound already documented above it, so a sentence in that paragraph is all it needs.

[hiagofranco]

Pushed both fixes, pointing no_stdout_on_the_vm_path.rs at src/commands/vm was rejected. That guard's rule is "never stdout", justified by src/utils/vm running before dispatch.

[jetm]

Confirmed. 2fb97ee corrects all three - comment-only, no functional lines changed, so nothing to re-test.

The SbomCommand::execute line now says accurately what happens: print_summary does call print_info/print_success, on the output_path.is_some() arm specifically, and that arm is why the record still can't land in the document. The vm update framing no longer claims a clean beneficiary - it names the ungated println! still on the apply path, so the tradeoff is stated honestly rather than optimistically. And the PASS_THROUGH bounds note now covers the global-flag-value direction (--runs-on sitting between parent and child), landing safe the same way the existing bound does.

No other threads open.

[jetm]

Approving. All three prior findings are correctly addressed at 2fb97ee, verified directly against the diff, and the change is comment-only - no functional lines moved, so nothing new needs testing. Repo defines no required contexts; the optional Test Suite, both build targets, Security Audit, and Windows compile are all green on this head.