Migrate own identity local trust to rust crypto #4090

BillCarsonFr · 2024-03-04T08:37:31Z

When you log in with a new session, that session will do a /keys/query to see if the user has existing published cross-signing keys. The new login has now to decide if these published keys can be trusted.

This is done by performing verification:

Using 4S if available: the user enters the 4S passphrase/key, then can retrieve the private parts of the cross-signing keys to see if they match the public parts.
By using another already trusted session. At the end of this process the js-sdk will mark the cross-signing keys as trusted. This is done by storing the public keys using storeTrustedSelfKeys. Later on the the new session will request the private keys from the existing session via secret gossiping.

There are several cases where you can end up with a trusted session that doesn't have the private MSK, for example it can happen if the session you verified against don't have it, or if secret gossiping failed for some reason.

The problem is that rust crypto will only consider the current session as trusted if we import the private MSK itself.
That means that a legacy trusted session that does not have the MSK in cache will revert to untrusted after migration.

This PR adds a way to detect that case, and then mark the migrated session as trusted (mark_as_verified) if the legacy session was trusted. (aka migration of local trust of own identity).

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)

src/rust-crypto/libolm_migration.ts

richvdh · 2024-03-06T10:40:33Z

This is done by performing verification:

Using 4S if available, the user enters the 4S passphrase/key, then can retrieve the private parts of the cross-signing keys to see if they match the public parts.

I might be confused, but I don't think that is what we normally mean by "verification"? Or is it?

richvdh · 2024-03-06T10:43:47Z

By using another already trusted session. At the end of this process the js-sdk will mark the cross-signing keys as trusted. This is done by storing the public keys using storeTrustedSelfKeys. Later on the the new session will request the private keys from the existing session via secret gossiping.

Am I right in understanding that this only applies to legacy crypto? When I read the description in the PR I assumed it was a general description of how MSK trust works in both legacy and ER -- is that not correct?

dkasak · 2024-03-06T11:29:49Z

I might be confused, but I don't think that is what we normally mean by "verification"? Or is it?

It's a bit ambiguous. I think we generally lack a great term for the concept that encompasses both

obtaining the user identity from an authenticated storage where we've previously stashed it ourselves
obtaining it from an existing device which we have verified in the spec sense of Device Verification

Semantically, this is authentication of the user identity. We're also referring to this as "trusting" or "establishing trust" in various places, but some people found that confusing for whatever reason.

This is an area which would benefit from some spec work, in order to introduce and define sensible terms and consolidate all public resources to use them consistently.

src/rust-crypto/index.ts

src/rust-crypto/libolm_migration.ts

spec/test-utils/test_indexeddb_cryptostore_dump/unverified/README.md

spec/test-utils/test_indexeddb_cryptostore_dump/no_cached_msk_dump/index.ts

spec/integ/crypto/rust-crypto.spec.ts

src/rust-crypto/libolm_migration.ts

richvdh · 2024-03-08T14:25:03Z

src/rust-crypto/libolm_migration.ts

