951315 - Add telemetry to PK pinning

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Camilo Viecco (:cviecco)]

before enabling pinning, we should have telemetry to help us detect how much failure rates we will be starting to observe once this is enabled.

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: pinning-telemetry

Comment 2

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

Comment on attachment 8415676 [details] [diff] [review]
pinning-telemetry

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

This can now be unittested in the test_pinning unittest. It might even be useful distinguishing between pinning enforcement levels disabled and mitm.

::: toolkit/components/telemetry/Histograms.json
@@ +5920,5 @@
> +  "PINNING_EVALUATION_RESULTS": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 4,
> +    "description": "Certificate pinning evalutation results(pinned host)(failure=0, success=1)"

Why does this have n_values = 4 but actual number of values only 2?

Comment 3

[Camilo Viecco (:cviecco)]

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #2)
> Comment on attachment 8415676 [details] [diff] [review]
> pinning-telemetry
> 
> Review of attachment 8415676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This can now be unittested in the test_pinning unittest. It might even be
> useful distinguishing between pinning enforcement levels disabled and mitm.
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +5920,5 @@
> > +  "PINNING_EVALUATION_RESULTS": {
> > +    "expires_in_version": "never",
> > +    "kind": "enumerated",
> > +    "n_values": 4,
> > +    "description": "Certificate pinning evalutation results(pinned host)(failure=0, success=1)"
> 
> Why does this have n_values = 4 but actual number of values only 2?
Because for we will certainly add fail/success when using non-built-in.

Comment 4

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

Comment on attachment 8415676 [details] [diff] [review]
pinning-telemetry

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

I think it would be a good idea to have a simple test that ensures we're getting the telemetry results we're expecting. This can go in test_pinning.js. See check_ocsp_stapling_telemetry in test_ocsp_stapling.js for an example. Feel free to do this in a separate patch that gets folded with this one before checking in.
r=me on this part with nits addressed.

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +4,5 @@
>  
>  #include "PublicKeyPinningService.h"
>  #include "StaticHPKPins.h" // autogenerated by genHPKPStaticpins.js
>  #include "ScopedNSSTypes.h"
> +#include "mozilla/Telemetry.h"

nit: re-do all of these #includes according to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices

@@ +148,5 @@
>      evalHost++;
>    } // end while
>  
>    if (foundEntry && foundEntry->pinset) {
> +    bool pinningEvaluation = EvalPinWithPinset(certList, foundEntry->pinset);

nit: maybe "result" instead of "pinningEvaluation"

@@ +150,5 @@
>  
>    if (foundEntry && foundEntry->pinset) {
> +    bool pinningEvaluation = EvalPinWithPinset(certList, foundEntry->pinset);
> +    int telemetryValue = pinningEvaluation ? 1 : 0;
> +    Telemetry::Accumulate(Telemetry::PINNING_EVALUATION_RESULTS, telemetryValue);

nit: might as well do this with an inline if instead of factoring out "telemetryValue"

::: toolkit/components/telemetry/Histograms.json
@@ +5920,5 @@
> +  "PINNING_EVALUATION_RESULTS": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 4,
> +    "description": "Certificate pinning evalutation results(pinned host)(failure=0, success=1)"

nit: 0 = failure, 1 = success

Comment 5

[Camilo Viecco (:cviecco)]

 https://tbpl.mozilla.org/?tree=Try&rev=d5f6ccea0898

Comment 6

[Camilo Viecco (:cviecco)]

Attachment: pinning-telemetry (v2)

keeping r+ from keeler

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f88ba7ff4196

Comment 8

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

Camilo, please remember to comment with a link to mozilla-inbound when landing patches there. It helps others keep track of the progress of bugs/patches.