916629 - Add OCSP unit tests

Status: RESOLVED FIXED

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

Description

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

Attachment: Part 1: Add gyp build script

Comment 1

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

Attachment: Part 2: Generate OCSP error responses for testing

The gyp file almost definitely needs to be tweaked to build on Linux. It would be great if you could make those tweaks so that everything builds with -Werror -Wall and other appropriate flags.

You can download gyp by doing "svn co http://gyp.googlecode.com/svn/trunk".

Note that the gyp script is just for our convenience; it won't be part of the Gecko build process. It allows us to have a much faster edit/build/test cycle  than we get when we go through the Gecko build process and also helps us ensure that insanity doesn't have any hidden dependencies on the rest of Gecko. gyp is especially convenient on Windows because it can generate Visual Studio project files so that building is just "press F7" and testing/debugging is just "press F5."

Comment 2

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

Attachment: Part 3: Generate successful OCSP responses signed directly by the CA issuing the cert that the OCSP response is for

Comment 3

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

Attachment: Part 4: Generate successful OCSP responses with a missing delegated OCSP responder

Comment 4

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

These four patches implement the generation of OCSP responses that are useful for various tests. They are used by the GTest-based tests I have written for testing insanity::pkix's OCSP parsing/verification and they should also make it easier to implement the mock OCSP responder that cviecco is working on--the mock OCSP responder should basically just need to get configured with the path to one of the generate files. Then when it gets the request, it just needs to read that file and return it as the response body, after setting the correct HTTP headers.

It should be easy to adapt the generate.sh script so that it can be executed as part of our test infrastructure on desktop platforms. We may need to change how we build NSS so that we start building the certutil tool and the secutil library, if we aren't already. On B2G or Android, things might be a little more tricky given that those platforms don't have a real shell.

Comment 5

[Camilo Viecco (:cviecco)]

Comment on attachment 805081 [details] [diff] [review]
Part 1: Add gyp build script

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

Looks good but:
-has wrong file locations, (require sed)
- only works on windows (and I dare to say only win7)

Comment 6

[Camilo Viecco (:cviecco)]

Comment on attachment 805085 [details] [diff] [review]
Part 2: Generate OCSP error responses for testing

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

Build related issues. but ovreall looks good. It would be feedback+ but r-

