From c93f9c351bdc9d3a5b33b4dc48a4572e6b4f0d79 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 19 May 2020 11:23:08 +1000 Subject: [PATCH] Improve bls::SecretKey privacy (#1164) * Improve bls::SecretKey privacy * Add missed file * Remove more methods from bls::SecretKey * Add as_bytes() to SecretKey, remove as_raw * Remove as_raw * Add back as_raw * Address review comments --- Cargo.lock | 4 +- beacon_node/operation_pool/src/lib.rs | 4 +- common/validator_dir/Cargo.toml | 2 - common/validator_dir/src/unencrypted_keys.rs | 44 ++---- common/validator_dir/tests/tests.rs | 6 +- .../block_processing_builder.rs | 2 +- consensus/state_processing/src/test_utils.rs | 2 +- .../examples/flamegraph_beacon_state.rs | 8 +- consensus/types/src/beacon_state/tests.rs | 8 +- .../builders/testing_beacon_state_builder.rs | 42 +----- .../types/src/test_utils/keypairs_file.rs | 128 ------------------ consensus/types/src/test_utils/mod.rs | 2 - crypto/bls/Cargo.toml | 1 + crypto/bls/src/keypair.rs | 3 +- crypto/bls/src/lib.rs | 2 + .../{eth2_keystore => bls}/src/plain_text.rs | 6 + crypto/bls/src/secret_key.rs | 55 ++------ crypto/eth2_keystore/src/keystore.rs | 4 +- crypto/eth2_keystore/src/lib.rs | 3 +- crypto/eth2_keystore/tests/eip2335_vectors.rs | 2 +- crypto/eth2_keystore/tests/tests.rs | 12 +- crypto/eth2_wallet/Cargo.toml | 1 - crypto/eth2_wallet/tests/tests.rs | 21 ++- lcli/src/main.rs | 2 +- lighthouse/tests/account_manager.rs | 12 +- validator_client/src/validator_store.rs | 21 ++- 26 files changed, 102 insertions(+), 295 deletions(-) delete mode 100644 consensus/types/src/test_utils/keypairs_file.rs rename crypto/{eth2_keystore => bls}/src/plain_text.rs (89%) diff --git a/Cargo.lock b/Cargo.lock index 65dc5b105..5da3ad0dd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -433,6 +433,7 @@ dependencies = [ "serde_derive", "serde_hex", "tree_hash", + "zeroize", ] [[package]] @@ -1334,7 +1335,6 @@ version = "0.1.0" dependencies = [ "eth2_key_derivation", "eth2_keystore", - "eth2_ssz", "hex 0.3.2", "rand 0.7.3", "serde", @@ -5294,8 +5294,6 @@ dependencies = [ "bls", "deposit_contract", "eth2_keystore", - "eth2_ssz", - "eth2_ssz_derive", "eth2_wallet", "rand 0.7.3", "rayon", diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index 9eb72ba55..79eb3f890 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -479,7 +479,7 @@ mod release_tests { let num_validators = num_committees * E::slots_per_epoch() as usize * spec.target_committee_size; let mut state_builder = - TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(num_validators, &spec); + TestingBeaconStateBuilder::from_deterministic_keypairs(num_validators, &spec); let slot_offset = 1000 * E::slots_per_epoch() + E::slots_per_epoch() / 2; let slot = spec.genesis_slot + slot_offset; state_builder.teleport_to_slot(slot); @@ -897,7 +897,7 @@ mod release_tests { let spec = MainnetEthSpec::default_spec(); let num_validators = 32; let mut state_builder = - TestingBeaconStateBuilder::::from_default_keypairs_file_if_exists( + TestingBeaconStateBuilder::::from_deterministic_keypairs( num_validators, &spec, ); diff --git a/common/validator_dir/Cargo.toml b/common/validator_dir/Cargo.toml index 7bb58c9b6..3ce0fea45 100644 --- a/common/validator_dir/Cargo.toml +++ b/common/validator_dir/Cargo.toml @@ -17,8 +17,6 @@ eth2_keystore = { path = "../../crypto/eth2_keystore" } types = { path = "../../consensus/types" } rand = "0.7.2" deposit_contract = { path = "../deposit_contract" } -eth2_ssz = { path = "../../consensus/ssz" } -eth2_ssz_derive = { path = "../../consensus/ssz_derive" } rayon = "1.3.0" tree_hash = { path = "../../consensus/tree_hash" } diff --git a/common/validator_dir/src/unencrypted_keys.rs b/common/validator_dir/src/unencrypted_keys.rs index 05f7eff09..53e5c5fdf 100644 --- a/common/validator_dir/src/unencrypted_keys.rs +++ b/common/validator_dir/src/unencrypted_keys.rs @@ -3,9 +3,8 @@ //! we're confident that no-one is using these keypairs anymore (hopefully mid-June 2020). #![cfg(feature = "unencrypted_keys")] +use bls::{BLS_PUBLIC_KEY_BYTE_SIZE as PK_LEN, BLS_SECRET_KEY_BYTE_SIZE as SK_LEN}; use eth2_keystore::PlainText; -use ssz::Decode; -use ssz_derive::{Decode, Encode}; use std::fs::File; use std::io::Read; use std::path::Path; @@ -32,35 +31,18 @@ pub fn load_unencrypted_keypair>(path: P) -> Result for SszEncodableKeypair { - fn into(self) -> Keypair { - Keypair { - sk: self.sk, - pk: self.pk, - } + if bytes.len() != PK_LEN + SK_LEN { + return Err(format!("Invalid keypair byte length: {}", bytes.len())); } -} -impl From for SszEncodableKeypair { - fn from(kp: Keypair) -> Self { - Self { - sk: kp.sk, - pk: kp.pk, - } - } + let pk_bytes = &bytes.as_bytes()[..PK_LEN]; + let sk_bytes = &bytes.as_bytes()[PK_LEN..]; + + let pk = PublicKey::from_bytes(pk_bytes) + .map_err(|e| format!("Unable to decode public key: {:?}", e))?; + + let sk = SecretKey::from_bytes(sk_bytes) + .map_err(|e| format!("Unable to decode secret key: {:?}", e))?; + + Ok(Keypair { pk, sk }) } diff --git a/common/validator_dir/tests/tests.rs b/common/validator_dir/tests/tests.rs index 221c1ae59..1458fca1c 100644 --- a/common/validator_dir/tests/tests.rs +++ b/common/validator_dir/tests/tests.rs @@ -118,7 +118,7 @@ impl Harness { check_keystore(&validator.dir().join(VOTING_KEYSTORE_FILE), &password_dir); if !config.random_voting_keystore { - assert_eq!(voting_keypair, generate_deterministic_keypair(0)) + assert_eq!(voting_keypair.pk, generate_deterministic_keypair(0).pk) } // Use OR here instead of AND so we *always* check for the withdrawal keystores if random @@ -128,11 +128,11 @@ impl Harness { let withdrawal_keypair = check_keystore(&withdrawal_keystore_path, &password_dir); if !config.random_withdrawal_keystore { - assert_eq!(withdrawal_keypair, generate_deterministic_keypair(1)) + assert_eq!(withdrawal_keypair.pk, generate_deterministic_keypair(1).pk) } // The withdrawal keys should be distinct from the voting keypairs. - assert_ne!(withdrawal_keypair, voting_keypair); + assert_ne!(withdrawal_keypair.pk, voting_keypair.pk); } if !config.store_withdrawal_keystore && !config.random_withdrawal_keystore { diff --git a/consensus/state_processing/src/per_block_processing/block_processing_builder.rs b/consensus/state_processing/src/per_block_processing/block_processing_builder.rs index 1fca43f68..dbdd1fc5e 100644 --- a/consensus/state_processing/src/per_block_processing/block_processing_builder.rs +++ b/consensus/state_processing/src/per_block_processing/block_processing_builder.rs @@ -15,7 +15,7 @@ pub struct BlockProcessingBuilder<'a, T: EthSpec> { impl<'a, T: EthSpec> BlockProcessingBuilder<'a, T> { pub fn new(num_validators: usize, state_slot: Slot, spec: &'a ChainSpec) -> Self { let mut state_builder = - TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(num_validators, &spec); + TestingBeaconStateBuilder::from_deterministic_keypairs(num_validators, &spec); state_builder.teleport_to_slot(state_slot); let (state, keypairs) = state_builder.build(); let block_builder = TestingBeaconBlockBuilder::new(spec); diff --git a/consensus/state_processing/src/test_utils.rs b/consensus/state_processing/src/test_utils.rs index 20e792343..e54a936ed 100644 --- a/consensus/state_processing/src/test_utils.rs +++ b/consensus/state_processing/src/test_utils.rs @@ -22,7 +22,7 @@ pub struct BlockBuilder { impl BlockBuilder { pub fn new(num_validators: usize, spec: &ChainSpec) -> Self { let state_builder = - TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(num_validators, &spec); + TestingBeaconStateBuilder::from_deterministic_keypairs(num_validators, &spec); let block_builder = TestingBeaconBlockBuilder::new(spec); Self { diff --git a/consensus/tree_hash/examples/flamegraph_beacon_state.rs b/consensus/tree_hash/examples/flamegraph_beacon_state.rs index ac085eccf..f4934ec83 100644 --- a/consensus/tree_hash/examples/flamegraph_beacon_state.rs +++ b/consensus/tree_hash/examples/flamegraph_beacon_state.rs @@ -5,11 +5,9 @@ const TREE_HASH_LOOPS: usize = 1_000; const VALIDATOR_COUNT: usize = 1_000; fn build_state(validator_count: usize) -> BeaconState { - let (state, _keypairs) = TestingBeaconStateBuilder::from_default_keypairs_file_if_exists( - validator_count, - &T::default_spec(), - ) - .build(); + let (state, _keypairs) = + TestingBeaconStateBuilder::from_deterministic_keypairs(validator_count, &T::default_spec()) + .build(); assert_eq!(state.validators.len(), validator_count); assert_eq!(state.balances.len(), validator_count); diff --git a/consensus/types/src/beacon_state/tests.rs b/consensus/types/src/beacon_state/tests.rs index 5e5f0f8ce..2fd8f27ca 100644 --- a/consensus/types/src/beacon_state/tests.rs +++ b/consensus/types/src/beacon_state/tests.rs @@ -11,7 +11,7 @@ fn test_beacon_proposer_index() { // Build a state for testing. let build_state = |validator_count: usize| -> BeaconState { let builder: TestingBeaconStateBuilder = - TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(validator_count, &spec); + TestingBeaconStateBuilder::from_deterministic_keypairs(validator_count, &spec); let (mut state, _keypairs) = builder.build(); state.build_committee_cache(relative_epoch, &spec).unwrap(); @@ -114,7 +114,7 @@ fn cache_initialization() { let spec = MinimalEthSpec::default_spec(); let builder: TestingBeaconStateBuilder = - TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(16, &spec); + TestingBeaconStateBuilder::from_deterministic_keypairs(16, &spec); let (mut state, _keypairs) = builder.build(); state.slot = @@ -176,7 +176,7 @@ fn clone_config() { let spec = MinimalEthSpec::default_spec(); let builder: TestingBeaconStateBuilder = - TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(16, &spec); + TestingBeaconStateBuilder::from_deterministic_keypairs(16, &spec); let (mut state, _keypairs) = builder.build(); state.build_all_caches(&spec).unwrap(); @@ -374,7 +374,7 @@ mod get_outstanding_deposit_len { fn state() -> BeaconState { let spec = MinimalEthSpec::default_spec(); let builder: TestingBeaconStateBuilder = - TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(16, &spec); + TestingBeaconStateBuilder::from_deterministic_keypairs(16, &spec); let (state, _keypairs) = builder.build(); state diff --git a/consensus/types/src/test_utils/builders/testing_beacon_state_builder.rs b/consensus/types/src/test_utils/builders/testing_beacon_state_builder.rs index 84b8067a2..1dda9de98 100644 --- a/consensus/types/src/test_utils/builders/testing_beacon_state_builder.rs +++ b/consensus/types/src/test_utils/builders/testing_beacon_state_builder.rs @@ -1,10 +1,10 @@ -use super::super::{generate_deterministic_keypairs, KeypairsFile}; +use super::super::generate_deterministic_keypairs; use crate::test_utils::{AttestationTestTask, TestingPendingAttestationBuilder}; use crate::*; use bls::get_withdrawal_credentials; use log::debug; use rayon::prelude::*; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; pub const KEYPAIRS_FILE: &str = "keypairs.raw_keypairs"; @@ -29,44 +29,6 @@ pub struct TestingBeaconStateBuilder { } impl TestingBeaconStateBuilder { - /// Attempts to load validators from a file in `$HOME/.lighthouse/keypairs.raw_keypairs`. If - /// the file is unavailable, it generates the keys at runtime. - /// - /// If the `$HOME` environment variable is not set, the local directory is used. - /// - /// See the `Self::from_keypairs_file` method for more info. - /// - /// # Panics - /// - /// If the file does not contain enough keypairs or is invalid. - pub fn from_default_keypairs_file_if_exists(validator_count: usize, spec: &ChainSpec) -> Self { - let dir = dirs::home_dir() - .map(|home| home.join(".lighthouse")) - .unwrap_or_else(|| PathBuf::from("")); - let file = dir.join(KEYPAIRS_FILE); - - if file.exists() { - TestingBeaconStateBuilder::from_keypairs_file(validator_count, &file, spec) - } else { - TestingBeaconStateBuilder::from_deterministic_keypairs(validator_count, spec) - } - } - - /// Loads the initial validator keypairs from a file on disk. - /// - /// Loading keypairs from file is ~10x faster than generating them. Use the `gen_keys` command - /// on the `test_harness` binary to generate the keys. In the `test_harness` dir, run `cargo - /// run -- gen_keys -h` for help. - /// - /// # Panics - /// - /// If the file does not exist, is invalid or does not contain enough keypairs. - pub fn from_keypairs_file(validator_count: usize, path: &Path, spec: &ChainSpec) -> Self { - debug!("Loading {} keypairs from file...", validator_count); - let keypairs = Vec::from_raw_file(path, validator_count).unwrap(); - TestingBeaconStateBuilder::from_keypairs(keypairs, spec) - } - /// Generates the validator keypairs deterministically. pub fn from_deterministic_keypairs(validator_count: usize, spec: &ChainSpec) -> Self { debug!("Generating {} deterministic keypairs...", validator_count); diff --git a/consensus/types/src/test_utils/keypairs_file.rs b/consensus/types/src/test_utils/keypairs_file.rs deleted file mode 100644 index 8d45b7f51..000000000 --- a/consensus/types/src/test_utils/keypairs_file.rs +++ /dev/null @@ -1,128 +0,0 @@ -use crate::*; -use rayon::prelude::*; -use std::fs::File; -use std::io::{Error, ErrorKind, Read, Write}; -use std::path::Path; - -pub const PUBLIC_KEY_BYTES_LEN: usize = 96; -pub const SECRET_KEY_BYTES_LEN: usize = 32; - -pub const BATCH_SIZE: usize = 1_000; // ~15MB - -pub const KEYPAIR_BYTES_LEN: usize = PUBLIC_KEY_BYTES_LEN + SECRET_KEY_BYTES_LEN; -pub const BATCH_BYTE_LEN: usize = KEYPAIR_BYTES_LEN * BATCH_SIZE; - -/// Defines a trait that allows reading/writing a vec of `Keypair` from/to a file. -pub trait KeypairsFile { - /// Write to file, without guaranteeing interoperability with other clients. - fn to_raw_file(&self, path: &Path, keypairs: &[Keypair]) -> Result<(), Error>; - /// Read from file, without guaranteeing interoperability with other clients. - fn from_raw_file(path: &Path, count: usize) -> Result, Error>; -} - -impl KeypairsFile for Vec { - /// Write the keypairs to file, using the fastest possible method without guaranteeing - /// interoperability with other clients. - fn to_raw_file(&self, path: &Path, keypairs: &[Keypair]) -> Result<(), Error> { - let mut keypairs_file = File::create(path)?; - - for keypair_batch in keypairs.chunks(BATCH_SIZE) { - let mut buf = Vec::with_capacity(BATCH_BYTE_LEN); - - for keypair in keypair_batch { - buf.append(&mut keypair.sk.as_raw().as_bytes()); - buf.append(&mut keypair.pk.clone().as_uncompressed_bytes()); - } - - keypairs_file.write_all(&buf)?; - } - - Ok(()) - } - - /// Read the keypairs from file, using the fastest possible method without guaranteeing - /// interoperability with other clients. - fn from_raw_file(path: &Path, count: usize) -> Result, Error> { - let mut keypairs_file = File::open(path)?; - - let mut keypairs = Vec::with_capacity(count); - - let indices: Vec = (0..count).collect(); - - for batch in indices.chunks(BATCH_SIZE) { - let mut buf = vec![0; batch.len() * KEYPAIR_BYTES_LEN]; - keypairs_file.read_exact(&mut buf)?; - - let mut keypair_batch = batch - .par_iter() - .enumerate() - .map(|(i, _)| { - let sk_start = i * KEYPAIR_BYTES_LEN; - let sk_end = sk_start + SECRET_KEY_BYTES_LEN; - let sk = SecretKey::from_bytes(&buf[sk_start..sk_end]) - .map_err(|_| Error::new(ErrorKind::Other, "Invalid SecretKey bytes")) - .unwrap(); - - let pk_start = sk_end; - let pk_end = pk_start + PUBLIC_KEY_BYTES_LEN; - let pk = PublicKey::from_uncompressed_bytes(&buf[pk_start..pk_end]) - .map_err(|_| Error::new(ErrorKind::Other, "Invalid PublicKey bytes")) - .unwrap(); - - Keypair { sk, pk } - }) - .collect(); - - keypairs.append(&mut keypair_batch); - } - - Ok(keypairs) - } -} - -#[cfg(test)] -mod tests { - use super::*; - use rand::{distributions::Alphanumeric, thread_rng, Rng}; - use std::fs::remove_file; - - fn random_keypairs(n: usize) -> Vec { - (0..n).into_par_iter().map(|_| Keypair::random()).collect() - } - - fn random_tmp_file() -> String { - let rng = thread_rng(); - - rng.sample_iter(&Alphanumeric).take(7).collect() - } - - #[test] - #[ignore] - fn read_write_consistency_small_batch() { - let num_keypairs = 10; - let keypairs = random_keypairs(num_keypairs); - - let keypairs_path = Path::new("/tmp").join(random_tmp_file()); - keypairs.to_raw_file(&keypairs_path, &keypairs).unwrap(); - - let decoded = Vec::from_raw_file(&keypairs_path, num_keypairs).unwrap(); - remove_file(keypairs_path).unwrap(); - - assert_eq!(keypairs, decoded); - } - - #[test] - #[ignore] - fn read_write_consistency_big_batch() { - let num_keypairs = BATCH_SIZE + 1; - let keypairs = random_keypairs(num_keypairs); - - let keypairs_path = Path::new("/tmp").join(random_tmp_file()); - keypairs.to_raw_file(&keypairs_path, &keypairs).unwrap(); - - let decoded = Vec::from_raw_file(&keypairs_path, num_keypairs).unwrap(); - remove_file(keypairs_path).unwrap(); - - assert_eq!(keypairs, decoded); - } -} diff --git a/consensus/types/src/test_utils/mod.rs b/consensus/types/src/test_utils/mod.rs index 719fd2e3f..63d90a83f 100644 --- a/consensus/types/src/test_utils/mod.rs +++ b/consensus/types/src/test_utils/mod.rs @@ -4,14 +4,12 @@ mod macros; mod builders; mod generate_deterministic_keypairs; -mod keypairs_file; mod test_random; pub use builders::*; pub use generate_deterministic_keypairs::generate_deterministic_keypair; pub use generate_deterministic_keypairs::generate_deterministic_keypairs; pub use generate_deterministic_keypairs::load_keypairs_from_yaml; -pub use keypairs_file::KeypairsFile; pub use rand::{RngCore, SeedableRng}; pub use rand_xorshift::XorShiftRng; pub use test_random::{test_random_instance, TestRandom}; diff --git a/crypto/bls/Cargo.toml b/crypto/bls/Cargo.toml index 41e2a0558..6c3d2a8f3 100644 --- a/crypto/bls/Cargo.toml +++ b/crypto/bls/Cargo.toml @@ -16,6 +16,7 @@ eth2_ssz = "0.1.2" eth2_ssz_types = { path = "../../consensus/ssz_types" } tree_hash = "0.1.0" arbitrary = { version = "0.4.4", features = ["derive"], optional = true } +zeroize = { version = "1.0.0", features = ["zeroize_derive"] } [features] fake_crypto = [] diff --git a/crypto/bls/src/keypair.rs b/crypto/bls/src/keypair.rs index 4a3b1e437..756197c2f 100644 --- a/crypto/bls/src/keypair.rs +++ b/crypto/bls/src/keypair.rs @@ -1,9 +1,8 @@ use super::{PublicKey, SecretKey}; -use serde_derive::{Deserialize, Serialize}; use std::fmt; use std::hash::{Hash, Hasher}; -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone)] pub struct Keypair { pub sk: SecretKey, pub pk: PublicKey, diff --git a/crypto/bls/src/lib.rs b/crypto/bls/src/lib.rs index e7ed09234..23cc6a6ea 100644 --- a/crypto/bls/src/lib.rs +++ b/crypto/bls/src/lib.rs @@ -4,6 +4,7 @@ extern crate ssz; #[macro_use] mod macros; mod keypair; +mod plain_text; mod public_key_bytes; mod secret_key; mod signature_bytes; @@ -14,6 +15,7 @@ pub use crate::public_key_bytes::PublicKeyBytes; pub use crate::secret_key::SecretKey; pub use crate::signature_bytes::SignatureBytes; pub use milagro_bls::{compress_g2, hash_to_curve_g2}; +pub use plain_text::PlainText; pub use signature_set::{verify_signature_sets, SignatureSet}; #[cfg(feature = "arbitrary")] diff --git a/crypto/eth2_keystore/src/plain_text.rs b/crypto/bls/src/plain_text.rs similarity index 89% rename from crypto/eth2_keystore/src/plain_text.rs rename to crypto/bls/src/plain_text.rs index a643cdfb1..5aae300e5 100644 --- a/crypto/eth2_keystore/src/plain_text.rs +++ b/crypto/bls/src/plain_text.rs @@ -32,3 +32,9 @@ impl From> for PlainText { Self(vec) } } + +impl AsRef<[u8]> for PlainText { + fn as_ref(&self) -> &[u8] { + &self.0 + } +} diff --git a/crypto/bls/src/secret_key.rs b/crypto/bls/src/secret_key.rs index 893ab1669..3cb0cd8b0 100644 --- a/crypto/bls/src/secret_key.rs +++ b/crypto/bls/src/secret_key.rs @@ -1,21 +1,18 @@ extern crate rand; -use super::BLS_SECRET_KEY_BYTE_SIZE; -use hex::encode as hex_encode; +use crate::PlainText; use milagro_bls::SecretKey as RawSecretKey; -use serde::de::{Deserialize, Deserializer}; -use serde::ser::{Serialize, Serializer}; -use serde_hex::PrefixedHexVisitor; -use ssz::{ssz_encode, Decode, DecodeError, Encode}; +use ssz::DecodeError; /// A single BLS signature. /// /// This struct is a wrapper upon a base type and provides helper functions (e.g., SSZ /// serialization). -#[derive(Debug, PartialEq, Clone, Eq)] +#[derive(Clone)] pub struct SecretKey(RawSecretKey); impl SecretKey { + /// Generate a new `Self` using `rand::thread_rng`. pub fn random() -> Self { SecretKey(RawSecretKey::random(&mut rand::thread_rng())) } @@ -24,9 +21,13 @@ impl SecretKey { Self(raw) } - /// Returns the underlying point as compressed bytes. - fn as_bytes(&self) -> Vec { - self.as_raw().as_bytes() + /// Returns the secret key as a byte array (wrapped in `PlainText` wrapper so it is zeroized on + /// `Drop`). + /// + /// Extreme care should be taken not to leak these bytes as they are the unencrypted secret + /// key. + pub fn as_bytes(&self) -> PlainText { + self.as_raw().as_bytes().into() } /// Instantiate a SecretKey from existing bytes. @@ -42,40 +43,14 @@ impl SecretKey { } /// Returns the underlying secret key. - pub fn as_raw(&self) -> &RawSecretKey { + pub(crate) fn as_raw(&self) -> &RawSecretKey { &self.0 } } -impl_ssz!(SecretKey, BLS_SECRET_KEY_BYTE_SIZE, "SecretKey"); - -impl_tree_hash!(SecretKey, BLS_SECRET_KEY_BYTE_SIZE); - -impl Serialize for SecretKey { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - serializer.serialize_str(&hex_encode(ssz_encode(self))) - } -} - -impl<'de> Deserialize<'de> for SecretKey { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - let bytes = deserializer.deserialize_str(PrefixedHexVisitor)?; - let secret_key = SecretKey::from_ssz_bytes(&bytes[..]) - .map_err(|e| serde::de::Error::custom(format!("invalid ssz ({:?})", e)))?; - Ok(secret_key) - } -} - #[cfg(test)] mod tests { use super::*; - use ssz::ssz_encode; #[test] pub fn test_ssz_round_trip() { @@ -85,9 +60,9 @@ mod tests { ]; let original = SecretKey::from_bytes(&byte_key).unwrap(); - let bytes = ssz_encode(&original); - let decoded = SecretKey::from_ssz_bytes(&bytes).unwrap(); + let bytes = original.as_bytes(); + let decoded = SecretKey::from_bytes(bytes.as_ref()).unwrap(); - assert_eq!(original, decoded); + assert!(original.as_bytes() == decoded.as_bytes()); } } diff --git a/crypto/eth2_keystore/src/keystore.rs b/crypto/eth2_keystore/src/keystore.rs index a5a8976b9..d531df680 100644 --- a/crypto/eth2_keystore/src/keystore.rs +++ b/crypto/eth2_keystore/src/keystore.rs @@ -6,7 +6,7 @@ use crate::json_keystore::{ Aes128Ctr, ChecksumModule, Cipher, CipherModule, Crypto, EmptyMap, EmptyString, JsonKeystore, Kdf, KdfModule, Scrypt, Sha256Checksum, Version, }; -use crate::plain_text::PlainText; +use crate::PlainText; use crate::Uuid; use bls::{Keypair, PublicKey, SecretKey}; use crypto::{digest::Digest, sha2::Sha256}; @@ -130,7 +130,7 @@ impl Keystore { uuid: Uuid, path: String, ) -> Result { - let secret = PlainText::from(keypair.sk.as_raw().as_bytes()); + let secret: PlainText = keypair.sk.as_bytes(); let (cipher_text, checksum) = encrypt(secret.as_bytes(), password, &kdf, &cipher)?; diff --git a/crypto/eth2_keystore/src/lib.rs b/crypto/eth2_keystore/src/lib.rs index bb34e221f..93dbabe5d 100644 --- a/crypto/eth2_keystore/src/lib.rs +++ b/crypto/eth2_keystore/src/lib.rs @@ -3,13 +3,12 @@ mod derived_key; mod keystore; -mod plain_text; pub mod json_keystore; +pub use bls::PlainText; pub use keystore::{ decrypt, default_kdf, encrypt, keypair_from_secret, Error, Keystore, KeystoreBuilder, DKLEN, HASH_SIZE, IV_SIZE, SALT_SIZE, }; -pub use plain_text::PlainText; pub use uuid::Uuid; diff --git a/crypto/eth2_keystore/tests/eip2335_vectors.rs b/crypto/eth2_keystore/tests/eip2335_vectors.rs index 1f3d435bb..60bb02be6 100644 --- a/crypto/eth2_keystore/tests/eip2335_vectors.rs +++ b/crypto/eth2_keystore/tests/eip2335_vectors.rs @@ -14,7 +14,7 @@ pub fn decode_and_check_sk(json: &str) -> Keystore { let keystore = Keystore::from_json_str(json).expect("should decode keystore json"); let expected_sk = hex::decode(EXPECTED_SECRET).unwrap(); let keypair = keystore.decrypt_keypair(PASSWORD.as_bytes()).unwrap(); - assert_eq!(keypair.sk.as_raw().as_bytes(), expected_sk); + assert_eq!(keypair.sk.as_bytes().as_ref(), &expected_sk[..]); keystore } diff --git a/crypto/eth2_keystore/tests/tests.rs b/crypto/eth2_keystore/tests/tests.rs index 10d46ce83..763db1267 100644 --- a/crypto/eth2_keystore/tests/tests.rs +++ b/crypto/eth2_keystore/tests/tests.rs @@ -38,8 +38,8 @@ fn string_round_trip() { ); assert_eq!( - decoded.decrypt_keypair(GOOD_PASSWORD).unwrap(), - keypair, + decoded.decrypt_keypair(GOOD_PASSWORD).unwrap().pk, + keypair.pk, "should decrypt with good password" ); } @@ -77,8 +77,8 @@ fn file() { ); assert_eq!( - decoded.decrypt_keypair(GOOD_PASSWORD).unwrap(), - keypair, + decoded.decrypt_keypair(GOOD_PASSWORD).unwrap().pk, + keypair.pk, "should decrypt with good password" ); } @@ -102,8 +102,8 @@ fn scrypt_params() { ); assert_eq!( - decoded.decrypt_keypair(GOOD_PASSWORD).unwrap(), - keypair, + decoded.decrypt_keypair(GOOD_PASSWORD).unwrap().pk, + keypair.pk, "should decrypt with good password" ); } diff --git a/crypto/eth2_wallet/Cargo.toml b/crypto/eth2_wallet/Cargo.toml index e6098744b..b43939e54 100644 --- a/crypto/eth2_wallet/Cargo.toml +++ b/crypto/eth2_wallet/Cargo.toml @@ -18,5 +18,4 @@ tiny-bip39 = "0.7.3" [dev-dependencies] hex = "0.3" -eth2_ssz = { path = "../../consensus/ssz" } tempfile = "3.1.0" diff --git a/crypto/eth2_wallet/tests/tests.rs b/crypto/eth2_wallet/tests/tests.rs index 4b24ba91b..bcc579d86 100644 --- a/crypto/eth2_wallet/tests/tests.rs +++ b/crypto/eth2_wallet/tests/tests.rs @@ -4,7 +4,6 @@ use eth2_wallet::{ bip39::{Language, Mnemonic, Seed}, recover_validator_secret, DerivedKey, Error, KeyType, KeystoreError, Wallet, WalletBuilder, }; -use ssz::Encode; use std::fs::OpenOptions; use tempfile::tempdir; @@ -243,14 +242,14 @@ fn key_derivation_from_seed() { .expect("should decrypt voting keypair"); assert_eq!( - voting_keypair.sk.as_ssz_bytes(), - manually_derived_voting_key(i), + voting_keypair.sk.as_bytes().as_ref(), + &manually_derived_voting_key(i)[..], "voting secret should match manually derived" ); assert_eq!( - voting_keypair.sk.as_ssz_bytes(), - recovered_voting_key(&wallet, i), + voting_keypair.sk.as_bytes().as_ref(), + &recovered_voting_key(&wallet, i)[..], "voting secret should match recovered" ); @@ -260,20 +259,20 @@ fn key_derivation_from_seed() { .expect("should decrypt withdrawal keypair"); assert_eq!( - withdrawal_keypair.sk.as_ssz_bytes(), - manually_derived_withdrawal_key(i), + withdrawal_keypair.sk.as_bytes().as_ref(), + &manually_derived_withdrawal_key(i)[..], "withdrawal secret should match manually derived" ); assert_eq!( - withdrawal_keypair.sk.as_ssz_bytes(), - recovered_withdrawal_key(&wallet, i), + withdrawal_keypair.sk.as_bytes().as_ref(), + &recovered_withdrawal_key(&wallet, i)[..], "withdrawal secret should match recovered" ); assert_ne!( - withdrawal_keypair.sk.as_ssz_bytes(), - voting_keypair.sk.as_ssz_bytes(), + withdrawal_keypair.sk.as_bytes().as_ref(), + voting_keypair.sk.as_bytes().as_bytes(), "voting and withdrawal keypairs should be distinct" ); diff --git a/lcli/src/main.rs b/lcli/src/main.rs index cd872c993..42aa9a0b8 100644 --- a/lcli/src/main.rs +++ b/lcli/src/main.rs @@ -553,7 +553,7 @@ fn genesis_yaml(validator_count: usize, genesis_time: u64, output: P let spec = &T::default_spec(); let builder: TestingBeaconStateBuilder = - TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(validator_count, spec); + TestingBeaconStateBuilder::from_deterministic_keypairs(validator_count, spec); let (mut state, _keypairs) = builder.build(); state.genesis_time = genesis_time; diff --git a/lighthouse/tests/account_manager.rs b/lighthouse/tests/account_manager.rs index a6bee2628..31a02786d 100644 --- a/lighthouse/tests/account_manager.rs +++ b/lighthouse/tests/account_manager.rs @@ -296,7 +296,7 @@ impl TestValidator { let withdrawal_keypair = withdrawal_result.unwrap(); assert_ne!(voting_keypair.pk, withdrawal_keypair.pk); } else { - withdrawal_result.unwrap_err(); + withdrawal_result.err().unwrap(); } // Deposit tx file should not exist yet. @@ -370,7 +370,7 @@ fn write_legacy_keypair>(name: &str, dir: P) -> Keypair { let keypair = Keypair::random(); let mut keypair_bytes = keypair.pk.as_bytes(); - keypair_bytes.extend_from_slice(&keypair.sk.as_raw().as_bytes()); + keypair_bytes.extend_from_slice(keypair.sk.as_bytes().as_ref()); fs::write(dir.as_ref().join(name), &keypair_bytes).unwrap(); @@ -409,12 +409,12 @@ fn upgrade_legacy_keypairs() { let dir = ValidatorDir::open(&validator_dir).unwrap(); assert_eq!( - voting_keypair, - dir.voting_keypair(secrets_dir.path()).unwrap() + voting_keypair.pk, + dir.voting_keypair(secrets_dir.path()).unwrap().pk ); assert_eq!( - withdrawal_keypair, - dir.withdrawal_keypair(secrets_dir.path()).unwrap() + withdrawal_keypair.pk, + dir.withdrawal_keypair(secrets_dir.path()).unwrap().pk ); } } diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index 9eef3d23f..73c5edd25 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -15,12 +15,31 @@ use types::{ }; use validator_dir::{Manager as ValidatorManager, ValidatorDir}; -#[derive(PartialEq)] struct LocalValidator { validator_dir: ValidatorDir, voting_keypair: Keypair, } +/// We derive our own `PartialEq` to avoid doing equality checks between secret keys. +/// +/// It's nice to avoid secret key comparisons from a security perspective, but it's also a little +/// risky when it comes to `HashMap` integrity (that's why we need `PartialEq`). +/// +/// Currently, we obtain keypairs from keystores where we derive the `PublicKey` from a `SecretKey` +/// via a hash function. In order to have two equal `PublicKey` with different `SecretKey` we would +/// need to have either: +/// +/// - A serious upstream integrity error. +/// - A hash collision. +/// +/// It seems reasonable to make these two assumptions in order to avoid the equality checks. +impl PartialEq for LocalValidator { + fn eq(&self, other: &Self) -> bool { + self.validator_dir == other.validator_dir + && self.voting_keypair.pk == other.voting_keypair.pk + } +} + #[derive(Clone)] pub struct ValidatorStore { validators: Arc>>,