feat(evidence): aicr evidence verify (directory input)#879
Conversation
Adds the first slice of the evidence verifier: an offline `aicr evidence verify <dir>` command that recomputes every file's sha256 against the bundle's manifest.json (bound to the predicate's manifest.digest) and renders a Markdown or JSON summary of the signed predicate (recipe, fingerprint, phase counts, BOM info). Directory input only; signature verification, inline constraint replay, OCI pull, and pointer-file support ship in follow-up PRs. Exit codes: 0 bundle valid; every check passed 1 bundle valid; recorded validator results show failures 2 bundle invalid (manifest hash mismatch or predicate malformed)
Tighten the verifier slice now that the surrounding scaffolding is in: - Drop the unused verifier.OutputFormat type and VerifyOptions field; the CLI already validates the format string and dispatches on it. - Reuse attestation.HashFileSHA256 in inventory.go instead of an inlined sha256/io.Copy block. - Map bundle-invalid to ErrCodeInvalidRequest (was ErrCodeUnauthorized); the failure is structural, not an auth denial. - Rewrite forward-looking "first slice / PR 3 / PR 4 / ADR-007 §..." references in package, type, and function comments to describe the current behavior instead of an ordering that will rot. - Collapse the CLI exit-code table to two rows (0 and 2); the library API still distinguishes 1 (recorded phase failures) via VerifyResult.Exit, called out in the help and docs.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The new `aicr evidence verify` command added a third `Name: "format"`
StringFlag declaration, pushing goconst over its 3-occurrence threshold
and failing CI. Hoist the literal into the existing pkg/cli/consts.go
flag-name block and update the three flagged sites (and the two
cmd.String("format") reads in the same files).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 1899-1924: Add blank lines before and after the fenced code block
under "Synopsis" (the ```shell block), before the table that follows
"**Flags:**", and before the fenced code block under "**Examples:**" so that
each fence/table is separated by at least one empty line; update the markdown
around the "Synopsis" code fence, the "**Flags:**" table, and the
"**Examples:**" code fence to satisfy MD031/MD058 spacing rules.
In `@pkg/evidence/verifier/verify.go`:
- Around line 79-87: Change CheckInventory to accept a context.Context (e.g.,
func CheckInventory(ctx context.Context, mat Manifest) (...)) and update its
internal loops (the hashing loop over manifest entries) to regularly check
ctx.Done() and return early with ctx.Err() (or a wrapped error) when cancelled;
update every caller (including the call here that currently invokes
CheckInventory(mat)) to pass the ctx from the surrounding verification function
(the same ctx used for other steps) so cancellation propagates, and ensure the
returned error case in verify.go still records the failure via record(r,
stepInventory, ...) and sets r.Exit appropriately.
🪄 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: e924e9eb-23eb-4039-9af9-3a4d266af140
📒 Files selected for processing (16)
docs/user/cli-reference.mdpkg/cli/evidence.gopkg/cli/evidence_verify.gopkg/cli/evidence_verify_test.gopkg/cli/root.gopkg/evidence/verifier/doc.gopkg/evidence/verifier/fetch.gopkg/evidence/verifier/fetch_test.gopkg/evidence/verifier/input.gopkg/evidence/verifier/input_test.gopkg/evidence/verifier/inventory.gopkg/evidence/verifier/report.gopkg/evidence/verifier/testbundle_test.gopkg/evidence/verifier/types.gopkg/evidence/verifier/verify.gopkg/evidence/verifier/verify_test.go
There was a problem hiding this comment.
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 `@pkg/cli/evidence_verify.go`:
- Around line 89-99: Replace the bare error returns in the evidence verification
flow with errors.PropagateOrWrap to preserve error codes: where
verifier.Verify(ctx, opts) currently returns err, wrap/propagate that error
before returning; where writeVerifyOutput(cmd.Root().Writer, format, result) is
called, use errors.PropagateOrWrap on its returned error; and where
verifier.WriteMarkdown(opts.MarkdownPath, result) returns an error, also
wrap/propagate it. Use the errors.PropagateOrWrap function so existing coded
errors are preserved and otherwise supply a fallback code when wrapping; update
the return statements in the Verify flow and in writeVerifyOutput (and any bare
returns in that function) to use errors.PropagateOrWrap.
🪄 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: 414c35f7-f409-4de8-b10f-29804a5b6da2
📒 Files selected for processing (4)
pkg/cli/bundle_verify.gopkg/cli/consts.gopkg/cli/evidence_verify.gopkg/cli/root.go
Plumb the verify context into CheckInventory and findExtras so a large bundle or a hostile manifest with many entries can be canceled mid-walk. Checks ctx.Err() between files in the manifest loop and between filesystem entries in findExtras.
There was a problem hiding this comment.
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 `@pkg/evidence/verifier/inventory.go`:
- Line 106: The direct return of attestation.HashFileSHA256(full) must capture
its error and wrap it with pkg/errors.PropagateOrWrap to add context; replace
the single return with calling attestation.HashFileSHA256(full), check the
returned err, and return errors.PropagateOrWrap(err, errors.EINTERNAL, "hashing
file "+full) (or another appropriate exported error code constant) so any
existing structured error is preserved while adding context; reference
attestation.HashFileSHA256 and errors.PropagateOrWrap when locating the change.
🪄 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: 033bcfe7-33f2-4641-a6b1-589e5c01581a
📒 Files selected for processing (3)
pkg/evidence/verifier/inventory.gopkg/evidence/verifier/verify.gopkg/evidence/verifier/verify_test.go
CheckInventory's per-file hash step was returning the raw error from attestation.HashFileSHA256, which loses the bundle-relative path in the reported mismatch row. Wrap via errors.PropagateOrWrap so any inner StructuredError code is preserved while the message gains the offending file name.
There was a problem hiding this comment.
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 `@pkg/evidence/verifier/inventory.go`:
- Around line 91-92: The hashFile function must reject non-local relative paths
from manifest.json before joining into bundleDir: call filepath.IsLocal(rel) at
the top of hashFile (or equivalent validation) and return a clear error if it
returns false, preventing traversal like "../../../etc/passwd"; then proceed to
compute full := filepath.Join(bundleDir, filepath.FromSlash(rel)) only for
validated local paths so the code path in hashFile uses a safe rel value.
🪄 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: 482d676c-068a-44a9-9996-e03a14cf402c
📒 Files selected for processing (1)
pkg/evidence/verifier/inventory.go
A hostile bundle could include a manifest.json with path entries like "../../../etc/passwd"; the verifier would happily stat and hash files outside the bundle directory before failing the digest check. Gate hashFile on filepath.IsLocal so traversal entries are refused before any filesystem access.
There was a problem hiding this comment.
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 `@pkg/evidence/verifier/inventory.go`:
- Around line 160-162: The error wrapping of walkErr in the WalkDir handling
should preserve context cancellation semantics: inspect walkErr (or ctx.Err())
before returning in the block that currently does "return nil,
errors.Wrap(errors.ErrCodeInternal, ...)" and if the cause is context.Canceled
or context.DeadlineExceeded return/wrap with errors.ErrCodeUnavailable instead
of errors.ErrCodeInternal; update the code path around the WalkDir return
(referencing walkErr and the WalkDir call in inventory.go) so cancellation
errors are translated the same way as in CheckInventory (lines handling
ctx.Err()).
🪄 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: d556f212-27be-4b3c-b0fe-2da136e44358
📒 Files selected for processing (2)
pkg/evidence/verifier/inventory.gopkg/evidence/verifier/verify_test.go
There was a problem hiding this comment.
Nice clean structure — three-step pipeline, good separation between library and CLI, traversal guard via filepath.IsLocal, sensible test coverage including the stray-file and traversal cases. CI is green.
One blocker on the trust property this slice is meant to ship: the manifest-digest binding from manifest.json → predicate.Manifest.Digest is never actually checked. The inventory step recomputes per-file hashes against manifest.json, but manifest.json itself is trusted as-is, so if someone wanted to fake a bundle file and also rewrote manifest.json to match will passe verification with Exit=0. Until signature verification lands in a follow-up, this single digest comparison is what closes the loop — see the inline comment on inventory.go for the suggested fix and a regression test.
The other inline comments are smaller: a step-naming clarity issue (predicate parse failures surface as Step 2 / manifest-hash-check), a wording mismatch around exit-code branching, and a few nits (dead MarkdownPath field, ignored ctx, unvalidated SchemaVersion).
Blocking on the manifest-digest check; everything else can be follow-up.
Tighten the integrity chain and the CLI surface in one pass. Verifier (predicate → manifest → files binding): - Promote predicate parsing to its own step (was folded into inventory) so a malformed Statement reports as predicate-parse failure, not as manifest-hash-check failure. - CheckInventory now requires expectedManifestDigest (the predicate's Manifest.Digest) and verifies sha256(manifest.json) matches it before walking entries. Without this gate, a tampered bundle could rewrite manifest.json to match its own contents and pass file-by-file checks; this binds the unsigned manifest to the (eventually signed) predicate. - Reject unsupported manifest schemaVersion (verifier accepts 1.0.x). - MaterializeBundle now honors ctx cancellation for symmetry with the rest of the pipeline. CLI (unified output handling): - Replace --output-markdown with the standard --output/-o flag, and add the -t alias on --format. -o writes whichever format -t selects; empty -o keeps stdout behavior. No more double-rendering when a path is supplied. - Remove verifier.WriteMarkdown — superseded by the CLI's unified output path; the library returns the result, callers decide where the bytes go. Docs and tests updated for the new flag shape, the four-step pipeline, and the new failure modes (tampered manifest digest, unsupported schema, predicate-parse failure surfaced on its own step).
findExtras returns context.Canceled / DeadlineExceeded from the filepath.WalkDir callback, but the post-walk wrap was unconditionally labeling them ErrCodeInternal. That makes a normal cancellation look like a verifier bug. Detect the context-error case and wrap as ErrCodeUnavailable, matching the cancellation handling in the manifest entry loop above.
2 participants
Summary
First slice of
aicr evidence verify(ADR-007 PR-B): offline verification of an unpacked bundle directory produced byaicr validate --emit-attestation. Ships the receiving end of the bundle integrity chain plus a Markdown/JSON report.Motivation / Context
ADR-007 ships a recipe-evidence v1 bundle so contributors on hardware AICR maintainers can't reach can hand reviewers a signed, verifiable artifact. The emitter landed in a prior PR; this is the start of the consumer. Shipped as a vertical slice so each PR adds one capability an end user can invoke independently. This PR gives contributors directory self-debug —
aicr evidence verify ./out/summary-bundleafteraicr validate --emit-attestation ./out.Signature verification, inline constraint replay, OCI pull, and pointer files follow in three subsequent PRs.
Fixes: N/A (#753 closes with PR 4 of this series)
Related: #753, ADR-007
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)pkg/evidence/verifier(new package)Implementation Notes
Four steps run:
materialize-bundle→predicate-parse→manifest-hash-check→render-summary.Trust property shipped — the receiving end of the bundle integrity chain. Every file in the bundle is sha256-verified against
manifest.json, andsha256(manifest.json)is verified againstpredicate.Manifest.Digestread from the bundled in-toto Statement (statement.intoto.json).The Statement file is unsigned at this slice — both sides of the manifest-digest comparison come from the same untrusted bundle directory, so a tamperer who rewrites
manifest.jsonand the unsigned Statement in lockstep passes verification. The check catches accidental corruption and lazy tampering, which is the right contract for the contributor-self-debug case PR 1 targets.How PR 3 closes the chain. When signature verification lands, predicate extraction switches to the DSSE payload inside
attestation.intoto.jsonl(a Sigstore Bundle with a Fulcio-issued cert and Rekor inclusion proof). At that point,predicate.Manifest.Digestis cryptographically anchored to an OIDC identity at sign-time, and PR 1's manifest-hash check extends that anchor to every bundled file. PR 4 (OCI pull) adds redundancy via content-addressed registry storage.ADR-007 §"Verifier steps" enumerates twelve steps. This implementation runs four, collapsing the redundant per-field digest checks the manifest-hash chain already covers and folding display of signed fields (fingerprint, phase counts, BOM) into the renderer. The package doc records the mapping.
Non-directory inputs (pointer-like
.yaml, OCI refs) are rejected with a clear "not yet supported" message rather than silently failing — keeps the contract visible in the error surface.Output flags. Standard project pattern:
--output/-oselects the destination (stdout when empty, a file otherwise);--format/-tselects text (Markdown) or json. Format applies regardless of destination; no double-rendering.Exit codes. OS exit codes are
0(valid) and2(anything else, including recorded phase failures), perpkg/errors/exitcode.go's code-to-OS mapping. The 0/1/2 distinction from ADR-007 is preserved in the structured output —VerifyResult.Exitfield in JSON / Markdown carries1for the informational phase-failure case so JSON consumers can branch. Shell consumers can branch viajq '.exit'.Testing
go test ./pkg/evidence/verifier/... ./pkg/cli/... golangci-lint -c .golangci.yaml run ./pkg/evidence/verifier/... ./pkg/cli/... ./tools/check-docs-mdx ./tools/check-docs-sidebarAll green locally (lint 0 issues; tests pass).
Tests cover: happy-path verify of a synthesized bundle, tampered-file → exit 2, stray-file → exit 2, manifest-digest mismatch → exit 2, manifest schemaVersion rejection, traversal-entry rejection, ctx cancellation in inventory, predicate-parse failure attributed to its own step, auto-detect rejects unsupported input forms with clear errors,
-owrites to file with empty stdout,-o -t jsonwrites JSON to file,-t jsonemits thestepsarray on stdout.Risk Assessment
aicr evidence; no existing behavior changes; fully covered by unit tests; the trust property (self-consistency on an unsigned Statement) is partial-by-design and explicitly described in the package doc.Rollout notes: N/A. Users who don't invoke
aicr evidence verifysee no change.Checklist
docs/user/cli-reference.md)