650208 - More considered handling of key generation

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

Filing a new bug for this, because I don't want to attach patches to a tracking bug!

Per discussion on IRC, we'd like to mitigate the possibility for clients to get into inconsistent states with keys. That means:

 * If we generate keys, call _freshStart to wipe the server
 * If we generate keys, do so by generating them, uploading, then downloading them to verify round-tripping.

The former avoids the possibility of exchanging new keys while data encrypted with old keys remains on the server. The latter avoids the possibility of a server or client bug preventing the exchange of keys, whilst this client happily encrypts and uploads new records with those new keys.

Here come the patches...

Comment 1

[Richard Newman [:rnewman]]

Attachment: Part 0: refactor timestamp logic in tests.

This avoids some code duplication, and more closely matches what the server and client do wrt rounding.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Part 1: improve printing of record ID mismatches.

Another improvement along the way.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 2: augment tests to actually track collections.

This allows tests to continue to pass when logic changes to require uploads during key generation, as well as verifying the contents of info/collections.

(In short, it makes more tests behave more like a Sync server.)

Comment 4

[Richard Newman [:rnewman]]

Attachment: Part 3: whitespace!

This patch is purely:

  :%s,\s\+$,,

Lots of trailing whitespace in a couple of files. I CANNAE STAND IT, CAP'N!

Comment 5

[Richard Newman [:rnewman]]

Attachment: Part 4: implement discussed changes.

Not marking as r? yet, because CrossWeave hasn't finished.

Comment 6

[Richard Newman [:rnewman]]

CrossWeave passes with Part 4, but I'll spend a little time tomorrow (today!) doing some manual testing and considering.

Comment 7

[Richard Newman [:rnewman]]

Working on some minor improvements to Part 4, along with a specific set of tests.

This came to mind: if the server reports that it has no keys, but we have some, should we still generate new ones?

It seems like it would be better all-round to upload the ones we have and go through the usual redownload dance -- this results in a good chance that other clients can potentially do less work because their keys don't keep changing. (We should still wipe the server, though.)

On the other hand, this ideally should never happen...

Thoughts?

Comment 8

[Richard Newman [:rnewman]]

Attachment: Part 4: implement discussed changes.

This version still always generates new keys.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Part 5: additional test for freshStart.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Part 6: fix a test

The engine disabling tests were failing, because we now strictly validate the server! Fixxored. All 98 tests pass.

Comment 11

[Philipp von Weitershausen [:philikon]]

Comment on attachment 526189 [details] [diff] [review]
Part 0: refactor timestamp logic in tests.

Nice.

>+// Use the same method that record.js does, which mirrors the server.
>+function new_timestamp()
>+  Math.round(Date.now() / 10) / 100;
>+  

Would be nice to explain this with a few more words, along the lines of: server returns timestamps with 1/100 sec granularity, also note that this may not always be the case in the future. Bonus points for filing or finding the server bug that reduces the granularity to seconds and referring to it in the comment.

Also nit: braces + return statement for a function statement.

>-      response.setHeader('X-Weave-Timestamp', ''+Date.now()/1000, false);
>+      response.setHeader('X-Weave-Timestamp', ''+new_timestamp(), false);

nit: s/''/""/ and spaces around operator
        
>-    response.setHeader('X-Weave-Timestamp', ''+Date.now()/1000, false);
>+    response.setHeader('X-Weave-Timestamp',
>+                       ''+(new_timestamp()),
>+                       false);

ditto

Comment 12

[Philipp von Weitershausen [:philikon]]

Comment on attachment 526191 [details] [diff] [review]
Part 2: augment tests to actually track collections.

>+function login_handling(handler) {
>+  return function (request, response) {
>+    // btoa('johndoe:ilovejane') == am9obmRvZTppbG92ZWphbmU=
>+    // btoa('janedoe:ilovejohn') == amFuZWRvZTppbG92ZWpvaG4=
>+    let header = request.getHeader("Authorization");
>+    if (header &&
>+        header == "Basic am9obmRvZTppbG92ZWphbmU=" ||
>+        header == "Basic amFuZWRvZTppbG92ZWpvaG4=") {
>+      handler(request, response);
>+    } else {
>+      let body = "Unauthorized";
>+      response.setStatusLine(request.httpVersion, 401, "Unauthorized");
>+      response.bodyOutputStream.write(body, body.length);
>+    }
>+  };
> }

Can we steal btoa from one of the BackstagePasses, e.g:

  btoa = Cu.import("resource://services-sync/util.js").btoa;

and get rid of those magic strings? Maybe we should even add something like this to head_http_server():

  function basic_auth_header(user, password) {
    return "Basic " + btoa(user + ":" + password);
  }

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 526192 [details] [diff] [review]
Part 3: whitespace!

