feat(REQ-27): from_sigstore_bundle — ingest cosign bundles + fix non-conformant emitter (#260) - #264
Open
avrabe wants to merge 2 commits into
Open
feat(REQ-27): from_sigstore_bundle — ingest cosign bundles + fix non-conformant emitter (#260)#264avrabe wants to merge 2 commits into
avrabe wants to merge 2 commits into
Conversation
…260) varve (#260) already produces cosign bundles (keyless, GitHub-OIDC) but wsc could only verify artifacts it signed itself — SigstoreBundle can emit wsc signatures but nothing converts an existing cosign bundle back into a KeylessSignature. Adds KeylessSignature::from_sigstore_bundle(json) parsing BOTH wire shapes: - Legacy `rekorBundle` JSON (cosign v2.4.x — what varve's v0.28.0 ships): {base64Signature, cert, rekorBundle:{SignedEntryTimestamp, Payload}}. - Protobuf `bundle.sigstore.dev/v0.3+json` envelope (verificationMaterial + messageSignature + tlogEntries). Faithful extraction (verified against two REAL fixtures committed here): - module_hash is read from the hashedrekord body's spec.data.hash.value, never recomputed; the negative-control test flips one hex char and asserts the extracted hash changes. - integratedTime (unix int) -> RFC3339, the form RekorEntry documents and verify_cert_chain parses (confirmed: cert-chain + body-binding both accept the ingested varve bundle). - v0.3 requires a Fulcio certificate; a raw-public-key (non-keyless) bundle is rejected with a specific error, proven on a real cosign v0.3 bundle. Round-trip fidelity test: from_sigstore_bundle -> from_keyless_signature -> to_json -> from_json preserves signature, module_hash, cert chain and rekor fields (uuid/inclusion_proof intentionally empty for legacy — documented). KNOWN LIMITATION → REQ-28 (#231, the verify half): cosign emits ECDSA signatures in ASN.1 DER (varve's is 71 bytes, 3045…), but the offline verifier's verify_crypto uses P256Signature::from_slice (fixed 64-byte P1363), so it currently rejects an ingested DER signature. Making the verifier accept DER (from_der fallback) and handling the empty Rekor uuid on the offline path is REQ-28's scope — that is where "verify an ingested cosign bundle offline" completes. from_sigstore_bundle here is the faithful ingestion half. Fixtures: legacy = varve v0.28.0 public release; v0.3 = real cosign --new-bundle-format output (see fixtures README). Tests: wsc lib 610 pass/3 ignored; sigstore_bundle 7 pass. Refs: #260, #231 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012aR3Md1h46K9wAUWMQiESH
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ap (#260) Two parts: a real interop DEFECT FIX found by a round-trip test, and the coverage gap that was hiding it. ## The coverage gap (codecov/patch was failing: 164 of 397 new lines uncovered) from_v03_bundle was almost entirely unexercised: the only v0.3 fixture is a LOCAL-KEY bundle, so parsing bailed at the "requires a Fulcio certificate" check and the whole v0.3 happy path never ran. We claimed "supports both wire shapes" while only one shape's happy path was tested — the vacuous-oracle shape REQ-30/#258 exists to kill, and exactly what varve warned about in #260 ("supporting only one will surprise someone"). Fixed by building a genuine cert-bearing v0.3 bundle from REAL material: legacy fixture -> from_sigstore_bundle -> SigstoreBundle::from_keyless_signature -> to_json -> from_sigstore_bundle again, asserting field-by-field fidelity. Plus a spec-shaped keyless positive (the singular certificate.rawBytes branch real cosign keyless bundles use), a v0.3 negative control, and 43 error-path unit tests each asserting a specific message. Added lines uncovered: 164 -> 4, and those 4 are provably unreachable (a map_err closure guarded by an is_ascii_hexdigit + length check, and two test-helper panic arms). ## The defects that round-trip test found (emitter was non-conformant) Ground truth: a real `cosign sign-blob --new-bundle-format` bundle emits `logId.keyId` as BASE64 and places the SET under `inclusionPromise`, with no top-level field. 1. logId.keyId encoding. The emitter wrote RekorEntry::log_id (hex, the Rekor REST form) straight into LogId.key_id, which the Sigstore protobuf spec types as `bytes` — base64 in JSON. A 64-char hex string is ALSO valid base64, so it did not error: it decoded to 48 junk bytes, corrupting the Rekor log identity with no diagnostic (c0d23d6a…801d -> 734776dd…7dce). Fixed: transcode hex -> base64; a non-hex value passes through unchanged rather than emitting mangled base64. 2. SET placement. The emitter wrote the SET at the top-level tlogEntries[].signedEntryTimestamp; every other implementation (and wsc's own ingest) reads inclusionPromise.signedEntryTimestamp. The SET — the only offline transparency proof a legacy bundle carries — was silently dropped on wsc's own round trip. Fixed: emit the spec location via a new InclusionPromise type. Both mean bundle.rs's documented claim that emitted bundles verify with `cosign verify-blob --bundle` was false. Backward compatible on read: the legacy top-level SET is still accepted (deserialize-only field + a signed_entry_timestamp() accessor), with a test proving pre-0.11.0 bundles still round-trip. The two KNOWN-DEFECT assertions that pinned the buggy behaviour are replaced with true losslessness assertions, so the round-trip test now proves fidelity rather than documenting corruption. Tests: wsc lib 655 pass/3 ignored (+45); sigstore_bundle 10 pass (+3). Vacuous-oracle gate clean. Clippy clean. Refs: #260, #231 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012aR3Md1h46K9wAUWMQiESH
avrabe
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the ingestion half of #260 — and, via a round-trip test, found and fixed two real interop defects in wsc's bundle emitter.
1. Ingestion —
KeylessSignature::from_sigstore_bundle(json)Parses both wire shapes: legacy
rekorBundleJSON (cosign v2.4.x — varve's v0.28.0 shape) and protobufbundle.sigstore.dev/v0.3+json.Verified against two real fixtures (committed): varve's actual v0.28.0 public bundle, and real
cosign --new-bundle-formatoutput.module_hashread from the hashedrekord body, never recomputed — negative controls (legacy and v0.3) flip a hex char and assert the extracted digest changes.verify_rekor_body_binds_to_bundle()+verify_cert_chain().2.⚠️ Emitter defects found by the round-trip test — now fixed
Ground truth: a real
cosign sign-blob --new-bundle-formatbundle emitslogId.keyIdas base64 and puts the SET underinclusionPromise, with no top-level field.LogId.key_idwritten as hex; spec types itbytes(base64 in JSON)c0d23d6a…801d→734776dd…7dce)signedEntryTimestampinclusionPromise.signedEntryTimestamp— the SET, the only offline transparency proof a legacy bundle carries, was silently dropped on wsc's own round tripBoth mean
bundle.rs's documented claim that emitted bundles verify withcosign verify-blob --bundlewas false. Fixed; backward compatible on read (legacy top-level SET still accepted via a deserialize-only field + accessor, with a test proving pre-0.11.0 bundles still round-trip).3. Coverage gap that was hiding it
codecov/patchwas failing: 164 of 397 new lines uncovered. Cause: the only v0.3 fixture is local-key, so parsing bailed at the cert check and the entire v0.3 happy path never ran — we claimed "both wire shapes" while testing one. Exactly the vacuous-oracle shape REQ-30/#258 kills, and what varve warned about.Fixed with a genuine cert-bearing v0.3 round-trip built from real material, a spec-shaped keyless positive (the
certificate.rawBytesbranch real cosign uses), and 43 error-path unit tests each asserting a specific message. Uncovered added lines: 164 → 4, all four provably unreachable (amap_errclosure guarded byis_ascii_hexdigit+ length, and two test-helper panic arms).Remaining limitation → REQ-28 (#231)
cosign emits ECDSA signatures in ASN.1 DER (varve's is 71 bytes,
3045…);verify_cryptousesP256Signature::from_slice(64-byte P1363), so an ingested DER signature is still rejected. DER acceptance +SigstoreBundle::verifyis REQ-28's scope — no test here claims otherwise.Evidence
wsclib 655 pass/3 ignored;sigstore_bundle10 pass; vacuous-oracle gate clean; clippy clean.Part of the v0.12.0 varve-interop line (plan #261). Refs #260, #231.
🤖 Generated with Claude Code