Dragana Damjanovic [:dragana] Assignee
	Description • 9 years ago

Implementing:

https://tools.ietf.org/html/draft-thomson-webpush-encryption-00

	Dragana Damjanovic [:dragana] Assignee
	Comment 1 • 9 years ago

User agent will create keys for each subscription.
We need a way to give an application the keyId and created public key.
The easiest way would be to extend web push api.

	Martin Thomson [:mt:]
	Comment 2 • 9 years ago

See also https://github.com/w3c/push-api/pull/130

	Dragana Damjanovic [:dragana] Assignee
	Comment 3 • 9 years ago

I use some part of you client implementation:
https://github.com/martinthomson/webpush-client

I hope it is ok.

	Martin Thomson [:mt:]
	Comment 4 • 9 years ago

Comment on attachment 8621535 [details] [diff] [review]
bug_1172502_v1.patch

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

I created that implementation so that you could use it :)

I have no problem using jwk for testing and the interface, but you should ultimately be using CryptoKey objects for most of the operations here.

::: dom/push/Push.js
@@ +35,2 @@
>    this._scope = scope;
>    this._pageURL = pageURL;

This should instead call `this.__init(...)`

@@ +63,3 @@
>      this._pushEndpoint = endpoint;
> +    this._pushKeyId = keyId;
> +    this._pushPublucKey = publicKey;

Fix typo.

::: dom/push/PushServiceHttp2.jsm
@@ +153,5 @@
> +      if (!params) {
> +        return;
> +      }
> +
> +      var size = this._message.reduce((total, a) => total + a.length, 0);

s/length/byteLength/  It's something I discovered late in the process: then you can append the ArrayBuffer to the message.

He is a slightly more general structure:

    var size = this._message.reduce((total, a) => total + a.byteLength, 0);
    var index = 0;
    var msg = arrays.reduce((result, a) => {
      result.set(new Uint8Array(a), index);
      index += a.byteLength;
      return result;
    }, new Uint8Array(size));

