crypto: Rotate fallback keys in a time-based manner

bindings/matrix-sdk-crypto-ffi/src/lib.rs

@@ -245,7 +245,8 @@ async fn migrate_data(
         pickle,
         shared: data.account.shared,
         uploaded_signed_key_count: data.account.uploaded_signed_key_count as u64,
-        creation_local_time: MilliSecondsSinceUnixEpoch(UInt::default()),
+        creation_local_time: MilliSecondsSinceUnixEpoch::now(),
+        fallback_key_creation_timestamp: Some(MilliSecondsSinceUnixEpoch::now()),
     };
     let account = matrix_sdk_crypto::olm::Account::from_pickle(pickled_account)?;
 

crates/matrix-sdk-crypto/CHANGELOG.md

@@ -1,5 +1,11 @@
 # UNRELEASED
 
+Changed:
+
+- Fallback keys are rotated in a time-based manner, instead of waiting for the
+  server to tell us that a fallback key got used.
+  ([#3151](https://github.com/matrix-org/matrix-rust-sdk/pull/3151))
+
 Breaking changes:
 
 - Rename the `OlmMachine::invalidate_group_session` method to

crates/matrix-sdk-crypto/src/dehydrated_devices.rs

@@ -318,7 +318,7 @@ impl DehydratedDevice {
         let mut transaction = self.store.transaction().await;
 
         let account = transaction.account().await?;
-        account.generate_fallback_key_helper();
+        account.generate_fallback_key_if_needed();
 
         let (device_keys, one_time_keys, fallback_keys) = account.keys_for_upload();
 

crates/matrix-sdk-crypto/src/machine.rs

@@ -2287,7 +2287,7 @@ pub(crate) mod tests {
             .store()
             .with_transaction(|mut tr| async {
                 let account = tr.account().await.unwrap();
-                account.generate_fallback_key_helper();
+                account.generate_fallback_key_if_needed();
                 account.update_uploaded_key_count(0);
                 account.generate_one_time_keys_if_needed();
                 let request = machine

crates/matrix-sdk-crypto/src/olm/account.rs

@@ -17,6 +17,7 @@ use std::{
     fmt,
     ops::{Deref, Not as _},
     sync::Arc,
+    time::Duration,
 };
 
 use ruma::{
@@ -327,6 +328,14 @@ pub struct Account {
     /// needs to set this for us, depending on the count we will suggest the
     /// client to upload new keys.
     uploaded_signed_key_count: u64,
+    /// The timestamp of the last time we generated a fallback key. Fallback
+    /// keys are rotated in a time-based manner. This field records when we
+    /// either generated our first fallback key or rotated one.
+    ///
+    /// Will be `None` if we never created a fallback key, or if we're migrating
+    /// from a `AccountPickle` that didn't use time-based fallback key
+    /// rotation.
+    fallback_creation_timestamp: Option<MilliSecondsSinceUnixEpoch>,
 }
 
 impl Deref for Account {
@@ -358,6 +367,9 @@ pub struct PickledAccount {
     /// as creation time of own device
     #[serde(default = "default_account_creation_time")]
     pub creation_local_time: MilliSecondsSinceUnixEpoch,
+    /// The timestamp of the last time we generated a fallback key.
+    #[serde(default)]
+    pub fallback_key_creation_timestamp: Option<MilliSecondsSinceUnixEpoch>,
 }
 
 fn default_account_creation_time() -> MilliSecondsSinceUnixEpoch {
@@ -404,6 +416,7 @@ impl Account {
             inner: Box::new(account),
             shared: false,
             uploaded_signed_key_count: 0,
+            fallback_creation_timestamp: None,
         }
     }
 
@@ -496,11 +509,11 @@ impl Account {
             self.generate_one_time_keys_if_needed();
         }
 
-        if let Some(unused) = unused_fallback_keys {
-            if !unused.contains(&DeviceKeyAlgorithm::SignedCurve25519) {
-                // Generate a new fallback key if we don't have one.
-                self.generate_fallback_key_helper();
-            }
+        // If the server supports fallback keys or if it did so in the past, shown by
+        // the existence of a fallback creation timestamp, generate a new one if
+        // we don't have one, or if the current fallback key expired.
+        if unused_fallback_keys.is_some() || self.fallback_creation_timestamp.is_some() {
+            self.generate_fallback_key_if_needed();
         }
     }
 
@@ -543,17 +556,61 @@ impl Account {
         Some(key_count as u64)
     }
 
-    pub(crate) fn generate_fallback_key_helper(&mut self) {
-        if self.inner.fallback_key().is_empty() {
+    /// Generate a new fallback key iff a unpublished one isn't already inside
+    /// of vodozemac and if the currently active one expired.
+    ///
+    /// The former is checked using [`Account::fallback_key().is_empty()`],
+    /// which is a hashmap that gets cleared by the
+    /// [`Account::mark_keys_as_published()`] call.
+    pub(crate) fn generate_fallback_key_if_needed(&mut self) {
+        if self.inner.fallback_key().is_empty() && self.fallback_key_expired() {
             let removed_fallback_key = self.inner.generate_fallback_key();
+            self.fallback_creation_timestamp = Some(MilliSecondsSinceUnixEpoch::now());
 
             debug!(
                 ?removed_fallback_key,
-                "No unused fallback keys were found on the server, generated a new fallback key.",
+                "The fallback key either expired or we didn't have one: generated a new fallback key.",
             );
         }
     }
 
+    /// Check if our most recent fallback key has expired.
+    ///
+    /// We consider the fallback key to be expired if it's older than a week.
+    /// This is the lower bound for the recommended signed pre-key bundle
+    /// rotation interval in the X3DH spec[1].
+    ///
+    /// [1]: https://signal.org/docs/specifications/x3dh/#publishing-keys
+    fn fallback_key_expired(&self) -> bool {
+        const FALLBACK_KEY_MAX_AGE: Duration = Duration::from_secs(3600 * 24 * 7);
+
+        if let Some(time) = self.fallback_creation_timestamp {
+            // `to_system_time()` returns `None` if the the UNIX_EPOCH + `time` doesn't fit
+            // into a i64. This will likely never happen, but let's rotate the
+            // key in case the values are messed up for some other reason.
+            let Some(system_time) = time.to_system_time() else {
+                return true;
+            };
+
+            // `elapsed()` errors if the `system_time` is in the future, this should mean
+            // that our clock has changed to the past, let's rotate just in case
+            // and then we'll get to a normal time.
+            let Ok(elapsed) = system_time.elapsed() else {
+                return true;
+            };
+
+            // Alright, our times are normal and we know how much time elapsed since the
+            // last time we created/rotated a fallback key.
+            //
+            // If the key is older than a week, then we rotate it.
+            elapsed > FALLBACK_KEY_MAX_AGE
+        } else {
+            // We never created a fallback key, or we're migrating to the time-based
+            // fallback key rotation, so let's generate a new fallback key.
+            true
+        }
+    }
+
     fn fallback_key(&self) -> HashMap<KeyId, Curve25519PublicKey> {
         self.inner.fallback_key()
     }
@@ -595,6 +652,7 @@ impl Account {
             shared: self.shared(),
             uploaded_signed_key_count: self.uploaded_key_count(),
             creation_local_time: self.static_data.creation_local_time,
+            fallback_key_creation_timestamp: self.fallback_creation_timestamp,
         }
     }
 
@@ -651,6 +709,7 @@ impl Account {
             inner: Box::new(account),
             shared: pickle.shared,
             uploaded_signed_key_count: pickle.uploaded_signed_key_count,
+            fallback_creation_timestamp: pickle.fallback_key_creation_timestamp,
         })
     }
 
@@ -1371,6 +1430,7 @@ mod tests {
     use std::{
         collections::{BTreeMap, BTreeSet},
         ops::Deref,
+        time::Duration,
     };
 
     use anyhow::Result;
@@ -1442,30 +1502,59 @@ mod tests {
         // We don't create fallback keys since we don't know if the server
         // supports them, we need to receive a sync response to decide if we're
         // going to create them or not.
-        assert!(fallback_keys.is_empty());
+        assert!(
+            fallback_keys.is_empty(),
+            "We should not upload fallback keys until we know if the server supports them."
+        );
 
         let one_time_keys = BTreeMap::from([(DeviceKeyAlgorithm::SignedCurve25519, 50u8.into())]);
 
         // A `None` here means that the server doesn't support fallback keys, no
         // fallback key gets uploaded.
         account.update_key_counts(&one_time_keys, None);
         let (_, _, fallback_keys) = account.keys_for_upload();
-        assert!(fallback_keys.is_empty());
+        assert!(
+            fallback_keys.is_empty(),
+            "We should not upload a fallback key if we're certain that the server doesn't support \
+             them."
+        );
 
         // The empty array means that the server supports fallback keys but
         // there isn't a unused fallback key on the server. This time we upload
         // a fallback key.
         let unused_fallback_keys = &[];
         account.update_key_counts(&one_time_keys, Some(unused_fallback_keys.as_ref()));
         let (_, _, fallback_keys) = account.keys_for_upload();
-        assert!(!fallback_keys.is_empty());
+        assert!(
+            !fallback_keys.is_empty(),
+            "We should upload the initial fallback key if the server supports them."
+        );
         account.mark_keys_as_published();
 
-        // There's an unused fallback key on the server, nothing to do here.
-        let unused_fallback_keys = &[DeviceKeyAlgorithm::SignedCurve25519];
+        // There's no unused fallback key on the server, but our initial fallback key
+        // did not yet expire.
+        let unused_fallback_keys = &[];
         account.update_key_counts(&one_time_keys, Some(unused_fallback_keys.as_ref()));
         let (_, _, fallback_keys) = account.keys_for_upload();
-        assert!(fallback_keys.is_empty());
+        assert!(
+            fallback_keys.is_empty(),
+            "We should not upload new fallback keys unless our current fallback key expires."
+        );
+
+        let fallback_key_timestamp =
+            account.fallback_creation_timestamp.unwrap().to_system_time().unwrap()
+                - Duration::from_secs(3600 * 24 * 30);
+
+        account.fallback_creation_timestamp =
+            Some(MilliSecondsSinceUnixEpoch::from_system_time(fallback_key_timestamp).unwrap());
+
+        account.update_key_counts(&one_time_keys, None);
+        let (_, _, fallback_keys) = account.keys_for_upload();
+        assert!(
+            !fallback_keys.is_empty(),
+            "Now that our fallback key has expired, we should try to upload a new one, even if the \
+             server supposedly doesn't support fallback keys anymore"
+        );
 
         Ok(())
     }