From f5b20e698cb4651b7cee2bd83d82ea331bfe78fe Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Mon, 3 Jun 2024 11:43:59 +0200 Subject: [PATCH 1/6] Change naming scheme of disposable signing keys --- crates/sdk/src/wallet/mod.rs | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/crates/sdk/src/wallet/mod.rs b/crates/sdk/src/wallet/mod.rs index 01db64feb8..a6ddfe3acc 100644 --- a/crates/sdk/src/wallet/mod.rs +++ b/crates/sdk/src/wallet/mod.rs @@ -763,32 +763,22 @@ impl Wallet { &mut self, rng: &mut (impl CryptoRng + RngCore), ) -> common::SecretKey { - // Create the alias - let mut ctr = 1; - let mut alias = format!("disposable_{ctr}"); + let sk = gen_secret_key(SchemeType::Ed25519, rng); + let key_alias = { + let pkh: PublicKeyHash = (&sk.to_public()).into(); + format!("disposable-{pkh}") + }; + + println!("Created disposable keypair with alias {key_alias}"); - while self.store().contains_alias(&Alias::from(&alias)) { - ctr += 1; - alias = format!("disposable_{ctr}"); - } - // Generate a disposable keypair to sign the wrapper if requested - // // 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. From 44965da0b9ada3c83814fa779f4dba9ff1eb1a09 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Mon, 3 Jun 2024 12:22:06 +0200 Subject: [PATCH 2/6] Convert a datetime to a unix timestamp --- crates/core/src/time.rs | 6 ++++++ 1 file changed, 6 insertions(+) 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 { From c2018a8f3f0d8bd3ceb1bab564a91d0fff451845 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Mon, 3 Jun 2024 12:41:37 +0200 Subject: [PATCH 3/6] Automatically clear disposable keys from the wallet --- crates/sdk/src/wallet/mod.rs | 140 ++++++++++++++++++++++++++++++++++- 1 file changed, 139 insertions(+), 1 deletion(-) diff --git a/crates/sdk/src/wallet/mod.rs b/crates/sdk/src/wallet/mod.rs index a6ddfe3acc..50d552f5d7 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,10 +767,41 @@ impl Wallet { &mut self, rng: &mut (impl CryptoRng + RngCore), ) -> common::SecretKey { + #[allow(clippy::disallowed_methods)] + let current_unix_timestamp = DateTimeUtc::now().to_unix_timestamp(); + + 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); + } + let sk = gen_secret_key(SchemeType::Ed25519, rng); let key_alias = { let pkh: PublicKeyHash = (&sk.to_public()).into(); - format!("disposable-{pkh}") + disposable_key_alias(&pkh, current_unix_timestamp) }; println!("Created disposable keypair with alias {key_alias}"); @@ -1084,3 +1119,106 @@ 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_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 + _ = 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) + ); + } +} From 4fe196be5672fbc8ca296468f1e4d84f66474d63 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Mon, 3 Jun 2024 12:44:42 +0200 Subject: [PATCH 4/6] Test that non-disposable keys are not garbage collected --- crates/sdk/src/wallet/mod.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/crates/sdk/src/wallet/mod.rs b/crates/sdk/src/wallet/mod.rs index 50d552f5d7..1c33a679ca 100644 --- a/crates/sdk/src/wallet/mod.rs +++ b/crates/sdk/src/wallet/mod.rs @@ -1163,6 +1163,14 @@ mod tests { 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(); From cf7a9eeb9bb8cdfd52877f3c5d256273bd812175 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Mon, 3 Jun 2024 12:47:14 +0200 Subject: [PATCH 5/6] Make `test_disposable_keys_are_garbage_collected` more robust --- crates/sdk/src/wallet/mod.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/crates/sdk/src/wallet/mod.rs b/crates/sdk/src/wallet/mod.rs index 1c33a679ca..da92f73f99 100644 --- a/crates/sdk/src/wallet/mod.rs +++ b/crates/sdk/src/wallet/mod.rs @@ -1216,7 +1216,7 @@ mod tests { } // add a new key - length should be 2 now - _ = wallet.gen_disposable_signing_key(&mut OsRng); + 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 @@ -1228,5 +1228,15 @@ mod tests { .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) + ); } } From 2e526737ae889b2078c9e27dd181e44abf120c71 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Mon, 3 Jun 2024 12:49:53 +0200 Subject: [PATCH 6/6] Changelog for #3350 --- .../unreleased/improvements/3350-diposable-keys-cleanup.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/3350-diposable-keys-cleanup.md 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