generate some PSM xpcshell test certificates at build time

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

(Depends on: 1 bug)

unspecified
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

PSM has a number of xpcshell tests that require specially crafted certificates. Currently we store these as raw DER in the tree. There are a number of downsides to this approach:

* They're not human-readable. This has a few consequences:
  * Dealing with them as a developer is cumbersome
  * Revision control can't meaningfully operate on them
  * Merges are hard (if there's a conflict, you basically have to run the generation script again)
* They have an unreasonable validity period (because we don't want to keep regenerating them, they are valid for 30 or 40 years, which is much longer than anything we should be encountering in the wild)
* They take up more space than necessary in the tree (see also the point about revision control)

What we should do is generate them as part of the build process in a way that's similar to how the mozilla::pkix gtests already operate.

This bug will add the necessary python libraries and initial implementation of the script that generates the files. It will also convert one directory (currently security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints) to use the new system as a proof of concept. If this is successful, we can improve the script and convert other PSM xpcshell test directories.
Created attachment 8608407 [details]
MozReview Request: bz://1166976/keeler

/r/9153 - bug 1166976 - add pyasn1-modules python library
/r/9155 - bug 1166976 - add Python-RSA python library
/r/9157 - bug 1166976 - generate some PSM xpcshell test certificates at build time

Pull down these commits:

hg pull -r 148d5d9cf069072ad38975ed5ac6c7cd3963550b https://reviewboard-hg.mozilla.org/gecko/
Attachment #8608407 - Flags: review?(ted)
Attachment #8608407 - Flags: review?(nfroyd)
Attachment #8608407 - Flags: review?(mgoodwin)
Attachment #8608407 - Flags: review?(gerv)
Attachment #8608407 - Flags: review?(cykesiopka.bmo)
In terms of importing the python libraries, I'm basically going off of bug 1091668, hence the r? for Ted for including the libraries and Gerv for the licenses. I think they're BSD and Apache 2.0, respectively.
Also, I forgot to mention that it seems the reviewboard interface doesn't properly show deleted binary files (e.g. security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints/ca.der was deleted, not modified), so keep that in mind (looks like we have a bug on it: bug 1151121).
Comment on attachment 8608407 [details]
MozReview Request: bz://1166976/keeler

r=gerv.

Gerv
Attachment #8608407 - Flags: review?(gerv) → review+
Comment on attachment 8608407 [details]
MozReview Request: bz://1166976/keeler

r=me on the moz.build parts.  It might be useful to |hg cp| the pycert.py file from the generate.py and then make changes, so blame/log/etc. information is preserved.  Not sure if you did that and it's just not reflected in reviewboard.
Attachment #8608407 - Flags: review?(nfroyd) → review+

Comment 6

4 years ago
https://reviewboard.mozilla.org/r/9157/#review7897

Very nice! r+ with comments addressed.

Note: I only briefly checked the pyasn1 methods etc used look reasonable. However, this patch builds correctly on my machine, and tests pass, so everything should be fine.

::: security/manager/ssl/tests/unit/pycert.py:1
(Diff revision 1)
> +#!/usr/bin/python

Minor nit: Try using |#!/usr/bin/env python|? Apparently it's better than assuming python is at a specific location.

::: security/manager/ssl/tests/unit/pycert.py:44
(Diff revision 1)
> +    # Ensure that the most significant bit isn't set (which would indicate a

Nit: It looks like most of this file uses a 72 char limit, but the comments here use 80 instead.

Probably best to just stick to one as much as reasonable.

::: security/manager/ssl/tests/unit/pycert.py:66
(Diff revision 1)
> +        algorithm = univ.ObjectIdentifier('1.2.840.113549.1.1.11')

Nit: Both "" and '' style strings are used in this file - can a particular style be used throughout for consistency?

::: security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints/ca.der.certspec:3
(Diff revision 1)
> +extension:basicConstraints:cA,

It looks like the "critical" part was lost/removed in the transition, but feel free to ignore if you don't think supporting this is important.

::: security/manager/ssl/tests/unit/pycert.py:22
(Diff revision 1)
> +The only required fields are issuer and subject. In the future it will

Maybe tweak the wording here?

I initially interpreted this to mean that both param lines must be present, but it looks like the requirement is just that a value must be present if either "issuer:" or "subject:" is used.

::: security/manager/ssl/tests/unit/pycert.py:220
(Diff revision 1)
> +        keyUsageExtension = rfc2459.KeyUsage(keyUsage)

I guess it doesn't really matter much, but will pyasn1 do something appropriate if I do something silly like not specify any purposes at all?

::: security/manager/ssl/tests/unit/pycert.py:2
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public

The comment in test_intermediate_basic_usage_constraints.js about how to regenerate the certs should be removed as well.

::: security/manager/ssl/tests/unit/pycert.py:110
(Diff revision 1)
> +    return "'%s'H" % hexString

Is this "H" here on purpose? Sadly I am weak and can't figure this out myself.

Comment 7

4 years ago
Comment on attachment 8608407 [details]
MozReview Request: bz://1166976/keeler

https://reviewboard.mozilla.org/r/9151/#review7899
Attachment #8608407 - Flags: review?(cykesiopka.bmo) → review+
https://reviewboard.mozilla.org/r/9157/#review7911

I like this.  I don't have much to add to Cykesiopka's comments.

::: security/manager/ssl/tests/unit/pycert.py:106
(Diff revision 1)
> +def byteStringToHexifiedBitString(string):

Doesn't binascii.hexlify do this?
Attachment #8608407 - Flags: review?(mgoodwin) → review+
https://reviewboard.mozilla.org/r/9157/#review7965

Thanks for the review!

> Minor nit: Try using |#!/usr/bin/env python|? Apparently it's better than assuming python is at a specific location.

Good call.

> Maybe tweak the wording here?
> 
> I initially interpreted this to mean that both param lines must be present, but it looks like the requirement is just that a value must be present if either "issuer:" or "subject:" is used.

This turned out to be a holdover from a previous implementation. I just removed the first sentence.

> Nit: It looks like most of this file uses a 72 char limit, but the comments here use 80 instead.
> 
> Probably best to just stick to one as much as reasonable.

Good catch. My understanding is the suggested style for python is to keep docstrings and comments to 72 characters, but code can be longer. Since the pyasn1_modules package has such long names, I went with a 100 character limit for code.

> Nit: Both "" and '' style strings are used in this file - can a particular style be used throughout for consistency?

I decided to go with ' as much as possible.

> Is this "H" here on purpose? Sadly I am weak and can't figure this out myself.

This wouldn't have been obvious, but pyasn1.type.univ.BitString expects input of the form "'abcd'H". I added some documentation to this effect.

> I guess it doesn't really matter much, but will pyasn1 do something appropriate if I do something silly like not specify any purposes at all?

It creates an empty bitstring, so at least it doesn't crash/raise an exception.

> It looks like the "critical" part was lost/removed in the transition, but feel free to ignore if you don't think supporting this is important.

Right - I don't think it's necessary to support that at the moment. It shouldn't be too hard to add in later.
https://reviewboard.mozilla.org/r/9157/#review7969

> Doesn't binascii.hexlify do this?

Ah - thanks. That simplifies the implementation.

Comment 12

4 years ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #0)
> If this is successful, we can
> improve the script and convert other PSM xpcshell test directories.

I went ahead and converted the test_cert_trust and test_cert_eku certs (this one by partially repurposing the generate.py file) and it was relatively straightforward, so the design here looks good so far!
Comment on attachment 8608407 [details]
MozReview Request: bz://1166976/keeler

/r/9153 - bug 1166976 - add pyasn1-modules python library
/r/9155 - bug 1166976 - add Python-RSA python library
/r/9157 - bug 1166976 - generate some PSM xpcshell test certificates at build time

Pull down these commits:

hg pull -r d7d86058c9a1bdd0c5d9245285bbb52a2dfa7527 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8608407 - Flags: review?(nfroyd)
Attachment #8608407 - Flags: review?(mgoodwin)
Attachment #8608407 - Flags: review?(gerv)
Attachment #8608407 - Flags: review?(cykesiopka.bmo)
Attachment #8608407 - Flags: review+
Comment on attachment 8608407 [details]
MozReview Request: bz://1166976/keeler

/r/9153 - bug 1166976 - add pyasn1-modules python library
/r/9155 - bug 1166976 - add Python-RSA python library
/r/9157 - bug 1166976 - generate some PSM xpcshell test certificates at build time

Pull down these commits:

hg pull -r d7d86058c9a1bdd0c5d9245285bbb52a2dfa7527 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8608407 - Flags: review?(gerv)
Attachment #8608407 - Flags: review?(nfroyd)
Comment on attachment 8608407 [details]
MozReview Request: bz://1166976/keeler

/r/9153 - bug 1166976 - add pyasn1-modules python library
/r/9155 - bug 1166976 - add Python-RSA python library
/r/9157 - bug 1166976 - generate some PSM xpcshell test certificates at build time

Pull down these commits:

hg pull -r d7d86058c9a1bdd0c5d9245285bbb52a2dfa7527 https://reviewboard-hg.mozilla.org/gecko/
Sorry for the bug churn, all. There were some cross-platform issues that needed to be addressed (specifically, not outputting binary files because python "helpfully" converts newlines on Windows and making the output reproducible on OS X because of unified build issues).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77dcd0df5247
Assignee: nobody → dkeeler
https://reviewboard.mozilla.org/r/9153/#review8207

Please strip the -0.0.5 version number from the directory name.
https://reviewboard.mozilla.org/r/9155/#review8211

Please strip the version number from the directory name.

Also, is there a reason you didn't go with pycrypto? That package is pretty popular.

Comment 20

4 years ago
https://reviewboard.mozilla.org/r/9157/#review8311

I reviewed the interdiff, and it mostly LGTM.
I'm going to assume nothing changed between the .der.certspec and .pem.certspec files: reviewing the original .der.certspec files was rather time consuming.

::: security/manager/ssl/tests/unit/head_psm.js:112
(Diff revision 2)
> +                 .replace(/[\r\n]/g, "");

Might be worth it to factor this out into a helper function - seems like this will be used more as other tests get converted.

At the very least, I have two patches queued that could use this helper instead of replicating the same code.

::: security/manager/ssl/tests/unit/pycert.py:190
(Diff revision 2)
> +    def generateSerialNumber(self):

I guess the change from a random to deterministic serial number is to satisfy OS X universal build requirements as well?

::: security/manager/ssl/tests/unit/pycert.py:384
(Diff revision 2)
> +    buildid = open('%s/buildid' % buildIDPath).read().strip()

I guess this file is small, but does this file object not need to be closed somewhere?

::: security/manager/ssl/tests/unit/head_psm.js:110
(Diff revision 2)
> +    let pem = der.replace(/-----BEGIN CERTIFICATE-----/, "")

Very minor nit: I guess this isn't really PEM anymore, but just a Base64 string? Same for the other places where this pattern occurs.

Comment 22

4 years ago
Comment on attachment 8608407 [details]
MozReview Request: bz://1166976/keeler

Hmm, either the MozReview -> BZ thing that sets r+ when a Ship It is given is wonky, or I'm doing it wrong.
Attachment #8608407 - Flags: review?(cykesiopka.bmo) → review+
https://reviewboard.mozilla.org/r/9157/#review8357

> Might be worth it to factor this out into a helper function - seems like this will be used more as other tests get converted.
> 
> At the very least, I have two patches queued that could use this helper instead of replicating the same code.

Good call - I added a helper.

> I guess the change from a random to deterministic serial number is to satisfy OS X universal build requirements as well?

Yes - I added a docstring about it.

> I guess this file is small, but does this file object not need to be closed somewhere?

Yeah, I guess it's best-practice and all that. I added another 'with' block.
Thanks for the reviews, everyone!
Oh, hmmm - I guess I didn't get re-reviews from everyone I asked. Oh, well - carrying over r+. We can address any issues in a follow-up.
Created attachment 8612520 [details]
MozReview Request: bug 1166976 - add pyasn1-modules python library

bug 1166976 - add pyasn1-modules python library
Created attachment 8612521 [details]
MozReview Request: bug 1166976 - add Python-RSA python library

bug 1166976 - add Python-RSA python library
Created attachment 8612522 [details]
MozReview Request: bug 1166976 - generate some PSM xpcshell test certificates at build time

bug 1166976 - generate some PSM xpcshell test certificates at build time
Attachment #8612522 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8612522 [details]
MozReview Request: bug 1166976 - generate some PSM xpcshell test certificates at build time

bug 1166976 - generate some PSM xpcshell test certificates at build time
Attachment #8612522 - Flags: review?(cykesiopka.bmo)
I'm starting to dislike reviewboard.
Attachment #8608407 - Flags: review?(ted)
Attachment #8608407 - Flags: review?(mgoodwin)
https://hg.mozilla.org/mozilla-central/rev/580d7094b03a
https://hg.mozilla.org/mozilla-central/rev/6b9de5038795
https://hg.mozilla.org/mozilla-central/rev/ec1e8a7a3610
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
I didn't back it out, because it sort of amuses me how we'll just retrigger and clobber and clobber and retrigger bustage, but:

The OS X opt builds are universal builds, so they build once for 32-bit and again for 64-bit and they expect everything other than the binary bits to be identical, so they stick the two together in postflight_all, doing a bunch of copyIfIdentical, which is quite frequently failing and thus causing the entire build to fail, like https://treeherder.mozilla.org/logviewer.html#?job_id=10339841&repo=mozilla-inbound

What I find surprising is that it sometimes passes, since the code creating the cert appears to be fiddling with datetimes. Does it only use the hour, and only fail when the i386 build happens during one hour and the x86_64 build happens during a different hour?

Updated

4 years ago
Depends on: 1170431
Comment on attachment 8608407 [details]
MozReview Request: bz://1166976/keeler
Attachment #8608407 - Attachment is obsolete: true
Depends on: 1179660

Updated

3 years ago
Depends on: 1202097
You need to log in before you can comment on or make changes to this bug.