1009720 - implement test_mode telemetry

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

Everything is in one big bucket. Also fix previous histogram to use the appropriate type.

Note: this only is useful if absolutely nothing goes wrong, ever.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Telemetry for CERT_PINNING_TEST_RESULTS (

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8421912 [details] [diff] [review]
Telemetry for CERT_PINNING_TEST_RESULTS (

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

This also prefs the default back on, which we could have done yesterday after test mode landed.

Comment 3

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

Comment on attachment 8421912 [details] [diff] [review]
Telemetry for CERT_PINNING_TEST_RESULTS (

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

Looks good to me with comments addressed.

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +201,3 @@
>        PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG,
> +             ("pkpin: Skipping %s test mode pinning check for host: '%s'\n",
> +              evalHost, result ? "passing": "failing"));

I think these arguments are backwards - the host is last, right?

@@ +204,5 @@
> +      retval = true;
> +    } else {
> +      PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG,
> +             ("pkpin: Enforcing %s pinning check for host: '%s'\n",
> +              evalHost, result ? "passing": "failing"));

evalHost last, I think

@@ +205,5 @@
> +    } else {
> +      PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG,
> +             ("pkpin: Enforcing %s pinning check for host: '%s'\n",
> +              evalHost, result ? "passing": "failing"));
> +      Telemetry::Accumulate(Telemetry::CERT_PINNING_RESULTS, result ? 1 : 0);

I'm wondering how this would look if we only had one call to Telemetry::Accumulate (and, similarly, one call to PR_LOG). (i.e. set up a number of parameters that default to the non-test-mode behavior and then change them in the "if (foundEntry->mTestMode)" case).
I don't feel strongly either way, but it might make this a bit more clear.

::: security/manager/ssl/tests/unit/test_pinning.js
@@ +92,5 @@
>  };
>  
>  function check_pinning_telemetry() {
> +  let service = Cc["@mozilla.org/base/telemetry;1"]
> +                   .getService(Ci.nsITelemetry);

nit: indent one less space

@@ +96,5 @@
> +                   .getService(Ci.nsITelemetry);
> +  let prod_histogram = service.getHistogramById("CERT_PINNING_RESULTS")
> +                       .snapshot();
> +  let test_histogram = service.getHistogramById("CERT_PINNING_TEST_RESULTS")
> +                       .snapshot();

nit: I would indent .snapshot(); two more spaces each here

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Telemetry for CERT_PINNING_TEST_RESULTS (

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Telemetry for CERT_PINNING_TEST_RESULTS (

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Fixed, logs look like:

Pin check passed for host 'pinning.example.com' (mode=production)
Pin check failed for host 'pinning.example.com' (mode=test)

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/39916313fa86

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/39916313fa86