1390131 - Modify GenerateOCSPResponse.cpp to allow thisUpdate to be modified.

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Mark Goodwin [:mgoodwin]]

Various PSM tests make use of GenerateOCSPResponse.cpp for OCSP response generation. In some of these cases (in particular test_ocsp_caching.js) we employ timeouts to force differing thisUpdate values. This is fragile and causes test failures.

It would be preferable to allow thisUpdate to be specified in some way to ensure that thisUpdate values can differ without relying on timeouts.

Comment 1

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1390131 - Modify GenerateOCSPResponse.cpp to allow thisUpdate to be modified

Review commit: https://reviewboard.mozilla.org/r/168404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168404/

Comment 2

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

Comment on attachment 8897111 [details]
Bug 1390131 - Modify GenerateOCSPResponse.cpp to allow thisUpdate to be modified

https://reviewboard.mozilla.org/r/168404/#review173658

Looks good to me. I think if we ever need to add more parameters we'll have to figure out a better way of doing this, but this is fine for now.

::: security/manager/ssl/tests/unit/head_psm.js:544
(Diff revision 1)
>  }
>  
>  // Returns an Array of OCSP responses for a given ocspRespArray and a location
>  // for a nssDB where the certs and public keys are prepopulated.
>  // ocspRespArray is an array of arrays like:
>  // [ [typeOfResponse, certnick, extracertnick]...]

nit: documentation needs updating, looks like

::: security/manager/ssl/tests/unit/head_psm.js:566
(Diff revision 1)
>      do_print("argArray = " + argArray);
>  
>      let process = Cc["@mozilla.org/process/util;1"]
>                      .createInstance(Ci.nsIProcess);
>      process.init(ocspGenBin);
> -    process.run(true, argArray, 5);
> +    process.run(true, argArray, 6);

Presumably this should have always been `argArray.length`, right?

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:116
(Diff revision 1)
>  
>  int
>  main(int argc, char* argv[])
>  {
>  
> -  if (argc < 6 || (argc - 6) % 4 != 0) {
> +  if (argc < 7 || (argc - 7) % 5 != 0) {

So, we originally wrote this with the intention that we would send it a batch of responses to generate, but we never actually used it that way (generateOCSPResponses invokes the binary once per response request it's given). I'm not sure it's worth it to do this right now, but sometime we might think about reconciling these two behaviors one way or another).

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:138
(Diff revision 1)
>  
> -  for (int i = 2; i + 3 < argc; i += 4) {
> +  for (int i = 2; i + 3 < argc; i += 5) {
>      const char* ocspTypeText  = argv[i];
>      const char* certNick      = argv[i + 1];
>      const char* extraCertname = argv[i + 2];
>      const char* filename      = argv[i + 3];

Let's actually switch these so the input arguments come before the output arguments (so filename will be at i + 4 and skew will be i + 3).

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:139
(Diff revision 1)
> -  for (int i = 2; i + 3 < argc; i += 4) {
> +  for (int i = 2; i + 3 < argc; i += 5) {
>      const char* ocspTypeText  = argv[i];
>      const char* certNick      = argv[i + 1];
>      const char* extraCertname = argv[i + 2];
>      const char* filename      = argv[i + 3];
> +    const char* skewChars          = argv[i + 4];

nit: indentation

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:156
(Diff revision 1)
>        PR_fprintf(PR_STDERR, "Failed to find certificate with nick '%s'\n",
>                   certNick);
>        exit(EXIT_FAILURE);
>      }
>  
> +    time_t skew = (time_t) atoll(skewChars);

nit: let's use C++ casts

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.h:60
(Diff revision 1)
>  SECItemArray*
>  GetOCSPResponseForType(OCSPResponseType aORT,
>                         const mozilla::UniqueCERTCertificate& aCert,
>                         const mozilla::UniquePLArenaPool& aArena,
> -                       const char* aAdditionalCertName);
> +                       const char* aAdditionalCertName,
> +                       time_t aSkew);

nit: maybe aThisUpdateSkew?

Comment 3

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8897111 [details]
Bug 1390131 - Modify GenerateOCSPResponse.cpp to allow thisUpdate to be modified

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168404/diff/1-2/

Comment 4

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8897111 [details]
Bug 1390131 - Modify GenerateOCSPResponse.cpp to allow thisUpdate to be modified

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168404/diff/2-3/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e2bbc07c946
Modify GenerateOCSPResponse.cpp to allow thisUpdate to be modified r=keeler

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/2e2bbc07c946