Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use SafePassword struct instead of String for passwords #4320

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions applications/tari_console_wallet/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ use clap::{Args, Parser, Subcommand};
use tari_app_utilities::{common_cli_args::CommonCliArgs, utilities::UniPublicKey};
use tari_comms::multiaddr::Multiaddr;
use tari_core::transactions::{tari_amount, tari_amount::MicroTari};
use tari_utilities::hex::{Hex, HexError};
use tari_utilities::{
hex::{Hex, HexError},
SafePassword,
};

const DEFAULT_NETWORK: &str = "dibbler";

Expand All @@ -45,7 +48,7 @@ pub(crate) struct Cli {
/// command line, since it's visible using `ps ax` from anywhere on the system, so always use the env var where
/// possible.
#[clap(long, env = "TARI_WALLET_PASSWORD", hide_env_values = true)]
pub password: Option<String>,
pub password: Option<SafePassword>,
/// Change the password for the console wallet
#[clap(long, alias = "update-password")]
pub change_password: bool,
Expand Down
24 changes: 12 additions & 12 deletions applications/tari_console_wallet/src/init/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ use tari_crypto::keys::PublicKey;
use tari_key_manager::{cipher_seed::CipherSeed, mnemonic::MnemonicLanguage};
use tari_p2p::{initialization::CommsInitializationError, peer_seeds::SeedPeer, TransportType};
use tari_shutdown::ShutdownSignal;
use tari_utilities::SafePassword;
use tari_wallet::{
error::{WalletError, WalletStorageError},
output_manager_service::storage::database::OutputManagerDatabase,
Expand Down Expand Up @@ -72,20 +73,19 @@ pub enum WalletBoot {
/// Gets the password provided by command line argument or environment variable if available.
/// Otherwise prompts for the password to be typed in.
pub fn get_or_prompt_password(
arg_password: Option<String>,
config_password: Option<String>,
) -> Result<Option<String>, ExitError> {
arg_password: Option<SafePassword>,
config_password: Option<SafePassword>,
) -> Result<Option<SafePassword>, ExitError> {
if arg_password.is_some() {
return Ok(arg_password);
}

let env = std::env::var_os(TARI_WALLET_PASSWORD);
if let Some(p) = env {
let env_password = Some(
p.into_string()
.map_err(|_| ExitError::new(ExitCode::IOError, "Failed to convert OsString into String"))?,
);
return Ok(env_password);
let env_password = p
.into_string()
.map_err(|_| ExitError::new(ExitCode::IOError, "Failed to convert OsString into String"))?;
return Ok(Some(env_password.into()));
}

if config_password.is_some() {
Expand All @@ -97,7 +97,7 @@ pub fn get_or_prompt_password(
Ok(Some(password))
}

fn prompt_password(prompt: &str) -> Result<String, ExitError> {
fn prompt_password(prompt: &str) -> Result<SafePassword, ExitError> {
let password = loop {
let pass = prompt_password_stdout(prompt).map_err(|e| ExitError::new(ExitCode::IOError, e))?;
if pass.is_empty() {
Expand All @@ -108,13 +108,13 @@ fn prompt_password(prompt: &str) -> Result<String, ExitError> {
}
};

Ok(password)
Ok(SafePassword::from(password))
}

/// Allows the user to change the password of the wallet.
pub async fn change_password(
config: &ApplicationConfig,
arg_password: Option<String>,
arg_password: Option<SafePassword>,
shutdown_signal: ShutdownSignal,
) -> Result<(), ExitError> {
let mut wallet = init_wallet(config, arg_password, None, None, shutdown_signal).await?;
Expand Down Expand Up @@ -221,7 +221,7 @@ pub(crate) fn wallet_mode(cli: &Cli, boot_mode: WalletBoot) -> WalletMode {
#[allow(clippy::too_many_lines)]
pub async fn init_wallet(
config: &ApplicationConfig,
arg_password: Option<String>,
arg_password: Option<SafePassword>,
seed_words_file_name: Option<PathBuf>,
recovery_seed: Option<CipherSeed>,
shutdown_signal: ShutdownSignal,
Expand Down
3 changes: 2 additions & 1 deletion base_layer/wallet/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use tari_common::{
};
use tari_comms::multiaddr::Multiaddr;
use tari_p2p::P2pConfig;
use tari_utilities::SafePassword;

use crate::{
base_node_service::config::BaseNodeServiceConfig,
Expand Down Expand Up @@ -72,7 +73,7 @@ pub struct WalletConfig {
/// The main wallet db sqlite database backend connection pool size for concurrent reads
pub db_connection_pool_size: usize,
/// The main wallet password
pub password: Option<String>, // TODO: Make clear on drop
pub password: Option<SafePassword>,
/// The auto ping interval to use for contacts liveness data
#[serde(with = "serializers::seconds")]
pub contacts_auto_ping_interval: Duration,
Expand Down
5 changes: 3 additions & 2 deletions base_layer/wallet/src/storage/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use tari_comms::{
tor::TorIdentity,
};
use tari_key_manager::cipher_seed::CipherSeed;
use tari_utilities::SafePassword;

use crate::{error::WalletStorageError, utxo_scanner_service::service::ScannedBlock};

Expand All @@ -46,7 +47,7 @@ pub trait WalletBackend: Send + Sync + Clone {
/// Modify the state the of the backend with a write operation
fn write(&self, op: WriteOperation) -> Result<Option<DbValue>, WalletStorageError>;
/// Apply encryption to the backend.
fn apply_encryption(&self, passphrase: String) -> Result<Aes256Gcm, WalletStorageError>;
fn apply_encryption(&self, passphrase: SafePassword) -> Result<Aes256Gcm, WalletStorageError>;
/// Remove encryption from the backend.
fn remove_encryption(&self) -> Result<(), WalletStorageError>;

Expand Down Expand Up @@ -276,7 +277,7 @@ where T: WalletBackend + 'static
Ok(())
}

pub async fn apply_encryption(&self, passphrase: String) -> Result<Aes256Gcm, WalletStorageError> {
pub async fn apply_encryption(&self, passphrase: SafePassword) -> Result<Aes256Gcm, WalletStorageError> {
let db_clone = self.db.clone();
tokio::task::spawn_blocking(move || db_clone.apply_encryption(passphrase))
.await
Expand Down
21 changes: 11 additions & 10 deletions base_layer/wallet/src/storage/sqlite_db/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ use tari_key_manager::cipher_seed::CipherSeed;
use tari_utilities::{
hex::{from_hex, Hex},
message_format::MessageFormat,
SafePassword,
};
use tokio::time::Instant;

Expand All @@ -72,7 +73,7 @@ pub struct WalletSqliteDatabase {
impl WalletSqliteDatabase {
pub fn new(
database_connection: WalletDbConnection,
passphrase: Option<String>,
passphrase: Option<SafePassword>,
) -> Result<Self, WalletStorageError> {
let cipher = check_db_encryption_status(&database_connection, passphrase)?;

Expand Down Expand Up @@ -383,7 +384,7 @@ impl WalletBackend for WalletSqliteDatabase {
}
}

fn apply_encryption(&self, passphrase: String) -> Result<Aes256Gcm, WalletStorageError> {
fn apply_encryption(&self, passphrase: SafePassword) -> Result<Aes256Gcm, WalletStorageError> {
let mut current_cipher = acquire_write_lock!(self.cipher);
if current_cipher.is_some() {
return Err(WalletStorageError::AlreadyEncrypted);
Expand All @@ -404,13 +405,13 @@ impl WalletBackend for WalletSqliteDatabase {
let passphrase_salt = SaltString::generate(&mut OsRng);

let passphrase_hash = argon2
.hash_password_simple(passphrase.as_bytes(), &passphrase_salt)
.hash_password_simple(passphrase.reveal(), &passphrase_salt)
.map_err(|e| WalletStorageError::AeadError(e.to_string()))?
.to_string();
let encryption_salt = SaltString::generate(&mut OsRng);

let derived_encryption_key = argon2
.hash_password_simple(passphrase.as_bytes(), encryption_salt.as_str())
.hash_password_simple(passphrase.reveal(), encryption_salt.as_str())
.map_err(|e| WalletStorageError::AeadError(e.to_string()))?
.hash
.ok_or_else(|| WalletStorageError::AeadError("Problem generating encryption key hash".to_string()))?;
Expand Down Expand Up @@ -560,7 +561,7 @@ impl WalletBackend for WalletSqliteDatabase {
/// Master Public Key that is stored in the db
fn check_db_encryption_status(
database_connection: &WalletDbConnection,
passphrase: Option<String>,
passphrase: Option<SafePassword>,
) -> Result<Option<Aes256Gcm>, WalletStorageError> {
let start = Instant::now();
let conn = database_connection.get_pooled_connection()?;
Expand All @@ -581,13 +582,13 @@ fn check_db_encryption_status(
let argon2 = Argon2::default();
let stored_hash =
PasswordHash::new(&db_passphrase_hash).map_err(|e| WalletStorageError::AeadError(e.to_string()))?;
if let Err(e) = argon2.verify_password(passphrase.as_bytes(), &stored_hash) {
if let Err(e) = argon2.verify_password(passphrase.reveal(), &stored_hash) {
error!(target: LOG_TARGET, "Incorrect passphrase ({})", e);
return Err(WalletStorageError::InvalidPassphrase);
}

let derived_encryption_key = argon2
.hash_password_simple(passphrase.as_bytes(), encryption_salt.as_str())
.hash_password_simple(passphrase.reveal(), encryption_salt.as_str())
.map_err(|e| WalletStorageError::AeadError(e.to_string()))?
.hash
.ok_or_else(|| WalletStorageError::AeadError("Problem generating encryption key hash".to_string()))?;
Expand Down Expand Up @@ -770,7 +771,7 @@ impl Encryptable<Aes256Gcm> for ClientKeyValueSql {
mod test {
use tari_key_manager::cipher_seed::CipherSeed;
use tari_test_utils::random::string;
use tari_utilities::hex::Hex;
use tari_utilities::{hex::Hex, SafePassword};
use tempfile::tempdir;

use crate::storage::{
Expand Down Expand Up @@ -826,7 +827,7 @@ mod test {
let db_folder = db_tempdir.path().to_str().unwrap().to_string();
let connection = run_migration_and_create_sqlite_connection(&format!("{}{}", db_folder, db_name), 16).unwrap();

let passphrase = "an example very very secret key.".to_string();
let passphrase = SafePassword::from("an example very very secret key.".to_string());

assert!(WalletSqliteDatabase::new(connection.clone(), Some(passphrase.clone())).is_err());

Expand Down Expand Up @@ -879,7 +880,7 @@ mod test {
};
assert_eq!(seed, read_seed1);

let passphrase = "an example very very secret key.".to_string();
let passphrase = "an example very very secret key.".to_string().into();
db.apply_encryption(passphrase).unwrap();
let read_seed2 = match db.fetch(&DbKey::MasterSeed).unwrap().unwrap() {
DbValue::MasterSeed(sk) => sk,
Expand Down
3 changes: 2 additions & 1 deletion base_layer/wallet/src/storage/sqlite_utilities/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use std::{fs::File, path::Path, time::Duration};
use fs2::FileExt;
use log::*;
use tari_common_sqlite::sqlite_connection_pool::SqliteConnectionPool;
use tari_utilities::SafePassword;
pub use wallet_db_connection::WalletDbConnection;

use crate::{
Expand Down Expand Up @@ -125,7 +126,7 @@ pub fn acquire_exclusive_file_lock(db_path: &Path) -> Result<File, WalletStorage

pub fn initialize_sqlite_database_backends<P: AsRef<Path>>(
db_path: P,
passphrase: Option<String>,
passphrase: Option<SafePassword>,
sqlite_pool_size: usize,
) -> Result<
(
Expand Down
3 changes: 2 additions & 1 deletion base_layer/wallet/src/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ use tari_p2p::{
use tari_script::{script, ExecutionStack, TariScript};
use tari_service_framework::StackBuilder;
use tari_shutdown::ShutdownSignal;
use tari_utilities::SafePassword;

use crate::{
assets::{infrastructure::initializer::AssetManagerServiceInitializer, AssetManagerHandle},
Expand Down Expand Up @@ -685,7 +686,7 @@ where

/// Apply encryption to all the Wallet db backends. The Wallet backend will test if the db's are already encrypted
/// in which case this will fail.
pub async fn apply_encryption(&mut self, passphrase: String) -> Result<(), WalletError> {
pub async fn apply_encryption(&mut self, passphrase: SafePassword) -> Result<(), WalletError> {
debug!(target: LOG_TARGET, "Applying wallet encryption.");
let cipher = self.db.apply_encryption(passphrase).await?;
self.output_manager_service.apply_encryption(cipher.clone()).await?;
Expand Down
14 changes: 7 additions & 7 deletions base_layer/wallet/tests/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ use tari_p2p::{
use tari_script::{inputs, script};
use tari_shutdown::{Shutdown, ShutdownSignal};
use tari_test_utils::{collect_recv, random};
use tari_utilities::Hashable;
use tari_utilities::{Hashable, SafePassword};
use tari_wallet::{
contacts_service::{
handle::ContactsLivenessEvent,
Expand Down Expand Up @@ -114,7 +114,7 @@ async fn create_wallet(
database_name: &str,
factories: CryptoFactories,
shutdown_signal: ShutdownSignal,
passphrase: Option<String>,
passphrase: Option<SafePassword>,
recovery_seed: Option<CipherSeed>,
) -> Result<WalletSqlite, WalletError> {
const NETWORK: Network = Network::LocalNet;
Expand Down Expand Up @@ -316,14 +316,14 @@ async fn test_wallet() {
let current_wallet_path = alice_db_tempdir.path().join("alice_db").with_extension("sqlite3");

alice_wallet
.apply_encryption("It's turtles all the way down".to_string())
.apply_encryption("It's turtles all the way down".to_string().into())
.await
.unwrap();

// Second encryption should fail
#[allow(clippy::match_wild_err_arm)]
match alice_wallet
.apply_encryption("It's turtles all the way down".to_string())
.apply_encryption("It's turtles all the way down".to_string().into())
.await
{
Ok(_) => panic!("Should not be able to encrypt twice"),
Expand All @@ -342,15 +342,15 @@ async fn test_wallet() {
panic!("Should not be able to instantiate encrypted wallet without cipher");
}

let result = WalletSqliteDatabase::new(connection.clone(), Some("wrong passphrase".to_string()));
let result = WalletSqliteDatabase::new(connection.clone(), Some("wrong passphrase".to_string().into()));

if let Err(err) = result {
assert!(matches!(err, WalletStorageError::InvalidPassphrase));
} else {
panic!("Should not be able to instantiate encrypted wallet without cipher");
}

let db = WalletSqliteDatabase::new(connection, Some("It's turtles all the way down".to_string()))
let db = WalletSqliteDatabase::new(connection, Some("It's turtles all the way down".to_string().into()))
.expect("Should be able to instantiate db with cipher");
drop(db);

Expand All @@ -360,7 +360,7 @@ async fn test_wallet() {
"alice_db",
factories.clone(),
shutdown_a.to_signal(),
Some("It's turtles all the way down".to_string()),
Some("It's turtles all the way down".to_string().into()),
None,
)
.await
Expand Down
8 changes: 4 additions & 4 deletions base_layer/wallet_ffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ use tari_p2p::{
};
use tari_script::{inputs, script};
use tari_shutdown::Shutdown;
use tari_utilities::{hex, hex::Hex};
use tari_utilities::{hex, hex::Hex, SafePassword};
use tari_wallet::{
connectivity_service::WalletConnectivityInterface,
contacts_service::storage::database::Contact,
Expand Down Expand Up @@ -4256,7 +4256,7 @@ pub unsafe extern "C" fn wallet_create(
.to_str()
.expect("A non-null passphrase should be able to be converted to string")
.to_owned();
Some(pf)
Some(SafePassword::from(pf))
};

let network = if network_str.is_null() {
Expand Down Expand Up @@ -6792,8 +6792,8 @@ pub unsafe extern "C" fn wallet_apply_encryption(

let pf = CStr::from_ptr(passphrase)
.to_str()
.expect("A non-null passphrase should be able to be converted to string")
.to_owned();
.map(|s| SafePassword::from(s.to_owned()))
.expect("A non-null passphrase should be able to be converted to string");

if let Err(e) = (*wallet).runtime.block_on((*wallet).wallet.apply_encryption(pf)) {
error = LibWalletError::from(e).code;
Expand Down