Send one time key count and unused fallback keys for rust-crypto #3215

florianduros · 2023-03-14T09:53:15Z

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
Sign-off given on the changes (see CONTRIBUTING.md)

Fix element-hq/element-web#24795 (comment)

Here's what your changelog entry will look like:

✨ Features

Send one time key count and unused fallback keys for rust-crypto (#3215). Fixes Element-R: Wire up remaining bits of /sync response element-hq/element-web#24795. Contributed by @florianduros.

…parameters

richvdh

@florianduros thanks for getting this working! it's a great start but there's a few things that need a bit of polish before we can call it done imho.

richvdh · 2023-03-22T11:09:06Z

src/common-crypto/CryptoBackend.ts

+    /**
+     * Called by the /sync loop whenever there are incoming to-device messages.
+     *
+     * The implementation may preprocess the received messages (eg, decrypt them) and return an
+     * updated list of messages for dispatch to the rest of the system.
+     *
+     * Note that, unlike {@link ClientEvent.ToDeviceEvent} events, this is called on the raw to-device
+     * messages, rather than the results of any decryption attempts.
+     *
+     * @param oneTimeKeysCounts - the received one time key counts
+     * @returns A list of preprocessed to-device messages.
+     */


These doc-comments are inaccurate: these methods aren't doing anything with received messages, nor returning them.

richvdh · 2023-03-22T11:10:09Z

src/common-crypto/CryptoBackend.ts

+     * @param oneTimeKeysCounts - the received one time key counts
+     * @returns A list of preprocessed to-device messages.
+     */
+    preprocessOneTimeKeyCounts(oneTimeKeysCounts: Map<string, number>): Promise<void>;


preprocess... is not a good name for these methods. "Preprocess" implies there is still some "processing" to be done after the "preprocessing" takes place. You can just use "process" here.

richvdh · 2023-03-22T11:12:44Z

src/sync.ts

-            const currentCount = data.device_one_time_keys_count.signed_curve25519 || 0;
-            this.syncOpts.crypto.updateOneTimeKeyCount(currentCount);
+        if (data.device_one_time_keys_count) {
+            const map = new Map<string, number>(Object.entries(data.device_one_time_keys_count));


I think we should leave this conversion into a Map (or Set, below) to the cryptoCallbacks implementation.

richvdh · 2023-03-22T11:13:01Z

src/sync.ts

+            this.syncOpts.cryptoCallbacks?.preprocessOneTimeKeyCounts(map);
        }
-        if (
-            this.syncOpts.crypto &&
-            (data.device_unused_fallback_key_types || data["org.matrix.msc2732.device_unused_fallback_key_types"])
-        ) {
+        if (data.device_unused_fallback_key_types || data["org.matrix.msc2732.device_unused_fallback_key_types"]) {
            // 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.
            const unusedFallbackKeys =
                data.device_unused_fallback_key_types || data["org.matrix.msc2732.device_unused_fallback_key_types"];
-            this.syncOpts.crypto.setNeedsNewFallback(
-                Array.isArray(unusedFallbackKeys) && !unusedFallbackKeys.includes("signed_curve25519"),
-            );
+            this.syncOpts.cryptoCallbacks?.preprocessUnusedFallbackKeys(new Set<string>(unusedFallbackKeys || null));


It might be easier to combine these two methods into one?

this.syncOpts.cryptoCallbacks?.processKeyCounts(data.device_one_time_keys_count, data.device_unused_fallback_key_types ?? data["org.matrix.msc2732.device_unused_fallback_key_types"]);

richvdh · 2023-03-22T11:15:47Z

src/sync.ts

-        if (this.syncOpts.crypto && data.device_one_time_keys_count) {
-            const currentCount = data.device_one_time_keys_count.signed_curve25519 || 0;
-            this.syncOpts.crypto.updateOneTimeKeyCount(currentCount);
+        if (data.device_one_time_keys_count) {
+            const map = new Map<string, number>(Object.entries(data.device_one_time_keys_count));
+            this.syncOpts.cryptoCallbacks?.preprocessOneTimeKeyCounts(map);
        }
-        if (
-            this.syncOpts.crypto &&
-            (data.device_unused_fallback_key_types || data["org.matrix.msc2732.device_unused_fallback_key_types"])
-        ) {
+        if (data.device_unused_fallback_key_types || data["org.matrix.msc2732.device_unused_fallback_key_types"]) {
            // 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.
            const unusedFallbackKeys =
                data.device_unused_fallback_key_types || data["org.matrix.msc2732.device_unused_fallback_key_types"];
-            this.syncOpts.crypto.setNeedsNewFallback(
-                Array.isArray(unusedFallbackKeys) && !unusedFallbackKeys.includes("signed_curve25519"),
-            );
+            this.syncOpts.cryptoCallbacks?.preprocessUnusedFallbackKeys(new Set<string>(unusedFallbackKeys || null));
        }


we also need to do this for sliding-sync-sdk.ts, by the way.

richvdh · 2023-03-22T11:17:49Z

spec/integ/crypto.spec.ts

+                    {
+                        overwriteRoutes: true,
+                    },


why do we overwriteRoutes here? it could do with a comment at least.

richvdh · 2023-03-22T11:18:45Z

spec/integ/crypto.spec.ts

+                fetchMock.post(
+                    new URL("/_matrix/client/r0/keys/upload", aliceClient.getHomeserverUrl()).toString(),
+                    listener,
+                    {
+                        overwriteRoutes: true,
+                    },
+                );
+                fetchMock.post(
+                    new URL("/_matrix/client/v3/keys/upload", aliceClient.getHomeserverUrl()).toString(),
+                    listener,
+                    {
+                        overwriteRoutes: true,
+                    },
+                );


maybe to avoid the repetition here, do for (const path in ["/_matrix/client/r0/keys/upload", "/_matrix/client/v3/keys/upload"]).

richvdh · 2023-03-22T11:19:14Z

spec/integ/crypto.spec.ts

+        it("should make key upload request after sync", async () => {
+            let uploadPromise = listenToUpload();
+            expectAliceKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });
+            await startClientAndAwaitFirstSync();
+
+            syncResponder.sendOrQueueSyncResponse(getSyncResponse([]));
+
+            await syncPromise(aliceClient);
+            expect(await uploadPromise).toBeGreaterThan(0);
+
+            uploadPromise = listenToUpload();
+            syncResponder.sendOrQueueSyncResponse({
+                next_batch: 2,
+                device_one_time_keys_count: { signed_curve25519: 0 },
+            });
+
+            // Advance local date to 2 minutes
+            // The old crypto only runs the upload every 60 seconds
+            jest.setSystemTime(Date.now() + 2 * 60 * 1000);
+
+            await syncPromise(aliceClient);
+
+            expect(await uploadPromise).toBeGreaterThan(0);
+        });


we could probably do with a test for the fallback keys too.

richvdh · 2023-03-22T11:20:18Z

spec/integ/crypto.spec.ts

+                    return {
+                        one_time_key_counts: {
+                            signed_curve25519: keysCount ? 60 : keysCount,
+                        },
+                    };


what's going on here? how did we pick these numbers? why does it depend on keysCount? add some comments please!

richvdh · 2023-03-22T11:20:51Z

spec/integ/crypto.spec.ts

+            let uploadPromise = listenToUpload();
+            expectAliceKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });
+            await startClientAndAwaitFirstSync();
+
+            syncResponder.sendOrQueueSyncResponse(getSyncResponse([]));
+
+            await syncPromise(aliceClient);
+            expect(await uploadPromise).toBeGreaterThan(0);
+
+            uploadPromise = listenToUpload();
+            syncResponder.sendOrQueueSyncResponse({
+                next_batch: 2,
+                device_one_time_keys_count: { signed_curve25519: 0 },
+            });
+
+            // Advance local date to 2 minutes
+            // The old crypto only runs the upload every 60 seconds
+            jest.setSystemTime(Date.now() + 2 * 60 * 1000);
+
+            await syncPromise(aliceClient);
+
+            expect(await uploadPromise).toBeGreaterThan(0);


this could do with some more comments imho. What is going on, what are we checking for?

* Improve key upload request * Add fallback keys check * Review fixes * Add comments about sliding sync usage of `processKeyCounts` * Review fixes * Better wording

@andybalaam

* Allow via_servers property in findPredecessor (update to MSC3946) ([\matrix-org#3240](matrix-org#3240)). Contributed by @andybalaam. * Fire `closed` event when IndexedDB closes unexpectedly ([\matrix-org#3218](matrix-org#3218)). * Implement MSC3952: intentional mentions ([\matrix-org#3092](matrix-org#3092)). Fixes element-hq/element-web#24376. * Send one time key count and unused fallback keys for rust-crypto ([\matrix-org#3215](matrix-org#3215)). Fixes element-hq/element-web#24795. Contributed by @florianduros. * Improve `processBeaconEvents` hotpath ([\matrix-org#3200](matrix-org#3200)). * Implement MSC3966: a push rule condition to check if an array contains a value ([\matrix-org#3180](matrix-org#3180)). * indexddb-local-backend - return the current sync to database promise … ([\matrix-org#3222](matrix-org#3222)). Contributed by @texuf. * Revert "Add the call object to Call events" ([\matrix-org#3236](matrix-org#3236)). * Handle group call redaction ([\matrix-org#3231](matrix-org#3231)). Fixes vector-im/voip-internal#128. * Stop doing O(n^2) work to find event's home (`eventShouldLiveIn`) ([\matrix-org#3227](matrix-org#3227)). Contributed by @jryans. * Fix bug where video would not unmute if it started muted ([\matrix-org#3213](matrix-org#3213)). Fixes element-hq/element-call#925. * Fixes to event encryption in the Rust Crypto implementation ([\matrix-org#3202](matrix-org#3202)).

github-actions bot deployed to PR Documentation Preview March 14, 2023 09:55 View deployment

florianduros added the T-Enhancement label Mar 14, 2023

Send one time key count and unused fallback keys for rust-crypto

051d1e9

florianduros force-pushed the florianduros/feat/add-oldmachine-missing-parameters branch from 5651a58 to 051d1e9 Compare March 14, 2023 15:53

github-actions bot deployed to PR Documentation Preview March 14, 2023 15:56 View deployment

florianduros added 2 commits March 15, 2023 17:51

Add tests

5b39072

Merge branch 'develop' into florianduros/feat/add-oldmachine-missing-…

ccfcf19

…parameters

github-actions bot deployed to PR Documentation Preview March 15, 2023 16:55 View deployment

florianduros added 2 commits March 16, 2023 15:21

Remove useless type in promise return

677d0bf

Add test for one time key upload

bb2f2ae

github-actions bot deployed to PR Documentation Preview March 16, 2023 17:41 View deployment

Fix rust-crypto.spec.ts tests

ee0d82a

github-actions bot deployed to PR Documentation Preview March 17, 2023 14:08 View deployment

Remove unneeded code in test

a67a474

github-actions bot deployed to PR Documentation Preview March 17, 2023 14:12 View deployment

Merge branch 'develop' into florianduros/feat/add-oldmachine-missing-…

7cc770d

…parameters

github-actions bot deployed to PR Documentation Preview March 17, 2023 14:17 View deployment

Add key upload request test

a0410c3

github-actions bot deployed to PR Documentation Preview March 21, 2023 14:42 View deployment

Merge branch 'develop' into florianduros/feat/add-oldmachine-missing-…

7dc7d78

…parameters

github-actions bot deployed to PR Documentation Preview March 21, 2023 14:54 View deployment

florianduros added 2 commits March 22, 2023 11:05

Fix tests

7ea6318

Merge branch 'develop' into florianduros/feat/add-oldmachine-missing-…

9342739

…parameters

github-actions bot deployed to PR Documentation Preview March 22, 2023 10:08 View deployment

florianduros marked this pull request as ready for review March 22, 2023 10:16

florianduros requested a review from a team as a code owner March 22, 2023 10:16

florianduros requested review from t3chguy and weeman1337 March 22, 2023 10:16

t3chguy approved these changes Mar 22, 2023

View reviewed changes

florianduros added this pull request to the merge queue Mar 22, 2023

florianduros merged commit f795577 into develop Mar 22, 2023

florianduros deleted the florianduros/feat/add-oldmachine-missing-parameters branch March 22, 2023 10:45

richvdh reviewed Mar 22, 2023

View reviewed changes

richvdh mentioned this pull request Mar 22, 2023

Element-R: Wire up remaining bits of /sync response element-hq/element-web#24795

Closed

florianduros mentioned this pull request Mar 23, 2023

Improve #3215 implementation #3226

Merged

3 tasks

florianduros added a commit that referenced this pull request Apr 5, 2023

Improve #3215 implementation (#3226)

2daa429

* Improve key upload request * Add fallback keys check * Review fixes * Add comments about sliding sync usage of `processKeyCounts` * Review fixes * Better wording

florianduros mentioned this pull request Apr 5, 2023

Use processKeyCounts in sliding sync #3253

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send one time key count and unused fallback keys for rust-crypto #3215

Send one time key count and unused fallback keys for rust-crypto #3215

florianduros commented Mar 14, 2023 •

edited by github-actions bot

Loading

richvdh left a comment

richvdh Mar 22, 2023

richvdh Mar 22, 2023

richvdh Mar 22, 2023

richvdh Mar 22, 2023

richvdh Mar 22, 2023

richvdh Mar 22, 2023

richvdh Mar 22, 2023

richvdh Mar 22, 2023

richvdh Mar 22, 2023

richvdh Mar 22, 2023

Send one time key count and unused fallback keys for rust-crypto #3215

Send one time key count and unused fallback keys for rust-crypto #3215

Conversation

florianduros commented Mar 14, 2023 • edited by github-actions bot Loading

Checklist

✨ Features

richvdh 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

Choose a reason for hiding this comment

florianduros commented Mar 14, 2023 •

edited by github-actions bot

Loading