diff --git a/.changelog/unreleased/improvements/3350-diposable-keys-cleanup.md b/.changelog/unreleased/improvements/3350-diposable-keys-cleanup.md new file mode 100644 index 0000000000..df6b443e30 --- /dev/null +++ b/.changelog/unreleased/improvements/3350-diposable-keys-cleanup.md @@ -0,0 +1,2 @@ +- Remove old disposable keys from the wallet. + ([\#3350](https://github.com/anoma/namada/pull/3350)) \ No newline at end of file diff --git a/crates/core/src/time.rs b/crates/core/src/time.rs index b4ed29e6b6..ed1815efca 100644 --- a/crates/core/src/time.rs +++ b/crates/core/src/time.rs @@ -162,6 +162,12 @@ impl DateTimeUtc { ) } + /// Returns the unix timestamp associated with this [`DateTimeUtc`]. + #[inline] + pub fn to_unix_timestamp(&self) -> i64 { + self.0.timestamp() + } + /// Returns a [`DateTimeUtc`] corresponding to the provided Unix timestamp. #[inline] pub fn from_unix_timestamp(timestamp: i64) -> Option { diff --git a/crates/sdk/src/wallet/mod.rs b/crates/sdk/src/wallet/mod.rs index 01db64feb8..da92f73f99 100644 --- a/crates/sdk/src/wallet/mod.rs +++ b/crates/sdk/src/wallet/mod.rs @@ -15,12 +15,14 @@ use alias::Alias; use bip39::{Language, Mnemonic, MnemonicType, Seed}; use borsh::{BorshDeserialize, BorshSerialize}; use namada_core::address::Address; +use namada_core::arith::checked; use namada_core::collections::{HashMap, HashSet}; use namada_core::ibc::is_ibc_denom; use namada_core::key::*; use namada_core::masp::{ ExtendedSpendingKey, ExtendedViewingKey, PaymentAddress, }; +use namada_core::time::DateTimeUtc; pub use pre_genesis::gen_key_to_store; use rand::CryptoRng; use rand_core::RngCore; @@ -33,6 +35,8 @@ pub use self::keys::{DecryptionError, StoredKeypair}; pub use self::store::{ConfirmationResponse, ValidatorData, ValidatorKeys}; use crate::wallet::store::{derive_hd_secret_key, derive_hd_spending_key}; +const DISPOSABLE_KEY_LIFETIME_IN_SECONDS: i64 = 5 * 60; // 5 minutes + /// Captures the interactive parts of the wallet's functioning pub trait WalletIo: Sized + Clone { /// Secure random number generator @@ -763,32 +767,53 @@ impl Wallet { &mut self, rng: &mut (impl CryptoRng + RngCore), ) -> common::SecretKey { - // Create the alias - let mut ctr = 1; - let mut alias = format!("disposable_{ctr}"); + #[allow(clippy::disallowed_methods)] + let current_unix_timestamp = DateTimeUtc::now().to_unix_timestamp(); - while self.store().contains_alias(&Alias::from(&alias)) { - ctr += 1; - alias = format!("disposable_{ctr}"); + let disposable_keys_to_gc = self + .store + .get_public_keys() + .keys() + .filter(|key_alias| { + check_if_disposable_key_and( + key_alias, + |_pkh, key_creation_unix_timestamp| { + let seconds_since_key_creation = checked!( + current_unix_timestamp + - key_creation_unix_timestamp + ) + .expect( + "Key should have been created before the current \ + time instant!", + ); + seconds_since_key_creation + > DISPOSABLE_KEY_LIFETIME_IN_SECONDS + }, + ) + }) + .cloned() + .collect::>(); + + for key_alias in disposable_keys_to_gc { + self.store.remove_alias(&key_alias); } - // Generate a disposable keypair to sign the wrapper if requested - // + + let sk = gen_secret_key(SchemeType::Ed25519, rng); + let key_alias = { + let pkh: PublicKeyHash = (&sk.to_public()).into(); + disposable_key_alias(&pkh, current_unix_timestamp) + }; + + println!("Created disposable keypair with alias {key_alias}"); + // TODO(namada#3239): once the wrapper transaction has been applied, // this key can be deleted from wallet (the transaction being // accepted is not enough cause we could end up doing a // rollback) - let (alias, disposable_keypair) = self - .gen_store_secret_key( - SchemeType::Ed25519, - Some(alias), - false, - None, - rng, - ) - .expect("Failed to initialize disposable keypair"); + self.insert_keypair(key_alias, false, sk.clone(), None, None, None) + .expect("Failed to store disposable signing key"); - println!("Created disposable keypair with alias {alias}"); - disposable_keypair + sk } /// Find the stored key by an alias, a public key hash or a public key. @@ -1094,3 +1119,124 @@ impl Wallet { self.store.remove_alias(&alias.into()) } } + +#[inline] +fn disposable_key_alias(pkh: &PublicKeyHash, timestamp: i64) -> String { + format!("disposable-key-{pkh}-created-at-{timestamp}") +} + +#[inline] +fn check_if_disposable_key_and bool>( + key_alias: &Alias, + callback: F, +) -> bool { + let Some((_, rest)) = key_alias.as_ref().split_once("disposable-key-") + else { + return false; + }; + let Some((key_hash_str, timestamp_str)) = rest.split_once("-created-at-") + else { + return false; + }; + let Some(key_hash): Option = + key_hash_str.to_uppercase().parse().ok() + else { + return false; + }; + let Some(timestamp): Option = timestamp_str.parse().ok() else { + return false; + }; + callback(&key_hash, timestamp) +} + +#[cfg(test)] +mod tests { + use namada_core::key::testing::{keypair_1, keypair_2, keypair_3}; + use rand_core::OsRng; + + use super::*; + + #[derive(Clone)] + struct TestWalletUtils; + + impl WalletIo for TestWalletUtils { + type Rng = OsRng; + } + + #[test] + fn test_disposable_key_alias_invalid() { + assert!(!check_if_disposable_key_and( + &Alias::from("bertha"), + |_h, _t| { panic!("Bertha's key is not disposable!") } + )); + } + + #[test] + fn test_disposable_key_alias_parsing() { + let sk = keypair_1(); + + let pkh: PublicKeyHash = (&sk.to_public()).into(); + let timestamp = 12345; + let alias = Alias::from(disposable_key_alias(&pkh, timestamp)); + + assert!(check_if_disposable_key_and(&alias, |h, t| { + assert_eq!(pkh, *h); + assert_eq!(timestamp, t); + true + })); + } + + #[test] + fn test_disposable_keys_are_garbage_collected() { + let mut wallet = Wallet { + utils: TestWalletUtils, + store: Default::default(), + decrypted_key_cache: Default::default(), + decrypted_spendkey_cache: Default::default(), + }; + + #[allow(clippy::disallowed_methods)] + let keys = [ + (keypair_1(), DateTimeUtc::now().to_unix_timestamp()), + // NB: these keys should be deleted + (keypair_2(), 0), + (keypair_3(), 0), + ]; + + for (sk, timestamp) in keys { + let pkh: PublicKeyHash = (&sk.to_public()).into(); + wallet.insert_keypair( + disposable_key_alias(&pkh, timestamp), + true, + sk, + None, + None, + None, + ); + } + + // add a new key - length should be 2 now + let new_key = wallet.gen_disposable_signing_key(&mut OsRng); + assert_eq!(wallet.store.get_public_keys().len(), 2); + + // check that indeed the first keypair was not gc'd + let keypair_1_pk = keypair_1().to_public(); + assert!( + wallet + .store + .get_public_keys() + .values() + .any(|pk| *pk == keypair_1_pk) + ); + + // check that the only other present key is the newly generated sk + let new_key_pk = new_key.to_public(); + assert!( + wallet + .store + .get_public_keys() + .values() + .any(|pk| *pk == new_key_pk) + ); + } +}