<3

Comment 14

[Richard Newman [:rnewman]]

(In reply to comment #12)
 
> Can we steal btoa from one of the BackstagePasses, e.g:

Yeah, I didn't do that because I was primarily moving code around, rather than rewriting, but this is a good opportunity. Goodbye, magic strings.

Comment 15

[Richard Newman [:rnewman]]

Attachment: Part 2b: clean up basic auth mess in tests.

As promised, philikon!

Comment 16

[Philipp von Weitershausen [:philikon]]

Comment on attachment 526807 [details] [diff] [review]
Part 2b: clean up basic auth mess in tests.

byootifool

Comment 17

[Richard Newman [:rnewman]]

Part 0:
 https://hg.mozilla.org/services/services-central/rev/e7560f6fcc8f

Part 1:
 https://hg.mozilla.org/services/services-central/rev/e71e755ed1e2

Part 2:
 https://hg.mozilla.org/services/services-central/rev/053e21ba8d5a

Part 2b:
 https://hg.mozilla.org/services/services-central/rev/1ac10c3832b6

Comment 18

[Richard Newman [:rnewman]]

Attachment: Part 4: implement discussed changes. v2

Explicitly don't set or use local WBO modification times for the keys that we upload. Everything comes from the server.

Comment 19

[Philipp von Weitershausen [:philikon]]

Comment on attachment 526392 [details] [diff] [review]
Part 4: implement discussed changes.

>   /**
>    * If `collections` (an array of strings) is provided, iterate
>    * over it and generate random keys for each collection.
>+   * Create a WBO for the given data.
>    */
>-  generateNewKeys: function(collections) {
>+  newWBO: function(collections, def, lastModified, collection, id) {

s/def/defaultBundle/ for better readability.

Seems like this is a helper method, so let's make it pseudo private by prepending an underscore (_newWBO or _makeWBO).

>+    wbo.modified = lastModified;
>+    return wbo;
>+  },

wbo.modified is set by the server, not the client. See https://wiki.mozilla.org/Labs/Weave/Sync/1.1/API#Weave_Basic_Object_.28WBO.29. You can get rid of it and the lastModified parameter.

>+  /**
>+   * Create a WBO for the current keys.
>+   */
>+  asWBO: function(collection, id)
>+    this.newWBO(this._collections,
>+                this._default,
>+                this._lastModified,
>+                collection || "crypto",
>+                id         || "keys"),
>+

Why are we pretending to allow other collections and ids than crypto/keys? We have so many other places where CollectionKeyManager hard codes those values, we might as well const them at the top and do it properly. Gets rid of those parameters too (here and in newWBO)

>+  /**
>+   * If `collections` (an array of strings) is provided, iterate
>+   * over it and generate random keys for each collection.
>+   */
>+  generateNewKeys: function(collections) {
>+    let newDefaultKey, newColls;
>+    [newDefaultKey, newColls] = this.newKeys(collections);
>+
>+    let modified = (Math.round(Date.now()/10)/100);    this._default = newDefaultKey;
>+    this._collections = newColls;
>+    this._lastModified = modified;
>+  },

Ignoring the obviously missing newline, I'm worried about the _lastModified property here because we're comparing it to other stuff later. That's fine so long as that other stuff is local time, too. But then there's no reason to do the funny math rounding stuff that the server does. In fact, we should not have any of that in the client. The client should be completely oblivious to the server's time precision. Server time and client time should be considered as if there were in different Minkowski frames and cannot be compared without some sort of transformation. ;)

So I think the solution is to set _lastModified when we PUT the keys to the server. The PUT returns a timestamp, we use that to set _lastModified. We might want to promote it to a public property then.

Btw, I see another instance of that Math.round business in CollectionKeys.setContents(). Please get rid of it there, too! The 'modified' parameter should be mandatory there.

>+  /**
>+   * Generates new keys, but does not replace our local copy. Use this to
>+   * verify an upload before storing.
>+   */
>+  generateNewKeysWBO: function(collections, collection, id) {
>+    let newDefaultKey, newColls;
>+    [newDefaultKey, newColls] = this.newKeys(collections);
>+
>+    let modified = (Math.round(Date.now()/10)/100);
>+    return this.newWBO(newColls,
>+                       newDefaultKey,
>+                       modified,
>+                       collection || "crypto",
>+                       id         || "keys");

Same as above (hard code collection, id).

>   generateNewSymmetricKeys:
>   function WeaveSvc_generateNewSymmetricKeys() {
>-    this._log.info("Generating new keys....");
>-    CollectionKeys.generateNewKeys();
>-    let wbo = CollectionKeys.asWBO("crypto", "keys");
>+    this._log.info("Generating new keys WBO...");
>+    let wbo = CollectionKeys.generateNewKeysWBO();
>+    let modified = wbo.modified;

Get rid of wbo.modified here. It shouldn't exist at this point.

>+    if (CollectionKeys._lastModified != modified) {

This is the line that makes me shudder. This will pretty much always be true because CollectionKeys._lastModified will be server time and modified (= wbo.modified) will be local time (also, the aliasing of 'wbo.modified' to 'modified' makes it extra hard to understand what's going on).

Comment 20

[Richard Newman [:rnewman]]

Attachment: Part 4: implement discussed changes. v3

This should do it... running CrossWeave now. *fingers crossed*

Comment 21

[Richard Newman [:rnewman]]

CrossWeave — sorry, TPS — passes locally.

Comment 22

[Richard Newman [:rnewman]]

(In reply to comment #19)

> So I think the solution is to set _lastModified when we PUT the keys to the
> server.

We actually explicitly don't want to set in PUT, because we don't update the local keys when we upload our new WBO! We update lastModified (now a public property) in setContents, as soon as we're done validating the downloaded keys. This happens in the subsequent GET.

In short: CollectionKeys is always set from a WBO (in real code (and some tests) downloaded from the server; in other tests, bypassing the server round-trip), and the modified time comes from the same source.

Comment 23

[Philipp von Weitershausen [:philikon]]

Comment on attachment 526928 [details] [diff] [review]
Part 4: implement discussed changes. v3

Nice work!

>   generateNewSymmetricKeys:
>   function WeaveSvc_generateNewSymmetricKeys() {
...
>+    info = info.obj;
>+    if (!("crypto" in info)) {

CRYPTO_COLLECTION

>+      this._log.error("Consistency failure: info/collections excludes " + 
>+                      "crypto after successful upload.");
>+      throw new Error("Symmetric key upload failed.");
>+    }
>+
>+    // Can't check against local modified: clock drift.
>+    if (info.crypto < serverModified) {

CRYPTO_COLLECTION

r=me with that

Comment 24

[Philipp von Weitershausen [:philikon]]

Comment on attachment 526393 [details] [diff] [review]
Part 5: additional test for freshStart.

Please use run_next_test instead of do_test_pending/do_test_finished. r=me with that.

Comment 25

[Richard Newman [:rnewman]]

Part 3:
  https://hg.mozilla.org/services/services-central/rev/a8e836c166e0

Part 4:
  https://hg.mozilla.org/services/services-central/rev/c79e44ac168c

Part 5:
  https://hg.mozilla.org/services/services-central/rev/9fd1441dfc34

Part 6:
  https://hg.mozilla.org/services/services-central/rev/bc93f7962f69

Comment 26

[Richard Newman [:rnewman]]

Attachment: Part 7: follow-up.

Mini follow-up to address Comment 23.

Comment 27

[Richard Newman [:rnewman]]

Part 7:
  https://hg.mozilla.org/services/services-central/rev/b90415de0478

Comment 28

[Richard Newman [:rnewman]]

Can't think of any STR for most of this -- it would require two racing clients. There are automated tests, at least!

I seem to recall an earlier bug when I wrote STR that involved wiping keys… will look.

Comment 29

[Richard Newman [:rnewman]]

Attachment: STR example log.

Ah, I was thinking of Bug 642727. Those STR are no longer viable, so here are some new ones:

* Set up Sync on two machines. Sync both as usual.

* On one machine, open a privileged web console:

  const Cu = Components.utils;
  Cu.import("resource://services-sync/resource.js");
  var svc = Cu.import("resource://services-sync/service.js").Service;
  new Resource(svc.storageURL + "crypto").delete();

This will delete keys from the server. You can ignore the output.

* Sync. Observe that the log on that machine has a huge pile of text like the attachment.

Comment 30

[Tracy Walker [:tracy]]

verified in s-c builds of 1303795082

Comment 31

[Philipp von Weitershausen [:philikon]]

Part 0: https://hg.mozilla.org/mozilla-central/rev/e7560f6fcc8f
Part 1: https://hg.mozilla.org/mozilla-central/rev/e71e755ed1e2
Part 2: https://hg.mozilla.org/mozilla-central/rev/053e21ba8d5a
Part 2b: https://hg.mozilla.org/mozilla-central/rev/1ac10c3832b6
Part 3: https://hg.mozilla.org/mozilla-central/rev/a8e836c166e0
Part 4: https://hg.mozilla.org/mozilla-central/rev/c79e44ac168c
Part 5: https://hg.mozilla.org/mozilla-central/rev/9fd1441dfc34
Part 6: https://hg.mozilla.org/mozilla-central/rev/bc93f7962f69

Comment 32

[Philipp von Weitershausen [:philikon]]

Part 7: https://hg.mozilla.org/mozilla-central/rev/b90415de0478