feat(libsignal): let a consumer provide the X25519 agreement - #1218
Conversation
SignalCryptoProvider already lets a consumer supply AES-256-CBC, AES-256-GCM, HMAC-SHA256 and the transport AEAD, but the X25519 agreement was wired straight to the in-crate implementation, so it could not be routed anywhere. That gap is what blocks a consumer whose requirement is that every primitive run inside one approved module, and it also puts a platform's faster X25519 out of reach while the same consumer can already reach its faster AES. The trait gains x25519_agreement over raw 32-byte keys, with a default that runs the same code the agreement ran before, so existing trait implementations keep compiling and the default build produces the same bytes. Routing happens at PrivateKey::calculate_agreement, the one place the agreement is decided: keeping it there means the ratchet and X3DH call sites stay untouched and there is a single point to keep in sync with the provider.
|
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change routes X25519 agreement through the active crypto provider with a pure-Rust default. It adds typed provider-error propagation, preserves uncached signing behavior, and validates current and archived session decryption recovery. ChangesX25519 Provider Agreement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SessionCipher
participant x25519_agreement
participant SignalCryptoProvider
participant SignalProtocolError
SessionCipher->>x25519_agreement: Request X25519 agreement
x25519_agreement->>SignalCryptoProvider: Forward key material
SignalCryptoProvider-->>x25519_agreement: Return shared secret or provider error
x25519_agreement-->>SessionCipher: Return agreement result
SessionCipher->>SignalProtocolError: Convert provider failure
SignalProtocolError-->>SessionCipher: Return KeyAgreementFailed
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
| Filename | Overview |
|---|---|
| wacore/libsignal/src/core/curve.rs | Routes key agreement through the active provider while retaining the prior curve implementation as the default. |
| wacore/libsignal/src/crypto/provider.rs | Extends the provider contract with a backward-compatible, fallible X25519 hook. |
| wacore/libsignal/src/protocol/error.rs | Adds typed propagation of provider agreement failures into the protocol error surface. |
| wacore/libsignal/src/protocol/session_cipher.rs | Preserves backend agreement failures after candidate-session search while retaining duplicate-message precedence. |
| wacore/libsignal/tests/crypto_provider_agreement_failure.rs | Exercises refusal, recovery, archived-session restoration, and sibling-session fallback behavior. |
| wacore/libsignal/tests/crypto_provider_x25519_agreement.rs | Verifies provider dispatch, typed failures, and the effect of an inconsistent custom agreement. |
Sequence Diagram
sequenceDiagram
participant Session as X3DH / Double Ratchet
participant Key as PrivateKey
participant Crypto as crypto::x25519_agreement
participant Provider as SignalCryptoProvider
Session->>Key: calculate_agreement(peer_public)
Key->>Crypto: x25519_agreement(private, public)
Crypto->>Provider: x25519_agreement(private, public)
alt Agreement succeeds
Provider-->>Session: shared secret
else Backend refuses
Provider-->>Key: CryptoProviderError
Key-->>Session: CurveError::AgreementFailed
Session-->>Session: SignalProtocolError::KeyAgreementFailed
end
Reviews (6): Last reviewed commit: "fix(libsignal): let the candidate search..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@wacore/libsignal/tests/crypto_provider_x25519_agreement.rs`:
- Line 241: Update the test fixture’s Peer::new and bundle() flow to retain the
provided device_id alongside ProtocolAddress and pass that stored value into
PreKeyBundle::new instead of the hardcoded 1u32. Preserve the existing device-id
behavior for all peers while ensuring bundle() reports the peer’s actual device
id.
🪄 Autofix
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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 962515e3-00a5-4b7e-bbc0-5b82a96e377d
📒 Files selected for processing (4)
wacore/libsignal/src/core/curve.rswacore/libsignal/src/crypto/mod.rswacore/libsignal/src/crypto/provider.rswacore/libsignal/tests/crypto_provider_x25519_agreement.rs
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
![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: abdb0d27b6
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: This adds a security-sensitive override point to the public SignalCryptoProvider trait and changes when the provider global is resolved. Approving the new crypto API and its operational tradeoffs should go to a human.
Re-trigger cubic
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
A substituted agreement can be backed by something that refuses the operation, and an infallible return left such a provider two bad options: panic, or answer with fabricated bytes that a session would then build a root key from, surfacing much later as a MAC failure that points nowhere near the cause. x25519_agreement now returns Result. The trait default is still this crate's own implementation and still cannot fail, so a build without a custom provider is unchanged and existing implementations keep compiling. The error travels the path calculate_agreement already had: a new CurveError variant carrying the provider error, mapped onto a SignalProtocolError variant of its own, so a caller can tell "the backend refused" from "the peer key was malformed".
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e69916de07
ℹ️ 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".
There was a problem hiding this comment.
0 issues found across 5 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Adds a public crypto-provider override for X25519 and moves provider resolution into the DH path, changing a security-sensitive API and install-ordering contract; a human should approve this architectural/security tradeoff.
Re-trigger cubic
The candidate-session search collects each state's failure and reports one verdict for the message: duplicate, bad MAC, or "decryption failed". A backend that refuses the agreement mid-ratchet was landing in that pile and coming out as InvalidMessage, so the caller would ask the peer to resend a message that was never corrupt, and the real cause would be invisible. A refused agreement is not evidence about the message: every sibling state would ask the same backend and get the same answer. Return it as itself, the way an already-consumed body already does. Unreachable without a custom provider, since the default agreement cannot fail.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@wacore/libsignal/tests/crypto_provider_agreement_failure.rs`:
- Around line 384-403: Extend the agreement-failure test around the existing
ratcheted-message scenario to archive the live session before delivery, then
send and receive a message readable only by that archived session while REFUSING
is enabled. Assert the receive returns CryptoProviderError::BackendFailed and
verify record.previous_session_count() remains unchanged after the failure,
covering the archived-session restore_previous_session path.
🪄 Autofix
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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7456bd2-0f5b-4e1c-83f0-f136f979330f
📒 Files selected for processing (2)
wacore/libsignal/src/protocol/session_cipher.rswacore/libsignal/tests/crypto_provider_agreement_failure.rs
The first half of the test returns from the current-state arm, so the candidate loop over archived sessions never ran. Archiving the live session leaves no current state, which makes that loop the only path, and the message still decrypts afterwards: the session the loop borrowed has to come back where it was.
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Adds a pluggable X25519 agreement to the crypto provider trait with a default that preserves existing behavior byte-for-byte, backed by RFC 7748 vectors, backward-compat tests, and a new error path that correctly separates provider failures from message corruption.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae73eaca79
ℹ️ 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".
Returning the refusal on the spot was too eager. The backend is asked with the candidate's own ratchet key, and a sibling session that already has an open receiver chain agrees nothing at all, so a message that session could read was being reported as a backend failure and lost. The refusal goes back in the pile and the search continues. It then outranks the MAC-based verdicts when nothing could read the message, so a caller still tells "this backend is down" from "this message is corrupt", and a replay still wins over both.
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Adds a new X25519 agreement method to the crypto provider trait and modifies session decryption error prioritization. These changes affect the security contract of the provider and introduce a new error flow, which require human review of architectural and security tradeoffs.
Re-trigger cubic
|
CI note: Everything else that has finished is green: format, clippy, rustdoc, stable tests, no-simd tests, E2E, wasm32, cargo-deny, all four Miri jobs (including Generated by Claude Code |
Merging this PR will improve performance by 11.13%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
2 participants
Summary
SignalCryptoProvideris how a consumer swaps out our symmetric crypto: AES-256-CBC, AES-256-GCM, HMAC-SHA256 and the transport AEAD all route through it, but the X25519 agreement never did.PrivateKey::calculate_agreementcalled intocurve25519directly, so the implementation was fixed at compile time with no way around it from consumer code. That asymmetry costs anyone whose requirement is that every primitive run inside one approved cryptographic module (routing AES and HMAC but not the agreement is enough to sink the whole claim), and it puts a platform's faster X25519 out of reach while the same consumer can already reach its faster AES. The agreement is not a rare call either:RootKey::create_chainruns one per ratchet turn, which in an alternating conversation is per message. This addsx25519_agreementto the trait, with a default that is exactly the code the agreement ran before.Changes
crypto/provider.rs— newx25519_agreement(&self, private_key: &[u8; 32], their_public_key: &[u8; 32]) -> Result<[u8; 32], CryptoProviderError>onSignalCryptoProvider. Raw bytes rather thanPrivateKey/PublicKeyso the contract external implementors write against doesn't carry our internal key representation, and curve-specific in name so a future curve gets its own method instead of every implementor having to handle a discriminator it may never see.RustCryptoProviderdeliberately does not override it: the default already is the pure path, and a second copy would be a second thing to keep correct.calculate_agreementalready had: a newCurveError::AgreementFailedcarrying the provider error, mapped toSignalProtocolError::KeyAgreementFailed, so "the backend refused" is distinguishable from "the peer key was malformed".protocol/session_cipher.rs— a refused agreement now outranks the MAC-based verdicts in the decrypt candidate search. That search reports one verdict per message (duplicate, bad MAC, or "decryption failed"), and a backend failure landing in that pile came out asInvalidMessage, which would have the caller ask the peer to resend a message that was never corrupt. The refusal stays in the pile rather than returning on the spot, because the backend is asked with each candidate's own ratchet key and a sibling session with an open receiver chain agrees nothing at all — returning early there would lose a message that session could read. A replay still outranks both. Unreachable without a custom provider.core/curve.rs—PrivateKey::calculate_agreementnow resolves through the active provider. Samematch, sameCurveErrorresult type, same public signature;KeyPair::calculate_agreementdelegates here, andKeyTypehas one variant, so this is the single interception point. The ratchet and X3DH call sites are untouched on purpose: one behavior, one place to keep in sync. The pure body moved to apub(crate) fn x25519_agreementnext to it, which is what the trait default calls.crypto/mod.rs—pub fn x25519_agreementalongside the other delegating helpers, and the provider module doc now names the agreement in the list of primitives a provider owns.Checked while doing this:
from_bytes_without_cacheis still what the pure path uses, so routing does not start building the Edwards cache. Pinned by a test that asserts the signing cache stays cold after an agreement.provider()resolution can happen at the first DH instead of at the first symmetric operation. In the Noise handshake that is a few microseconds earlier in the same handshake (mix_shared_secretagrees, then mixes). A consumer that installs its provider before connecting is unaffected; one that installed it between the first DH and the first AES call would now getcrypto provider already set. Worth a release note.src/message/receive.rs) still treats an unrecognized decrypt error asUnhandledError. Nothing regresses today, since no build without a custom provider can produceKeyAgreementFailed, but a consumer installing a fallible backend will want that path to treat it as transient. Left for a change in the client, alongside whatever retry policy that consumer wants.Cost
Dispatch goes from a static, inlinable call to
&'static dyn. Measured with the crate's divan bench, a temporarybench_x25519_agreementdoing 10 agreements per iteration (added for the measurement, not part of this diff), 4 runs per variant, same machine back to back:Best-of-runs spread across all three is under 1 µs on 572 µs, ~0.1%, well inside the run-to-run noise (medians on this box wander between 590 µs and 850 µs). Neither the virtual call nor the
Resultshows up against the scalar multiplication.Validation
Tests added:
PrivateKey::calculate_agreementwith no provider installed, plus wrong-length key rejection: the default result is unchanged.tests/crypto_provider_x25519_agreement.rs(its own process becauseset_crypto_providerwrites a global): a provider whose agreement is observably used, with the counter moving and the returned bytes being the provider's. Its answer is deliberately order-dependent, so the two sides of a session derive different roots and decryption fails as a typedSignalProtocolErrorrather than a panic. A third test drives that provider into a backend failure and assertsCurveError::AgreementFailedandSignalProtocolError::KeyAgreementFailed, never bytes.tests/crypto_provider_agreement_failure.rs: a provider on the real primitive builds a live session, then refuses. Three shapes, all failing without thesession_cipher.rschange or passing for the wrong reason with an early return: refusal mid-DH-ratchet on the current session, the same through the archived-session half of the candidate search (with the archived session still in place afterwards), and a refusal that must not end the search — a sibling session with an open receiver chain reads the message with no agreement at all.tests/session_divergence.rs(baseline_dm_ping_pong,dh_ratchet_step_preserves_decryption); both still pass.Full matrix left to CI.