Make weekly security scan actionable: suppress documented false positives + fix broken email notification#1458
Closed
vikrantpuppala wants to merge 1 commit into
Closed
Conversation
…ives + fix broken email notification Two related changes to the weekly OWASP scan: 1. Add suppressions for CVEs that scanner matches via shared CPE namespaces but that do not actually affect the artifacts we ship: - CVE-2024-52338: Apache Arrow R package on CRAN only; not Java - CVE-2026-33186: gRPC-Go server authz bypass; grpc-java unaffected, and the driver runs no gRPC server anyway - CVE-2026-0994: pip protobuf (Python) only; not protobuf-java - 5 libthrift CVEs in the May 2026 batch scoped to non-Java bindings (C_glib / Go / Node.js / Rust) Each entry includes a comment block with the rationale, the affected binding, and a link to the authoritative advisory, following the existing precedent for CVE-2026-25087 (Arrow C++ vs Java). The remaining libthrift CVEs that may apply to Java (CVE-2026-41603, 41604, 41605, 43869) are NOT suppressed; they will be cleared by a follow-up bump to libthrift 0.23.0 (which requires regenerating TCLIService.java with the matching compiler). 2. Fix vulnerabilityCatcher.yml so the email notification actually runs when findings exist. Previously the OWASP step exited non-zero (because <failBuildOnCVSS>7</failBuildOnCVSS> is set in jdbc-core/pom.xml), short-circuiting the rest of the job. The "Check for vulnerabilities" and "Send Email" steps never ran. Result: 7+ consecutive weeks of red workflow runs since early April with no email sent, even though the scanner was correctly detecting CVEs every Sunday. Add continue-on-error to the maven step so the explicit "Check for vulnerabilities" gate downstream is what decides whether to alert (and fail the job, after the email has been sent). Also add if: always() to the artifact upload so the HTML/JSON report is uploaded even when findings cause the job to fail. Verified: after this change, mvn org.owasp:dependency-check-maven:check shows 4 remaining flagged CVEs (all libthrift, all in the unspecified- binding bucket) instead of the previous noise of ~20+ findings. No real findings are hidden by the suppressions. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
5 tasks
Collaborator
Author
|
Closing in favor of a single unified PR that consolidates this work with #1459 (the per-PR security gate). 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
Two related changes to the weekly OWASP security scan, both aimed at getting the scanner back to a state where its output is useful instead of noise.
1. Add suppressions for documented false positives
After PR #1456 landed the real CVE fixes, the remaining findings in the weekly scan are dominated by CPE-mismatch false positives: scanner matching a Java artifact against an advisory that's actually scoped to a different language binding (R, Go, Python, C_glib, Node.js, Rust). Each entry below has been independently verified against the upstream advisory:
json_format.ParseDict(). No Java fix exists because Java isn't affected.Each suppression has a comment block above it explaining the rationale, affected binding, and advisory link — same pattern as the existing CVE-2026-25087 suppression for Arrow C++ vs Java.
The remaining libthrift CVEs whose binding is unspecified or known to affect Java (CVE-2026-41603, 41604, 41605, 43869) are NOT suppressed. They will be cleared by a follow-up PR bumping libthrift 0.19 → 0.23.0, which requires regenerating
TCLIService.javawith the matching compiler (separate, larger work item).2. Fix
vulnerabilityCatcher.ymlso the email notification actually runsThe weekly workflow 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 injdbc-core/pom.xml:357). That non-zero exit short-circuits the rest of the job, so the subsequentCheck for vulnerabilitiesandSend Emailsteps never run. Result: emails fire only when the scan is clean — exactly backwards from intent.Fixes:
continue-on-error: trueto the OWASP maven step. The explicitCheck for vulnerabilitiesgate downstream is now what decides whether to alert and fail the job (it already doesexit 1on findings, after writing the email HTML).if: always()to the artifact upload so the dependency-check HTML/JSON report is uploaded even when findings cause the job to fail. Recipients need the report to triage.Verification
No real findings are hidden by the suppressions. The 4 libthrift entries that remain are exactly the ones we want the next person to see when the weekly scan fires.
Out of scope — follow-up work
TCLIService.javawith the 0.23 compiler fromsrc/main/java/com/databricks/jdbc/dbclient/impl/thrift/TCLIService.thrift; no codegen tooling currently in repo). Tracked separately.OVERRIDE_SECURITY=trueescape hatch. Out of scope here; needs separate proposal.Test plan
mvn org.owasp:dependency-check-maven:check -pl jdbc-coreshows the expected reduced finding set.workflow_dispatch) post-merge to verify the email notification fires correctly when findings exist.NO_CHANGELOG=true
This pull request and its description were written by Isaac.