942728 - Fix telemetry for cipher suites and crypto algorithms and key sizes

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: fix-telemetry.patch

The telemetry added in bug 707275, that was uplifted to mozilla-aurora and mozilla-beta, isn't providing information as useful as it should be.

In particular, the cipher suite telemetry is in the false start callback. However, that callback is never called because false start is disabled.

It is important that we get this telemetry fixed in mozilla-beta and mozilla-aurora because this telemetry is being used to justify the changes we're making or have already made in mozilla-aurora.

Note that we collect some of the cipher suite telemetry separately for full handshakes and resumed handshakes. This is because I want to explore some correlations between cipher suite and session resumption--in particular, whether we can predict whether a server will support session resumption based on the cipher suite.

Also note that we only collect some information during full handshakes, because that is where that information is relevant. For example, we don't really need to collect certificate key size telemetry for resumption handshakes, because the certificate's key is not used during a session resumption.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: [NOT FOR CHECKIN] fix-telemetry-no-ws-changes.patch

David, here is the "diff -w" version of the patch, which will be easier to review because I changed the indention of quite a bit of the code in the patch.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This bug doesn't cause users any trouble, so it isn't an absolute must-have. However, having this telemetry in Firefox 26 is very important for us to make decisions for Firefox 27 and Firefox 28, so I'm adding the tracking flags. I will request the uplift as soon as the patch lands in m-i. It is a very, very safe patch, because it is just shuffling some telemetry around.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8337594 [details] [diff] [review]
[NOT FOR CHECKIN] fix-telemetry-no-ws-changes.patch

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

This appears to do what you intend, as far as I can tell.
It looks like the patch that landed for bug 707275 didn't actually match the patch I reviewed - what happened there?

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

If this can get uplift today and be in beta 9 that would be great, putting it in the last beta (and the release build) without any user facing time is more risky. Brian you can uplift to mozilla-beta and flag me for a+ based on the low risk you state in comment 2.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to [e-mail] [:lsblakk] from comment #4)
> If this can get uplift today and be in beta 9 that would be great, putting
> it in the last beta (and the release build) without any user facing time is
> more risky. Brian you can uplift to mozilla-beta and flag me for a+ based on
> the low risk you state in comment 2.

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfd3a8087c26
https://hg.mozilla.org/releases/mozilla-aurora/rev/6034648945f3
https://hg.mozilla.org/releases/mozilla-beta/rev/43cd7d6a3970

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) from comment #3)
> It looks like the patch that landed for bug 707275 didn't actually match the
> patch I reviewed - what happened there?

<briansmith> keeler: what was the difference between the landed patch and the reviewed patch that you were wondering about?
<keeler> briansmith: the patch I reviewed had that code in HandshakeCallback, not CanFalseStartCallback
<briansmith> keeler: it seems like it was a bad merge, on my part.
<briansmith> I think I searched for the first place that called SSL_GetCipherSuiteInfo and didn't realize that I was merging into the wrong spot.
<keeler> briansmith: ah, I see.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/43cd7d6a3970

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dfd3a8087c26