Refactor device key upload

Use another-json instead of awful manual json building. Sign the device keys at
the point of upload, instead of having to keep the signed string in
memory. Only upload device keys once (they are correctly merged with the
one-time keys by synapse).

lib/base-apis.js

@@ -696,6 +696,40 @@ MatrixBaseApis.prototype.search = function(opts, callback) {
     );
 };
 
+// Crypto
+// ======
+
+/**
+ * Upload keys
+ *
+ * @param {Object} content  body of upload request
+ *
+ * @param {Object=} opts
+
+ * @param {string=} opts.device_id  explicit device_id to use for upload
+ *    (default is to use the same as that used during auth).
+ *
+ * @param {module:client.callback=} callback
+ *
+ * @return {module:client.Promise} Resolves: result object. Rejects: with
+ *     an error response ({@link module:http-api.MatrixError}).
+ */
+MatrixBaseApis.prototype.uploadKeysRequest = function(content, opts, callback) {
+    opts = opts || {};
+    var deviceId = opts.device_id;
+    var path;
+    if (deviceId) {
+        path = utils.encodeUri("/keys/upload/$deviceId", {
+            $deviceId: deviceId,
+        });
+    } else {
+        path = "/keys/upload";
+    }
+    return this._http.authedRequestWithPrefix(
+        callback, "POST", path, undefined, content, httpApi.PREFIX_UNSTABLE
+    );
+};
+
 
 // Identity Server Operations
 // ==========================

lib/client.js

@@ -24,6 +24,7 @@ var PushProcessor = require('./pushprocessor');
 var EventEmitter = require("events").EventEmitter;
 var q = require("q");
 var url = require('url');
+var anotherjson = require('another-json');
 
 var httpApi = require("./http-api");
 var MatrixEvent = require("./models/event").MatrixEvent;
@@ -126,31 +127,24 @@ function MatrixClient(opts) {
     };
 
     this._olmDevice = null;
