Closed Bug 1441338 Opened 2 years ago Closed 2 years ago

Update genpgocert.py to be able to recreate the pgo/certs databases

Categories

(Core :: Security: PSM, defect, P1)

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 60+ fixed
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(3 files, 1 obsolete file)

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]
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]
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.
<<Full change message TBD.>>

Doesn't yet pass HPKP tests, but I think I've resolved all the others...
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
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.
(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 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 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+
Blocks: 1456089
The patch fails to apply on beta. Please provide a person which applies cleanly.
Flags: needinfo?(jjones)
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
Attached patch bug1432181-beta.patch (obsolete) — Splinter Review
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)
Fixing incorrect filename. (Patch is fine, but I named the file wrong)
Attachment #8970336 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ec5e58165939
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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.
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)
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)
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.
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.
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.
(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)
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.
Attached patch Patch for ESR52Splinter Review
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
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).
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
It looks like there are still windows build issues from the try push. Can you take another look? Thanks!
Flags: needinfo?(jjones)
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)
OK, Aryx and Ryan, what do you think, can we land this patch on esr52 ?
Flags: needinfo?(ryanvm)
Flags: needinfo?(aryx.bugmail)
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
Flags: needinfo?(ryanvm)
Flags: needinfo?(aryx.bugmail)
You need to log in before you can comment on or make changes to this bug.