feat(jans-cedarling): implement Sigstore/Cosign verification library - #14636
feat(jans-cedarling): implement Sigstore/Cosign verification library #14636olehbozhok wants to merge 92 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>
… cert validate_leaf() checked non-CA and the code-signing EKU but never the digitalSignature bit — Fulcio sets this (critical) on every leaf it issues specifically to scope the certificate to signing use, and it went unenforced. Extend Cert with has_digital_signature and check it alongside the existing EKU check. Test fixtures previously didn't set any KeyUsage on non-CA leaves at all; give them digitalSignature by default so this doesn't regress every e2e test, and add a corner-case negative test where the extension is present but carries an unrelated bit instead (not just "extension absent entirely", which is a different code path with the same outcome). Confirmed real Fulcio-issued certs (real_bundle.rs, conformance_scan.rs) already carry this bit, so no behavior change for genuine bundles. #[allow(clippy::struct_excessive_bools)] on Cert: the four bools are independent facts from unrelated X.509 extensions, not a state machine. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…, not just intermediates Intermediates already had their pathLen checked during the chain walk; the terminating root didn't — an asymmetry that would let an extra forged intermediate slip through unnoticed if Fulcio's root cert ever carries a pathLenConstraint. Add the same check at the point the walk terminates at a trusted root, using the same `depth` (intermediates already traversed) that intermediates are checked against. Add make_root_constrained() to test_support and two tests: pathLen=0 rejects any intermediate below the root, pathLen=1 allows exactly one. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…scan into CI Real diagnosis of a gap the conformance corpus surfaced: the DSSE envelope's keyid must be OMITTED from the reconstructed canonical JSON when empty, not serialized as "keyid": "" — protobuf3 JSON marshaling drops zero-value fields. Emitting the empty key produced a different byte sequence than what Rekor actually hashed, causing a spurious envelopeHash mismatch and false-rejecting a genuine DSSE bundle (happy-path-intoto-in-dsse-v3 in the sigstore-conformance corpus). Confirmed by hash-comparing both candidate encodings against the real envelopeHash. tests/conformance_scan.rs now asserts pass_gaps == 0 in addition to the existing fail_gaps == 0 — previously an over-rejection regression would only show up as a "**GAP**" line in captured output, not a test failure. Add a dedicated CI workflow (test-sigstore-verifier-conformance.yml), scoped to changes under jans-cedarling/sigstore-verifier/** only, that checks out the sigstore-conformance corpus (pinned commit) and runs this scan. Kept separate from the main jans-cedarling test workflow: this crate has no protobuf dependencies and doesn't need protoc, and there's no reason to pull in the full corpus checkout for unrelated cedarling changes. Without this, the test was inert in CI — it silently skips unless SIGSTORE_CONFORMANCE_DIR is set, which nothing ever set. Verified locally end-to-end (checked out both repos matching the CI directory layout, ran the exact command): 5 pass-cases (0 gaps), 25 fail-cases (0 false-accepts), 37 skipped (out-of-scope: managed-key, custom-trusted-root). Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…injection IdentityMatch::Regex anchored a caller-supplied pattern by string-wrapping it as `\A(?:pattern)\z`. A pattern with an unbalanced top-level `)` or `|` (e.g. `)|(?:.*`) can close the wrapping group early and open a new top-level alternative, producing a regex whose first branch matches any string at all — defeating the anchor entirely. Not exploitable by an attacker forging a certificate (the pattern comes from the policy author's own config, not certificate data), but a real footgun for a security library's own documented safety claim. Switch to compiling the pattern as-is and checking the match span covers the whole string, instead of concatenating it into a larger regex string. Structurally eliminates the whole class of wrapping-injection bugs, not just this one pattern. Covered by a regression test using the exact adversarial pattern from the finding. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
IdentityMatch::Regex(String) recompiles the pattern from scratch on every VerificationPolicy::verify call — wasted work for a caller that verifies many bundles against the same policy. Add IdentityMatch::CompiledRegex(regex_lite::Regex): regex_lite::Regex is Arc-backed internally, so cloning it (e.g. to build several policies from one compiled pattern) is cheap, unlike recompiling from a string. Factor the full-string-match check (span-based, not string-wrapped — see the prior paren-injection fix) into a shared helper used by both variants. Re-export regex_lite from the crate root so callers can construct the Regex without adding their own regex-lite dependency and risking a version mismatch with the one this crate was built against. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
… not pubkey byte length chain.rs and verifier.rs each independently inferred P-256 vs P-384 from the raw uncompressed point's byte length (65 vs 97). Works today only because the two curves happen to produce differently-sized points — it's algorithm-confusion-shaped code, not curve authentication, and the same inference logic was duplicated in two places. Extract the curve once, at Cert-parse time, from the SPKI's declared AlgorithmIdentifier (id-ecPublicKey + namedCurve OID) instead. Cert gains a `curve: Option<EcCurve>` field; EcCurve moves from chain.rs to cert.rs since it's now a parsed-certificate property, not a chain-validation concept. Both chain.rs (issuer key, for chain-link signature checks) and verifier.rs (leaf key, for the artifact signature) now read cert.curve instead of re-deriving it — removes the duplication too. Verified against both synthetic P-256 fixtures and the real Fulcio P-384 chain (real_bundle.rs, conformance_scan.rs). Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…gh a real chain walk test_support.rs's make_root/make_intermediate always generated P-256 keys regardless of the common_name passed — "fulcio-root"/"p384-root" naming was cosmetic. Every chain.rs test built P-256 CAs, so verify_cert_signature's P-384 branch (the curve real Fulcio actually uses for its root/intermediate) had zero coverage through the actual chain-walk code; only crypto.rs's isolated primitive tests touched it. Add make_root_p384/make_intermediate_p384 and a chain.rs test building a genuine P-384 leaf -> intermediate -> root chain through validate_chain, asserting both links' curve == Some(EcCurve::P384) along the way. Also fix a misleading test name/comment in verifier.rs: p384_bundle_verifies_with_sha384_prehash's root was P-256 despite being named "p384-root" and the comment claiming it "builds a P-384 chain" — that test is actually about the leaf's own SHA-384 artifact-signature prehash selection, not chain-link verification; renamed and re-commented for accuracy, no behavior change. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…input vs genuine-mismatch RekorInconsistency covered everything from "canonicalizedBody is absent" and "checkpoint signature is not valid base64" to "artifact hash mismatch" and "checkpoint root hash != inclusion proof root hash" — a caller matching on it couldn't tell a structurally broken tlog entry/proof (retry-worthy, or just reject the bundle) from a well-formed value that fails a cryptographic/equality check against another independently-derived value (the actual "possible tampering" signal). Add RekorMalformed for the former category and reclassify every call site in tlog.rs, merkle.rs, and verifier.rs accordingly. RekorInconsistency now only covers genuine mismatches: artifact/signature/cert hash mismatches (CVE-2022-36056), DSSE envelope/payload hash mismatches, checkpoint tree-size/root-hash mismatches, Merkle proof reconstruction failure, and "checkpoint signature not verified by any trusted key" — each case where well-formed, independently-derived data disagrees. Existing tests already asserting RekorInconsistency for genuine mismatches (artifact hash, signature, real-bundle corrupted proof/checkpoint) needed no changes — confirms the reclassification didn't touch those. Added new tests asserting RekorMalformed for missing 'spec', unsupported tlog kind, and malformed Merkle proof shape (wrong count/length/out-of-range index). Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…tion paths chain.rs and tlog.rs had the lowest function coverage (58% and 73%) despite holding the core CVE-2022-36056 tamper checks and RFC 5280 pathLen enforcement — add negative tests for pathLen violations, the chain-depth loop guard, DN/signature/algorithm mismatches, and Rekor SET/checkpoint/DSSE consistency mismatches, each asserting the specific error variant. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…ifier and sct verifier.rs and sct.rs had 0% coverage on their malformed-bundle and malformed-SCT-byte parsing branches — add negative tests for bad base64, missing fields, unsupported curves, and RFC 6962 SCT parser edge cases (short body, bad version, ext/sig length overrun, wrong DER tags), each asserting the specific error variant. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Hardened parsers (X.509 DER, bundle JSON, RFC 6962 SCT list) and the full verify() pipeline take fully attacker-controlled bytes but only had hand-picked negative-case coverage; wire up standalone cargo-fuzz targets, seeded from existing test fixtures via symlink, gated behind a `fuzzing` feature so the parsers stay pub(crate) in normal builds. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Manually watching libFuzzer output for "has it stopped finding anything" doesn't scale across 4 targets; script loops each target in 5-min chunks across all cores, stops once corpus growth flatlines for 2 consecutive chunks (no time cap by default), then runs cargo fuzz cmin — crashes abort that target immediately and skip minimization. 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>
| // only forgoes the extra offline Merkle/checkpoint consistency | ||
| // check, not authentication itself — matching upstream cosign/ | ||
| // sigstore-go, which also treats v0.1 SET-only bundles as valid. | ||
| if parsed.version()? >= crate::bundle::BundleVersion::Bundle0_2 |
There was a problem hiding this comment.
This gate reads mediaType, which is an unsigned string inside the bundle, so the bundle itself
decides whether the inclusion proof and checkpoint are checked. I relabelled the committed v0.3
fixtures as version=0.1 and deleted inclusionProof, and all three negative fixtures
(inclusion-proof-corrupted-hash, invalid-checkpoint-signature, checkpoint-wrong-roothash)
verified successfully. The comment above argues this is safe because the SET is unforgeable, but
that assumes the Rekor key is honest the inclusion proof against a signed checkpoint is exactly
the control that survives a compromised or split-view log, and the SET and checkpoint are signed by
the same key. The decision needs to come from the caller, not the bundle: either always require an
inclusion proof, or add an explicit VerificationPolicy opt-in for SET-only v0.1.
|
|
||
| // Real sigstore-conformance negative fixtures — each corrupts one part of the | ||
| // transparency-log evidence; all must be rejected for the *right* reason. | ||
|
|
There was a problem hiding this comment.
Related to the verifier.rs comment each of these fixtures still verifies after a one-field
downgrade, so as written they prove the corrupted-proof path is rejected only when nobody edits the
media type. Once the gate is fixed, please add the downgraded variant of each fixture as its own
must-reject case so the bypass can't come back.
| } | ||
|
|
||
| /// Returns the tlog entry for Rekor verification. | ||
| #[must_use] |
There was a problem hiding this comment.
.first() is the only read of tlog_entries anywhere in the crate, so steps 3, 9 and 10 all run
against entry [0] and everything after it is parsed and ignored. I appended a second entry with a
garbage canonicalizedBody and an integratedTime in year 5138 and the bundle still verified. The
issue spec requires every entry to be verified, and the practical problem is that a bundle this
crate has marked "verified" can carry attacker-controlled log entries onward to whoever stores or
re-publishes it. Either verify all entries or reject tlog_entries.len() != 1 silently ignoring
them is the worst of the three options.
| pub(crate) verification_material: VerificationMaterial, | ||
|
|
||
| /// The signed content. | ||
| #[serde(flatten)] |
There was a problem hiding this comment.
A bundle carrying both messageSignature and dsseEnvelope is accepted, and serde just picks one
I added a dsseEnvelope to a valid messageSignature bundle and it took the DSSE path, so an
attacker-added field decided which content got authenticated. It fails closed today because
verify_body_consistency dispatches on the log body's kind and catches the mismatch, but that's
incidental rather than structural, and it goes live the moment the two paths diverge or a third
content type is added. Deserialize into one Option per variant and reject unless exactly one is
present.
|
|
||
| /// How to match the certificate identity (SAN). | ||
| #[derive(Debug, Clone)] | ||
| pub enum IdentityMatch { |
There was a problem hiding this comment.
IdentityMatch::CompiledRegex can't be fixed at the call site Same issue as full_match, but worse the caller hands over an already-compiled unanchored Regex,so there's no way to repair it later. This wants a constructor-validated newtype that can only be built through compile_anchored suggestion
| // schedule instead of as an emergency once the cert has already expired. | ||
| let seconds_left = not_after - now; | ||
| let warn_window_seconds = EXPIRY_WARN_WINDOW_DAYS * 24 * 60 * 60; | ||
| assert!( |
There was a problem hiding this comment.
a hard assert! on wall-clock time makes the build self-destruct
This means the crate stops compiling on a future date with no code change, every downstream consumer of jans-cedarling breaks at once, and previously released tags become un-buildable reproducing an old release would need a build.rs patch or a faked clock.
It's also cached rerun-if-changed=src/trust/ means this time-dependent check only re-runs when the PEM files change, so the failure lands at an unpredictable moment rather than on a knowable date.
There was a problem hiding this comment.
It is fine and designed to be like that. We need to add new certificates at that time.
| @@ -0,0 +1,49 @@ | |||
| name: sigstore-verifier Conformance Scan | |||
|
|
|||
There was a problem hiding this comment.
the WASM target has no CI coverage cargo build --target wasm32-unknown-unknown is in the issue's acceptance criteria, but sigstore-verifier is never built for wasm anywhere in CI.
| ecdsa = { version = "0.16", default-features = false, features = ["der", "verifying"] } | ||
| x509-parser = { version = "0.18", default-features = false } | ||
| serde_json_canonicalizer = "0.3" | ||
| der = { version = "0.8", default-features = false, features = ["oid"] } |
There was a problem hiding this comment.
declared but never used There isn't a single reference to der in src/ or build.rs;
| ecdsa = { version = "0.16", default-features = false, features = ["der", "verifying"] } | ||
| x509-parser = { version = "0.18", default-features = false } | ||
| serde_json_canonicalizer = "0.3" | ||
| der = { version = "0.8", default-features = false, features = ["oid"] } |
There was a problem hiding this comment.
declared but never used There isn't a single reference to der in src/ or build.rs;
| /// Re-exported so callers can construct [`IdentityMatch::CompiledRegex`] | ||
| /// without adding their own `regex-lite` dependency (and risking a version | ||
| /// mismatch with the one this crate was built against). | ||
| pub use regex_lite; |
There was a problem hiding this comment.
Any semver bump of regex-lite becomes a breaking change here. The reason given (letting callers
build CompiledRegex without a version mismatch) is real, but the anchored-regex newtype suggested
on policy.rs removes the need to expose the foreign type at all, which solves both.
There was a problem hiding this comment.
The semver point is fair — pub use regex_lite; does put a foreign type on the public surface.
The proposed remedy no longer applies though: following your policy.rs thread I checked what upstream does, and nobody auto-anchors. sigstore-go's SubjectAlternativeNameMatcher uses Regexp.MatchString and documents "regexp matching is not anchored by default; use ^...$ if you intend to match the entire SAN value"; cosign's CheckCertificatePolicy is the same; sigstore-python has no regex at all. Anchoring is now removed from this crate, so there's no invariant left for an anchored-regex newtype to enforce.
Keeping the re-export for now: so there's nothing for a semver bump to break — and the re-export is what lets callers configure their own Regex (RegexBuilder has case_insensitive, multi_line, size_limit), which a newtype taking &str would remove. Worth revisiting when the crate is published and there's a real consumer to design against.
The `der` crate was declared in [dependencies] but never referenced — no `der::` path, `use der`, or `extern crate der` anywhere in src/, build.rs or tests/. DER parsing goes through x509-parser instead. The `features = ["der"]` entry on the `ecdsa` dependency is a feature name of `ecdsa` itself and is satisfied by that crate's own dependency tree, so it does not need this declaration. Removes der 0.8.1 from the lock file entirely; the remaining der 0.7.10 is transitive via ecdsa/pkcs8/spki. One less crate compiled into every build, including the wasm target. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
parse_pem_to_der accepted three inputs it should not have: - No `-----BEGIN` line: `in_body` never flipped, so the empty base64 string decoded to `Ok(vec![])` and the function returned `Some([])` instead of None. Any valid-UTF-8 non-PEM input (JSON, plain text, empty) produced an empty DER buffer that only failed one layer later. - A block truncated before its `-----END` line was accepted; PEM body lines are 64 chars, a multiple of 4, so a cut at a line boundary decodes cleanly to truncated DER. - `break` on the first `-----END` kept only the first block of a concatenated file. The standard Fulcio `fulcio.crt.pem` ships root and intermediate concatenated, so a caller passing it to SigstoreTrustRootRaw lost a trust anchor with no error. Require exactly one complete, non-empty block instead; multiple certificates go in as separate `Vec<Vec<u8>>` entries, which is what that field is already shaped for. A stray `-----END` without a `BEGIN` is rejected too. The embedded src/trust/*.pem files each hold a single block, so the static trust root is unaffected. The raw-DER fallbacks in tlog.rs are unaffected as well — from_utf8 rejects real DER before this code runs — but now route valid-UTF-8 non-PEM through the fallback to a clear RekorMalformed instead of an empty-DER parse failure. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…ontent `messageSignature` and `dsseEnvelope` are a protobuf `oneof` in sigstore_bundle.proto, but they were parsed through a single `#[serde(flatten)]` externally-tagged enum. A flattened enum stops at the first variant key it finds and drops the remaining buffered keys, and no struct in the crate sets `deny_unknown_fields`, so a bundle carrying both parsed successfully with JSON key order deciding which content got authenticated — the other was never even checked for consistency. It failed closed only incidentally: verify_body_consistency dispatches on the Rekor log body's `kind` and caught the mismatch. That is not a structural guarantee, and it disappears the moment the two paths diverge or a third content type is added. Split the wire form into a RawBundle with two independent Option fields and resolve them to exactly one BundleContent in Bundle::from_json. Bundles with both, or with neither, are now rejected as InvalidBundleFormat at parse time, so no later step has to choose between two candidate contents. This is the behaviour docs/cosign-keyless-verification-algorithm.md already specified in its edge-case table (REJECT (ambiguous) for both present, REJECT for neither); only the code was missing. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
`tlog_entry()` returned `tlog_entries.first()`, and it was the only read of that field anywhere in the crate. Steps 3, 9 and 10 all ran against entry [0]; everything after it was deserialized and dropped. An appended entry with a forged body and an arbitrary integratedTime therefore travelled inside a bundle this crate had declared verified, on to whoever stored or re-published it. `tlogEntries` is `repeated` in sigstore_bundle.proto with no cardinality constraint, so rejecting `len != 1` would refuse spec-legal bundles. Verify all of them instead: `tlog_entries()` now returns the whole slice, and SET verification, body consistency and the inclusion proof each run for every entry. Steps 4 and 6 need one timestamp to anchor on. `split_first()` provides it: the "at least one entry" requirement is established at the point of use rather than by indexing a list a different function promised was non-empty, and no intermediate Vec of timestamps is built. Any entry's time would do — all are Rekor-signed by then, and step 9 ties every one to the same certificate, signature and artifact digest — so pinning the first keeps VerifiedSignature.verified_at defined rather than incidental. Tests cover all three cases: a multi-entry bundle whose entries are all valid still verifies; an appended entry whose SET doesn't cover its body is rejected at step 3; and an appended entry carrying a genuine Rekor SET over a different artifact — which step 3 accepts — is rejected at step 9. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…opts out The inclusion-proof gate was `version() >= Bundle0_2`, and `version()` reads `mediaType` — an unsigned string inside the bundle. A producer could relabel a v0.3 bundle as `version=0.1`, delete `inclusionProof`, and skip the Merkle and signed-checkpoint checks entirely; the reviewer reproduced this against all three committed negative fixtures. The bundle decided how thoroughly it would be checked. The comment defending the gate argued the SET makes the proof redundant, but the SET and the checkpoint are signed by the same Rekor key, so the SET cannot detect a compromised or split-view log — the inclusion proof against a signed checkpoint is exactly the control that can. `inclusion_proof` is REQUIRED in sigstore_rekor.proto, so require it for every entry by default. The one legitimate exemption — legacy v0.1 bundles, which predate the rule — becomes an explicit caller decision: `SigstoreBlobVerifier::allowing_set_only_v01()`. With it off, the relabelling gains nothing; with it on, the bundle's self-declaration only picks among behaviours the caller already accepted. The switch is on the verifier rather than VerificationPolicy: the policy states whom to trust (identity, issuer), this states how much log evidence to demand. sigstore-go draws the same line, configuring transparency-log requirements on the verifier and identity on the policy. Verified against the sigstore-conformance corpus at the commit CI pins (080de1d): 5 expected-pass cases still pass and 25 expected-fail cases are still rejected. happy-path-v0.1 carries an inclusion proof and is unaffected; every corpus bundle lacking one is an expected-fail case, including the v0.1 bundle-negative-log-index_fail. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
verify_checkpoint selected its Rekor key by scanning every trusted key for a 4-byte keyhint match, and discarded the checkpoint's origin line. verify_integrated_time already did the right thing for the SET — filter by the full SHA-256 SPKI key ID against the entry's logId, with a comment explaining that a bundle must not get to pick which key verifies it — but that narrowing was never carried down to the checkpoint. With more than one Rekor key in the trust root, which is what key rotation produces, an entry could name one log in its logId and carry a checkpoint signed by a different trusted key. A 4-byte hint is a disambiguator, not an identity. Extract the filter into rekor_keys_for_entry() and use it for both the SET and the checkpoint, so every piece of Rekor-signed material for an entry is verified by that entry's log key or not at all. The origin line is now required to be present and non-empty: sigstore_rekor.proto says a checkpoint MUST carry an origin identifying its log. It is not compared against an expected value — the trust root holds keys, not log names, and trusted_root.json is out of scope for this crate — so the binding to this entry's log is made through logId, where it can actually be checked. The new test is not vacuous: reverting to passing the whole key set makes it fail. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…wngrade Each committed sigstore-conformance negative fixture corrupts one piece of transparency-log evidence, and each was rejected only for as long as the bundle kept carrying the evidence that convicted it: relabelling `mediaType` as version=0.1 and deleting `inclusionProof` made all three verify, because the inclusion-proof requirement was keyed off that unsigned field. Add the downgraded variant of each fixture as its own must-reject case, so the bypass cannot come back unnoticed. Two of the three fixtures already declare version=0.1 (checkpoint-wrong-roothash, invalid-checkpoint-signature), so for those the attack was even shorter than relabel-and-strip — deleting the proof alone was enough. The shared helper does both edits so all three cases read the same. These tests are not vacuous: restoring the old `version >= Bundle0_2` gate makes all three fail. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
`cargo build --target wasm32-unknown-unknown` is an acceptance criterion of issue #14465 and the reason this crate exists instead of a sigstore-rs dependency, but nothing in CI ever built it for wasm. The workspace's other wasm steps are scoped to `-p cedarling_wasm` (test-cedarling.yml, build-packages.yml), and no workspace crate depends on sigstore-verifier, so it got no transitive coverage either. Add a wasm_build job to the crate's existing path-filtered workflow and rename the workflow, which now does two things. Job names are unchanged, so existing checks keep their identity. This also puts a standing check under the one wall-clock call in the crate: on wasm32, chrono resolves with js-sys and wasm-bindgen, so `Utc::now()` in trust_root.rs depends on a JS host. That is a deliberate trade-off (see the P0.1 discussion), but it should not be able to change unobserved. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Identity regex matching auto-anchored the pattern: the match span had to cover the whole SAN, so `evil\.com` did not match `not-evil.com.attacker.io`. No Sigstore implementation does this. - sigstore-go, pkg/verify/certificate_identity.go, uses `Regexp.MatchString` and documents the contract on both the SAN and issuer matchers: "regexp matching is not anchored by default; use ^...$ if you intend to match the entire SAN value." - cosign, CheckCertificatePolicy, likewise uses `regex.MatchString` for SubjectRegExp and IssuerRegExp, unanchored. - sigstore-python offers no regex at all — sigstore/verify/policy.py does not import `re`; Identity.verify is `self._identity in all_sans`. The sigstore documentation says nothing either way: it never mentions anchoring, partial matches, or the semantics of `--certificate-identity-regexp`. Auto-anchoring was therefore a local invention, and a silent one: a pattern carried over from `cosign verify --certificate-identity-regexp` meant something different here than in the tool that produced the bundle. Use `is_match` and document that the caller writes `^...$`. Removing the span check also removes an artefact of it: `find` is leftmost-first, so `foo|foobar` never full-matched `foobar` while `foobar|foo` did. That asymmetry is gone with the mechanism that caused it. An invalid pattern still rejects rather than degrading to a wildcard. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…build.rs validate_x509_cert asserted `not_after >= now` and then, immediately below, `seconds_left >= 90 days`. The second subsumes the first — if a cert has at least 90 days left it has not expired — so the first assert could never fire. Keep the window check, and say in the comment why there is no separate already-expired branch, so it doesn't get added back as an oversight. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…uild The wasm_build job failed with "can't find crate for `core`". jans-cedarling/rust-toolchain.toml pins channel 1.95.0, so cargo in that directory uses the pinned toolchain, while dtolnay/rust-toolchain had installed wasm32-unknown-unknown for stable. Use `cargo +stable`, the same toolchain selection test-cedarling.yml already applies to its wasm clippy step for the same reason. Reproduced by removing the wasm target from 1.95.0 and building into a clean target dir: plain cargo fails as CI did, `cargo +stable` builds. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
|
@coderabbitai do you have any suggestions, or can you approve? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Repository guidance ---'
cat jans-cedarling/AGENTS.md
printf '%s\n' '--- Changed files ---'
git diff --name-status HEAD^ HEAD
printf '%s\n' '--- Core module outlines ---'
ast-grep outline jans-cedarling/sigstore-verifier/src/verifier.rs --view expanded
ast-grep outline jans-cedarling/sigstore-verifier/src/tlog.rs --view expanded
ast-grep outline jans-cedarling/sigstore-verifier/src/chain.rs --view expanded
ast-grep outline jans-cedarling/sigstore-verifier/src/sct.rs --view expanded
printf '%s\n' '--- Verification and trust-root implementation ---'
cat -n jans-cedarling/sigstore-verifier/src/verifier.rs
cat -n jans-cedarling/sigstore-verifier/src/tlog.rs
cat -n jans-cedarling/sigstore-verifier/src/chain.rs
cat -n jans-cedarling/sigstore-verifier/src/trust_root.rsLength of output: 204480 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Pull request diff against merge base ---'
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
git diff --stat "$base"...HEAD
git diff --name-status "$base"...HEAD
printf '%s\n' '--- Bundle and cryptographic verifier outlines ---'
ast-grep outline jans-cedarling/sigstore-verifier/src/bundle.rs --view expanded
ast-grep outline jans-cedarling/sigstore-verifier/src/crypto.rs --view expanded
printf '%s\n' '--- Bundle parser and signature primitive ---'
cat -n jans-cedarling/sigstore-verifier/src/bundle.rs
cat -n jans-cedarling/sigstore-verifier/src/crypto.rs
printf '%s\n' '--- Pull request checks and review state ---'
gh pr view 14636 --repo JanssenProject/jans \
--json number,state,isDraft,mergeable,reviewDecision,statusCheckRollup \
--jq '{number, state, isDraft, mergeable, reviewDecision, checks: [.statusCheckRollup[] | {name, status, conclusion}]}'Length of output: 39834 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Existing pull request reviews ---'
gh api repos/JanssenProject/jans/pulls/14636/reviews \
--paginate \
--jq '.[] | {user: .user.login, state: .state, submitted_at: .submitted_at, body: .body}'
printf '%s\n' '--- Existing pull request review comments ---'
gh api repos/JanssenProject/jans/pulls/14636/comments \
--paginate \
--jq '.[] | {user: .user.login, path: .path, line: .line, body: .body}'
printf '%s\n' '--- Digest algorithm handling and tests ---'
rg -n -C 4 'messageDigest|MessageDigest|SHA2_256|SHA2_384|algorithm' \
jans-cedarling/sigstore-verifier/src \
jans-cedarling/sigstore-verifier/testsLength of output: 221844 🤖 web_search executed:
Length of output: 13182
I cannot approve yet because GitHub still reports
The completed verifier checks are successful, including When the remaining checks pass and the existing change requests are resolved, ask You are interacting with an AI system. |
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