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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

This fixes an oversight in our OCSP stapling testing. (An empty OCSP response != not sending an OCSP response.)

Comment 2

[Camilo Viecco (: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?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v2

Good call. Updated the patch to return early if the OCSP response type is "none".

Comment 4

[Camilo Viecco (: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.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v2.1

Thanks! Carrying over r+.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9677c6c4e94a

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9677c6c4e94a

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/releases/mozilla-beta/rev/dfb33820c920
(landed with bug 887321, bug 929617, bug 943115, bug 938805, bug 932519, and bug 934327)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/5a831cc94168