From 4853f671312782e871baad9ae745278f0cc78cd6 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 1 Sep 2025 16:25:31 +0000 Subject: [PATCH 1/6] test: remove deprecated StepRng The docs for rand::StepRng say "deprecated without replacement". In fact, according to https://github.com/rust-random/rand/pull/1634 which removed them, the "replacements" are SmallRng, or Pcg*, or Xoshiro*. That is, the replacement is a separate crate. `rand` exposes Xoshiro256PlusPlus as "SmallRng", if you enable a feature that enables the extra dep. Unfortunately we cannot do this for tests only, because Cargo does not support optional dev-dependencies. (Ten year old issue: https://github.com/rust-lang/cargo/issues/1596 .. last comment three years ago.) Directly use rand_xoshiro instead. --- Cargo-minimal.lock | 11 ++++++++++- Cargo-recent.lock | 11 ++++++++++- Cargo.toml | 2 +- src/key.rs | 30 +++++++++++++++--------------- src/schnorr.rs | 12 +++++++----- 5 files changed, 43 insertions(+), 23 deletions(-) diff --git a/Cargo-minimal.lock b/Cargo-minimal.lock index f9e9046fe..52a07d578 100644 --- a/Cargo-minimal.lock +++ b/Cargo-minimal.lock @@ -183,6 +183,15 @@ dependencies = [ "getrandom", ] +[[package]] +name = "rand_xoshiro" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f703f4665700daf5512dcca5f43afa6af89f09db47fb56be587f80636bda2d41" +dependencies = [ + "rand_core", +] + [[package]] name = "rustversion" version = "1.0.20" @@ -204,7 +213,7 @@ dependencies = [ "getrandom", "hex_lit", "rand", - "rand_core", + "rand_xoshiro", "secp256k1-sys", "serde", "serde_cbor", diff --git a/Cargo-recent.lock b/Cargo-recent.lock index 788f9181f..1fa979269 100644 --- a/Cargo-recent.lock +++ b/Cargo-recent.lock @@ -174,6 +174,15 @@ dependencies = [ "getrandom", ] +[[package]] +name = "rand_xoshiro" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f703f4665700daf5512dcca5f43afa6af89f09db47fb56be587f80636bda2d41" +dependencies = [ + "rand_core", +] + [[package]] name = "rustversion" version = "1.0.20" @@ -195,7 +204,7 @@ dependencies = [ "getrandom", "hex_lit", "rand", - "rand_core", + "rand_xoshiro", "secp256k1-sys", "serde", "serde_cbor", diff --git a/Cargo.toml b/Cargo.toml index 9603fb36f..f565a5933 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,7 +42,7 @@ rand = { version = "0.9", default-features = false, optional = true } serde = { version = "1.0.103", default-features = false, optional = true } [dev-dependencies] -rand_core = "0.9" +rand_xoshiro = { version = "0.7.0", default-features = false } serde_cbor = "0.10.0" serde_test = "1.0.19" bincode = "1.3.3" diff --git a/src/key.rs b/src/key.rs index a4ce0cee7..09b63c591 100644 --- a/src/key.rs +++ b/src/key.rs @@ -1709,7 +1709,9 @@ mod test { #[cfg(not(secp256k1_fuzz))] use hex_lit::hex; #[cfg(feature = "rand")] - use rand::{self, rngs::mock::StepRng, RngCore}; + use rand::{self, RngCore, SeedableRng as _}; + #[cfg(feature = "rand")] + use rand_xoshiro::Xoshiro128PlusPlus as SmallRng; use serde_test::{Configure, Token}; #[cfg(target_arch = "wasm32")] use wasm_bindgen_test::wasm_bindgen_test as test; @@ -1881,14 +1883,14 @@ mod test { #[cfg(all(feature = "rand", feature = "alloc"))] fn test_debug_output() { let s = Secp256k1::new(); - let (sk, _) = s.generate_keypair(&mut StepRng::new(1, 1)); + let (sk, _) = s.generate_keypair(&mut SmallRng::from_seed([0; 16])); - assert_eq!(&format!("{:?}", sk), "SecretKey(7ad2d060fb2971d6)"); + assert_eq!(&format!("{:?}", sk), "SecretKey(55f970894288f83a)"); let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2]; assert_eq!( to_hex(&sk[..], &mut buf).unwrap(), - "0100000000000000020000000000000003000000000000000400000000000000" + "a3da5346582b9273dd4a2bb83bbdfad9c398863cff589b1c4646accc16fde327" ); } @@ -1996,28 +1998,26 @@ mod test { } #[test] - // In fuzzing mode the Y coordinate is expected to match the X, so this - // test uses invalid public keys. #[cfg(not(secp256k1_fuzz))] #[cfg(all(feature = "alloc", feature = "rand"))] fn test_pubkey_serialize() { let s = Secp256k1::new(); - let (_, pk1) = s.generate_keypair(&mut StepRng::new(1, 1)); + let (_, pk1) = s.generate_keypair(&mut SmallRng::from_seed([1; 16])); assert_eq!( &pk1.serialize_uncompressed()[..], &[ - 4, 124, 121, 49, 14, 253, 63, 197, 50, 39, 194, 107, 17, 193, 219, 108, 154, 126, - 9, 181, 248, 2, 12, 149, 233, 198, 71, 149, 134, 250, 184, 154, 229, 185, 28, 165, - 110, 27, 3, 162, 126, 238, 167, 157, 242, 221, 76, 251, 237, 34, 231, 72, 39, 245, - 3, 191, 64, 111, 170, 117, 103, 82, 28, 102, 163 - ][..] + 4, 56, 223, 70, 19, 126, 3, 194, 155, 88, 167, 37, 54, 98, 138, 203, 136, 98, 66, + 247, 172, 101, 50, 193, 106, 108, 55, 252, 115, 176, 125, 88, 41, 21, 0, 223, 206, + 246, 198, 43, 80, 189, 247, 183, 48, 90, 209, 30, 195, 64, 214, 36, 109, 251, 121, + 60, 160, 172, 76, 9, 164, 224, 180, 189, 228 + ] ); assert_eq!( &pk1.serialize()[..], &[ - 3, 124, 121, 49, 14, 253, 63, 197, 50, 39, 194, 107, 17, 193, 219, 108, 154, 126, - 9, 181, 248, 2, 12, 149, 233, 198, 71, 149, 134, 250, 184, 154, 229 - ][..] + 2, 56, 223, 70, 19, 126, 3, 194, 155, 88, 167, 37, 54, 98, 138, 203, 136, 98, 66, + 247, 172, 101, 50, 193, 106, 108, 55, 252, 115, 176, 125, 88, 41 + ] ); } diff --git a/src/schnorr.rs b/src/schnorr.rs index 8afdb3770..d016a9030 100644 --- a/src/schnorr.rs +++ b/src/schnorr.rs @@ -454,16 +454,18 @@ mod tests { #[cfg(not(secp256k1_fuzz))] #[cfg(all(feature = "rand", feature = "alloc"))] fn test_pubkey_serialize() { - use rand::rngs::mock::StepRng; + use rand::SeedableRng as _; + use rand_xoshiro::Xoshiro128PlusPlus as SmallRng; + let secp = Secp256k1::new(); - let kp = Keypair::new(&secp, &mut StepRng::new(1, 1)); + let kp = Keypair::new(&secp, &mut SmallRng::from_seed([2; 16])); let (pk, _parity) = kp.x_only_public_key(); assert_eq!( &pk.serialize()[..], &[ - 124, 121, 49, 14, 253, 63, 197, 50, 39, 194, 107, 17, 193, 219, 108, 154, 126, 9, - 181, 248, 2, 12, 149, 233, 198, 71, 149, 134, 250, 184, 154, 229 - ][..] + 235, 200, 214, 152, 58, 148, 189, 127, 234, 11, 121, 32, 156, 24, 104, 237, 193, + 213, 193, 109, 109, 38, 46, 213, 160, 189, 210, 41, 17, 237, 208, 74 + ] ); } From 0619163ea2df49f70e0d30e36186af8a2506ec18 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 22 Jun 2025 15:54:33 +0000 Subject: [PATCH 2/6] tests: add API test This test is full of FIXMEs because our existing APIs are not very consistent, but this should at least help check that things are not getting worse. --- tests/api.rs | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 tests/api.rs diff --git a/tests/api.rs b/tests/api.rs new file mode 100644 index 000000000..2e279f18e --- /dev/null +++ b/tests/api.rs @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: CC0-1.0 + +#![allow(dead_code)] +#![allow(unused_imports)] + +use secp256k1::{ + ecdh, ecdsa, ellswift, schnorr, Keypair, Message, Parity, PublicKey, Scalar, SecretKey, + XOnlyPublicKey, +}; + +/// A struct that includes all public non-error enums. +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +struct Enums { + a: Parity, + b: ellswift::Party, + #[cfg(feature = "recovery")] + c: ecdsa::RecoveryId, +} + +/// A struct that includes all "public" (i.e. not secret key material) structures. +#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Copy, Clone)] +struct PublicStructs { + a: Message, + b: PublicKey, + c: XOnlyPublicKey, + d: ecdsa::Signature, + e: ecdsa::SerializedSignature, + #[cfg(feature = "recovery")] + f: ecdsa::RecoverableSignature, + g: ellswift::ElligatorSwift, + h: Scalar, + i: schnorr::Signature, +} + +/// A struct that includes all "secret" structures. +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +struct SecretStructs { + a: SecretKey, + b: ecdh::SharedSecret, + c: Keypair, + // FIXME should be renamed + d: ellswift::ElligatorSwiftSharedSecret, +} + +macro_rules! bytes_rtt_test { + ($name: ident, $ty:ty) => { + fn $name(obj: &$ty) { + let x = obj.to_byte_array(); + let y = <$ty>::from_byte_array(x); + assert_eq!(*obj, y); + } + }; +} + +// Message is special because its to/from methods havve the name "digest" in them +// PublicKey is special because it has two serialization forms with different names (but maybe I should rename them?) +// FIXME XOnlyPublicKey should pass this +// ecdsa::Signature and SerializedSignature and RecoverableSignature are variable-length +// FIXME ElligatorSwift should pass this +// Scalar has to_be_bytes and to_le_bytes (and corresponding froms) +bytes_rtt_test!(rtt_i, schnorr::Signature); + +macro_rules! secret_bytes_rtt_test { + ($name: ident, $ty:ty) => { + fn $name(obj: &$ty) { + // FIXME should have to_ prefix and probably as_ variant as well to minimize copies + let x = obj.secret_bytes(); + // FIXME should have a symmetric name to secret_bytes + let _ = <$ty>::from_byte_array(x); + obj.clone().non_secure_erase(); + } + }; +} +secret_bytes_rtt_test!(secret_rtt_a, SecretKey); +// FIXME ecdh::SharedSecret should pass this +// FIXME unsure about Keypair -- it currently only roundtrips through secret keys +// FIXME ellswift::ElligatorSwiftharedSecret should pass this From e86555ad3e2991cfa72a0fb7725b50b3a6c9f352 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 22 Jun 2025 18:18:03 +0000 Subject: [PATCH 3/6] move key.rs to key/mod.rs I'd like to pull the different key types into their own modules. It will be easier if we create a subdirector for them all. In the end we will re-export everything. --- src/{key.rs => key/mod.rs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/{key.rs => key/mod.rs} (100%) diff --git a/src/key.rs b/src/key/mod.rs similarity index 100% rename from src/key.rs rename to src/key/mod.rs From 9b84072757d3916969d7c8ed6ba08e3e749044cc Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 22 Jun 2025 18:42:33 +0000 Subject: [PATCH 4/6] move SecretKey into its own module Move-only, except for a couple import paths. --- src/key/mod.rs | 339 +------------------------------------------- src/key/secret.rs | 347 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 352 insertions(+), 334 deletions(-) create mode 100644 src/key/secret.rs diff --git a/src/key/mod.rs b/src/key/mod.rs index 09b63c591..a97860d01 100644 --- a/src/key/mod.rs +++ b/src/key/mod.rs @@ -3,7 +3,9 @@ //! Public and secret keys. //! -use core::ops::{self, BitXor}; +mod secret; + +use core::ops::BitXor; use core::{fmt, ptr, str}; #[cfg(feature = "arbitrary")] @@ -12,115 +14,17 @@ use secp256k1_sys::secp256k1_ec_pubkey_sort; #[cfg(feature = "serde")] use serde::ser::SerializeTuple; +pub use self::secret::SecretKey; use crate::ellswift::ElligatorSwift; use crate::ffi::types::c_uint; use crate::ffi::{self, CPtr}; -use crate::Error::{self, InvalidPublicKey, InvalidPublicKeySum, InvalidSecretKey}; +use crate::Error::{self, InvalidPublicKey, InvalidPublicKeySum}; #[cfg(feature = "global-context")] use crate::SECP256K1; use crate::{ constants, ecdsa, from_hex, schnorr, Message, Scalar, Secp256k1, Signing, Verification, }; -/// Secret key - a 256-bit key used to create ECDSA and Taproot signatures. -/// -/// This value should be generated using a [cryptographically secure pseudorandom number generator]. -/// -/// # Side channel attacks -/// -/// We have attempted to reduce the side channel attack surface by implementing a constant time `eq` -/// method. For similar reasons we explicitly do not implement `PartialOrd`, `Ord`, or `Hash` on -/// `SecretKey`. If you really want to order secret keys then you can use `AsRef` to get at the -/// underlying bytes and compare them - however this is almost certainly a bad idea. -/// -/// # Serde support -/// -/// Implements de/serialization with the `serde` feature enabled. We treat the byte value as a tuple -/// of 32 `u8`s for non-human-readable formats. This representation is optimal for some formats -/// (e.g. [`bincode`]) however other formats may be less optimal (e.g. [`cbor`]). -/// -/// # Examples -/// -/// Basic usage: -/// -/// ``` -/// # #[cfg(all(feature = "rand", feature = "std"))] { -/// use secp256k1::{rand, Secp256k1, SecretKey}; -/// -/// let secp = Secp256k1::new(); -/// let secret_key = SecretKey::new(&mut rand::rng()); -/// # } -/// ``` -/// [`bincode`]: https://docs.rs/bincode -/// [`cbor`]: https://docs.rs/cbor -/// [cryptographically secure pseudorandom number generator]: https://en.wikipedia.org/wiki/Cryptographically_secure_pseudorandom_number_generator -#[derive(Copy, Clone)] -pub struct SecretKey([u8; constants::SECRET_KEY_SIZE]); -impl_display_secret!(SecretKey); -impl_non_secure_erase!(SecretKey, 0, [1u8; constants::SECRET_KEY_SIZE]); - -impl PartialEq for SecretKey { - /// This implementation is designed to be constant time to help prevent side channel attacks. - #[inline] - fn eq(&self, other: &Self) -> bool { - let accum = self.0.iter().zip(&other.0).fold(0, |accum, (a, b)| accum | a ^ b); - unsafe { core::ptr::read_volatile(&accum) == 0 } - } -} - -impl Eq for SecretKey {} - -impl AsRef<[u8; constants::SECRET_KEY_SIZE]> for SecretKey { - /// Gets a reference to the underlying array. - /// - /// # Side channel attacks - /// - /// Using ordering functions (`PartialOrd`/`Ord`) on a reference to secret keys leaks data - /// because the implementations are not constant time. Doing so will make your code vulnerable - /// to side channel attacks. [`SecretKey::eq`] is implemented using a constant time algorithm, - /// please consider using it to do comparisons of secret keys. - #[inline] - fn as_ref(&self) -> &[u8; constants::SECRET_KEY_SIZE] { - let SecretKey(dat) = self; - dat - } -} - -impl ops::Index for SecretKey -where - [u8]: ops::Index, -{ - type Output = <[u8] as ops::Index>::Output; - - #[inline] - fn index(&self, index: I) -> &Self::Output { &self.0[index] } -} - -impl ffi::CPtr for SecretKey { - type Target = u8; - - fn as_c_ptr(&self) -> *const Self::Target { - let SecretKey(dat) = self; - dat.as_ptr() - } - - fn as_mut_c_ptr(&mut self) -> *mut Self::Target { - let &mut SecretKey(ref mut dat) = self; - dat.as_mut_ptr() - } -} - -impl str::FromStr for SecretKey { - type Err = Error; - fn from_str(s: &str) -> Result { - let mut res = [0u8; constants::SECRET_KEY_SIZE]; - match from_hex(s, &mut res) { - Ok(constants::SECRET_KEY_SIZE) => SecretKey::from_byte_array(res), - _ => Err(Error::InvalidSecretKey), - } - } -} - /// Public key - used to verify ECDSA signatures and to do Taproot tweaks. /// /// # Serde support @@ -184,239 +88,6 @@ impl str::FromStr for PublicKey { } } -impl SecretKey { - /// Generates a new random secret key. - /// - /// # Examples - /// - /// ``` - /// # #[cfg(all(feature = "std", feature = "rand"))] { - /// use secp256k1::{rand, SecretKey}; - /// let secret_key = SecretKey::new(&mut rand::rng()); - /// # } - /// ``` - #[inline] - #[cfg(feature = "rand")] - pub fn new(rng: &mut R) -> SecretKey { - let mut data = crate::random_32_bytes(rng); - unsafe { - while ffi::secp256k1_ec_seckey_verify( - ffi::secp256k1_context_no_precomp, - data.as_c_ptr(), - ) == 0 - { - data = crate::random_32_bytes(rng); - } - } - SecretKey(data) - } - - /// Converts a 32-byte slice to a secret key. - /// - /// # Examples - /// - /// ``` - /// use secp256k1::SecretKey; - /// let sk = SecretKey::from_slice(&[0xcd; 32]).expect("32 bytes, within curve order"); - /// ``` - #[deprecated(since = "0.31.0", note = "Use `from_byte_array` instead.")] - #[inline] - pub fn from_slice(data: &[u8]) -> Result { - match <[u8; constants::SECRET_KEY_SIZE]>::try_from(data) { - Ok(data) => Self::from_byte_array(data), - Err(_) => Err(InvalidSecretKey), - } - } - - /// Converts a 32-byte array to a secret key. - /// - /// # Examples - /// - /// ``` - /// use secp256k1::SecretKey; - /// let sk = SecretKey::from_byte_array([0xcd; 32]).expect("32 bytes, within curve order"); - /// ``` - #[inline] - pub fn from_byte_array(data: [u8; constants::SECRET_KEY_SIZE]) -> Result { - unsafe { - if ffi::secp256k1_ec_seckey_verify(ffi::secp256k1_context_no_precomp, data.as_c_ptr()) - == 0 - { - return Err(InvalidSecretKey); - } - } - Ok(SecretKey(data)) - } - - /// Creates a new secret key using data from BIP-340 [`Keypair`]. - /// - /// # Examples - /// - /// ``` - /// # #[cfg(all(feature = "rand", feature = "std"))] { - /// use secp256k1::{rand, Secp256k1, SecretKey, Keypair}; - /// - /// let secp = Secp256k1::new(); - /// let keypair = Keypair::new(&secp, &mut rand::rng()); - /// let secret_key = SecretKey::from_keypair(&keypair); - /// # } - /// ``` - #[inline] - pub fn from_keypair(keypair: &Keypair) -> Self { - let mut sk = [0u8; constants::SECRET_KEY_SIZE]; - unsafe { - let ret = ffi::secp256k1_keypair_sec( - ffi::secp256k1_context_no_precomp, - sk.as_mut_c_ptr(), - keypair.as_c_ptr(), - ); - debug_assert_eq!(ret, 1); - } - SecretKey(sk) - } - - /// Returns the secret key as a byte value. - #[inline] - pub fn secret_bytes(&self) -> [u8; constants::SECRET_KEY_SIZE] { self.0 } - - /// Negates the secret key. - #[inline] - #[must_use = "you forgot to use the negated secret key"] - pub fn negate(mut self) -> SecretKey { - unsafe { - let res = ffi::secp256k1_ec_seckey_negate( - ffi::secp256k1_context_no_precomp, - self.as_mut_c_ptr(), - ); - debug_assert_eq!(res, 1); - } - self - } - - /// Tweaks a [`SecretKey`] by adding `tweak` modulo the curve order. - /// - /// # Errors - /// - /// Returns an error if the resulting key would be invalid. - #[inline] - pub fn add_tweak(mut self, tweak: &Scalar) -> Result { - unsafe { - if ffi::secp256k1_ec_seckey_tweak_add( - ffi::secp256k1_context_no_precomp, - self.as_mut_c_ptr(), - tweak.as_c_ptr(), - ) != 1 - { - Err(Error::InvalidTweak) - } else { - Ok(self) - } - } - } - - /// Tweaks a [`SecretKey`] by multiplying by `tweak` modulo the curve order. - /// - /// # Errors - /// - /// Returns an error if the resulting key would be invalid. - #[inline] - pub fn mul_tweak(mut self, tweak: &Scalar) -> Result { - unsafe { - if ffi::secp256k1_ec_seckey_tweak_mul( - ffi::secp256k1_context_no_precomp, - self.as_mut_c_ptr(), - tweak.as_c_ptr(), - ) != 1 - { - Err(Error::InvalidTweak) - } else { - Ok(self) - } - } - } - - /// Constructs an ECDSA signature for `msg` using the global [`SECP256K1`] context. - #[inline] - #[cfg(feature = "global-context")] - pub fn sign_ecdsa(&self, msg: impl Into) -> ecdsa::Signature { - SECP256K1.sign_ecdsa(msg, self) - } - - /// Returns the [`Keypair`] for this [`SecretKey`]. - /// - /// This is equivalent to using [`Keypair::from_secret_key`]. - #[inline] - pub fn keypair(&self, secp: &Secp256k1) -> Keypair { - Keypair::from_secret_key(secp, self) - } - - /// Returns the [`PublicKey`] for this [`SecretKey`]. - /// - /// This is equivalent to using [`PublicKey::from_secret_key`]. - #[inline] - pub fn public_key(&self, secp: &Secp256k1) -> PublicKey { - PublicKey::from_secret_key(secp, self) - } - - /// Returns the [`XOnlyPublicKey`] (and its [`Parity`]) for this [`SecretKey`]. - /// - /// This is equivalent to `XOnlyPublicKey::from_keypair(self.keypair(secp))`. - #[inline] - pub fn x_only_public_key(&self, secp: &Secp256k1) -> (XOnlyPublicKey, Parity) { - let kp = self.keypair(secp); - XOnlyPublicKey::from_keypair(&kp) - } - - /// Constructor for unit testing. - #[cfg(test)] - #[cfg(all(feature = "rand", feature = "std"))] - pub fn test_random() -> Self { Self::new(&mut rand::rng()) } - - /// Constructor for unit testing. - #[cfg(test)] - #[cfg(not(all(feature = "rand", feature = "std")))] - pub fn test_random() -> Self { - loop { - if let Ok(ret) = Self::from_byte_array(crate::test_random_32_bytes()) { - return ret; - } - } - } -} - -#[cfg(feature = "serde")] -impl serde::Serialize for SecretKey { - fn serialize(&self, s: S) -> Result { - if s.is_human_readable() { - let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2]; - s.serialize_str(crate::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization")) - } else { - let mut tuple = s.serialize_tuple(constants::SECRET_KEY_SIZE)?; - for byte in self.0.iter() { - tuple.serialize_element(byte)?; - } - tuple.end() - } - } -} - -#[cfg(feature = "serde")] -impl<'de> serde::Deserialize<'de> for SecretKey { - fn deserialize>(d: D) -> Result { - if d.is_human_readable() { - d.deserialize_str(super::serde_util::FromStrVisitor::new( - "a hex string representing 32 byte SecretKey", - )) - } else { - let visitor = super::serde_util::Tuple32Visitor::new( - "raw 32 bytes SecretKey", - SecretKey::from_byte_array, - ); - d.deserialize_tuple(constants::SECRET_KEY_SIZE, visitor) - } - } -} - impl PublicKey { /// Obtains a raw const pointer suitable for use with FFI functions. #[inline] diff --git a/src/key/secret.rs b/src/key/secret.rs new file mode 100644 index 000000000..c480ad7a7 --- /dev/null +++ b/src/key/secret.rs @@ -0,0 +1,347 @@ +// SPDX-License-Identifier: CC0-1.0 +//! Secret signing keys. + +use core::{ops, str}; + +#[cfg(feature = "serde")] +use serde::ser::SerializeTuple; + +use crate::ffi::CPtr as _; +use crate::{ + constants, ffi, from_hex, Error, Keypair, Parity, PublicKey, Scalar, Secp256k1, Signing, + XOnlyPublicKey, +}; +#[cfg(feature = "global-context")] +use crate::{ecdsa, Message, SECP256K1}; + +/// Secret key - a 256-bit key used to create ECDSA and Taproot signatures. +/// +/// This value should be generated using a [cryptographically secure pseudorandom number generator]. +/// +/// # Side channel attacks +/// +/// We have attempted to reduce the side channel attack surface by implementing a constant time `eq` +/// method. For similar reasons we explicitly do not implement `PartialOrd`, `Ord`, or `Hash` on +/// `SecretKey`. If you really want to order secret keys then you can use `AsRef` to get at the +/// underlying bytes and compare them - however this is almost certainly a bad idea. +/// +/// # Serde support +/// +/// Implements de/serialization with the `serde` feature enabled. We treat the byte value as a tuple +/// of 32 `u8`s for non-human-readable formats. This representation is optimal for some formats +/// (e.g. [`bincode`]) however other formats may be less optimal (e.g. [`cbor`]). +/// +/// # Examples +/// +/// Basic usage: +/// +/// ``` +/// # #[cfg(all(feature = "rand", feature = "std"))] { +/// use secp256k1::{rand, Secp256k1, SecretKey}; +/// +/// let secp = Secp256k1::new(); +/// let secret_key = SecretKey::new(&mut rand::rng()); +/// # } +/// ``` +/// [`bincode`]: https://docs.rs/bincode +/// [`cbor`]: https://docs.rs/cbor +/// [cryptographically secure pseudorandom number generator]: https://en.wikipedia.org/wiki/Cryptographically_secure_pseudorandom_number_generator +#[derive(Copy, Clone)] +pub struct SecretKey([u8; constants::SECRET_KEY_SIZE]); +impl_display_secret!(SecretKey); +impl_non_secure_erase!(SecretKey, 0, [1u8; constants::SECRET_KEY_SIZE]); + +impl PartialEq for SecretKey { + /// This implementation is designed to be constant time to help prevent side channel attacks. + #[inline] + fn eq(&self, other: &Self) -> bool { + let accum = self.0.iter().zip(&other.0).fold(0, |accum, (a, b)| accum | a ^ b); + unsafe { core::ptr::read_volatile(&accum) == 0 } + } +} + +impl Eq for SecretKey {} + +impl AsRef<[u8; constants::SECRET_KEY_SIZE]> for SecretKey { + /// Gets a reference to the underlying array. + /// + /// # Side channel attacks + /// + /// Using ordering functions (`PartialOrd`/`Ord`) on a reference to secret keys leaks data + /// because the implementations are not constant time. Doing so will make your code vulnerable + /// to side channel attacks. [`SecretKey::eq`] is implemented using a constant time algorithm, + /// please consider using it to do comparisons of secret keys. + #[inline] + fn as_ref(&self) -> &[u8; constants::SECRET_KEY_SIZE] { + let SecretKey(dat) = self; + dat + } +} + +impl ops::Index for SecretKey +where + [u8]: ops::Index, +{ + type Output = <[u8] as ops::Index>::Output; + + #[inline] + fn index(&self, index: I) -> &Self::Output { &self.0[index] } +} + +impl ffi::CPtr for SecretKey { + type Target = u8; + + fn as_c_ptr(&self) -> *const Self::Target { + let SecretKey(dat) = self; + dat.as_ptr() + } + + fn as_mut_c_ptr(&mut self) -> *mut Self::Target { + let &mut SecretKey(ref mut dat) = self; + dat.as_mut_ptr() + } +} + +impl str::FromStr for SecretKey { + type Err = Error; + fn from_str(s: &str) -> Result { + let mut res = [0u8; constants::SECRET_KEY_SIZE]; + match from_hex(s, &mut res) { + Ok(constants::SECRET_KEY_SIZE) => SecretKey::from_byte_array(res), + _ => Err(Error::InvalidSecretKey), + } + } +} + +impl SecretKey { + /// Generates a new random secret key. + /// + /// # Examples + /// + /// ``` + /// # #[cfg(all(feature = "std", feature = "rand"))] { + /// use secp256k1::{rand, SecretKey}; + /// let secret_key = SecretKey::new(&mut rand::rng()); + /// # } + /// ``` + #[inline] + #[cfg(feature = "rand")] + pub fn new(rng: &mut R) -> SecretKey { + let mut data = crate::random_32_bytes(rng); + unsafe { + while ffi::secp256k1_ec_seckey_verify( + ffi::secp256k1_context_no_precomp, + data.as_c_ptr(), + ) == 0 + { + data = crate::random_32_bytes(rng); + } + } + SecretKey(data) + } + + /// Converts a 32-byte slice to a secret key. + /// + /// # Examples + /// + /// ``` + /// use secp256k1::SecretKey; + /// let sk = SecretKey::from_slice(&[0xcd; 32]).expect("32 bytes, within curve order"); + /// ``` + #[deprecated(since = "0.31.0", note = "Use `from_byte_array` instead.")] + #[inline] + pub fn from_slice(data: &[u8]) -> Result { + match <[u8; constants::SECRET_KEY_SIZE]>::try_from(data) { + Ok(data) => Self::from_byte_array(data), + Err(_) => Err(Error::InvalidSecretKey), + } + } + + /// Converts a 32-byte array to a secret key. + /// + /// # Examples + /// + /// ``` + /// use secp256k1::SecretKey; + /// let sk = SecretKey::from_byte_array([0xcd; 32]).expect("32 bytes, within curve order"); + /// ``` + #[inline] + pub fn from_byte_array(data: [u8; constants::SECRET_KEY_SIZE]) -> Result { + unsafe { + if ffi::secp256k1_ec_seckey_verify(ffi::secp256k1_context_no_precomp, data.as_c_ptr()) + == 0 + { + return Err(Error::InvalidSecretKey); + } + } + Ok(SecretKey(data)) + } + + /// Creates a new secret key using data from BIP-340 [`Keypair`]. + /// + /// # Examples + /// + /// ``` + /// # #[cfg(all(feature = "rand", feature = "std"))] { + /// use secp256k1::{rand, Secp256k1, SecretKey, Keypair}; + /// + /// let secp = Secp256k1::new(); + /// let keypair = Keypair::new(&secp, &mut rand::rng()); + /// let secret_key = SecretKey::from_keypair(&keypair); + /// # } + /// ``` + #[inline] + pub fn from_keypair(keypair: &Keypair) -> Self { + let mut sk = [0u8; constants::SECRET_KEY_SIZE]; + unsafe { + let ret = ffi::secp256k1_keypair_sec( + ffi::secp256k1_context_no_precomp, + sk.as_mut_c_ptr(), + keypair.as_c_ptr(), + ); + debug_assert_eq!(ret, 1); + } + SecretKey(sk) + } + + /// Returns the secret key as a byte value. + #[inline] + pub fn secret_bytes(&self) -> [u8; constants::SECRET_KEY_SIZE] { self.0 } + + /// Negates the secret key. + #[inline] + #[must_use = "you forgot to use the negated secret key"] + pub fn negate(mut self) -> SecretKey { + unsafe { + let res = ffi::secp256k1_ec_seckey_negate( + ffi::secp256k1_context_no_precomp, + self.as_mut_c_ptr(), + ); + debug_assert_eq!(res, 1); + } + self + } + + /// Tweaks a [`SecretKey`] by adding `tweak` modulo the curve order. + /// + /// # Errors + /// + /// Returns an error if the resulting key would be invalid. + #[inline] + pub fn add_tweak(mut self, tweak: &Scalar) -> Result { + unsafe { + if ffi::secp256k1_ec_seckey_tweak_add( + ffi::secp256k1_context_no_precomp, + self.as_mut_c_ptr(), + tweak.as_c_ptr(), + ) != 1 + { + Err(Error::InvalidTweak) + } else { + Ok(self) + } + } + } + + /// Tweaks a [`SecretKey`] by multiplying by `tweak` modulo the curve order. + /// + /// # Errors + /// + /// Returns an error if the resulting key would be invalid. + #[inline] + pub fn mul_tweak(mut self, tweak: &Scalar) -> Result { + unsafe { + if ffi::secp256k1_ec_seckey_tweak_mul( + ffi::secp256k1_context_no_precomp, + self.as_mut_c_ptr(), + tweak.as_c_ptr(), + ) != 1 + { + Err(Error::InvalidTweak) + } else { + Ok(self) + } + } + } + + /// Constructs an ECDSA signature for `msg` using the global [`SECP256K1`] context. + #[inline] + #[cfg(feature = "global-context")] + pub fn sign_ecdsa(&self, msg: impl Into) -> ecdsa::Signature { + SECP256K1.sign_ecdsa(msg, self) + } + + /// Returns the [`Keypair`] for this [`SecretKey`]. + /// + /// This is equivalent to using [`Keypair::from_secret_key`]. + #[inline] + pub fn keypair(&self, secp: &Secp256k1) -> Keypair { + Keypair::from_secret_key(secp, self) + } + + /// Returns the [`PublicKey`] for this [`SecretKey`]. + /// + /// This is equivalent to using [`PublicKey::from_secret_key`]. + #[inline] + pub fn public_key(&self, secp: &Secp256k1) -> PublicKey { + PublicKey::from_secret_key(secp, self) + } + + /// Returns the [`XOnlyPublicKey`] (and its [`Parity`]) for this [`SecretKey`]. + /// + /// This is equivalent to `XOnlyPublicKey::from_keypair(self.keypair(secp))`. + #[inline] + pub fn x_only_public_key(&self, secp: &Secp256k1) -> (XOnlyPublicKey, Parity) { + let kp = self.keypair(secp); + XOnlyPublicKey::from_keypair(&kp) + } + + /// Constructor for unit testing. + #[cfg(test)] + #[cfg(all(feature = "rand", feature = "std"))] + pub fn test_random() -> Self { Self::new(&mut rand::rng()) } + + /// Constructor for unit testing. + #[cfg(test)] + #[cfg(not(all(feature = "rand", feature = "std")))] + pub fn test_random() -> Self { + loop { + if let Ok(ret) = Self::from_byte_array(crate::test_random_32_bytes()) { + return ret; + } + } + } +} + +#[cfg(feature = "serde")] +impl serde::Serialize for SecretKey { + fn serialize(&self, s: S) -> Result { + if s.is_human_readable() { + let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2]; + s.serialize_str(crate::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization")) + } else { + let mut tuple = s.serialize_tuple(constants::SECRET_KEY_SIZE)?; + for byte in self.0.iter() { + tuple.serialize_element(byte)?; + } + tuple.end() + } + } +} + +#[cfg(feature = "serde")] +impl<'de> serde::Deserialize<'de> for SecretKey { + fn deserialize>(d: D) -> Result { + if d.is_human_readable() { + d.deserialize_str(crate::serde_util::FromStrVisitor::new( + "a hex string representing 32 byte SecretKey", + )) + } else { + let visitor = + crate::serde_util::Tuple32Visitor::new("raw 32 bytes SecretKey", |bytes| { + SecretKey::from_byte_array(bytes) + }); + d.deserialize_tuple(constants::SECRET_KEY_SIZE, visitor) + } + } +} From 41bb44294cb883740d938b16f0874424e095c018 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 1 Sep 2025 14:43:18 +0000 Subject: [PATCH 5/6] key: add {from,to,as}_secret_bytes methods Deprecate the from_byte_array method, which unfortunately we just added in 0.31.x. We may want to backport yet another deprecation. --- examples/sign_verify.rs | 2 +- examples/sign_verify_recovery.rs | 2 +- src/ecdsa/recovery.rs | 6 ++-- src/ellswift.rs | 16 ++++----- src/key/mod.rs | 58 +++++++------------------------- src/key/secret.rs | 38 ++++++++++----------- src/lib.rs | 10 +++--- src/scalar.rs | 2 +- src/secret.rs | 4 ++- tests/api.rs | 9 ++--- tests/serde.rs | 2 +- 11 files changed, 58 insertions(+), 91 deletions(-) diff --git a/examples/sign_verify.rs b/examples/sign_verify.rs index 8e0c33eb5..7879baff4 100644 --- a/examples/sign_verify.rs +++ b/examples/sign_verify.rs @@ -36,7 +36,7 @@ fn sign( seckey: [u8; 32], ) -> Result { let msg = Message::from_digest(msg_digest); - let seckey = SecretKey::from_byte_array(seckey)?; + let seckey = SecretKey::from_secret_bytes(seckey)?; Ok(secp.sign_ecdsa(msg, &seckey)) } diff --git a/examples/sign_verify_recovery.rs b/examples/sign_verify_recovery.rs index ec0f4606e..9a82aa923 100644 --- a/examples/sign_verify_recovery.rs +++ b/examples/sign_verify_recovery.rs @@ -15,7 +15,7 @@ fn sign_recovery( seckey: [u8; 32], ) -> Result { let msg = Message::from_digest(msg_digest); - let seckey = SecretKey::from_byte_array(seckey)?; + let seckey = SecretKey::from_secret_bytes(seckey)?; Ok(ecdsa::RecoverableSignature::sign_ecdsa_recoverable(msg, &seckey)) } diff --git a/src/ecdsa/recovery.rs b/src/ecdsa/recovery.rs index 8c0aac1bc..de674bd76 100644 --- a/src/ecdsa/recovery.rs +++ b/src/ecdsa/recovery.rs @@ -170,7 +170,7 @@ impl RecoverableSignature { let mut ret = ffi::RecoverableSignature::new(); // xor the secret key and message together to get a rerandomization seed // for timing analysis defense-in-depth - let mut rerandomize = sk.secret_bytes(); + let mut rerandomize = sk.to_secret_bytes(); for (rera, byte) in rerandomize.iter_mut().zip(msg[..].iter()) { *rera ^= *byte; } @@ -272,7 +272,7 @@ mod tests { #[cfg(not(secp256k1_fuzz))] // fixed sig vectors can't work with fuzz-sigs #[rustfmt::skip] fn sign() { - let sk = SecretKey::from_byte_array(ONE).unwrap(); + let sk = SecretKey::from_secret_bytes(ONE).unwrap(); let msg = Message::from_digest(ONE); let sig = RecoverableSignature::sign_ecdsa_recoverable(msg, &sk); @@ -292,7 +292,7 @@ mod tests { #[cfg(not(secp256k1_fuzz))] // fixed sig vectors can't work with fuzz-sigs #[rustfmt::skip] fn sign_with_noncedata() { - let sk = SecretKey::from_byte_array(ONE).unwrap(); + let sk = SecretKey::from_secret_bytes(ONE).unwrap(); let noncedata = [42u8; 32]; let msg = Message::from_digest(ONE); diff --git a/src/ellswift.rs b/src/ellswift.rs index 73a6688f3..fe640b779 100644 --- a/src/ellswift.rs +++ b/src/ellswift.rs @@ -111,7 +111,7 @@ impl ElligatorSwift { /// # #[cfg(feature = "alloc")] { /// use secp256k1::{ellswift::ElligatorSwift, PublicKey, Secp256k1, SecretKey}; /// let secp = Secp256k1::new(); - /// let sk = SecretKey::from_slice(&[1; 32]).unwrap(); + /// let sk = SecretKey::from_secret_bytes([1; 32]).unwrap(); /// let es = ElligatorSwift::from_seckey(&secp, sk, None); /// # } /// ``` @@ -140,7 +140,7 @@ impl ElligatorSwift { /// # #[cfg(feature = "alloc")] { /// use secp256k1::{ellswift::ElligatorSwift, PublicKey, Secp256k1, SecretKey}; /// let secp = Secp256k1::new(); - /// let sk = SecretKey::from_slice(&[1; 32]).unwrap(); + /// let sk = SecretKey::from_secret_bytes([1; 32]).unwrap(); /// let pk = PublicKey::from_secret_key(&secp, &sk); /// let es = ElligatorSwift::from_pubkey(pk); /// # } @@ -377,7 +377,7 @@ mod tests { // Test that we can round trip an ElligatorSwift encoding let secp = crate::Secp256k1::new(); let public_key = - PublicKey::from_secret_key(&secp, &SecretKey::from_byte_array([1u8; 32]).unwrap()); + PublicKey::from_secret_key(&secp, &SecretKey::from_secret_bytes([1u8; 32]).unwrap()); let ell = ElligatorSwift::from_pubkey(public_key); let pk = PublicKey::from_ellswift(ell); @@ -391,10 +391,10 @@ mod tests { let rand32 = [1u8; 32]; let priv32 = [1u8; 32]; let ell = - ElligatorSwift::from_seckey(&secp, SecretKey::from_byte_array(rand32).unwrap(), None); + ElligatorSwift::from_seckey(&secp, SecretKey::from_secret_bytes(rand32).unwrap(), None); let pk = PublicKey::from_ellswift(ell); let expected = - PublicKey::from_secret_key(&secp, &SecretKey::from_byte_array(priv32).unwrap()); + PublicKey::from_secret_key(&secp, &SecretKey::from_secret_bytes(priv32).unwrap()); assert_eq!(pk, expected); } @@ -407,13 +407,13 @@ mod tests { let priv32 = [2u8; 32]; let ell = ElligatorSwift::from_seckey( &secp, - SecretKey::from_byte_array(rand32).unwrap(), + SecretKey::from_secret_bytes(rand32).unwrap(), Some(rand32), ); let pk = ElligatorSwift::shared_secret_with_hasher( ell, ell, - SecretKey::from_byte_array(priv32).unwrap(), + SecretKey::from_secret_bytes(priv32).unwrap(), Party::Initiator, |_, _, _| ElligatorSwiftSharedSecret([0xff; 32]), ); @@ -627,7 +627,7 @@ mod tests { ElligatorSwift::from_array(ellswift_theirs), ) }; - let sec_key = SecretKey::from_byte_array(my_secret).unwrap(); + let sec_key = SecretKey::from_secret_bytes(my_secret).unwrap(); let initiator = if initiator == 0 { Party::Responder } else { Party::Initiator }; let shared = ElligatorSwift::shared_secret(el_a, el_b, sec_key, initiator); diff --git a/src/key/mod.rs b/src/key/mod.rs index a97860d01..36cfc8b6e 100644 --- a/src/key/mod.rs +++ b/src/key/mod.rs @@ -721,7 +721,7 @@ impl Keypair { let sk = SecretKey::test_random(); crate::with_global_context( |secp: &Secp256k1| Self::from_secret_key(secp, &sk), - Some(&sk.secret_bytes()), + Some(&sk.to_secret_bytes()), ) } } @@ -1347,7 +1347,7 @@ impl<'a> Arbitrary<'a> for SecretKey { } u.fill_buffer(&mut bytes[..])?; - if let Ok(sk) = SecretKey::from_byte_array(bytes) { + if let Ok(sk) = SecretKey::from_secret_bytes(bytes) { return Ok(sk); } } @@ -1391,16 +1391,6 @@ mod test { use crate::Error::{InvalidPublicKey, InvalidSecretKey}; use crate::{constants, from_hex, to_hex, Scalar}; - #[test] - #[allow(deprecated)] - fn skey_from_slice() { - let sk = SecretKey::from_slice(&[1; 31]); - assert_eq!(sk, Err(InvalidSecretKey)); - - let sk = SecretKey::from_slice(&[1; 32]); - assert!(sk.is_ok()); - } - #[test] fn pubkey_from_slice() { assert_eq!(PublicKey::from_slice(&[]), Err(InvalidPublicKey)); @@ -1424,7 +1414,7 @@ mod test { #[test] fn keypair_slice_round_trip() { let (sk1, pk1) = crate::test_random_keypair(); - assert_eq!(SecretKey::from_byte_array(sk1.secret_bytes()), Ok(sk1)); + assert_eq!(SecretKey::from_secret_bytes(sk1.to_secret_bytes()), Ok(sk1)); assert_eq!(PublicKey::from_slice(&pk1.serialize()[..]), Ok(pk1)); assert_eq!(PublicKey::from_slice(&pk1.serialize_uncompressed()[..]), Ok(pk1)); } @@ -1443,22 +1433,22 @@ mod test { #[rustfmt::skip] fn invalid_secret_key() { // Zero - assert_eq!(SecretKey::from_byte_array([0; 32]), Err(InvalidSecretKey)); + assert_eq!(SecretKey::from_secret_bytes([0; 32]), Err(InvalidSecretKey)); assert_eq!( SecretKey::from_str("0000000000000000000000000000000000000000000000000000000000000000"), Err(InvalidSecretKey) ); // -1 - assert_eq!(SecretKey::from_byte_array([0xff; 32]), Err(InvalidSecretKey)); + assert_eq!(SecretKey::from_secret_bytes([0xff; 32]), Err(InvalidSecretKey)); // Top of range - assert!(SecretKey::from_byte_array([ + assert!(SecretKey::from_secret_bytes([ 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFE, 0xBA, 0xAE, 0xDC, 0xE6, 0xAF, 0x48, 0xA0, 0x3B, 0xBF, 0xD2, 0x5E, 0x8C, 0xD0, 0x36, 0x41, 0x40, ]).is_ok()); // One past top of range - assert!(SecretKey::from_byte_array([ + assert!(SecretKey::from_secret_bytes([ 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFE, 0xBA, 0xAE, 0xDC, 0xE6, 0xAF, 0x48, 0xA0, 0x3B, @@ -1526,30 +1516,6 @@ mod test { assert_eq!(PublicKey::from_slice(&[]), Err(InvalidPublicKey)); } - #[test] - #[allow(deprecated)] - fn test_seckey_from_bad_slice() { - // Bad sizes - assert_eq!( - SecretKey::from_slice(&[0; constants::SECRET_KEY_SIZE - 1]), - Err(InvalidSecretKey) - ); - assert_eq!( - SecretKey::from_slice(&[0; constants::SECRET_KEY_SIZE + 1]), - Err(InvalidSecretKey) - ); - // Bad parse - assert_eq!( - SecretKey::from_slice(&[0xff; constants::SECRET_KEY_SIZE]), - Err(InvalidSecretKey) - ); - assert_eq!( - SecretKey::from_slice(&[0x00; constants::SECRET_KEY_SIZE]), - Err(InvalidSecretKey) - ); - assert_eq!(SecretKey::from_slice(&[]), Err(InvalidSecretKey)); - } - #[test] #[cfg(all(feature = "rand", feature = "alloc"))] fn test_debug_output() { @@ -1578,7 +1544,7 @@ mod test { #[cfg(not(secp256k1_fuzz))] let s = Secp256k1::signing_only(); - let sk = SecretKey::from_byte_array(SK_BYTES).expect("sk"); + let sk = SecretKey::from_secret_bytes(SK_BYTES).expect("sk"); // In fuzzing mode secret->public key derivation is different, so // hard-code the expected result. @@ -1925,7 +1891,7 @@ mod test { #[cfg(not(secp256k1_fuzz))] let s = Secp256k1::new(); - let sk = SecretKey::from_byte_array(SK_BYTES).unwrap(); + let sk = SecretKey::from_secret_bytes(SK_BYTES).unwrap(); // In fuzzing mode secret->public key derivation is different, so // hard-code the expected result. @@ -2064,7 +2030,7 @@ mod test { pk_bytes[0] = 0x02; // Use positive Y co-ordinate. pk_bytes[1..].clone_from_slice(&PK_BYTES); - let sk = SecretKey::from_byte_array(SK_BYTES).expect("failed to parse sk bytes"); + let sk = SecretKey::from_secret_bytes(SK_BYTES).expect("failed to parse sk bytes"); let pk = PublicKey::from_slice(&pk_bytes).expect("failed to create pk from iterator"); let kp = Keypair::from_secret_key(&secp, &sk); let xonly = @@ -2227,7 +2193,7 @@ mod test { fn test_keypair_from_str() { let keypair = Keypair::test_random(); let mut buf = [0_u8; constants::SECRET_KEY_SIZE * 2]; // Holds hex digits. - let s = to_hex(&keypair.secret_key().secret_bytes(), &mut buf).unwrap(); + let s = to_hex(&keypair.secret_key().to_secret_bytes(), &mut buf).unwrap(); let parsed_key = Keypair::from_str(s).unwrap(); assert_eq!(parsed_key, keypair); } @@ -2240,7 +2206,7 @@ mod test { serde_test::assert_tokens(&keypair.readable(), &[Token::String(sec_key_str)]); - let sec_key_bytes = keypair.secret_key().secret_bytes(); + let sec_key_bytes = keypair.secret_key().to_secret_bytes(); let tokens = std::iter::once(Token::Tuple { len: 32 }) .chain(sec_key_bytes.iter().copied().map(Token::U8)) .chain(std::iter::once(Token::TupleEnd)) diff --git a/src/key/secret.rs b/src/key/secret.rs index c480ad7a7..714791825 100644 --- a/src/key/secret.rs +++ b/src/key/secret.rs @@ -107,7 +107,7 @@ impl str::FromStr for SecretKey { fn from_str(s: &str) -> Result { let mut res = [0u8; constants::SECRET_KEY_SIZE]; match from_hex(s, &mut res) { - Ok(constants::SECRET_KEY_SIZE) => SecretKey::from_byte_array(res), + Ok(constants::SECRET_KEY_SIZE) => SecretKey::from_secret_bytes(res), _ => Err(Error::InvalidSecretKey), } } @@ -140,21 +140,10 @@ impl SecretKey { SecretKey(data) } - /// Converts a 32-byte slice to a secret key. - /// - /// # Examples - /// - /// ``` - /// use secp256k1::SecretKey; - /// let sk = SecretKey::from_slice(&[0xcd; 32]).expect("32 bytes, within curve order"); - /// ``` - #[deprecated(since = "0.31.0", note = "Use `from_byte_array` instead.")] - #[inline] - pub fn from_slice(data: &[u8]) -> Result { - match <[u8; constants::SECRET_KEY_SIZE]>::try_from(data) { - Ok(data) => Self::from_byte_array(data), - Err(_) => Err(Error::InvalidSecretKey), - } + /// Converts a 32-byte array to a secret key. + #[deprecated(since = "0.32.0", note = "use from_secret_bytes instead")] + pub fn from_byte_array(data: [u8; constants::SECRET_KEY_SIZE]) -> Result { + Self::from_secret_bytes(data) } /// Converts a 32-byte array to a secret key. @@ -163,10 +152,10 @@ impl SecretKey { /// /// ``` /// use secp256k1::SecretKey; - /// let sk = SecretKey::from_byte_array([0xcd; 32]).expect("32 bytes, within curve order"); + /// let sk = SecretKey::from_secret_bytes([0xcd; 32]).expect("32 bytes, within curve order"); /// ``` #[inline] - pub fn from_byte_array(data: [u8; constants::SECRET_KEY_SIZE]) -> Result { + pub fn from_secret_bytes(data: [u8; constants::SECRET_KEY_SIZE]) -> Result { unsafe { if ffi::secp256k1_ec_seckey_verify(ffi::secp256k1_context_no_precomp, data.as_c_ptr()) == 0 @@ -204,8 +193,17 @@ impl SecretKey { SecretKey(sk) } + /// Returns a reference to the secret key as a byte value. + #[inline] + pub fn as_secret_bytes(&self) -> &[u8; constants::SECRET_KEY_SIZE] { &self.0 } + + /// Returns the secret key as a byte value. + #[inline] + pub fn to_secret_bytes(&self) -> [u8; constants::SECRET_KEY_SIZE] { self.0 } + /// Returns the secret key as a byte value. #[inline] + #[deprecated(since = "0.32.0", note = "use to_secret_bytes instead")] pub fn secret_bytes(&self) -> [u8; constants::SECRET_KEY_SIZE] { self.0 } /// Negates the secret key. @@ -306,7 +304,7 @@ impl SecretKey { #[cfg(not(all(feature = "rand", feature = "std")))] pub fn test_random() -> Self { loop { - if let Ok(ret) = Self::from_byte_array(crate::test_random_32_bytes()) { + if let Ok(ret) = Self::from_secret_bytes(crate::test_random_32_bytes()) { return ret; } } @@ -339,7 +337,7 @@ impl<'de> serde::Deserialize<'de> for SecretKey { } else { let visitor = crate::serde_util::Tuple32Visitor::new("raw 32 bytes SecretKey", |bytes| { - SecretKey::from_byte_array(bytes) + SecretKey::from_secret_bytes(bytes) }); d.deserialize_tuple(constants::SECRET_KEY_SIZE, visitor) } diff --git a/src/lib.rs b/src/lib.rs index 393a96d5c..ee0fe5a98 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -80,7 +80,7 @@ //! # fn compute_hash(_: &[u8]) -> [u8; 32] { [0xab; 32] } //! //! let secp = Secp256k1::new(); -//! let secret_key = SecretKey::from_slice(&[0xcd; 32]).expect("32 bytes, within curve order"); +//! let secret_key = SecretKey::from_secret_bytes([0xcd; 32]).expect("32 bytes, within curve order"); //! let public_key = PublicKey::from_secret_key(&secp, &secret_key); //! // If the supplied byte slice was *not* the output of a cryptographic hash function this would //! // be cryptographically broken. It has been trivially used in the past to execute attacks. @@ -689,7 +689,7 @@ mod tests { // Check that we can produce keys from slices with no precomputation let pk_slice = &pk.serialize(); let new_pk = PublicKey::from_slice(pk_slice).unwrap(); - let new_sk = SecretKey::from_byte_array(sk.secret_bytes()).unwrap(); + let new_sk = SecretKey::from_secret_bytes(sk.to_secret_bytes()).unwrap(); assert_eq!(sk, new_sk); assert_eq!(pk, new_pk); } @@ -839,7 +839,7 @@ mod tests { wild_keys[1][0] -= 1; wild_msgs[1][0] -= 1; - for key in wild_keys.iter().copied().map(SecretKey::from_byte_array).map(Result::unwrap) { + for key in wild_keys.iter().copied().map(SecretKey::from_secret_bytes).map(Result::unwrap) { for msg in wild_msgs.into_iter().map(Message::from_digest) { let sig = s.sign_ecdsa(msg, &key); let low_r_sig = s.sign_ecdsa_low_r(msg, &key); @@ -1011,7 +1011,7 @@ mod tests { let s = Secp256k1::new(); let msg = Message::from_digest([1; 32]); - let sk = SecretKey::from_byte_array([2; 32]).unwrap(); + let sk = SecretKey::from_secret_bytes([2; 32]).unwrap(); let sig = s.sign_ecdsa(msg, &sk); static SIG_BYTES: [u8; 71] = [ 48, 69, 2, 33, 0, 157, 11, 173, 87, 103, 25, 211, 42, 231, 107, 237, 179, 76, 119, 72, @@ -1038,7 +1038,7 @@ mod tests { fn test_global_context() { use crate::SECP256K1; let sk_data = hex!("e6dd32f8761625f105c39a39f19370b3521d845a12456d60ce44debd0a362641"); - let sk = SecretKey::from_byte_array(sk_data).unwrap(); + let sk = SecretKey::from_secret_bytes(sk_data).unwrap(); let msg_data = hex!("a4965ca63b7d8562736ceec36dfa5a11bf426eb65be8ea3f7a49ae363032da0d"); let msg = Message::from_digest(msg_data); diff --git a/src/scalar.rs b/src/scalar.rs index 9cf444be5..c462c5d45 100644 --- a/src/scalar.rs +++ b/src/scalar.rs @@ -135,7 +135,7 @@ where } impl From for Scalar { - fn from(value: crate::SecretKey) -> Self { Scalar(value.secret_bytes()) } + fn from(value: crate::SecretKey) -> Self { Scalar(value.to_secret_bytes()) } } /// Error returned when the value of scalar is invalid - larger than the curve order. diff --git a/src/secret.rs b/src/secret.rs index 35f7b464b..8a93c8947 100644 --- a/src/secret.rs +++ b/src/secret.rs @@ -114,7 +114,9 @@ impl SecretKey { /// # } /// ``` #[inline] - pub fn display_secret(&self) -> DisplaySecret { DisplaySecret { secret: self.secret_bytes() } } + pub fn display_secret(&self) -> DisplaySecret { + DisplaySecret { secret: self.to_secret_bytes() } + } } impl Keypair { diff --git a/tests/api.rs b/tests/api.rs index 2e279f18e..9cfcf8e06 100644 --- a/tests/api.rs +++ b/tests/api.rs @@ -63,10 +63,11 @@ bytes_rtt_test!(rtt_i, schnorr::Signature); macro_rules! secret_bytes_rtt_test { ($name: ident, $ty:ty) => { fn $name(obj: &$ty) { - // FIXME should have to_ prefix and probably as_ variant as well to minimize copies - let x = obj.secret_bytes(); - // FIXME should have a symmetric name to secret_bytes - let _ = <$ty>::from_byte_array(x); + let x = obj.to_secret_bytes(); + let y = obj.as_secret_bytes(); + let z = obj.as_ref(); + let _ = y == z; + let _ = <$ty>::from_secret_bytes(x); obj.clone().non_secure_erase(); } }; diff --git a/tests/serde.rs b/tests/serde.rs index 7bc70cc24..63a9aea14 100644 --- a/tests/serde.rs +++ b/tests/serde.rs @@ -73,7 +73,7 @@ static MUSIG_PARTIAL_SIG_BYTES: [u8; 40] = [ ]; fn secret_key() -> SecretKey { - SecretKey::from_byte_array(SK_BYTES).expect("failed to create sk from slice") + SecretKey::from_secret_bytes(SK_BYTES).expect("failed to create sk from slice") } // Our current serde serialization implementation is only guaranteed to be fixed From 44dae89894f8e40069f717f5746206cb83703e06 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 22 Jun 2025 19:18:48 +0000 Subject: [PATCH 6/6] key/secret: encapsulate SecretKey in module that enforces invariants --- src/key/secret.rs | 227 +++++++++++++++++++++++++--------------------- 1 file changed, 122 insertions(+), 105 deletions(-) diff --git a/src/key/secret.rs b/src/key/secret.rs index 714791825..386c76bd7 100644 --- a/src/key/secret.rs +++ b/src/key/secret.rs @@ -14,48 +14,117 @@ use crate::{ #[cfg(feature = "global-context")] use crate::{ecdsa, Message, SECP256K1}; -/// Secret key - a 256-bit key used to create ECDSA and Taproot signatures. -/// -/// This value should be generated using a [cryptographically secure pseudorandom number generator]. -/// -/// # Side channel attacks -/// -/// We have attempted to reduce the side channel attack surface by implementing a constant time `eq` -/// method. For similar reasons we explicitly do not implement `PartialOrd`, `Ord`, or `Hash` on -/// `SecretKey`. If you really want to order secret keys then you can use `AsRef` to get at the -/// underlying bytes and compare them - however this is almost certainly a bad idea. -/// -/// # Serde support -/// -/// Implements de/serialization with the `serde` feature enabled. We treat the byte value as a tuple -/// of 32 `u8`s for non-human-readable formats. This representation is optimal for some formats -/// (e.g. [`bincode`]) however other formats may be less optimal (e.g. [`cbor`]). -/// -/// # Examples -/// -/// Basic usage: -/// -/// ``` -/// # #[cfg(all(feature = "rand", feature = "std"))] { -/// use secp256k1::{rand, Secp256k1, SecretKey}; -/// -/// let secp = Secp256k1::new(); -/// let secret_key = SecretKey::new(&mut rand::rng()); -/// # } -/// ``` -/// [`bincode`]: https://docs.rs/bincode -/// [`cbor`]: https://docs.rs/cbor -/// [cryptographically secure pseudorandom number generator]: https://en.wikipedia.org/wiki/Cryptographically_secure_pseudorandom_number_generator -#[derive(Copy, Clone)] -pub struct SecretKey([u8; constants::SECRET_KEY_SIZE]); -impl_display_secret!(SecretKey); -impl_non_secure_erase!(SecretKey, 0, [1u8; constants::SECRET_KEY_SIZE]); +mod encapsulate { + use crate::constants::SECRET_KEY_SIZE; + use crate::ffi::{self, CPtr}; + use crate::Error; + + /// Secret key - a 256-bit key used to create ECDSA and Taproot signatures. + /// + /// This value should be generated using a [cryptographically secure pseudorandom number generator]. + /// + /// # Side channel attacks + /// + /// We have attempted to reduce the side channel attack surface by implementing a constant time `eq` + /// method. For similar reasons we explicitly do not implement `PartialOrd`, `Ord`, or `Hash` on + /// `SecretKey`. If you really want to order secret keys then you can use `AsRef` to get at the + /// underlying bytes and compare them - however this is almost certainly a bad idea. + /// + /// # Serde support + /// + /// Implements de/serialization with the `serde` feature enabled. We treat the byte value as a tuple + /// of 32 `u8`s for non-human-readable formats. This representation is optimal for some formats + /// (e.g. [`bincode`]) however other formats may be less optimal (e.g. [`cbor`]). + /// + /// # Examples + /// + /// Basic usage: + /// + /// ``` + /// # #[cfg(all(feature = "rand", feature = "std"))] { + /// use secp256k1::{rand, Secp256k1, SecretKey}; + /// + /// let secp = Secp256k1::new(); + /// let secret_key = SecretKey::new(&mut rand::rng()); + /// # } + /// ``` + /// [`bincode`]: https://docs.rs/bincode + /// [`cbor`]: https://docs.rs/cbor + /// [cryptographically secure pseudorandom number generator]: https://en.wikipedia.org/wiki/Cryptographically_secure_pseudorandom_number_generator + #[derive(Copy, Clone)] + pub struct SecretKey([u8; SECRET_KEY_SIZE]); + // FIXME these two macro call should be moved outside of the encapsulate module + impl_display_secret!(SecretKey); + impl_non_secure_erase!(SecretKey, 0, [1u8; SECRET_KEY_SIZE]); + + impl SecretKey { + /// Returns the secret key as a byte value. + /// + /// # Side channel attacks + /// + /// Using ordering functions (`PartialOrd`/`Ord`) on a reference to secret keys leaks data + /// because the implementations are not constant time. Doing so will make your code vulnerable + /// to side channel attacks. [`SecretKey::eq`] is implemented using a constant time algorithm, + /// please consider using it to do comparisons of secret keys. + #[inline] + pub fn to_secret_bytes(&self) -> [u8; SECRET_KEY_SIZE] { self.0 } + + /// Returns a reference to the secret key as a byte array. + /// + /// See note on [`Self::to_secret_bytes`]. + #[inline] + pub fn as_secret_bytes(&self) -> &[u8; SECRET_KEY_SIZE] { &self.0 } + + /// Converts a 32-byte array to a secret key. + /// + /// See note on [`Self::to_secret_bytes`]. + /// + /// # Errors + /// + /// Returns an error when the secret key is invalid: when it is all-zeros or would exceed + /// the curve order when interpreted as a big-endian unsigned integer. + /// + /// # Examples + /// + /// ``` + /// use secp256k1::SecretKey; + /// let sk = SecretKey::from_byte_array([0xcd; 32]).expect("32 bytes, within curve order"); + /// ``` + #[inline] + pub fn from_secret_bytes(data: [u8; SECRET_KEY_SIZE]) -> Result { + crate::with_raw_global_context( + |ctx| unsafe { + if ffi::secp256k1_ec_seckey_verify(ctx.as_ptr(), data.as_c_ptr()) == 0 { + return Err(Error::InvalidSecretKey); + } + Ok(SecretKey(data)) + }, + None, + ) + } + } + + // Must be inside the `encapsulate` module since there is no way to obtain mutable + // access to the internal array outside of the module. + impl CPtr for SecretKey { + type Target = u8; + + fn as_c_ptr(&self) -> *const Self::Target { self.as_secret_bytes().as_ptr() } + + fn as_mut_c_ptr(&mut self) -> *mut Self::Target { self.0.as_mut_ptr() } + } +} +pub use encapsulate::SecretKey; impl PartialEq for SecretKey { /// This implementation is designed to be constant time to help prevent side channel attacks. #[inline] fn eq(&self, other: &Self) -> bool { - let accum = self.0.iter().zip(&other.0).fold(0, |accum, (a, b)| accum | a ^ b); + let accum = self + .as_secret_bytes() + .iter() + .zip(other.as_secret_bytes()) + .fold(0, |accum, (a, b)| accum | a ^ b); unsafe { core::ptr::read_volatile(&accum) == 0 } } } @@ -65,17 +134,9 @@ impl Eq for SecretKey {} impl AsRef<[u8; constants::SECRET_KEY_SIZE]> for SecretKey { /// Gets a reference to the underlying array. /// - /// # Side channel attacks - /// - /// Using ordering functions (`PartialOrd`/`Ord`) on a reference to secret keys leaks data - /// because the implementations are not constant time. Doing so will make your code vulnerable - /// to side channel attacks. [`SecretKey::eq`] is implemented using a constant time algorithm, - /// please consider using it to do comparisons of secret keys. + /// See note on [`Self::to_secret_bytes`]. #[inline] - fn as_ref(&self) -> &[u8; constants::SECRET_KEY_SIZE] { - let SecretKey(dat) = self; - dat - } + fn as_ref(&self) -> &[u8; constants::SECRET_KEY_SIZE] { self.as_secret_bytes() } } impl ops::Index for SecretKey @@ -85,26 +146,12 @@ where type Output = <[u8] as ops::Index>::Output; #[inline] - fn index(&self, index: I) -> &Self::Output { &self.0[index] } -} - -impl ffi::CPtr for SecretKey { - type Target = u8; - - fn as_c_ptr(&self) -> *const Self::Target { - let SecretKey(dat) = self; - dat.as_ptr() - } - - fn as_mut_c_ptr(&mut self) -> *mut Self::Target { - let &mut SecretKey(ref mut dat) = self; - dat.as_mut_ptr() - } + fn index(&self, index: I) -> &Self::Output { &self.as_secret_bytes()[index] } } impl str::FromStr for SecretKey { type Err = Error; - fn from_str(s: &str) -> Result { + fn from_str(s: &str) -> Result { let mut res = [0u8; constants::SECRET_KEY_SIZE]; match from_hex(s, &mut res) { Ok(constants::SECRET_KEY_SIZE) => SecretKey::from_secret_bytes(res), @@ -126,18 +173,13 @@ impl SecretKey { /// ``` #[inline] #[cfg(feature = "rand")] - pub fn new(rng: &mut R) -> SecretKey { - let mut data = crate::random_32_bytes(rng); - unsafe { - while ffi::secp256k1_ec_seckey_verify( - ffi::secp256k1_context_no_precomp, - data.as_c_ptr(), - ) == 0 - { - data = crate::random_32_bytes(rng); + pub fn new(rng: &mut R) -> Self { + loop { + let data = crate::random_32_bytes(rng); + if let Ok(key) = Self::from_secret_bytes(data) { + return key; } } - SecretKey(data) } /// Converts a 32-byte array to a secret key. @@ -146,26 +188,6 @@ impl SecretKey { Self::from_secret_bytes(data) } - /// Converts a 32-byte array to a secret key. - /// - /// # Examples - /// - /// ``` - /// use secp256k1::SecretKey; - /// let sk = SecretKey::from_secret_bytes([0xcd; 32]).expect("32 bytes, within curve order"); - /// ``` - #[inline] - pub fn from_secret_bytes(data: [u8; constants::SECRET_KEY_SIZE]) -> Result { - unsafe { - if ffi::secp256k1_ec_seckey_verify(ffi::secp256k1_context_no_precomp, data.as_c_ptr()) - == 0 - { - return Err(Error::InvalidSecretKey); - } - } - Ok(SecretKey(data)) - } - /// Creates a new secret key using data from BIP-340 [`Keypair`]. /// /// # Examples @@ -190,21 +212,13 @@ impl SecretKey { ); debug_assert_eq!(ret, 1); } - SecretKey(sk) + Self::from_secret_bytes(sk).expect("a valid Keypair has a valid SecretKey") } - /// Returns a reference to the secret key as a byte value. - #[inline] - pub fn as_secret_bytes(&self) -> &[u8; constants::SECRET_KEY_SIZE] { &self.0 } - - /// Returns the secret key as a byte value. - #[inline] - pub fn to_secret_bytes(&self) -> [u8; constants::SECRET_KEY_SIZE] { self.0 } - /// Returns the secret key as a byte value. #[inline] #[deprecated(since = "0.32.0", note = "use to_secret_bytes instead")] - pub fn secret_bytes(&self) -> [u8; constants::SECRET_KEY_SIZE] { self.0 } + pub fn secret_bytes(&self) -> [u8; constants::SECRET_KEY_SIZE] { self.to_secret_bytes() } /// Negates the secret key. #[inline] @@ -316,10 +330,13 @@ impl serde::Serialize for SecretKey { fn serialize(&self, s: S) -> Result { if s.is_human_readable() { let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2]; - s.serialize_str(crate::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization")) + s.serialize_str( + crate::to_hex(self.as_secret_bytes(), &mut buf) + .expect("fixed-size hex serialization"), + ) } else { let mut tuple = s.serialize_tuple(constants::SECRET_KEY_SIZE)?; - for byte in self.0.iter() { + for byte in self.as_secret_bytes().iter() { tuple.serialize_element(byte)?; } tuple.end()