Closed Bug 1225968 Opened 4 years ago Closed 4 years ago

Push message encryption change

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: mt, Assigned: mt)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.
Bug 1225968 - Add authentication secret to push API, r?kcambridge
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.
Attachment #8689248 - Flags: feedback?(kcambridge)
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/
Attachment #8689248 - Flags: feedback?(kcambridge) → review?(kcambridge)
Bug 1225968 - Refactor data delivery tests to support addition of new tests, r?kcambridge
Attachment #8690272 - Flags: review?(kcambridge)
Bug 1225968 - Refactoring to move some of the push crypto logic, r?kcambridge
Attachment #8690273 - Flags: review?(kcambridge)
Bug 1225968 - Adding more messages to the push message tests, r?kcambridge
Attachment #8690274 - Flags: review?(kcambridge)
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.
This is rare enough that I have to highlight it:

ALL GREEN: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7658864fda1
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?
Attachment #8690272 - Flags: review?(kcambridge) → review+
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
Attachment #8690273 - Flags: review?(kcambridge) → review+
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
Attachment #8690274 - Flags: review?(kcambridge) → review+
Attachment #8689248 - Flags: review?(kcambridge) → review+
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.
Blocks: 1230038
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 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/
Attachment #8689248 - Attachment description: MozReview Request: Bug 1225968 - Add authentication secret to push API, r?kcambridge → MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge
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/
Attachment #8690272 - Attachment description: MozReview Request: Bug 1225968 - Refactor data delivery tests to support addition of new tests, r?kcambridge → MozReview Request: Bug 1225968 - Refactor data delivery tests to support addition of new tests, r=kitcambridge
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/
Attachment #8690273 - Attachment description: MozReview Request: Bug 1225968 - Refactoring to move some of the push crypto logic, r?kcambridge → MozReview Request: Bug 1225968 - Refactoring to move some of the push crypto logic, r=kitcambridge
Attachment #8690274 - Attachment description: MozReview Request: Bug 1225968 - Adding more messages to the push message tests, r?kcambridge → MozReview Request: Bug 1225968 - Adding more messages to the push message tests, r=kitcambridge
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/
Attachment #8689248 - Attachment description: MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge → MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge?smaug
Attachment #8689248 - Flags: review?(bugs)
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/
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.)
Assignee: nobody → martin.thomson
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)
Attachment #8689248 - Flags: review?(bugs) → review+
Yeah, a dictionary is probably a good idea.  It's right at the threshold now.
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/
Attachment #8689248 - Attachment description: MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge?smaug → MozReview Request: Bug 1225968 - Add authentication secret to push API, r=kitcambridge,smaug
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 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 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 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 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 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 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/
Try run looks good.
Keywords: checkin-needed
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!
Flags: needinfo?(martin.thomson)
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 :)
Flags: needinfo?(martin.thomson)
Keywords: checkin-needed
Depends on: 1233315
You need to log in before you can comment on or make changes to this bug.