fix: validate rootfs diff-ids before use to prevent SBOM panic on digest-less images

[matthyx]

Overview

Node-agent crashed with panic: no ':' separator in digest "" when the SBOM manager processed a container image without a resolvable digest (e.g. an image referenced only by tag, such as :latest, which CRI-O/OpenShift can report with an empty layer diff-id). The empty digest string reached go-digest's Digest.Algorithm()/Digest.Hex(), which panic on malformed input instead of returning an error, taking down the whole node-agent process.

This adds a validateDiffIDs helper (using the digest library's own non-panicking Validate() method) and calls it in NewSource before any diff-id is touched. Images with an invalid/empty diff-id now surface as a reported SBOM-generation failure through the existing reportFailure error path instead of crashing the process — the agent keeps running.

Additional Information

This change is deliberately scoped to pkg/sbommanager/v1/syftutil/source.go + its test file only — no changes to sbom_manager.go. During root-cause investigation a separate, pre-existing defect was found (SBOM-generation-failure objects aren't marked terminal and get silently reprocessed/re-logged on every container restart); that is out of scope here and is addressed in a follow-up PR.

How to Test

go test ./pkg/sbommanager/v1/syftutil/...
go build ./...

Test_NewSource_InvalidDiffID reproduces the exact crash input (diff_ids: [""]) and asserts no panic + a wrapped error.

Related issues/PRs

• Resolved Node-Agent crash: panic no ':' separator in digest #853

Checklist before requesting a review

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• New and existing unit tests pass locally with my changes

Summary by CodeRabbit

• Bug Fixes
  • Improved validation of image filesystem layer identifiers.
  • Invalid image metadata now returns a clear error instead of risking a crash.
  • Added detailed error reporting to identify the problematic layer identifier.

[coderabbitai]

Warning

Review limit reached

@matthyx, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 33 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: aa8f643a-ac1f-498a-ada4-e8984bc28b68

📥 Commits

Reviewing files that changed from the base of the PR and between 02238bc and 1b15b32.

📒 Files selected for processing (2)

• pkg/sbommanager/v1/syftutil/source.go
• pkg/sbommanager/v1/syftutil/source_test.go

📝 Walkthrough

Walkthrough

NewSource now validates root filesystem diff-IDs before constructing image layers. Invalid digests return an invalid image diff-ids error, and tests cover validation behavior and invalid CRI image metadata without panics.

Changes

Image diff-ID validation

Layer / File(s)	Summary
Validate diff-IDs before layer construction pkg/sbommanager/v1/syftutil/source.go, pkg/sbommanager/v1/syftutil/source_test.go	NewSource validates each root filesystem diff-ID before building layers, reports invalid indexes and digest values, and tests valid, invalid, empty, and nil inputs plus invalid CRI image metadata handling.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes validating diff-IDs to prevent SBOM panics on digest-less images.
Linked Issues check	✅ Passed	The change addresses #853 by rejecting invalid diff-IDs through an error path instead of panicking, and the tests cover the crash case.
Out of Scope Changes check	✅ Passed	The PR stays within scope, touching only SBOM source validation and its tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-853-digest-panic

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Performance Benchmark Results

Node-Agent Resource Usage

Metric	BEFORE	AFTER	Delta
Avg CPU (cores)	0.221	0.000	-100.0%
Peak CPU (cores)	0.230	0.000	-100.0%
Avg Memory (MiB)	317.647	0.000	-100.0%
Peak Memory (MiB)	320.047	0.000	-100.0%

Dedup Effectiveness

No data available.

[matthyx]

Review: no blockers

Verified locally on fix/issue-853-digest-panic (go1.25.8):

• go build ./pkg/sbommanager/... ./pkg/sbomscanner/... — clean.
• go test ./pkg/sbommanager/v1/syftutil/... — pass.
• The regression test is real. I removed just the validateDiffIDs call from NewSource (keeping the helper so it still compiles) and re-ran Test_NewSource_InvalidDiffID: it fails with the exact panic: no ':' separator in digest "" from Node-Agent crash: panic no ':' separator in digest #853, caught by assert.NotPanics. With the call restored it passes.
• The fix covers every panic site in the repo. d.Algorithm() / d.Hex() at pkg/sbommanager/v1/syftutil/source.go:148-149 are the only two panicking go-digest calls in pkg/ (no other .Hex(), .Algorithm(), .Encoded(), or digest.Parse outside tests), and both are reachable only through toRootFS → toDiffIDs, which now runs after validation.
• Both callers handle the new error path. pkg/sbommanager/v1/sbom_manager.go:458 routes it to reportFailure(..., ReasonSBOMGenerationFailed, ...); pkg/sbomscanner/v1/server.go:57 returns codes.Internal. Neither swallows it, neither retries in a tight loop.

Scope is tight and the Validate()-instead-of-manual-parsing choice is the right one. Good to merge.

Non-blocking notes

1. imageStatus.Image is still dereferenced without a nil check (source.go:79,82,87). CRI's ImageStatus is specified to return a response with a nil Image rather than an error when the image isn't in the store — the same digest-less/:latest situation that produced Node-Agent crash: panic no ':' separator in digest #853. In practice the Info["info"] unmarshal fails first (nil map → "" → "unexpected end of JSON input"), so it's shadowed today, but that's incidental. A if imageStatus == nil || imageStatus.Image == nil { return nil, fmt.Errorf(...) } at the top of NewSource would close the same crash class for one line.

2. Validate() is slightly stricter than panic-avoidance requires. It returns ErrDigestUnsupported for a well-formed digest whose algorithm isn't registered in go-digest (i.e. anything outside sha256/sha384/sha512). Those inputs would not have panicked — Algorithm()/Hex() handle them fine — so such images now fail SBOM generation where they previously succeeded. Almost certainly irrelevant in practice, and I'd still keep Validate(); just noting the behavior isn't purely additive.

3. The aliasing on the lines you're now sitting on top of. reverseLayers := imageInfo.ImageSpec.RootFS.DiffIDs; slices.Reverse(reverseLayers) (source.go:48-49) reverses in place, so the struct field itself is mutated — and toRootFS (line 58, feeding rawConfig) and toLayers (line 68) both read it after the reversal. toLayers's reverse mount indexing (ms[msLen-i-1]) means the size pairing works out, but the RootFS.DiffIDs written into RawConfig end up in the opposite order from the OCI base→top convention. Pre-existing and out of scope for this PR — flagging it because it's easy to miss and the new validation call is now the line above it. A slices.Clone before reversing would make the intent explicit either way.

4. Merge-order thought on the follow-up you mention. This PR turns a crash into a reported failure, which is strictly better — but for the exact OpenShift :latest fleet in Node-Agent crash: panic no ':' separator in digest #853 it means every one of those containers now re-enters SBOM generation and re-reports a failure on every restart, since the failure objects aren't marked terminal. Worth landing the follow-up close behind this one so the reporter doesn't trade a crashloop for a log/report loop.

[github-actions]

Performance Benchmark Results

Node-Agent Resource Usage

Metric	BEFORE	AFTER	Delta
Avg CPU (cores)	0.162	0.160	-1.4%
Peak CPU (cores)	0.166	0.169	+1.4%
Avg Memory (MiB)	334.911	276.527	-17.4%
Peak Memory (MiB)	337.988	282.527	-16.4%

Dedup Effectiveness

No data available.