Update/Revise SDK to implement reference flowchart for key sharing/forwarding + use backup #5559

BillCarsonFr · 2022-03-16T17:16:17Z

Type of change

Bugfix

Content

A complete revision of the SDK key requesting and forwarding code.
The code has been modified to follow the reference recommandation for key forwarding and requesting.

Key Requesting is now managed by the OutgoingKeyRequestManager. The lifecycle is now based on the sync pulse:
- Improvement Before sending a key request we first check the megolm backup for that session
- Improvement Key requests where cancelled too early, now we keep track of the wanted ratchet index and keep the request alive until we have the proper index
- Improvement We wait for catchup syncs before sending request to avoid unneccessary to_device traffic
- Improvement using MSC3735 we can now have a track of the replies from a key request (withheld from xxx, forwarded from xxx with index....) That would allow to provide better UI feedback
- Keys are now reshared to our own unverified device if it was previously shared
- Most of the code as been refactored, database entities have changed
Secret management has been reworked and separated from key request, as it happens during interactive verification it's not using anymore workers, but it is replying directly and faster.
We don't download the full backup after verification as it can be very long and was causing incomplete session as the megolm backup key was only saved after the full import (for big backups the megolm key might not be saved at all if the app is killed before download complete). With the new system it's much faster as you will query keys on the fly when the timeline is opened (you can still download the complete backup from settings, with proper feedback).
A new SDK configuration to limit key request to the users own device has been added (requested by some forks), see MXCryptoConfig
** Update ** Remove Woker usage for verification, as it's an interactive process there's no reason to use workers
Resurrected Withheld and verification tests

Motivation and context

Screenshots / GIFs

Tests

Step 1
Step 2
Step ...

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

github-actions · 2022-03-17T08:11:42Z

Unit Test Results

122 files ±0 122 suites ±0 2m 8s ⏱️ -16s
205 tests ±0 205 ✔️ ±0 0 💤 ±0 0 ❌ ±0
690 runs ±0 690 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 3948f26. ± Comparison against base commit 04cadb9.

♻️ This comment has been updated with latest results.

bmarty · 2022-04-14T10:20:55Z

(Waiting for this PR to be rebased to review it)

bmarty

Amazing work, thanks! A few remarks after a static review.

...x-sdk-android/src/androidTest/java/org/matrix/android/sdk/internal/crypto/E2eeSanityTests.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/crypto/MXCryptoConfig.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/crypto/CryptoService.kt

...rc/main/java/org/matrix/android/sdk/internal/crypto/store/db/migration/MigrateCryptoTo016.kt

.../src/main/java/org/matrix/android/sdk/internal/crypto/store/db/model/GossipingEventEntity.kt

...src/main/java/im/vector/app/features/crypto/verification/VerificationBottomSheetViewModel.kt

bmarty

Thanks for the update, more remarks.

I have also tested the Realm migration, it is working fine.

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

bmarty · 2022-04-26T13:10:58Z

...java/org/matrix/android/sdk/internal/crypto/verification/VerificationTransportRoomMessage.kt

