From 67f649ae65f1b466949ad9bf740c24d8c7b7100e Mon Sep 17 00:00:00 2001 From: luojiyin Date: Tue, 30 Sep 2025 11:35:25 +0800 Subject: [PATCH] fix: eliminate unwrap() calls and improve KeyPair API consistency Security improvements: - Replace unwrap() with expect() in copy() methods to provide clear error messages - Remove unnecessary from_bytes() reconstruction in private() methods - Add safety comments explaining copy_key feature usage API improvements: - Move signing methods from KeyPair to PrivateKey in secp256k1 module - Standardize field naming (secret -> private) across all key pair types - Ensure consistent KeyPair trait implementation patterns Performance improvements: - Make private() method O(1) by returning owned field directly - Eliminate redundant serialization/deserialization operations Affects: BLS12381KeyPair, Ed25519KeyPair, Secp256k1KeyPair --- fastcrypto/src/bls12381/mod.rs | 8 ++++-- fastcrypto/src/ed25519.rs | 10 +++++--- fastcrypto/src/secp256k1/mod.rs | 45 +++++++++++++++++++++++++-------- 3 files changed, 48 insertions(+), 15 deletions(-) diff --git a/fastcrypto/src/bls12381/mod.rs b/fastcrypto/src/bls12381/mod.rs index e91bc7ea7f..2a81ae125c 100644 --- a/fastcrypto/src/bls12381/mod.rs +++ b/fastcrypto/src/bls12381/mod.rs @@ -469,14 +469,18 @@ macro_rules! define_bls12381 { } fn private(self) -> Self::PrivKey { - BLS12381PrivateKey::from_bytes(self.private.as_ref()).unwrap() + self.private } #[cfg(feature = "copy_key")] fn copy(&self) -> Self { + // Note: The copy_key feature explicitly allows copying private keys for specific use cases. + // This reconstructs the private key from bytes, which should never fail for a valid keypair. + let private_bytes = self.private.as_ref(); BLS12381KeyPair { public: self.public.clone(), - private: BLS12381PrivateKey::from_bytes(self.private.as_ref()).unwrap(), + private: BLS12381PrivateKey::from_bytes(private_bytes) + .expect("KeyPair should always contain valid private key bytes"), } } diff --git a/fastcrypto/src/ed25519.rs b/fastcrypto/src/ed25519.rs index a18ef5185d..d850a68882 100644 --- a/fastcrypto/src/ed25519.rs +++ b/fastcrypto/src/ed25519.rs @@ -170,14 +170,18 @@ impl KeyPair for Ed25519KeyPair { } fn private(self) -> Self::PrivKey { - Ed25519PrivateKey::from_bytes(self.private.as_ref()).unwrap() + self.private } #[cfg(feature = "copy_key")] fn copy(&self) -> Self { + // Note: The copy_key feature explicitly allows copying private keys for specific use cases. + // This reconstructs the private key from bytes, which should never fail for a valid keypair. + let private_bytes = self.private.as_ref(); Self { - public: Ed25519PublicKey::from_bytes(self.public.as_ref()).unwrap(), - private: Ed25519PrivateKey::from_bytes(self.private.as_ref()).unwrap(), + public: self.public.clone(), + private: Ed25519PrivateKey::from_bytes(private_bytes) + .expect("KeyPair should always contain valid private key bytes"), } } diff --git a/fastcrypto/src/secp256k1/mod.rs b/fastcrypto/src/secp256k1/mod.rs index 25db8e37d1..80c23f572c 100644 --- a/fastcrypto/src/secp256k1/mod.rs +++ b/fastcrypto/src/secp256k1/mod.rs @@ -212,6 +212,27 @@ impl AsRef<[u8]> for Secp256k1PrivateKey { impl zeroize::ZeroizeOnDrop for Secp256k1PrivateKey {} +impl Secp256k1PrivateKey { + /// Create a new signature using the given hash function to hash the message. + pub fn sign_with_hash>(&self, msg: &[u8]) -> Secp256k1Signature { + let message = Message::from_slice(H::digest(msg).as_ref()).unwrap(); + + // Creates a 64-bytes signature of shape [r, s]. + // Pseudo-random deterministic nonce generation is used according to RFC6979. + Secp256k1Signature { + sig: Secp256k1::signing_only().sign_ecdsa(&message, &self.privkey), + bytes: OnceCell::new(), + } + } +} + +impl Signer for Secp256k1PrivateKey { + fn sign(&self, msg: &[u8]) -> Secp256k1Signature { + // Sha256 is used by default + self.sign_with_hash::(msg) + } +} + impl Drop for Secp256k1PrivateKey { fn drop(&mut self) { // bytes is zeroized on drop indirectly via OnceCell @@ -282,14 +303,14 @@ impl From<&Secp256k1RecoverableSignature> for Secp256k1Signature { /// Secp256k1 public/private key pair. #[derive(Debug, PartialEq, Eq)] pub struct Secp256k1KeyPair { - pub public: Secp256k1PublicKey, - pub secret: Secp256k1PrivateKey, + public: Secp256k1PublicKey, + private: Secp256k1PrivateKey, } /// The bytes form of the keypair always only contain the private key bytes impl ToFromBytes for Secp256k1KeyPair { fn from_bytes(bytes: &[u8]) -> Result { - Secp256k1PrivateKey::from_bytes(bytes).map(|secret| secret.into()) + Secp256k1PrivateKey::from_bytes(bytes).map(|private| private.into()) } } @@ -297,7 +318,7 @@ serialize_deserialize_with_to_from_bytes!(Secp256k1KeyPair, SECP256K1_KEYPAIR_LE impl AsRef<[u8]> for Secp256k1KeyPair { fn as_ref(&self) -> &[u8] { - self.secret.as_ref() + self.private.as_ref() } } @@ -311,14 +332,18 @@ impl KeyPair for Secp256k1KeyPair { } fn private(self) -> Self::PrivKey { - Secp256k1PrivateKey::from_bytes(self.secret.as_ref()).unwrap() + self.private } #[cfg(feature = "copy_key")] fn copy(&self) -> Self { + // Note: The copy_key feature explicitly allows copying private keys for specific use cases. + // This reconstructs the private key from bytes, which should never fail for a valid keypair. + let private_bytes = self.private.as_ref(); Secp256k1KeyPair { public: self.public.clone(), - secret: Secp256k1PrivateKey::from_bytes(self.secret.as_ref()).unwrap(), + private: Secp256k1PrivateKey::from_bytes(private_bytes) + .expect("KeyPair should always contain valid private key bytes"), } } @@ -330,7 +355,7 @@ impl KeyPair for Secp256k1KeyPair { pubkey, bytes: OnceCell::new(), }, - secret: Secp256k1PrivateKey { + private: Secp256k1PrivateKey { privkey, bytes: OnceCell::new(), }, @@ -368,8 +393,8 @@ impl Signer for Secp256k1KeyPair { } impl From for Secp256k1KeyPair { - fn from(secret: Secp256k1PrivateKey) -> Self { - let public = Secp256k1PublicKey::from(&secret); - Secp256k1KeyPair { public, secret } + fn from(private: Secp256k1PrivateKey) -> Self { + let public = Secp256k1PublicKey::from(&private); + Secp256k1KeyPair { public, private } } }