Skip to content

feat(jans-cedarling): implement Sigstore/Cosign verification library - #14636

Open
olehbozhok wants to merge 60 commits into
mainfrom
jans-cedarling-14465
Open

feat(jans-cedarling): implement Sigstore/Cosign verification library #14636
olehbozhok wants to merge 60 commits into
mainfrom
jans-cedarling-14465

Conversation

@olehbozhok

@olehbozhok olehbozhok commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

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 or
supplied 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:

  lib.rs (pub re-exports)
     │

┌────────┼────────┐
verifier policy trust_root

├── bundle
├── cert ── chain ── crypto
├── crypto
├── sct ── crypto
├── tlog ── crypto
└── merkle

All modules depend on error.rs (11-variant thiserror enum).

Trust roots

Production Fulcio/Rekor/CTFE keys embedded at compile time via
include_bytes!. build.rs validates every PEM: CA certs must have
BasicConstraints CA:true + KeyUsage keyCertSign, and their expiration
is 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 messageSignature and dsseEnvelope
media types. DSSE payloads are bound to the artifact via in-toto Statement
subject[].digest.sha256 comparison (not just envelope-level PAE — full
subject binding). Unknown media types, managed-key bundles, and Rekor v2
proof-only bundles are explicitly rejected.

Testing strategy — 66 tests, three layers

  1. Unit tests (58): each module tested in isolation. Synthetic certs/keys
    via rcgen (pure Rust, no OpenSSL). Negative tests assert exact error
    variant, not just is_err().

  2. 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.

  3. Conformance scan (1, opt-in): runs the full sigstore-conformance
    bundle-verify corpus when SIGSTORE_CONFORMANCE_DIR is set. Status:
    all hashedrekord positives pass, all negatives rejected, zero
    false-accepts.

Key files for review

File Lines Role
src/verifier.rs 989 10-step orchestrator + e2e tests
src/chain.rs ~350 DN-based path building, P-384 Fulcio chain
src/tlog.rs ~250 SET verify, body consistency (CVE-2022-36056)
src/cert.rs 497 X.509 parse via x509-parser, OIDC extensions
src/merkle.rs ~100 Offline RFC 6962 inclusion proof (Trillian fold)
src/sct.rs 558 RFC 6962 SCT verification, precert reconstruction
src/bundle.rs ~300 v0.1–v0.3 format enum, media type dispatch

Deliberately out of scope (do not flag)

  • TUF / trusted_root.json — PEM-only trust material.
  • Rekor v2 / TSA timestamps — SET is always required.
  • RSA / Ed25519 — only ECDSA P-256 (leaf/Rekor/CTFE) and P-384 (Fulcio CAs).
  • Managed keys — keyless/certificate-based bundles only.
  • Fulcio deprecated issuer OID 1.3.6.1.4.1.57264.1.1 — v2 only.

Test and Document the changes

  • Static code analysis has been run locally and issues have been fixed
  • Relevant unit and integration tests have been added/updated
  • Relevant documentation has been updated if any (i.e. user guides, installation and configuration guides, technical design docs etc)

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.

  • I confirm that there is no impact on the docs due to the code changes in this PR.

Summary by CodeRabbit

  • New Features
    • Added offline, WASM-compatible Sigstore/Cosign blob signature verification with certificate-chain validation, SCT checks, Rekor SET authentication, optional Merkle inclusion-proof + checkpoint verification, and strict identity/issuer policy enforcement.
    • Supports both message-signature and DSSE bundle verification.
  • Documentation
    • Added quick start plus detailed architecture and verification-algorithm references.
  • Tests
    • Added unit/integration/real-bundle coverage, conformance scanning, and new fixture cases (including inclusion-proof and checkpoint failures).
  • Chores/Style
    • Updated code style guidance to discourage panic-based runtime validation for input handling.

olehbozhok added 17 commits July 6, 2026 17:01
… 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>
@mo-auto

mo-auto commented Jul 27, 2026

Copy link
Copy Markdown
Member

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds sigstore-verifier as an offline, WASM-compatible Sigstore/Cosign blob verification crate with bundle parsing, certificate, ECDSA, SCT, Rekor, policy, trust-root, DSSE, Merkle-proof, documentation, and integration-test support.

Changes

Sigstore verifier implementation

