Skip to content

Commit

Permalink
Remove ZeroizeString in favour of Zeroizing<String> (#6661)
Browse files Browse the repository at this point in the history 
* Remove ZeroizeString in favour of Zeroizing<String>

* cargo fmt

* remove unrelated line that slipped in

* Update beacon_node/store/Cargo.toml

thanks michael!

Co-authored-by: Michael Sproul <[email protected]>

* Merge branch 'unstable' into remove-zeroizedstring
  • Loading branch information
@dknopik
dknopik authored Dec 11, 2024
1 parent c5a48a9 commit a2b0009
Show file tree
Hide file tree
Showing 27 changed files with 99 additions and 217 deletions.
11 changes: 7 additions & 4 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ tree_hash_derive = "0.8"
url = "2"
uuid = { version = "0.8", features = ["serde", "v4"] }
warp = { version = "0.3.7", default-features = false, features = ["tls"] }
zeroize = { version = "1", features = ["zeroize_derive"] }
zeroize = { version = "1", features = ["zeroize_derive", "serde"] }
zip = "0.6"

# Local crates.
Expand Down
1 change: 1 addition & 0 deletions account_manager/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ safe_arith = { workspace = true }
slot_clock = { workspace = true }
filesystem = { workspace = true }
sensitive_url = { workspace = true }
zeroize = { workspace = true }

[dev-dependencies]
tempfile = { workspace = true }
2 changes: 1 addition & 1 deletion account_manager/src/validator/create.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ pub fn read_wallet_password_from_cli(
eprintln!();
eprintln!("{}", WALLET_PASSWORD_PROMPT);
let password =
PlainText::from(read_password_from_user(stdin_inputs)?.as_ref().to_vec());
PlainText::from(read_password_from_user(stdin_inputs)?.as_bytes().to_vec());
Ok(password)
}
}
Expand Down
16 changes: 10 additions & 6 deletions account_manager/src/validator/import.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use account_utils::{
recursively_find_voting_keystores, PasswordStorage, ValidatorDefinition,
ValidatorDefinitions, CONFIG_FILENAME,
},
ZeroizeString, STDIN_INPUTS_FLAG,
STDIN_INPUTS_FLAG,
};
use clap::{Arg, ArgAction, ArgMatches, Command};
use clap_utils::FLAG_HEADER;
Expand All @@ -16,6 +16,7 @@ use std::fs;
use std::path::PathBuf;
use std::thread::sleep;
use std::time::Duration;
use zeroize::Zeroizing;

pub const CMD: &str = "import";
pub const KEYSTORE_FLAG: &str = "keystore";
Expand Down Expand Up @@ -148,7 +149,7 @@ pub fn cli_run(matches: &ArgMatches, validator_dir: PathBuf) -> Result<(), Strin
// Skip keystores that already exist, but exit early if any operation fails.
// Reuses the same password for all keystores if the `REUSE_PASSWORD_FLAG` flag is set.
let mut num_imported_keystores = 0;
let mut previous_password: Option<ZeroizeString> = None;
let mut previous_password: Option<Zeroizing<String>> = None;

