Reconstruct bundles for key-signed cosign signatures

[samuv]

Summary

Follow-up to #192. RetrieveBundles reported classic key-signed cosign signatures (cosign sign --key) as unsigned: bundle reconstruction required the Fulcio certificate and tlog-entry annotations on the simple-signing layer, which only keyless signatures carry. The key-signed layout attaches just the signature annotation — trust is established by the verifier supplying the public key, so there is no certificate or transparency-log material on the manifest by design.

Key-signed layers (signature annotation present, certificate absent) now build public-key verification material, making them retrievable and verifiable through VerifyBundleWithKey. Certificate-bearing layers keep the existing strict requirements, and a reconstructed key-signed bundle still fails keyless (Fulcio) verification — nothing certificate-less can sneak through that path (covered by a test).

Needed by ToolHive's skill-signing flow (stacklok/toolhive#5899), which signs skill artifacts with cosign keys (stacklok/toolhive#6023) and verifies them at install time via this package. Would be great to include in the same release as #192.

Testing

• TestRetrieveKeySignedBundleRoundTrip: pushes a key-signed artifact (payload layer + signature annotation at the sha256-<hex>.sig tag) to an in-process registry, retrieves the bundle, verifies it against the signing key, and rejects a different key.
• TestKeySignedBundleFailsKeylessVerification: the reconstructed bundle must not pass the Fulcio path.
• Full package tests and lint green.

[JAORMX]

Panel review — PR #197 (key-signed bundle reconstruction)

