feat(ci): replace pip-audit with OSV-Scanner for CVE detection#30
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a scheduled OSV-Scanner-based vulnerability scanning and auto-remediation workflow, shifting security auditing away from the pip-audit dev dependency.
Changes:
- Removed
pip-auditfrom Python dev dependencies. - Added a nightly GitHub Actions workflow to run OSV-Scanner, upload SARIF, and open automated PRs with dependency fixes.
- Added helper scripts to parse
uv treeoutput, compare PEP 440 versions, and manage[tool.uv] override-dependenciesinpyproject.toml.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Drops pip-audit from the dev extras. |
| .github/workflows/osv-scanner.yaml | New scheduled OSV scan workflow with SARIF upload + auto-fix PR creation. |
| .github/scripts/update_overrides.py | Adds/updates override-dependencies in pyproject.toml for security pinning. |
| .github/scripts/extract_version.py | Extracts resolved package version from uv tree output. |
| .github/scripts/compare_versions.py | Compares versions using PEP 440 semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Hey @Kartikeya-trivedi , thanks for picking up this work !
overall LGTM , pretty well aligned with sdk ,
can you also please include a unit test for the scripts as well , like we have in sdk :
https://github.com/kubeflow/sdk/blob/main/.github/scripts/test_scripts.py
edit : Please provide a link to the workflow test run on your fork if possible
cc : @abhijeet-dhumal
|
@Krishna-kg732: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Hi @Krishna-kg732 thanks for the review, I am adding the tests, btw the workflow link is https://github.com/Kartikeya-trivedi/mcp-server/actions/runs/27156215089 will also update it in pr aswell |
Krishna-kg732
left a comment
Krishna-kg732
left a comment
There was a problem hiding this comment.
Hey @Kartikeya-trivedi , OSV-Scanner version pinning: The hardcoded OSV_VERSION="2.3.8" and checksum require manual monthly updates , I think we should automate this process wdyt?
overall looks good
|
HI @Krishna-kg732 yeah agreed, the manual bump is a known annoyance, it's inherited from the sdk version (Dependabot can't bump curl-installed binaries, and we keep the pin + checksum since the workflow runs with write permissions, so blindly pulling latest isn't great supply-chain-wise). Good news is osv-scanner releases ship an official osv-scanner_SHA256SUMS asset, so this is automatable without losing the checksum verification: a tiny scheduled job that checks the latest release, pulls the version + matching checksum from the SHA256SUMS file, updates the two lines, and opens a PR for review. Same pattern as the security-fix PRs this workflow already creates. I'd keep this PR aligned with sdk as-is (v2.3.8 is actually the latest release right now, so nothing's stale) and do the auto-bump as a follow-up issue that way it can be contributed to kubeflow/sdk too instead of the two copies drifting. Happy to pick that up. wdyt? |
|
OSV-Scanner replacement for pip-audit is release-critical. Branch has merge conflicts with main — please rebase. Once rebased I'll review. /assign @Kartikeya-trivedi |
537489c to
da02c87
Compare
|
Hi @abhijeet-dhumal, I have rebased my branch and it is free of merge conflicts now, Feel free to review! |
|
quicknote: I rebased on main - picked up the [dependency-groups] layout, removed the security-audit job that landed with #3, kept the constraint-dependencies pins per the issue. |
|
/ok-to-test |
|
@Kartikeya-trivedi can you resolve merge conflicts? |
da02c87 to
a616f77
Compare
|
Thanks for the contribution, @Kartikeya-trivedi ! 🙏 |
|
@Kartikeya-trivedi can you confirm and verify workflow adds uv sync before running helper scripts (or confirm uv run auto-syncs in GHA context) ? |
|
Hello @abhijeet-dhumal, thanks for the review, for your questions above yes uv run auto-syncs acc to docs: "uv will ensure that the project environment is up-to-date before running the given command" and the fork run already exercised compare_versions.py (imports packaging) across ~8 packages with no import errors. That said, I'm still applying --with packaging per your other comment, because there's a subtle failure mode: if packaging weren't resolved, the script exits 1 (ModuleNotFoundError), which the fixer would misread as "upgrade sufficient" and silently skip a needed override. --with closes that. auto-bump job: I'd keep it out of this PR and track it as a follow-up (filing now) — v2.3.8 is current so nothing's stale, this PR is the release unblock so I'd rather not grow its scope, and it's self-contained (same nightly-PR mechanism). I'll mirror it to kubeflow/sdk too so both stay in sync. Can pick it up right after this merges. |
|
/ok-to-test |
|
@Kartikeya-trivedi Update SECURITY.md ,.. it still says pip-audit runs in CI; should point at the OSV-Scanner nightly workflow after this lands. Nits (can be same push or quick follow-up):
|
Port the nightly OSV-Scanner workflow from kubeflow/sdk: scan uv.lock, upload SARIF to the GitHub Security tab, and open automated fix PRs. Add the helper scripts it invokes (extract_version, compare_versions, update_overrides). Drop pip-audit from the dev dependencies and relock; it only queried PyPI's advisory DB, opened no fix PRs, and surfaced nothing in the Security tab. Fixes kubeflow#29 Signed-off-by: Kartikeya Trivedi <kartikeyatrivedi4oct2004@gmail.com>
The removal regex was anchored at column 0, so an indented (yet valid TOML) override-dependencies key survived and the script wrote a duplicate key, producing invalid TOML. Allow leading horizontal whitespace in the removal patterns. Signed-off-by: Kartikeya Trivedi <kartikeyatrivedi4oct2004@gmail.com>
Run: uv run pytest .github/scripts/test_scripts.py -v Signed-off-by: Kartikeya Trivedi <kartikeyatrivedi4oct2004@gmail.com>
…ixer Drop continue-on-error from the SARIF scan so an unexpected exit (e.g. 2) fails the job instead of being swallowed; add if: always() to the JSON scan so a SARIF crash still doesn't block the independent auto-fix path. Invoke compare_versions.py via 'uv run --with packaging' so it no longer relies on packaging being a transitive lockfile dep (a missing packaging would exit 1 and be misread as 'upgrade sufficient'). Signed-off-by: Kartikeya Trivedi <kartikeyatrivedi4oct2004@gmail.com>
The PR body declares a two-column table but fix_details lines were bullets, so they rendered outside the table. Emit '| pkg | advisory |' rows instead. Signed-off-by: Kartikeya Trivedi <kartikeyatrivedi4oct2004@gmail.com>
a67c6dc to
2970f30
Compare
|
hi @abhijeet-dhumal , Fixed the table in 2970f30 — the rows now come out as | pkg | advisory | so they actually render inside the table instead of dangling under the header as bullets. On SECURITY.md — small catch: it's not on main yet, it's coming in via #14, and the "pip-audit runs in CI" line is in that PR's diff. If I touch it here we'd just conflict with #14. if this merges first I'll do a quick 2-line follow-up. And yep, the label + "allow Actions to create PRs" setting are called out in the PR description as the post-merge admin checklist, only the auto-fix step needs them, scan + SARIF work either way. |
|
@Kartikeya-trivedi Almost ready. Direction matches sdk, prior crash/packaging/table issues look fixed. Two blockers before /lgtm: Two things before
|
Add a test-scripts Makefile target mirroring kubeflow/sdk and invoke it in the test job so the OSV-Scanner helper script tests actually gate CI. Signed-off-by: Kartikeya Trivedi <kartikeyatrivedi4oct2004@gmail.com>
Pin v2.4.0 with the linux_amd64 checksum from the release's official SHA256SUMS, cross-verified against the downloaded binary. Use printf instead of echo when piping multi-line uv tree output, and reword the pyproject constraint comment now that pip-audit is gone. Signed-off-by: Kartikeya Trivedi <kartikeyatrivedi4oct2004@gmail.com>
|
Both blockers + nits done: make test-scripts added (mirrors sdk's target) and wired into the test job right after Verify, so the script tests now gate CI — 8f09847 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhijeet-dhumal The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thanks @abhijeet-dhumal learned a lot, looking forward to contribute more :) |
Description
We were using pip-audit for the security scan, but it's fairly limited ie it only checks PyPI's advisory database, never opens fix PRs, and nothing shows up in GitHub's Security tab. This swaps it out for OSV-Scanner, ported over from kubeflow/sdk.
What changes:
uv.lock(also runnable on demand from the Actions tab)[tool.uv]override when a plain upgrade isn't enoughThe auto-fix part pulls in three small helper scripts (
extract_version,compare_versions,update_overrides). pip-audit is dropped from the dev dependencies and the lockfile is regenerated.Heads-up, since the repo wasn't quite in the state the issue's task list assumed:
security-auditjob existed intest-python.yamlto remove, pip-audit was only a dev dependency, never run in CI.[project.optional-dependencies].dev, not[dependency-groups].dev; removed from there.[tool.uv] constraint-dependenciessection exists yet, so nothing to keep. The scanner only writesoverride-dependencieswhen it needs to.For a maintainer with admin access: the auto-fix PR step needs an
area/securitylabel to exist and "Allow GitHub Actions to create and approve pull requests" enabled (Settings -> Actions -> General). Scanning and the Security-tab upload work without either.Workflow link = https://github.com/Kartikeya-trivedi/mcp-server/actions/runs/27156215089
Type of Change
Checklist
make test-python)make verify)Related Issues
Fixes #29