Merge pull request #5380 from vector-im/feature/bca/crypto_fix_rollin…

…g_uisi Refactoring for safer olm and megolm session usage
element-hq · Mar 10, 2022 · ce4ad88 · ce4ad88
2 parents d3fc379 + 96b5174
commit ce4ad88
Show file tree

Hide file tree

Showing 32 changed files with 1,517 additions and 515 deletions.
diff --git a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/TestConstants.kt b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/TestConstants.kt
@@ -23,7 +23,7 @@ object TestConstants {
     const val TESTS_HOME_SERVER_URL = "http://10.0.2.2:8080"
 
     // Time out to use when waiting for server response.
-    private const val AWAIT_TIME_OUT_MILLIS = 30_000
+    private const val AWAIT_TIME_OUT_MILLIS = 60_000
 
     // Time out to use when waiting for server response, when the debugger is connected. 10 minutes
     private const val AWAIT_TIME_OUT_WITH_DEBUGGER_MILLIS = 10 * 60_000

diff --git a/...dk-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/E2eeSanityTests.kt b/...dk-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/E2eeSanityTests.kt
diff --git a/...k-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/PreShareKeysTest.kt b/...k-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/PreShareKeysTest.kt
@@ -21,7 +21,6 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNotNull
 import org.junit.FixMethodOrder
-import org.junit.Ignore
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.junit.runners.MethodSorters
@@ -41,7 +40,6 @@ class PreShareKeysTest : InstrumentedTest {
     private val cryptoTestHelper = CryptoTestHelper(testHelper)
 
     @Test
-    @Ignore("This test will be ignored until it is fixed")
     fun ensure_outbound_session_happy_path() {
         val testData = cryptoTestHelper.doE2ETestWithAliceAndBobInARoom(true)
         val e2eRoomID = testData.roomId
@@ -92,7 +90,7 @@ class PreShareKeysTest : InstrumentedTest {
         // Just send a real message as test
         val sentEvent = testHelper.sendTextMessage(aliceSession.getRoom(e2eRoomID)!!, "Allo", 1).first()
 
-        assertEquals(megolmSessionId, sentEvent.root.content.toModel<EncryptedEventContent>()?.sessionId, "Unexpected megolm session")
+        assertEquals("Unexpected megolm session", megolmSessionId, sentEvent.root.content.toModel<EncryptedEventContent>()?.sessionId,)
         testHelper.waitWithLatch { latch ->
             testHelper.retryPeriodicallyWithLatch(latch) {
                 bobSession.getRoom(e2eRoomID)?.getTimelineEvent(sentEvent.eventId)?.root?.getClearType() == EventType.MESSAGE

diff --git a/...-sdk-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/UnwedgingTest.kt b/...-sdk-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/UnwedgingTest.kt
@@ -21,7 +21,6 @@ import org.amshove.kluent.shouldBe
 import org.junit.Assert
 import org.junit.Before
 import org.junit.FixMethodOrder
-import org.junit.Ignore
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.junit.runners.MethodSorters
@@ -85,7 +84,6 @@ class UnwedgingTest : InstrumentedTest {
      * -> This is automatically fixed after SDKs restarted the olm session
      */
     @Test
-    @Ignore("This test will be ignored until it is fixed")
     fun testUnwedging() {
         val cryptoTestData = cryptoTestHelper.doE2ETestWithAliceAndBobInARoom()
 
@@ -94,9 +92,7 @@ class UnwedgingTest : InstrumentedTest {
         val bobSession = cryptoTestData.secondSession!!
 
         val aliceCryptoStore = (aliceSession.cryptoService() as DefaultCryptoService).cryptoStoreForTesting
-
-        // bobSession.cryptoService().setWarnOnUnknownDevices(false)
-        // aliceSession.cryptoService().setWarnOnUnknownDevices(false)
+        val olmDevice = (aliceSession.cryptoService() as DefaultCryptoService).olmDeviceForTest
 
         val roomFromBobPOV = bobSession.getRoom(aliceRoomId)!!
         val roomFromAlicePOV = aliceSession.getRoom(aliceRoomId)!!
@@ -175,6 +171,7 @@ class UnwedgingTest : InstrumentedTest {
         Timber.i("## CRYPTO | testUnwedging: wedge the session now. Set crypto state like after the first message")
 
         aliceCryptoStore.storeSession(OlmSessionWrapper(deserializeFromRealm<OlmSession>(oldSession)!!), bobSession.cryptoService().getMyDevice().identityKey()!!)
+        olmDevice.clearOlmSessionCache()
         Thread.sleep(6_000)
 
         // Force new session, and key share
@@ -227,8 +224,10 @@ class UnwedgingTest : InstrumentedTest {
         testHelper.waitWithLatch {
             testHelper.retryPeriodicallyWithLatch(it) {
                 // we should get back the key and be able to decrypt
-                val result = tryOrNull {
-                    bobSession.cryptoService().decryptEvent(messagesReceivedByBob[0].root, "")
+                val result = testHelper.runBlockingTest {
+                    tryOrNull {
+                        bobSession.cryptoService().decryptEvent(messagesReceivedByBob[0].root, "")
+                    }
                 }
                 Timber.i("## CRYPTO | testUnwedging: decrypt result  ${result?.clearEvent}")
                 result != null

diff --git a/...id/src/androidTest/java/org/matrix/android/sdk/internal/crypto/gossiping/KeyShareTests.kt b/...id/src/androidTest/java/org/matrix/android/sdk/internal/crypto/gossiping/KeyShareTests.kt
@@ -97,7 +97,9 @@ class KeyShareTests : InstrumentedTest {
         assert(receivedEvent!!.isEncrypted())
 
         try {
-            aliceSession2.cryptoService().decryptEvent(receivedEvent.root, "foo")
+            commonTestHelper.runBlockingTest {
+                aliceSession2.cryptoService().decryptEvent(receivedEvent.root, "foo")
+            }
             fail("should fail")
         } catch (failure: Throwable) {
         }
@@ -152,7 +154,9 @@ class KeyShareTests : InstrumentedTest {
         }
 
         try {
-            aliceSession2.cryptoService().decryptEvent(receivedEvent.root, "foo")
+            commonTestHelper.runBlockingTest {
+                aliceSession2.cryptoService().decryptEvent(receivedEvent.root, "foo")
+            }
             fail("should fail")
         } catch (failure: Throwable) {
         }
@@ -189,7 +193,9 @@ class KeyShareTests : InstrumentedTest {
         }
 
         try {
-            aliceSession2.cryptoService().decryptEvent(receivedEvent.root, "foo")
+            commonTestHelper.runBlockingTest {
+                aliceSession2.cryptoService().decryptEvent(receivedEvent.root, "foo")
+            }
         } catch (failure: Throwable) {
             fail("should have been able to decrypt")
         }
@@ -384,7 +390,11 @@ class KeyShareTests : InstrumentedTest {
         val roomRoomBobPov = aliceSession.getRoom(roomId)
         val beforeJoin = roomRoomBobPov!!.getTimelineEvent(secondEventId)
 
-        var dRes = tryOrNull { bobSession.cryptoService().decryptEvent(beforeJoin!!.root, "") }
+        var dRes = tryOrNull {
+            commonTestHelper.runBlockingTest {
+                bobSession.cryptoService().decryptEvent(beforeJoin!!.root, "")
+            }
+        }
 
         assert(dRes == null)
 
@@ -395,7 +405,11 @@ class KeyShareTests : InstrumentedTest {
         Thread.sleep(3_000)
 
         // With the bug the first session would have improperly reshare that key :/
-        dRes = tryOrNull { bobSession.cryptoService().decryptEvent(beforeJoin.root, "") }
+        dRes = tryOrNull {
+            commonTestHelper.runBlockingTest {
+                bobSession.cryptoService().decryptEvent(beforeJoin.root, "")
+            }
+        }
         Log.d("#TEST", "KS: sgould not decrypt that ${beforeJoin.root.getClearContent().toModel<MessageContent>()?.body}")
         assert(dRes?.clearEvent == null)
     }

diff --git a/...id/src/androidTest/java/org/matrix/android/sdk/internal/crypto/gossiping/WithHeldTests.kt b/...id/src/androidTest/java/org/matrix/android/sdk/internal/crypto/gossiping/WithHeldTests.kt
@@ -93,7 +93,9 @@ class WithHeldTests : InstrumentedTest {
         // Bob should not be able to decrypt because the keys is withheld
         try {
             // .. might need to wait a bit for stability?
-            bobUnverifiedSession.cryptoService().decryptEvent(eventBobPOV.root, "")
+            testHelper.runBlockingTest {
+                bobUnverifiedSession.cryptoService().decryptEvent(eventBobPOV.root, "")
+            }
             Assert.fail("This session should not be able to decrypt")
         } catch (failure: Throwable) {
             val type = (failure as MXCryptoError.Base).errorType
@@ -118,7 +120,9 @@ class WithHeldTests : InstrumentedTest {
         // Previous message should still be undecryptable (partially withheld session)
         try {
             // .. might need to wait a bit for stability?
-            bobUnverifiedSession.cryptoService().decryptEvent(eventBobPOV.root, "")
+            testHelper.runBlockingTest {
+                bobUnverifiedSession.cryptoService().decryptEvent(eventBobPOV.root, "")
+            }
             Assert.fail("This session should not be able to decrypt")
         } catch (failure: Throwable) {
             val type = (failure as MXCryptoError.Base).errorType
@@ -165,7 +169,9 @@ class WithHeldTests : InstrumentedTest {
         val eventBobPOV = bobSession.getRoom(testData.roomId)?.getTimelineEvent(eventId)
         try {
             // .. might need to wait a bit for stability?
-            bobSession.cryptoService().decryptEvent(eventBobPOV!!.root, "")
+            testHelper.runBlockingTest {
+                bobSession.cryptoService().decryptEvent(eventBobPOV!!.root, "")
+            }
             Assert.fail("This session should not be able to decrypt")
         } catch (failure: Throwable) {
             val type = (failure as MXCryptoError.Base).errorType
@@ -233,7 +239,11 @@ class WithHeldTests : InstrumentedTest {
             testHelper.retryPeriodicallyWithLatch(latch) {
                 val timeLineEvent = bobSecondSession.getRoom(testData.roomId)?.getTimelineEvent(eventId)?.also {
                     // try to decrypt and force key request
-                    tryOrNull { bobSecondSession.cryptoService().decryptEvent(it.root, "") }
+                    tryOrNull {
+                        testHelper.runBlockingTest {
+                            bobSecondSession.cryptoService().decryptEvent(it.root, "")
+                        }
+                    }
                 }
                 sessionId = timeLineEvent?.root?.content?.toModel<EncryptedEventContent>()?.sessionId
                 timeLineEvent != null

diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/crypto/CryptoService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/crypto/CryptoService.kt
@@ -121,7 +121,7 @@ interface CryptoService {
     fun discardOutboundSession(roomId: String)
 
     @Throws(MXCryptoError::class)
-    fun decryptEvent(event: Event, timeline: String): MXEventDecryptionResult
+    suspend fun decryptEvent(event: Event, timeline: String): MXEventDecryptionResult
 
     fun decryptEventAsync(event: Event, timeline: String, callback: MatrixCallback<MXEventDecryptionResult>)
 

diff --git a/...-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt b/...-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt
@@ -434,6 +434,14 @@ internal class DefaultCryptoService @Inject constructor(
                     val currentCount = syncResponse.deviceOneTimeKeysCount.signedCurve25519 ?: 0
                     oneTimeKeysUploader.updateOneTimeKeyCount(currentCount)
                 }
+
+                // unwedge if needed
+                try {
+                    eventDecryptor.unwedgeDevicesIfNeeded()
+                } catch (failure: Throwable) {
+                    Timber.tag(loggerTag.value).w("unwedgeDevicesIfNeeded failed")
+                }
+
                 // There is a limit of to_device events returned per sync.
                 // If we are in a case of such limited to_device sync we can't try to generate/upload
                 // new otk now, because there might be some pending olm pre-key to_device messages that would fail if we rotate
@@ -723,7 +731,7 @@ internal class DefaultCryptoService @Inject constructor(
      * @return the MXEventDecryptionResult data, or throw in case of error
      */
     @Throws(MXCryptoError::class)
-    override fun decryptEvent(event: Event, timeline: String): MXEventDecryptionResult {
+    override suspend fun decryptEvent(event: Event, timeline: String): MXEventDecryptionResult {
         return internalDecryptEvent(event, timeline)
     }
 
@@ -746,7 +754,7 @@ internal class DefaultCryptoService @Inject constructor(
      * @return the MXEventDecryptionResult data, or null in case of error
      */
     @Throws(MXCryptoError::class)
-    private fun internalDecryptEvent(event: Event, timeline: String): MXEventDecryptionResult {
+    private suspend fun internalDecryptEvent(event: Event, timeline: String): MXEventDecryptionResult {
         return eventDecryptor.decryptEvent(event, timeline)
     }
 
@@ -1364,6 +1372,9 @@ internal class DefaultCryptoService @Inject constructor(
     @VisibleForTesting
     val cryptoStoreForTesting = cryptoStore
 
+    @VisibleForTesting
+    val olmDeviceForTest = olmDevice
+
     companion object {
         const val CRYPTO_MIN_FORCE_SESSION_PERIOD_MILLIS = 3_600_000 // one hour
     }