Fixed point: 6b57b0d (merge-base with main, i.e. #192) · Diff: 2 files, +150/−2, 1 commit
Axes: Spec (PR body + RFC THV-0080 trust model + toolhive#6023 contract + #192 API contract) · Standards (CLAUDE.md + package conventions) · Domain (secure-code-reviewer, software-architect, library-reuse-reviewer, code-duplication-reviewer, devex-reviewer)
go test ./container/... green on the PR head. Axes are reported separately by design — they ask different questions and don't mask each other.

---

Spec — does this implement what was asked?

The PR delivers exactly what its body claims: key-signed layers become retrievable and verifiable via VerifyBundleWithKey, certificate-bearing layers keep strict requirements, and the keyless path still rejects certificate-less bundles (pinned by TestKeySignedBundleFailsKeylessVerification). No scope creep.

• [Missing] Offline re-verification of a stored key-signed bundle. RFC THV-0080: "Sigstore bundles are stored alongside local install state. sync --check … re-verifies the stored bundle + locked identity offline — no network." VerifyBundleOffline (bundles.go:263) hardcodes OfflineTrustedMaterial() + DefaultVerifierOptions() (Fulcio + tlog requirements), so a stored key-signed Raw always fails there — there is no VerifyBundleOfflineWithKey. The PR body scopes itself to online VerifyBundleWithKey, so this is a partial implementation of the broader spec, not a regression — but the root cause (no key-path offline entry point) is unfixed, and the toolhive signer flow this unblocks is exactly the store-then-reverify-offline consumer.
• [Missing, minor] No test asserts the retrieved bundle's MessageDigest binds the simple-signing payload digest — the binding currently holds only because DigestHex and MessageDigest both derive from layer.Digest.Hex (sigstore.go:80,324). The coincidence is untested.

Standards — does this follow project conventions?

No hard violations. Error wrapping matches the package (%w), the new helper has godoc, no exported signatures changed, no tests removed, go.mod/go.sum untouched, and the new code path is exercised by both new tests.

• [Judgement] CLAUDE.md asks for table-driven tests for new functions; hasCosignSignatureAnnotation is only covered indirectly via the two round-trip tests. Defensible for a one-line boolean — the integration tests cover both branches of the modified getBundleVerificationMaterial — noted per convention.
• [Judgement] The expanded step-1 comment block (sigstore.go:169–174) deviates from the file's terse single-line numbered-step style, but documents a non-obvious fallback; acceptable.

Domain — what do the specialists say?

Ship-blockers

1. [High] Corrupt-certificate layers are silently reclassified as key-signed — sigstore.go:175–185. getVerificationMaterialX509CertificateChain returns the same "failed to decode PEM block" error for annotation absent (legitimate key-signed layout) and annotation present but PEM-corrupt (tampered/malformed keyless layer). The fallback fires on both whenever a signature annotation exists, so a corrupt-cert keyless layer — previously a retrieval error — now becomes a "key-signed" bundle with an empty PublicKeyIdentifier. The annotations are attacker-controlled registry data (the file's own comments say so at sigstore.go:239–241). Fix: discriminate on annotation presence, not on the decoder error — check Annotations["dev.sigstore.cosign/certificate"] == "" first and only then fall to the public-key branch; a present-but-undecodable cert must stay an error. Add a test: garbage cert annotation + signature annotation → retrieval must not yield a bundle. Sources: software-architect, echoed by secure-code-reviewer's trust-boundary read. Cross-confirmed.

2. [High] The offline re-verify contract is unmet for exactly the bundles this PR makes storable — and the docs currently promise it — bundles.go:42 (Raw is "suitable for durable storage and later re-verification with VerifyBundleOffline"), bundles.go:258–262, doc.go:12–30. The documented store-Raw-then-VerifyBundleOffline narrative is false for key-signed bundles (Fulcio material + tlog requirement hardcoded). A consumer doing what the docs say gets ErrVerificationFailed with no way to supply a key. Fix (either): add VerifyBundleOfflineWithKey(rawBundle, artifactDigest, pubKeyPEM) (~15 lines mirroring VerifyBundleWithKey, and restores symmetry with the VerifyBundle/VerifyBundleWithKey pair) — preferred given the stated skills-flow consumer — or scope the doc comments on Bundle.Raw, VerifyBundleOffline, and doc.go to keyless-only and track the key variant as a follow-up. Sources: software-architect, devex-reviewer, Spec axis. Cross-confirmed — the panel's most important issue.

Judgement calls (discuss, not necessarily blocking)

3. [Medium] Layout classification lives one level too deep — "certificate absent && signature present ⇒ key-signed" is embedded inside the material extractor getBundleVerificationMaterial, while the signature annotation is independently re-read by getBundleMsgSignature (sigstore.go:329). Lifting classification into bundleFromSigstoreSignedImage's loop makes the corrupt-cert fix natural and the two-layout design explicit. Fine to defer if finding 1 is fixed in place. Source: software-architect.

4. [Medium] Stored key-signed bundles are not self-describing — reconstructed bundles pin version=0.1 media type with an empty PublicKeyIdentifier{} as an implicit "this is key-signed" sentinel; the attestation path stores v0.3. A reader of stored Raw can't recover the layout, and offline re-verify requires out-of-band key knowledge. Cheap improvement: populate PublicKeyIdentifier.Hint (e.g. "cosign-keypair") + document the sentinel and the two media types on Bundle/RetrieveBundles. Related boundary worth a comment: cosign v3 bundle-format layers are silently dropped by the simple-signing media-type filter (sigstore.go:156). Source: software-architect.

5. [Medium] Bundle gives no keyless/key-signed discrimination — a consumer that doesn't already know the signing flow must try VerifyBundle then VerifyBundleWithKey and interpret opaque errors. For the paired toolhive signer/verifier use case this is fine; for library generality a HasCertificate() helper or doc guidance would close it. Also: ErrNoBundles/RetrieveBundles doc comments still read as if bundles imply keyless — update to say key-signed layouts are now returned. Source: devex-reviewer.

Mechanical fixes

6. [Low] Strengthen the negative test's assertion — keysigned_test.go:129: require.Error → assert.ErrorIs(t, err, ErrVerificationFailed), matching the package's public error contract (bundles_test.go:127 already does this). Sources: devex-reviewer, secure-code-reviewer.
7. [Low] Log the layer digest + error on per-layer extraction failure — sigstore.go:56,63: slog.Error("error getting bundle verification material") drops the err and which layer failed; with the new reclassification path, operators need that breadcrumb. Source: software-architect.
8. [Info] Drop testCosignSimpleSigningMediaType — keysigned_test.go:31 duplicates exported MediaTypeCosignSimpleSigningV1JSON (utils.go:44) in the same package. Sources: library-reuse-reviewer, code-duplication-reviewer (as Info), Standards.
9. [Info] v1.NewHash(imgDigest.String()) round-trip — keysigned_test.go:74–76 re-parses a v1.Hash whose .Algorithm/.Hex fields are directly available; the error check is dead. Source: library-reuse-reviewer.

Clean bill

• Security: no downgrade path — a public-key-material bundle with zero tlog entries cannot satisfy WithTransparencyLog(1), so the Fulcio path rejects it before signature verification (proven end-to-end by the new test). The empty PublicKeyIdentifier is purely informational: PublicKeyMaterial's callback ignores the hint. No new panic/DoS surface. Source: secure-code-reviewer.
• Duplication/reuse: production change is minimal and correctly shaped for sigstore-go; no extraction warranted anywhere (net ~4 lines of test polish only).

Gaps / follow-ups beyond this PR

• [Info, pre-existing — file separately] The package never parses the simple-signing payload's critical.image.docker-manifest-digest; bundle MessageDigest and the policy artifact digest both come from registry-supplied layer.Digest.Hex, making sigstore-go's digest cross-check tautological for both keyless and key-signed flows (CWE-354 / OWASP A08:2021). Exploitation needs registry compromise; not introduced or worsened here, but worth a tracking issue since this package is becoming the skills trust anchor.

---

Summary

• Spec: 2 findings (offline-with-key gap against the RFC; untested digest-binding coincidence)
• Standards: clean, 2 judgement notes
• Domain: 2 ship-blockers (corrupt-cert reclassification; offline/docs contract gap), 3 judgement calls, 4 mechanical/info fixes

Most important single issue: finding 2 — the stored-bundle story this PR enables (and the docs promise) only works for keyless; either ship VerifyBundleOfflineWithKey or narrow the docs before the skills flow builds on it. Finding 1 (corrupt-cert reclassification) is a small, sharp correctness fix that should land with it.

Each axis is orthogonal — verify each independently before shipping.

[samuv]

All findings addressed in 71d805a — point-by-point:

Ship-blockers

1. Corrupt-cert reclassification — fixed as prescribed. Classification now discriminates on certificate-annotation presence: an empty annotation with a signature annotation is the key-signed layout; a present-but-undecodable certificate stays a hard error. TestCorruptCertificateIsNotReclassifiedAsKeySigned pushes exactly the attack shape (garbage cert + valid signature annotation) and asserts retrieval yields ErrNoBundles, not a bundle.
2. Offline contract — took the preferred option. Added VerifyBundleOfflineWithKey(rawBundle, artifactDigest, pubKeyPEM) (parse + delegate to VerifyBundleWithKey, restoring the pair symmetry), and the round-trip test now stores Raw and re-verifies it offline with the key — plus wrong-key rejection through the same entry point. Docs on Bundle.Raw and the package doc now route each layout to its verify entry point instead of promising VerifyBundleOffline universally.

Judgement calls

3. Classification depth — deferred as you allowed, since finding 1 is fixed in place; noted for a future refactor if a third layout ever appears.
4. Self-describing stored bundles — reconstructed key-signed bundles now carry PublicKeyIdentifier.Hint = "cosign-keypair" (documented const), so stored Raw bytes identify their layout.
5. Discrimination helper — added Bundle.HasCertificate() with doc guidance mapping true→VerifyBundle, false→VerifyBundleWithKey/VerifyBundleOfflineWithKey; ErrNoBundles doc updated to enumerate all three layouts.

Mechanical

6. Negative test asserts ErrVerificationFailed (both keyless-rejection and wrong-key offline).
7. Per-layer slog.Error calls now carry layer_digest and error.
8. testCosignSimpleSigningMediaType dropped in favor of the exported MediaTypeCosignSimpleSigningV1JSON.
9. v1.NewHash round-trip removed — the tag is built from imgDigest.Algorithm/.Hex directly.

Spec-axis minors

• The round-trip test now asserts bundles[0].DigestHex equals the simple-signing payload's SHA-256 — the digest binding is no longer an untested coincidence.
• The offline-with-key gap is the blocker fix above.

Follow-up filed

• verifier: simple-signing payload digest binding is not independently validated #198 tracks the pre-existing tautological docker-manifest-digest cross-check you flagged for separate filing.

Package tests and lint green on the new head.