Closed Bug 1178988 Opened 5 years ago Closed 5 years ago

convert test_ocsp_url.js to generate certificates at build time

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(3 files)

test_ocsp_url.js should generate certificates at build time, like the other PSM tests. One tricky part about this test is that it uses GenerateOCSPResponse, which relies on having NSS databases of certificates and keys. My current approach is to extend GenerateOCSPResponse to read all .pem and .key files in the specified directory so that they'll be available for use as if they had been in the NSS DBs.
bug 1178988 - refactor key-specific parts of pycert.py into pykey.py
Attachment #8628977 - Flags: review?(mgoodwin)
Attachment #8628977 - Flags: review?(cykesiopka.bmo)
bug 1178988 - convert test_ocsp_url to generate certificates at build time

Also enable loading of certificates and private keys into GenerateOCSPResponse
Attachment #8628978 - Flags: review?(mgoodwin)
Attachment #8628978 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8628978 [details]
MozReview Request: bug 1178988 - convert test_ocsp_url to generate certificates at build time r=Cykesiopka

https://reviewboard.mozilla.org/r/12491/#review11045

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:252
(Diff revision 1)
> +  memset(buf, sizeof(buf), 0);

It'll take me a while to review the changes here, but just as a heads up, the default gcc on Lubuntu 15.04 doesn't seem to like this line:
> /home/m-c_drive/mozilla-inbound/security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp: In function ‘void AddCertificateFromFile(const char*, const char*)’:
> /home/m-c_drive/mozilla-inbound/security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:252:29: error: ‘memset’ used with constant zero length parameter; this could be due to transposed parameters [-Werror=memset-transposed-args]
>    memset(buf, sizeof(buf), 0);

gcc --version
> gcc (Ubuntu 4.9.2-10ubuntu13) 4.9.2

My .mozconfig includes:
> ac_add_options --enable-warnings-as-errors
Attachment #8628978 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8628977 [details]
MozReview Request: bug 1178988 - refactor key-specific parts of pycert.py into pykey.py r=Cykesiopka,mgoodwin

https://reviewboard.mozilla.org/r/12489/#review11095

LGTM.
Attachment #8628977 - Flags: review?(cykesiopka.bmo) → review+
https://reviewboard.mozilla.org/r/12491/#review11121

::: security/manager/ssl/tests/unit/pycert.py:128
(Diff revision 1)
> +    accessMethod = univ.ObjectIdentifier('1.3.6.1.5.5.7.48.1')

Try |rfc2459.id_ad_ocsp|? Seems to work, and is clearer.

::: security/manager/ssl/tests/unit/pycert.py:343
(Diff revision 1)
> +        self.addExtension(univ.ObjectIdentifier('1.3.6.1.5.5.7.1.1'), sequence)

Try |rfc2459.id_pe_authorityInfoAccess|? Seems to work, and is clearer.

::: security/manager/ssl/tests/unit/test_ocsp_url/moz.build:37
(Diff revision 1)
> +    input_file = test_key + '.certspec'

Maybe '.keyspec' instead?

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:110
(Diff revision 1)
> +DecodeCertCallback(void* arg, SECItem** certs, int numcerts)

Optional: move this closer to AddCertificateFromFile().

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:159
(Diff revision 1)
> +const char* PRIVATE_KEY_HEADER = "-----BEGIN PRIVATE KEY-----";

Optional: move these two consts inside AddKeyFromFile().

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:165
(Diff revision 1)
> +  fprintf(stderr, "adding %s/%s\n", basePath, filename);

Maybe |fprintf(stdout, ...)| or |PR_fprintf(PR_STDOUT, ...)|? This message doesn't reflect an error, so using stderr seems weird.

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:272
(Diff revision 1)
> +  if (NSS_Init(nssCertDBDir) == SECSuccess) {

It looks like this call will also succeed on subsequent uses of GenerateOCSPResponse, because the DB files are cached under "_tests/xpcshell/security/manager/ssl/tests/unit/test_ocsp_url".

Not entirely sure whether this was intended, or if it really matters.

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:125
(Diff revision 1)
> +  if (PR_snprintf(buf, N - 1, "%s/%s", basePath, filename) == 0) {

I'm probably missing something, but doesn't this need to handle negative values signifying an error as well?

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:252
(Diff revision 1)
> +  memset(buf, sizeof(buf), 0);

Ah, this is an actual bug: the size arg goes last.
Comment on attachment 8628977 [details]
MozReview Request: bug 1178988 - refactor key-specific parts of pycert.py into pykey.py r=Cykesiopka,mgoodwin

https://reviewboard.mozilla.org/r/12489/#review11129

Looks good to me!
Attachment #8628977 - Flags: review?(mgoodwin) → review+
Attachment #8628978 - Flags: review?(mgoodwin)
Comment on attachment 8628978 [details]
MozReview Request: bug 1178988 - convert test_ocsp_url to generate certificates at build time r=Cykesiopka

https://reviewboard.mozilla.org/r/12491/#review11227

I've nothing to add to Cykesiopka's comments
https://reviewboard.mozilla.org/r/12491/#review11121

Thanks for the reviews!

> Try |rfc2459.id_ad_ocsp|? Seems to work, and is clearer.

Thanks. I guess I searched for 'OCSP' and figured there wasn't support for it when I didn't find anything.

> I'm probably missing something, but doesn't this need to handle negative values signifying an error as well?

Looks like PR_snprintf returns an unsigned value, so I think it can only indicate an error by returning 0.

> Maybe |fprintf(stdout, ...)| or |PR_fprintf(PR_STDOUT, ...)|? This message doesn't reflect an error, so using stderr seems weird.

That was just some debug spew - I removed it.

> Ah, this is an actual bug: the size arg goes last.

Good catch - thanks!

> It looks like this call will also succeed on subsequent uses of GenerateOCSPResponse, because the DB files are cached under "_tests/xpcshell/security/manager/ssl/tests/unit/test_ocsp_url".
> 
> Not entirely sure whether this was intended, or if it really matters.

Nice catch. I think this will be ok, though, because the certificates and keys are permanently added to those DB files, so the next time we run GenerateOCSPResponse, all the necessary data is already there.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> Looks like PR_snprintf returns an unsigned value, so I think it can only
> indicate an error by returning 0.

Hmm, indeed.

https://hg.mozilla.org/mozilla-central/file/d37d4edce6dd/nsprpub/pr/include/prprf.h#l35 :
> or (PRUint32)-1 if an error occurs

My brain ignored the PRUint32 part for some reason, but looking at the implementation, this comment is probably just incorrect anyways.
(In reply to Pulsebot from comment #10)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/aeae195846c8

I technically haven't given r+ for this yet, so just as a formality, I think I should take a look over the updated patch.
(In reply to Cykesiopka from comment #13)
> (In reply to Pulsebot from comment #10)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/aeae195846c8
> 
> I technically haven't given r+ for this yet, so just as a formality, I think
> I should take a look over the updated patch.

Whoops - my mistake. I'll post an updated patch that also should fix the bustage shortly.
Attachment #8628977 - Attachment description: MozReview Request: bug 1178988 - refactor key-specific parts of pycert.py into pykey.py → MozReview Request: bug 1178988 - refactor key-specific parts of pycert.py into pykey.py r=Cykesiopka,mgoodwin
Attachment #8628977 - Flags: review+
Comment on attachment 8628977 [details]
MozReview Request: bug 1178988 - refactor key-specific parts of pycert.py into pykey.py r=Cykesiopka,mgoodwin

bug 1178988 - refactor key-specific parts of pycert.py into pykey.py r=Cykesiopka,mgoodwin
Comment on attachment 8628978 [details]
MozReview Request: bug 1178988 - convert test_ocsp_url to generate certificates at build time r=Cykesiopka

bug 1178988 - convert test_ocsp_url to generate certificates at build time

Also enable loading of certificates and private keys into GenerateOCSPResponse
Attachment #8628978 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8628978 [details]
MozReview Request: bug 1178988 - convert test_ocsp_url to generate certificates at build time r=Cykesiopka

https://reviewboard.mozilla.org/r/12491/#review11389

LGTM!
Attachment #8628978 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8628977 [details]
MozReview Request: bug 1178988 - refactor key-specific parts of pycert.py into pykey.py r=Cykesiopka,mgoodwin

bug 1178988 - refactor key-specific parts of pycert.py into pykey.py r=Cykesiopka,mgoodwin
Attachment #8628977 - Flags: review?(mgoodwin)
Comment on attachment 8628978 [details]
MozReview Request: bug 1178988 - convert test_ocsp_url to generate certificates at build time r=Cykesiopka

bug 1178988 - convert test_ocsp_url to generate certificates at build time r=Cykesiopka

Also enable loading of certificates and private keys into GenerateOCSPResponse
Attachment #8628978 - Attachment description: MozReview Request: bug 1178988 - convert test_ocsp_url to generate certificates at build time → MozReview Request: bug 1178988 - convert test_ocsp_url to generate certificates at build time r=Cykesiopka
Attachment #8628978 - Flags: review+ → review?(cykesiopka.bmo)
Comment on attachment 8628977 [details]
MozReview Request: bug 1178988 - refactor key-specific parts of pycert.py into pykey.py r=Cykesiopka,mgoodwin

bug 1178988 - refactor key-specific parts of pycert.py into pykey.py r=Cykesiopka,mgoodwin
Comment on attachment 8628978 [details]
MozReview Request: bug 1178988 - convert test_ocsp_url to generate certificates at build time r=Cykesiopka

bug 1178988 - convert test_ocsp_url to generate certificates at build time r=Cykesiopka

Also enable loading of certificates and private keys into GenerateOCSPResponse
Attachment #8628978 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8634385 [details]
MozReview Request: bug 1178988 - work around PR_ReadDir bug on B2G ICS emulator by loading certs/keys in two phases

bug 1178988 - work around PR_ReadDir bug on B2G ICS emulator by loading certs/keys in two phases
Attachment #8634385 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8634385 [details]
MozReview Request: bug 1178988 - work around PR_ReadDir bug on B2G ICS emulator by loading certs/keys in two phases

https://reviewboard.mozilla.org/r/13391/#review12113

Sigh. Maybe one day the B2G ICS Emulator will stop being horrible.
Anyways, LGTM.

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:313
(Diff revision 1)
> +  for (std::vector<std::string>::iterator it = certificates.begin();

Just FYI, this seems to work on my Linux setup:
> for (std::string& it : certificates) {
>   AddCertificateFromFile(basePath, it.c_str());
> }

I don't remember whether this approach has any gotchas though, so feel free to ignore.
Attachment #8634385 - Flags: review?(cykesiopka.bmo) → review+
(In reply to Cykesiopka from comment #27)
> Comment on attachment 8634385 [details]
> MozReview Request: bug 1178988 - work around PR_ReadDir bug on B2G ICS
> emulator by loading certs/keys in two phases
> 
> https://reviewboard.mozilla.org/r/13391/#review12113
> 
> Sigh. Maybe one day the B2G ICS Emulator will stop being horrible.
> Anyways, LGTM.
> 
> :::
> security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp:313
> (Diff revision 1)
> > +  for (std::vector<std::string>::iterator it = certificates.begin();
> 
> Just FYI, this seems to work on my Linux setup:
> > for (std::string& it : certificates) {
> >   AddCertificateFromFile(basePath, it.c_str());
> > }
> 
> I don't remember whether this approach has any gotchas though, so feel free
> to ignore.

Cool - looks like it works: https://treeherder.mozilla.org/#/jobs?repo=try&revision=853ec2946ede
Ok. I can't reproduce that failure locally and it didn't show up in the try run that tested that exact same platform. After spending more than a week on this, I'm disabling that test on the B2G emulators. Anyone with a problem with this can take it up with sworkman.
Flags: needinfo?(dkeeler)
Sounds good to me. Can you file a new bug to track the failure on B2G emu, please?
You need to log in before you can comment on or make changes to this bug.