1172495 - Update nsNSSComponent and Telemetry to handle new OCSP options

Status: NEW

(Core :: Security: PSM, defect, P3)

Description

[Richard Barnes [:rbarnes]]

In bug 1010068, we added a third option (besides 0 and 1) to the preference security.OCSP.enabled.  (Option 2 means "only fetch for EV".)  We need to update the telemetry collection in nsNSSComponent::setValidationOptions() to account for this change.

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: bug-1172495.0.patch

* Translate ocspEnabled to (ocspLevel > 0)
* Add a histogram for ocsp level

Comment 2

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

Comment on attachment 8616704 [details] [diff] [review]
bug-1172495.0.patch

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

Ok - r=me with comments addressed.

::: security/manager/ssl/nsNSSComponent.cpp
@@ +863,3 @@
>                                              OCSP_ENABLED_DEFAULT);
>  
> +  bool ocspRequired = (ocspLevel > 0) &&

With the current implementation (if I'm understanding it correctly), hard-fail really only applies when ocspLevel is 1, because otherwise we either don't fetch OCSP (0) or we do, but only for EV (2), and if that fails, we just fall back to not fetching OCSP. We can either document that this is the expected behavior or fix it to not fall back if fetching for EV fails.

@@ +868,5 @@
>    // We measure the setting of the pref at startup only to minimize noise by
>    // addons that may muck with the settings, though it probably doesn't matter.
>    if (isInitialSetting) {
> +    Telemetry::Accumulate(Telemetry::CERT_OCSP_LEVEL, ocspLevel);
> +    Telemetry::Accumulate(Telemetry::CERT_OCSP_ENABLED, (ocspLevel > 0));

CERT_OCSP_ENABLED seems superfluous now.

::: toolkit/components/telemetry/Histograms.json
@@ +7209,5 @@
>      "kind": "boolean",
>      "description": "Is OCSP fetching enabled? (pref security.OCSP.enabled)"
>    },
> +  "CERT_OCSP_LEVEL": {
> +    "expires_in_version": "never",

I think we can set this to default here.

@@ +7212,5 @@
> +  "CERT_OCSP_LEVEL": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 10,
> +    "description": "Which certificates get OCSP checking? (security.OCSP.enabled; see nsNSSComponent.cpp)"

Since there are only 3 expected values for now, let's enumerate them here.