matrix-react-sdk
matrix-react-sdk copied to clipboard
Fix error migrating key backup to 4S
The initial situation is that the user has set up key backups but not 4s (at least possible via the settings of the android-client.) The aim here is to setup 4s if the passphrase / recovery key is entered correctly and we have access to the backup. Furthermore the signature of the backup is updated to ensure that all clients accept this backup is trusted. It is assumed that an update of the backup's signature is no problem in this case because this session is the first using 4s and "obviously" trusted.
This is related to the issue element-web issue 27383 and is maybe also related to element-web issue 27100.
restoreBackup & checkForBackupMigrationAndApply: Start migrating the key backup to 4s if it is observed, that the passphrase /recovery key was entered correctly (checked viabackupTrustInfo?.matchesDecryptionKey) but the user has no 4s. The start of the migration is introduced by invokingbootstrapSecretStorage.keyCallback: Before switching to the rust-crypto.ts the bootstrapSecretStorage got the same value for the "createSecretStorageKey" parameter, but just used it if!storageExists && !keyBackupInfo. The setting in this PR didn't cause this condition to become true before v1.11.57. Now the CreateSecretStorageDialog opens the RestoreBackupDialog, but does not delegate the recoveryKey to the CreateSecretStorageDialog. This is fixed via extending keyCallback.resignBackupIfRequired: At the end the signature of the backup is updated using the cross signing keys introduced in this session. This step does NOT upload a new backup and does NOT change the version. The "PUT /room_keys/version/{version}" endpoint is used.
What alternatives were considered
- It was tried to setup 4s without touching the backup. As result the backup was still not trusted and no active backup existed with all leading consequences.
- It was tried to create a new backup by resetting the backup (using the
setupNewKeyBackupparameter which leads torust-crypto.ts resetKeyBackup). This led to a situation where a (quick sign out) caused all messages from the last backup being encrypted forever, because all old room keys where lost and not replaced by new. - It was tried to create a new backup by posting a new backup. In this case the old messages could not be decrypted again when signing out and in again.
How you know this is the right solution
I don't know it, but all tests indicate that the original problem does not appear with this. The following database entries where checked in many steps:
account_data, e2e_room_keys_versions, e2e_room_keys, e2e_cross_signing_keys, e2e_cross_signing_signatures, e2e_device_keys_json
The review of this PR should help clearing the question if these changes fix both described problems or give at least an acceptable workaround until proper fixes are done.
Checklist
- [ ] Tests written for new code (and old code if feasible).
- [ ] New or updated
public/exportedsymbols have accurate TSDoc documentation. - [ ] Linter and other CI checks pass.
- [x] Sign-off given on the changes (see CONTRIBUTING.md).
Signed-off-by: Michael Schrader [email protected]
Thanks for looking into this!
I must admit I'm struggling to follow the reproduction steps exactly, and how the proposed fixes relate to them. Am I right in understanding that the fixes to the two cases are quite independent? If so, please could you break them into separate PRs, so that we can discuss them separately? Please could you also open issues clearly describing the user experience, separately from discussion of potential solutions; I can then discuss these with the rest of the crypto team.
Generally, I'm not convinced that we ought to be implementing this sort of "migration" at all. If clients are populating the key backup without first setting up 4S, that sounds like a bug which should be fixed rather than something we should work around. (I'd argue the whole "Upgrade your encryption" flow should be removed from Element Web, rather than attempting to fix it. It's horribly untested, and as you discovered, probably doesn't work.)
Endless many POST matrix/client/v3/keys/query requests with 200 Response.
FYI, this is tracked as https://github.com/element-hq/element-web/issues/27165.
This seems to be related to: element-web PR 27100 This seems to be related to: element-web PR 27253, element-web PR 26322
Note, by the way, that these are issues, not PRs.
I added the issue https://github.com/element-hq/element-web/issues/27382 with the pr https://github.com/matrix-org/matrix-react-sdk/pull/12447 and for this pr the issue https://github.com/element-hq/element-web/issues/27383
I hope now it is more clear.
Per https://github.com/element-hq/element-web/issues/27455: we don't think that trying to fix the "Upgrade your encryption" flow is the right solution here.
In any case if we were to accept this we'd also need to see playwright tests to demonstrate that it works correctly.
Thanks for spending time on this but I don't think we can accept it.