Create standalone oscpResponseGenerator

RESOLVED FIXED in Firefox 27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cviecco, Assigned: cviecco)

Tracking

unspecified
mozilla28
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox27 fixed, firefox28 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 8 obsolete attachments)

To simplify and unifiy tests we should have common tool to generate ocsp responses given a pre-populated NSS certificate DB.
Blocks: 927016
Posted patch add-genocspresponse (obsolete) — Splinter Review
Attachment #832545 - Flags: review?(dkeeler)
Attachment #832545 - Flags: review?(dkeeler)
Posted patch add-genocspresponse (v1.1) (obsolete) — Splinter Review
Attachment #832545 - Attachment is obsolete: true
Attachment #832546 - Flags: review?(dkeeler)
Assignee: nobody → cviecco
Comment on attachment 832546 [details] [diff] [review]
add-genocspresponse (v1.1)

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

Good. There's some clean-up needed. In particular, some of your operators still don't have spaces around them.
Also, we still need a function in head_psm.js that we can call that will take an array of parameters and return an array of responses.

Here's what jst-review says:

L 45: +  { "Unknown",        ORTUnknown},        // the responder doesn't know if the cert is good
L 46: +//  { "GoodOtherCert" ORTGoodOtherCert,   // the response references a different certificate
L 47: +  { "GoodExplicitCA", ORTGoodOtherCA},    // the wrong CA has signed the response
L 48: +  { "Expired",        ORTExpired},        // the signature on the response has expired
L 49: +  { "ExpiredFreshCA", ORTExpiredFreshCA}, // fresh signature, but old validity period
L 55: +  { "Malformed",      ORTMalformed},      // the response from the responder was malformed
L 56: +  { "Srverr",         ORTSrverr},         // the response indicates there was a server error
L 57: +  { "TryLater",       ORTTryLater},       // the responder replied with "try again later"
{ 64: +stringToOCSPResponseType(const char *respText) {
W 65: +  for(int i =0; i < sizeof(kOCSPResponseNameList)/sizeof(OCSPResponseName);
W 67: +    if(0 == strncmp(respText,kOCSPResponseNameList[i].mHost,40)) {
W 98: +  if((uint32_t) rv != item->len) {
W 123: +  if(rv !=SECSuccess) {
W 133: +  for(int i = 2; i + 3 < argc; i += 4) {
W 141: +      PR_fprintf(PR_STDERR, "Cannot generate OCSP response of type %s\n", 
W 154: +    if(!response) {
W 165: +    if(!WriteResponse(outfilename, &response->items[0])) {

Found these problems in the patch:
L: The line was more than 80 characters long.
W: There is extra whitespace at the end of the line, tabs on the line, or no space after ',', if, while, for or switch
{: Methods should always have the opening brace on their own line.

::: security/manager/ssl/tests/unit/tlsserver/cmd/generateOCSPResponse.cpp
@@ +1,1 @@
> +

Modeline (as much as I think they're almost entirely useless)

@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* this simple priogram takes a database directory a a group of tuples

nits: capitalize "this", s/priogram/program/

@@ +25,5 @@
> +#include "prtime.h"
> +#include "p12.h"
> +#include "ssl.h"
> +#include "p12plcy.h"
> +#include "base64.h"

All of these #includes should follow the guidelines in https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#C.2FC.2B.2B_practices

@@ +38,5 @@
> +    const OCSPResponseType mORT;
> +};
> +
> +const static OCSPResponseName kOCSPResponseNameList[] = {
> +//  { "Null",           ORTNull },        //ORT NULL indicates failure.

let's just remove the commented-out types for now

@@ +63,5 @@
> +OCSPResponseType
> +stringToOCSPResponseType(const char *respText) {
> +  for(int i =0; i < sizeof(kOCSPResponseNameList)/sizeof(OCSPResponseName);
> +      i++) {
> +    if(0 == strncmp(respText,kOCSPResponseNameList[i].mHost,40)) {

why 40 for strncmp? use min(strlen(respText), strlen(kOCSPResponseNameList[i].mHost)) (there's probably a utility function that does this in the tree already)

@@ +75,5 @@
> +WriteResponse(const char* filename,
> +              SECItem* item)
> +{
> +  if ( !filename || !*filename) {
> +    PR_SetError(SEC_ERROR_INVALID_ARGS, 0);

I'm not sure this is necessary.

@@ +85,5 @@
> +    PR_fprintf(PR_STDERR, "error encoding OCSP response \"%s\"", filename);
> +    return false;
> +  }
> +
> +  PRFileDesc* outFile = PR_Open(filename,

ScopedPRFileDesc

@@ +87,5 @@
> +  }
> +
> +  PRFileDesc* outFile = PR_Open(filename,
> +                                PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE,
> +                                0664);

0644

@@ +89,5 @@
> +  PRFileDesc* outFile = PR_Open(filename,
> +                                PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE,
> +                                0664);
> +  if (!outFile) {
> +       PR_fprintf(PR_STDERR, "cannot open outfile for writing\n");

Will PrintPRError work for these kinds of errors?

@@ +160,5 @@
> +    #define MAX_OUTFILENAME_LEN 384
> +    char outfilename[MAX_OUTFILENAME_LEN];
> +    const char *outdir=dbdir;
> +
> +    PR_snprintf(outfilename, MAX_OUTFILENAME_LEN-1, "%s/%s", outdir, filename);

I was thinking we would just trust the caller to give this program the right path to the output file.

::: security/manager/ssl/tests/unit/tlsserver/cmd/moz.build
@@ +8,5 @@
>  
>  FAIL_ON_WARNINGS = True
>  
>  SIMPLE_PROGRAMS = [
> +    'generateOCSPResponse',

There are other places where we need to add a line similar to OCSPStaplingServer - you should just be able to do a string search in mxr for them.
Attachment #832546 - Flags: review?(dkeeler) → review-
Comment on attachment 832546 [details] [diff] [review]
add-genocspresponse (v1.1)

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

::: security/manager/ssl/tests/unit/tlsserver/cmd/generateOCSPResponse.cpp
@@ +89,5 @@
> +  PRFileDesc* outFile = PR_Open(filename,
> +                                PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE,
> +                                0664);
> +  if (!outFile) {
> +       PR_fprintf(PR_STDERR, "cannot open outfile for writing\n");

For the ones without extra params, yes. (but for simplicity I decided to only use PR_fprintf.

@@ +160,5 @@
> +    #define MAX_OUTFILENAME_LEN 384
> +    char outfilename[MAX_OUTFILENAME_LEN];
> +    const char *outdir=dbdir;
> +
> +    PR_snprintf(outfilename, MAX_OUTFILENAME_LEN-1, "%s/%s", outdir, filename);

The issue here is that if we generate multiple ocsp calls in one iteration, the argument list could become very large (I am ok with either way as I do generate them one at a time, your choice).
(In reply to Camilo Viecco (:cviecco) from comment #4)
> > +       PR_fprintf(PR_STDERR, "cannot open outfile for writing\n");
> 
> For the ones without extra params, yes. (but for simplicity I decided to
> only use PR_fprintf.

I think it would be simplest to follow as much as possible the style in OCSPStaplingServer and TLSServer.

> @@ +160,5 @@
> > +    #define MAX_OUTFILENAME_LEN 384
> > +    char outfilename[MAX_OUTFILENAME_LEN];
> > +    const char *outdir=dbdir;
> > +
> > +    PR_snprintf(outfilename, MAX_OUTFILENAME_LEN-1, "%s/%s", outdir, filename);
> 
> The issue here is that if we generate multiple ocsp calls in one iteration,
> the argument list could become very large (I am ok with either way as I do
> generate them one at a time, your choice).

The argument list length limit is something like 32 or 64K, so I don't think we have to worry about that. Also, as far as I can tell, the paths can be relative, so they won't be that long.
Posted patch ocsp-head-remade (obsolete) — Splinter Review
Please take a look at the prosed interface in head_psm only.
Attachment #8334191 - Flags: feedback?(dkeeler)
Comment on attachment 8334191 [details] [diff] [review]
ocsp-head-remade

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

Looks good - just a couple of comments.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +308,5 @@
>    });
>  }
> +
> +//function 
> +function generateOCSPResponses(ocsp_desc_array, nssDBlocation)

We should document the arguments. Also, let's stick to camel-case for arguments/variables.

@@ +311,5 @@
> +//function 
> +function generateOCSPResponses(ocsp_desc_array, nssDBlocation)
> +{
> +  let utilBinName =  "generateOCSPResponse";
> +  let ocspGenBin = _getBinaryUtil(utilBinName);

Where's _getBinaryUtil?

@@ +320,5 @@
> +    arg_array[0] = nssDBlocation;
> +    arg_array[1] = ocsp_desc_array[i][0];//"Good";
> +    arg_array[2] = ocsp_desc_array[i][1];//cert_name;
> +    arg_array[3] = ocsp_desc_array[i][2];"unused_param"
> +    arg_array[4] = i.toString() + ".ocsp";

Maybe arg_array.push(...) for all of these

@@ +325,5 @@
> +    do_print("arg_array_foo="+arg_array);
> +
> +    let process = Cc["@mozilla.org/process/util;1"].
> +                    createInstance(Ci.nsIProcess);
> +    process.init(ocspGenBin);

I was thinking _getBinaryUtil would return an nsIProcess, but this works too.

@@ +332,5 @@
> +    do_check_eq(0, process.exitValue);
> +    let filename = nssDBlocation + "/" + i.toString() + ".ocsp";
> +    let ocspFile =  Cc["@mozilla.org/file/local;1"].
> +           createInstance(Ci.nsILocalFile);
> +    ocspFile.initWithPath(filename);

you can use do_get_file here

@@ +333,5 @@
> +    let filename = nssDBlocation + "/" + i.toString() + ".ocsp";
> +    let ocspFile =  Cc["@mozilla.org/file/local;1"].
> +           createInstance(Ci.nsILocalFile);
> +    ocspFile.initWithPath(filename);
> +    retArray[i] = readFile(ocspFile);

Do we care about cleaning up these files afterwards?
Attachment #8334191 - Flags: feedback?(dkeeler) → feedback+
Comment on attachment 832546 [details] [diff] [review]
add-genocspresponse (v1.1)

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

::: security/manager/ssl/tests/unit/tlsserver/cmd/generateOCSPResponse.cpp
@@ +32,5 @@
> +using namespace mozilla::test;
> +
> +class OCSPResponseName
> +{
> +  public:

Nit: use a struct if you're going to make this work like a struct anyway.

@@ +55,5 @@
> +  { "Malformed",      ORTMalformed},      // the response from the responder was malformed
> +  { "Srverr",         ORTSrverr},         // the response indicates there was a server error
> +  { "TryLater",       ORTTryLater},       // the responder replied with "try again later"
> +  { "NeedsSig",       ORTNeedsSig},       // the response needs a signature
> +  { "Unauthorized",   ORTUnauthorized},

I think these should be all-lowercase names that are readable, legal hostname components. That way, we can create programs that simply iterate through this array and construct <name>-testname.example.org hostnames for the test.

@@ +60,5 @@
> +};
> +
> +
> +OCSPResponseType
> +stringToOCSPResponseType(const char *respText) {

How about SECStatus
stringToOCSPResponseType(const char* respText, /*out*/ OCSPRespondType* type);

Also, please follow the Mozilla Coding style guidelines for new source files: "const char* respText" not "const char *respText".

@@ +156,5 @@
> +                            "for %s\n", ocspTypeText, certNick);
> +      return 255;
> +    }
> +
> +    #define MAX_OUTFILENAME_LEN 384

PATH_MAX? (On Windows, PATH_MAX is ~255, so 384 seems like a bad idea.) Or...

@@ +160,5 @@
> +    #define MAX_OUTFILENAME_LEN 384
> +    char outfilename[MAX_OUTFILENAME_LEN];
> +    const char *outdir=dbdir;
> +
> +    PR_snprintf(outfilename, MAX_OUTFILENAME_LEN-1, "%s/%s", outdir, filename);

1. Check the return value.
2. Use PR_smprintf instead.

::: security/manager/ssl/tests/unit/tlsserver/cmd/moz.build
@@ +8,5 @@
>  
>  FAIL_ON_WARNINGS = True
>  
>  SIMPLE_PROGRAMS = [
> +    'generateOCSPResponse',

Capitalize this consistently with the other programs (i.e. GenerateOCSPResponse).
(In reply to David Keeler (:keeler) from comment #7)
> Comment on attachment 8334191 [details] [diff] [review]
> ocsp-head-remade
> 
> Review of attachment 8334191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good - just a couple of comments.
> 
> ::: security/manager/ssl/tests/unit/head_psm.js
> @@ +308,5 @@
> >    });
> >  }
> > +
> > +//function 
> > +function generateOCSPResponses(ocsp_desc_array, nssDBlocation)
> 
> We should document the arguments. Also, let's stick to camel-case for
> arguments/variables.
> 
> @@ +311,5 @@
> > +//function 
> > +function generateOCSPResponses(ocsp_desc_array, nssDBlocation)
> > +{
> > +  let utilBinName =  "generateOCSPResponse";
> > +  let ocspGenBin = _getBinaryUtil(utilBinName);
> 
> Where's _getBinaryUtil?
> 
> @@ +320,5 @@
> > +    arg_array[0] = nssDBlocation;
> > +    arg_array[1] = ocsp_desc_array[i][0];//"Good";
> > +    arg_array[2] = ocsp_desc_array[i][1];//cert_name;
> > +    arg_array[3] = ocsp_desc_array[i][2];"unused_param"
> > +    arg_array[4] = i.toString() + ".ocsp";
> 
> Maybe arg_array.push(...) for all of these
Tried it and I was not more clear (even longer lines).

> 
> @@ +325,5 @@
> > +    do_print("arg_array_foo="+arg_array);
> > +
> > +    let process = Cc["@mozilla.org/process/util;1"].
> > +                    createInstance(Ci.nsIProcess);
> > +    process.init(ocspGenBin);
> 
> I was thinking _getBinaryUtil would return an nsIProcess, but this works too.
> 
> @@ +332,5 @@
> > +    do_check_eq(0, process.exitValue);
> > +    let filename = nssDBlocation + "/" + i.toString() + ".ocsp";
> > +    let ocspFile =  Cc["@mozilla.org/file/local;1"].
> > +           createInstance(Ci.nsILocalFile);
> > +    ocspFile.initWithPath(filename);
> 
> you can use do_get_file here
do_get_file will not work with the new sematics of any location for the file
destination. 
> 
> @@ +333,5 @@
> > +    let filename = nssDBlocation + "/" + i.toString() + ".ocsp";
> > +    let ocspFile =  Cc["@mozilla.org/file/local;1"].
> > +           createInstance(Ci.nsILocalFile);
> > +    ocspFile.initWithPath(filename);
> > +    retArray[i] = readFile(ocspFile);
> 
> Do we care about cleaning up these files afterwards?

Yes, need to do
Posted patch add-genocspresponse (v2) (obsolete) — Splinter Review
Attachment #832546 - Attachment is obsolete: true
Attachment #8334191 - Attachment is obsolete: true
Posted patch add-genocspresponse (v2.1) (obsolete) — Splinter Review
Attachment #8334749 - Attachment is obsolete: true
Posted patch add-genocspresponse (v2.2) (obsolete) — Splinter Review
Attachment #8334823 - Attachment is obsolete: true
Comment on attachment 8334834 [details] [diff] [review]
add-genocspresponse (v2.2)

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

Added all changes suggeested by keeler and all but one from bsmith:

stringToOCSPResponseType returns a bool instead of a SECStatus. (it makes more sense to me).
Attachment #8334834 - Flags: review?(dkeeler)
Comment on attachment 8334834 [details] [diff] [review]
add-genocspresponse (v2.2)

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

Looking good - just a couple of nits.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +318,5 @@
> +  let ocspGenBin = _getBinaryUtil(utilBinName);
> +  let retArray = new Array();
> +
> +  for (let i = 0; i < ocspRespArray.length; i++) {
> +    let arg_array = new Array();

camel case

@@ +319,5 @@
> +  let retArray = new Array();
> +
> +  for (let i = 0; i < ocspRespArray.length; i++) {
> +    let arg_array = new Array();
> +    let filename = nssDBlocation + "/" + i.toString() + ".ocsp";

I think if you have the filename be something like "response" + i.toString() + ".ocsp" (i.e. no path component), it will be created in the current working directory. This way, you can refer to it with do_get_file() later (instead of creating an nsILocalFile by hand).

@@ +324,5 @@
> +    arg_array[0] = nssDBlocation;
> +    arg_array[1] = ocspRespArray[i][0]; // ocsRespType;
> +    arg_array[2] = ocspRespArray[i][1]; // nick;
> +    arg_array[3] = ocspRespArray[i][2]; // extranickname
> +    arg_array[4] = filename;

argArray.push(nssDBlocation);
argArray.push(ocspRespArray[i][0]); // ocsRespType;
argArray.push(ocspRespArray[i][1]); // nick;
argArray.push(ocspRespArray[i][2]); // extranickname
argArray.push(filename);

@@ +325,5 @@
> +    arg_array[1] = ocspRespArray[i][0]; // ocsRespType;
> +    arg_array[2] = ocspRespArray[i][1]; // nick;
> +    arg_array[3] = ocspRespArray[i][2]; // extranickname
> +    arg_array[4] = filename;
> +    do_print("arg_array_foo ="+arg_array);

let's remove unnecessary debug printing before checking this in

@@ +328,5 @@
> +    arg_array[4] = filename;
> +    do_print("arg_array_foo ="+arg_array);
> +
> +    let process = Cc["@mozilla.org/process/util;1"].
> +                    createInstance(Ci.nsIProcess);

'.' on the next line

@@ +331,5 @@
> +    let process = Cc["@mozilla.org/process/util;1"].
> +                    createInstance(Ci.nsIProcess);
> +    process.init(ocspGenBin);
> +    process.run(true, arg_array, 5);
> +    do_print("exit_value ="+process.exitValue);

again, this debug printing shouldn't be necessary for the final product

@@ +336,5 @@
> +    do_check_eq(0, process.exitValue);
> +    let ocspFile =  Cc["@mozilla.org/file/local;1"].
> +           createInstance(Ci.nsILocalFile);
> +    ocspFile.initWithPath(filename);
> +    retArray[i] = readFile(ocspFile);

retArray.push(readFile(ocspFile));

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp
@@ +1,1 @@
> +

Modelines

@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* this simple priogram takes a database directory a a group of tuples

you still have multiple typos here

@@ +25,5 @@
> +using namespace mozilla::test;
> +
> +struct OCSPResponseName
> +{
> +    const char *mHost;

It's not clear why this is called mHost - maybe mDescription? mTypeString? mTypeAsString?

@@ +26,5 @@
> +
> +struct OCSPResponseName
> +{
> +    const char *mHost;
> +    const OCSPResponseType mORT;

I think we can do 2-space indents here

@@ +61,5 @@
> +{
> +  if (!OCSPType) {
> +    return false;
> +  }
> +  for (int i = 0; i < sizeof(kOCSPResponseNameList)/sizeof(OCSPResponseName);

mozilla::ArrayLength

@@ +63,5 @@
> +    return false;
> +  }
> +  for (int i = 0; i < sizeof(kOCSPResponseNameList)/sizeof(OCSPResponseName);
> +      i++) {
> +    if (0 == strcmp(respText, kOCSPResponseNameList[i].mHost)) {

I prefer 'if (strcmp(...) == 0)'

@@ +73,5 @@
> +}
> +
> +bool
> +WriteResponse(const char* filename,
> +              SECItem* item)

This can go on the previous line

@@ +76,5 @@
> +WriteResponse(const char* filename,
> +              SECItem* item)
> +{
> +  if ( !filename || !*filename) {
> +    PrintPRError("invalid parameters to WriteResponse");

WriteResponse is only called from one place and it's internal to this file. I don't think we need to check the arguments for correctness. If the filename isn't good, PR_Open will fail. That should be enough.

@@ +81,5 @@
> +    return false;
> +  }
> +
> +  if (!item || !item->data) {
> +    PR_fprintf(PR_STDERR, "error encoding OCSP response \"%s\"", filename);

By the same, if item isn't good, GetOCSPResponseForType will have already failed.

@@ +89,5 @@
> +  ScopedPRFileDesc outFile(PR_Open(filename,
> +                                   PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE,
> +                                   0644));
> +  if (!outFile) {
> +       PR_fprintf(PR_STDERR, "cannot open outfile for writing\n");

PrintPRError

@@ +90,5 @@
> +                                   PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE,
> +                                   0644));
> +  if (!outFile) {
> +       PR_fprintf(PR_STDERR, "cannot open outfile for writing\n");
> +       return 255;

return false

@@ +92,5 @@
> +  if (!outFile) {
> +       PR_fprintf(PR_STDERR, "cannot open outfile for writing\n");
> +       return 255;
> +  }
> +  int32_t rv = PR_Write(outFile, item->data, item->len);

rv puts me in mind of an nsresult. Let's call it bytesWritten or something.

@@ +93,5 @@
> +       PR_fprintf(PR_STDERR, "cannot open outfile for writing\n");
> +       return 255;
> +  }
> +  int32_t rv = PR_Write(outFile, item->data, item->len);
> +  if ((uint32_t) rv != item->len) {

How about 'if (bytesWritten < 0 || (uint32_t)bytesWritten != item->len)'

@@ +94,5 @@
> +       return 255;
> +  }
> +  int32_t rv = PR_Write(outFile, item->data, item->len);
> +  if ((uint32_t) rv != item->len) {
> +    return false;

PrintPRError as well

@@ +106,5 @@
> +int
> +main(int argc, char* argv[])
> +{
> +
> +  if (argc < 6 || (argc-6)%4 != 0) {

spaces around operators

@@ +117,5 @@
> +
> +  SECStatus rv;
> +  rv = NSS_Init(dbdir);
> +  if (rv != SECSuccess) {
> +    PR_fprintf(PR_STDERR, "%s, Failed to initialize NSS\n", argv[0]);

PrintPRError

@@ +118,5 @@
> +  SECStatus rv;
> +  rv = NSS_Init(dbdir);
> +  if (rv != SECSuccess) {
> +    PR_fprintf(PR_STDERR, "%s, Failed to initialize NSS\n", argv[0]);
> +    return 255;

Let's just standardize on 1 for failure and 0 for success for exit values.

@@ +120,5 @@
> +  if (rv != SECSuccess) {
> +    PR_fprintf(PR_STDERR, "%s, Failed to initialize NSS\n", argv[0]);
> +    return 255;
> +  }
> +  PLArenaPool *arena = PORT_NewArena(256*argc);

spaces around operators

@@ +128,5 @@
> +  }
> +
> +  for (int i = 2; i + 3 < argc; i += 4) {
> +    const char *ocspTypeText  = argv[i];
> +    const char *certNick      = argv[i+1];

spaces around operators

@@ +130,5 @@
> +  for (int i = 2; i + 3 < argc; i += 4) {
> +    const char *ocspTypeText  = argv[i];
> +    const char *certNick      = argv[i+1];
> +    const char *extraCertname = argv[i+2];
> +    const char *filename   = argv[i+3];

this last one doesn't line up
Attachment #8334834 - Flags: review?(dkeeler) → review-
Posted patch add-genocspresponse (v3) (obsolete) — Splinter Review
Attachment #8334834 - Attachment is obsolete: true
Comment on attachment 8335455 [details] [diff] [review]
add-genocspresponse (v3)

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

Kept a simplified error check in writeresponse (we dont care about perf here).
Attachment #8335455 - Flags: review?(dkeeler)
Comment on attachment 8335455 [details] [diff] [review]
add-genocspresponse (v3)

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

Thanks for working on this, Camilo. I know I'm being pretty strict about style and other nits, but I think attention to detail is important in writing quality software. I know this is only for tests, but if we don't have quality tests, we won't have a quality browser.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +307,5 @@
>      process.kill();
>    });
>  }
> +
> +// Returns an Array of OCSP responses for given an ocspRespArray and a location

nit: "for a given" or "given an" (no for)

@@ +309,5 @@
>  }
> +
> +// Returns an Array of OCSP responses for given an ocspRespArray and a location
> +// for a nssDB where the certs and public keys are prepopulated.
> +// ocspRespArray is and array of arrays like:

nit: "an array"

@@ +332,5 @@
> +                    .createInstance(Ci.nsIProcess);
> +    process.init(ocspGenBin);
> +    process.run(true, argArray, 5);
> +    do_check_eq(0, process.exitValue);
> +    let ocspFile =  Cc["@mozilla.org/file/local;1"]

nit: there's an extra space before "Cc"

@@ +333,5 @@
> +    process.init(ocspGenBin);
> +    process.run(true, argArray, 5);
> +    do_check_eq(0, process.exitValue);
> +    let ocspFile =  Cc["@mozilla.org/file/local;1"]
> +          .createInstance(Ci.nsILocalFile);

nit: line up the '.' with the '[' on the previous line

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* This simple program takes a database directory, one of more tuples like

Nit: "directory and one or more".
I know I'm being very pedantic about grammar in comments, but it's important to have clear, understandable documentation. Otherwise, we shouldn't even bother.

@@ +75,5 @@
> +bool
> +WriteResponse(const char* filename, const SECItem* item)
> +{
> +  if (!filename || !item || !item->data) {
> +    PrintPRError("invalid parameters to WriteResponse");

PrintPRError isn't useful here (see comment below).

@@ +83,5 @@
> +  ScopedPRFileDesc outFile(PR_Open(filename,
> +                                   PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE,
> +                                   0644));
> +  if (!outFile) {
> +       PR_fprintf(PR_STDERR, "cannot open '%s' for writing\n", filename);

I guess I wasn't clear about the use of PrintPRError. PrintPRError is designed to be useful for printing errors set in NSS/NSPR functions. It's not very useful for errors in non-library functions (like above, where you're checking the parameters to WriteResponse).
So, I think it's best to use PrintPRError for library calls and PR_fprintf for non-library calls.

@@ +87,5 @@
> +       PR_fprintf(PR_STDERR, "cannot open '%s' for writing\n", filename);
> +       return 255;
> +  }
> +  int32_t rv = PR_Write(outFile, item->data, item->len);
> +  if ((uint32_t) rv != item->len) {

The reason I suggested 'if (rv < 0 || (uint32_t)rv != item->len)' in the previous review is that there are negative values that, when cast to an unsigned value, can end up being the same as item->len. This would yield incorrect behavior. To avoid this, first check if the value you're casting is negative (if it is, it's an error and we should report that (and also we should call PrintPRError()).
Phrasing it as a suggestion was probably a mistake in the previous review; I'll try to be more clear in the future. If any of my comments are ever unclear, however, please ask for clarification. If the comment was simply overlooked, I understand - I've done that too. What I do to avoid overlooking previous review comments is to run through previous reviews before posting a new patch to make sure I addressed everything.

@@ +105,5 @@
> +    PR_fprintf(PR_STDERR, "usage: %s <NSS DB directory> <responsetype> "
> +                          "<cert_nick> <extranick> <outfilename> [<resptype> "
> +                          "<cert_nick> <extranick> <outfilename>]* \n",
> +                          argv[0]);
> +    exit(EXIT_FAILURE);

We probably shouldn't ever call exit() directly - return is enough.

@@ +112,5 @@
> +
> +  SECStatus rv;
> +  rv = NSS_Init(dbdir);
> +  if (rv != SECSuccess) {
> +    PR_fprintf(PR_STDERR, "%s, Failed to initialize NSS\n", argv[0]);

Since NSS_Init is a library function, I think calling PrintPRError would be more useful.

@@ +113,5 @@
> +  SECStatus rv;
> +  rv = NSS_Init(dbdir);
> +  if (rv != SECSuccess) {
> +    PR_fprintf(PR_STDERR, "%s, Failed to initialize NSS\n", argv[0]);
> +    exit(EXIT_FAILURE);;

There's an extra ';'

@@ +115,5 @@
> +  if (rv != SECSuccess) {
> +    PR_fprintf(PR_STDERR, "%s, Failed to initialize NSS\n", argv[0]);
> +    exit(EXIT_FAILURE);;
> +  }
> +  PLArenaPool *arena = PORT_NewArena(256 * argc);

We should be consistent about where the '*' goes in pointer types. Apparently in new code it's supposed to be directly after the type name (i.e. no space in front, one space after).

@@ +117,5 @@
> +    exit(EXIT_FAILURE);;
> +  }
> +  PLArenaPool *arena = PORT_NewArena(256 * argc);
> +  if (!arena) {
> +    PrintPRError("PORT_NewArena failed\n");

The trailing "\n" isn't necessary

@@ +118,5 @@
> +  }
> +  PLArenaPool *arena = PORT_NewArena(256 * argc);
> +  if (!arena) {
> +    PrintPRError("PORT_NewArena failed\n");
> +    exit(EXIT_FAILURE);;

Extra ';'
Attachment #8335455 - Flags: review?(dkeeler) → review-
Comment on attachment 8335455 [details] [diff] [review]
add-genocspresponse (v3)

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

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp
@@ +83,5 @@
> +  ScopedPRFileDesc outFile(PR_Open(filename,
> +                                   PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE,
> +                                   0644));
> +  if (!outFile) {
> +       PR_fprintf(PR_STDERR, "cannot open '%s' for writing\n", filename);

Sure, lets comment this somewhere so that future coders are not confused by this (I would also suggest modifying PrintPRError so that it takes a variable number of inputs like printf)

@@ +87,5 @@
> +       PR_fprintf(PR_STDERR, "cannot open '%s' for writing\n", filename);
> +       return 255;
> +  }
> +  int32_t rv = PR_Write(outFile, item->data, item->len);
> +  if ((uint32_t) rv != item->len) {

Yes, we could have a ocspresponse of size 0xFFFFFFFF (4GB - 1) and a failed write (PR_Write only returns -1 on failure). And in this case your suggested logic will fail too, but I digress I will make the changes.

@@ +105,5 @@
> +    PR_fprintf(PR_STDERR, "usage: %s <NSS DB directory> <responsetype> "
> +                          "<cert_nick> <extranick> <outfilename> [<resptype> "
> +                          "<cert_nick> <extranick> <outfilename>]* \n",
> +                          argv[0]);
> +    exit(EXIT_FAILURE);

As discussed in IRC there is exit(EXIT_FAILURE) for failure conditions is OK.
Posted patch add-genocspresponse (v4) (obsolete) — Splinter Review
Attachment #8335455 - Attachment is obsolete: true
Attachment #8335671 - Flags: review?(dkeeler)
Comment on attachment 8335671 [details] [diff] [review]
add-genocspresponse (v4)

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

Awesome.
Attachment #8335671 - Flags: review?(dkeeler) → review+
Fixed a compilation and mochtest path issue on windows. (keeping r+)
Attachment #8335671 - Attachment is obsolete: true
Attachment #8336367 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2656e9b93114
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: in-testsuite?
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.