Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactoring for safer olm and megolm session usage #5380

Merged
merged 28 commits into from
Mar 10, 2022
Merged

Refactoring for safer olm and megolm session usage #5380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
10ea166
Extract olm cache store
BillCarsonFr Feb 25, 2022
33f9bc5
Protect olm session from concurrent access
BillCarsonFr Feb 28, 2022
9df5f17
protect olm account access
BillCarsonFr Feb 28, 2022
87d9308
Fix test compilation
BillCarsonFr Feb 28, 2022
24c51ea
Clean megolm import code
BillCarsonFr Feb 28, 2022
c97de48
Added e2ee sanity tests
BillCarsonFr Feb 28, 2022
9b3c5d2
Improve inbound group session cache + mutex
BillCarsonFr Feb 28, 2022
ade16a0
protect race on prekey + logs
BillCarsonFr Feb 28, 2022
9eb0473
better logs
BillCarsonFr Feb 28, 2022
11e8881
test forward better key
BillCarsonFr Feb 28, 2022
2f665dd
cleaning
BillCarsonFr Feb 28, 2022
122e785
clean test
BillCarsonFr Feb 28, 2022
2d9beb6
extract test to dedicated class
BillCarsonFr Feb 28, 2022
f238739
Clean ensure olm, fix unwedging, better logs
BillCarsonFr Mar 1, 2022
078ed1b
dispatch network calls to io
BillCarsonFr Mar 1, 2022
b7bf39b
resurrect unwedge test + cleaning
BillCarsonFr Mar 1, 2022
87de51b
Use loggerTag
BillCarsonFr Mar 1, 2022
49d33f3
avoid duplicate userId on key download
BillCarsonFr Mar 1, 2022
6546f98
use mutex on suspend and not synchronized
BillCarsonFr Mar 1, 2022
714e1d7
clean log level
BillCarsonFr Mar 1, 2022
ada83d0
fix test
BillCarsonFr Mar 2, 2022
5d952fe
code review cleaning
BillCarsonFr Mar 4, 2022
7616e2d
better comment
BillCarsonFr Mar 4, 2022
31d3fe3
Better comment
BillCarsonFr Mar 4, 2022
99a07af
Better comment
BillCarsonFr Mar 4, 2022
db84c67
Code review cleaning
BillCarsonFr Mar 4, 2022
3c931d6
Save valid backup key before downloading keys
BillCarsonFr Mar 4, 2022
96b5174
Fix ktlint
BillCarsonFr Mar 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/TestConstants.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@michaelkaye michaelkaye Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found myself doing this in response to slow integration tests in #5459 - do you have any more information on why 60s is better?

(we changed 30s -> 60s here, but the github is not showing me both sides of the diff atm)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When doing test with several sessions everything is getting a lot slower (like E2E sanity test), each session is slower to sync, etc...
Everytime we test something with await/waitWithLatch it's using that as default timeout, and test are then failing when waiting from something coming back from the sync.
It also happens when running all tests, I wonder if we don't clean properly between test?
Previously I used the ANDROIDX_TEST_ORCHESTRATOR option to ensure that all test start with a clear state, but looks like it's not working anymore.

Copy link
Contributor

@michaelkaye michaelkaye Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely don't clean up the server between tests in a single junit run, but will clean up between CI builds generally (and if you just run a demo server locally, then that won't be cleaned up unless you explicitly do so)

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
Expand Down
648 changes: 648 additions & 0 deletions ...dk-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/E2eeSanityTests.kt
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 1 addition & 3 deletions ...k-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/PreShareKeysTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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,)
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
testHelper.waitWithLatch { latch ->
testHelper.retryPeriodicallyWithLatch(latch) {
bobSession.getRoom(e2eRoomID)?.getTimelineEvent(sentEvent.eventId)?.root?.getClearType() == EventType.MESSAGE
Expand Down
13 changes: 6 additions & 7 deletions ...-sdk-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/UnwedgingTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()

Expand All @@ -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)!!
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
24 changes: 19 additions & 5 deletions ...id/src/androidTest/java/org/matrix/android/sdk/internal/crypto/gossiping/KeyShareTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
}
Expand Down Expand Up @@ -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) {
}
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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)

Expand All @@ -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)
}
Expand Down
18 changes: 14 additions & 4 deletions ...id/src/androidTest/java/org/matrix/android/sdk/internal/crypto/gossiping/WithHeldTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/crypto/CryptoService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>)

Expand Down
15 changes: 13 additions & 2 deletions ...-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}

Expand All @@ -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)
}

Expand Down Expand Up @@ -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
}
Expand Down
Loading