convert the tlsserver certificates to be generated at build time

RESOLVED FIXED in Firefox 44

Status

()

Core
Security: PSM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

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.
Created 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
Attachment #8658961 - Flags: review?(mgoodwin)
Attachment #8658961 - Flags: review?(cykesiopka.bmo)
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).
Attachment #8658961 - Flags: review?(mgoodwin)

Comment 3

3 years ago
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.
Attachment #8658961 - Flags: review?(cykesiopka.bmo) → review+
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.
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.
(Assignee)

Updated

3 years ago
See Also: → bug 1205406
(Assignee)

Updated

3 years ago
Attachment #8658961 - Attachment description: MozReview Request: bug 1203312 - convert tlsserver to generate certificates at build time → MozReview Request: bug 1203312 - convert tlsserver to generate certificates at build time r=Cykesiopka,mgoodwin
Attachment #8658961 - Flags: review?(mgoodwin)
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
Mark, if you could have a quick look at the interdiff (see also bug 1205406), that would be great - thanks.
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.
Attachment #8658961 - Flags: review?(mgoodwin) → review+

Updated

3 years ago
Blocks: 1205962
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.
Flags: needinfo?(dkeeler)
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
Created attachment 8665525 [details]
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.
Attachment #8665525 - Flags: review?(mgoodwin)
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.
Attachment #8665525 - Flags: review?(mgoodwin) → review+
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!
https://hg.mozilla.org/mozilla-central/rev/c72e571e7270
https://hg.mozilla.org/mozilla-central/rev/04db0fb529b9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.