keypair / signature / ed25519: Deprecate dalek types from public API (#107)

* ed25519-program: Deprecate function taking dalek keypair

* signature: Add opaque error for signing uses

* keypair: Remove dalek crates from API

Author: joncinque

ed25519-program/src/lib.rs

@@ -37,7 +37,7 @@ pub struct Ed25519SignatureOffsets {
 /// buffer.
 ///
 /// Note: If the signer for these messages are the same, it is cheaper to concatenate the messages
-/// and have the signer sign the single buffer and use [`new_ed25519_instruction`].
+/// and have the signer sign the single buffer and use [`new_ed25519_instruction_with_signature`].
 pub fn offsets_to_ed25519_instruction(offsets: &[Ed25519SignatureOffsets]) -> Instruction {
     let mut instruction_data = Vec::with_capacity(
         SIGNATURE_OFFSETS_START
@@ -58,13 +58,18 @@ pub fn offsets_to_ed25519_instruction(offsets: &[Ed25519SignatureOffsets]) -> In
     }
 }
 
+#[deprecated(since = "2.2.3", note = "Use new_ed25519_instruction_with_signature")]
 pub fn new_ed25519_instruction(keypair: &ed25519_dalek::Keypair, message: &[u8]) -> Instruction {
     let signature = keypair.sign(message).to_bytes();
     let pubkey = keypair.public.to_bytes();
+    new_ed25519_instruction_with_signature(message, &signature, &pubkey)
+}
 
-    assert_eq!(pubkey.len(), PUBKEY_SERIALIZED_SIZE);
-    assert_eq!(signature.len(), SIGNATURE_SERIALIZED_SIZE);
-
+pub fn new_ed25519_instruction_with_signature(
+    message: &[u8],
+    signature: &[u8; SIGNATURE_SERIALIZED_SIZE],
+    pubkey: &[u8; PUBKEY_SERIALIZED_SIZE],
+) -> Instruction {
     let mut instruction_data = Vec::with_capacity(
         DATA_START
             .saturating_add(SIGNATURE_SERIALIZED_SIZE)
@@ -94,11 +99,11 @@ pub fn new_ed25519_instruction(keypair: &ed25519_dalek::Keypair, message: &[u8])
 
     debug_assert_eq!(instruction_data.len(), public_key_offset);
 
-    instruction_data.extend_from_slice(&pubkey);
+    instruction_data.extend_from_slice(pubkey);
 
     debug_assert_eq!(instruction_data.len(), signature_offset);
 
-    instruction_data.extend_from_slice(&signature);
+    instruction_data.extend_from_slice(signature);
 
     debug_assert_eq!(instruction_data.len(), message_data_offset);
 
@@ -444,7 +449,10 @@ pub mod test {
 
         let privkey = ed25519_dalek::Keypair::generate(&mut thread_rng());
         let message_arr = b"hello";
-        let mut instruction = new_ed25519_instruction(&privkey, message_arr);
+        let signature = privkey.sign(message_arr).to_bytes();
+        let pubkey = privkey.public.to_bytes();
+        let mut instruction =
+            new_ed25519_instruction_with_signature(message_arr, &signature, &pubkey);
         let mint_keypair = Keypair::new();
         let feature_set = FeatureSet::all_enabled();
 

keypair/Cargo.toml

@@ -18,7 +18,7 @@ solana-derivation-path = { workspace = true, optional = true }
 solana-pubkey = { workspace = true }
 solana-seed-derivable = { workspace = true, optional = true }
 solana-seed-phrase = { workspace = true }
-solana-signature = { workspace = true, features = ["verify"] }
+solana-signature = { workspace = true, features = ["std", "verify"] }
 solana-signer = { workspace = true }
 
 [target.'cfg(target_arch = "wasm32")'.dependencies]

keypair/src/lib.rs

@@ -7,7 +7,7 @@ use {
     rand0_7::{rngs::OsRng, CryptoRng, RngCore},
     solana_pubkey::Pubkey,
     solana_seed_phrase::generate_seed_from_seed_phrase_and_passphrase,
-    solana_signature::Signature,
+    solana_signature::{error::Error as SignatureError, Signature},
     solana_signer::{EncodableKey, EncodableKeypair, Signer, SignerError},
     std::{
         error,
@@ -25,6 +25,8 @@ pub mod signable;
 #[derive(Debug)]
 pub struct Keypair(ed25519_dalek::Keypair);
 
+pub const KEYPAIR_LENGTH: usize = 64;
+
 impl Keypair {
     /// Can be used for generating a Keypair without a dependency on `rand` types
     pub const SECRET_KEY_LENGTH: usize = 32;
@@ -44,34 +46,21 @@ impl Keypair {
     }
 
     /// Recovers a `Keypair` from a byte array
+    #[deprecated(since = "2.2.2", note = "Use Keypair::try_from(&[u8]) instead")]
     pub fn from_bytes(bytes: &[u8]) -> Result<Self, ed25519_dalek::SignatureError> {
-        if bytes.len() < ed25519_dalek::KEYPAIR_LENGTH {
-            return Err(ed25519_dalek::SignatureError::from_source(String::from(
-                "candidate keypair byte array is too short",
-            )));
-        }
-        let secret =
-            ed25519_dalek::SecretKey::from_bytes(&bytes[..ed25519_dalek::SECRET_KEY_LENGTH])?;
-        let public =
-            ed25519_dalek::PublicKey::from_bytes(&bytes[ed25519_dalek::SECRET_KEY_LENGTH..])?;
-        let expected_public = ed25519_dalek::PublicKey::from(&secret);
-        (public == expected_public)
-            .then_some(Self(ed25519_dalek::Keypair { secret, public }))
-            .ok_or(ed25519_dalek::SignatureError::from_source(String::from(
-                "keypair bytes do not specify same pubkey as derived from their secret key",
-            )))
+        Self::try_from(bytes).map_err(ed25519_dalek::SignatureError::from_source)
     }
 
     /// Returns this `Keypair` as a byte array
-    pub fn to_bytes(&self) -> [u8; 64] {
+    pub fn to_bytes(&self) -> [u8; KEYPAIR_LENGTH] {
         self.0.to_bytes()
     }
 
     /// Recovers a `Keypair` from a base58-encoded string
     pub fn from_base58_string(s: &str) -> Self {
         let mut buf = [0u8; ed25519_dalek::KEYPAIR_LENGTH];
         bs58::decode(s).onto(&mut buf).unwrap();
-        Self::from_bytes(&buf).unwrap()
+        Self::try_from(&buf[..]).unwrap()
     }
 
     /// Returns this `Keypair` as a base58-encoded string
@@ -80,10 +69,16 @@ impl Keypair {
     }
 
     /// Gets this `Keypair`'s SecretKey
+    #[deprecated(since = "2.2.2", note = "Use secret_bytes()")]
     pub fn secret(&self) -> &ed25519_dalek::SecretKey {
         &self.0.secret
     }
 
+    /// Gets this `Keypair`'s secret key bytes
+    pub fn secret_bytes(&self) -> &[u8; Self::SECRET_KEY_LENGTH] {
+        self.0.secret.as_bytes()
+    }
+
     /// Allows Keypair cloning
     ///
     /// Note that the `Clone` trait is intentionally unimplemented because making a
@@ -100,6 +95,30 @@ impl Keypair {
     }
 }
 
+impl TryFrom<&[u8]> for Keypair {
+    type Error = SignatureError;
+
+    fn try_from(bytes: &[u8]) -> Result<Self, Self::Error> {
+        if bytes.len() < ed25519_dalek::KEYPAIR_LENGTH {
+            return Err(SignatureError::from_source(String::from(
+                "candidate keypair byte array is too short",
+            )));
+        }
+        let secret =
+            ed25519_dalek::SecretKey::from_bytes(&bytes[..ed25519_dalek::SECRET_KEY_LENGTH])
+                .map_err(SignatureError::from_source)?;
+        let public =
+            ed25519_dalek::PublicKey::from_bytes(&bytes[ed25519_dalek::SECRET_KEY_LENGTH..])
+                .map_err(SignatureError::from_source)?;
+        let expected_public = ed25519_dalek::PublicKey::from(&secret);
+        (public == expected_public)
+            .then_some(Self(ed25519_dalek::Keypair { secret, public }))
+            .ok_or(SignatureError::from_source(String::from(
+                "keypair bytes do not specify same pubkey as derived from their secret key",
+            )))
+    }
+}
+
 #[cfg(target_arch = "wasm32")]
 #[allow(non_snake_case)]
 #[wasm_bindgen]
@@ -117,7 +136,7 @@ impl Keypair {
 
     /// Recover a `Keypair` from a `Uint8Array`
     pub fn fromBytes(bytes: &[u8]) -> Result<Keypair, JsValue> {
-        Keypair::from_bytes(bytes).map_err(|e| e.to_string().into())
+        Keypair::try_from(bytes).map_err(|e| e.to_string().into())
     }
 
     /// Return the `Pubkey` for this `Keypair`
@@ -128,6 +147,9 @@ impl Keypair {
     }
 }
 
+// This should also be marked deprecated, but it's not possible to put a
+// `#[deprecated]` attribute on a trait implementation.
+// Remove during the next major version bump.
 impl From<ed25519_dalek::Keypair> for Keypair {
     fn from(value: ed25519_dalek::Keypair) -> Self {
         Self(value)
@@ -223,7 +245,7 @@ pub fn read_keypair<R: Read>(reader: &mut R) -> Result<Keypair, Box<dyn error::E
         let parsed: u8 = element.parse()?;
         out[idx] = parsed;
     }
-    Keypair::from_bytes(&out)
+    Keypair::try_from(&out[..])
         .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()).into())
 }
 

signature/Cargo.toml

@@ -36,15 +36,16 @@ solana-short-vec = { workspace = true }
 solana-signature = { path = ".", features = ["serde"] }
 
 [features]
-default = ["std"]
+default = ["std", "alloc"]
+alloc = []
 frozen-abi = [
     "dep:solana-frozen-abi",
     "dep:solana-frozen-abi-macro",
     "std"
 ]
 rand = ["dep:rand"]
 serde = ["dep:serde", "dep:serde_derive", "dep:serde-big-array"]
-std = []
+std = ["alloc"]
 verify = ["dep:ed25519-dalek"]
 
 [package.metadata.docs.rs]

signature/src/error.rs

@@ -0,0 +1,86 @@
+//! Signature error copied directly from RustCrypto's opaque signature error at
+//! https://github.com/RustCrypto/traits/tree/master/signature
+
+#[cfg(feature = "alloc")]
+use alloc::boxed::Box;
+use core::fmt::{self, Debug, Display};
+
+/// Signature errors.
+///
+/// This type is deliberately opaque as to avoid sidechannel leakage which
+/// could potentially be used recover signing private keys or forge signatures
+/// (e.g. [BB'06]).
+///
+/// When the `std` feature is enabled, it impls [`std::error::Error`].
+///
+/// When the `alloc` feature is enabled, it supports an optional
+/// [`std::error::Error::source`], which can be used by things like remote
+/// signers (e.g. HSM, KMS) to report I/O or auth errors.
+///
+/// [BB'06]: https://en.wikipedia.org/wiki/Daniel_Bleichenbacher
+#[derive(Default)]
+#[non_exhaustive]
+pub struct Error {
+    /// Source of the error (if applicable).
+    #[cfg(feature = "std")]
+    source: Option<Box<dyn std::error::Error + Send + Sync + 'static>>,
+}
+
+impl Error {
+    /// Create a new error with an associated source.
+    ///
+    /// **NOTE:** The "source" should **NOT** be used to propagate cryptographic
+    /// errors e.g. signature parsing or verification errors. The intended use
+    /// cases are for propagating errors related to external signers, e.g.
+    /// communication/authentication errors with HSMs, KMS, etc.
+    #[cfg(feature = "std")]
+    pub fn from_source(
+        source: impl Into<Box<dyn std::error::Error + Send + Sync + 'static>>,
+    ) -> Self {
+        Self {
+            source: Some(source.into()),
+        }
+    }
+}
+
+impl Debug for Error {
+    #[cfg(not(feature = "std"))]
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str("signature::Error {}")
+    }
+
+    #[cfg(feature = "std")]
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str("signature::Error { source: ")?;
+
+        if let Some(source) = &self.source {
+            write!(f, "Some({})", source)?;
+        } else {
+            f.write_str("None")?;
+        }
+
+        f.write_str(" }")
+    }
+}
+
+impl Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str("signature error")
+    }
+}
+
+#[cfg(feature = "std")]
+impl From<Box<dyn std::error::Error + Send + Sync + 'static>> for Error {
+    fn from(source: Box<dyn std::error::Error + Send + Sync + 'static>) -> Error {
+        Self::from_source(source)
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for Error {
+    fn source(&self) -> Option<&(dyn core::error::Error + 'static)> {
+        self.source
+            .as_ref()
+            .map(|source| source.as_ref() as &(dyn core::error::Error + 'static))
+    }
+}

signature/src/lib.rs

@@ -8,6 +8,8 @@ use core::{
     fmt,
     str::{from_utf8, FromStr},
 };
+#[cfg(feature = "alloc")]
+extern crate alloc;
 #[cfg(feature = "std")]
 extern crate std;
 #[cfg(feature = "std")]
@@ -18,6 +20,8 @@ use {
     serde_derive::{Deserialize, Serialize},
 };
 
+pub mod error;
+
 /// Number of bytes in a signature
 pub const SIGNATURE_BYTES: usize = 64;
 /// Maximum string length of a base58 encoded signature