Closed Bug 1257405 Opened 4 years ago Closed 4 years ago

Increase auth secret length to 16 octets

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(1 file, 1 obsolete file)

This was changed from 12 to 16 in https://github.com/webpush-wg/webpush-encryption/commit/2d1b41c16adf058d5d95ba32593405807a97dee7, but our implementation still uses the old value.

Thanks for noticing this, Marco!
Attached patch auth.patch (obsolete) — Splinter Review
Attachment #8731523 - Flags: review?(martin.thomson)
Comment on attachment 8731523 [details] [diff] [review]
auth.patch

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

Yep, oops.
Attachment #8731523 - Flags: review?(martin.thomson) → review+
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Attached patch rebased.patchSplinter Review
Carrying forward r+.
Attachment #8731523 - Attachment is obsolete: true
Attachment #8732017 - Flags: review+
Oops, thanks, RyanVM. This is a tiny change, so it can wait until the next checkin-needed batch.
Keywords: checkin-needed
Kit, Why don't you ask ötto to land it for you?
https://hg.mozilla.org/mozilla-central/rev/5fe5459a786b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: WADI
See Also: → 1257821
Comment on attachment 8732017 [details] [diff] [review]
rebased.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1257405, Bug 1257821.
[User impact if declined]: Chrome 50 is shipping a newer implementation of Push message crypto [http://blog.chromium.org/2016/03/chrome-50-beta-push-notification.html]. Developers will need to use workarounds to send encrypted messages to Firefox 46 and 47, causing web compat pain.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7d4363629bc
[Risks and why]: Low to medium risk. Tests cover valid and invalid encryption, but this change requires both bugs to be uplifted to Beta for completeness.
[String/UUID change made/needed]: None.

This is the second bug that covers the encryption change, along with bug 1257821. We can only uplift if approval for Beta is granted for all three patches (attachment 8732017 [details] [diff] [review], https://hg.mozilla.org/integration/mozilla-inbound/rev/7e9d1b43cf76, and attachment 8733668 [details]). If not, we shouldn't uplift any of them.
Attachment #8732017 - Flags: approval-mozilla-beta?
Attachment #8732017 - Flags: approval-mozilla-aurora?
Comment on attachment 8732017 [details] [diff] [review]
rebased.patch

Fix needed for Push notification encrypted message webcompat, Aurora47+
Attachment #8732017 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
HOw can we test or verify these changes? We could uplift to beta, but I'd like to know that someone would be following up. 
Is there anyone else you could ask for extra reviewing/risk assessment, and testing help? Andrew, what do you think of the risks here for breaking things in late beta/release 46?
Flags: needinfo?(overholt)
Flags: needinfo?(kcambridge)
There are additional unit tests in bug 1257821 that use the new encryption scheme. We could also run the tests for Marco's encryption library (https://github.com/marco-c/web-push) against Aurora (and Beta, if both bugs are accepted). I'm happy to follow up and make sure we're compatible with Chrome.
Flags: needinfo?(kcambridge)
(In reply to Kit Cambridge [:kitcambridge] from comment #14)
> We could also run the tests for Marco's encryption library
> (https://github.com/marco-c/web-push) against Aurora (and Beta, if both bugs
> are accepted).

I'm already running tests with Firefox Beta, so if the change is accepted we have coverage. Running them with Aurora is a little harder because it would require changing the test infrastructure to use Marionette.
I defer the stability risk to Kit et al but the web compat risk is pretty large so I'd like to see us fix this ASAP if possible. It sounds like Marco's testing and the tests we have should be sufficient. Maybe we can also get a partner or two to verify, Harald?
Flags: needinfo?(overholt) → needinfo?(hkirschner)
Comment on attachment 8732017 [details] [diff] [review]
rebased.patch

Uplift needed for web compat, let's try to get this into the next beta. 
All 3 patches mentioned in comment 10.
Attachment #8732017 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Kit has one of those 3 patches not landed?
Flags: needinfo?(kcambridge)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #18)
> Kit has one of those 3 patches not landed?

Indeed. :-/ I didn't land attachment 8733668 [details] yet, because landing it on m-i was conditional on getting Aurora and Beta approval for the other two. I'm very sorry for the confusion.
Flags: needinfo?(kcambridge)
Flags: needinfo?(hkirschner)
You need to log in before you can comment on or make changes to this bug.