Closed Bug 951315 Opened 6 years ago Closed 6 years ago

Add telemetry to PK pinning

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: cviecco, Assigned: cviecco)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 744204
Assignee: nobody → cviecco
Attached patch pinning-telemetry (obsolete) — Splinter Review
Attachment #8415676 - Flags: review?(dkeeler)
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?
(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 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
Attachment #8415676 - Flags: review?(dkeeler) → review+
keeping r+ from keeler
Attachment #8415676 - Attachment is obsolete: true
Attachment #8416583 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f88ba7ff4196
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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.
You need to log in before you can comment on or make changes to this bug.