Skip to content

ci: gate PRs on govulncheck - #224

Open
peatey wants to merge 2 commits into
developfrom
ci/govulncheck-gate
Open

ci: gate PRs on govulncheck#224
peatey wants to merge 2 commits into
developfrom
ci/govulncheck-gate

Conversation

@peatey

@peatey peatey commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Why

We generate SBOMs on every PR but never fail on a vulnerability. sbom.yml runs
trivy twice purely for inventory (spdx-json, cyclonedx); pr.yaml covers
tests, lint, build and e2e. Nothing blocks a PR that introduces a
known-vulnerable dependency.

The existing tooling covers other axes — CodeQL (GitHub default setup), Mend
SAST, GitGuardian — but those are all SAST or secrets. Dependency
vulnerabilities were the uncovered one, which is exactly how
klauspost/compress and go.opentelemetry.io/otel sat at versions with
published advisories until they were found by hand in #223.

Why govulncheck rather than trivy

Chosen for its failure mode. govulncheck does reachability analysis and
fails only when our code actually calls a vulnerable symbol; advisories in
modules we merely require are reported without failing the job.

That distinction is what makes this gate viable. We carry three advisories that
no version bump can fix:

Advisory Module Fixed in
GO-2026-5932 golang.org/x/crypto openpgp, unmaintained N/A
GO-2022-0646 aws-sdk-go v1 s3crypto CBC padding oracle N/A
GO-2022-0635 aws-sdk-go v1 s3crypto in-band key negotiation N/A

aws-sdk-go v1 stays in the module graph regardless of our own code, because
opencost/pkg/cloud/provider imports it. A scanner that failed on unreachable
findings would be red from day one and need a permanently maintained ignore
list that quietly rots.

I originally suggested trivy fs --exit-code 1 as well, then tested it:

$ trivy fs --scanners vuln --exit-code 1 --quiet .
1        <-- fails on GO-2026-5932, severity UNKNOWN, no fix available

Dropped verbatim, that job never goes green. It would need
ignore-unfixed: true plus a severity floor to be usable. Left out of this PR
to keep it to one clean change — worth revisiting as a second opinion later,
since trivy catches things govulncheck won't (notably OS-package CVEs in the
ubi9-micro base image).

Verification

Confirmed green before opening, on both relevant bases:

develop (896c9c0)           govulncheck ./... -> exit 0
  "0 vulnerabilities ... 2 in packages you import, 3 in modules you require,
   but your code doesn't appear to call these"

chore/deps-currency-sweep   govulncheck ./... -> exit 0

So the gate is green on arrival and has no ordering dependency on #223 — it
can merge before or after.

Workflow YAML parsed and the job graph checked: vuln-scan has no needs and
runs in parallel; build-and-test, e2e-test and set-labels are untouched.

Design notes for reviewers

  • Separate job, not a step in build-and-test — so a failing unit test cannot mask a vulnerability, or vice versa.
  • Inherits the nightly cron already on this workflow. This is where most of the value sits: newly published advisories get caught against unchanged code, instead of waiting for someone to open a PR.
  • Pinned to govulncheck@v1.6.0, not @latest, so the scanner cannot change under a review in progress. This does not stale the data — the advisory database is fetched from vuln.go.dev at run time, so a pinned binary still sees new advisories. (v1.6.0 is the current latest.)
  • set-labels deliberately untouched. Its needs list drives a label reading "unit tests passed/failed"; wiring a vulnerability result into it would make that label mean something it doesn't say. Happy to change if you'd rather it gate the label too.
  • Plain checkout at repo root, rather than the path: ./ibm-finops-agent convention the other jobs use, since this job is self-contained and the nested path buys nothing here. Easy to align if you prefer consistency.

If you want this as a required check, that's a branch-protection setting rather
than anything in this file.

Not included

Adjacent supply-chain hardening from the same review, deliberately left for a
separate pass so this PR stays reviewable:

  • .golangci.yaml has no gosec — would have caught the missing ReadHeaderTimeout (G112) and os.MkdirAll(path, os.ModePerm) = 0777 (G301)
  • Actions not SHA-pinned (actions/checkout@v6, docker/build-push-action@v6, azure/setup-helm@v1); only trivy-action is pinned
  • Makefile:20 runs setup-envtest@latest unpinned in CI
  • Dockerfile:25 uses redhat/ubi9-micro:latest (trivy DS-0001)
  • sbom.yml:44 interpolates ${{ inputs.release_version }} straight into a run: block — script injection for anyone with workflow_dispatch

peatey and others added 2 commits August 6, 2026 08:37
We generate SBOMs on every PR but never fail on a vulnerability. sbom.yml
runs trivy twice purely for inventory (spdx-json and cyclonedx), and
pr.yaml covers tests, lint, build and e2e. Nothing blocks a PR that
introduces a known-vulnerable dependency.

The existing security tooling covers other axes: CodeQL (via GitHub
default setup), Mend SAST, and GitGuardian are all SAST or secrets.
Dependency vulnerabilities were the uncovered one, which is how
klauspost/compress and go.opentelemetry.io/otel sat at versions with
published advisories until they were found by hand.

Adds a govulncheck job. It was chosen over a trivy gate specifically for
its failure mode: govulncheck does reachability analysis and fails only
when our code actually calls a vulnerable symbol. Advisories in modules
we merely require are reported without failing.

That distinction matters here. We carry three advisories that cannot be
fixed by any version bump:

  GO-2026-5932  golang.org/x/crypto openpgp, unmaintained
  GO-2022-0646  aws-sdk-go v1 s3crypto CBC padding oracle
  GO-2022-0635  aws-sdk-go v1 s3crypto in-band key negotiation

All are Fixed in: N/A, and aws-sdk-go v1 stays in the graph regardless of
our own code because opencost's cloud provider package imports it. A
scanner that failed on unreachable findings would be red from day one and
would need a permanently maintained ignore list. Verified against both
develop and the current dependency branch: govulncheck exits 0 on each,
so this gate is green on arrival and has no ordering dependency on the
in-flight dependency PR.

The job runs independently of build-and-test rather than as a step inside
it, so a failing unit test cannot mask a vulnerability or vice versa. It
inherits this workflow's nightly cron, which is where most of the value
sits: newly published advisories get caught against unchanged code
instead of waiting for someone to open a PR.

govulncheck is pinned to v1.6.0 rather than @latest so the scanner cannot
change under a review in progress. This does not stale the data, as the
advisory database is fetched from vuln.go.dev at run time.

set-labels is deliberately left untouched. Its needs list drives a label
that reads "unit tests passed/failed", and wiring a vulnerability result
into it would make that label mean something it does not say.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread .github/workflows/pr.yaml
- uses: actions/checkout@v6

- name: Install Go
uses: actions/setup-go@v5

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see v7 is current. any reason to not use it?
https://github.com/actions/setup-go/tags

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it is all over this file. nevermind. future PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants