Skip to content

Export Sigstore bundle retrieval, offline verification, and identity API - #192

Merged
samuv merged 2 commits into
mainfrom
export-sigstore-verifier-api
Jul 28, 2026
Merged

Export Sigstore bundle retrieval, offline verification, and identity API#192
samuv merged 2 commits into
mainfrom
export-sigstore-verifier-api

Conversation

@samuv

@samuv samuv commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

ToolHive's skills lock-file feature (RFC THV-0080, Stack 2 — tracked in stacklok/toolhive#5899) needs to verify skill artifacts on install and re-verify stored bundles during sync --check, without duplicating this package's Sigstore internals. This promotes the existing machinery in container/verifier to a public API:

  • RetrieveBundles(imageRef, keychain) ([]Bundle, error) — surfaces the bundles from both layouts the package already handles internally (cosign .sig-tag signature manifests and attestations). Each Bundle carries its canonical JSON serialization (Raw) so consumers can store it durably and re-verify later. Returns ErrNoBundles for unsigned artifacts.
  • VerifyBundle(b, trustedMaterial, expected *Identity, opts...) — when expected is non-nil, the signer identity is bound into the Sigstore verification policy itself (verify.WithCertificateIdentity) rather than compared post-hoc in Go; nil expected is the trust-on-first-use case (chain-of-trust only, caller records the identity from the result). GitHub-Actions-derived identities match the SAN via a repo-anchored prefix.
  • VerifyBundleOffline(raw, digestAlgo, digestHex, expected) — genuinely offline re-verification of a stored bundle against an embedded public-good trusted_root.json (new OfflineTrustedMaterial()): no TUF refresh, no network. The embedded root is a point-in-time snapshot, documented as such; live-freshness callers keep using New's TUF fetch.
  • VerifyBundleWithKey(b, pubKeyPEM) — the cosign key-pair flow: key-signed bundles carry no certificate or transparency material, so this uses PublicKeyMaterial + WithNoObserverTimestamps + a WithKey policy, kept as an explicit entry point rather than auto-detection.
  • Identity / IdentityFromResult — exports the signer-identity extraction (including the GitHub Actions issuer normalization in signerIdentityFromCertificate).

Existing exported surface (New, VerifyServer, GetVerificationResults) is unchanged. github.com/sigstore/sigstore moves from indirect to direct (cryptoutils/signature used by the key-material path).

Fixes #176

