1225968 - Push message encryption change

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Martin Thomson [:mt:]]

Unfortunately, the final details are not settled, but here is the WIP:
  https://github.com/webpush-wg/webpush-encryption/pull/4

This is the head of a wide tree of changes.  Once we sort out where the secret is mixed in, we can finalize this.

Comment 1

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge,smaug

Bug 1225968 - Add authentication secret to push API, r?kcambridge

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8689248 [details]
MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge,smaug

This isn't a review request yet, because I haven't added any actual tests.  I need to do some crypto work first.  That and the fact that I haven't sorted out the spec issues yet.

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8689248 [details]
MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge,smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25583/diff/1-2/

Comment 4

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1225968 - Refactor data delivery tests to support addition of new tests, r=kitcambridge

Bug 1225968 - Refactor data delivery tests to support addition of new tests, r?kcambridge

Comment 5

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1225968 - Refactoring to move some of the push crypto logic, r=kitcambridge

Bug 1225968 - Refactoring to move some of the push crypto logic, r?kcambridge

Comment 6

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1225968 - Adding more messages to the push message tests, r=kitcambridge

Bug 1225968 - Adding more messages to the push message tests, r?kcambridge

Comment 7

[Martin Thomson [:mt:]]

Latest proposal is here: 
https://github.com/w3c/push-api/issues/174
https://github.com/webpush-wg/webpush-encryption/pull/4
https://github.com/martinthomson/http-encryption/pull/5

All of which need to be considered together.  I'm looking to merge all of those shortly.

Comment 8

[Martin Thomson [:mt:]]

This is rare enough that I have to highlight it:

ALL GREEN: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7658864fda1

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8690272 [details]
MozReview Request: Bug 1225968 - Refactor data delivery tests to support addition of new tests, r=kitcambridge

https://reviewboard.mozilla.org/r/25821/#review24117

Looks good, thanks for doing this!

::: dom/push/test/xpcshell/test_notification_data.js:178
(Diff revision 1)
> +          dump("push-ack\n");

Leftover debugging?

Comment 10

[Lina Butler [:lina]]

Comment on attachment 8690273 [details]
MozReview Request: Bug 1225968 - Refactoring to move some of the push crypto logic, r=kitcambridge

https://reviewboard.mozilla.org/r/25823/#review24119

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8690274 [details]
MozReview Request: Bug 1225968 - Adding more messages to the push message tests, r=kitcambridge

https://reviewboard.mozilla.org/r/25825/#review24125

Comment 12

[Lina Butler [:lina]]

Comment on attachment 8689248 [details]
MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge,smaug

https://reviewboard.mozilla.org/r/25583/#review23521

This looks good to me; just noted a few nits and questions.

::: dom/push/PushCrypto.jsm:17
(Diff revision 2)
> -var NONCE_INFO = new TextEncoder('utf-8').encode('Content-Encoding: nonce');
> +var ENCRYPT_INFO = UTF8.encode('Content-Encoding: aesgcm128');

Just to make sure I understand...

If we have a secret, the context for encryption and HKDF will be `'Content-Encoding: aesgcm128\0P-256\0{length(recipientKey)}{recipientKey}{length(senderKey)}{senderKey}`.

Otherwise, it's just `Content-Encoding: aesgcm128`, as before. Is that right?

::: dom/push/PushCrypto.jsm:223
(Diff revision 2)
> +        this._encodeLength(receiverKey), receiverKey,

You mention this is the "encoded public key of the recipient" in the spec, but it looks like this is the Base64-decoded buffer. Does "encoded" mean something different in that context?

::: dom/push/PushManager.cpp:198
(Diff revision 2)
> +    authSecret.SetLength(sekrit.Length());

I think you can merge these two calls into `authSecret.InsertElementsAt(0, sekrit.Data(), sekrit.Length())`.

::: dom/push/PushManager.cpp:322
(Diff revision 2)
> +    const ArrayBuffer& sekrit = aAuthSecret.Value();

:-)

::: dom/push/PushManager.cpp:637
(Diff revision 2)
> +    return OnPushEndpoint(aStatus, EmptyString(), 0, nullptr, 0, nullptr);

Looks like we ignore the return value. Could you make this `Unused << NS_WARN_IF(NS_FAILED(...))`, please?

