Closed Bug 977865 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 + fixed
firefox32 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
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).
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8393210 - Flags: feedback?(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?
Attachment #8393210 - Flags: feedback?(cviecco) → feedback+
Summary: add backoff for ocsp fetching when a responder fails in insanity::pkix → add backoff for ocsp fetching when a responder fails in mozilla::pkix
(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.
Attached patch patch v2Splinter Review
Attachment #8393210 - Attachment is obsolete: true
Attachment #8407081 - Flags: review?(cviecco)
Attachment #8407081 - Flags: review?(cviecco) → review+
https://hg.mozilla.org/mozilla-central/rev/6d813156e491
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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
Attachment #8407081 - Flags: approval-mozilla-aurora?
Tracking because pkix is a new feature.
Attachment #8407081 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.