Merge pull request #3200 from matrix-org/andybalaam/feature-flag-for-overriding-expiration-min

crypto: Add a feature flag to disable the minimum session rotation time

Author: andybalaam

crates/matrix-sdk-crypto/Cargo.toml

@@ -22,6 +22,7 @@ qrcode = ["dep:matrix-sdk-qrcode"]
 message-ids = ["dep:ulid"]
 experimental-algorithms = []
 uniffi = ["dep:uniffi"]
+_disable-minimum-rotation-period-ms = []
 
 # Testing helpers for implementations based upon this
 testing = ["dep:http"]

crates/matrix-sdk-crypto/README.md

@@ -70,4 +70,6 @@ The following crate feature flags are available:
 
 * `qrcode`: Enbles QRcode generation and reading code
 
-* `testing`: provides facilities and functions for tests, in particular for integration testing store implementations. ATTENTION: do not ever use outside of tests, we do not provide any stability warantees on these, these are merely helpers. If you find you _need_ any function provided here outside of tests, please open a Github Issue and inform us about your use case for us to consider.
+* `testing`: Provides facilities and functions for tests, in particular for integration testing store implementations. ATTENTION: do not ever use outside of tests, we do not provide any stability warantees on these, these are merely helpers. If you find you _need_ any function provided here outside of tests, please open a Github Issue and inform us about your use case for us to consider.
+
+* `_disable-minimum-rotation-period-ms`: Do not use except for testing. Disables the floor on the rotation period of room keys.

crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs

@@ -59,7 +59,10 @@ use crate::{
     ReadOnlyDevice, ToDeviceRequest,
 };
 
-const ROTATION_PERIOD: Duration = Duration::from_millis(604800000);
+const ONE_HOUR: Duration = Duration::from_secs(60 * 60);
+const ONE_WEEK: Duration = Duration::from_secs(60 * 60 * 24 * 7);
+
+const ROTATION_PERIOD: Duration = ONE_WEEK;
 const ROTATION_MESSAGES: u64 = 100;
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -415,15 +418,27 @@ impl OutboundGroupSession {
     fn elapsed(&self) -> bool {
         let creation_time = Duration::from_secs(self.creation_time.get().into());
         let now = Duration::from_secs(SecondsSinceUnixEpoch::now().get().into());
-
-        // Since the encryption settings are provided by users and not
-        // checked someone could set a really low rotation period so
-        // clamp it to an hour.
         now.checked_sub(creation_time)
-            .map(|elapsed| elapsed >= max(self.settings.rotation_period, Duration::from_secs(3600)))
+            .map(|elapsed| elapsed >= self.safe_rotation_period())
             .unwrap_or(true)
     }
 
+    /// Returns the rotation_period_ms that was set for this session, clamped
+    /// to be no less than one hour.
+    ///
+    /// This is to prevent a malicious or careless user causing sessions to be
+    /// rotated very frequently.
+    ///
+    /// The feature flag `_disable-minimum-rotation-period-ms` can
+    /// be used to prevent this behaviour (which can be useful for tests).
+    fn safe_rotation_period(&self) -> Duration {
+        if cfg!(feature = "_disable-minimum-rotation-period-ms") {
+            self.settings.rotation_period
+        } else {
+            max(self.settings.rotation_period, ONE_HOUR)
+        }
+    }
+
     /// Check if the session has expired and if it should be rotated.
     ///
     /// A session will expire after some time or if enough messages have been
@@ -776,6 +791,8 @@ mod tests {
 
         use crate::{olm::OutboundGroupSession, Account, EncryptionSettings, MegolmError};
 
+        const TWO_HOURS: Duration = Duration::from_secs(60 * 60 * 2);
+
         #[async_test]
         async fn session_is_not_expired_if_no_messages_sent_and_no_time_passed() {
             // Given a session that expires after one message
@@ -816,10 +833,10 @@ mod tests {
         }
 
         #[async_test]
-        async fn session_with_short_rotation_period_is_not_expired_after_no_time() {
-            // Given a session with a 100ms expiration
+        async fn session_with_rotation_period_is_not_expired_after_no_time() {
+            // Given a session with a 2h expiration
             let session = create_session(EncryptionSettings {
-                rotation_period: Duration::from_millis(100),
+                rotation_period: TWO_HOURS,
                 ..Default::default()
             })
             .await;
@@ -832,21 +849,64 @@ mod tests {
 
         #[async_test]
         async fn session_is_expired_after_rotation_period() {
+            // Given a session with a 2h expiration
+            let mut session = create_session(EncryptionSettings {
+                rotation_period: TWO_HOURS,
+                ..Default::default()
+            })
+            .await;
+
+            // When 3 hours have passed
+            let now = SecondsSinceUnixEpoch::now();
+            session.creation_time = SecondsSinceUnixEpoch(now.get() - uint!(10800));
+
+            // Then the session is expired
+            assert!(session.expired());
+        }
+
+        #[async_test]
+        #[cfg(not(feature = "_disable-minimum-rotation-period-ms"))]
+        async fn session_does_not_expire_under_one_hour_even_if_we_ask_for_shorter() {
             // Given a session with a 100ms expiration
             let mut session = create_session(EncryptionSettings {
                 rotation_period: Duration::from_millis(100),
                 ..Default::default()
             })
             .await;
 
-            // When one hour has passed
+            // When less than an hour has passed
             let now = SecondsSinceUnixEpoch::now();
-            session.creation_time = SecondsSinceUnixEpoch(now.get() - uint!(3600));
+            session.creation_time = SecondsSinceUnixEpoch(now.get() - uint!(1800));
+
+            // Then the session is not expired: we enforce a minimum of 1 hour
+            assert!(!session.expired());
+
+            // But when more than an hour has passed
+            session.creation_time = SecondsSinceUnixEpoch(now.get() - uint!(3601));
 
             // Then the session is expired
             assert!(session.expired());
         }
 
+        #[async_test]
+        #[cfg(feature = "_disable-minimum-rotation-period-ms")]
+        async fn with_disable_minrotperiod_feature_sessions_can_expire_quickly() {
+            // Given a session with a 100ms expiration
+            let mut session = create_session(EncryptionSettings {
+                rotation_period: Duration::from_millis(100),
+                ..Default::default()
+            })
+            .await;
+
+            // When less than an hour has passed
+            let now = SecondsSinceUnixEpoch::now();
+            session.creation_time = SecondsSinceUnixEpoch(now.get() - uint!(1800));
+
+            // Then the session is expired: the feature flag has prevented us enforcing a
+            // minimum
+            assert!(session.expired());
+        }
+
         #[async_test]
         async fn session_with_zero_msgs_rotation_is_not_expired_initially() {
             // Given a session that is supposed to expire after zero messages