Closed
Bug 1166976
Opened 10 years ago
Closed 10 years ago
generate some PSM xpcshell test certificates at build time
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: keeler, Assigned: keeler)
References
Details
Attachments
(3 files, 1 obsolete file)
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.
Assignee | ||
Comment 1•10 years ago
|
||
/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)
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
Comment on attachment 8608407 [details]
MozReview Request: bz://1166976/keeler
r=gerv.
Gerv
Attachment #8608407 -
Flags: review?(gerv) → review+
Comment 5•10 years ago
|
||
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•10 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•10 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+
Comment 8•10 years ago
|
||
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?
Comment 9•10 years ago
|
||
https://reviewboard.mozilla.org/r/9157/#review7913
/me fails to work out how to r+
Updated•10 years ago
|
Attachment #8608407 -
Flags: review?(mgoodwin) → review+
Assignee | ||
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
https://reviewboard.mozilla.org/r/9157/#review7969
> Doesn't binascii.hexlify do this?
Ah - thanks. That simplifies the implementation.
Comment 12•10 years ago
•
|
||
(In reply to Dana 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!
Assignee | ||
Comment 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8608407 -
Flags: review?(nfroyd)
Assignee | ||
Comment 15•10 years ago
|
||
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/
Assignee | ||
Comment 16•10 years ago
|
||
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
Comment 17•10 years ago
|
||
Comment 18•10 years ago
|
||
https://reviewboard.mozilla.org/r/9153/#review8207
Please strip the -0.0.5 version number from the directory name.
Comment 19•10 years ago
|
||
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•10 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 21•10 years ago
|
||
Comment 22•10 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+
Assignee | ||
Comment 23•10 years ago
|
||
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.
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 years ago
|
||
Thanks for the reviews, everyone!
Assignee | ||
Comment 26•10 years ago
|
||
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.
Assignee | ||
Comment 27•10 years ago
|
||
bug 1166976 - add pyasn1-modules python library
Assignee | ||
Comment 28•10 years ago
|
||
bug 1166976 - add Python-RSA python library
Assignee | ||
Comment 29•10 years ago
|
||
bug 1166976 - generate some PSM xpcshell test certificates at build time
Attachment #8612522 -
Flags: review?(cykesiopka.bmo)
Assignee | ||
Comment 30•10 years ago
|
||
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)
Assignee | ||
Comment 31•10 years ago
|
||
I'm starting to dislike reviewboard.
Assignee | ||
Updated•10 years ago
|
Attachment #8608407 -
Flags: review?(ted)
Attachment #8608407 -
Flags: review?(mgoodwin)
Comment 32•10 years ago
|
||
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
Closed: 10 years ago
status-firefox41:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 33•10 years ago
|
||
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?
Assignee | ||
Comment 34•9 years ago
|
||
Attachment #8608407 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•