+    this._cryptoAlgorithms = [];
 
     if (CRYPTO_ENABLED && this.sessionStore !== null && userId !== null &&
             this.deviceId !== null) {
         this._olmDevice = new OlmDevice(opts.sessionStore);
+        this._cryptoAlgorithms.push(OLM_ALGORITHM);
 
-        var json = '{"algorithms":["' + OLM_ALGORITHM + '"]';
+        // build our device keys: these will later be uploaded
-        json += ',"device_id":"' + this.deviceId + '"';
+        this._deviceKeys = {};
-        json += ',"keys":';
+        this._deviceKeys["ed25519:" + this.deviceId] =
-        json += '{"ed25519:' + this.deviceId + '":';
+            this._olmDevice.deviceEd25519Key;
-        json += JSON.stringify(this._olmDevice.deviceEd25519Key);
+        this._deviceKeys["curve25519:" + this.deviceId] =
-        json += ',"curve25519:' + this.deviceId + '":';
+            this._olmDevice.deviceCurve25519Key;
-        json += JSON.stringify(this._olmDevice.deviceCurve25519Key);
-        json += '}';
-        json += ',"user_id":' + JSON.stringify(userId);
-        json += '}';
-        var signature = this._olmDevice.sign(json);
-        this.deviceKeys = JSON.parse(json);
-        var signatures = {};
-        signatures[userId] = {};
-        signatures[userId]["ed25519:" + this.deviceId] = signature;
-        this.deviceKeys.signatures = signatures;
 
+        // add our own deviceinfo to the sessionstore
         var deviceInfo = {
-            keys: this.deviceKeys.keys,
+            keys: this._deviceKeys,
-            algorithms: this.deviceKeys.algorithms,
+            algorithms: this._cryptoAlgorithms,
             verified: DeviceVerification.VERIFIED,
         };
         var myDevices = this.sessionStore.getEndToEndDevicesForUser(
@@ -160,8 +154,10 @@ function MatrixClient(opts) {
         this.sessionStore.storeEndToEndDevicesForUser(
             userId, myDevices
         );
+
         setupCryptoEventHandler(this);
     }
+
     this.scheduler = opts.scheduler;
     if (this.scheduler) {
         var self = this;
@@ -287,6 +283,9 @@ MatrixClient.prototype.retryImmediately = function() {
     return this._syncApi.retryImmediately();
 };
 
+// Crypto bits
+// ===========
+
 /**
  * Is end-to-end crypto enabled for this client.
  * @return {boolean} True if end-to-end is enabled.
@@ -318,7 +317,7 @@ MatrixClient.prototype.getDeviceEd25519Key = function() {
  */
 MatrixClient.prototype.uploadKeys = function(maxKeys, deferred) {
     var self = this;
-    return _doKeyUpload(this).then(function(res) {
+    return _uploadDeviceKeys(this).then(function(res) {
         var keyCount = res.one_time_key_counts.curve25519 || 0;
         var maxOneTimeKeys = self._olmDevice.maxNumberOfOneTimeKeys();
         var keyLimit = Math.floor(maxOneTimeKeys / 2);
@@ -332,12 +331,42 @@ MatrixClient.prototype.uploadKeys = function(maxKeys, deferred) {
         }
 
         self._olmDevice.generateOneTimeKeys(numberToGenerate);
-        return _doKeyUpload(self);
+        return _uploadOneTimeKeys(self);
     });
 };
 
-// build the upload request, and return a promise which resolves to the response
+// returns a promise which resolves to the response
-function _doKeyUpload(client) {
+function _uploadDeviceKeys(client) {
+    if (!client._olmDevice) {
+        return q.reject(new Error("End-to-end encryption disabled"));
+    }
+
+    var userId = client.credentials.userId;
+    var deviceId = client.deviceId;
+
+    var deviceKeys = {
+        algorithms: client._cryptoAlgorithms,
+        device_id: deviceId,
+        keys: client._deviceKeys,
+        user_id: userId,
+        signatures: {},
+    };
+
+    deviceKeys.signatures[userId] = {};
+    deviceKeys.signatures[userId]["ed25519:" + deviceId] =
+        client._olmDevice.sign(anotherjson.stringify(deviceKeys));
+
+    return client.uploadKeysRequest({
+        device_keys: deviceKeys,
+    }, {
+        // for now, we set the device id explicitly, as we may not be using the
+        // same one as used in login.
+        device_id: deviceId,
+    });
+}
+
+// returns a promise which resolves to the response
+function _uploadOneTimeKeys(client) {
     if (!client._olmDevice) {
         return q.reject(new Error("End-to-end encryption disabled"));
     }
@@ -350,14 +379,13 @@ function _doKeyUpload(client) {
             oneTimeJson["curve25519:" + keyId] = oneTimeKeys.curve25519[keyId];
         }
     }
-    var content = {
+    return client.uploadKeysRequest({
-        device_keys: client.deviceKeys,
         one_time_keys: oneTimeJson
-    };
+    }, {
-    var path = "/keys/upload/" + client.deviceId;
+        // for now, we set the device id explicitly, as we may not be using the
-    return client._http.authedRequestWithPrefix(
+        // same one as used in login.
-        undefined, "POST", path, undefined, content, httpApi.PREFIX_UNSTABLE
+        device_id: client.deviceId,
-    ).then(function(res) {
+    }).then(function(res) {
         client._olmDevice.markKeysAsPublished();
         return res;
     });
@@ -474,6 +502,10 @@ function _updateStoredDeviceKeysForUser(userId, userStore, userResult) {
             deviceStore = userStore[deviceId];
 
             if (deviceStore.keys["ed25519:" + deviceId] != signKey) {
+                // this should only happen if the list has been MITMed; we are
+                // best off sticking with the original keys.
+                //
+                // Should we warn the user about it somehow?
                 console.warn("Ed25519 key for device" + userId + ": " +
                              deviceId + " has changed");
                 continue;
@@ -852,6 +884,7 @@ MatrixClient.prototype.isRoomEncrypted = function(roomId) {
     }
 };
 
+
 /**
  * Get the room for the given room ID.
  * This function will return a valid room for any room for which a Room event

package.json

@@ -24,6 +24,7 @@
   "author": "matrix.org",
   "license": "Apache-2.0",
   "dependencies": {
+    "another-json": "^0.2.0",
     "browser-request": "^0.3.3",
     "browserify": "^10.2.3",
     "q": "^1.4.1",

spec/integ/matrix-client-crypto.spec.js

@@ -48,19 +48,23 @@ var bobMessages;
  * @param {string} deviceId expected device id in upload request
  * @param {object} httpBackend
  *
- * @return {promise} completes once the http requests have completed, returning the
+ * @return {promise} completes once the http requests have completed, returning combined
- * content of the upload request.
+ * {one_time_keys: {}, device_keys: {}}
  */
 function expectKeyUpload(deviceId, httpBackend) {
     var uploadPath = "/keys/upload/" + deviceId;
+    var keys = {};
+
     httpBackend.when("POST", uploadPath).respond(200, function(path, content) {
-        expect(content.one_time_keys).toEqual({});
+        expect(content.one_time_keys).not.toBeDefined();
+        expect(content.device_keys).toBeDefined();
+        keys.device_keys = content.device_keys;
         return {one_time_key_counts: {curve25519: 0}};
     });
 
-    var uploadContent;
     httpBackend.when("POST", uploadPath).respond(200, function(path, content) {
-        uploadContent = content;
+        expect(content.device_keys).not.toBeDefined();
+        expect(content.one_time_keys).toBeDefined();
         expect(content.one_time_keys).not.toEqual({});
         var count = 0;
         for (var key in content.one_time_keys) {
@@ -69,11 +73,12 @@ function expectKeyUpload(deviceId, httpBackend) {
             }
         }
         expect(count).toEqual(5);
+        keys.one_time_keys = content.one_time_keys;
         return {one_time_key_counts: {curve25519: count}};
     });
 
     return httpBackend.flush(uploadPath, 2).then(function() {
-        return uploadContent;
+        return keys;
     });
 }
 
@@ -425,15 +430,6 @@ describe("MatrixClient crypto", function() {
         bobClient.stopClient();
     });
 
-    describe("Ali account setup", function() {
-        it("should have device keys", function(done) {
-            expect(aliClient.deviceKeys).toBeDefined();
-            expect(aliClient.deviceKeys.user_id).toEqual(aliUserId);
-            expect(aliClient.deviceKeys.device_id).toEqual(aliDeviceId);
-            done();
-        });
-    });
-
     it("Bob uploads without one-time keys and with one-time keys", function(done) {
         q()
             .then(bobUploadsKeys)