932519 - refactor OCSP response generation test code into shared library

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

The code to generate OCSP responses in OCSPStaplingServer is useful elsewhere, so it should be factored out.

Comment 1

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

Attachment: patch

This moves the relevant code to tlsserver/lib/OCSPCommon.{cpp,h}.

Comment 2

[Camilo Viecco (:cviecco)]

Comment on attachment 824295 [details] [diff] [review]
patch

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

This is a start, but I would actually refactor even more:

0. refactor GetOCSPResponseForType to include the ca  cert that will be used for signing

1. Make a new enum: 
OCSPResponseType  that should be limited to:
  OSRTGood,             // the certificate is good
  OSRTRevoked,          // the certificate has been revoked
  OSRTUnknown,          // the responder doesn't know if the cert is good
  OSRTExpired,          // the signature on the response has expired
  OSRTExpiredFreshCA,   // fresh signature, but old validity period
  OSRTNone,             // no stapled response
  OSRTMalformed,        // the response from the responder was malformed
  OSRTSrverr,           // the response indicates there was a server error
  OSRTTryLater,         // the responder replied with "try again later"
  OSRTNeedsSig,         // the response needs a signature
  OSRTUnauthorized      // the responder is not authorized for this certificate

modify struct OCSPHost to include the ca nick of the signer (if any).
{
  const char *mHostName;
  OCSPStapleResponseType mOSRT;
};

this way we can factor out the who is signing from the signing.


OCSPStapleResponseType to include the ca cert that will be

Comment 3

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

Attachment: patch 1/3

This just moves the common code to a sharable location.

Comment 4

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

Attachment: patch 2/3

This renames anything that had "stapling" in the name so the types are more generic.

Comment 5

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

Attachment: patch 3/3 obsolete

This adds the signing cert as a parameter to GetOCSPResponseForType and makes the caller responsible for doing things like passing in a different signing cert or ee cert for testing cases where those were different than expected.

Comment 6

[Camilo Viecco (:cviecco)]

Comment on attachment 826034 [details] [diff] [review]
patch 3/3 obsolete

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

With comment addressed (variable without init)

::: security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp
@@ +81,5 @@
> +      return SSL_SNI_SEND_ALERT;
> +    }
> +  }
> +
> +  ScopedCERTCertificate otherCert;

I would initialize this one to null here or set it to null after the if, or even better:

(on the next if, release the current cert, asigne the othercert to it and remove the if logic from the call to getOCSPResponseForType).

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 826034 [details] [diff] [review]
patch 3/3 obsolete

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

::: security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp
@@ +100,5 @@
>    // response is contained by the arena - freeing the arena will free it
> +  SECItemArray *response = GetOCSPResponseForType(host->mORT,
> +                                                  otherCert
> +                                                  ? otherCert
> +                                                  : cert,

nit:

    otherCert ? otherCert : cert,

or:

    otherCert ? otherCert
              : cert

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ -95,5 @@
> -        return nullptr;
> -      }
> -      otherID.forget(); // owned by sr now
> -      break;
> -    }

In general, every case that we test for OCSP stapling we should test for OCSP fetching and vice versa. So, it makes to keep this stuff here and to add additional cases (e.g. ORTGoodSignedByDelegatedResponderWithAIA) here as needed.

This way, OCSPCommon provides the menu of different OCSP responses for us to choose from, and different tests can choose different cases, and all the certificate-twiddling stuff is in OCSPCommon.

Would this be problematic?

Comment 8

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO me if you want a response) from comment #7)
> In general, every case that we test for OCSP stapling we should test for
> OCSP fetching and vice versa. So, it makes to keep this stuff here and to
> add additional cases (e.g. ORTGoodSignedByDelegatedResponderWithAIA) here as
> needed.
> 
> This way, OCSPCommon provides the menu of different OCSP responses for us to
> choose from, and different tests can choose different cases, and all the
> certificate-twiddling stuff is in OCSPCommon.
> 
> Would this be problematic?

