fix(security): zeroize signer secret key material#345
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the security posture of the application by implementing a strategy to minimize the in-memory exposure of cryptographic secret keys. By utilizing zeroizing byte buffers, sensitive key material is actively cleared from memory when no longer strictly required, thereby mitigating risks associated with memory forensics or accidental leakage. This change ensures that long-lived private key data is not persistently held in its raw form, enhancing overall system resilience against certain types of attacks. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances security by integrating the zeroize crate to handle secret keys more securely. The KeyManager and Signer structs now store secret keys as Zeroizing<[u8; 32]>, ensuring they are zeroed out from memory when no longer needed. Signing operations reconstruct SecretKey instances on demand from these zeroized byte arrays. The review suggests extracting the secret key reconstruction logic in KeyManager into a helper method for improved clarity and consistency, and using SecretKey::from_slice for consistency across crates.
| fn sign_ecdsa(&self, msg: Message) -> BitcoinSignerResult<ECDSASignature> { | ||
| let signature = self.secp.sign_ecdsa(&msg, &self.sk); | ||
| let sk = SecretKey::from_slice(self.sk.as_ref()) | ||
| .map_err(|e| BitcoinError::SigningError(format!("Invalid cached secret key: {}", e)))?; | ||
| let signature = self.secp.sign_ecdsa(&msg, &sk); | ||
| Ok(signature) | ||
| } | ||
|
|
||
| fn sign_schnorr(&self, msg: Message) -> BitcoinSignerResult<SchnorrSignature> { | ||
| let signature = self.secp.sign_schnorr_no_aux_rand(&msg, &self.keypair); | ||
| let sk = SecretKey::from_slice(self.sk.as_ref()) | ||
| .map_err(|e| BitcoinError::SigningError(format!("Invalid cached secret key: {}", e)))?; | ||
| let keypair = Keypair::from_secret_key(&self.secp, &sk); | ||
| let signature = self.secp.sign_schnorr_no_aux_rand(&msg, &keypair); | ||
| Ok(signature) | ||
| } | ||
|
|
||
| fn get_public_key(&self) -> PublicKey { | ||
| self.sk.public_key(&self.secp) | ||
| SecretKey::from_slice(self.sk.as_ref()) | ||
| .map(|sk| sk.public_key(&self.secp)) | ||
| .expect("KeyManager secret key must be valid") | ||
| } |
There was a problem hiding this comment.
To improve code clarity and reduce duplication, consider extracting the logic for reconstructing the SecretKey into a private helper method within impl KeyManager. This pattern is already used in via_verifier/lib/via_musig2/src/lib.rs with signing_secret_key.
You could add a method like this to impl KeyManager:
fn reconstruct_secret_key(&self) -> BitcoinSignerResult<SecretKey> {
SecretKey::from_slice(self.sk.as_ref())
.map_err(|e| BitcoinError::SigningError(format!("Invalid cached secret key: {}", e)))
}Then, you can simplify sign_ecdsa, sign_schnorr, and get_public_key to use this new helper method.
| } | ||
|
|
||
| fn signing_secret_key(&self) -> Result<SecretKey, MusigError> { | ||
| SecretKey::from_byte_array(self.secret_key.as_ref()) |
There was a problem hiding this comment.
For consistency with core/lib/via_btc_client/src/signer/mod.rs, consider using SecretKey::from_slice instead of SecretKey::from_byte_array. The from_slice method is standard in secp256k1 and would make the code more uniform across the crates.
| SecretKey::from_byte_array(self.secret_key.as_ref()) | |
| SecretKey::from_slice(self.secret_key.as_ref()) |
|
@gemini-code-assist Thanks — addressed your review requests. Updated in latest commit (
Could you please take another look? |
There was a problem hiding this comment.
Code Review
This pull request integrates the zeroize crate to enhance the secure handling of private keys within both the KeyManager in via_btc_client and the Signer in via_musig2. The change involves storing secret keys as Zeroizing<[u8; 32]> and reconstructing SecretKey instances only when required for signing operations. A review comment highlights a potential panic and performance inefficiency in KeyManager::get_public_key due to the use of .expect() and repeated public key derivation, suggesting caching the public key as an improvement.
| fn get_public_key(&self) -> PublicKey { | ||
| self.sk.public_key(&self.secp) | ||
| self.signing_secret_key() | ||
| .map(|sk| sk.public_key(&self.secp)) | ||
| .expect("KeyManager secret key must be valid") | ||
| } |
There was a problem hiding this comment.
Using .expect() here introduces a potential panic if signing_secret_key() were to fail. While the key should be valid if constructed via KeyManager::new or KeyManager::default, it's safer and more efficient to avoid re-deriving the public key on every call.
A better approach is to cache the PublicKey in the KeyManager struct when it's created. This avoids the potential panic and is more performant.
This would involve:
- Adding a
pk: PublicKeyfield to theKeyManagerstruct. - In
KeyManager::newandKeyManager::default, deriving the public key once and storing it in the newpkfield. - This function can then simply return the stored public key:
self.pk.
|
@gemini-code-assist Thanks for the follow-up review — addressed. Updated in latest commit (
Please re-review. |
There was a problem hiding this comment.
Code Review
This pull request enhances security by integrating the zeroize crate to manage secret keys within the KeyManager and Signer structs. Secret keys are now stored using Zeroizing<[u8; 32]> to ensure their memory is zeroed upon dropping. The KeyManager struct has been refactored to store the public key directly and reconstruct the SecretKey and Keypair on demand for signing operations, improving memory hygiene and reducing the exposure of sensitive data. Additionally, the SecretKey creation method was updated from from_byte_array to from_slice in relevant functions. There is no feedback to provide.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #337 by reducing long-lived in-memory exposure of Bitcoin/secp256k1 secret key material. It migrates signer implementations to store secrets in zeroizing byte buffers and reconstruct SecretKey/Keypair values only at the point of signing.
Changes:
- Add
zeroizeas a dependency tovia_btc_clientandvia_musig2. - Store signer secret material as
Zeroizing<[u8; 32]>inKeyManagerandSigner. - Reconstruct ephemeral
SecretKey/Keypairvalues during signing instead of retaining long-livedSecretKey/Keypairfields.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
core/lib/via_btc_client/src/signer/mod.rs |
Switch KeyManager to store secret key bytes in Zeroizing<[u8; 32]> and derive ephemeral keys for ECDSA/Schnorr signing. |
core/lib/via_btc_client/Cargo.toml |
Add zeroize dependency. |
via_verifier/lib/via_musig2/src/lib.rs |
Store Signer secret as Zeroizing<[u8; 32]> and reconstruct SecretKey when creating nonces / partial signatures. |
via_verifier/lib/via_musig2/Cargo.toml |
Add zeroize dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Clone)] | ||
| pub struct KeyManager { | ||
| secp: Secp256k1<All>, | ||
| sk: SecretKey, | ||
| sk: Zeroizing<[u8; 32]>, | ||
| public_key: PublicKey, |
There was a problem hiding this comment.
KeyManager still derives Clone, which will duplicate the underlying Zeroizing<[u8; 32]> secret buffer whenever the struct is cloned. That increases the number of in-memory copies of the secret key material and undercuts the goal of minimizing exposure; consider removing Clone (it doesn’t appear required by BitcoinSigner) or implementing a custom clone strategy that doesn’t copy secret material (e.g., wrap the secret bytes in an Arc<Zeroizing<_>> if sharing is needed).
| let secret_key = secp256k1_musig2::SecretKey::from_slice(&private_key.inner.secret_bytes()) | ||
| .with_context(|| "Error to compute the coordinator sk")?; |
There was a problem hiding this comment.
This constructs a SecretKey from &private_key.inner.secret_bytes(), which creates a temporary [u8; 32] copy of the secret on the stack that is dropped without being zeroized. Since this PR is explicitly about reducing lingering key material, consider wrapping the secret_bytes() result in Zeroizing (or otherwise explicitly zeroizing that temporary) before calling SecretKey::from_slice.
| let secret_key = secp256k1_musig2::SecretKey::from_slice(&private_key.inner.secret_bytes()) | |
| .with_context(|| "Error to compute the coordinator sk")?; | |
| let secret_key_bytes = Zeroizing::new(private_key.inner.secret_bytes()); | |
| let secret_key = | |
| secp256k1_musig2::SecretKey::from_slice(&*secret_key_bytes) | |
| .with_context(|| "Error to compute the coordinator sk")?; |
| let private_key = PrivateKey::from_wif(private_key_wif)?; | ||
| let secret_key = | ||
| secp256k1_musig2::SecretKey::from_byte_array(&private_key.inner.secret_bytes()) | ||
| .with_context(|| "Error to compute the coordinator sk")?; | ||
| let secret_key = secp256k1_musig2::SecretKey::from_slice(&private_key.inner.secret_bytes()) | ||
| .with_context(|| "Error to compute the coordinator sk")?; | ||
| let secp = secp256k1_musig2::Secp256k1::new(); |
There was a problem hiding this comment.
Same as above: private_key.inner.secret_bytes() produces a temporary [u8; 32] secret copy that is not zeroized after SecretKey::from_slice returns. To align with the zeroization goal, use a Zeroizing<[u8; 32]> (or explicit zeroize() on the temp) for these bytes as well.
|
@copilot-pull-request-reviewer Thanks — addressed your comments in latest commit ( Changes made:
Please re-review. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot-pull-request-reviewer Thanks — addressed the 3 new suggestions in commit Changes:
Please re-review. @romanornr |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates the zeroize crate to improve the security of sensitive cryptographic material by ensuring secret keys are cleared from memory after use. The KeyManager and Signer structures were refactored to store secret keys within Zeroizing wrappers, and helper methods were added to safely retrieve SecretKey instances for signing. Furthermore, the KeyManager now caches the public key, and the default network in its Default implementation has been updated to Regtest. I have no feedback to provide as no review comments were present.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let signer = Signer::new( | ||
| Zeroizing::new(secret_key.secret_bytes()), | ||
| signer_index, | ||
| all_pubkeys.clone(), | ||
| None, |
There was a problem hiding this comment.
get_signer currently creates secret_key_bytes (Zeroizing) and later creates a second copy of the secret via Zeroizing::new(secret_key.secret_bytes()), and also clones all_pubkeys even though it’s no longer used after this call. This increases in-memory exposure and allocations unnecessarily; prefer reusing/moving the existing secret_key_bytes into Signer::new and pass all_pubkeys by value (no clone) after computing public_key in a narrower scope.
| Zeroizing::new(secret_key.secret_bytes()), | ||
| signer_index, | ||
| all_pubkeys.clone(), |
There was a problem hiding this comment.
Same issue as get_signer: this call site duplicates the secret key bytes with Zeroizing::new(secret_key.secret_bytes()) despite already having secret_key_bytes, and clones all_pubkeys unnecessarily. Reuse/move the original Zeroizing<[u8;32]> into Signer::new and avoid cloning the pubkey vector to reduce secret copies and allocations.
| Zeroizing::new(secret_key.secret_bytes()), | |
| signer_index, | |
| all_pubkeys.clone(), | |
| secret_key_bytes, | |
| signer_index, | |
| all_pubkeys, |
|
@copilot-pull-request-reviewer Thanks, addressed both new comments in commit d29c013. Changes made:
Please re-review. |
Review updateI did a full local pass on PR #345 in a checked-out branch ( What I checked:
Validation run:
Result:
From this pass, I don't see a blocker to merge. |
Summary
This addresses issue #337 by moving signer secret storage to zeroizing byte buffers and reconstructing ephemeral
SecretKeyvalues only when needed for signing.Changes
core/lib/via_btc_clientzeroizedependencyKeyManagersecret key asZeroizing<[u8; 32]>SecretKey/Keypairfor signing operationsvia_verifier/lib/via_musig2zeroizedependencySignersecret key asZeroizing<[u8; 32]>SecretKeywhen creating nonce / partial signatureWhy
Keypair/SecretKeyretention where not strictly necessaryCloses #337.