1191414 - gather telemetry on usage of <keygen>

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Kate, any interest in another telemetry bug? :)

Comment 2

[Kate McKinley [:kmckinley, :Kate]]

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

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.)

Comment 4

[Kate McKinley [:kmckinley, :Kate]]

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?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 6

[Kate McKinley [:kmckinley, :Kate]]

Attachment: gather telemetry on usage of <keygen>

Gather counts of each time we see keygen actually used in a keyed histogram.

Comment 7

[Vladan Djeric (:vladan)]

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?

Comment 8

[Kate McKinley [:kmckinley, :Kate]]

(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 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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/

Comment 10

[Kate McKinley [:kmckinley, :Kate]]

(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.

Comment 11

[Kate McKinley [:kmckinley, :Kate]]

Attachment: gather telemetry on usage of <keygen>

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 13

[Kate McKinley [:kmckinley, :Kate]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5f8f05863d8

Comment 14

[Kate McKinley [:kmckinley, :Kate]]

Attachment: gather telemetry on usage of <keygen>

Comment 15

[Vladan Djeric (:vladan)]

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?

Comment 16

[Kate McKinley [:kmckinley, :Kate]]

Attachment: gather telemetry on usage of <keygen>

Updated to limit the number of buckets we gather, added an email, reduced time live.

Comment 17

[Kate McKinley [:kmckinley, :Kate]]

Attachment: gather telemetry on usage of <keygen>

Carried over r+ from keeler and vladan

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/780ae7350ded

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/780ae7350ded