One of my goals was to make GetOCSPResponseForType not ever look up a certificate by a hard-coded nickname. Do you think a good solution would be to have a parameter 'const char *addtionalNickname' that is used to look up an additional certificate that may get used in various ways depending on the OCSP response type? Or maybe common wrapper functions that take a nickname and call GetOCSPResponderForType with the right parameters? (Like GetOCSPResponseForWrongEE, GetOCSPResponseForWrongSigner, GetOCSPResponseForDelegatedSigner, etc.)

Comment 9

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

Attachment: patch 3/3

Something like this?

Comment 10

[Camilo Viecco (:cviecco)]

Comment on attachment 826981 [details] [diff] [review]
patch 3/3

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

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.h
@@ +38,5 @@
>  };
>  
>  SECItemArray *
>  GetOCSPResponseForType(OCSPResponseType aORT, CERTCertificate *aCert,
> +                       PLArenaPool *aArena, const char *aAdditionalCertName);

I really dont see how this is more convenient that the previos mechanism. 

the 'ORTGoodSignedByDelegatedResponderWithAIA' can be handled by the previos approach. And the 'smarts' of what should be generated should be decopled from the generation. If we latter see that this could be integrated then I would do it in a follow up bug.

The 'menu' approach might be good for gTests but I think is not appropiate for xpcshell tests as each tests does not need to know about all the posibilities.

Comment 11

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

(In reply to Camilo Viecco (:cviecco) from comment #10)
> I really dont see how this is more convenient that the previos mechanism. 
> 
> the 'ORTGoodSignedByDelegatedResponderWithAIA' can be handled by the previos
> approach. And the 'smarts' of what should be generated should be decopled
> from the generation. If we latter see that this could be integrated then I
> would do it in a follow up bug.

The benefit is that every test that uses this shared code can simply make a function call to get the type of OCSP response it needs instead of re-implementing code that looks up alternate certificates.

> The 'menu' approach might be good for gTests but I think is not appropiate
> for xpcshell tests as each tests does not need to know about all the
> posibilities.

I disagree - I think each test that involves OCSP needs to test each scenario we know about, not just the good scenarios. For instance, when verifying a cert for EV, do we correctly handle the case that a responder gives a stale response?

Comment 12

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

Comment on attachment 826981 [details] [diff] [review]
patch 3/3

Camilo and I spoke on IRC and my understanding is that we'll go with this approach. Asking for formal r? now.

Comment 13

[Camilo Viecco (:cviecco)]

Comment on attachment 826981 [details] [diff] [review]
patch 3/3

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

overruled on mechanism, but technically r+

Comment 14

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

Comment on attachment 826981 [details] [diff] [review]
patch 3/3

Brian should probably also have a look at this.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 826981 [details] [diff] [review]
patch 3/3

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

Seems reasonable to me though it isn't what I was expecting. I was expecting, for example,, that if we needed a new variant that was just like ORTGoodOtherCert but with a different cert, then we'd define a new entry called ORTGoodOtherCert<WithWhateverSpecialProperty> that could be looked up by that name. Parameterizing on the certificate name seems like it will make it unnecessarily tedious to keep different tests in sync. But, we can cross that bridge if/when we come to it if you don't think it is worth changing it now.

::: security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp
@@ +34,5 @@
> +  { "ocsp-stapling-srverr.example.com", ORTSrverr, nullptr },
> +  { "ocsp-stapling-trylater.example.com", ORTTryLater, nullptr },
> +  { "ocsp-stapling-needssig.example.com", ORTNeedsSig, nullptr },
> +  { "ocsp-stapling-unauthorized.example.com", ORTUnauthorized, nullptr },
> +  { nullptr, ORTNull, nullptr }

This last terminator entry should be removed based on the refactoring of GetHostForSNI we did recently.

Comment 16

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

https://tbpl.mozilla.org/?tree=Try&rev=e4513794557f

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO me if you want a response) from comment #15)
> Seems reasonable to me though it isn't what I was expecting. I was
> expecting, for example,, that if we needed a new variant that was just like
> ORTGoodOtherCert but with a different cert, then we'd define a new entry
> called ORTGoodOtherCert<WithWhateverSpecialProperty> that could be looked up
> by that name. Parameterizing on the certificate name seems like it will make
> it unnecessarily tedious to keep different tests in sync. But, we can cross
> that bridge if/when we come to it if you don't think it is worth changing it
> now.

