1366100 - Disable OCSP fetching for domain-validated certificates on nightly

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[J.C. Jones [:jcj] (he/him)]

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.

Comment 1

[Darkspirit]

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

Comment 2

[Darkspirit]

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?

Comment 3

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

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 4

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

Attachment: bug 1366100 - disable OCSP fetching for DV certificates in nightly

After this change, the platform will only fetch OCSP responses for EV
certificates (in nightly).

Review commit: https://reviewboard.mozilla.org/r/142080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142080/

Comment 5

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8870629 [details]
bug 1366100 - disable OCSP fetching for DV certificates in nightly

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

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bac4b657887b

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Hey Keeler, I sent this over to jaws since he's overseeing the about:preferences work

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 9

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

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 10

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

Comment on attachment 8870629 [details]
bug 1366100 - disable OCSP fetching for DV certificates in nightly

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142080/diff/1-2/

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 12

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

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 13

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

Comment on attachment 8870629 [details]
bug 1366100 - disable OCSP fetching for DV certificates in nightly

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142080/diff/2-3/

Comment 14

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

Thanks for the reviews!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/247e0aab1a84
disable OCSP fetching for DV certificates in nightly r=jaws,jcj

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/247e0aab1a84