Closed Bug 1020598 Opened 5 years ago Closed 5 years ago

Add telemetry to measure WebCrypto usage

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: rbarnes, Assigned: rbarnes)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch webcrypto-1020598.patch (obsolete) — Splinter Review
Adds measurements for a whole bunch of WebCrypto properties:
* Which methods are used
* Which algorithms are used
* Whether keys are extractable
* Whether operations succeed or fail (and how they fail)
Attachment #8434534 - Flags: review?(bzbarsky)
Attached patch webcrypto-1020598.1.patch (obsolete) — Splinter Review
Missed a Telemetry::Accumulate() call.
Attachment #8434534 - Attachment is obsolete: true
Attachment #8434534 - Flags: review?(bzbarsky)
Attachment #8434623 - Flags: review?(bzbarsky)
Richard, I'm pretty behind on reviews right now, and not that familiar with the telemetry APIs.  I think you can probably get a faster and better review from someone who is familiar with those...
Attachment #8434623 - Flags: review?(bzbarsky) → review?(vdjeric)
Comment on attachment 8434623 [details] [diff] [review]
webcrypto-1020598.1.patch

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

Is this something that's going to be used for a long time? Otherwise set the expiration field to a specific version.

::: dom/crypto/WebCryptoTask.cpp
@@ +26,4 @@
>  namespace mozilla {
>  namespace dom {
>  
> +// Pre-defined identifiers for telemetry histograms

you could use an enum here instead

::: toolkit/components/telemetry/Histograms.json
@@ +6183,5 @@
> +  "WEBCRYPTO_EXTRACTABLE_IMPORT": {
> +    "expires_in_version": "never",
> +    "kind": "boolean",
> +    "description": "Whether an imported key was marked as extractable"
> +  },

Do you want to count the number of times this was done in a session or do you just want to report whether it was used or not used in a session? If it's the latter, a flag histogram would be preferable

https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe#Choosing_a_Histogram_Type

@@ +6209,5 @@
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 6,
> +    "description": "Errors returned by WebCrypto (NS_ERROR_DOM_*_ERR; 0=NOT_SUPPORTED, 1=SYNTAX, 2=DATA, 3=OPERATION, 4=UNKNOWN, 5=other"
> +  },

Are you planning to do custom Telemetry queries for this data? I wonder if the default histogram view on telemetry.mozilla.org is going to be useful for these histograms. 

For example, with this WEBCRYPTO_ERROR histogram, telemetry.mozilla.org would show you 1) the daily submission counts in the top graph, and 2) the relative distribution of these errors with respect to each other in the histogram on the bottom, with each occurrence of an error weighted equally.

If you're interested in per-session or per-webpage or per-domain statistics, then you should implement the data gathering for these histograms differently.

@@ +6215,5 @@
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 12,
> +    "description": "Methods invoked under window.crypto.subtle (0=encrypt, 1=decrypt, 2=sign, 3=verify, 4=digest, 5=generateKey, 6=deriveKey, 7=deriveBits, 8=importKey, 9=exportKey, 10=wrapKey, 11=unwrapKey)"
> +  },

You should reserve a few extra values if there's a chance you'll want to add new enum values to this histogram. We don't redefine histograms -- if the definition of a histogram needs to change, we just create a new histogram.
Attachment #8434623 - Flags: review?(vdjeric)
Attached patch webcrypto-1020598.2.patch (obsolete) — Splinter Review
(In reply to Vladan Djeric (:vladan) from comment #5)
> Comment on attachment 8434623 [details] [diff] [review]
> webcrypto-1020598.1.patch
> 
> Review of attachment 8434623 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this something that's going to be used for a long time? Otherwise set the
> expiration field to a specific version.

I think we're going to want to track usage of these feature for the forseeable future, so I'm going to leave this.

> ::: dom/crypto/WebCryptoTask.cpp
> @@ +26,4 @@
> >  namespace mozilla {
> >  namespace dom {
> >  
> > +// Pre-defined identifiers for telemetry histograms
> 
> you could use an enum here instead

Done.


> ::: toolkit/components/telemetry/Histograms.json
> @@ +6183,5 @@
> > +  "WEBCRYPTO_EXTRACTABLE_IMPORT": {
> > +    "expires_in_version": "never",
> > +    "kind": "boolean",
> > +    "description": "Whether an imported key was marked as extractable"
> > +  },
> 
> Do you want to count the number of times this was done in a session or do
> you just want to report whether it was used or not used in a session? If
> it's the latter, a flag histogram would be preferable
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Performance/
> Adding_a_new_Telemetry_probe#Choosing_a_Histogram_Type

This is about tracking use of the "extractable" feature, which you do at one stage by setting a boolean when you import/generate a key, and at another when you use the key.  In other words, with these measurements, we can gauge how common it is that keys are extractable or not, and how much use they get relative to each other.  Separating import from generation is relevant because only generated keys exist entirely inside the browser.  Likewise, there are different use cases for encryption and signing with non-extractable keys, so we want to measure them separately.


> @@ +6209,5 @@
> > +    "expires_in_version": "never",
> > +    "kind": "enumerated",
> > +    "n_values": 6,
> > +    "description": "Errors returned by WebCrypto (NS_ERROR_DOM_*_ERR; 0=NOT_SUPPORTED, 1=SYNTAX, 2=DATA, 3=OPERATION, 4=UNKNOWN, 5=other"
> > +  },
> 
> Are you planning to do custom Telemetry queries for this data? I wonder if
> the default histogram view on telemetry.mozilla.org is going to be useful
> for these histograms. 
> 
> For example, with this WEBCRYPTO_ERROR histogram, telemetry.mozilla.org
> would show you 1) the daily submission counts in the top graph, and 2) the
> relative distribution of these errors with respect to each other in the
> histogram on the bottom, with each occurrence of an error weighted equally.
> 
> If you're interested in per-session or per-webpage or per-domain statistics,
> then you should implement the data gathering for these histograms
> differently.

