Add per-PR security scan: OWASP + OSV-Scanner with cyclonedx SBOM#1459
Closed
vikrantpuppala wants to merge 1 commit into
Closed
Add per-PR security scan: OWASP + OSV-Scanner with cyclonedx SBOM#1459vikrantpuppala wants to merge 1 commit into
vikrantpuppala wants to merge 1 commit into
Conversation
Phase 1 of the security gate. A new workflow runs on every PR to main and on workflow_dispatch, failing the job if either scanner reports a CVSS >= 7 finding that isn't covered by an existing suppression entry. Two scanners give complementary coverage: * OWASP dependency-check (NVD CPE-based) -- the existing setup the weekly scan uses. Reuses owasp-suppressions.xml and the failBuildOnCVSS=7 threshold already configured in jdbc-core/pom.xml. * OSV-Scanner v2.3.8 (purl-based via the OSV database, which federates GHSA / NVD / PyPA / RustSec). Catches advisories OWASP misses because they have no NVD CPE entry -- the lz4 CVE-2025-66566 case was the motivating example. Also caught a new finding I would not otherwise have seen: bouncycastle CVE-2026-5598 (severity 8.9) is invisible to OWASP's CPE-only resolution path. OSV scans the cyclonedx aggregate SBOM produced by `mvn package`, not the source pom.xml. This matters because OSV's default resolver pulls artifact metadata from deps.dev, which lags behind whatever bumps are sitting on main (deps.dev only knows about published GAs). The SBOM captures the actually-resolved local dependency tree. Adds: * `.github/workflows/prSecurityScan.yml` -- the new workflow. * `osv-scanner.toml` -- OSV suppressions mirroring owasp-suppressions.xml. Keep the two files in sync when adding or removing entries; both scanners should agree on what is and is not a real finding. * `cyclonedx-maven-plugin` execution in the parent pom that emits an aggregate `target/bom.json` during the package phase, with `skipNotDeployed=false` so it runs on the parent and jdbc-core (which have maven.deploy.skip=true). This PR does not flip branch protection -- the workflow runs visibly red but doesn't block merges. Once the outstanding findings are cleared (bouncycastle 1.79 -> 1.84 and the libthrift 0.19 -> 0.23 follow-up), mark the check required so the gate actually has teeth. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
3 tasks
Collaborator
Author
|
Closing in favor of a single unified PR that consolidates this work with #1458. The new PR will replace |
8 tasks
vikrantpuppala
added a commit
that referenced
this pull request
May 21, 2026
## Summary Consolidates security scanning into one workflow with one job (`securityScan.yml`). Replaces `vulnerabilityCatcher.yml`. Adds OSV-Scanner alongside the existing OWASP check, plus cyclonedx SBOM generation so OSV scans the actually-resolved local dependency tree. This bundles the work that was originally split across (now-closed) PRs #1458 (suppressions + email-notification fix) and #1459 (per-PR gate). One unified workflow file, one set of suppression rules, one place to look. ### Design: single job, terminal steps gated by event ``` Triggers: pull_request, schedule (weekly cron), workflow_dispatch │ ├─ shared: checkout, JDK, JFrog OIDC, maven config ├─ shared: NVD database cache (saves ~2 min) ├─ shared: mvn package (generates cyclonedx aggregate SBOM) ├─ shared: OWASP dependency-check (continue-on-error) ├─ shared: install + run OSV-Scanner ├─ shared: Collect findings (writes job summary; sets outputs) │ ├─ if pull_request + findings ⇒ fail the job └─ if schedule|dispatch + findings ⇒ send email + fail the job ``` No duplicated steps across jobs. Same scan logic for every trigger; only the notification mechanism differs. ### What changed | File | Change | |---|---| | `.github/workflows/vulnerabilityCatcher.yml` | **Removed.** Superseded by `securityScan.yml`. | | `.github/workflows/securityScan.yml` | **New.** Single job with event-gated terminal steps. Includes NVD database caching for faster runs. | | `owasp-suppressions.xml` | **8 new entries** for documented CPE/ecosystem false positives — Arrow R-only (CVE-2024-52338), gRPC-Go-only (CVE-2026-33186), protobuf Python-only (CVE-2026-0994), 5 libthrift non-Java bindings. Each entry has a justification comment block. | | `osv-scanner.toml` | **New.** Mirrors the OWASP suppressions in TOML format. Keep the two files in sync when adding/removing entries. | | `pom.xml` | **cyclonedx-maven-plugin 2.9.1** added, bound to `package` phase, `skipNotDeployed=false`. Emits an aggregate `target/bom.json` for OSV to read. | ### Why two scanners | Scanner | Source | Catches | Misses | |---|---|---|---| | OWASP dependency-check | NVD (CPE-based) | Anything with an NVD CPE entry. Reuses existing `owasp-suppressions.xml` and `failBuildOnCVSS=7` threshold. | GHSA-only advisories without an NVD CPE (e.g., CVE-2025-66566 in lz4-java — the #1455 finding). | | OSV-Scanner v2.3.8 | OSV.dev (purl-based; federates GHSA + NVD + PyPA + RustSec) | The GHSA-only gap. Already turned up a new finding: **bouncycastle CVE-2026-5598 (severity 8.9)** in bcprov-jdk18on 1.79, invisible to OWASP. | Findings without a CVSS score (rare for high-severity). | ### Why cyclonedx SBOM OSV-Scanner's Maven resolver consults deps.dev's published-artifact metadata. For the project's *own* GA, that means it sees the previously-published 3.3.3 pom — not whatever bumps are sitting on `main`. Result: in CI, OSV would keep showing yesterday's released state instead of the PR branch's actual dependency tree. Producing a CycloneDX aggregate SBOM at build time captures the actually-resolved local tree. ### Fixes from #1458 included here The weekly scan has been failing every Sunday for 7+ consecutive weeks (since early April 2026) without sending a notification email. Root cause: the OWASP maven plugin step exits non-zero when CVEs are found (because `<failBuildOnCVSS>7</failBuildOnCVSS>` is set in `jdbc-core/pom.xml:357`), short-circuiting the rest of the job. The subsequent steps never ran. The new workflow has `continue-on-error: true` on the OWASP step and routes finding-detection through an explicit `Collect findings` step that reads the OWASP JSON report directly. Whether the job ultimately fails (and whether email goes out) is decided after that step, not by maven's exit code. ### Findings on `main` today This workflow, run against current `main`, reports unsuppressed findings that will be addressed in **separate follow-up PRs**: 1. \`org.bouncycastle:bcprov-jdk18on@1.79\` → CVE-2026-5598 (severity 8.9). Fixed in bouncycastle 1.84. **OWASP doesn't see this — OSV-only finding.** 2. \`org.apache.thrift:libthrift@0.19.0\` → 4 CVEs (CVE-2026-41603/04/05/43869, max severity 8.2). Cleared by the libthrift 0.19 → 0.23 follow-up (requires regenerating \`TCLIService.java\` with the matching compiler). So this PR's own CI is expected to fail on these 2 known findings until the follow-ups land. Aligned with the rollout plan: add the workflow now, mark it required-to-merge in branch protection after the burn-down. ### Expected runtime Each invocation: **~5–7 minutes** (vs ~4 min for the old weekly). Breakdown: - `mvn package` (with cyclonedx SBOM): ~45–90s - OWASP dependency-check (with NVD cache hit): ~1–2 min - OSV-Scanner install + run: ~15s - Other steps: ~30s NVD caching cuts ~2 minutes off OWASP's runtime when the cache is warm. The cache key rotates daily so we still get fresh CVE data. ### What this PR is NOT * It is **not** a required check. Branch protection is unchanged — PRs aren't blocked by the red ❌ yet. * It does **not** introduce diff-mode (net-new findings only) gating. Every PR has to ship into a clean codebase; this is the design we agreed on, to force burn-down rather than just \"not making it worse.\" * It does **not** include a Phase 2 design (override flags, severity tiers, auto-issue filing). Phase 2 is deferred until we have ~2 weeks of operational experience. ### Test plan - [x] \`mvn package -DskipTests -Ddependency-check.skip=true\` produces \`target/bom.json\` (69 components, all expected versions). - [x] \`mvn -pl jdbc-core org.owasp:dependency-check-maven:check\` reports 4 unsuppressed CVEs (all libthrift; matches expectation). - [x] \`osv-scanner scan source --config=osv-scanner.toml --format=json target/bom.json\` + severity≥7 filter reports 2 findings (bouncycastle + libthrift cluster). - [x] Findings-step jq logic correctly counts OWASP CVSS>=7 from the JSON report. - [x] XML validation of \`owasp-suppressions.xml\`. - [x] YAML validation of \`securityScan.yml\`. - [ ] CI run on this PR — expected to fail on the 2 known findings above. **The CI failure on this PR is the intended outcome.** - [ ] Manual \`workflow_dispatch\` run post-merge to verify the email path fires. NO_CHANGELOG=true This pull request and its description were written by Isaac. --------- Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a per-PR security gate (Phase 1 of the broader rollout). A new workflow runs on every PR to
mainand fails the job if either OWASP dependency-check or OSV-Scanner reports an unsuppressed CVSS ≥ 7 finding.Why two scanners
owasp-suppressions.xmland thefailBuildOnCVSS=7threshold injdbc-core/pom.xml.Both scanners use suppression files (
owasp-suppressions.xmlandosv-scanner.toml) listing documented false positives. Each entry has a justification comment. Keep the two in sync when adding/removing entries.Why cyclonedx SBOM (and not just `pom.xml`)
OSV-Scanner's Maven resolver consults deps.dev's published-artifact metadata. For the project's own GA, that means it sees the previously-published 3.3.3 pom (with lz4@1.8.1, etc.) — not whatever bumps are sitting on `main`. Result: in CI, OSV would keep showing yesterday's released state instead of the PR branch's actual dependency tree.
Fix: produce a CycloneDX aggregate SBOM at build time (`mvn package`) and feed that to OSV. The SBOM captures the actually-resolved local tree.
Added `cyclonedx-maven-plugin` 2.9.1 to the parent pom, bound to the `package` phase, with `skipNotDeployed=false` to override its default behavior of skipping modules with `maven.deploy.skip=true` (parent and jdbc-core).
Findings on `main` today
This workflow, run against current `main` (post-#1456, pre-#1458), reports 2 unsuppressed CVSS ≥ 7 findings:
So this PR's CI will be red on its own scan until those two follow-ups land. That's intentional and aligned with the rollout plan:
What this PR is NOT
Test plan
NO_CHANGELOG=true
This pull request and its description were written by Isaac.