974715 - create more flexible OCSP response generation code (usable from GTests/TLSServer)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Dana Keeler (she/her) [:keeler]]

For instance, we need to be able to generate OCSP responses with SHA-2 functions as the CertID hashing algorithm, include various certificates, etc.

Comment 1

[Dana Keeler (she/her) [:keeler]]

Attachment: patch

Here's the patch for how we can generate more varied OCSP responses for our tests.

Comment 2

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

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

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

I will look at this more. This is just my initial feedback after a quick glance.

::: security/certverifier/moz.build
@@ +9,5 @@
>      '../insanity/lib/pkixcheck.cpp',
>      '../insanity/lib/pkixder.cpp',
>      '../insanity/lib/pkixkey.cpp',
>      '../insanity/lib/pkixocsp.cpp',
> +    '../insanity/lib/pkixocspcreate.cpp',

Do you think this should be part of insanity::pkix itself, or should it just be part of the testing framework in security/manager/ssl/tests/unit/tlsserver/lib? I feel like, since we really only need it for tests, that it should live in the test library. That means we can also be less strict about this code.

If you agree, then all the changes to security/insanity/**/* and security/certverifier/* should be moved to security/manager/ssl/tests/unit/tlsserver/lib.

::: security/insanity/lib/pkixder.h
@@ +513,5 @@
>    }
>    return Success;
>  }
>  
> +static inline void

s/static inline/inline/.

@@ +528,5 @@
> +    data[2] = length % 256;
> +  }
> +}
> +
> +#define MAX_SEQUENCE_ITEMS 5

Don't use #define when we can use const.

@@ +540,5 @@
> +  {
> +  }
> +
> +  Result Add(const SECItem* item)
> +  {

PR_ASSERT(item);
PR_ASSERT(item->data);

@@ +551,5 @@
> +
> +    contents[numItems] = item;
> +    numItems++;
> +    length += item->len;
> +    return Success;

Add a comment above the Add method that the Add method makes a shallow copy of the input item and that all the input items must have lifetime that extends at least to the point where Squash is called.

@@ +555,5 @@
> +    return Success;
> +  }
> +
> +  SECItem* Squash(PLArenaPool* arena, uint8_t tag)
> +  {

PR_ASSERT(arena);

@@ +558,5 @@
> +  SECItem* Squash(PLArenaPool* arena, uint8_t tag)
> +  {
> +    size_t lengthLength = (length < 128 ? 1 :
> +                           length < 256 ? 2 :
> +                           3);

NIT: I think this style is better:

  size_t lengthLength = length < 128 ? 1
                      : length < 256 ? 2
                                     : 3;

(Copy this into a text editor that uses a monospaced font.)

::: security/insanity/lib/pkixocspcreate.cpp
@@ +36,5 @@
> +SECItem* CertID(OCSPResponseContext& context);
> +SECItem* CertStatus(OCSPResponseContext& context);
> +
> +SECItem*
> +Wrap(PLArenaPool* arena, SECItem* inner, uint8_t tag)

Is there a better name than "Wrap"? "EncodeNested"?

Also, I recommend that you put the tag argument before the inner argument (and similarly elsewhere below). The reason is simply that, in the encoded output, the tag appears before the data.

@@ +44,5 @@
> +  return output.Squash(arena, tag);
> +}
> +
> +// Only SHA-1, SHA-256, SHA-384, and SHA-512 are supported.
> +// Returns 0 if anything else is given as hashAlgorithm.

NIT: delete the comment. The reader can read the code to figure out what is supported.

@@ +45,5 @@
> +}
> +
> +// Only SHA-1, SHA-256, SHA-384, and SHA-512 are supported.
> +// Returns 0 if anything else is given as hashAlgorithm.
> +static inline size_t

no need for "static" with "inline." Also, this OCSP response encoding code isn't perf-sensitive (since it is test-only) so we don't really need to inline anything.

@@ +59,5 @@
> +    return SHA384_LENGTH;
> +  }
> +  if (hashAlg == SEC_OID_SHA512) {
> +    return SHA512_LENGTH;
> +  }

NIT: Switch seems clearer. Also, we should do PR_NOT_REACHED() in the "unknown hashAlg" case.

@@ +83,5 @@
> +  der::Output output;
> +  if (output.Add(hashBuf) != der::Success) {
> +    return nullptr;
> +  }
> +  return output.Squash(arena, der::OCTET_STRING);

"return Wrap(arena, hashBuf, der::OCTET_STRING);"?

@@ +178,5 @@
> +  if (!id_pkix_ocsp_basic) {
> +    return nullptr;
> +  }
> +  memcpy(id_pkix_ocsp_basic->data, id_pkix_ocsp_basic_encoded,
> +         PR_ARRAY_SIZE(id_pkix_ocsp_basic_encoded));

SECItem id_pkix_ocsp_basic = {
  siBuffer,
  const_cast<uint8_t*>(&id_pkix_ocsp_basic_encoded),
  PR_ARRAY_SIZE(id_pkix_ocsp_basic_encoded)}
};

@@ +239,5 @@
> +  // not part of the PLArenaPool, so copy the data to a SECItem that is.
> +  // Also, we have to add a byte at the beginning indicating no unused bits,
> +  // so this is a good place to do it.
> +  SECItem* signatureInPool = SECITEM_AllocItem(context.arena, nullptr,
> +                                               signature.len + 1);

I suggest "prefixedSignature" as a better name, since you are adding the prefix which is more important than the SECItem being in the pool. (Nit: Actually, the SECItem doesn't need to be in the pool, AFAICT, except for the fact that you are adding that prefix).

@@ +252,5 @@
> +    PR_ASSERT(signatureInPool->len > 8);
> +    signatureInPool->data[8]++;
> +  }
> +  SECItem* signatureWrapped = Wrap(context.arena, signatureInPool,
> +                                   der::BIT_STRING);

Check return value.

Comment 3

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

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

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

Good work!

::: security/insanity/include/insanity/pkixtypes.h
@@ +102,5 @@
> +public:
> +  enum ResponderIDType {
> +    ByName = 1,
> +    ByKeyHash = 2
> +  };

Nit: Why declare this type up here, but then not use it until the end of the class definition? It seems better to declare the type closer to where it is used.

@@ +107,5 @@
> +
> +  PLArenaPool* arena;
> +  const CERTCertificate* cert; // The subject of the OCSP response
> +  CERTCertificate* issuerCert; // The issuer of the subject
> +  CERTCertificate* signerCert; // The certificate that signs the response

Please document the ownership of the above variables. It would be more clearly correct if they were ScopedXXX types that owned what they point to, and since speed/efficiency is not what we're optimizing for, I suggest we do that.

At some point soon, we'll need to be able to control which certificate(s), if any, get embedded in the OCSP response's cert list. Are you planning to defer that to another bug/patch?

@@ +111,5 @@
> +  CERTCertificate* signerCert; // The certificate that signs the response
> +  PRTime producedAt;
> +  PRTime thisUpdate;
> +  PRTime nextUpdate; // Set to 0 to omit
> +  SECOidTag certIDHashAlg;

Note that this design is restricted to a single SingleResponse per response. I think that is OK for now. I imagine that we'll want to extend it to support multiple SingleResponses, but we can/should do that in a follow-up bug so that this can land.

I suggest, though, that we at least sort all the SingleResponse fields at the end, and that we sort all the fields here in the order that they appear in the encoded OCSP response.

::: security/insanity/lib/pkixocspcreate.cpp
@@ +288,5 @@
> +                                     context.producedAt) != SECSuccess) {
> +    return nullptr;
> +  }
> +  SECItem* producedAtWrapped = Wrap(context.arena, &producedAt,
> +                                    der::GENERALIZED_TIME);

Check return value.

@@ +298,5 @@
> +  if (!responsesWrapped) {
> +    return nullptr;
> +  }
> +  der::Output output;
> +  if (output.Add(responderID) != der::Success) {

Consider adding a variant of Add() that is an Add+Wrap combined. That will make a lot of this code much simpler.

@@ +376,5 @@
> +  SECItem* thisUpdateWrapped = Wrap(context.arena, &thisUpdate,
> +                                    der::GENERALIZED_TIME);
> +  if (!thisUpdateWrapped) {
> +    return nullptr;
> +  }

Note that you repeat this pattern for every PRTime. Worth putting the repeated code into a helper function.

@@ +378,5 @@
> +  if (!thisUpdateWrapped) {
> +    return nullptr;
> +  }
> +  SECItem *nextUpdateWrappedWrapped = nullptr;
> +  if (context.nextUpdate != 0) {

Perhaps we should have a separate flag to indicate whether we should encode the nextUpdate? Because, I could see how testing the value zero for nextUpdate would be useful.

@@ +394,5 @@
> +                                    der::CONSTRUCTED | der::CONTEXT_SPECIFIC);
> +    if (!nextUpdateWrappedWrapped) {
> +      return nullptr;
> +    }
> +  }

Blank line here.

@@ +468,5 @@
> +// CertStatus ::= CHOICE {
> +//    good                [0] IMPLICIT NULL,
> +//    revoked             [1] IMPLICIT RevokedInfo,
> +//    unknown             [2] IMPLICIT UnknownInfo }
> +// TODO: pass in status here

I don't understand this TODO.

@@ +473,5 @@
> +SECItem*
> +CertStatus(OCSPResponseContext& context)
> +{
> +  // UnknownInfo ::= NULL
> +  if (context.certStatus == 0 || context.certStatus == 2) {

The comment is confusing. I suggest you handle the good case separately from unknown, or improve the comment to explain that an empty UnknownInfo and NULL have the same encoding.

@@ +487,5 @@
> +  // RevokedInfo ::= SEQUENCE {
> +  //    revocationTime              GeneralizedTime,
> +  //    revocationReason    [0]     EXPLICIT CRLReason OPTIONAL }
> +  //
> +  if (context.certStatus == 1) {

IMO, a switch on context.certStatus would be better. Then it would be clearer that we're not handling cases other than 0-2. We should have a PR_NOT_REACHED and fail for other values.

::: security/manager/ssl/tests/unit/test_ocsp_required.js
@@ -57,2 @@
>    add_connection_test("ocsp-stapling-none.example.com",
> -                      getXPCOMStatusFromNSS(SEC_ERROR_OCSP_INVALID_SIGNING_CERT));

Why does this patch make this change? Is this change in the wrong patch?

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ +38,5 @@
> +  PRTime now = PR_Now();
> +  PRTime oneDay = 60*60*24 * (PRTime)PR_USEC_PER_SEC;
> +  //PRTime expiredTime = now - oneDay;
> +  PRTime oldNow = now - (8 * oneDay);
> +  //PRTime oldNextUpdate = oldNow + 10 * PR_USEC_PER_SEC;

What's with the two commented-out lines? Are they staying or going?

@@ +52,5 @@
> +  context.certIDHashAlg = SEC_OID_SHA1;
> +  context.responseStatus = 0;
> +  context.certStatus = 0;
> +  context.signTime = now;
> +  context.responderIDType = insanity::pkix::OCSPResponseContext::ByKeyHash;

Please sort the above field assignments according to the order that the fields appear in the OCSP response.

@@ +65,5 @@
> +    context.thisUpdate = oldNow;
> +    context.nextUpdate = oldNow + 10 * PR_USEC_PER_SEC;
> +  }
> +
> +  ScopedCERTCertificate otherCert;

If you change the context to own the certificates it references, then perhaps these local variables will be unnecessary?

@@ +113,5 @@
> +  } else if (aORT == ORTNeedsSig) {
> +    context.responseStatus = 5;
> +  } else if (aORT == ORTUnauthorized) {
> +    context.responseStatus = 6;
> +  }

Y U HATE SWITCH?

@@ +123,5 @@
> +  }
> +  mozilla::psm::NSSCertDBTrustDomain td(trustSSL, false, false, nullptr);
> +  if (VerifyEncodedOCSPResponse(td, aCert, context.signerCert, now, response)
> +        != SECSuccess) {
> +    PrintPRError("VerifyEncodedOCSPResponse failed (which might be expected)");

If we're going to do this, let's figure out whether we're expected to succeed or fail and handle it accordingly.

::: security/manager/ssl/tests/unit/tlsserver/lib/moz.build
@@ +10,5 @@
>  ]
>  
> +LOCAL_INCLUDES += [
> +    '../../../../../../certverifier',
> +    '../../../../../../insanity/include',

Beautiful.

Comment 4

[Camilo Viecco (:cviecco)]

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

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

great work, besides brian's comments (yes please use switch) my only other issue is:
1. I think is worth it making the length der calculations being placed in a single place.

::: security/insanity/lib/pkixder.h
@@ +525,5 @@
> +  } else {
> +    data[0] = 0x82;
> +    data[1] = length / 256;
> +    data[2] = length % 256;
> +  }

what about things that whose size is > 2^16 ?, also dont you need the number of bytes encoded in sqash?

::: security/insanity/lib/pkixocspcreate.cpp
@@ +59,5 @@
> +    return SHA384_LENGTH;
> +  }
> +  if (hashAlg == SEC_OID_SHA512) {
> +    return SHA512_LENGTH;
> +  }

+1

@@ +63,5 @@
> +  }
> +  return 0;
> +}
> +
> +SECItem*

static?

@@ +87,5 @@
> +  return output.Squash(arena, der::OCTET_STRING);
> +}
> +
> +SECItem*
> +AlgorithmIdentifier(PLArenaPool* arena, SECOidTag algTag)

static?

::: security/manager/ssl/tests/unit/test_ocsp_required.js
@@ -57,2 @@
>    add_connection_test("ocsp-stapling-none.example.com",
> -                      getXPCOMStatusFromNSS(SEC_ERROR_OCSP_INVALID_SIGNING_CERT));

+1

Comment 5

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

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

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

::: security/certverifier/moz.build
@@ +9,5 @@
>      '../insanity/lib/pkixcheck.cpp',
>      '../insanity/lib/pkixder.cpp',
>      '../insanity/lib/pkixkey.cpp',
>      '../insanity/lib/pkixocsp.cpp',
> +    '../insanity/lib/pkixocspcreate.cpp',

Actually, I guess my brain thought about this more while I was sleeping. I don't think that this should go into insanity/lib but I do think it makes a lot of sense for it to be in insanity::pkix. In my other OCSP testing patches, I had created an insanity/test (or gtest) subdirectory where test-specific code lives. So, instead of changing this to be under security/manager/ssl/tests/unit/, please put it in insanity/test/<somewhere>.

This also solves the problem that my originally suggestion would have created, where security/manager would have had to start depending on the private headers in security/insanity/lib. It is OK for security/insanity/test to depend on the private stuff in security/insanity/lib.

Comment 6

[Dana Keeler (she/her) [:keeler]]

(In reply to Camilo Viecco (:cviecco) from comment #4)
> 1. I think is worth it making the length der calculations being placed in a
> single place.

I'm not sure what you mean here.

> ::: security/insanity/lib/pkixder.h
> @@ +525,5 @@
> > +  } else {
> > +    data[0] = 0x82;
> > +    data[1] = length / 256;
> > +    data[2] = length % 256;
> > +  }
> 
> what about things that whose size is > 2^16 ?, also dont you need the number
> of bytes encoded in sqash?

The total length is prevented from growing beyond 2^16 - 1 in Add.
I don't know what you mean by your second question.

> @@ +63,5 @@
> > +  }
> > +  return 0;
> > +}
> > +
> > +SECItem*
> 
> static?

Well, it's not part of a class, so I'm not sure it would be useful to be static.

Comment 7

[Dana Keeler (she/her) [:keeler]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #3)
> ::: security/manager/ssl/tests/unit/test_ocsp_required.js
> @@ -57,2 @@
> >    add_connection_test("ocsp-stapling-none.example.com",
> > -                      getXPCOMStatusFromNSS(SEC_ERROR_OCSP_INVALID_SIGNING_CERT));
> 
> Why does this patch make this change? Is this change in the wrong patch?

(Note that the change is from SEC_ERROR_OCSP_INVALID_SIGNING_CERT to SEC_ERROR_OCSP_BAD_SIGNATURE.) The way OCSP responses with bad signatures are generated has changed a little. The old way was to have the signature consist of a single byte. In addition, the responder ID was a hash of that byte, which doesn't make any sense at all, because no key would ever hash to that value, so the verification code would always just say the signing cert was invalid, which isn't what we wanted to test. So, this fixes that bogosity.

Comment 8

[Dana Keeler (she/her) [:keeler]]

Attachment: patch v2

Comment 9

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8386274 [details] [diff] [review]
patch v2

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

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ +121,5 @@
> +  mozilla::psm::NSSCertDBTrustDomain td(trustSSL,
> +    mozilla::psm::NSSCertDBTrustDomain::NeverFetchOCSP, nullptr);
> +  if (VerifyEncodedOCSPResponse(td, aCert, context.signerCert.get(), now,
> +                                response) != SECSuccess) {
> +    PrintPRError("VerifyEncodedOCSPResponse failed (which might be expected)");

Oh, shoot. I forgot to address this (i.e. figuring out when we're expecting this to fail). I'll take care of it on the next revision.

Comment 10

[Camilo Viecco (:cviecco)]

Comment on attachment 8386274 [details] [diff] [review]
patch v2

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

Just nits from me. Logic is r+

::: security/insanity/include/insanity/pkixtypes.h
@@ +109,5 @@
>    TrustDomain(const TrustDomain&) /* = delete */;
>    void operator=(const TrustDomain&) /* = delete */;
>  };
>  
> +class OCSPResponseContext

