Update nsNSSComponent and Telemetry to handle new OCSP options

NEW
Unassigned

Status

()

Core
Security: PSM
P3
normal
3 years ago
9 months ago

People

(Reporter: rbarnes, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [psm-cleanup])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8616704 [details] [diff] [review]
bug-1172495.0.patch

* Translate ocspEnabled to (ocspLevel > 0)
* Add a histogram for ocsp level
Attachment #8616704 - Flags: review?(dkeeler)
(Reporter)

Updated

3 years ago
Assignee: nobody → rlb
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.
Attachment #8616704 - Flags: review?(dkeeler) → review+
Whiteboard: [psm-assigned]
Assignee: rlb → nobody
Priority: -- → P3
Whiteboard: [psm-assigned] → [psm-cleanup]
You need to log in before you can comment on or make changes to this bug.