e2e key backups #684

uhoreg · 2018-08-08T03:19:32Z

Follow-on work to #595

ara4n · 2018-08-09T00:52:09Z

src/client.js

+            keys.push(key);
+            return this.importRoomKeys(keys);
+        } else {
+            callback("aargh!");


we might need to do better than aargh ;)

spec/unit/crypto/backup.spec.js

uhoreg · 2018-08-23T04:28:05Z

@dbkr here is the initial draft of the API. I haven't really tested it to see if it actually works yet -- this is mainly to give a general idea of what the API might look like (and to give a somewhat plausible implementation). If you have any suggestions for improving the API (especially to make it fit in better with the rest of the SDK), let me know. As far as implementation goes, I'm guessing you'll probably want to just replace things with stubs when you're doing the UI work, until I get it all working and tested.

The general idea is that you would call client.getKeyBackupVersion to see if there is a usable backup. It'll throw an exception if there isn't. If there is, it will return an object, which you can pass to client.enableKeyBackup. Once client.enableKeyBackup is called, then any keys that are received will be backed up. Backups can be disabled by calling client.disableKeyBackup. A client can create a new backup version by calling client.createKeyBackupVersion. This will automatically enable the backup. client.createKeyBackupVersion will return a promise that will resolve to the backup recovery key, which can be presented to the user.

To restore from the key backup, you would call client.restoreKeyBackups with the backup recovery key, room ID (optional if session ID is not given), and session ID (optional). This should fetch the key(s) from the server and supposedly re-attempt to decrypt previous events.

You can also delete keys from the backup by calling client.deleteKeyBackups.

Currently missing are:

an API for backing up a key to the server (there is client.sendKeyBackup, but that's mainly intended for internal use, because it has the "data" parameter, which would be fiddly for people to attempt to build themselves)
a proper backup recovery key (need to figure out what it should look like)
probably other things that I'm forgetting right now

…nto uhoreg-e2e_backups

Continues from uhoreg's branch

Add method to check if a given string is a valid recovery key

Remove commented code block as it's not immediately obvious it makes sense or is the right way of suggesting a key restore.

wasm Olm has a new interface: it now has an init method that needs to be called and the promise it returns waited on before the Olm module is used. Support that, and allow Crypto etc to be imported whether Olm is enabled or not. Change whether olm is enabled to be async since now it will be unavailable if the async module init fails. Don't call getOlmVersion() until the Olm.init() is done.

By doing `Olm = global.Olm` on script load, we require that Olm is available right from the start, which isn't really necessary. As long as it appears some time before we actually want to use it, this is fine (we can probably assume it's not going to go away again..?) This means Riot doesn't need to faff about making sure olm is loaded before starting anything else.

base58check seems way overcomplicated for this purpose (plus the module was exporting an es6 file, breaking the js-sdk build). A parity check empirically detects single substitution and transposition errors. Another option would be Luhn's algorithm.

dbkr

Looking good. Remaining things I think:

Remove XXXs from

matrix-js-sdk/src/crypto/algorithms/megolm.js

Line 852 in 258adda

// XXX: No retries on this at all: if this request dies for whatever
Localstorage crypto store (for Firefox in private mode).
We should also trigger a backup from

matrix-js-sdk/src/crypto/algorithms/megolm.js

Line 969 in 258adda

MegolmDecryption.prototype.importRoomKey = function(session) {

dbkr · 2018-10-11T14:40:33Z

src/crypto/store/indexeddb-crypto-store-backend.js

+                            sessionData: sessionGetReq.result.session
+                        });
+                    }
+                    //sessions.push(cursor.value);


should probably remove this

dbkr · 2018-10-11T15:03:30Z

src/crypto/store/indexeddb-crypto-store-backend.js

+        const objectStore = txn.objectStore("sessions_needing_backup");
+        return Promise.all(sessions.map((session) => {
+            return new Promise((resolve, reject) => {
+                console.log(session);


stray debug logging

dbkr · 2018-10-11T15:14:54Z

src/crypto/index.js

+        }
+    );
+
+    this._maybeSendKeyBackup();


should await on this so the backupAllGroupSessions promise doesn't complete until the backup is done

dbkr · 2018-10-11T15:15:42Z

src/crypto/index.js

+        sessionId: sessionId,
+    }]);
+
+    this._maybeSendKeyBackup();


