Closed
Bug 1078108
Opened 10 years ago
Closed 10 years ago
The validity periods for test OCSP responses are not realistic
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: briansmith, Assigned: briansmith)
References
Details
Attachments
(1 file, 1 obsolete file)
2.02 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
Our OCSP-related tests xpcshell often generate OCSP responses up front and then validate responses using them. It seems like there is a race condition related to the default value of the nextUpdate field in the test OCSP responses. Recently I changed some details of how OCSP responses were generated in pkixtestutil.cpp, and surprisingly various OCSP-related OCSP tests started failing. After increasing the default value of the nextUpdate field of the test OCSP responses, the tests fail less frequently (never so far). However, I do not understand why this makes things better, because we are supposed to have a full day of slop in our comparisons. Thus, I don't think that such an increase should have any effect. What am I missing?
Attachment #8500205 -
Flags: review?(dkeeler)
Assignee | ||
Comment 1•10 years ago
|
||
BTW, the most common failures were test_ev_certs.js on debug builds on Mac OS X 10.6 and Mac OS X 10.8, but other OCSP-related tests on other platforms were also failing frequently.
Comment on attachment 8500205 [details] [diff] [review] longer-validity-for-test-ocsp-responses.patch Review of attachment 8500205 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I don't know why this would fix the intermittents either, but at least we would be testing something much more similar to what we actually see in the wild, so this is probably good in that sense, anyway. ::: security/pkix/test/lib/pkixtestutil.cpp @@ +134,5 @@ > > , certStatus(good) > , revocationTime(0) > , thisUpdate(time) > + , nextUpdate(time + (60 * 60)) // time + 1 hour While we're here, it would be nice to fix the same issue in security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp (line 155).
Attachment #8500205 -
Flags: review?(dkeeler) → review+
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 3•10 years ago
|
||
This patch changes the test logic in two places so that the validity period of most OCSP responses is set to one day, which is much more realistic than 10 seconds. However, this in OCSPCommon.cpp code still looks weird: if (aORT == ORTLongValidityAlmostExpired) { context.thisUpdate = now - (320 * Time::ONE_DAY_IN_SECONDS); } if (aORT == ORTAncientAlmostExpired) { context.thisUpdate = now - (640 * Time::ONE_DAY_IN_SECONDS); } context.thisUpdate will be set to a year or two ago, but context.nextUpdate will be set to tomorrow? Is it really the intent of these tests to be testing validity periods of one or two years? Note that pkixocsp.cpp automatically shortens validity periods to a shorter interval when they are too long (>10 days, for end-entities, and >1 year for intermediates, IIRC). This bug doesn't block anything anymore. I figured out the problems that were causing the test failures for my other patches.
Attachment #8500205 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Assignee: brian → nobody
Target Milestone: mozilla35 → ---
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #3) > This patch changes the test logic in two places so that the validity period > of most OCSP responses is set to one day, which is much more realistic than > 10 seconds. > > However, this in OCSPCommon.cpp code still looks weird: > > if (aORT == ORTLongValidityAlmostExpired) { > context.thisUpdate = now - (320 * Time::ONE_DAY_IN_SECONDS); > } > if (aORT == ORTAncientAlmostExpired) { > context.thisUpdate = now - (640 * Time::ONE_DAY_IN_SECONDS); > } > > context.thisUpdate will be set to a year or two ago, but context.nextUpdate > will be set to tomorrow? Is it really the intent of these tests to be > testing validity periods of one or two years? Note that pkixocsp.cpp > automatically shortens validity periods to a shorter interval when they are > too long (>10 days, for end-entities, and >1 year for intermediates, IIRC). Yes, that's exactly it - it's to test long-lived OCSP responses for intermediates.
Assignee | ||
Updated•10 years ago
|
Attachment #8501471 -
Flags: review?(dkeeler)
Comment on attachment 8501471 [details] [diff] [review] longer-validity-for-test-ocsp-responses.patch Review of attachment 8501471 [details] [diff] [review]: ----------------------------------------------------------------- Great - thanks.
Attachment #8501471 -
Flags: review?(dkeeler) → review+
Comment 6•10 years ago
|
||
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ef257fcbcdd But this was actually not the right culprit, so relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/b8968e6434df
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b8968e6434df
Assignee: nobody → brian
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•9 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•