Wrong description in telemetry dashboard regarding SSL_KEA_ECDHE_CURVE_FULL

RESOLVED FIXED in mozilla38

Status

()

Toolkit
Telemetry
--
minor
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: David Bar, Assigned: Cykesiopka)

Tracking

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.104 Safari/537.36

Steps to reproduce:

Went to the following URL

http://telemetry.mozilla.org/#filter=release%2F32%2FSSL_KEA_ECDHE_CURVE_FULL&aggregates=multiselect-all!Submissions&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Table

(My apologies if this bug is not opened on the correct product, I'm not sure what's the product of the Telemetry Dashboard.)


Actual results:

The description of the histogram as it appears in the page is:
ECDHE KEA (TLS_ECDHE_*) curve (1=P-256, 2=P-384, 3=P-521) in full handshake

This is not consistent with the code, and not consistent with the histogram displayed.
The code which actually increments the stats is in 
source/security/manager/ssl/src/nsNSSCallbacks.cpp AccumulateECCCurve()
It uses the number of bits of the key (256, 384, 521), and increments 23, 24, 25 in the telemetry stats, not 1, 2 or 3.


Expected results:

The description of the telemetry field should be something like:
ECDHE KEA (TLS_ECDHE_*) curve (23=P-256, 24=P-384, 25=P-521) in full handshake

The fix should be done in
source/toolkit/components/telemetry/Histograms.json
around the line of SSL_KEA_ECDHE_CURVE_FULL
(Reporter)

Updated

3 years ago
Severity: normal → minor

Comment 1

3 years ago
Brian, can you help here? :-)
Component: Untriaged → Telemetry
Flags: needinfo?(brian)
OS: Windows 7 → All
Product: Firefox → Toolkit
Hardware: x86_64 → All
I agree.
Flags: needinfo?(brian)
(Assignee)

Comment 3

3 years ago
Created attachment 8554112 [details] [diff] [review]
bug1089588_telemetry-SSL_KEA_ECDHE_CURVE_FULL-fix-desc_v1.patch
Assignee: nobody → cykesiopka.bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8554112 - Flags: review?(dkeeler)
Comment on attachment 8554112 [details] [diff] [review]
bug1089588_telemetry-SSL_KEA_ECDHE_CURVE_FULL-fix-desc_v1.patch

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

SSL_AUTH_ECDSA_CURVE_FULL needs a similar change, looks like. r=me with that. Thanks!
Attachment #8554112 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 5

3 years ago
Created attachment 8554817 [details] [diff] [review]
bug1089588_telemetry-SSL_KEA_ECDHE_CURVE_FULL-SSL_AUTH_ECDSA_CURVE_FULL-fix-desc_v2.patch

+ Make change to SSL_AUTH_ECDSA_CURVE_FULL as well
Attachment #8554112 - Attachment is obsolete: true
Attachment #8554817 - Flags: review+
(Assignee)

Comment 6

3 years ago
Thanks for the review.

Try push (for v1, but the difference is minor): https://tbpl.mozilla.org/?tree=Try&rev=19b106ae2985
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/a910bc402adc
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a910bc402adc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.