key/secret: encapsulate SecretKey in module that enforces invariants

Author: apoelstra

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<SecretKey, Error> {
+            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<I> ops::Index<I> for SecretKey
@@ -85,26 +146,12 @@ where
     type Output = <[u8] as ops::Index<I>>::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<SecretKey, Error> {
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
         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<R: rand::Rng + ?Sized>(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<R: rand::Rng + ?Sized>(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<SecretKey, Error> {
-        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<S: serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
         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()