Test plan

  • Key-signed bundle round trip: sign with an ephemeral key via sigstore-go → verify through VerifyBundleWithKey (this is the contract ToolHive's signing E2E will rely on); wrong-digest and wrong-key rejections
  • OfflineTrustedMaterial parses the embedded root and carries Fulcio CAs + Rekor logs
  • VerifyBundleOffline round-trips stored Raw bundles (failure at verification, not parsing) and rejects malformed input
  • Identity policy construction for plain SAN, GitHub-Actions repo-anchored, nil (TOFU), and empty (rejected) identities; an expected identity fails closed against a certificate-less bundle
  • go test ./container/... and task lint green

🤖 Generated with Claude Code

ToolHive's skills lock-file feature (RFC THV-0080, Stack 2) needs to
verify skill artifacts on install and re-verify stored bundles on
sync without duplicating this package's Sigstore internals. Promote
the existing machinery to a public API: RetrieveBundles surfaces the
bundles from both supported layouts (cosign .sig-tag manifests and
attestations) with their canonical serialization for durable
storage; VerifyBundle binds an expected signer identity into the
verification policy itself rather than comparing after the fact,
with nil meaning trust-on-first-use; VerifyBundleOffline re-verifies
a stored bundle against an embedded public-good trusted root with no
network access; VerifyBundleWithKey covers the cosign key-pair flow,
whose bundles carry no certificate or transparency material; and
Identity/IdentityFromResult export the signer-identity extraction
including the GitHub Actions issuer normalization.

The embedded trusted_root.json is a point-in-time snapshot of the
public-good instance, documented as such — callers needing live
freshness keep using New's TUF fetch.

Fixes #176
@JAORMX

JAORMX commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Multi-axis panel review, verified against sigstore-go v1.2.2 source (module cache) and its docs. Posting findings; happy to discuss any.

✅ Verified correct against the sigstore-go API

These were checked directly in sigstore-go@v1.2.2 source, not assumed:

  • Identity binding is genuinely in-policy, not post-hoc. WithCertificateIdentity appends to policy.certificateIdentities (pkg/verify/signed_entity.go:433), and Verify() calls policy.certificateIdentities.Verify(certSummary) inside the flow (:802), setting result.VerifiedIdentity. The GitHub-Actions SAN regex is anchored both ends, case-sensitive, and QuoteMeta'd — no prefix/substring bypass.
  • WithKeyWithCertificateIdentity are mutually exclusive (:438,:452), and Verify() rejects cert-bearing bundles under WithKey ("expected key signature, not certificate", :632). Key-pair path can't be confused with the cert path.
  • Offline verification still verifies the tlog cryptographicallyVerifyTlogEntry (pkg/verify/tlog.go:48) verifies the SET + inclusion proof against the embedded Rekor keys, no network, and enforces MaxAllowedTlogEntries / rejects duplicate entries. Offline only skips freshness (live STH), matching upstream's offline model.
  • The embedded trusted_root.json is justified, not reinvention. sigstore-go embeds only the TUF bootstrap root.json (to start a refresh), not the trusted_root.json target; its own verification example (examples/sigstore-go-verification/main.go:167) loads exactly this file via root.NewTrustedRootFromJSON. Vendoring the target is the canonical offline pattern.
  • Backward compat: existing exported surface (New/VerifyServer/GetVerificationResults) is byte-identical; the only sigstore.go change is "sha256"DigestAlgorithmSHA256 (improvement).
  • Duplication: lean — correctly reuses getSigstoreBundles/signerIdentityFromCertificate/verifierOptions; Rule-of-Three not triggered on the Verify* skeletons (two genuinely different trust models).

🔴 Ship-blocker

Panic on attacker-controlled OCI annotations — now on a public trust boundary. container/verifier/sigstore.go:218,223,231,256-257getVerificationMaterialTlogEntries uses single-value type assertions on the registry-supplied dev.sigstore.cosign/bundle annotation (jsonData["Payload"].(map[string]interface{}), jsonData["apiVersion"].(string), etc.). A malformed annotation panics rather than errors. Pre-existing, but RetrieveBundles (bundles.go:83) now exposes it publicly → a registry/manifest attacker can crash the verifier process (DoS, CWE-248). Fix: two-value ok assertions returning the existing error path.

🟠 Spec mismatch (vs #176)

The PR ships a superset API whose shape doesn't match the three requested signatures, so the toolhive-side thin wrapper can't be written against the spec:

  1. RetrieveBundles drops the spec'd ctx context.Context param (bundles.go:83) — no caller-side cancellation of registry fetches.
  2. VerifyBundleOffline is a package-level func with split digestAlgo/digestHex + an added expected param, not the spec'd (s *Sigstore) VerifyBundleOffline(bundleBytes, artifactDigest).
  3. IdentityFromResult returns *Identity (spec: Identity value).
  4. Out-of-scope surface: VerifyBundle implements the "match a registry-declared publisher at first install" semantics Export Sigstore bundle retrieval, offline verification, and identity extraction from container/verifier #176 lists verbatim as OUT of scope; VerifyBundleWithKey + PublicKeyMaterial (key-pair flow) aren't requested either. (Defensible iff a real dockyard/registry key-signing consumer exists — please confirm; otherwise cut to the three symbols.)

🟡 Corrected finding (downgraded)

Bundle.Raw at bundles.go:97 uses json.Marshal(b.bundle). A first pass flagged this as not canonical protojson — that was wrong: sigstore-go's bundle.Bundle.MarshalJSON() is protojson.Marshal (pkg/bundle/bundle.go:239), and json.Marshal dispatches to it, so the bytes are canonical. Only a clarity nit: call b.bundle.MarshalJSON() explicitly so it doesn't rely on the dispatch.

🟡 Confirmed footgun

VerifyBundle default-opts hardcode the public-good instance (bundles.go:170-176). verifierOptions(PublicGood) requires SCTs; the GitHub instance does not. A caller passing GitHub trust material while omitting verifierOpts gets public-good opts fed to the wrong root → confusing sigstore-go internal error (e.g. "SCTs required but bundle is signed with a public key") instead of a clear opts/material mismatch. Either derive the default from tm, or require explicit opts on the general VerifyBundle and keep the default only on VerifyBundleOffline where the instance is known.

🟡 Lifecycle gap

Frozen trusted_root.json has no provenance / freshness signal. If a Fulcio CA or Rekor key in the snapshot rotates out due to compromise, all consumers keep trusting it until a release + bump. Since sigstore-go offers no helper for the target file, keeping the embedded file is right — but please (a) record its provenance (source TUF repo + fetch date/commit), (b) add a CI/task drift-check against a freshly-fetched root, and (c) document the compromise-rotation case on OfflineTrustedMaterial, not just the "can't verify newly-rotated keys" case.

🔵 Lower-priority

  • Missing doc.go / stability declaration. container/verifier has no # Stability section and isn't in the CLAUDE.md package table, despite 11 new exported symbols. Every other Alpha/Beta package declares it — please add a doc.go marking it Alpha and add it to the table. (This is also the cover for the leaky-abstraction trade-off below.)
  • Leaky sigstore-go types on the happy path. Bundle.Parsed *bundle.Bundle (bundles.go:37) and *verify.VerificationResult returned by all VerifyBundle* force every consumer to import sigstore-go — which the Identity/Raw abstractions otherwise aim to avoid. Acceptable under Alpha, but consider an Identity-returning convenience (or an Example* showing retrieve → store Raw → re-verify offline → IdentityFromResult) so the common path doesn't require sigstore-go imports.
  • Error-chain drop: RetrieveBundles converts ErrProvenanceNotFoundOrIncompleteErrNoBundles and drops the chain (bundles.go:85-86), breaking errors.Is(…, ErrProvenanceNotFoundOrIncomplete) for callers. Consider wrapping instead.
  • No sentinel for "verification failed" — only ErrNoBundles is branchable; everything else is sigstore-go's (unstable) error strings. A typed ErrVerificationFailed would let consumers distinguish "signed but failed" from "malformed input".
  • go.mod dep growth is undiscussed. The sigstore/sigstore indirect→direct promotion is mechanically required by PublicKeyMaterial (cryptoutils/signature), but it drags ~12 new indirect deps (cobra, pflag, go-tuf, pkg/browser, …). CLAUDE.md treats go.mod changes as needing discussion — worth a note in the PR body.
  • Unbounded io.ReadAll on the attestation referrer layer (attestations.go:119) — no size cap (the sig-manifest path caps at MaxAttestationsBytesLimit).

Net: the security core is sound and correctly uses sigstore-go. Before merge I'd want the annotation panic fixed and the #176 signature alignment + scope question resolved; the rest is hardening/ergonomics. Nice work on the in-policy identity binding and the clean reuse of the existing internals.

Harden and align the exported bundle API from review feedback:

- Fix panics on attacker-controlled OCI annotations: every type
  assertion in getVerificationMaterialTlogEntries is now two-valued,
  returning an error for malformed registry-supplied bundle
  annotations instead of crashing the process.
- Align with the #176 spec: RetrieveBundles takes a context (threaded
  through all registry fetches), IdentityFromResult returns an
  Identity value, and VerifyBundleOffline takes the combined
  "<algorithm>:<hex>" artifact digest.
- Require explicit verifier options in VerifyBundle so public-good
  defaults are never silently applied to mismatched trusted material;
  DefaultVerifierOptions() exposes the public-good set and
  VerifyBundleOffline keeps applying it (instance is known there).
- Wrap verification failures in a branchable ErrVerificationFailed
  sentinel and preserve the error chain when mapping to ErrNoBundles.
- Serialize Bundle.Raw via the bundle's own MarshalJSON explicitly.
- Cap the attestation referrer layer read at
  MaxAttestationsBytesLimit, matching the signature-manifest path.
- Document the embedded trusted root's provenance and
  compromise-rotation staleness (tufroots/README.md, doc comment),
  add package doc.go with an Alpha stability marker, and list
  container/verifier in the CLAUDE.md package table.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@samuv

samuv commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough panel review — all actionable items addressed in 5127935. Point-by-point:

🔴 Ship-blocker — fixed

Every type assertion in getVerificationMaterialTlogEntries is now two-valued (the Payload map is extracted once with an ok check; apiVersion/kind likewise), returning the existing error path on malformed shapes. Added sigstore_test.go with a malformed-annotation matrix (require.NotPanics on 10 shapes that previously hit single-value assertions, plus a well-formed control case).

🟠 Spec mismatch — aligned where it helps, justified where it deviates

  1. ctx: RetrieveBundles(ctx, imageRef, keychain) now threads context through the entire retrieval chain (remote.WithContext/crane.WithContext on every registry fetch). The pre-existing exported GetVerificationResults/VerifyServer keep their signatures (backward compat) and pass context.Background() internally, with a comment pointing at RetrieveBundles as the cancellable entry point.
  2. VerifyBundleOffline now takes the combined artifactDigest string ("sha256:<hex>") per spec. It stays package-level rather than a *Sigstore method deliberately: Sigstore is the online TUF-refreshed verifier, and hanging an offline function off it would imply the instance's live root is involved when it isn't. The expected *Identity param is the one intentional superset over Export Sigstore bundle retrieval, offline verification, and identity extraction from container/verifier #176 — it's what makes identity pinning enforced inside the sigstore policy (WithCertificateIdentity) rather than post-hoc, which your review verified as the right mechanism.
  3. IdentityFromResult returns an Identity value now.
  4. Scope: real consumers exist for the key flow — Add Sigstore signer package for skill OCI artifacts toolhive#6023 (merged) implements cosign key-pair signing for skills, and the RFC THV-0080 install-verification work consumes VerifyBundleWithKey for the signed round-trip and VerifyBundle+expected for TOFU enforcement of lock-recorded identities (Track: Skills lock file + Sigstore signing (RFC THV-0080) toolhive#5899). Happy to link the RFC section if useful.

🟡 Footgun — resolved by requiring explicit opts

VerifyBundle now errors when no verifier options are passed, with a message pointing at the two valid pairings; DefaultVerifierOptions() exposes the public-good set for callers using OfflineTrustedMaterial/the live root, and VerifyBundleOffline keeps applying it internally since the instance is known there. Guarded by a test.

🟡 Lifecycle — documented; drift-check proposed as follow-up

Added tufroots/README.md recording provenance (fetched 2026-07-24 from tuf-repo-cdn.sigstore.dev via sigstore-go's TUF client, which verifies the target against the TUF metadata chain) and the compromise-rotation staleness in both directions; OfflineTrustedMaterial's doc comment now spells out the compromise case explicitly. A CI drift-check needs network in CI and a refresh policy decision, so I'd prefer to do it as a follow-up issue rather than grow this PR — will file it on merge unless you want it in here.

🔵 Lower-priority — all taken except one

  • doc.go added (Alpha, with a retrieve → verify → store → re-verify-offline example in the package doc) + CLAUDE.md table row.
  • Bundle.Raw now calls b.bundle.MarshalJSON() explicitly.
  • ErrNoBundles mapping now wraps (%w: %w), preserving errors.Is(…, ErrProvenanceNotFoundOrIncomplete).
  • ErrVerificationFailed sentinel added, wrapping all Verify failures in VerifyBundle/VerifyBundleWithKey (and hence VerifyBundleOffline); tests assert errors.Is both positively and negatively (malformed input is not a verification failure).
  • Attestation referrer layer read is now capped at MaxAttestationsBytesLimit, matching the sig-manifest path.
  • go.mod: noted — the sigstore/sigstore direct promotion is mechanically forced by PublicKeyMaterial (cryptoutils/signature imports); all of it was already in the module graph via sigstore-go, so no new module versions enter the tree, only direct/indirect reclassification.
  • Leaky *bundle.Bundle/*verify.VerificationResult on the happy path: acknowledged as an Alpha trade-off; the package doc now shows the common path needing only Raw + Identity. If a second consumer appears I'd wrap the result type before Beta.

@samuv
samuv merged commit 6b57b0d into main Jul 28, 2026
5 checks passed
@samuv
samuv deleted the export-sigstore-verifier-api branch July 28, 2026 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Export Sigstore bundle retrieval, offline verification, and identity extraction from container/verifier

2 participants