for src_keystore in &keystore_paths {
let keystore = Keystore::from_json_file(src_keystore)
Expand Down Expand Up @@ -182,14 +183,17 @@ pub fn cli_run(matches: &ArgMatches, validator_dir: PathBuf) -> Result<(), Strin

let password = match keystore_password_path.as_ref() {
Some(path) => {
let password_from_file: ZeroizeString = fs::read_to_string(path)
let password_from_file: Zeroizing<String> = fs::read_to_string(path)
.map_err(|e| format!("Unable to read {:?}: {:?}", path, e))?
.into();
password_from_file.without_newlines()
password_from_file
.trim_end_matches(['\r', '\n'])
.to_string()
.into()
}
None => {
let password_from_user = read_password_from_user(stdin_inputs)?;
if password_from_user.as_ref().is_empty() {
if password_from_user.is_empty() {
eprintln!("Continuing without password.");
sleep(Duration::from_secs(1)); // Provides nicer UX.
break None;
Expand Down Expand Up @@ -314,7 +318,7 @@ pub fn cli_run(matches: &ArgMatches, validator_dir: PathBuf) -> Result<(), Strin
/// Otherwise, returns the keystore error.
fn check_password_on_keystore(
keystore: &Keystore,
password: &ZeroizeString,
password: &Zeroizing<String>,
) -> Result<bool, String> {
match keystore.decrypt_keypair(password.as_ref()) {
Ok(_) => {
Expand Down
4 changes: 2 additions & 2 deletions account_manager/src/wallet/create.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,14 +226,14 @@ pub fn read_new_wallet_password_from_cli(
eprintln!();
eprintln!("{}", NEW_WALLET_PASSWORD_PROMPT);
let password =
PlainText::from(read_password_from_user(stdin_inputs)?.as_ref().to_vec());
PlainText::from(read_password_from_user(stdin_inputs)?.as_bytes().to_vec());

// Ensure the password meets the minimum requirements.
match is_password_sufficiently_complex(password.as_bytes()) {
Ok(_) => {
eprintln!("{}", RETYPE_PASSWORD_PROMPT);
let retyped_password =
PlainText::from(read_password_from_user(stdin_inputs)?.as_ref().to_vec());
PlainText::from(read_password_from_user(stdin_inputs)?.as_bytes().to_vec());
if retyped_password == password {
break Ok(password);
} else {
Expand Down
108 changes: 8 additions & 100 deletions common/account_utils/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,14 @@ use eth2_wallet::{
};
use filesystem::{create_with_600_perms, Error as FsError};
use rand::{distributions::Alphanumeric, Rng};
use serde::{Deserialize, Serialize};
use std::fs::{self, File};
use std::io;
use std::io::prelude::*;
use std::path::{Path, PathBuf};
use std::str::from_utf8;
use std::thread::sleep;
use std::time::Duration;
use std::{
fs::{self, File},
str::FromStr,
};
use zeroize::Zeroize;
use zeroize::Zeroizing;

pub mod validator_definitions;

Expand Down Expand Up @@ -69,8 +65,8 @@ pub fn read_password<P: AsRef<Path>>(path: P) -> Result<PlainText, io::Error> {
fs::read(path).map(strip_off_newlines).map(Into::into)
}

/// Reads a password file into a `ZeroizeString` struct, with new-lines removed.
pub fn read_password_string<P: AsRef<Path>>(path: P) -> Result<ZeroizeString, String> {
/// Reads a password file into a `Zeroizing<String>` struct, with new-lines removed.
pub fn read_password_string<P: AsRef<Path>>(path: P) -> Result<Zeroizing<String>, String> {
fs::read(path)
.map_err(|e| format!("Error opening file: {:?}", e))
.map(strip_off_newlines)
Expand Down Expand Up @@ -112,8 +108,8 @@ pub fn random_password() -> PlainText {
random_password_raw_string().into_bytes().into()
}

/// Generates a random alphanumeric password of length `DEFAULT_PASSWORD_LEN` as `ZeroizeString`.
pub fn random_password_string() -> ZeroizeString {
/// Generates a random alphanumeric password of length `DEFAULT_PASSWORD_LEN` as `Zeroizing<String>`.
pub fn random_password_string() -> Zeroizing<String> {
random_password_raw_string().into()
}

Expand Down Expand Up @@ -141,7 +137,7 @@ pub fn strip_off_newlines(mut bytes: Vec<u8>) -> Vec<u8> {
}

/// Reads a password from TTY or stdin if `use_stdin == true`.
pub fn read_password_from_user(use_stdin: bool) -> Result<ZeroizeString, String> {
pub fn read_password_from_user(use_stdin: bool) -> Result<Zeroizing<String>, String> {
let result = if use_stdin {
rpassword::prompt_password_stderr("")
.map_err(|e| format!("Error reading from stdin: {}", e))
Expand All @@ -150,7 +146,7 @@ pub fn read_password_from_user(use_stdin: bool) -> Result<ZeroizeString, String>
.map_err(|e| format!("Error reading from tty: {}", e))
};

result.map(ZeroizeString::from)
result.map(Zeroizing::from)
}

/// Reads a mnemonic phrase from TTY or stdin if `use_stdin == true`.
Expand Down Expand Up @@ -210,46 +206,6 @@ pub fn mnemonic_from_phrase(phrase: &str) -> Result<Mnemonic, String> {
Mnemonic::from_phrase(phrase, Language::English).map_err(|e| e.to_string())
}

/// Provides a new-type wrapper around `String` that is zeroized on `Drop`.
///
/// Useful for ensuring that password memory is zeroed-out on drop.
#[derive(Clone, PartialEq, Serialize, Deserialize, Zeroize)]
#[zeroize(drop)]
#[serde(transparent)]
pub struct ZeroizeString(String);

impl FromStr for ZeroizeString {
type Err = String;

fn from_str(s: &str) -> Result<Self, Self::Err> {
Ok(Self(s.to_owned()))
}
}

impl From<String> for ZeroizeString {
fn from(s: String) -> Self {
Self(s)
}
}

impl ZeroizeString {
pub fn as_str(&self) -> &str {
&self.0
}

/// Remove any number of newline or carriage returns from the end of a vector of bytes.
pub fn without_newlines(&self) -> ZeroizeString {
let stripped_string = self.0.trim_end_matches(['\r', '\n']).into();
Self(stripped_string)
}
}

impl AsRef<[u8]> for ZeroizeString {
fn as_ref(&self) -> &[u8] {
self.0.as_bytes()
}
}

pub fn read_mnemonic_from_cli(
mnemonic_path: Option<PathBuf>,
stdin_inputs: bool,
Expand Down Expand Up @@ -294,54 +250,6 @@ pub fn read_mnemonic_from_cli(
mod test {
use super::*;

#[test]
fn test_zeroize_strip_off() {
let expected = "hello world";

assert_eq!(
ZeroizeString::from("hello world\n".to_string())
.without_newlines()
.as_str(),
expected
);
assert_eq!(
ZeroizeString::from("hello world\n\n\n\n".to_string())
.without_newlines()
.as_str(),
expected
);
assert_eq!(
ZeroizeString::from("hello world\r".to_string())
.without_newlines()
.as_str(),
expected
);
assert_eq!(
ZeroizeString::from("hello world\r\r\r\r\r".to_string())
.without_newlines()
.as_str(),
expected
);
assert_eq!(
ZeroizeString::from("hello world\r\n".to_string())
.without_newlines()
.as_str(),
expected
);
assert_eq!(
ZeroizeString::from("hello world\r\n\r\n".to_string())
.without_newlines()
.as_str(),
expected
);
assert_eq!(
ZeroizeString::from("hello world".to_string())
.without_newlines()
.as_str(),
expected
);
}

#[test]
fn test_strip_off() {
let expected = b"hello world".to_vec();
Expand Down
11 changes: 5 additions & 6 deletions common/account_utils/src/validator_definitions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@
//! Serves as the source-of-truth of which validators this validator client should attempt (or not
//! attempt) to load into the `crate::intialized_validators::InitializedValidators` struct.
use crate::{
default_keystore_password_path, read_password_string, write_file_via_temporary, ZeroizeString,
};
use crate::{default_keystore_password_path, read_password_string, write_file_via_temporary};
use directory::ensure_dir_exists;
use eth2_keystore::Keystore;
use regex::Regex;
Expand All @@ -17,6 +15,7 @@ use std::io;
use std::path::{Path, PathBuf};
use types::{graffiti::GraffitiString, Address, PublicKey};
use validator_dir::VOTING_KEYSTORE_FILE;
use zeroize::Zeroizing;

/// The file name for the serialized `ValidatorDefinitions` struct.
pub const CONFIG_FILENAME: &str = "validator_definitions.yml";
Expand Down Expand Up @@ -52,7 +51,7 @@ pub enum Error {
/// Defines how a password for a validator keystore will be persisted.
pub enum PasswordStorage {
/// Store the password in the `validator_definitions.yml` file.
ValidatorDefinitions(ZeroizeString),
ValidatorDefinitions(Zeroizing<String>),
/// Store the password in a separate, dedicated file (likely in the "secrets" directory).
File(PathBuf),
/// Don't store the password at all.
Expand Down Expand Up @@ -93,7 +92,7 @@ pub enum SigningDefinition {
#[serde(skip_serializing_if = "Option::is_none")]
voting_keystore_password_path: Option<PathBuf>,
#[serde(skip_serializing_if = "Option::is_none")]
voting_keystore_password: Option<ZeroizeString>,
voting_keystore_password: Option<Zeroizing<String>>,
},
/// A validator that defers to a Web3Signer HTTP server for signing.
///
Expand All @@ -107,7 +106,7 @@ impl SigningDefinition {
matches!(self, SigningDefinition::LocalKeystore { .. })
}

pub fn voting_keystore_password(&self) -> Result<Option<ZeroizeString>, Error> {
pub fn voting_keystore_password(&self) -> Result<Option<Zeroizing<String>>, Error> {
match self {
SigningDefinition::LocalKeystore {
voting_keystore_password: Some(password),
Expand Down
Loading

0 comments on commit a2b0009

Please sign in to comment.