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

Send m.room_key_request events when we fail to decrypt an event #448

Merged
merged 2 commits into from
Jun 6, 2017
Merged
Show file tree
Hide file tree
Changes from all 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
217 changes: 217 additions & 0 deletions src/crypto/OutgoingRoomKeyRequestManager.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,217 @@
/*
Copyright 2017 Vector Creations Ltd

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import q from 'q';

import utils from '../utils';

/**
* Internal module. Management of outgoing room key requests.
*
* See https://docs.google.com/document/d/1m4gQkcnJkxNuBmb5NoFCIadIY-DyqqNAS3lloE73BlQ
* for draft documentation on what we're supposed to be implementing here.
*
* @module
*/

// delay between deciding we want some keys, and sending out the request, to
// allow for (a) it turning up anyway, (b) grouping requests together
const SEND_KEY_REQUESTS_DELAY_MS = 500;

/** possible states for a room key request
*
* @enum {number}
*/
const ROOM_KEY_REQUEST_STATES = {
/** request not yet sent */
UNSENT: 0,

/** request sent, awaiting reply */
SENT: 1,
};

export default class OutgoingRoomKeyRequestManager {
constructor(baseApis, deviceId, cryptoStore) {
this._baseApis = baseApis;
this._deviceId = deviceId;
this._cryptoStore = cryptoStore;

// handle for the delayed call to _sendOutgoingRoomKeyRequests. Non-null
// if the callback has been set, or if it is still running.
this._sendOutgoingRoomKeyRequestsTimer = null;

// sanity check to ensure that we don't end up with two concurrent runs
// of _sendOutgoingRoomKeyRequests
this._sendOutgoingRoomKeyRequestsRunning = false;

this._clientRunning = false;
}

/**
* Called when the client is started. Sets background processes running.
*/
start() {
this._clientRunning = true;

// set the timer going, to handle any requests which didn't get sent
// on the previous run of the client.
this._startTimer();
}

/**
* Called when the client is stopped. Stops any running background processes.
*/
stop() {
// stop the timer on the next run
this._clientRunning = false;
}

/**
* Send off a room key request, if we haven't already done so.
*
* The `requestBody` is compared (with a deep-equality check) against
* previous queued or sent requests and if it matches, no change is made.
* Otherwise, a request is added to the pending list, and a job is started
* in the background to send it.
*
* @param {module:crypto~RoomKeyRequestBody} requestBody
* @param {Array<{userId: string, deviceId: string}>} recipients
*
* @returns {Promise} resolves when the request has been added to the
* pending list (or we have established that a similar request already
* exists)
*/
sendRoomKeyRequest(requestBody, recipients) {
return this._cryptoStore.getOrAddOutgoingRoomKeyRequest({
requestBody: requestBody,
recipients: recipients,
requestId: this._baseApis.makeTxnId(),
state: ROOM_KEY_REQUEST_STATES.UNSENT,
}).then((req) => {
if (req.state === ROOM_KEY_REQUEST_STATES.UNSENT) {
this._startTimer();
}
});
}

// start the background timer to send queued requests, if the timer isn't
// already running
_startTimer() {
if (this._sendOutgoingRoomKeyRequestsTimer) {
return;
}

const startSendingOutgoingRoomKeyRequests = () => {
if (this._sendOutgoingRoomKeyRequestsRunning) {
throw new Error("RoomKeyRequestSend already in progress!");
}
this._sendOutgoingRoomKeyRequestsRunning = true;
Copy link
Member

Choose a reason for hiding this comment

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

Is this state variable strictly required? Can the presence of _sendOutgoingRoomKeyRequestsTimer be enough of an indicator? It's preferable to reduce the number of states your state machine can be in, so if we can get by without this and still have it be clear I would prefer it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not required - it is strictly a sanity check, as per https://github.com/matrix-org/matrix-js-sdk/pull/448/files#diff-d8c620291b1726321a300d1e99bb1256R53.

I see your point that it is adding complexity. I still think it is a net improvement, but will remove it if you feel strongly.

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 feel strongly.


this._sendOutgoingRoomKeyRequests().finally(() => {
this._sendOutgoingRoomKeyRequestsRunning = false;
}).done();
};

this._sendOutgoingRoomKeyRequestsTimer = global.setTimeout(
startSendingOutgoingRoomKeyRequests,
SEND_KEY_REQUESTS_DELAY_MS,
);
}

// look for and send any queued requests. Runs itself recursively until
// there are no more requests, or there is an error (in which case, the
// timer will be restarted before the promise resolves).
_sendOutgoingRoomKeyRequests() {
if (!this._clientRunning) {
this._sendOutgoingRoomKeyRequestsTimer = null;
return q();
}

console.log("Looking for queued outgoing room key requests");

return this._cryptoStore.getOutgoingRoomKeyRequestByState([
ROOM_KEY_REQUEST_STATES.UNSENT,
]).then((req) => {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like a weird design decision to have this return a single request rather than an array of requests. I'm guessing that there is some batching going on or something? Why return only 1 request if N match? Is there significance to which request is selected (FIFO/LIFO/arbitrary?).

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like a weird design decision to have this return a single request rather than an array of requests. I'm guessing that there is some batching going on or something? Why return only 1 request if N match?

well, I have to loop anyway, even if I pull N out of the database at once, because more might turn up while I'm sending those N. So I might as well just do one at a time and save some complexity.

Is there significance to which request is selected (FIFO/LIFO/arbitrary?).

well, it's done by order of pkey - ie, request_id - so is more-or-less FIFO. But basically, I'm assuming that arbitrary order is good enough.

if (!req) {
console.log("No more outgoing room key requests");
this._sendOutgoingRoomKeyRequestsTimer = null;
return;
}

return this._sendOutgoingRoomKeyRequest(req).then(() => {
// go around the loop again
return this._sendOutgoingRoomKeyRequests();
}).catch((e) => {
console.error("Error sending room key request; will retry later.", e);
this._sendOutgoingRoomKeyRequestsTimer = null;
this._startTimer();
}).done();
});
}

// given a RoomKeyRequest, send it and update the request record
_sendOutgoingRoomKeyRequest(req) {
console.log(
`Requesting keys for ${stringifyRequestBody(req.requestBody)}` +
` from ${stringifyRecipientList(req.recipients)}` +
`(id ${req.requestId})`,
);

const requestMessage = {
action: "request",
requesting_device_id: this._deviceId,
request_id: req.requestId,
body: req.requestBody,
};

return this._sendMessageToDevices(
requestMessage, req.recipients, req.requestId,
).then(() => {
return this._cryptoStore.updateOutgoingRoomKeyRequest(
req.requestId, ROOM_KEY_REQUEST_STATES.UNSENT,
{ state: ROOM_KEY_REQUEST_STATES.SENT },
);
});
}

// send a RoomKeyRequest to a list of recipients
_sendMessageToDevices(message, recipients, txnId) {
const contentMap = {};
for (const recip of recipients) {
if (!contentMap[recip.userId]) {
contentMap[recip.userId] = {};
}
contentMap[recip.userId][recip.deviceId] = message;
}

return this._baseApis.sendToDevice(
'm.room_key_request', contentMap, txnId,
);
}
}

