Closed
Bug 1342090
Opened 7 years ago
Closed 7 years ago
Update SSL_HANDSHAKE_RESULT to opt-out on release
Categories
(Core :: Security: PSM, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: ekr, Assigned: ekr)
Details
Attachments
(1 file)
1.19 KB,
patch
|
benjamin
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This is a basic piece of health information about the browser, and release has a very different population, so I would like to make it opt-out. The information it reports is just the frequency of different types of SSL/TLS errors, but not the details of the errors themselves. E.g., it tells you that you have a certificate error of type X but not the certificate you saw.
Attachment #8840461 -
Flags: review?(benjamin)
Assignee | ||
Updated•7 years ago
|
Summary: Update SSL_HANDSHAKE_VERSION to opt-out on release → Update SSL_HANDSHAKE_RESULT to opt-out on release
Updated•7 years ago
|
Assignee: nobody → ekr
Priority: -- → P3
Updated•7 years ago
|
Component: Telemetry → Security: PSM
Product: Toolkit → Core
Assignee | ||
Comment 1•7 years ago
|
||
Benjamin, Sorry if I put this in the wrong component. I assumed since it was just a change to Histograms.json it was in telemetry. In any case, it needs your review :)
Comment 2•7 years ago
|
||
Comment on attachment 8840461 [details] [diff] [review] telemetry.diff Do we have automation to make sure this metric still works as expected? That is typically requested as risk mitigation of opt-out probes. Also who exactly is committing to use this data? ekr is that you personally, or is that a team (or is there a PM?)
Flags: needinfo?(ekr)
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #2) > Comment on attachment 8840461 [details] [diff] [review] > telemetry.diff > > Do we have automation to make sure this metric still works as expected? That > is typically requested as risk mitigation of opt-out probes. Can you elaborate on this? We don't have any, but I'm happy to write some if I have a better sense of what is needed. > Also who exactly is committing to use this data? ekr is that you personally, > or is that a team (or is there a PM?) Me like pretty much the day it ships. We are trying to understand why we are seeing increased errors on TLS 1.3 and this is for diagnosing that.
Flags: needinfo?(ekr)
Comment 4•7 years ago
|
||
Comment on attachment 8840461 [details] [diff] [review] telemetry.diff data-r=me for now and let's do a followup for tests. Expect me to ping you every 6 months or so to make sure you're still using this. A typical telemetry test progressively runs a bunch of actions that should trigger telemetry and then checks that the histogram records what we expect. https://reviewboard.mozilla.org/r/78732/diff/7#index_header and bug 1301452 has an example of a test for tab-switch-spinner probes.
Attachment #8840461 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•7 years ago
|
Whiteboard: checkin-needed
Assignee | ||
Comment 5•7 years ago
|
||
Comment on attachment 8840461 [details] [diff] [review] telemetry.diff Approval Request Comment [Feature/Bug causing the regression]: Final patch in sequence for reasonable TLS telemetry [User impact if declined]: Bad visibility on TLS 1.3 [Is this code covered by automated tests?]: No. [Has the fix been verified in Nightly?]: No. I built it and verified that it didn't break stuff and then marked it checkin-needed. It's just a telemetry setting, but if we wanted we could miss b9 and drop it for RC1. This seems like a judgement call, but I do want it on FF52. [Needs manual test from QE? If yes, steps to reproduce]: Do a build with release settings and make sure that SSL_HANDSHAKE_RESULT gets sent in telemetry. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: Minimally. [Why is the change risky/not risky?]: Telemetry setting frob [String changes made/needed]:
Flags: needinfo?(jcristau)
Attachment #8840461 -
Flags: approval-mozilla-beta?
Attachment #8840461 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 6•7 years ago
|
||
Thanks Benjamin. I filed this as: 1342090
Comment 7•7 years ago
|
||
Comment on attachment 8840461 [details] [diff] [review] telemetry.diff Improvements for tracking TLS 1.3 issues, let's uplift now. This may or may not make today's build, but it should make it ok to the RC build on Monday (though if it misses then we need to uplift to m-r)
Attachment #8840461 -
Flags: approval-mozilla-beta?
Attachment #8840461 -
Flags: approval-mozilla-beta+
Attachment #8840461 -
Flags: approval-mozilla-aurora?
Attachment #8840461 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 8•7 years ago
|
||
Thank you everyone!
Comment 9•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/a09f50f513b8
status-firefox53:
--- → fixed
Whiteboard: checkin-needed
Comment 10•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3a585de719c2 Upgrade SSL_HANDSHAKE_RESULT to opt-out on release. r=bsmedberg
Comment 11•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e54150339ae6
status-firefox52:
--- → fixed
Updated•7 years ago
|
Flags: needinfo?(jcristau)
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3a585de719c2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 13•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/e54150339ae6
status-firefox-esr52:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•