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

richvdh · 2017-06-01T17:28:11Z

No description provided.

kegsay · 2017-06-02T10:47:42Z

src/crypto/OutgoingRoomKeyRequestManager.js

+    }
+
+    /**
+     * Send off a room key request, if we don't already have one


if we don't already have one

Doesn't make sense to me. Do you mean: "if we don't already have one queued."? How do we determine if one request is the same as another request?

Updated the comment.

kegsay · 2017-06-02T10:48:57Z

src/crypto/OutgoingRoomKeyRequestManager.js

+            requestId: this._baseApis.makeTxnId(),
+            state: ROOM_KEY_REQUEST_STATES.UNSENT,
+        }).then((req) => {
+            if (!req.state) {


It's not obvious what this is meant to mean. Why do we only start a timer if there is no state prop?

I'm actually relying on the fact that ROOM_KEY_REQUEST_STATES.UNSENT is zero. I agree it's horribly unclear. Updated.

kegsay · 2017-06-02T10:52:23Z

src/crypto/OutgoingRoomKeyRequestManager.js

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


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?).

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.

kegsay · 2017-06-02T10:54:09Z

src/crypto/OutgoingRoomKeyRequestManager.js

+            if (this._sendOutgoingRoomKeyRequestsRunning) {
+                throw new Error("RoomKeyRequestSend already in progress!");
+            }
+            this._sendOutgoingRoomKeyRequestsRunning = true;


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.

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.

I don't feel strongly.

kegsay · 2017-06-02T10:57:01Z

src/crypto/OutgoingRoomKeyRequestManager.js

+    }
+}
+
+function stringifyRequestBody(requestBody) {


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.

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

kegsay · 2017-06-02T11:01:23Z

src/crypto/index.js

+    this._outgoingRoomKeyRequestManager.makeRoomKeyRequest(
+        requestBody, recipients,
+    ).catch((e) => {
+        // this normally means we couldn't talk to the store */


kegsay · 2017-06-02T11:02:44Z

src/crypto/OutgoingRoomKeyRequestManager.js

