Disable OCSP fetching for domain-validated certificates on nightly

RESOLVED FIXED in Firefox 55

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: jcj, Assigned: keeler)

Tracking

(Blocks: 1 bug)

55 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
Telemetry indicates that fetching OCSP results is an important cause of slowness in the first TLS handshake. Firefox is, today, the only major browser still fetching OCSP by default for DV certificates.

In Bug 1361201 we tried reducing the OCSP timeout to 1 second (based on CERT_VALIDATION_HTTP_REQUEST_SUCCEEDED_TIME), but that seems to have caused only a 2% improvement in SSL_TIME_UNTIL_HANDSHAKE_FINISHED.

This bug is to disable OCSP fetching for DV certificates. OCSP fetching should remain enabled for EV certificates.

OCSP stapling will remain fully functional. We encourage everyone to use OCSP stapling.
1. OCSP Stapling is the solution: Please evaluate a MOSS project providing a fix for https://trac.nginx.org/nginx/ticket/812 and Apache [1] instead of this bug. After that, we could think about OCSP Must Staple.
2. As Letsencrypt does not have a CRL(?): How do you want to check for revocation then when connecting to non-stapling hosts? Just by doing OCSP checking in parallel and maybe switching from the website away to a cert error page after max. 1 sec?
3. Just to rely on the certificate validity dates seems undesirable. Plead for wontfix.

[1] https://bz.apache.org/bugzilla/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&bug_status=NEEDINFO&field0-0-0=product&field0-0-1=component&field0-0-2=alias&field0-0-3=short_desc&field0-0-4=status_whiteboard&field0-0-5=content&order=changeddate%2Cbug_status%2Cpriority%2Cassigned_to%2Cbug_id&query_format=advanced&type0-0-0=substring&type0-0-1=substring&type0-0-2=substring&type0-0-3=substring&type0-0-4=substring&type0-0-5=matches&value0-0-0=OCSP&value0-0-1=OCSP&value0-0-2=OCSP&value0-0-3=OCSP&value0-0-4=OCSP&value0-0-5=%22OCSP%22
(Assignee)

Updated

3 months ago
Assignee: nobody → dkeeler
Priority: -- → P1
Summary: Disable OCSP fetching for domain-validated certificates → Disable OCSP fetching for domain-validated certificates on nightly
Whiteboard: [psm-assigned]
What's with security.OCSP.enabled?

(In reply to David Keeler [:keeler] (use needinfo?) from bug 1010068 comment 6)
> We might also add a value to security.OCSP.enabled so we'll have 3 states
> instead of two:
> 0: never, ever fetch OCSP
> 1: fetch OCSP for DV and EV
> 2: fetch OCSP for EV only

There are security.ssl.enable_ocsp_stapling and security.ssl.enable_ocsp_must_staple,
so 0 should be stapling-only (is it already?)
What's with OV certs?
This bug here just wants to set security.OCSP.enabled;2, or not?
(Assignee)

Comment 3

3 months ago
security.OCSP.enabled currently behaves as described in bug 1010068 comment 6. Setting it to 0 means only OCSP information from stapling is considered. The platform treats OV certificates as DV certificates. This bug will set security.OCSP.enabled to 2 for Nightly.
Comment hidden (mozreview-request)
(Reporter)

Comment 5

3 months ago
mozreview-review
Comment on attachment 8870629 [details]
bug 1366100 - disable OCSP fetching for DV certificates in nightly

https://reviewboard.mozilla.org/r/142080/#review146038
Attachment #8870629 - Flags: review?(jjones) → review+
(Assignee)

Comment 6

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bac4b657887b
Attachment #8870629 - Flags: review?(felipc) → review?(jaws)
Hey Keeler, I sent this over to jaws since he's overseeing the about:preferences work

Comment 8

3 months ago
mozreview-review
Comment on attachment 8870629 [details]
bug 1366100 - disable OCSP fetching for DV certificates in nightly

https://reviewboard.mozilla.org/r/142080/#review146478

