1203312 - convert the tlsserver certificates to be generated at build time

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

This is one of the last large pieces of work to convert the PSM xpcshell test certificates to being generated automatically. This borrows some work from bug 1183718, but since it's not clear when that will land, let's go ahead with this in the meantime.

Comment 1

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

Attachment: MozReview Request: bug 1203312 - convert tlsserver to generate certificates at build time r=Cykesiopka,mgoodwin

bug 1203312 - convert tlsserver to generate certificates at build time

Comment 2

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8658961 [details]
MozReview Request: bug 1203312 - convert tlsserver to generate certificates at build time r=Cykesiopka,mgoodwin

https://reviewboard.mozilla.org/r/18733/#review16909

This looks good to me.

It's a shame that we have hardcoded hosts and cert names in tlsserver - but can't think of a better way of doing that now. 

I don't know whether this applies in this case, but when I made changes to ssltunnel for mochitests there were issues with getting the correct versions of hostutils in the right places (see bug 1096197 comment 20 onwards) - does anything need doing to avoid this sort of problem in this case?

::: security/manager/ssl/tests/unit/test_cert_overrides.js:59
(Diff revision 1)
> -  equal(keySizeHistogram.counts[1], 0,
> +  equal(keySizeHistogram.counts[1], 12,

These (and the changed values in security/manager/ssl/tests/unit/test_ocsp_caching.js) feel like tests for observed (rather than expected) behavior. Whilst this is in keeping with what's already there, I wonder what can be done to make what's happening here more explicit.

This isn't a request to change things (more an observation).

Comment 3

[:Cykesiopka]

Comment on attachment 8658961 [details]
MozReview Request: bug 1203312 - convert tlsserver to generate certificates at build time r=Cykesiopka,mgoodwin

https://reviewboard.mozilla.org/r/18733/#review16971

Cool.

::: security/manager/ssl/tests/unit/pycert.py:436
(Diff revision 1)
> +        if critical:

Optional: may or may not be worth documenting that |critical| is a string, since intuitively I would assume it's a boolean.

::: security/manager/ssl/tests/unit/tlsserver/beforeEpoch.pem.certspec:3
(Diff revision 1)
> +validity:19460214-20310101

Optional: We might want to move the notBefore to just before the epoch now that we have the ability to do so (maybe in a follow up), to make the cert lifetime slightly more realistic.

::: security/manager/ssl/tests/unit/tlsserver/beforeEpochINT.pem.certspec:3
(Diff revision 1)
> +validity:19460214-20310101

Same here.

::: security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp:81
(Diff revision 1)
> +DoSNISocketConfigBySubjectCN(PRFileDesc *aFd, const SECItem *aSrvNameArr,

Nit: star should stick to type.

::: security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp:19
(Diff revision 1)
> +#include "base64.h"

Nit: can this be sorted alphabetically?

::: security/manager/ssl/tests/unit/tlsserver/mismatch-untrusted-expired.pem.certspec:2
(Diff revision 1)
> +subject:Untrusted-Expired Test End-entity

Looks like this should be |Mismatch-Untrusted-Expired Test End-entity| instead.

Comment 4

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

https://reviewboard.mozilla.org/r/18733/#review16909

> These (and the changed values in security/manager/ssl/tests/unit/test_ocsp_caching.js) feel like tests for observed (rather than expected) behavior. Whilst this is in keeping with what's already there, I wonder what can be done to make what's happening here more explicit.
> 
> This isn't a request to change things (more an observation).

The reason the 12 from bucket 2 went to bucket 1 is because some of the keys in tlsserver were 1024-bit RSA, whereas they're now all 2048-bits (well, except for some specific cases). I don't have a good explanation for the changes in bucket 3.

Comment 5

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

https://reviewboard.mozilla.org/r/18733/#review16971

> Optional: We might want to move the notBefore to just before the epoch now that we have the ability to do so (maybe in a follow up), to make the cert lifetime slightly more realistic.

Any validity period that starts before the epoch but is still valid for the forseeable future will be unrealistic for certificates, so I think this is fine as-is.

> Looks like this should be |Mismatch-Untrusted-Expired Test End-entity| instead.

Good catch.

Comment 6

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

Thanks for the reviews.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9be61ca874d5

Comment 7

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

Comment on attachment 8658961 [details]
MozReview Request: bug 1203312 - convert tlsserver to generate certificates at build time r=Cykesiopka,mgoodwin

bug 1203312 - convert tlsserver to generate certificates at build time r=Cykesiopka,mgoodwin

Comment 8

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

Mark, if you could have a quick look at the interdiff (see also bug 1205406), that would be great - thanks.

Comment 9

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8658961 [details]
MozReview Request: bug 1203312 - convert tlsserver to generate certificates at build time r=Cykesiopka,mgoodwin

https://reviewboard.mozilla.org/r/18733/#review17615

LGTM.

I think your suggestion in Bug 1205406 comment 1 is a good approach - provided we revisit, of course.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a08287c70962

Comment 11

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1d45333a4690 for various b2g xpcshell failures like:

https://treeherder.mozilla.org/logviewer.html#?job_id=14347812&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=14347794&repo=mozilla-inbound

Comment 12

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

I think I've figured out what was going on here. The B2G emulators are so slow that loading the ~50 certificates in the tlsserver directory was taking a long time (>5 minutes), leading to timeouts. My plan is to split the directory up so that it doesn't take so long. Obviously this doesn't fix the problem of the emulators running like molasses in January, but I can't do much about that.

Comment 13

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

Comment on attachment 8658961 [details]
MozReview Request: bug 1203312 - convert tlsserver to generate certificates at build time r=Cykesiopka,mgoodwin

bug 1203312 - convert tlsserver to generate certificates at build time r=Cykesiopka,mgoodwin

Comment 14

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

Attachment: MozReview Request: bug 1203312 - split tlsserver certificates into ocsp_common and bad_certs

bug 1203312 - split tlsserver certificates into ocsp_common and bad_certs

The B2G emulators apparently take ~5 minutes to read 50 certificates into
memory, which causes intermittent test timeouts. This is an attempt to
reduce the number of certificates needed to be read at any given time.

Comment 15

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

This appears to have improved things: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6b68924f19a
The idea now is that all of the certificates needed by OCSPStaplingServer are in ocsp_certs and all of the certificates needed by BadCertServer are in bad_certs. Some certificates that were only used by the test_blocklist.js test and weren't needed by either server were moved into test_onecrl. This whole situation could still be cleaned up a bit, but this is at least a step in the right direction.

Comment 16

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8665525 [details]
MozReview Request: bug 1203312 - split tlsserver certificates into ocsp_common and bad_certs

https://reviewboard.mozilla.org/r/20241/#review18427

Looks good to me!

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c72e571e7270
https://hg.mozilla.org/integration/mozilla-inbound/rev/04db0fb529b9

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c72e571e7270
https://hg.mozilla.org/mozilla-central/rev/04db0fb529b9