Closed
Bug 1257405
Opened 8 years ago
Closed 8 years ago
Increase auth secret length to 16 octets
Categories
(Core :: DOM: Notifications, defect)
Core
DOM: Notifications
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: lina, Assigned: lina)
References
Details
Attachments
(1 file, 1 obsolete file)
2.03 KB,
patch
|
lina
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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!
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8731523 -
Flags: review?(martin.thomson)
Comment 2•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09900a4952cd
Assignee | ||
Comment 5•8 years ago
|
||
Carrying forward r+.
Attachment #8731523 -
Attachment is obsolete: true
Attachment #8732017 -
Flags: review+
Assignee | ||
Comment 6•8 years ago
|
||
Oops, thanks, RyanVM. This is a tiny change, so it can wait until the next checkin-needed batch.
Keywords: checkin-needed
Comment 7•8 years ago
|
||
Kit, Why don't you ask ötto to land it for you?
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5fe5459a786b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 10•8 years ago
|
||
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?
status-firefox46:
--- → affected
status-firefox47:
--- → affected
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+
Comment 12•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/0d98f9a4b20e
Comment 13•8 years ago
|
||
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)
Assignee | ||
Comment 14•8 years ago
|
||
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)
Comment 15•8 years ago
|
||
(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.
Comment 16•8 years ago
|
||
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 17•8 years ago
|
||
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+
Assignee | ||
Comment 19•8 years ago
|
||
(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)
Comment 20•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/143160fcd2ab
Updated•8 years ago
|
Flags: needinfo?(hkirschner)
You need to log in
before you can comment on or make changes to this bug.
Description
•