refactor: unify signature verification behind one pluggable provider - #154
Open
kaidokert wants to merge 3 commits into
Open
refactor: unify signature verification behind one pluggable provider#154kaidokert wants to merge 3 commits into
kaidokert wants to merge 3 commits into
Conversation
Every cert-signature algorithm (Ed25519, RSA, ECDSA-P256/P384, ML-DSA) now sits behind a single `SigVerifierProvider<A>` abstraction over a `SigAlgo` GAT descriptor, aggregated by `VerifierBackend`. This removes the asymmetry where Ed25519/RSA were pluggable provider type params but ECDSA and ML-DSA were hardcoded `PreparedVerifier` variants, and collapses the growing `<E, R, ...>` type-param list across the verify stack (`PreparedVerifier`, `VerifyStrategy`, `TrustRootDecision`, `SafeStrategy`, `verify_server_flight`, `ClientConfig`) to a single backend param `P`. `ClientConfig` gains one `Verifiers` slot in place of `Ed25519` + `Rsa`; adding an algorithm or backend is now one impl, not a new trait. Behaviour-preserving: RSA's PSS/PKCS1 dispatch folds onto a scheme-carrying `RsaSig`, and ECDSA's per-curve SHA prehash moves inside its `Verifier` impl (same hash + verify, relocated). Deletes the `Ed25519VerifierProvider` / `RsaVerifierProvider` traits. Default-build footprint +68 B on M3 (no new monomorphizations — the uniform fallible `prepare` on the formerly-infallible Ed25519 leaf-prep).
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_63181452-1291-4e34-ba24-163143b70230) |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="krabitls/src/traits/verify_strategy.rs" line_range="465" />
<code_context>
/// any parsed cert — a chain entry or a `PinnedRoots` stored anchor parsed from
/// flash — the dispatch is on `parent`'s key family, nothing is bound to "self".
-pub(crate) fn verify_link<E, R>(child: &CertView<'_>, parent: &CertView<'_>) -> Result<(), LinkErr>
+pub(crate) fn verify_link<P>(child: &CertView<'_>, parent: &CertView<'_>) -> Result<(), LinkErr>
where
- E: Ed25519VerifierProvider,
</code_context>
<issue_to_address>
**issue (broader_impact):** The new `P: VerifierBackend` parameter is ignored for ML-DSA and ECDSA certificate signatures: `verify_link` and `verify_self_sig` still construct `MlDsaVerifierKey` and call the hardcoded `verify_p256`/`verify_p384` functions directly. Custom providers therefore affect leaf CertificateVerify and RSA/Ed25519 checks but are bypassed for chain-link and self-signature verification.
**Triggers:** When a caller supplies a custom verifier backend for ML-DSA or ECDSA and verifies a chain or self-signed certificate.
**Suggested fix:** Prepare the ML-DSA/ECDSA verifier through the corresponding `SigVerifierProvider<A>` implementation and call its `signature::Verifier` implementation in these paths as well.
</issue_to_address>
### Comment 2
<location path="krabitls/src/traits/verify_provider.rs" line_range="3-6" />
<code_context>
+//! Unified per-algorithm signature-verification backend surface.
+//!
+//! One [`SigVerifierProvider`] per algorithm ([`SigAlgo`]) routes every cert /
+//! CertificateVerify signature through the same prepare→`Verifier` shape, so
+//! the verify stack threads a single [`VerifierBackend`] type param rather than
+//! one provider param per algorithm.
+
</code_context>
<issue_to_address>
**nitpick:** The module documentation claims one `SigVerifierProvider` routes every certificate signature through the uniform prepare-to-`Verifier` path, but ML-DSA and ECDSA chain-link and self-signature paths still bypass the provider and call concrete backends directly.
**Suggested fix:** Update the remaining ML-DSA and ECDSA certificate-signature paths to use `SigVerifierProvider`, or narrow the documentation until that migration is complete.
```suggestion
//! [`SigVerifierProvider`] defines a uniform prepare→`Verifier` shape for
//! provider-backed algorithms. The verify stack threads a single
//! [`VerifierBackend`] type param rather than one provider param per algorithm.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Coverage Report for CI Build 32823777884Coverage increased (+0.5%) to 80.414%Details
Uncovered Changes
Coverage Regressions5 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 765f8f6b1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
kaidokert
commented
Aug 25, 2026
Route the ECDSA and ML-DSA chain-link / self-sig verification through the pluggable provider `P`; they still called `verify_p256`/`verify_p384` and `MlDsaVerifierKey::new` directly, so a custom backend was honored only on the leaf. Export the per-algorithm markers (`Rsa`, `EcdsaP256`, `EcdsaP384`, `MlDsa`) from `krabitls::client` so downstream backends can name them. Un-gate `signature::Verifier` (it was `cfg(ecdsa)`, which broke the default non-ecdsa build once the Ed25519 arm started calling `.verify()`), and migrate `krabitls_cli` and `footprint` to the single `Verifiers` config slot.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_393c54ff-8c09-463f-9c06-f003905e3d5e) |
Fix a stale rustdoc link (`prepare_ed25519` → the `SigVerifierProvider::prepare` impl) and a `matches` cfg list that omitted the `ecdsa` build. Trim three what/how comments to the WHY they carry.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_9d208868-c9e5-4351-aa42-180716411dbf) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pure architecture/extensibility refactor of the certificate-signature verification layer — behaviour-preserving, no protocol change.
Problem
Verification was asymmetric: Ed25519 and RSA were pluggable via provider type params (
PreparedVerifier<E: Ed25519VerifierProvider, R: RsaVerifierProvider>), while ECDSA-P256/P384 and ML-DSA were hardcoded enum variants verified by direct backend calls. Every time an algorithm became pluggable the<E, R, …>type-param list grew, threaded through the whole verify stack.Design
One uniform abstraction for all five algorithms:
SigAlgo— the algorithm names its borrowedSig/Pubkey/KeyMattypes (GATs), shared across backends.SigVerifierProvider<A>— a backend prepares a per-key verifier; the prepared verifier is a standardsignature::Verifier<A::Sig>and exposes its key material for the SPKI cross-check.VerifierBackend— aggregates the per-algorithm bounds behind one bound (the single place per-algorithm feature gating lives), so the stack threads one backend param.Adding an algorithm or a backend is now one impl, not a new trait.
What changed
PreparedVerifier<E,R>→PreparedVerifier<P>; same collapse forTrusted,VerifyStrategy,TrustRootDecision,SafeStrategy,verify_server_flight. ECDSA and ML-DSA are now provider-backed variants like the others — no hardcoding.ClientConfig:Ed25519+Rsaslots → oneVerifiers: VerifierBackend(defaultRustCrypto).RsaSig; ECDSA's per-curve SHA prehash moves inside itsVerifierimpl (same hash + verify, relocated so verification is uniformverify(message, sig)).Ed25519VerifierProvider/RsaVerifierProvider.Verification
Default + all-features build, the full feature-combo matrix + lib tests,
clippy --all-targetszero-warnings, and the existing canned/mTLS server-cert verify tests (Ed25519 / RSA / ECDSA / ML-DSA incl. the ML-DSA scheme-binding rejection) all pass — the behaviour-preservation proof.Two failures are pre-existing on
main(verified independently), unrelated to this layer:--all-featurestest-compile trips thecarrier-bnum/carrier-crypto-bigintmutual-exclusion guard, andclient_hello_advertises_mldsa_schemesisn'tnot(ecdsa)-gated.Footprint
M3 default
.text+68 B (50,992 → 51,060). No new monomorphizations — the delta is the uniform fallibleprepareapplied to the formerly-infallible Ed25519 leaf-prep (its never-takenErrbranch doesn't fold atopt-level="z"). Clawing it back would mean special-casing Ed25519, defeating the uniformity.Summary by Sourcery
Replace the per-algorithm verification wiring with one extensible backend abstraction without changing protocol behavior.
Enhancements:
Tests:
Chores:
Summary by CodeRabbit
New Features
Improvements