::: test/GenerateOCSPResponses.cpp
@@ +26,5 @@
> +#include "nss.h"
> +#include "ocsp.h"
> +#include "pk11pub.h"
> +extern "C" {
> +#include "secutil.h"

secutil is not available outside nss. => All calls to SECU_* are  not compilable. As this is a standalone program, you could replace them with fprintfs....

::: test/generate.sh
@@ +11,5 @@
> +
> +set -e
> +set -x
> +
> +bin=../../dist/Debug

is this needed?

::: test/pkixtestutil.cpp
@@ +11,5 @@
> +using namespace insanity::pkix;
> +using namespace std;
> +
> +namespace {
> +inline void fclose_void(FILE* file) { (void) std::fclose(file); }

this was also defined in the .hg

::: test/pkixtestutil.h
@@ +13,5 @@
> +
> +namespace insanity { namespace testing {
> +
> +namespace {
> +inline void fclose_void(FILE * file) { (void) std::fclose(file); }

why define it here and in the cpp?

Comment 7

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

Hi,

I agree with all your review comments. My understanding is that you're trying to integrate this into your tests for OCSP for EV. Can you just attach the interdiff between what I've written and what works for what you're doing here?

It would be great to have access to secutil in these test programs. If we already build certutil then we're already building the secutil library, because certutil uses it. If the issue is that secutil.h cannot be found, then I think all you have to do is add $(DIST)/private/nss to LOCAL_INCLUDES, or something like that.

As for the paths within generate.sh that are currently hard-coded to ../dist/Debug: Wherever you put this program to get your tests to work is fine with me. Feel free to change generate.sh to whatever works.

Comment 8

[Camilo Viecco (:cviecco)]

Comment on attachment 805085 [details] [diff] [review]
Part 2: Generate OCSP error responses for testing

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

::: test/pkixtestutil.cpp
@@ +37,5 @@
> +    }
> +    file = rawFile;
> +  }
> +#else
> +  file = fopen(path.c_str(), mode);

There is no c_str. There is get().

Comment 9

[Camilo Viecco (:cviecco)]

Comment on attachment 805086 [details] [diff] [review]
Part 3: Generate successful OCSP responses signed directly by the CA issuing the cert that the OCSP response is for

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

mostly need some nit fixes.

::: test/GenerateOCSPResponses.cpp
@@ +61,5 @@
> +
> +  /* Non-owning; the response will be owned by the arena */
> +  CERTOCSPSingleResponse** responses = PORT_ArenaNewArray(arena.get(), CERTOCSPSingleResponse*, 2);
> +  responses[0] = CERT_CreateOCSPSingleResponseGood(arena.get(), cid.get(),
> +                                                    thisUpdate, nextUpdate);

nit alignment

@@ +86,5 @@
> +  if (!cid) {
> +    return nullptr;
> +  }
> +
> +  /* Non-owning; the response will be owned by the arena */ 

nit: spaces

@@ +89,5 @@
> +
> +  /* Non-owning; the response will be owned by the arena */ 
> +  CERTOCSPSingleResponse* responses[2] = { nullptr, nullptr };
> +  responses[0] = CERT_CreateOCSPSingleResponseRevoked(arena.get(), cid.get(),
> +                                                    thisUpdate, nextUpdate,

nit align

@@ +164,5 @@
> +bool
> +CertFromDatabase(const char* nickname, ScopedCERTCertificate& cert)
> +{
> +  cert = CERT_FindCertByNickname(CERT_GetDefaultCertDB(), nickname);
> +  return !!cert;

!! cert? can you just do cert and allow for autoconversion?

@@ +203,5 @@
>    const char* dbdir = argv[1];
>    const char* outDir = argv[2];
>  
> +  const PRTime now = PR_Now();
> +  const PRTime oneWeekAfterNow = now + (7 * ONE_DAY);

We still need to figure out how to plug this into the build system so that these ocsp requriest are generated at build time

@@ +252,5 @@
> +  if (!CertFromDatabase("ee1", endEntity)) {
> +    exit(1);
> +  }
> +
> +  if (!WriteResponse(outDir, "good_direct_byKey_with_nextUpdate.ocsp",

this feels like a nice loop... but is OK as is

Comment 10

[Camilo Viecco (:cviecco)]

Comment on attachment 805088 [details] [diff] [review]
Part 4: Generate successful OCSP responses with a missing delegated OCSP responder

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

::: test/GenerateOCSPResponses.cpp
@@ +321,5 @@
> +  ScopedCERTCertList ocsp_signer_missing_1;
> +  if (!ChainFromDatabase("ocsp_signer_missing_1", ocsp_signer_missing_1)) {
> +    fprintf(stderr, "failed to read ocsp_signer_missing_1 cert\n");
> +    exit(1);
> +  }

do we care on the next-update for these cases?

Comment 11

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

(In reply to Camilo Viecco (:cviecco) from comment #5)
> Comment on attachment 805081 [details] [diff] [review]
> Part 1: Add gyp build script
> 
> Looks good but:
> -has wrong file locations, (require sed)

I will fix this at landing time.

> - only works on windows (and I dare to say only win7)

Since this is and will be not part of the build (NPOTB), but at the same time is very useful for me to actually get work done by making Visual Studio understand the code much better, just just call it Windows-only for now. We can add support for other platforms later if the gyp file is useful to others.

Comment 12

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

(In reply to Brian Smith (:briansmith, was :[e-mail]) from comment #11)
> Since this is and will be not part of the build (NPOTB), but at the same
> time is very useful for me to actually get work done by making Visual Studio
> understand the code much better, just just call it Windows-only for now. We
> can add support for other platforms later if the gyp file is useful to
> others.

Also, nearly every single one of my insanity::pkix patches depends on this patch because they all update the gyp file.

Comment 13

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

Comment on attachment 805081 [details] [diff] [review]
Part 1: Add gyp build script

I just stripped the gyp-related stuff from my repo. I also couldn't figure out how to make gyp work on any platform but Windows, so I will just keep the gyp file in my own local tree and not expose it for now.

Comment 14

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

Attachment: Part 1: Generate OCSP responses for testing

Part 1: Generate OCSP error responses for testing

Review comments addressed.

Comment 15

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

Attachment: Part 2: Add good OCSP responses directly signed by the cert issuer for testing

Comment 16

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

Attachment: Part 3: Generate OCSP test responses for tesitng missing OCSP responder certificate

Comment 17

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

Camilo, you mentioned that you got this building/running using Gecko's build system. Could you post that patch?

Comment 18

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

Attachment: Part 1: Generate OCSP responses for testing

Comment 19

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

Attachment: Part 2: Add good OCSP responses directly signed by the cert issuer for testing

Comment 20

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

Attachment: Part 3: Generate OCSP test responses for tesitng missing OCSP responder certificate

Comment 21

[Camilo Viecco (:cviecco)]

Attachment: Builds on Linux

SO this is the patch of integration with the build system... but it is really bad as has unixlike names for libs.

Comment 22

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

Comment on attachment 812455 [details] [diff] [review]
Part 1: Generate OCSP responses for testing

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

::: security/insanity/test/pkixtestutil.h
@@ +24,5 @@
> +
> +namespace insanity { namespace testing {
> +
> +namespace {
> +inline void fclose_void(FILE * file) { (void) std::fclose(file); }

whitespace.

@@ +30,5 @@
> +
> +typedef insanity::pkix::ScopedPtr<std::FILE, fclose_void> ScopedFILE;
> +
> +std::FILE *
> +OpenFile(const char * dir, const char * filename, const char * mode);

whitespace.

Comment 23

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

I need to update the patches to conform to the style guide w.r.t. whitespace and line length, but the patches probably work fine.

David (or Camilo), once I've posted these patches, can you figure out how to get these tests to build and run as part of Gecko? I can also figure it out, but I suspect it will be faster for you to do it due to the other bugs I have assigned to me, and I suspect that this will soon be blocking your other OCSP-related insanity::pkix bugs.

Comment 24

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

I attached a bunch more GTest-based unit tests for OCSP to bug 915931, but I decided to move them to this bug.

Comment 25

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

Attachment: Part 1(a): Build GenerateOCSPResponses and the test lib

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8384905 [details] [diff] [review]
Part 1(a): Build GenerateOCSPResponses and the test lib

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

::: security/insanity/test/cmd/Makefile.in
@@ +22,5 @@
> +  ../../../nss/cmd/lib/$(LIB_PREFIX)sectool.$(LIB_SUFFIX) \
> +  ../lib/$(LIB_PREFIX)pkixtestutil.$(LIB_SUFFIX) \
> +  $(NULL)
> +
> +DEFINES += $(TK_CFLAGS)

TK_CFLAGS should be available as CONFIG['TK_CFLAGS'] in moz.build and you can now assign CFLAGS and CXXFLAGS in moz.build files. Please try to move this.

Comment 27

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

From: http://blog.bro.org/2014/03/dissecting-gnutls-bug.html
"From an attacker perspective, making the signature lookup fail is probably the easiest method to exploit the bug. According to the function _gnutls_x509_get_signature, the signature verification fails if the number of bits in the certificate signature is not divisible by 8. Certificate signatures have an ASN.1 bit string encoding, which theoretically may exhibit an arbitrary number of zeros and ones, and may not necessarily be divisible by 8. But the GnuTLS function assumes a byte-aligned signature, hence the check. In the ASN.1 DER encoding, used in X509 certificates, a bit string begins with a single byte which denotes the number of missing bits in the last byte. For all normal certificates, this value is always zero, and the function continues. But if we change this byte to a non-zero value, the function fails the divisibility check and returns with wrong error code, causing the certificate verification to succeed."

We also do the same check for OCSP response signatures. We should try to test this.

Comment 28

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

Attachment: Part 1(a): Build GenerateOCSPResponses and the test lib [v2]

This addresses gps's request to remove the TK_CFLAGS stuff to moz.build.

It also changes security/manager/ssl/tests/tlsserver/moz.build to do:

TEST_DIRS += [
    'lib',
]

TEST_TOOL_DIRS += [
    'cmd',
]

and it changes security/insanity/moz.build to do:

TEST_DIRS += [
    'test/lib',
    'test/gtest',
]

TEST_TOOL_DIRS += [
    'test/cmd',
]

I made these latter two changes based on build failures on tryserver. Here is the newest tryserver run, still in progress:

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

Comment 29

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

It seems like we need a test for the case where there are two SingleResponses in the OCSP response and the first one doesn't match. See bug 977870.

Comment 30

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

Comment on attachment 812455 [details] [diff] [review]
Part 1: Generate OCSP responses for testing

This patch was moved to bug 983853.

Comment 31

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

Comment on attachment 8391485 [details] [diff] [review]
Part 1(a): Build GenerateOCSPResponses and the test lib [v2]

This patch was moved to bug 983853.

Comment 32

[[:mmc] Monica Chew (no longer reading bugmail)]

Is this ready to land?

Comment 33

[[:mmc] Monica Chew (no longer reading bugmail)]

Hey Brian,

I'm trying to help David and Camilo finish up the mozpkix and pinning work. This bug is one of the last remaining blockers on https://etherpad.mozilla.org/mozilla--pkix. Do we need anything else from bug 984608? Do these need rebasing or re-reviewing? I blocked out a lot of time the week of 4/28 and am trying to pick off easier tasks before then to ramp up.

Thanks,
Monica

Comment 34

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

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #33)
> I'm trying to help David and Camilo finish up the mozpkix and pinning work.
> This bug is one of the last remaining blockers on
> https://etherpad.mozilla.org/mozilla--pkix. Do we need anything else from
> bug 984608? Do these need rebasing or re-reviewing? I blocked out a lot of
> time the week of 4/28 and am trying to pick off easier tasks before then to
> ramp up.

I've rebased them and also rewrote them. It looks like I need to rebase them again and then attach them to this bug. I will do so within the next few days.

Comment 35

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

Brian, what's the status of this? I think this is a higher priority than some of the refactoring that you're doing.

Comment 36

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

(In reply to David Keeler (:keeler) [use needinfo?] from comment #35)
> Brian, what's the status of this? I think this is a higher priority than
> some of the refactoring that you're doing.

Perhaps, but the refactorings actually make the tests easier to do. Regardless, here is where they are in my queue:
https://tbpl.mozilla.org/?tree=Try&rev=f15a3200668a

The patch for this bug is here:
5b8447f66dd3 Brian Smith – Bug 916629, Part 2: Add OCSP GTests

I will break it up into smaller pieces for review.

It depends on :

    f60de81085d4 Brian Smith – Bug 1035009
    6e8edb996022 Brian Smith – Bug 1035470
    e3414a0dc98f Brian Smith – SignedDataWithSignature
    acee7a3fe7f4 Brian Smith – factor out HashBuf

The last two bugs are not filed yet, and I need to write some tests for them, which I'm planning to do tomorrow. So, ETA for this is around Wednesday. Reviewing the refactoring patches (especially bug 1035009) will help move this along.

Comment 37

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

Attachment: Part 1: Add tests for OCSP responses without responseBytes

Comment 38

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

Attachment: Part 2: Add unit tests for "successful" OCSP responses signed directly by the issuer

Comment 39

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

Attachment: Part 3: Add unit tests for OCSP responses signed by delegated OCSP responder cert

Comment 40

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

Attachment: Part 4: Add unit tests for trust of delegated OCSP responder cert

Comment 41

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

Tryserver run:
https://tbpl.mozilla.org/?tree=Try&rev=826dfd8e7679

Comment 42

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

Comment on attachment 8454285 [details] [diff] [review]
Part 1: Add tests for OCSP responses without responseBytes

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

r=me with comments addressed.

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* Copyright 2013 Mozilla Foundation

nit: use the dual-license header

@@ +78,5 @@
> +    return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
> +                                             nullptr);
> +  }
> +
> +  virtual SECStatus DigestBuf(const SECItem& item, /*out*/ uint8_t *digestBuf,

nit: uint8_t*

@@ +131,5 @@
> +                                                      ++rootIssuedCount));
> +    if (!endEntitySerialNumber) {
> +      PR_Abort();
> +    }
> +    endEntityCertID = new (std::nothrow) CertID(*rootNameDER, *rootSPKI,

Does this ever get deleted?

@@ +173,5 @@
> +  { 4/*unused*/, SEC_ERROR_OCSP_UNKNOWN_RESPONSE_STATUS },
> +  { OCSPResponseContext::sigRequired, SEC_ERROR_OCSP_REQUEST_NEEDS_SIG },
> +  { OCSPResponseContext::unauthorized, SEC_ERROR_OCSP_UNAUTHORIZED_REQUEST },
> +  { OCSPResponseContext::unauthorized + 1,
> +    SEC_ERROR_OCSP_UNKNOWN_RESPONSE_STATUS

in this case it's probably fine to go over 80 chars, but your call

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +93,5 @@
>    return file.release();
>  }
>  
> +SECStatus
> +TamperOnce(SECItem& item,

Looks like this isn't used yet - it should probably go with the patch it's used in (unless you're planning to fold them all together?). Also, this implementation is slightly hard to follow without some comments explaining how it works.

::: security/pkix/test/lib/pkixtestutil.h
@@ +76,5 @@
>  const SECItem* ASCIIToDERName(PLArenaPool* arena, const char* cn);
>  
> +// Replace one substring in item with another of the same length, but only if
> +// the substring was found exactly once. The "only once" restriction is helpful
> +// for avoiding making multiple changes at once. 

nit: trailing whitespace

Comment 43

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

Comment on attachment 8454285 [details] [diff] [review]
Part 1: Add tests for OCSP responses without responseBytes

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

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ +153,5 @@
> +/*static*/ ScopedSECItem pkixocsp_VerifyEncodedResponse::rootSPKI;
> +/*static*/ long pkixocsp_VerifyEncodedResponse::rootIssuedCount = 0;
> +
> +// Alias for nullptr to aid readability in the code below.
> +static const char* byKey = nullptr;

This should go in the patches that use this.

Comment 44

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

Comment on attachment 8454286 [details] [diff] [review]
Part 2: Add unit tests for "successful" OCSP responses signed directly by the issuer

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

r=me with comments addressed.

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ +219,5 @@
> +class pkixocsp_VerifyEncodedResponse_successful
> +  : public pkixocsp_VerifyEncodedResponse
> +{
> +public:
> +  static bool SetUpTestCaseInner()

Isn't a lot of this setup shared with pkixocsp_VerifyEncodedResponse_WithoutResponseBytes? Could they both inherit from a pkixocsp_VerifyEncodedResponse_commonSetup class or something?

@@ +269,5 @@
> +                    /*optional*/ const char* signerName,
> +                    const ScopedSECKEYPrivateKey& signerPrivateKey,
> +                    PRTime producedAt, PRTime thisUpdate,
> +                    /*optional*/ const PRTime* nextUpdate,
> +                    /*optional*/ SECItem const* const* certs = nullptr)

Doesn't look like certs is used yet - let's omit it until it is used.

@@ +307,5 @@
> +                      OCSPResponseContext::good, *endEntityCertID, byKey,
> +                      rootPrivateKey, oneDayBeforeNow, oneDayBeforeNow,
> +                      &oneDayAfterNow));
> +  ASSERT_TRUE(response);
> +  bool expired;

Add ASSERT_FALSE(expired)? (Here and in the other tests)

@@ +331,5 @@
> +{
> +  SECItem* response(CreateEncodedOCSPSuccessfulResponse(
> +                      OCSPResponseContext::good, *endEntityCertID, byKey,
> +                      rootPrivateKey, oneDayBeforeNow, oneDayBeforeNow,
> +                      &oneDayAfterNow));

nullptr?

Comment 45

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

Comment on attachment 8454287 [details] [diff] [review]
Part 3: Add unit tests for OCSP responses signed by delegated OCSP responder cert

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

These look great.

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ +380,5 @@
> +  // we generate a new keypair on each call. Either one of these should be
> +  // sufficient to avoid issues with the NSS cache, but we do both to be
> +  // cautious.
> +  //
> +  // ocspResponseSignerName should be byKey to use the byKey ResponderID

Is this just "signerName" now?

@@ +472,5 @@
> +  SECItem* response(CreateEncodedIndirectOCSPSuccessfulResponse(
> +                      "CN=good_indirect_byKey", OCSPResponseContext::good,
> +                      byKey));
> +  ASSERT_TRUE(response);
> +  bool expired;

Add ASSERT_FALSE(expired); where appropriate?

Comment 46

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

Comment on attachment 8454286 [details] [diff] [review]
Part 2: Add unit tests for "successful" OCSP responses signed directly by the issuer

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

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ +219,5 @@
> +class pkixocsp_VerifyEncodedResponse_successful
> +  : public pkixocsp_VerifyEncodedResponse
> +{
> +public:
> +  static bool SetUpTestCaseInner()

You are right. Please see the new version of the patch.

@@ +269,5 @@
> +                    /*optional*/ const char* signerName,
> +                    const ScopedSECKEYPrivateKey& signerPrivateKey,
> +                    PRTime producedAt, PRTime thisUpdate,
> +                    /*optional*/ const PRTime* nextUpdate,
> +                    /*optional*/ SECItem const* const* certs = nullptr)

I'm not going to make this change, because I'm going to push the patch that would re-add certs and use it in the same hg push. In general, I agree that I should have done it the way you suggest. (Actually, all four of these parts used to be one patch, and I broke it up mostly to make it easier to review. The fact that each part builds independently of the following parts is just a side-effect of me trying to make each individual part easier to review incrementally.)

@@ +307,5 @@
> +                      OCSPResponseContext::good, *endEntityCertID, byKey,
> +                      rootPrivateKey, oneDayBeforeNow, oneDayBeforeNow,
> +                      &oneDayAfterNow));
> +  ASSERT_TRUE(response);
> +  bool expired;

OK. We need to add some tests that do ASSERT_TRUE(expired) too. (I originally wrote these tests way before we added the "expired" output parameter.)

@@ +331,5 @@
> +{
> +  SECItem* response(CreateEncodedOCSPSuccessfulResponse(
> +                      OCSPResponseContext::good, *endEntityCertID, byKey,
> +                      rootPrivateKey, oneDayBeforeNow, oneDayBeforeNow,
> +                      &oneDayAfterNow));

I do not understand what you mean here? Are you referring to byKey? If so, then yes, it is nullptr. I thought that "byKey" would make the test easier to understand, so I created a constant of that name that is nullptr. Let me know if you think it is too confusing. Or, let me know if you meant something else.

Comment 47

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

Comment on attachment 8454287 [details] [diff] [review]
Part 3: Add unit tests for OCSP responses signed by delegated OCSP responder cert

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

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ +380,5 @@
> +  // we generate a new keypair on each call. Either one of these should be
> +  // sufficient to avoid issues with the NSS cache, but we do both to be
> +  // cautious.
> +  //
> +  // ocspResponseSignerName should be byKey to use the byKey ResponderID

Yep.

@@ +472,5 @@
> +  SECItem* response(CreateEncodedIndirectOCSPSuccessfulResponse(
> +                      "CN=good_indirect_byKey", OCSPResponseContext::good,
> +                      byKey));
> +  ASSERT_TRUE(response);
> +  bool expired;

Done.

Comment 48

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

Comment on attachment 8454286 [details] [diff] [review]
Part 2: Add unit tests for "successful" OCSP responses signed directly by the issuer

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

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ +331,5 @@
> +{
> +  SECItem* response(CreateEncodedOCSPSuccessfulResponse(
> +                      OCSPResponseContext::good, *endEntityCertID, byKey,
> +                      rootPrivateKey, oneDayBeforeNow, oneDayBeforeNow,
> +                      &oneDayAfterNow));

Sorry - that wasn't clear. My understanding is this is testing a response without nextUpdate, thus requiring CreateEncodedOCSPSuccessfulResponse to be passed nullptr for its nextUpdate parameter (instead of &oneDayAfterNow).

Comment 49

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

(In reply to David Keeler (:keeler) [use needinfo?] from comment #48)
> Sorry - that wasn't clear. My understanding is this is testing a response
> without nextUpdate, thus requiring CreateEncodedOCSPSuccessfulResponse to be
> passed nullptr for its nextUpdate parameter (instead of &oneDayAfterNow).

Thanks for catching that! I fixed this before I pushed this to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3192a23f0884
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5c52b2046a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc520f8b95e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8700531ef25f

Thanks for the reviews! Couldn't have been fun to review ~900 lines of GTests.

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3192a23f0884
https://hg.mozilla.org/mozilla-central/rev/a5c52b2046a5
https://hg.mozilla.org/mozilla-central/rev/6dc520f8b95e
https://hg.mozilla.org/mozilla-central/rev/8700531ef25f