::: dom/push/PushService.jsm:791
(Diff revision 2)
> +    if (!record.authenticationSecret) {

I think we'll want to call `updateRecordAndNotifyApp` for this case, too.

Comment 13

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/25583/#review23521

> Just to make sure I understand...
> 
> If we have a secret, the context for encryption and HKDF will be `'Content-Encoding: aesgcm128\0P-256\0{length(recipientKey)}{recipientKey}{length(senderKey)}{senderKey}`.
> 
> Otherwise, it's just `Content-Encoding: aesgcm128`, as before. Is that right?

Yes, I have added a comment to the key derivation function to explain that.  It's pretty detailed.

> You mention this is the "encoded public key of the recipient" in the spec, but it looks like this is the Base64-decoded buffer. Does "encoded" mean something different in that context?

Oh, I hope that it isn't base64 encoded.  It's the raw byte encoded form.  I should patch the spec there to make it clear.

Comment 14

[Martin Thomson [:mt:]]

Comment on attachment 8689248 [details]
MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge,smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25583/diff/2-3/

Comment 15

[Martin Thomson [:mt:]]

Comment on attachment 8690272 [details]
MozReview Request: Bug 1225968 - Refactor data delivery tests to support addition of new tests, r=kitcambridge

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25821/diff/1-2/

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8690273 [details]
MozReview Request: Bug 1225968 - Refactoring to move some of the push crypto logic, r=kitcambridge

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25823/diff/1-2/

Comment 17

[Martin Thomson [:mt:]]

Comment on attachment 8690274 [details]
MozReview Request: Bug 1225968 - Adding more messages to the push message tests, r=kitcambridge

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25825/diff/1-2/

Comment 18

[Martin Thomson [:mt:]]

Comment on attachment 8689248 [details]
MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge,smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25583/diff/3-4/

Comment 19

[Martin Thomson [:mt:]]

Smaug, the webidl changes are pretty minimal.  The spec has a PR that describes the changes here:
  https://github.com/w3c/push-api/pull/174
(I'm confirming that the changes it depends on are OK before proceeding.)

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689248 [details]
MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge,smaug

https://reviewboard.mozilla.org/r/25583/#review24557

::: dom/webidl/PushSubscription.webidl:19
(Diff revision 3)
> - ChromeConstructor(DOMString pushEndpoint, DOMString scope, ArrayBuffer? key)]
> + ChromeConstructor(DOMString pushEndpoint, DOMString scope,

Just wondering if we'll want to use some dictionary as the only constructor param, and not adding more and more parameters.
up to you (and definitely no need to add the dictionary in this bug)

Comment 21

[Martin Thomson [:mt:]]

Yeah, a dictionary is probably a good idea.  It's right at the threshold now.

Comment 22

[Martin Thomson [:mt:]]

Comment on attachment 8689248 [details]
MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge,smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25583/diff/3-4/

Comment 23

[Martin Thomson [:mt:]]

Comment on attachment 8690272 [details]
MozReview Request: Bug 1225968 - Refactor data delivery tests to support addition of new tests, r=kitcambridge

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25821/diff/2-3/

Comment 24

[Martin Thomson [:mt:]]

Comment on attachment 8690273 [details]
MozReview Request: Bug 1225968 - Refactoring to move some of the push crypto logic, r=kitcambridge

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25823/diff/2-3/

Comment 25

[Martin Thomson [:mt:]]

Comment on attachment 8690274 [details]
MozReview Request: Bug 1225968 - Adding more messages to the push message tests, r=kitcambridge

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25825/diff/2-3/

Comment 26

[Martin Thomson [:mt:]]

Comment on attachment 8689248 [details]
MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge,smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25583/diff/4-5/

Comment 27

[Martin Thomson [:mt:]]

Comment on attachment 8690272 [details]
MozReview Request: Bug 1225968 - Refactor data delivery tests to support addition of new tests, r=kitcambridge

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25821/diff/3-4/

Comment 28

[Martin Thomson [:mt:]]

Comment on attachment 8690273 [details]
MozReview Request: Bug 1225968 - Refactoring to move some of the push crypto logic, r=kitcambridge

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25823/diff/3-4/

Comment 29

[Martin Thomson [:mt:]]

Comment on attachment 8690274 [details]
MozReview Request: Bug 1225968 - Adding more messages to the push message tests, r=kitcambridge

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25825/diff/3-4/

Comment 30

[Martin Thomson [:mt:]]

Try run looks good.

Comment 31

[Carsten Book [:Tomcat]]

this causes during checkin:

remote: *************************** ERROR ***************************
remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsIPushEndpointCallback in changeset 900b71dcd7f6
remote: 
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsIPushEndpointCallback

can you take a look, thanks!

Comment 32

[Martin Thomson [:mt:]]

It would be really nice if there were some way to catch that before this point.  Sorry about not catching it myself.  Since I'm there I'll check this in myself :)

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99dd62654b77
https://hg.mozilla.org/integration/mozilla-inbound/rev/065e03c7e416
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d619f945154
https://hg.mozilla.org/integration/mozilla-inbound/rev/c456aa0d72e4

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99dd62654b77
https://hg.mozilla.org/mozilla-central/rev/065e03c7e416
https://hg.mozilla.org/mozilla-central/rev/9d619f945154
https://hg.mozilla.org/mozilla-central/rev/c456aa0d72e4