Might be good to track occurrences of TLS version intolerance fallback causes the server to send an inappropriate_fallback alert. The simplest solution is to increment a single counter each time the alert is received. A more thorough solution might include separate counters for each of the reasons, so that we can track what types of errors cause inappropriate fallbacks.

:keeler, do you have any opinion on whether the more detailed information would help? We'd have to modify the intolerance store to track the error code that triggered the version fallback. The upside of doing the more complex option is that, tracking the original error would allow be necessary for bug 1075167 (fixing error reporting) if we went there.

I've taken the high road here which should allow us to report the reason when TLS version intolerance is falsely detected. This actually includes unit tests :) The next one won't :(

I'm a little sad about how big this diff is, but it was the only way to ensure that the telemetry buckets are consistent between the various version-fallback options. Tested using about:telemetry.

Comment on attachment 8498481 [details] [diff] [review] 0001-Bug-1075991-Remember-version-intolerance-reason-code.patch Review of attachment 8498481 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +878,5 @@ > if (entry.intolerant != 0) { > // We've tried connecting at a higher range but failed, so try at the > // version we haven't tried yet, unless we have reached the minimum. > if (range.min < entry.intolerant) { > range.max = entry.intolerant - 1; Do we need to do anything with the intoleranceReason in adjustForTLSIntolerance here?

Comment on attachment 8498482 [details] [diff] [review] 0002-Bug-1075991-Tracking-cause-of-inappropriate-TLS-vers.patch Review of attachment 8498482 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +1090,5 @@ > + // same buckets as the telemetry below, except that bucket 0 will include > + // all cases where there wasn't an original reason. > + PRErrorCode originalReason = socketInfo->SharedState().IOLayerHelpers() > + .getIntoleranceReason(socketInfo->GetHostName(), > + socketInfo->GetPort()); nit: seems like this line could go on the end of the previous one

(In reply to David Keeler (:keeler) [use needinfo?] from comment #5) > > if (range.min < entry.intolerant) { > > range.max = entry.intolerant - 1; > > Do we need to do anything with the intoleranceReason in > adjustForTLSIntolerance here? Not especially. The things that remember things set multiple things, but when we get it out again, there are two separate ways to access the data. This uses the version information only; the "why" isn't needed right here.

Rebased, carrying r=keeler

The line just fit :) r=keeler

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a9b31ab26e https://hg.mozilla.org/integration/mozilla-inbound/rev/59ef591ec318

https://hg.mozilla.org/mozilla-central/rev/b1a9b31ab26e https://hg.mozilla.org/mozilla-central/rev/59ef591ec318