gather telemetry on usage of <keygen>

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: keeler, Assigned: kmckinley)

Tracking

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox42 affected, firefox44 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

We should gather some telemetry on how often <keygen> is actually used. Filing in Core :: Security: PSM for now since it's probably someone from seceng who will be implementing this.
Kate, any interest in another telemetry bug? :)
Flags: needinfo?(kmckinley)
(Assignee)

Updated

3 years ago
Assignee: nobody → kmckinley
Flags: needinfo?(kmckinley)
(Assignee)

Comment 2

3 years ago
Add enum telemetry with the following values to nsKeygenHandler.cpp

0=No key generated
1-3=RSA key generated (size>=2048, 2048>size>=1024, 1024>size)
4-6=EC key generated (secp384r1, secp256r1 or secp192r1)
7=DSA key generated
I just saw the announcement that use counters landed ( https://groups.google.com/d/msg/mozilla.dev.platform/ktoEer_cdJA/NfyYRhNmKQAJ ), so maybe we should look into using that framework?
(That said, it would be interesting to know what sizes of keys people are generating.)
(Assignee)

Comment 4

3 years ago
Might as well gather both.

Question: we can bucket key sizes for RSA pretty easily as above, but with EC we would almost prefer to collect the curve, which will make this telemetry harder to read. All told, it is < 200 values, so I'm open to doing either. Thoughts?
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Well, most of those buckets will be 0. That either means we shouldn't bother or it won't make the telemetry harder to read since we can just filter those buckets. I guess more data is better than less, so feel free to go ahead with that.
(Assignee)

Comment 6

3 years ago
Created attachment 8662090 [details] [diff] [review]
gather telemetry on usage of <keygen>

Gather counts of each time we see keygen actually used in a keyed histogram.
Attachment #8662090 - Flags: review?(vladan.bugzilla)
Attachment #8662090 - Flags: review?(dkeeler)
You should really use DOM use-counters to report on <keygen> usage.
You can report algorithm usage in an enum histogram, and the RSA key sizes in a regular exponential histogram.

What are examples of curve strings?
Attachment #8662090 - Flags: review?(vladan.bugzilla)
(Assignee)

Comment 8

3 years ago
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #7)
> You should really use DOM use-counters to report on <keygen> usage.
> You can report algorithm usage in an enum histogram, and the RSA key sizes
> in a regular exponential histogram.
> 
> What are examples of curve strings?

We actually don't care how many times the user is presented with <keygen>, but how many times a key is generated, which happens on form submit. We expect this value to be extremely low.

Curve strings look like: "secp256k1","secp256r1","nistp256","secp384r1","nistp384","secp521r1","nistp521","c2pnb163v1","c2pnb272w1","c2pnb304w1","c2tnb359v1"

Valid curve strings are defined here: http://lxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsKeygenHandler.cpp#135

In practice, we would expect to see only rsa2048, rsa1024, and maybe a small number of other curves, most commonly secp384r1 and secp256r1. We expect most of the buckets to be empty, so an enumerated bucket for the algorithms would contain mostly empty buckets.
Comment on attachment 8662090 [details] [diff] [review]
gather telemetry on usage of <keygen>

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

This is good, but there are few items that need addressing. So r- for now, but this is almost ready to go.

::: security/manager/ssl/nsKeygenHandler.cpp
@@ -132,5 @@
>      SECOidTag curveOidTag;
>  } CurveNameTagPair;
>  
>  static CurveNameTagPair nameTagPair[] =
> -{ 

The trailing whitespace fixups are nice, but can distract from what's important in a review. Feel free to have a whitespace-only patch and then another patch for the functionality changes. Or just don't bother, since hopefully we'll remove this entire file in a year :)

@@ +465,5 @@
>        }
>        return rv;
>  }
>  
> +

nit: unnecessary extra blank line

@@ +467,5 @@
>  }
>  
> +
> +void
> +GatherKeygenTelemetry(uint32_t keyGenMechanism, int keysize, char *curve)

nit: for new code, 'char* curve' instead

