Closed
Bug 951315
Opened 10 years ago
Closed 10 years ago
Add telemetry to PK pinning
Categories
(Core :: Security: PSM, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: cviecco, Assigned: cviecco)
References
Details
Attachments
(1 file, 1 obsolete file)
4.25 KB,
patch
|
cviecco
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → cviecco
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8415676 -
Flags: review?(dkeeler)
Comment 2•10 years ago
|
||
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?
Assignee | ||
Comment 3•10 years ago
|
||
(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+
Assignee | ||
Comment 5•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=d5f6ccea0898
Assignee | ||
Comment 6•10 years ago
|
||
keeping r+ from keeler
Attachment #8415676 -
Attachment is obsolete: true
Attachment #8416583 -
Flags: review+
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f88ba7ff4196
Status: NEW → RESOLVED
Closed: 10 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.
Depends on: 1153444
You need to log in
before you can comment on or make changes to this bug.
Description
•