fix(verifier): fail closed on invalid attestation; unify allowlist by mchmarny · Pull Request #1607 · NVIDIA/aicr

mchmarny · 2026-07-03T12:07:56Z

Summary

aicr verify now fails closed: a binary attestation that is present but fails verification yields trust level unknown instead of degrading to attested. Also retires the two stale hand-rolled allowlist loaders (GP2/GP4) in favor of the shared pkg/evidence/allowlist schema so the real recipes/evidence/allowlist.yaml parses everywhere.

Motivation / Context

A tampered or invalid binary attestation was treated the same as a legitimately missing one (attested), hiding an active verification failure (P1 security correctness). Separately, the GP2 producer and GP4 consumer loaders still expected the outdated issuer+identity schema, so neither could parse the canonical identityPattern/source allowlist — forcing the dashboard publish workflow to drop -allowlist.

Fixes: #1550
Fixes: #1505

Type of Change

Bug fix (non-breaking change that fixes an issue)

Component(s) Affected

Bundlers (pkg/bundler, pkg/component/*)
Docs/examples (docs/, examples/)
Other: pkg/corroborate, pkg/evidence/project, pkg/cli help text, .github/workflows/evidence-dashboard-publish.yaml

Implementation Notes

Verifier: missing binary attestation still reports attested (incomplete chain — nothing claims more). Present-but-unverifiable (digest extraction failure or VerifyBinaryAttestation error) now reports unknown — a claim exists and could not be substantiated. MaxAchievableTrustLevel is unchanged (computed from contents), so --min-trust-level max correctly fails such bundles; a regression test pins this interaction. Steps 4–5 were extracted into verifyBinaryStep for network-free testability.
Allowlist: corroborate.Allowlist is now a type alias to allowlist.Allowlist with LoadAllowlist delegating to the shared loader; project.Allowlist is a thin wrapper (not an alias) to preserve the documented nil-receiver first-party heuristic used when -allowlist is absent. Unlisted signers still classify as (community, allowlisted=false). Testdata rewritten to the canonical schema; both packages gained a test loading the real recipes/evidence/allowlist.yaml.
-allowlist recipes/evidence/allowlist.yaml re-enabled on both corroborate invocations in the publish workflow (defense in depth over the class baked into meta.json at ingest).
Docs: trust-level tables in docs/user/artifact-verification.md and docs/user/cli-reference.md updated (failed verify → unknown); docs/contributor/evidence-ingest.md classification section now documents the real schema; evidence-dashboard-publish.md deferral note replaced.

Testing

make qualify

make qualify passed locally (unit tests with -race, golangci-lint + yamllint, e2e incl. chainsaw 23/23, grype scan clean, license headers). Acceptance from #1505 verified: corroborate -allowlist recipes/evidence/allowlist.yaml loads and classifies the canonical file. New tests: TestVerifyBinaryStep (missing→attested, invalid→unknown, no-digest→unknown), TestVerifyBinaryStep_FailedVerifyMaxStaysVerified, rewritten classify_test.go in both packages (precedence, unlisted→reported, fail-closed on malformed/legacy-schema files).

Coverage (pkg/corroborate + pkg/evidence/project + pkg/bundler/verifier combined): 85.1% → 85.3% (+0.2%).

Risk Assessment

Low — Isolated change, well-tested, easy to revert

Rollout notes: Bundles with a valid or absent binary attestation are unaffected. Bundles whose binary attestation fails verification now report unknown, so policies pinned to --min-trust-level attested will (correctly) start rejecting them.

Checklist

Tests pass locally (make test with -race)
Linter passes (make lint)
I did not skip/disable tests to make CI green
I added/updated tests for new functionality
I updated docs if user-facing behavior changed
Changes follow existing patterns in the codebase
Commits are cryptographically signed (git commit -S) — GPG signing info

mchmarny · 2026-07-03T12:08:32Z

@CodeRabbit full review

coderabbitai · 2026-07-03T12:08:39Z

✅ Action performed

Full review finished.

github-actions · 2026-07-03T12:09:10Z

🌿 Preview your docs: https://nvidia-preview-fix-verify-trust-allowlist-schema.docs.buildwithfern.com/aicr

github-actions · 2026-07-03T12:10:03Z

Recipe evidence check

No leaf overlays affected by this PR.

This gate is warning-only and never blocks merge.

coderabbitai · 2026-07-03T12:16:54Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: ccace5a8-82e1-48fa-b31e-0a71543e95c7

📥 Commits

Reviewing files that changed from the base of the PR and between c599b1e and 1d7835e.

📒 Files selected for processing (18)

.github/workflows/evidence-dashboard-publish.yaml
docs/contributor/evidence-dashboard-publish.md
docs/contributor/evidence-ingest.md
docs/user/artifact-verification.md
docs/user/cli-reference.md
pkg/bundler/verifier/trust.go
pkg/bundler/verifier/verifier.go
pkg/bundler/verifier/verifier_test.go
pkg/cli/bundle_verify.go
pkg/corroborate/classify.go
pkg/corroborate/classify_test.go
pkg/corroborate/generate.go
pkg/corroborate/generate_test.go
pkg/corroborate/meta.go
pkg/corroborate/meta_test.go
pkg/corroborate/testdata/allowlist.yaml
pkg/evidence/project/classify.go
pkg/evidence/project/classify_test.go

📝 Walkthrough

Walkthrough

The verifier now routes binary-attestation handling through verifyBinaryStep, changing digest-extraction and verification failures to TrustUnknown while keeping missing binary attestations as TrustAttested. The corroborate and evidence/project allowlist loaders now delegate to pkg/evidence/allowlist, with updated fixtures, tests, workflow wiring, and contributor documentation to match the shared identityPattern/source schema. readBoundedFile also now enforces its size limit internally, and its call sites and tests were updated accordingly.

Estimated code review effort: 4 (Complex) | ~60 minutes

Related issues: #1550, #1505

Suggested labels: verifier, allowlist, documentation, testing

Suggested reviewers: Reviewers familiar with pkg/bundler/verifier and pkg/evidence/allowlist

Poem
One chain now fails to stay “attested,”
and falls to unknown when rejected.
One schema now speaks with one shared voice,
with tests and docs aligned by choice.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	pkg/corroborate/meta.go changes readBoundedFile's API and tests, but that refactor is unrelated to the attestation and allowlist objectives.	Move the readBoundedFile refactor into a separate cleanup PR unless it is required for this feature.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title matches the two main changes: invalid attestation fail-closed and shared allowlist schema.
Description check	✅ Passed	Description is on-topic and accurately describes the attestation and allowlist changes.
Linked Issues check	✅ Passed	Implements [`#1550`, `#1505`]: invalid binary attestation now degrades to unknown, and allowlist loading/docs/workflow now use the shared schema.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/verify-trust-allowlist-schema

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/user/cli-reference.md`:
- Around line 2524-2526: The user-facing verification table is inconsistent
because the `unknown` row in the CLI reference includes an inline issue
reference that should not appear in docs. Update the table entry in the
`attested`/`unverified`/`unknown` section so `unknown` matches the wording used
in `docs/user/artifact-verification.md`, and remove the `(`#1550`)` suffix while
keeping the semantic meaning unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: a47de9b8-5267-474a-9988-a6d551013e67

📥 Commits

Reviewing files that changed from the base of the PR and between bdd91fc and c599b1e.

📒 Files selected for processing (18)

.github/workflows/evidence-dashboard-publish.yaml
docs/contributor/evidence-dashboard-publish.md
docs/contributor/evidence-ingest.md
docs/user/artifact-verification.md
docs/user/cli-reference.md
pkg/bundler/verifier/trust.go
pkg/bundler/verifier/verifier.go
pkg/bundler/verifier/verifier_test.go
pkg/cli/bundle_verify.go
pkg/corroborate/classify.go
pkg/corroborate/classify_test.go
pkg/corroborate/generate.go
pkg/corroborate/generate_test.go
pkg/corroborate/meta.go
pkg/corroborate/meta_test.go
pkg/corroborate/testdata/allowlist.yaml
pkg/evidence/project/classify.go
pkg/evidence/project/classify_test.go

github-actions · 2026-07-03T12:17:23Z

Coverage Report ✅

Metric	Value
Coverage	78.3%
Threshold	75%
Status	Pass

Coverage Badge

![Coverage](https://img.shields.io/badge/coverage-78.3%25-green)

Merging this branch changes the coverage (2 decrease, 1 increase)

Impacted Packages	Coverage Δ	🤖
github.com/NVIDIA/aicr/pkg/bundler/verifier	72.29% (+8.09%)	👍
github.com/NVIDIA/aicr/pkg/cli	69.00% (ø)
github.com/NVIDIA/aicr/pkg/corroborate	91.51% (-0.65%)	👎
github.com/NVIDIA/aicr/pkg/evidence/project	81.48% (-4.55%)	👎

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/NVIDIA/aicr/pkg/bundler/verifier/trust.go	84.31% (+5.88%)	51	43 (+3)	8 (-3)	👍
github.com/NVIDIA/aicr/pkg/bundler/verifier/verifier.go	69.19% (+8.78%)	198 (+6)	137 (+21)	61 (-15)	👍
github.com/NVIDIA/aicr/pkg/cli/bundle_verify.go	76.19% (ø)	63	48	15
github.com/NVIDIA/aicr/pkg/corroborate/classify.go	100.00% (+3.19%)	8 (-86)	8 (-83)	0 (-3)	👍
github.com/NVIDIA/aicr/pkg/corroborate/generate.go	90.70% (ø)	430	390	40
github.com/NVIDIA/aicr/pkg/corroborate/meta.go	87.88% (ø)	33	29	4
github.com/NVIDIA/aicr/pkg/evidence/project/classify.go	100.00% (+7.14%)	11 (-87)	11 (-80)	0 (-7)	👍

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

…list A binary attestation that is present but fails verification (or whose binary digest cannot be extracted from the bundle attestation) now yields trust level 'unknown' instead of degrading to 'attested'. A missing binary attestation still reports 'attested' (incomplete chain). Docs and CLI help updated to match. Retire the hand-rolled issuer+identity allowlist loaders in pkg/corroborate (GP4) and pkg/evidence/project (GP2) in favor of the shared pkg/evidence/allowlist parser, so both read the canonical identityPattern/source schema of recipes/evidence/allowlist.yaml. Re-enable -allowlist in the evidence-dashboard-publish workflow and update the contributor docs to the real schema. Fixes: #1550 Fixes: #1505 Signed-off-by: Mark Chmarny <[email protected]>

mchmarny · 2026-07-03T12:23:07Z

Re CodeRabbit's "Out of Scope Changes" warning on pkg/corroborate/meta.go: the readBoundedFile signature change is a direct consequence of this PR, not an unrelated refactor. Retiring the hand-rolled allowlist loader removed the only caller that passed a non-constant size limit, leaving maxBytes always maxRunFileBytes — which fails the unparam linter. Dropping the now-constant parameter (and updating its test) is the minimal fix to keep golangci-lint green.

mchmarny requested review from a team as code owners July 3, 2026 12:07

mchmarny added the theme/supply-chain SLSA, SBOM, Sigstore, and provenance verification label Jul 3, 2026

mchmarny self-assigned this Jul 3, 2026

github-actions Bot added area/ci area/docs area/bundler area/cli size/XL labels Jul 3, 2026

coderabbitai Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread docs/user/cli-reference.md Outdated

mchmarny force-pushed the fix/verify-trust-allowlist-schema branch from c599b1e to 1d7835e Compare July 3, 2026 12:22

mchmarny merged commit 480b7ff into main Jul 3, 2026
285 of 287 checks passed

mchmarny deleted the fix/verify-trust-allowlist-schema branch July 3, 2026 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(verifier): fail closed on invalid attestation; unify allowlist#1607

fix(verifier): fail closed on invalid attestation; unify allowlist#1607
mchmarny merged 1 commit into
mainfrom
fix/verify-trust-allowlist-schema

mchmarny commented Jul 3, 2026

Uh oh!

mchmarny commented Jul 3, 2026

Uh oh!

coderabbitai Bot commented Jul 3, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 3, 2026

Uh oh!

github-actions Bot commented Jul 3, 2026

Uh oh!

coderabbitai Bot commented Jul 3, 2026 •

edited

Loading

Walkthrough

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 3, 2026

Changed files (no unit tests)

Uh oh!

mchmarny commented Jul 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

mchmarny commented Jul 3, 2026

Summary

Motivation / Context

Type of Change

Component(s) Affected

Implementation Notes

Testing

Risk Assessment

Checklist

Uh oh!

mchmarny commented Jul 3, 2026

Uh oh!

coderabbitai Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 3, 2026

Uh oh!

github-actions Bot commented Jul 3, 2026

Recipe evidence check

Uh oh!

coderabbitai Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 3, 2026

Coverage Report ✅

Merging this branch changes the coverage (2 decrease, 1 increase)

Changed files (no unit tests)

Uh oh!

mchmarny commented Jul 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Jul 3, 2026 •

edited

Loading

coderabbitai Bot commented Jul 3, 2026 •

edited

Loading