Layer / File(s) Summary
Crate foundation and verification specification
jans-cedarling/Cargo.toml, jans-cedarling/sigstore-verifier/Cargo.toml, README.md, build.rs, docs/*
Registers the crate, defines dependencies, validates embedded trust material at build time, and documents the verification model and supported scope.
Bundle, certificate, policy, and trust contracts
src/bundle.rs, src/cert.rs, src/error.rs, src/lib.rs, src/policy.rs, src/trust_root.rs
Adds typed bundle parsing, structured errors, certificate extraction and constraints, identity and issuer policies, public exports, and configurable or embedded trust roots.
Certificate, ECDSA, and SCT validation
src/chain.rs, src/crypto.rs, src/sct.rs, src/test_support.rs
Implements certificate-chain validation, P-256/P-384 signature checks, SCT verification, DER handling, and synthetic certificate/SCT helpers with unit coverage.
Rekor and Merkle proof validation
src/tlog.rs, src/merkle.rs
Authenticates Rekor SETs and checkpoints, checks hashedrekord/DSSE body consistency, and verifies RFC 6962 inclusion proofs.
Public verifier flow and integration coverage
src/verifier.rs, tests/*
Connects verification components through SigstoreBlobVerifier, with conformance, fixture, DSSE, and real-bundle tests.

Estimated code review effort: 5 (Critical) | ~120 minutes

Suggested reviewers: tareknaser, dagregi, haileyesus2433, 0xtinkle

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR adds DSSE/in-toto verification and tests, but #14465 scoped the initial library to v0.3 MessageSignature with P-256 only. Remove or split DSSE/in-toto support into a follow-up PR, or update the linked issue scope before merging.
Out of Scope Changes check ⚠️ Warning DSSE/in-toto support, fixtures, and verifier logic extend beyond the issue’s initial MessageSignature-only scope. Move DSSE/in-toto code and tests to a separate follow-up PR unless the issue scope is expanded.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding a Sigstore/Cosign verification library.
Description check ✅ Passed The description matches the template with target issue, implementation details, and testing/documentation sections filled in.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jans-cedarling-14465

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mo-auto mo-auto added area-documentation Documentation needs to change as part of issue or PR comp-docs Touching folder /docs comp-jans-cedarling Touching folder /jans-cedarling kind-feature Issue or PR is a new feature request labels Jul 27, 2026
@coderabbitai coderabbitai Bot added the kind-dependencies Pull requests that update a dependency file label Jul 27, 2026
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>
@coderabbitai coderabbitai Bot removed the kind-enhancement Issue or PR is an enhancement to an existing functionality label Jul 27, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Add 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 win

A single unparseable Rekor key aborts checkpoint verification entirely.

crate::crypto::p256_key_id(key)? inside the for key in rekor_keys loop 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 in verifier.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 win

Curve dispatch logic is correct; Line 232 exceeds the 100-char rustfmt limit.

The P-256/P-384 verifier selection based on EcCurve::from_point_len correctly 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4c6f35c and fed599c.

📒 Files selected for processing (16)
  • jans-cedarling/AGENTS.md
  • jans-cedarling/sigstore-verifier/README.md
  • jans-cedarling/sigstore-verifier/docs/ARCHITECTURE.md
  • jans-cedarling/sigstore-verifier/docs/cosign-keyless-verification-algorithm.md
  • jans-cedarling/sigstore-verifier/src/bundle.rs
  • jans-cedarling/sigstore-verifier/src/cert.rs
  • jans-cedarling/sigstore-verifier/src/chain.rs
  • jans-cedarling/sigstore-verifier/src/crypto.rs
  • jans-cedarling/sigstore-verifier/src/lib.rs
  • jans-cedarling/sigstore-verifier/src/merkle.rs
  • jans-cedarling/sigstore-verifier/src/policy.rs
  • jans-cedarling/sigstore-verifier/src/sct.rs
  • jans-cedarling/sigstore-verifier/src/test_support.rs
  • jans-cedarling/sigstore-verifier/src/tlog.rs
  • jans-cedarling/sigstore-verifier/src/trust_root.rs
  • jans-cedarling/sigstore-verifier/src/verifier.rs

Comment thread jans-cedarling/sigstore-verifier/src/cert.rs Outdated
Comment thread jans-cedarling/sigstore-verifier/src/merkle.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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between b9e7c8a and 09ed923.

⛔ Files ignored due to path filters (1)
  • jans-cedarling/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (4)
  • jans-cedarling/sigstore-verifier/docs/cosign-keyless-verification-algorithm.md
  • jans-cedarling/sigstore-verifier/src/crypto.rs
  • jans-cedarling/sigstore-verifier/src/tlog.rs
  • jans-cedarling/sigstore-verifier/src/verifier.rs

Comment thread jans-cedarling/sigstore-verifier/src/crypto.rs
Comment thread jans-cedarling/sigstore-verifier/src/verifier.rs Outdated
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 tareknaser left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ossdhaval ossdhaval added this to the 3.0.0 milestone Jul 30, 2026
Comment on lines +80 to +82
return leaf_issuer.ok_or_else(|| SigstoreVerificationError::CertificateChain {
reason: "leaf issuer not found on chain path".into(),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doc comment is wrong it's base64 encoded not hex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-documentation Documentation needs to change as part of issue or PR comp-docs Touching folder /docs comp-jans-cedarling Touching folder /jans-cedarling kind-feature Issue or PR is a new feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(jans-cedarling): implement Sigstore/Cosign verification library

5 participants