Closed Bug 1254653 Opened 4 years ago Closed 4 years ago

Add telemetry to measure how often we encounter EV certificates

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: rbarnes, Assigned: rbarnes)

Details

Attachments

(1 file)

It would be good to know how much usage EV is actually getting in practice.
Assignee: nobody → rlb
Comment on attachment 8728115 [details]
MozReview Request: Bug 1254653 - Add telemetry to measure how often we encounter EV certificates r?keeler

https://reviewboard.mozilla.org/r/38789/#review35469

::: security/manager/ssl/SSLServerCertVerification.cpp:1246
(Diff revision 1)
> +  uint32_t evStatus = ((rv == SECSuccess)? 2 : 0) +

From an API standpoint, if rv != SECSuccess, evOidPolicy won't necessarily have a meaningful value. Looking at the actual code, this would work in cases where the certificate successfully validates as EV but then isn't valid for the given hostname (for example). That said, I'm not sure we want to break the "if the return value indicates failure, no output variables are valid" guideline for this. Would having 3 buckets work for the data you want? 0: failed to validate, 1: DV, 2: EV?

::: security/manager/ssl/SSLServerCertVerification.cpp:1247
(Diff revision 1)
> +                      ((evOidPolicy != SEC_OID_UNKNOWN)? 1: 0);

nit: spaces around operators: a ? b : c

::: toolkit/components/telemetry/Histograms.json:8154
(Diff revision 1)
> +  "CERT_EV_STATUS": {

I think technically you need a privacy peer to sign off on this: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe
Attachment #8728115 - Flags: review?(dkeeler)
Comment on attachment 8728115 [details]
MozReview Request: Bug 1254653 - Add telemetry to measure how often we encounter EV certificates r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38789/diff/1-2/
Attachment #8728115 - Flags: review?(dkeeler)
Attachment #8728115 - Flags: feedback?(benjamin)
https://reviewboard.mozilla.org/r/38789/#review35469

> From an API standpoint, if rv != SECSuccess, evOidPolicy won't necessarily have a meaningful value. Looking at the actual code, this would work in cases where the certificate successfully validates as EV but then isn't valid for the given hostname (for example). That said, I'm not sure we want to break the "if the return value indicates failure, no output variables are valid" guideline for this. Would having 3 buckets work for the data you want? 0: failed to validate, 1: DV, 2: EV?

Yep, that'll be fine.

> nit: spaces around operators: a ? b : c

Borrowing a pattern from AccumulateNonECCKeySize instead.
https://reviewboard.mozilla.org/r/38789/#review35601

::: toolkit/components/telemetry/Histograms.json:8160
(Diff revision 2)
> +    "description": "How often do we encounter EV certificates? 0=invalid/DV, 1=invalid/EV, 2=valid/DV, 3=valid/EV"

Could you phrase this in the form of a statement instead of a question? e.g.

"Recorded for every TLS connection attempted."

Obviously I made this up because I don't know if there is caching or other bits involved. Or is this only recorded for a subset of TLS connections that have already passed other checks?

Who is going to monitor this permanently and how will that be done? Note that telemetry alerts are going to be useless for this; they are currently only useful for scalar data such as performance. Do you intend to monitor the total volume of certificates or just the ratios between them? Is there somebody I can ping in particular every 6 months to make sure that this data is still valuable?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> https://reviewboard.mozilla.org/r/38789/#review35601
> 
> ::: toolkit/components/telemetry/Histograms.json:8160
> (Diff revision 2)
> > +    "description": "How often do we encounter EV certificates? 0=invalid/DV, 1=invalid/EV, 2=valid/DV, 3=valid/EV"
> 
> Could you phrase this in the form of a statement instead of a question? e.g.
> 
> "Recorded for every TLS connection attempted."

Done.  Also updated the values to match the revisions after keeler's comments.

> Obviously I made this up because I don't know if there is caching or other
> bits involved. Or is this only recorded for a subset of TLS connections that
> have already passed other checks?

This is done once per TLS connection; no caching.


> Who is going to monitor this permanently and how will that be done? Note
> that telemetry alerts are going to be useless for this; they are currently
> only useful for scalar data such as performance. Do you intend to monitor
> the total volume of certificates or just the ratios between them? Is there
> somebody I can ping in particular every 6 months to make sure that this data
> is still valuable?

This is going to be one of the longitudinal metrics that we use to monitor the certificate ecosystem, like HTTP_PAGELOAD_IS_SSL.  User benefit is not coming from solving a specific problem in the browser, but rather by helping us keep track of how Firefox sees the PKI in general.

I know alerts are useless here.  I just included alert_emails because it was required :)
Comment on attachment 8728115 [details]
MozReview Request: Bug 1254653 - Add telemetry to measure how often we encounter EV certificates r?keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38789/diff/2-3/
Attachment #8728115 - Flags: feedback?(benjamin)
Comment on attachment 8728115 [details]
MozReview Request: Bug 1254653 - Add telemetry to measure how often we encounter EV certificates r?keeler

data-review=me

If you have a bug report specifically for dashboards or longitudinal monitoring, I'd love to be cc'ed on them.
Attachment #8728115 - Flags: feedback+
Comment on attachment 8728115 [details]
MozReview Request: Bug 1254653 - Add telemetry to measure how often we encounter EV certificates r?keeler

https://reviewboard.mozilla.org/r/38789/#review35633

LGTM.
Attachment #8728115 - Flags: review?(dkeeler) → review+
Comment on attachment 8728115 [details]
MozReview Request: Bug 1254653 - Add telemetry to measure how often we encounter EV certificates r?keeler

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Delay in getting measurements
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2cd3d18cdb2e
[Risks and why]: Very minor; no logic change, just a telemetry probe
[String/UUID change made/needed]: n/a
Attachment #8728115 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/2cd3d18cdb2e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8728115 [details]
MozReview Request: Bug 1254653 - Add telemetry to measure how often we encounter EV certificates r?keeler

New telemetry probe for TLS, taking it.
Attachment #8728115 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.