+     * @returns {Promise} resolves when the request has been added to the
+     *    pending list
+     */
+    makeRoomKeyRequest(requestBody, recipients) {


Naming: I dislike the name makeRoomKeyRequest because it sounds to me like it is constructing a request rather than sending a request. The docs are clear but at call-sites I find it confusing, see here for an example. Can we s/make/send/ please?

agreed. done.

kegsay · 2017-06-02T13:36:52Z

src/crypto/algorithms/megolm.js

+    this._decryptEvent(event, true);
+};
+
+MegolmDecryption.prototype._decryptEvent = function(event, requestKeysOnFail) {


I understand this is an internal method, but I'd like there to be a bit of prose by what you mean by "fail" on requestKeysOnFail. I see this exclusively means if the error message is OLM.UNKNOWN_MESSAGE_INDEX or res === null but that's as clear as mud.

kegsay · 2017-06-02T13:39:43Z

src/crypto/algorithms/megolm.js

@@ -543,6 +547,9 @@ MegolmDecryption.prototype.decryptEvent = function(event) {
    } catch (e) {
        if (e.message === 'OLM.UNKNOWN_MESSAGE_INDEX') {
            this._addEventToPendingList(event);
+            if (requestKeysOnFail) {
+                this._requestKeysForEvent(event);
+            }


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.

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.

kegsay · 2017-06-02T13:43:09Z

src/crypto/algorithms/megolm.js

@@ -657,7 +689,7 @@ MegolmDecryption.prototype._retryDecryption = function(senderKey, sessionId) {

    for (let i = 0; i < pending.length; i++) {
        try {
-            this.decryptEvent(pending[i]);
+            this.decryptEvent(pending[i], false);


Wrong method call. Should be this._decryptEvent. Also, it would be good to explain why you're passing false here, presumably because we're retrying once we've got the keys, so getting the keys again isn't going to make a difference.

done, thanks.

kegsay · 2017-06-02T13:48:01Z

src/crypto/store/base.js

+ * @property {string} requestId    unique id for this request
+ *
+ * @property {string?} cancellationRequestId
+ *    transaction id for the cancellation, if any


Terminology: The variable states its a request ID, but the comment says it's a transaction ID. Is there a difference? If not, can we stick to one term throughout please, or else I get confused.

this one is certainly a transaction id. I can change that.

The other one (requestId) is both: we use it both as a request id within the message as a unique identifier for the recipient to correlate with the cancellation when it arrives, and as the transaction_id for sending the to-device message.

It might be worth mentioning that on :18 then when describing what requestId is then, since I didn't manage to grok that from looking at the code alone.

kegsay · 2017-06-02T13:50:08Z

src/crypto/store/base.js

+ *
+ * @typedef {Object} OutgoingRoomKeyRequest
+ *
+ * @property {string} requestId    unique id for this request


The usual questions I ask whenever I see people add in IDs:

Is there a min length? Max length?

Can I use smiley poo? Spaces?

it's an internal method. I don't think this needs documenting.

This goes over the wire as request_id - so either link to the docs for the wire protocol or state the protocol here please. There's no other way for me to know what is valid and what is not otherwise.

https://docs.google.com/document/d/1m4gQkcnJkxNuBmb5NoFCIadIY-DyqqNAS3lloE73BlQ/edit#heading=h.2rxyh346wc9c

In absence of this being in the spec, please add a link to the gdoc so readers know where to go to find the wire protocol.

Added at the top of OutgoingKeyRequestManager.js

kegsay · 2017-06-02T13:53:32Z

src/crypto/index.js

+/**
+ * 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.


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.

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.

kegsay · 2017-06-02T13:57:00Z

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

+                    );
+                    deferred.resolve(existing);
+                },
+                onNotFound: () => {


API shape feels weird. Is there any reason why we don't invoke a single function and give a null parameter if it was not found?

good question. fixed.

kegsay · 2017-06-02T14:01:41Z

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

+     *
+     * @return {Promise} resolves to the a
+     *    {@link module:crypto/store/base~OutgoingRoomKeyRequest}, or null if
+     *    there are no pending requests in those states


As per your comment on https://github.com/matrix-org/matrix-js-sdk/pull/448/files#r119830241 - I would mention here that if there are multiple matching requests, an arbitrary request is chosen.

kegsay · 2017-06-02T14:02:43Z

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

+     */
+    getOutgoingRoomKeyRequestByState(wantedStates) {
+        if (wantedStates.length === 0) {
+            return null;


The JSDoc clearly states this function will always return a Promise. Please wrap this up.

richvdh · 2017-06-05T11:56:28Z

@kegsay ptal?

kegsay · 2017-06-05T12:14:39Z

I'd like for you to add a link to https://docs.google.com/document/d/1m4gQkcnJkxNuBmb5NoFCIadIY-DyqqNAS3lloE73BlQ/edit#heading=h.2rxyh346wc9c somewhere so people can see the wire protocol that this code is supposed to be implementing. I don't need to see the code afterwards.

Other than that, LGTM!

richvdh · 2017-06-05T17:12:15Z

I think this is ready to land, but blocked on the un-reversion of matrix-org/matrix-react-sdk#983, which is blocked on the landing of the guest access branch

richvdh · 2017-06-06T13:18:47Z

I'm going to land this and #449 on a feature branch, until we have a fix in place for matrix-org/matrix-react-sdk#983 and have the rest of this feature in place.

When we are missing the keys to decrypt an event, send out a request for those keys to our other devices and to the original sender.

implement matrix-org/matrix-js-sdk#448

richvdh assigned kegsay Jun 1, 2017

kegsay reviewed Jun 2, 2017

View reviewed changes

kegsay assigned richvdh and unassigned kegsay Jun 5, 2017

richvdh changed the base branch from develop to room_key_sharing June 6, 2017 13:20

richvdh added 2 commits June 6, 2017 14:24

Storage layer for management of outgoing room key requests

6d88c76

Send a room key request on decryption failure

ea2a041

When we are missing the keys to decrypt an event, send out a request for those keys to our other devices and to the original sender.

richvdh force-pushed the rav/send_room_key_requests branch from c48fbaa to ea2a041 Compare June 6, 2017 13:25

richvdh merged commit ff323d0 into room_key_sharing Jun 6, 2017

richvdh deleted the rav/send_room_key_requests branch June 6, 2017 13:25

manuroe mentioned this pull request Oct 25, 2017

E2E Key sharing: Add keyshare request dialogs on mobile application element-hq/riot-meta#113

Closed

ylecollen pushed a commit to matrix-org/matrix-android-sdk that referenced this pull request Oct 26, 2017

add_room_key_request

27b1eaf

implement matrix-org/matrix-js-sdk#448

manuroe mentioned this pull request Nov 2, 2017

Send m.room_key_request events when we fail to decrypt an event matrix-org/matrix-ios-sdk#401

Merged

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

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

Conversation

richvdh commented Jun 1, 2017

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

kegsay Jun 2, 2017 • 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

kegsay Jun 2, 2017 • 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

kegsay Jun 2, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh Jun 5, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Jun 5, 2017

kegsay commented Jun 5, 2017

richvdh commented Jun 5, 2017

richvdh commented Jun 6, 2017

kegsay Jun 2, 2017 •

edited

Loading

kegsay Jun 2, 2017 •

edited

Loading

kegsay Jun 2, 2017 •

edited

Loading

richvdh Jun 5, 2017 •

edited

Loading