Fallback keys implementation #3970

onurays · 2021-09-06T07:53:10Z

Problem

Olm uses a set of one-time keys when initializing a session between two devices: Alice uploads one-time keys to her homeserver, and Bob claims one of them to perform a Diffie-Hellman to generate a shared key. As implied by the name, a one-time key is only to be used once. However, if all of Alice's one-time keys are claimed, Bob will not be able to create a session with Alice.

Proposal

A new response parameter, device_unused_fallback_key_types is added to /sync. If the client wants the server to have a fallback key for a given key algorithm, but that algorithm is not listed in device_unused_fallback_key_types, the client will upload a new key.

Here is the proposed logic to decide whether we need to generate a new fallback keys or not:

// The presence of device_unused_fallback_key_types indicates that the server supports fallback keys.
// If there's no unused signed_curve25519 fallback key we need a new one.
val shouldGenerateFallbackKey = if (syncResponse.deviceUnusedFallbackKeyTypes != null) {
    // Generate a fallback key only if the server does not already have an unused fallback key.
    !syncResponse.deviceUnusedFallbackKeyTypes.contains(KEY_SIGNED_CURVE_25519_TYPE)
} else {
    // Server does not support fallbackKey
    false
}

We need to use the existing UploadKeysTask by adding fallbackKeys parameter to the request.

bmarty

Some remarks

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/MXOlmDevice.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt

...dk-android/src/main/java/org/matrix/android/sdk/internal/crypto/model/rest/KeysUploadBody.kt

bmarty · 2021-09-06T13:49:40Z

Also, of course you will have to remove the binary and upgrade the dependency on Olm library once this one will be release.
Please ensure to git squash to avoid binaries in the git history.

github-actions · 2021-10-05T19:16:43Z

Unit Test Results

  66 files ±0   66 suites ±0 50s ⏱️ ±0s
135 tests ±0 135 ✔️ ±0 0 💤 ±0 0 ❌ ±0
418 runs ±0 418 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 76960f8. ± Comparison against base commit 7cf92ec.

♻️ This comment has been updated with latest results.

bmarty · 2021-10-07T10:14:38Z

Also, of course you will have to remove the binary and upgrade the dependency on Olm library once this one will be release. Please ensure to git squash to avoid binaries in the git history.

Done

poljar

Looks sensible.

It's a bit weird that we're not persisting the fact that a fallback key needs to be uploaded, but the server seems to return fallback key info always and the MSC seems to suggest that this is intentional so.

So all seems to be good.

bmarty · 2021-10-11T13:57:37Z

@onurays can you add a file for the changelog please?

bmarty · 2021-10-11T14:30:28Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/MXOlmDevice.kt