@@ +179,5 @@
> +    var encryptKeyField = aRequest.getRequestHeader("Encryption-Key");
> +    var encryptField = aRequest.getRequestHeader("Encryption");
> +
> +    var params = encryptKeyField.split(';');
> +    params.forEach(param => {

This is going to take the last entry, right?

That's not going to work very well if someone screws with the header fields.

I would build a map of keys from `Encryption-Key`.

Then I would index that map based on the keyid in the *first* `Encryption` header field value to get the actual value.

@@ +549,5 @@
> +      .then(exportedKeys => {
> +        // We take scope as keyId.
> +        result.pushKeyId = result.scope;
> +        result.pushPublicKey = exportedKeys.publicKey;
> +        result.pushPrivateKey = exportedKeys.keys;

Gah, splinter sucks.  I need a little more context here.  It looks fine, but I'm not sure.

@@ +864,5 @@
> +          self._mainPushService.receivedPushMessage(aPushRecord, msg),
> +          self._ackMsgRecv(aAckUri)
> +        ]), err => {
> +          // On error discharge message.
> +          debug("Error decoding received message." + err);

Note: we have agreed with Google that zero length messages will be passed on, with the message being null (not the empty string, which we reserve for authenticated messages).

::: dom/push/PushServiceHttp2Crypto.jsm
@@ +29,5 @@
> +  return result;
> +}
> +
> +/* I can't believe that this is needed here, in this day and age ... */
> +var base64url = {

I wouldn't use this.  Try just calling atoi and doing string substitution on "+/" to/from "-_".  This base64 implementation is super-slow and crappy.

@@ +71,5 @@
> +hmac.prototype.hash = function(input) {
> +  return this.keyPromise.then(k => crypto.subtle.sign('HMAC', k, input));
> +};
> +
> +function hkdf(salt, ikm, info, len) {

I have an updated hkdf implementation here:
https://github.com/martinthomson/secure-chat/blob/master/hkdf.js

Also, you have two places where you do the array concatenation now.  You should export that function from this JSM and reuse it for building the message up (OR you can take an array of ArrayBufferView as input and do the concatenation here.

@@ +99,5 @@
> +}
> +
> +/* generate a 96-bit IV for use in GCM, 48-bits of which are populated */
> +function generateIV(index) {
> +  var iv = new Uint8Array(12);

Please throw if index >= Math.pow(2, 48)

@@ +106,5 @@
> +  }
> +  return iv;
> +}
> +
> +function concatArrayViews(arrays) {

see previous comment about code duplication

@@ +130,5 @@
> +// DER encoding describing an ECDH public key on P-256
> +var spkiPrefix = Uint8Array.from([
> +  48, 86, 48, 16, 6, 4, 43, 129, 4, 112, 6, 8,
> +  42, 134, 72, 206, 61, 3, 1, 7, 3, 66, 0
> +]);

This is a horrible kludge, I only did this because bug 1050175 hadn't landed when I wrote the original code.  Just change the export and import functions to use "raw".

@@ +142,5 @@
> +      .then(cryptoKey =>
> +         Promise.all([
> +           crypto.subtle.exportKey("spki", cryptoKey.publicKey)
> +             .then(spki => new Uint8Array(spki, spkiPrefix.length)),
> +           crypto.subtle.exportKey("jwk", cryptoKey.privateKey)

Very clever.  This will be unnecessary when bug 1048931 lands.  Please add a TODO here and reference that bug number.

@@ +145,5 @@
> +             .then(spki => new Uint8Array(spki, spkiPrefix.length)),
> +           crypto.subtle.exportKey("jwk", cryptoKey.privateKey)
> +         ]))
> +      .then(exportedKeys => {
> +        return { publicKey: base64url.encode(exportedKeys[0]),

Style nit: multi-line object initializers should look like:

return {
  a: b,
  c: d
};

@@ +173,5 @@
> +    .then(gcmKey =>
> +      Promise.all( chunkArray(aData, 4096).map((slice, index) =>
> +        crypto.subtle.decrypt({ name: 'AES-GCM', iv: generateIV(index)},
> +                              gcmKey, slice))))
> +    .then(r => concatArrayViews(r.map(a => new Uint8Array(a))))

If you take my advice regarding the implementation of concatArrayViews, then you don't need to use map here.

::: testing/xpcshell/moz-http2/moz-http2.js
@@ +101,5 @@
>    arg.onTimeout();
>  }
>  
> +// Convert a hex string to an ArrayBufferView
> +function hex2abv(hex) {

Rather than use an ABV, you could use Buffer and `new Buffer('12346798aadaf', 'hex')`

https://nodejs.org/docs/latest/api/buffer.html#buffer_new_buffer_str_encoding

	Martin Thomson [:mt:]
	Comment 5 • 9 years ago

Oh, I forgot to check this against the proposed API changes: https://github.com/w3c/push-api/pull/130

The API uses a different attribute name (p256dh), and doesn't have a key identifier.  You don't need to worry about key identifiers in this context.  The sender can pick one.  Just use the key identifier to match the values in the Encryption and Encryption-Key header fields.

	Dragana Damjanovic [:dragana] Assignee
	Comment 6 • 9 years ago

I have left jwk for now.

	Martin Thomson [:mt:]
	Comment 7 • 9 years ago

Comment on attachment 8623533 [details] [diff] [review]
bug_1172502_v2.patch

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

Check the WebIDL.   Also note the comments about key identifiers.  I've recommended some new header field processing code.  I just typed it into splinter, so I'll leave the testing to you :/

::: dom/push/Push.js
@@ +71,5 @@
> +    return this._pushKeyId;
> +  },
> +
> +  get p256dh() {
> +    return this._pushPublicKey;

This needs to be an ArrayBuffer.

::: dom/push/PushServiceHttp2.jsm
@@ +170,5 @@
> +  var keyId = {};
> +  var lastKeyId;
> +  var keyIdEncriptField, salt;
> +  try {
> +    var encryptKeyField = aRequest.getRequestHeader("Encryption-Key");

I think that we need two functions: one to parse Encryption-Key and produce a map of keyid -> public keys.

For the keys, I would do this:

  var toMap = (m, v) => {
    var i = v.indexOf('=');
    if (i >= 0) {
      // note that you need to strip quoted strings here too.
      // I didn't because I'm lazy.
      m[v.substring(0, i).trim()] = v.substring(i + 1).trim();
    }
    return m;
  };

  var params = encryptKeyField.split(',');
  var keymap = params.reduce((m, p) => {
    var pmap = p.split(';').reduce(toMap, {});
    if (pmap.keyid && pmap.dh) {
      m[pmap.keyid] = pmap.dh;
    }
    return m;
  }, {});

Then a separate function can do this:

  var enc = aRequest.getRequestHeader("Encryption")
              .split(',', 0)[0]
              .split(';')
              .map(toMap, {});
  var dh = keymap[enc.keyid];
  var salt = enc.salt;

Don't worry about passing out the keyid.  It doesn't need checking.

@@ +555,5 @@
> +    .then(result =>
> +      PushServiceHttp2Crypto.generateKeys()
> +      .then(exportedKeys => {
> +        // We take the scope as a keyId.
> +        result.pushKeyId = result.scope;

We don't need a keyid.

@@ +861,5 @@
>      debug("pushChannelOnStop() ");
>  
>      let sendNotification = function(aAckUri, aPushRecord, self) {
> +
> +      if (keyId !== aPushRecord.pushKeyId) {

Delete this.

::: dom/push/PushServiceHttp2Crypto.jsm
@@ +56,5 @@
> +  }
> +  return array;
> +}
> +
> +function base64UrlEncode(bytes, pad=false) {

You don't need the default argument (undefined is falsy).

@@ +57,5 @@
> +  return array;
> +}
> +
> +function base64UrlEncode(bytes, pad=false) {
> +  let s = btoa(String.fromCharCode.apply(null,bytes)).replace(/\+/g, "-")

nit: space

@@ +104,5 @@
> +          throw new Error('Too many hmac invocations for hkdf');
> +        }
> +        return prkh.hash(concatArray([t, info, counter]))
> +          .then(tnext => {
> +            tnext = new Uint8Array(tnext);

You shouldn't need to do this wrapping now.  Just change the line below to use .byteLength instead of .length.

@@ +130,5 @@
> +  }
> +  return iv;
> +}
> +
> +function ua2hex(b) {

Unused.

::: dom/webidl/PushSubscription.webidl
@@ +12,4 @@
>  interface PushSubscription
>  {
>      readonly attribute USVString endpoint;
> +    readonly attribute USVString keyid;

This doesn't appear in the specification.  The sender is allowed to choose one.

@@ +12,5 @@
>  interface PushSubscription
>  {
>      readonly attribute USVString endpoint;
> +    readonly attribute USVString keyid;
> +    readonly attribute USVString p256dh;

ArrayBuffer

	Martin Thomson [:mt:]
	Comment 8 • 9 years ago

Can you work around bug 1176236 by passing a (base64) string?

	Dragana Damjanovic [:dragana] Assignee
	Comment 9 • 9 years ago

no, there is another problem, see https://bugzilla.mozilla.org/show_bug.cgi?id=1176236#c2

	Dragana Damjanovic [:dragana] Assignee
	Comment 10 • 9 years ago

webidl uses DOMString until the problem with ArrayBuffer is fixed.

	Martin Thomson [:mt:]
	Comment 11 • 9 years ago

Comment on attachment 8625259 [details] [diff] [review]
bug_1172502_v2.patch

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

::: dom/webidl/PushSubscription.webidl
@@ +12,4 @@
>  interface PushSubscription
>  {
>      readonly attribute USVString endpoint;
> +    readonly attribute USVString p256dh;

You can have the constructor take a string and still return an ArrayBuffer here.

Why does this have a constructor anyway? This should have a ChromeConstructor at most.  No content code is going to be able to construct a useful one of these.

	Dragana Damjanovic [:dragana] Assignee
	Comment 12 • 9 years ago

(In reply to Martin Thomson [:mt:] from comment #11)
> Comment on attachment 8625259 [details] [diff] [review]
> bug_1172502_v2.patch
> 
> Review of attachment 8625259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/PushSubscription.webidl
> @@ +12,4 @@
> >  interface PushSubscription
> >  {
> >      readonly attribute USVString endpoint;
> > +    readonly attribute USVString p256dh;
> 
> You can have the constructor take a string and still return an ArrayBuffer
> here.

I know, and i just had made a mistake of accessing p256dh, an ArrayBuffer, parameter form non chrome context. sorry


> 
> Why does this have a constructor anyway? This should have a
> ChromeConstructor at most.  No content code is going to be able to construct
> a useful one of these.

There is only a ChromeOnly constructor

	Martin Thomson [:mt:]
	Comment 14 • 9 years ago

Comment on attachment 8625603 [details] [diff] [review]
bug_1172502_v2.patch

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

::: dom/push/PushServiceHttp2.jsm
@@ +172,5 @@
>      }
>    }
>  };
>  
> +var toMap = (m, v) => {

I think that I'd name this parseHeaderFieldParams or something like that.

@@ +176,5 @@
> +var toMap = (m, v) => {
> +  var i = v.indexOf('=');
> +  if (i >= 0) {
> +    m[v.substring(0, i).trim()] = v.substring(i + 1).trim()
> +                                    .replace(/^"(.*)"$/, '$1');

Note that this isn't a completely valid quoted-string parser, but it is OK.

Please add a comment here noting that a quoted string with internal quotes is invalid for all the possible values of this header field.

@@ +531,5 @@
>  
>      return new Promise((resolve, reject) => {
> +      this._subscribeResourceInternal({
> +        record: aRecord,
> +        resolve,

Please don't use this form of object initializer.  Valid !== good.

@@ +542,5 @@
> +      .then(exportedKeys => {
> +        result.pushPublicKey = exportedKeys.publicKey;
> +        result.pushPrivateKey = exportedKeys.privateKey;
> +      })
> +      .then(_ => {

You don't need this extra dispatch.  Delete the above two lines.

::: dom/push/PushServiceHttp2Crypto.jsm
@@ +119,5 @@
> +    .then(chunks => concatArray(chunks).slice(0, len));
> +}
> +
> +/* generate a 96-bit IV for use in GCM, 48-bits of which are populated */
> +function generateIV(index) {

I'm going to be dropping a bomb on this in the next week or so.  There were concerns raised about dictionary attacks on AES-GCM that a change to the nonce structure can fix.  I should have an updated example implementation in a short time.

@@ +174,5 @@
> +      hkdf(base64UrlDecode(aSalt), new Uint8Array(rawKey), INFO, 16))
> +    .then(gcmBits =>
> +      crypto.subtle.importKey('raw', gcmBits, 'AES-GCM', false, ['decrypt']))
> +    .then(gcmKey =>
> +      Promise.all( chunkArray(aData, 4096).map((slice, index) =>

I missed this on the way in.  You need to parse out the rs="" parameter from the Encryption header field.  AND you need to add 16.

A test that covers this would be a good idea.  The current code doesn't really have thorough testing of the decryption part.

::: testing/xpcshell/moz-http2/moz-http2.js
@@ +553,5 @@
> +      { hostname: 'localhost:' + serverPort, port: serverPort,
> +        path : '/pushNotificationsDeliver3', method : 'GET',
> +        headers: { 'Encryption-Key': 'keyid="notification3";dh="BKrCSrfkhMMtqo0pVe2kop0v95vmmDwZwgdDLTIKoU2fGAWRo09wnp3epiWd7II6PopQgGMhBuFEkXI2fT1ZEjI"',
> +                   'Encryption': 'keyid="notification3";salt="Po_8TPrwU_1Hbmz9JAq0SA"'
> +                 }

Can you add one of these that uses a small rs= value please?

	Martin Thomson [:mt:]
	Comment 15 • 9 years ago

I've just updated my private copy of the spec and implementations to deal with an AES-GCM dictionary attack concern.

https://martinthomson.github.io/http-encryption/
https://github.com/martinthomson/webpush-client
https://github.com/martinthomson/encrypted-content-encoding

The main challenge here is the generation of the AES-GCM IV (or nonce).

	Martin Thomson [:mt:]
	Comment 17 • 9 years ago

Comment on attachment 8632865 [details] [diff] [review]
bug_1172502_v2.patch

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

This looks pretty good.

There is one important change you need to make to the decode.  Otherwise, just nits.

Since I've effectively written some of this code, can you get someone else to take a look at PushServiceHttp2Crypto.jsm so that I'm not just blind to my own bugs.  Perhaps kit is willing to help you.

::: dom/push/PushServiceHttp2.jsm
@@ +198,5 @@
> +      }
> +      return m;
> +    }, {});
> +
> +  } catch(e) { return null;}

Maybe add a comment here saying what is being caught.  I can't see anything here that needs catching unless it is `getRequestHeader`

@@ +207,5 @@
> +    return aRequest.getRequestHeader("Encryption")
> +             .split(',', 1)[0]
> +             .split(';')
> +             .reduce(parseHeaderFieldParams, {});
> +  } catch(e) { return null;}

Same

@@ +562,1 @@
>      })

I know that this isn't new, but I would have _subscribeResourceInternal return a promise rather than have it passed resolve and reject functions.  That should trim this down slightly.

@@ +875,5 @@
> +          self._mainPushService.receivedPushMessage(aPushRecord, msg),
> +          self._ackMsgRecv(aAckUri)
> +        ]), err => {
> +          // On error discharge message.
> +          debug("Error decoding received message." + err);

nit: s/\./:/

::: dom/push/PushServiceHttp2Crypto.jsm
@@ +6,5 @@
> +"use strict";
> +
> +const Cu = Components.utils;
> +
> +Cu.importGlobalProperties(["crypto"]);

Nit: I think that we need quote consistency.  A lot of my quotes are single-quotes, all those you've added seem to be double.  Pick one or other.

@@ +106,5 @@
> +
> +/* generate a 96-bit IV for use in GCM, 48-bits of which are populated */
> +function generateNonce(base, index) {
> +  if (index >= Math.pow(2, 48)) {
> +    throw "Error generating IV - index is too large.";

nit: new Error()

@@ +136,5 @@
> +    if (aData.length === 0) {
> +      // Zero length messages will be passed as null.
> +      return null;
> +    }
> +

IMPORTANT: you need to check that the input data is not a whole multiple of (aRs + 16).

if (aData.byteLength % (aRs + 16) === 0) {
  throw new Error('Data truncated');
}

@@ +161,5 @@
> +      })
> +    })
> +    .then(r => {
> +      return Promise.all(chunkArray(aData, aRs + 16).map((slice, index) => {
> +        return crypto.subtle.decrypt({name: 'AES-GCM',

I think that I would prefer to see the decoding of individual chunks as a new function, just for the sake of clarity.

@@ +168,5 @@
> +                                     r.key, slice)
> +          .then(decoded => {
> +            decoded = new Uint8Array(decoded);
> +            if (decoded.length == 0) {
> +              throw "Decoded array is too short!";

new Error()  and throughout.

	Lina Butler [:lina]
	Comment 19 • 9 years ago

Comment on attachment 8633715 [details] [diff] [review]
bug_1172502_v2.patch

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

LGTM. I've noted a few small nits and questions, with the caveat that I'm really not qualified to review crypto code. Looks like this follows the draft accurately, but I'd like to see a decryption test as a sanity check. :-)

::: dom/push/PushServiceHttp2.jsm
@@ +138,5 @@
>  
> +    inputStream.setInputStream(aStream);
> +    let chunk = new ArrayBuffer(aCount);
> +    inputStream.readArrayBuffer(aCount, chunk);
> +    this._message.push(new Uint8Array(chunk));

Looks like `concatArray` already wraps buffers in a `Uint8Array`?

@@ +158,5 @@
> +      }
> +      var dh = keymap[enc.keyid];
> +      var salt = enc.salt;
> +      var rs = (enc.rs)? parseInt(enc.rs) : 4096;
> +      if (!dh || !salt || (rs > 4096)) {

Two nits:

1. `parseInt(enc.rs, 10)`.
2. Does it makes sense to have `isNaN(rs)` as an additional check, so that it doesn't even attempt decryption for bogus record sizes?

@@ +602,5 @@
> +    .then(result => {
> +      if ("subscriptionUri" in result) {
> +        return result;
> +      } else {
> +        return this._subscribeResourceInternal(result);

I think I understand what this does, but it's a little hard to follow. AIUI, this replaces `retrySubscription`: if subscribing fails, the promise is resolved with the same `aSubInfo` (instead of the new record) after a delay, and that's used as a signal to retry.

Is that accurate, or am I totally off? :-) If that's right, could you please remove `retrySubscription` and add a comment (and/or reject with a sentinel, like `{retry: true}`)?

@@ +884,5 @@
>        aPushRecord.lastPush = new Date().getTime();
> +      PushServiceHttp2Crypto.decodeMsg(aMessage, aPushRecord.pushPrivateKey,
> +                                       dh, salt, rs)
> +      .then(msg =>
> +        Promise.all([

I think you can remove `Promise.all` here, since, AFAICT, neither `receivedPushMessage` nor `_ackMsgRecv` returns a promise.

@@ +887,5 @@
> +      .then(msg =>
> +        Promise.all([
> +          self._mainPushService.receivedPushMessage(aPushRecord, msg),
> +          self._ackMsgRecv(aAckUri)
> +        ]), err => {

Please move this into a separate `.catch()`, in case `receivedPushMessage` or `_ackMsgRecv` throws.

@@ +921,5 @@
>    prepareRegister: function(aPushRecord) {
>      return {
>        pushEndpoint: aPushRecord.pushEndpoint,
> +      pushReceiptEndpoint: aPushRecord.pushReceiptEndpoint,
> +      pushPublicKey: aPushRecord.pushPublicKey

Maybe just `publicKey`?

::: dom/push/PushServiceHttp2Crypto.jsm
@@ +22,5 @@
> +  while(index + size <= array.byteLength) {
> +    result.push(new Uint8Array(array, start + index, size));
> +    index += size;
> +  }
> +  if (index <= array.byteLength) {

I'm a bit surprised to see `<=` here, since it'll append an empty array if `array.byteLength` is a multiple of `size`. Is that intentional?

@@ +77,5 @@
> +hmac.prototype.hash = function(input) {
> +  return this.keyPromise.then(k => crypto.subtle.sign('HMAC', k, input));
> +};
> +
> +function hkdf(salt, ikm) {

Looks good! I'm surprised WebCrypto doesn't support HKDF natively...

@@ +134,5 @@
> +  decodeMsg: function(aData, aPrivateKey, aRemotePublicKey, aSalt, aRs) {
> +
> +    if (aData.byteLength === 0) {
> +      // Zero length messages will be passed as null.
> +      return null;

Please change this to `return Promise.resolve(null)`. It looks like `_pushChannelOnStop` expects this to always return a promise.

@@ +138,5 @@
> +      return null;
> +    }
> +
> +    if (aData.byteLength % (aRs + 16) === 0) {
> +      throw new Error('Data truncated');

As above, `return Promise.reject(new Error('Data truncated'))`.

@@ +155,5 @@
> +    .then(keys =>
> +      crypto.subtle.deriveBits({ name: 'ECDH', public: keys[0] }, keys[1], 256))
> +    .then(rawKey => {
> +      var kdf = new hkdf(base64UrlDecode(aSalt), new Uint8Array(rawKey));
> +      return Promise.allMap({

If `Promise.allMap` is only used here, you could use `Promise.all([...])`, and use destructuring assignment to unpack `([key, nonce])`.

@@ +164,5 @@
> +        nonce: kdf.generate(NONCE_INFO, 12)
> +      })
> +    })
> +    .then(r => {
> +      return Promise.all(chunkArray(aData, aRs + 16).map((slice, index) =>

This is the "16 octet authentication tag" mentioned in https://martinthomson.github.io/http-encryption, right?

::: dom/push/test/xpcshell/test_notification_http2.js
@@ +113,5 @@
>    for (let record of records) {
>      yield db.put(record);
>    }
>  
>    let notifyPromise = Promise.all([

1. I think you'll want to convert the typed array to a string in `_notifyApp`, when the `push-notification` is emitted. Otherwise, it'll coerce the typed array to a string, and give you `"[object Uint8Array]"`.

2. Could you please test that the messages are decrypted correctly?

::: testing/xpcshell/moz-http2/moz-http2.js
@@ +543,4 @@
>        'subresource' : '1'
>        });
> +
> +    var buf = BufferStream('66df5d11daa01e5c802ff97cdf7f39684b5bf7c6418a5cf9b609c6826c04b25e403823607ac514278a7da945');

Does `BufferStream` test response streaming (i.e., `pushPushServer2.end('66df...', 'hex')` wouldn't work)?

	Dragana Damjanovic [:dragana] Assignee
	Comment 20 • 9 years ago

(In reply to Kit Cambridge [:kitcambridge] from comment #19)
> Comment on attachment 8633715 [details] [diff] [review]
> bug_1172502_v2.patch
> 
> Review of attachment 8633715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM. I've noted a few small nits and questions, with the caveat that I'm
> really not qualified to review crypto code. Looks like this follows the
> draft accurately, but I'd like to see a decryption test as a sanity check.
> :-)
> 
> ::: dom/push/PushServiceHttp2.jsm
> @@ +138,5 @@
> >  
> > +    inputStream.setInputStream(aStream);
> > +    let chunk = new ArrayBuffer(aCount);
> > +    inputStream.readArrayBuffer(aCount, chunk);
> > +    this._message.push(new Uint8Array(chunk));
> 
> Looks like `concatArray` already wraps buffers in a `Uint8Array`?
> 

I have changed concatArray and forgot to change this


> @@ +602,5 @@
> > +    .then(result => {
> > +      if ("subscriptionUri" in result) {
> > +        return result;
> > +      } else {
> > +        return this._subscribeResourceInternal(result);
> 
> I think I understand what this does, but it's a little hard to follow. 

Martin wanted me to change that, First I did it differently because I though this way is rather confusing. It does take long to figure out what is happening, but at the end they are confusing the same...

I forgot to delete `retrySubscription` as made change above in the last iteration. 


> @@ +884,5 @@
> >        aPushRecord.lastPush = new Date().getTime();
> > +      PushServiceHttp2Crypto.decodeMsg(aMessage, aPushRecord.pushPrivateKey,
> > +                                       dh, salt, rs)
> > +      .then(msg =>
> > +        Promise.all([
> 
> I think you can remove `Promise.all` here, since, AFAICT, neither
> `receivedPushMessage` nor `_ackMsgRecv` returns a promise.
> 

I changed it bit, actually I will send ack when message is send to a listener.


> 
> @@ +921,5 @@
> >    prepareRegister: function(aPushRecord) {
> >      return {
> >        pushEndpoint: aPushRecord.pushEndpoint,
> > +      pushReceiptEndpoint: aPushRecord.pushReceiptEndpoint,
> > +      pushPublicKey: aPushRecord.pushPublicKey
> 
> Maybe just `publicKey`?

It is just a name and any is good...it will change it.

> 
> ::: dom/push/PushServiceHttp2Crypto.jsm
> @@ +22,5 @@
> > +  while(index + size <= array.byteLength) {
> > +    result.push(new Uint8Array(array, start + index, size));
> > +    index += size;
> > +  }
> > +  if (index <= array.byteLength) {
> 
> I'm a bit surprised to see `<=` here, since it'll append an empty array if
> `array.byteLength` is a multiple of `size`. Is that intentional?

not intentional :)



> 
> @@ +155,5 @@
> > +    .then(keys =>
> > +      crypto.subtle.deriveBits({ name: 'ECDH', public: keys[0] }, keys[1], 256))
> > +    .then(rawKey => {
> > +      var kdf = new hkdf(base64UrlDecode(aSalt), new Uint8Array(rawKey));
> > +      return Promise.allMap({
> 
> If `Promise.allMap` is only used here, you could use `Promise.all([...])`,
> and use destructuring assignment to unpack `([key, nonce])`.
> 
> @@ +164,5 @@
> > +        nonce: kdf.generate(NONCE_INFO, 12)
> > +      })
> > +    })
> > +    .then(r => {
> > +      return Promise.all(chunkArray(aData, aRs + 16).map((slice, index) =>
> 
> This is the "16 octet authentication tag" mentioned in
> https://martinthomson.github.io/http-encryption, right?
> 

yes, I have added a comment.

> 2. Could you please test that the messages are decrypted correctly?
> 

We get multiple notification probably it is connected to bug 1184626. We can resolve that in a separate bug.


> ::: testing/xpcshell/moz-http2/moz-http2.js
> @@ +543,4 @@
> >        'subresource' : '1'
> >        });
> > +
> > +    var buf = BufferStream('66df5d11daa01e5c802ff97cdf7f39684b5bf7c6418a5cf9b609c6826c04b25e403823607ac514278a7da945');
> 
> Does `BufferStream` test response streaming (i.e.,
> `pushPushServer2.end('66df...', 'hex')` wouldn't work)?

I did not know that that works.

	Lina Butler [:lina]
	Comment 22 • 9 years ago

Comment on attachment 8634843 [details] [diff] [review]
bug_1172502_v2.patch

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

LGTM, pending a rebase and removal of `refreshSubscription`.

::: dom/push/PushServiceHttp2.jsm
@@ +604,5 @@
> +        reject({status: 0, error: "NetworkError"});
> +      }
> +    })
> +    .catch(err => {
> +      if ("retry" in err) {

`if (err.retry)`.

@@ +896,5 @@
> +              }
> +              return self._mainPushService.receivedPushMessage(aPushRecord,
> +                                                               msgString);
> +            })
> +      .then(self._ackMsgRecv(aAckUri))

`.then(() => self._ackMsgRecv(aAckUri))`?

	Martin Thomson [:mt:]
	Comment 23 • 9 years ago

Comment on attachment 8634843 [details] [diff] [review]
bug_1172502_v2.patch

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

Fix the ack and I think we're good to land.

	Dragana Damjanovic [:dragana] Assignee
	Comment 24 • 9 years ago

rebase.

Kit can you have a short look, 4 eyes are better than 2, since a bit of code has changed because of the rebase?

	Dragana Damjanovic [:dragana] Assignee
	Comment 25 • 9 years ago

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

	Lina Butler [:lina]
	Comment 26 • 9 years ago

Comment on attachment 8637088 [details] [diff] [review]
bug_1172502_v2.patch

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

Looking great. Thanks, Dragana! Sorry for the rebase churn.

Olli, could you review the WebIDL change, please?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 27 • 9 years ago

Comment on attachment 8637088 [details] [diff] [review]
bug_1172502_v2.patch

PushSubscription should not be a ChromeOnly interface.
'"PushSubscription" in window' should be true in a web page.
Are we going to expose this all to web by default or
shouldn't PushSubscription have some Pref="foo.bar.pref.enabled" or something?

I think you want ChromeConstructor, and then drop ChromeOnly

I don't see readonly attribute ArrayBuffer? p256dh; in the spec.
Where is it coming? p256dh as an attribute name in a web API looks rather weird.
Any more descriptive name perhaps?

	Dragana Damjanovic [:dragana] Assignee
	Comment 28 • 9 years ago

There will be p256dh attribute.

The change i did to webidl is because of the websocket push not having publicKey. Other possibility is to use a dummy ArrayBuffer  for the websocket push.

	Martin Thomson [:mt:]
	Comment 29 • 9 years ago

:smaug, this is from https://github.com/w3c/push-api/pull/130

p256dh is a public P-256 Diffie-Hellman parameter.  I'm happy to entertain suggestions on the name.

I agree that we want to have dom.push.enabled (or whatever it is) attached to all the relevant interfaces so that we can turn this off if we need to and not have vestigal bits of API left.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 30 • 9 years ago

(I'm about to use some wrong terminology...)
Is it possible we'll want support also something else than "P-256 Diffie-Hellman"?
If so, would it make sense to have
something like
readonly attribute WebPushMessageEncryption? encryption;

enum WebPushMessageEncryptionType {
  "p256dh"
}

interface WebPushMessageEncryption // (or could this be dictionary?)
{
  readonly attribute WebPushMessageEncryptionType type;
  readonly attribute ArrayBuffer key;
}

Or is that over-engineering? p256dh as an attribute name just feels really weird. It looks more like a value of some attribute, not the name of the attribute..

	Martin Thomson [:mt:]
	Comment 31 • 9 years ago

That seems like it would work, but it doesn't actually offer algorithm agility.  My plan here is this:

partial interface PushSubscription
{
  readonly attribute ArrayBuffer p25519dh;
  readonly attribute ArrayBuffer p256dh;
};

	Ben Bangert [:benbangert]
	Comment 32 • 9 years ago

Out of curiosity, who from our security/crypto team is reviewing the crypto here?

	JR Conlin [:jrconlin,:jconlin]
	Comment 33 • 9 years ago

Since there's crypto involved, we really need to have someone from Security review and sign off on the methodology. Ideally someone with a strong crypto background so that we're limiting the number of potential exposure points. 

Can we ping Yvan or April to chime in on this?

	Martin Thomson [:mt:]
	Comment 34 • 9 years ago

Comment on attachment 8637088 [details] [diff] [review]
bug_1172502_v2.patch

See previous comments.

 More review wouldn't hurt here; sworkman might have more relevant expertise available.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 35 • 9 years ago

(In reply to Martin Thomson [:mt:] from comment #31)
> That seems like it would work, but it doesn't actually offer algorithm
> agility.  My plan here is this:
> 
> partial interface PushSubscription
> {
>   readonly attribute ArrayBuffer p25519dh;
>   readonly attribute ArrayBuffer p256dh;
> };
This looks still rather odd API. How much will the API grow with odd looking attribute names?
Would ArrayBuffer? getEncryptionData(WebPushMessageEncryptionType type); work?

	Martin Thomson [:mt:]
	Comment 36 • 9 years ago

(In reply to Olli Pettay [:smaug] (away-ish until July 26, expect slower than usual reviews) from comment #35)
> This looks still rather odd API. How much will the API grow with odd looking
> attribute names?

Well, hopefully it doesn't grow at all.

> Would ArrayBuffer? getEncryptionData(WebPushMessageEncryptionType type);
> work?

That would work.  I will ask that same question.

	Dragana Damjanovic [:dragana] Assignee
	Comment 37 • 9 years ago

Steve, can you point to someone how can review security part of this?

	Dragana Damjanovic [:dragana] Assignee
	Comment 38 • 9 years ago

Sorry, just very small change.

Steve, can you find someone to review this?

	Martin Thomson [:mt:]
	Comment 39 • 9 years ago

Comment on attachment 8654031 [details] [diff] [review]
bug_1172502_v2.patch

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

This looks pretty good.  The only thing I think we need is to ensure that there is always a key.  See the comment on the .webidl

::: dom/push/PushClient.js
@@ +109,5 @@
> +                             key);
> +      return;
> +    }
> +
> +    request.onPushEndpoint(Cr.NS_OK, registration.pushEndpoint, 0, null);

Here is where you would generate a new key for the subscription.

::: dom/push/PushManager.cpp
@@ +124,5 @@
> +                                       mRawP256dhKey.Length(),
> +                                       mRawP256dhKey.Elements());
> +      MOZ_ALWAYS_TRUE(mP256dhKey);
> +    }
> +    JS::ExposeObjectToActiveJS(mP256dhKey);

I don't completely understand this stuff, but does this mean that I can *change* the value of the internally held key?  I think that we want^Wneed to return a copy every time the method is called.  We can't have scripts overwriting the value of the key (I mean, they only hurt themselves in the end, but we generally don't allow that sort of thing)

That probably means that you can drop a lot of the HoldJSObjects() calls and the cycle collection for mP256dhKey

@@ +162,5 @@
> +  nsRefPtr<PushSubscription> sub = new PushSubscription(global,
> +                                                        aEndpoint,
> +                                                        aScope,
> +                                                        rawKey);
> +  

trailing spaces

::: dom/push/PushService.jsm
@@ +765,2 @@
>        if (!record) {
> +        return Promise.resolve(notified);

Since this is running in a .then(), you can just return notified directly (also below).

@@ +767,2 @@
>        }
> +      

space

::: dom/push/PushServiceHttp2.jsm
@@ +354,1 @@
>                                     subscriptionUri});

fix indent

@@ +559,4 @@
>  
> +      var chan = this._makeChannel(this._serverURI.spec);
> +      chan.requestMethod = "POST";
> +      try{

space after try

::: dom/webidl/PushSubscription.webidl
@@ +17,2 @@
>  [Exposed=(Window,Worker), Func="nsContentUtils::PushEnabled",
> + ChromeConstructor(DOMString pushEndpoint, DOMString scope, ArrayBuffer? key)]

I think that the right way to do this is to make the key mandatory here.  That would simplify the code on the C++ end a little.

That introduces a problem for the stuff that is already created prior to this feature shipping.  That's OK, we can just generate a new key for those when we lift them from the database.  (We could, as part of the indexDB version upgrade, generate a key for each, but that likely imposes a startup performance hit.)  By doing so, we allow pushes with data for previously created subscriptions, though I don't think you need to worry about surfacing a pushsubscriptionchange event for this.

	Steve Workman [:sworkman] (INACTIVE)
	Comment 40 • 9 years ago

Comment on attachment 8654031 [details] [diff] [review]
bug_1172502_v2.patch

Keeler is probably the best one to look at this. He's on PTO and should be back Web Sept 2nd.

	Dragana Damjanovic [:dragana] Assignee
	Comment 41 • 9 years ago

(In reply to Martin Thomson [:mt:] from comment #39)
> Comment on attachment 8654031 [details] [diff] [review]
> bug_1172502_v2.patch

> 
> ::: dom/webidl/PushSubscription.webidl
> @@ +17,2 @@
> >  [Exposed=(Window,Worker), Func="nsContentUtils::PushEnabled",
> > + ChromeConstructor(DOMString pushEndpoint, DOMString scope, ArrayBuffer? key)]
> 
> I think that the right way to do this is to make the key mandatory here. 
> That would simplify the code on the C++ end a little.


ArrayBuffer is optional because of the old push, WebSocket push and not the new one. So we cannot remove this.

	Martin Thomson [:mt:]
	Comment 42 • 9 years ago

(In reply to Dragana Damjanovic [:dragana] from comment #41)
> ArrayBuffer is optional because of the old push, WebSocket push and not the
> new one. So we cannot remove this.

Will old WS push look like it is broken because it doesn't have a key?

This dependency on an API that supports unencrypted message delivery is exactly why we (and Google) rejected payloads.  We need to talk about a deprecation plan for that; on a short timescale preferably.

	Dragana Damjanovic [:dragana] Assignee
	Comment 43 • 9 years ago

(In reply to Martin Thomson [:mt:] from comment #42)
> (In reply to Dragana Damjanovic [:dragana] from comment #41)
> > ArrayBuffer is optional because of the old push, WebSocket push and not the
> > new one. So we cannot remove this.
> 
> Will old WS push look like it is broken because it doesn't have a key?
> 

The old WS push will be completely broken. It is not possible to subscribe for it without optional ArrayBuffer. I cannot change this now because all test for the old ws push would fail.

> This dependency on an API that supports unencrypted message delivery is
> exactly why we (and Google) rejected payloads.  We need to talk about a
> deprecation plan for that; on a short timescale preferably.

I will leave interface as it is for now. And when we remove the old WS push we will change this too.

	Lina Butler [:lina]
	Comment 44 • 9 years ago

Our Push server will support encrypted data delivery over WebSockets, so I think it's OK to make the `ArrayBuffer` mandatory after bug 1185544 lands.

	Martin Thomson [:mt:]
	Comment 45 • 9 years ago

Let's just land this as-is (assuming that Richard is happy with the code) and we can see what we can do about the WS stuff.  (I don't know how it would move the keys, for example).

Dragana, if you could fix the other stuff I noted, I'll give you an r+ and you only need a second security check.

	Dragana Damjanovic [:dragana] Assignee
	Comment 46 • 9 years ago

Update:

It is not possible to update all records inside upgradeSchema where it would be the most proper place. Because of the way IndexDB is implemented this is not possible.

I have added a check whether a subscription has a key and if it does not the subscription is updated and a push subscription change event is fired. (I added a test for this too.)

I have changed PushSubscription to deliver a copy of the key each time. I think the old code works as it should be (it is not possible to overwrite key value in both  version), but changing that made the code easier.

	Martin Thomson [:mt:]
	Comment 47 • 9 years ago

Comment on attachment 8656010 [details] [diff] [review]
bug_1172502_v2.patch

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

::: dom/push/PushManager.cpp
@@ +115,5 @@
> +{
> +  if (aType == PushEncryptionKeyName::P256dh && !mRawP256dhKey.IsEmpty()) {
> +    aP256dhKey.set(ArrayBuffer::Create(aCx,
> +                                       mRawP256dhKey.Length(),
> +                                       mRawP256dhKey.Elements()));

This was the important change, thanks.

::: dom/push/test/xpcshell/test_updateRecordNoEncryptionKeys.js
@@ +16,5 @@
> +
> +function listenHandler(metadata, response) {
> +  do_check_true(true, "Start listening");
> +  do_test_finished();
> +  response.setHeader("Retry-After", '10');

Nit: consistent use of quotes. "10"

@@ +42,5 @@
> +add_task(function* test1() {
> +
> +  let db = PushServiceHttp2.newPushDB();
> +  do_register_cleanup(() => {
> +    return db.drop().then(_ => db.close());

Nit: Try to be consistent about () vs _ in arrow functions.

@@ +64,5 @@
> +
> +  let notifyPromise = promiseObserverNotification('push-subscription-change',
> +    (subject, data) => {
> +      return true;
> +    }

() => true

	Martin Thomson [:mt:]
	Comment 48 • 9 years ago

BTW, I don't know if you have a problem with reviewboard, but non-trivial patches are much harder to review in splinter.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 49 • 9 years ago

(Some people prefer Bugzilla's 'Details' over splinter or mozreview ;) )

	Dragana Damjanovic [:dragana] Assignee
	Comment 50 • 9 years ago

(In reply to Martin Thomson [:mt:] from comment #48)
> BTW, I don't know if you have a problem with reviewboard, but non-trivial
> patches are much harder to review in splinter.

Yes, I have a problem with Reviewboard. After the last mercurial update it just didn't want to work. I will try to fix it soon.

	Dragana Damjanovic [:dragana] Assignee
	Comment 51 • 9 years ago

from Martin's comments.

	Dana Keeler (she/her) (use needinfo) [:keeler]
	Comment 52 • 9 years ago

Comment on attachment 8656077 [details] [diff] [review]
bug_1172502_v2.patch

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

I'm not really familiar enough with the dom/push areas to do a good review. If the intention was more that I verify that PushServiceHttp2Crypto.jsm implements https://tools.ietf.org/html/draft-thomson-webpush-encryption-00 as specified, I can do that, but I probably won't be done until next week sometime.

	Dragana Damjanovic [:dragana] Assignee
	Comment 53 • 9 years ago

(In reply to David Keeler [:keeler] (use needinfo?) from comment #52)
> Comment on attachment 8656077 [details] [diff] [review]
> bug_1172502_v2.patch
> 
> Review of attachment 8656077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not really familiar enough with the dom/push areas to do a good review.
> If the intention was more that I verify that PushServiceHttp2Crypto.jsm
> implements https://tools.ietf.org/html/draft-thomson-webpush-encryption-00
> as specified, I can do that, but I probably won't be done until next week
> sometime.

Can you please review the crypto part. Thanks.

	Dana Keeler (she/her) (use needinfo) [:keeler]
	Comment 54 • 9 years ago

Comment on attachment 8656077 [details] [diff] [review]
bug_1172502_v2.patch

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

As far as I can tell, this is an accurate implementation of the relevant parts of the spec(s). A bit more documentation would be nice.

::: dom/push/test/xpcshell/test_notification_http2.js
@@ +120,5 @@
>    let notifyPromise = Promise.all([
> +    promiseObserverNotification('push-notification', function(subject, data) {
> +      var notification = subject.QueryInterface(Ci.nsIPushObserverNotification);
> +      if (notification && (data == "https://example.com/page/1")){
> +        equal(subject.data, "Some message", "decoded message is incorect");

spelling nit: "incorrect"

	Dragana Damjanovic [:dragana] Assignee
	Comment 56 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d1c0679d198

	Dragana Damjanovic [:dragana] Assignee
	Comment 57 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6df9b093ba40

	Dragana Damjanovic [:dragana] Assignee
	Comment 58 • 9 years ago

just fix a test.

	Carsten Book [:Tomcat]
	Comment 59 • 9 years ago

i get 
remote: *************************** ERROR ***************************
remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsIPushEndpointCallback in changeset 23d5be211247
remote: 
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsIPushEndpointCallback
remote: 
remote: If you intentionally want to keep the current UUID, include
remote: 'IGNORE IDL' in the commit message.
remote: *************************************************************

when i want to push this, can you check this ? thanks!

	Lina Butler [:lina]
	Comment 60 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa29b59bd185be3ca22ea62796e509f8ec13ba0
Bug 1172502 - Add message encription for WebPush. r=mt r=kitcambridge r=keeler r=smaug

	Phil Ringnalda (:philor)
	Comment 61 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/7fa29b59bd18

	Boris Zbarsky [:bzbarsky]
	Comment 62 • 9 years ago

Shouldn't the IDL document that the getKey method returns a new object each time?

	Martin Thomson [:mt:]
	Comment 63 • 9 years ago

I updated the (proposed) spec.  That should do, I hope.

	Boris Zbarsky [:bzbarsky]
	Comment 64 • 9 years ago

The IDL doesn't seem to have a link to this spec.  Or at least the spec it has a link to doesn't have getKey.

	Dragana Damjanovic [:dragana] Assignee
	Comment 65 • 9 years ago

Update patch to match pushed one.

Thanks , Kit!

	Dragana Damjanovic [:dragana] Assignee
	Comment 66 • 9 years ago

Message encryption part is work in progress tracked by:

https://github.com/w3c/push-api/pull/130