Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle backup secret gossip #3778

Merged
merged 32 commits into from
Oct 19, 2023
Merged

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Oct 4, 2023

Fixes element-hq/element-web#26288
Second step will be to automatically restore after the secret is received (in a following PR)

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)

This change is marked as an internal change (Task), so will not be included in the changelog.

Copy link
Contributor

@florianduros florianduros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/rust-crypto/rust-crypto.ts Outdated Show resolved Hide resolved
src/rust-crypto/rust-crypto.ts Outdated Show resolved Hide resolved
src/rust-crypto/rust-crypto.ts Show resolved Hide resolved
src/rust-crypto/rust-crypto.ts Outdated Show resolved Hide resolved
src/rust-crypto/rust-crypto.ts Outdated Show resolved Hide resolved
src/rust-crypto/backup.ts Outdated Show resolved Hide resolved
* @param backupDecryptionKey - The `BackupDecryptionKey` private key to check against.
* @returns `true` if the private key can decrypt the backup, `false` otherwise.
*/
private backupInfoMatchesBackupDecryptionKey(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use this in isKeyBackupTrusted? I think we just need

const backupMatchesSavedPrivateKey = backupKeys?.decryptionKey !== undefined && backupInfoMatchesBackupDecruptionKey(info, backupKeys.decryptionKey);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not anymore because as per previous comment we move it to a standalone function

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a big deal, but I don't understand why that makes any difference.

Here's what I'm suggesting, in full:

    /**
     * Determine if a key backup can be trusted.
     *
     * @param info - key backup info dict from {@link MatrixClient#getKeyBackupVersion}.
     */
    public async isKeyBackupTrusted(info: KeyBackupInfo): Promise<BackupTrustInfo> {
        const signatureVerification: SignatureVerification = await this.olmMachine.verifyBackup(info);
        const backupKeys: RustSdkCryptoJs.BackupKeys = await this.olmMachine.getBackupKeys();
        const decryptionKey = backupKeys?.decryptionKey;
        const backupMatchesSavedPrivateKey = !!decryptionKey && backupInfoMatchesBackupDecryptionKey(info, decryptionKey);
        return {
            matchesDecryptionKey: backupMatchesSavedPrivateKey,
            trusted: signatureVerification.trusted(),
        };
    }

does that not work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know about the !! trick.
Yes it should work, looks better, updated.

src/rust-crypto/backup.ts Outdated Show resolved Hide resolved
spec/integ/crypto/verification.spec.ts Outdated Show resolved Hide resolved
spec/integ/crypto/verification.spec.ts Outdated Show resolved Hide resolved
BillCarsonFr and others added 6 commits October 17, 2023 16:52
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM apart from the two suggestions I have made

Co-authored-by: Richard van der Hoff <[email protected]>
@BillCarsonFr BillCarsonFr added this pull request to the merge queue Oct 19, 2023
Merged via the queue into develop with commit febc4c9 Oct 19, 2023
@BillCarsonFr BillCarsonFr deleted the valere/element-r/backup/gossip branch October 19, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Element-R: After interactive verification the backup decryption key is not in cache
3 participants