Yes, let's deal with that when we come to it.

> ::: security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp
> @@ +34,5 @@
> > +  { "ocsp-stapling-srverr.example.com", ORTSrverr, nullptr },
> > +  { "ocsp-stapling-trylater.example.com", ORTTryLater, nullptr },
> > +  { "ocsp-stapling-needssig.example.com", ORTNeedsSig, nullptr },
> > +  { "ocsp-stapling-unauthorized.example.com", ORTUnauthorized, nullptr },
> > +  { nullptr, ORTNull, nullptr }
> 
> This last terminator entry should be removed based on the refactoring of
> GetHostForSNI we did recently.

I think we'll need to do something else to get rid of the terminator entry here - the recent changes to GetHostForSNI don't affect it knowing when to stop looping through the Host array.

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) from comment #16)
> I think we'll need to do something else to get rid of the terminator entry
> here - the recent changes to GetHostForSNI don't affect it knowing when to
> stop looping through the Host array.

> template <typename Host, size_t HostsLen>
> inline const Host *
> GetHostForSNI(const SECItem *aSrvNameArr, uint32_t aSrvNameArrSize,
>               const Host (&hosts)[HostsLen])
> {
>   for (uint32_t i = 0; i < aSrvNameArrSize; i++) {
>     for (size_t h = 0; h < HostsLen; ++h) {

GetHostForSNI knows where to stop because it knows the length of the array. It would actually be a bug to null-terminate the array. Or, am I misunderstanding something?

Comment 18

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO me if you want a response) from comment #17)
> (In reply to David Keeler (:keeler) from comment #16)
> > template <typename Host, size_t HostsLen>
> > inline const Host *
> > GetHostForSNI(const SECItem *aSrvNameArr, uint32_t aSrvNameArrSize,
> >               const Host (&hosts)[HostsLen])
> > {
> >   for (uint32_t i = 0; i < aSrvNameArrSize; i++) {
> >     for (size_t h = 0; h < HostsLen; ++h) {
> 
> GetHostForSNI knows where to stop because it knows the length of the array.
> It would actually be a bug to null-terminate the array. Or, am I
> misunderstanding something?

Looks like that change is in the patch for bug 936818, which includes removing the terminator (it looks like it didn't land with the rest of bug 839310).

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8c890fa1808
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a86c15b3d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/98e2f5b76cc8

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) from comment #18)
> Looks like that change is in the patch for bug 936818, which includes
> removing the terminator (it looks like it didn't land with the rest of bug
> 839310).

Gotcha. Thanks for letting me know that I was overlooking that.

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f8c890fa1808
https://hg.mozilla.org/mozilla-central/rev/f5a86c15b3d8
https://hg.mozilla.org/mozilla-central/rev/98e2f5b76cc8

Comment 22

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

https://hg.mozilla.org/releases/mozilla-beta/rev/e23605810318
https://hg.mozilla.org/releases/mozilla-beta/rev/2ce9a1d999a5
https://hg.mozilla.org/releases/mozilla-beta/rev/6187f61356ce
(landed with bug 887321, bug 929617, bug 943115, bug 938805, bug 934327, and bug 930209)

Comment 23

[u279076]

David, does this have or need in-testsuite tests?

Comment 24

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

This is basically support code for the OCSP tests in security/manager/ssl/tests/unit. If it were faulty, those tests would be failing, so I don't think it needs its own tests.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/fc79468b7763
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ed03062a59a2
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/9a26897e204d