feat(sdk): expose verification and signing on pkg/client/v1 - #2218
Conversation
The facade could produce signed artifacts but not check one, so an integrator embedding AICR had to import pkg/bundler/verifier, pkg/evidence/verifier, and pkg/recipe/catalog to establish trust — packages carrying no stability guarantee. Adds the consumer-side surface (VerifyBundle, VerifyEvidence, VerifyCatalog, RecipeDigest, plus the stateless VerifyBinaryAttestation, ValidateIdentityPattern, TrustLevels, and evidence renderers) and the producer-side counterparts (PublishEvidence, SignCatalog). BundleVerifyOptions mirrors config.VerifySpec field-for-field so the AICRConfig binding is mechanical. MinTrustLevel departs from verifier.Policy: empty means "max", not "no check", so a caller who never considered the trust floor still gets a gate. Verification methods are bounded by defaults.VerifyOperationTimeout; the signing methods deliberately are not, since keyless OIDC can block on a human completing a browser or device-code flow. Routes six CLI commands and pkg/server/signing.go through the facade. pkg/server non-test code no longer imports pkg/bundler/verifier. `aicr evidence sign` stays CLI-only: it orchestrates six primitives across two packages including in-place pointer relocation, so wrapping it would mean exposing five more primitives or inventing a coarse method around filesystem side effects. Fixes: #2024 Signed-off-by: Mark Chmarny <mark@chmarny.com>
mchmarny
added
the
theme/supply-chain
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Coverage Report ✅
Coverage BadgeMerging this branch changes the coverage (2 decrease, 1 increase)
Coverage by fileChanged files (no unit tests)
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. |
Two review findings on the verification surface.
ValidateIdentityPattern only required the NVIDIA/aicr repository prefix
as a substring, so a pattern could carry it and still match everything:
^https://github\.com/NVIDIA/aicr/.*|.*$
That contains the prefix, compiles, and is anchored at the prefix, yet
its second branch matches any string. The identity matcher pins only the
OIDC issuer beyond this pattern, so such a value silently degraded
binary and catalog verification to "any GitHub Actions workflow in any
repository".
Adds two layers, neither of which subsumes the other. Structural:
reject top-level alternation, parsed via regexp/syntax rather than
split on "|" so separators inside groups and character classes are not
misread. This covers branches too narrow for any fixed canary set to
catch, such as one attacker-controlled repository. Behavioral: reject
any pattern that still matches a foreign-identity canary, which covers
widening a structural read cannot see, such as an empty branch inside a
group. Fixed in pkg/bundler/verifier so the CLI, server, and catalog
paths are all covered, not just the facade re-export.
SignCatalog accepted private Fulcio and key-based signing while
VerifyCatalog verifies only keyless GitHub OIDC certificates against
the public-good root with a required transparency-log entry. An
integrator could sign a catalog successfully and then find the
documented counterpart unable to verify it. SignCatalog now rejects
SigningKey, FulcioURL, and DisableTLogUpload before any signing work
runs; RekorURL and SigningConfigPath still pass through because
verification handles both. The undocumented `recipe sign-catalog
--fulcio-url` flag is removed for the same reason -- it could only
produce an unverifiable artifact. The release path uses neither.
The bundle pair is unaffected: `bundle --fulcio-url` is symmetric with
`verify --trust-root`, which is exactly what the catalog pair lacked.
Signed-off-by: Mark Chmarny <mark@chmarny.com>
|
Both findings confirmed and fixed in c242a4d. 1 — identity pinning was bypassableConfirmed. One correction to the prescribed fix: structural anchoring alone does not close it. Your example is already anchored at the repo prefix and still bypasses —
Fixed in Test cases added for all six bypasses you'd expect: trailing/leading alternation, alternation to one foreign repo, empty branch in a group, trailing empty branch, and inline flags plus alternation. Both legitimate in-tree overrides (the release default and Note this is a tightening of existing behavior, not only new surface — a previously-accepted pattern using top-level alternation will now be rejected. That is the intent, and no in-tree pattern is affected. 2 — sign/verify asymmetry on the catalogConfirmed. Took the restrict-signing branch of your suggestion rather than widening verification. Widening means extending
Also removed the undocumented On the round-trip test: a real one needs an OIDC token, and driving Worth noting for the broader review: the bundle pair was already symmetric — |
|
🌿 Preview your docs: https://nvidia-preview-feat-sdk-verification-surface.docs.buildwithfern.com/aicr |
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Review round two on the verification surface.
The previous fix rejected top-level alternation, but syntax.Parse returns
OpCapture for a grouped pattern, so a nested alternation slipped past
both layers:
(https://github.com/NVIDIA/aicr/.*|https://github.com/attacker/isolated/.*)
The root op is a capture rather than an alternation, and the foreign
branch names a repository no fixed canary set can enumerate. Confirmed
as accepted by the code as pushed.
Rather than recursing to classify which alternations can match before
the prefix -- fragile with nesting, inline flags, and repetition -- the
substring test becomes a prefix test. A pattern must BEGIN with
https://github.com/NVIDIA/aicr/ (one optional leading ^, escaped-dot
form accepted) and its root must not be an alternation. Those two are
sound rather than heuristic: a pattern starting with a literal whose
root is a concatenation can only match strings starting with that
literal. Alternatives placed after the prefix stay valid, since every
branch is already behind the pin. The canary layer is kept for widening
a structural read cannot see, such as an empty branch inside a group.
This tightens existing behavior: a pattern that merely contains the
prefix somewhere is now rejected. No in-tree pattern is affected, and
unanchored spellings still work because the leading ^ stays optional.
SignCatalog also now rejects RekorURL. The earlier reasoning -- that the
flag may name the public-good v1 URL, which verifies -- failed open,
because that is indistinguishable from a private log by URL alone. The
CLI `recipe sign-catalog --rekor-url` flag is removed for the same
reason --fulcio-url was. The residual gap is documented rather than
implied: SigningConfigPath still passes through because the release
requires it, and a signing config can name private endpoints.
Removes a test that reached the interactive OIDC flow. SigningConfigPath
and RekorURL passed the symmetry guard, so SignCatalog resolved an
attester, found no ambient token, and opened a browser -- which hangs a
headless CI run. Canceling the context does not help, since
ResolveAttesterLazy ignores its ctx parameter. The both-directions
assertion moved onto rejectUnverifiableCatalogSigning directly, which is
why that function was extracted.
Also fixes an unescaped table-cell pipe, a stale option-type name in a
doc comment, a nil-result dereference in SignCatalog, and a timeout
claim in the integrator guide that contradicted the signing exception.
Signed-off-by: Mark Chmarny <mark@chmarny.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Review round three, all documentation and comment accuracy. The identity-pattern rule changed to "must begin with the repository prefix" but several descriptions still said "must contain", which is the worse direction to be stale in: a reader follows the older rule and only finds out at validation time. Two were flagged (the api-server prose and the AICRConfig YAML example); a sweep found seven more in Go doc comments, two of which are --help strings, and one of which is VerifyTrustSpec.CertificateIdentityRegexp -- the committed-config path, so the likeliest to be copied into version control before failing. SignCatalog's rejected-setting count said three in three places while the list beneath had four; RekorURL was added without updating them. The counts now read four and the field comment names all four, so the next addition cannot leave a bare number to drift. The go-library section asserted that SignCatalog accepts only what VerifyCatalog can verify, then withdrew that guarantee three paragraphs later for SigningConfigPath. The claim is now scoped up front and the exception gets its own paragraph: a signing config can still produce a catalog VerifyCatalog rejects. Validating the loaded config against the public-good endpoints is noted as the principled fix, deliberately not taken here -- it needs a maintained list of public-good endpoints, and the release path depends on --signing-config working. Also excludes MakeBundle from the uncapped-context statement, since its cap is opt-in via BundleOptions.Timeout and only absent at the default. Signed-off-by: Mark Chmarny <mark@chmarny.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The CLI E2E chainsaw suite failed on a88c794: bundle-attestation-ci asserted the rejection message contained "must contain", which the begin-with rule reworded. The rejection itself was correct (exit 2) -- only the asserted substring was stale. Local `make qualify` did not catch it. tools/e2e runs the same chainsaw directory, but skips the attestation tests when no binary attestation is present, which is the case locally and not in CI. Rather than swap one prose substring for another, the step now asserts what actually matters and fails closed: - captures the exit status instead of `|| true`, so a regression that ACCEPTS a widened pattern fails the test rather than passing it silently -- the dangerous direction for a negative test - covers both rejection paths, the missing prefix ('.*') and the top-level alternation, which previously had no e2e coverage - matches on the repository path rather than the wording, so the next rewording does not break it again That assertion depends on every rejection naming the repository, which was not true: the alternation message described the rule without saying what to anchor to, and the CLI surfaces only the message, not the structured context. It now names the prefix and shows the accepted form. A unit test pins the invariant so the e2e assertion stays honest. Verified separately that all ten identity patterns in-tree -- the release default, the server/vault/ministack/scaffolding e2e workflows, and the four UAT workflows -- still validate under the new rule. Signed-off-by: Mark Chmarny <mark@chmarny.com>
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Approve with comments — 0 🔴 · 0 🟠 · 1 🟡 · 6 🔵
Well-engineered, thoroughly-documented additive SDK surface. The security crux — the 3-layer certificate-identity confinement (HasPrefix anchor + reject root OpAlternate + foreignIdentityCanaries sweep) — survived an adversarial bypass battery with no findings. Sign/verify symmetry, inflight accounting, timeout gating, and CLI behavior-preservation all verified sound. Everything remaining is documentation accuracy plus one intentional CLI flag removal on a Hidden CI-only command.
Inline comments below. The only 🟡 is F1 — a stale exported godoc that contradicts the security contract this PR just hardened; worth fixing before merge, the rest are optional nits.
Verified sound (examined, not defects): identity confinement (adversarial battery, no bypass) · default TrustedRepositoryPattern passes its own new validation · sign/verify symmetry covers all 4 dangerous fields · IgnoreTLog-requires-Key gating · inflight accounting (no leak/double-count/Close race) · MinTrustLevel empty→"max" · no --data/config regression · all 6 rewired commands Close() exactly once · api-diff + stability tests pin every new symbol/field/constant.
Seven review findings, all documentation and comment accuracy on the verification surface. BinaryAttestationVerifyOptions.IdentityRegexp still described the old "must contain" rule. It was the last one left and the one that mattered most: exported and stability-pinned, so unlike the CLI usage strings an out-of-tree consumer has no other source of truth. A substring pattern built from that godoc is now rejected at runtime. TrustedIdentityPattern claimed an override could verify a fork. Under the begin-with rule a fork's certificate identity can never satisfy it, so the clause described pre-hardening behavior. Now says the override exists to pin a different workflow within NVIDIA/aicr, and that verifying a fork is not possible and not intended. VerifyOperationTimeout was framed as a fallback for deadline-less callers, but the four verify methods wrap unconditionally, so context.WithTimeout caps a caller who deliberately allowed longer. The other facade methods all wrap unconditionally too, and making verify behave differently would trade a doc bug for an inconsistency while dropping protection for the unbounded-context case it exists for. So the constant now states it is an unconditional ceiling, and the sharp consequence is documented on VerifyEvidence: a cap breach returns an error, NOT EvidenceExitIncomplete, so a gate distinguishing "could not check this" from "checked it and it failed" must treat a context-deadline error as the former. TrustLevels disclosed that "max" is absent without saying what to do about it. A validator built from the list rejects only the documented default, so it passes every test where a level was set explicitly. Now states the list is not the accepted set for MinTrustLevel input. PublishEvidence reads c.version without the lock while SignCatalog and RecipeDigest snapshot it under one. Not a race -- version is immutable post-construction and MergeReports reads it the same way. The asymmetry is that those two take the lock for the DataProvider, which Close DOES clear, and pick up version while already holding it. Recorded inline, including the hazard a bare lock would not convey: do not copy the pattern for a field Close mutates. VerifyBundle and VerifyEvidence now state that the recipe catalog is not consulted, so any open Client will do and a hot-path caller can reuse one instead of constructing per call. Also marks the PR breaking and expands the rollout notes to cover the stricter pattern validation and the sign-catalog flag removal -- including that dropping the AICR_FULCIO_URL / AICR_REKOR_URL env sources makes an ambient AICR_REKOR_URL ignored rather than silently retargeting the log. Signed-off-by: Mark Chmarny <mark@chmarny.com>
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Re-review — Approve. Confirmed the two follow-up commits (a88c794, c2aba76) resolve all seven findings from the prior round, and did so as docs/comments only — no logic changed, no regressions, nothing new introduced.
- F1/F3
IdentityRegexpandTrustedIdentityPatterngodoc now matchValidateIdentityPatternexactly (begin-withhttps://github.com/NVIDIA/aicr/, optional leading^, no top-level alternation; the impossible "verify a fork" clause dropped). Verified against verifier.go:279. - F4
VerifyOperationTimeoutreworded to an unconditional ceiling and now documents the error-vs-Incompleteedge, called out onVerifyEvidence. - F5
TrustLevels()godoc now directs input-validation to also acceptmaxand the empty string. - F6
PublishEvidencecarries a precise rationale for the lock-freeversionread, with a warning not to copy the pattern for a Close-mutated field. - F7
VerifyBundle/VerifyEvidencedocument that any open Client is reusable on the hot path. - F2 correctly unchanged (intentional CLI-flag removal).
Nice, thorough follow-through.
2 participants
Summary
Adds the verification and signing surface to
pkg/client/v1, so an integrator can check every artifact AICR produces without importingpkg/bundler/verifier,pkg/evidence/verifier, orpkg/recipe/catalog. Routes six CLI commands andpkg/server/signing.gothrough it.Motivation / Context
The facade could produce signed artifacts but not check one. That capped #1149's acceptance bar — "every artifact has retrievable provenance and a documented verification path" — at CLI users only, and left every verification workflow reaching around the stability contract.
One finding reshaped the design: #1567 is already closed, so
config.VerifySpec{Policy, Trust}exists andpkg/cli/bundle_verify.goalready resolves verify settings from it. The "trust configuration is the hard part" blocker in the issue body is stale — the options shape was a committed schema to mirror, not a design to invent.Fixes: #2024
Related: #2016, #1149, #1567, #2026
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)pkg/client/v1), API-diff tooling (tools/api-diff*)Implementation Notes
Surface. Client-bound where the operation binds to the Client's catalog or needs a home for a future
WithConfig(#2026):VerifyBundle,VerifyEvidence,VerifyCatalog,RecipeDigest,PublishEvidence,SignCatalog. Package-level for the stateless primitives with noAICRConfigcounterpart:VerifyBinaryAttestation,ValidateIdentityPattern,TrustLevels,RenderEvidenceJSON,RenderEvidenceMarkdown.MinTrustLevelempty means"max", not "no check." This is a deliberate departure fromverifier.Policy, where empty skips the trust gate entirely. A caller who never considered the trust floor should get the strict default; lowering it stays an explicit act. Pinned by a test that proves empty agrees with explicit"max"and is not equivalent to a disabled check.A failed policy is data, not an error.
VerifyBundlereturns the fullReportalongsidePolicyFailureso callers can render why a bundle fell short. A non-nil error means verification could not run at all. Same split forVerifyEvidence: an invalid bundle is a verdict onExit, not an error.Two new transparent aliases, joining the five recorded under #2019:
BundleVerifyReportandEvidenceVerification. The evidence result reaches into the pointer, the in-toto predicate, the per-step records, the signer claims, and the failure cause. Owning translated copies means five more nested types tracking a still-evolving predicate shape, for no consumer benefit — callers read these through:=and never name the type. TheEvidenceExit*constants are re-exported so verdict branching needs no import.Timeouts.
defaults.VerifyOperationTimeout(5m) bounds the four verify methods, where none existed before. The two signing methods are deliberately uncapped: keyless OIDC can block on a human completing a browser or device-code flow, so a fixed cap would regress an interactive run that works today.CLI behavior is unchanged. The rewired commands use a new
embeddedClient()rather than the config-awarerecipeClientFromCmd. None of them defines--data, and routing them through the config-aware constructor would let aspec.recipe.dataentry in an unrelatedAICRConfigalter — or fail — an artifact verification that never reads the catalog.verify-catalog/sign-catalogtarget the embedded catalog specifically, which is what ships signed as a release asset.Deliberately out of scope:
aicr evidence sign. Not in the issue's table, and it orchestrates six primitives across two packages including in-place pointer relocation that rewrites files. Wrapping it means either exposing five more primitives or inventing a coarse method around filesystem side effects. Recorded on #2024 per #2025's "record the answer per command" rule.Tooling. The alias contract gate caught both new aliases on the first run, which is the gate working.
tools/api-diffregisters them;tools/api-diff_test.shneeded its mock closure output kept in sorted order (the gate compares against anLC_ALL=C-sorted contract without re-sorting) plus the two target packages added to its synthetic fixture repos;tools/api-diff-closure/main_test.gohad a hardcoded expectation.Testing
make qualify # exit 0Full gate green, including
api-diff(all additions, no incompatible changes since v0.19.0) and the 100+tools/api-diff_test.shshell assertions.New tests:
pkg/client/v1/verify_test.goandsign_test.gocover the guard matrix for every entry point, a fixture-backedVerifyBundlehappy path built throughchecksum.GenerateChecksums, theMinTrustLeveldefault semantics, the policy-failure-is-data contract, digest determinism, and the re-exported verdict constants.TestStability_VerificationandTestStability_Signingpin the surface.SignCatalog's success path is intentionally not unit tested: it needs a real OIDC token, and driving it without one falls through to the interactive browser flow and would hang CI. The goreleaser release hook exercises it on every tagged build.Coverage deltas:
pkg/client/v1: 83.2% → 82.9% (-0.3%)pkg/cli: 74.9% → 74.8% (-0.1%)pkg/server: 82.3% → 82.3% (0.0%)No new exported function is at 0% coverage.
Risk Assessment
Purely additive on the facade; the risk is the CLI/server rewiring, where six commands and the server's startup self-check now route through new code. Mitigated by keeping the adapters thin and behavior-preserving — every flag/config precedence rule, error code, and output format is unchanged, and the existing CLI and server test suites pass untouched.
Rollout notes: The facade additions need no migration —
pkg/bundler/verifier,pkg/evidence/verifier, andpkg/recipe/catalogremain importable; this adds a supported path without removing one. Two behavior changes do need calling out:ValidateIdentityPatternis stricter. A certificate-identity pattern must now begin withhttps://github.com/NVIDIA/aicr/(a leading^is allowed) and must not use top-level alternation. Previously it only had to contain the repository somewhere, which let^https://github\.com/NVIDIA/aicr/.*|.*$and(good|https://github.com/attacker/x/.*)through — both reduce the gate to "any GitHub Actions workflow in any repository". This affects--certificate-identity-regexp,--identity-pattern,AICR_BINARY_ATTESTATION_IDENTITY_REGEXP, andspec.verify.trust.certificateIdentityRegexp. All ten patterns in-tree (release default, four e2e workflows, four UAT workflows) still validate; a test asserts that. A pattern that merely contained the prefix now fails closed at validation time with a message naming what to anchor to.recipe sign-catalogdrops--fulcio-urland--rekor-url, along with theirAICR_FULCIO_URL/AICR_REKOR_URLenv sources. Both could only produce a signaturerecipe verify-catalogcannot verify. The command isHidden/CI-only and the goreleaser hook passes only--signing-configand--output, so the release path is unaffected. Two consequences for anyone scripting the hidden command: an explicit--rekor-urlnow gets urfave'sflag provided but not definedrather than a descriptive error, and an ambientAICR_REKOR_URL(set forbundle --attest) is now ignored bysign-cataloginstead of silently retargeting the log — the latter is the point, since that ambient pickup was producing unverifiable catalogs.Checklist
make testwith-race)make lint)git commit -S)