ocsp stapling testing: differentiate between an empty stapled response and no stapled response

RESOLVED FIXED in Firefox 27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

unspecified
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox27 fixed, firefox28 fixed, b2g-v1.2 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

There's a difference between a server not stapling an OCSP response and a server stapling an empty response. We're not explicitly testing the former. Also, we'll need this for testing OCSP must-staple.
Posted patch patch (obsolete) — Splinter Review
This fixes an oversight in our OCSP stapling testing. (An empty OCSP response != not sending an OCSP response.)
Attachment #822643 - Flags: review?(cviecco)
Comment on attachment 822643 [details] [diff] [review]
patch

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

There are some issues with the logic that could be improved. (unless I somehow misread the code).

::: security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp
@@ +138,5 @@
>        }
>        otherID.forget(); // owned by sr now
>        break;
>      }
>      case OSRTNone:

With the logic changes below.. this case should never be reached? and thus error be created? Ditto below

@@ +266,5 @@
>                                                      &cert, &certKEA)) {
>      return SSL_SNI_SEND_ALERT;
>    }
>  
> +  if (host->mOSRT != OSRTNone) {

it is not easier to do a:
if (host->mOSRT == OSRTNode) {
  return 0;
}

and keep the rest of the logic the same?
Attachment #822643 - Flags: review?(cviecco) → review-
Posted patch patch v2 (obsolete) — Splinter Review
Good call. Updated the patch to return early if the OCSP response type is "none".
Attachment #822643 - Attachment is obsolete: true
Attachment #823441 - Flags: review?(cviecco)
Comment on attachment 823441 [details] [diff] [review]
patch v2

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

r+ with comment addressed.

::: security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp
@@ +217,5 @@
>          PrintPRError("CERT_CreateEncodedOCSPErrorResponse failed");
>          return nullptr;
>        }
>        break;
>      case OSRTNone:

Forgot to explicitly ask to move this one to error too.
Attachment #823441 - Flags: review?(cviecco) → review+
Posted patch patch v2.1Splinter Review
Thanks! Carrying over r+.
Attachment #823441 - Attachment is obsolete: true
Attachment #823545 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9677c6c4e94a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.