I think this class name makes things confusing. I would change it to:
OCSPResponseGenerationContext

and Context 
to OCSPResponseContext

::: security/manager/ssl/tests/unit/test_ocsp_required.js
@@ +48,5 @@
>      run_next_test();
>    });
>  
>    add_connection_test("ocsp-stapling-none.example.com",
> +                      getXPCOMStatusFromNSS(SEC_ERROR_OCSP_BAD_SIGNATURE));

how come these ganges affect the classic code?

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ +76,4 @@
>        return nullptr;
>      }
>    }
> +  switch (aORT) {

For clarity, please move the initialization of  context.responseStatus to just above the cert OR explicitly set a default to 0. And for all other initializations too

Comment 11

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

Comment on attachment 8386274 [details] [diff] [review]
patch v2

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

The main things I want to see if where the code ends up and how we deal with the verification of the generated OCSP response.

::: security/certverifier/moz.build
@@ +21,5 @@
>      ]
>  
>  LOCAL_INCLUDES += [
>      '../insanity/include',
> +    '../insanity/lib/', # this is for ../insanity/test/pkixocspgeneration.cpp, but we probably want to do this differently...

See my latest patch in bug 916629. If we put it in insanity/test/lib it will make more sense, right?

::: security/insanity/include/insanity/pkix.h
@@ +104,5 @@
>                                    const CERTCertificate* issuerCert);
>  
> +// The return value, if non-null, is owned by the arena in the context
> +// and MUST NOT be freed.
> +SECItem* CreateEncodedOCSPResponse(OCSPResponseContext& context);