::: browser/components/preferences/in-content-old/advanced.js:221
(Diff revision 1)
>      return 0;
>    },
>  
>    /**
> -   * security.OCSP.enabled is an integer value for legacy reasons.
> -   * A value of 1 means OCSP is enabled. Any other value means it is disabled.
> +   * security.OCSP.enabled is an integer value.
> +   * A non-zero value means OCSP is enabled. A value of 0 means it is disabled.

Can you please change this comment to describe the return value? Right now it appears it is describing the possible pref values, but this function returns a Boolean.

::: browser/components/preferences/in-content-old/advanced.js:225
(Diff revision 1)
>      // This is the case if the preference is the default value.
>      if (preference.value === undefined) {
>        return true;
>      }

This assumes that the default value for the preference is "OCSP=enabled" (non-zero) since it will result in the function returning `true` if the preference is the default value.

With your change to make the default value equal to 0, doesn't that conflict with this assumption?

A few lines lower you're reading the default value, would it make more sense to return a value based on whatever the default value is?
Attachment #8870629 - Flags: review?(jaws) → review-
(Assignee)

Comment 9

3 months ago
mozreview-review-reply
Comment on attachment 8870629 [details]
bug 1366100 - disable OCSP fetching for DV certificates in nightly

https://reviewboard.mozilla.org/r/142080/#review146478

> Can you please change this comment to describe the return value? Right now it appears it is describing the possible pref values, but this function returns a Boolean.

Ok - I updated the documentation for both functions. (I might have gone a bit overboard, but hopefully it's a bit more future-proof now.)

> This assumes that the default value for the preference is "OCSP=enabled" (non-zero) since it will result in the function returning `true` if the preference is the default value.
> 
> With your change to make the default value equal to 0, doesn't that conflict with this assumption?
> 
> A few lines lower you're reading the default value, would it make more sense to return a value based on whatever the default value is?

This patch sets the default is 2 (fetch for EV certificates only). I updated the documentation to be more clear that this will indeed not work if the default is 0.
Comment hidden (mozreview-request)

Comment 11

3 months ago
mozreview-review
Comment on attachment 8870629 [details]
bug 1366100 - disable OCSP fetching for DV certificates in nightly

https://reviewboard.mozilla.org/r/142080/#review146586

::: browser/components/preferences/in-content-old/advanced.js:257
(Diff revision 2)
> +    var defaultValue = defaults.getIntPref("security.OCSP.enabled", undefined);
> +    return checkbox.checked ? defaultValue : 0;

The second argument to getIntPref is the value that should be returned if the pref doesn't exist. I don't think it makes sense to return `undefined` from this function, and since the pref is set in security-prefs.js we shouldn't have to worry about a missing default value. I think you should remove this second argument from the function call (here and elsewhere in your patch where you use `undefined`).

::: security/manager/ssl/security-prefs.js:54
(Diff revision 2)
>  
> +// The supported values of this pref are:
> +// 0: do not fetch OCSP
> +// 1: fetch OCSP for DV and EV certificates
> +// 2: fetch OCSP only for EV certificates
> +#ifdef RELEASE_OR_BETA

What is the timeline/plan for letting this ride the trains to release?
Attachment #8870629 - Flags: review?(jaws) → review+
(Assignee)

Comment 12

3 months ago
mozreview-review-reply
Comment on attachment 8870629 [details]
bug 1366100 - disable OCSP fetching for DV certificates in nightly

https://reviewboard.mozilla.org/r/142080/#review146586

> The second argument to getIntPref is the value that should be returned if the pref doesn't exist. I don't think it makes sense to return `undefined` from this function, and since the pref is set in security-prefs.js we shouldn't have to worry about a missing default value. I think you should remove this second argument from the function call (here and elsewhere in your patch where you use `undefined`).

Sounds good - fixed.

> What is the timeline/plan for letting this ride the trains to release?

The plan is to monitor telemetry to see if this impacts TLS handshake time. If there is a significant improvement, we have a stronger case for moving forward with it. If not, we'll probably run a study on a limited release population to see if it would make a difference there. Either way, hopefully we'll resolve this one way or another in the next few months and/or releases.
Comment hidden (mozreview-request)
Thanks for the reviews!

Comment 15

3 months ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/247e0aab1a84
disable OCSP fetching for DV certificates in nightly r=jaws,jcj

Comment 16

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/247e0aab1a84
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Depends on: 1368523
(Assignee)

Updated

2 months ago
Blocks: 803582
You need to log in before you can comment on or make changes to this bug.