Crypto: fix backed-up keys being re-backed-up #3448
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
No need, I think it's manageable. |
richvdh
force-pushed
the
rav/fix_backup_import
branch
from
b1b5a7d to
e9b02c1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3448 +/- ##
==========================================
+ Coverage 83.27% 83.29% +0.02%
==========================================
Files 247 247
Lines 25091 25111 +20
==========================================
+ Hits 20895 20917 +22
+ Misses 4196 4194 -2 ☔ View full report in Codecov by Sentry. |
poljar
approved these changes
May 23, 2024
| @@ -619,7 +619,15 @@ impl BackupMachine { | |||
| } | |||
| } | |||
|
|
|||
| self.store.import_room_keys(decrypted_room_keys, true, progress_listener).await | |||
| // This method is a bit flawed: we have no real idea which backup version these | |||
There was a problem hiding this comment.
Could you annotate this with a TODO: or FIXME: , so we can more easily find this fact.
There was a problem hiding this comment.
I can, but this is the main reason I deprecated the method. The real fix is to remove it.
| @@ -1823,7 +1823,11 @@ impl OlmMachine { | |||
| from_backup: bool, | |||
| progress_listener: impl Fn(usize, usize), | |||
| ) -> StoreResult<RoomKeyImportResult> { | |||
| self.store().import_room_keys(exported_keys, from_backup, progress_listener).await | |||
| let backup_version = | |||
There was a problem hiding this comment.
I think we can just remove this method. We had a 0.7.x release.
There was a problem hiding this comment.
Even better! Will remove it.
When we add a batch of inbound group sessions to the store, if they came from a backup, we need to record which backup version they came from. `CryptoStore::save_changes` doesn't give us an easy way to set the backup version (we could add it to the `Changes` struct, but ugh). So, we add a new method `save_inbound_group_sessions` which takes the backup version as a separate param. This works fine, because whenever we import sessions there are no other changes to store. The new method isn't used outside of tests yet -- that comes in a follow-up commit.
When we are importing a batch of room keys, use the newly-added `CryptoStore::save_inbound_group_sessions` method instead of `CryptoStore::save_changes`. To do this, we need to pass the backup version into `Store::import_room_keys` instead of just `from_backup` flag.
This whole method is a bit broken. It assumes that, when we did an import from backup, the backup that they came from is the same as the "current" version. We *could* add another argument, but to be honest I find the whole method a bit pointless. It's not particularly tied to the `BackupMachine` abstraction. Let's instead expose `Store::import_room_keys` and `ExportedRooomKey::from_backed_up_room_key` publicly, and tell callers to use that directly.
... and use its replacement, `Store::import_room_keys`
... and expose a new method `import_room_keys_from_backup` which does the right thing.
richvdh
force-pushed
the
rav/fix_backup_import
branch
from
815c4cd to
ef85db4
Compare
richvdh
changed the title
richvdh
force-pushed
the
rav/fix_backup_import
branch
from
ef85db4 to
0777aa6
Compare
richvdh
added a commit
that referenced
this pull request
May 23, 2024
… stream #3448 added a new method `save_inbound_group_sessions` to `CrytoStore`, but forgot to wire it up to the `room_keys_received` stream.
This was referenced May 23, 2024
richvdh
added a commit
to matrix-org/matrix-rust-sdk-crypto-wasm
that referenced
this pull request
May 24, 2024
Bump to a version of the rust SDK that includes matrix-org/matrix-rust-sdk#3448 and matrix-org/matrix-rust-sdk#3456, to fix matrix-org/matrix-rust-sdk#3447. Also, pass the backup version into import_backed_up_room_keys to avoid using the deprecated method on BackupMachine.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to fix #3447.
The first thing to do is to make sure that a backup version is stored for any keys that are imported from backup. As a quick fix, which doesn't break backwards compatibility, we therefore update
BackupMachine::import_backed_up_room_keysto pass through the current backup version to the store.However, that is very much a bodge: we have no guarantee that is the correct version. A better fix is to have the application pass through the version that it got the keys from. We therefore deprecate the inherently-broken
BackupMachine::import_backed_up_room_keys, and chase the changes through to the SDK and FFI layers.Should be reviewable commit-by-commit. [I'd also be open to splitting this into two PRs: one for a quick fix, and one for the deprecation stuff. LMK what you think]