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

Process received room key requests #449

Merged
merged 4 commits into from
Jun 6, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ const SyncApi = require("./sync");
const MatrixBaseApis = require("./base-apis");
const MatrixError = httpApi.MatrixError;

import reEmit from './reemit';

const SCROLLBACK_DELAY_MS = 3000;
let CRYPTO_ENABLED = false;

Expand Down Expand Up @@ -166,6 +168,10 @@ function MatrixClient(opts) {
this.store,
opts.cryptoStore,
);
reEmit(this, this._crypto, [
"crypto.roomKeyRequest",
"crypto.roomKeyRequestCancellation",
]);

this.olmVersion = Crypto.getOlmVersion();
}
Expand Down
20 changes: 20 additions & 0 deletions src/crypto/algorithms/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,26 @@ class DecryptionAlgorithm {
importRoomKey(session) {
// ignore by default
}

/**
* Determine if we have the keys necessary to respond to a room key request
*
* @param {module:crypto#RoomKeyRequest} keyRequest
* @return {boolean} true if we have the keys and could (theoretically) share
Copy link
Member

Choose a reason for hiding this comment

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

You'll probably be querying indexeddb to get the answer right? Probably want this to be a Promise then?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, not currently - the relevant info is in localstorage. Admittedly we'd probably like it to move to indexeddb, but that's a whole separate project

Copy link
Member

Choose a reason for hiding this comment

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

If it's in local storage, won't we hit size limits again? It's pretty easy to convert this to be q(bool) right? I would prefer if we did an async design up-front, as it's often a pain to turn a synchronous API into an async one retrospectively.

Copy link
Member Author

Choose a reason for hiding this comment

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

the relevant info is already in localstorage.

Copy link
Member

Choose a reason for hiding this comment

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

OK.

* them; else false.
*/
hasKeysForKeyRequest(keyRequest) {
return false;
}

/**
* Send the response to a room key request
*
* @param {module:crypto#RoomKeyRequest} keyRequest
*/
shareKeysWithDevice(keyRequest) {
throw new Error("shareKeysWithDevice not supported for this DecryptionAlgorithm");
}
}
export {DecryptionAlgorithm}; // https://github.com/jsdoc3/jsdoc/issues/1272

Expand Down
174 changes: 174 additions & 0 deletions src/crypto/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ limitations under the License.

const anotherjson = require('another-json');
const q = require("q");
import {EventEmitter} from 'events';