please put this in security/insanity/test/lib/pkixtestutils.h. (You can just create the file since the patches from bug 916629 won't be checked in when this gets checked in.)

::: security/insanity/test/pkixocspgeneration.cpp
@@ +94,5 @@
> +        data[1] = length / 256;
> +        data[2] = length % 256;
> +        break;
> +      default:
> +        PR_NOT_REACHED("EncodeLength: bad lengthLength");

Add PR_Abort();

@@ +118,5 @@
> +SECItem* ResponderID(OCSPResponseContext& context);
> +SECItem* KeyHash(OCSPResponseContext& context);
> +SECItem* SingleResponse(OCSPResponseContext& context);
> +SECItem* CertID(OCSPResponseContext& context);
> +SECItem* CertStatus(OCSPResponseContext& context);

These should all be static.

@@ +130,5 @@
> +  }
> +  return output.Squash(arena, tag);
> +}
> +
> +size_t

Add a comment that a return value of zero indicates failure.

@@ +177,5 @@
> +  aid.algorithm.len = 0;
> +  aid.parameters.data = nullptr;
> +  aid.parameters.len = 0;
> +  if (SECOID_SetAlgorithmID(arena, &aid, algTag, nullptr)
> +        != SECSuccess) {

NIT: One line

@@ +186,5 @@
> +    { SEC_ASN1_OBJECT_ID, offsetof(SECAlgorithmID, algorithm) },
> +    { SEC_ASN1_OPTIONAL | SEC_ASN1_ANY, offsetof(SECAlgorithmID, parameters) },
> +    { 0 }
> +  };
> +  SECItem *algorithmID = SEC_ASN1EncodeItem(arena, nullptr, &aid,

Nit: "SECItem*" not "SECItem *"

@@ +302,5 @@
> +  if (!tbsResponseData) {
> +    return nullptr;
> +  }
> +
> +  // Apparently PK11_FindKeyByAnyCert takes a non-const CERTCertificate*

You don't need this comment any more, right? Also, you can file a bug for changing the type of PK11_FindKeyByAnyCert to const CERTCertificate* if you want. I've made similar changes to NSS before many times.

@@ +417,5 @@
> +    contents = KeyHash(context);
> +    if (!contents) {
> +      return nullptr;
> +    }
> +  } else {

Should we be calling PR_SetError()? Does this code respect the NSPR error code conventions? We should mention this in the header file.

@@ +436,5 @@
> +//                          -- bits] in the responder's certificate)
> +SECItem*
> +KeyHash(OCSPResponseContext& context)
> +{
> +  SECItem spk = context.signerCert->subjectPublicKeyInfo.subjectPublicKey;

Add a comment that this is a shallow copy so people don't think this is a bug.

@@ +463,5 @@
> +                                                   context.thisUpdate);
> +  if (!thisUpdateEncoded) {
> +    return nullptr;
> +  }
> +  SECItem *nextUpdateEncodedNested = nullptr;

"SECItem* " not "SECItem *".

may be good to do a search-and-replace in case I missed any others.

@@ +472,5 @@
> +      return nullptr;
> +    }
> +    nextUpdateEncodedNested = EncodeNested(context.arena,
> +                                           der::CONSTRUCTED |
> +                                           der::CONTEXT_SPECIFIC,

add " | 0" for the tag, for clarity.

@@ +516,5 @@
> +                                              context.certIDHashAlg);
> +  if (!issuerNameHash) {
> +    return nullptr;
> +  }
> +  SECItem spk = context.issuerCert->subjectPublicKeyInfo.subjectPublicKey;

