977865 - add backoff for ocsp fetching when a responder fails in mozilla::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

The idea being, if we can't talk to an OCSP responder (or, e.g. it responds with "trylater"), we shouldn't keep hammering at it.

Comment 1

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

Attachment: patch

Camilo, if you could give me some feedback on this, I would appreciate it. In particular, I'm trying to figure out how to refactor things so this isn't as much of a mess. Also, ideas on how to properly test this would be great (e.g. I'm thinking we need something like verifyCertAtTime to properly test that we do actually try getting an OCSP response after waiting).

Comment 2

[Camilo Viecco (:cviecco)]

Comment on attachment 8393210 [details] [diff] [review]
patch

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

So far looks good. Where did you get the 60 seconds timer?

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +366,5 @@
>  
>  SECStatus
>  NSSCertDBTrustDomain::VerifyAndMaybeCacheEncodedOCSPResponse(
>    const CERTCertificate* cert, CERTCertificate* issuerCert, PRTime time,
> +  const SECItem* encodedResponse, bool fromNetwork)

I woould change the name from fromNetwork, to fromHandShake and invert logic.

::: security/certverifier/OCSPCache.cpp
@@ +195,5 @@
>  
>    int32_t index = FindInternal(aCert, aIssuerCert, lock);
>  
>    if (index >= 0) {
> +    MakeMostRecentlyUsed(index, lock);

dont the location of the entry(and this index) gets modified with this? affecting the comparisons below?

Comment 3

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

(In reply to Camilo Viecco (:cviecco) from comment #2)
> So far looks good. Where did you get the 60 seconds timer?

It was just a guess, really. I changed it to 5 minutes. NSS seems to wait an hour, which I think is excessive.

Comment 4

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

Attachment: patch v2

Comment 5

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

https://tbpl.mozilla.org/?tree=Try&rev=8e5fbb4ce31f

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d813156e491

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d813156e491

Comment 8

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

Comment on attachment 8407081 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): mozilla::pkix
User impact if declined: Firefox will mercilessly hammer broken OCSP responders
Testing completed (on m-c, etc.): has tests, on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Tracking because pkix is a new feature.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0c8a23a93371