Closed Bug 930209 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox27 --- fixed
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached 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-
Attached 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+
Attached 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: 7 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.