T8542: refactor SBOM generation - run after ISO build - #1198
Conversation
|
❌ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🧰 Additional context used🔍 Remote MCP Context7Additional factual context relevant to this PR
Sources:
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughMoves SBOM generation from a VM transfer-disk workflow to host-side syft usage: removes Makefile ChangesSBOM generation migration from VM to host-side
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker/Dockerfile`:
- Around line 351-354: The Dockerfile currently downloads
syft_1.44.0_linux_$(dpkg-architecture -qDEB_HOST_ARCH).tar.gz to /tmp as
syft.tar.gz and extracts the syft binary to /usr/local/bin/syft without
integrity checks; update the RUN step to also download the matching release
checksum (or signature) artifact for that same ${arch} (e.g.,
syft_1.44.0_SHA256SUMS or a .sig), verify syft.tar.gz against the checksum (or
verify the checksum file signature with gpg) and fail the build if verification
fails, then proceed to tar --extract and mv syft /usr/local/bin/ only after
successful verification so the symbols syft.tar.gz,
syft_1.44.0_linux_$(dpkg-architecture -qDEB_HOST_ARCH).tar.gz, and
/usr/local/bin/syft are protected by integrity checks.
In `@scripts/image-build/build-vyos-image`:
- Around line 729-741: The SBOM is being generated from the current BUILD_DIR
contents because the code calls syft with '.' (variable cmd uses 'syft', '.')
after switching into defaults.BUILD_DIR; change this to mount the iso_file to a
temporary mount directory and run syft against that mountpoint instead of '.'.
Concretely: create a temp dir, loop-mount iso_file (the variable iso_file
referenced earlier) into it, update the cmd array used for subprocess.Popen
(currently contains 'syft' and '.') to use the mount dir path, run the same
stdout/stderr handling, then unmount and remove the temp dir in a
finally/cleanup block to ensure proper cleanup and error handling. Ensure
subprocess.CalledProcessError is still raised on non-zero rc.
- Around line 729-741: The function-scope variable name cmd is being shadowed by
later assignment, causing UnboundLocalError for earlier uses like cmd("lb
clean"), cmd(lb_config_command) and cmd("lb build 2>&1"); rename the local list
to a unique identifier (e.g., syft_cmd or sbom_cmd) inside the SBOM creation
block where cmd = ['syft', ...] is defined (and update the subprocess.Popen call
to use that new name) so the existing cmd() helper calls remain referring to the
intended function; ensure only the list and its Popen invocation are changed
(keep subprocess.PIPE, stderr, text, bufsize and error handling the same).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b7b22d6b-7883-4dde-b617-64402574cfd0
📒 Files selected for processing (4)
Makefiledocker/Dockerfilescripts/check-qemu-installscripts/image-build/build-vyos-image
💤 Files with no reviewable changes (1)
- Makefile
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_iso
- GitHub Check: codeql-analysis-call / Analyze (python)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
🔍 Remote MCP Context7
The specific task T8542 details are not publicly accessible. Let me search for more information about the architectural implications and potential concerns with this refactoring:Based on my research, I have gathered sufficient context to provide a comprehensive review summary. Let me compile the findings:
Summary of Additional Context Found
Tool Information: Syft
Syft version 1.44.0 was released on May 1, 2026, making it very recent and likely stable. Syft is a CLI tool for generating SBOMs for container images, filesystems, and archives that supports dozens of packaging ecosystems and multiple output formats (CycloneDX, SPDX, Syft JSON, and more) [::Context7_resolve-library-id::] [::Context7_query-docs::].
The PR's implementation aligns with standard syft usage patterns. Syft supports multiple simultaneous SBOM outputs using the format -o spdx-json=./spdx.json -o cyclonedx-json=./cdx.json [::Context7_query-docs::], which matches the approach taken in scripts/image-build/build-vyos-image where the code invokes syft to generate both CycloneDX JSON and SPDX JSON formats.
Architectural Refactoring Implications
Key Change: Moving SBOM generation from inside QEMU (guest VM) to the build host post-ISO assembly represents a significant architectural shift:
-
Guest-to-Host Migration: The original approach created a "transfer disk" within QEMU to extract SBOMs from the guest environment. This PR removes that complexity by generating SBOMs from the final ISO directly on the build host.
-
Simplified Build Process: Syft runs entirely offline with no cloud dependencies, making it safe to run during the Docker build stage without external network calls.
-
Error Handling Change: The script now exits successfully (
sys.exit(0)) even if exceptions were recorded, which differs from the previous behavior. This is a notable behavioral change that should be validated against the PR requirements.
Review Considerations
Critical Points for Review:
-
Version Pinning: The Dockerfile pins syft to version 1.44.0 from
cdn.vyos.io. This provides reproducibility but should be verified for:- Architecture compatibility across target platforms
- Security/vulnerability status of this version
-
Error Handling: The change from
sys.exit(1)on exception tosys.exit(0)could mask build failures. Verify this aligns with T8542 requirements and doesn't suppress legitimate error conditions. -
No Functional Changes Claim: The PR is marked as a "refactoring (no functional changes)," but the exit behavior change is functional. This should be clarified or justified.
-
Integration Points: The removal of the transfer-disk mechanism and related QEMU parameters represents a significant reduction in code (-137 lines) and eliminates a potential point of failure in the VM-based build process. [web_search::]
🔇 Additional comments (2)
scripts/check-qemu-install (1)
193-193: LGTM!Also applies to: 650-650, 1307-1307
scripts/image-build/build-vyos-image (1)
3-3: LGTM!Also applies to: 33-33, 724-727
| # Install syft binary required for SBOM generation | ||
| RUN cd /tmp && curl -sSfL -o syft.tar.gz \ | ||
| https://cdn.vyos.io/tools/syft_1.44.0_linux_$(dpkg-architecture -qDEB_HOST_ARCH).tar.gz; \ | ||
| tar --extract --file=syft.tar.gz syft; mv syft /usr/local/bin/ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the relevant section around the provided line numbers.
nl -ba docker/Dockerfile | sed -n '320,390p'Repository: vyos/vyos-build
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "syft|SBOM|cdn\.vyos\.io/tools" docker/DockerfileRepository: vyos/vyos-build
Length of output: 335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for checksum or signature verification patterns in the Dockerfile
rg -n "(sha256sum|sha512sum|gpg|cosign|verify|checksums|SIGNATURE|sum -c|-c -)" docker/DockerfileRepository: vyos/vyos-build
Length of output: 41
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "syft[_-]|/usr/local/bin/syft|SBOM generation" -S .Repository: vyos/vyos-build
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "SYFT_VERSION" docker/DockerfileRepository: vyos/vyos-build
Length of output: 41
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '330,370p' docker/DockerfileRepository: vyos/vyos-build
Length of output: 1667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "(sha256sum|sha512sum|sha1sum|gpg|gpgv|cosign|sigstore|verify|gpg --|--check|checksums)" docker/Dockerfile || trueRepository: vyos/vyos-build
Length of output: 41
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "curl .*tools/|cdn\.vyos\.io/tools/|tar --extract" docker/DockerfileRepository: vyos/vyos-build
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '330,370p' docker/DockerfileRepository: vyos/vyos-build
Length of output: 1667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "(sha256sum|sha512sum|sha1sum|gpg|gpgv|cosign|sigstore|verify|gpg --|--check|checksums)" docker/Dockerfile || trueRepository: vyos/vyos-build
Length of output: 41
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "curl .*tools/|cdn\.vyos\.io/tools/|tar --extract" docker/DockerfileRepository: vyos/vyos-build
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '330,370p' docker/DockerfileRepository: vyos/vyos-build
Length of output: 1667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "(sha256sum|sha512sum|sha1sum|gpg|gpgv|cosign|sigstore|verify|gpg --|--check|checksums)" docker/Dockerfile || trueRepository: vyos/vyos-build
Length of output: 41
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "curl .*tools/|cdn\.vyos\.io/tools/|tar --extract" docker/DockerfileRepository: vyos/vyos-build
Length of output: 233
Add integrity verification for downloaded syft (docker/Dockerfile 351-354).
The Dockerfile downloads the syft_1.44.0_linux_${arch}.tar.gz tarball and immediately extracts/moves the syft binary without checksum/signature verification; add verification using syft’s official release checksum (or signature) artifacts for the matching ${arch} before extracting/installing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker/Dockerfile` around lines 351 - 354, The Dockerfile currently downloads
syft_1.44.0_linux_$(dpkg-architecture -qDEB_HOST_ARCH).tar.gz to /tmp as
syft.tar.gz and extracts the syft binary to /usr/local/bin/syft without
integrity checks; update the RUN step to also download the matching release
checksum (or signature) artifact for that same ${arch} (e.g.,
syft_1.44.0_SHA256SUMS or a .sig), verify syft.tar.gz against the checksum (or
verify the checksum file signature with gpg) and fail the build if verification
fails, then proceed to tar --extract and mv syft /usr/local/bin/ only after
successful verification so the symbols syft.tar.gz,
syft_1.44.0_linux_$(dpkg-architecture -qDEB_HOST_ARCH).tar.gz, and
/usr/local/bin/syft are protected by integrity checks.
asklymenko
left a comment
asklymenko
left a comment
There was a problem hiding this comment.
Perform SBOM scan during the build process.
sarthurdev
left a comment
sarthurdev
left a comment
There was a problem hiding this comment.
Refactors SBOM generation. Not tested locally, trust Christian's testing.
|
Actionable comments posted: 0 |
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 invalid-title label must be absent to mergeWaiting for
This rule is failing.Block merge while the invalid-title label is present. The label is auto-toggled by the
|
|
CI integration 👍 passed! Details
|
vyos-bot
Bot
added
mirror-initiated
4 participants
Change summary
Revert current SBOM generation commits and replace them with a simplified approach where the SBOM is directly generated after the VyOS ISO assembly step. This saves a lot of code, time (no need to spawn a QEMU VM), and also code which must be maintained.
Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
Just buiid a VyOS ISO image
Checklist: