Closed Bug 1390131 Opened 5 years ago Closed 5 years ago

Modify GenerateOCSPResponse.cpp to allow thisUpdate to be modified.

Categories

(Core :: Security: PSM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: mgoodwin, Assigned: mgoodwin)

References

Details

Attachments

(1 file)

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 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?
Attachment #8897111 - Flags: review?(dkeeler) → review+
Pushed by mgoodwin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e2bbc07c946
Modify GenerateOCSPResponse.cpp to allow thisUpdate to be modified r=keeler
https://hg.mozilla.org/mozilla-central/rev/2e2bbc07c946
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.