1352262 - add preferences to configure OCSP timeout values

Status: RESOLVED FIXED

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

Description

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

Having hard-coded timeouts of 2 seconds and 10 seconds for DV and EV respectively isn't very flexible - we should make them configurable.

Comment 1

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

Attachment: bug 1352262 - make OCSP timeout values configurable

The default OCSP timeout for soft-fail DV is still 2 seconds. This patch makes
it configurable on the interval (0, 5] seconds.

The default OCSP timeout for EV and hard-fail DV is still 10 seconds. This patch
makes it configurable on the interval (0, 20] seconds.

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

Comment 2

[:Cykesiopka]

Comment on attachment 8858163 [details]
bug 1352262 - make OCSP timeout values configurable

https://reviewboard.mozilla.org/r/130106/#review132896

Looks good.

::: security/certverifier/NSSCertDBTrustDomain.cpp:278
(Diff revision 1)
>  {
>    return DigestBufNSS(item, digestAlg, digestBuf, digestBufLen);
>  }
>  
> -static TimeDuration
> -OCSPFetchingTypeToTimeoutTime(NSSCertDBTrustDomain::OCSPFetching ocspFetching)
> +TimeDuration
> +NSSCertDBTrustDomain::OCSPFetchingTypeToTimeoutTime() const

Optional: Maybe we can rename this to something like `GetOCSPTimeout()` instead?
Feels slightly weird to have this be named "XToY" when it no longer takes an X.

Comment 3

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

Comment on attachment 8858163 [details]
bug 1352262 - make OCSP timeout values configurable

https://reviewboard.mozilla.org/r/130106/#review133050

This patch looks good, but shouldn't we set those prefs to some defaults in security-prefs.js before merging?

::: security/manager/ssl/nsNSSComponent.cpp:183
(Diff revision 1)
>                           static_cast<uint32_t>(0));
>  
> +  uint32_t softTimeoutMillis =
> +    Preferences::GetUint("security.OCSP.timeoutMilliseconds.soft",
> +                         OCSP_TIMEOUT_MILLISECONDS_SOFT_DEFAULT);
> +  if (softTimeoutMillis > OCSP_TIMEOUT_MILLISECONDS_SOFT_MAX) {

Nit: Why not use `std::min(softTimeoutMillis, OCSP_TIMEOUT_MILLISECONDS_SOFT_MAX)` here and next?

Comment 4

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

Comment on attachment 8858163 [details]
bug 1352262 - make OCSP timeout values configurable

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

Comment 5

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

Comment on attachment 8858163 [details]
bug 1352262 - make OCSP timeout values configurable

https://reviewboard.mozilla.org/r/130106/#review132896

Thanks!

> Optional: Maybe we can rename this to something like `GetOCSPTimeout()` instead?
> Feels slightly weird to have this be named "XToY" when it no longer takes an X.

Sounds good.

Comment 6

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

Comment on attachment 8858163 [details]
bug 1352262 - make OCSP timeout values configurable

https://reviewboard.mozilla.org/r/130106/#review133050

I suppose so. Not having them in security-prefs.js makes them hidden, but that tends to confuse people (e.g. when we said "just flip security.enterprise_roots.enabled!" they actually had to add the pref).

> Nit: Why not use `std::min(softTimeoutMillis, OCSP_TIMEOUT_MILLISECONDS_SOFT_MAX)` here and next?

Heh - good call.

Comment 7

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

Comment on attachment 8858163 [details]
bug 1352262 - make OCSP timeout values configurable

https://reviewboard.mozilla.org/r/130106/#review133100

Interdiff looks great. 11/10 would review again

Comment 8

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

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f78cc6e36f8
Thanks for the reviews!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0e27739f475
make OCSP timeout values configurable r=Cykesiopka,jcj

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0e27739f475