+    /** The logger to use */
+    logger: Logger;
+}): Promise<void> {
+    const { legacyCryptoStore, rustCrypto, logger } = args;
    // Now get the cross-signing identity from rust.
    // If this is null that means that there are no cross-signing keys published server side.


this line is redundant to the comment on line 416 now: that's why I asked you to split out the if statements, and why my suggestion removed this comment.

Suggested change

// If this is null that means that there are no cross-signing keys published server side.

richvdh · 2024-03-08T14:41:19Z

src/rust-crypto/index.ts

@@ -197,6 +201,14 @@ async function initOlmMachine(
                // It will be retried by the sdk.


It's a bit more annoying for migrating local trust, as if it fails we wont migrate local trust. But worst case you'll have to verify manually after?

Right, this is my point. We decided that having to re-verify was unacceptable, didn't we? Especially given we don't actually tell the user they have to re-verify.

Tbh, I feel like it would be a lesser evil to block the application here - ie, sit in a loop repeatedly retrying userHasCrossSigningKeys until it works. [We know that the server is reachable, because we've already done a /room_keys/version request?] Or we could try once, and if that doesn't work, fire off a background thread to keep retrying in a loop?

spec/test-utils/test_indexeddb_cryptostore_dump/full_account/index.ts

spec/test-utils/test_indexeddb_cryptostore_dump/README.md

spec/test-utils/test_indexeddb_cryptostore_dump/full_account/README.md

spec/integ/crypto/rust-crypto.spec.ts

richvdh

Still a few comments I'm afraid. Some of this is getting a bit nit-picky so feel free to push back if you think it's excessive. The only thing that I think really matters is the part about updating the trust if the initial /keys/query request fails.

Also please look above: there are a few unresolved threads.

…eeded

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

…s://github.com/matrix-org/matrix-js-sdk into valere/element-r/migrate_local_trust_if_needed

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

richvdh

LGTM other than a couple of nits

richvdh · 2024-03-15T16:35:48Z

src/rust-crypto/libolm_migration.ts

+    /** The logger to use */
+    logger: Logger;
+}): Promise<void> {
+    const { legacyCryptoStore, rustCrypto, logger } = args;
    // Now get the cross-signing identity from rust.
    // If this is null that means that there are no cross-signing keys published server side.


Suggested change

// If this is null that means that there are no cross-signing keys published server side.

spec/test-utils/test_indexeddb_cryptostore_dump/index.ts

richvdh · 2024-03-15T16:45:47Z

spec/test-utils/test_indexeddb_cryptostore_dump/index.ts

+    /** Additional dump info specific for some tests.*/
+    [key: string]: any;


I don't think this is necessary: a class can have additional fields which aren't specified in an interface.

Suggested change

/** Additional dump info specific for some tests.*/

[key: string]: any;

I am getting that error Object literal may only specify known properties if it's not there

Migrate own identity trust to rust crypto

2a5e165

BillCarsonFr changed the title ~~Migrate own identity trust to rust crypto~~ Migrate own identity local trust to rust crypto Mar 4, 2024

BillCarsonFr added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Mar 4, 2024

Fix gendoc not happy if msk of IDownloadKeyResult has a signature

eebd5e1

github-actions bot deployed to PR Documentation Preview March 4, 2024 08:51 View deployment

add missing mock

4c76d71

github-actions bot deployed to PR Documentation Preview March 4, 2024 09:05 View deployment

BillCarsonFr marked this pull request as ready for review March 4, 2024 09:13

BillCarsonFr requested review from a team as code owners March 4, 2024 09:13

BillCarsonFr requested review from uhoreg, richvdh, dbkr and dkasak March 4, 2024 09:13

dbkr reviewed Mar 5, 2024

View reviewed changes

src/rust-crypto/libolm_migration.ts Outdated Show resolved Hide resolved

src/rust-crypto/libolm_migration.ts Outdated Show resolved Hide resolved

src/rust-crypto/libolm_migration.ts Outdated Show resolved Hide resolved

code review

f24f67b

BillCarsonFr requested a review from dbkr March 6, 2024 08:33

github-actions bot deployed to PR Documentation Preview March 6, 2024 08:34 View deployment

richvdh requested changes Mar 6, 2024

View reviewed changes

Code review

6f389f5

github-actions bot deployed to PR Documentation Preview March 6, 2024 23:02 View deployment

BillCarsonFr requested a review from richvdh March 6, 2024 23:16

richvdh reviewed Mar 8, 2024

View reviewed changes

richvdh requested changes Mar 8, 2024

View reviewed changes

Merge branch 'develop' into valere/element-r/migrate_local_trust_if_n…

027b025

…eeded

github-actions bot deployed to PR Documentation Preview March 14, 2024 10:53 View deployment

Review gh suggestion

bff57ad

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

BillCarsonFr and others added 3 commits March 14, 2024 12:16

Review gh suggestion

7e95829

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Review gh suggestion

0dd4ef6

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Review gh suggestion

230e79a

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

github-actions bot deployed to PR Documentation Preview March 14, 2024 11:19 View deployment

BillCarsonFr and others added 5 commits March 14, 2024 13:17

review move function down in file

0695d44

Merge branch 'valere/element-r/migrate_local_trust_if_needed' of http…

6dfc534

…s://github.com/matrix-org/matrix-js-sdk into valere/element-r/migrate_local_trust_if_needed

Review gh suggestion

c5fffba

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Review gh suggestion

7965819

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Review: Cleaning tests, renaming

7345204

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

Review: better comment

82a69c6

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

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

Comment paragraphs

a008231

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

retry until initial key query is successfull

e034bc7

github-actions bot deployed to PR Documentation Preview March 15, 2024 13:32 View deployment

BillCarsonFr requested a review from richvdh March 15, 2024 13:43

richvdh approved these changes Mar 15, 2024

View reviewed changes

review quick nits

9a37f04

github-actions bot deployed to PR Documentation Preview March 18, 2024 08:23 View deployment

missing mock in test

d3a5082

github-actions bot deployed to PR Documentation Preview March 18, 2024 08:56 View deployment

BillCarsonFr added this pull request to the merge queue Mar 18, 2024

Merged via the queue into develop with commit 3e98900 Mar 18, 2024
23 checks passed

BillCarsonFr deleted the valere/element-r/migrate_local_trust_if_needed branch March 18, 2024 09:21

BillCarsonFr mentioned this pull request Apr 5, 2024

ElementR| Retry query backup until it works during migration to avoid spurious correption error popup #4113

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate own identity local trust to rust crypto #4090

Migrate own identity local trust to rust crypto #4090

BillCarsonFr commented Mar 4, 2024 •

edited by richvdh

richvdh commented Mar 6, 2024

richvdh commented Mar 6, 2024

dkasak commented Mar 6, 2024

richvdh Mar 8, 2024

richvdh Mar 15, 2024

richvdh Mar 8, 2024 •

edited

richvdh left a comment

richvdh left a comment

richvdh Mar 15, 2024

richvdh Mar 15, 2024

BillCarsonFr Mar 18, 2024

		@@ -197,6 +201,14 @@ async function initOlmMachine(
		// It will be retried by the sdk.

		/** Additional dump info specific for some tests.*/
		[key: string]: any;

Migrate own identity local trust to rust crypto #4090

Migrate own identity local trust to rust crypto #4090

Conversation

BillCarsonFr commented Mar 4, 2024 • edited by richvdh

Checklist

richvdh commented Mar 6, 2024

richvdh commented Mar 6, 2024

dkasak commented Mar 6, 2024

richvdh Mar 8, 2024

Choose a reason for hiding this comment

richvdh Mar 15, 2024

Choose a reason for hiding this comment

richvdh Mar 8, 2024 • edited

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

richvdh Mar 15, 2024

Choose a reason for hiding this comment

richvdh Mar 15, 2024

Choose a reason for hiding this comment

BillCarsonFr Mar 18, 2024

Choose a reason for hiding this comment

BillCarsonFr commented Mar 4, 2024 •

edited by richvdh

richvdh Mar 8, 2024 •

edited