From cf58640937d740951fb9c356e9043f630dc35d1f Mon Sep 17 00:00:00 2001 From: Christian Kamm Date: Sat, 6 Aug 2022 19:54:38 +0200 Subject: [PATCH] Keypair: implement clone() (#26248) * Keypair: implement clone() This was not implemented upstream in ed25519-dalek to force everyone to think twice before creating another copy of a potentially sensitive private key in memory. See https://github.com/dalek-cryptography/ed25519-dalek/issues/76 However, there are now 9 instances of Keypair::from_bytes(&keypair.to_bytes()) in the solana codebase and it would be preferable to have a function. In particular since this also comes up when writing programs and can cause users to either start messing with lifetimes or discover the from_bytes() workaround themselves. This patch opts to not implement the Clone trait. This avoids automatic use in order to preserve some of the original "let developers think twice about this" intention. * Use Keypair::clone --- core/src/replay_stage.rs | 4 ++-- local-cluster/src/local_cluster.rs | 16 +++++++--------- local-cluster/tests/local_cluster_slow_1.rs | 5 +++-- runtime/src/genesis_utils.rs | 5 ++--- sdk/src/signer/keypair.rs | 20 ++++++++++++++++++++ 5 files changed, 34 insertions(+), 16 deletions(-) diff --git a/core/src/replay_stage.rs b/core/src/replay_stage.rs index 8d2fec624..85fe23137 100644 --- a/core/src/replay_stage.rs +++ b/core/src/replay_stage.rs @@ -3522,7 +3522,7 @@ pub(crate) mod tests { hash::{hash, Hash}, instruction::InstructionError, poh_config::PohConfig, - signature::{Keypair, Signer}, + signature::{Keypair, KeypairInsecureClone, Signer}, system_transaction, transaction::TransactionError, }, @@ -3602,7 +3602,7 @@ pub(crate) mod tests { let my_pubkey = my_keypairs.node_keypair.pubkey(); let cluster_info = ClusterInfo::new( Node::new_localhost_with_pubkey(&my_pubkey).info, - Arc::new(Keypair::from_bytes(&my_keypairs.node_keypair.to_bytes()).unwrap()), + Arc::new(my_keypairs.node_keypair.clone()), SocketAddrSpace::Unspecified, ); assert_eq!(my_pubkey, cluster_info.id()); diff --git a/local-cluster/src/local_cluster.rs b/local-cluster/src/local_cluster.rs index 6f3b13e5e..aaf54bb6b 100644 --- a/local-cluster/src/local_cluster.rs +++ b/local-cluster/src/local_cluster.rs @@ -37,7 +37,7 @@ use { message::Message, poh_config::PohConfig, pubkey::Pubkey, - signature::{Keypair, Signer}, + signature::{Keypair, KeypairInsecureClone, Signer}, stake::{ config as stake_config, instruction as stake_instruction, state::{Authorized, Lockup}, @@ -55,6 +55,7 @@ use { collections::HashMap, io::{Error, ErrorKind, Result}, iter, + ops::Deref, path::{Path, PathBuf}, sync::{Arc, RwLock}, }, @@ -203,10 +204,8 @@ impl LocalCluster { if *in_genesis { Some(( ValidatorVoteKeypairs { - node_keypair: Keypair::from_bytes(&node_keypair.to_bytes()) - .unwrap(), - vote_keypair: Keypair::from_bytes(&vote_keypair.to_bytes()) - .unwrap(), + node_keypair: node_keypair.deref().clone(), + vote_keypair: vote_keypair.deref().clone(), stake_keypair: Keypair::new(), }, stake, @@ -265,9 +264,8 @@ impl LocalCluster { let mut leader_config = safe_clone_config(&config.validator_configs[0]); leader_config.rpc_addrs = Some((leader_node.info.rpc, leader_node.info.rpc_pubsub)); Self::sync_ledger_path_across_nested_config_fields(&mut leader_config, &leader_ledger_path); - let leader_keypair = Arc::new(Keypair::from_bytes(&leader_keypair.to_bytes()).unwrap()); - let leader_vote_keypair = - Arc::new(Keypair::from_bytes(&leader_vote_keypair.to_bytes()).unwrap()); + let leader_keypair = Arc::new(leader_keypair.clone()); + let leader_vote_keypair = Arc::new(leader_vote_keypair.clone()); let leader_server = Validator::new( leader_node, @@ -315,7 +313,7 @@ impl LocalCluster { .map(|keypairs| { ( keypairs.node_keypair.pubkey(), - Arc::new(Keypair::from_bytes(&keypairs.vote_keypair.to_bytes()).unwrap()), + Arc::new(keypairs.vote_keypair.clone()), ) }) .collect(); diff --git a/local-cluster/tests/local_cluster_slow_1.rs b/local-cluster/tests/local_cluster_slow_1.rs index dab4f7807..29a5f314c 100644 --- a/local-cluster/tests/local_cluster_slow_1.rs +++ b/local-cluster/tests/local_cluster_slow_1.rs @@ -29,12 +29,13 @@ use { clock::{Slot, MAX_PROCESSING_AGE}, hash::Hash, pubkey::Pubkey, - signature::{Keypair, Signer}, + signature::{KeypairInsecureClone, Signer}, }, solana_streamer::socket::SocketAddrSpace, solana_vote_program::{vote_state::MAX_LOCKOUT_HISTORY, vote_transaction}, std::{ collections::{BTreeSet, HashSet}, + ops::Deref, path::Path, sync::{ atomic::{AtomicBool, Ordering}, @@ -462,7 +463,7 @@ fn test_duplicate_shreds_broadcast_leader() { let (gossip_service, _tcp_listener, cluster_info) = gossip_service::make_gossip_node( // Need to use our validator's keypair to gossip EpochSlots and votes for our // node later. - Keypair::from_bytes(&node_keypair.to_bytes()).unwrap(), + node_keypair.deref().clone(), Some(&cluster.entry_point_info.gossip), &exit, None, diff --git a/runtime/src/genesis_utils.rs b/runtime/src/genesis_utils.rs index 5fec8cea1..73ab5c105 100644 --- a/runtime/src/genesis_utils.rs +++ b/runtime/src/genesis_utils.rs @@ -7,7 +7,7 @@ use { genesis_config::{ClusterType, GenesisConfig}, pubkey::Pubkey, rent::Rent, - signature::{Keypair, Signer}, + signature::{Keypair, KeypairInsecureClone, Signer}, stake::state::StakeState, system_program, }, @@ -109,8 +109,7 @@ pub fn create_genesis_config_with_vote_accounts_and_cluster_type( assert_eq!(voting_keypairs.len(), stakes.len()); let mint_keypair = Keypair::new(); - let voting_keypair = - Keypair::from_bytes(&voting_keypairs[0].borrow().vote_keypair.to_bytes()).unwrap(); + let voting_keypair = voting_keypairs[0].borrow().vote_keypair.clone(); let validator_pubkey = voting_keypairs[0].borrow().node_keypair.pubkey(); let genesis_config = create_genesis_config_with_leader_ex( diff --git a/sdk/src/signer/keypair.rs b/sdk/src/signer/keypair.rs index 4ee5c51b0..2bf454855 100644 --- a/sdk/src/signer/keypair.rs +++ b/sdk/src/signer/keypair.rs @@ -97,6 +97,26 @@ where } } +/// Allows Keypair cloning +/// +/// Note that the `Clone` trait is intentionally unimplemented because making a +/// second copy of sensitive secret keys in memory is usually a bad idea. +/// +/// Only use this in tests or when strictly required. +pub trait KeypairInsecureClone { + fn clone(&self) -> Self; +} + +impl KeypairInsecureClone for Keypair { + fn clone(&self) -> Self { + Self(ed25519_dalek::Keypair { + // This will never error since self is a valid keypair + secret: ed25519_dalek::SecretKey::from_bytes(self.0.secret.as_bytes()).unwrap(), + public: self.0.public, + }) + } +} + /// Reads a JSON-encoded `Keypair` from a `Reader` implementor pub fn read_keypair(reader: &mut R) -> Result> { let bytes: Vec = serde_json::from_reader(reader)?;