probably worth awaiting on this too? Actually possibly not: when we call it from megolm we only want to wait for it to be saved to indexeddb, not actually backed up. worth a comment though.

Actually, I think it does make sense to await here, and then remove the awaits from megolm.

dbkr · 2018-10-11T15:17:45Z

src/crypto/index.js

+                    // FIXME: pause
+                }
+            } while (!successful);
+            // FIXME: pause between iterations?


to consolidate my comments from the individual commits, we should put the pause in, or just stop & retry next time a backup is triggered which would be my inclination.

…kups

dbkr · 2018-10-16T09:38:16Z

lgtm once new Olm is released

uhoreg

Argh! Why can't I "Request changes" on my own PR. :(

uhoreg · 2018-10-16T22:22:47Z

src/crypto/index.js

+                    const encrypted = this.backupKey.encrypt(JSON.stringify(sessionData));
+
+                    data[roomId]['sessions'][session.sessionId] = {
+                        first_message_index: 1, // FIXME


Comment to make sure I don't keep forgetting to fix this

dbkr · 2018-10-19T17:04:18Z

src/crypto/DeviceList.js

+                return null;
+            }
+        }
+


This is a bit magic, but given the name of the function I can't really suggest anything better. Maybe in the future we can deprecate the 3 arg usage of this function altogether.

manuroe · 2018-10-24T07:34:51Z

src/crypto/index.js

+        },
+    );
+
+    await this._maybeSendKeyBackup();


backupAllGroupSessions is called on user action but _maybeSendKeyBackup can delay its actual execution randomly up to 10s.
In the context of this function, we would like _maybeSendKeyBackup to start ASAP.

NB. I've added a delay param in #736

manuroe · 2018-10-24T09:10:14Z

src/crypto/index.js

+                    delete sessionData.first_known_index;
+                    const encrypted = this.backupKey.encrypt(JSON.stringify(sessionData));
+
+                    const forwardedCount =


There is no forwardingCurve25519KeyChain field in the dict returned by exportInboundGroupSession but forwarding_curve25519_key_chain.

The mix of forwarding_curve25519_key_chain and forwardingCurve25519KeyChain in the js code makes me terrified.

Good spot - turns out this was completely broken

manuroe · 2018-10-26T12:52:01Z

src/crypto/algorithms/megolm.js

