Closed Bug 1191414 Opened 4 years ago Closed 4 years ago

gather telemetry on usage of <keygen>

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- affected
firefox44 --- fixed

People

(Reporter: keeler, Assigned: kmckinley)

Details

Attachments

(1 file, 4 obsolete files)

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: nobody → kmckinley
Flags: needinfo?(kmckinley)
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.)
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?
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.
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)
(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-
(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.
Attachment #8662655 - Flags: review?(dkeeler)
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+
Attachment #8662655 - Attachment is obsolete: true
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)
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+
Carried over r+ from keeler and vladan
Attachment #8670002 - Flags: review+
Attachment #8668705 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/780ae7350ded
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.