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.

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

Fix QR code login support in rust #8654

Merged
merged 8 commits into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions changelog.d/8653.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix Login with QR code not working with rust crypto.
2 changes: 1 addition & 1 deletion matrix-sdk-android/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ dependencies {

implementation libs.google.phonenumber

rustCryptoImplementation("org.matrix.rustcomponents:crypto-android:0.3.14")
rustCryptoImplementation("org.matrix.rustcomponents:crypto-android:0.3.15")
// rustCryptoApi project(":library:rustCrypto")

testImplementation libs.tests.junit
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.JUnit4
import org.junit.runners.MethodSorters
import org.matrix.android.sdk.BuildConfig
import org.matrix.android.sdk.InstrumentedTest
import org.matrix.android.sdk.api.query.QueryStringValue
import org.matrix.android.sdk.api.session.Session
Expand Down Expand Up @@ -196,6 +197,7 @@ class E2eeShareKeysHistoryTest : InstrumentedTest {

@Test
fun testNeedsRotationFromSharedToWorldReadable() {
Assume.assumeTrue("Test is flacky on legacy crypto", BuildConfig.FLAVOR == "rustCrypto")
testRotationDueToVisibilityChange(RoomHistoryVisibility.SHARED, RoomHistoryVisibilityContent("world_readable"))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,13 @@ internal class SecretShareManager @Inject constructor(
.w("handleSecretRequest() : malformed request norequestingDeviceId ")
}

if (deviceId == credentials.deviceId) {
return Unit.also {
Timber.tag(loggerTag.value)
.v("handleSecretRequest() : Ignore request from self device")
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
}
}

val device = cryptoStore.getUserDevice(credentials.userId, deviceId)
?: return Unit.also {
Timber.tag(loggerTag.value)
Expand Down Expand Up @@ -254,6 +261,37 @@ internal class SecretShareManager @Inject constructor(
}
}

suspend fun requestMissingSecrets() {
// quick implementation for backward compatibility with rust, will request all secrets to all own devices
val secretNames = listOf(MASTER_KEY_SSSS_NAME, SELF_SIGNING_KEY_SSSS_NAME, USER_SIGNING_KEY_SSSS_NAME, KEYBACKUP_SECRET_SSSS_NAME)

secretNames.forEach { secretName ->
val toDeviceContent = SecretShareRequest(
requestingDeviceId = credentials.deviceId,
secretName = secretName,
requestId = createUniqueTxnId()
)

val contentMap = MXUsersDevicesMap<Any>()
contentMap.setObject(credentials.userId, "*", toDeviceContent)
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved

val params = SendToDeviceTask.Params(
eventType = EventType.REQUEST_SECRET,
contentMap = contentMap
)
try {
withContext(coroutineDispatchers.io) {
sendToDeviceTask.execute(params)
}
Timber.tag(loggerTag.value)
.d("Secret request sent for $secretName")
} catch (failure: Throwable) {
Timber.tag(loggerTag.value)
.w("Failed to request secret $secretName")
}
}
}

suspend fun onSecretSendReceived(toDevice: Event, handleGossip: ((name: String, value: String) -> Boolean)) {
Timber.tag(loggerTag.value)
.i("onSecretSend() from ${toDevice.senderId} : onSecretSendReceived ${toDevice.content?.get("sender_key")}")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ import org.matrix.android.sdk.api.rendezvous.model.SecureRendezvousChannelAlgori
import org.matrix.android.sdk.api.rendezvous.transports.SimpleHttpRendezvousTransport
import org.matrix.android.sdk.api.session.Session
import org.matrix.android.sdk.api.session.crypto.crosssigning.DeviceTrustLevel
import org.matrix.android.sdk.api.session.crypto.crosssigning.KEYBACKUP_SECRET_SSSS_NAME
import org.matrix.android.sdk.api.session.crypto.crosssigning.MASTER_KEY_SSSS_NAME
import org.matrix.android.sdk.api.session.crypto.crosssigning.SELF_SIGNING_KEY_SSSS_NAME
import org.matrix.android.sdk.api.session.crypto.crosssigning.USER_SIGNING_KEY_SSSS_NAME
import org.matrix.android.sdk.api.util.MatrixJsonParser
import timber.log.Timber

Expand Down Expand Up @@ -222,15 +218,10 @@ class Rendezvous(
Timber.tag(TAG).i("No master key given by verifying device")
}

// request secrets from the verifying device
Timber.tag(TAG).i("Requesting secrets from $verifyingDeviceId")
// request secrets from other sessions.
Timber.tag(TAG).i("Requesting secrets from other sessions")

session.sharedSecretStorageService().let {
it.requestSecret(MASTER_KEY_SSSS_NAME, verifyingDeviceId)
it.requestSecret(SELF_SIGNING_KEY_SSSS_NAME, verifyingDeviceId)
it.requestSecret(USER_SIGNING_KEY_SSSS_NAME, verifyingDeviceId)
it.requestSecret(KEYBACKUP_SECRET_SSSS_NAME, verifyingDeviceId)
}
session.sharedSecretStorageService().requestMissingSecrets()
Copy link
Member

Choose a reason for hiding this comment

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

I understand that we request the secrets from all our devices, and not the device which was used to verify (verifyingDeviceId). I think it's fine, but maybe update the comment and the log above?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

} else {
Timber.tag(TAG).i("Not doing verification")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,5 +135,11 @@ interface SharedSecretStorageService {

fun checkShouldBeAbleToAccessSecrets(secretNames: List<String>, keyId: String?): IntegrityResult

@Deprecated("Requesting custom secrets not yet support by rust stack, prefer requestMissingSecrets")
suspend fun requestSecret(name: String, myOtherDeviceId: String)

/**
* Request the missing local secrets to other sessions.
*/
suspend fun requestMissingSecrets()
}
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,12 @@ internal class DefaultSharedSecretStorageService @Inject constructor(
return IntegrityResult.Success(keyInfo.content.passphrase != null)
}

@Deprecated("Requesting custom secrets not yet support by rust stack, prefer requestMissingSecrets")
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
override suspend fun requestSecret(name: String, myOtherDeviceId: String) {
secretShareManager.requestSecretTo(myOtherDeviceId, name)
}

override suspend fun requestMissingSecrets() {
secretShareManager.requestMissingSecrets()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ import org.matrix.rustcomponents.sdk.crypto.DeviceLists
import org.matrix.rustcomponents.sdk.crypto.EncryptionSettings
import org.matrix.rustcomponents.sdk.crypto.KeyRequestPair
import org.matrix.rustcomponents.sdk.crypto.KeysImportResult
import org.matrix.rustcomponents.sdk.crypto.LocalTrust
import org.matrix.rustcomponents.sdk.crypto.Logger
import org.matrix.rustcomponents.sdk.crypto.MegolmV1BackupKey
import org.matrix.rustcomponents.sdk.crypto.Request
Expand Down Expand Up @@ -869,6 +870,11 @@ internal class OlmMachine @Inject constructor(
}
}

suspend fun requestMissingSecretsFromOtherSessions(): Boolean {
return withContext(coroutineDispatchers.io) {
inner.queryMissingSecretsFromOtherSessions()
}
}
@Throws(CryptoStoreException::class)
suspend fun enableBackupV1(key: String, version: String) {
return withContext(coroutineDispatchers.computation) {
Expand Down Expand Up @@ -934,4 +940,11 @@ internal class OlmMachine @Inject constructor(
inner.verifyBackup(serializedAuthData)
}
}

@Throws(CryptoStoreException::class)
suspend fun setDeviceLocalTrust(userId: String, deviceId: String, trusted: Boolean) {
withContext(coroutineDispatchers.io) {
inner.setLocalTrust(userId, deviceId, if (trusted) LocalTrust.VERIFIED else LocalTrust.UNSET)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import org.matrix.android.sdk.api.MatrixCoroutineDispatchers
import org.matrix.android.sdk.api.auth.UserInteractiveAuthInterceptor
import org.matrix.android.sdk.api.crypto.MXCRYPTO_ALGORITHM_MEGOLM
import org.matrix.android.sdk.api.crypto.MXCryptoConfig
import org.matrix.android.sdk.api.extensions.orFalse
import org.matrix.android.sdk.api.extensions.tryOrNull
import org.matrix.android.sdk.api.listeners.ProgressListener
import org.matrix.android.sdk.api.logger.LoggerTag
Expand Down Expand Up @@ -536,7 +537,8 @@ internal class RustCryptoService @Inject constructor(
}

override suspend fun setDeviceVerification(trustLevel: DeviceTrustLevel, userId: String, deviceId: String) {
TODO("Not yet implemented")
Timber.w("Rust stack only support API to set local trust")
olmMachine.setDeviceLocalTrust(userId, deviceId, trustLevel.isLocallyVerified().orFalse())
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,29 @@

package org.matrix.android.sdk.internal.crypto

import org.matrix.android.sdk.BuildConfig
import org.matrix.android.sdk.api.session.events.model.EventType
import org.matrix.android.sdk.internal.crypto.network.OutgoingRequestsProcessor
import org.matrix.rustcomponents.sdk.crypto.Request
import timber.log.Timber
import javax.inject.Inject
import javax.inject.Provider

internal class SecretShareManager @Inject constructor() {
internal class SecretShareManager @Inject constructor(
private val olmMachine: Provider<OlmMachine>,
private val outgoingRequestsProcessor: OutgoingRequestsProcessor) {

suspend fun requestSecretTo(deviceId: String, secretName: String) {
// nop in rust?
if (BuildConfig.DEBUG) TODO("requestSecretTo Not implemented in Rust")
Timber.e("SecretShareManager Not supported in rust $deviceId, $secretName")
Timber.w("SecretShareManager requesting custom secrets not supported $deviceId, $secretName")
// rust stack only support requesting secrets defined in the spec (not custom secret yet)
requestMissingSecrets()
}

suspend fun requestMissingSecrets() {
this.olmMachine.get().requestMissingSecretsFromOtherSessions()

// immediately send the requests
outgoingRequestsProcessor.processOutgoingRequests(this.olmMachine.get()) {
it is Request.ToDevice && it.eventType == EventType.REQUEST_SECRET
}
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,15 @@ class FakeSharedSecretStorageService : SharedSecretStorageService by mockk() {

override fun checkShouldBeAbleToAccessSecrets(secretNames: List<String>, keyId: String?) = integrityResult

@Deprecated("Requesting custom secrets not yet support by rust stack, prefer requestMissingSecrets")
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
override suspend fun requestSecret(name: String, myOtherDeviceId: String) {
TODO("Not yet implemented")
}

override suspend fun requestMissingSecrets() {
TODO("Not yet implemented")
}

fun givenIsRecoverySetupReturns(isRecoverySetup: Boolean) {
every { isRecoverySetup() } returns isRecoverySetup
}
Expand Down
Loading