@@ -956,6 +973,18 @@ MegolmDecryption.prototype.importRoomKey = function(session) {
        session.sender_claimed_keys,
        true,
    ).then(() => {
+        if (this._crypto.backupInfo) {
+            // don't wait for it to complete
+            this._crypto.backupGroupSession(


If this method is called because we are restoring keys from the current backup session, it looks inefficient to send back up the key to the homeserver.

Yeah, at some point we may want to flag keys that came from the backup in the first place (and what version)

uhoreg · 2018-10-30T14:26:50Z

replaced by #736

ara4n and others added 4 commits January 15, 2018 01:50

initial pseudocode WIP for e2e online backups

fb1b554

blindly move crypto.suggestKeyRestore over to /sync

e0c9b99

Merge branch 'develop' into matthew/e2e_backups

69204d4

initial implementation of e2e key backup and restore

d556189

ara4n requested changes Aug 9, 2018

View reviewed changes

ara4n reviewed Aug 16, 2018

View reviewed changes

spec/unit/crypto/backup.spec.js Show resolved Hide resolved

uhoreg added 4 commits August 22, 2018 23:58

fix formatting and fix authedRequest usage

1faf477

initial draft of API for working with backup versions

fb8efe3

pass in key rather than decryption object to restoreKeyBackups

75107f9

check that crypto is enabled

e5ec479

uhoreg and others added 19 commits August 23, 2018 00:29

add copyright header to backup.spec

73e294b

Merge branch 'e2e_backups' of git://github.com/uhoreg/matrix-js-sdk i…

ec5fff2

…nto uhoreg-e2e_backups

fix some bugs

017f81e

split the backup version creation into two different methods

bf873bd

Merge branch 'e2e_backups' of git://github.com/uhoreg/matrix-js-sdk i…

29db856

…nto uhoreg-e2e_backups

Merge remote-tracking branch 'origin/develop' into uhoreg-e2e_backups

72bd51f

WIP e2e key backup support

3838fab

Continues from uhoreg's branch

Check sigs on e2e backup & enable it if we can

e789747

Make multi-room key restore work

073fb73

Add isValidRecoveryKey

009430e

Add method to check if a given string is a valid recovery key

Soe progress on linting

f75d188

More linting

3af9af9

Make tests pass

54c443a

Fix lint mostly

e4bb37b

Fix lint

0bad7b2

Remove commented code block as it's not immediately obvious it makes sense or is the right way of suggesting a key restore.

Bump to Olm 2.3.0 for PkEncryption

a78825e

Free PkEncryption/Decryption objects

1b62a21

Test all 3 code paths on backup restore

2f4c1df

dbkr and others added 12 commits September 26, 2018 16:39

Merge remote-tracking branch 'origin/develop' into dbkr/e2e_backups

e9b0aca

Merge branch 'dbkr/wasm' into dbkr/e2e_backups

ce2058a

Fix recovery key format

7cd101d

commit the recovery key util file

262ace1

retry key backups when they fail

258adda

Merge remote-tracking branch 'origin/develop' into dbkr/e2e_backups

89c3f6f

Merge remote-tracking branch 'origin/develop' into dbkr/e2e_backups

b3fe05e

Lint

ada4b6e

wrap backup sending in a try, and add delays

da65f43

add localstorage support for key backups

fc59bc2

dbkr reviewed Oct 11, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/dbkr/e2e_backups' into e2e_bac…

3957006

…kups

dbkr mentioned this pull request Oct 12, 2018

Support for e2e key backups #736

Merged

uhoreg added 4 commits October 12, 2018 10:38

de-lint plus some minor fixes

9b12c22

fix unit tests for backup recovery

91fb7b0

more de-linting and fixing

d49c0a1

remove accidental change to eslintrc

40d0a82

uhoreg changed the title ~~[WIP] e2e key backups~~ e2e key backups Oct 16, 2018

uhoreg commented Oct 16, 2018

View reviewed changes

properly fill out the is_verified and first_message_index fields

434ac86

dbkr approved these changes Oct 19, 2018

View reviewed changes

update backup algorithm name to agree with the proposal

322ef1f

manuroe reviewed Oct 24, 2018

View reviewed changes

manuroe reviewed Oct 26, 2018

View reviewed changes

dbkr merged commit 322ef1f into matrix-org:develop Nov 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e key backups #684

e2e key backups #684

uhoreg commented Aug 8, 2018 •

edited

Loading

ara4n Aug 9, 2018

uhoreg commented Aug 23, 2018

dbkr left a comment •

edited

Loading

dbkr Oct 11, 2018

dbkr Oct 11, 2018

dbkr Oct 11, 2018

dbkr Oct 11, 2018

uhoreg Oct 12, 2018

dbkr Oct 11, 2018

dbkr commented Oct 16, 2018

uhoreg left a comment

uhoreg Oct 16, 2018

dbkr Oct 19, 2018

manuroe Oct 24, 2018

dbkr Oct 31, 2018

manuroe Oct 24, 2018

dbkr Oct 31, 2018

manuroe Oct 26, 2018

dbkr Oct 31, 2018

uhoreg commented Oct 30, 2018 •

edited

Loading

e2e key backups #684

e2e key backups #684

Conversation

uhoreg commented Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

uhoreg commented Aug 23, 2018

dbkr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr commented Oct 16, 2018

uhoreg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhoreg commented Oct 30, 2018 • edited Loading

uhoreg commented Aug 8, 2018 •

edited

Loading

dbkr left a comment •

edited

Loading

uhoreg commented Oct 30, 2018 •

edited

Loading