Merge branch 'tiago/diposable-keys-cleanup' (#3350)

* tiago/diposable-keys-cleanup:
  Changelog for #3350
  Make `test_disposable_keys_are_garbage_collected` more robust
  Test that non-disposable keys are not garbage collected
  Automatically clear disposable keys from the wallet
  Convert a datetime to a unix timestamp
  Change naming scheme of disposable signing keys

.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))

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<Self> {

crates/sdk/src/wallet/mod.rs

@@ -15,11 +15,13 @@ use alias::Alias;
 use bip39::{Language, Mnemonic, MnemonicType, Seed};
 use borsh::{BorshDeserialize, BorshSerialize};
 use namada_core::address::{Address, ImplicitAddress};
+use namada_core::arith::checked;
 use namada_core::collections::{HashMap, HashSet};
 use namada_core::key::*;
 use namada_core::masp::{
  ExtendedSpendingKey, ExtendedViewingKey, PaymentAddress,
 };
+use namada_core::time::DateTimeUtc;
 use namada_ibc::is_ibc_denom;
 pub use pre_genesis::gen_key_to_store;
 use rand::CryptoRng;
@@ -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<U: WalletIo> Wallet<U> {
  &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::<Vec<_>>();
+
+ 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.
@@ -1113,3 +1138,124 @@ impl<U: WalletIo> Wallet<U> {
  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<F: FnOnce(&PublicKeyHash, i64) -> 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<PublicKeyHash> =
+ key_hash_str.to_uppercase().parse().ok()
+ else {
+ return false;
+ };
+ let Some(timestamp): Option<i64> = 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)
+ );
+ }
+}