Embed device keys in Olm-encrypted messages

[uhoreg]

• Public API changes documented in changelogs (optional)

Signed-off-by:

[uhoreg]

A couple issues with the current approach:

• we don't seem to have the cross-signed device key available, so we need to re-cross-sign it every time, but that's an async operation, so it makes a bunch of things async. Is there a better way?
• the function for cross-signing is only available within the crypto crate, but I'd rather do the signing in the storage crates so that we only need to do the signing once (and avoid some complexity), but I'm not sure what are the implications of changing the function from pub(crate) to pub

[poljar]

• we don't seem to have the cross-signed device key available, so we need to re-cross-sign it every time, but that's an async operation, so it makes a bunch of things async. Is there a better way?

We do, well we do if a /keys/query has been done. It's part of the Device in the store. You'll have the same async problem though, since the Device isn't cached. I think re-cross-signing the device keys is not a good idea. We should use this signature iff it has been published to the server and it has to be the exact same signature that was published to the server, otherwise we'll just confuse ourselves. We'll have multiple valid, albeit different signatures for the same device keys.

• the function for cross-signing is only available within the crypto crate, but I'd rather do the signing in the storage crates so that we only need to do the signing once (and avoid some complexity), but I'm not sure what are the implications of changing the function from pub(crate) to pub

I think that letting stores cross-sign should not happen at all, we don't control all store implementations and this is an obvious shift of responsiblity where store implementations need to care about cryptography to get things working correctly.

[uhoreg]

I think that this is ready to be reviewed now.

[andybalaam]

This looks good, with some minor comments and questions scattered around.

My main question is: if we don't find the device keys in the store, we generate them. Shouldn't we save them to the store at that point, so we don't end up generating them multiple times and risking inconsistency as @poljar mentioned in his previous comment?

I don't feel confident to give this full approval so will ask @poljar to take a look.

[andybalaam]

Is there some test we can add to cryptostore_integration_tests for this? Then it would cover indexeddb, which I think is not covered so far in this PR.

[uhoreg]

I don't think we can add this to cryptostore_integration_tests because memorystore works differently, and always keeps the sessions in cache (the cache can't be cleared). (But yes, I should add a test for indexeddb -- once I figure out how to run the indexeddb tests locally)

[andybalaam]

Here is now I ran the tests locally here:

cd crates/matrix-sdk-indexeddb/
WASM_BINDGEN_TEST_TIMEOUT=2400 CARGO_TARGET_DIR=/tmp/andyb-rust-target wasm-pack test --firefox --headless -- --no-default-features --features e2e-encryption

[uhoreg]

I tried that, but it gave me an error saying:

Try find `webdriver.json` for configure browser's capabilities:
Not found
driver status: signal: 9 (SIGKILL)

I wrote a test anyways and it seems to pass CI, so I'll ignore it for now, and try to figure it out later if needed.

[poljar]

My main question is: if we don't find the device keys in the store, we generate them.

Have not looked at the whole PR yet, but please take a look at our constructor:

matrix-rust-sdk/crates/matrix-sdk-crypto/src/machine.rs

Lines 297 to 311 in abda959

	let device = ReadOnlyDevice::from_account(&account);
	// We just created this device from our own Olm `Account`. Since we are the
	// owners of the private keys of this device we can safely mark
	// the device as verified.
	device.set_trust_state(LocalTrust::Verified);
	let changes = Changes {
	devices: DeviceChanges { new: vec![device], ..Default::default() },
	..Default::default()
	};
	store.save_changes(changes).await?;
	store.save_pending_changes(PendingChanges { account: Some(account) }).await?;
	debug!("Created a new Olm account");

We are guaranteed to have our own device keys and device in the store. We can add a method to the store called get_own_device() which will always return a device to make this clearer. Of course, this does not mean that we're guaranteed to have the cross-signing keys attached to our device.

[uhoreg]

@andybalaam I think that I've addressed your comments. There's some CI issues that I still need to look into, but it should be reviewable otherwise.

Oh, I also need to change it to not generate a device key, since we're always guaranteed to have our own device key in the store.

[andybalaam]

Looks good from my point of view, when the not generating device key part is done. Thanks!

[codecov]

Codecov Report

Attention: Patch coverage is 84.90566% with 8 lines in your changes missing coverage. Please review.

Project coverage is 84.11%. Comparing base (ed9ab87) to head (fc1dabf).

Files	Patch %	Lines
crates/matrix-sdk-crypto/src/olm/session.rs	66.66%	4 Missing ⚠️
crates/matrix-sdk-crypto/src/olm/account.rs	75.00%	3 Missing ⚠️
crates/matrix-sdk-sqlite/src/crypto_store.rs	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3517      +/-   ##
==========================================
- Coverage   84.14%   84.11%   -0.04%     
==========================================
  Files         255      255              
  Lines       26345    26369      +24     
==========================================
+ Hits        22168    22180      +12     
- Misses       4177     4189      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[uhoreg]

@poljar I think this is ready for review from you now. Some notes

• It currently just uses the device keys from storage, and assumes that it will get updated after cross-signing is done, to include the cross-signing signatures. Is this true, or is there anything else that needs to be done? Is there a test that I should write to make sure that it gets updated?
• Can you give your opinion on Embed device keys in Olm-encrypted messages #3517 (comment) ?
• I'm currently not refreshing the sessions in memorystore with the device keys. Do I need to? Is memorystore used for any real purposes, or is it just for testing.

[poljar]

I think the general direction is good, left some questions/nits/suggestions.

[poljar]

Left some final nits, we should revert the SQLite query patch since it doesn't help.

Approving ahead of time, feel free to merge once the final nits have been resolved.

[poljar]

I think SessionUnpickleError might be marginally better, feel free to disagree.

[uhoreg]

I think I've addressed all outstanding comments. I don't have permissions to hit the merge button. Can someone do so for me?

[poljar]

Doesn't this property need a different name:

Until this MSC is accepted, the new property should be named org.matrix.msc4147.device_keys.

We can do this in a follow up, so we don't risk more merge conflicts.

[poljar]

#3618

[richvdh]

For some reason it doesn't seem to be mentioned in the PR 😠 but I believe this is implementing MSC4147.