+        return null
+    }
+
+    fun generateFallbackKey() {


This can be called several times if uploading the keys failed for some reason. Is it OK?

I think it is ok since we have the condition on sync response. I can't see any other way to protect it in client side.

This was discussed in chat, though never fully answered. The one-time keys will be gone from the json output if we call mark_keys_as_published(), so there's a boolean flag on the libolm side that helps.

Let me reintroduce the context. When you call the method to generate the keys (this is true for both one-time as well as fallback keys) you populate some limited amount of memory with keypairs (a private and public ed25519 key). The public parts of those keypairs will be shown in the output of your getFallbackKey() method.

Now for one-time keys, if we call mark_keys_as_published(), the public parts of the keys will be gone from the getter. This means that you can do the following flow:

Sync tells us the new one-time key count

If it's lower than our MAX and the output of getOneTimeKeys() is empty, generate some one-time keys

If there are keys in the getOneTimeKeys() output, upload them.

It's crucial that we don't call 2. blindly every time the server tells us so. As mentioned we store an limited amount of keypairs and if we generate new ones, the oldest ones will be replaced by the freshly generated ones.

For fallback keys the amount of keypairs we store in libolm is 2. One slot is for the current, unused, fallback key, the other one for the previous fallback key.

Now the problem is, mark_keys_as_published() does not touch the fallback keys. It isn't quite clear how to handle this considering the above.

@uhoreg could you clarify how this is supposed to work?

This is a more global problem, would be fixed by some changes on lib olm. Will be handle in a next PR

Why? If you want to merge this PR before libolm is fixed you can easily store the fact that a fallback key has been uploaded or not yourself:

In [1]: import olm In [2]: x = olm.Account() In [3]: x.fallback_key Out[3]: {'curve25519': {}} In [4]: x.generate_fallback_key() In [5]: x.fallback_key Out[5]: {'curve25519': {'AAAAAQ': '2XcMkoLlaJrKeNxV7yH4lUlJKc87u4QSrjbs/Js2KxQ'}} In [6]: # Store the key ID 'AAAAAQ` and a boolean if it has been uploaded or not

On the other hand, this PR has been open now for more than a month, I raised the issue 3 weeks ago without any significant movement on it until now. Waiting a day or two for the libolm fix wouldn't add a significant amount of delay here.

Ok, can wait it will be on next EA release

PR is updated, the java binding are now directly calling the new unpublished_* olm calls for fallback keys.
So if a new fallback key is needed but there is still an unpublished one locally we retry to upload it.
Also added simple mechanism to forget fallback key if a new one as been published more than 5mn ago.

Tested against https://gitlab.matrix.org/uhoreg/matrix-client-test-bot

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt

poljar

As mentioned, we should indeed somewhere store the fact if a fallback key is uploaded or not. But let's see what the intended method here is.

bmarty

LGTM

Author: Onuray - Benoit squashes the 4 commit to cancel the addition on binaries

And forgetFallback after 5mn

poljar

Seems like we can simplify this a bit. Removing the flag and moving the hasUnpublishedFallbackKey() check would simplify the code quite a bit.

Otherwise looks good.

poljar · 2021-12-08T10:18:57Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt

+                    if (syncResponse.deviceUnusedFallbackKeyTypes != null &&
+                            // Generate a fallback key only if the server does not already have an unused fallback key.
+                            !syncResponse.deviceUnusedFallbackKeyTypes.contains(KEY_SIGNED_CURVE_25519_TYPE)) {
+                        oneTimeKeysUploader.setNeedsNewFallback()


Can't we just generate the key directly here? It seems to be a bit convoluted to set the flag here and then somewhere else in the code the flag may or may not trigger the generation of the fallback key.

yes updated.

poljar · 2021-12-08T10:22:29Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt

+            keysThisLoop = min(keyLimit - keyCount, ONE_TIME_KEY_GENERATION_MAX_NUMBER)
+            olmDevice.generateOneTimeKeys(keysThisLoop)
+        }
+        if (needNewFallbackKey && !hasUnpublishedFallbackKey()) {


I would move the condition hasUnpublishedFallbackKey() into the generateFallbackKey() method. If the code changes around someone could call generateFallbackKey() in another place, or this check may be forgotten.

This would lead to overrotation of the fallback key quite easily, if we instead only generate a fallback key if there isn't one already there, the chances of misuse are lowered.

poljar · 2021-12-08T10:29:49Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt

+
+        // Check if we need to forget a fallback key
+        val latestPublishedTime = getLastFallbackKeyPublishTime()
+        if (latestPublishedTime != 0L && System.currentTimeMillis() - latestPublishedTime > FIVE_MINUTES) {


I would probably give this a bit more leeway, 5 minutes is quite the short amount of time in a federated world where your server might be down for a while.

Perhaps 1h?

Done, but would need to sync with js-sdk implementation

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt

poljar · 2021-12-08T13:53:23Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt

 ) {
    // tell if there is a OTK check in progress
    private var oneTimeKeyCheckInProgress = false

    // last OTK check timestamp
    private var lastOneTimeKeyCheck: Long = 0
    private var oneTimeKeyCount: Int? = null
+    private var needNewFallbackKey: Boolean = false


This flag is now always false?

It is, but the spelling issues up above aren't addressed yet. I think we can merge after those are addressed as well.

…al/crypto/OneTimeKeysUploader.kt Co-authored-by: poljar <[email protected]>

poljar

Looks good.

bmarty

Some minor remarks. Having a store could be nice though, to have unit test.

bmarty · 2021-12-10T15:49:10Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt

@@ -41,6 +47,9 @@ internal class OneTimeKeysUploader @Inject constructor(
    private var lastOneTimeKeyCheck: Long = 0
    private var oneTimeKeyCount: Int? = null

+    // Simple storage to remember when was uploaded the last fallback key
+    private val storage = context.getSharedPreferences("OneTimeKeysUploader_${olmDevice.deviceEd25519Key.hashCode()}", Context.MODE_PRIVATE)


A bit dirty... A proper store should be injected in the constructor...

bmarty · 2021-12-10T15:50:46Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt

        } else {
            Timber.e("## uploadOTK() : response for uploading keys does not contain one_time_key_counts.signed_curve25519")
            throw Exception("response for uploading keys does not contain one_time_key_counts.signed_curve25519")
        }
    }

+    private fun saveLastFallbackKeyPublishTime(timeMillis: Long) {
+        storage.edit().putLong("last_fb_key_publish", timeMillis).apply()


Nit: use edit {}

bmarty · 2021-12-10T15:51:02Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt

+    }
+
+    private fun getLastFallbackKeyPublishTime(): Long {
+        return storage.getLong("last_fb_key_publish", 0)


Defining a const should be better (in a store...)

bmarty · 2021-12-10T15:51:53Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/MXOlmDevice.kt

+    }
+
+    internal fun hasUnpublishedFallbackKey(): Boolean {
+        return getFallbackKey()?.get(OlmAccount.JSON_KEY_ONE_TIME_KEY).orEmpty().isNotEmpty()


orEmpty().isNotEmpty() is a bit weird :/

bmarty · 2021-12-13T22:36:16Z

Latest remarks will be handled later... or not.

bmarty requested changes Sep 6, 2021

View reviewed changes

bmarty mentioned this pull request Sep 17, 2021

Bump olm from 3.2.4 to 3.2.6 #4031

Closed

onurays marked this pull request as ready for review October 5, 2021 19:03

bmarty force-pushed the feature/ons/fallback_keys branch from c7bb75b to 953dcea Compare October 7, 2021 10:13

bmarty force-pushed the feature/ons/fallback_keys branch from 953dcea to a820555 Compare October 7, 2021 11:41

poljar approved these changes Oct 8, 2021

View reviewed changes

bmarty reviewed Oct 11, 2021

View reviewed changes

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt Outdated Show resolved Hide resolved

bmarty reviewed Oct 11, 2021

View reviewed changes

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt Outdated Show resolved Hide resolved

bmarty mentioned this pull request Oct 19, 2021

MSC2732: Olm fallback keys #3473

Closed

onurays linked an issue Oct 25, 2021 that may be closed by this pull request

MSC2732: Olm fallback keys #3473

Closed

onurays requested a review from bmarty October 25, 2021 09:17

poljar suggested changes Oct 25, 2021

View reviewed changes

bmarty requested a review from uhoreg October 27, 2021 08:08

BillCarsonFr force-pushed the feature/ons/fallback_keys branch from 4d4eee4 to aacde9d Compare November 15, 2021 12:52

BillCarsonFr approved these changes Nov 15, 2021

View reviewed changes

bmarty approved these changes Nov 15, 2021

View reviewed changes

BillCarsonFr force-pushed the feature/ons/fallback_keys branch from 3a34d96 to c092656 Compare December 6, 2021 16:18

bmarty and others added 5 commits December 7, 2021 19:05

Fallback keys implementation.

4ac90f1

Author: Onuray - Benoit squashes the 4 commit to cancel the addition on binaries

Code review fixes.

c603135

Quick refactor to use same mechanism as updateOneTimeKeyCount

10671a5

Cleaning

f843ddd

Support using unpublished fallback key instead of generating

5d35f02

And forgetFallback after 5mn

BillCarsonFr force-pushed the feature/ons/fallback_keys branch from c092656 to 5d35f02 Compare December 7, 2021 18:56

BillCarsonFr requested review from poljar and bmarty December 8, 2021 08:10

poljar suggested changes Dec 8, 2021

View reviewed changes

Code review

01b8b7d

poljar suggested changes Dec 8, 2021

View reviewed changes

BillCarsonFr and others added 3 commits December 8, 2021 15:47

code review

a026137

Update matrix-sdk-android/src/main/java/org/matrix/android/sdk/intern…

38a8e8b

…al/crypto/OneTimeKeysUploader.kt Co-authored-by: poljar <[email protected]>

Update matrix-sdk-android/src/main/java/org/matrix/android/sdk/intern…

76960f8

…al/crypto/OneTimeKeysUploader.kt Co-authored-by: poljar <[email protected]>

poljar approved these changes Dec 10, 2021

View reviewed changes

bmarty reviewed Dec 10, 2021

View reviewed changes

bmarty merged commit fa06005 into develop Dec 13, 2021

bmarty deleted the feature/ons/fallback_keys branch December 13, 2021 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fallback keys implementation #3970

Fallback keys implementation #3970

onurays commented Sep 6, 2021 •

edited

Loading

bmarty left a comment

bmarty commented Sep 6, 2021

github-actions bot commented Oct 5, 2021 •

edited

Loading

bmarty commented Oct 7, 2021

poljar left a comment

bmarty commented Oct 11, 2021

bmarty Oct 11, 2021

onurays Oct 25, 2021

poljar Oct 25, 2021 •

edited

Loading

BillCarsonFr Nov 15, 2021

poljar Nov 15, 2021

BillCarsonFr Nov 15, 2021

BillCarsonFr Dec 8, 2021

poljar left a comment •

edited

Loading

bmarty left a comment

poljar left a comment

poljar Dec 8, 2021

BillCarsonFr Dec 8, 2021

poljar Dec 8, 2021

BillCarsonFr Dec 8, 2021

poljar Dec 8, 2021

BillCarsonFr Dec 8, 2021

poljar Dec 8, 2021

BillCarsonFr Dec 9, 2021

poljar Dec 9, 2021

poljar left a comment

bmarty left a comment

bmarty Dec 10, 2021

bmarty Dec 10, 2021

bmarty Dec 10, 2021

bmarty Dec 10, 2021

bmarty commented Dec 13, 2021

Fallback keys implementation #3970

Fallback keys implementation #3970

Conversation

onurays commented Sep 6, 2021 • edited Loading

Problem

Proposal

bmarty left a comment

Choose a reason for hiding this comment

bmarty commented Sep 6, 2021

github-actions bot commented Oct 5, 2021 • edited Loading

Unit Test Results

bmarty commented Oct 7, 2021

poljar left a comment

Choose a reason for hiding this comment

bmarty commented Oct 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poljar Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poljar left a comment • edited Loading

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

poljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poljar left a comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty commented Dec 13, 2021

onurays commented Sep 6, 2021 •

edited

Loading

github-actions bot commented Oct 5, 2021 •

edited

Loading

poljar Oct 25, 2021 •

edited

Loading

poljar left a comment •

edited

Loading