-        Timber.w("## SAS ignored verification ready with methods: ${keyReq.methods}")
-    }
-}
+/*


.../src/main/java/im/vector/app/features/settings/devtools/GossipingEventsPaperTrailFragment.kt

...droid/src/androidTest/java/org/matrix/android/sdk/internal/crypto/gossiping/KeyShareTests.kt

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/crypto/MXCryptoConfig.kt

...rc/main/java/org/matrix/android/sdk/internal/crypto/store/db/migration/MigrateCryptoTo005.kt

...roid/src/main/java/org/matrix/android/sdk/internal/crypto/store/db/model/AuditTrailMapper.kt

.../main/java/org/matrix/android/sdk/internal/crypto/store/db/model/OutgoingKeyRequestEntity.kt

...rc/main/java/org/matrix/android/sdk/internal/session/room/membership/joining/JoinRoomTask.kt

vector/src/main/java/im/vector/app/features/crypto/keysrequest/KeyRequestHandler.kt

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

use megolm backup before sending key request

Crypto config to only request to own device. Only cancel request if ratchet index is low enough

and disabled prompting for untrusted key sharing

bmarty

One more remark, also there are some un-handled remarks

...java/org/matrix/android/sdk/internal/crypto/verification/VerificationTransportRoomMessage.kt

ericmigi · 2022-05-05T15:46:24Z

FYI Beeper is now testing this PR internally, we'll see how it goes!

bmarty

Thanks for the update!

bmarty

Thanks for the update. Let's merge it!

github-actions · 2022-05-11T11:46:42Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
= passed=15 failures=5 errors=0 skipped=3
[org.matrix.android.sdk.account]
= passed=3 failures=0 errors=0 skipped=2
[org.matrix.android.sdk.internal]
= passed=90 failures=40 errors=0 skipped=31
[org.matrix.android.sdk.ordering]
= passed=16 failures=0 errors=0 skipped=0
[org.matrix.android.sdk.PermalinkParserTest]
= passed=2 failures=0 errors=0 skipped=0

BillCarsonFr force-pushed the feature/bca/crypto_better_key_share branch from 5ce8765 to 569a3fc Compare March 18, 2022 16:12

BillCarsonFr requested review from bmarty, ganfra and yostyle March 18, 2022 16:15

BillCarsonFr marked this pull request as ready for review March 18, 2022 16:16

BillCarsonFr changed the title ~~Feature/bca/crypto better key share~~ Update/Revise SDK to implement reference flowchart for key sharing/forwarding + use backup Mar 18, 2022

BillCarsonFr force-pushed the feature/bca/crypto_better_key_share branch from 32e5033 to 7e68b1d Compare March 29, 2022 12:10

BillCarsonFr force-pushed the feature/bca/crypto_better_key_share branch 4 times, most recently from 3d64727 to ba03413 Compare April 20, 2022 16:06

bmarty reviewed Apr 21, 2022

View reviewed changes

BillCarsonFr force-pushed the feature/bca/crypto_better_key_share branch 2 times, most recently from afd905b to a4314c0 Compare April 25, 2022 15:06

BillCarsonFr mentioned this pull request Apr 25, 2022

Add a cryptoConfig to limit room key requests #4333

Closed

BillCarsonFr force-pushed the feature/bca/crypto_better_key_share branch from 70720e9 to 58c4c4e Compare April 26, 2022 07:14

bmarty requested changes Apr 26, 2022

View reviewed changes

bmarty reviewed Apr 26, 2022

View reviewed changes

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

BillCarsonFr added 10 commits April 26, 2022 17:27

Refactor key and secret request managers

9177cb1

use megolm backup before sending key request

Add share test + fix

9747eb2

Crypto config to only request to own device. Only cancel request if ratchet index is low enough

fix db migration

b1db6ca

Add incoming key forward trail

ae6df46

fix unused var

6a509ce

Add option to disable key gossip, clear key request on trust change

1d948d6

Add change log + quick quality fix

81b114f

Fix database migration

cc10749

Fix unneeded re-uploade of key got from backup

88cf1a5

and disabled prompting for untrusted key sharing

proper initial withheld support

54fb4ae

BillCarsonFr added 12 commits April 26, 2022 17:30

Stop using workers for interactive verification

f9dd3b9

Reactivate withheld and verification tests

a60171c

Fix tests

3f5f2dc

Move some crypto classes to API + cleaning

631ea50

FIx unit test compilation

effbc47

Cleaning, review

885f836

Fix: ignore key request form self devices

058d2e6

Fix trail display (from instead of to for incoming types)

9385d19

Cleaning, code review

eaf1044

Change log for SDK apis

012dfeb

Fix missing mapper for incoming key forward trail

728faae

Code review

8920ed3

BillCarsonFr force-pushed the feature/bca/crypto_better_key_share branch from df8eb40 to 8920ed3 Compare April 27, 2022 07:45

Merge branch 'develop' into feature/bca/crypto_better_key_share

23f32aa

bmarty requested changes Apr 27, 2022

View reviewed changes

...java/org/matrix/android/sdk/internal/crypto/verification/VerificationTransportRoomMessage.kt Outdated Show resolved Hide resolved

BillCarsonFr added 2 commits April 28, 2022 09:09

Code review

0f06368

Fix make verif scope as a child of crypto scope

de580cc

BillCarsonFr mentioned this pull request May 6, 2022

Share Megolm session keys when inviting a new user #5853

Merged

bmarty approved these changes May 10, 2022

View reviewed changes

BillCarsonFr added 4 commits May 10, 2022 19:30

Merge branch 'develop' into feature/bca/crypto_better_key_share

12ab478

post merge fix

546d6fe

use clock time instead of system

992f477

fix method visibility

3948f26

bmarty approved these changes May 11, 2022

View reviewed changes

BillCarsonFr merged commit 304cb07 into develop May 11, 2022

BillCarsonFr deleted the feature/bca/crypto_better_key_share branch May 11, 2022 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update/Revise SDK to implement reference flowchart for key sharing/forwarding + use backup #5559

Update/Revise SDK to implement reference flowchart for key sharing/forwarding + use backup #5559

BillCarsonFr commented Mar 16, 2022 •

edited

Loading

github-actions bot commented Mar 17, 2022 •

edited

Loading

bmarty commented Apr 14, 2022

bmarty left a comment

bmarty left a comment

bmarty Apr 26, 2022

bmarty left a comment •

edited

Loading

ericmigi commented May 5, 2022

bmarty left a comment

bmarty left a comment

github-actions bot commented May 11, 2022

Update/Revise SDK to implement reference flowchart for key sharing/forwarding + use backup #5559

Update/Revise SDK to implement reference flowchart for key sharing/forwarding + use backup #5559

Conversation

BillCarsonFr commented Mar 16, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Mar 17, 2022 • edited Loading

Unit Test Results

bmarty commented Apr 14, 2022

bmarty left a comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

bmarty Apr 26, 2022

Choose a reason for hiding this comment

bmarty left a comment • edited Loading

Choose a reason for hiding this comment

ericmigi commented May 5, 2022

bmarty left a comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

github-actions bot commented May 11, 2022

Matrix SDK

Integration Tests Results:

BillCarsonFr commented Mar 16, 2022 •

edited

Loading

github-actions bot commented Mar 17, 2022 •

edited

Loading

bmarty left a comment •

edited

Loading