Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors key handling to better encapsulate key material within the crypto crate, reducing direct dependency on x25519-dalek types in downstream crates and clarifying symmetric vs asymmetric key usage.
Changes:
- Introduces internal
PrivateKey,PublicKey, andSymmetricKey32types incrypto, replacingSecretKey/raw[u8; 32]usage in updated call sites. - Updates X3DH and XEdDSA signing APIs to accept the new internal key types.
- Migrates
conversationsinbox handshake/intro/identity andPrivateV1Convoinitialization to the new key types and removesInstallationKeypairfromPrivateV1Convoconstructors.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
crypto/src/keys.rs |
Adds internal key newtypes and a generic symmetric key container with zeroize-on-drop. |
crypto/src/lib.rs |
Re-exports new key types from crypto public API. |
crypto/src/x3dh.rs |
Switches X3DH DH outputs/shared secret derivation to SymmetricKey32 and uses PrivateKey/PublicKey. |
crypto/src/xeddsa_sign.rs |
Updates signing/verification to use internal PrivateKey/PublicKey. |
conversations/src/crypto.rs |
Re-exports crypto crate key types for local use. |
conversations/src/identity.rs |
Stores identity secret as PrivateKey and updates accessors. |
conversations/src/inbox/introduction.rs |
Uses internal key types for intro bundle signing/verification and tests. |
conversations/src/inbox/handshake.rs |
Migrates handshake output to SymmetricKey32 and inputs to PrivateKey. |
conversations/src/inbox/handler.rs |
Stores ephemeral keys as PrivateKey and updates responder path to new PrivateV1Convo ctor. |
conversations/src/conversation/privatev1.rs |
Updates convo constructors and DR initialization plumbing to use SymmetricKey32 and PrivateKey. |
Comments suppressed due to low confidence (1)
crypto/src/x3dh.rs:46
derive_shared_secretconcatenates DH outputs into a plainVec<u8>(km). This leaves sensitive key material in heap memory without zeroization, which undermines the goal of safer key handling. Use a zeroizing container (e.g.,zeroize::Zeroizing<Vec<u8>>) or a fixed-size stack buffer and explicitly zero it after HKDF.
// Concatenate all DH outputs
let mut km = Vec::new();
km.extend_from_slice(dh1.as_bytes());
km.extend_from_slice(dh2.as_bytes());
km.extend_from_slice(dh3.as_bytes());
if let Some(dh4) = dh4 {
km.extend_from_slice(dh4.as_bytes());
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ); | ||
| let mut sr_convo = PrivateV1Convo::new_initiator(seed_key_saro, pub_raya); | ||
| let mut rs_convo = | ||
| PrivateV1Convo::new_responder(SymmetricKey32::from(seed_key_raya), &raya); |
There was a problem hiding this comment.
seed_key_raya is already a SymmetricKey32; calling SymmetricKey32::from(seed_key_raya) won’t compile because there’s no From<SymmetricKey32> impl. Pass seed_key_raya directly (or clone it if needed).
| PrivateV1Convo::new_responder(SymmetricKey32::from(seed_key_raya), &raya); | |
| PrivateV1Convo::new_responder(seed_key_raya, &raya); |
| // TODO: (P3) Rename; This accepts a Ephemeral key in most cases | ||
| let dh_self_installation_keypair = | ||
| InstallationKeyPair::from_secret_bytes(dh_self.DANGER_to_bytes()); | ||
| // TODO: Danger - Fix double-ratchets types to Accept SymmetricKey32 | ||
| let dr_state = | ||
| RatchetState::init_receiver(seed_key.DANGER_to_bytes(), dh_self_installation_keypair); |
There was a problem hiding this comment.
PrivateV1Convo::new_responder converts dh_self into raw secret bytes via DANGER_to_bytes() to build an InstallationKeyPair, which creates an extra copy of secret material that isn’t obviously zeroized. Consider adding/using an API that can construct InstallationKeyPair without exposing raw bytes (or ensures the temporary byte buffer is zeroized).
There was a problem hiding this comment.
Correct. The plan is to remove InstallationKeypair all together
crypto/src/keys.rs
Outdated
| } | ||
|
|
||
| /// Returns internal [u8; N]. | ||
| /// This function by passes zeroize_on_drop, and will be deprecated once all consumers have been migrated |
There was a problem hiding this comment.
Typo in doc comment: “by passes” should be “bypasses”.
| /// This function by passes zeroize_on_drop, and will be deprecated once all consumers have been migrated | |
| /// This function bypasses zeroize_on_drop, and will be deprecated once all consumers have been migrated |
This PR provides better handling of key material. It separates the key updates from #37 which have more alignment than the rest of the changes.
SecretKeystoSymmetricKey32keys for added clarity