Add a comment that this is a shallow copy.

@@ +519,5 @@
> +  }
> +  SECItem spk = context.issuerCert->subjectPublicKeyInfo.subjectPublicKey;
> +  DER_ConvertBitString(&spk); // bits to bytes
> +  SECItem* issuerKeyHash = HashedOctetString(context.arena, &spk,
> +                                             context.certIDHashAlg);

This is basically the same code as KeyHash. Consider factoring it out into a function.

@@ +564,5 @@
> +SECItem*
> +CertStatus(OCSPResponseContext& context)
> +{
> +  switch (context.certStatus) {
> +    case 0:

Please add a comment explaining why [0] and [1] share code even though the ASN.1 definitions look pretty different.

@@ +581,5 @@
> +      SECItem* revocationTime = PRTimeToEncodedTime(context.arena,
> +                                                    context.revocationTime);
> +      if (!revocationTime) {
> +        return nullptr;
> +      }

Add a comment saying that we skip the revocation reason.

We should have a follow-up bug to make it possible to add the revocation reason so we can write a test to make sure we don't die when we parse an OCSP response that has one.

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ +76,4 @@
>        return nullptr;
>      }
>    }
> +  switch (aORT) {

I think he's trying to initialize everything in order that the members appear in the structure.

@@ +91,5 @@
> +      break;
> +    case ORTUnauthorized:
> +      context.responseStatus = 6;
> +      break;
> +    default:

context.responseStatus = 0;

@@ +121,5 @@
> +  mozilla::psm::NSSCertDBTrustDomain td(trustSSL,
> +    mozilla::psm::NSSCertDBTrustDomain::NeverFetchOCSP, nullptr);
> +  if (VerifyEncodedOCSPResponse(td, aCert, context.signerCert.get(), now,
> +                                response) != SECSuccess) {
> +    PrintPRError("VerifyEncodedOCSPResponse failed (which might be expected)");

Perhaps we should just remove this verification so that we can put this into insanity/test/lib. And/or we can create our own simple TrustDomain implementation in this file.

Comment 12

[Dana Keeler (she/her) [:keeler]]

(In reply to Camilo Viecco (:cviecco) from comment #10)
> > +class OCSPResponseContext
> 
> I think this class name makes things confusing. I would change it to:
> OCSPResponseGenerationContext

This got moved to a file/directory that's just for tests - is the name still a problem there? The reason I ask is adding "Generation" to the name makes it even more cumbersomely long.

> and Context 
> to OCSPResponseContext

This was already here (although now the two classes aren't declared in the same file). If we want to change the name of this, that can be a follow-up bug.

> ::: security/manager/ssl/tests/unit/test_ocsp_required.js
> @@ +48,5 @@
> >      run_next_test();
> >    });
> >  
> >    add_connection_test("ocsp-stapling-none.example.com",
> > +                      getXPCOMStatusFromNSS(SEC_ERROR_OCSP_BAD_SIGNATURE));
> 
> how come these ganges affect the classic code?

The classic code is now consuming responses generated from this new code, and since the new code tests what we actually want to test, the result is different here (see comment 7).

Comment 13

[Dana Keeler (she/her) [:keeler]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #11)
> @@ +186,5 @@
> > +    { SEC_ASN1_OBJECT_ID, offsetof(SECAlgorithmID, algorithm) },
> > +    { SEC_ASN1_OPTIONAL | SEC_ASN1_ANY, offsetof(SECAlgorithmID, parameters) },
> > +    { 0 }
> > +  };
> > +  SECItem *algorithmID = SEC_ASN1EncodeItem(arena, nullptr, &aid,
> 
> Nit: "SECItem*" not "SECItem *"

Ah - hoisted by my own petard.

Comment 14

[Dana Keeler (she/her) [:keeler]]

(In reply to David Keeler (:keeler) from comment #6)
> (In reply to Camilo Viecco (:cviecco) from comment #4)
> > @@ +63,5 @@
> > > +  }
> > > +  return 0;
> > > +}
> > > +
> > > +SECItem*
> > 
> > static?
> 
> Well, it's not part of a class, so I'm not sure it would be useful to be
> static.

Nevermind - I was confused about what static meant in this context.

Comment 15

[Dana Keeler (she/her) [:keeler]]

(In reply to Camilo Viecco (:cviecco) from comment #10)
> @@ +76,4 @@
> >        return nullptr;
> >      }
> >    }
> > +  switch (aORT) {
> 
> For clarity, please move the initialization of  context.responseStatus to
> just above the cert OR explicitly set a default to 0. And for all other
> initializations too

The ordering of the fields in OCSPResponseContext is the order in which they appear in the encoding of a response. Similarly, the initialization and configuration of context is in the same order (unless I made a mistake there).

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #11)
> @@ +417,5 @@
> > +    contents = KeyHash(context);
> > +    if (!contents) {
> > +      return nullptr;
> > +    }
> > +  } else {
> 
> Should we be calling PR_SetError()? Does this code respect the NSPR error
> code conventions? We should mention this in the header file.

I don't think it's particularly useful to call PR_SetError in this code, but if any NSS/NSPR function fails, we basically bail and return a null pointer all the way up, so the caller can call PR_GetError to see what the error was.

Comment 16

[Dana Keeler (she/her) [:keeler]]

Attachment: patch v3

Thank you to both of you for your quick yet thorough reviews. (And apologies for the bugspam.)

Comment 17

[Camilo Viecco (:cviecco)]

Comment on attachment 8387095 [details] [diff] [review]
patch v3

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

r+ with comments addressed

::: security/insanity/test/lib/pkixtestutil.h
@@ +21,5 @@
> +#include "insanity/ScopedPtr.h"
> +#include "insanity/pkixtypes.h"
> +#include "seccomon.h"
> +
> +namespace insanity { namespace pkix {

namespace test {?

@@ +24,5 @@
> +
> +namespace insanity { namespace pkix {
> +
> +typedef ScopedPtr<SECKEYPrivateKey, SECKEY_DestroyPrivateKey>
> +        ScopedSECKEYPrivateKey;

move this to pkixtypes

@@ +26,5 @@
> +
> +typedef ScopedPtr<SECKEYPrivateKey, SECKEY_DestroyPrivateKey>
> +        ScopedSECKEYPrivateKey;
> +
> +class OCSPResponseContext

I understand why you want to keep this name and I am fine with It as long we we place it in some other name space:

isanity::pkix::test?

Comment 18

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

Comment on attachment 8387095 [details] [diff] [review]
patch v3

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

I think I've reviewed this enough times, so I only reviewed the location of the files and how we verify the generate responses in GenerateOCSPResponse.cpp/OCSPCommon.cpp. Please sort out the rest with cviecco.

::: security/insanity/test/lib/pkixtestutil.h
@@ +24,5 @@
> +
> +namespace insanity { namespace pkix {
> +
> +typedef ScopedPtr<SECKEYPrivateKey, SECKEY_DestroyPrivateKey>
> +        ScopedSECKEYPrivateKey;

It shouldn't be in pkixtypes.h because this is only needed for testing. (Certificate verification only uses public keys, never private keys.)

@@ +26,5 @@
> +
> +typedef ScopedPtr<SECKEYPrivateKey, SECKEY_DestroyPrivateKey>
> +        ScopedSECKEYPrivateKey;
> +
> +class OCSPResponseContext

I think the name is fine. Even "Context" would be fine since it is defined and used solely within this file.

Note that you don't need to use a named namespace for it. namespace insanity { namespace pkix { namespace { } } } is better.

Comment 19

[Dana Keeler (she/her) [:keeler]]

Attachment: patch v3.1

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #18)
> @@ +26,5 @@
> > +
> > +typedef ScopedPtr<SECKEYPrivateKey, SECKEY_DestroyPrivateKey>
> > +        ScopedSECKEYPrivateKey;
> > +
> > +class OCSPResponseContext
> 
> I think the name is fine. Even "Context" would be fine since it is defined
> and used solely within this file.
> 
> Note that you don't need to use a named namespace for it. namespace insanity
> { namespace pkix { namespace { } } } is better.

Well, but it's used in OCSPCommon.cpp, so it can't be anonymous.

Comment's addressed - carrying over r+es.

Here's how try went: https://tbpl.mozilla.org/?tree=Try&rev=fc5c49a52d66

Comment 20

[Dana Keeler (she/her) [:keeler]]

(In reply to David Keeler (:keeler) from comment #19)
> Comment's addressed - carrying over r+es.

Or, rather, "Comments".

Also, inbound is closed, so I'm just going to rely on checkin-needed magic here.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec47c16ac24d

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec47c16ac24d