feat(secret-scan): add severity-aware failure policies#11
Conversation
be7c0c4 to
f9d7d40
Compare
| # With fail-policy=verified, unverified secrets (exit 185) should be detected | ||
| # but the workflow should still pass when using fail-policy=verified. | ||
| # Here we verify the scanner detects them; the reusable workflow tests the policy. | ||
| if docker run --rm -v "$RUNNER_TEMP/dirty-verified:/scan:ro" \ |
There was a problem hiding this comment.
This test does not test the reusable workflow. It also passes for any Docker error, not only exit code 185. Can we check for exit code 185 and test the policy logic directly?
There was a problem hiding this comment.
A true end-to-end negative test through the reusable workflow is structurally hard on GitHub Actions.
- A uses: job can't run setup steps, so you can't plant a fixture before the reusable workflow's own checkout scans the tree.
- Committing a fixture into the repo would trip the scanner on every other job (including the positive/clean tests).
There was a problem hiding this comment.
added a parameterised test stage - https://github.com/NVIDIA/security-workflows/pull/11/changes#diff-a68373eb11f8afb3fb42a99d6822680588bdf8a4caa85424aa265275df7c15d1R58
to generate the disposable secrets and test the reusable workflow.
| --entrypoint pulse-secret-scanner \ | ||
| "$PULSE_IMAGE" \ | ||
| filesystem /repo "--results=${RESULTS}" --json --fail \ | ||
| filesystem /repo --results=verified,unknown,unverified --json --fail \ |
There was a problem hiding this comment.
We now include unknown results, but the SARIF code labels them as unverified. Can we keep unknown and unverified as separate result types?
There was a problem hiding this comment.
Yes. changes applied.
| The scan always surfaces every finding class i.e. verified, unknown, and unverified. | ||
| This input only decides which classes fail the job. | ||
| Allowed: | ||
| - all (fail on verified or unverified) |
There was a problem hiding this comment.
These policy names do not match the GitLab component. For example, all has a different meaning here. Can we use the same names or choose clearer names? Please see https://gitlab-master.nvidia.com/security/devops-tools/ci-cd-components/secret-scan/-/blob/6.0.0/templates/source-code-secret-scan/template.yml#L64-75
@acrow-nvidia can you confirm
There was a problem hiding this comment.
Yes I agree. The GitLab component has the following policies:
- unverified - this is the default, allows unverified but fails on verified
- strict - fails on all findings: verified and unverified
- all - allows any job failure.
It would be good for the policies to match
There was a problem hiding this comment.
changes applied to reflect the policy names similar to gitlab and actual secret scanner.
fa202db to
d64c9ef
Compare
Make failure_policy the single enforcement lever for the Pulse reusable workflow, with names and default matching the GitLab secret-scan component so the GitLab and GitHub surfaces behave identically. failure_policy (default unverified): - unverified — fail on verified/live secrets (exit 183); keep the job green on unverified findings (exit 185). - strict — fail on any finding (183 or 185). - all — warn only; never fail the job on findings. Scanner/infra errors always fail closed; missing scan state fails the job. Scan and reporting: - Remove the caller-facing `results` input. The scan always runs with the broadest filter (--results=verified,unknown,unverified) so every finding class — including unverified findings such as private keys — reaches SARIF and is eligible for enforcement. - SARIF labels each finding's verification state distinctly (verified / unknown / unverified), derived from the scanner's Verified + VerificationError fields; verified -> error, unknown/unverified -> warning, with the state also in properties.verificationState. Testing: - ci.yml exercises positive and negative cases across all three policies. The unverified and all negatives run end-to-end through the reusable workflow via the repo-guarded, CI-only ci_test_setup input (both expected to pass); the strict negative asserts the scanner's exact exit code (185). Docs: - README, workflow catalogue, and CHANGELOG updated. Migration (pre-release interface change): - Remove any `results:` input from consumer `with:` blocks. - fail-on-findings: true -> failure_policy: strict (or unverified to tolerate unverified findings) - fail-on-findings: false -> failure_policy: all
d64c9ef to
2ee7fba
Compare
| # results: verified,unknown # workflow default | ||
| # fail-on-findings: false # warn-only during initial rollout | ||
| # runs-on: linux-amd64-cpu4 # nv-gha-runners label | ||
| # failure_policy: unverified # fail on any finding (default: unverified — fail verified, warn unverified) |
There was a problem hiding this comment.
"fail on any finding" is incorrect for this failure policy, update comment
3 participants
Replace the boolean fail-on-findings input with a constrained fail-policy input supporting all, verified, and none.