Closed Bug 1311893 Opened 4 years ago Closed 4 years ago

Collect telemetry on timing of HSTS priming requests

Categories

(Core :: DOM: Security, defect)

51 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: kmckinley, Assigned: kmckinley)

Details

Attachments

(1 file)

In the presence of HTTPS timeouts, for example when a firewall drops packets to port 443 instead of sending RST, the first priming request times out after 30 seconds before the load can continue. The following telemetry will be added to measure the time of the HSTS priming request, not including the actual load.

HSTS_PRIMING_SUCCESS_TIMING - how long successful HSTS priming requests take
HSTS_PRIMING_FAILURE_TIMING - how long failed HSTS priming requests take
Change to use a keyed histogram instead of 2 histograms.
Comment on attachment 8803226 [details]
Bug 1311893 - HSTS Priming timing telemetry  p=francois

Francois, can you take a look at this for privacy / data collection review?
Attachment #8803226 - Flags: feedback?(francois)
Comment on attachment 8803226 [details]
Bug 1311893 - HSTS Priming timing telemetry  p=francois

https://reviewboard.mozilla.org/r/87536/#review86510

lgtm, the nit is totally up to you to incorporate or not. I can't say anything about the low and high buckets though, but I suppose francois will look at that anyway. Code looks good, r=me

::: netwerk/protocol/http/HSTSPrimerListener.cpp:51
(Diff revision 2)
> +    TimeStamp channelCreationTime;
> +    nsresult rv = timingChannel->GetChannelCreation(&channelCreationTime);
> +    if (NS_SUCCEEDED(primingResult) && !channelCreationTime.IsNull()) {
> +      PRUint32 interval = (PRUint32)
> +        (TimeStamp::Now() - channelCreationTime)
> +        .ToMilliseconds();

nit: I would prefer
PRUint32 interval =
  (PRUint32)(TimeStamp::Now() - channelCreationTime).ToMilliseconds();
Attachment #8803226 - Flags: review?(ckerschb) → review+
Attachment #8803226 - Flags: feedback?(francois)
Comment on attachment 8803226 [details]
Bug 1311893 - HSTS Priming timing telemetry  p=francois

https://reviewboard.mozilla.org/r/87536/#review87376

::: netwerk/protocol/http/HSTSPrimerListener.cpp:48
(Diff revision 2)
> -  if (NS_FAILED(rv)) {
> +  nsCOMPtr<nsITimedChannel> timingChannel =
> +    do_QueryInterface(callback);
> +  if (timingChannel) {
> +    TimeStamp channelCreationTime;
> +    nsresult rv = timingChannel->GetChannelCreation(&channelCreationTime);
> +    if (NS_SUCCEEDED(primingResult) && !channelCreationTime.IsNull()) {

Shouldn't we check for `NS_SUCCEEDED(rv)` here?

::: netwerk/protocol/http/HSTSPrimerListener.cpp:53
(Diff revision 2)
> +    if (NS_SUCCEEDED(primingResult) && !channelCreationTime.IsNull()) {
> +      PRUint32 interval = (PRUint32)
> +        (TimeStamp::Now() - channelCreationTime)
> +        .ToMilliseconds();
> +      Telemetry::Accumulate(Telemetry::HSTS_PRIMING_REQUEST_DURATION,
> +          (NS_SUCCEEDED(rv)) ? NS_LITERAL_CSTRING("success")

`rv` here is the result of `GetChannelCreation()`. Is that the right value to check? I would have thought you wanted to look at `primingResult` instead.

::: toolkit/components/telemetry/Histograms.json:7800
(Diff revision 2)
>      "kind": "enumerated",
>      "n_values": 16,
>      "description": "How often do we get back an HSTS priming result which upgrades the connection to HTTPS? 0=cached (no upgrade), 1=cached (do upgrade), 2=cached (blocked), 3=already upgraded, 4=priming succeeded, 5=priming succeeded (block due to pref), 6=priming succeeded (no upgrade due to pref), 7=priming failed (block), 8=priming failed (accept)"
>    },
> +  "HSTS_PRIMING_REQUEST_DURATION": {
> +    "alert_emails": ["seceng@mozilla.org"],

We have a separate alias for telemetry: seceng-telemetry@mozilla.com

::: toolkit/components/telemetry/Histograms.json:7802
(Diff revision 2)
>      "description": "How often do we get back an HSTS priming result which upgrades the connection to HTTPS? 0=cached (no upgrade), 1=cached (do upgrade), 2=cached (blocked), 3=already upgraded, 4=priming succeeded, 5=priming succeeded (block due to pref), 6=priming succeeded (no upgrade due to pref), 7=priming failed (block), 8=priming failed (accept)"
>    },
> +  "HSTS_PRIMING_REQUEST_DURATION": {
> +    "alert_emails": ["seceng@mozilla.org"],
> +    "bug_numbers": [1311893],
> +    "expires_in_version": "61",

If it's an exploratory measurement, we usually ask for it to be limited to 6 releases (so Fx 58 in this case). You can always extend it later if you're still using it.

::: toolkit/components/telemetry/Histograms.json:7808
(Diff revision 2)
> +    "kind": "exponential",
> +    "low": 100,
> +    "high": 30000,
> +    "n_buckets": 100,
> +    "keyed": true,
> +    "description": "The amount of time required for HSTS priming requests (ms), keyed by success or failure of the priming request. (success, fail)"

The key for failed priming attempts is "failure" in the code, not "fail".
Attachment #8803226 - Flags: review-
Attachment #8803226 - Flags: feedback?(francois)
Attachment #8803226 - Flags: feedback?(francois)
Comment on attachment 8803226 [details]
Bug 1311893 - HSTS Priming timing telemetry  p=francois

https://reviewboard.mozilla.org/r/87536/#review87552

Looks good. datareview+ and r+.
Attachment #8803226 - Flags: review+
Attachment #8803226 - Flags: feedback?(francois)
Pushed by kmckinley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2aa6df19f5f2
HSTS Priming timing telemetry r=ckerschb,francois p=francois
https://hg.mozilla.org/mozilla-central/rev/2aa6df19f5f2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.