Use OSV for ubuntu vulnerability scanning#42063
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #42063 +/- ##
==========================================
- Coverage 66.89% 66.83% -0.07%
==========================================
Files 2573 2560 -13
Lines 206426 206318 -108
Branches 9281 8983 -298
==========================================
- Hits 138088 137890 -198
- Misses 55784 55856 +72
- Partials 12554 12572 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds end-to-end OSV-based Ubuntu vulnerability scanning: new config flag Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/fleet/cron.go`:
- Around line 199-205: The code currently calls checkOvalVulnerabilities
unconditionally and then, when config.OSVForVulnerabilities is true, calls
checkOSVVulnerabilities and appends its results, which can cause OVAL to be
skipped for OSV-backed platforms even if OSV fails; modify the flow so that when
config.OSVForVulnerabilities is true you first call checkOSVVulnerabilities and
check its success (e.g., return value or error indicator from
checkOSVVulnerabilities), and only suppress OVAL processing for OSV-backed
platforms if checkOSVVulnerabilities succeeded; if checkOSVVulnerabilities
fails, fall back to calling checkOvalVulnerabilities for those platforms (or
keep its original results) so Ubuntu/stale platforms are processed by OVAL; use
the existing symbols checkOSVVulnerabilities, checkOvalVulnerabilities,
config.OSVForVulnerabilities, vulnAutomationEnabled and ovalVulns to implement
the gating and fallback.
In `@server/vulnerabilities/osv/analyzer.go`:
- Around line 117-129: The code in loadOSVArtifact recomputes the dated filename
with time.Now(), causing mismatch if sync used a different run time; change
loadOSVArtifact to accept the run date (e.g., a time.Time runDate) or the
already-resolved artifact filename and use that when calling osvFilename instead
of time.Now(), and ensure the caller (e.g., the Refresh/Sync orchestration that
downloads artifacts) passes the same runDate/filename so both sync and analysis
use a single consistent run timestamp; update references to loadOSVArtifact,
osvFilename, and callers accordingly.
- Around line 67-71: The current call to ds.HostIDsByOSVersion(ctx, ver, 0,
10000) only fetches the first 10k hosts and drops the rest; replace this single
call with a paginated loop that repeatedly calls ds.HostIDsByOSVersion with
increasing offsets (or using a next-token style if supported) until no more IDs
are returned, appending results into the hostIDs slice (or a new accumulator)
and returning any errors from each call; ensure you use the same ctx and ver and
handle the case where large result sets require multiple requests so that
downstream logic (the variable hostIDs) receives the complete set.
- Around line 195-205: The function extractUbuntuVersion trims suffix after
trimming spaces which fails for inputs like "24.04 LTS "; change the order to
TrimSpace first, then TrimSuffix(" LTS"), and TrimSpace again before splitting
so inputs with trailing/leading whitespace are normalized; update
extractUbuntuVersion accordingly (keep the existing split logic returning
parts[0]+parts[1]).
In `@server/vulnerabilities/osv/downloader.go`:
- Around line 120-134: The current download block writes directly to dstPath and
risks leaving a partial file if rc/io.Copy fails; instead create a temp file in
the same directory (e.g., using os.CreateTemp or ioutil.TempFile), copy rc into
that temp file (using io.Copy), close and sync the temp file, then atomically
rename the temp to dstPath via os.Rename; update the code around rc, outFile and
io.Copy in downloader.go to create/close/sync the temp and only rename on
successful copy, and ensure temp is removed on error.
In `@server/vulnerabilities/osv/sync.go`:
- Around line 35-52: The early return when len(toDownload) == 0 skips
removeOldOSVArtifacts so stale osv-ubuntu-* files are never pruned; update the
flow in the function that calls whatToDownloadOSV/SyncOSV so
removeOldOSVArtifacts(vulnPath, now) is always invoked regardless of whether
toDownload is empty (i.e., call removeOldOSVArtifacts before the early return or
call it in both branches), and propagate its error similarly (e.g., return
toDownload, fmt.Errorf("warning: failed to clean up old OSV artifacts: %w",
err)) while keeping existing SyncOSV and error handling for the download path
intact.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4a04710-e225-4558-b00e-a492917cccee
📒 Files selected for processing (14)
changes/40057-osv-vulnscmd/fleet/cron.gocmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.ymlcmd/osv-processor/main.gocmd/osv-processor/main_test.goserver/config/config.goserver/fleet/app.goserver/fleet/vulnerabilities.goserver/vulnerabilities/osv/analyzer.goserver/vulnerabilities/osv/analyzer_test.goserver/vulnerabilities/osv/downloader.goserver/vulnerabilities/osv/sync.goserver/vulnerabilities/osv/sync_test.go
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
Adds an OSV-backed vulnerability scanning path for Ubuntu hosts and wires it into the existing vulnerability cron workflow behind a new server configuration flag.
Changes:
- Introduces OSV artifact sync/download, parsing, and matching logic for Ubuntu packages (including kernel-specific handling).
- Adds
vulnerabilities.osv_for_vulnerabilitiesconfig flag (default enabled) and updates the cron scanner to run OSV for supported platforms while excluding those platforms from OVAL when enabled. - Updates the OSV processor and CLI golden fixtures to reflect new/changed configuration/output.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
server/vulnerabilities/osv/sync.go |
Refreshes local OSV artifacts and cleans up old ones. |
server/vulnerabilities/osv/downloader.go |
Fetches latest release metadata and downloads OSV artifacts. |
server/vulnerabilities/osv/analyzer.go |
Loads OSV artifacts and analyzes Ubuntu hosts’ installed packages for CVEs. |
server/vulnerabilities/osv/sync_test.go |
Unit tests for artifact cleanup/version selection/filename/digest logic. |
server/vulnerabilities/osv/analyzer_test.go |
Unit tests for Ubuntu parsing and matching logic. |
server/fleet/vulnerabilities.go |
Adds a new UbuntuOSVSource vulnerability source constant. |
server/fleet/app.go |
Adds OSVForVulnerabilities to the vulnerabilities config struct. |
server/config/config.go |
Adds config schema + default/loader wiring for osv_for_vulnerabilities. |
cmd/fleet/cron.go |
Runs OSV scanning when enabled and excludes OSV-supported platforms from OVAL scanning. |
cmd/osv-processor/main.go |
Uses map[string]struct{} for changed-file sets in delta generation. |
cmd/osv-processor/main_test.go |
Updates tests for the changed-file set type change. |
cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.yml |
Updates golden output to include osv_for_vulnerabilities. |
cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.json |
Updates golden output to include osv_for_vulnerabilities. |
changes/40057-osv-vulns |
Release note entry for OSV-based Ubuntu vulnerability scanning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mostlikelee
left a comment
There was a problem hiding this comment.
LGTM. a few nits, but nothing worth blocking
Bug fix for #42063 **Related issue:** Resolves #40057 # Checklist for submitter ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually For unreleased bug fixes in a release candidate, one of: - [ ] Confirmed that the fix is not expected to adversely impact load test results --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Bug fix for #42063 **Related issue:** Resolves #40057 # Checklist for submitter ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually For unreleased bug fixes in a release candidate, one of: - [ ] Confirmed that the fix is not expected to adversely impact load test results --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Unreleased bug fix for #42063 **Related issue:** Resolves #39900 ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually For unreleased bug fixes in a release candidate, one of: - [x] Confirmed that the fix is not expected to adversely impact load test results - [x] Alerted the release DRI if additional load testing is needed We shouldn't need any additional load testing. This change will not have a large impact on load.
Unreleased bug fix for #42063 **Related issue:** Resolves #39900 ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually For unreleased bug fixes in a release candidate, one of: - [x] Confirmed that the fix is not expected to adversely impact load test results - [x] Alerted the release DRI if additional load testing is needed We shouldn't need any additional load testing. This change will not have a large impact on load.
Related issue: Resolves #40057
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
Summary by CodeRabbit
New Features
Features
Chores
Tests