On thinking about it, I don't have an immediate use case, so I've removed this measurement.


> @@ +6215,5 @@
> > +    "expires_in_version": "never",
> > +    "kind": "enumerated",
> > +    "n_values": 12,
> > +    "description": "Methods invoked under window.crypto.subtle (0=encrypt, 1=decrypt, 2=sign, 3=verify, 4=digest, 5=generateKey, 6=deriveKey, 7=deriveBits, 8=importKey, 9=exportKey, 10=wrapKey, 11=unwrapKey)"
> > +  },
> 
> You should reserve a few extra values if there's a chance you'll want to add
> new enum values to this histogram. We don't redefine histograms -- if the
> definition of a histogram needs to change, we just create a new histogram.

I'll do you one better :)  I just collapsed all these algorithms into a single one, and made it an enum, and gave it some room.
Attachment #8434623 - Attachment is obsolete: true
Attachment #8436387 - Flags: review?(vdjeric)
Comment on attachment 8436387 [details] [diff] [review]
webcrypto-1020598.2.patch

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

(In reply to Richard Barnes [:rbarnes] from comment #6)
> > ::: toolkit/components/telemetry/Histograms.json
> > @@ +6183,5 @@
> > > +  "WEBCRYPTO_EXTRACTABLE_IMPORT": {
> > > +    "expires_in_version": "never",
> > > +    "kind": "boolean",
> > > +    "description": "Whether an imported key was marked as extractable"
> > > +  },
> > 
> > Do you want to count the number of times this was done in a session or do
> > you just want to report whether it was used or not used in a session? If
> > it's the latter, a flag histogram would be preferable
> > 
> 
> This is about tracking use of the "extractable" feature, which you do at one
> stage by setting a boolean when you import/generate a key, and at another
> when you use the key.  In other words, with these measurements, we can gauge
> how common it is that keys are extractable or not, and how much use they get
> relative to each other.  Separating import from generation is relevant
> because only generated keys exist entirely inside the browser.  Likewise,
> there are different use cases for encryption and signing with
> non-extractable keys, so we want to measure them separately.

My question was about what the numbers collected will represent. With your current implementation, you collect a sample every time the "extractable" feature is used or not used so the data you collect could be misleading, e.g. one out of ten popular WebCrypto sites always uses the "extractable" feature when importing a key but it imports many keys so that single site has a lot of undue weight in the final tally. If you were to collect the data per-site or per-session it might be more meaningful, although admittedly trickier to implement.
I'm fine with the probe being implemented this way, I'm just pointing out a potential pitfall.

> > You should reserve a few extra values if there's a chance you'll want to add
> > new enum values to this histogram. We don't redefine histograms -- if the
> > definition of a histogram needs to change, we just create a new histogram.
> 
> I'll do you one better :)  I just collapsed all these algorithms into a
> single one, and made it an enum, and gave it some room.

n_values for WEBCRYPTO_METHOD is still 12. Are you sure there won't be additional methods in the future? It wouldn't hurt to leave extra room just in case

Also, please note I reviewed the Telemetry bits of the patch, I don't know anything about WebCrypto implementation
Attachment #8436387 - Flags: review?(vdjeric) → review+
Raised n_values on WEBCRYPTO_METHOD, just in case.
Attachment #8436387 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 8437440 [details] [diff] [review]
webcrypto-1020598.3.patch r=vdjeric

[Triage Comment]
Safe fix - We should have telemetry data for WebCrypto
Attachment #8437440 - Flags: approval-mozilla-aurora+
Comment on attachment 8437440 [details] [diff] [review]
webcrypto-1020598.3.patch r=vdjeric

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

hmm. this hasn't landed on m-c yet.  We should get m-a+ after it lands.
Attachment #8437440 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1585beade496
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Attachment #8437440 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.