feat(jans-cedarling): implement Sigstore/Cosign verification library - #14636
feat(jans-cedarling): implement Sigstore/Cosign verification library #14636olehbozhok wants to merge 60 commits into
Conversation
… tests Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
- Rename verify_ecdsa_p256_raw → verify_ecdsa_p256_prehashed, use PrehashVerifier to avoid double-hashing - Split APIs: verify_ecdsa_p256 for raw messages, verify_ecdsa_p256_prehashed for pre-digested - Fix SET verification: pass base64 string (what Rekor signs), not parsed JSON object - Update all callsites in chain.rs, sct.rs, tlog.rs, verifier.rs - Improve test assertions to verify specific error types, not just is_err() - Add PEM fallback for certificate encoding in tlog verification Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…certificate TBS reconstruction - Parse SCT list from leaf cert x.509 extension (OID 1.3.6.1.4.1.11129.2.4.2) - Reconstruct precertificate TBS by removing SCT extension from cert DER - Compute issuer_key_hash = SHA-256(issuer SPKI) - Verify SCT signatures against CTFE keys per RFC 6962 §3.2 - Add minimal DER TLV encoder/decoder for extension removal - Export SPKI from cert.rs for issuer key hashing - Update verifier to pass issuer cert to SCT verification - Add comprehensive unit tests with synthetic CTFE keys Previously non-functional stub now fully implemented and unit-tested. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…entry validation - merkle.rs: RFC 6962 §2.1 Merkle tree leaf hash & inclusion proof verification - tlog.rs: TLOG entry parsing, consistency checking, CVE-2022-36056 mitigation - verifier.rs: Integrate Merkle + TLOG checks into 9-step verification flow - tests/conformance_scan.rs: End-to-end fixture validation against real bundles - tests/real_bundle.rs: Real Fulcio+Rekor bundle verification (payload binding, timestamp anchoring) - Fixture files: Inclusion proofs, corrupted hashes, invalid checksums for negative tests - ARCHITECTURE.md: Update status matrix — Merkle/TLOG complete, DSSE binding pending Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…ror handling - chain.rs: Simplify path validation logic, improve error messages - cert.rs: Enhanced certificate constraint checking - tlog.rs: Better entry validation and edge case handling - bundle.rs: Streamline bundle parsing logic - verifier.rs: Tighten error propagation - ARCHITECTURE.md: Update module descriptions Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…tfeKey.key_id, tighten bundle/tlog validation Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…ltering, and SCT logID checks Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…Result propagation Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Replace assert!(is_err()) with expect_err(), bare assert! with messages, panic! with expect_err, and bare assert!(matches!()) with messages. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
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:
📝 WalkthroughWalkthroughAdds ChangesSigstore verifier implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
… functions Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…ension test Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…_value Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…t error Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…og DSSE body Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…arison Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…reuse in SCT Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…ure verification Dispatch to P-256 or P-384 verifier based on the leaf certificate's public key length instead of always using P-256. Also refactors validate_chain to return the leaf's verified issuer, removing the duplicate DN-based issuer lookup in the SCT verification step. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…-line test Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
jans-cedarling/sigstore-verifier/src/crypto.rs (1)
81-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the P-384 verification path.
The tests shown below cover P-256 only. Add valid and invalid P-384 cases, including DER and raw signature encodings, because this path is used for certificate-chain verification.
🤖 Prompt for 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. In `@jans-cedarling/sigstore-verifier/src/crypto.rs` around lines 81 - 115, Add regression tests for verify_ecdsa_p384_prehashed covering successful and failing verification with valid P-384 keys and SHA-384 prehashes, exercising both DER and raw r||s signature encodings. Include invalid-signature cases and assert the expected SigstoreVerificationError behavior, matching the existing P-256 test style.jans-cedarling/sigstore-verifier/src/tlog.rs (1)
556-567: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA single unparseable Rekor key aborts checkpoint verification entirely.
crate::crypto::p256_key_id(key)?inside thefor key in rekor_keysloop propagates on the first key that fails key-ID derivation, instead of skipping it and trying the remaining keys. If the trust root holds multiple Rekor keys (key rotation, per PR objectives) and any one of them isn't a valid 65-byte P-256 SEC1 point, checkpoint verification fails outright even if a later key in the list would have matched and verified correctly. Compare with the sibling fix inverifier.rs(crate::crypto::p256_key_id(k).is_ok_and(|id| ...)), which correctly skips bad keys rather than propagating.🐛 Proposed fix
for key in rekor_keys { - let key_digest = crate::crypto::p256_key_id(key)?; + let Ok(key_digest) = crate::crypto::p256_key_id(key) else { + continue; + }; if &key_digest[..4] != keyhint { continue; }🤖 Prompt for 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. In `@jans-cedarling/sigstore-verifier/src/tlog.rs` around lines 556 - 567, Update the Rekor key iteration around p256_key_id so key-ID derivation failures are skipped rather than propagated, allowing verification to continue through all remaining keys. Preserve the existing keyhint filtering and verify_ecdsa_p256_prehashed success path, matching the resilient handling used in verifier.rs.jans-cedarling/sigstore-verifier/src/verifier.rs (1)
230-285: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCurve dispatch logic is correct; Line 232 exceeds the 100-char rustfmt limit.
The P-256/P-384 verifier selection based on
EcCurve::from_point_lencorrectly resolves the prior review comment about fixed-P-256 verification. However, Line 232 is a single very long line (function-pointer cast included) that clearly exceeds the project's mandated 100-character width.♻️ Proposed fix
let verify_sig = match EcCurve::from_point_len(cert.pubkey_bytes.len()) { - Some(EcCurve::P256) => verify_ecdsa_p256_prehashed as fn(&[u8], &[u8], &[u8]) -> Result<(), SigstoreVerificationError>, + Some(EcCurve::P256) => { + verify_ecdsa_p256_prehashed + as fn(&[u8], &[u8], &[u8]) -> Result<(), SigstoreVerificationError> + }, Some(EcCurve::P384) => verify_ecdsa_p384_prehashed,As per coding guidelines, "Format Rust code with the project's rustfmt.toml settings, including a maximum line width of 100 characters and four-space indentation without tabs."
🤖 Prompt for 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. In `@jans-cedarling/sigstore-verifier/src/verifier.rs` around lines 230 - 285, Reformat the P-256 verifier arm in the `verify_sig` curve-dispatch match so the function-pointer cast and assignment comply with the project’s 100-character rustfmt width. Preserve the existing `EcCurve::P256`/`P384` dispatch behavior and four-space indentation.Source: Coding guidelines
🤖 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
`@jans-cedarling/sigstore-verifier/docs/cosign-keyless-verification-algorithm.md`:
- Around line 123-128: Add the `text` language tag to the fenced code block
containing the DSSE PAE formula under “DSSE PAE (step 8)”, leaving the formula
content unchanged.
- Line 195: Update the bundle example’s inclusionProof comment to state that the
proof is verified offline using the checkpoint and Merkle validation, removing
the outdated “online only” and ignored wording. Keep the example consistent with
the documented rejection conditions in the inclusionProof verification section.
In `@jans-cedarling/sigstore-verifier/src/cert.rs`:
- Around line 268-291: Update decode_der_length to reject non-canonical
encodings: require short form for lengths below 128 and reject long-form length
bytes beginning with zero. In the surrounding UTF8String decoding method,
require the declared DER value to consume the entire input, rejecting any
trailing bytes after end. Preserve valid canonical short- and long-form
decoding.
In `@jans-cedarling/sigstore-verifier/src/merkle.rs`:
- Around line 52-74: Add an explicit invariant check after computing inner and
expected in the Merkle verification flow, using debug_assert! to ensure inner
does not exceed expected before proof[..inner] is accessed. Keep the existing
proof-size validation and sibling checks unchanged, and anchor the assertion to
the inner and expected values.
---
Outside diff comments:
In `@jans-cedarling/sigstore-verifier/src/crypto.rs`:
- Around line 81-115: Add regression tests for verify_ecdsa_p384_prehashed
covering successful and failing verification with valid P-384 keys and SHA-384
prehashes, exercising both DER and raw r||s signature encodings. Include
invalid-signature cases and assert the expected SigstoreVerificationError
behavior, matching the existing P-256 test style.
In `@jans-cedarling/sigstore-verifier/src/tlog.rs`:
- Around line 556-567: Update the Rekor key iteration around p256_key_id so
key-ID derivation failures are skipped rather than propagated, allowing
verification to continue through all remaining keys. Preserve the existing
keyhint filtering and verify_ecdsa_p256_prehashed success path, matching the
resilient handling used in verifier.rs.
In `@jans-cedarling/sigstore-verifier/src/verifier.rs`:
- Around line 230-285: Reformat the P-256 verifier arm in the `verify_sig`
curve-dispatch match so the function-pointer cast and assignment comply with the
project’s 100-character rustfmt width. Preserve the existing
`EcCurve::P256`/`P384` dispatch behavior and four-space indentation.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7164aafe-719f-4642-b19f-f133751e1ea9
📒 Files selected for processing (16)
jans-cedarling/AGENTS.mdjans-cedarling/sigstore-verifier/README.mdjans-cedarling/sigstore-verifier/docs/ARCHITECTURE.mdjans-cedarling/sigstore-verifier/docs/cosign-keyless-verification-algorithm.mdjans-cedarling/sigstore-verifier/src/bundle.rsjans-cedarling/sigstore-verifier/src/cert.rsjans-cedarling/sigstore-verifier/src/chain.rsjans-cedarling/sigstore-verifier/src/crypto.rsjans-cedarling/sigstore-verifier/src/lib.rsjans-cedarling/sigstore-verifier/src/merkle.rsjans-cedarling/sigstore-verifier/src/policy.rsjans-cedarling/sigstore-verifier/src/sct.rsjans-cedarling/sigstore-verifier/src/test_support.rsjans-cedarling/sigstore-verifier/src/tlog.rsjans-cedarling/sigstore-verifier/src/trust_root.rsjans-cedarling/sigstore-verifier/src/verifier.rs
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…r Result<(),> Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…ected invariant Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
p256_key_id failures now skip to next key instead of short-circuiting the entire checkpoint check, matching the resilient pattern in verifier.rs. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
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 `@jans-cedarling/sigstore-verifier/src/crypto.rs`:
- Line 121: Reorder the imports in the crypto module so the external p256 and
sha2 imports appear before the internal use super::* import, preserving all
imported symbols and functionality.
In `@jans-cedarling/sigstore-verifier/src/verifier.rs`:
- Around line 231-243: Update the leaf and messageDigest verification paths
around the curve-selected verifier to construct SHA-256 prehashes for P-256 and
SHA-384 prehashes for P-384, keeping the selected digest paired with its
corresponding verifier. Add an end-to-end test covering successful verification
of a P-384 bundle.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a4cfaa49-eb3b-4192-9737-8e60f5781a2a
⛔ Files ignored due to path filters (1)
jans-cedarling/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
jans-cedarling/sigstore-verifier/docs/cosign-keyless-verification-algorithm.mdjans-cedarling/sigstore-verifier/src/crypto.rsjans-cedarling/sigstore-verifier/src/tlog.rsjans-cedarling/sigstore-verifier/src/verifier.rs
Satisfies clippy::type-complexity and wraps P-256/P-384 cast within 100-char width. No behavioral change. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…Crate Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Certificate chains already validated P-384 links with SHA-384, but step 8 always prehashed the artifact with SHA-256 regardless of the leaf's curve. Now P-384 leaves use SHA-384 for both MessageSignature and DSSE PAE prehashes, matching the hash algorithm to the key's security level. tlog body consistency accepts "sha384" hashedrekord entries, and key-ID derivation failures in checkpoint verification skip to the next key instead of short-circuiting. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…allows Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
verify() was 254 lines behind #[allow(too_many_lines)]. Split out parse_cert_and_signature, verify_integrated_time, candidate_intermediates, and the two step-8 branches (verify_message_signature, verify_dsse_envelope). Curve selection moved into SignatureInputs::new, so the inputs struct is built once instead of duplicated across both match arms. enc_len encodes DER long-form lengths via to_be_bytes with leading zeros stripped, dropping both cast_possible_truncation allows; added tests for short form, long form, and the u24 range Fulcio precertificates use. The P-384 test reused the inclusion-proof and SET-signing code already present in Fixture, so both are now free functions shared by the two call sites. missing_errors_doc is dropped from lib.rs with # Errors sections on the three public Result-returning functions that needed them. No behavioral change. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
tareknaser
left a comment
There was a problem hiding this comment.
Thank you. The PR is very dense and there's a lot to go through. I'll do another review later on
| })?; | ||
| // Bundle spec: media type v0.2+ requires an inclusion proof (with | ||
| // checkpoint). v0.1 predates that and may be SET-only. | ||
| if parsed.version()? >= crate::bundle::BundleVersion::Bundle0_2 |
There was a problem hiding this comment.
If I understand this right, a bundle can get out of this check by rewriting its own mediaType.
Nothing signs that field (not the cert or the SET or the Rekor body) so if set it to ...version=0.1 then drop inclusionProof and this gate passes. Step 10 is if let Some(proof) so with the proof gone the Merkle check and the checkpoint check both don't run.
I see v01_bundle_without_inclusion_proof_still_verifies so I think the v0.1 case is intentional. What I am not sure about is that the version comes from whoever hands you the bundle so it isn't really a property of the bundle (anyone can flip it). Is that the intent?
|
|
||
| for key in ctfe_keys { | ||
| // Only try keys whose key ID matches the SCT's logID. | ||
| if crate::crypto::p256_key_id(&key.pubkey_bytes)? != sct.log_id { |
There was a problem hiding this comment.
The ? here gives up on the whole SCT step at the first key that isn't a 65-byte point instead of skipping that key and trying the rest. I think that makes the outcome depend on list order.
verify_checkpoint in tlog.rs and verify_integrated_time skip bad keys
| ); | ||
| } | ||
|
|
||
| fn validate_public_key(pem_bytes: &[u8], filename: &str) { |
There was a problem hiding this comment.
This parses the SPKI and prints the OID but I don't see it assert anything about it. So for rekor.pem, ctfe.pem, and ctfe_2021.pem, the only thing checked is that the base64 decodes and the DER parses.
seems like a few places lean on this being a real check. The module doc says a corrupt PEM fails the build and that this "guarantees that with_static_trust_root() can unwrap() safely at runtime".
verifier.rs has .expect("trust root keys validated at build time")
|
|
||
| #[test] | ||
| fn scan_conformance_bundle_verify() { | ||
| let Ok(root) = std::env::var("SIGSTORE_CONFORMANCE_DIR") else { |
There was a problem hiding this comment.
test-cedarling.yml runs cargo test --locked --workspace --exclude cedarling_pg so it does execute.
also nothing in .github/workflows/ sets SIGSTORE_CONFORMANCE_DIR
not sure if it's checked in another workflow maybe?
| return leaf_issuer.ok_or_else(|| SigstoreVerificationError::CertificateChain { | ||
| reason: "leaf issuer not found on chain path".into(), | ||
| }); |
There was a problem hiding this comment.
This part is unreachable since leaf_issuer is set to Some right above it
| /// A message digest within a `MessageSignature`. | ||
| #[derive(Debug, Clone, Deserialize)] | ||
| pub(crate) struct MessageDigest { | ||
| /// The hex-encoded digest value. |
There was a problem hiding this comment.
This doc comment is wrong it's base64 encoded not hex
Prepare
Description
Target issue
closes #14465
Implementation Details
New crate for offline, WASM-compatible Sigstore/Cosign bundle verification.
No network calls during
verify()— all trust material is compiled in orsupplied by the caller.
Architecture
The 10-step verification pipeline runs in
verifier.rs(the orchestrator).Each step delegates to a dedicated module:
verifier → bundle(parse) → cert(extract) → tlog(SET)
→ chain(validate) → sct(CTFE) → crypto(signature)
→ policy(identity) → merkle(inclusion proof)
Module dependency tree:
┌────────┼────────┐
verifier policy trust_root
│
├── bundle
├── cert ── chain ── crypto
├── crypto
├── sct ── crypto
├── tlog ── crypto
└── merkle
All modules depend on
error.rs(11-variantthiserrorenum).Trust roots
Production Fulcio/Rekor/CTFE keys embedded at compile time via
include_bytes!.build.rsvalidates every PEM: CA certs must haveBasicConstraints CA:true+KeyUsage keyCertSign, and their expirationis checked. Two paths for callers:
SigstoreBlobVerifier::with_static_trust_root()— embedded keys.SigstoreBlobVerifier::new(trust_root_raw)— caller-provided PEMs.Supported bundle formats
Sigstore bundles v0.1–v0.3, both
messageSignatureanddsseEnvelopemedia types. DSSE payloads are bound to the artifact via in-toto Statement
subject[].digest.sha256comparison (not just envelope-level PAE — fullsubject binding). Unknown media types, managed-key bundles, and Rekor v2
proof-only bundles are explicitly rejected.
Testing strategy — 66 tests, three layers
Unit tests (58): each module tested in isolation. Synthetic certs/keys
via
rcgen(pure Rust, no OpenSSL). Negative tests assert exact errorvariant, not just
is_err().Real-bundle parity (7): verifies a genuine public-good Sigstore v0.3
bundle from the sigstore-conformance corpus against the embedded trust
root. Committed negative fixtures: corrupted inclusion proof, invalid
checkpoint signature, wrong checkpoint root hash, messageDigest mismatch
— all must reject.
Conformance scan (1, opt-in): runs the full sigstore-conformance
bundle-verifycorpus whenSIGSTORE_CONFORMANCE_DIRis set. Status:all hashedrekord positives pass, all negatives rejected, zero
false-accepts.
Key files for review
src/verifier.rssrc/chain.rssrc/tlog.rssrc/cert.rssrc/merkle.rssrc/sct.rssrc/bundle.rsDeliberately out of scope (do not flag)
trusted_root.json— PEM-only trust material.1.3.6.1.4.1.57264.1.1— v2 only.Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit