1152761 - Add local room storage for keys in case recovery is required

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

As part of the encryption algorithms, we should be storing room keys locally to enable recovery of the data if the user's key changes - e.g. in the case where an FxA user resets their password.

Comment 1

[Mark Banner (:standard8)]

xref https://wiki.mozilla.org/Loop/Architecture/Context#Local_Key_Storage_and_Recovery

Comment 2

[Mark Banner (:standard8)]

We should also move the Guest keys into the storage, either in this bug or a separate bug (xref bug 1142522 comment 16).

Comment 3

[Mark Banner (:standard8)]

Attachment: Add local storage for Loop's room keys in case recovery is required, and handle the recovery.

This implements the required cache. I've based it on json, as the data is simple, and that's what FxA is using for some of the keys.

The patch causes the keys to be saved, and if decryption fails, then it'll try using the saved key to do the decryption.

At the moment, it doesn't then re-save the data with the encrypted value - I'm pushing that out to bug 1152764 as I'm starting to think we'll want to use the same, or a similar mechanism.

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8596726 [details] [diff] [review]
Add local storage for Loop's room keys in case recovery is required, and handle the recovery.

Review of attachment 8596726 [details] [diff] [review]:
-----------------------------------------------------------------

The tests and API are looking good here. I have a few questions about the new RoomsCache object.

::: browser/components/loop/LoopRooms.jsm
@@ +10,5 @@
>  const {MozLoopService, LOOP_SESSION_TYPE} = Cu.import("resource:///modules/loop/MozLoopService.jsm", {});
>  XPCOMUtils.defineLazyModuleGetter(this, "Promise",
>                                    "resource://gre/modules/Promise.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Task",
>                                    "resource://gre/modules/Task.jsm");

Since we're using `Task.async` here now directly upon module initialization, can you turn this into a regular Cu.import?

@@ +112,5 @@
>      }
>    }
>  };
>  
> +const LOOP_ROOMS_CACHE_FILENAME = "loopRoomsCache.json";

Can you move this to a separate module and lazily create an instance of the cache when needed?
This module has grown quite a bit, so I think a bit of modularization would be good.

@@ +115,5 @@
>  
> +const LOOP_ROOMS_CACHE_FILENAME = "loopRoomsCache.json";
> +
> +/**
> + * RoomsCache is a cache for saving simple rooms data to the disk in case we

s/RoomsCache/RoomKeysCache/

@@ +143,5 @@
> +  this.path = OS.Path.join(options.baseDir, options.filename);
> +  this._cache = null;
> +}
> +
> +RoomsCache.prototype = {

I think the prototype only needs to contain three methods: 

 - get(sessionType, roomToken)
 - set(sessionType, roomToken, roomKey)
 - clear()

Inside the module you can have two private functions that

 1. the cache is read from disk upon `_cache` access - something like `getCache()` that always yields an object.
 2. the cache is stored to disk upon setting a value on `_cache` - something like `persistCache(json)` that stores to disk.

@@ +166,5 @@
> +  _readCache: function() {
> +    return CommonUtils.readJSON(this.path).then(result => {
> +      this._cache = result;
> +    }).catch(error => {
> +      MozLoopService.log.debug("Error reading the cache:", error);

Please only log this when the error is _not_ a FILE_NOT_FOUND error.

@@ +217,5 @@
> +   * @param {String}            roomKey      The encryption key for the room.
> +   * @return {Promise}  A promise that is resolved when the data has been stored.
> +   */
> +  storeRoomKey: Task.async(function* (sessionType, roomToken, roomKey) {
> +    if (sessionType != LOOP_SESSION_TYPE.FXA) {

if this is only going to be used for FXA types, is it needed to keep the sessionType in the cache data format?

@@ +240,5 @@
> +
> +    // Only save it if there's no key, or it is different.
> +    if (!this._cache[sessionType][roomToken].key ||
> +        this._cache[sessionType][roomToken].key != roomKey) {
> +      this._cache[sessionType][roomToken].key = roomKey;

If the only thing we save in the cache is a roomKey, wouldn't it make sense to flatten the format to `this._cache[roomToken] = roomKey`?

Comment 5

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> @@ +115,5 @@
> >  
> > +const LOOP_ROOMS_CACHE_FILENAME = "loopRoomsCache.json";
> > +
> > +/**
> > + * RoomsCache is a cache for saving simple rooms data to the disk in case we
> 
> s/RoomsCache/RoomKeysCache/

I don't mind changing this for now, but I was envisioning it might be used later for other things as well and hence I was designing it to be more generic.

> @@ +240,5 @@
> > +
> > +    // Only save it if there's no key, or it is different.
> > +    if (!this._cache[sessionType][roomToken].key ||
> > +        this._cache[sessionType][roomToken].key != roomKey) {
> > +      this._cache[sessionType][roomToken].key = roomKey;
> 
> If the only thing we save in the cache is a roomKey, wouldn't it make sense
> to flatten the format to `this._cache[roomToken] = roomKey`?

I didn't want to do that - if we ended up, for some reason, needing to save other data to this cache then it'd make it harder to upgrade nicely.

With this format, adding in new items is simply a matter of a new key/value pair - if the user/developer happens to swap version to downgrade temporarily, then the old version would still be able to read the key.

It'd also mean that we would hopefully be less likely to need migration routines for different versions.

Comment 6

[Mark Banner (:standard8)]

Attachment: Add local storage for Loop's room keys in case recovery is required, and handle the recovery. v2

Updated for comments. I've kept with the generic cache idea, as I think that's the easier thing to do looking forward, at the slight expense of some minor additional complexity.

I'll do the rename if its still wanted though, and change it back if we start using it for more things.

Comment 7

[Mark Banner (:standard8)]

Attachment: Add local storage for Loop's room keys in case recovery is required, and handle the recovery. v2a

This time I'll include all the changes...

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8599820 [details] [diff] [review]
Add local storage for Loop's room keys in case recovery is required, and handle the recovery. v2a

Review of attachment 8599820 [details] [diff] [review]:
-----------------------------------------------------------------

Cool! Kudos for the test coverage here... quite essential.

Alright, let's ship it! (please do check out the notes I left below)

::: browser/components/loop/modules/LoopRooms.jsm
@@ +306,5 @@
> +
> +    try {
> +      key = yield this.promiseDecryptRoomKey(roomData.context.wrappedKey);
> +    }
> +    catch (error) {

nit: please put all the braces on the same line.

@@ +545,5 @@
>        }
>  
> +      // Now we've got the room token, we can save the key to disk.
> +      yield this.roomsCache.setKey(this.sessionType,
> +        room.roomToken, room.roomKey);

nit: I *think* `all.roomToken` will still fit in the 80-120 char limit.

@@ +760,5 @@
> +      } else {
> +        // This might be an upgrade to encrypted rename, so store the key
> +        // just in case.
> +        yield this.roomsCache.setKey(this.sessionType,
> +          all.roomToken, all.roomKey);

nit: I *think* `all.roomToken` will still fit in the 80-120 char limit.

::: browser/components/loop/modules/LoopRoomsCache.jsm
@@ +40,5 @@
> + *   - {String} filename  The filename for the cache file.
> + */
> +function LoopRoomsCache(options) {
> +  this.baseDir = options.baseDir;
> +  this.path = OS.Path.join(options.baseDir, options.filename);

It feels more 'natural' to have the default filename defined and used in this file.

```js
const LOOP_ROOMS_CACHE_FILENAME = "loopRoomsCache.json";

...

  this.path = OS.Path.join(
    options.baseDir || OS.Constants.Path.profileDir,
    options.filename || LOOP_ROOM_CACHE_FILENAME
  );
```

How does that look? Saves an import in LoopRooms.jsm too.

@@ +51,5 @@
> +   *
> +   * @param  {Object} contents An object to be saved in json format.
> +   * @return {Promise}         A promise that is resolved once the save is complete.
> +   */
> +  _setCache: function(contents) {

You *could* declare this function in the module scope as `function setCache(content) {...}` and call it in `setKey` as: `yield setCache.call(this, cache);`.
The same for _getCache, of course.

Up to you!

@@ +75,5 @@
> +      return (this._cache = yield CommonUtils.readJSON(this.path));
> +    } catch(error) {
> +      // This is really complex due to OSFile's error handling, see bug 1160109.
> +      if ((OS.Constants.libc && error.unixErrno != OS.Constants.libc.ENOENT) ||
> +          (OS.Constants.Win && error.winLastError != OS.Constants.Win.ERROR_FILE_NOT_FOUND)) {

Heh, well, kudos for figuring this mess out!!

@@ +120,5 @@
> +   *
> +   * @param {LOOP_SESSION_TYPE} sessionType  The session type for the room.
> +   * @param {String}            roomToken    The token for the room.
> +   * @param {String}            roomKey      The encryption key for the room.
> +   * @return {Promise}  A promise that is resolved when the data has been stored.

nit: one space too many.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d94ef5bca70e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell orange.
https://hg.mozilla.org/integration/fx-team/rev/16b6d04aa9cf

https://treeherder.mozilla.org/logviewer.html#?job_id=2887819&repo=fx-team

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/89a69f417877

Comment 12

[Mark Banner (:standard8)]

I took a look at this, and came to the conclusion it was some weird interaction between the things implemented for the cache and the FxA side of things. Given that bug 1153806 was cleaning this up, I pushed both to the try server, and the results came out green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02097ee8c5ac

Hence I've re-pushed both patches together, so hopefully they'll stick.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for the same failures as before.
https://hg.mozilla.org/integration/fx-team/rev/6be8f0db2eea

Comment 14

[Mark Banner (:standard8)]

Attachment: Bug 1152764 - Loop should encrypt room context information for rooms that aren't encrypted.

Ok, I eventually figured this out. The issue was that test_roomUpdates wasn't always waiting for all notifications. Due to the new context work, I think it can take slightly longer - possibly more async - to get all the required information. As a result, the expected updates notifications were spilling over into the channel ids test and confusing things.

I've added some waits, and also fixed a couple of test issues. Successful try run here from before I removed all the debug:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60e33633cbbd

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8601727 [details] [diff] [review]
Bug 1152764 - Loop should encrypt room context information for rooms that aren't encrypted.

Oops, wrong patch.

Comment 16

[Mark Banner (:standard8)]

Attachment: Intermittent failure fixes

This time with the correct patch...

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8601728 [details] [diff] [review]
Intermittent failure fixes

Review of attachment 8601728 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it! Nice changes.

::: browser/components/loop/test/xpcshell/test_looprooms.js
@@ +494,4 @@
>    function badRoomJoin() {
>      Assert.ok(false, "Unexpected 'joined' event emitted!");
>    }
> +  LoopRooms.on("join", badRoomJoin);

heh, whoopsie :)

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/08e566362fc9
https://hg.mozilla.org/integration/fx-team/rev/5cffb3b5b2fb

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08e566362fc9
https://hg.mozilla.org/mozilla-central/rev/5cffb3b5b2fb