Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Conversation

MichaelErjemenko
Copy link

@MichaelErjemenko MichaelErjemenko commented Apr 19, 2024

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 via backupTrustInfo?.matchesDecryptionKey) but the user has no 4s. The start of the migration is introduced by invoking bootstrapSecretStorage.
  • 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 setupNewKeyBackup parameter which leads to rust-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/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

Signed-off-by: Michael Schrader [email protected]

Michael Schrader added 3 commits April 19, 2024 08:46
This aims to fix the problem, that a key backup exists but no 4s. bootstrapSecretStorage is triggered and the backup is resigned.
@MichaelErjemenko MichaelErjemenko requested a review from a team as a code owner April 19, 2024 07:33
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Apr 19, 2024
@richvdh
Copy link
Member

richvdh commented Apr 22, 2024

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 element-hq/element-web#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.

@MichaelErjemenko MichaelErjemenko changed the title Fix missing CrossSigningKeys upload with sso sign in and backup migration Fix migrating key backup to 4s fails Apr 23, 2024
@MichaelErjemenko
Copy link
Author

I added the issue
element-hq/element-web#27382 with the pr #12447
and for this pr the issue
element-hq/element-web#27383

I hope now it is more clear.

@richvdh richvdh changed the title Fix migrating key backup to 4s fails Fix error migrating key backup to 4S Apr 23, 2024
@richvdh
Copy link
Member

richvdh commented May 13, 2024

Per element-hq/element-web#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.

@richvdh richvdh closed this May 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants