1178988 - convert test_ocsp_url.js to generate certificates at build time

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

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.

Comment 1

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

Attachment: 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

Comment 2

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

Attachment: 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

Comment 3

[:Cykesiopka]

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

Comment 4

[:Cykesiopka]

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.

Comment 5

[:Cykesiopka]

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 6

[Mark Goodwin [:mgoodwin]]

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!

Comment 7

[Mark Goodwin [: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

Comment 8

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

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.

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c00e71ebe67c

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e38e844e0af
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeae195846c8

Comment 11

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0837a8fb8a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/967a25a6c468

Comment 12

[:Cykesiopka]

(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.

Comment 13

[:Cykesiopka]

(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.

Comment 14

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

(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.

Comment 15

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

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 16

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

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

Comment 17

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7212bbaaef2

Comment 18

[:Cykesiopka]

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!

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/936d991c4cbc
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c5d5eb434b9

Comment 20

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/342eec4a3986 for permaorange timeouts on b2g emulator opt like https://treeherder.mozilla.org/logviewer.html#?job_id=11502599&repo=mozilla-inbound

Comment 21

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

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 22

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

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

Comment 23

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

Attachment: 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

Comment 24

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

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 25

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

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

Comment 26

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

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

Comment 27

[:Cykesiopka]

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.

Comment 28

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

(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

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07b9c2331ac1
https://hg.mozilla.org/integration/mozilla-inbound/rev/2700ec4adc3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb6a9114916

Comment 30

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

I had to back these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2de10a365b6b for frequent b2g xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11854828&repo=mozilla-inbound

Comment 31

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

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.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9808c11dfd35
https://hg.mozilla.org/integration/mozilla-inbound/rev/8193d5fee663
https://hg.mozilla.org/integration/mozilla-inbound/rev/a53e7c63a0e7

Comment 33

[Steve Workman [:sworkman] (INACTIVE)]

Sounds good to me. Can you file a new bug to track the failure on B2G emu, please?

Comment 34

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

Thanks. Filed bug 1185732.

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9808c11dfd35
https://hg.mozilla.org/mozilla-central/rev/8193d5fee663
https://hg.mozilla.org/mozilla-central/rev/a53e7c63a0e7