function stringifyRequestBody(requestBody) {
Copy link
Member

@kegsay kegsay Jun 2, 2017

Choose a reason for hiding this comment

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

If requestBody was an actual type you could leverage the built-in toString method so you don't need to explicitly stringify here. Might be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, but sometimes requestBodies come out of the db (as sub-objects of requests), so making it into an actual class is non-trivial.

Copy link
Member

Choose a reason for hiding this comment

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

OK.

// we assume that the request is for megolm keys, which are identified by
// room id and session id
return requestBody.room_id + " / " + requestBody.session_id;
}

function stringifyRecipientList(recipients) {
return '['
+ utils.map(recipients, (r) => `${r.userId}:${r.deviceId}`).join(",")
+ ']';
}

39 changes: 38 additions & 1 deletion src/crypto/algorithms/megolm.js
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,14 @@ utils.inherits(MegolmDecryption, base.DecryptionAlgorithm);
* problem decrypting the event
*/
MegolmDecryption.prototype.decryptEvent = function(event) {
this._decryptEvent(event, true);
};


// helper for the real decryptEvent and for _retryDecryption. If
// requestKeysOnFail is true, we'll send an m.room_key_request when we fail
// to decrypt the event due to missing megolm keys.
MegolmDecryption.prototype._decryptEvent = function(event, requestKeysOnFail) {
const content = event.getWireContent();

if (!content.sender_key || !content.session_id ||
Expand All @@ -543,6 +551,9 @@ MegolmDecryption.prototype.decryptEvent = function(event) {
} catch (e) {
if (e.message === 'OLM.UNKNOWN_MESSAGE_INDEX') {
this._addEventToPendingList(event);
if (requestKeysOnFail) {
this._requestKeysForEvent(event);
}
Copy link
Member

Choose a reason for hiding this comment

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

We don't recover at this point, we just throw as if requestKeysOnFail == false so from the caller's perspective they have no idea whether they will ever be able to decrypt this message. This might be fine? I don't know how people are using MegolmDecryption.prototype.decryptEvent currently.

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, it's fine for now. Until the key turns up it is just an undecryptable event.

decryptEvent is called here which in turn called here.

I was toying with setting a flag on the event to show that its key had been requested or something, but I didn't need it yet.

Copy link
Member

Choose a reason for hiding this comment

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

OK.

}
throw new base.DecryptionError(
e.toString(), {
Expand All @@ -554,6 +565,9 @@ MegolmDecryption.prototype.decryptEvent = function(event) {
if (res === null) {
// We've got a message for a session we don't have.
this._addEventToPendingList(event);
if (requestKeysOnFail) {
this._requestKeysForEvent(event);
}
throw new base.DecryptionError(
"The sender's device has not sent us the keys for this message.",
{
Expand All @@ -576,6 +590,28 @@ MegolmDecryption.prototype.decryptEvent = function(event) {
event.setClearData(payload, res.keysProved, res.keysClaimed);
};

MegolmDecryption.prototype._requestKeysForEvent = function(event) {
const sender = event.getSender();
const wireContent = event.getWireContent();

// send the request to all of our own devices, and the
// original sending device if it wasn't us.
const recipients = [{
userId: this._userId, deviceId: '*',
}];
if (sender != this._userId) {
recipients.push({
userId: sender, deviceId: wireContent.device_id,
});
}

this._crypto.requestRoomKey({
room_id: event.getRoomId(),
algorithm: wireContent.algorithm,
sender_key: wireContent.sender_key,
session_id: wireContent.session_id,
}, recipients);
};

/**
* Add an event to the list of those we couldn't decrypt the first time we
Expand Down Expand Up @@ -657,7 +693,8 @@ MegolmDecryption.prototype._retryDecryption = function(senderKey, sessionId) {

for (let i = 0; i < pending.length; i++) {
try {
this.decryptEvent(pending[i]);
// no point sending another m.room_key_request here.
this._decryptEvent(pending[i], false);
console.log("successful re-decryption of", pending[i]);
} catch (e) {
console.log("Still can't decrypt", pending[i], e.stack || e);
Expand Down
33 changes: 33 additions & 0 deletions src/crypto/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ const DeviceInfo = require("./deviceinfo");
const DeviceVerification = DeviceInfo.DeviceVerification;
const DeviceList = require('./DeviceList').default;

import OutgoingRoomKeyRequestManager from './OutgoingRoomKeyRequestManager';

/**
* Cryptography bits
*
Expand Down Expand Up @@ -93,6 +95,10 @@ function Crypto(baseApis, eventEmitter, sessionStore, userId, deviceId,

this._globalBlacklistUnverifiedDevices = false;

this._outgoingRoomKeyRequestManager = new OutgoingRoomKeyRequestManager(
baseApis, this._deviceId, this._cryptoStore,
);

let myDevices = this._sessionStore.getEndToEndDevicesForUser(
this._userId,
);
Expand Down Expand Up @@ -124,8 +130,10 @@ function _registerEventHandlers(crypto, eventEmitter) {
try {
if (syncState === "STOPPED") {
crypto._clientRunning = false;
crypto._outgoingRoomKeyRequestManager.stop();
} else if (syncState === "PREPARED") {
crypto._clientRunning = true;
crypto._outgoingRoomKeyRequestManager.start();
}
if (syncState === "SYNCING") {
crypto._onSyncCompleted(data);
Expand Down Expand Up @@ -787,6 +795,23 @@ Crypto.prototype.userDeviceListChanged = function(userId) {
// processing the sync.
};

/**
* Send a request for some room keys, if we have not already done so
*
* @param {module:crypto~RoomKeyRequestBody} requestBody
* @param {Array<{userId: string, deviceId: string}>} recipients
*/
Crypto.prototype.requestRoomKey = function(requestBody, recipients) {
this._outgoingRoomKeyRequestManager.sendRoomKeyRequest(
requestBody, recipients,
).catch((e) => {
// this normally means we couldn't talk to the store
console.error(
'Error requesting key for event', e,
);
}).done();
};

/**
* handle an m.room.encryption event
*
Expand Down Expand Up @@ -1126,5 +1151,13 @@ Crypto.prototype._signObject = function(obj) {
};


/**
* The parameters of a room key request. The details of the request may
* vary with the crypto algorithm, but the management and storage layers for
* outgoing requests expect it to have 'room_id' and 'session_id' properties.
Copy link
Member

Choose a reason for hiding this comment

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

the management and storage layers for outgoing requests expect it to have 'room_id' and 'session_id' properties.

Is this just a long way of saying:

@typedef {Object} RoomKeyRequestBody
@prop {string} room_id
@prop {string} session_id

Other keys are determined by the precise crypto algorithm used.

right? There may be value in @proping up the mandatory fields.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, but I think it gives more emphasis to the presence of those fields than is entirely justified. The management layer only uses them for debug, and the storage layer just uses them as keys in the indexeddb for efficiency. Both should be perfectly happy with them being absent.

Copy link
Member

Choose a reason for hiding this comment

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

Okay then.

*
* @typedef {Object} RoomKeyRequestBody
*/

/** */
module.exports = Crypto;
23 changes: 23 additions & 0 deletions src/crypto/store/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,26 @@
*
* @interface CryptoStore
*/

/**
* Represents an outgoing room key request
*
* @typedef {Object} OutgoingRoomKeyRequest
*
* @property {string} requestId unique id for this request. Used for both
* an id within the request for later pairing with a cancellation, and for
* the transaction id when sending the to_device messages to our local
* server.
*
* @property {string?} cancellationTxnId
* transaction id for the cancellation, if any
*
* @property {Array<{userId: string, deviceId: string}>} recipients
* list of recipients for the request
*
* @property {module:crypto~RoomKeyRequestBody} requestBody
* parameters for the request.
*
* @property {Number} state current state of this request (states are defined
* in {@link module:crypto/OutgoingRoomKeyRequestManager~ROOM_KEY_REQUEST_STATES})
*/
Loading