@@ +478,5 @@
> +    nsCString telemetryValue("rsa");
> +    telemetryValue.AppendPrintf("%d", keysize);
> +    mozilla::Telemetry::Accumulate(
> +        mozilla::Telemetry::KEYGEN_GENERATED_KEY_TYPE, telemetryValue);
> +  } else if (keyGenMechanism == CKM_EC_KEY_PAIR_GEN) {

What about CKM_DSA_KEY_PAIR_GEN? nsKeygenFormProcessor::GetPublicKey acts like it's possible that a DSA key may be requested, so we should probably handle that too.

@@ +479,5 @@
> +    telemetryValue.AppendPrintf("%d", keysize);
> +    mozilla::Telemetry::Accumulate(
> +        mozilla::Telemetry::KEYGEN_GENERATED_KEY_TYPE, telemetryValue);
> +  } else if (keyGenMechanism == CKM_EC_KEY_PAIR_GEN) {
> +    if (decode_ec_params(curve) == nullptr) {

Looks like decode_ec_params allocates memory and returns it, so this will leak if we don't keep a pointer to it and SECITEM_FreeItem it if it's non-null.
Also, the current style for null-checks is "if (!ptr) { ... }"

@@ +481,5 @@
> +        mozilla::Telemetry::KEYGEN_GENERATED_KEY_TYPE, telemetryValue);
> +  } else if (keyGenMechanism == CKM_EC_KEY_PAIR_GEN) {
> +    if (decode_ec_params(curve) == nullptr) {
> +      switch (keysize) {
> +      case 2048:

Looks like the current style for switch statements is to indent each 'case ...:' two spaces and each case body another two spaces.

@@ +488,5 @@
> +          break;
> +      case 1024:
> +      case 512:
> +          mozilla::Telemetry::Accumulate(
> +              mozilla::Telemetry::KEYGEN_GENERATED_KEY_TYPE, nsCString("secp256r1"));

I think we want to use NS_LITERAL_CSTRING("...") instead of nsCString("...") for these.

::: toolkit/components/telemetry/Histograms.json
@@ +1368,5 @@
>      "n_values": 16,
>      "description": "SSL Handshake Key Exchange Algorithm for resumed handshake (null=0, rsa=1, dh=2, fortezza=3, ecdh=4)"
>    },
> +  "KEYGEN_GENERATED_KEY_TYPE": {
> +    "expires_in_version": "48",

What's the maximum we can put on this? This will probably be a long-running telemetry measurement.

@@ +1371,5 @@
> +  "KEYGEN_GENERATED_KEY_TYPE": {
> +    "expires_in_version": "48",
> +    "kind": "count",
> +    "keyed": "true",
> +    "description": "They number of times we generate a key via keygen, keyed on algorithm and keysize."

nit: s/They/The/
Attachment #8662090 - Flags: review?(dkeeler) → review-
(Assignee)

Comment 10

3 years ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #9)
> Comment on attachment 8662090 [details] [diff] [review]
> gather telemetry on usage of <keygen>
> 
> Review of attachment 8662090 [details] [diff] [review]:
> -----------------------------------------------------------------

> 
> @@ +478,5 @@
> > +    nsCString telemetryValue("rsa");
> > +    telemetryValue.AppendPrintf("%d", keysize);
> > +    mozilla::Telemetry::Accumulate(
> > +        mozilla::Telemetry::KEYGEN_GENERATED_KEY_TYPE, telemetryValue);
> > +  } else if (keyGenMechanism == CKM_EC_KEY_PAIR_GEN) {
> 
> What about CKM_DSA_KEY_PAIR_GEN? nsKeygenFormProcessor::GetPublicKey acts
> like it's possible that a DSA key may be requested, so we should probably
> handle that too.

I left DSA out since we actually don't generate DSA keys. Currently, this only reports on successfully generating a key.
(Assignee)

Comment 11

3 years ago
Created attachment 8662655 [details] [diff] [review]
gather telemetry on usage of <keygen>
Attachment #8662655 - Flags: review?(dkeeler)
(Assignee)

Updated

3 years ago
Attachment #8662090 - Attachment is obsolete: true
Comment on attachment 8662655 [details] [diff] [review]
gather telemetry on usage of <keygen>

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

Great - r=me with nits addressed.

::: security/manager/ssl/nsKeygenHandler.cpp
@@ +468,5 @@
>  }
>  
> +
> +void
> +GatherKeygenTelemetry(uint32_t keyGenMechanism, int keysize, char *curve)

nit: 'char* curve'

@@ +480,5 @@
> +    telemetryValue.AppendPrintf("%d", keysize);
> +    mozilla::Telemetry::Accumulate(
> +        mozilla::Telemetry::KEYGEN_GENERATED_KEY_TYPE, telemetryValue);
> +  } else if (keyGenMechanism == CKM_EC_KEY_PAIR_GEN) {
> +    SECKEYECParams * decoded = decode_ec_params(curve);

nit: 'SECKEYECParams* decoded'

@@ +499,5 @@
> +      mozilla::Telemetry::Accumulate(
> +          mozilla::Telemetry::KEYGEN_GENERATED_KEY_TYPE, nsCString(curve));
> +    }
> +  } else if (keyGenMechanism == CKM_DSA_KEY_PAIR_GEN) {
> +    // We currently don't generate DSA keys, so we should never reach this

We should maybe MOZ_CRASH("DSA key generation is unsupported"); or something here.
Attachment #8662655 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 14

3 years ago
Created attachment 8662694 [details] [diff] [review]
gather telemetry on usage of <keygen>
(Assignee)

Updated

3 years ago
Attachment #8662655 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8662694 - Flags: review?(vladan.bugzilla)
Comment on attachment 8662694 [details] [diff] [review]
gather telemetry on usage of <keygen>

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

::: toolkit/components/telemetry/Histograms.json
@@ +1368,5 @@
>      "n_values": 16,
>      "description": "SSL Handshake Key Exchange Algorithm for resumed handshake (null=0, rsa=1, dh=2, fortezza=3, ecdh=4)"
>    },
> +  "KEYGEN_GENERATED_KEY_TYPE": {
> +    "expires_in_version": "56",

This is about 2 years in the future. We are accumulating probes very quickly and most of them are not being monitored. So it's a bit of a potential privacy & performance issue.
Can you make it expire in 50?
Also, add an alert_emails field so that sudden changes in the distribution trigger email alerts (once we add alerting support for keyed histograms).

@@ +1370,5 @@
>    },
> +  "KEYGEN_GENERATED_KEY_TYPE": {
> +    "expires_in_version": "56",
> +    "kind": "count",
> +    "keyed": "true",

Are you expecting a lot of different keys? If so, can you report the atypical keys under one key e.g. "other"? Our backend database doesn't handle more than a few dozen keys well.
If you need to collect a wide variety of "arbitrary" strings, then a histogram probe is not the right approach.

@@ +1371,5 @@
> +  "KEYGEN_GENERATED_KEY_TYPE": {
> +    "expires_in_version": "56",
> +    "kind": "count",
> +    "keyed": "true",
> +    "description": "The number of times we generate a key via keygen, keyed on algorithm and keysize."

Are you ok with collecting this data on an opt-in basis on the Release channel?
Attachment #8662694 - Flags: review?(vladan.bugzilla)
(Assignee)

Comment 16

3 years ago
Created attachment 8668705 [details] [diff] [review]
gather telemetry on usage of <keygen>

Updated to limit the number of buckets we gather, added an email, reduced time live.
Attachment #8662694 - Attachment is obsolete: true
Attachment #8668705 - Flags: review?(vladan.bugzilla)
Attachment #8668705 - Attachment is patch: true
Attachment #8668705 - Flags: review?(vladan.bugzilla) → review+
(Assignee)

Comment 17

3 years ago
Created attachment 8670002 [details] [diff] [review]
gather telemetry on usage of <keygen>

Carried over r+ from keeler and vladan
Attachment #8670002 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8668705 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/780ae7350ded
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.