const utils = require("../utils");
const OlmDevice = require("./OlmDevice");
Expand Down Expand Up @@ -93,6 +94,11 @@ function Crypto(baseApis, eventEmitter, sessionStore, userId, deviceId,

this._globalBlacklistUnverifiedDevices = false;

// list of IncomingRoomKeyRequests/IncomingRoomKeyRequestCancellations
// we received in the current sync.
this._receivedRoomKeyRequests = [];
this._receivedRoomKeyRequestCancellations = [];
Copy link
Member

Choose a reason for hiding this comment

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

Storing them like this means we lose ordering information from /sync. Is this a problem?

Copy link
Member

Choose a reason for hiding this comment

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

E.g if I send a key request then immediately cancel it, currently we will process the key request due to how _processReceivedRoomKeyRequests is currently implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

well now.

AFAIK there is no guarantee in the protocol that the ordering of to-device messages is preserved anyway, so this whole thing is a steaming pile of fragility.

That said - in the case you mention, we will process the key request, and then process the cancellation. Which is considerably better than processing (and, presumably, ignoring) the cancellation and then the request.

Copy link
Member

Choose a reason for hiding this comment

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

we will process the key request, and then process the cancellation.

Doesn't that race? We should at least mention the ordering concerns and decisions / things we've punted on for now somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, of course it races:

AFAIK there is no guarantee in the protocol that the ordering of to-device messages is preserved anyway, so this whole thing is a steaming pile of fragility.

the order in which requests and cancellations order are, tbh, the least of my concerns here. I want to ship it and iterate it rather than think through the edge cases any more. This work has taken weeks already.

Copy link
Member

Choose a reason for hiding this comment

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

OK.


let myDevices = this._sessionStore.getEndToEndDevicesForUser(
this._userId,
);
Expand All @@ -118,6 +124,8 @@ function Crypto(baseApis, eventEmitter, sessionStore, userId, deviceId,

_registerEventHandlers(this, eventEmitter);
}
utils.inherits(Crypto, EventEmitter);


function _registerEventHandlers(crypto, eventEmitter) {
eventEmitter.on("sync", function(syncState, oldState, data) {
Expand Down Expand Up @@ -149,6 +157,8 @@ function _registerEventHandlers(crypto, eventEmitter) {
crypto._onRoomKeyEvent(event);
} else if (event.getType() == "m.new_device") {
crypto._onNewDeviceEvent(event);
} else if (event.getType() == "m.room_key_request") {
crypto._onRoomKeyRequestEvent(event);
}
} catch (e) {
console.error("Error handling toDeviceEvent:", e);
Expand Down Expand Up @@ -868,6 +878,7 @@ Crypto.prototype._onSyncCompleted = function(syncData) {
// (https://github.com/vector-im/riot-web/issues/2782).
if (!syncData.catchingUp) {
_maybeUploadOneTimeKeys(this);
this._processReceivedRoomKeyRequests();
}
};

Expand Down Expand Up @@ -1056,6 +1067,106 @@ Crypto.prototype._onNewDeviceEvent = function(event) {
this._deviceList.invalidateUserDeviceList(userId);
};

/**
* Called when we get an m.room_key_request event.
*
* @private
* @param {module:models/event.MatrixEvent} event key request event
*/
Crypto.prototype._onRoomKeyRequestEvent = function(event) {
const content = event.getContent();
if (content.action === "request") {
// Queue it up for now, because they tend to arrive before the room state
// events at initial sync, and we want to see if we know anything about the
// room before passing them on to the app.
const req = new IncomingRoomKeyRequest(event);
this._receivedRoomKeyRequests.push(req);
} else if (content.action === "request_cancellation") {
const req = new IncomingRoomKeyRequestCancellation(event);
this._receivedRoomKeyRequestCancellations.push(req);
}
};

/**
* Process any m.room_key_request events which were queued up during the
* current sync.
*
* @private
*/
Crypto.prototype._processReceivedRoomKeyRequests = function() {
const requests = this._receivedRoomKeyRequests;
this._receivedRoomKeyRequests = [];
for (const req of requests) {
const userId = req.userId;
const deviceId = req.deviceId;

const body = req.requestBody;
const roomId = body.room_id;
const alg = body.algorithm;

console.log(`m.room_key_request from ${userId}:${deviceId}` +
` for ${roomId} / ${body.session_id} (id ${req.requestId})`);

if (userId !== this._userId) {
// TODO: determine if we sent this device the keys already: in
// which case we can do so again.
console.log("Ignoring room key request from other user for now");
return;
}

// todo: should we queue up requests we don't yet have keys for,
// in case they turn up later?

// if we don't have a decryptor for this room/alg, we don't have
// the keys for the requested events, and can drop the requests.
if (!this._roomDecryptors[roomId]) {
console.log(`room key request for unencrypted room ${roomId}`);
continue;
}

const decryptor = this._roomDecryptors[roomId][alg];
if (!decryptor) {
console.log(`room key request for unknown alg ${alg} in room ${roomId}`);
continue;
}

if (!decryptor.hasKeysForKeyRequest(req)) {
console.log(
`room key request for unknown session ${roomId} / ` +
body.session_id,
);
continue;
}

req._shareCallback = () => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't like that Crypto is clobbering an _ variable on a different class (IncomingRoomKeyRequest) like this. Please either make a public function to do this, or don't pretend that this is private. Just because both classes are in the same 1300 line file is not a good enough reason to gut wrench.

Copy link
Member Author

Choose a reason for hiding this comment

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

fair. Hopefully fixed in 1664312.

Copy link
Member

Choose a reason for hiding this comment

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

WFM!

decryptor.shareKeysWithDevice(req);
};

// if the device is is verified already, share the keys
const device = this._deviceList.getStoredDevice(userId, deviceId);
if (device && device.isVerified()) {
console.log('device is already verified: sharing keys');
req.share();
return;
}

this.emit("crypto.roomKeyRequest", req);
}

const cancellations = this._receivedRoomKeyRequestCancellations;
this._receivedRoomKeyRequestCancellations = [];
for (const cancellation of cancellations) {
console.log(
`m.room_key_request cancellation for ${cancellation.userId}:` +
`${cancellation.deviceId} (id ${cancellation.requestId})`,
);

// we should probably only notify the app of cancellations we told it
// about, but we don't currently have a record of that, so we just pass
// everything through.
this.emit("crypto.roomKeyRequestCancellation", cancellation);
}
};

/**
* Get a decryptor for a given room and algorithm.
Expand Down Expand Up @@ -1126,5 +1237,68 @@ Crypto.prototype._signObject = function(obj) {
};


/**
* Represents a received m.room_key_request event
*
* @property {string} userId user requesting the key
* @property {string} deviceId device requesting the key
* @property {string} requestId unique id for the request
* @property {RoomKeyRequestBody} requestBody
*/
class IncomingRoomKeyRequest {
constructor(event) {
const content = event.getContent();

this.userId = event.getSender();
this.deviceId = content.requesting_device_id;
this.requestId = content.request_id;
this.requestBody = content.body || {};
this._shareCallback = null;
}

/**
* Ask the relevant crypto algorithm implementation to share the keys for
* this request
*/
share() {
if (this._shareCallback) {
this._shareCallback();
return;
}
throw new Error("don't know how to share keys for this request yet");
}
}

/**
* Represents a received m.room_key_request cancellation
*
* @property {string} userId user requesting the cancellation
* @property {string} deviceId device requesting the cancellation
* @property {string} requestId unique id for the request to be cancelled
*/
class IncomingRoomKeyRequestCancellation {
constructor(event) {
const content = event.getContent();

this.userId = event.getSender();
this.deviceId = content.requesting_device_id;
this.requestId = content.request_id;
}
}

/**
* Fires when we receive a room key request
*
* @event module:client~MatrixClient#"crypto.roomKeyRequest"
* @param {module:crypto~IncomingRoomKeyRequest} req request details
*/

/**
* Fires when we receive a room key request cancellation
*
* @event module:client~MatrixClient#"crypto.roomKeyRequestCancellation"
* @param {module:crypto~IncomingRoomKeyRequestCancellation} req
*/

/** */
module.exports = Crypto;