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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file, 1 obsolete file)

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)
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.
See Also: → 1059883, 1003622, 967444, 964673
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+
No longer blocks: 1063281, 1077926, 1079436
Summary: The default validity period for test OCSP responses is too short → The validity periods for test OCSP responses are not realistic
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: 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.
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+
https://hg.mozilla.org/mozilla-central/rev/b8968e6434df
Assignee: nobody → brian
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.