Closed
Bug 1441338
Opened 6 years ago
Closed 6 years ago
Update genpgocert.py to be able to recreate the pgo/certs databases
Categories
(Core :: Security: PSM, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: jcj, Assigned: jcj)
References
Details
(Whiteboard: [psm-assigned])
Attachments
(3 files, 1 obsolete file)
45 bytes,
text/x-phabricator-request
|
keeler
:
review+
franziskus
:
review+
jcristau
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52+
|
Details | Review |
89.75 KB,
patch
|
Details | Diff | Splinter Review | |
73.07 KB,
patch
|
Details | Diff | Splinter Review |
build/pgo/genpgocert.py is intended to create the databases in build/pgo/certs, but those databases have manually-included certificates within them. As of this bug's filing, these certs are required but not created using the genpgocert.py script: selfsigned Pu,u,u Unknown CA Cu,u,u escapeattack1 Pu,u,u untrustedandexpired Pu,u,u alternateTrustedAuthority Cu,u,u dynamicPinningGood Pu,u,u staticPinningBad Pu,u,u sha1_end_entity Pu,u,u bug413909cert u,u,u untrusted Pu,u,u escapeattack2 Pu,u,u expired Pu,u,u dynamicPinningBad Pu,u,u sha256_end_entity Pu,u,u These should be re-creatable using the scripts. Perhaps they should be updated to use certspecs?
Priority: -- → P3
Whiteboard: [psm-backlog]
Assignee | ||
Comment 1•6 years ago
|
||
It looks like we need to move fast on this one per Bug 1435946. The two steps I see are: 1) Convert all of the certs in the current DBs to use Certspec/Keyspec files and update build/pgo/genpgocert.py to regenerate those DBs from those files 2) Remove the DBs entirely, instead using dynamic loading in all affected tests. I think #2 should be done in a different follow-on bug, and we'll tackle #1 in this bug to unblock Bug 1435946. Marking P1 and taking this (initially) myself.
Assignee: nobody → jjones
Blocks: 1435946
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [psm-backlog] → [psm-assigned]
Assignee | ||
Comment 2•6 years ago
|
||
I'm making some progress here; I've got certspec files for all of the affected certs, but I haven't gotten everything scripted and tested yet.
Updated•6 years ago
|
status-firefox59:
--- → wontfix
status-firefox61:
--- → affected
status-firefox-esr52:
--- → affected
Comment 3•6 years ago
|
||
<<Full change message TBD.>> Doesn't yet pass HPKP tests, but I think I've resolved all the others...
Updated•6 years ago
|
Attachment #8968959 -
Attachment description: Bug #1441338 - Change pgo certificates to use certspec/keyspec files WIP f?keeler → Bug #1441338 - Change pgo certificates to use certspec/keyspec files r?keeler r?franziskus
Assignee | ||
Comment 4•6 years ago
|
||
all/all try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0815f82119e9f9bab4e622cd033b28ca7d875ce&selectedJob=174814173 I'm a bit concerned about the TV failures.
Comment 5•6 years ago
|
||
TV only runs on tests when they are added or changed, and has only run on those tests which have been added or changed since TV has existed, which is just a few months. Sure, it's possible that browser_ssl_error_reports.js is right on the edge of timing out on webrender for some reason, and your added logging pushes it over, but the first thing I would do is push a patch to try which does absolutely nothing except remove that stray extra blank line, which will still trigger TV on that test, and see whether you are just triggering an existing trap which has been waiting for someone to touch the test in any way.
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Phil Ringnalda (:philor) from comment #5) > TV only runs on tests when they are added or changed, and has only run on > those tests which have been added or changed since TV has existed, which is > just a few months. > > Sure, it's possible that browser_ssl_error_reports.js is right on the edge > of timing out on webrender for some reason, and your added logging pushes it > over, but the first thing I would do is push a patch to try which does > absolutely nothing except remove that stray extra blank line, which will > still trigger TV on that test, and see whether you are just triggering an > existing trap which has been waiting for someone to touch the test in any > way. OK, giving that a shot: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f152329bae5b8000edae41905e5b65632cfb9de Thanks, Phil.
Comment 7•6 years ago
|
||
Comment on attachment 8968959 [details] Bug #1441338 - Change pgo certificates to use certspec/keyspec files r?keeler r?franziskus David Keeler [:keeler] (use needinfo) has approved the revision. Franziskus Kiefer [:fkiefer or :franziskus] has approved the revision. https://phabricator.services.mozilla.com/D971
Attachment #8968959 -
Flags: review+
Pushed by jjones@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec5e58165939 Change pgo certificates to use certspec/keyspec files r=keeler r=franziskus
Comment 9•6 years ago
|
||
Comment on attachment 8968959 [details] Bug #1441338 - Change pgo certificates to use certspec/keyspec files r?keeler r?franziskus We need this fix in 52 and 60. approved for 60.0b15.
Attachment #8968959 -
Flags: approval-mozilla-esr52+
Attachment #8968959 -
Flags: approval-mozilla-beta+
Comment 10•6 years ago
|
||
The patch fails to apply on beta. Please provide a person which applies cleanly.
Flags: needinfo?(jjones)
Assignee | ||
Comment 11•6 years ago
|
||
This is super weird, but somehow an npm file got updated between Phabricator and pushing to inbound [1], and that's what's an issue on beta. I'm going to revert it in a subsequent push to inbound, and produce the beta patch. I'm not even sure how that happened; I had two people looking over my shoulder most of the push. [1] https://hg.mozilla.org/integration/mozilla-inbound/diff/ec5e58165939/npm-shrinkwrap.json
Assignee | ||
Comment 12•6 years ago
|
||
Here's the rebased patch against beta. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2143545d0ed78cbe4c99ebeaaf6c58c69384c0fa (Note: The try is just getting started, but it's pretty late here, so I wanted to get this up, assuming the try run goes well.)
Flags: needinfo?(jjones)
Assignee | ||
Comment 13•6 years ago
|
||
Fixing incorrect filename. (Patch is fine, but I named the file wrong)
Attachment #8970336 -
Attachment is obsolete: true
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ec5e58165939
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 15•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/8471e4e4f0df
Comment 16•6 years ago
|
||
Thanks for the fixup here! This was all in a somewhat OK state when it started, but over the years it clearly grew to become unmaintainable.
Comment 17•6 years ago
|
||
JC, this has ESR52 backport conflicts due to bug 1439378 (mostly). I'm not sure how to rebase through some of it, so can you please take a look? Thanks!
Flags: needinfo?(jjones)
Assignee | ||
Comment 18•6 years ago
|
||
I'm pretty sure that we can just clobber those changes, actually, but let me do it locally and kick off a try.
Flags: needinfo?(jjones)
Assignee | ||
Comment 19•6 years ago
|
||
We cannot just clobber them... sigh, all the test fixes need to be re-implemented for ESR52. I might be able to finish this tomorrow, but this is too much for today.
Comment 20•6 years ago
|
||
Can you just regenerate the CA and standard testing certs and disable whatever tests use the custom certs on ESR52? It's going out of support in a short while, doesn't seem worth doing a bunch of extra work to make a few tests keep working.
Comment 21•6 years ago
|
||
I'm still uplifting patches for ESR52, if you would like to get these changes in. Sounds like we need them for 52.8.0esr release next week. I'm hoping to go to build tomorrow or Wednesday.
tracking-firefox-esr52:
--- → 60+
Assignee | ||
Comment 22•6 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21) > I'm still uplifting patches for ESR52, if you would like to get these > changes in. Sounds like we need them for 52.8.0esr release next week. I'm > hoping to go to build tomorrow or Wednesday. I'm still trying to make it work, even in a simple form re: Ted's comment. (Complicated by international travel over the weekend). (pgoserver/ssltunnel is refusing to recognize the new private keys for some reason)
Assignee | ||
Comment 23•6 years ago
|
||
Update: I've figured out how to make ESR52 happy with this patch. It's going to be a bunch of manual tweaks to the key3/cert8.db files, as opposed to the scripted work done for 60/61. That makes it practically impossible to review, sadly. I guess we'll just have to make sure that the patch doesn't touch any new files that it shouldn't, since it's pretty much a big binary diff. I've a try push going out right now to help me narrow down any other certs that need manual attention, and then I'll post the patch.
Assignee | ||
Comment 24•6 years ago
|
||
This is against ESR52, which is decidedly less well behaved than trunk for these changes. Particularly, the client cert tests pass locally on two different boxes, but fail on Linux64 on Try. For whatever reason, Try isn't even trying these on other platforms with a "mach try -b o -p all -u all -t none", so I'm a bit at a loss. The commit message has the nitty gritty of what's changed versus 60/61, but to copy: * Changed build/pgo/genpgocert.py to use '-d dbm:'-style database arguments to ensure we don't use SQLite. * Back-port two test changes, one for browser_ssl_error_reports.js, the other for browser_clientAuth_ui.js, so as to use the new cert names. * Disable browser_clientAuth_ui.js and browser_clientAuth_connection.js, as they are misbehaving on try with these changes. (I could revert the edits to browser_clientAuth_ui.js given that, but it seems fair to just disable.) * Reverted build/pgo/certs/jartests-object.ca because modules/libjar/test/chrome/test_bug386153.html still exists. * Manually constructed PKCS12 objects for most of the server certspecs; this is necessary for ssltunnel to find the private keys for unknown reasons. I'm at a loss as to how to make this any better, given the purples showing up in Try, and the lack of multi-platform tests. Suggestions are welcome.... but in truth, this is pretty much a binary patch at this point, as all the changes that matter are really in cert8.db/key3.db , which makes it practically impossible to even review. Last try run before disabling client cert tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eded7a1d26d3a5093869076f058bfe8900d3f0a Try run disabling client cert tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4e8e52ff90e3d7adcf89a0522ee8b7f6f570f2d
Comment 25•6 years ago
|
||
You need to add --buildbot to your Try syntax to get the tests on other platforms (the checkbox is on the right side of TryChooser).
Assignee | ||
Comment 26•6 years ago
|
||
Ah. Took me a while to figure out how to do that (mach in esr52 doesn't seem to know it), but here's a --buildbot try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=00678b8326eeff6bff20dc9e5e36e38e16c22900
Comment 27•6 years ago
|
||
It looks like there are still windows build issues from the try push. Can you take another look? Thanks!
Flags: needinfo?(jjones)
Assignee | ||
Comment 28•6 years ago
|
||
From conversations in #releng on Friday, I believe that's Bug 1459249. I don't think I can fix that. (Not sure if I should mark this whole bug as blocked on Bug 1459249 just for the ESR52 uplift though.)
Flags: needinfo?(jjones)
Comment 29•6 years ago
|
||
OK, Aryx and Ryan, what do you think, can we land this patch on esr52 ?
Flags: needinfo?(ryanvm)
Flags: needinfo?(aryx.bugmail)
Comment 30•6 years ago
|
||
uplift |
Needed a follow-up fix for ESLint bustage, but appears to have landed and stuck now! https://hg.mozilla.org/releases/mozilla-esr52/rev/303139c887e7 https://hg.mozilla.org/releases/mozilla-esr52/rev/f2f38b868a00
You need to log in
before you can comment on or make changes to this bug.
Description
•